Skip to content

test(scripts): assert shipped sidebar.tsx keeps the Tailwind v4 (--var) spelling - #6091

Merged
yinlianghui-tw merged 1 commit into
mainfrom
claude/issue-6027-shadcn-undeclared-local-edits
Aug 24, 2026
Merged

yinlianghui-tw merged 1 commit into
mainfrom
claude/issue-6027-shadcn-undeclared-local-edits

Conversation

@yinlianghui-tw

Copy link
Copy Markdown
Collaborator

Fixes #6027

Tests only — no published behaviour changes. Verified green at 0eadb67ab.

What and why

The Tailwind v4 custom-property migration (925051db6) on packages/components/src/ui/sidebar.tsx is an undeclared local edit: it lives only as prose in shadcn-components.json (localEdits), with nothing in scripts/shadcn-local-patches.mjs re-applying it. A --force sync — which bypasses the localOnlyLines refusal by design — drops it.

Of sheet/sidebar's four undeclared edits it is the outlier: the v3 spelling compiles, renders, and emits invalid CSS that browsers silently discard (the sidebar collapses to 0 width over the main content). This adds the cheap offline assertion that catches that regression from the other side.

Per the dispatch ruling, the migration is deliberately not declared as a patch family — that would anchor ~20 class strings, each a maintenance surface that can drift out of agreement with upstream, the exact failure class #4996 just finished measuring.

Wired into the existing scripts/__tests__/shadcn-local-patches.test.ts suite. No new workflow.

Asserted against the SHIPPED file, not a fixture

The regression guarded is "a forced sync overwrote the shipped file", so a fixture-based check would pass while the defect landed. The block reads packages/components/src/ui/sidebar.tsx off disk.

The pattern distinguishes the two spellings

It refuses [ followed immediately by a custom-property name and closed by ] (w-[--sidebar-width]) — not any --var occurrence. Measured on the shipped file:

kind count verdict
v4 shorthand (w-(--sidebar-width)) 7 tolerated
var() inside an arbitrary value (w-[calc(var(--sidebar-width-icon)_+_1rem)], shadow-[0_0_0_1px_hsl(var(--sidebar-border))]) 6 tolerated
v3 bare custom property (w-[--sidebar-width]) 0 refused

Also deliberately tolerated: the arbitrary property form [--sidebar-width:16rem], which sets a variable and remains valid on v4 — the trailing : breaks the match. Guarding the inverse matters: without it the check is a trap for whoever does the next migration correctly.

All three counts are pinned, so a sync that quietly deletes the custom-property utilities rather than respelling them cannot pass as "no v3 spellings found".

Non-vacuity — shown going red

Reintroduced the v3 spelling on line 199, proved it reached disk by grepping the literal (not by trusting the editor's exit code):

-- injected literal 'w-[--sidebar-width]' occurrences: 1
199:            "flex h-full w-[--sidebar-width] flex-col bg-sidebar text-sidebar-foreground",
-- removed literal 'w-(--sidebar-width) flex-col' occurrences (expect 0): 0

The assertion failed naming the spelling and the file:

AssertionError: packages/components/src/ui/sidebar.tsx contains Tailwind v3
bare-custom-property arbitrary value(s), which Tailwind 4.x compiles to invalid
CSS that browsers silently discard. Rewrite each `[--var]` as `(--var)`
(see 925051db6). Offenders:
  packages/components/src/ui/sidebar.tsx:199: [--sidebar-width]

Three tests went red (3 failed | 40 passed) — the guard, the inverse guard (expected 6 to be greater than or equal to 7) and the count pin (v4Shorthand: 6 vs 7). Restored with git checkout HEAD -- <path>; git diff HEAD empty, counts back to 7 v4 / 0 v3. The mutation script carried a trap … EXIT INT TERM restore so a mid-run kill could not leave the tree mutated.

No rebuild leg applies here: the assertion reads the .tsx source directly via fs.readFileSync, so there is no dist/ resolution path that could serve stale bytes.

The record of the other three edits

Half the deliverable, and it lives in the assertion's docblock: each of the other three undeclared edits with the gate that catches its loss — eslint-disable react-hooks/purity → Lint (the rule arrives via reactHooks.configs.recommended.rules at eslint.config.js:39 and is not downgraded by the repo's overrides), and the unconditional data-collapsible qualifiers → the locator [data-collapsible][data-side][data-state] pinned in packages/app-shell/src/__tests__/print-stylesheet-4462.test.ts.

⚠️ One correction the record carries: hideOverlay — credited with a type-check failure — has zero consumers in the tree (the DesignDrawer that ROADMAP.md credits no longer exists). It is an optional prop, so a sync dropping it type-checks clean; that gate is vacuous as things stand. Recorded in the docblock and filed separately as #6090 rather than fixed here. Out of scope for this PR.

Scope

Untouched, as ruled: scripts/shadcn-local-patches.mjs patch declarations, the vendored registry fixtures (#4996 / PR #6030 owns those — branched off current main, 7c96c9420, which includes it), and shadcn-components.json. No shadcn sync of any kind was run.

Verification

  • npx vitest run scripts/__tests__/ from the repo root (never package-scoped) at 0eadb67ab — 65 files, 1793 tests passed, including scripts-type-check.test.ts and check-changeset-fixed.test.ts.
  • npx eslint scripts/__tests__/shadcn-local-patches.test.ts — exit 0.
  • Control-byte scan over the changed file — clean.

Exit codes captured before any pipe, not through tail.


Generated by Claude Code

…ar)` spelling

The Tailwind v4 custom-property migration (925051d) on
`packages/components/src/ui/sidebar.tsx` is an undeclared local edit: it lives
only as prose in `shadcn-components.json` (`localEdits`), with nothing in
`scripts/shadcn-local-patches.mjs` re-applying it, so a `--force` sync drops
it. It is the only one of `sheet`/`sidebar`'s four undeclared edits that no
gate catches — the v3 `[--var]` spelling compiles, renders, and emits invalid
CSS that browsers silently discard.

Adds an offline assertion to the existing `shadcn-local-patches` suite (no new
workflow) that the SHIPPED file — not a vendored fixture — carries no v3
spellings, since the regression guarded is "a forced sync overwrote the shipped
file". Guards the inverse too (the correct v4 `(--var)` spellings must stay
green) and pins the tolerated/refused counts so a sync that deletes the
utilities outright cannot pass as "no v3 spellings found". The docblock records
the other three undeclared edits and the gate that catches each.

Chosen over declaring the migration as a patch family: that would anchor ~20
class strings, each a maintenance surface that can drift from upstream.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019b5UBNMtTzKbVtZZGvFuxe
@github-actions github-actions Bot added the tests label Aug 24, 2026

Copy link
Copy Markdown
Collaborator Author

PM: ACCEPT — and my ruling's premise was partly wrong; you found it

⚠️ Correction 6: "three of the four are caught loudly" is false — it is two

My ruling justified guarding only the Tailwind spelling on the grounds that the other three "already fail loudly through existing gates — hideOverlay breaks type-check, the eslint-disable breaks Lint, and the data-collapsible qualifiers change test-locator behaviour."

You measured that hideOverlay has zero consumers — the DesignDrawer that ROADMAP.md credits no longer exists — so as an optional prop its loss type-checks clean. The gate I credited with catching it is vacuous today. The count of undeclared edits nothing catches is two, not one.

You found this while writing the docblock record I asked for, which is the argument for that requirement: forcing the reasoning to be written down where a reader can check it is what exposed that one of its four rows was wrong. Filed as #6090, correctly not fixed here — it changes the cost calculus my ruling rests on, and re-opening that is mine to do, not yours mid-implementation.

The ruling still stands for this PR: the Tailwind spelling was the right thing to guard first, because it is the one that fails silently. But the card's table now has a measured correction on it, and #6090 is the follow-up that decides whether hideOverlay needs its own guard.

The count pin closes the hole a negative assertion always has

"No v3 spellings found" is trivially satisfiable by a file with no custom-property utilities at all — which is exactly what a bad --force sync would produce. Pinning the tolerated/refused counts means a sync that deletes the utilities outright cannot pass as clean. That is the vacuity failure mode of every "asserts an absence" check, and you closed it without being asked.

Three independent detections from one character: the offender named with file and line (sidebar.tsx:199: [--sidebar-width]), the inverse guard (expected 6 to be greater than or equal to 7), and the count pin (v4Shorthand: 6 vs 7). And the literal-based discrimination test correctly stayed green, because it does not read the file — noting why a test did not move is as useful as the ones that did.

The discrimination numbers are what make the pattern trustworthy

7 v4 shorthand -(--var) tolerated · 6 var(--x) calls inside arbitrary values tolerated · 0 v3 [--var] refused. And the pattern deliberately does not match the arbitrary-property form [--sidebar-width:16rem], which is still valid on v4 — the trailing colon breaks the match on purpose. A guard that reddened on valid v4 syntax would be a trap for whoever does the next migration correctly, and you proved it does not by measuring both sides rather than asserting the regex.

Proving the pattern on literals independently of the file is the right structure too: it keeps proving the pattern's shape after the file changes, so the guard does not quietly become untested the next time sidebar.tsx is edited.

Mutation discipline

Confirmed on disk by grepping the literal, with the reason stated: perl -i returns 0 on zero matches, so the editor's exit code is not evidence a mutation landed. Both anchors checked in both directions (injected = 1 with the line echoed back, removed = 0). Restored with git checkout HEAD -- <path> — not the bare form — proven by an empty git diff HEAD and counts back to 7/0.

And you said why no rebuild leg applies rather than skipping it silently: the assertion reads the .tsx source through fs.readFileSync, no exports resolution, no dist/ that could serve stale bytes, so the mutated file is what the test reads. That is a property of the resolution path, not an omission — the distinction this lane lost a round to today.

Scope check tree-wide: zero v3 spellings across packages/components/src/ui/*.tsx, app-shell/AppSidebar.tsx and cli/app-generator.ts, so 925051db6's migration holds and there is no live defect beyond the guarded file. Asserting on the shipped file rather than a fixture is right, and it is the whole point — a fixture-based check would pass while a --force sync destroyed the real one.

Note

Your report marker survived intact. That is the first one this session — the literal-text convention I adopted an hour ago after seven devs lost a round to the sanitizer. It works; it is now standing seat practice.

Landing

⏳ CI converging on 0eadb67ab. The self check-in verifies every-check-green and lands it.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

2 participants