Roam: ENG-622: Node tag to node conversions #304
Conversation
… into eng-621-show-node-tag-options-in-inline-node-creation-menu
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…enu' of https://github.com/DiscourseGraphs/discourse-graph into eng-621-show-node-tag-options-in-inline-node-creation-menu
This reverts commit 3cdac68.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughA new modal dialog component for creating discourse nodes is introduced, along with a mechanism to trigger this dialog from a popup that appears when hovering over discourse node tags. Supporting utilities for rendering and removing the popup are implemented. Event listeners are updated to handle tag hover events and manage the lifecycle of the popup and dialog components. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DOM
participant nodeTagHoverListener
participant renderNodeTagPopup
participant CreateNodeDialog
User->>DOM: Hover over .rm-page-ref tag
DOM-->>nodeTagHoverListener: mouseover event
nodeTagHoverListener->>renderNodeTagPopup: Show popup near tag
User->>renderNodeTagPopup: Click "Create node" button
renderNodeTagPopup->>CreateNodeDialog: Open dialog with context (block text, tag, node types)
User->>CreateNodeDialog: Enter title, select type, click Create
CreateNodeDialog->>DOM: Create discourse node, update block, show toast
CreateNodeDialog->>renderNodeTagPopup: Remove popup on close
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 4
🧹 Nitpick comments (6)
apps/roam/src/utils/renderNodeTagPopup.tsx (2)
28-35: Add error handling for DOM operations.Consider adding error handling around DOM manipulation operations to prevent runtime errors if the element positioning fails or DOM access is restricted.
export const renderNodeTagPopup = ({ tagElement, onClick, label = "Create node", }: { tagElement: HTMLElement; onClick: () => void; label?: string; }) => { removeNodeTagPopup(); + try { const rect = tagElement.getBoundingClientRect(); currentPopup = document.createElement("div"); currentPopup.id = "discourse-node-tag-popup"; currentPopup.style.position = "absolute"; currentPopup.style.left = `${rect.left + window.scrollX}px`; currentPopup.style.top = `${rect.bottom + window.scrollY + 4}px`; currentPopup.className = "z-[9999] max-w-none font-inherit bg-white"; document.body.appendChild(currentPopup); + } catch (error) { + console.error("Failed to create popup:", error); + return; + }
33-33: Consider making z-index configurable or using CSS custom properties.The hardcoded z-index value of 9999 might conflict with other UI elements. Consider making it configurable or using a CSS custom property for better maintainability.
- currentPopup.className = "z-[9999] max-w-none font-inherit bg-white"; + currentPopup.className = "z-[var(--popup-z-index,9999)] max-w-none font-inherit bg-white";apps/roam/src/utils/initializeObserversAndListeners.ts (2)
297-343: Consider performance optimization for the hover listener.The current implementation processes every mouseover event on the document, which could impact performance during heavy DOM interaction. Consider adding debouncing or early filtering to reduce computational overhead.
+import { debounce } from "lodash"; // or implement a simple debounce - const nodeTagHoverListener = (e: Event) => { + const nodeTagHoverListener = debounce((e: Event) => { const target = e.target as HTMLElement | null; if (!target || !target.classList?.contains("rm-page-ref")) return; // ... rest of the function - }; + }, 100); // 100ms debounceAlternatively, consider event delegation with more specific targeting:
const nodeTagHoverListener = (e: Event) => { const target = e.target as HTMLElement | null; - if (!target || !target.classList?.contains("rm-page-ref")) return; + if (!target?.classList?.contains("rm-page-ref") || + target.closest('.discourse-node-tag-popup')) return; // Avoid self-triggering
325-326: Improve regex pattern for tag removal.The current regex
/#\w+/gonly matches word characters. Consider a more comprehensive pattern that handles various tag formats in Roam.- const cleanedBlockText = rawBlockText.replace(/#\w+/g, "").trim(); + const cleanedBlockText = rawBlockText.replace(/#[\w-]+/g, "").trim();Or for more comprehensive tag removal:
- const cleanedBlockText = rawBlockText.replace(/#\w+/g, "").trim(); + const cleanedBlockText = rawBlockText + .replace(/#[\w\-_.]+/g, "") // Handle hyphens, underscores, dots + .replace(/\[\[#?[^\]]+\]\]/g, "") // Handle page references with tags + .trim();apps/roam/src/components/CreateNodeDialog.tsx (2)
84-103: Simplify cursor positioning logic and add error handling.The cursor positioning logic is complex with a fallback approach. Consider simplifying and adding error handling for better maintainability.
const newCursorPosition = pageRef.length; - const windowId = - window.roamAlphaAPI.ui.getFocusedBlock?.()?.["window-id"] || "main"; - - if (window.roamAlphaAPI.ui.setBlockFocusAndSelection) { - window.roamAlphaAPI.ui.setBlockFocusAndSelection({ - location: { "block-uid": blockUid, "window-id": windowId }, - selection: { start: newCursorPosition }, - }); - } else { - setTimeout(() => { - const textareaElements = document.querySelectorAll("textarea"); - for (const el of textareaElements) { - if (getUids(el as HTMLTextAreaElement).blockUid === blockUid) { - (el as HTMLTextAreaElement).focus(); - (el as HTMLTextAreaElement).setSelectionRange( - newCursorPosition, - newCursorPosition, - ); - break; - } - } - }, 50); - } + + // Attempt cursor positioning with error handling + try { + const windowId = + window.roamAlphaAPI.ui.getFocusedBlock?.()?.["window-id"] || "main"; + + if (window.roamAlphaAPI.ui.setBlockFocusAndSelection) { + window.roamAlphaAPI.ui.setBlockFocusAndSelection({ + location: { "block-uid": blockUid, "window-id": windowId }, + selection: { start: newCursorPosition }, + }); + } else { + // Fallback approach + requestAnimationFrame(() => { + const textareaElements = document.querySelectorAll("textarea"); + for (const el of textareaElements) { + try { + if (getUids(el as HTMLTextAreaElement).blockUid === blockUid) { + (el as HTMLTextAreaElement).focus(); + (el as HTMLTextAreaElement).setSelectionRange( + newCursorPosition, + newCursorPosition, + ); + break; + } + } catch (uidError) { + // Skip this element if UID extraction fails + continue; + } + } + }); + } + } catch (focusError) { + console.warn("Failed to set cursor position:", focusError); + }
128-133: Add input validation and improve placeholder text.Consider adding validation for the title input and making the placeholder more dynamic based on the selected node type.
<InputGroup - placeholder={`This is a potential ${selectedType.text.toLowerCase()}`} + placeholder={selectedType.format + ? `Format: ${selectedType.format.replace(/{text}|{content}/gi, "your text here")}` + : `Enter ${selectedType.text.toLowerCase()} title`} value={title} onChange={(e) => setTitle(e.currentTarget.value)} inputRef={inputRef} + onKeyDown={(e) => { + if (e.key === "Enter" && title.trim() && !loading) { + onCreate(); + } + }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/roam/src/components/CreateNodeDialog.tsx(1 hunks)apps/roam/src/components/DiscourseNodeMenu.tsx(1 hunks)apps/roam/src/index.ts(2 hunks)apps/roam/src/utils/initializeObserversAndListeners.ts(4 hunks)apps/roam/src/utils/renderNodeTagPopup.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#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.
apps/roam/src/index.ts (5)
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/package.json : Prefer existing dependencies from package.json when adding or using dependencies in the Roam Research extension
Learnt from: maparent
PR: #220
File: apps/roam/src/utils/conceptConversion.ts:11-40
Timestamp: 2025-06-23T11:49:45.457Z
Learning: In the DiscourseGraphs/discourse-graph codebase, direct access to window.roamAlphaAPI is the established pattern throughout the codebase. The team prefers to maintain this pattern consistently rather than making piecemeal changes, and plans to address dependency injection as a global refactor when scheduled.
apps/roam/src/components/DiscourseNodeMenu.tsx (6)
Learnt from: sid597
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.
Learnt from: maparent
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.
Learnt from: maparent
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.
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-07-19T22:34:08.725Z
Learning: Applies to {apps/website,packages/ui}/**/*.{tsx,jsx} : When refactoring inline styles, use tailwind classes
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API
Learnt from: sid597
PR: #232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:15-16
Timestamp: 2025-06-22T10:40:21.679Z
Learning: In the getAllDiscourseNodesSince function in apps/roam/src/utils/getAllDiscourseNodesSince.ts, date validation is performed before the function is called, so additional date validation within the function is not needed.
apps/roam/src/utils/initializeObserversAndListeners.ts (6)
Learnt from: sid597
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.
Learnt from: maparent
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.
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API
Learnt from: maparent
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.
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API
apps/roam/src/utils/renderNodeTagPopup.tsx (3)
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API
apps/roam/src/components/CreateNodeDialog.tsx (7)
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Learnt from: sid597
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.
Learnt from: maparent
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.
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API
Learnt from: maparent
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.
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API
Learnt from: maparent
PR: #220
File: apps/roam/src/utils/conceptConversion.ts:11-40
Timestamp: 2025-06-23T11:49:45.457Z
Learning: In the DiscourseGraphs/discourse-graph codebase, direct access to window.roamAlphaAPI is the established pattern throughout the codebase. The team prefers to maintain this pattern consistently rather than making piecemeal changes, and plans to address dependency injection as a global refactor when scheduled.
🔇 Additional comments (2)
apps/roam/src/index.ts (1)
105-105: LGTM! Clean integration following established patterns.The nodeTagHoverListener integration follows the existing patterns perfectly - proper import, registration on document for mouseover events, and cleanup in the unload function.
Also applies to: 112-112, 151-151
apps/roam/src/components/DiscourseNodeMenu.tsx (1)
245-245: LGTM! Good stylistic consistency.The change from single to double quotes maintains consistency with string literals in the codebase.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Good start! There is lots to consider with something like CreateNodeDialog so there's quite a few comment/suggestions.
We generate a type of "CreateNodeDialog" in multiple places (Canvas/DiscourseNodeMenu/DG Popover Menu/command palette (forthcoming)), so there are lots of previous patterns to glean from.
Just FYI, we'll be combing these in the future ENG-555: Consolidate node creation implementations but your CreateNodeDialog looks like a pretty good start 👍
|
|
||
| const rect = tagElement.getBoundingClientRect(); | ||
|
|
||
| currentPopup = document.createElement("div"); |
There was a problem hiding this comment.
Let's use Blueprint's <Popover> here. It's a pattern we've used quite a bit in the repo, and it handles things like positioning based on screen/DOM, click/hover, esc, overflow, etc.
See NodeMenu for the render/<Popover> pattern.
There was a problem hiding this comment.
tried doing it but not able to get anywhere with it for quite some time, the popover needs a target to bind and creating the target then attaching to the popover leads to popover button rendered but not closing on mouse move from the button. keeping this part the same, need your help. Will give it another try tomorrow.
There was a problem hiding this comment.
This should be automatic. Have you looked over the Popover docs? https://blueprintjs.com/docs/versions/3/#core/components/popover
There was a problem hiding this comment.
You can see this exact pattern in Kanban as well: https://github.com/DiscourseGraphs/discourse-graph/blob/main/apps/roam/src/components/results-view/Kanban.tsx#L651
https://www.loom.com/share/c4b314168eee48108177c368f40b56c3
Attach the to the span of the hashtag as the target, as it already exists.
There was a problem hiding this comment.
Yeah the issue is that the span that we get is not a react element whereas the target requires (property) target?: string | JSX.Element | undefined so what we have to do is create a dummy element and attach it to dom and then render popover that element
There was a problem hiding this comment.
Let's see what you have at our next meeting and walk through it together.
…enu' into eng-622-node-tag-to-node-conversions
There was a problem hiding this comment.
The biggest change we need to do is use an observer instead of a listener. We talked about this at the Dev meeting and we walked through some example code in our 1:1.
Here is the example code from our meeting:
const attributeObserver = createHTMLObserver({
className: "rm-page-ref--tag",
tag: "SPAN",
callback: (s: HTMLSpanElement) => {
// if (discourseTagSet.has(tag)) {
renderAttributeButton(s);
// }
},
});I'll also attach the example button and styling code as it had a nice y offset and was centered.
export const renderAttributeButton = (parent: HTMLSpanElement) => {
if (parent.dataset.attributeButtonRendered === "true") return;
parent.dataset.attributeButtonRendered = "true";
const wrapper = document.createElement("span");
wrapper.style.position = "relative";
wrapper.style.display = "inline-block";
parent.parentNode?.insertBefore(wrapper, parent);
wrapper.appendChild(parent);
const reactRoot = document.createElement("span");
reactRoot.style.position = "absolute";
reactRoot.style.top = "0";
reactRoot.style.left = "0";
reactRoot.style.width = "100%";
reactRoot.style.height = "100%";
reactRoot.style.pointerEvents = "auto";
reactRoot.style.zIndex = "10";
wrapper.appendChild(reactRoot);
ReactDOM.render(<AttributeButtonPopover />, reactRoot);
};Popover modifiers
modifiers={{
offset: {
offset: "0, 10",
},
arrow: {
enabled: false,
},
}}
Popover target
<span
style={{
display: "block",
width: "100%",
height: "100%",
}}
/>
sid597
changed the base branch from
eng-621-show-node-tag-options-in-inline-node-creation-menu
to
main
| onChange={handleTagChange} | ||
| onBlur={handleTagBlur} | ||
| error={tagError} | ||
| placeholder={`#${node.text}`} |
There was a problem hiding this comment.
Actually I liked the # 😁.
I'm going to create a task to handle #, as we discussed today. We can re-add it once the use case is handled 😃
There was a problem hiding this comment.
Summary by CodeRabbit
New Features
Style