Roam: Eng-752 node formalization menu doesnt pop up in query results forr #417
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. |
📝 WalkthroughWalkthroughAdds a tag set cache from discourse nodes, a MutationObserver to detect and format bare #tags inside table cells, and integrates it into initialization. Extends popup rendering to handle tags within table-embedded contexts via a new internal React popup component; maintains non-embed popup flow with portal-based rendering. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant DOM as DOM (Roam Page)
participant IO as initObservers
participant TTO as tableTagObserver
participant RNP as renderNodeTagPopupButton
participant TEP as TableEmbedPopup
participant POP as Popover (non-embed)
participant API as extensionAPI
Note over IO,TTO: Initialization
IO->>TTO: Observe TD.relative with data-cell-content
IO->>IO: Build discourseTagSet from discourseNodes
Note over TTO,DOM: Detection & Formatting
DOM-->>TTO: Mutation (cell content updated)
TTO->>TTO: Find bare #tags in inner containers
TTO->>DOM: Replace with span.rm-page-ref[data-tag] if in discourseTagSet
TTO-->>RNP: For new/updated tag elements, schedule popup render
alt Tag inside table embed
RNP->>TEP: Mount TableEmbedPopup via portal
U->>TEP: Hover tag
TEP->>TEP: Show "Create" control
U->>TEP: Click Create
TEP->>API: renderCreateNodeDialog(sourceBlockUid, initialTitle,…)
API-->>TEP: Dialog handled
TEP->>TEP: Close popup
else Non-embed context
RNP->>POP: Render Popover via portal
U->>POP: Click Create
POP->>API: renderCreateNodeDialog(...)
end
Note over RNP,TEP: Event listeners cleaned on unmount
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
apps/roam/src/utils/renderNodeTagPopup.tsx (5)
10-16: Type safety: use DiscourseNode instead of any for matchedNodeAvoids runtime slips and improves autocomplete.
-const TableEmbedPopup: React.FC<{ +const TableEmbedPopup: React.FC<{ parent: HTMLElement; - matchedNode: any; + matchedNode: DiscourseNode; extensionAPI: OnloadArgs["extensionAPI"]; blockUid?: string; cleanedBlockText: string; }>
27-33: Reposition popup on scroll/resize and restore cursor style on cleanupCurrent position freezes after mouseenter; also cursor style is never restored.
Minimal patch to restore cursor:
useEffect(() => { + const prevCursor = parent.style.cursor; parent.addEventListener("mouseenter", handleMouseEnter); parent.addEventListener("mouseleave", handleMouseLeave); parent.style.cursor = "pointer"; return () => { parent.removeEventListener("mouseenter", handleMouseEnter); parent.removeEventListener("mouseleave", handleMouseLeave); + parent.style.cursor = prevCursor; if (timeoutRef.current) { clearTimeout(timeoutRef.current); } }; }, []);Add another effect to track scroll/resize while visible:
useEffect(() => { if (!showPopup) return; const onReposition = () => { const rect = parent.getBoundingClientRect(); setPopupPosition({ x: rect.left + rect.width / 2, y: rect.top - 8 }); }; window.addEventListener("scroll", onReposition, true); window.addEventListener("resize", onReposition); return () => { window.removeEventListener("scroll", onReposition, true); window.removeEventListener("resize", onReposition); }; }, [showPopup, parent]);Also applies to: 58-71
136-138: Make cleanedBlockText removal whitespace-safeAvoid leftover double spaces when stripping the tag text.
-const cleanedBlockText = rawBlockText.replace(textContent, "").trim(); +const cleanedBlockText = rawBlockText + .replace(textContent, "") + .replace(/\s{2,}/g, " ") + .trim();
171-213: Popover modifiers may be Popper v1-style; validate against your Blueprint version'{ offset: { offset: "0, 10" } }' is Popper v1 style. Blueprint v4+ (Popper v2) expects an array/object with 'options.offset: [skidding, distance]'.
If on Blueprint v4+:
- modifiers={{ - offset: { - offset: "0, 10", - }, - arrow: { - enabled: false, - }, - }} + modifiers={[ + { name: "offset", options: { offset: [0, 10] } }, + { name: "arrow", enabled: false }, + ]}
155-170: Hover target overlay can intercept interactionsThe absolute overlay may capture clicks/selections on the tag. Verify this doesn’t regress inline editing or selection; if it does, shrink the target to a small icon or only enable pointer-events when hovered.
apps/roam/src/utils/initializeObserversAndListeners.ts (3)
226-241: Prefer rAF over arbitrary 50ms timeoutSchedules after DOM mutations without magic delays.
- setTimeout(() => { + requestAnimationFrame(() => { const newTags = innerSpan.querySelectorAll( '.rm-page-ref--tag:not([data-attribute-button-rendered="true"])', ); newTags.forEach((tag) => { if (tag instanceof HTMLSpanElement) { renderNodeTagPopupButton( tag, discourseNodes, onloadArgs.extensionAPI, ); } }); - }, 50); + });
210-218: Case-insensitive alreadyFormatted checkattribute selectors are case-sensitive; avoid duplicate wrapping by normalizing data-tag to lowercase when creating spans and comparing in lowercase.
- const alreadyFormatted = innerSpan.querySelector( - `.rm-page-ref--tag[data-tag="${tag}"]`, - ); + const alreadyFormatted = Array.from( + innerSpan.querySelectorAll(".rm-page-ref--tag") + ).some((el) => el.getAttribute("data-tag")?.toLowerCase() === tag.toLowerCase());And when creating:
- return `<span class="rm-page-ref rm-page-ref--tag" data-tag="${tagName}">${match}</span>`; + return `<span class="rm-page-ref rm-page-ref--tag" data-tag="${tagName.toLowerCase()}">${match}</span>`;
209-222: Preserve multi-# intent when formatting (aligns with prior learning)For inputs like "##foo", ensure you don’t drop or separate the first “#”. Consider matching /(#+)([\w-]+)/ and wrapping the whole match while storing data-tag as the second group lowercased.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/roam/src/utils/initializeObserversAndListeners.ts(4 hunks)apps/roam/src/utils/renderNodeTagPopup.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-25T15:53:21.799Z
Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#372
File: apps/roam/src/components/DiscourseNodeMenu.tsx:116-116
Timestamp: 2025-08-25T15:53:21.799Z
Learning: In apps/roam/src/components/DiscourseNodeMenu.tsx, when handling tag insertion, multiple leading hashtags (like ##foo) should be preserved as they represent user intent, not normalized to a single hashtag. The current regex /^#/ is correct as it only removes one leading # before adding one back, maintaining any additional hashtags the user intended.
Applied to files:
apps/roam/src/utils/initializeObserversAndListeners.ts
🪛 ast-grep (0.38.6)
apps/roam/src/utils/initializeObserversAndListeners.ts
[warning] 223-223: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: innerSpan.innerHTML = newHtml
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 194-194: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(#${tag}(?![\\w-]), "i")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 223-223: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: innerSpan.innerHTML = newHtml
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
⏰ 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). (9)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (5)
apps/roam/src/utils/renderNodeTagPopup.tsx (2)
139-154: Retain ReactDOM.render for React 17 in apps/roam
apps/roam is locked to React 17, so createRoot isn’t available—keep using ReactDOM.render.Likely an incorrect or invalid review comment.
96-104: No changes needed:matchedNode.typeis the UID field used by CreateNodeDialogapps/roam/src/utils/initializeObserversAndListeners.ts (3)
67-69: Good: cache a lowercase tag set for O(1) membership checksThis simplifies lookups elsewhere.
157-172: Re-rendering existing tags: LGTMRemoving the render guard attribute and re-invoking the popup renderer is correct.
458-459: Observer registration: LGTMNew tableTagObserver is properly added to the observers list.
sid597
changed the base branch from
eng-737-use-node-color-to-style-node-tags
to
graphite-base/417
sid597
force-pushed
the
eng-752-node-formalization-menu-doesnt-pop-up-in-query-results-forr
branch
from
8b34224 to
8b7b883
Compare
sid597
changed the base branch from
graphite-base/417
to
eng-693-handle-in-tag
sid597
force-pushed
the
eng-752-node-formalization-menu-doesnt-pop-up-in-query-results-forr
branch
from
8b7b883 to
d72b1c2
Compare
sid597
force-pushed
the
eng-752-node-formalization-menu-doesnt-pop-up-in-query-results-forr
branch
from
d72b1c2 to
3109273
Compare
sid597
force-pushed
the
eng-693-handle-in-tag
branch
from
dfcb44b to
94c5a08
Compare
sid597
deleted the
eng-752-node-formalization-menu-doesnt-pop-up-in-query-results-forr
branch
https://www.loom.com/share/4cfe2885a7154d47bdfbf1af1296f509?sid=02e3e20e-1d9b-4c2f-8a41-da4984a64528
Summary by CodeRabbit