Repository navigation
ENG-2228 Add link support to tldraw text shapes in Obsidian - #1442
trangdoan982 wants to merge 19 commits into
Conversation
Text shapes could not carry a link, so a canvas narrative could not open the source its text stands for. tldraw gates its Edit link action on the duck-type `'url' in shape.props`, and TLTextShape has no such prop -- so this is a schema change, not UI wiring. Adding the prop lights up the stock menu item, and a retroactive migration backfills `url: ""` on pre-existing text records, which is required both for validation and for those shapes to become link-eligible. The ticket names page links as the primary target, so the prop uses a wider protocol allowlist than tldraw's T.linkUrl (which permits only http/https/ mailto) and tldraw's EditLinkDialog is forked, since it rejects non-http protocols on input before the schema sees them. The fork keeps geo shapes on the strict validator, as their url prop is still stock. Clicking an obsidian:// link resolves it in-app rather than via the OS protocol handler. Both shape-util registration sites now share baseShapeUtils: the schema and the editor are built from separate arrays and nothing asserts they agree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
The protocol allowlist admitted any obsidian: URI, and the click handler only called preventDefault once it had parsed an open URL -- so anything else (advanced-uri's commandid, new?file=&content=) fell through to the anchor and reached the OS handler, letting a shared canvas act on the reader's vault in one click. Only obsidian://open with a file param is accepted now, and any obsidian: href is swallowed regardless of whether it parses. The link icon sat on the text: tldraw parks it in a geo shape's empty top-right corner, but a text shape is exactly as big as its text. It now sits just past the right edge. The override needs both class names -- styles.css is concatenated ahead of the vendored tldraw CSS, so one class ties on specificity and loses on source order, which had silently dropped the zoomed-out hide too. Also restores the two guards the edit-link override had dropped, so the dialog can no longer open empty; stops an invalid Enter writing the "https://" fallback that both validators reject; scopes the obsidian:// hint to text shapes, since the dialog also serves geo; and makes the shape-util swap assert rather than silently no-op. Removes the unit tests and vitest setup at the author's request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s files The icon SVG now follows the TOOL_ARROW_ICON_SVG pattern and is applied inline the way tldraw's own HyperlinkButton does, so the vendored tl-hyperlink__icon class supplies size and color and the data URI is no longer duplicated in styles.css. processInitialData's schema fallback now drops only this plugin's own sequence ids. Substituting the full current schema told loadSnapshot the records were already migrated, so the url backfill was skipped and store.put threw. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
readDocumentRecords, the path that runs when the backing markdown changes on disk, had its own copy of the fallback and still substituted the full current schema. Both call sites now share schemaBeforeAppMigrations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
This needs a deeper review because it changes the built-in text shape’s schema, adds a migration, and introduces custom link components.
Before proceeding, I’d like us to explore storing the link in meta.url so we can keep the existing props schema and avoid backfilling existing text shapes. I’d lean toward that if it reduces the amount of custom code we need to maintain.
Please also investigate how much of tldraw’s existing link button and dialog we can reuse, including whether we can extend URL support to accept obsidian:// consistently for both text and geo shapes. I’d consider a small pnpm patch if it lets us reuse those components instead of maintaining separate copies. (Here's a simple example of us extending the hostnames for embeds using patch package)
The goal is to stay closer to the stock text shape and link UI. Please compare that approach with the current implementation before we commit to the schema change.
Regardless of your choice, could you include a more detailed Loom video testing and showing different possible surface areas, like adding a text shape via the toolbar, adding the initial link to the text shape, converting the text shape to a discourse node, etc.
Lastly, could you compare the click behavior to existing click behavior of Discourse nodes? We should mirror those as much as possible (I acknowledge that there are two separate behaviors we would need to balance here: existing dg links and geo shape links). If a user clicks a Discourse node and they go directly to that node or shift-click to open the sidebar, but they have different behavior when they click a text shape, that would be unexpected and poor UX, for example.
…port-to-tldraw-text-shapes-in-obsidian
Michael asked us to compare meta.url against the props schema change before committing to it, and to see how much stock tldraw we could reuse. Both point the same way. meta is validated as T.jsonValue, so existing canvases load untouched: the migration, the schema fallback and its two call sites are gone, along with the code flagged in review. The deciding argument is compatibility, not code volume -- a file written with props.url and opened by an older build throws Unexpected property and renders a blank canvas, while meta.url loads, is ignored, and round-trips intact. Canvas files are markdown synced between machines on different plugin versions, so that mattered more than keeping the props-gated menu item for free. Rendering the icon from InFrontOfTheCanvas, next to the existing Relations and DragHandle overlays, means the stock text shape is never subclassed at all, so the custom shape util and the two-shape-util-lists hazard go with it. The trade is that the icon does not rotate with the shape, is not clipped by a frame, and draws above shapes stacked over the text. A two-line patch keeps tldraw's own Edit link item: useHasLinkShapeSelected gates on 'url' in props, so it now also accepts text shapes -- always, the way geo qualifies through a props.url that defaults to "". Gating on the presence of meta.url instead would have made the first link unaddable. Click gestures now match discourse nodes: plain opens the sidebar, cmd opens a new tab, cmd+alt splits. Since meta is unvalidated, the URL allowlist is re-applied when reading it rather than relying on the schema. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
apps/obsidian/AGENTS.md forbids hardcoded inline styles. Only the per-shape left/top can be inline; the container, the button placement and the icon mask are now classes, with the mask data URI passed as a custom property. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A delegated review found the allowlist and the click guard disagreeing. `new URL` strips leading control characters, so " obsidian://open?vault=Other&file=x" passed `isAllowedTextLinkUrl` while a raw prefix test said it was not an obsidian URL -- so preventDefault never ran and the anchor handed the URI to the OS handler, skipping the cross-vault check. getTextShapeLinkUrl now returns the parsed form and every protocol test derives from the parser. Host comparison is case-insensitive too: obsidian: is a non-special scheme, so WHATWG left obsidian://Open uppercased and the link was rejected outright. A page link is no longer placed in href at all. Middle-click and the context menu bypass onClick, so the URI escaped that way regardless of the guard. Also from the review: the wrong-tool branch of the edit-link override returned instead of falling through, so invoking it from a non-select tool switched tool and silently did nothing; upstream opens the dialog. The dialog's selection-changed guard compared types and could never fire, because the tracked parent re-renders with the new shape -- keying the inner component by shape id resets its state instead. The overlay now culls to the viewport and skips hidden shapes rather than scanning the whole page each camera frame. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Entire-Checkpoint: 01M3TGQGPWAVEG55KD1P9NKX28
|
@mdroidian the loom and PR body is updated with the new approach you suggested. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@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: 5253da0dc7
ℹ️ 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".
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Here I asked "Please also investigate how much of tldraw’s existing link button and dialog we can reuse ... I’d consider a small pnpm patch if it lets us reuse those components instead of maintaining separate copies."
I see a new TextLinkDialog.tsx but I didn't see a reason/explanation why we wouldn't reuse the existing dialog. Did you investigate reusing the existing dialog (eg: exporting it via a patch)?
If yes, please add the findings/explanation of why we chose to not use this method.
If no, please investigate and report your findings.
|
@trangdoan982 also, re:
I see gesture alignment mentioned in the PR body, but I’m not sure where to find the comparison. Did you compare text-link click behavior with existing Discourse node and geo shape links? Could you summarize any changes made to match those behaviors and any decisions about differences that should remain? If that’s already documented, please point me to it. Thanks! |
Michael's review of the Obsidian twin (ENG-2228, #1442) asked to store the link in meta.url and reuse tldraw's own link UI instead of changing the text shape schema, migrating, and copying components. The same reasoning applies here more strongly: a Roam graph has collaborators on different extension versions, and each sequence bump locks out whoever has not updated yet. This removes the custom text shape util, the shared util list, migration v6 and the sync worker change. The meta-based implementation follows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Not for merge -- a comparison branch against 5253da0, which ships a two-line eligibility patch and keeps a vendored hyperlink button. Patches @tldraw/validate so linkUrl accepts obsidian://open?file=, editing the check body rather than the protocol Set so advanced-uri's commandid stays rejected. Patches tldraw to export HyperlinkButton, dispatch a cancelable tldraw.navigate-link event from it, and keep non-web URIs out of href. One listener in the app then serves every link button tldraw renders, so geo, note and image shapes get the same in-app open and cross-vault check as text. App code drops 477 -> 434 lines: the vendored button and our own allowlist go, and the dialog's text-vs-geo validator split collapses. Patch surface grows 26 -> 241 lines across two packages, and becomes a behaviour patch on a shared component rather than a predicate flip. Verified over CDP: the five text-link scenarios still pass, and a geo shape now accepts an obsidian page link, omits href, and opens it in the sidebar. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Entire-Checkpoint: 01M3WAA741724YSM5FFQ91Q97J
Validation was unified across shape types but the hint still told geo shapes "Enter a URL." Also drops the unused T import and the never-narrowed isValid call that tripped two lint warnings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@mdroidian i created this Loom to discuss 2 options on how much we patch to unify the experience for text and other tldraw shapes, re: your question https://www.loom.com/share/3b3e00004560475db042ee5604647e2a more detailed considerations if you prefer text: https://claude.ai/artifact/PF19PibST9JmHjkVuNWrxF?sk=aqEDLbGKXhtBeuYz4H0mOw let me know what you think. |
* ENG-2227 Add link support to tldraw text shapes in Roam
Text shapes could not carry a link, so a canvas narrative could not point at
its detailed source. tldraw gates its built-in Edit link action on
`'url' in shape.props`, and the stock text shape has no such prop, so the fix
is schema-level rather than UI wiring.
Subclass TextShapeUtil to add a `url` prop and render the hyperlink button,
and replace the stock util through a shared list so all four stores agree.
Existing text shapes are backfilled by migration v6 of the repo's own
sequence; without it they fail validation and the canvas will not open.
The sync worker validates `text` against stock props, so the client now
declares it alongside the custom shape types.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Fix sync room schema and move the text link icon out of the glyphs
Declaring "text" to the sync worker as if it were a custom shape type blanked
its schema entry to {}, which dropped textShapeMigrations. The room then
reported com.tldraw.shape.text version 0 against every client's 2, so
getMigrationsSince returned 'Incompatible schema?' and every client was
rejected on connect while existing rooms failed to load. The worker now
extends the default text schema instead, keeping its migrations, and refuses
to blank any default shape type. Covered by syncWorkerRoomSchema.test.ts.
A text shape's bounds hug its glyphs, so tldraw's in-bounds link button landed
on the last word. Sit it just outside the right edge instead.
Also fix a missing separator that merged the two hyperlink button class names
into one invalid token, guard isTextShapeRecord against a null props object,
and assert that the text util was actually replaced.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Remove unnecessary type assertions in sync worker schema test
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Revert props-schema approach for text shape links
Michael's review of the Obsidian twin (ENG-2228, #1442) asked to store the
link in meta.url and reuse tldraw's own link UI instead of changing the text
shape schema, migrating, and copying components. The same reasoning applies
here more strongly: a Roam graph has collaborators on different extension
versions, and each sequence bump locks out whoever has not updated yet.
This removes the custom text shape util, the shared util list, migration v6
and the sync worker change. The meta-based implementation follows.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* ENG-2227 Store text shape links in meta and reuse tldraw's link UI
Text shapes keep their link in shape.meta.url, so the stock text schema is
unchanged: no migration, no sync worker change, and a canvas saved by this
build still opens in an older one, which ignores the link and round-trips it.
The existing tldraw patch now makes text shapes eligible for the stock Edit
link action unconditionally (gating on meta.url would stop a shape ever
getting its first link), has EditLinkDialog read and write meta.url for text,
and exports HyperlinkButton. An InFrontOfTheCanvas overlay renders that button
just outside each linked text shape, since text bounds hug their glyphs.
meta has no schema, so the overlay validates with T.linkUrl and renders the
parsed URL; relative paths linkUrl would resolve against a dummy origin are
rejected.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Co-locate the text link guard with its only caller
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Entire-Checkpoint: 01M3TK0B5ZF8BWDS8PXDES5Y8Y
* Hide the text link icon while its shape is selected
The overlay renders above the selection foreground, so a 44px button at the
right edge swallowed drags on the right resize and rotate handles and opened
the link instead.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Entire-Checkpoint: 01M3TX7QGKXMB58AK5840S5CQ8
* Hide text link icons that overlap the selection handles
The overlay renders above every shape and handle, so hiding the icon only
for its own shape still let it cover a neighbouring selection's resize,
rotate, and relation drag handles. Hide any icon within handle reach of the
selection box instead.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Entire-Checkpoint: 01M3W9CHZ0FXJKXJVVRP73J6NE
* Keep the text link icon visible while the shape is selected
Hiding the icon near the selection kept it off the resize, rotate, and
relation handles, but the icon disappearing on selection was the larger
cost. The icon now always shows and stays clickable, so a drag that starts
under it on a short text shape opens the link instead of resizing.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Entire-Checkpoint: 01M3WDDCGRQEZ0D8949E9E23XB
* Use Tailwind for the text link overlay's static styles
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Entire-Checkpoint: 01M46AF6E7M6V0KQY6S76ZEX9Q
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Discussed and decided on Option 2 PR #1504 |
…port-to-tldraw-text-shapes-in-obsidian
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The CJS hunk called a bare useCallback the file never imports. The listener now normalises stored geo links before parsing, since the patched validator accepts `Obsidian://` and leading whitespace. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Stock edit-link is a no-op outside the select tool; the override switched tools instead. The text-link button sat on the shape's right-edge resize target, so a press there opened the link rather than resizing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4H0A8S531N9Q5XZTXNYKCNC
A bookmark links its title with a raw href rather than HyperlinkButton, so a page link there would reach the OS handler and skip the same-vault check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4H0NW3MN4QX44TBKQFRF6J6
The missing-protocol fallback parsed `https://obsidian://open?…` as a valid https URL, so a bookmark still saved a mangled link. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4H0R1R4MVE9PNY1BE3QF3YM
The input starts as https://, so pasting a page link after it produced https://obsidian://…, which parses as a web URL and saved silently. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4H3VPANDJZBD31YAET7ZWGM
An obsidian:// value the validator rejects fell through to the missing-protocol fallback and saved as https://obsidian://… on every shape. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Entire-Checkpoint: 01M4H47KFXF427H52MWP79Q6ZT
This branch was successfully deployed
2 participants
Reviewer brief
Result: Text shapes get tldraw's Edit link action. Text, geo, note, image and video shapes can all hold an
obsidian://open?file=…page link, and clicking it opens the note inside Obsidian. Web links work as before.This follows the decision on the approach comparison: patch tldraw so every shape shares one URL rule and one link button, rather than giving text shapes their own. The comparison branch was #1504, now closed.
Follow a link from the menu item to the click. Orange is patched tldraw, blue is our code.
flowchart TD A["text shape selected"] --> B{"patches/tldraw@3.14.2.patch<br/>useHasLinkShapeSelected: url in props OR type === text"} B --> C["stock Edit link item<br/>TldrawViewComponent.tsx swaps onSelect only"] C --> D["TextLinkDialog.tsx<br/>one validator for every shape"] D --> E{"patches/@tldraw__validate@3.14.2.patch<br/>linkUrl accepts obsidian://open?file="} E -->|text| F["shape.meta.url<br/>TextLinkOverlay.tsx draws tldraw's button"] E -->|geo, note, image, video| G["shape.props.url<br/>tldraw draws its own button"] F --> H{"patched HyperlinkButton<br/>no href for obsidian:, fires tldraw.navigate-link"} G --> H H --> I["utils/linkNavigation.ts<br/>one listener: same-vault check, then sidebar / Cmd new tab / Cmd+Alt split"] classDef patched fill:#fed7aa,stroke:#c2410c,color:#111 classDef app fill:#bfdbfe,stroke:#1d4ed8,color:#111 class B,E,H patched class C,D,F,G,I appReview focus:
props.urlis checked every time a canvas loads. A tldraw withoutpatches/@tldraw__validate@3.14.2.patchrejects these shapes and can't load the canvas. That includes older plugin versions and any future upgrade that drops the patch. Text links avoid this by living inmeta, which tldraw never checks. This is the trade-off accepted in choosing this approach.obsidian://openwith afileparam passes. Addingobsidian:tovalidLinkProtocolswould also admit action URIs such asadvanced-uri?commandid=. The scenario matrix pins both rejections.hrefoff for non-web URLs, so middle-click and "open link" can't hand them to the OS handler.linkNavigation.tscancels the event and resolves the file inside this vault only.<a href>, notHyperlinkButton, so a page link there would reach the OS handler.TextLinkDialog.tsxrejectsobsidian:input for bookmarks, and keeps everyobsidian:input away from itshttps://fallback, which would otherwise parsehttps://obsidian://…as a web URL.Size: 682 changed lines excluding the lockfile; 241 of them are the two patch files. It doesn't split usefully: the validator patch, the button patch and the listener only work together, and without all three a page link either can't be saved or escapes to the OS. Review order:
patches/@tldraw__validate@3.14.2.patch, thenpatches/tldraw@3.14.2.patch,utils/linkNavigation.ts,overlays/TextLinkOverlay.tsx,TextLinkDialog.tsx. Testing path: add a text shape from the toolbar, right-click, Edit, Edit link, paste a page's Obsidian URL.Risk or follow-up:
allowUnusedPatches: false).selectNone()targets the old editor; the link still opens. The existing Meta+Alt+Enter effect has the same dependency pattern.obsidian://links in the button can't be activated with Enter, because an anchor withouthrefgets no keyboard activation. Stock tldraw never accepted these links, so this isn't a regression.convertToDiscourseNodecopies neitherpropsnormeta. This predates the PR.Verification
pnpm install --frozen-lockfileandpnpm ci:validatepass with the turbo cache forced off (0 of 14 tasks cached). Prettier is clean on every changed file. ESLint reports no warnings on added lines; CI'slint-changed-filesfails on warnings, not just errors.Driven in Obsidian over CDP with
dg-obsidian-cdp-verifyagainst this head:Tests added: none. Rerun the scenarios with:
Not verified:
@tldraw/validaterejecting the record was checked directly.Loom video
pendingScope check
$scope-checkagainst ENG-2228 and the final diff.Done When:obsidian://open?file=page links, a link format the ticket lists as out of scope; the same page links on geo, note, image and video shapes, which already had web links; pnpm patches totldrawand@tldraw/validate, including a behaviour change to tldraw's shared link button; and click gestures matched to discourse nodes.linkUrlcan't express one. Once tldraw's validator accepts page links, every shape type that uses it accepts them, so the other shape types follow from the first change. The patches and gestures were requested in review.One
Done Whenitem is not met: "Unit tests cover link persistence and the text-shape eligibility behavior."Standards check
$dg-pr-adherence-checkagainst the final diff and PR metadata.Outstanding:
STYLE_GUIDE.mdasks for unit tests for new functionality, andtextShapeLink.tsis pure logic. This also leaves the ticket's fourthDone Whenunmet.externalContentHandlers.tscarries three formatting-only hunks. The pre-commit hook runs Prettier over the whole file onceresolveObsidianUrlToFileis exported.Local delegated full review
Six rounds; each fix restarted the checks. The final round, on this head, found nothing new at medium or above.
Fixed in this PR:
HyperlinkButtoncalled a bareuseCallbackthat file never imports. The plugin bundle loads the ESM build, so it didn't fail at runtime.Obsidian://…or with leading whitespace passed the validator but failed the listener's raw prefix check, showing "not in this vault". The listener now normalises withnew URLfirst.edit-linkoverride switched tools where stock is a no-op outside the select tool.https://saved ashttps://obsidian//….obsidian://open?vault=Vwith nofile, fell through to thehttps://fallback and saved as a web link. Everyobsidian:input now stays out of that fallback.TldrawViewComponent.tsx, and a double cast inlinkNavigation.ts.Raised and not acted on, with reasons in Reviewer brief: older builds and an unpatched tldraw can't load canvases with page links on non-text shapes (the accepted trade-off), Enter doesn't activate a page-link button, and the listener's stale editor after a store swap. Also not acted on:
TextLinkOverlayreturns a new array on every camera change, so it re-renders on each pan even with no text links. Perf only; a follow-up can return a stable empty value.Raised and found incorrect against tldraw 3.14.2's source:
updateShapesreplacing all ofmeta(it merges key by key), andBox.includesrequiring full containment (it's collides-or-contains). The two matching Codex threads are answered and resolved.🤖 Generated with Claude Code