From c7f6ffed6df77e58f41b0dd15ce65199f55ed7c0 Mon Sep 17 00:00:00 2001 From: Destin Date: Wed, 2 Sep 2026 06:07:35 -0700 Subject: [PATCH] refactor(ui): the 25 hand-rolled field errors become one primitive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ROADMAP "Adopt the `FieldError` primitive". 21 sites across 11 files now render 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 ; 21 of the sites were block

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) Claude-Session: https://claude.ai/code/session_0127SETij5i44MFf1wYBnDxE --- .../renderer/components/AccountSection.tsx | 14 +-- .../renderer/components/ConnectedAccounts.tsx | 4 +- .../src/renderer/components/EngineCard.tsx | 4 +- .../src/renderer/components/HandlePrompt.tsx | 4 +- .../components/LocalModelsSection.tsx | 12 +-- .../components/PermissionsSection.tsx | 5 +- .../renderer/components/ProvidersSection.tsx | 8 +- .../src/renderer/components/SessionDrawer.tsx | 6 +- .../src/renderer/components/SettingsPanel.tsx | 4 +- desktop/src/renderer/components/SyncPanel.tsx | 4 +- .../renderer/components/SyncSetupWizard.tsx | 6 +- desktop/src/renderer/components/ui/states.tsx | 23 ++++- desktop/tests/field-error-adoption.test.ts | 98 +++++++++++++++++++ desktop/tests/ui-primitives.test.tsx | 23 +++++ 14 files changed, 177 insertions(+), 38 deletions(-) create mode 100644 desktop/tests/field-error-adoption.test.ts diff --git a/desktop/src/renderer/components/AccountSection.tsx b/desktop/src/renderer/components/AccountSection.tsx index 56de761ae..42dea6df2 100644 --- a/desktop/src/renderer/components/AccountSection.tsx +++ b/desktop/src/renderer/components/AccountSection.tsx @@ -4,7 +4,7 @@ import { useEscClose } from '../hooks/use-esc-close'; import { useAccount } from '../state/account-context'; import type { MarketplaceUser } from '../../main/marketplace-auth-store'; import type { BlockRow } from '../state/marketplace-api-client'; -import { Button, Dialog, InputGroup, SettingRow, Callout } from './ui'; +import { Button, Dialog, FieldError, InputGroup, SettingRow, Callout } from './ui'; import { ConnectedAccountsBody } from './ConnectedAccounts'; // Settings → Account section. One self-contained row-button + popup, mounted in @@ -272,7 +272,7 @@ function SignedOutBody({

Uses your GitHub profile to sign in — GitHub only shares your public info.

