ENG-1153 maximum update limit error on canvas after adding supports relation between evd and clm #628
Conversation
…eRelationsBindings:reparentArrow
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughModifies arrow reparenting logic in DiscourseRelationBindings to prevent index conflicts with bound relation arrows. Introduces exclusion set logic to filter out relation IDs and current arrow type during sibling calculations, replacing prior type-mismatch filtering with broader exclusion criteria. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
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/canvas/DiscourseRelationShape/DiscourseRelationBindings.tsx (2)
233-234: Consider potential performance impact of repeated calls.The function
getDiscourseRelations()is called every timereparentArrowexecutes. If reparenting occurs frequently or ifgetDiscourseRelations()is expensive, consider caching the relation IDs at a higher scope.However, since this change fixes a critical cascading update bug, the current implementation is acceptable as-is.
💡 Optional optimization: cache relation IDs
If performance becomes a concern, consider caching the relation IDs:
+// Cache relation IDs at module level or in a useMemo/useCallback if in a component +const cachedRelationIds = new Set(getDiscourseRelations().map((r) => r.id)); + // eslint-disable-next-line preferArrows/prefer-arrow-functions function reparentArrow(editor: Editor, arrowId: TLShapeId) { // ... existing code ... - const relationIds = new Set(getDiscourseRelations().map((r) => r.id)); + const relationIds = new Set(cachedRelationIds); relationIds.add(arrow.type);Note: Only implement if profiling shows this is a bottleneck.
251-268: Consider renaming variable for clarity.The variable
nextHighestNonArrowSiblingnow represents the next sibling that is neither an arrow nor a relation (as correctly noted in the comment on lines 236-238). The name could be updated to reflect this broader exclusion.💡 Optional: rename for clarity
- const nextHighestNonArrowSibling = higherSiblings[0]; + const nextHighestNonRelationSibling = higherSiblings[0]; if ( // ...then, if we're above the last shape we want to be above... reparentedArrow.index > highestSibling.index && // ...but below the next non-arrow sibling... - reparentedArrow.index < nextHighestNonArrowSibling.index + reparentedArrow.index < nextHighestNonRelationSibling.index ) { // ...then we're already in the right place. no need to update! return; } // otherwise, we need to find the index between the highest sibling // we want to be above, and the next highest sibling we want to be // below: finalIndex = getIndexBetween( highestSibling.index, - nextHighestNonArrowSibling.index, + nextHighestNonRelationSibling.index, );Alternatively:
nextHighestNonRelatedSiblingornextNonBoundSibling
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationBindings.tsx(3 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/DiscourseRelationBindings.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/DiscourseRelationBindings.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/DiscourseRelationBindings.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/DiscourseRelationBindings.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/DiscourseRelationBindings.tsx
🧠 Learnings (1)
📚 Learning: 2025-06-17T23:42:29.279Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:95-104
Timestamp: 2025-06-17T23:42:29.279Z
Learning: In the DiscourseGraphs/discourse-graph codebase, DiscourseRelation type properties are either string or Triple[], and the STANDARD_ROLES filter in conceptConversion.ts excludes Triple[] properties, so only string values remain after filtering.
Applied to files:
apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationBindings.tsx
🔇 Additional comments (1)
apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationBindings.tsx (1)
236-245: Excellent fix for the cascading reparent issue!The broadened exclusion criteria (from checking
sibling.type !== arrow.typeto checking!relationIds.has(sibling.type)) correctly prevents index conflicts with all relation bindings, not just the current arrow type. This should resolve the maximum update limit error described in the PR objectives.
|
I think relations are kinds of arrows, so the variable rename is unnecessary. |
https://linear.app/discourse-graphs/issue/ENG-1153/maximum-update-limit-error-on-canvas-after-adding-supports-relation
The issue was that the reparent arrow function looking at non-arrow siblings in a way that did not take relations into account, leading to a reparenting cascade.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.