[ENG-979] Consolidate create node flow #546
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughIntroduces ModifyNodeDialog component for creating and editing discourse nodes with fuzzy-search-enabled content selection, replacing CreateNodeDialog. Updates multiple integration points across the codebase to use the new dialog and adds supporting reusable FuzzySelectInput component with fuzzy search and locking capabilities. Changes
Sequence DiagramsequenceDiagram
participant User
participant ModifyNodeDialog
participant FuzzySelectInput
participant RoamAPI as Roam API
participant Canvas
User->>ModifyNodeDialog: Open dialog (create mode)
rect rgb(240, 248, 255)
Note over ModifyNodeDialog,RoamAPI: Load node types & options
ModifyNodeDialog->>RoamAPI: Fetch available discourse node types
RoamAPI-->>ModifyNodeDialog: Node type list
ModifyNodeDialog->>RoamAPI: Fetch content options
RoamAPI-->>ModifyNodeDialog: Content option list
end
ModifyNodeDialog->>FuzzySelectInput: Pass options for content
User->>FuzzySelectInput: Type to filter (fuzzy search)
FuzzySelectInput-->>User: Filtered options
User->>FuzzySelectInput: Select an item
FuzzySelectInput->>ModifyNodeDialog: Update value + emit onLockedChange
rect rgb(240, 255, 240)
Note over ModifyNodeDialog,RoamAPI: Create new node
User->>ModifyNodeDialog: Click Confirm
ModifyNodeDialog->>RoamAPI: Create discourse node / Update block
RoamAPI-->>ModifyNodeDialog: Success
end
ModifyNodeDialog->>ModifyNodeDialog: onSuccess handler
ModifyNodeDialog->>Canvas: Update shape (title, uid, image, size)
ModifyNodeDialog->>User: Show success toast + close
Canvas-->>User: Node updated/created on canvas
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50–70 minutes Areas requiring extra attention:
Possibly related PRs
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: 3
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/utils/formatUtils.ts (1)
64-91: Remove redundant DOM manipulation code.The
setupButtonControlfunction manipulates the dialog's DOM to enable/disable the submit button based on input. However,ModifyNodeDialogalready handles this internally (line 481 disables the button when!contentText.trim()). This code is redundant and could cause issues if the dialog's DOM structure changes.Apply this diff to remove the obsolete code:
}); - const setupButtonControl = () => { - const dialogs = document.querySelectorAll(".bp3-dialog"); - const dialog = dialogs[dialogs.length - 1] as HTMLElement; - - const input = dialog.querySelector( - 'input[type="text"].bp3-input', - ) as HTMLInputElement; - const submitBtn = dialog.querySelector( - ".bp3-dialog-footer .bp3-button.bp3-intent-primary", - ) as HTMLButtonElement; - - const updateButtonState = () => { - const currentValue = input.value; - const hasValue = currentValue.trim().length > 0; - const shouldBeDisabled = !hasValue; - submitBtn.disabled = shouldBeDisabled; - }; - - updateButtonState(); - - const listenerKey = "_discourseNodeListenerAttached"; - if (!(input as any)[listenerKey]) { - input.addEventListener("input", updateButtonState); - (input as any)[listenerKey] = true; - } - }; - requestAnimationFrame(setupButtonControl); });
🧹 Nitpick comments (2)
apps/roam/src/components/LockableAutocompleteInput.tsx (1)
34-37: Remove debug logging from production code.The
console.logcalls will fire on every selection and clutter the console. Please drop these before merging.Apply this cleanup:
- console.log("locking value", q); - console.log("initialUid", initialUid); - console.log("q.uid", q.uid); - console.log("mode", mode);apps/roam/src/utils/renderNodeTagPopup.tsx (1)
45-48: Track the TODO for initialReferencedNode implementation.The placeholder implementation returns empty values. Ensure this TODO is tracked in your issue tracker to implement proper referenced node detection from the block context.
Would you like me to open a new issue to track this implementation task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/roam/package.json(2 hunks)apps/roam/src/components/CreateNodeDialog.tsx(1 hunks)apps/roam/src/components/FuzzySelectInput.tsx(1 hunks)apps/roam/src/components/LockableAutocompleteInput.tsx(1 hunks)apps/roam/src/components/ModifyNodeDialog.tsx(1 hunks)apps/roam/src/components/canvas/DiscourseNodeUtil.tsx(3 hunks)apps/roam/src/utils/formatUtils.ts(2 hunks)apps/roam/src/utils/renderNodeTagPopup.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Prefertypeoverinterface
Use explicit return types for functions
Avoidanytypes when possible
Files:
apps/roam/src/components/FuzzySelectInput.tsxapps/roam/src/components/CreateNodeDialog.tsxapps/roam/src/components/LockableAutocompleteInput.tsxapps/roam/src/utils/formatUtils.tsapps/roam/src/components/ModifyNodeDialog.tsxapps/roam/src/utils/renderNodeTagPopup.tsxapps/roam/src/components/canvas/DiscourseNodeUtil.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx,js,jsx}: Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use Prettier with the project's configuration
Maintain consistent naming conventions: PascalCase for components and types
Maintain consistent naming conventions: camelCase for variables and functions
Maintain consistent naming conventions: UPPERCASE for constants
Files:
apps/roam/src/components/FuzzySelectInput.tsxapps/roam/src/components/CreateNodeDialog.tsxapps/roam/src/components/LockableAutocompleteInput.tsxapps/roam/src/utils/formatUtils.tsapps/roam/src/components/ModifyNodeDialog.tsxapps/roam/src/utils/renderNodeTagPopup.tsxapps/roam/src/components/canvas/DiscourseNodeUtil.tsx
apps/roam/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
apps/roam/**/*.{js,jsx,ts,tsx}: Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API
Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API
Files:
apps/roam/src/components/FuzzySelectInput.tsxapps/roam/src/components/CreateNodeDialog.tsxapps/roam/src/components/LockableAutocompleteInput.tsxapps/roam/src/utils/formatUtils.tsapps/roam/src/components/ModifyNodeDialog.tsxapps/roam/src/utils/renderNodeTagPopup.tsxapps/roam/src/components/canvas/DiscourseNodeUtil.tsx
apps/roam/package.json
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Prefer existing dependencies from package.json when adding or using dependencies in the Roam Research extension
Files:
apps/roam/package.json
🧠 Learnings (11)
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 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
Applied to files:
apps/roam/src/components/FuzzySelectInput.tsxapps/roam/src/components/LockableAutocompleteInput.tsxapps/roam/package.jsonapps/roam/src/components/ModifyNodeDialog.tsx
📚 Learning: 2025-07-19T22:34:08.725Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-07-19T22:34:08.725Z
Learning: Applies to {apps/website,packages/ui}/**/*.{tsx,jsx} : Use platform-native UI components first with shadcn/ui as a fallback
Applied to files:
apps/roam/src/components/FuzzySelectInput.tsx
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 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
Applied to files:
apps/roam/src/components/CreateNodeDialog.tsxapps/roam/package.json
📚 Learning: 2025-08-11T19:09:58.252Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 337
File: apps/roam/src/components/DiscourseFloatingMenu.tsx:43-43
Timestamp: 2025-08-11T19:09:58.252Z
Learning: The roam subdirectory (apps/roam) is constrained to React 17 and cannot use React 18+ features like createRoot API. ReactDOM.render should be used instead of createRoot in this subdirectory.
Applied to files:
apps/roam/src/components/CreateNodeDialog.tsxapps/roam/package.json
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 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
Applied to files:
apps/roam/src/components/CreateNodeDialog.tsxapps/roam/package.jsonapps/roam/src/components/ModifyNodeDialog.tsxapps/roam/src/utils/renderNodeTagPopup.tsx
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 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
Applied to files:
apps/roam/package.json
📚 Learning: 2025-07-19T22:34:16.794Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/obsidian.mdc:0-0
Timestamp: 2025-07-19T22:34:16.794Z
Learning: Applies to apps/obsidian/package.json : Prefer existing dependencies from package.json when adding or using dependencies in the Obsidian plugin
Applied to files:
apps/roam/package.json
📚 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/formatUtils.tsapps/roam/src/components/canvas/DiscourseNodeUtil.tsx
📚 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/formatUtils.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/formatUtils.tsapps/roam/src/components/ModifyNodeDialog.tsx
📚 Learning: 2025-08-25T15:53:21.799Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 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/components/ModifyNodeDialog.tsxapps/roam/src/utils/renderNodeTagPopup.tsx
🔇 Additional comments (12)
apps/roam/src/utils/formatUtils.ts (2)
15-18: LGTM: Clean import addition.The new imports for
ModifyNodeDialogand related types are well-organized and follow the project's import conventions.
20-23: LGTM: Type parameter correctly updated.The generic type parameter change from
FormDialogPropstoModifyNodeDialogPropsproperly reflects the migration to the new dialog component.apps/roam/src/utils/renderNodeTagPopup.tsx (2)
8-8: LGTM: Import updated correctly.The import change from the old dialog to
renderModifyNodeDialogaligns with the consolidation objective.
57-70: ****The no-op callbacks are intentional and appropriate. This function renders a simple UI button that triggers dialog-driven node creation—it doesn't manage node state or handle post-creation logic. The
ModifyNodeDialogcomponent itself handles creation, persistence, and cleanup. Unlike other call sites (e.g.,DiscourseNodeUtil.tsx) where the parent needs to track the created node's UID for state updates,renderNodeTagPopupButtonhas no such responsibility. The comment "Success is handled by the dialog itself" accurately reflects this design.Likely an incorrect or invalid review comment.
apps/roam/src/components/ModifyNodeDialog.tsx (8)
1-33: LGTM: Comprehensive imports.The imports are well-organized and include all necessary dependencies for the dialog functionality.
52-97: LGTM: Well-structured component initialization.The component uses appropriate React hooks with proper initialization and memoization. The filtering logic for discourse nodes based on
isFromCanvasis clear and well-documented.
98-124: LGTM: Robust referenced node detection.The logic correctly parses the node format string to extract referenced node information, with proper filtering of special cases like "content" and "context".
126-202: LGTM: Excellent async race condition handling.The use of request IDs and alive flags properly handles race conditions when fetching options. The cleanup function ensures no state updates occur after unmounting.
239-391: LGTM: Comprehensive create and edit logic.The submission logic handles both create and edit modes appropriately, with proper error handling, loading state management, and user feedback via toasts. The locked content flow is handled correctly.
393-497: LGTM: Well-structured dialog UI.The component follows BlueprintJS patterns correctly, with proper event handling to prevent TLDraw interference, appropriate disabled states, and clear error messaging.
As per coding guidelines
499-505: LGTM: Standard overlay render pattern.The helper function and default export follow established patterns in the codebase.
320-328: Fix invalidtypevalue; remove incorrect error suppression.The
@ts-expect-errorsuppression on"block-uid"is misplaced. According to the Roam API type definition,"block-uid"is a valid optional property. The actual TypeScript error is caused bytype: "outline", which is invalid. The validtypevalues are"block" | "page" | "query" | "graph" | "search".Fix: Replace
type: "outline"with a valid type value (e.g.,"block"), or adjust the window configuration to match the intended functionality. Remove the@ts-expect-errorsuppression.⛔ Skipped due to learnings
Learnt from: maparent Repo: DiscourseGraphs/discourse-graph PR: 0 File: :0-0 Timestamp: 2025-11-06T13:48:35.007Z Learning: In apps/roam/src/utils/createReifiedBlock.ts, the `setBlockProps` function does not need await as it is synchronous or fire-and-forget.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.Learnt from: CR Repo: DiscourseGraphs/discourse-graph PR: 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
| useEffect(() => { | ||
| if (mode === "edit" && referencedNode) { | ||
| setReferencedNodeLoading(true); | ||
| // TODO: replace this with a regex. this is extremely hacky primitive | ||
| const parseResult = contentText.trim().split("-"); | ||
| if (parseResult) { | ||
| // parseResult[1] is content, parseResult[2] is refnode | ||
| if (parseResult[1]) { | ||
| setContentText(parseResult[1].trim()); | ||
| } | ||
| if (parseResult[2]) { | ||
| const refText = parseResult[2].trim().replace(/[[\]]/g, ""); | ||
| setReferencedNodeText(refText); | ||
| const result = getPageUidByPageTitle(refText); | ||
| setReferencedNodeUid(result); | ||
| } | ||
| } | ||
| } | ||
| setReferencedNodeLoading(false); | ||
| }, [mode, referencedNode, nodeFormat, contentText]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fragile parsing logic needs improvement.
The comment on line 207 correctly identifies this as "extremely hacky primitive" parsing. Splitting on "-" is fragile and will break if the content contains hyphens. The regex mentioned in the TODO would be more robust.
Consider using the nodeFormat pattern to parse the content more reliably:
useEffect(() => {
if (mode === "edit" && referencedNode) {
setReferencedNodeLoading(true);
// Use the nodeFormat to extract content and referenced node
const regex = new RegExp(
nodeFormat
.replace(/{content}/gi, '(.+?)')
.replace(/{[\w\d-]+}/g, '\\[\\[(.+?)\\]\\]')
.replace(/[-[\]{}()*+?.,\\^$|#\s]/g, '\\$&')
);
const match = contentText.match(regex);
if (match) {
setContentText(match[1]?.trim() || "");
if (match[2]) {
const refText = match[2].trim();
setReferencedNodeText(refText);
const result = getPageUidByPageTitle(refText);
setReferencedNodeUid(result);
}
}
}
setReferencedNodeLoading(false);
}, [mode, referencedNode, nodeFormat, contentText]);🤖 Prompt for AI Agents
In apps/roam/src/components/ModifyNodeDialog.tsx around lines 204 to 223, the
current split-on-hyphen parsing is fragile; replace it with a regex derived from
nodeFormat: escape regex metacharacters, replace {content} with a non-greedy
capture group (e.g. (.+?)), replace reference placeholders with a capture for
the inner page title and literal double-bracket markers (e.g. \[\[(.+?)\]\]),
then run contentText.match(regex) and if there is a match
setContentText(match[1]?.trim() || "") and, if present,
setReferencedNodeText(match[2].trim()) and
setReferencedNodeUid(getPageUidByPageTitle(match[2].trim())); keep the
referencedNodeLoading toggles around the parse and ensure you don’t rely on
brittle string splitting.
There was a problem hiding this comment.
the regex needs to be more complex I'll follow up with another ticket that address this issue + also auto detecting Source/ RefNode when created from tagged block of child node
There was a problem hiding this comment.
@trangdoan982, sounds good! That makes sense to address this along with the auto-detection feature in a follow-up.
Would you like me to create a GitHub issue to track this work? I can include:
- The parsing logic improvement (replacing the split-on-hyphen with proper regex)
- Auto-detecting Source/RefNode when created from tagged block or child node
- Links back to this PR and comment for context
🧠 Learnings used
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.
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 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.
trangdoan982
changed the title
|
next action: rebase |
trangdoan982
force-pushed
the
eng-979-consolidate-create-node-flow
branch
from
b0eedde to
a3ed78a
Compare
| zod@3.25.76: {} | ||
|
|
||
| zwitch@2.0.4: {} | ||
| zwitch@2.0.4: {} No newline at end of file |
There was a problem hiding this comment.
i'm not sure how it got here. there's no extra space in my local
There was a problem hiding this comment.
I think there are IDE settings you can toggle to show different styles of whitespaces. This might show something that you aren't currently seeing.
There was a problem hiding this comment.
I would revert this change to be safe. If/when it happens again, take a look into your IDE settings or try to keep an eye on when this change is introduced and what was the action that happen right before that caused it to be introduced.
|
@mdroidian this PR is ready for review. I've addressed Matt & Johnny's comments on Linear ENG-979: Implement the new Create node flow. here are the changes and fixes:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
apps/roam/src/components/canvas/uiOverrides.tsx (1)
187-198: Consider: Text shape conversion still uses direct flow.For consistency, the text shape conversion could also use the dialog-based flow like image shapes. Currently, text shapes are converted immediately without user confirmation, while image shapes open a dialog.
This may be intentional (text content is already visible), but worth confirming if the UX should be unified.
apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (1)
480-578: Complex effect with potential race condition.The dialog rendering logic is sound, but there are a few concerns:
Stale closure risk:
setSizeAndImgPropsLocalis defined inside the effect and capturesextensionAPIandthis.type. If these change while the dialog is open, the callback will use stale values.Missing
thisin dependencies: The effect usesthis.type,this.updateProps,this.deleteRelationsInCanvas, andthis.createExistingRelations, butthisisn't in the dependency array. This works because class instance identity is stable, but could be clearer.Void promise: Line 530 uses
void createDiscourseNode(...)which means errors won't be caught in the try-catch block below.Consider moving error handling to cover the create action:
if ( action === "create" && !getPageUidByPageTitle(text) && !newPageUid ) { - void createDiscourseNode({ + await createDiscourseNode({ configPageUid: shape.type, text, newPageUid: finalUid, extensionAPI, }); }apps/roam/src/components/FuzzySelectInput.tsx (1)
227-230: Potential race condition on blur.The 200ms
setTimeoutdelay before closing the dropdown is a common workaround to allow click events to fire before blur closes the menu. However, this can cause flickering if the user rapidly focuses/unfocuses. Consider usingonMouseDownwithe.preventDefault()on menu items (which you already do on line 204-205) and removing the timeout:onBlur={() => { setIsFocused(false); - setTimeout(() => setIsOpen(false), 200); + setIsOpen(false); }}Since
onMouseDownalready prevents the blur from firing before selection, the timeout may not be necessary.apps/roam/src/utils/formatUtils.ts (1)
10-10: Remove unused import.
FormDialogis imported but no longer used after the migration toModifyNodeDialog.-import FormDialog from "roamjs-components/components/FormDialog";apps/roam/src/components/ModifyNodeDialog.tsx (2)
103-129: RedundantgetDiscourseNodes()calls inside memo.The
referencedNodememo (lines 112-114) callsgetDiscourseNodes()again instead of using the already-memoizeddiscourseNodesfrom line 88. This is inefficient and could lead to inconsistent behavior.const referencedNode = useMemo(() => { const regex = /{([\w\d-]*)}/g; const matches = [...nodeFormat.matchAll(regex)]; for (const match of matches) { const val = match[1]; if (val.toLowerCase() === "content") continue; if (val.toLowerCase() === "context") continue; - const allNodes = isFromCanvas - ? getDiscourseNodes() - : getDiscourseNodes().filter(excludeDefaultNodes); - - const refNode = allNodes.find(({ text }) => + const refNode = discourseNodes.find(({ text }) => new RegExp(text, "i").test(val), ); if (refNode) { return { name: refNode.text, nodeType: refNode.type, }; } } return null; - }, [nodeFormat, isFromCanvas]); + }, [nodeFormat, discourseNodes]);
53-65: Consider adding explicit return types.Per coding guidelines, functions should have explicit return types. The component and several callbacks (
setValue,setReferencedNodeValue,onCancelClick,addImageToPage,onSubmit) lack them. This is a low-priority improvement.Example for the component:
const ModifyNodeDialog = ({ // ...props }: RoamOverlayProps<ModifyNodeDialogProps>): React.ReactElement => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/roam/src/components/CreateNodeDialog.tsx(1 hunks)apps/roam/src/components/FuzzySelectInput.tsx(1 hunks)apps/roam/src/components/ModifyNodeDialog.tsx(1 hunks)apps/roam/src/components/canvas/DiscourseNodeUtil.tsx(4 hunks)apps/roam/src/components/canvas/uiOverrides.tsx(2 hunks)apps/roam/src/styles/styles.css(1 hunks)apps/roam/src/utils/formatUtils.ts(2 hunks)apps/roam/src/utils/renderNodeTagPopup.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
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/styles/styles.cssapps/roam/src/components/FuzzySelectInput.tsxapps/roam/src/utils/formatUtils.tsapps/roam/src/components/ModifyNodeDialog.tsxapps/roam/src/utils/renderNodeTagPopup.tsxapps/roam/src/components/canvas/DiscourseNodeUtil.tsxapps/roam/src/components/canvas/uiOverrides.tsxapps/roam/src/components/CreateNodeDialog.tsx
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/styles/styles.cssapps/roam/src/components/FuzzySelectInput.tsxapps/roam/src/utils/formatUtils.tsapps/roam/src/components/ModifyNodeDialog.tsxapps/roam/src/utils/renderNodeTagPopup.tsxapps/roam/src/components/canvas/DiscourseNodeUtil.tsxapps/roam/src/components/canvas/uiOverrides.tsxapps/roam/src/components/CreateNodeDialog.tsx
**/*.{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/components/FuzzySelectInput.tsxapps/roam/src/utils/formatUtils.tsapps/roam/src/components/ModifyNodeDialog.tsxapps/roam/src/utils/renderNodeTagPopup.tsxapps/roam/src/components/canvas/DiscourseNodeUtil.tsxapps/roam/src/components/canvas/uiOverrides.tsxapps/roam/src/components/CreateNodeDialog.tsx
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/components/FuzzySelectInput.tsxapps/roam/src/utils/formatUtils.tsapps/roam/src/components/ModifyNodeDialog.tsxapps/roam/src/utils/renderNodeTagPopup.tsxapps/roam/src/components/canvas/DiscourseNodeUtil.tsxapps/roam/src/components/canvas/uiOverrides.tsxapps/roam/src/components/CreateNodeDialog.tsx
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/components/FuzzySelectInput.tsxapps/roam/src/utils/formatUtils.tsapps/roam/src/components/ModifyNodeDialog.tsxapps/roam/src/utils/renderNodeTagPopup.tsxapps/roam/src/components/canvas/DiscourseNodeUtil.tsxapps/roam/src/components/canvas/uiOverrides.tsxapps/roam/src/components/CreateNodeDialog.tsx
🧠 Learnings (10)
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/**/*.{ts,tsx,jsx,js,css,scss} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Applied to files:
apps/roam/src/styles/styles.cssapps/roam/src/components/FuzzySelectInput.tsxapps/roam/src/utils/renderNodeTagPopup.tsxapps/roam/src/components/canvas/DiscourseNodeUtil.tsx
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/**/*.{ts,tsx,js,jsx} : Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Applied to files:
apps/roam/src/components/FuzzySelectInput.tsxapps/roam/src/components/ModifyNodeDialog.tsxapps/roam/src/utils/renderNodeTagPopup.tsxapps/roam/src/components/CreateNodeDialog.tsx
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/**/*.{ts,tsx,js,jsx} : Use Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when implementing extension functionality
Applied to files:
apps/roam/src/utils/formatUtils.tsapps/roam/src/utils/renderNodeTagPopup.tsxapps/roam/src/components/canvas/DiscourseNodeUtil.tsxapps/roam/src/components/CreateNodeDialog.tsx
📚 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/formatUtils.tsapps/roam/src/components/ModifyNodeDialog.tsxapps/roam/src/utils/renderNodeTagPopup.tsxapps/roam/src/components/canvas/DiscourseNodeUtil.tsx
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/**/*.{js,ts,tsx,jsx,json} : Prefer existing dependencies from package.json when working on the Roam Research extension
Applied to files:
apps/roam/src/utils/formatUtils.tsapps/roam/src/components/canvas/DiscourseNodeUtil.tsx
📚 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/formatUtils.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/formatUtils.ts
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/** : Implement the Discourse Graph protocol in the Roam Research extension
Applied to files:
apps/roam/src/utils/formatUtils.tsapps/roam/src/components/canvas/DiscourseNodeUtil.tsx
📚 Learning: 2025-08-25T15:53:21.799Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 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/components/ModifyNodeDialog.tsxapps/roam/src/utils/renderNodeTagPopup.tsx
📚 Learning: 2025-08-11T19:09:58.252Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 337
File: apps/roam/src/components/DiscourseFloatingMenu.tsx:43-43
Timestamp: 2025-08-11T19:09:58.252Z
Learning: The roam subdirectory (apps/roam) is constrained to React 17 and cannot use React 18+ features like createRoot API. ReactDOM.render should be used instead of createRoot in this subdirectory.
Applied to files:
apps/roam/src/utils/renderNodeTagPopup.tsxapps/roam/src/components/CreateNodeDialog.tsx
🧬 Code graph analysis (6)
apps/roam/src/components/FuzzySelectInput.tsx (1)
apps/roam/src/utils/types.ts (1)
Result(42-46)
apps/roam/src/utils/formatUtils.ts (1)
apps/roam/src/components/ModifyNodeDialog.tsx (1)
ModifyNodeDialogProps(35-51)
apps/roam/src/components/ModifyNodeDialog.tsx (4)
apps/roam/src/utils/types.ts (1)
Result(42-46)apps/roam/src/utils/getDiscourseNodes.ts (1)
excludeDefaultNodes(9-11)apps/roam/src/utils/formatUtils.ts (1)
getNewDiscourseNodeText(25-86)apps/obsidian/src/utils/createNode.ts (1)
createDiscourseNode(110-141)
apps/roam/src/utils/renderNodeTagPopup.tsx (2)
apps/roam/src/utils/formatUtils.ts (1)
getReferencedNodeInFormat(88-119)apps/roam/src/components/ModifyNodeDialog.tsx (1)
renderModifyNodeDialog(519-523)
apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (3)
apps/roam/src/components/ModifyNodeDialog.tsx (1)
renderModifyNodeDialog(519-523)apps/roam/src/components/canvas/Tldraw.tsx (1)
isPageUid(121-124)apps/roam/src/utils/extensionSettings.ts (1)
getSetting(3-11)
apps/roam/src/components/canvas/uiOverrides.tsx (1)
apps/roam/src/components/ModifyNodeDialog.tsx (1)
renderModifyNodeDialog(519-523)
🔇 Additional comments (13)
apps/roam/src/styles/styles.css (1)
149-152: LGTM!The CSS rule ensures the popover target fills the container width, which is necessary for the new FuzzySelectInput component to display correctly. This aligns with BlueprintJS 3 styling patterns.
apps/roam/src/components/CreateNodeDialog.tsx (1)
1-9: LGTM!The deprecation notice is clear and provides helpful migration guidance with prop mappings. This allows existing consumers to gradually migrate to ModifyNodeDialog.
apps/roam/src/components/canvas/uiOverrides.tsx (1)
144-185: LGTM!The image-to-node conversion flow is well-structured:
- Captures original coordinates before dialog opens
- Uses the dialog for user confirmation before node creation
- Properly resolves the final UID with
newPageUid || uid- Deletes original shape and creates new one at the same position
The empty
onClosecallback is appropriate since cancellation should preserve the original image.apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (1)
525-536: Verify create condition logic.The condition
!getPageUidByPageTitle(text) && !newPageUidensures we only create when:
- No existing page has this title, AND
- The dialog didn't already create a page (newPageUid is falsy)
This seems correct for avoiding duplicate page creation. However,
getPageUidByPageTitleis a synchronous Roam query that might not reflect very recent changes. If timing is critical, consider verifying this behaves correctly when rapidly creating nodes.apps/roam/src/components/FuzzySelectInput.tsx (3)
14-25: Props type is well-defined.The generic type constraint
T extends Result = Resultprovides good flexibility while ensuring the minimum required fields are present. The props interface is clear and follows the coding guidelines for usingtypeoverinterface.
49-54: LGTM!The fuzzy filtering implementation is clean and efficient using memoization. The
extractfunction correctly pulls thetextfield for matching.
85-107: LGTM!Keyboard navigation is well-implemented with proper bounds checking and event handling. The
e.stopPropagation()on Enter is a good addition to prevent form submission if this component is used within a form.apps/roam/src/utils/renderNodeTagPopup.tsx (2)
49-83: Well-structured helper with proper error handling.The
getInitialReferencedNodefunction properly handles edge cases (no blockUid, no referenced node type) and includes try-catch for the Roam query. The fallback to empty values ensures the dialog can still open even if referenced node lookup fails.
91-104: Dialog invocation looks correct.The
renderModifyNodeDialogcall properly passes all required props. Since this isn't wrapped in a promise (unlikeformatUtils.ts), the emptyonSuccessandonClosecallbacks are appropriate—the dialog handles its own success flow internally.apps/roam/src/components/ModifyNodeDialog.tsx (4)
34-51: Type definitions are well-structured.The
ModifyNodeDialogModeandModifyNodeDialogPropstypes provide good type safety with appropriate optional fields for different use cases (canvas, image conversion).
367-401: Edit mode logic looks correct.The edit flow properly formats content with the referenced node, updates the block, shows a success toast, and returns the appropriate values via
onSuccess.
411-516: Dialog UI implementation follows guidelines.The component correctly uses BlueprintJS 3 components (
Dialog,Label,Button,Spinner) combined with Tailwind CSS classes for styling. The event propagation handling for TLDraw compatibility is well-documented with a Discord reference. As per coding guidelines forapps/roam/**/*.{ts,tsx}.
519-523: Export follows established pattern.The
renderModifyNodeDialogfunction correctly wraps the component withrenderOverlay, matching the pattern used elsewhere in the codebase.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
@trangdoan982 I called coderabbit to do another review because there were more changes.
Could you review those (5 of them) and tag me for review once they have been looked it (either addressed or just resolve the thread if you don't feel they need to be addressed).
| zod@3.25.76: {} | ||
|
|
||
| zwitch@2.0.4: {} | ||
| zwitch@2.0.4: {} No newline at end of file |
There was a problem hiding this comment.
I think there are IDE settings you can toggle to show different styles of whitespaces. This might show something that you aren't currently seeing.
trangdoan982
force-pushed
the
eng-979-consolidate-create-node-flow
branch
from
9f9a1ad to
30dc59c
Compare
There was a problem hiding this comment.
Almost there (!) — a few more changes suggested.
Because this PR is large and touches many parts of the system, once the remaining changes are addressed, could you please:
- List all user actions you can identify that are affected by this PR
- Record a Loom video demonstrating and validating each of those actions
For example:
- Create a node on the canvas
- Create a node using /
- Create a node using …
- Edit a node using …
- Convert text or images into a node on the canvas
- Create node that has a pre-defined reference node
- etc
This will help ensure we haven’t missed any regressions across the different interaction paths.
| import { getSetting } from "~/utils/extensionSettings"; | ||
| import DiscourseContextOverlay from "~/components/DiscourseContextOverlay"; | ||
| import { getDiscourseNodeColors } from "~/utils/getDiscourseNodeColors"; | ||
| import { OnloadArgs } from "roamjs-components/types"; |
| text: string; | ||
| uid: string; | ||
| action: string; | ||
| newPageUid?: string; |
There was a problem hiding this comment.
Why do we need newPageUid? Isn't this derivable from action? Eg: action === create means uid by definition is "new"?
If it isn't 100% necessary, let's remove it.
There was a problem hiding this comment.
Looks like onSuccess has the same value for uid and newPageUid
await onSuccess({
text: formattedTitle,
uid: newPageUid,
action: "create",
newPageUid,
});| finalUid, | ||
| }); | ||
| } catch (error) { | ||
| console.error( |
There was a problem hiding this comment.
Let's notify the user. There is a tldraw toast you can use (see examples in the code)
| imageUrl: src, | ||
| onSuccess: async ({ text, uid, newPageUid }) => { | ||
| const finalUid = newPageUid || uid; | ||
| // Delete the original image shape |
| // Delete the original image shape | ||
| editor.deleteShapes([shape.id]); | ||
|
|
||
| // Create the discourse node shape |
| zod@3.25.76: {} | ||
|
|
||
| zwitch@2.0.4: {} | ||
| zwitch@2.0.4: {} No newline at end of file |
There was a problem hiding this comment.
I would revert this change to be safe. If/when it happens again, take a look into your IDE settings or try to keep an eye on when this change is introduced and what was the action that happen right before that caused it to be introduced.
| nodeType: selectedNodeType.type, | ||
| blockUid: sourceBlockUid, | ||
| }); | ||
| console.log("formattedTitle", formattedTitle); |
| @@ -0,0 +1,536 @@ | |||
| import { | |||
There was a problem hiding this comment.
There are multiple es-lints on this page.
Let's get this to 0 (by fixing or disabling inline).
If we need to disable, let's add either an inline comment or comment on github as to why, if it's not obvious.
|
|
||
| type FormDialogProps = Parameters<typeof FormDialog>[0]; | ||
| const renderFormDialog = createOverlayRender<FormDialogProps>( | ||
| const renderFormDialog = createOverlayRender<ModifyNodeDialogProps>( |
There was a problem hiding this comment.
remove unused imports cause from changes on this page
| if (!referencedNodeType) return { text: "", uid: "" }; | ||
|
|
||
| try { | ||
| const referenced = window.roamAlphaAPI.data.fast.q( |
There was a problem hiding this comment.
use data.async.fast.q to future proof. note: this returns a promise.
https://roamresearch.com/#/app/developer-documentation/page/VJfOx4mgR
Summary by CodeRabbit
Release Notes
New Features
Deprecations
Style
✏️ Tip: You can customize this high-level summary in your review settings.