feat: use meta settings service if configured #3054
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
artemmufazalov
force-pushed
the
use-external-settings
branch
2 times, most recently
from
4e570d2 to
2edf288
Compare
artemmufazalov
force-pushed
the
use-external-settings
branch
from
2edf288 to
7603a75
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+74
to
+82
| // Sycn value from backend with LS and store | ||
| React.useEffect(() => { | ||
| if (shouldUseMetaSettings && metaSetting?.value) { | ||
| if (!preventSyncWithLS) { | ||
| setSettingValueToLS(name, metaSetting.value); | ||
| } | ||
| const parsedValue = parseSettingValue<T>(metaSetting.value); | ||
| dispatch(setSettingValue(name, parsedValue)); | ||
| } |
Contributor
There was a problem hiding this comment.
logic: empty dependency array means this effect only runs once on mount, but it references shouldUseMetaSettings, metaSetting, preventSyncWithLS, name, and dispatch which can all change
Suggested change
| // Sycn value from backend with LS and store | |
| React.useEffect(() => { | |
| if (shouldUseMetaSettings && metaSetting?.value) { | |
| if (!preventSyncWithLS) { | |
| setSettingValueToLS(name, metaSetting.value); | |
| } | |
| const parsedValue = parseSettingValue<T>(metaSetting.value); | |
| dispatch(setSettingValue(name, parsedValue)); | |
| } | |
| }, [shouldUseMetaSettings, metaSetting, preventSyncWithLS, name, dispatch]); |
Comment on lines
+105
to
+110
| const debouncedSetMetaSetting = React.useMemo( | ||
| () => | ||
| debounce((params: SetSingleSettingParams) => { | ||
| setMetaSetting(params); | ||
| }, debounceTime), | ||
| [debounceTime], |
Contributor
There was a problem hiding this comment.
logic: debounceTime in deps but missing setMetaSetting - this creates a new debounced function whenever debounceTime changes but the underlying setMetaSetting reference changes on every render, causing unexpected behavior
Suggested change
| const debouncedSetMetaSetting = React.useMemo( | |
| () => | |
| debounce((params: SetSingleSettingParams) => { | |
| setMetaSetting(params); | |
| }, debounceTime), | |
| [debounceTime], | |
| const debouncedSetMetaSetting = React.useMemo( | |
| () => | |
| debounce((params: SetSingleSettingParams) => { | |
| setMetaSetting(params); | |
| }, debounceTime), | |
| [debounceTime, setMetaSetting], | |
| ); |
Comment on lines
+114
to
+116
| React.useEffect(() => { | ||
| return () => debouncedSetMetaSetting.flush(); | ||
| }, []); |
Contributor
There was a problem hiding this comment.
logic: missing debouncedSetMetaSetting in deps - when the debounced function reference changes, this cleanup won't flush the correct instance
Suggested change
| React.useEffect(() => { | |
| return () => debouncedSetMetaSetting.flush(); | |
| }, []); | |
| React.useEffect(() => { | |
| return () => debouncedSetMetaSetting.flush(); | |
| }, [debouncedSetMetaSetting]); |
Comment on lines
+64
to
+97
| private flushBatch() { | ||
| if (!this.requestQueue || !this.requestQueue.size || !this.currentUser) { | ||
| return; | ||
| } | ||
|
|
||
| const batch = this.requestQueue; | ||
| const user = this.currentUser; | ||
| this.requestQueue = undefined; | ||
| clearTimeout(this.batchTimeout); | ||
|
|
||
| const settingNames = Array.from(batch.keys()); | ||
|
|
||
| this.getSettings({user, name: settingNames}) | ||
| .then((response) => { | ||
| batch.forEach((pendingRequests, name) => { | ||
| const settingResult = response[name]; | ||
| if (settingResult) { | ||
| pendingRequests.forEach((request) => { | ||
| request.resolve(settingResult); | ||
| }); | ||
| } else { | ||
| pendingRequests.forEach((request) => { | ||
| request.resolve({name, user, value: undefined}); | ||
| }); | ||
| } | ||
| }); | ||
| }) | ||
| .catch((error) => { | ||
| batch.forEach((pendingRequests) => { | ||
| pendingRequests.forEach((request) => { | ||
| request.reject(error); | ||
| }); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
logic: batch timeout not cleared if flushBatch returns early due to empty queue - the batchTimeout is only cleared after checking conditions, meaning the timeout handle could leak if conditions fail
Suggested change
| private flushBatch() { | |
| if (!this.requestQueue || !this.requestQueue.size || !this.currentUser) { | |
| return; | |
| } | |
| const batch = this.requestQueue; | |
| const user = this.currentUser; | |
| this.requestQueue = undefined; | |
| clearTimeout(this.batchTimeout); | |
| const settingNames = Array.from(batch.keys()); | |
| this.getSettings({user, name: settingNames}) | |
| .then((response) => { | |
| batch.forEach((pendingRequests, name) => { | |
| const settingResult = response[name]; | |
| if (settingResult) { | |
| pendingRequests.forEach((request) => { | |
| request.resolve(settingResult); | |
| }); | |
| } else { | |
| pendingRequests.forEach((request) => { | |
| request.resolve({name, user, value: undefined}); | |
| }); | |
| } | |
| }); | |
| }) | |
| .catch((error) => { | |
| batch.forEach((pendingRequests) => { | |
| pendingRequests.forEach((request) => { | |
| request.reject(error); | |
| }); | |
| }); | |
| }); | |
| private flushBatch() { | |
| clearTimeout(this.batchTimeout); | |
| if (!this.requestQueue || !this.requestQueue.size || !this.currentUser) { | |
| return; | |
| } | |
| const batch = this.requestQueue; | |
| const user = this.currentUser; | |
| this.requestQueue = undefined; | |
| const settingNames = Array.from(batch.keys()); | |
| this.getSettings({user, name: settingNames}) | |
| .then((response) => { | |
| batch.forEach((pendingRequests, name) => { | |
| const settingResult = response[name]; | |
| if (settingResult) { | |
| pendingRequests.forEach((request) => { | |
| request.resolve(settingResult); | |
| }); | |
| } else { | |
| pendingRequests.forEach((request) => { | |
| request.resolve({name, user, value: undefined}); | |
| }); | |
| } | |
| }); | |
| }) | |
| .catch((error) => { | |
| batch.forEach((pendingRequests) => { | |
| pendingRequests.forEach((request) => { | |
| request.reject(error); | |
| }); | |
| }); | |
| }); | |
| } |
Comment on lines
+37
to
+43
| export function deleteValueFromLS(name: string | undefined) { | ||
| if (!name) { | ||
| return; | ||
| } | ||
|
|
||
| delete localStorage[name]; | ||
| } |
Contributor
There was a problem hiding this comment.
syntax: using delete localStorage[name] instead of localStorage.removeItem(name) - the delete operator doesn't properly remove items from localStorage in all browsers
Suggested change
| export function deleteValueFromLS(name: string | undefined) { | |
| if (!name) { | |
| return; | |
| } | |
| delete localStorage[name]; | |
| } | |
| export function deleteValueFromLS(name: string | undefined) { | |
| if (!name) { | |
| return; | |
| } | |
| localStorage.removeItem(name); | |
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #2892
CI Results
Test Status: ✅ PASSED
📊 Full Report
Test Changes Summary 🗑️189
🗑️ Deleted Tests (189)
Bundle Size:⚠️
Current: 0.00 KB | Main: 0.00 KB
Diff: 0.00 KB (N/A)
ℹ️ CI Information
Greptile Overview
Greptile Summary
This PR integrates YDB's meta settings service to store user settings server-side instead of only in localStorage. The implementation adds a new
MetaSettingsAPIclass with request batching, RTK Query endpoints for settings management, and a refactoreduseSettinghook that synchronizes between localStorage and the backend.Key Changes
MetaSettingsAPIclass with 100ms request batching to optimize multiple simultaneous setting requestsgetSingleSetting,setSingleSetting,getSettings) with optimistic cache updatesuseSettinghook to handle dual storage (localStorage + meta API) with configurable sync behavioruseMetaSettingsflag inuiFactoryto enable the feature conditionallyUserID) for settings attributionpreventSyncWithLSflagservices/settings.tsand related Redux actionsIssues Found
useSettinghook has multiple React dependency issues causing stale closures and missed updatesMetaSettingsAPI.flushBatch()when batch is emptydelete localStorage[name]instead of properlocalStorage.removeItem()methodArchitecture
The implementation uses a layered approach: UI components call
useSettinghook → hook manages Redux state + localStorage + meta API calls →MetaSettingsAPIbatches requests → backend stores settings. Settings can be configured to sync with localStorage or only use external storage viaSETTINGS_OPTIONS.Confidence Score: 2/5
useSettinghook has multiple missing dependencies in useEffect hooks that will cause stale closures and missed updates. The empty dependency array on line 82 means the sync effect never re-runs when dependencies change. The debounced function memo is missingsetMetaSettingcausing it to capture stale references. The cleanup effect is also missing its dependency. Additionally, there's a timeout leak in the batch flushing logic. These are logical errors that will cause settings to not sync properly between localStorage and the backend, potentially losing user data.src/store/reducers/settings/useSetting.ts- it has multiple critical dependency issues that must be fixed before mergingImportant Files Changed
File Analysis
Sequence Diagram
sequenceDiagram participant User participant Component participant useSetting participant Redux participant LocalStorage participant MetaSettingsAPI participant YDB User->>Component: Interact with UI Component->>useSetting: Call useSetting(name) alt useMetaSettings enabled useSetting->>Redux: Get user/id from auth state useSetting->>MetaSettingsAPI: getSingleSetting({name, user}) alt Request batching MetaSettingsAPI->>MetaSettingsAPI: Add to requestQueue MetaSettingsAPI->>MetaSettingsAPI: setTimeout(100ms) MetaSettingsAPI->>YDB: Batch getSettings({user, name[]}) YDB-->>MetaSettingsAPI: Return settings map MetaSettingsAPI-->>useSetting: Resolve batched requests end useSetting->>LocalStorage: Read initial value (if sync enabled) useSetting->>Redux: setSettingValue(name, value) alt Meta setting exists useSetting->>LocalStorage: Sync from backend (if enabled) useSetting->>Redux: Update with backend value else Meta setting empty AND localStorage has value useSetting->>MetaSettingsAPI: Upload localStorage value useSetting->>LocalStorage: Delete from LS (if preventSyncWithLS) end else useMetaSettings disabled useSetting->>LocalStorage: Read value useSetting->>Redux: setSettingValue(name, value) end useSetting-->>Component: Return {value, saveValue, isLoading} User->>Component: Change setting Component->>useSetting: Call saveValue(newValue) alt useMetaSettings enabled useSetting->>MetaSettingsAPI: setSingleSetting (debounced) MetaSettingsAPI->>YDB: POST /meta/user_settings YDB-->>MetaSettingsAPI: Success response MetaSettingsAPI->>Redux: Update cache optimistically end useSetting->>LocalStorage: Save value (if sync enabled) useSetting->>Redux: Update state