Skip to content

feat(settings): simplify hook, split meta and LS cases #3111

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
artemmufazalov merged 9 commits into from
Dec 10, 2025
Merged

feat(settings): simplify hook, split meta and LS cases #3111

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e9bd6a1
feat(settings): simplify hook, split meta and LS cases
artemmufazalov Dec 5, 2025
c25b134
update comment
artemmufazalov Dec 5, 2025
a4ab272
fix: bots review
artemmufazalov Dec 5, 2025
c9989ef
fix: do not set setting with invalid params
artemmufazalov Dec 10, 2025
d7dd85e
fix: return default from hook instead of api
artemmufazalov Dec 10, 2025
71b2224
fix: bot review - api params always defined
artemmufazalov Dec 10, 2025
dfa745a
fix: make condition straightforward
artemmufazalov Dec 10, 2025
7ff81e2
fix: move serializeReduxError to common utils
artemmufazalov Dec 10, 2025
5d48c26
fix: missed optional chaining
artemmufazalov Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/containers/Tenant/Diagnostics/TopQueries/QueriesTableWithDrawer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ const b = cn('kv-top-queries');
interface SimpleTableWithDrawerProps {
columns: Column<KeyValueRow>[];
data: KeyValueRow[];
loading?: boolean;
isFetching?: boolean;
isLoading?: boolean;
onRowClick?: (
row: KeyValueRow | null,
index?: number,
Expand All @@ -39,7 +40,8 @@ interface SimpleTableWithDrawerProps {
export function QueriesTableWithDrawer({
columns,
data,
loading,
isFetching,
isLoading,
onRowClick,
columnsWidthLSKey,
emptyDataMessage,
Expand Down Expand Up @@ -104,7 +106,8 @@ export function QueriesTableWithDrawer({
columnsWidthLSKey={columnsWidthLSKey}
columns={columns}
data={data}
isFetching={loading}
isFetching={isFetching}
isLoading={isLoading}
Comment on lines +109 to +110
Copy link
Member Author

@artemmufazalov artemmufazalov Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Table loading changes are needed to synchronize settings and data loading states, without it tests somehow fail to find table container

settings={tableSettings}
onRowClick={handleRowClick}
rowClassName={(row) => b('row', {active: isEqual(row, selectedRow)})}
Expand Down
5 changes: 3 additions & 2 deletions src/containers/Tenant/Diagnostics/TopQueries/RunningQueriesData.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,12 @@ export const RunningQueriesData = ({
</TableWithControlsLayout.Controls>

{error ? <ResponseError error={parseQueryErrorToString(error)} /> : null}
<TableWithControlsLayout.Table loading={isLoading}>
<TableWithControlsLayout.Table>
<QueriesTableWithDrawer
columns={columnsToShow}
data={rows || []}
loading={isFetching && currentData === undefined}
isFetching={isFetching && currentData === undefined}
isLoading={isLoading}
columnsWidthLSKey={RUNNING_QUERIES_COLUMNS_WIDTH_LS_KEY}
emptyDataMessage={i18n('no-data')}
sortOrder={tableSort}
Expand Down
5 changes: 3 additions & 2 deletions src/containers/Tenant/Diagnostics/TopQueries/TopQueriesData.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,11 +162,12 @@ export const TopQueriesData = ({
</TableWithControlsLayout.Controls>

{error ? <ResponseError error={parseQueryErrorToString(error)} /> : null}
<TableWithControlsLayout.Table loading={isLoading}>
<TableWithControlsLayout.Table>
<QueriesTableWithDrawer
columns={columnsToShow}
data={rows || []}
loading={isFetching && currentData === undefined}
isFetching={isFetching && currentData === undefined}
isLoading={isLoading}
columnsWidthLSKey={TOP_QUERIES_COLUMNS_WIDTH_LS_KEY}
emptyDataMessage={i18n('no-data')}
sortOrder={tableSort}
Expand Down
5 changes: 3 additions & 2 deletions src/services/api/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import type {AxiosWrapperOptions} from '@gravity-ui/axios-wrapper';
import type {AxiosRequestConfig} from 'axios';

import {codeAssistBackend} from '../../store';
import {uiFactory} from '../../uiFactory/uiFactory';

import {AuthAPI} from './auth';
import {CodeAssistAPI} from './codeAssist';
Expand All @@ -27,6 +26,7 @@ interface YdbEmbeddedAPIProps {
proxyMeta: undefined | boolean;
// this setting allows to use schema object path relative to database in api requests
useRelativePath: undefined | boolean;
useMetaSettings: undefined | boolean;
Copy link
Member Author

@artemmufazalov artemmufazalov Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved useMetaSettings from uiFactory to api setup. The reason is that uiFactory property requires api and uiFactory to be inited in the specific order: first we need to configureUIFactory and only after setup api. Otherwise we need two settings - for uiFactory and to api.

So this help to reduce it to only one setting without any additional requirements

csrfTokenGetter: undefined | (() => string | undefined);
defaults: undefined | AxiosRequestConfig;
}
Expand Down Expand Up @@ -54,6 +54,7 @@ export class YdbEmbeddedAPI {
csrfTokenGetter = () => undefined,
defaults = {},
useRelativePath = false,
useMetaSettings = false,
}: YdbEmbeddedAPIProps) {
const axiosParams: AxiosWrapperOptions = {config: {withCredentials, ...defaults}};
const baseApiParams = {singleClusterMode, proxyMeta, useRelativePath};
Expand All @@ -62,7 +63,7 @@ export class YdbEmbeddedAPI {
if (webVersion) {
this.meta = new MetaAPI(axiosParams, baseApiParams);
}
if (uiFactory.useMetaSettings) {
if (useMetaSettings) {
Copy link
Collaborator

@astandrik astandrik Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previosly we had this in uiFactory - so this value could be overriden
Is it an intentional removal of this possibility? (probably a breaking change - major version)

Copy link
Member Author

@artemmufazalov artemmufazalov Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it can be configured in configureStore.

In internal version we configure two entities - store and uiFactory. With setting from uiFactory there was an issue when uiFactory was configured after store

Code with bug:

const {store, history} = configureStore({
    singleClusterMode: SINGLE_CLUSTER_MODE,
    api: new YdbInternalAPI({...params}),
});

configureUIFactory<IssueType, MetricsIds>({
    ...params
});

Correct code:

configureUIFactory<IssueType, MetricsIds>({
    ...params
});

const {store, history} = configureStore({
    singleClusterMode: SINGLE_CLUSTER_MODE,
    api: new YdbInternalAPI({...params}),
});

It's not obvious at all and should not be that way. So I moved this setting to api.

I don't consider this major, because this feature is not finished yet and does not work anywhere

this.metaSettings = new MetaSettingsAPI(axiosParams, baseApiParams);
}

Expand Down
2 changes: 1 addition & 1 deletion src/services/api/metaSettings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export class MetaSettingsAPI extends BaseMetaAPI {
preventBatching,
}: GetSingleSettingParams & {preventBatching?: boolean}) {
if (preventBatching) {
return this.get<Setting>(this.getPath('/meta/user_settings'), {name, user});
return this.get<Setting | undefined>(this.getPath('/meta/user_settings'), {name, user});
}

return new Promise<Setting>((resolve, reject) => {
Expand Down
1 change: 1 addition & 0 deletions src/store/configureStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ export function configureStore({
proxyMeta: false,
csrfTokenGetter: undefined,
useRelativePath: false,
useMetaSettings: false,
defaults: undefined,
}),
} = {}) {
Expand Down
5 changes: 3 additions & 2 deletions src/store/reducers/authentication/authentication.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const initialState: AuthenticationState = {
isAuthenticated: true,
user: undefined,
id: undefined,
metaUser: undefined,
Copy link
Member Author

@artemmufazalov artemmufazalov Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not figured out how to properly receive metaUser value yet, so I decided to test everything with some hardcoded user and to do all metaUser code separately - there are additional user points in issue checklist.

What are the problems with metaUser:

  1. The logic may be different in different installations
  2. It should not break current authentication cases
  3. We need somehow to tackle the case, when user do not have access to meta cluster

};

export const slice = createSlice({
Expand Down Expand Up @@ -45,13 +46,13 @@ export const slice = createSlice({
selectIsUserAllowedToMakeChanges: (state) => state.isUserAllowedToMakeChanges,
selectIsViewerUser: (state) => state.isViewerUser,
selectUser: (state) => state.user,
selectID: (state) => state.id,
selectMetaUser: (state) => state.metaUser ?? state.id,
},
});

export default slice.reducer;
export const {setIsAuthenticated, setUser} = slice.actions;
export const {selectIsUserAllowedToMakeChanges, selectIsViewerUser, selectUser, selectID} =
export const {selectIsUserAllowedToMakeChanges, selectIsViewerUser, selectUser, selectMetaUser} =
slice.selectors;

export const authenticationApi = api.injectEndpoints({
Expand Down
2 changes: 2 additions & 0 deletions src/store/reducers/authentication/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,6 @@ export interface AuthenticationState {

user: string | undefined;
id: string | undefined;

Copy link
Contributor

@greptile-apps greptile-apps bot Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: metaUser field is never set anywhere in the codebase - no reducer action populates this field. Since selectMetaUser returns metaUser ?? id, this will always fall back to id.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/store/reducers/authentication/types.ts
Line: 8:8

Comment:
**logic:** `metaUser` field is never set anywhere in the codebase - no reducer action populates this field. Since `selectMetaUser` returns `metaUser ?? id`, this will always fall back to `id`.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Member Author

@artemmufazalov artemmufazalov Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's intentional, user will be added with another PR

#3111 (comment)

metaUser: string | undefined;
Copy link
Contributor

@greptile-apps greptile-apps bot Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: metaUser field is never populated anywhere in the codebase - no reducer action sets this value. Since selectMetaUser returns metaUser ?? id, it will always fall back to id. If this field is intended for future use, consider adding a comment explaining its purpose.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/store/reducers/authentication/types.ts
Line: 9:9

Comment:
**style:** `metaUser` field is never populated anywhere in the codebase - no reducer action sets this value. Since `selectMetaUser` returns `metaUser ?? id`, it will always fall back to `id`. If this field is intended for future use, consider adding a comment explaining its purpose.

How can I resolve this? If you propose a fix, please make it concise.

}
6 changes: 2 additions & 4 deletions src/store/reducers/capabilities/capabilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {createSelector} from '@reduxjs/toolkit';

import type {Capability, MetaCapability, SecuritySetting} from '../../../types/api/capabilities';
import type {AppDispatch, RootState} from '../../defaultStore';
import {serializeError} from '../../utils';

import {api} from './../api';

Expand Down Expand Up @@ -30,10 +31,7 @@ export const capabilitiesApi = api.injectEndpoints({
} catch (error) {
// If capabilities endpoint is not available, there will be an error
// That means no new features are available
// Serialize the error to make it Redux-compatible
const serializedError =
error instanceof Error ? {message: error.message, name: error.name} : error;
return {error: serializedError};
return {error: serializeError(error)};
}
},
}),
Expand Down
81 changes: 30 additions & 51 deletions src/store/reducers/settings/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,68 +1,69 @@
import {isNil} from 'lodash';

import type {
GetSettingsParams,
GetSingleSettingParams,
SetSingleSettingParams,
Setting,
} from '../../../types/api/settings';
import type {AppDispatch} from '../../defaultStore';
import {serializeError} from '../../utils';
import {api} from '../api';

import {SETTINGS_OPTIONS} from './constants';
import {parseSettingValue, stringifySettingValue} from './utils';

export const settingsApi = api.injectEndpoints({
endpoints: (builder) => ({
getSingleSetting: builder.query({
queryFn: async ({name, user}: GetSingleSettingParams, baseApi) => {
getSingleSetting: builder.query<unknown, GetSingleSettingParams>({
queryFn: async ({name, user}) => {
try {
if (!window.api.metaSettings) {
if (!window.api?.metaSettings) {
throw new Error('MetaSettings API is not available');
}

const data = await window.api.metaSettings.getSingleSetting({
name,
user,
// Directly access options here to avoid them in cache key
preventBatching: SETTINGS_OPTIONS[name]?.preventBatching,
});

const dispatch = baseApi.dispatch as AppDispatch;

// Try to sync local value if there is no backend value
syncLocalValueToMetaIfNoData(data, dispatch);

return {data};
return {data: parseSettingValue(data?.value)};
} catch (error) {
return {error};
return {error: serializeError(error)};
}
},
}),
setSingleSetting: builder.mutation({
queryFn: async (params: SetSingleSettingParams) => {
queryFn: async ({
name,
user,
value,
}: Omit<SetSingleSettingParams, 'value'> & {value: unknown}) => {
try {
if (!window.api.metaSettings) {
if (!window.api?.metaSettings) {
throw new Error('MetaSettings API is not available');
}

const data = await window.api.metaSettings.setSingleSetting(params);
const data = await window.api.metaSettings.setSingleSetting({
name,
user,
value: stringifySettingValue(value),
});

if (data.status !== 'SUCCESS') {
throw new Error('Setting status is not SUCCESS');
throw new Error('Cannot set setting - status is not SUCCESS');
}

return {data};
} catch (error) {
return {error};
return {error: serializeError(error)};
}
},
async onQueryStarted(args, {dispatch, queryFulfilled}) {
const {name, user, value} = args;

// Optimistically update existing cache entry
const patchResult = dispatch(
settingsApi.util.updateQueryData('getSingleSetting', {name, user}, (draft) => {
return {...draft, name, user, value};
}),
settingsApi.util.updateQueryData('getSingleSetting', {name, user}, () => value),
);
try {
await queryFulfilled;
Expand All @@ -74,39 +75,31 @@ export const settingsApi = api.injectEndpoints({
getSettings: builder.query({
queryFn: async ({name, user}: GetSettingsParams, baseApi) => {
try {
if (!window.api.metaSettings) {
if (!window.api?.metaSettings) {
throw new Error('MetaSettings API is not available');
}
const data = await window.api.metaSettings.getSettings({name, user});

const patches: Promise<void>[] = [];
const patches: Promise<unknown>[] = [];
const dispatch = baseApi.dispatch as AppDispatch;

// Upsert received data in getSingleSetting cache
// Upsert received data in getSingleSetting cache to prevent further redundant requests
name.forEach((settingName) => {
const settingData = data[settingName] ?? {};
const settingData = data[settingName];

const cacheEntryParams: GetSingleSettingParams = {
name: settingName,
user,
};
const newValue = {name: settingName, user, value: settingData?.value};
const newSettingValue = parseSettingValue(settingData?.value);

const patch = dispatch(
settingsApi.util.upsertQueryData(
'getSingleSetting',
cacheEntryParams,
newValue,
newSettingValue,
),
).then(() => {
// Try to sync local value if there is no backend value
// Do it after upsert if finished to ensure proper values update order
// 1. New entry added to cache with nil value
// 2. Positive entry update - local storage value replace nil in cache
// 3.1. Set is successful, local value in cache
// 3.2. Set is not successful, cache value reverted to previous nil
syncLocalValueToMetaIfNoData(settingData, dispatch);
});
);

patches.push(patch);
});
Expand All @@ -116,24 +109,10 @@ export const settingsApi = api.injectEndpoints({

return {data};
} catch (error) {
return {error};
return {error: serializeError(error)};
}
},
}),
}),
overrideExisting: 'throw',
});

function syncLocalValueToMetaIfNoData(params: Setting, dispatch: AppDispatch) {
const localValue = localStorage.getItem(params.name);

if (isNil(params.value) && !isNil(localValue)) {
dispatch(
settingsApi.endpoints.setSingleSetting.initiate({
name: params.name,
user: params.user,
value: localValue,
}),
);
}
}
8 changes: 1 addition & 7 deletions src/store/reducers/settings/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,8 @@ export const DEFAULT_USER_SETTINGS = {
[SETTING_KEYS.STORAGE_TYPE]: STORAGE_TYPES.groups,
} as const satisfies Record<SettingKey, unknown>;

export const SETTINGS_OPTIONS: Record<string, SettingOptions> = {
export const SETTINGS_OPTIONS: Record<string, SettingOptions | undefined> = {
[SETTING_KEYS.THEME]: {
preventBatching: true,
},
[SETTING_KEYS.SAVED_QUERIES]: {
preventSyncWithLS: true,
},
[SETTING_KEYS.QUERIES_HISTORY]: {
preventSyncWithLS: true,
},
} as const;
Loading
Loading