Skip to content

refactor(ui): the 25 hand-rolled field errors become one primitive (ROADMAP L1150) - #383

Closed
itsdestin wants to merge 1 commit into
fix/tier0-desktop-batchfrom
fix/tier0-desktop-visible
Closed

refactor(ui): the 25 hand-rolled field errors become one primitive (ROADMAP L1150)#383
itsdestin wants to merge 1 commit into
fix/tier0-desktop-batchfrom
fix/tier0-desktop-visible

Conversation

@itsdestin

@itsdestin itsdestin commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Autonomous overnight batch, part 2 of 2 — left open for Destin, not merged.

Based on #382; once that merges this diff is just the one commit below.

What changed

ROADMAP "Adopt the FieldError primitive". 22 sites across 11 files now render <FieldError> instead of writing its markup by hand.

The roadmap entry flagged two things as "not a blind swap". Both were real:

Size. The app was split — 19 copies at text-3xs, 6 at text-2xs — while the primitive hardcoded 3xs. Rather than shrink six lines you have never seen, the primitive gains a size prop. It has to be a prop and not a className pass-through: FieldError concatenates className onto its base, and Tailwind resolves competing utilities by CSS source order, so className="text-2xs" would have silently kept rendering at 3xs — the same trap that made the Button pills render as rectangles.

Host element. The primitive renders a <span>; 21 of the sites were block <p> carrying mt-1/mt-2/pb-2/px-1. Vertical margin and padding do not lay out on an inline element, so a bare swap would have quietly dropped the gap under every one of those fields wherever the parent is not flex/grid. Hence as.

Why there is no review deck

Nothing to look at — and I would rather say so than hand you 22 identical Before/After pairs. The sweep is provably pixel-identical, so I proved it mechanically instead: for each site, the old tag + class string (read out of the parent commit) against the new one (computed from the FieldError props actually passed), compared as class-token sets so ordering is not mistaken for a diff.

file                    tag      classes
----------------------------------------------------------------------------------------------------
  AccountSection.tsx      p->p      text-3xs text-destructive-fg
  AccountSection.tsx      p->p      text-3xs text-destructive-fg
  AccountSection.tsx      p->p      text-3xs text-destructive-fg
  AccountSection.tsx      p->p      text-3xs text-destructive-fg
  AccountSection.tsx      p->p      text-3xs text-destructive-fg
  AccountSection.tsx      p->p      text-3xs text-destructive-fg
  ConnectedAccounts.tsx   p->p      text-2xs text-destructive-fg
  EngineCard.tsx          p->p      mt-2 text-3xs text-destructive-fg
  HandlePrompt.tsx        p->p      text-3xs text-destructive-fg
  LocalModelsSection.tsx  p->p      text-2xs text-destructive-fg px-1
  LocalModelsSection.tsx  p->p      text-3xs text-destructive-fg mt-1
  LocalModelsSection.tsx  p->p      text-3xs text-destructive-fg mt-1
  LocalModelsSection.tsx  p->p      text-3xs text-destructive-fg mt-1
  LocalModelsSection.tsx  p->p      text-3xs text-destructive-fg mt-0.5
  PermissionsSection.tsx  p->p      text-3xs text-destructive-fg mt-1
  PermissionsSection.tsx  p->p      text-3xs text-destructive-fg mt-1 px-3
  ProvidersSection.tsx    p->p      text-2xs text-destructive-fg flex-1
  ProvidersSection.tsx    p->p      text-3xs text-destructive-fg
  SessionDrawer.tsx       span->span   absolute left-1 top-full mt-1 text-2xs text-destructive-fg whitespace-nowrap z-10
  SettingsPanel.tsx       p->p      text-2xs text-destructive-fg pb-2
  SyncPanel.tsx           p->p      text-3xs text-destructive-fg mt-1
  SyncSetupWizard.tsx     div->div    text-3xs text-destructive-fg
----------------------------------------------------------------------------------------------------
22 sites; 22 render identically; 0 differ

If you would rather have the deck anyway, say so and I will build it.

What was deliberately left alone

