ENG-1884 Add 'convert to' node option to right click menu for tldraw shape to allow user to convert shape to node (Roam) - #1431
Conversation
The canvas Convert To submenu and its ?C dialog only accepted text and image shapes, so sticky notes and labelled rectangles fell through to a no-op. Both gates now call one helper, getConvertibleShapeText, which returns the dialog's initial text or null when the shape cannot convert. Geo and note shapes qualify only when they carry text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a976b32780
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (shape.type === "image") return ""; | ||
| if (!TEXT_SHAPE_TYPES.includes(shape.type)) return null; | ||
| if (!("text" in shape.props)) return null; | ||
| const text = shape.props.text.trim(); |
There was a problem hiding this comment.
When a text, note, or geo shape begins or ends with whitespace—for example, an intentionally indented multiline snippet—this trimmed value is passed to the node dialog, so conversion silently changes the shape's content and existing text shapes no longer behave as before. Use the trimmed value only to decide whether a note or geo shape is empty, while returning the original shape.props.text.
Useful? React with 👍 / 👎.
| }; | ||
| } | ||
| return () => {}; | ||
| return () => openDialogAndCreateShape({ initialText: shapeText }); |
There was a problem hiding this comment.
When one of the newly supported geo or note shapes is inside a frame or group, this route eventually calls replaceShapeWithDiscourseNode, which copies only the shape's local x/y and creates the replacement without its parentId. Tldraw then interprets those coordinates in page or newly inferred parent space, so the replacement can move far from the original or leave its container; pass the original parent and parent-relative transform through the replacement.
Useful? React with 👍 / 👎.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Could you run $dg-delegated-full-review and also address the codex comments? Thanks!
2 participants
https://www.loom.com/share/14e6c9d2102b49349869f7afd55a00de
The canvas already had a Convert To submenu, gated to a single
textorimageshape. Sticky notes and labelled rectangles hit thereturn () => {}fallback ingetOnSelectForShapeand did nothing. This widens the gate togeoandnote, and routes all three call sites through one helper so the right-click menu and the?Cdialog cannot drift apart.Stacked on #1427 (ENG-1356), which extracted
replaceShapeWithDiscourseNode. #1427 must merge first. Base is that branch, so this diff is only the 24 added lines.Reviewer brief
textandimagebehave as before.apps/roam/src/components/canvas/convertShapeToDiscourseNode.ts—getConvertibleShapeTextreturns the dialog's initial text, ornullwhen the shape cannot convert. Every gate is now=== nullon that one value, replacing theisTextSelected || isImageSelectedbooleans that existed in two files.Decisions worth challenging
TLArrowShapePropscarriestext. Arrows already convert to relationsisImageSelectedsurvives in both files. It no longer gates the submenu, but it still filterspage-nodeout of the type list, which is about images lacking a key-image flag rather than about convertibility."text" in shape.propsrather than a cast. It narrows the props union, soshape.props.texttypes asstringwith noasand no optional chaining. Same idiom asdefaultHandleExternalTextContent.ts:79. The precedingTEXT_SHAPE_TYPEScheck is the policy allow-list, not a redundant guard: it is what keeps arrows out.🤖 Generated with Claude Code