Roam: [Eng-621] show node tag options in inline node creation menu #298
Conversation
sid597
changed the base branch from
eng-620-update-actions-to-use-npm-ci
to
eng-610-tag-assignment-in-roam-dg-plugin-settings-menu
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe changes introduce a toggleable mode for the discourse node menu in the Roam app, allowing users to switch between viewing full node types and a simplified tag view. This mode is controlled by Shift key state and text selection, affecting menu display, filtering, selection logic, and keyboard interactions across related components and utility functions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Textarea
participant NodeMenu
participant UtilityFunctions
User->>Textarea: Selects text or presses Shift
Textarea->>UtilityFunctions: handleNodeMenuRender(event)
UtilityFunctions->>NodeMenu: renderDiscourseNodeMenu(textarea, isShift, isTextSelected)
NodeMenu->>User: Displays menu (node types or tags)
User->>NodeMenu: Selects menu item
NodeMenu->>UtilityFunctions: onSelect (node type or tag logic)
Estimated code review effort3 (30–60 minutes) 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: 1
🧹 Nitpick comments (2)
apps/roam/src/components/DiscourseNodeMenu.tsx (2)
51-51: Add null check for node tag propertyWhen
showNodeTypesis false, the filter conditionn.tagmight evaluate to a falsy value like empty string or undefined. Consider being more explicit:- (n) => n.backedBy === "user" && (showNodeTypes || n.tag), + (n) => n.backedBy === "user" && (showNodeTypes || (n.tag && n.tag.trim())),
105-111: Incomplete tag selection implementationThe tag selection branch only logs the selected tag without performing any action. This appears to be incomplete functionality that doesn't align with the PR objective of showing node tag options in the creation menu.
Would you like me to help implement the tag selection functionality or open an issue to track this TODO?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/roam/src/components/DiscourseNodeMenu.tsx(6 hunks)apps/roam/src/utils/initializeObserversAndListeners.ts(1 hunks)apps/roam/src/utils/renderTextSelectionPopup.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 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.
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#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.
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#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.610Z
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: DiscourseGraphs/discourse-graph#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.610Z
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
apps/roam/src/utils/renderTextSelectionPopup.tsx (3)
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.610Z
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.610Z
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.610Z
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/utils/initializeObserversAndListeners.ts (3)
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: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.610Z
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/components/DiscourseNodeMenu.tsx (5)
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/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.610Z
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.610Z
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
🔇 Additional comments (2)
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
162-169: Clean implementation for detecting text selection and keyboard stateThe changes correctly capture the text selection state and shift key modifier to enable the new menu display modes. Good use of typed variables to avoid repeated casting.
apps/roam/src/utils/renderTextSelectionPopup.tsx (1)
62-62: Correct prop value for text selection contextSetting
isTextSelected={true}is appropriate here since this popup only appears when text is actually selected.
… 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.
|
@sid597 When no text is selected and you press |
|
| (n) => n.backedBy === "user" && (showNodeTypes || n.tag), | ||
| ), | ||
| [showNodeTypes], | ||
| () => userDiscourseNodes.filter((n) => showNodeTypes || n.tag), |
There was a problem hiding this comment.
Because getDiscourseNodes is memoized (which is the expensive query), discourseNodes doesn't also need to be memoized because it is a simple filter.
2 participants
https://www.loom.com/share/d3d53d536c1d445c9052e01517eac201?sid=76e85ee4-1412-4a83-b48a-6000774dbe66
Summary by CodeRabbit
New Features
Improvements