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

artemmufazalov · 2025-11-19T17:35:27Z

Part of #2892

Currently it can be checked with table columns width.
Stand (without meta): https://nda.ya.ru/t/BEIjuuLI7N8Qd7

Sync all values within api onQueryStarted instead of useEffects.

This approach proved itself good with and without meta api. Without meta api there is only 1 failed test (against 100-200 before): #3054

greptile-review

CI Results

Test Status: ⚠️ FLAKY

📊 Full Report

Total	Passed	Failed	Flaky	Skipped
378	374	0	2	2

Test Changes Summary ⏭️2

⏭️ Skipped Tests (2)

Scroll to row, get shareable link, navigate to URL and verify row is scrolled into view (tenant/diagnostics/tabs/queries.test.ts)
Copy result button copies to clipboard (tenant/queryEditor/queryEditor.test.ts)

Bundle Size: ✅

Current: 62.48 MB | Main: 62.49 MB
Diff: 1.98 KB (-0.00%)

✅ Bundle size unchanged.

ℹ️ CI Information

Test recordings for failed tests are available in the full report.
Bundle size is measured for the entire 'dist' directory.
📊 indicates links to detailed reports.
🔺 indicates increase, 🔽 decrease, and ✅ no change in bundle size.

Copilot

Pull Request Overview

This PR refactors the settings synchronization architecture by moving the sync logic from useEffect hooks in useSetting.ts to RTK Query's onQueryStarted lifecycle hooks in the API layer. The main goal is to centralize settings synchronization between localStorage, Redux store, and the meta settings API.

Key Changes:

Consolidates all settings sync logic into the settingsApi endpoints' onQueryStarted hooks
Simplifies useSetting hook by removing multiple useEffect dependencies
Handles localStorage and API settings synchronization at the API layer instead of component layer

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
`src/store/reducers/settings/utils.ts`	Adds helper functions `getSettingDefault` and `shouldSyncSettingToLS` to support centralized settings sync logic
`src/store/reducers/settings/useSetting.ts`	Removes all `useEffect`-based sync logic and simplifies hook to only trigger API queries and mutations
`src/store/reducers/settings/constants.ts`	Updates `SETTINGS_OPTIONS` type to explicitly allow `undefined` values for better type safety
`src/store/reducers/settings/api.ts`	Implements comprehensive sync logic in `onQueryStarted` hooks for both query and mutation endpoints, handling localStorage and store updates
`src/services/api/metaSettings.ts`	Updates return type to allow `undefined` for settings that don't exist in the backend

src/store/reducers/settings/useSetting.ts

src/store/reducers/settings/api.ts

src/store/reducers/settings/utils.ts

src/store/reducers/settings/api.ts

Raubzeug · 2025-11-20T06:39:36Z

@greptile-review

greptile-apps

_{8 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

src/store/reducers/settings/api.ts

astandrik · 2025-11-20T13:23:57Z

src/store/reducers/settings/api.ts


                    return {data};
                } catch (error) {
+                    console.error('Cannot get settings:', error);


Can it be that error is "Cannot get setting" - but the real situation is that one of patches failed ?

astandrik · 2025-11-20T13:27:31Z

Now as I see we have three sources of truth:

LS
Backend
Redux Store

And we struggle to keep all of them in sync
Is my assumption correct?

artemmufazalov · 2025-11-20T13:38:10Z

Now as I see we have three sources of truth:

LS

Backend

Redux Store

And we struggle to keep all of them in sync Is my assumption correct?

Yes, everything is to keep them in sync

astandrik · 2025-11-20T14:06:23Z

Now as I see we have three sources of truth:

LS

Backend

Redux Store

And we struggle to keep all of them in sync Is my assumption correct?

Yes, everything is to keep them in sync

Is it a goal or temporary state?

artemmufazalov · 2025-11-20T14:16:44Z

Is it a goal or temporary state?

It's a goal

astandrik · 2025-11-20T14:28:49Z

src/store/reducers/settings/api.ts

+                        const newValue = data ?? {name, user};
+
+                        // Try to sync local value if there is no backend value
+                        syncLocalValueToMetaIfNoData(newValue, dispatch);


is it possible that null/undefined on backend is intentional?
i mean we removed some setting for example

but LS has some old value (in another browser for example) and pushes this setting to backend?

It's a current limitation, yes. We cannot have null or undefined as backend value, there should be at least something, otherwise defined localStorage value will be loaded to backend. Without this it's unclear, when we should sync local value to backend

astandrik · 2025-11-20T14:32:59Z

src/store/reducers/settings/api.ts

+        return;
+    }
+
    const localValue = localStorage.getItem(params.name);


