Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds sticky tool functionality that tracks user tool selections and automatically restores the last "sticky" tool (discourse nodes, relations, actions) when users select the default selection tool, with tracking through refs and context propagation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/roam/src/components/canvas/Tldraw.tsx`:
- Around line 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.
🧹 Nitpick comments (1)
apps/roam/src/components/canvas/Tldraw.tsx (1)
279-303: Add an explicit return type to the new callback.This keeps the new
handleToolSelectedaligned with the TS return-type guideline.As per coding guidelines, please add explicit return types for new functions.♻️ Proposed fix
- const handleToolSelected = useCallback( - (toolId: string) => { + const handleToolSelected = useCallback( + (toolId: string): void => { toolSelectionRef.current.lastExplicitToolId = toolId; if (stickyToolIds.includes(toolId)) { toolSelectionRef.current.lastStickyToolId = toolId; } }, [stickyToolIds], );
| 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]); |
There was a problem hiding this comment.
🧩 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 3Repository: 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 2Repository: DiscourseGraphs/discourse-graph
Length of output: 239
🏁 Script executed:
rg "setCurrentTool" apps/roam/src/components/canvas/Tldraw.tsx -B 5 -A 5Repository: 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 -20Repository: 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 -30Repository: 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 10Repository: 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 2Repository: 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.
1 participant
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.