ENG-1095 Optimize Discourse Node page observer check #591
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
sid597
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe pull request refactors linked references rendering in the Roam app by removing an observer-based approach and replacing it with direct function invocation. The rendering function is renamed, adds explicit UID parameter, and removes async behavior. Internal UID resolution logic is eliminated. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks✅ Passed checks (3 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/roam/src/utils/initializeObserversAndListeners.ts(2 hunks)apps/roam/src/utils/renderLinkedReferenceAdditions.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Prefertypeoverinterfacein TypeScript
Use explicit return types for functions
Avoidanytypes when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Function names should describe their purpose clearly
Prefer early returns over nested conditionals for better readability
Files:
apps/roam/src/utils/initializeObserversAndListeners.tsapps/roam/src/utils/renderLinkedReferenceAdditions.ts
apps/roam/**/*.{js,ts,tsx,jsx,json}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Prefer existing dependencies from package.json when working on the Roam Research extension
Files:
apps/roam/src/utils/initializeObserversAndListeners.tsapps/roam/src/utils/renderLinkedReferenceAdditions.ts
apps/roam/**/*.{ts,tsx,jsx,js,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Files:
apps/roam/src/utils/initializeObserversAndListeners.tsapps/roam/src/utils/renderLinkedReferenceAdditions.ts
apps/roam/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
apps/roam/**/*.{ts,tsx,js,jsx}: Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Use Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when implementing extension functionality
Files:
apps/roam/src/utils/initializeObserversAndListeners.tsapps/roam/src/utils/renderLinkedReferenceAdditions.ts
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/utils/initializeObserversAndListeners.tsapps/roam/src/utils/renderLinkedReferenceAdditions.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 534
File: apps/roam/src/utils/createReifiedBlock.ts:40-48
Timestamp: 2025-11-05T21:57:14.909Z
Learning: In the discourse-graph repository, the function `getPageUidByPageTitle` from `roamjs-components/queries/getPageUidByPageTitle` is a synchronous function that returns a string directly (the page UID or an empty string if not found), not a Promise. It should be called without `await`.
📚 Learning: 2025-06-25T22:56:17.522Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-06-25T22:56:17.522Z
Learning: In the Roam discourse-graph system, the existence of the configuration page (identified by DISCOURSE_CONFIG_PAGE_TITLE) and its corresponding UID is a system invariant. The code can safely assume this page will always exist, so defensive null checks are not needed when using `getPageUidByPageTitle(DISCOURSE_CONFIG_PAGE_TITLE)`.
Applied to files:
apps/roam/src/utils/initializeObserversAndListeners.ts
📚 Learning: 2025-11-05T21:57:14.909Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 534
File: apps/roam/src/utils/createReifiedBlock.ts:40-48
Timestamp: 2025-11-05T21:57:14.909Z
Learning: In the discourse-graph repository, the function `getPageUidByPageTitle` from `roamjs-components/queries/getPageUidByPageTitle` is a synchronous function that returns a string directly (the page UID or an empty string if not found), not a Promise. It should be called without `await`.
Applied to files:
apps/roam/src/utils/initializeObserversAndListeners.tsapps/roam/src/utils/renderLinkedReferenceAdditions.ts
📚 Learning: 2025-06-22T10:40:52.752Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:18-31
Timestamp: 2025-06-22T10:40:52.752Z
Learning: In apps/roam/src/utils/getAllDiscourseNodesSince.ts, the user confirmed that querying for `?title` with `:node/title` and mapping it to the `text` field in the DiscourseGraphContent type is the correct implementation for retrieving discourse node content from Roam Research, despite it appearing to query page titles rather than block text content.
Applied to files:
apps/roam/src/utils/initializeObserversAndListeners.ts
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `type` serves as the unique identifier field, not a type classification field. The interface has no `uid` or `id` field, making `node.type` the correct field to use for UID-related operations.
Applied to files:
apps/roam/src/utils/renderLinkedReferenceAdditions.ts
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.
Applied to files:
apps/roam/src/utils/renderLinkedReferenceAdditions.ts
🧬 Code graph analysis (1)
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
apps/roam/src/utils/renderLinkedReferenceAdditions.ts (1)
renderDiscourseContextAndCanvasReferences(7-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (2)
apps/roam/src/utils/renderLinkedReferenceAdditions.ts (2)
7-11: LGTM: Function signature improvements.The refactored signature is well-designed:
- More descriptive name clearly indicates dual rendering responsibility
- Explicit
uidparameter eliminates DOM-based resolution- Synchronous return type matches the actual behavior
- Follows coding guidelines for explicit return types and clear function naming
18-39: Verify DOM insertion order matches intended rendering sequence.Both
divelements are inserted using the sameinsertBeforereference. The execution order results in:
- Line 21:
pinserted beforeinsertBefore→pbecomes the new first child- Line 32:
canvasPinserted beforeinsertBefore→canvasPbecomes the new first childThis means
CanvasReferenceswill appear aboveDiscourseContextin the DOM, reversing the code's visual sequence.Verify the intended visual order and apply this diff if
DiscourseContextshould appear first:const insertBefore = parent.firstElementChild; const p = document.createElement("div"); parent.insertBefore(p, insertBefore); renderWithUnmount( createElement(DiscourseContext, { uid, results: [], }), p, onloadArgs, ); const canvasP = document.createElement("div"); - parent.insertBefore(canvasP, insertBefore); + parent.insertBefore(canvasP, insertBefore?.nextSibling || null); renderWithUnmount( createElement(CanvasReferences, { uid, }), canvasP, onloadArgs, );Alternatively, if the current order is correct, update the code comments or variable names to reflect that
CanvasReferencesappears first.
| const linkedReferencesDiv = document.querySelector( | ||
| ".rm-reference-main", | ||
| ) as HTMLDivElement; | ||
| if (linkedReferencesDiv) { | ||
| renderDiscourseContextAndCanvasReferences( | ||
| linkedReferencesDiv, | ||
| uid, | ||
| onloadArgs, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the file in question
find . -name "initializeObserversAndListeners.ts" -type fRepository: DiscourseGraphs/discourse-graph
Length of output: 131
🏁 Script executed:
# Get the file size first
wc -l apps/roam/src/utils/initializeObserversAndListeners.tsRepository: DiscourseGraphs/discourse-graph
Length of output: 133
🏁 Script executed:
# Read the entire file to understand context
cat -n apps/roam/src/utils/initializeObserversAndListeners.tsRepository: DiscourseGraphs/discourse-graph
Length of output: 15852
🏁 Script executed:
# Also check for the linkedReferencesObserver and pageTitleObserver patterns
rg -n "linkedReferencesObserver|pageTitleObserver|createHTMLObserver" apps/roam/src/utils/initializeObserversAndListeners.ts -A 3 -B 1Repository: DiscourseGraphs/discourse-graph
Length of output: 1012
🏁 Script executed:
# Check how createHTMLObserver is used elsewhere in the codebase
rg -l "createHTMLObserver" apps/roam/src/ --type ts --type tsxRepository: DiscourseGraphs/discourse-graph
Length of output: 102
🏁 Script executed:
# Look at renderDiscourseContextAndCanvasReferences to understand timing requirements
rg -n "renderDiscourseContextAndCanvasReferences" apps/roam/src/ --type ts --type tsx -A 2Repository: DiscourseGraphs/discourse-graph
Length of output: 102
🏁 Script executed:
# Check renderDiscourseContextAndCanvasReferences function
rg -n "renderDiscourseContextAndCanvasReferences" apps/roam/src/ -A 10Repository: DiscourseGraphs/discourse-graph
Length of output: 3125
🏁 Script executed:
# Search for linkedReferences references to understand timing
rg -n "rm-reference-main|linkedReferences" apps/roam/src/ -B 2 -A 2Repository: DiscourseGraphs/discourse-graph
Length of output: 1017
🏁 Script executed:
# Check if there are similar deferred rendering patterns in the codebase
rg -n "requestAnimationFrame|setTimeout" apps/roam/src/utils/ -B 2 -A 2Repository: DiscourseGraphs/discourse-graph
Length of output: 7673
🏁 Script executed:
# Look for other patterns where observers wait for elements
rg -n "createHTMLObserver" apps/roam/src/ -A 8 | head -80Repository: DiscourseGraphs/discourse-graph
Length of output: 5962
🏁 Script executed:
# Check createDiscourseNode.ts for the setTimeout pattern with DOM elements
sed -n '36,50p' apps/roam/src/utils/createDiscourseNode.tsRepository: DiscourseGraphs/discourse-graph
Length of output: 632
🏁 Script executed:
# Check if there's a pattern of retrying querySelector in the codebase
rg -n "querySelector.*if|if.*querySelector" apps/roam/src/ -B 2 -A 5 | head -60Repository: DiscourseGraphs/discourse-graph
Length of output: 1427
🏁 Script executed:
# Check renderLinkedReferenceAdditions.ts in full to understand all requirements
cat -n apps/roam/src/utils/renderLinkedReferenceAdditions.ts | head -50Repository: DiscourseGraphs/discourse-graph
Length of output: 1436
Potential race condition: .rm-reference-main may not exist when queried.
The direct querySelector assumes .rm-reference-main is already in the DOM when pageTitleObserver fires. However, the page title element (h1.rm-title-display) and the linked references container may render at different times in Roam Research.
Impact: If the linked references section hasn't rendered yet, linkedReferencesDiv will be null, and the rendering silently fails—users won't see DiscourseContext or CanvasReferences.
Previous behavior: The observer-based approach waited for the element to appear in the DOM.
Suggested solutions:
Option 1: Add a fallback observer for late-rendering cases:
if (isDiscourseNode(uid)) {
renderPossibleDuplicates(h1, title);
const linkedReferencesDiv = document.querySelector(
".rm-reference-main",
) as HTMLDivElement;
if (linkedReferencesDiv) {
renderDiscourseContextAndCanvasReferences(
linkedReferencesDiv,
uid,
onloadArgs,
);
+ } else {
+ // Element not ready yet; defer until it appears
+ const tempObserver = createHTMLObserver({
+ className: "rm-reference-main",
+ callback: (el) => {
+ renderDiscourseContextAndCanvasReferences(
+ el as HTMLDivElement,
+ uid,
+ onloadArgs,
+ );
+ tempObserver?.disconnect();
+ },
+ });
}
}Option 2: Use requestAnimationFrame or setTimeout to defer the query:
if (isDiscourseNode(uid)) {
renderPossibleDuplicates(h1, title);
- const linkedReferencesDiv = document.querySelector(
- ".rm-reference-main",
- ) as HTMLDivElement;
- if (linkedReferencesDiv) {
- renderDiscourseContextAndCanvasReferences(
- linkedReferencesDiv,
- uid,
- onloadArgs,
- );
- }
+ requestAnimationFrame(() => {
+ const linkedReferencesDiv = document.querySelector(
+ ".rm-reference-main",
+ ) as HTMLDivElement;
+ if (linkedReferencesDiv) {
+ renderDiscourseContextAndCanvasReferences(
+ linkedReferencesDiv,
+ uid,
+ onloadArgs,
+ );
+ }
+ });
}Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.