ENG-1107 Update drag and drop image for nodes in discourse toolbar #578
Conversation
…or logic with getDiscourseNodeColors utility function for improved maintainability and clarity.
…oved color handling, adjust drag behavior for relations, and refine drag preview styling based on zoom level.
…dependent offsets, adjust padding in drag preview, and enhance color retrieval logic in getDiscourseNodeColors for improved palette handling.
… are only clickable, preventing pointermove listener from being added for relation items.
mdroidian
deleted the
eng-1107-update-drag-and-drop-image-for-nodes-in-discourse-toolbar
branch
mdroidian
restored the
eng-1107-update-drag-and-drop-image-for-nodes-in-discourse-toolbar
branch
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
mdroidian
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR refactors discourse node color computation by extracting it into a dedicated utility function, exports sizing and dimension constants for reuse, and enhances the drag-and-drop panel with color properties, zoom-aware preview rendering, and relation-specific handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 (3)
apps/roam/src/utils/getDiscourseNodeColors.ts (1)
21-22: Remove redundant nullish coalescing operator.
Array.findIndex()returns-1when no match is found, nevernullorundefined. The?? -1fallback is unreachable.- const discourseNodeIndex = - discourseNodes.findIndex((node) => node.type === nodeType) ?? -1; + const discourseNodeIndex = discourseNodes.findIndex( + (node) => node.type === nodeType, + );apps/roam/src/components/canvas/DiscourseToolPanel.tsx (2)
82-111: Minor inconsistency: relation colors use tldraw color names, not hex values.For nodes,
getDiscourseNodeColorsreturns hex colors (e.g.,#4263eb), but for relations,getRelationColorreturns tldraw color style names (e.g.,"green","red"). While this doesn't cause issues currently since relations aren't draggable and their preview is hidden, the asymmetry could cause confusion if relation dragging is enabled in the future.Consider documenting this distinction or converting relation colors to hex for consistency.
261-264: Minor:zoomLevelis computed twice.
zoomLevelis derived at line 261-264 viauseValue, then re-computed at line 295 insideuseQuickReactor. The reactor recomputation is necessary for reactivity within that callback, but the outerzoomLevel(lines 261-264) is only used at line 432 for font sizing.This is functionally correct but could be consolidated if the reactor's logic were restructured. Low priority.
Also applies to: 294-295
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/roam/src/components/canvas/DiscourseNodeUtil.tsx(3 hunks)apps/roam/src/components/canvas/DiscourseToolPanel.tsx(10 hunks)apps/roam/src/components/canvas/Tldraw.tsx(1 hunks)apps/roam/src/utils/getDiscourseNodeColors.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/canvas/Tldraw.tsxapps/roam/src/utils/getDiscourseNodeColors.tsapps/roam/src/components/canvas/DiscourseNodeUtil.tsxapps/roam/src/components/canvas/DiscourseToolPanel.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/canvas/Tldraw.tsxapps/roam/src/utils/getDiscourseNodeColors.tsapps/roam/src/components/canvas/DiscourseNodeUtil.tsxapps/roam/src/components/canvas/DiscourseToolPanel.tsx
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/components/canvas/Tldraw.tsxapps/roam/src/utils/getDiscourseNodeColors.tsapps/roam/src/components/canvas/DiscourseNodeUtil.tsxapps/roam/src/components/canvas/DiscourseToolPanel.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/canvas/Tldraw.tsxapps/roam/src/utils/getDiscourseNodeColors.tsapps/roam/src/components/canvas/DiscourseNodeUtil.tsxapps/roam/src/components/canvas/DiscourseToolPanel.tsx
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/components/canvas/Tldraw.tsxapps/roam/src/utils/getDiscourseNodeColors.tsapps/roam/src/components/canvas/DiscourseNodeUtil.tsxapps/roam/src/components/canvas/DiscourseToolPanel.tsx
🧠 Learnings (5)
📚 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/getDiscourseNodeColors.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 `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/getDiscourseNodeColors.tsapps/roam/src/components/canvas/DiscourseNodeUtil.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/getDiscourseNodeColors.tsapps/roam/src/components/canvas/DiscourseNodeUtil.tsx
📚 Learning: 2025-06-22T10:40:21.679Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph 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.
Applied to files:
apps/roam/src/utils/getDiscourseNodeColors.ts
📚 Learning: 2025-07-08T14:51:55.299Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-08T14:51:55.299Z
Learning: DiscourseGraphs maintainers prefer not to add narrowly typed definitions for individual `process.env` keys to avoid unnecessary coupling.
Applied to files:
apps/roam/src/components/canvas/DiscourseToolPanel.tsx
🧬 Code graph analysis (3)
apps/roam/src/utils/getDiscourseNodeColors.ts (2)
apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (2)
COLOR_ARRAY(78-78)COLOR_PALETTE(81-95)packages/utils/src/getPleasingColors.ts (1)
getPleasingColors(118-188)
apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (1)
apps/roam/src/utils/getDiscourseNodeColors.ts (1)
getDiscourseNodeColors(14-41)
apps/roam/src/components/canvas/DiscourseToolPanel.tsx (5)
apps/roam/src/utils/getDiscourseNodeColors.ts (1)
getDiscourseNodeColors(14-41)apps/roam/src/components/settings/DiscourseNodeCanvasSettings.tsx (1)
formatHexColor(16-26)apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx (1)
getRelationColor(86-107)apps/roam/src/components/canvas/Tldraw.tsx (2)
DEFAULT_WIDTH(117-117)DEFAULT_HEIGHT(118-118)apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (2)
DEFAULT_STYLE_PROPS(71-77)FONT_SIZES(60-65)
🔇 Additional comments (11)
apps/roam/src/components/canvas/Tldraw.tsx (1)
117-118: LGTM!Exporting
DEFAULT_WIDTHandDEFAULT_HEIGHTenables consistent centering behavior across the codebase. The values are now shared withDiscourseToolPanel.tsxfor drag-and-drop offset calculations.apps/roam/src/utils/getDiscourseNodeColors.ts (1)
14-41: Good extraction of color computation logic.The utility correctly encapsulates the color derivation logic, handles edge cases (missing node, missing color), and returns contrast-safe colors via
getPleasingColors. The explicit return type and use oftypealign with the coding guidelines.apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (3)
48-48: LGTM!Import added for the new centralized color utility.
60-65: LGTM!Exporting
FONT_SIZESenables consistent typography across canvas components. TheRecord<TLDefaultSizeStyle, number>type ensures type safety when accessing font sizes by tldraw's size style keys.
344-346: Good refactor to centralize color computation.Delegating to
getDiscourseNodeColorseliminates duplicate color logic and ensures consistency withDiscourseToolPanel.tsx.apps/roam/src/components/canvas/DiscourseToolPanel.tsx (6)
9-18: LGTM!Imports are well-organized, bringing in the necessary constants and utilities for consistent sizing, styling, and color computation.
37-38: LGTM!Extending
DragStatewithbackgroundColorandtextColorenables the drag preview to display accurate colors matching the node's canvas appearance.Also applies to: 49-50
131-134: LGTM!Good defensive guards ensuring relations are click-to-activate only, not draggable. The three locations (pointermove transition, listener attachment, and preview display) are consistently guarded.
Also applies to: 227-232, 282-286
180-181: LGTM!Using
DEFAULT_WIDTH / 2andDEFAULT_HEIGHT / 2as offsets ensures dropped shapes are centered at the cursor position, improving the drag-and-drop UX.Also applies to: 188-189
294-321: Zoom-aware preview rendering looks good.The preview correctly scales dimensions, border radius, and positioning based on zoom level, with a sensible 0.5 minimum to prevent overly small previews. The viewport-relative positioning ensures correct placement.
425-438: LGTM!The drag preview text styling now uses shared constants (
DEFAULT_STYLE_PROPS,FONT_FAMILIES,FONT_SIZES) for consistency with the actual rendered nodes. The zoom-scaled font size ensures the preview matches the expected canvas appearance.
mdroidian
deleted the
eng-1107-update-drag-and-drop-image-for-nodes-in-discourse-toolbar
branch
Before
After
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.