Skip to content

[ENG-1108] Make discourse tool lock #709

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
trangdoan982 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[ENG-1108] Make discourse tool lock #709

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
69 changes: 68 additions & 1 deletion apps/roam/src/components/canvas/Tldraw.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
/* eslint-disable @typescript-eslint/naming-convention */
import React, { useState, useRef, useMemo, useEffect } from "react";
import React, {
useState,
useRef,
useMemo,
useEffect,
useCallback,
} from "react";
import ExtensionApiContextProvider, {
useExtensionAPI,
} from "roamjs-components/components/ExtensionApiContext";
Expand Down Expand Up @@ -43,6 +49,7 @@ import {
StateNode,
DefaultSpinner,
Box,
useValue,
} from "tldraw";
import "tldraw/tldraw.css";
import tldrawStyles from "./tldrawStyles";
Expand Down Expand Up @@ -269,6 +276,31 @@ const TldrawCanvas = ({ title }: { title: string }) => {
const allAddReferencedNodeActions = useMemo(() => {
return Object.keys(allAddReferencedNodeByAction);
}, [allAddReferencedNodeByAction]);
const stickyToolIds = useMemo(
() => [
"discourse-tool",
...allNodes.map((node) => node.type),
...allRelationNames,
...allAddReferencedNodeActions,
],
[allNodes, allRelationNames, allAddReferencedNodeActions],
);
const toolSelectionRef = useRef<{
lastStickyToolId: string | null;
lastExplicitToolId: string | null;
}>({
lastStickyToolId: null,
lastExplicitToolId: null,
});
const handleToolSelected = useCallback(
(toolId: string) => {
toolSelectionRef.current.lastExplicitToolId = toolId;
if (stickyToolIds.includes(toolId)) {
toolSelectionRef.current.lastStickyToolId = toolId;
}
},
[stickyToolIds],
);

const isRelationTool = (toolId: string) => {
return (
Expand Down Expand Up @@ -433,6 +465,7 @@ const TldrawCanvas = ({ title }: { title: string }) => {
toggleMaximized: handleMaximizedChange,
setConvertToDialogOpen,
discourseContext,
onToolSelected: handleToolSelected,
});

// STORE
Expand Down Expand Up @@ -688,6 +721,8 @@ const TldrawCanvas = ({ title }: { title: string }) => {
<InsideEditorAndUiContext
extensionAPI={extensionAPI}
allNodes={allNodes}
stickyToolIds={stickyToolIds}
toolSelectionRef={toolSelectionRef}
// allRelationIds={allRelationIds}
// allAddReferencedNodeActions={allAddReferencedNodeActions}
/>
Expand All @@ -707,17 +742,49 @@ const TldrawCanvas = ({ title }: { title: string }) => {
const InsideEditorAndUiContext = ({
extensionAPI,
allNodes,
stickyToolIds,
toolSelectionRef,
// allRelationIds,
// allAddReferencedNodeActions,
}: {
extensionAPI: OnloadArgs["extensionAPI"];
allNodes: DiscourseNode[];
stickyToolIds: string[];
toolSelectionRef: React.MutableRefObject<{
lastStickyToolId: string | null;
lastExplicitToolId: string | null;
}>;
// allRelationIds: string[];
// allAddReferencedNodeActions: string[];
}) => {
const editor = useEditor();
const toasts = useToasts();
const msg = useTranslation();
const currentToolId = useValue(
"currentToolId",
() => editor.getCurrentToolId(),
[editor],
);

useEffect(() => {
if (stickyToolIds.includes(currentToolId)) {
toolSelectionRef.current.lastStickyToolId = currentToolId;
return;
}

if (currentToolId !== "select") {
return;
}

if (toolSelectionRef.current.lastExplicitToolId === "select") return;

const lastStickyTool = toolSelectionRef.current.lastStickyToolId;
if (!lastStickyTool) return;
if (lastStickyTool === "select") return;
if (editor.getCurrentToolId() !== "select") return;

editor.setCurrentTool(lastStickyTool);
}, [currentToolId, editor, stickyToolIds, toolSelectionRef]);
Comment on lines +763 to +787
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 16, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cat -n apps/roam/src/components/canvas/Tldraw.tsx | sed -n '750,800p'

Repository: DiscourseGraphs/discourse-graph

Length of output: 2024

🏁 Script executed:

# First, let's understand the stickyToolIds and how it's populated
rg "stickyToolIds" apps/roam/src/components/canvas/Tldraw.tsx -B 3 -A 3

Repository: DiscourseGraphs/discourse-graph

Length of output: 1856

🏁 Script executed:

# Check what version of tldraw is being used
cat apps/roam/package.json | grep -A 5 -B 5 "tldraw"

Repository: DiscourseGraphs/discourse-graph

Length of output: 931

🏁 Script executed:

# Look for how tools are managed and if there's any validation
rg "setCurrentTool" apps/roam/src/components/canvas/Tldraw.tsx -B 2 -A 2

Repository: DiscourseGraphs/discourse-graph

Length of output: 239

🏁 Script executed:

rg "setCurrentTool" apps/roam/src/components/canvas/Tldraw.tsx -B 5 -A 5

Repository: DiscourseGraphs/discourse-graph

Length of output: 562

🏁 Script executed:

# Check if there's any try-catch or error handling around tool setting
rg "try|catch" apps/roam/src/components/canvas/Tldraw.tsx | head -20

Repository: DiscourseGraphs/discourse-graph

Length of output: 405

🏁 Script executed:

# Look for any validation of tool IDs in the codebase
rg "isValidTool|validateTool|validToolIds" apps/roam/src/

Repository: DiscourseGraphs/discourse-graph

Length of output: 57

🏁 Script executed:

# Check the type definitions or usage of setCurrentTool to understand its behavior
rg "editor\." apps/roam/src/components/canvas/Tldraw.tsx | grep -i "tool"

Repository: DiscourseGraphs/discourse-graph

Length of output: 210

🏁 Script executed:

# Check if there's any documentation or type info about setCurrentTool in node_modules or imports
rg "setCurrentTool" --type ts -A 10 | head -30

Repository: DiscourseGraphs/discourse-graph

Length of output: 2381

🏁 Script executed:

# Look for how handleToolSelected validates the toolId
rg "handleToolSelected" apps/roam/src/components/canvas/Tldraw.tsx -B 2 -A 10

Repository: DiscourseGraphs/discourse-graph

Length of output: 807

🏁 Script executed:

# Check what happens when we look at the full context of tool ID management
sed -n '600,800p' apps/roam/src/components/canvas/Tldraw.tsx | grep -A 20 -B 5 "stickyToolIds"

Repository: DiscourseGraphs/discourse-graph

Length of output: 2677

🌐 Web query:

tldraw 2.3.0 setCurrentTool invalid tool id error handling

💡 Result:

Short answer: in tldraw v2.3.0 editor.setCurrentTool(id) does not expose a documented thrown exception for an unknown/invalid tool id — the API is documented as taking a string id and returning this, and the docs show using it for deep transitions (e.g. "select.eraser.pointing"). If you pass an id that isn't in the editor's state chart it will not succeed in switching to that state (effectively a no-op) — there's no documented runtime error case to rely on. [1][2]

Recommended handling

  • Validate before calling: check the tool id exists in your editor tooling (your UI overrides / tools map or the StateNode ids you registered) and only call setCurrentTool with a known id.
  • After calling, you can verify the switch with editor.getCurrentToolId() and handle a fallback if it didn't change.
  • If you need dynamic tools, use editor.setTool(...) / editor.removeTool(...) to add/remove tools at runtime (available on recent SDK releases). [2][3]

References

  • Editor API (setCurrentTool, getCurrentToolId). [1]
  • Tools / setCurrentTool usage (deep transitions example). [2]
  • Dynamic tools (setTool / removeTool) docs. [3]

References:
[1] tldraw Editor reference — setCurrentTool / getCurrentToolId.
[2] tldraw Tools docs (examples showing editor.setCurrentTool usage).
[3] tldraw example: Dynamic tools with setTool/removeTool.

🏁 Script executed:

# Check if there are any other guard patterns in the codebase for tool validation
rg "includes.*Tool" apps/roam/src/components/canvas/ -A 2 -B 2

Repository: DiscourseGraphs/discourse-graph

Length of output: 438

Add guard to prevent restoring invalid tool IDs.

When allNodes changes and a node type is removed, stickyToolIds updates accordingly, but lastStickyToolId remains in the ref. If a user previously selected a tool that's no longer in stickyToolIds, attempting to restore it with setCurrentTool will silently fail (no exception, just a no-op). Add the guard to validate the stored tool is still available before attempting to restore it.

🐛 Suggested guard 
     const lastStickyTool = toolSelectionRef.current.lastStickyToolId;
     if (!lastStickyTool) return;
+    if (!stickyToolIds.includes(lastStickyTool)) return;
     if (lastStickyTool === "select") return;
     if (editor.getCurrentToolId() !== "select") return;

     editor.setCurrentTool(lastStickyTool);
🤖 Prompt for AI Agents 
In `@apps/roam/src/components/canvas/Tldraw.tsx` around lines 763 - 787, The
effect that restores lastStickyToolId should validate that the stored ID is
still available in stickyToolIds before calling editor.setCurrentTool; in the
useEffect referencing currentToolId, editor, stickyToolIds, and
toolSelectionRef, add a guard that ensures
toolSelectionRef.current.lastStickyToolId exists and is included in
stickyToolIds (and not equal to "select") before invoking
editor.setCurrentTool(lastStickyTool), so stale/removed tool IDs are not
attempted to be restored.


// const isCustomArrowShape = (shape: TLShape) => {
// // TODO: find a better way to identify custom arrow shapes
Expand Down
17 changes: 17 additions & 0 deletions apps/roam/src/components/canvas/uiOverrides.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,13 +260,15 @@ export const createUiOverrides = ({
discourseContext,
toggleMaximized,
setConvertToDialogOpen,
onToolSelected,
}: {
allNodes: DiscourseNode[];
allRelationNames: string[];
allAddReferencedNodeByAction: AddReferencedNodeType;
discourseContext: DiscourseContextType;
toggleMaximized: () => void;
setConvertToDialogOpen: (open: boolean) => void;
onToolSelected?: (toolId: string) => void;
}): TLUiOverrides => ({
tools: (editor, tools) => {
// Get the custom keyboard shortcut for the discourse tool
Expand All @@ -285,9 +287,21 @@ export const createUiOverrides = ({
kbd: discourseToolShortcut,
readonlyOk: true,
onSelect: () => {
onToolSelected?.("discourse-tool");
editor.setCurrentTool("discourse-tool");
},
};
if (tools["select"]) {
const selectTool = tools["select"];
tools["select"] = {
...selectTool,
onSelect: (source) => {
onToolSelected?.("select");
selectTool.onSelect?.(source);
editor.setCurrentTool("select");
},
};
}
allNodes.forEach((node, index) => {
const nodeId = node.type;
tools[nodeId] = {
Expand All @@ -296,6 +310,7 @@ export const createUiOverrides = ({
label: `shape.node.${node.type}` as TLUiTranslationKey,
kbd: node.shortcut,
onSelect: () => {
onToolSelected?.(nodeId);
editor.setCurrentTool(nodeId);
},
readonlyOk: true,
Expand All @@ -315,6 +330,7 @@ export const createUiOverrides = ({
kbd: "",
readonlyOk: true,
onSelect: () => {
onToolSelected?.(name);
editor.setCurrentTool(name);
},
style: {
Expand All @@ -338,6 +354,7 @@ export const createUiOverrides = ({
kbd: "",
readonlyOk: true,
onSelect: () => {
onToolSelected?.(name);
editor.setCurrentTool(`${name}`);
},
style: {
Expand Down