ENG-1138 Migrate canvas shape node type to discourse-node#894
Open
ENG-1138 Migrate canvas shape node type to discourse-node#894
Conversation
- Replaced the use of createNodeShapeUtils with DiscourseNodeUtil across components to streamline discourse node management. - Introduced DISCOURSE_NODE_SHAPE_TYPE constant for consistent node type identification. - Updated various components to utilize nodeTypeId for better clarity and functionality in shape creation and manipulation. - Enhanced migration logic to accommodate new node type structure, ensuring backward compatibility and improved data integrity. This refactor improves the maintainability and readability of the codebase while ensuring that new features can be integrated more seamlessly.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+649
to
+651
| const targetNodeTypeId = getDiscourseNodeTypeId({ shape: target }); | ||
| if (targetNodeTypeId && arrow.type !== targetNodeTypeId) { | ||
| editor.updateShapes([{ id: arrow.id, type: targetNodeTypeId }]); |
Contributor
There was a problem hiding this comment.
🔴 Relation arrow type changed to unregistered node type ID, causing runtime errors
After a relation is successfully drawn between two discourse nodes, handleCreateRelationsInRoam in createAllRelationShapeUtils updates the arrow shape's type to targetNodeTypeId (e.g., "page-node"). Since arrow.type is always a relation ID (a Roam block UID) and targetNodeTypeId is always a discourse node type like "page-node", the condition arrow.type !== targetNodeTypeId is always true, so this update always executes.
In the old system this didn't crash because createNodeShapeUtils registered a ShapeUtil for each node type (e.g., "page-node"). This PR removes those per-node-type utils in favor of a single DiscourseNodeUtil with type = "discourse-node". Now "page-node" has no registered ShapeUtil, so editor.updateShapes([{ id: arrow.id, type: "page-node" }]) will cause tldraw to fail when it tries to look up the util for the shape. This breaks every relation creation between discourse nodes on the canvas.
Prompt for agents
In apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx, lines 649-651 inside createAllRelationShapeUtils's handleCreateRelationsInRoam method, the code updates the arrow shape's type to targetNodeTypeId (a discourse node type like "page-node"). This is incorrect because (1) "page-node" is no longer a registered ShapeUtil type after this PR (only "discourse-node" is), and (2) even before this PR, the intent of changing an arrow/relation shape's type to a node type was likely a bug. The arrow.type should remain a valid relation shape type, not a node type. You should either remove this type-update block entirely (lines 649-651) if it served no real purpose, or if the original intent was to update the arrow to a more specific relation ID based on the target type, fix the logic to use the correct relation ID instead of the node type ID.
Was this helpful? React with 👍 or 👎 to provide feedback.
Contributor
📝 WalkthroughWalkthroughThis PR refactors discourse node shape handling by introducing a standardized shape type constant ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/roam/src/components/canvas/useCanvasStoreAdapterArgs.ts (1)
58-65: Remove staleallNodesplumbing from shape util creation.
createShapeUtilsno longer usesallNodes, but it’s still part of its typed input, call sites, and memo deps. This introduces avoidable adapter identity churn.♻️ Proposed cleanup
const createShapeUtils = ({ - allRelationIds, - allAddReferencedNodeByAction, + allRelationIds, + allAddReferencedNodeByAction, }: { - allNodes: DiscourseNode[]; allRelationIds: string[]; allAddReferencedNodeByAction: AddReferencedNodeType; }): TLAnyShapeUtilConstructor[] => { return [ DiscourseNodeUtil, @@ const customShapeUtils = createShapeUtils({ - allNodes, allRelationIds, allAddReferencedNodeByAction, }); @@ const stableCustomShapeUtils = useMemo( () => ({ pageUid, value: createShapeUtils({ - allNodes, allRelationIds, allAddReferencedNodeByAction, }), }), - [pageUid, allNodes, allRelationIds, allAddReferencedNodeByAction], + [pageUid, allRelationIds, allAddReferencedNodeByAction], ).value;Also applies to: 99-103, 125-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/roam/src/components/canvas/useCanvasStoreAdapterArgs.ts` around lines 58 - 65, The createShapeUtils function signature and related plumbing still include the stale allNodes parameter which is unused; update the createShapeUtils definition to remove allNodes from its parameter list and typed input, update all call sites that pass allNodes to stop passing it, and remove allNodes from any memoization dependency arrays (e.g., where createShapeUtils is used in useMemo or similar). Ensure references to parameters use the remaining names (allRelationIds, allAddReferencedNodeByAction) and keep the return type TLAnyShapeUtilConstructor[] unchanged so adapter identity no longer churns due to the unused allNodes.apps/roam/src/utils/syncCanvasNodeTitlesOnLoad.ts (1)
2-5: Prefer shared node-shape helpers instead of duplicating detection logic.This segment re-implements discourse node detection and nodeType extraction inline. Switching to
isDiscourseNodeShape+getDiscourseNodeTypeIdwill keep behavior consistent with the rest of the migration-aware code paths.♻️ Proposed refactor
import type { Editor } from "tldraw"; import { - DISCOURSE_NODE_SHAPE_TYPE, + getDiscourseNodeTypeId, + isDiscourseNodeShape, type DiscourseNodeShape, } from "~/components/canvas/DiscourseNodeUtil"; @@ - const discourseNodeShapes = allRecords.filter((r) => { + const discourseNodeShapes = allRecords.filter((r): r is DiscourseNodeShape => { if (r.typeName !== "shape") return false; - if (r.type !== DISCOURSE_NODE_SHAPE_TYPE) return false; const shape = r as DiscourseNodeShape; - if (!nodeTypeSet.has(shape.props.nodeTypeId)) return false; - return typeof shape.props?.uid === "string"; - }) as DiscourseNodeShape[]; + if (!isDiscourseNodeShape(shape)) return false; + if (!nodeTypeSet.has(getDiscourseNodeTypeId({ shape }))) return false; + return typeof shape.props.uid === "string"; + });As per coding guidelines "Prefer util functions for reusable logic and common operations".
Also applies to: 70-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/roam/src/utils/syncCanvasNodeTitlesOnLoad.ts` around lines 2 - 5, Replace the inline discourse node detection and nodeType extraction in syncCanvasNodeTitlesOnLoad with the shared helpers: use isDiscourseNodeShape(...) to detect a discourse node shape instead of checking DISCOURSE_NODE_SHAPE_TYPE/DiscourseNodeShape manually, and call getDiscourseNodeTypeId(...) to extract the node type ID (update the code around the current detection/assignment at lines ~70-76). Remove or stop using the duplicated logic referencing DISCOURSE_NODE_SHAPE_TYPE/DiscourseNodeShape and import isDiscourseNodeShape and getDiscourseNodeTypeId from the shared util so behavior stays consistent with other migration-aware code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/roam/src/components/canvas/DiscourseRelationShape/discourseRelationMigrations.ts`:
- Around line 165-170: The migration's filter function is too strict because it
excludes legacy or deleted node shapes by requiring
allNodeTypes.includes(r.type); change the filter logic in the migration (the
filter handling the variable r and using allNodeTypes) to only check r.typeName
=== "shape" (or otherwise include shapes even if r.type is not present in
allNodeTypes) so that the up migration (which sets shape.props.nodeTypeId =
shape.type and shape.type = DISCOURSE_NODE_SHAPE_TYPE) runs for historical
shapes as well and assigns nodeTypeId and the unified DISCOURSE_NODE_SHAPE_TYPE.
In
`@apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx`:
- Around line 649-652: The code in DiscourseRelationUtil is incorrectly
retagging relation arrows by setting arrow.type to a node type from
getDiscourseNodeTypeId({ shape: target }) (referenced as arrow.id and
getDiscourseNodeTypeId), which can orphan relation behavior; remove or change
the update so we do NOT assign a node type to the arrow.type — either delete the
editor.updateShapes call that sets type to targetNodeTypeId or replace it with
logic that computes the proper relation shape type (e.g., via a
getDiscourseRelationTypeId or mapping function) and only update arrow.type with
a valid relation type, leaving arrow.type unchanged when no valid relation type
is found.
---
Nitpick comments:
In `@apps/roam/src/components/canvas/useCanvasStoreAdapterArgs.ts`:
- Around line 58-65: The createShapeUtils function signature and related
plumbing still include the stale allNodes parameter which is unused; update the
createShapeUtils definition to remove allNodes from its parameter list and typed
input, update all call sites that pass allNodes to stop passing it, and remove
allNodes from any memoization dependency arrays (e.g., where createShapeUtils is
used in useMemo or similar). Ensure references to parameters use the remaining
names (allRelationIds, allAddReferencedNodeByAction) and keep the return type
TLAnyShapeUtilConstructor[] unchanged so adapter identity no longer churns due
to the unused allNodes.
In `@apps/roam/src/utils/syncCanvasNodeTitlesOnLoad.ts`:
- Around line 2-5: Replace the inline discourse node detection and nodeType
extraction in syncCanvasNodeTitlesOnLoad with the shared helpers: use
isDiscourseNodeShape(...) to detect a discourse node shape instead of checking
DISCOURSE_NODE_SHAPE_TYPE/DiscourseNodeShape manually, and call
getDiscourseNodeTypeId(...) to extract the node type ID (update the code around
the current detection/assignment at lines ~70-76). Remove or stop using the
duplicated logic referencing DISCOURSE_NODE_SHAPE_TYPE/DiscourseNodeShape and
import isDiscourseNodeShape and getDiscourseNodeTypeId from the shared util so
behavior stays consistent with other migration-aware code paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dd8e4a86-2f21-4a2c-bbab-28e3ac7fee3b
📒 Files selected for processing (11)
apps/roam/src/components/Export.tsxapps/roam/src/components/canvas/Clipboard.tsxapps/roam/src/components/canvas/DiscourseNodeUtil.tsxapps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsxapps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsxapps/roam/src/components/canvas/DiscourseRelationShape/discourseRelationMigrations.tsapps/roam/src/components/canvas/DiscourseToolPanel.tsxapps/roam/src/components/canvas/Tldraw.tsxapps/roam/src/components/canvas/uiOverrides.tsxapps/roam/src/components/canvas/useCanvasStoreAdapterArgs.tsapps/roam/src/utils/syncCanvasNodeTitlesOnLoad.ts
Comment on lines
+165
to
+170
| filter: (r: any) => | ||
| r.typeName === "shape" && allNodeTypes.includes(r.type), | ||
| up: (shape: any) => { | ||
| shape.props.nodeTypeId = shape.type; | ||
| shape.type = DISCOURSE_NODE_SHAPE_TYPE; | ||
| }, |
Contributor
There was a problem hiding this comment.
Migration filter is too strict for legacy/deleted node types.
At Line 166, filtering by allNodeTypes.includes(r.type) skips historical node shapes whose type no longer exists in current config. Those shapes will never get nodeTypeId + unified discourse-node type, which breaks backward-compatibility flows for older canvases.
🔧 Proposed fix
{
id: versions["MigrateNodeTypeToDiscourseNode"],
scope: "record",
- filter: (r: any) =>
- r.typeName === "shape" && allNodeTypes.includes(r.type),
+ filter: (r: any) =>
+ r.typeName === "shape" &&
+ r.type !== DISCOURSE_NODE_SHAPE_TYPE &&
+ !allRelationShapeIds.includes(r.type) &&
+ typeof r.props?.uid === "string" &&
+ typeof r.props?.title === "string",
up: (shape: any) => {
- shape.props.nodeTypeId = shape.type;
+ if (!shape.props.nodeTypeId) shape.props.nodeTypeId = shape.type;
shape.type = DISCOURSE_NODE_SHAPE_TYPE;
},
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/roam/src/components/canvas/DiscourseRelationShape/discourseRelationMigrations.ts`
around lines 165 - 170, The migration's filter function is too strict because it
excludes legacy or deleted node shapes by requiring
allNodeTypes.includes(r.type); change the filter logic in the migration (the
filter handling the variable r and using allNodeTypes) to only check r.typeName
=== "shape" (or otherwise include shapes even if r.type is not present in
allNodeTypes) so that the up migration (which sets shape.props.nodeTypeId =
shape.type and shape.type = DISCOURSE_NODE_SHAPE_TYPE) runs for historical
shapes as well and assigns nodeTypeId and the unified DISCOURSE_NODE_SHAPE_TYPE.
Comment on lines
+649
to
652
| const targetNodeTypeId = getDiscourseNodeTypeId({ shape: target }); | ||
| if (targetNodeTypeId && arrow.type !== targetNodeTypeId) { | ||
| editor.updateShapes([{ id: arrow.id, type: targetNodeTypeId }]); | ||
| } |
Contributor
There was a problem hiding this comment.
Do not retag relation arrows with a node type ID.
At Line 651, arrow.type is overwritten with targetNodeTypeId. That value is a node type, not a relation shape type, and can orphan the arrow from relation util behavior.
🐛 Proposed fix
- const targetNodeTypeId = getDiscourseNodeTypeId({ shape: target });
- if (targetNodeTypeId && arrow.type !== targetNodeTypeId) {
- editor.updateShapes([{ id: arrow.id, type: targetNodeTypeId }]);
- }
+ const targetNodeTypeId = getDiscourseNodeTypeId({ shape: target });
+ if (targetNodeTypeId) {
+ const relationForTarget = discourseContext.relations[relation.label]
+ ?.find(
+ (r) =>
+ r.source === relation.source && r.destination === targetNodeTypeId,
+ );
+ if (relationForTarget && arrow.type !== relationForTarget.id) {
+ editor.updateShapes([{ id: arrow.id, type: relationForTarget.id }]);
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx`
around lines 649 - 652, The code in DiscourseRelationUtil is incorrectly
retagging relation arrows by setting arrow.type to a node type from
getDiscourseNodeTypeId({ shape: target }) (referenced as arrow.id and
getDiscourseNodeTypeId), which can orphan relation behavior; remove or change
the update so we do NOT assign a node type to the arrow.type — either delete the
editor.updateShapes call that sets type to targetNodeTypeId or replace it with
logic that computes the proper relation shape type (e.g., via a
getDiscourseRelationTypeId or mapping function) and only update arrow.type with
a valid relation type, leaving arrow.type unchanged when no valid relation type
is found.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[[Page]]into the canvas and confirm it creates a discourse node correctly.((block-uid))into the canvas and confirm it creates a discourse node correctly.Summary by CodeRabbit