fix(ui): point Button at the control-* tokens it was only aliasing - #620
Merged
Conversation
Juliusolsson05
force-pushed
the
feat/ui-primitives-theme-fidelity
branch
from
July 28, 2026 14:46
f092fa1 to
cbe9c86
Compare
`Button`'s `outline` and `ghost` variants hardcoded `border-border`, `bg-transparent`, and `text-ink-dim` — the values the `control-*` family happens to alias to in styles.css, rather than the `control-*` tokens themselves. That renders identically under every built-in theme, which is why it survived. It is wrong anyway: customAppearance.ts exposes all seven `control-*` tokens as independently user-editable and documents them as button chrome, and applyCustomAppearance writes them as inline custom properties on <html>. A user who edited controlFg got every hand-rolled control button in the tree honouring it while every <Button> ignored it — making the primitive less theme-correct than the markup it exists to replace, and plausibly why adoption stalled at 17 files. NumberInput already builds its steppers on `control-*`, so a single DialogActions footer could render a `ghost` Cancel on ink-dim beside a NumberInput stepper on control-fg: two token families in one control strip. `default`, `destructive`, and `link` stay as they are — accent and danger chrome, not control chrome. `secondary` stays too: it is the filled/raised treatment and `controlActiveBg` means *selected*, a different state. Also in this change, now that the variant is a true drop-in: - DictationGuideModal becomes a DialogContent. It was the last hand-rolled app modal — own Escape handler, own Tab focus trap, own backdrop, own focus restoration, and a hand-copied interaction-ownership marker, each forbidden by name in components/ui/README.md. - SettingsSearch and the dictation key field move onto `Input`. The key field was styled with `control-*` (button chrome) rather than `input-*`. - 18 hand-rolled control-button call sites move onto `Button`. Toggles with a selected state keep their conditional classes; `Button` has no pressed variant and inventing one was out of scope. Verified: `npm run typecheck` clean, `npm run test:renderer` 258 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR #618 landed while this branch was in review and added six fresh copies of the exact hand-rolled recipe this branch exists to remove: border border-control-border bg-control-bg px-2 py-1 text-[11px] text-control-fg hover:border-control-border-hover hover:text-ink That is `Button variant="outline" size="sm"` written longhand. Folding it in here rather than leaving it for a follow-up, because shipping a change that claims to converge the button chrome while ten fresh copies sit beside it converges nothing. The two destructive-looking buttons keep `outline` with a danger override rather than `variant="destructive"`: "Reset Statistics" and "Delete" only OPEN a confirmation, and a filled destructive button overstates what the click does. The filled treatment stays reserved for the Confirm. The Cancel button's `ref={node => node?.focus()}` is preserved — Button forwards its ref to the underlying <button>, so the deliberate focus-the-safe-option behaviour #618 documented still holds. That is load-bearing: the surrounding remount destroys the previously focused node, and focus landing on Confirm instead would be a destructive keyboard trap. The StatTile containers keep their control-* classes. They are panel chrome, not buttons. Verified: `npm run typecheck` clean, `npm run test:renderer` 60 files / 262 tests passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…anger variant From the orchestrated contract review of this PR. Every item below was re-verified against the code before acting on it. 1. SettingsList's `action` arm was left hand-rolled on the grounds that its ternary needed a conditional class. Wrong: that ternary switches on TONE, not on a pressed/selected state, so both arms are ordinary variants. The carve-out only ever applied to toggles. 2. AgentCodeConventionsRow's On/Off toggle was missed entirely, and the reason is worth recording: the completeness check grepped `bg-control-bg`, and this site carries `border-control-border` + `text-control-fg` WITHOUT it. Any future sweep should grep `text-control-fg` too. 3. `secondary` now takes `control-border`. The original reasoning was half right — there genuinely is no `control-*` token for "raised resting" fill or for its higher-contrast text, so `bg-surface-hi`/`text-ink` stay. But `controlBorder` is documented as "Resting control border" and a secondary button is a control, so leaving it on the generic `border` token left the exact defect this PR exists to fix, half-fixed. SettingsPage renders an `outline` Close and a `secondary` Cancel in one view; they must not disagree. 4. New `destructive-outline` variant. Five call sites had independently written some spelling of "outline chrome, danger text", and each had to actively CANCEL `outline`'s `hover:text-ink` — that cancellation is the signal it was a variant rather than feature layout. Four move onto it. ComposerActions' Stop deliberately does NOT: `destructive-outline` colours text danger at rest, and Stop sits permanently beside Send while an agent runs, where a permanently red control reads as an error state rather than an available action. It keeps hover-only danger, with a comment saying why. 5. Corrected counts. The "17 files import Button" figure was measured on a stale branch; `main` has 20 (renderer) / 24 (all of src). That number was baked into button.tsx's comment, where a wrong figure is worse than in a doc because the comment is the durable artifact. Plan-doc audit numbers likewise corrected (223 raw buttons excluding tests, checkbox x7) and the counting method recorded so they are reproducible. The "provider renderers have zero raw buttons" line was ambiguous — the MODALS have zero; the surface as a whole has 13 in feed rows. Reworded. Separately verified while resolving, both by reading and empirically: - The dropped Cmd/Option swallow in DictationGuideModal is genuinely redundant. useKeybinds.ts:338-363 consults hasAppInteractionOwner() in CAPTURE phase ahead of every workspace shortcut and returns unconditionally, and shouldPreventOwnedApplicationShortcut covers the macOS-default preventDefault the old hand-rolled listener was doing. - `cn` is twMerge(clsx(...)), and every className override in this PR beats the variant class it conflicts with. Checked by running tailwind-merge over the actual class pairs, not by assuming argument order. Verified: `npm run typecheck` clean, `npm run test:renderer` 60 files / 262 tests passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Juliusolsson05
force-pushed
the
feat/ui-primitives-theme-fidelity
branch
from
July 28, 2026 15:00
158c6e5 to
a028dc9
Compare
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.
The bug
Button'soutlineandghostvariants hardcodedborder-border,bg-transparent, andtext-ink-dim— the values thecontrol-*family happens to alias to instyles.css, rather than thecontrol-*tokens themselves.Under every built-in theme those render identically, which is why this survived. It is wrong anyway:
customAppearance.tsexposes all sevencontrol-*tokens as independently user-editable, documented as button chrome (controlBg: "Resting button/toggle/select background",controlFg: "Resting control text/icon color").theme.ts→applyCustomAppearance()writes every one of them as an inline custom property on<html>, where they outrank the[data-mode]blocks.So a user who edited
controlFggot the 25 hand-rolled control buttons in the tree honouring it while every<Button>silently ignored it — making the primitive less theme-correct than the ad-hoc markup it exists to replace. That is also the most plausible reason adoption stalled at 17 files:Buttonwas not a drop-in for the app's most common button.NumberInputalready builds its steppers oncontrol-*, so a singleDialogActionsfooter could render aghostCancel onink-dimbeside aNumberInputstepper oncontrol-fg— two token families in one control strip.What changed
1.
Buttonretokenized.outlineandghostnow namecontrol-*.default,destructive, andlinkare untouched — accent/danger chrome, not control chrome.secondaryis untouched too: it is the filled/raised treatment, andcontrolActiveBgmeans selected, which is a different state. Mapping it there would invent a meaning the theme schema does not have.Accepted visual change:
outlinegainshover:bg-control-hover-bg, which it previously lacked. That is the hover fill the hand-rolled control buttons already painted, so the migrations below converge the two looks rather than splitting them again.2.
DictationGuideModal→DialogContent. It was the last hand-rolled app modal: its own Escape handler, its own Tab focus trap, its ownfixed inset-0backdrop, its own focus restoration, and a hand-copieddata-agent-code-interaction-owner="app".components/ui/README.mdforbids each by name, and it is not one of the sanctioned full-screen takeovers.Its Cmd/Option window-capture swallow is deliberately not carried over — that existed because a hand-rolled overlay had no way to tell the shortcut router an app surface owned the turn.
DialogContentmounts the ownership marker for exactly its own lifetime and the router already checks it synchronously, so suppression is now structural instead of a second listener racing the first.3. Text inputs onto
Input. Three recipes existed for one control.SettingsSearchcarried a hand-copy ofInput's own class list; the dictation key field was styled withcontrol-*(button chrome) instead ofinput-*(field chrome) — under a custom theme separating them, it rendered as a button.4. 18 hand-rolled control buttons onto
Button. Toggles with a selected state keep their conditional classes —Buttonhas no pressed variant and inventing one was out of scope. The 7 remainingcontrol-*sites are exactly those toggles plus one status panel, and they are now consistent with the primitive rather than diverging from it.Deliberately not in scope
The other ~150 raw
<button>s.components/ui/README.mdsays directly: "No migration of every existing button as a prerequisite." Most are tab strips and segmented controls whereButtonis not a drop-in. The full audit — including what came back clean — is in the plan doc, which is the first commit here.Verification
npm run typecheck— cleannpm run test:renderer— 59 files, 258 tests passedWorth a human look at the
outlinehover fill in a couple of themes, since that is the one intentional appearance change.🤖 Generated with Claude Code