tldraw

[mdroidian]

---

Summary by CodeRabbit

• New Features

  • Added TLDraw canvas support with integrated inspector for searching and placing Roam pages and blocks.
  • Canvas pages are configurable via page title patterns.
  • Inspector includes keyboard navigation, drag-and-drop, and maximizable display for full-screen canvas editing.

• Chores

  • Migrated build system to pnpm.
  • Updated development tooling: Prettier, Tailwind CSS, enhanced TypeScript configuration, and VSCode workspace settings.

[coderabbitai]

Walkthrough

This pull request modernizes the project infrastructure by migrating from npm to pnpm, establishing comprehensive build tooling with esbuild-based compilation (dev and build scripts), and refining TypeScript configuration. It integrates tldraw canvas capabilities into the Roam Research extension, adding visual canvas editor components with Roam node rendering, inspector UI for page/block selection, and persistent state synchronization with Roam storage. Configuration files are expanded to include Prettier with Tailwind plugin support, VSCode workspace settings, Tailwind CSS configuration, and Blueprint.js patches that add React.ReactNode to component prop interfaces across multiple module variants.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The PR title 'tldraw' is extremely vague and generic. While the changeset does add tldraw integration, the single word 'tldraw' provides almost no meaningful information about what the PR accomplishes or why it matters.	Revise the title to be more descriptive, such as 'Add tldraw canvas integration for Roam pages' or 'Integrate tldraw visual editor with Roam' to clearly convey the main objective.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch tldraw

---

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.}

[devin-ai-integration]

Devin Review found 2 potential issues.

View 7 additional findings in Devin Review.

[coderabbitai]

Actionable comments posted: 9

🤖 Fix all issues with AI agents

In `@build.sh`:
- Line 1: Add a bash shebang as the very first line of build.sh (use the env
variant to locate bash) so the script runs with a defined shell when invoked
directly; place this line before the existing "pnpm run build:roam" command and
make the file executable (chmod +x) to avoid ShellCheck SC2148 and ensure
consistent execution.

In `@scripts/compile.ts`:
- Around line 205-213: Remove the CommonJS guard (require.main === module) in
compile.ts and replace it with an ESM-compatible entry check: import the URL
helpers (url.fileURLToPath) and compare fileURLToPath(import.meta.url) against
process.argv[1] (or otherwise use import.meta.url-based detection) to
conditionally call main(); ensure the existing main and compile functions are
unchanged and only the invocation guard is swapped; apply the same change
pattern to other script entrypoints (dev.ts, build.ts) so they use
import.meta.url-based detection when executed via tsx.

In `@scripts/dev.ts`:
- Around line 9-16: The current wrapper Promise never observes rejections from
compile(...), so failures from compile or esbuild.context are lost; modify the
block using compile({ ... }) so its returned Promise is handled — either await
it or attach .then/.catch — and ensure the outer Promise returned by the
function rejects on error instead of only resolving on process "exit".
Specifically, reference the compile(...) call and the esbuild.context(...)
builder and propagate any error into the outer Promise (call reject or reject
the Promise) so build/setup failures do not become unhandled rejections.

In `@src/components/canvas/DefaultNodeUtil.tsx`:
- Around line 316-328: The Datomic query string built in
getNodeTypeFromRoamRefText currently only escapes double quotes, leaving
backslashes unescaped which can break the query (e.g., a title ending with '\').
Update the title escaping before insertion into the q call by first escaping
backslashes (replace "\" with "\\") and then escaping double quotes (replace `"`
with `\"`) — matching the escapeRegex helper behavior — so the final string
passed to window.roamAlphaAPI.q is safe and won’t produce malformed queries.

In `@src/components/canvas/Tldraw.tsx`:
- Line 264: The hook order is violated because the early return "if (!pageUid)
return null;" short-circuits hooks (notably the useEffect block starting at
useEffect(...) lines 361-391); move that early return below all hooks or make
hooks unconditional: relocate the useEffect(...) block (and any helper functions
it references such as cancelInspector, handleDropPayload, openInspector) above
the pageUid check, or convert those helpers to stable useCallback hooks and keep
the pageUid guard inside the effect (e.g., return early inside useEffect if
!pageUid); ensure all React hooks in the Tldraw component are called
unconditionally and in the same order on every render.

In `@src/components/canvas/uiOverrides.tsx`:
- Around line 81-95: The Toolbar implementation calls the hook useIsToolSelected
inside a .map callback (violates Rules of Hooks); refactor by extracting a
standalone component (e.g., ToolbarToolItem) that accepts a tool (from
DEFAULT_NODE_TOOLS) or toolId and uses useTools and useIsToolSelected at the top
level of that component, then render <ToolbarToolItem key={tool.id} tool={tool}
/> from the Toolbar map; ensure Toolbar keeps DefaultToolbar and
DefaultToolbarContent and that TldrawUiMenuItem props are spread from the
tool-specific component so hooks are only called at component top-level
(reference: Toolbar, DEFAULT_NODE_TOOLS, useTools, useIsToolSelected,
TldrawUiMenuItem, DefaultToolbar, DefaultToolbarContent).

In `@src/index.ts`:
- Around line 63-71: The unload handler currently deletes the signia symbol but
doesn’t call the cleanup functions returned by renderTldrawCanvas and
renderTldrawCanvasInSidebar, causing DOM/React leaks; fix by storing those
cleanup functions (e.g., in a module-level array or variables when you call
renderTldrawCanvas and renderTldrawCanvasInSidebar) and on unload iterate and
invoke each cleanup, then clear the storage before deleting
window[Symbol.for("__signia__")]; ensure the references to the cleanup functions
are removed after calling them so repeated loads/unloads won’t call stale
handlers.

In `@tailwind.config.js`:
- Around line 1-8: tsconfig.json currently includes "tailwind.config.ts" but the
repo has tailwind.config.js, so update the include to match the real filename or
add a TypeScript config file; specifically either change the tsconfig.json
include entry referencing "tailwind.config.ts" to "tailwind.config.js" or create
a matching tailwind.config.ts export (e.g., export default) so TypeScript
tooling and readers are consistent — look for the include entry in tsconfig.json
and the existing tailwind.config.js module.exports to reconcile the names.

In `@tsconfig.json`:
- Around line 3-9: The tsconfig "include" currently lists "tailwind.config.ts"
which doesn't exist; update the include entry to reference the actual file name
"tailwind.config.js" (or remove the entry entirely if you don't want config
files included) so the tsconfig accurately reflects the repo; change the string
"tailwind.config.ts" in the "include" array to "tailwind.config.js" or remove
it, and confirm behavior with your allowJs setting.

🧹 Nitpick comments (16)

src/components/canvas/tldrawStyles.ts (1)

77-90: Hardcoded color #dadddf won't adapt to dark themes.

The hover/active background color for node inspector menu items is hardcoded. If Roam supports dark mode (or may in the future), consider using a CSS variable or an inherited Blueprint token instead.

tsconfig.json (1)

29-29: Consider "react-jsx" for the automatic JSX transform.

With React 18, "react-jsx" eliminates the need for import React from 'react' in every JSX file. Since esbuild handles bundling independently, this primarily affects tsc type-checking, but switching would align with modern React conventions and reduce boilerplate imports.

src/utils/getBlockProps.ts (1)

9-23: Inner type guard in normalizeProps object branch is redundant.

Within the Object.entries mapping (lines 18-20), the typeof value === "object" && value !== null check duplicates what normalizeProps already handles at the top level. You can simplify by unconditionally recursing:

♻️ Simplify by always recursing on values

         Object.fromEntries(
             Object.entries(props).map(([key, value]) => [
               key.replace(/^:+/, ""),
-              typeof value === "object" && value !== null
-                ? normalizeProps(value as JsonValue)
-                : value,
+              normalizeProps(value as JsonValue),
             ]),
           )

scripts/compile.ts (2)

21-21: Unused variable envContents.

let envContents = null; is declared but never read or assigned elsewhere in this file. Remove it to avoid confusion.

🧹 Remove dead code

-let envContents = null;

---

85-90: external is required in the Zod schema but has no default.

If compile is called with a partial opts object (e.g., compile({ opts: { out: "foo" } })), Zod will throw because external is not optional and has no default. Consider adding .default([]) to make it safe for callers that don't need external mappings.

♻️ Add default for `external`

 const cliArgs = z.object({
   out: z.string().optional(),
   root: z.string().optional(),
   format: z.enum(["esm"]).optional(),
-  external: z.array(z.string()),
+  external: z.array(z.string()).default([]),
 });

scripts/build.ts (1)

3-26: Redundant double try/catch — build() already exits on failure.

build() catches errors and calls process.exit(1) on line 15, so main()'s outer catch (lines 22-24) is unreachable for any error originating from compile(). If the intent is defense-in-depth, consider removing the inner process.exit(1) and letting the error propagate to main(), or collapsing to a single level.

Simplified single-level alternative

-const build = async () => {
-  process.env = {
-    ...process.env,
-    NODE_ENV: process.env.NODE_ENV || "production",
-  };
-
-  console.log("Compiling ...");
-  try {
-    await compile({});
-    console.log("Compiling complete");
-  } catch (error) {
-    console.error("Build failed on compile:", error);
-    process.exit(1);
-  }
-};
-
-const main = async () => {
-  try {
-    await build();
-  } catch (error) {
-    console.error(error);
-    process.exit(1);
-  }
-};
+const main = async () => {
+  process.env.NODE_ENV = process.env.NODE_ENV || "production";
+  console.log("Compiling ...");
+  try {
+    await compile({});
+    console.log("Compiling complete");
+  } catch (error) {
+    console.error("Build failed on compile:", error);
+    process.exit(1);
+  }
+};

.github/workflows/main.yaml (1)

24-30: Consider omitting version: 9 and letting pnpm/action-setup read from packageManager.

package.json pins "packageManager": "pnpm@9.15.0", but the workflow specifies version: 9, which may install a different 9.x minor. pnpm/action-setup@v4 can auto-detect the version from the packageManager field if version is omitted, ensuring CI and local dev use the exact same pnpm version.

Proposed fix

       - uses: pnpm/action-setup@v4
-        with:
-          version: 9
       - uses: actions/setup-node@v4
         with:
           node-version: "20"
           cache: "pnpm"

src/index.ts (1)

68-70: TODO: Migration from roamjs-query-builder.tldraw.

Flagging the TODO on line 68. This notes a pending migration path for users coming from the older roamjs-query-builder.tldraw data format.

Would you like me to open an issue to track this migration task?

package.json (1)

35-35: Update esbuild from 0.17.14 to a more recent version.

The pinned version is nearly 3 years old. Unlike suggested in the original note, roamjs-components@0.86.4 does not list esbuild as a dependency, so this pinning is not required for compatibility. Newer versions offer significant performance improvements and bug fixes—current stable version is 0.27.3.

src/components/canvas/uiOverrides.tsx (1)

109-124: Inline component ViewMenu defined inside MainMenu render body causes remounts.

ViewMenu is declared as a new function component on every render of MainMenu. React sees a new component type each time, so it will unmount and remount the subtree on every render. Hoist it outside the MainMenu factory or memoize it.

Proposed fix — hoist ViewMenu

+const ViewMenu = () => {
+  const actions = useActions();
+  return (
+    <TldrawUiMenuSubmenu id="view" label="menu.view">
+      <TldrawUiMenuGroup id="view-actions">
+        <TldrawUiMenuItem {...actions["zoom-in"]} />
+        <TldrawUiMenuItem {...actions["zoom-out"]} />
+        <ZoomTo100MenuItem />
+        <ZoomToFitMenuItem />
+        <ZoomToSelectionMenuItem />
+        <TldrawUiMenuItem {...actions["toggle-full-screen"]} />
+      </TldrawUiMenuGroup>
+    </TldrawUiMenuSubmenu>
+  );
+};
+
 export const createUiComponents = (): TLUiComponents => ({
   ...
   MainMenu: () => {
-    const ViewMenu = () => {
-      const actions = useActions();
-      return (
-        <TldrawUiMenuSubmenu id="view" label="menu.view">
-          <TldrawUiMenuGroup id="view-actions">
-            <TldrawUiMenuItem {...actions["zoom-in"]} />
-            <TldrawUiMenuItem {...actions["zoom-out"]} />
-            <ZoomTo100MenuItem />
-            <ZoomToFitMenuItem />
-            <ZoomToSelectionMenuItem />
-            <TldrawUiMenuItem {...actions["toggle-full-screen"]} />
-          </TldrawUiMenuGroup>
-        </TldrawUiMenuSubmenu>
-      );
-    };
     return (
       <DefaultMainMenu>
         <EditSubmenu />
         <ViewMenu />

src/components/canvas/Tldraw.tsx (3)

458-492: editor.on("event", ...) fires on every editor event — potential performance concern and fragile cast.

refreshInspectorTarget() is invoked on every single editor event (pointer moves, wheel, keyboard, etc.), each time calling editor.getOnlySelectedShape(). This is wasteful when only selection changes matter. The cast event as TLPointerEventInfo on line 459 is also unsafe for non-pointer events (fields like shiftKey/ctrlKey will be undefined), though the e.name === "pointer_up" guard prevents misuse.

Consider listening to selection changes specifically (e.g., editor.store.listen filtered to selection changes or editor.on("change", ...) with appropriate filtering) instead of all events.

---

408-520: Large onMount callback — consider extracting editor setup logic.

The onMount handler spans ~110 lines, registering an event listener, defining refreshInspectorTarget, and registering an external content handler. Extracting these into named setup functions would improve readability and testability.

---

59-67: getPageUidByPageTitle(title) called on every render.

This synchronous Roam API call runs unconditionally on each render. If the function has any cost, wrap it in useMemo keyed on title.

Proposed fix

-  const pageUid = getPageUidByPageTitle(title);
+  const pageUid = useMemo(() => getPageUidByPageTitle(title), [title]);

src/utils/isCanvasPage.ts (1)

15-18: Wildcard * maps to .+ (one-or-more) — is zero-or-more intended?

The * wildcard is replaced with .+, requiring at least one character. In typical glob semantics, * matches zero or more characters (.*). With the current implementation, a pattern Canvas/* won't match a page titled exactly Canvas/ (trailing slash, no suffix). This may be intentional but is worth confirming.

Regarding the static analysis ReDoS warning: the input is fully escaped before the \* → .+ substitution, so user-supplied patterns cannot inject arbitrary regex. The warning is a false positive here.

src/components/canvas/useRoamStore.ts (1)

95-111: useMemo performs side effects (loadSnapshot).

useMemo is intended for pure computation. The loadSnapshot call mutates the store, which is a side effect. React may re-run useMemo in future concurrent mode without guarantees. This is a common pattern in tldraw integrations, but be aware that React Strict Mode in development will double-invoke this, potentially loading the snapshot twice.

src/components/canvas/DefaultNodeUtil.tsx (1)

57-58: Duplicate escapeRegex function — also defined in isCanvasPage.ts.

This file's version additionally escapes double quotes (for Datomic embedding). Consider extracting a shared base escapeRegex and composing the Datomic-specific variant to avoid duplication.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/components/canvas/DefaultNodeUtil.tsx`:
- Around line 57-58: escapeRegex currently escapes regex metacharacters and
quotes but does not double-escape backslashes for EDN string interpolation, so
add a step that replaces each single backslash with two backslashes after the
existing metacharacter-escaping; update the function escapeRegex to perform
value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&").replace(/\\/g,
"\\\\").replace(/"/g, '\\"') (and apply the same change to the other similar
helpers in the 60-94 and 96-130 blocks) so the EDN parser consumes one level of
escaping and the Java regex receives the intended escaped backslashes.

🧹 Nitpick comments (7)

package.json (1)

39-43: Exact-pinned peer dependencies are unusual and may cause installation warnings.

Peer dependencies are typically specified as ranges (e.g., "react": "^18.2.0") to allow flexibility for the consuming environment. Exact pins ("18.2.0") will emit warnings or errors if the host has even a patch-level difference (e.g., 18.2.1). Since this is a Roam Research extension where the host provides React, consider using >=18.2.0 <19 or similar ranges unless exact versions are strictly required.

Suggested change

   "peerDependencies": {
-    "react": "18.2.0",
-    "react-dom": "18.2.0",
-    "@blueprintjs/core": "3.50.4"
+    "react": "^18.2.0",
+    "react-dom": "^18.2.0",
+    "@blueprintjs/core": "^3.50.4"
   },

src/components/canvas/uiOverrides.tsx (1)

118-143: ViewMenu is redefined on every render of MainMenu, causing unnecessary unmount/remount cycles.

Declaring ViewMenu as a component inside the MainMenu render body means React sees a new component type on each render, discarding the subtree and recreating it (losing internal state and triggering extra DOM work). Hoist it outside createUiComponents or at least make it a stable reference.

♻️ Proposed fix — hoist ViewMenu

+const ViewMenu = () => {
+  const actions = useActions();
+  return (
+    <TldrawUiMenuSubmenu id="view" label="menu.view">
+      <TldrawUiMenuGroup id="view-actions">
+        <TldrawUiMenuItem {...actions["zoom-in"]} />
+        <TldrawUiMenuItem {...actions["zoom-out"]} />
+        <ZoomTo100MenuItem />
+        <ZoomToFitMenuItem />
+        <ZoomToSelectionMenuItem />
+        <TldrawUiMenuItem {...actions["toggle-full-screen"]} />
+      </TldrawUiMenuGroup>
+    </TldrawUiMenuSubmenu>
+  );
+};
+
 export const createUiComponents = (): TLUiComponents => ({
   // ...
   MainMenu: () => {
-    const ViewMenu = () => {
-      const actions = useActions();
-      return (
-        <TldrawUiMenuSubmenu id="view" label="menu.view">
-          <TldrawUiMenuGroup id="view-actions">
-            <TldrawUiMenuItem {...actions["zoom-in"]} />
-            <TldrawUiMenuItem {...actions["zoom-out"]} />
-            <ZoomTo100MenuItem />
-            <ZoomToFitMenuItem />
-            <ZoomToSelectionMenuItem />
-            <TldrawUiMenuItem {...actions["toggle-full-screen"]} />
-          </TldrawUiMenuGroup>
-        </TldrawUiMenuSubmenu>
-      );
-    };
     return (
       <DefaultMainMenu>
         <EditSubmenu />
         <ViewMenu />
         <ExportFileContentSubMenu />
         <ExtrasGroup />
         <PreferencesGroup />
       </DefaultMainMenu>
     );
   },
 });

src/components/canvas/Tldraw.tsx (4)

159-162: Stale toggleMaximized closure captured by useMemo — safe today, fragile under refactoring.

toggleMaximized is recreated every render but useMemo on line 159 only re-runs when maximizeKbd changes. This works today because toggleMaximized only reads refs (containerRef, appRef), which are always current. However, if someone later adds state or prop dependencies to toggleMaximized, the memoized override will silently use the stale version.

Consider wrapping toggleMaximized in useCallback (with [] deps since it only touches refs) and including it in the useMemo dep array, or adding a comment noting the ref-only constraint.

---

223-231: Including selectedUid in the dependency array of its own setter effect is a code smell.

The effect that synchronizes selectedUid with visibleResults lists selectedUid itself as a dependency. It works because React bails out on setting the same value, but it causes the effect to re-run every time selectedUid changes (including from keyboard navigation in moveSelection), doing a redundant scan of visibleResults. Prefer a functional update or remove selectedUid from the dep list.

♻️ Proposed fix — drop selectedUid from deps

  useEffect(() => {
    if (!visibleResults.length) {
      setSelectedUid("");
      return;
    }
-   if (!selectedUid || !visibleResults.some((r) => r.uid === selectedUid)) {
-     setSelectedUid(visibleResults[0].uid);
-   }
- }, [visibleResults, selectedUid]);
+   setSelectedUid((prev) =>
+     prev && visibleResults.some((r) => r.uid === prev) ? prev : visibleResults[0].uid,
+   );
+ }, [visibleResults]);

---

456-490: Event listener on editor.on("event", ...) fires refreshInspectorTarget on every editor event — consider filtering.

refreshInspectorTarget() (line 458) runs on every editor event (pointer move, wheel, key, etc.), each time calling editor.getOnlySelectedShape() and running through the setter logic. For high-frequency events like pointer moves, this adds overhead. Consider limiting the listener to selection-relevant events (e.g., "change" or specific event names) or debouncing the refresh.

---

307-337: useEffect with [] deps captures handleDropPayload closure — correct but non-obvious.

The empty dependency array means onDropNative captures the initial render's handleDropPayload. This works only because handleDropPayload reads appRef.current (a ref, always current) and calls stable imported functions. A comment explaining this invariant would help future maintainers, or alternatively wrap handleDropPayload in useCallback.

src/components/canvas/DefaultNodeUtil.tsx (1)

271-301: Dynamically generated StateNode classes share a single class identity per call — verify tldraw handles this correctly.

createDefaultNodeShapeTools uses .map() to generate two classes both named DefaultNodeTool (the class declaration name), differentiated only by the static id override. Since the consumer in Tldraw.tsx wraps this in useMemo([], []), the classes are stable across renders. However, the shared class name may cause confusion in React DevTools or error stack traces.

♻️ Optional: give each class a distinct name

 export const createDefaultNodeShapeTools = (): TLStateNodeConstructor[] =>
   DEFAULT_NODE_TOOLS.map(
     ({ id }) => {
-      class DefaultNodeTool extends StateNode {
+      const Tool = class extends StateNode {
         static override id = id;
         static override initial = "idle";
         shapeType = id;
         // ...
-      }
-      return DefaultNodeTool;
+      };
+      Object.defineProperty(Tool, "name", { value: `${id}Tool` });
+      return Tool;
     },
   );

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 10 additional findings in Devin Review.

[devin-ai-integration]

🔴 Regex escaping produces invalid EDN escape sequences in Datalog search queries

The escapeRegex function in DefaultNodeUtil.tsx escapes regex special characters by prefixing them with \, but the resulting pattern is interpolated directly into a Datalog/EDN string literal without double-escaping the backslashes for the EDN string context. This causes search queries containing common characters like ., (, ), *, +, etc. to silently fail.

Root Cause and Impact

The escapeRegex function at src/components/canvas/DefaultNodeUtil.tsx:57-58 produces regex-escaped strings (e.g., input a.b → output a\.b). This escaped pattern is then interpolated into a Datalog query at lines 74 and 110:

[(re-pattern "(?i)${pattern}") ?re]

The resulting EDN string "(?i)a\.b" contains \. which is not a valid EDN/Clojure escape sequence. Only \\, \", \n, \r, \t, and \uXXXX are valid. The Roam Datalog API (based on Clojure's reader) will throw a reader exception for unrecognized escapes like \., \(, \), etc.

The error is caught by the .catch() handler at lines 204-209, which silently returns an empty result array. This means any search query containing ., *, +, ?, ^, $, {, }, (, ), |, [, ], or \ will return no results at all, even if matching pages/blocks exist.

These characters are common in Roam page titles (e.g., "Project (2024)", "Mr. Smith", "Q&A"), so this significantly impacts search usability.

To fix this, backslashes produced by regex escaping must be double-escaped for the EDN string context. For example, a.b should produce a\\.b so that the EDN parser reads it as a\.b, which re-pattern then interprets as a regex matching a literal dot.

Suggested change

	const escapeRegex = (value: string): string =>
	value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&").replace(/"/g, '\\"');
	const escapeRegex = (value: string): string =>
	value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&").replace(/\\/g, "\\\\").replace(/"/g, '\\"');

---

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