- {signInError &&

{signInError}

} + {signInError && {signInError}} ); } @@ -435,7 +435,7 @@ function SignedInBody({ {unblockingId === b.id ? 'Unblocking…' : 'Unblock'} - {unblockErrors[b.id] &&

{unblockErrors[b.id]}

} + {unblockErrors[b.id] && {unblockErrors[b.id]}} ))} @@ -484,7 +484,7 @@ function SignedInBody({ Downloads a file containing everything YouCoded's server stores about your account.

{exportSavedPath &&

Saved to {exportSavedPath}

} - {exportError &&

{exportError}

} + {exportError && {exportError}} ) : ( @@ -634,7 +634,7 @@ function EditAccountBody({ {nameSaving ? 'Saving…' : 'Save'} - {nameError &&

{nameError}

} + {nameError && {nameError}} {nameSaved && !nameError &&

Saved

} @@ -708,7 +708,7 @@ function EditAccountBody({ )} {/* Plain words for status, never glyphs. */} - {handleError &&

{handleError}

} + {handleError && {handleError}} {handleSaved && !handleError &&

Saved

} @@ -782,7 +782,7 @@ function EditAccountBody({ {deleting ? 'Deleting…' : 'Delete my account'} - {deleteError &&

{deleteError}

} + {deleteError && {deleteError}} )} diff --git a/desktop/src/renderer/components/ConnectedAccounts.tsx b/desktop/src/renderer/components/ConnectedAccounts.tsx index ca1a7c04e..d01903a83 100644 --- a/desktop/src/renderer/components/ConnectedAccounts.tsx +++ b/desktop/src/renderer/components/ConnectedAccounts.tsx @@ -1,5 +1,5 @@ import { useCallback, useState } from 'react'; -import { Button } from './ui'; +import { Button, FieldError } from './ui'; import ConnectGithubModal from './ConnectGithubModal'; // Connected-accounts sub-page INSIDE the Account popup (Destin feedback, @@ -128,7 +128,7 @@ export function ConnectedAccountsBody({ status, refresh }: { )} - {error &&

{error}

} + {error && {error}} {showConnect && ( diff --git a/desktop/src/renderer/components/EngineCard.tsx b/desktop/src/renderer/components/EngineCard.tsx index 926846628..1df94fc47 100644 --- a/desktop/src/renderer/components/EngineCard.tsx +++ b/desktop/src/renderer/components/EngineCard.tsx @@ -5,7 +5,7 @@ // so the card reads as part of the section. As of change 25 the surface IS that // row surface (bg-inset/50, borderless), not a lookalike. import { useEffect, useState } from 'react'; -import { Button, TextInput } from './ui'; +import { Button, FieldError, TextInput } from './ui'; interface EngineStatusView { installed: boolean; @@ -149,7 +149,7 @@ export default function EngineCard({ showDetails = false }: { showDetails?: bool {busy && (progress?.kind === 'verify' || progress?.kind === 'unpack') && (

{progress.kind === 'verify' ? 'Verifying download…' : 'Unpacking…'}

)} - {error &&

{error}

} + {error && {error}} {/* Say WHY the button is there. "A newer engine is available" alone tells a non-developer nothing about whether they need it. */} {updateAvailable && !busy && ( diff --git a/desktop/src/renderer/components/HandlePrompt.tsx b/desktop/src/renderer/components/HandlePrompt.tsx index be11e6ea0..b829dae6e 100644 --- a/desktop/src/renderer/components/HandlePrompt.tsx +++ b/desktop/src/renderer/components/HandlePrompt.tsx @@ -6,7 +6,7 @@ import React, { useEffect, useRef, useState } from 'react'; import { createPortal } from 'react-dom'; import { useEscClose } from '../hooks/use-esc-close'; import { useAccount } from '../state/account-context'; -import { Button, Dialog, InputGroup } from './ui'; +import { Button, Dialog, FieldError, InputGroup } from './ui'; // Persisted "don't nag me again" flag. Set on skip (and on ESC, which is the // same as skip), never set when the user actually claims a handle. @@ -128,7 +128,7 @@ function HandlePromptPopup({ {/* Plain words for status, never glyphs. */} - {error &&

{error}

} + {error && {error}} @@ -478,7 +478,7 @@ function AddProviderForm({ onDone, onCancel }: { onDone: () => Promise; on /> - {error &&

{error}

} + {error && {error}}
) : ( diff --git a/desktop/src/renderer/components/SettingsPanel.tsx b/desktop/src/renderer/components/SettingsPanel.tsx index 35ca437ec..2309fb3bf 100644 --- a/desktop/src/renderer/components/SettingsPanel.tsx +++ b/desktop/src/renderer/components/SettingsPanel.tsx @@ -31,7 +31,7 @@ import { DonateConfirm } from './DonateConfirm'; import { formatVersionLine } from '../../shared/version-line'; // UiToggle is aliased because this file still exports its own `Toggle` (the // compat wrapper below) that AboutPopup imports by that name. -import { Button, CloseButton, Toggle as UiToggle, TextInput, InputGroup, LoadingState, RadioGroup, SegmentedTabs, Dialog, SettingRow, Callout, StatusStrip, ErrorState } from './ui'; +import { Button, CloseButton, Toggle as UiToggle, TextInput, InputGroup, LoadingState, RadioGroup, SegmentedTabs, Dialog, SettingRow, Callout, StatusStrip, ErrorState, FieldError } from './ui'; // Both are Vite `define` substitutions, so they're constants at module scope. // The typeof guard covers paths where the define isn't applied (unit tests). @@ -1216,7 +1216,7 @@ function RemoteButton({ (port already bound, permission denied). Show the real reason here — the toggle has already snapped back off. */} {enableError && ( -

{enableError}

+ {enableError} )}
diff --git a/desktop/src/renderer/components/SyncPanel.tsx b/desktop/src/renderer/components/SyncPanel.tsx index c15b250b6..d526dec24 100644 --- a/desktop/src/renderer/components/SyncPanel.tsx +++ b/desktop/src/renderer/components/SyncPanel.tsx @@ -10,7 +10,7 @@ */ import React, { useState, useEffect, useCallback, useRef } from 'react'; -import { Button, Dialog, TextInput, Toggle, LoadingState, SettingRow } from './ui'; +import { Button, Dialog, FieldError, TextInput, Toggle, LoadingState, SettingRow } from './ui'; import type { SyncWarning } from '../../main/sync-state'; import { deriveSettingsRowState, type SyncDisplayState } from '../state/sync-display-state'; import { createPortal } from 'react-dom'; @@ -1818,7 +1818,7 @@ function DevicesTab({ devices, onRename, onRemove, syncInProgress, lastSyncByDev {/* Why the remove didn't take. Never invents a cause: the handler's own reason when it gave one, otherwise non-committal. */} {removeNote?.id === d.id && ( -

{removeNote.text}

+ {removeNote.text} )} ); diff --git a/desktop/src/renderer/components/SyncSetupWizard.tsx b/desktop/src/renderer/components/SyncSetupWizard.tsx index f06666c54..bb7e80a59 100644 --- a/desktop/src/renderer/components/SyncSetupWizard.tsx +++ b/desktop/src/renderer/components/SyncSetupWizard.tsx @@ -10,7 +10,7 @@ */ import { useState, useEffect, useCallback } from 'react'; -import { Button, CloseButton, TextInput, Toggle, Radio, RadioGroup, Callout } from './ui'; +import { Button, CloseButton, FieldError, TextInput, Toggle, Radio, RadioGroup, Callout } from './ui'; import { isAndroid as checkIsAndroid } from '../platform'; import { useEscClose } from '../hooks/use-esc-close'; import { useScrollFade } from '../hooks/useScrollFade'; @@ -846,9 +846,9 @@ function GhInstallHelp({ onRecheck }: { onRecheck: () => void }) { )} {installError && ( -
+ Couldn't install it automatically: {installError} -
+ )} {/* Manual instructions: only after an automated attempt failed (or on a diff --git a/desktop/src/renderer/components/ui/states.tsx b/desktop/src/renderer/components/ui/states.tsx index a2cb359c9..285c3fe24 100644 --- a/desktop/src/renderer/components/ui/states.tsx +++ b/desktop/src/renderer/components/ui/states.tsx @@ -164,13 +164,30 @@ export function ErrorState(props: ErrorStateProps) { export type FieldErrorProps = { children: React.ReactNode; className?: string; + /** Type step. The app has always used both: 19 of the 25 hand-rolled copies + * this primitive replaced were `text-3xs`, 6 were `text-2xs`. It is a PROP + * rather than something a caller passes through `className` because this + * component CONCATENATES className onto the base — and Tailwind resolves two + * competing utilities by CSS SOURCE ORDER, not by the order they appear in + * the attribute, so `className="text-2xs"` would silently keep rendering at + * 3xs (the same trap that made Button's pills render as rectangles). */ + size?: '3xs' | '2xs'; + /** Element to render. Default `span` (inline) matches how the primitive + * shipped. Pass `p`/`div` where the line is a BLOCK under a field: vertical + * margin and padding (`mt-1`, `pb-2`) do not lay out on an inline element, + * so a `

` swapped to a bare span would silently lose + * its gap wherever the parent is not a flex/grid container. */ + as?: 'span' | 'p' | 'div'; }; /** Field-level errors stay short lines under the input — not cards. */ -export function FieldError({ children, className = '' }: FieldErrorProps) { +export function FieldError({ children, className = '', size = '3xs', as: Tag = 'span' }: FieldErrorProps) { + // Literal class strings, not `text-${size}` — Tailwind scans source text for + // whole class names and never sees an interpolated one. + const sizeClass = size === '2xs' ? 'text-2xs' : 'text-3xs'; return ( - + {children} - + ); } diff --git a/desktop/tests/field-error-adoption.test.ts b/desktop/tests/field-error-adoption.test.ts new file mode 100644 index 000000000..ce2f8f05c --- /dev/null +++ b/desktop/tests/field-error-adoption.test.ts @@ -0,0 +1,98 @@ +import { describe, it, expect } from 'vitest'; +import { readFileSync, readdirSync, statSync } from 'node:fs'; +import { join } from 'node:path'; + +const RENDERER = join(__dirname, '..', 'src', 'renderer'); + +function walk(dir: string, out: string[] = []): string[] { + for (const e of readdirSync(dir)) { + const p = join(dir, e); + if (statSync(p).isDirectory()) walk(p, out); + else if (p.endsWith('.tsx') && !p.endsWith('.test.tsx')) out.push(p); + } + return out; +} + +/** + * ROADMAP "Adopt the `FieldError` primitive": 25 sites across 14 files wrote the + * primitive's exact markup by hand. They are gone; this keeps them gone. + * + * The check is on the CLASS PAIR, because that pair *is* the primitive's body — + * anything that renders it by hand is a copy, and the copies drift (the sweep + * found the app split between text-3xs and text-2xs, which is why the primitive + * gained a `size` prop rather than silently resizing six lines). + * + * Each exemption below is a site that matches the pair but is NOT a field error. + * A new one needs a reason here, not just a name. + */ +const EXEMPT: Record = { + // Four copies of one static caption under the skip-permissions toggle. Not a + // failure report — it is always-on warning copy, and FieldError carries + // role="alert", which would make a screen reader interrupt with it every time + // the toggle flips. Their real problem is that there are four of them; the fix + // is a shared warning component, not this primitive. + 'App.tsx': { count: 1, why: 'skip-permissions caption, not a field error' }, + 'SessionStrip.tsx': { count: 1, why: 'skip-permissions caption, not a field error' }, + 'ResumeBrowser.tsx': { count: 1, why: 'skip-permissions caption, not a field error' }, + 'ResumeOptionsPopover.tsx': { count: 1, why: 'skip-permissions caption, not a field error' }, + // NOT here: SettingsPanel's confirm-dialog prose. It is dimmed to + // `text-destructive-fg/80`, and the pattern below excludes the opacity + // variants on purpose — an opacity modifier is prose styling, not this + // primitive's body. So it needs no exemption, and granting one would have + // exempted a file full of real fields. + // A destructive text BUTTON (hover fill, padding, rounded), not an error line. + 'GitReviewView.tsx': { count: 1, why: 'destructive text button' }, + // Deliberately role="status" (polite): a failed update check must not + // interrupt what the user is reading. FieldError is role="alert". + 'UpdateButton.tsx': { count: 1, why: 'role="status" by design' }, +}; + +/** How many times a file writes the primitive's class pair by hand. The pattern + * is deliberately NOT anchored to a tag: a copy is a copy whether it lands on a + *

, a or a

. `text-destructive-fg/80` (an opacity variant) is + * excluded — that is prose styling, not this primitive. */ +function handRolledCount(src: string): number { + return (src.match(/text-[23]xs text-destructive-fg(?![/\w-])/g) ?? []).length; +} + +describe('FieldError adoption', () => { + it('no file hand-rolls the primitive markup', () => { + const offenders: string[] = []; + for (const file of walk(RENDERER)) { + if (file.endsWith(join('ui', 'states.tsx'))) continue; // the primitive itself + const base = file.split(/[\\/]/).pop()!; + if (base in EXEMPT) continue; + if (handRolledCount(readFileSync(file, 'utf8')) > 0) offenders.push(base); + } + expect( + offenders, + 'Use (components/ui/states.tsx). If the site is not a field ' + + 'error, add it to EXEMPT above with the reason.', + ).toEqual([]); + }); + + // COUNTS, not just names. Exempting a whole FILE would let the next + // hand-rolled copy hide inside one — SettingsPanel is exempt for exactly one + // paragraph and holds plenty of real fields, so "SettingsPanel is allowed to + // match" is too coarse a permission to grant. + it('an exemption covers exactly the occurrences it was granted for', () => { + const counts = new Map( + walk(RENDERER).map((f) => [f.split(/[\\/]/).pop()!, handRolledCount(readFileSync(f, 'utf8'))]), + ); + for (const [name, { count, why }] of Object.entries(EXEMPT)) { + expect(counts.get(name) ?? 0, `${name} (exempt: ${why})`).toBe(count); + } + }); + + it('every exemption still matches something', () => { + // An exemption that stops being true is a place for the next copy to hide. + const seen = new Set( + walk(RENDERER) + .filter((f) => handRolledCount(readFileSync(f, 'utf8')) > 0) + .map((f) => f.split(/[\\/]/).pop()!), + ); + for (const name of Object.keys(EXEMPT)) { + expect(seen.has(name), `${name} no longer hand-rolls it — drop the exemption.`).toBe(true); + } + }); +}); diff --git a/desktop/tests/ui-primitives.test.tsx b/desktop/tests/ui-primitives.test.tsx index ce6aa0660..7ed4f7a7f 100644 --- a/desktop/tests/ui-primitives.test.tsx +++ b/desktop/tests/ui-primitives.test.tsx @@ -667,5 +667,28 @@ describe('state family', () => { expect(el.className).toContain('text-destructive'); expect(el.className).toContain('text-3xs'); expect(el.className).not.toContain('text-red-500'); + expect(el.tagName).toBe('SPAN'); + }); + + // The adoption sweep found the app split between two type steps and a mix of + // block/inline hosts, so the primitive takes both as PROPS. Neither could be a + // className pass-through: this component concatenates className onto its base, + // and Tailwind resolves competing utilities by CSS source order, so a caller's + // `text-2xs` would silently keep rendering at 3xs. + it('size="2xs" replaces the base step rather than piling on next to it', () => { + render(Too short); + const el = screen.getByText('Too short'); + expect(el.className).toContain('text-2xs'); + expect(el.className).not.toContain('text-3xs'); + }); + + it('as="p" renders a block host so vertical margin still lays out', () => { + // `mt-1` on an inline element does nothing; 21 of the swapped sites were + //

carrying exactly that kind of spacing class. + render(Nope); + const el = screen.getByText('Nope'); + expect(el.tagName).toBe('P'); + expect(el.className).toContain('mt-1'); + expect(el.getAttribute('role')).toBe('alert'); }); });