fix(design-system): LinkAction refuses tone; correct #270, #118 and #269 by measurement - #1720
Conversation
…ing it
LinkActionProps extended BaseProps, which declares tone?: 'accent' | 'inherit',
but LinkAction destructures only { href, children, className }. tone='inherit'
therefore type-checked, read as working, and did nothing — while TextLink,
ExternalTextLink and DownloadLink all honour it. Omitting tone from the props
makes the compiler reject the call instead.
Refusing rather than implementing: a forward action is the accent by design, which
is what makes it read as the card's next step rather than as prose, so there is no
'inherit' variant to select. LinkAction has zero production importers today, so
this is free to change now and expensive once call sites exist.
.design-sync/config.json is regenerated, not hand-edited: npm run
design-system:design-sync:update. Changing a public *Props type without it fails
check:design-sync-contract.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e mechanism #270 said 22 call sites pair a tap token with a dead numeric height and that declaring `tap` in tailwind-merge would drop 18 production targets from 48px. Re-measured against origin/main 00826bc and none of that holds at this HEAD. Method: scan every string literal in src (mockups excluded, comments blanked — backticks inside JSX comments otherwise let a literal scanner span lines and invent hits), then group height utilities by variant prefix AND property, and flag a numeric only where a tap token shares its group. Result: zero same-variant pairs in components, 84 cross-variant ones. DocumentManagerPanel.tsx and settings-dialog.tsx carry no tap token at all any more. The survivors are responsive step-downs, not dead classes: `min-h-tap` with `sm:min-h-9`, `h-10.5` with `sm:h-tap`. The later variant wins at its breakpoint, so deleting the numeric raises the control — a visual change, not the no-op the row promised. And tailwind-merge groups by variant, so the stated blocker is false: a probe over the real pairings returns identical output with and without `tap` declared; only synthetic same-variant controls change. What stays open is narrower and is now written down: the per-literal scan cannot see a conflict composed across cn() arguments, where order decides whether a recipe raises to 48px or drops to 28px. #118 records the owner's 2026-08-09 decision (adopt, advisory-only first, promote after a soak) and the sequencing constraint — baselines frozen while design changes are still merging go red on the next merge. #269's 'baselines cannot be generated on Windows' is corrected: true of win32 PNGs, but the ubuntu CI job already produces the ones that count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 24 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR removes the unsupported ChangesLinkAction contract
Review documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/branch-review-ledger.md`:
- Line 747: Update the ledger entry for claude/ds-doc-corrections so its scope
includes src/components/ui/link.tsx, .design-sync/config.json, and
docs/outstanding-issues.md entries `#118`, `#269`, and `#270`, and record the reported
TypeScript, lint, design-system contract, icon-scale, targeted test,
outstanding-issues, and Prettier checks. If retaining the docs-only scope,
assign it a separate ref and head instead of identifying it as PR `#1719`, using
the ledger workflow.
In `@docs/outstanding-issues.md`:
- Line 215: Update the dated evidence in the outstanding-issues entry for `#118`
and the related entries `#310`–311 so it does not reference August 9, 2026 before
that date has occurred. Replace those dates with the actual evidence date, or
move the updates to a post-August 9 entry while preserving the documented
decisions and measurements.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3334aebc-8394-4e2c-8474-e29aab91d1f0
📒 Files selected for processing (4)
.design-sync/config.jsondocs/branch-review-ledger.mddocs/outstanding-issues.mdsrc/components/ui/link.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 824c1b74a2
ℹ️ 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".
Same defect Codex flagged on PR #1719: the commits recording these measurements were authored 2026-08-08 (07:xx UTC), so a 2026-08-09 stamp places every re-measurement after the commit that recorded it. Four occurrences, in the #118, #269 and #270 rows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…y a literal Codex review on PR #1720, and it is right. Omit<BaseProps, "tone"> only closes the object-literal path: excess-property checking runs on literals, so <LinkAction tone="inherit" /> is rejected, but a caller who builds props in a variable or wrapper and writes <LinkAction {...props} /> is doing an ordinary assignment, where extra properties are allowed. Verified rather than taken on trust — a focused tsc probe accepted the spread form against the Omit type with no diagnostic, and rejected it against tone?: never with TS2345. So the advertised refusal was still silently rendering the accent for the one call shape most likely to carry a stale tone. tone?: never rejects both forms. The contract test asserts the spread case at the type level, so it stops compiling if the prop ever widens back to something assignable, plus a render assertion that the accent is what actually ships. Manifests regenerated, not hand-edited: design-system:design-sync:update picks up tone?: undefined, and design-system:adoption:update records the new test file against LinkAction. Adoption counts are unchanged at 53 registered / 30 adopted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CodeRabbit flagged the claude/ds-doc-corrections row as understating the scope of PR #1720. That finding conflates two things: the row records a different ref at a different head, and it was accurate as written. But it does predate the review pass on #1719, whose scope grew by one commit afterwards, so a superseding row is warranted on its own merits — appended through ledger:append --supersede rather than editing the append-only original. Same for #1720's own row, which recorded 824c1b7 and now supersedes to 6916c80 with the tone?: never fix and its type-contract test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review outcome for the post-sync tip after the sm:py guard fix and late main sync (#1720). Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Behind-but-clean against origin/main; clear GitHub mergeability DIRTY so required CI can build the merge ref. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
* Fix safety plan phone header safe-area collision Own the OS top inset on the standalone tool header and restack the phone chrome so the back control sits below the status bar. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * fix(search): stop Scope Escape from reopening command listbox Focus restore onto the composer + trigger was reopening the documents actions panel via onFocusCapture, covering Browse library and failing the deferred-request @critical journey after the Button ref sync. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * test(safety-plan): reject sm:py axis padding in header guard Match the standalone shell contract so breakpoint-prefixed py utilities cannot bypass the safe-area top-pad regression check. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * docs(ledger): record PR #1711 heavy review-and-fix Review outcome for the post-sync tip after the sm:py guard fix and late main sync (#1720). Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Summary
Three commits, each independently revertible. One small source fix plus the ledger corrections that came out of measuring the next item rather than doing it.
LinkActionrefusestoneinstead of silently ignoring it (src/components/ui/link.tsx).LinkActionPropsextendedBaseProps, which declarestone?: "accent" | "inherit", butLinkActiondestructures only{ href, children, className }.tone="inherit"therefore type-checked, read as working, and did nothing — whileTextLink,ExternalTextLinkandDownloadLinkall honour it. Refusing rather than implementing: a forward action is the accent by design, which is what makes it read as the card's next step rather than as prose, so there is noinheritvariant to select.LinkActionhas zero production importers today (docs/design-system/adoption-manifest.json), so this is free to change now and expensive once call sites exist. Closes#276..design-sync/config.jsonis regenerated bynpm run design-system:design-sync:update, not hand-edited.#270corrected — its premise did not survive re-measurement. The row said 22 call sites pair ataptoken with a dead numeric height and that declaringtapinCLINICAL_TWMERGE_THEME.spacingwould drop 18 production targets from 48px. Measured againstorigin/main, none of that holds at this HEAD:DocumentManagerPanel.tsxandsettings-dialog.tsxcarry notaptoken at all any more;document-adminandservice-detail-pagehavetapsites with no numeric height.min-h-tapwithsm:min-h-9/lg:min-h-9/md:min-h-9, andh-10.5withsm:h-tap. The later variant wins at its breakpoint, so deleting the numeric raises the control. That is a visual change, not the no-op the row promised.min-h-tapandsm:min-h-9are never in one conflict group. A probe over the real pairings returned identical output with and withouttapdeclared; only synthetic same-variant controls (min-h-tap min-h-9,min-h-9 min-h-tap,h-tap h-5) changed.cn()arguments, where order decides whether a recipe raises to 48px or drops to 28px. That sweep is the real remaining work, and it is why no source file was touched here.#118and#269corrected on the visual baselines.#118records the owner's decision — adopt, advisory-only first, promote topr-requiredonly after the soak the workflow comment asks for — and the sequencing constraint that baselines frozen while design changes are still merging go red on the next merge.#269's "baselines cannot be generated on Windows" is corrected: true of win32 PNGs, becausesnapshotPathTemplatecarries{platform}, but thevisual-baselinejob onubuntu-24.04already produces the ones that count and uploads them asvisual-baseline-<run_id>.Also carries the
docs/branch-review-ledger.mdrow for PR #1719, appended withnpm run ledger:appendrather than pushed as a ledger-only tip on that PR.Verification
Note on the environment first, because it invalidated an earlier gate run: this worktree's
node_moduleswas left partially written by twonpm installruns that were killed mid-extraction (a concurrent session rannpm cache clean). Symptom was a zero-byte@testing-library/dom/types/index.d.tsand a six-file-short@types/node, which produced 163 phantom typecheck errors. Measured identically with and without this diff, so no finding here rests on that state; it was repaired by a clean reify from the lockfile before any gate below was run, andpackage.json/package-lock.jsonare untouched.node ./node_modules/typescript/bin/tsc -p tsconfig.typecheck.json --noEmit— exit 0, zero diagnostics (captured to a file, not piped, so the exit code is tsc's own).npm run lint— exit 0, no findings.npm run check:design-system-contract— exit 0:design-system adoption checked: 53 components, 55 roots,design-sync contract checked: 53 components and 7 guidelines.node scripts/check-icon-scale.mjs --strict— exit 0:✓ icon-scale: no retired 4.5 (18px) half-step icon sizes in src.Run explicitly because it is not part ofcheck:design-system-contract.node node_modules/vitest/vitest.mjs run --pool=threads --no-file-parallelism tests/ui-v2-components.dom.test.tsx tests/design-sync-visual-exports.test.ts tests/ui-primitives.dom.test.tsx tests/tailwind-merge-config.test.ts—Test Files 4 passed (4),Tests 175 passed (175).npm run check:outstanding-issues— passed:274 rows (131 open, 143 archived), unique ids, next-id=277 above the highest, no merge driver, no ids deleted from base 00826bca2381.npx prettier --check .— passed whole-tree:All matched files use Prettier code style!LinkActionhas zero production importers, so this diff renders nothing that is on a route today. The ledger commits are docs-only.Clinical Governance Preflight
scripts/pr-policy.mjsclassifies this diff as non-clinical-risk, butAGENTS.mdasks for the preflight whenever source rendering or clinical output could be involved, so it is written out rather than skipped.LinkActionis a navigation affordance with zero production importers; no answer, citation, source-rendering or document-access path is touched.src/lib/rag/**, clinical-search, retrieval-selection, ranking-config or the eval harness is touched. RAG impact: no retrieval behaviour change — the diff is one component prop type, one generated design-sync entry, and three documentation rows.git revertthe single source commit; the two docs commits are independent of it.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation