Adds web/GUARDRAILS.md capturing the recurring CI-guardrail and test-harness lessons in one place: the check:size (250 KB-gz largest chunk) and check:colors (design-token) guards, the jsdom/storybook vitest split, MSW onUnhandledRequest:"error" overrides, RTL accessible-name collisions, Storybook nested-router/portal handling, and the components/ui code-style split. Wires a pointer from CLAUDE.md. All claims verified against the live scripts, ci.yaml, and src/test/. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -28,3 +28,4 @@ cargo clippy --workspace --all-targets -- -D warnings # lint before committing
|
|||||||
- **Code navigation:** prefer the insikt LSP server over grep/glob — it resolves macro-generated symbols that text search misses. (insikt runs standalone, not via the gateway MCP.)
|
- **Code navigation:** prefer the insikt LSP server over grep/glob — it resolves macro-generated symbols that text search misses. (insikt runs standalone, not via the gateway MCP.)
|
||||||
- **Dependencies:** manage via the `cargo-mcp` server rather than editing `Cargo.toml` by hand.
|
- **Dependencies:** manage via the `cargo-mcp` server rather than editing `Cargo.toml` by hand.
|
||||||
- **Formatting:** `cargo +nightly fmt` (nightly toolchain required).
|
- **Formatting:** `cargo +nightly fmt` (nightly toolchain required).
|
||||||
|
- **Frontend guardrails:** before touching `web/`, read **[web/GUARDRAILS.md](web/GUARDRAILS.md)** — it covers the CI gate (`check:size` 250 KB-gz budget, `check:colors` design-token enforcement) and the test-harness quirks (MSW `onUnhandledRequest: "error"`, the jsdom/storybook vitest split, RTL accessible-name collisions, Storybook nested-router and portal handling, and the `components/ui/` code-style split).
|
||||||
|
|||||||
@@ -0,0 +1,146 @@
|
|||||||
|
# Frontend Guardrails & Test Harness
|
||||||
|
|
||||||
|
The web frontend has a handful of CI guardrails and test-harness quirks that trip
|
||||||
|
new work in the same predictable ways. This file consolidates them so the lessons
|
||||||
|
are enforced up front instead of rediscovered one failing run at a time.
|
||||||
|
|
||||||
|
The canonical gate is `.gitea/workflows/ci.yaml`, which runs (in order) from `web/`:
|
||||||
|
|
||||||
|
```
|
||||||
|
pnpm typecheck # tsc -b --noEmit
|
||||||
|
pnpm lint # eslint .
|
||||||
|
pnpm test # vitest run (jsdom + storybook projects)
|
||||||
|
pnpm build # tsc -b && vite build
|
||||||
|
pnpm check:size # scripts/check-bundle-size.mjs
|
||||||
|
pnpm check:colors # scripts/check-no-raw-colors.mjs
|
||||||
|
```
|
||||||
|
|
||||||
|
Run the whole gate locally before pushing. The two `check:*` scripts only have
|
||||||
|
meaningful output **after** `pnpm build` has produced `dist/`.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Guardrail: bundle size (`check:size`)
|
||||||
|
|
||||||
|
`scripts/check-bundle-size.mjs` fails if the **largest single JS chunk** exceeds
|
||||||
|
**250 KB gzipped**. It measures every file in `dist/assets/*.js` and reports the
|
||||||
|
biggest.
|
||||||
|
|
||||||
|
- The budget is on the *largest chunk*, not the total. Code-split chunks are
|
||||||
|
measured independently — a heavy primitive that lands in its own lazy chunk
|
||||||
|
costs that chunk's size, not the entry bundle's. Past additions (Base UI
|
||||||
|
Select ≈12.6 KB gz, the Drawer ≈12.7 KB gz) paid their cost in a separate
|
||||||
|
`*.js` chunk and left the main `index` chunk untouched.
|
||||||
|
- **When a UI primitive pushes the main chunk over budget**, extract the
|
||||||
|
component that uses it into its own module and `React.lazy()` it (the codebase
|
||||||
|
already lazy-loads `ObjectEditForm` / `FieldsPage`). This is the standard fix,
|
||||||
|
especially for anything rendered conditionally (e.g. only on a narrow
|
||||||
|
viewport) — the wide path then never pays for it.
|
||||||
|
- Adding one more Base UI primitive to the always-loaded app shell costs
|
||||||
|
single-digit KB gz; there is comfortable headroom under 250.
|
||||||
|
|
||||||
|
## Guardrail: design tokens / no raw colors (`check:colors`)
|
||||||
|
|
||||||
|
`scripts/check-no-raw-colors.mjs` fails if any raw Tailwind palette utility
|
||||||
|
appears **outside `src/components/ui/`**. The regex matches
|
||||||
|
`{text|bg|border|ring|fill|stroke|from|to|via|decoration|outline|divide|placeholder}-{palette}-{50..950}`
|
||||||
|
(palette = `neutral|gray|slate|…|rose`).
|
||||||
|
|
||||||
|
- In app / shell code use **token classes only**: `bg-accent`, `text-foreground`,
|
||||||
|
`text-muted-foreground`, `bg-primary`, etc. Numerics like `gap-2` and token
|
||||||
|
names are never flagged — only `palette-shade` pairs are.
|
||||||
|
- `src/components/ui/` (the vendored kit) is **exempt** and may use raw colors.
|
||||||
|
- New status/semantic colors must be exposed as tokens, not raw palette classes.
|
||||||
|
In Tailwind v4 a token utility (`bg-success`, `text-warning`) only *exists* if
|
||||||
|
the token is declared in `@theme inline` as `--color-<name>: var(--<name>)` —
|
||||||
|
defining the raw CSS var in `:root` alone is not enough. Opacity modifiers on
|
||||||
|
tokens (`bg-primary/10`) do resolve.
|
||||||
|
- For a sweeping token migration, add/enable the `check:colors` guard **last**
|
||||||
|
(after the file-by-file swap) — it can only pass once the whole migration is
|
||||||
|
complete.
|
||||||
|
|
||||||
|
> **Contrast caveat:** do not trust a WCAG contrast ratio quoted in a plan or a
|
||||||
|
> prior review — recompute it (OKLCH → linear sRGB → relative luminance) before
|
||||||
|
> changing a color token. A near-black foreground on a *lighter* colored
|
||||||
|
> background has *higher* contrast; a single wrong measurement has propagated
|
||||||
|
> through spec + plan more than once here.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Test harness: two vitest projects
|
||||||
|
|
||||||
|
`vite.config.ts` defines **two** vitest projects, and a test file is claimed by
|
||||||
|
exactly one:
|
||||||
|
|
||||||
|
| Project | Environment | Claims |
|
||||||
|
|-------------|----------------------|-------------------------|
|
||||||
|
| (default) | jsdom + `src/test/setup.ts` | `*.test.ts(x)` |
|
||||||
|
| `storybook` | chromium (Playwright, via `storybookTest`) | `*.stories.tsx` |
|
||||||
|
|
||||||
|
- Component tests render through **`renderApp`** (`src/test/render.tsx`), which
|
||||||
|
wraps `QueryClientProvider` + a `createMemoryRouter` and **side-imports
|
||||||
|
`../i18n`** — so i18n is auto-initialized and you do **not** add extra
|
||||||
|
providers. Pass an initial route via `renderApp(ui, { route: "/x" })`.
|
||||||
|
- Story-as-test runs the story's `play()` fn in a real browser:
|
||||||
|
`pnpm vitest run <file.stories.tsx>`. A canonical pass reports
|
||||||
|
"1 Test File, 1 Test passed". A first-cold-cache storybook run can emit a
|
||||||
|
transient `Cannot read properties of null (reading useEffect)` from
|
||||||
|
`QueryClientProvider` — it does not reproduce once the cache warms; re-run to
|
||||||
|
confirm green.
|
||||||
|
|
||||||
|
## Test harness: MSW handlers (`onUnhandledRequest: "error"`)
|
||||||
|
|
||||||
|
`src/test/setup.ts` starts the MSW server with **`onUnhandledRequest: "error"`**
|
||||||
|
and `resetHandlers()` after each test. Shared handlers live in
|
||||||
|
`src/test/handlers.ts`.
|
||||||
|
|
||||||
|
- Any request with **no matching handler fails the test** with
|
||||||
|
`[MSW] Cannot bypass a request when using the error strategy`. If a hook hits
|
||||||
|
an endpoint the shared handlers don't cover (e.g. `PATCH /…/terms/:id` for an
|
||||||
|
update-mutation success test), **add a per-test override** —
|
||||||
|
`server.use(http.patch(url, () => new HttpResponse(null, { status: 204 })))` —
|
||||||
|
rather than editing the shared `handlers.ts`. The `afterEach` reset keeps the
|
||||||
|
override test-local.
|
||||||
|
|
||||||
|
## Test harness: React Testing Library accessible-name collisions
|
||||||
|
|
||||||
|
`getByRole('textbox', { name: /key/i })` throws on **multiple matches** when two
|
||||||
|
inputs resolve to the same accessible name — e.g. a create-form input
|
||||||
|
(`<Label htmlFor>`) and a rename input (`aria-label`) both named "Key" once a
|
||||||
|
rename form is open.
|
||||||
|
|
||||||
|
- Use `getAllByRole(...)` and select by position (the rename form renders after
|
||||||
|
the create form in the DOM, so take the last), or give the inputs distinct
|
||||||
|
accessible names.
|
||||||
|
|
||||||
|
## Test harness: Storybook routers and portals
|
||||||
|
|
||||||
|
- `.storybook/preview.tsx` already wraps every story in
|
||||||
|
`QueryClientProvider` + `ConfigProvider` + `MemoryRouter`. **Do not add a
|
||||||
|
`MemoryRouter` decorator** inside a story — nesting throws
|
||||||
|
*"You cannot render a `<Router>` inside another `<Router>`"*. To exercise
|
||||||
|
URL-dependent state, drive it through `play()` interactions (click a header
|
||||||
|
link) rather than seeding `initialEntries` via a nested router.
|
||||||
|
- Portalled content (Base UI Popup/Portal, toasts) renders outside the canvas.
|
||||||
|
Assert it with `within(document.body).findByText(...)`, and open triggers from
|
||||||
|
the `play` context with `userEvent.click(canvas.getByRole('button', { name: 'Open' }))`.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Code-style split (convention, not lint)
|
||||||
|
|
||||||
|
ESLint passes either way, so this is enforced by review, not tooling — **check a
|
||||||
|
neighbor file before writing**:
|
||||||
|
|
||||||
|
- `src/components/ui/` (vendored shadcn / base-nova kit): **no semicolons**,
|
||||||
|
single-quote base style.
|
||||||
|
- The rest of `src/` (app source, hooks, pages): **double-quote + semicolons**.
|
||||||
|
- `*.stories.tsx`: single-quote, no semicolons.
|
||||||
|
|
||||||
|
A component placed in `ui/` must match the no-semicolon kit style even though the
|
||||||
|
rest of the app uses semicolons.
|
||||||
|
|
||||||
|
> Note: `react-refresh/only-export-components` is configured as `warn`
|
||||||
|
> (`allowConstantExport: true`), so exporting a component **and** a helper from
|
||||||
|
> one `.tsx` (e.g. `button.tsx` exporting `buttonVariants`) is an accepted
|
||||||
|
> tradeoff when a plan needs the helper colocated — `pnpm lint` still exits 0.
|
||||||
Reference in New Issue
Block a user