ENG-600: Discourse Relation shape definition

[trangdoan982]

Summary by CodeRabbit

• New Features
  • Added relation arrows on Canvas to connect nodes, supporting straight and curved paths with adjustable bend and handles.
  • Arrow bindings automatically follow, update, and clean up when connected shapes move or are deleted.
  • Improved arrow labeling with smart placement, font sizing, and styling.
  • Multiple arrowhead styles available (e.g., dot, cross, triangle, diamond).
  • Enhanced export quality with embedded fonts and pattern fills for SVG/canvas exports.
  • Relation arrows are now available in the drawing UI alongside existing shapes.

[linear]

ENG-600 Discourse Relation Shape definition

[supabase]

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[trangdoan982]

• [ENG-603] Add existing relations #412
• ENG-605: Add new relation flow #411
• ENG-600: Discourse Relation shape definition #408 👈 (View in Graphite)
• tldraw-obsidian

This stack of pull requests is managed by Graphite. Learn more about stacking.

[trangdoan982]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

Caution

Review failed

An error occurred during the review process. Please try again later.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 9

🧹 Nitpick comments (20)

apps/obsidian/src/components/canvas/shapes/DiscourseRelationShape.tsx (4)

133-159: Handle missing arrow info defensively.

Non-null assertion on getArrowInfo can crash; provide a fallback to raw start/end handles.

Apply this diff:

-  const info = getArrowInfo(this.editor, shape)!;
-
-  return [
+  const info = getArrowInfo(this.editor, shape);
+  if (!info) {
+    const { start, end } = shape.props;
+    return [
+      { id: ARROW_HANDLES.START, type: "vertex", index: "a0", x: start.x, y: start.y },
+      { id: ARROW_HANDLES.MIDDLE, type: "virtual", index: "a2", x: (start.x + end.x) / 2, y: (start.y + end.y) / 2 },
+      { id: ARROW_HANDLES.END, type: "vertex", index: "a3", x: end.x, y: end.y },
+    ] as TLHandle[];
+  }
+  return [
     {
       id: ARROW_HANDLES.START,
       type: "vertex",
       index: "a0",
       x: info.start.handle.x,
       y: info.start.handle.y,
     },

---

289-328: Prefer instance method over prototype call for defaults.

Use this.getDefaultProps() to respect overrides and avoid unusual prototype access.

Apply this diff:

-      props: {
-        ...DiscourseRelationUtil.prototype.getDefaultProps(),
+      props: {
+        ...this.getDefaultProps(),
         relationTypeId,
         text: relationLabel,
         start: startPoint,
         end: endPoint,
       },

---

184-202: Curve control point could bend in the wrong axis.

Using midPoint.x + bend shifts only horizontally. For more natural bending, project along the normal of the start→end segment.

I can provide a small helper to compute a perpendicular control point if you want to refine the arc later.

---

242-254: Indicator should mirror curved geometry for accurate hit testing.

Straight-line indicator on a bent arrow underestimates the selectable area.

Consider using the same path “d” with stroke="transparent" and thicker strokeWidth.

apps/obsidian/src/components/canvas/shapes/DiscourseRelationBinding.tsx (2)

121-123: Remove stray console log.

Leftover debug logging in a hot path; please drop it.

-    console.log("deleted arrow");
-    // this.editor.deleteShape(arrow.id); // we don't want to keep the arrow
+    // this.editor.deleteShape(arrow.id); // we don't want to keep the arrow

---

131-138: Confirm deletion semantics on bound-shape removal.

Currently deletes the arrow when its bound shape is deleted. Is the desired UX “auto-unbind” instead? If deletion is correct, consider gating behind a user setting.

-  if (arrow) this.editor.deleteShape(arrow.id);
+  if (!arrow) return;
+  // Option A: unbind to keep the arrow
+  // removeArrowBinding(this.editor, arrow, binding.props.terminal);
+  // Option B: keep current behavior (delete)
+  this.editor.deleteShape(arrow.id);

apps/obsidian/src/components/canvas/utils/relationUtils.tsx (14)

851-859: Remove duplicate guard.

The second identical guard is dead code; drop it.

-  if (!intersectionPoints || intersectionPoints.length !== 2) {
-    return { start: 0.5, end: 0.5 };
-  }
-
-  // there should be two intersection points - one near the start, and one near the end
-  // after the existing guard:
-  if (!intersectionPoints || intersectionPoints.length !== 2)
-    return { start: 0.5, end: 0.5 };
+  if (!intersectionPoints || intersectionPoints.length !== 2) {
+    return { start: 0.5, end: 0.5 };
+  }

---

1125-1141: Export key namespace: use a distinct key for fill defs.

getFillDefForExport uses DefaultFontStyle.id in the key. Use a fill-specific prefix to avoid collisions with font defs.

-    key: `${DefaultFontStyle.id}:${fill}`,
+    key: `tldraw:fill:${fill}`,

---

2056-2087: Consolidate identical fill cases.

"solid", "semi", and "fill" all render the same. Consider collapsing to reduce branches.

-    case "solid": {
-      return <path fill={color} d={d} />;
-    }
-    case "semi": {
-      return <path fill={color} d={d} />;
-    }
-    case "fill": {
-      return <path fill={color} d={d} />;
-    }
+    case "solid":
+    case "semi":
+    case "fill":
+      return <path fill={color} d={d} />;

---

1700-1713: Consider relocating shared types to break mutual imports.

Both files import each other. Moving RelationBinding(s)/RelationInfo to a separate types module would eliminate the cycle and allow deduping utilities like updateArrowTerminal.

---

1955-2005: Mask id stability on Safari workaround: verify for collisions.

maskId uses shape.id + changeIndex; ensure it stays unique across pages and sessions. Consider prefixing with a namespace.

-  const maskId = (shape.id + "_clip_" + changeIndex).replace(":", "_");
+  const maskId = (`dg_rel_clip_${shape.id}_${changeIndex}`).replace(":", "_");

---

1602-1608: Dead code: isLegacyAlign unused.

Remove if unused to keep the module lean.

-function isLegacyAlign(align: TLDefaultHorizontalAlignStyle | string): boolean {
-  return (
-    align === "start-legacy" ||
-    align === "middle-legacy" ||
-    align === "end-legacy"
-  );
-}
+// (remove if not needed)

---

1422-1436: Revoke data URLs for default pixels on teardown.

getDefaultPixels creates long-lived data URLs. If re-initialized often (e.g., HMR/tests), consider a teardown path to avoid leaks.

---

1570-1593: Marker ids should be namespaced.

Plain ids like arrowhead-dot/arrowhead-cross can collide if multiple canvases exist. Namespace with a stable prefix.

-      id="arrowhead-dot"
+      id="dg-arrowhead-dot"
...
-      id="arrowhead-cross"
+      id="dg-arrowhead-cross"

---

1641-1650: Binding cleanup robustness.

When deleting existing bindings, ensure you don't pass an empty array or already-deleted ids; defensive guard avoids multi-player races.

-  editor.deleteBindings(existing);
+  if (existing.length) editor.deleteBindings(existing);

---

1673-1699: Idempotent create-or-update binding.

Between read/update, the binding may change in multiplayer. Wrap in a small retry or re-read before updateBinding.

---

1256-1267: Line-break detection in SVG spans may break with fractional y.

Comparing box.y > currentLineTop without epsilon can mis-detect lines at high zoom. Consider epsilon threshold.

-    const didBreakLine = currentLineTop !== null && box.y > currentLineTop;
+    const didBreakLine =
+      currentLineTop !== null && box.y - currentLineTop > 0.5;

---

1841-1864: approximately duplicates tldraw util name.

This local export shadows the approximately you import/use elsewhere. Consider renaming to avoid confusion.

---

1501-1537: Pattern image object URLs: ensure cleanup on each render cycle.

Cleanup already revokes on unmount; also revoke and replace when DPR/zoom changes to prevent leaks during long sessions.

---

1715-1724: mapObjectMapValues: potential perf nit.

Micro: can use for..in to avoid interim arrays from Object.entries for large objects. Not critical.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between e587380 and 3b90e93.

⛔ Files ignored due to path filters (1)

• apps/obsidian/package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (6)

• apps/obsidian/src/components/canvas/TldrawViewComponent.tsx (2 hunks)
• apps/obsidian/src/components/canvas/shapes/DiscourseRelationBinding.tsx (1 hunks)
• apps/obsidian/src/components/canvas/shapes/DiscourseRelationShape.tsx (1 hunks)
• apps/obsidian/src/components/canvas/utils/relationUtils.tsx (1 hunks)
• apps/obsidian/src/components/canvas/utils/tldraw.ts (2 hunks)
• apps/obsidian/src/utils/generateUid.ts (1 hunks)

🧰 Additional context used

🪛 GitHub Actions: CI

apps/obsidian/src/utils/generateUid.ts

[error] 1-1: TypeScript error TS2595: 'nanoid' can only be imported by using a default import. (Command: tsc --noEmit --skipLibCheck)

🪛 Biome (2.1.2)

apps/obsidian/src/components/canvas/utils/relationUtils.tsx

[error] 1785-1785: This code will never be reached ...

... because this statement will throw an exception beforehand

(lint/correctness/noUnreachable)

---

[error] 555-555: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.

For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level

(lint/correctness/useHookAtTopLevel)

🔇 Additional comments (3)

apps/obsidian/src/components/canvas/utils/tldraw.ts (1)

23-23: Registering DiscourseRelationUtil during store init looks correct.

Order (Node before Relation) is sensible for binding dependencies. No issues.

Also applies to: 50-51

apps/obsidian/src/components/canvas/TldrawViewComponent.tsx (1)

35-35: Good: DiscourseRelationUtil configured in the editor’s shape utils.

Mirrors processInitialData registration; consistent integration.

Also applies to: 63-67

apps/obsidian/src/components/canvas/utils/relationUtils.tsx (1)

588-590: Log-scale pattern LOD: confirm perceptual step.

Math.ceil(log2(max(1, zoom))) can jump aggressively at low zooms; verify intended for Obsidian canvases.

[mdroidian]

For code that we copy/pasted from tldraw, we should make a note of that and link to it. Looks like I missed this for roam (I'll go back and add it).

Biggest blockers are the migrations and the package-lock.json doesn't seem right.

[mdroidian]

.tsx if you are returning react jsx. Just .ts for helper utils.

[trangdoan982]

lots of functions in this utils return react jsx components (eg: ArrowheadCrossDef), so i needed to switch it to tsx

[mdroidian]

I'm going to assume all of this is copy/pasted from tldraw arrows (with minor edits for our relation arrows)? If not, let me know. If so, let's make a note of that and link directly to it where possible.

[trangdoan982]

yes it was all copied and slightly modified to fit our shape. I will make a comment

[mdroidian]