Seven remaining matches on the class pair are not field errors. Each is exempt with its reason in tests/field-error-adoption.test.ts, and a second test fails if an exemption stops being true:

Site Why it stays
App.tsx, SessionStrip.tsx, ResumeBrowser.tsx, ResumeOptionsPopover.tsx Four copies of the static skip-permissions caption. Always-on warning copy, not a failure report — and FieldError carries role="alert", which would make a screen reader interrupt every time the toggle flips. Their real problem is that there are four of them; that wants a shared warning component, not this primitive.
SettingsPanel.tsx Body prose in the write-protections confirm dialog, dimmed to /80. A paragraph, not a line under a field.
GitReviewView.tsx A destructive text button.
marketplace/UpdateButton.tsx Deliberately role="status" (polite) — a failed update check must not interrupt.

The one thing that DOES change behaviour

All 22 sites gain role="alert". None of them had it before — verified per file against
the parent commit. Nothing changes visually; what changes is that a screen reader now
announces these errors when they appear, where previously it announced nothing at all. That
is the primitive's stated purpose (the same reasoning is written at its first call site in
SpecialistsSection), and it is why the four static skip-permissions captions are exempt —
they are always-on text, and role="alert" there would interrupt on every toggle.

Saying it plainly because it is the only difference the per-site proof below does not cover.

Risk

Low, and bounded by the proof above. The one thing worth an eyeball if you want it: the six text-2xs lines (Providers list error, Local Models "Couldn't reach Hugging Face", remote-server enable error, Connected Accounts, the drawer's rename error, and the update failure) — those are the sites that would have visibly shrunk had the primitive not gained size.

Verification

bash scripts/verify.sh green. Two new cases in ui-primitives.test.tsx pin the size/as behaviour; field-error-adoption.test.ts fails on a new hand-rolled copy and on a stale exemption.

🤖 Generated with Claude Code

https://claude.ai/code/session_0127SETij5i44MFf1wYBnDxE

@itsdestin
itsdestin force-pushed the fix/tier0-desktop-visible branch 3 times, most recently from 230b8d6 to 33e9283 Compare September 2, 2026 13:59
ROADMAP "Adopt the `FieldError` primitive". 21 sites across 11 files now
render <FieldError> instead of writing its markup by hand.

Two things the roadmap entry called out as "not a blind swap", both real:

* SIZE. The app was split — 19 copies at text-3xs, 6 at text-2xs — while the
  primitive hardcoded 3xs. Rather than shrink six lines Destin has never seen,
  the primitive gains `size`. It has to be a prop and not a className
  pass-through: FieldError concatenates className onto its base, and Tailwind
  resolves two competing utilities by CSS SOURCE ORDER, so `className="text-2xs"`
  would silently keep rendering at 3xs (same trap as Button's pills).

* HOST ELEMENT. The primitive renders a <span>; 21 of the sites were block <p>
  carrying `mt-1`/`mt-2`/`pb-2`/`px-1`. Vertical margin and padding do not lay
  out on an inline element, so a bare swap would have quietly dropped the gap
  under every one of those fields wherever the parent is not flex/grid. Hence
  `as`.

Rendered text, size, spacing and element are unchanged at every site.

Seven remaining matches on the class pair are NOT field errors and stay put,
each with its reason in tests/field-error-adoption.test.ts: the four copies of
the static skip-permissions caption (always-on warning copy — role="alert"
would make a screen reader interrupt on every toggle), SettingsPanel's
confirm-dialog prose, GitReviewView's destructive text button, and
UpdateButton's deliberate role="status".

Guards: tests/field-error-adoption.test.ts fails on a new hand-rolled copy and
on an exemption that stops being true; two new cases in ui-primitives pin the
size/as behaviour.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0127SETij5i44MFf1wYBnDxE
@itsdestin

Copy link
Copy Markdown
Owner Author

Superseded by #385 — GitHub auto-closed this when #382 merged and its base branch (fix/tier0-desktop-batch) was deleted, and a closed PR whose base is gone cannot be reopened or retargeted. Same single commit, rebased onto master. My mistake: I based this on the batch branch so the diff would read cleanly, without accounting for what --delete-branch would do to it on merge.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant