ENG-1785: Cache getAllDiscourseNodes

[sid597]

No video because this is a performance bug. I measured the performance using #1055 and we have following improvements before the fix:

• Before:
  getDiscourseNodes ~572ms / 10 calls
  imageMenu ~366ms / 6 calls

• After cache getAllDiscourseNodes:
  getDiscourseNodes ~80-129ms / 11-12 calls
  imageMenu ~68-122ms / 7 calls

Summary by CodeRabbit

• Bug Fixes

  • Improved accuracy and responsiveness of discourse node configuration changes by implementing proper cache invalidation mechanisms, ensuring updates are immediately reflected without stale data.

• Chores

  • Enhanced internal caching strategy for discourse node operations to optimize performance.

[linear-code]

ENG-1785

[supabase]

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[sid597]

@coderabbitai full review

[sid597]

@codex review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR implements a versioned cache invalidation system for discourse node types. It introduces a new cache version counter that increments when mutations occur, allowing internal caches in findDiscourseNode and getAllDiscourseNodes to detect staleness and refresh. Cache invalidation is triggered from config panel mutations, feature flag changes, node setting updates, and legacy migrations.

Changes

Discourse node type cache versioning and invalidation

Layer / File(s)	Summary
Cache version infrastructure apps/roam/src/utils/discourseNodeTypeCache.ts	New module exports invalidateDiscourseNodeTypeCaches() to increment a version counter and getDiscourseNodeTypeCacheVersion() to read the current version.
findDiscourseNode cache version synchronization apps/roam/src/utils/findDiscourseNode.ts	Changes internal discourseNodeTypeCache from immutable to mutable and adds version tracking; clears the cache when the version increments.
getAllDiscourseNodes caching with invalidation triggers apps/roam/src/components/settings/utils/accessors.ts	Adds a module-level versioned cache for getAllDiscourseNodes results with early-return logic when the version matches; invalidates caches when the "Use new settings store" feature flag changes or when node settings are updated; populates the cache after node computation.
Config panel cache invalidation on mutations apps/roam/src/components/settings/DiscourseNodeConfigPanel.tsx	Calls invalidateDiscourseNodeTypeCaches() after node type deletion and creation to refresh derived caches.
Migration callback integration for cache invalidation apps/roam/src/components/settings/utils/migrateLegacyToBlockProps.ts	Extends migrateSection to accept an optional onWrite callback; wires invalidateDiscourseNodeTypeCaches through the discourse-node migration path so caches invalidate when legacy block props are migrated.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• DiscourseGraphs/discourse-graph#856: Both PRs modify the legacy block-props migration-on-parse-failure flow inside getAllDiscourseNodes; this PR wraps that path with a versioned cache.
• DiscourseGraphs/discourse-graph#853: Both PRs modify discourse-node retrieval logic in getAllDiscourseNodes, with the main PR adding versioned caching and invalidation.
• DiscourseGraphs/discourse-graph#805: Both PRs modify the node-type create and delete flows in DiscourseNodeConfigPanel.tsx at the same code points.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'ENG-1785: Cache getAllDiscourseNodes' directly and clearly summarizes the main objective of the pull request, which is to implement caching for the getAllDiscourseNodes function as evidenced by all file changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

apps/roam/src/components/settings/utils/accessors.ts (1)

1149-1214: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Invalidate cache version after migration writes inside getAllDiscourseNodes.

Line 1194 writes migrated block props, but the cache version is not bumped. This can leave findDiscourseNode serving stale cached values for the same node UID until another unrelated invalidation happens.

Suggested fix

       if (retryResult.success) {
         setBlockProps(pageUid, retryResult.data, false);
+        invalidateDiscourseNodeTypeCaches();
         nodes.push(
           toDiscourseNode({
             ...retryResult.data,
             type: pageUid,
             text: nodeText,

🤖 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 1149 -
1214, In getAllDiscourseNodes, after you call setBlockProps(pageUid,
retryResult.data, false) as part of the legacy migration, invalidate the
in-memory cache (allDiscourseNodesCache) or bump its version so subsequent calls
(and findDiscourseNode) won't return stale data; use
getDiscourseNodeTypeCacheVersion() to compute the current version or simply set
allDiscourseNodesCache = undefined (or { version:
getDiscourseNodeTypeCacheVersion(), nodes: [] }) immediately after the write
inside the migrate branch so the function rebuilds the cache with the migrated
props on the next access.

🤖 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 1149-1214: In getAllDiscourseNodes, after you call
setBlockProps(pageUid, retryResult.data, false) as part of the legacy migration,
invalidate the in-memory cache (allDiscourseNodesCache) or bump its version so
subsequent calls (and findDiscourseNode) won't return stale data; use
getDiscourseNodeTypeCacheVersion() to compute the current version or simply set
allDiscourseNodesCache = undefined (or { version:
getDiscourseNodeTypeCacheVersion(), nodes: [] }) immediately after the write
inside the migrate branch so the function rebuilds the cache with the migrated
props on the next access.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 2930771e-0bd4-475b-a4fc-addaefb729e6

📥 Commits

Reviewing files that changed from the base of the PR and between e837ef2 and 8fa8a02.

📒 Files selected for processing (5)

• apps/roam/src/components/settings/DiscourseNodeConfigPanel.tsx
• apps/roam/src/components/settings/utils/accessors.ts
• apps/roam/src/components/settings/utils/migrateLegacyToBlockProps.ts
• apps/roam/src/utils/discourseNodeTypeCache.ts
• apps/roam/src/utils/findDiscourseNode.ts

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. What shall we delve into next?

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[mdroidian]

is this string a const somewhere? If not, it should be. Someone could easily mistype, eg: setting instead of settings