ENG-1892 Add 'convert to' node option to right click menu for tldraw shape to allow user to convert shape to node (Obsidian) - #1432
Conversation
Extend the canvas right-click "Convert To" submenu beyond text and image shapes to geo (labelled rectangles, ellipses, etc.) and note (sticky) shapes, gated on the shape actually having text. Menu gating and converter dispatch now share canConvertShapeToNode so they cannot drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…adonly Rename the allowlist to RICH_TEXT_SHAPE_TYPES so it is obvious that getShapeText only reads props.richText -- adding a shape that stores its label elsewhere (arrow, frame) would otherwise silently do nothing. Also disable the Convert To items in readonly canvases, matching the Relation items, and note that images are gated at conversion time. 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: 8acf5b0aa8
ℹ️ 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".
| } else if (RICH_TEXT_SHAPE_TYPES.includes(shape.type)) { | ||
| return convertTextBearingShapeToNode(args); |
There was a problem hiding this comment.
When a newly supported geo or note is a child of a frame, this dispatch eventually passes its parent-relative shape.x/shape.y to editor.createShape without preserving parentId or converting the coordinates to page space. The replacement node can therefore jump elsewhere on the canvas or be assigned to the wrong container. Preserve the original parent and transform, or derive page coordinates before creating the node.
Useful? React with 👍 / 👎.
| @@ -95,6 +100,7 @@ export const CustomContextMenu = ({ | |||
| id={`convert-to-${nodeType.id}`} | |||
| label={"Convert to " + nodeType.name} | |||
There was a problem hiding this comment.
Let's take this opportunity to drop the duplicate "Convert to" from the submenu
We did this already in Roam:
ENG-1336: Remove redundant "Convert To" in tldraw context menu
2 participants
https://www.loom.com/share/012f47508e87410db4aaa383da36d25d
Reviewer brief
geoshapes (labelled rectangles, ellipses, etc.) andnote(sticky) shapes, not justtextandimage. It is gated on the shape actually having text, so an unlabelled rectangle shows nothing.arrowis deliberately excluded — it owns the separate "Relation" submenucreateDiscourseNodeShapepasses the shape's parent-relative x/y toeditor.createShapewithout aparentId, which treats them as page coordinates. Converting a shape parented to a frame would place the node at the wrong position. This is pre-existing and affects text and image conversions identically, so it is left out of this diff — but sticky notes and labelled rectangles inside frames are common, so this change makes it easier to hit. I did not independently reproduce it. Same for two other pre-existing items:markHistoryStoppingPointis called after the mutations rather than before, and an image whose asset does not resolve to a vault file converts to an empty-titled node.Verification
Driven against a running Obsidian over CDP, after confirming the loaded bundle was this branch's build:
Full conversion on a labelled rectangle: submenu listed every configured node type, the modal opened pre-filled with the shape's label, and confirming created the node file, removed the geo shape, and placed the node card at the original x/y.
pnpm install --frozen-lockfile+pnpm ci:validatepass (5/5 tasks, 0 cached).eslinton both changed files reports no errors and no warnings.Scope check
$scope-checkagainst the ENG ticket and final diff.Done When: readonly-mode gating and the allowlist rename came from the delegated review, not the ticket. Otherwise, the ticket'sDone Whenis empty and the parent (FEE-824) specifies only "convert tldraw shape to discourse node (if it has text in it)". The text/image case already shipped, so this change covers the remaining text-bearing shapes. No other behaviour changed; toast copy was generalised off "text" wording.Local delegated full review
useValuegate is genuinely reactive torichTextedits and cheap (rich-text rendering memoizes per object), that the allowlist matches the tldraw 3.14.2 schema exactly, and that the menu gate and converter dispatch cannot disagree. Its actionable points on this diff are applied in the second commit; the rest are the pre-existing items noted above.🤖 Generated with Claude Code