Skip to content

ENG-1790: Reduce repeated settings reads during page load#1069

Open
sid597 wants to merge 1 commit into
mainfrom
eng-1790-reduce-repeated-settings-reads-during-roam-page-load
Open

ENG-1790: Reduce repeated settings reads during page load#1069
sid597 wants to merge 1 commit into
mainfrom
eng-1790-reduce-repeated-settings-reads-during-roam-page-load

Conversation

@sid597
Copy link
Copy Markdown
Collaborator

@sid597 sid597 commented May 22, 2026

No video because this is a performance bug. I measured the performance using #1055

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

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

Current pr: pass settings snapshots:
getDiscourseNodes ~0.0-0.3ms per call
imageMenu node-read portion ~1.6ms / 7 calls

Summary by CodeRabbit

  • Refactor
    • Optimized settings handling throughout the application by implementing batched and cached settings snapshots, improving performance when rendering menus and popups.

Review Change Stack

@linear-code
Copy link
Copy Markdown

linear-code Bot commented May 22, 2026

ENG-1790

@supabase
Copy link
Copy Markdown

supabase Bot commented May 22, 2026

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
Copy link
Copy Markdown
Collaborator Author

sid597 commented May 22, 2026

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 22, 2026

✅ Actions performed

Full review triggered.

@sid597
Copy link
Copy Markdown
Collaborator Author

sid597 commented May 22, 2026

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3efb2e79cd

ℹ️ 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".

Comment thread apps/roam/src/utils/renderImageToolsMenu.tsx Outdated
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 22, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: d40fa32a-faa3-460b-996a-2894c56dfda8

📥 Commits

Reviewing files that changed from the base of the PR and between e837ef2 and 3efb2e7.

📒 Files selected for processing (6)
  • apps/roam/src/components/DiscourseNodeMenu.tsx
  • apps/roam/src/utils/getExportTypes.ts
  • apps/roam/src/utils/initializeObserversAndListeners.ts
  • apps/roam/src/utils/pageRefObserverHandlers.ts
  • apps/roam/src/utils/renderImageToolsMenu.tsx
  • apps/roam/src/utils/renderTextSelectionPopup.tsx

📝 Walkthrough

Walkthrough

This pull request threads SettingsSnapshot parameters through component contracts, render entry points, and observer batching infrastructure. Components now accept optional settingsSnapshot props; render functions accept settings getters or snapshots; and observer handlers introduce batching to capture settings once per microtask rather than repeatedly during render.

Changes

Settings snapshot threading

Layer / File(s) Summary
NodeMenu component contract updates
apps/roam/src/components/DiscourseNodeMenu.tsx
NodeMenu and TextSelectionNodeMenu now accept optional settingsSnapshot?: SettingsSnapshot prop; discourse node queries now call getDiscourseNodes(undefined, settingsSnapshot) to use the provided snapshot instead of default settings access.
Image tools menu render integration
apps/roam/src/utils/renderImageToolsMenu.tsx
ImageToolsMenu component accepts and wires settingsSnapshot into NodeMenu; renderImageToolsMenu function signature extends to accept getSettingsSnapshot: () => SettingsSnapshot getter, extracts the snapshot, and passes it to the component.
Text selection popup render integration
apps/roam/src/utils/renderTextSelectionPopup.tsx
renderTextSelectionPopup function signature extends to accept settingsSnapshot: SettingsSnapshot parameter and forwards it to TextSelectionNodeMenu component props.
Observer batching infrastructure
apps/roam/src/utils/initializeObserversAndListeners.ts, apps/roam/src/utils/pageRefObserverHandlers.ts
Introduce getImageMenuSettingsForBatch() in observers to lazily capture settings once per microtask for image menu rendering; add getBatchSettingsSnapshot() and update getBatchDiscourseNodes() in page-ref handlers to batch and memoize settings snapshots alongside discourse nodes.
Export types data computation
apps/roam/src/utils/getExportTypes.ts
getExportTypes function now accepts optional settingsSnapshot?: SettingsSnapshot and derives settings from it, passing the snapshot into getDiscourseRelations and getDiscourseNodes calls instead of no-argument invocations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • DiscourseGraphs/discourse-graph#218: Introduces TextSelectionNodeMenu and NodeMenu trigger prop; this PR extends the same path by threading settingsSnapshot through those components.
  • DiscourseGraphs/discourse-graph#298: Also modifies DiscourseNodeMenu, TextSelectionNodeMenu, and renderTextSelectionPopup, adding isShift/isTextSelected props for menu behavior alongside this PR's settingsSnapshot threading.
  • DiscourseGraphs/discourse-graph#36: Modifies pageRefObserverHandlers.ts restructuring; overlaps with this PR's settings batching infrastructure in the same file.
🚥 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 accurately describes the main objective of the changeset—reducing repeated settings reads during page load through batched and cached settings snapshots.
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

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

@sid597 sid597 force-pushed the eng-1790-reduce-repeated-settings-reads-during-roam-page-load branch from 3efb2e7 to 60531f8 Compare May 25, 2026 06:16
@sid597 sid597 requested a review from mdroidian May 25, 2026 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant