Eng 1616 add getconfigtree equivalent for block pros on init#944
Merged
sid597 merged 7 commits intoeng-1470-test-dual-read-with-flag-on-and-fix-gapsv2from Apr 15, 2026
Conversation
Cut plugin load from ~20925ms to ~1327ms (94%) on a real graph by collapsing
per-call settings accessors into a single bulk read at init and threading
that snapshot through the init chain + observer callbacks.
Key changes:
- accessors.ts: bulkReadSettings() runs ONE pull query against the settings
page's direct children and returns { featureFlags, globalSettings,
personalSettings } parsed via Zod. readPathValue exported.
- getDiscourseNodes / getDiscourseRelations / getAllRelations: optional
snapshot param threaded through, no breaking changes to existing callers.
- initializeDiscourseNodes + refreshConfigTree (+ registerDiscourseDatalog-
Translators, getDiscourseRelationLabels): accept and forward snapshot.
- index.ts: bulkReadSettings() at the top of init; snapshot threaded into
initializeDiscourseNodes, refreshConfigTree, initObservers,
installDiscourseFloatingMenu, setInitialQueryPages, and the 3 sync sites
inside index.ts itself.
- initializeObserversAndListeners.ts: snapshot threaded into the sync-init
body; pageTitleObserver + leftSidebarObserver callbacks call
bulkReadSettings() per fire (fresh, not stale); nodeTagPopupButtonObserver
uses per-sync-batch memoization via queueMicrotask; hashChangeListener
and nodeCreationPopoverListener use bulkReadSettings() per fire.
- findDiscourseNode: snapshot param added; getDiscourseNodes() default-arg
moved inside the cache-miss branch so cache hits don't waste the call.
- isQueryPage / isCanvasPage / QueryPagesPanel.getQueryPages: optional
snapshot param.
- LeftSidebarView.buildConfig / useConfig / mountLeftSidebar: optional
initialSnapshot threaded for the first render; emitter-driven updates
keep using live reads for post-mount reactivity.
- DiscourseFloatingMenu.installDiscourseFloatingMenu: optional snapshot.
- posthog.initPostHog: removed redundant internal getPersonalSetting check
(caller already guards from the snapshot).
- migrateLegacyToBlockProps.hasGraphMigrationMarker: accepts the existing
blockMap and does an O(1) lookup instead of a getBlockUidByTextOnPage scan.
Includes per-phase timing console.logs across index.ts, refreshConfigTree,
init.ts, initSettingsPageBlocks, and initObservers. Committed as a
checkpoint so we can reference measurements later; will be removed in the
next commit.
Removes the per-phase console.log instrumentation added in the previous commit. All the [DG Plugin] / [DG Nav] logs and their `mark()` / `markPhase()` helpers are gone. Code behavior unchanged. Dropped in this commit: - index.ts: mark() closure, load start/done logs, and all phase marks. - initializeObserversAndListeners.ts: markPhase() closure, per-observer marks, pageTitleObserver fire log, hashChangeListener [DG Nav] logs. - LeftSidebarView.tsx: openTarget [DG Nav] click/resolve logs. - refreshConfigTree.ts: mark() closure and all phase marks. - init.ts: mark() closures in initSchema and initSettingsPageBlocks. - accessors.ts: bulkReadSettings internal timing log. - index.ts: unused getPluginElapsedTime import. Previous commit (343dc11) kept as a checkpoint for future drill-downs.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![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.
…ional snapshot
- index.ts: move initPluginTimer() back to its original position (after
early-return checks) so timing isn't started for graphs that bail out.
- Replace readPathValue + `as T | undefined` casts with direct typed
indexing on the Zod-derived snapshot types across:
- index.ts (disallowDiagnostics, isStreamlineStylingEnabled)
- initializeObserversAndListeners.ts (suggestiveModeOverlay,
pagePreview, discourseContextOverlay, globalTrigger,
personalTriggerCombo, customTrigger) — also drops dead `?? "\\"`
and `?? "@"` fallbacks since Zod defaults already populate them.
- isCanvasPage.ts (canvasPageFormat)
- setQueryPages.ts + QueryPagesPanel.tsx (nested [Query][Query pages])
- setQueryPages.setInitialQueryPages: snapshot is now optional with a
getPersonalSetting fallback, matching the pattern used elsewhere
(getQueryPages, isCanvasPage, etc.).
- init.ts: restore logDualReadComparison + window.dgDualReadLog so the
on-demand console helper is available again. NOT auto-called on init —
invoke window.dgDualReadLog() manually to dump the comparison.
Capture performance.now() at the top of runExtension and log the elapsed milliseconds just before the unload handler is wired, so we have a single broad measurement of plugin init cost on each load.
…mpliance Make snapshot required at six init-only leaves where caller audit showed every site already passed one: installDiscourseFloatingMenu, initializeDiscourseNodes, setInitialQueryPages, isQueryPage, isCurrentPageCanvas, isSidebarCanvas. No cascade — only at the leaves. Drop dead fallback code that was reachable only via the optional path: - setQueryPages: legacy string|Record coercion ladder (snapshot is Zod-typed string[]) - DiscourseFloatingMenu: getPersonalSetting<boolean> cast site - DiscourseFloatingMenu: unused props parameter (no caller ever overrode default) - initializeObserversAndListeners: !== false dead pattern (Zod boolean default) - initializeObserversAndListeners: as IKeyCombo cast (schema is structurally compatible) AGENTS.md compliance for >2-arg functions: - mountLeftSidebar: object-destructured params, both call sites updated - installDiscourseFloatingMenu: kept at 2 positional via dead-props removal posthog: collapse doInitPostHog wrapper into initPostHog (caller-side gating). accessors: revert speculative readPathValue export (no consumer). LeftSidebarView/DiscourseFloatingMenu: eslint-disable react/no-deprecated on ReactDOM.render rewritten lines, matching existing codebase convention.
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
📝 WalkthroughWalkthroughThe PR introduces a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/roam/src/utils/posthog.ts (1)
8-9: Keep diagnostics gating insideinitPostHogfor defense-in-depth.Current call sites are guarded, but removing the internal check makes privacy compliance depend on every future caller. Consider preserving this function-level invariant as a safety net.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/roam/src/utils/posthog.ts` around lines 8 - 9, The function initPostHog currently lacks its internal gating check so privacy/diagnostics depend on callers; restore an early-return guard inside initPostHog that checks the existing initialized flag (and any local diagnostics-enabled flag used by this module) and returns immediately if already initialized or diagnostics are disabled, ensuring the function itself enforces the privacy/diagnostics invariant rather than relying on every caller to do so.apps/roam/src/components/settings/utils/accessors.ts (1)
920-957: Consider adding explicit return type for consistency with coding guidelines.The
bulkReadSettingsfunction correctly performs a single bulk read viaroamAlphaAPI.pulland parses each settings category with schema defaults. The implementation handles missing/malformed data gracefully via Zod's.parse()with default-equipped schemas.Per coding guidelines ("Use explicit return types for functions in TypeScript"), consider adding the return type:
💡 Optional: Add explicit return type
-export const bulkReadSettings = (): SettingsSnapshot => { +export const bulkReadSettings = (): SettingsSnapshot => {(Already correct - ignore if the return type is already there. If not, add
: SettingsSnapshotafter the parentheses.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/roam/src/components/settings/utils/accessors.ts` around lines 920 - 957, The function bulkReadSettings should have an explicit return type; ensure its signature includes ": SettingsSnapshot" (e.g., export const bulkReadSettings = (): SettingsSnapshot => { ... }); if the return type is missing, add SettingsSnapshot and keep the existing uses of FeatureFlagsSchema, GlobalSettingsSchema, and PersonalSettingsSchema unchanged.apps/roam/src/index.ts (1)
49-58: Keep the query-builder conflict branch truly fail-fast.This PR adds
bulkReadSettings()before the existing bailout, so the conflict path now does settings I/O and keeps other startup side effects ahead of the return. Move the guard above the snapshot-dependent initialization so the extension does no work when it already knows it will abort.♻️ Suggested reorder
export default runExtension(async (onloadArgs) => { const pluginLoadStart = performance.now(); - - const settingsSnapshot = bulkReadSettings(); - - const isEncrypted = window.roamAlphaAPI.graph.isEncrypted; - const isOffline = window.roamAlphaAPI.graph.type === "offline"; - const disallowDiagnostics = - settingsSnapshot.personalSettings[PERSONAL_KEYS.disableProductDiagnostics]; - if (!isEncrypted && !isOffline && !disallowDiagnostics) { - initPostHog(); - } - - initFeedbackWidget(); if (window?.roamjs?.loaded?.has("query-builder")) { renderToast({ @@ }); return; } + + const settingsSnapshot = bulkReadSettings(); + const isEncrypted = window.roamAlphaAPI.graph.isEncrypted; + const isOffline = window.roamAlphaAPI.graph.type === "offline"; + const disallowDiagnostics = + settingsSnapshot.personalSettings[PERSONAL_KEYS.disableProductDiagnostics]; + if (!isEncrypted && !isOffline && !disallowDiagnostics) { + initPostHog(); + } + + initFeedbackWidget();Also applies to: 63-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/roam/src/index.ts` around lines 49 - 58, The startup bailout check using window.roamAlphaAPI.graph (isEncrypted/isOffline) and disallowDiagnostics should run before calling bulkReadSettings(), so move the guard that inspects isEncrypted, isOffline and PERSONAL_KEYS.disableProductDiagnostics above the bulkReadSettings() call (and remove any other side-effecting initialization like initPostHog() or other startup work before the guard). Specifically, ensure bulkReadSettings() is only invoked when the guard permits startup (so change the order around bulkReadSettings, the isEncrypted/isOffline/disallowDiagnostics checks, and initPostHog), and apply the same reorder to the analogous code block referenced (lines 63-70) to keep the conflict branch fail-fast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/roam/src/utils/initializeObserversAndListeners.ts`:
- Around line 294-299: The type guard for personalTriggerComboRaw can treat null
as an object; update the check for PERSONAL_KEYS.personalNodeMenuTrigger so
personalTriggerCombo is only assigned when personalTriggerComboRaw is non-null
and an object (e.g., ensure personalTriggerComboRaw !== null && typeof
personalTriggerComboRaw === "object"), and optionally validate expected
properties (like .key) before downstream use in
initializeObserversAndListeners.ts to avoid runtime errors when accessing
personalTriggerCombo.key.
---
Nitpick comments:
In `@apps/roam/src/components/settings/utils/accessors.ts`:
- Around line 920-957: The function bulkReadSettings should have an explicit
return type; ensure its signature includes ": SettingsSnapshot" (e.g., export
const bulkReadSettings = (): SettingsSnapshot => { ... }); if the return type is
missing, add SettingsSnapshot and keep the existing uses of FeatureFlagsSchema,
GlobalSettingsSchema, and PersonalSettingsSchema unchanged.
In `@apps/roam/src/index.ts`:
- Around line 49-58: The startup bailout check using window.roamAlphaAPI.graph
(isEncrypted/isOffline) and disallowDiagnostics should run before calling
bulkReadSettings(), so move the guard that inspects isEncrypted, isOffline and
PERSONAL_KEYS.disableProductDiagnostics above the bulkReadSettings() call (and
remove any other side-effecting initialization like initPostHog() or other
startup work before the guard). Specifically, ensure bulkReadSettings() is only
invoked when the guard permits startup (so change the order around
bulkReadSettings, the isEncrypted/isOffline/disallowDiagnostics checks, and
initPostHog), and apply the same reorder to the analogous code block referenced
(lines 63-70) to keep the conflict branch fail-fast.
In `@apps/roam/src/utils/posthog.ts`:
- Around line 8-9: The function initPostHog currently lacks its internal gating
check so privacy/diagnostics depend on callers; restore an early-return guard
inside initPostHog that checks the existing initialized flag (and any local
diagnostics-enabled flag used by this module) and returns immediately if already
initialized or diagnostics are disabled, ensuring the function itself enforces
the privacy/diagnostics invariant rather than relying on every caller to do so.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4f0c6cb4-7a80-43e0-ba7a-df5ed987b5e8
📒 Files selected for processing (19)
apps/roam/src/components/DiscourseFloatingMenu.tsxapps/roam/src/components/LeftSidebarView.tsxapps/roam/src/components/settings/QueryPagesPanel.tsxapps/roam/src/components/settings/utils/accessors.tsapps/roam/src/components/settings/utils/init.tsapps/roam/src/components/settings/utils/migrateLegacyToBlockProps.tsapps/roam/src/index.tsapps/roam/src/utils/findDiscourseNode.tsapps/roam/src/utils/getDiscourseNodes.tsapps/roam/src/utils/getDiscourseRelationLabels.tsapps/roam/src/utils/getDiscourseRelations.tsapps/roam/src/utils/initializeDiscourseNodes.tsapps/roam/src/utils/initializeObserversAndListeners.tsapps/roam/src/utils/isCanvasPage.tsapps/roam/src/utils/isQueryPage.tsapps/roam/src/utils/posthog.tsapps/roam/src/utils/refreshConfigTree.tsapps/roam/src/utils/registerDiscourseDatalogTranslators.tsapps/roam/src/utils/setQueryPages.ts
Comment on lines
+294
to
+299
| const personalTriggerComboRaw = | ||
| settingsSnapshot.personalSettings[PERSONAL_KEYS.personalNodeMenuTrigger]; | ||
| const personalTriggerCombo = | ||
| typeof personalTriggerComboRaw === "object" | ||
| ? personalTriggerComboRaw | ||
| : undefined; |
Contributor
There was a problem hiding this comment.
Potential null handling issue in type check.
The runtime type check typeof personalTriggerComboRaw === "object" will also return true for null values (JavaScript quirk). If null is a possible value from settings, this could cause issues downstream when accessing .key.
🛡️ Suggested defensive fix
const personalTriggerComboRaw =
settingsSnapshot.personalSettings[PERSONAL_KEYS.personalNodeMenuTrigger];
const personalTriggerCombo =
- typeof personalTriggerComboRaw === "object"
+ personalTriggerComboRaw && typeof personalTriggerComboRaw === "object"
? personalTriggerComboRaw
: undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/roam/src/utils/initializeObserversAndListeners.ts` around lines 294 -
299, The type guard for personalTriggerComboRaw can treat null as an object;
update the check for PERSONAL_KEYS.personalNodeMenuTrigger so
personalTriggerCombo is only assigned when personalTriggerComboRaw is non-null
and an object (e.g., ensure personalTriggerComboRaw !== null && typeof
personalTriggerComboRaw === "object"), and optionally validate expected
properties (like .key) before downstream use in
initializeObserversAndListeners.ts to avoid runtime errors when accessing
personalTriggerCombo.key.
mdroidian
approved these changes
Apr 9, 2026
Member
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
nit: Calling the settings snapshot "snapshot" or "callbackSnapshot" is ambiguous and will get confusing when it is referenced further down the page. At the same time, settingsSnapshot would be quite long. Let's just use settings instead of the multiple other uses: snapshot, callbackSnapshot, snap, settingsSnapshot, etc
| personalSettings: PersonalSettings; | ||
| }; | ||
|
|
||
| export const bulkReadSettings = (): SettingsSnapshot => { |
Member
There was a problem hiding this comment.
I believe getPersonalSetting was checking if isNewSettingsStoreEnabled, but it doesn't look like bulkReadSettings is doing this anymore.
Specifically example would be: const disallowDiagnostics = settingsSnapshot.personalSettings[PERSONAL_KEYS.disableProductDiagnostics]; If the new store settings aren't enabled, would the migration have happened yet?
Collaborator
Author
There was a problem hiding this comment.
added the check, I missed it.
| const props = { title, h1, onloadArgs }; | ||
|
|
||
| const isSuggestiveModeEnabled = getFeatureFlag("Suggestive mode enabled"); | ||
| const callbackSnapshot = bulkReadSettings(); |
Member
There was a problem hiding this comment.
nit: callbackSnapshot is confusing here. This is not returning a callback and snapshot is ambiguous. Just settings would probably be more clear.
|
|
||
| const isEncrypted = window.roamAlphaAPI.graph.isEncrypted; | ||
| const isOffline = window.roamAlphaAPI.graph.type === "offline"; | ||
| const disallowDiagnostics = getPersonalSetting<boolean>([ |
Member
There was a problem hiding this comment.
We should move all three checks inside of initPostHog because it is also called from a flag in settings (enable/disable). This is technically out of scope here.
…move PostHog guards - Rename settingsSnapshot/callbackSnapshot/snap/navSnapshot → settings - bulkReadSettings now checks "Use new settings store" flag and falls back to legacy reads when off, matching individual getter behavior - Move encryption/offline guards into initPostHog (diagnostics check stays at call site to avoid race with async setSetting in enablePostHog)
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
sid597
merged commit d8ff16f
into
eng-1470-test-dual-read-with-flag-on-and-fix-gapsv2
8 checks passed
Comment on lines
+15
to
+16
| const queryPageArray = | ||
| snapshot.personalSettings[PERSONAL_KEYS.query][QUERY_KEYS.queryPages]; |
Contributor
There was a problem hiding this comment.
🔴 Legacy query-pages format (string/Record) causes data corruption or crash in setInitialQueryPages
When the new settings store is disabled, bulkReadSettings() returns legacy values via an unsafe as PersonalSettings cast (accessors.ts:958), without Zod validation. The legacy extensionAPI could store query-pages as string | string[] | Record<string, string>. The old setInitialQueryPages had coercion logic to handle these formats, but the new code directly accesses snapshot.personalSettings[PERSONAL_KEYS.query][QUERY_KEYS.queryPages] assuming it's always string[]. If the legacy value is a plain string (e.g. "queries/*"), [...queryPageArray, ...] spreads it into individual characters, corrupting the saved setting. If it's a Record<string, string>, .includes() throws a TypeError. The sibling function getQueryPages() in QueryPagesPanel.tsx:17 already handles this coercion correctly and should be reused here.
Suggested change
| const queryPageArray = | |
| snapshot.personalSettings[PERSONAL_KEYS.query][QUERY_KEYS.queryPages]; | |
| const queryPageArray = getQueryPages(snapshot); |
Was this helpful? React with 👍 or 👎 to provide feedback.
mdroidian
reviewed
Apr 15, 2026
| personalSettings: PersonalSettingsSchema.parse( | ||
| readAllLegacyPersonalSettings(), | ||
| ), | ||
| globalSettings: readAllLegacyGlobalSettings() as GlobalSettings, |
sid597
added a commit
that referenced
this pull request
Apr 15, 2026
* ENG-1470: Fix dual-read gaps found during flag-ON validation
Bootstrap legacy config blocks in initSchema so the plugin works on
fresh graphs without createConfigObserver/configPageTabs:
- trigger, grammar/relations, export, Suggestive Mode, Left Sidebar
- Reuses existing ensureBlocksExist/buildBlockMap helpers + DEFAULT_RELATION_VALUES
Fix duplicate block accumulation bugs:
- Page Groups: getSubTree auto-create race (ensureLegacyConfigBlocks pre-creates)
- Folded: lookup-based delete via getBasicTreeByParentUid instead of stale uid
- scratch/enabled: switched getSubTree({ parentUid }) to tree-based reads
- Folded in convertToComplexSection: removed erroneous block creation
Fix dual-read comparison:
- Replace JSON.stringify with deepEqual (handles key order, undefined/empty/false)
- Deterministic async ordering: await legacy write → refreshConfigTree → blockProp write
(BlockPropSettingPanels, LeftSidebarView toggleFoldedState, AdminPanel suggestive mode)
- Use getPersonalSettings() (raw read) in toggleFoldedState to avoid mid-write comparison
Fix storedRelations import path (renderNodeConfigPage → data/constants)
* Fix dual-read mismatches and ZodError on discourse node parse
* Fix dual-read mismatches: alias timing, key-image re-render, deepEqual null
* Fix prettier formatting
* ENG-1574: Add dual-read console logs to setting setters (#914)
* ENG-1574: Add dual-read console logs to setting setters
Log legacy and block prop values with match/mismatch status
when a setting is changed. Fix broken import in storedRelations.
* ENG-1574: Add init-time dual-read log and window.dgDualReadLog()
Log all legacy vs block prop settings on init. Remove setter
logging. Expose dgDualReadLog() on window for on-demand use.
* ENG-1574: Fix eslint naming-convention warnings in init.ts
* ENG-1574: Use deepEqual instead of JSON.stringify for comparison
JSON.stringify is key-order dependent, causing false mismatches
when legacy and block props return keys in different order.
* ENG-1574: Remove dead code, use deepEqual for comparison
* ENG-1574: Fix review feedback — try-catch, flag exclusion, type guard
* Eng 1616 add getconfigtree equivalent for block pros on init (#944)
* ENG-1616: Bulk-read settings + thread snapshot (with timing logs)
Cut plugin load from ~20925ms to ~1327ms (94%) on a real graph by collapsing
per-call settings accessors into a single bulk read at init and threading
that snapshot through the init chain + observer callbacks.
Key changes:
- accessors.ts: bulkReadSettings() runs ONE pull query against the settings
page's direct children and returns { featureFlags, globalSettings,
personalSettings } parsed via Zod. readPathValue exported.
- getDiscourseNodes / getDiscourseRelations / getAllRelations: optional
snapshot param threaded through, no breaking changes to existing callers.
- initializeDiscourseNodes + refreshConfigTree (+ registerDiscourseDatalog-
Translators, getDiscourseRelationLabels): accept and forward snapshot.
- index.ts: bulkReadSettings() at the top of init; snapshot threaded into
initializeDiscourseNodes, refreshConfigTree, initObservers,
installDiscourseFloatingMenu, setInitialQueryPages, and the 3 sync sites
inside index.ts itself.
- initializeObserversAndListeners.ts: snapshot threaded into the sync-init
body; pageTitleObserver + leftSidebarObserver callbacks call
bulkReadSettings() per fire (fresh, not stale); nodeTagPopupButtonObserver
uses per-sync-batch memoization via queueMicrotask; hashChangeListener
and nodeCreationPopoverListener use bulkReadSettings() per fire.
- findDiscourseNode: snapshot param added; getDiscourseNodes() default-arg
moved inside the cache-miss branch so cache hits don't waste the call.
- isQueryPage / isCanvasPage / QueryPagesPanel.getQueryPages: optional
snapshot param.
- LeftSidebarView.buildConfig / useConfig / mountLeftSidebar: optional
initialSnapshot threaded for the first render; emitter-driven updates
keep using live reads for post-mount reactivity.
- DiscourseFloatingMenu.installDiscourseFloatingMenu: optional snapshot.
- posthog.initPostHog: removed redundant internal getPersonalSetting check
(caller already guards from the snapshot).
- migrateLegacyToBlockProps.hasGraphMigrationMarker: accepts the existing
blockMap and does an O(1) lookup instead of a getBlockUidByTextOnPage scan.
Includes per-phase timing console.logs across index.ts, refreshConfigTree,
init.ts, initSettingsPageBlocks, and initObservers. Committed as a
checkpoint so we can reference measurements later; will be removed in the
next commit.
* ENG-1616: Remove plugin-load timing logs
Removes the per-phase console.log instrumentation added in the previous
commit. All the [DG Plugin] / [DG Nav] logs and their `mark()` / `markPhase()`
helpers are gone. Code behavior unchanged.
Dropped in this commit:
- index.ts: mark() closure, load start/done logs, and all phase marks.
- initializeObserversAndListeners.ts: markPhase() closure, per-observer marks,
pageTitleObserver fire log, hashChangeListener [DG Nav] logs.
- LeftSidebarView.tsx: openTarget [DG Nav] click/resolve logs.
- refreshConfigTree.ts: mark() closure and all phase marks.
- init.ts: mark() closures in initSchema and initSettingsPageBlocks.
- accessors.ts: bulkReadSettings internal timing log.
- index.ts: unused getPluginElapsedTime import.
Previous commit (343dc11) kept as a checkpoint for future drill-downs.
* ENG-1616: Address review — typed indexing, restore dgDualReadLog, optional snapshot
- index.ts: move initPluginTimer() back to its original position (after
early-return checks) so timing isn't started for graphs that bail out.
- Replace readPathValue + `as T | undefined` casts with direct typed
indexing on the Zod-derived snapshot types across:
- index.ts (disallowDiagnostics, isStreamlineStylingEnabled)
- initializeObserversAndListeners.ts (suggestiveModeOverlay,
pagePreview, discourseContextOverlay, globalTrigger,
personalTriggerCombo, customTrigger) — also drops dead `?? "\\"`
and `?? "@"` fallbacks since Zod defaults already populate them.
- isCanvasPage.ts (canvasPageFormat)
- setQueryPages.ts + QueryPagesPanel.tsx (nested [Query][Query pages])
- setQueryPages.setInitialQueryPages: snapshot is now optional with a
getPersonalSetting fallback, matching the pattern used elsewhere
(getQueryPages, isCanvasPage, etc.).
- init.ts: restore logDualReadComparison + window.dgDualReadLog so the
on-demand console helper is available again. NOT auto-called on init —
invoke window.dgDualReadLog() manually to dump the comparison.
* ENG-1616: Log total plugin load time
Capture performance.now() at the top of runExtension and log the
elapsed milliseconds just before the unload handler is wired, so we
have a single broad measurement of plugin init cost on each load.
* ENG-1616: Tighten init-only leaves to required snapshot, AGENTS.md compliance
Make snapshot required at six init-only leaves where caller audit
showed every site already passed one: installDiscourseFloatingMenu,
initializeDiscourseNodes, setInitialQueryPages, isQueryPage,
isCurrentPageCanvas, isSidebarCanvas. No cascade — only at the leaves.
Drop dead fallback code that was reachable only via the optional path:
- setQueryPages: legacy string|Record coercion ladder (snapshot is Zod-typed string[])
- DiscourseFloatingMenu: getPersonalSetting<boolean> cast site
- DiscourseFloatingMenu: unused props parameter (no caller ever overrode default)
- initializeObserversAndListeners: !== false dead pattern (Zod boolean default)
- initializeObserversAndListeners: as IKeyCombo cast (schema is structurally compatible)
AGENTS.md compliance for >2-arg functions:
- mountLeftSidebar: object-destructured params, both call sites updated
- installDiscourseFloatingMenu: kept at 2 positional via dead-props removal
posthog: collapse doInitPostHog wrapper into initPostHog (caller-side gating).
accessors: revert speculative readPathValue export (no consumer).
LeftSidebarView/DiscourseFloatingMenu: eslint-disable react/no-deprecated on
ReactDOM.render rewritten lines, matching existing codebase convention.
* ENG-1616: Address review — rename snapshot vars, flag-gate bulkRead, move PostHog guards
- Rename settingsSnapshot/callbackSnapshot/snap/navSnapshot → settings
- bulkReadSettings now checks "Use new settings store" flag and falls
back to legacy reads when off, matching individual getter behavior
- Move encryption/offline guards into initPostHog (diagnostics check
stays at call site to avoid race with async setSetting in enablePostHog)
* Fix legacy bulk settings fallback
* ENG-1617: se existing 'getConfigTree equivalent functions' for specific setting groups (#946)
* ENG-1616: Bulk-read settings + thread snapshot (with timing logs)
Cut plugin load from ~20925ms to ~1327ms (94%) on a real graph by collapsing
per-call settings accessors into a single bulk read at init and threading
that snapshot through the init chain + observer callbacks.
Key changes:
- accessors.ts: bulkReadSettings() runs ONE pull query against the settings
page's direct children and returns { featureFlags, globalSettings,
personalSettings } parsed via Zod. readPathValue exported.
- getDiscourseNodes / getDiscourseRelations / getAllRelations: optional
snapshot param threaded through, no breaking changes to existing callers.
- initializeDiscourseNodes + refreshConfigTree (+ registerDiscourseDatalog-
Translators, getDiscourseRelationLabels): accept and forward snapshot.
- index.ts: bulkReadSettings() at the top of init; snapshot threaded into
initializeDiscourseNodes, refreshConfigTree, initObservers,
installDiscourseFloatingMenu, setInitialQueryPages, and the 3 sync sites
inside index.ts itself.
- initializeObserversAndListeners.ts: snapshot threaded into the sync-init
body; pageTitleObserver + leftSidebarObserver callbacks call
bulkReadSettings() per fire (fresh, not stale); nodeTagPopupButtonObserver
uses per-sync-batch memoization via queueMicrotask; hashChangeListener
and nodeCreationPopoverListener use bulkReadSettings() per fire.
- findDiscourseNode: snapshot param added; getDiscourseNodes() default-arg
moved inside the cache-miss branch so cache hits don't waste the call.
- isQueryPage / isCanvasPage / QueryPagesPanel.getQueryPages: optional
snapshot param.
- LeftSidebarView.buildConfig / useConfig / mountLeftSidebar: optional
initialSnapshot threaded for the first render; emitter-driven updates
keep using live reads for post-mount reactivity.
- DiscourseFloatingMenu.installDiscourseFloatingMenu: optional snapshot.
- posthog.initPostHog: removed redundant internal getPersonalSetting check
(caller already guards from the snapshot).
- migrateLegacyToBlockProps.hasGraphMigrationMarker: accepts the existing
blockMap and does an O(1) lookup instead of a getBlockUidByTextOnPage scan.
Includes per-phase timing console.logs across index.ts, refreshConfigTree,
init.ts, initSettingsPageBlocks, and initObservers. Committed as a
checkpoint so we can reference measurements later; will be removed in the
next commit.
* ENG-1616: Remove plugin-load timing logs
Removes the per-phase console.log instrumentation added in the previous
commit. All the [DG Plugin] / [DG Nav] logs and their `mark()` / `markPhase()`
helpers are gone. Code behavior unchanged.
Dropped in this commit:
- index.ts: mark() closure, load start/done logs, and all phase marks.
- initializeObserversAndListeners.ts: markPhase() closure, per-observer marks,
pageTitleObserver fire log, hashChangeListener [DG Nav] logs.
- LeftSidebarView.tsx: openTarget [DG Nav] click/resolve logs.
- refreshConfigTree.ts: mark() closure and all phase marks.
- init.ts: mark() closures in initSchema and initSettingsPageBlocks.
- accessors.ts: bulkReadSettings internal timing log.
- index.ts: unused getPluginElapsedTime import.
Previous commit (343dc11) kept as a checkpoint for future drill-downs.
* ENG-1616: Address review — typed indexing, restore dgDualReadLog, optional snapshot
- index.ts: move initPluginTimer() back to its original position (after
early-return checks) so timing isn't started for graphs that bail out.
- Replace readPathValue + `as T | undefined` casts with direct typed
indexing on the Zod-derived snapshot types across:
- index.ts (disallowDiagnostics, isStreamlineStylingEnabled)
- initializeObserversAndListeners.ts (suggestiveModeOverlay,
pagePreview, discourseContextOverlay, globalTrigger,
personalTriggerCombo, customTrigger) — also drops dead `?? "\\"`
and `?? "@"` fallbacks since Zod defaults already populate them.
- isCanvasPage.ts (canvasPageFormat)
- setQueryPages.ts + QueryPagesPanel.tsx (nested [Query][Query pages])
- setQueryPages.setInitialQueryPages: snapshot is now optional with a
getPersonalSetting fallback, matching the pattern used elsewhere
(getQueryPages, isCanvasPage, etc.).
- init.ts: restore logDualReadComparison + window.dgDualReadLog so the
on-demand console helper is available again. NOT auto-called on init —
invoke window.dgDualReadLog() manually to dump the comparison.
* ENG-1616: Log total plugin load time
Capture performance.now() at the top of runExtension and log the
elapsed milliseconds just before the unload handler is wired, so we
have a single broad measurement of plugin init cost on each load.
* ENG-1616: Tighten init-only leaves to required snapshot, AGENTS.md compliance
Make snapshot required at six init-only leaves where caller audit
showed every site already passed one: installDiscourseFloatingMenu,
initializeDiscourseNodes, setInitialQueryPages, isQueryPage,
isCurrentPageCanvas, isSidebarCanvas. No cascade — only at the leaves.
Drop dead fallback code that was reachable only via the optional path:
- setQueryPages: legacy string|Record coercion ladder (snapshot is Zod-typed string[])
- DiscourseFloatingMenu: getPersonalSetting<boolean> cast site
- DiscourseFloatingMenu: unused props parameter (no caller ever overrode default)
- initializeObserversAndListeners: !== false dead pattern (Zod boolean default)
- initializeObserversAndListeners: as IKeyCombo cast (schema is structurally compatible)
AGENTS.md compliance for >2-arg functions:
- mountLeftSidebar: object-destructured params, both call sites updated
- installDiscourseFloatingMenu: kept at 2 positional via dead-props removal
posthog: collapse doInitPostHog wrapper into initPostHog (caller-side gating).
accessors: revert speculative readPathValue export (no consumer).
LeftSidebarView/DiscourseFloatingMenu: eslint-disable react/no-deprecated on
ReactDOM.render rewritten lines, matching existing codebase convention.
* ENG-1617: Single-pull settings reads + dialog snapshot threading
`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.
* ENG-1617: Refresh snapshot on Home tab nav + reuse readPathValue
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.
* ENG-1617: Per-tab snapshot threading via bulkReadSettings
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.
* ENG-1617: Lift settings snapshot to SettingsDialog, thread to all tabs
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.
* ENG-1617: Remove timing instrumentation, per-call dual-read, flag-aware 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
* ENG-1617: Eliminate double bulkReadSettings calls in accessor functions
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.
* ENG-1617: Re-read snapshot on tab change to prevent stale initialValues
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.
* ENG-1616: Address review — rename snapshot vars, flag-gate bulkRead, move PostHog guards
- Rename settingsSnapshot/callbackSnapshot/snap/navSnapshot → settings
- bulkReadSettings now checks "Use new settings store" flag and falls
back to legacy reads when off, matching individual getter behavior
- Move encryption/offline guards into initPostHog (diagnostics check
stays at call site to avoid race with async setSetting in enablePostHog)
* ENG-1617: Fix DiscourseNodeSelectPanel fallback to use first option instead of empty string
* ENG-1617: Rename snapshot variables to settings for clarity
* Fix legacy bulk settings fallback
* fix bug similar code
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
2 participants
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.
Part1: https://www.loom.com/share/c1f619a80e5c4faeb97982ecf7963e7d
Part 2: https://www.loom.com/share/bde6da6766f0435aa4af39b57678706f
Summary by CodeRabbit
New Features
Bug Fixes
Refactor