Eng 1616 add getconfigtree equivalent for block pros on init#944
Open
sid597 wants to merge 6 commits intoeng-1470-test-dual-read-with-flag-on-and-fix-gapsv2from
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)
Comment on lines
+954
to
+961
| if (!featureFlags["Use new settings store"]) { | ||
| return { | ||
| featureFlags, | ||
| globalSettings: GlobalSettingsSchema.parse(readAllLegacyGlobalSettings()), | ||
| personalSettings: PersonalSettingsSchema.parse( | ||
| readAllLegacyPersonalSettings(), | ||
| ), | ||
| }; |
Contributor
There was a problem hiding this comment.
🔴 bulkReadSettings applies strict zod parse to legacy data that old code paths returned raw
When the new settings store is disabled (the default), bulkReadSettings() calls PersonalSettingsSchema.parse(readAllLegacyPersonalSettings()) which applies strict zod schema validation to legacy data. The old individual-read code path (getPersonalSetting) explicitly bypassed schema validation for legacy data, returning raw values (getLegacyPersonalSetting(keys) as T). The codebase documents that legacy query pages can be stored as string | string[] | Record<string, string> (apps/roam/src/components/settings/QueryPagesPanel.tsx:14), but QuerySettingsSchema defines "Query pages": z.array(z.string()) which would reject a Record<string, string> or bare string, causing .parse() to throw. Since bulkReadSettings() is called at the very start of extension loading (apps/roam/src/index.ts:51), before migration runs (apps/roam/src/index.ts:192), this would crash the extension on load for affected users.
Affected code path in bulkReadSettings
The legacy fallback at accessors.ts:954-961 uses .parse() (throws on failure) rather than .safeParse() with graceful fallback:
if (!featureFlags["Use new settings store"]) {
return {
featureFlags,
globalSettings: GlobalSettingsSchema.parse(readAllLegacyGlobalSettings()),
personalSettings: PersonalSettingsSchema.parse(
readAllLegacyPersonalSettings(),
),
};
}
The migration code (migrateLegacyToBlockProps.ts:73) correctly uses .safeParse() for this exact reason.
Prompt for agents
In bulkReadSettings() at accessors.ts lines 954-961, the legacy fallback path uses PersonalSettingsSchema.parse() and GlobalSettingsSchema.parse() on raw legacy data. This can throw for users with non-standard legacy formats (e.g., query pages stored as Record<string,string> or bare string, which the codebase explicitly handles in getQueryPages). The individual getPersonalSetting/getGlobalSetting functions bypass schema validation for legacy data, but bulkReadSettings does not.
The fix should use .safeParse() with a graceful fallback instead of .parse(). If parsing fails, fall back to schema defaults (e.g., GlobalSettingsSchema.parse({}) and PersonalSettingsSchema.parse({})) or log the issue without crashing. The migration code in migrateLegacyToBlockProps.ts already demonstrates the safeParse pattern. Alternatively, apply the same coercion logic that getQueryPages uses before parsing.
Was this helpful? React with 👍 or 👎 to provide feedback.
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