ENG-931 - Node Menu fixes #477
Conversation
…ing. Introduce early returns and arrow functions for better readability. Implement timeouts for block updates and streamline tag addition logic.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
mdroidian
requested review from
sid597 and
trangdoan982
and removed request for
sid597
| }); | ||
|
|
||
| createDiscourseNode({ | ||
| await createDiscourseNode({ |
There was a problem hiding this comment.
Having createDiscourseNode before update fixes the sidebar open issue. updateBlock was actually creating a page too fast, so we were getting an error saying "this page was already created", thus not able to set the pageuid for openinsidebar
|
|
||
| // Remove focus from the block to ensure updateBlock works properly | ||
| // https://github.com/RoamJS/query-builder/issues/286 | ||
| document.body.click(); |
There was a problem hiding this comment.
moved this to inside the onSelect because
- it can be in a single place
- let's node tags be added without removing focus from the block
- allows us to
getFocusedBlockin the future so that we can set the cursor after editing the block
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughRefactors DiscourseNodeMenu to introduce asynchronous node creation and delayed block updates via setTimeout, adds helper functions for node creation and tag insertion, consolidates keyboard navigation using getActiveIndex, updates keydown listener dependencies, and adjusts event handling to run updates after focus removal. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant M as DiscourseNodeMenu
participant DG as createDiscourseNode
participant RB as Roam Block
U->>M: Select option (Enter/Click/Shortcut)
M->>M: Remove focus
Note right of M: Schedule update (setTimeout 100ms)
M->>DG: await createDiscourseNode(pageName)
DG-->>M: Node created
M->>M: setTimeout(..., 100ms)
M-->>RB: Update block text
M->>M: posthog event
rect rgba(230,255,230,0.4)
note over M,RB: Tag insertion path
U->>M: Select tag
M->>M: setTimeout addTagToBlock (100ms)
M-->>RB: Update block with tag
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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: 0
🧹 Nitpick comments (2)
apps/roam/src/components/DiscourseNodeMenu.tsx (2)
85-111: Add error handling for async operations.The async helper
createNodeAndUpdateBlocklacks error handling. IfgetNewDiscourseNodeTextorcreateDiscourseNodefails, the user won't see any feedback, and the block won't be updated.Consider wrapping the async operations in a try-catch block:
const createNodeAndUpdateBlock = async () => { + try { const pageName = await getNewDiscourseNodeText({ text: highlighted, nodeType: nodeUid, blockUid, }); if (!pageName) return; const currentBlockText = getTextByBlockUid(blockUid); const newText = `${currentBlockText.substring( 0, textarea.selectionStart, )}[[${pageName}]]${currentBlockText.substring(textarea.selectionEnd)}`; await createDiscourseNode({ text: pageName, configPageUid: nodeUid, extensionAPI, }); void updateBlock({ text: newText, uid: blockUid }); posthog.capture("Discourse Node: Created via Node Menu", { nodeType: nodeUid, text: pageName, }); + } catch (error) { + console.error("Failed to create discourse node:", error); + // Consider showing user feedback here + } };
147-149: Add null safety to getActiveIndex.The
getActiveIndexhelper doesn't handle the case wheremenuRef.currentisnull, which could lead to a runtime error.Consider adding a fallback:
const getActiveIndex = () => { - return Number(menuRef.current?.getAttribute("data-active-index")); + return Number(menuRef.current?.getAttribute("data-active-index") ?? 0); };This ensures that if
menuRef.currentis null, the function returns0instead ofNaN.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/roam/src/components/DiscourseNodeMenu.tsx(3 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/components/DiscourseNodeMenu.tsx
🔇 Additional comments (2)
apps/roam/src/components/DiscourseNodeMenu.tsx (2)
81-83: Clarify focus removal behavior for tag insertion.According to the past review comment on line 83,
document.body.click()was moved insideonSelectto "let node tags be added without removing focus from the block." However, the current code removes focus for both node creation and tag insertion.Can you clarify whether focus removal is intentional for tag insertion, or if there should be a conditional check to only remove focus for node creation?
Based on past review comments.
172-172: LGTM! Dependency array fix is correct.Adding
shortcutsto the dependency array ensures that the keydown listener updates when the available shortcuts change. This prevents stale closure issues where the handler might reference an outdated set of shortcuts.
https://www.loom.com/share/4e4518added040f4a6dc232d1a3cb996
Summary by CodeRabbit
Bug Fixes
Refactor