ENG-1668: Perf: slow load time#984
Merged
sid597 merged 9 commits intomigration-block-init-staging-branchfrom May 6, 2026
Merged
ENG-1668: Perf: slow load time#984sid597 merged 9 commits intomigration-block-init-staging-branchfrom
sid597 merged 9 commits intomigration-block-init-staging-branchfrom
Conversation
…ConfigTree Remove initDiscourseNodePages (and helpers hasNonDefaultNodes, initSingleDiscourseNode) from initSchema — migrateDiscourseNodes already handles block prop writes, making this redundant on every load. Make the second refreshConfigTree(settings) conditional — only runs when initializeDiscourseNodes actually creates pages (first install). On existing graphs the tree is identical to what the first call already read.
|
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}
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
📝 WalkthroughWalkthroughThis PR refactors the Roam configuration initialization flow. It removes discourse node page creation logic from init.ts, updates setting accessor functions to accept an explicit config tree parameter instead of always fetching from the settings page, makes initializeDiscourseNodes return a boolean, and conditions the config tree refresh on whether nodes were created. ChangesSettings and Config Tree Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/roam/src/utils/initializeObserversAndListeners.ts (2)
123-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid the second settings pull in this observer.
Line 126 calls
getFeatureFlag(), which does anotherbulkReadSettings()even though this callback already loadedsettingson Line 113. This observer is on the startup hot path, so the extra pull directly works against the perf fix here. Read the flag from the local snapshot instead.Suggested change
- if (getFeatureFlag("Duplicate node alert enabled")) { + if (settings.featureFlags["Duplicate node alert enabled"]) { renderPossibleDuplicates(h1, title, node); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/roam/src/utils/initializeObserversAndListeners.ts` around lines 123 - 127, This observer currently calls getFeatureFlag("Duplicate node alert enabled") which triggers a bulkReadSettings() again; instead read the flag from the local settings snapshot that was loaded earlier in this function (the same settings variable populated around line 113) and use that boolean to gate renderPossibleDuplicates; replace the getFeatureFlag call with a check like settings["Duplicate node alert enabled"] (coerce to boolean as needed) so you avoid the extra settings pull while keeping renderDiscourseContext and renderPossibleDuplicates unchanged.
117-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInvalidate the
findDiscourseNodecache when settings change.Passing
snapshothere will not refresh prior matches becauseapps/roam/src/utils/findDiscourseNode.tsreturns the uid-cached value before it ever looks atsnapshot. A page that was cached asfalsewill keep missing discourse UI after node definitions change until the extension reloads.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/roam/src/utils/initializeObserversAndListeners.ts` around lines 117 - 121, The cached result from findDiscourseNode is not being invalidated when settings (snapshot) change, so a page previously cached as false will never pick up updated node definitions; fix by adding cache invalidation in findDiscourseNode (e.g., include snapshot in the cache key or expose a clear/reset function) and then call that invalidation from initializeObserversAndListeners before calling findDiscourseNode (the call shown: findDiscourseNode({ uid, title, snapshot: settings })) whenever settings change; alternatively, stop passing snapshot and instead ensure findDiscourseNode's cache logic considers the settings snapshot so results are recomputed on settings updates.apps/roam/src/utils/initializeDiscourseNodes.ts (1)
13-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn
trueonly when a node page was actually created.If the graph already has the default node pages but no user-defined nodes, this branch still returns
truebecausegetPageUidByPageTitle(...)short-circuits the||expression. That makesindex.tsrefresh the config tree on every startup, so the new boolean does not actually represent "writes happened".Suggested change
if (nodes.length === 0) { - await Promise.all( - INITIAL_NODE_VALUES.map( - (n) => - getPageUidByPageTitle(`discourse-graph/nodes/${n.text}`) || - createPage({ - title: `discourse-graph/nodes/${n.text}`, - uid: n.type, - tree: [ - { text: "Format", children: [{ text: n.format || "" }] }, - { text: "Shortcut", children: [{ text: n.shortcut || "" }] }, - { text: "Tag", children: [{ text: n.tag || "" }] }, - { text: "Graph Overview" }, - { - text: "Canvas", - children: [ - { - text: "color", - children: [{ text: n.canvasSettings?.color || "" }], - }, - ], - }, - ], - }), - ), - ); - return true; + const created = await Promise.all( + INITIAL_NODE_VALUES.map(async (n) => { + if (getPageUidByPageTitle(`discourse-graph/nodes/${n.text}`)) { + return false; + } + await createPage({ + title: `discourse-graph/nodes/${n.text}`, + uid: n.type, + tree: [ + { text: "Format", children: [{ text: n.format || "" }] }, + { text: "Shortcut", children: [{ text: n.shortcut || "" }] }, + { text: "Tag", children: [{ text: n.tag || "" }] }, + { text: "Graph Overview" }, + { + text: "Canvas", + children: [ + { + text: "color", + children: [{ text: n.canvasSettings?.color || "" }], + }, + ], + }, + ], + }); + return true; + }), + ); + return created.some(Boolean); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/roam/src/utils/initializeDiscourseNodes.ts` around lines 13 - 39, The current branch returns true even when no pages were created because getPageUidByPageTitle(...) short-circuits the ||; change the logic so each INITIAL_NODE_VALUES item checks existence first and returns a boolean indicating whether createPage(...) was actually called: for each n call await getPageUidByPageTitle(...); if it exists return false, otherwise await createPage(...) and return true; await Promise.all on that boolean array and return true only if any item is true (e.g., use Array.some) so initializeDiscourseNodes returns true only when at least one page was created.apps/roam/src/components/settings/utils/accessors.ts (1)
407-457:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't route legacy Export/Relations reads through
discourseConfigRef.tree.These readers used to fetch directly from the config page, but now they depend on
discourseConfigRef.tree. The hash-change handler inapps/roam/src/utils/initializeObserversAndListeners.tsalready documents that sidebar edits do not refresh that tree, so in legacy graphs Export and Relations can now stay stale until a manual refresh or reload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/roam/src/components/settings/utils/accessors.ts` around lines 407 - 457, The Export and Relations readers must not rely on the cached discourseConfigRef.tree (which isn't refreshed on sidebar edits); update the code so the Export branch and Relations branch use legacy readers that fetch the config page directly rather than reading discourseConfigRef.tree — specifically ensure getExportSettingsAndUids and getLegacyRelationsSetting (or the helper they call) read the config page/tree fresh from the source (or accept and use the passed-in tree that was loaded from the config page) instead of accessing discourseConfigRef.tree; make analogous changes inside any helper like getSuggestiveModeConfigAndUids if it currently routes through discourseConfigRef.tree so legacy settings stay current without a full reload.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/roam/src/components/settings/utils/accessors.ts`:
- Around line 407-457: The Export and Relations readers must not rely on the
cached discourseConfigRef.tree (which isn't refreshed on sidebar edits); update
the code so the Export branch and Relations branch use legacy readers that fetch
the config page directly rather than reading discourseConfigRef.tree —
specifically ensure getExportSettingsAndUids and getLegacyRelationsSetting (or
the helper they call) read the config page/tree fresh from the source (or accept
and use the passed-in tree that was loaded from the config page) instead of
accessing discourseConfigRef.tree; make analogous changes inside any helper like
getSuggestiveModeConfigAndUids if it currently routes through
discourseConfigRef.tree so legacy settings stay current without a full reload.
In `@apps/roam/src/utils/initializeDiscourseNodes.ts`:
- Around line 13-39: The current branch returns true even when no pages were
created because getPageUidByPageTitle(...) short-circuits the ||; change the
logic so each INITIAL_NODE_VALUES item checks existence first and returns a
boolean indicating whether createPage(...) was actually called: for each n call
await getPageUidByPageTitle(...); if it exists return false, otherwise await
createPage(...) and return true; await Promise.all on that boolean array and
return true only if any item is true (e.g., use Array.some) so
initializeDiscourseNodes returns true only when at least one page was created.
In `@apps/roam/src/utils/initializeObserversAndListeners.ts`:
- Around line 123-127: This observer currently calls getFeatureFlag("Duplicate
node alert enabled") which triggers a bulkReadSettings() again; instead read the
flag from the local settings snapshot that was loaded earlier in this function
(the same settings variable populated around line 113) and use that boolean to
gate renderPossibleDuplicates; replace the getFeatureFlag call with a check like
settings["Duplicate node alert enabled"] (coerce to boolean as needed) so you
avoid the extra settings pull while keeping renderDiscourseContext and
renderPossibleDuplicates unchanged.
- Around line 117-121: The cached result from findDiscourseNode is not being
invalidated when settings (snapshot) change, so a page previously cached as
false will never pick up updated node definitions; fix by adding cache
invalidation in findDiscourseNode (e.g., include snapshot in the cache key or
expose a clear/reset function) and then call that invalidation from
initializeObserversAndListeners before calling findDiscourseNode (the call
shown: findDiscourseNode({ uid, title, snapshot: settings })) whenever settings
change; alternatively, stop passing snapshot and instead ensure
findDiscourseNode's cache logic considers the settings snapshot so results are
recomputed on settings updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0473d32f-002a-4929-ab6c-0842ede7a04a
📒 Files selected for processing (6)
apps/roam/src/components/settings/utils/accessors.tsapps/roam/src/components/settings/utils/init.tsapps/roam/src/index.tsapps/roam/src/utils/getExportSettings.tsapps/roam/src/utils/initializeDiscourseNodes.tsapps/roam/src/utils/initializeObserversAndListeners.ts
mdroidian
approved these changes
May 6, 2026
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.
https://www.loom.com/share/1bb4e2d1d6254ee2b74f3cbf8c365afb
Results Macbook air 8gb ram 2020
Flag On: 602ms
Flag Off: 192ms
Summary by CodeRabbit