Skip to content

ENG-1138 Migrate canvas shape node type to discourse-node #894

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.

Sign up for GitHub

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

Open
mdroidian wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ENG-1138 Migrate canvas shape node type to discourse-node #894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions apps/roam/src/components/Export.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ import {
} from "@tldraw/editor";
import calcCanvasNodeSizeAndImg from "~/utils/calcCanvasNodeSizeAndImg";
import {
createNodeShapeUtils,
DiscourseNodeUtil,
DISCOURSE_NODE_SHAPE_TYPE,
DiscourseNodeShape,
} from "~/components/canvas/DiscourseNodeUtil";
import { discourseContext, MAX_WIDTH } from "~/components/canvas/Tldraw";
Expand Down Expand Up @@ -360,7 +361,7 @@ const ExportDialog: ExportDialogComponent = ({
);

// UTILS
const discourseNodeUtils = createNodeShapeUtils(allNodes);
const discourseNodeUtils = [DiscourseNodeUtil];
const discourseRelationUtils =
createAllRelationShapeUtils(allRelationIds);
const referencedNodeUtils = createAllReferencedNodeUtils(
Expand Down Expand Up @@ -551,12 +552,13 @@ const ExportDialog: ExportDialogComponent = ({
index: currentIndex,
rotation: 0,
isLocked: false,
type: nodeType,
type: DISCOURSE_NODE_SHAPE_TYPE,
props: {
w,
h,
uid: r.uid,
title: String(r[firstColumnKey]),
nodeTypeId: nodeType,
imageUrl,
size: "s",
fontFamily: "sans",
Expand Down
4 changes: 3 additions & 1 deletion apps/roam/src/components/canvas/Clipboard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import isDiscourseNode from "~/utils/isDiscourseNode";
import {
DiscourseNodeShape,
DEFAULT_STYLE_PROPS,
DISCOURSE_NODE_SHAPE_TYPE,
FONT_SIZES,
} from "./DiscourseNodeUtil";
import { openBlockInSidebar, createBlock } from "roamjs-components/writes";
Expand Down Expand Up @@ -658,7 +659,7 @@ const ClipboardPageSection = ({
const shapeId = createShapeId();
const shape = {
id: shapeId,
type: nodeType.type,
type: DISCOURSE_NODE_SHAPE_TYPE,
x: pagePoint.x - w / 2,
y: pagePoint.y - h / 2,
props: {
Expand All @@ -669,6 +670,7 @@ const ClipboardPageSection = ({
imageUrl,
size: "s" as TLDefaultSizeStyle,
fontFamily: "sans" as TLDefaultFontStyle,
nodeTypeId: nodeType.type,
},
};
editor.createShape<DiscourseNodeShape>(shape);
Expand Down
84 changes: 48 additions & 36 deletions apps/roam/src/components/canvas/DiscourseNodeUtil.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import {
TLBaseShape,
useEditor,
DefaultColorStyle,
Editor,
createShapeId,
TLDefaultHorizontalAlignStyle,
TLDefaultVerticalAlignStyle,
Expand All @@ -16,6 +15,7 @@ import {
DefaultSizeStyle,
T,
FONT_FAMILIES,
TLShape,
TLDefaultFontStyle,
DefaultFontStyle,
toDomPrecision,
Expand Down Expand Up @@ -92,6 +92,8 @@ export const COLOR_PALETTE: Record<string, string> = {
};
/* eslint-disable @typescript-eslint/naming-convention */

export const DISCOURSE_NODE_SHAPE_TYPE = "discourse-node";

const getRelationIds = () =>
new Set(
Object.values(discourseContext.relations).flatMap((rs) =>
Expand All @@ -106,7 +108,7 @@ export const createNodeShapeTools = (
return class DiscourseNodeTool extends StateNode {
static id = n.type;
static initial = "idle";
shapeType = n.type;
nodeTypeId = n.type;

override onEnter = () => {
this.editor.setCursor({
Expand All @@ -120,10 +122,14 @@ export const createNodeShapeTools = (
const shapeId = createShapeId();
this.editor.createShape({
id: shapeId,
type: this.shapeType,
type: DISCOURSE_NODE_SHAPE_TYPE,
x: currentPagePoint.x,
y: currentPagePoint.y,
props: { fontFamily: "sans", size: "s" },
props: {
fontFamily: "sans",
size: "s",
nodeTypeId: this.nodeTypeId,
},
});
this.editor.setEditingShape(shapeId);
this.editor.setCurrentTool("select");
Expand All @@ -132,23 +138,24 @@ export const createNodeShapeTools = (
});
};

export const createNodeShapeUtils = (nodes: DiscourseNode[]) => {
return nodes.map((node) => {
class DiscourseNodeUtil extends BaseDiscourseNodeUtil {
constructor(editor: Editor) {
super(editor, node.type);
}
static override type = node.type; // removing this gives undefined error
// getDefaultProps(): DiscourseNodeShape["props"] {
// const baseProps = super.getDefaultProps();
// return {
// ...baseProps,
// color: node.color,
// };
// }
}
return DiscourseNodeUtil;
});
type ShapeWithOptionalNodeTypeId = TLShape & {
props?: {
nodeTypeId?: string;
};
};

export const getDiscourseNodeTypeId = ({
shape,
}: {
shape: ShapeWithOptionalNodeTypeId;
}): string => {
return shape.props?.nodeTypeId || shape.type;
};

export const isDiscourseNodeShape = (
shape: TLShape,
): shape is DiscourseNodeShape => {
return shape.type === DISCOURSE_NODE_SHAPE_TYPE;
};

export type DiscourseNodeShape = TLBaseShape<
Expand All @@ -159,25 +166,22 @@ export type DiscourseNodeShape = TLBaseShape<
// opacity: TLOpacityType;
uid: string;
title: string;
nodeTypeId: string;
imageUrl?: string;
size: TLDefaultSizeStyle;
fontFamily: TLDefaultFontStyle;
}
>;
export class BaseDiscourseNodeUtil extends BaseBoxShapeUtil<DiscourseNodeShape> {
type: string;

constructor(editor: Editor, type: string) {
super(editor);
this.type = type;
}
export class DiscourseNodeUtil extends BaseBoxShapeUtil<DiscourseNodeShape> {
static override type = DISCOURSE_NODE_SHAPE_TYPE;

static override props = {
w: T.number,
h: T.number,
// opacity: T.number,
uid: T.string,
title: T.string,
nodeTypeId: T.string,
imageUrl: T.optional(T.string),
size: DefaultSizeStyle,
fontFamily: DefaultFontStyle,
Expand All @@ -194,6 +198,7 @@ export class BaseDiscourseNodeUtil extends BaseBoxShapeUtil<DiscourseNodeShape>
h: 64,
uid: window.roamAlphaAPI.util.generateUID(),
title: "",
nodeTypeId: "",
size: "s",
fontFamily: "sans",
};
Expand Down Expand Up @@ -239,7 +244,11 @@ export class BaseDiscourseNodeUtil extends BaseBoxShapeUtil<DiscourseNodeShape>
const nodesInCanvas = Object.fromEntries(
allRecords
.filter((r): r is DiscourseNodeShape => {
return r.typeName === "shape" && nodeIds.has(r.type);
if (r.typeName !== "shape") return false;
const nodeTypeId = getDiscourseNodeTypeId({ shape: r });
return (
r.typeName === "shape" && !!nodeTypeId && nodeIds.has(nodeTypeId)
);
})
.map((r) => [r.props.uid, r] as const),
);
Expand Down Expand Up @@ -330,12 +339,14 @@ export class BaseDiscourseNodeUtil extends BaseBoxShapeUtil<DiscourseNodeShape>
editor.createShapes(shapesToCreate).createBindings(bindingsToCreate);
}

getColors() {
return getDiscourseNodeColors({ nodeType: this.type });
getColors(shape: DiscourseNodeShape) {
return getDiscourseNodeColors({
nodeType: getDiscourseNodeTypeId({ shape }),
});
}

async toSvg(shape: DiscourseNodeShape): Promise<JSX.Element> {
const { backgroundColor, textColor } = this.getColors();
const { backgroundColor, textColor } = this.getColors(shape);
const padding = Number(DEFAULT_STYLE_PROPS.padding.replace("px", ""));
const props = shape.props;
const bounds = new Box(0, 0, props.w, props.h);
Expand Down Expand Up @@ -432,7 +443,7 @@ export class BaseDiscourseNodeUtil extends BaseBoxShapeUtil<DiscourseNodeShape>
const extensionAPI = useExtensionAPI();
const {
canvasSettings: { alias = "", "key-image": isKeyImage = "" } = {},
} = discourseContext.nodes[shape.type] || {};
} = discourseContext.nodes[getDiscourseNodeTypeId({ shape })] || {};
// eslint-disable-next-line react-hooks/rules-of-hooks
const isOverlayEnabled = useMemo(
() => getSetting(DISCOURSE_CONTEXT_OVERLAY_IN_CANVAS_KEY, false),
Expand Down Expand Up @@ -465,7 +476,7 @@ export class BaseDiscourseNodeUtil extends BaseBoxShapeUtil<DiscourseNodeShape>
}
}, [setLoaded, loaded, contentRef, shape.props.uid]);

const { backgroundColor, textColor } = this.getColors();
const { backgroundColor, textColor } = this.getColors(shape);

// eslint-disable-next-line react-hooks/rules-of-hooks
useEffect(() => {
Expand All @@ -482,15 +493,15 @@ export class BaseDiscourseNodeUtil extends BaseBoxShapeUtil<DiscourseNodeShape>
const { h, w, imageUrl } = await calcCanvasNodeSizeAndImg({
nodeText: text,
uid,
nodeType: this.type,
nodeType: getDiscourseNodeTypeId({ shape }),
extensionAPI,
});
this.updateProps(shape.id, shape.type, { h, w, imageUrl });
};

renderModifyNodeDialog({
mode: isCreating ? "create" : "edit",
nodeType: shape.type,
nodeType: getDiscourseNodeTypeId({ shape }),
initialValue: { text: shape.props.title, uid: shape.props.uid },
// Only pass it when editing an existing node that has a valid Roam block UID
sourceBlockUid:
Expand All @@ -514,6 +525,7 @@ export class BaseDiscourseNodeUtil extends BaseBoxShapeUtil<DiscourseNodeShape>
this.updateProps(shape.id, shape.type, {
title: text,
uid,
nodeTypeId: getDiscourseNodeTypeId({ shape }),
});

const autoCanvasRelations = getSetting<boolean>(
Expand Down
11 changes: 9 additions & 2 deletions apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
} from "./DiscourseRelationUtil";
import { discourseContext } from "~/components/canvas/Tldraw";
import { dispatchToastEvent } from "~/components/canvas/ToastListener";
import { getDiscourseNodeTypeId } from "~/components/canvas/DiscourseNodeUtil";

export type AddReferencedNodeType = Record<string, ReferenceFormatType[]>;
type ReferenceFormatType = {
Expand Down Expand Up @@ -77,7 +78,10 @@ export const createAllReferencedNodeTools = (

const sourceType = allAddReferencedNodeByAction[action][0].sourceType;
const sourceName = allAddReferencedNodeByAction[action][0].sourceName;
if (target?.type === sourceType) {
if (
target &&
getDiscourseNodeTypeId({ shape: target }) === sourceType
) {
this.shapeType = `${action}`;
} else {
this.cancelAndWarn(`Starting node must be one of ${sourceName}`);
Expand Down Expand Up @@ -362,8 +366,11 @@ export const createAllRelationShapeTools = (
// }
);

const targetNodeTypeId = target
? getDiscourseNodeTypeId({ shape: target })
: undefined;
const relation = discourseContext.relations[name].find(
(r) => r.source === target?.type,
(r) => r.source === targetNodeTypeId,
);
if (relation) {
this.shapeType = relation.id;
Expand Down
26 changes: 18 additions & 8 deletions apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,10 @@ import createPage from "roamjs-components/writes/createPage";
import openBlockInSidebar from "roamjs-components/writes/openBlockInSidebar";
import triplesToBlocks from "~/utils/triplesToBlocks";
import {
BaseDiscourseNodeUtil,
DiscourseNodeUtil,
DiscourseNodeShape,
getDiscourseNodeTypeId,
isDiscourseNodeShape as isDiscourseNodeShapeTypeGuard,
} from "~/components/canvas/DiscourseNodeUtil";
import getPageTitleByPageUid from "roamjs-components/queries/getPageTitleByPageUid";
import { AddReferencedNodeType } from "./DiscourseRelationTool";
Expand Down Expand Up @@ -146,8 +148,7 @@ export const createAllReferencedNodeUtils = (
static override type = action;

isDiscourseNodeShape(shape: TLShape): shape is DiscourseNodeShape {
const shapeUtil = this.editor.getShapeUtil(shape.type);
return shapeUtil instanceof BaseDiscourseNodeUtil;
return isDiscourseNodeShapeTypeGuard(shape);
}

handleCreateRelationsInRoam = async ({
Expand Down Expand Up @@ -177,7 +178,11 @@ export const createAllReferencedNodeUtils = (
const possibleTargets = allAddReferencedNodeByAction[arrow.type].map(
(action) => action.destinationType,
);
if (!possibleTargets.includes(target.type)) {
if (
!possibleTargets.includes(
getDiscourseNodeTypeId({ shape: target }) || "",
)
) {
return deleteAndWarn(
`Target node must be of type ${possibleTargets
.map((t) => discourseContext.nodes[t].text)
Expand Down Expand Up @@ -587,7 +592,7 @@ const asDiscourseNodeShape = (
editor: Editor,
): DiscourseNodeShape | null => {
const shapeUtil = editor.getShapeUtil(shape.type);
return shapeUtil instanceof BaseDiscourseNodeUtil
return shapeUtil instanceof DiscourseNodeUtil
? (shape as DiscourseNodeShape)
: null;
};
Expand Down Expand Up @@ -628,7 +633,11 @@ export const createAllRelationShapeUtils = (
.filter((r) => r.source === relation.source)
.map((r) => r.destination);

if (!possibleTargets.includes(target.type)) {
if (
!possibleTargets.includes(
getDiscourseNodeTypeId({ shape: target }) || "",
)
) {
const uniqueTargets = [...new Set(possibleTargets)];
const uniqueTargetTexts = uniqueTargets.map(
(t) => discourseContext.nodes[t].text,
Expand All @@ -637,8 +646,9 @@ export const createAllRelationShapeUtils = (
`Target node must be of type ${uniqueTargetTexts.join(", ")}`,
);
}
if (arrow.type !== target.type) {
editor.updateShapes([{ id: arrow.id, type: target.type }]);
const targetNodeTypeId = getDiscourseNodeTypeId({ shape: target });
if (targetNodeTypeId && arrow.type !== targetNodeTypeId) {
editor.updateShapes([{ id: arrow.id, type: targetNodeTypeId }]);
Comment on lines +649 to +651
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

}
Comment on lines +649 to 652
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

if (getStoredRelationsEnabled()) {
const sourceAsDNS = asDiscourseNodeShape(source, editor);
Expand Down
Loading
Loading