ENG-1617: se existing 'getConfigTree equivalent functions' for specific setting groups#946
Open
sid597 wants to merge 9 commits intoeng-1616-add-getconfigtree-equivalent-for-block-pros-on-initfrom
Conversation
`getBlockPropBasedSettings` now does one Roam `pull` that returns the settings page's children with their string + uid + props in one shot, replacing the `q`-based `getBlockUidByTextOnPage` (~290ms per call) plus a second `pull` for props. `setBlockPropBasedSettings` reuses the same helper for the uid lookup so we have a single pull-and-walk pattern. `SettingsDialog` captures a full settings snapshot once at mount via `useState(() => bulkReadSettings())` and threads `featureFlags` and `personalSettings` down to `HomePersonalSettings`. Each child component (`PersonalFlagPanel`, `NodeMenuTriggerComponent`, `NodeSearchMenuTriggerSetting`, `KeyboardShortcutInput`) accepts an `initialValue` prop and seeds its local state from the snapshot instead of calling `getPersonalSetting` on mount. `PersonalFlagPanel`'s `initialValue` precedence flips so the prop wins when provided; `QuerySettings` callers without a prop still hit the existing fallback. `getDiscourseNodes`, `getDiscourseRelations`, and `getAllRelations` narrow their snapshot parameter to `Pick<SettingsSnapshot, ...>` to declare which fields each function actually reads. Adds a one-line `console.log` in `SettingsDialog` reporting the dialog open time, kept as an ongoing perf monitor.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
📝 WalkthroughWalkthroughA refactoring that shifts settings initialization from component-level accessor reads to snapshot-injection. Components now receive initial values and settings data through props rather than calling Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
CodeRabbit catch: with `renderActiveTabPanelOnly={true}`, the Home tab's
panel unmounts/remounts when the user navigates away and back. Each
re-mount re-runs `useState(() => initialValue ?? false)` in
`BaseFlagPanel`, re-seeding from whatever `initialValue` is currently
passed. Because the dialog held the snapshot in a non-updating
`useState`, that path served stale values, so toggles made earlier in
the same dialog session would visually revert after a tab round-trip.
Fix: hold the snapshot in a stateful slot and refresh it via
`bulkReadSettings()` from the Tabs `onChange` handler when the user
navigates back to Home. The setState batches with `setActiveTabId`, so
the new mount sees the fresh snapshot in the same render.
Also replace the inline reducer in `getBlockPropBasedSettings` with the
existing `readPathValue` util — same traversal but consistent with the
rest of the file and adds array-index handling for free.
Replaces the dialog-level snapshot from earlier commits with a per-tab snapshot model that scales to every tab without per-tab plumbing in the dialog itself. In accessors.ts, the three plural getters (getFeatureFlags, getGlobalSettings, getPersonalSettings) now delegate to the existing bulkReadSettings, which does one Roam pull on the settings page and parses all three schemas in a single pass. The slow q-based getBlockPropBasedSettings is deleted (it was only used by the plural getters and the set path); setBlockPropBasedSettings goes back to calling getBlockUidByTextOnPage directly. Writes are infrequent enough that the q cost is acceptable on the set path. Each tab container that renders panels at mount captures one snapshot via useState(() => bulkReadSettings()) and threads the relevant slice as initialValue down to its panels: HomePersonalSettings, QuerySettings, GeneralSettings, ExportSettings. The Personal and Global panels in BlockPropSettingPanels.tsx flip their initialValue precedence to prefer the prop and fall back to the live read only when the prop is missing, so callers that don't pass initialValue (e.g. LeftSidebarGlobalSettings, which already passes its own value) continue to behave the same way. NodeMenuTriggerComponent, NodeSearchMenuTriggerSetting, and KeyboardShortcutInput accept an initialValue prop and seed local state from it instead of calling getPersonalSetting in their useState initializer. Settings.tsx wraps getDiscourseNodes() in useState so it doesn't re-run on every dialog re-render. The dialog-level snapshot, the snapshot-refresh-on-Home-tab-nav workaround, and the Pick<SettingsSnapshot, ...> type narrowings are all gone.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Move bulkReadSettings() from per-tab useState into a single call at SettingsDialog mount. Each tab receives its snapshot slice (globalSettings, personalSettings, featureFlags) as props. Remove dual-read mismatch console.warn logic from accessors. Make initialValue caller-provided in BlockPropSettingPanel wrappers. Add TabTiming wrapper for per-tab commit/paint perf measurement.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…re bulkReadSettings - Remove TabTiming component and all console.log timing from Settings dialog - Remove per-call dual-read comparison from getGlobalSetting, getPersonalSetting, getDiscourseNodeSetting (keep logDualReadComparison for manual use) - Make bulkReadSettings flag-aware: reads from legacy when flag is OFF, block props when ON - Remove accessor fallbacks from Global/Personal wrapper panels (initialValue now always passed from snapshot) - Remove getGlobalSetting/getPersonalSetting imports from BlockPropSettingPanels
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
getGlobalSetting, getPersonalSetting, getFeatureFlag, getDiscourseNodeSetting now each do a single bulkReadSettings() call instead of calling isNewSettingsStoreEnabled() (which triggered a separate bulkReadSettings) followed by another bulkReadSettings via the getter. bulkReadSettings already handles the flag check and legacy/block-props routing internally.
Replace useState with useMemo keyed on activeTabId so bulkReadSettings() runs fresh each time the user switches tabs. Fixes stale snapshot when renderActiveTabPanelOnly unmounts/remounts panels.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…nstead of empty string
sid597
commented
Apr 10, 2026
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
https://www.loom.com/share/3bed40fc5de943dcb4e7181888b42442
getBlockPropBasedSettingsnow does one Roampullthat returns the settings page's children with their string + uid + props in one shot, replacing theq-basedgetBlockUidByTextOnPage(~290ms per call) plus a secondpullfor props.setBlockPropBasedSettingsreuses the same helper for the uid lookup so we have a single pull-and-walk pattern.SettingsDialogcaptures a full settings snapshot once at mount viauseState(() => bulkReadSettings())and threadsfeatureFlagsandpersonalSettingsdown toHomePersonalSettings. Each child component (PersonalFlagPanel,NodeMenuTriggerComponent,NodeSearchMenuTriggerSetting,KeyboardShortcutInput) accepts aninitialValueprop and seeds its local state from the snapshot instead of callinggetPersonalSettingon mount.PersonalFlagPanel'sinitialValueprecedence flips so the prop wins when provided;QuerySettingscallers without a prop still hit the existing fallback.getDiscourseNodes,getDiscourseRelations, andgetAllRelationsnarrow their snapshot parameter toPick<SettingsSnapshot, ...>to declare which fields each function actually reads.Adds a one-line
console.loginSettingsDialogreporting the dialog open time, kept as an ongoing perf monitor.Summary by CodeRabbit
Release Notes