-
Notifications
You must be signed in to change notification settings - Fork 4
[ENG-1070] Don't move arrow onTranslate - Roam discourse relation arrow #622
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[ENG-1070] Don't move arrow onTranslate - Roam discourse relation arrow #622
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe PR modifies the translation logic in DiscourseRelationShape utilities. When both arrow endpoints are bound, dragging the arrow now adjusts its bend instead of moving the endpoints. The implementation computes a page-space delta, derives terminal positions in arrow space, and determines a bend value by projecting the translated midpoint onto the line connecting the endpoints. Changes
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx (1)
433-488: Consider extracting the bend-from-translation logic to a shared helper.This ~55-line block is duplicated verbatim in
DiscourseRelationUtil.onTranslate(lines 928-983). Extracting it into a shared helper function (e.g., inhelpers.tsx) would improve maintainability and ensure both classes stay in sync.Example helper signature:
// In helpers.tsx export const calculateBendFromTranslation = ( editor: Editor, initialShape: DiscourseRelationShape, shape: DiscourseRelationShape, atTranslationStart: { pagePosition: Vec }, ): TLShapePartial<DiscourseRelationShape> | undefined => { const bindings = getArrowBindings(editor, shape); if (!bindings.start || !bindings.end) return undefined; // ... bend calculation logic ... return { id: shape.id, type: shape.type, x: initialShape.x, y: initialShape.y, props: { bend: newBend }, }; };Then in both
onTranslatehandlers:const bendUpdate = calculateBendFromTranslation( this.editor, initialShape, shape, atTranslationStart ); if (bendUpdate) return bendUpdate; // ... rest of original translation logic
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx(2 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/DiscourseRelationShape/DiscourseRelationUtil.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/DiscourseRelationShape/DiscourseRelationUtil.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/DiscourseRelationShape/DiscourseRelationUtil.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/DiscourseRelationShape/DiscourseRelationUtil.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/DiscourseRelationShape/DiscourseRelationUtil.tsx
🧬 Code graph analysis (1)
apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx (1)
apps/roam/src/components/canvas/DiscourseRelationShape/helpers.tsx (2)
getArrowBindings(141-153)getArrowTerminalsInArrowSpace(407-441)
🔇 Additional comments (1)
apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx (1)
928-983: Logic is correct for preventing arrow translation when both ends are bound.The bend calculation correctly mirrors the existing
onHandleDragMIDDLE handle logic, ensuring consistent behavior. As noted above, this duplicates the logic inReferencedNodeUtiland would benefit from extraction to a shared helper.
https://www.loom.com/share/7c8291348ccb42189311a7cec2efa416
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.