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	375	0	1	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.34 MB | Main: 62.34 MB
Diff: 1.76 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

Additional Comments (1)

src/store/reducers/capabilities/capabilities.ts, line 19 (link)

style: inconsistent error handling - one catch block uses serializeError while the other returns raw error. Should both error handlers use the same serialization approach for consistency?

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

_{17 files reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

src/store/reducers/settings/api.ts

src/store/reducers/settings/useSetting.ts

Copilot

Pull request overview

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

src/store/reducers/settings/api.ts

src/store/reducers/settings/useSetting.ts

src/store/reducers/settings/api.ts

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

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

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

759ba53

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

update comment

a8309ba

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

Copilot started reviewing on behalf of artemmufazalov December 5, 2025 09:05 View session

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

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

Copilot finished reviewing on behalf of artemmufazalov December 5, 2025 09:08

Copilot AI reviewed Dec 5, 2025

View reviewed changes

fix: bots review

2f7f3b5

greptile-apps bot reviewed Dec 5, 2025

View reviewed changes

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

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

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

Are you sure you want to change the base?

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!

astandrik Nov 20, 2025

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!

astandrik Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

artemmufazalov Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

astandrik Nov 20, 2025

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!

artemmufazalov Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

artemmufazalov Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

artemmufazalov Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Additional Comments (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

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