ENG-2227 Add link support to tldraw text shapes in Roam - #1441
Open
trangdoan982 wants to merge 3 commits into
Open
trangdoan982 wants to merge 3 commits into
trangdoan982 wants to merge 3 commits into
Conversation
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>
|
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. 1 Skipped Deployment
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Contributor
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)
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>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reviewer brief
apps/roamto reuse: the whole feature is stock tldraw 2.4.6, gated on the duck-type check'url' in shape.props(menu-hooks.ts:173), and the text shape has nourlprop. So this is a schema change, not UI wiring — once the prop exists, the context-menu item andEditLinkDialogwork unmodified becauseuiOverrides.tsxleavesContextMenu,ActionsMenuandDialogsat their defaults. Worth confirming that reading of the ticket.migrateStoreSnapshotreports success and silently does nothing, thenonValidationFailurerethrows even atphase === 'initialize'. It is version 6 of our owncom.roam-research.discourse-graphssequence rather thancom.tldraw.shape.text/3so a future tldraw release bumping its text sequence cannot collide.textagainst stock props and would rejecturl, so the worker now extends the default text schema with the prop. The first attempt declared"text"to the worker as if it were a custom shape type; that blanks the entry to{}, which also dropstextShapeMigrations, leaving the room atcom.tldraw.shape.textversion 0 against every client's 2 —Incompatible schema?on connect, and existing rooms failing to load. A delegated review caught it andsyncWorkerRoomSchema.test.tsnow pins both directions. The worker must be deployed with or before the extension, or text links in sync-mode canvases will fail validation server-side.getMigrationsSincewill not findcom.roam-research.discourse-graphs/6. This is inherent to the existing migration pattern (the same is already true of v5), but worth a release note.Verification
pnpm ci:validate— passing, 5/5 tasks, 0 cached.pnpm test:unit— 233 passed (7 new insrc/utils/__tests__/textShapeLink.test.ts). Tests cover link persistence, migration idempotency, and the eligibility gate.pnpm build— 0 errors; confirmedAddTextShapeUrlandtl-hyperlink-buttonare present indist/extension.js.The migration deliberately only writes when
url === undefined. Migration v3 originally assigned unconditionally and clobbered user values (#916); a test asserts double application is a no-op.Loom video
Not recorded.
Scope check
$scope-checkagainst the ENG ticket and final diff.Done When: The ticket asks to reuse the geo-shape code path; that path turned out to be stock tldraw rather than app code, so the implementation adds theurlprop plus a migration instead. Same user-facing result, different mechanism than the ticket describes. The sync-worker change is not named in the ticket but is required for text links to survive in cloud-sync mode.Local delegated full review
It found a release-blocking bug in the sync worker (above), a broken class-name concatenation on the link button, and that the original tests asserted against a hand-rolled schema rather than the real utils. All three are fixed in
9c42b8423; tests now derive their schema frombaseShapeUtilsso they fail if the util swap or the props ever regress.🤖 Generated with Claude Code