we use localStorage directly here but there are already functions for this like

readSettingValueFromLS / shouldSyncSettingToLS

these functions as I may suppose were created to make work with LS somehow guarded

astandrik · 2025-11-20T14:37:55Z

Now as I see we have three sources of truth:

LS

Backend

Redux Store

And we struggle to keep all of them in sync Is my assumption correct?

Yes, everything is to keep them in sync

I thought this over again and came to a conclusion that actually there is one source of truth (backend) while LS and rtq storage are used as caches for interface rensponsiveness so actually its not "three sources of truth" but "one with two caches"

Does it make sense?

artemmufazalov · 2025-11-20T14:58:26Z

I thought this over again and came to a conclusion that actually there is one source of truth (backend) while LS and rtq storage are used as caches for interface rensponsiveness so actually its not "three sources of truth" but "one with two caches"

Does it make sense?

In general, you are right. But it depends on app installation. There are 3 cases.

No meta (embedded single cluster version). LS is used as source of truth, meta is not used at all, we read and load value to redux to use it in the app without a need to read it from LS.
There is meta service with settings. In this case value from LS is used as temporary initial value while setting from meta is not loaded. When value is loaded, it is used.

So, preload data from LS to store, load actual value from meta when request is finished.

There is meta service, but there is no meta setting value. It is possible case in migration period - users have all their setting in LS now. It is similar to case 2, but when we receive undefined value from backend, we load value from LS there

In all cases redux is used as sync cache for data.

astandrik · 2025-11-20T15:08:47Z

So there are two main scenarios with mainly opposite mechanics

with meta
without meta

It appears we try to handle both at once with one handler and therefore it looks a bit complex

I'm trying to imagine if we could explicitly split these scenarios for clarity and maintainability

its possible to postpone this and separate to some tech issue but would be great if we could handle it at once

artemmufazalov · 2025-12-05T08:33:41Z

src/containers/Tenant/Diagnostics/TopQueries/QueriesTableWithDrawer.tsx

+                isFetching={isFetching}
+                isLoading={isLoading}


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

artemmufazalov · 2025-12-05T08:36:52Z

src/services/api/index.ts

    proxyMeta: undefined | boolean;
    // this setting allows to use schema object path relative to database in api requests
    useRelativePath: undefined | boolean;
+    useMetaSettings: undefined | boolean;


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

artemmufazalov · 2025-12-05T08:43:29Z

src/store/reducers/authentication/authentication.ts

    isAuthenticated: true,
    user: undefined,
    id: undefined,
+    metaUser: undefined,


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:

The logic may be different in different installations

It should not break current authentication cases

We need somehow to tackle the case, when user do not have access to meta cluster

greptile-apps

_{17 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

src/store/reducers/settings/api.ts

src/store/reducers/settings/useSetting.ts

greptile-apps

_{17 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

src/store/reducers/authentication/types.ts

src/store/reducers/settings/useSetting.ts

greptile-apps

_{17 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

src/store/reducers/settings/useSetting.ts

astandrik · 2025-12-10T10:53:24Z

src/utils/hooks/useSetting.ts

 import {useTypedSelector} from './useTypedSelector';

-export const useSetting = <T>(key: string, defaultValue?: T): [T, (value: T) => void] => {
+export const useSetting = <T>(key?: string, defaultValue?: T): [T, (value: T) => void] => {


@greptile why key is optional here?

Looking at this useSetting hook, the key parameter is optional (marked with ?) which seems unusual and potentially problematic.

If you look at how the hook is used internally:

Line 13: getSettingValue(state, key) - passing potentially undefined key

Line 18: dispatch(setSettingValue(key, value)) - dispatching with potentially undefined key

This means the hook could be called without a key like useSetting(), which would result in trying to get/set a setting with an undefined key. This doesn't make much practical sense since you need a specific key to identify which setting you're working with.

The optional key seems like it could lead to bugs where:

Someone calls useSetting() without a key by mistake

The hook "works" but doesn't actually store/retrieve any meaningful setting

The setting operations silently fail or behave unexpectedly

This should probably be required: key: string instead of key?: string. The only scenario where optional might make sense is if there's some default key behavior in the getSettingValue/setSettingValue functions, but that would be unusual and error-prone.

Is there a specific use case where you need to call useSetting without a key, or should this be made required?

Key is optional in useSetting for one specific case - table columns width. We use useSetting hook in ResizeableDataTable via useTableResize, if there is no key provided, only value from useState will be used, if there is a key - it syncs with settings

astandrik · 2025-12-10T10:54:23Z

src/store/reducers/settings/useSetting.ts

-
-            if (!shouldUseOnlyExternalSettings) {
-                setSettingValueToLS(name, value);
+            if (isNil(window.api?.metaSettings)) {


upper in code we have straighforward condition but here is reverted logics - maybe needs unification

Unlike for settingValue, here we also check if user is present, so condition is different. Altered it a little, now it's not reversed

astandrik · 2025-12-10T10:56:29Z

src/services/api/index.ts

            this.meta = new MetaAPI(axiosParams, baseApiParams);
        }
-        if (uiFactory.useMetaSettings) {
+        if (useMetaSettings) {


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)

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

greptile-apps

_{17 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-12-10T10:57:04Z

src/store/reducers/authentication/types.ts


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


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.

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

#3111 (comment)

greptile-apps · 2025-12-10T10:57:05Z

src/store/reducers/settings/useSetting.ts

+            if (isNil(window.api?.metaSettings)) {
+                saveSettingToLS(value);
+            } else if (user) {
+                setMetaSetting({user, name, value});
            }


logic: when window.api?.metaSettings exists but user is undefined, settings changes are silently dropped. Since metaUser field is never populated (no reducer sets it), and selectMetaUser returns metaUser ?? id, this means settings will fail to save if id is also undefined during early app initialization

Prompt To Fix With AI

This is a comment left during a code review. Path: src/store/reducers/settings/useSetting.ts Line: 56:60 Comment: **logic:** when `window.api?.metaSettings` exists but `user` is `undefined`, settings changes are silently dropped. Since `metaUser` field is never populated (no reducer sets it), and `selectMetaUser` returns `metaUser ?? id`, this means settings will fail to save if `id` is also undefined during early app initialization How can I resolve this? If you propose a fix, please make it concise.

That's right, if there is no id provided, it will not work as expected. However, there should be always id in whoami as fallback

greptile-apps · 2025-12-10T10:57:05Z

src/store/reducers/settings/settings.ts

            dispatch(settingsSlice.actions.setSettingValue({name, value}));
+            setSettingValueToLS(name, value);


logic: every call to setSettingValue now writes to localStorage, even when meta settings API is available. In environments using meta settings, this duplicates storage and may cause sync issues

Prompt To Fix With AI

This is a comment left during a code review. Path: src/store/reducers/settings/settings.ts Line: 60:61 Comment: **logic:** every call to `setSettingValue` now writes to localStorage, even when meta settings API is available. In environments using meta settings, this duplicates storage and may cause sync issues How can I resolve this? If you propose a fix, please make it concise.

It's intentional, setSettingValue is called outside new useSetting hook only for settings that are not migrated

greptile-apps · 2025-12-10T10:57:06Z

src/store/reducers/settings/useSetting.ts

+        if (window.api?.metaSettings) {
+            value = settingFromMeta;
+        } else {
+            value = settingFromLS;
        }


logic: when window.api?.metaSettings exists but query hasn't completed (params are skipToken or loading), settingFromMeta is undefined and meta path is chosen, causing fallback to default instead of using LS value. This breaks the intended graceful degradation during loading states

Prompt To Fix With AI

This is a comment left during a code review. Path: src/store/reducers/settings/useSetting.ts Line: 43:47 Comment: **logic:** when `window.api?.metaSettings` exists but query hasn't completed (params are skipToken or loading), `settingFromMeta` is `undefined` and meta path is chosen, causing fallback to default instead of using LS value. This breaks the intended graceful degradation during loading states How can I resolve this? If you propose a fix, please make it concise.

It's intentional, when there is meta, LS should not be used

astandrik · 2025-12-10T11:23:58Z

src/store/utils.ts

+ *
+ * It prevents redux console error on string error in code - `throw new Error("description")`
+ */
+export function serializeError(error: unknown) {


nitpick: maybe we could enhance prepareErrorMessage in shared utils
or move this function to shared utils (with renaming to serializeReduxError)
since this may be needed elsewhere

prepareErrorMessage always return string error, while serializeReduxError could return an object

Moved function to utils

greptile-apps

_{17 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-12-10T11:59:42Z

src/store/reducers/settings/useSetting.ts

-
-            if (!shouldUseOnlyExternalSettings) {
-                setSettingValueToLS(name, value);
+            if (window.api.metaSettings && user) {


logic: missing optional chaining - will throw if window.api is undefined

Suggested change

if (window.api.metaSettings && user) {

if (window.api?.metaSettings && user) {

Prompt To Fix With AI

This is a comment left during a code review. Path: src/store/reducers/settings/useSetting.ts Line: 56:56 Comment: **logic:** missing optional chaining - will throw if `window.api` is undefined ```suggestion if (window.api?.metaSettings && user) { ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2025-12-10T11:59:43Z

src/store/reducers/settings/useSetting.ts

+        if (window.api?.metaSettings) {
+            value = settingFromMeta;
+        } else {
+            value = settingFromLS;
        }


logic: the check on line 43 evaluates to true even when query hasn't started (params are skipToken) or during loading state. This causes settingFromMeta to be undefined and the hook returns default value instead of using the localStorage fallback. During initial load before meta query completes, users will see default values instead of their saved LS values.

Check the query status to ensure meta is actually available:

Suggested change

if (window.api?.metaSettings) {

value = settingFromMeta;

} else {

value = settingFromLS;

}

if (window.api?.metaSettings && settingFromMeta !== undefined) {

value = settingFromMeta;

} else {

Prompt To Fix With AI

This is a comment left during a code review. Path: src/store/reducers/settings/useSetting.ts Line: 43:47 Comment: **logic:** the check on line 43 evaluates to true even when query hasn't started (params are skipToken) or during loading state. This causes `settingFromMeta` to be `undefined` and the hook returns default value instead of using the localStorage fallback. During initial load before meta query completes, users will see default values instead of their saved LS values. Check the query status to ensure meta is actually available: ```suggestion if (window.api?.metaSettings && settingFromMeta !== undefined) { value = settingFromMeta; } else { ``` How can I resolve this? If you propose a fix, please make it concise.

It should never use local setting if there is meta

greptile-apps

_{17 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

astandrik · 2025-12-10T13:13:37Z

@artemmufazalov I had a couple of questions in comments
upd: actually you answered them - my gh page seems to have used stale data

artemmufazalov changed the title ~~feat(useSetting); sync values with LS and store in api~~ feat(useSetting): sync values with LS and store in api Nov 19, 2025

artemmufazalov requested a review from Copilot November 19, 2025 17:36

artemmufazalov mentioned this pull request Nov 19, 2025

Support YDB settings service in EM #2892

Open

Copilot started reviewing on behalf of artemmufazalov November 19, 2025 17:36 View session

Copilot finished reviewing on behalf of artemmufazalov November 19, 2025 17:39

Copilot AI reviewed Nov 19, 2025

View reviewed changes

artemmufazalov marked this pull request as ready for review November 19, 2025 20:11

artemmufazalov requested review from Raubzeug, adameat and astandrik as code owners November 19, 2025 20:11

greptile-apps bot reviewed Nov 20, 2025

View reviewed changes

src/store/reducers/settings/api.ts Outdated Show resolved Hide resolved

src/store/reducers/settings/api.ts Show resolved Hide resolved

astandrik reviewed Nov 20, 2025

View reviewed changes

artemmufazalov marked this pull request as draft December 5, 2025 08:30

artemmufazalov force-pushed the settings-sync-values-in-api branch from 29a9e3d to 759ba53 Compare December 5, 2025 08:30

artemmufazalov commented Dec 5, 2025

View reviewed changes

artemmufazalov changed the title ~~feat(useSetting): sync values with LS and store in api~~ feat(settings): simplify hook, split meta and LS cases Dec 5, 2025

artemmufazalov marked this pull request as ready for review December 5, 2025 09:05

artemmufazalov requested a review from Copilot December 5, 2025 09:05

greptile-apps bot reviewed Dec 5, 2025

View reviewed changes

src/store/reducers/settings/api.ts Outdated Show resolved Hide resolved

src/store/reducers/settings/useSetting.ts Show resolved Hide resolved

artemmufazalov added 5 commits December 10, 2025 11:36

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

e9bd6a1

update comment

c25b134

fix: bots review

a4ab272

fix: do not set setting with invalid params

c9989ef

fix: return default from hook instead of api

d7dd85e

artemmufazalov force-pushed the settings-sync-values-in-api branch from 2f7f3b5 to d7dd85e Compare December 10, 2025 08:36

greptile-apps bot reviewed Dec 10, 2025

View reviewed changes

src/store/reducers/authentication/types.ts Show resolved Hide resolved

src/store/reducers/settings/useSetting.ts Outdated Show resolved Hide resolved

src/store/reducers/settings/useSetting.ts Outdated Show resolved Hide resolved

fix: bot review - api params always defined

71b2224

greptile-apps bot reviewed Dec 10, 2025

View reviewed changes

src/store/reducers/settings/useSetting.ts Show resolved Hide resolved

artemmufazalov requested a review from astandrik December 10, 2025 09:30

astandrik reviewed Dec 10, 2025

View reviewed changes

greptile-apps bot reviewed Dec 10, 2025

View reviewed changes

This comment was marked as off-topic.

Sign in to view

astandrik reviewed Dec 10, 2025

View reviewed changes

artemmufazalov added 2 commits December 10, 2025 14:43

fix: make condition straightforward

dfa745a

fix: move serializeReduxError to common utils

7ff81e2

greptile-apps bot reviewed Dec 10, 2025

View reviewed changes

fix: missed optional chaining

5d48c26

greptile-apps bot reviewed Dec 10, 2025

View reviewed changes

artemmufazalov requested a review from astandrik December 10, 2025 12:12

astandrik approved these changes Dec 10, 2025

View reviewed changes

artemmufazalov added this pull request to the merge queue Dec 10, 2025

Merged via the queue into main with commit e56ce4d Dec 10, 2025
9 checks passed

artemmufazalov deleted the settings-sync-values-in-api branch December 10, 2025 13:41

ydb-platform-bot mentioned this pull request Dec 10, 2025

chore(main): release 12.28.0 #3204

Merged

		dispatch(settingsSlice.actions.setSettingValue({name, value}));
		setSettingValueToLS(name, value);

	if (window.api.metaSettings && user) {
	if (window.api?.metaSettings && user) {

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

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

Uh oh!

Conversation

artemmufazalov commented Nov 19, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI Results

Test Status: ⚠️ FLAKY

⏭️ Skipped Tests (2)

Bundle Size: ✅

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Raubzeug commented Nov 20, 2025

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astandrik commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artemmufazalov commented Nov 20, 2025

Uh oh!

astandrik commented Nov 20, 2025

Uh oh!

artemmufazalov commented Nov 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astandrik commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artemmufazalov commented Nov 20, 2025

Uh oh!

astandrik commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artemmufazalov commented Nov 19, 2025 •

edited by github-actions bot

Loading

astandrik commented Nov 20, 2025 •

edited

Loading

astandrik commented Nov 20, 2025 •

edited

Loading

astandrik commented Nov 20, 2025 •

edited

Loading

greptile-apps bot left a comment •

edited

Loading