Skip to content

[ENG-1108] Canvas tool lock#851

Open
trangdoan982 wants to merge 9 commits intomainfrom
eng-1108-tool-lock
Open

[ENG-1108] Canvas tool lock#851
trangdoan982 wants to merge 9 commits intomainfrom
eng-1108-tool-lock

Conversation

@trangdoan982
Copy link
Collaborator

@trangdoan982 trangdoan982 commented Mar 3, 2026

@linear
Copy link

linear bot commented Mar 3, 2026

@supabase
Copy link

supabase bot commented Mar 3, 2026

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


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

@trangdoan982 trangdoan982 changed the title [ENG-1108] Tool lock [ENG-1108] Canvas tool lock Mar 3, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

@trangdoan982 trangdoan982 requested a review from mdroidian March 5, 2026 20:04
return (
<div
className="roamjs-tldraw-canvas-container relative z-10 h-full w-full overflow-hidden rounded-md border border-gray-300 bg-white"
data-page-uid={pageUid}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we need to add this back because i keep receiving error "No canvas page UID found"

Copy link
Contributor

@mdroidian mdroidian left a comment

Choose a reason for hiding this comment

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

It seems there was come confusion on what the task was requesting. Unfortunately, the ticket should have been more clear, sorry.

If a user:

  • selects dg tool
  • toggle lock on
  • creates a node

The dg tool/node should still be the current tool

If a user:

  • selects dg tool
  • toggles lock off (or leaves as default)
  • creates a node

The current tool should be the select tool


This follows the same behavior as all other tldraw tools.

https://www.loom.com/share/265d543710924f548202e006d033452b

@trangdoan982
Copy link
Collaborator Author

trangdoan982 commented Mar 9, 2026

@mdroidian is this the same behavior we want for ENG-1383: Stay at the relation arrow menu after creating a relation? Based on the description in this ticket I understood that once the discourse relation tool is selected, it should stay locked there without doing any extra lock tool operation.

In this latest push I had discourse node and relation tools to have the same behavior, but let me know if you interpret it differently.

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Contributor

@mdroidian mdroidian left a comment

Choose a reason for hiding this comment

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

https://www.loom.com/share/07aab48252a744f4ad016e00e619e7e1

  • cannot edit discourse nodes
  • changing default tldraw lock behavior unnecessarily (unlocking on choosing select tool)

@trangdoan982
Copy link
Collaborator Author

trangdoan982 commented Mar 19, 2026

https://www.loom.com/share/878e7f7432b645ceb310c914ed200d93

Here's my updated test with the bugs you pointed out. I couldn't reproduce the unable to edit bug. Can you try again?

@trangdoan982 trangdoan982 requested a review from mdroidian March 19, 2026 17:25
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 2 new potential issues.

View 13 additional findings in Devin Review.

Open in Devin Review

initialText: string;
imageUrl?: string;
}) => {
editor.updateInstanceState({ isToolLocked: false });
Copy link
Contributor

Choose a reason for hiding this comment

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

🟡 Convert To action permanently destroys user's tool lock preference

In uiOverrides.tsx:160, editor.updateInstanceState({ isToolLocked: false }) unconditionally clears the global isToolLocked state as a side effect of using the "Convert To" context menu. This persists beyond the Convert To operation — after converting a text/image shape to a discourse node, the user's tool lock preference is permanently disabled for all future tool usage. The intent is to ensure the tool switches to "select" after conversion, but the implementation mutates global state rather than just calling editor.setCurrentTool("select") directly in the onSuccess callback at uiOverrides.tsx:198.

Prompt for agents
In apps/roam/src/components/canvas/uiOverrides.tsx, remove the line `editor.updateInstanceState({ isToolLocked: false });` at line 160 inside `openDialogAndCreateShape`. Then at line 198, replace `setCurrentToolToSelectIfUnlocked(editor);` with `editor.setCurrentTool("select");` since Convert To should always end on the select tool regardless of tool lock state, without clearing the user's global tool lock preference.
Open in Devin Review

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@trangdoan982 Why are we changing the lock state here? Wthat problem is this solving?

Comment on lines +587 to +592
if (action === "create") {
restoreToolState();
} else {
editor.setEditingShape(null);
dialogRenderedRef.current = false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🟡 restoreToolState called twice on successful node creation, causing redundant tool re-entry

In DiscourseNodeUtil.tsx:587-588, when action === "create", restoreToolState() is called. Then ModifyNodeDialog.tsx:483 calls onClose() after await onSuccess(...) completes. The onClose handler at DiscourseNodeUtil.tsx:594-596 checks isCreating (which is true for new nodes) and calls restoreToolState() again. This causes editor.setCurrentTool(shape.type) to fire twice in succession when tool lock is active, which may trigger the tool's onEnter handler (setting cursor, etc.) redundantly. The onSuccess handler should not call restoreToolState() for create actions since onClose already handles it, or onClose should detect that restoration already happened.

Suggested change
if (action === "create") {
restoreToolState();
} else {
editor.setEditingShape(null);
dialogRenderedRef.current = false;
}
if (action === "create") {
restoreToolState();
} else {
editor.setEditingShape(null);
dialogRenderedRef.current = false;
}
Open in Devin Review

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

Copy link
Contributor

@mdroidian mdroidian left a comment

Choose a reason for hiding this comment

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

There are some undesired changes that I am unclear why they are included in this PR.

Also, this devin comment is worth looking into: #851 (comment)

...position,
},
]);
setCurrentToolToSelectIfUnlocked(app);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we changing the tool state here?

},
},
]);
setCurrentToolToSelectIfUnlocked(editor);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we changing the tool here? This diverges from the regular tldraw pattern. eg
If tool = rectangle and I paste text that is not a dg shape, tool stays as rectangle.
If tool = rectangle and I paste text that is a dg shape, tool changes to select.

initialText: string;
imageUrl?: string;
}) => {
editor.updateInstanceState({ isToolLocked: false });
Copy link
Contributor

Choose a reason for hiding this comment

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

@trangdoan982 Why are we changing the lock state here? Wthat problem is this solving?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants