ENG-62 - Upgrade Tldraw #235
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
📝 WalkthroughWalkthroughDependencies switch from @tldraw/* canary builds to tldraw 2.3.0 and remove Supabase libs. Extensive patches modify tldraw/editor/ui behavior and exports. Roam canvas is refactored to a TLStore-based model with new shape/binding utils, tools, UI overrides, migrations, store sync hook, and dialogs. Legacy utilities are removed. Minor CSS and utility updates added. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Roam UI
participant TD as Tldraw Editor
participant RS as useRoamStore
participant Roam as roamAlphaAPI
UI->>TD: Mount editor with custom shape/tools/bindings
TD->>RS: Request initial TLStore
RS->>Roam: Read block props (roamjs-query-builder)
Roam-->>RS: Return stored TLStore or legacy data
RS-->>TD: Provide TLStore (or needsUpgrade)
TD->>TD: User edits canvas (shapes/bindings)
TD-->>RS: Store change (user-origin)
RS->>Roam: Throttled update props with snapshot
Roam-->>RS: PullWatch remote changes
RS->>TD: Apply diff to local store
sequenceDiagram
participant User
participant TD as Tldraw Editor
participant UI as CustomContextMenu/ConvertToDialog
participant Roam as Roam APIs
User->>TD: Right-click or open dialog on shape
TD-->>UI: Selected shape (text or image)
UI->>Roam: Create/Update Discourse node/page
UI->>TD: Replace shape with DiscourseNodeShape
TD->>TD: Update canvas and bindings
sequenceDiagram
participant User
participant Tool as Relation Tool
participant TD as Editor
participant Bind as Binding Utils
User->>Tool: Select relation tool
Tool->>TD: Start arrow at source node
User->>TD: Drag to target node
TD->>Bind: Create/update bindings (start/end)
Bind-->>TD: Update arrow terminals/bend
User->>TD: Release pointer
TD->>TD: Finalize relation shape
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
mdroidian
force-pushed
the
eng-62-upgrade-tldraw
branch
2 times, most recently
from
2f87ccf to
c0c2eeb
Compare
mdroidian
force-pushed
the
eng-62-upgrade-tldraw
branch
from
330acfe to
91701bb
Compare
mdroidian
force-pushed
the
eng-62-upgrade-tldraw
branch
from
8d64053 to
b542cfe
Compare
…ad of "select.idle"
…on and name variables
…nUtil to display unique node types for better clarity
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
🔭 Outside diff range comments (4)
apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (1)
323-342: Off-by-one when indexing into COLOR_ARRAY
discourseNodeIndex < COLOR_ARRAY.length - 1skips the last palette entry. Use< COLOR_ARRAY.lengthto include the last color.Apply:
- COLOR_ARRAY[ - discourseNodeIndex >= 0 && discourseNodeIndex < COLOR_ARRAY.length - 1 - ? discourseNodeIndex - : 0 - ]; + COLOR_ARRAY[ + discourseNodeIndex >= 0 && discourseNodeIndex < COLOR_ARRAY.length + ? discourseNodeIndex + : 0 + ];apps/roam/src/components/Export.tsx (1)
288-297: Bug: Bottom bound is computed with subtraction; should be y + hCanvas coordinates are top-left origin; bottom should be
y + h. Usingy - hgives incorrect bounds, which skews placement of new shapes.Apply this diff:
- shapes.forEach((shape) => { - let rightX = shape.x + shape.w; - let leftX = shape.x; - let topY = shape.y; - let bottomY = shape.y - shape.h; + shapes.forEach((shape) => { + const rightX = shape.x + shape.w; + const leftX = shape.x; + const topY = shape.y; + const bottomY = shape.y + shape.h;apps/roam/src/components/canvas/LabelDialog.tsx (1)
124-155: Escape dynamic strings used in RegExp to avoid crashes and wrong matches
referencedNode.nameanddiscourseContext.nodes[*].textare interpolated intonew RegExp(...)without escaping. Titles likeC++,A(B), or[Draft]will either throw or match incorrectly.Apply this diff to introduce an escape helper and use it in all dynamic regexes:
import React, { useRef, useState, useMemo, useEffect, useCallback, } from "react"; @@ const setValue = useCallback( (r: Result) => { @@ if ( referencedNode && - new RegExp(referencedNode.name, "i").test(val) && + new RegExp(escapeRegExp(referencedNode.name), "i").test(val) && isAddReferencedNode ) return referencedNodeValue; @@ ], ); const setValueFromReferencedNode = useCallback( (r: Result) => { @@ } else { const pageName = format.replace(/{([\w\d-]*)}/g, (_, val) => { if (/content/i.test(val)) return content; - if (new RegExp(referencedNode.name, "i").test(val)) + if (new RegExp(escapeRegExp(referencedNode.name), "i").test(val)) return `[[${r.text}]]`; return ""; }); setLabel(pageName); } @@ <Label>{referencedNode?.name}</Label> @@ placeholder={ - isLoading ? "..." : `Enter a ${referencedNode?.name} ...` + isLoading ? "..." : `Enter a ${referencedNode?.name} ...` } @@ - const referencedNode = Object.values(discourseContext.nodes).find( - ({ text }) => new RegExp(text, "i").test(val), - ); + const referencedNode = Object.values(discourseContext.nodes).find( + ({ text }) => new RegExp(escapeRegExp(text), "i").test(val), + );Add this helper near the top of the file (outside components):
const escapeRegExp = (s: string) => s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");Also applies to: 156-181, 268-286, 335-342
apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationBindings.tsx (1)
297-404: Deduplicate: reuse updateArrowTerminal/intersectLineSegmentCircle from helpersThis file re-implements
updateArrowTerminalandintersectLineSegmentCircle, which already exist in helpers.tsx. Keeping two copies risks drift and subtle bugs.
- Remove the duplicated implementations here and import
updateArrowTerminalfrom./helpers.- Keep a single implementation of
intersectLineSegmentCirclein helpers only.Apply this diff to remove the duplicates in this file:
-// eslint-disable-next-line preferArrows/prefer-arrow-functions -export function updateArrowTerminal({ - editor, - arrow, - terminal, - unbind = false, - useHandle = false, -}: { - editor: Editor; - arrow: DiscourseRelationShape; - terminal: "start" | "end"; - unbind?: boolean; - useHandle?: boolean; -}) { - const info = getArrowInfo(editor, arrow); - if (!info) { - throw new Error("expected arrow info"); - } - - const startPoint = useHandle ? info.start.handle : info.start.point; - const endPoint = useHandle ? info.end.handle : info.end.point; - const point = terminal === "start" ? startPoint : endPoint; - - const update = { - id: arrow.id, - type: arrow.type, - props: { [terminal]: { x: point.x, y: point.y }, bend: arrow.props.bend }, - } satisfies TLShapePartial<DiscourseRelationShape>; - - // fix up the bend: - if (!info.isStraight) { - // find the new start/end points of the resulting arrow - const newStart = terminal === "start" ? startPoint : info.start.handle; - const newEnd = terminal === "end" ? endPoint : info.end.handle; - const newMidPoint = Vec.Med(newStart, newEnd); - - // intersect a line segment perpendicular to the new arrow with the old arrow arc to - // find the new mid-point - const lineSegment = Vec.Sub(newStart, newEnd) - .per() - .uni() - .mul(info.handleArc.radius * 2 * Math.sign(arrow.props.bend)); - - // find the intersections with the old arrow arc: - const intersections = intersectLineSegmentCircle( - info.handleArc.center, - Vec.Add(newMidPoint, lineSegment), - info.handleArc.center, - info.handleArc.radius, - ); - - assert(intersections?.length === 1); - const bend = - Vec.Dist(newMidPoint, intersections[0]) * Math.sign(arrow.props.bend); - // use `approximately` to avoid endless update loops - if (!approximately(bend, update.props.bend)) { - update.props.bend = bend; - } - } - - editor.updateShape(update); - if (unbind) { - removeArrowBinding(editor, arrow, terminal); - } -} - -// eslint-disable-next-line preferArrows/prefer-arrow-functions, max-params -function intersectLineSegmentCircle( - a1: VecLike, - a2: VecLike, - c: VecLike, - r: number, -) { - const a = (a2.x - a1.x) * (a2.x - a1.x) + (a2.y - a1.y) * (a2.y - a1.y); - const b = 2 * ((a2.x - a1.x) * (a1.x - c.x) + (a2.y - a1.y) * (a1.y - c.y)); - const cc = - c.x * c.x + - c.y * c.y + - a1.x * a1.x + - a1.y * a1.y - - 2 * (c.x * a1.x + c.y * a1.y) - - r * r; - const deter = b * b - 4 * a * cc; - - if (deter < 0) return null; // outside - if (deter === 0) return null; // tangent - - const e = Math.sqrt(deter); - const u1 = (-b + e) / (2 * a); - const u2 = (-b - e) / (2 * a); - - if ((u1 < 0 || u1 > 1) && (u2 < 0 || u2 > 1)) { - return null; // outside or inside - // if ((u1 < 0 && u2 < 0) || (u1 > 1 && u2 > 1)) { - // return null // outside - // } else return null // inside' - } - - const result: VecLike[] = []; - - if (0 <= u1 && u1 <= 1) result.push(Vec.Lrp(a1, a2, u1)); - if (0 <= u2 && u2 <= 1) result.push(Vec.Lrp(a1, a2, u2)); - - if (result.length === 0) return null; // no intersection - - return result; -}And add this import (outside the selected range) to use the canonical implementation:
// at lines 27-32, extend the helpers import: import { assert, getArrowBindings, getArrowInfo, removeArrowBinding, updateArrowTerminal, } from "./helpers";
🧹 Nitpick comments (32)
local/clean-install.ps1 (3)
2-4: Be cautious: wide recursive deletion can be slow and noisy; add basic error handling and confirmationCurrent approach nukes every node_modules in the repo tree with errors suppressed. Recommend adding a confirmation flag/param and surfacing failures to avoid silent partial cleanups.
Proposed refinement:
-Get-ChildItem -Path . -Directory -Recurse -Force -Filter "node_modules" | ForEach-Object { - Remove-Item -Path $_.FullName -Recurse -Force -ErrorAction SilentlyContinue -} +param( + [switch]$Yes +) + +if (-not $Yes) { + Write-Host "About to remove ALL node_modules folders recursively." -ForegroundColor Yellow + Write-Host "Re-run with -Yes to confirm." -ForegroundColor Yellow + exit 1 +} + +try { + Get-ChildItem -Path . -Directory -Recurse -Force -Filter "node_modules" | + ForEach-Object { + Write-Host "Removing $($_.FullName)" + Remove-Item -LiteralPath $_.FullName -Recurse -Force -ErrorAction Stop + } +} catch { + Write-Error "Failed to remove some node_modules: $($_.Exception.Message)" + exit 1 +}
8-9: Remove nested package-lock.json as well (monorepo-friendly)Only removing the root lock file can leave stale locks in subpackages/apps.
Proposed change:
-Remove-Item -Path .\package-lock.json -Force -ErrorAction SilentlyContinue -Write-Host "Removed package-lock.json" +Get-ChildItem -Path . -Filter "package-lock.json" -File -Recurse -Force -ErrorAction SilentlyContinue | + ForEach-Object { + Remove-Item -LiteralPath $_.FullName -Force -ErrorAction SilentlyContinue + } +Write-Host "Removed all package-lock.json files"
12-13: Check npm install exit code; optionally support npm ci when a lockfile is presentThe script currently prints success even if npm install fails. Also, when a lockfile exists, npm ci is more reproducible.
Suggested improvement:
-# Run npm install -npm install -Write-Host "Installed dependencies" +# Run install (prefer npm ci if a lock exists) +$lock = Get-ChildItem -Path . -Filter "package-lock.json" -File -Depth 0 -ErrorAction SilentlyContinue +if ($lock) { + npm ci +} else { + npm install +} +if ($LASTEXITCODE -ne 0) { + Write-Error "Dependency installation failed with exit code $LASTEXITCODE" + exit $LASTEXITCODE +} +Write-Host "Installed dependencies"Note: If you intend to always regenerate the lock, keep npm install; otherwise, use the conditional approach above.
apps/roam/src/components/canvas/tldrawStyles.ts (1)
39-44: Keyboard shortcut styling reset may conflict with app-wide themingResetting kbd.tlui-kbd styles to
initialcan ignore dark/light theme variables. If this is only needed inside the canvas, scope it similarly to the container to avoid theme inconsistencies.- kbd.tlui-kbd { + #roamjs-tldraw-canvas-container kbd.tlui-kbd { background-color: initial; box-shadow: initial; border-radius: initial; padding: initial; }apps/roam/src/data/legacyTldrawSchema.ts (2)
2-54: Freeze LEGACY_SCHEMA for immutability and stronger typingMark the object as deeply readonly to prevent accidental runtime mutation and improve inference.
-export const LEGACY_SCHEMA = { +export const LEGACY_SCHEMA = { schemaVersion: 1 as const, storeVersion: 1, recordVersions: { ... }, -}; +} as const;
56-463: Large test store blob: consider moving to a dev/test-only fixture or lazy importThis constant is substantial and will bloat the main bundle if imported in production paths. If it's only used for testing/migration demos, move it to a test fixture, guard behind a dynamic import, or export from a separate chunk to avoid penalizing runtime consumers.
Potential patterns:
- Place under a
__fixtures__directory and import only in tests/dev tools.export async function loadLegacyStoreTest() { return (await import('./legacyTldrawStoreFixture')).LEGACY_STORE_TEST }- Annotate imports so bundlers can tree-shake in production builds.
apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (1)
510-548: Editing flow is solid; minor race guard on async updatesAfter onSuccess, multiple async calls (roam updates, size calc, relations) can interleave if a user triggers edits quickly. Not critical, but consider disabling the dialog buttons until the sequence completes or guarding with a local "busy" state.
E.g., set a local isSaving state and gate onClose/setEditingShape until finished.
apps/roam/src/components/canvas/CanvasDrawer.tsx (2)
23-24: Missing dependency in useMemo
useMemo(() => getPageTitleByPageUid(pageUid), []);should includepageUidto be correct if the drawer is ever reused for a different page without remount.- const pageTitle = useMemo(() => getPageTitleByPageUid(pageUid), []); + const pageTitle = useMemo(() => getPageTitleByPageUid(pageUid), [pageUid]);
159-164: Filter to shape records explicitly to avoid false positivesThe store contains many record types; filtering only on the presence of
props.uidis usually fine, but being explicit avoids edge cases if any non-shape record happens to have apropsfield.- const shapes = Object.values(store).filter((s) => { - const shape = s as TLBaseShape<string, { uid: string }>; - const uid = shape.props?.uid; - return !!uid; - }) as DiscourseNodeShape[]; + const shapes = Object.values(store).filter((s) => { + const any = s as Record<string, unknown>; + if (any.typeName !== "shape") return false; + const shape = s as TLBaseShape<string, { uid: string }>; + return !!shape.props?.uid; + }) as DiscourseNodeShape[];apps/roam/src/components/canvas/ToastListener.tsx (2)
13-42: Make the listener type-safe and simplerAvoid asserting a CustomEvent-typed callback to EventListener. Handle the Event and cast inside to keep DOM typing correct and reduce boilerplate.
Apply this diff:
- useEffect(() => { - const handleToastEvent = ((event: CustomEvent<TLUiToast>) => { - const { - id, - icon, - title, - description, - actions, - keepOpen, - closeLabel, - severity, - } = event.detail; - addToast({ - id, - icon, - title, - description, - actions, - keepOpen, - closeLabel, - severity, - }); - }) as EventListener; - - document.addEventListener("show-toast", handleToastEvent); - - return () => { - document.removeEventListener("show-toast", handleToastEvent); - }; - }, [addToast]); + useEffect(() => { + const handleToastEvent = (event: Event) => { + const { detail } = event as CustomEvent<TLUiToast>; + addToast(detail); + }; + document.addEventListener("show-toast", handleToastEvent as EventListener); + return () => { + document.removeEventListener("show-toast", handleToastEvent as EventListener); + }; + }, [addToast]);
4-8: Optional: namespace the event and ensure it can traverse boundaries
- Namespacing reduces collision risk with other libs.
- Setting bubbles/composed future-proofs dispatch if the target ever changes.
-export const dispatchToastEvent = (toast: TLUiToast) => { - document.dispatchEvent( - new CustomEvent<TLUiToast>("show-toast", { detail: toast }), - ); -}; +const SHOW_TOAST_EVENT = "dg:show-toast"; + +export const dispatchToastEvent = (toast: TLUiToast) => { + document.dispatchEvent( + new CustomEvent<TLUiToast>(SHOW_TOAST_EVENT, { + detail: toast, + bubbles: true, + composed: true, + }), + ); +};- document.addEventListener("show-toast", handleToastEvent as EventListener); + document.addEventListener(SHOW_TOAST_EVENT, handleToastEvent as EventListener);- document.removeEventListener("show-toast", handleToastEvent as EventListener); + document.removeEventListener(SHOW_TOAST_EVENT, handleToastEvent as EventListener);Also applies to: 37-41
apps/roam/src/utils/calcCanvasNodeSizeAndImg.ts (1)
86-89: Harden CSS px parsing and guard width math; optionally log the error
- parseFloat handles values like "12px" or numeric strings robustly.
- Guard against negative effective widths.
- Capturing the error for console diagnostics helps when investigating broken images without leaking PII.
- const padding = Number(DEFAULT_STYLE_PROPS.padding.replace("px", "")); - const maxWidth = Number(MAX_WIDTH.replace("px", "")); - const effectiveWidth = maxWidth - 2 * padding; + const padding = parseFloat(String(DEFAULT_STYLE_PROPS.padding ?? "0")); + const maxWidth = parseFloat(String(MAX_WIDTH)); + const effectiveWidth = Math.max(0, maxWidth - 2 * padding);- } catch { + } catch (e) { renderToast({ id: "tldraw-image-load-fail", content: "Failed to load image", intent: "warning", }); + // Optional: leave a breadcrumb for debugging. + // console.debug("Image load failed for URL:", imageUrl, e); return { w, h, imageUrl: "" }; }Also applies to: 96-102
apps/roam/patches/@tldraw+editor+2.3.0.patch (2)
10-24: DefaultErrorFallback: prefer React.useEffect and add composed for robust event dispatchTwo tweaks to reduce risk:
- Use React.useEffect in case the compiled file doesn’t import useEffect (avoids ReferenceError).
- Add composed: true so the event can traverse shadow DOMs if used in the future.
-+ // Notify Discourse Graphs that an error occurred -+ useEffect(() => { ++ // Notify Discourse Graphs that an error occurred ++ React.useEffect(() => { ... -+ const event = new CustomEvent("tldraw:error", { -+ detail: errorDetails, -+ bubbles: true -+ }); ++ const event = new CustomEvent("tldraw:error", { ++ detail: errorDetails, ++ bubbles: true, ++ composed: true, ++ }); ... -+ }, [error]); ++ }, [error]);
33-39: Nit: flushSync import is now unusedSince you’ve removed its usage, consider updating the patch to drop the import to avoid warnings during bundling.
Would you like me to extend this patch to remove the flushSync import in Editor.mjs as well?
apps/roam/src/components/canvas/CanvasReferences.tsx (1)
37-47: Deduplicate references to avoid double-counting during migrationBlocks may appear in both old and new sources during transition. Deduplicate by uid before setting state and count.
- const newReferences = newCanvasReferences.map((res) => ({ + const newReferences = newCanvasReferences.map((res) => ({ uid: res[0][":block/uid"] || "", text: res[0][":block/string"] || res[0][":node/title"] || "", })); const oldReferences = oldCanvasReferences.map((res) => ({ uid: res[0][":block/uid"] || "", text: res[0][":block/string"] || res[0][":node/title"] || "", })); - setReferences([...oldReferences, ...newReferences]); - setReferenceCount(oldReferences.length + newReferences.length); + const merged = [...oldReferences, ...newReferences]; + const deduped = Array.from(new Map(merged.map((r) => [r.uid, r])).values()); + setReferences(deduped); + setReferenceCount(deduped.length);apps/roam/src/components/canvas/DiscourseRelationShape/discourseRelationMigrations.ts (2)
22-23: Avoid generating duplicate migration sequences for overlapping idsIf a relation id also appears in
allAddReferencedNodeActions, you'll generate two identical sequences with the samesequenceId. Deduplicate the combined list.Apply this diff:
- const allShapeIds = [...allRelationIds, ...allAddReferencedNodeActions]; + const allShapeIds = Array.from( + new Set([...allRelationIds, ...allAddReferencedNodeActions]), + );
41-43: Preserve existing color when presentHard-coding
"black"overwrites a shape’s existing color. Default only when missing.Apply this diff:
- // TODO: migrate colors - arrow.props.color = "black"; + // TODO: migrate colors + arrow.props.color = arrow.props.color ?? "black";apps/roam/src/components/canvas/useRoamStore.ts (1)
149-201: Registeringstore.listenwithout cleanup risks leaks across remounts
_store.listenreturns an unsubscribe. Since this is insideuseMemo, it won't be disposed on unmount/recreate, which can cause duplicate listeners.Consider moving the listener registration into a
useEffectthat depends onstore, and return the unsubscribe in the cleanup:useEffect(() => { if (!store) return; const unsub = store.listen((rec) => { /* ... */ }); return () => unsub(); }, [store, pageUid]);apps/roam/src/components/Export.tsx (3)
47-51: Shape index should be unique; avoid hard-coding "a1"All new shapes use
index: "a1", which can produce unstable stacking order and unpredictable z-indexing. Generate new indices for appended shapes.Apply this diff to import
getIndicesand use it:-import { createShapeId, IndexKey, TLParentId } from "tldraw"; +import { createShapeId, IndexKey, TLParentId, getIndices } from "tldraw";- const newShape: DiscourseNodeShape = { - index: "a1" as IndexKey, // TODO does this need to be unique? + const newShape: DiscourseNodeShape = { + index: getIndices(1)[0] as IndexKey, rotation: 0, isLocked: false, type: nodeType,If you want indices to strictly follow existing siblings, you can also compute the next index based on existing shapes under
pageKey.Also applies to: 318-331
244-260: Graceful failure when no page record foundEarly
console.log("no page key")provides no feedback to the user and silently aborts. At least surface a toast or error so the user knows why "Send to Page" produced nothing.Example:
if (!pageKey) { renderToast({ id: "export-error", content: "No canvas page found.", intent: "danger" }); return; }
148-153: Duplicate logic with isCanvasPage utilityYou have a local
checkForCanvasPagewhileutils/isCanvasPage.tsalready provides the same behavior. Prefer the shared utility to avoid drift.Replace
checkForCanvasPageand the call-site with theisCanvasPageimport.Also applies to: 159-159
apps/roam/src/components/canvas/LabelDialog.tsx (1)
327-347: Duplicate logic for referenced-node detection; consider centralizing
getReferencedNodeInFormatduplicates the logic that already exists inutils/formatUtils.getReferencedNodeInFormat. Prefer reusing the shared utility to keep behavior consistent across the app.If you need slight differences, consider parameterizing the shared utility rather than copying it here.
apps/roam/src/components/canvas/Tldraw.tsx (5)
107-111: Consider memoizing theisPageUidfunction result.The
isPageUidfunction performs a Roam API pull operation each time it's called. This could cause performance issues if called frequently, such as during shape rendering or validation loops.Consider caching the results or using a memoized version:
+const pageUidCache = new Map<string, boolean>(); + export const isPageUid = (uid: string) => { + if (pageUidCache.has(uid)) { + return pageUidCache.get(uid)!; + } + const result = !!window.roamAlphaAPI.pull("[:node/title]", [":block/uid", uid])?.[ ":node/title" ]; + pageUidCache.set(uid, result); + return result; };
189-201: Type narrowing function should be type predicate.The
isDiscourseNodeShapefunction at lines 196-200 correctly narrows the type but isn't consistently used throughout the file.Consider moving this to a more accessible location and ensuring consistent usage:
- const isDiscourseNodeShape = ( - shape: TLShape, - ): shape is DiscourseNodeShape => { - return allNodes.some((node) => node.type === shape.type); - }; + // Move to top of file or utils + const createDiscourseNodeShapeGuard = (allNodes: DiscourseNode[]) => { + const nodeTypes = new Set(allNodes.map(n => n.type)); + return (shape: TLShape): shape is DiscourseNodeShape => { + return nodeTypes.has(shape.type); + }; + }; + + const isDiscourseNodeShape = useMemo( + () => createDiscourseNodeShapeGuard(allNodes), + [allNodes] + );
212-282: Complex relation creation logic could be extracted.The
handleRelationCreationfunction is quite long and handles multiple responsibilities. Consider breaking it down into smaller, more focused functions.Extract the relation validation and creation logic:
const validateRelationTarget = ( shape: TLShape | undefined, allNodes: DiscourseNode[] ): shape is DiscourseNodeShape => { return shape != null && allNodes.some(node => node.type === shape.type); }; const handleRelationCompletion = ( app: TldrawApp, relationShape: TLShape, targetShape: DiscourseNodeShape | undefined ) => { if (!targetShape) { app.deleteShapes([relationShape.id]); dispatchToastEvent({ id: "tldraw-relation-no-target", title: "Relation must connect to a node. Relation deleted.", severity: "warning", }); return; } const util = app.getShapeUtil(relationShape); if (util && typeof (util as any).handleCreateRelationsInRoam === "function") { (util as any).handleCreateRelationsInRoam({ arrow: relationShape, targetId: targetShape.id, }); } };
437-442: Error object stack assignment could be more defensive.At lines 440-442, the error stack is conditionally set, but the type assertions and error handling could be more robust.
const handleTldrawError = ( e: CustomEvent<{ message: string; stack: string | null }>, ) => { const error = new Error(e.detail.message); if (e.detail.stack) { - error.stack = e.detail.stack; + // Preserve original stack trace format + error.stack = e.detail.stack.startsWith('Error:') + ? e.detail.stack + : `Error: ${e.detail.message}\n${e.detail.stack}`; }
507-522: Loading and error states could provide more actionable information.The error state UI at lines 507-522 shows a generic message. Consider providing more specific error information or retry options.
) : !store || !assetLoading.done || !extensionAPI ? ( <div className="flex h-full items-center justify-center"> <div className="text-center"> <h2 className="mb-2 text-2xl font-semibold"> {error || assetLoading.error ? "Error Loading Canvas" : "Loading Canvas"} </h2> <p className="mb-4 text-gray-600"> {error || assetLoading.error - ? "There was a problem loading the Tldraw canvas. Please try again later." + ? `Error: ${error?.message || assetLoading.error || "Unknown error"}. Please refresh the page or contact support if the issue persists.` : ""} </p> + {(error || assetLoading.error) && ( + <button + className="rounded bg-blue-500 px-4 py-2 font-bold text-white hover:bg-blue-700" + onClick={() => window.location.reload()} + > + Refresh Page + </button> + )} </div> </div>apps/roam/patches/tldraw+2.3.0.patch (1)
30-35: Style prop type should be more specific than React.CSSProperties.The addition of
style?: React.CSSPropertiestoTLUiToolIteminterface allows any CSS properties. Consider using a more restrictive type to prevent potential styling conflicts.Consider limiting the allowed style properties:
- style?: React.CSSProperties + style?: Pick<React.CSSProperties, 'color' | 'backgroundColor' | 'opacity'>apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationBindings.tsx (3)
91-109: Null-guard arrow retrieval in lifecycle hooks
this.editor.getShape(...) as DiscourseRelationShapeassumes the arrow exists. In multiplayer or rapid delete scenarios it may beundefined. Add a quick guard to avoid calling intoarrowDidUpdatewithundefined.Example:
- arrowDidUpdate( - this.editor, - this.editor.getShape(binding.fromId) as DiscourseRelationShape, - ); + const arrow = this.editor.getShape<DiscourseRelationShape>(binding.fromId); + if (arrow) { + arrowDidUpdate(this.editor, arrow); + }
118-124: Reparent-only on target shape change may miss terminal updatesWhen the bound shape changes (
onAfterChangeToShape), you reparent but don’t update the arrow’s terminals. Consider callingarrowDidUpdateas well to ensure endpoints/bends are recomputed immediately.Example:
- reparentArrow(this.editor, binding.fromId); + reparentArrow(this.editor, binding.fromId); + const arrow = this.editor.getShape<DiscourseRelationShape>(binding.fromId); + if (arrow) arrowDidUpdate(this.editor, arrow);
144-148: Arrow deletion on bound shape delete: confirm desired UXDeleting the entire arrow when the target shape is deleted is opinionated. If partial-unbound arrows are acceptable, you could instead unbind the corresponding terminal and keep the arrow.
If the intent is to keep arrows, replace deletion with unbinding:
- if (arrow) this.editor.deleteShape(arrow.id); + if (arrow) { + updateArrowTerminal({ + editor: this.editor, + arrow, + terminal: binding.props.terminal, + unbind: true, + }) + }apps/roam/src/components/canvas/DiscourseRelationShape/helpers.tsx (1)
1719-1785: ConsolidateupdateArrowTerminalintohelpers.tsxThe standalone implementation in
DiscourseRelationBindings.tsxshould be removed so that all callers use the single source of truth inhelpers.tsx.• In
apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationBindings.tsx:
– Delete the duplicateexport function updateArrowTerminal…block (around lines 296–305).
– Add at the top:
ts import { updateArrowTerminal } from './helpers'
– Update existing calls (≈lines 134 & 165):
diff - updateArrowTerminal({ - editor: this.editor, - arrow, - terminal: handle, - unbind: true, - }) + updateArrowTerminal({ + editor: this.editor, + relation: arrow, + terminal: handle, + unbind: true, + })• In
apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx:
– Replace anyimport { updateArrowTerminal } from './DiscourseRelationBindings'with
ts import { updateArrowTerminal } from './helpers'After making these changes, rerun your grep script to confirm there’s only one definition of
updateArrowTerminal.
📜 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.
⛔ Files ignored due to path filters (2)
apps/roam/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (30)
apps/roam/package.json(3 hunks)apps/roam/patches/@tldraw+editor+2.0.0-canary.ffda4cfb.patch(0 hunks)apps/roam/patches/@tldraw+editor+2.3.0.patch(1 hunks)apps/roam/patches/@tldraw+ui+2.0.0-canary.ffda4cfb.patch(0 hunks)apps/roam/patches/tldraw+2.3.0.patch(1 hunks)apps/roam/src/components/Export.tsx(16 hunks)apps/roam/src/components/canvas/CanvasDrawer.tsx(3 hunks)apps/roam/src/components/canvas/CanvasReferences.tsx(1 hunks)apps/roam/src/components/canvas/ConvertToDialog.tsx(1 hunks)apps/roam/src/components/canvas/DiscourseNodeUtil.tsx(6 hunks)apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationBindings.tsx(1 hunks)apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx(1 hunks)apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx(1 hunks)apps/roam/src/components/canvas/DiscourseRelationShape/discourseRelationMigrations.ts(1 hunks)apps/roam/src/components/canvas/DiscourseRelationShape/helpers.tsx(1 hunks)apps/roam/src/components/canvas/DiscourseRelationsUtil.tsx(0 hunks)apps/roam/src/components/canvas/LabelDialog.tsx(17 hunks)apps/roam/src/components/canvas/Tldraw.tsx(8 hunks)apps/roam/src/components/canvas/ToastListener.tsx(1 hunks)apps/roam/src/components/canvas/tldrawStyles.ts(1 hunks)apps/roam/src/components/canvas/uiOverrides.tsx(1 hunks)apps/roam/src/components/canvas/useRoamStore.ts(1 hunks)apps/roam/src/data/legacyTldrawSchema.ts(1 hunks)apps/roam/src/styles/styles.css(0 hunks)apps/roam/src/utils/calcCanvasNodeSizeAndImg.ts(2 hunks)apps/roam/src/utils/createInitialTldrawProps.ts(0 hunks)apps/roam/src/utils/useRoamStore.ts(0 hunks)local/clean-install.ps1(1 hunks)patches/@tldraw+primitives+2.0.0-canary.ffda4cfb.patch(0 hunks)patches/@tldraw+tlschema+2.0.0-canary.ffda4cfb.patch(0 hunks)
💤 Files with no reviewable changes (8)
- patches/@tldraw+primitives+2.0.0-canary.ffda4cfb.patch
- apps/roam/src/styles/styles.css
- patches/@tldraw+tlschema+2.0.0-canary.ffda4cfb.patch
- apps/roam/patches/@tldraw+ui+2.0.0-canary.ffda4cfb.patch
- apps/roam/src/utils/useRoamStore.ts
- apps/roam/src/utils/createInitialTldrawProps.ts
- apps/roam/src/components/canvas/DiscourseRelationsUtil.tsx
- apps/roam/patches/@tldraw+editor+2.0.0-canary.ffda4cfb.patch
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API
Applied to files:
apps/roam/src/components/canvas/useRoamStore.tsapps/roam/package.json
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/package.json : Prefer existing dependencies from package.json when adding or using dependencies in the Roam Research extension
Applied to files:
apps/roam/package.json
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API
Applied to files:
apps/roam/package.json
📚 Learning: 2025-08-11T19:09:58.252Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#337
File: apps/roam/src/components/DiscourseFloatingMenu.tsx:43-43
Timestamp: 2025-08-11T19:09:58.252Z
Learning: The roam subdirectory (apps/roam) is constrained to React 17 and cannot use React 18+ features like createRoot API. ReactDOM.render should be used instead of createRoot in this subdirectory.
Applied to files:
apps/roam/package.json
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Applied to files:
apps/roam/package.json
📚 Learning: 2025-06-22T10:40:52.752Z
Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:18-31
Timestamp: 2025-06-22T10:40:52.752Z
Learning: In apps/roam/src/utils/getAllDiscourseNodesSince.ts, the user confirmed that querying for `?title` with `:node/title` and mapping it to the `text` field in the DiscourseGraphContent type is the correct implementation for retrieving discourse node content from Roam Research, despite it appearing to query page titles rather than block text content.
Applied to files:
apps/roam/src/components/Export.tsxapps/roam/src/components/canvas/LabelDialog.tsx
🧬 Code Graph Analysis (13)
apps/roam/src/components/canvas/ConvertToDialog.tsx (1)
apps/roam/src/components/canvas/uiOverrides.tsx (1)
getOnSelectForShape(107-153)
apps/roam/src/components/canvas/CanvasDrawer.tsx (1)
apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (1)
DiscourseNodeShape(147-157)
apps/roam/src/components/canvas/useRoamStore.ts (2)
apps/roam/src/data/legacyTldrawSchema.ts (1)
LEGACY_SCHEMA(2-54)apps/roam/src/utils/getBlockProps.ts (2)
normalizeProps(9-25)json(1-7)
apps/roam/src/components/canvas/uiOverrides.tsx (8)
apps/roam/src/components/canvas/ToastListener.tsx (1)
dispatchToastEvent(4-8)apps/roam/src/utils/formatUtils.ts (1)
getNewDiscourseNodeText(22-126)apps/roam/src/components/canvas/CanvasDrawer.tsx (1)
openCanvasDrawer(153-178)apps/roam/src/utils/canvasUiOverrides.ts (1)
tools(117-176)apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx (1)
AddReferencedNodeType(12-12)apps/roam/src/components/canvas/Tldraw.tsx (2)
DiscourseContextType(90-96)discourseContext(98-102)apps/roam/src/components/settings/DiscourseNodeCanvasSettings.tsx (1)
formatHexColor(16-26)apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (1)
COLOR_ARRAY(56-56)
apps/roam/src/components/canvas/DiscourseRelationShape/discourseRelationMigrations.ts (1)
apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationBindings.tsx (1)
RelationBinding(78-78)
apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx (3)
apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx (1)
DiscourseRelationShape(876-876)apps/roam/src/components/canvas/ToastListener.tsx (1)
dispatchToastEvent(4-8)apps/roam/src/components/canvas/Tldraw.tsx (1)
discourseContext(98-102)
apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx (6)
apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (2)
COLOR_ARRAY(56-56)DiscourseNodeShape(147-157)apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx (1)
AddReferencedNodeType(12-12)apps/roam/src/components/canvas/ToastListener.tsx (1)
dispatchToastEvent(4-8)apps/roam/src/components/canvas/Tldraw.tsx (2)
discourseContext(98-102)isPageUid(108-111)apps/roam/src/components/canvas/DiscourseRelationShape/helpers.tsx (7)
getArrowBindings(141-153)getArrowTerminalsInArrowSpace(407-441)removeArrowBinding(1629-1639)createOrUpdateArrowBinding(1640-1688)shapeAtTranslationStart(1689-1702)getArrowInfo(134-140)updateArrowTerminal(1719-1785)apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationBindings.tsx (2)
updateArrowTerminal(298-361)RelationBindings(52-55)
apps/roam/src/components/canvas/Tldraw.tsx (10)
apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (3)
DiscourseNodeShape(147-157)createNodeShapeUtils(128-145)BaseDiscourseNodeUtil(158-560)apps/roam/src/components/canvas/ToastListener.tsx (1)
dispatchToastEvent(4-8)apps/roam/src/components/canvas/uiOverrides.tsx (2)
createUiComponents(212-303)createUiOverrides(304-450)apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx (1)
createAllRelationShapeUtils(472-873)apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx (1)
createAllRelationShapeTools(302-597)apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationBindings.tsx (2)
createAllRelationBindings(44-50)createAllReferencedNodeBindings(35-43)apps/roam/src/utils/canvasUiOverrides.ts (1)
createUiOverrides(37-418)apps/roam/src/components/canvas/DiscourseRelationShape/discourseRelationMigrations.ts (1)
createArrowShapeMigrations(15-128)apps/roam/src/components/canvas/useRoamStore.ts (1)
useRoamStore(83-387)apps/roam/src/utils/getDiscourseNodes.ts (1)
DiscourseNode(14-29)
apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationBindings.tsx (3)
apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx (1)
AddReferencedNodeType(12-12)apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx (1)
DiscourseRelationShape(876-876)apps/roam/src/components/canvas/DiscourseRelationShape/helpers.tsx (6)
updateArrowTerminal(1719-1785)getArrowBindings(141-153)getArrowInfo(134-140)assert(1826-1831)approximately(1848-1850)removeArrowBinding(1629-1639)
apps/roam/src/components/Export.tsx (4)
apps/roam/src/index.ts (1)
DEFAULT_CANVAS_PAGE_FORMAT(55-55)apps/roam/src/utils/isCanvasPage.ts (1)
isCanvasPage(4-9)apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (1)
DiscourseNodeShape(147-157)apps/roam/src/utils/getExportTypes.ts (1)
updateExportProgress(24-32)
apps/roam/src/components/canvas/LabelDialog.tsx (4)
apps/roam/src/utils/types.ts (1)
Result(42-46)apps/roam/src/components/canvas/Tldraw.tsx (2)
DiscourseContextType(90-96)discourseContext(98-102)apps/roam/src/utils/getPlainTitleFromSpecification.ts (1)
getPlainTitleFromSpecification(3-24)apps/roam/src/utils/formatUtils.ts (1)
getReferencedNodeInFormat(128-159)
apps/roam/src/components/canvas/DiscourseRelationShape/helpers.tsx (2)
apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx (2)
DiscourseRelationShape(876-876)BaseDiscourseRelationUtil(878-1453)apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationBindings.tsx (4)
RelationBindings(52-55)RelationBinding(78-78)RelationInfo(57-76)updateArrowTerminal(298-361)
apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (5)
apps/roam/src/utils/loadImage.ts (1)
loadImage(1-21)apps/roam/src/components/canvas/Tldraw.tsx (2)
discourseContext(98-102)isPageUid(108-111)apps/roam/src/utils/getDiscourseNodes.ts (1)
DiscourseNode(14-29)apps/roam/src/components/canvas/DiscourseRelationShape/helpers.tsx (1)
createTextJsxFromSpans(1216-1308)apps/obsidian/src/utils/createNode.ts (1)
createDiscourseNode(110-141)
🔇 Additional comments (21)
apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (8)
49-56: DEFAULT_STYLE_PROPS are fine as a starting pointBaseline font sizing and padding look reasonable for the node UI.
103-126: Tool creation flow looks correctDynamic StateNode tools set the shape type and convert to select mode after creating the shape. This aligns with tldraw’s recommended pattern.
170-176: Geometry is consistent with rectangular nodesReturning Rectangle2d with filled=true matches the rendered shape and selection behavior.
426-432: updateProps helper looks fineBatched updateShapes with id/props/type is a clean wrapper; no issues.
485-499: Good: avoid pointer events on the img to keep node interactions smoothPreventing the image from intercepting pointer events avoids selection glitches.
549-554: Delete-on-cancel is correct for non-live blocksNice UX touch to avoid orphan shapes when creation is cancelled and no backing block exists.
402-411: Verify thatshape.opacityis available in the v2 shape recordAccessing
shape.opacityhere assumes the TLBaseShape record defines anopacityfield. Please confirm on your local branch by inspecting the core types:rg -n 'interface TLBaseShape' -C2 node_modules/@tldraw/core/dist/types/index.d.ts | head -n 20If you don’t see an
opacity: numberentry, either pull the opacity from the style props (e.g.shape.props.opacityor from your style-props object) or remove theopacityprop on<rect>to avoid renderingundefined.
56-56: Verify tldraw v2.3.0 DefaultColorStyle API
Please confirm thatDefaultColorStyle.valuesis both exposed and iterable in tldraw v2.3.0. If the API has changed (for example, to a method likegetValues()), update the import or call accordingly.• apps/roam/src/components/canvas/DiscourseNodeUtil.tsx:
export const COLOR_ARRAY = Array.from(DefaultColorStyle.values).reverse();apps/roam/src/components/canvas/CanvasDrawer.tsx (2)
66-68: Compact state update is goodThe simplified toggleCollapse using functional setState is clearer and avoids stale closures.
121-136: UI grouping looks goodGrouping by UID with a collapsible list is straightforward and usable.
apps/roam/package.json (1)
22-22: LGTM: Pinned tldraw 2.3.0 and updated patch-package for patches alignment
- Pinning tldraw to 2.3.0 is appropriate given you ship patch-package patches targeted at this exact version.
- Upgrading patch-package to ^8.0.0 is fine for Node 16+ and aligns with modern patch flows.
Also applies to: 34-34
apps/roam/src/components/canvas/ToastListener.tsx (1)
10-12: Mount exactly onceEnsure ToastListener is mounted once at app root (or your TLUi root) so global dispatchToastEvent calls are observed. If it’s not wired in yet, you won’t see any toasts.
Where is ToastListener included? If not already, consider adding it alongside your canvas UI root component and verifying with a quick manual dispatch in dev tools:
// in the console after UI mounts document.dispatchEvent(new CustomEvent('dg:show-toast', { detail: { id: 'hello', title: 'Hello', description: 'ToastListener wired', severity: 'info' } }))apps/roam/src/utils/calcCanvasNodeSizeAndImg.ts (1)
95-103: User-facing fallback on image load failure is appropriateSwitching from email reporting to an in-product toast is a better UX for Roam. The empty imageUrl fallback is consistent with the earlier guards.
apps/roam/patches/@tldraw+editor+2.3.0.patch (1)
61-73: React 17 compatibility: good substitution for createRoot pathReplacing react-dom/client createRoot + flushSync with ReactDOM.render/unmount is the right call for the React 17 constraint in apps/roam. The returned detached SVG element mirrors the previous behavior.
apps/roam/src/components/canvas/CanvasReferences.tsx (1)
51-63: Nice: stable keys and voided async callsUsing r.uid as a key and prefixing open calls with void prevents unhandled Promise rejections from bubbling.
apps/roam/src/components/canvas/ConvertToDialog.tsx (1)
8-20: LGTM: Clear UX and correct editor integrationSelection gating, color handling, and dispatch via
getOnSelectForShapelook sound. Closing the dialog immediately after invoking the action keeps the UX tight.Also applies to: 21-31, 32-40, 44-53, 58-66, 70-79, 89-99
apps/roam/src/components/canvas/LabelDialog.tsx (1)
421-431: Good: Preventing TLDraw from hijacking pointer eventsThe wrapper with
onPointerDown={(e) => e.stopPropagation()}and the explicit dialog footer improve reliability when used inside TLDraw.Also applies to: 432-444, 459-480
apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationBindings.tsx (1)
35-50: Dynamic binding type factories look goodThe binding utilities generated from actions and relation ids correctly inherit props/migrations and override
type. This aligns with tldraw’sBindingUtilexpectations.apps/roam/src/components/canvas/DiscourseRelationShape/helpers.tsx (3)
1124-1194: SvgTextLabel export helper: overall approach LGTMThe label spans/outline approach matches tldraw’s export strategy and should preserve mixed RTL/LTR text via
unicodeBidi="plaintext". Once thefontStylefix is applied, this looks solid.
740-746: Length computation matches geometry mode
getLengthcorrectly switches between straight-edge length and arc length; appropriate for dash calculations and handle hints.
1950-2040: ArrowSvg rendering and masking logic looks consistent with tldraw v2.3.xMask id hack for Safari, handle hints, dash props, and label clipping all align with known patterns. No action needed.
…ity; update color assignments in DiscourseNodeUtil, uiOverrides, and DiscourseRelationTool components.
from
"@tldraw/tldraw": "^2.0.0-alpha.12"to"tldraw": "2.3.0"https://discoursegraphs.com/releases/roam/eng-62-upgrade-tldraw/
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores