fix(core): correct astw: prefix order in shipped components, and the prefix advice in docs/ - #515
Merged
Merged
Conversation
Contributor
Author
|
/review |
Contributor
|
✅ Code Review completed successfully! Code review complete for PR #515: no High or Medium issues found in changed package code or exports, so no inline review comments were needed.
|
Contributor
Code Metrics Report
Details | | main (c60dbf7) | #515 (52445f9) | +/- |
|---------------------|----------------|----------------|------|
| Coverage | 87.4% | 87.4% | 0.0% |
| Files | 204 | 204 | 0 |
| Lines | 6014 | 6014 | 0 |
| Covered | 5257 | 5257 | 0 |
- | Test Execution Time | 1m40s | 2m14s | +34s |Code coverage of files in pull request scope (20.0% → 20.0%)
Reported by octocov |
interacsean
added a commit
that referenced
this pull request
Sep 9, 2026
A hostile review of #515 found one real regression and several wrong or self-contradictory claims. Each fix below is measured, not reasoned. Regression (introduced by the docs commit): - form-demo.tsx used `astw:items-stretch` on Field.Root, which sets `astw:items-start`. As an `astw:` class it de-duped and worked; stripped to plain it does not, so both survived and align-items stayed `flex-start`. Measured in the browser at all three sites. Now `items-stretch!` -> `stretch`. This is an override, and the rule this PR documents was applied wrongly to it. The documented rule was incomplete: - A binary add/override test misses values set under a state variant or on a descendant. `.astw\:hover\:text-accent-foreground:hover` is (0,2,0) against a plain `.text-destructive` at (0,1,0), so a ghost Button's destructive text vanishes on hover — the library works around this itself in ActionPanel. Added rule 4, and `text-destructive!` in sheet.md. The cascade explanation was inverted: - It claimed AppShell's sheet is "imported before yours" yet "appears later in the cascade", which cannot both hold. It is imported *after* the app's Tailwind output in the documented setup — that is *why* it wins. Also states the precondition the rule depends on, since flipping the import order flips which value wins. - "Roughly half" of `astw:` classes are unshipped -> the measured 17 of 55. A second shipped JSDoc still advertised the prefix: - ActionPanel.tsx told consumers to "Use `astw:w-full`" and shipped in dist/app-shell.d.ts. Doubly wrong: ActionPanel already applies it. The .d.ts is now free of `astw:`. Guard test had exploitable false negatives (each probed, then fixed): - doubled `astw:hover:astw:x` passed `startsWith` — now checks that `astw:` occurs exactly once, at position 0 - `.css` was unscanned while globals.css drives five `@apply astw:*` rules — glob widened - the loose `> 100` file assertion tolerated losing all of components/ - decisions/date-picker.md carried a real `hover:astw:text-foreground` that no glob could see; fixed, and the test now names its boundaries Wrong or stale prose: - description-card.md advertised `p-6` as an addition; the card already insets content 24px, so it double-padded - purchase-order-demo.tsx kept a rationale this PR falsified - design-system.md mandated `!` then called it a smell, flagging the catalogue's own required table-in-card pattern - sidebar-item.md still handed consumers an internal class to paste - layout.md swapped `space-y-4` for `gap-6!`, changing the example's meaning; `space-y-4` is a legitimate addition Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
interacsean
changed the title
interacsean
force-pushed
the
fix/1651-1652-astw-prefix-order-and-docs
branch
from
September 16, 2026 01:47
12342d1 to
e85ced0
Compare
interacsean
changed the title
interacsean
marked this pull request as ready for review
September 16, 2026 02:30
…#1652) Tailwind v4 only recognises a configured prefix as the first segment of a class name, so `hover:astw:opacity-100` and friends emitted no CSS at all — no error, no warning. 14 such classes across Alert, MetricCard, AppearanceSwitcher and CsvImporter were dead in the shipped stylesheet. Verified against the built CSS: all 14 selectors are present by exact-string match where none were before, and confirmed in the browser — Alert's per- variant muted description text and its dismiss focus ring (previously no focus indicator at all), MetricCard's dark-mode deltas, the appearance switcher's radio indicator, and CsvImporter's last-row border. Adds src/astw-prefix-order.test.ts as a guard — snapshots record class names rather than resolved CSS, so they stay green on this defect. It checks that `astw:` occurs exactly once and at position 0, scans .css too (globals.css drives five `@apply astw:*` rules), and names the boundaries it cannot cover. Each evasion was probed before being closed. Also fixes the one misordered class outside packages/core, in decisions/date-picker.md, and records the ordering rule in the internal component-authoring reference. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`astw:` is the library's internal Tailwind prefix. A consumer's Tailwind build has no `astw` prefix configured, so it never generates `astw:*` classes — an `astw:` class in app code only resolves if app-shell happens to already ship that exact utility. 17 of 55 classes sampled from these pages are absent from the shipped stylesheet, and Tailwind emits nothing for an unknown utility: no error, no warning. Adopts the boundary #536 sets for the bundled skill, so the two agree once it lands: ordinary utilities on your own markup and on documented layout hooks; props, variants, or composition for a component's own appearance. Each site moved to the supported form — `Layout gap={6}`, `Button variant="destructive"`, and so on — rather than to a specificity hack. styling-theming.md keeps the measurement as explanation rather than advice: it says why a plain utility cannot override an AppShell default, which is what sends you to a prop, and points at #536's no-astw-prefix lint rule. Deliberately leaves catalogue/ and Layout's `gap` JSDoc to #536 so the two branches do not overlap. Table-in-card guidance follows #514: the cells carry their own first:pl-6 / last:pr-6, so the container adds no horizontal padding. Measured in examples/vite-app — with `containerClassName="px-6"` the first column sits at 49px against a card title at 25px; without it, both at 25px. Also converts the Vite showcase's ai-chat, layouts and primitives pages (layouts also carried two misordered `dark:astw:opacity-20`), and ActionPanel's JSDoc, which ships in the published .d.ts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#536 landed reconciled against #514, and its merged form differs from the branch this docs pass was written against: it keeps `Card.Content className="px-0!"` for the one table-in-card case rather than restructuring around it. Docs that never mention `!` would contradict the shipped skill. Props, variants and composition stay the primary guidance, matching §5. `!` is now documented as the last resort it is in the catalogue, using the catalogue's own example, and with the note that the table container still adds no horizontal padding — the cells carry first:pl-6 / last:pr-6, so `containerClassName="px-6"` there would push the first column 24px past the card title. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
interacsean
force-pushed
the
fix/1651-1652-astw-prefix-order-and-docs
branch
from
September 16, 2026 07:29
e85ced0 to
284ecee
Compare
IzumiSy
approved these changes
Sep 17, 2026
Review feedback: this belongs in linting, not in the test suite. Removed rather than left in place, so the gap is momentary instead of becoming tech debt. Backlogged as an oxlint rule. It is not a one-line config change: #536's no-astw-prefix is the consumer-facing rule and would flag every legitimate use inside packages/core, that plugin is not wired into this repo's own lint, and oxlint 1.64 ships no generic restricted-syntax rule that reaches inside class strings. The board item also notes #1700 may remove the prefix entirely and make the rule moot. The 14 fixes in this PR stand on their own; nothing else in the suite covers the defect, since snapshots record class names rather than resolved CSS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 17, 2026
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
2 participants
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.
Two related
astw:defects:astw:prefix advice from the public docs (docs only, no runtime effect)The code fix (#1652)
Tailwind v4 only recognises a configured prefix as the first segment of a class name, so
hover:astw:opacity-100emits no CSS at all — no error, no warning. 14 such classes across four components were dead in the shipped stylesheet, and still are onmain.Verified against the built CSS by exact-string match — all 14 selectors present where none were before — then in the browser:
Alertdescription, ×5 variantsAlertdismiss focus ringbox-shadow: none--ring— the a11y fixMetricCarddark-mode deltasAppearanceSwitcherindicatordisplay: noneCsvImporterlast mapping row1pxborder0px(first row still1px)Regression guard:
packages/core/src/astw-prefix-order.test.ts. Snapshots record class names, not resolved CSS, so they stay green on this defect. Each evasion was probed before being closed: a doubledastw:hover:astw:prefix,.cssfiles (globals.cssdrives five@apply astw:*rules), and a file-count assertion loose enough to tolerate losing all ofcomponents/. It also names the boundaries it cannot cover.The docs (#1651)
Seventeen of the 55 classes these pages recommended are absent from the shipped stylesheet.
docs/now states the same boundary #536 sets for the skill, so the two agree once it lands: ordinary utilities on your own markup and on documented layout hooks; props, variants, or composition for a component's own appearance. Each site moved to the supported form (Layout gap={6},Button variant="destructive", …) rather than to a specificity hack.styling-theming.mdkeeps the measurement as explanation of why a plain utility cannot override an AppShell default — which is what sends you to a prop — and points at #536'sno-astw-prefixrule.Also converts
docs/components/ai-chat.md(landed onmainwith the same advice), the Vite showcase'sai-chat/layouts/primitivespages, andActionPanel's JSDoc, which ships in the published.d.ts.One thing #536 will need to fix on rebase
#514 has since landed on
mainand corrects the table-in-card rule; #536 predates it and would regress it. Measured inexamples/vite-app, insets from the card's left edge:containerClassName="px-6"main)Table.HeadandTable.Cellalready carryfirst:pl-6/last:pr-6(table.tsx:122,138), so container padding stacks. Affected lines onfeat/oxlint-plugin-app-shell, all incatalogue/src/fundamental/components.md: 388, 464, 470, 479, and 488 (the DON'T example is inverted — it calls the absence ofpx-6the bug). This PR's docs follow #514.Known gap
Menuexposes no destructive variant, and itsdata-highlighted:text-accent-foregroundoutranks an unprefixed utility on specificity. The one destructiveMenu.Itemin the showcase uses an inline style; verified it holds the destructive colour while highlighted, where a utility class flips to accent-foreground. #536's props-or-composition line has no answer here — worth a follow-up.Verification
pnpm fmt:check;turbo run lint test type-check check-dts build— 21/21 tasksastw:left inexamples/, and none indocs/outside the deliberate "never write this" explanationmain🤖 Generated with Claude Code