fix(tailwind-merge): declare the tap spacing token now its blocker is disproved - #1738
Merged
Conversation
… disproved `tap` was held out of CLINICAL_TWMERGE_THEME.spacing because declaring it would hand same-property conflicts to the later class -- "22 call sites, 18 of which would drop from 48px to 32/36/40/42px". #270 re-measured that premise and it does not hold: zero same-variant pairs exist, and the 84 survivors are cross-variant responsive step-downs that tailwind-merge cannot reach because it groups by variant. Several named call sites were stale outright -- DocumentManagerPanel and settings-dialog carry no tap token at all. The gap that kept #270 open was that a per-string-literal scan cannot see a conflict composed across cn() arguments: cn(recipe, "min-h-tap") pairs the recipe's min-h-7 with the token, and order decides the outcome -- recipe-then-tap raises to 48px, tap-then-recipe drops to 28px, the forbidden direction. So the sweep resolves constant recipe identifiers at each call site before grouping by variant AND property. Result across 700 files and 1418 cn() call sites (1410 with resolvable arguments, 802 resolvable class constants): ZERO same-variant pairs, in either direction. Two defects were found and fixed in the sweep itself before trusting that zero, because its first run reported three drops and all three were artefacts: - Ternary arms were concatenated, so `size === 44 ? "h-tap w-tap" : "h-[38px] w-[38px]"` looked like one element carrying both. Arms are mutually exclusive; arguments now expand to alternatives and compositions are enumerated. - Class constants were keyed by bare name globally, so `fieldControl` in one module resolved to a same-named constant in another and invented a conflict that does not exist at the call site. Resolution is now per file, with a global fallback only where a name is unambiguous repo-wide. The zero is mutation-tested rather than assumed: synthetic probes cn("h-tap", "h-4") and cn("min-h-tap", recipe) are both flagged as drops, cn(recipe, "min-h-tap") is correctly reported as a raise, and cn("min-h-tap", "sm:min-h-9") is correctly not flagged at all. The pinning test is replaced rather than deleted. It now asserts both halves: a same-variant pair merges to the last class, and cross-variant responsive step-downs pass through untouched. That second case is the load-bearing one -- if it ever merges, a control loses its breakpoint step. No production tap target is lowered, and no cross-variant numeric is deleted -- they are live responsive steps, not dead classes. Verified: tests/tailwind-merge-config.test.ts 34 passed (34); npm run test -- 5590 passed, 2 pre-existing environmental failures unchanged from the same run before this change (mode-nav-addon-slot absolute Windows paths, pr-handoff-stop); check:design-system-contract, check:icon-scale, check:type-scale all exit 0; format:check clean. Chromium look is delegated to this PR's Production UI jobs and is not claimed as run locally. Refs #270
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 59 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 (2)
Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
8 tasks
BigSimmo
added a commit
that referenced
this pull request
Aug 9, 2026
…lection (#262 parts 2 and 3) (#1780) * docs(issues): close #218 and #270, both shipped before this session Both rows were still open in docs/outstanding-issues.md while their work was already live on main, which had scoped a third session from them. #218 (cn() lacks tailwind-merge) shipped in PR #1678, aeba5a2. src/components/ui-primitives.tsx:37 is twMergeClinical(...) rather than a plain join, package.json carries tailwind-merge ^3.6.0, and src/lib/tailwind-merge.ts declares the repo's @theme scales to twMerge. #270 (declare the tap spacing token) shipped in PR #1738, 80cf781, an ancestor of origin/main. "tap" is present in CLINICAL_TWMERGE_THEME.spacing, and tests/tailwind-merge-config.test.ts was inverted rather than deleted so the merge behaviour is now asserted rather than pinned out. Verified in source at origin/main 7aaf934, not inferred from the handover. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(design-system): ratchet raw padding, radius and line-height literals (#262 part 3) The design-system contract ratcheted colour, shadow, tap and tracking but not spacing, radius or line-height, so a value could bypass the scale as a bare literal in either a class or a stylesheet and nothing objected. Adds three per-path ratchets, covering both halves the way the colour and legacy-shadow metrics already do: rawPaddingLiterals 67 (17 CSS declarations, 50 class utilities) rawRadiusLiterals 24 (22 CSS declarations, 2 class utilities) rawLineHeightLiterals 3 (3 CSS declarations) The exemption is deliberately "contains no CSS function", not the narrower `(?!var\()` the tracking rule uses. Padding is not only ever a token or a literal: production ships pb-[env(safe-area-inset-bottom)], pt-[max(0.75rem,var(--safe-area-top))], pt-[clamp(1.5rem,5vh,3rem)] and pb-[calc(7rem+env(safe-area-inset-bottom))]. Those are computed from the viewport or the safe-area inset, cannot be spelled as a scale step, and a `var(`-only lookahead would have flagged every one of them. On the CSS side, zero in any unit, the CSS-wide keywords and custom-property declarations (the token definitions themselves) are exempt for the same reason. Every one of the 94 baseline entries was verified present at its cited line before pinning, and the baseline change is additive: all fifteen pre-existing metrics and every pre-existing debtByPath entry are byte-identical. Mutation-tested rather than assumed. Class side, in a file with no prior debt: - rawPaddingLiterals increased from 67 to 68 - rawPaddingLiterals at src/components/ui-primitives.tsx increased from 0 to 1 - rawRadiusLiterals increased from 24 to 25 - rawRadiusLiterals at src/components/ui-primitives.tsx increased from 0 to 1 - rawLineHeightLiterals increased from 3 to 4 - rawLineHeightLiterals at src/components/ui-primitives.tsx increased from 0 to 1 The CSS half fails the same way. Both probes also carried the sanctioned computed forms, and each count rose by exactly one, so the exemptions are proved by the same runs rather than argued. No new npm script: the metrics live inside check:design-system-contract, so docs:check-inventory and check:gate-manifest are untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(design-system): gate type-step selection on the decidable half (#262 part 2) check:type-scale blocks arbitrary text-[12px] values. Nothing has stopped the scale itself growing a step no surface ever picks, which is the drift that makes a wrong selection possible in the first place. Whether a heading should have chosen text-sm over text-sm-minus is not mechanically decidable, and this does not pretend otherwise. A step that is declared and consumed by nobody is decidable, and there is one today: --text-2xl-compact (globals.css:112) has zero consumers -- no utility use, no var() use -- while the next-rarest step, text-hero, has one real consumer. The analyzer reports every bare text-<name> it sees and does not decide which names are steps; the checker intersects that against the @theme block it parses from globals.css. So the scale is never written down twice, and a step added to globals.css is covered without touching this gate. Retiring the dead step edits @theme, so it gets its own revertible PR rather than riding along here: it is carried in UNUSED_TYPE_STEP_EXEMPTIONS and tracked as docs/outstanding-issues.md #295. The exemption cannot rot silently -- the gate also fails if an exempted step stops being declared or gains a consumer. Mutation-tested, three ways: - type steps are declared in globals.css @theme but no production surface selects them: --text-2xl-compact (text-2xl-compact). Retire the step or use it; do not leave the scale carrying a step nobody picks. - (a newly added --text-probe-step fails identically, so this catches future drift rather than only today's known case) - --text-2xl-compact is exempted as unused but production now selects text-2xl-compact -- drop the exemption Measurement note, since three different figures were in circulation for this row: the "1318 sites" is a repo-wide grep INCLUDING mockups, which the gate excludes (1360 at this HEAD). Production consumers of the nine non-standard steps total 705 -- text-2xs 421, sm-minus 160, base-minus 57, 3xs 42, 2xl-minus 9, 3xl-minus 9, lg-minus 6, hero 1, 2xl-compact 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(design-system): correct GATES.md for the two new scale gates GATES.md's own §1 is the list of what actually runs, and this series' recurring failure is that list lagging the code: four of #264's six prohibitions were already gated while it said "planned". Records the padding/radius/line-height ratchets and the type-step selection rule in the contract row, and rewrites the type-scale callout, which claimed a step-selection lint "does not exist". The decidable half now ships; the half that asks whether text-sm-minus was the right pick over text-sm still does not, and cannot. Also corrects the "1 318 call sites" figure quoted there. It was a repo-wide grep including src/app/mockups/**, which every one of these gates excludes (1 360 at 7aaf934). Production consumers total 705, and there are nine non-standard steps, not eight. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(ledger): record the #262 parts 2/3 gate work (PR #1780) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(design-system): close scale-ratchet and unused-step review gaps Cover Tailwind arbitrary-property forms and modern CSS zero units in the raw scale ratchets, and validate unused-step exemptions against the same class-or-CSS consumer predicate used for ordinary steps. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.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.
Summary
tapinCLINICAL_TWMERGE_THEME.spacing. It was held out because declaring it would hand same-property conflicts to the later class — "22 call sites, 18 of which would drop from 48px to 32/36/40/42px".#270re-measured that premise and it does not hold.tests/tailwind-merge-config.test.tswith one that asserts both halves of the real contract.src/lib/tailwind-merge.ts, which still recorded the disproved 22-site / 18-drop figure as the reason for the omission.Why the old premise fails
There are zero same-variant tap/numeric pairs in production source. The 84 survivors the original scan counted are cross-variant responsive step-downs (
min-h-tapwithsm:min-h-9,h-10.5withsm:h-tap), and tailwind-merge groups by variant, so declaringtapcannot reach them. They are live breakpoint steps, not dead classes — deleting them would raise those controls. Several named call sites were stale outright:DocumentManagerPanelandsettings-dialogcarry no tap token at all.The gap that kept
#270open, and how it was closedA per-string-literal scan cannot see a conflict composed across
cn()arguments:cn(recipe, "min-h-tap")pairs the recipe'smin-h-7with the token through a constant. Order decides — recipe-then-tap raises to 48px, tap-then-recipe drops to 28px, the forbidden direction.So the sweep resolves constant recipe identifiers at each call site before grouping by variant and property. Across 700 files and 1418
cn()call sites (1410 with resolvable arguments, 802 resolvable class constants): zero same-variant pairs, in either direction.Two defects were found in the sweep itself before that zero was trusted. Its first run reported three drops, and all three were artefacts:
size === 44 ? "h-tap w-tap" : "h-[38px] w-[38px]"looked like one element carrying both. The arms are mutually exclusive; arguments now expand to alternatives and compositions are enumerated.fieldControlin one module resolved to a same-named constant in another and invented a conflict that does not exist at the call site. Resolution is now per file, with a global fallback only where a name is unambiguous repo-wide.The zero is mutation-tested, not assumed. Synthetic probes
cn("h-tap", "h-4")andcn("min-h-tap", recipe)are both flagged as drops;cn(recipe, "min-h-tap")is correctly reported as a raise;cn("min-h-tap", "sm:min-h-9")is correctly not flagged. So the zero is a measurement rather than a pattern that never matches.What the replacement test pins
The pinning test is replaced rather than deleted. It now asserts a same-variant pair merges to the last class, and that cross-variant responsive step-downs pass through untouched. The second case is the load-bearing one: if it ever merges, a control silently loses its breakpoint step and the sweep's zero stops meaning anything.
No production tap target is lowered, and no cross-variant numeric is deleted.
Verification
tests/tailwind-merge-config.test.ts—Tests 34 passed (34), exit 0.npm run test—Test Files 2 failed | 521 passed (523),Tests 6 failed | 5590 passed | 14 skipped (5610). The two failing files aretests/mode-nav-addon-slot.dom.test.tsxandtests/pr-handoff-stop.test.ts, both pre-existing and environmental on a Windows worktree, and both failing identically on the same run before this change. Neither is touched here.npm run check:design-system-contract— exit 0,design-sync contract checked: 53 components and 7 guidelines.npm run check:icon-scale—no retired 4.5 (18px) half-step icon sizes in src.npm run check:type-scale—no arbitrary text-[<n>px|rem|em] font sizes in src.npm run format:check—All matched files use Prettier code style!#270asks for a Chromium look; this PR's Production UI jobs are that look, and it is not claimed as run on this machine.Risk and rollout
cn()runs every className in the product, so this changes which class wins wherever a same-propertyspacingpair is composed. The measured set of affected same-variant pairs is empty, and cross-variant pairs are provably out of scope because tailwind-merge groups by variant.git revert. That restores the held-out token and the previous pinning test together.Refs #270.