opensource/coder
3
0
Fork 0
mirror of https://github.com/coder/coder.git synced 2026-10-29 17:44:33 +08:00
Code Issues Packages Projects Releases Wiki Activity

docs: add frontend pattern rule contract (FE1-FE10) (#27407)

Browse Source 
## What

Adds `.claude/docs/FRONTEND_PATTERNS.md`, a canonical frontend rule
contract for `site/src/` with ten stable rule IDs (FE1 to FE10), each
with short incorrect/correct examples using real repo idioms. Adds a
compact non-negotiables index at the top of `site/AGENTS.md` and links
the contract from the root `AGENTS.md` frontend routing line.

## Why

Frontend PRs keep drawing the same review findings. Mining the last 18
months of `site/` PR review data (330 closed/merged PRs, 1,105 inline
comments, 358 actionable pattern findings across 57 PRs) shows the
recurring themes, in order of frequency: missing Storybook interaction
coverage, loose TypeScript, component duplication and PR scope,
redundant comments, unhandled UI states, accessibility, react-query
misuse, and effect misuse. The guidance largely existed in
`site/AGENTS.md` already, but as unprioritized prose with no shared
vocabulary.

Stable rule IDs give reviewers, agents, and tooling one shared language
("FE7: re-typed query key"), and the rules are ordered by how often
reviewers actually flagged them.

This is the first PR of a stack of two:

1. This PR: the rule contract.
2. A `frontend-review` skill that audits diffs against these rules
pre-PR (#27408).

A third PR with a deterministic ratchet script (#27409) was closed; the
machine-checkable subset will be enforced through proper linting
instead.

## Notes for reviewers

The rule content is derived from your own review comments (for example,
the re-typed query key example in FE7 mirrors feedback on #27385). If a
rule does not match how you want the frontend built, the contract file
is the place to change it.

> This PR was written by Mux, an AI coding agent, on behalf of Mike.
This commit is contained in:
Michael Suchacz
2026-07-23 07:21:27 +02:00
committed by GitHub
parent 80f8e4f624
commit 5565fcd03b
3 changed files with 238 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.claude/docs/FRONTEND_PATTERNS.md
+211
View File
@@ -0,0 +1,211 @@
# Frontend Patterns (FE rules)
The canonical rule contract for changes under `site/src/`. Each rule has a
stable ID (FE1 to FE10) so code review comments, agent guidance, and tooling
can all reference the same rule. The rules are ordered by how often reviewers
flagged violations in past frontend PRs.
How to use this document:
- Writing code: treat every rule as a default requirement, not a suggestion.
- Reviewing: cite rule IDs in comments (for example, "FE7: re-typed query key").
- Disagreeing: propose a change to this file instead of silently deviating.
`site/AGENTS.md` holds the one-line summary of each rule plus general frontend
workflow guidance. This file is the authoritative version with examples.
## FE1: UI behavior ships with Storybook interaction coverage
Every user-visible behavior change needs a Storybook story whose `play`
function actually exercises the interaction. Jest/RTL tests are for pure logic
(helpers, hooks without DOM interaction), not for UI interactions.
- The story must perform the real interaction: open the dropdown, submit the
form, pin the mobile viewport. A story that renders a closed popover tests
nothing.
- Cover the meaningful branches: error, empty, disabled, and mobile states,
not only the happy path.
- Assert both sides of an invariant: the item that changed and a neighboring
item that must not change.
**Incorrect (interaction test in Jest/RTL):**
```tsx
// ModelSelector.test.tsx
it("selects a model", async () => {
render(<ModelSelector {...props} />);
await userEvent.click(screen.getByRole("button"));
// ...
});
```
**Correct (Storybook story with a play function):**
```tsx
// ModelSelector.stories.tsx
export const SelectModel: Story = {
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
await userEvent.click(canvas.getByRole("button", { name: /model/i }));
await expect(canvas.getByRole("listbox")).toBeVisible();
},
};
```
## FE2: No loose types
- Never use `any`, `as unknown as X`, or non-null assertions in any form
(`x!.y`, `items[0]!`, `fn()!`, `value! as T`).
- Prefer type annotations and narrowing over `as` casts. If types do not
align, fix them at the source.
- Use generated types from `api/typesGenerated.ts` for all API data. Never
re-declare a type that the backend already generates.
- If a component requires a prop to function, make the prop required.
**Incorrect:**
```tsx
const config = data as unknown as ChatModelConfig;
```
**Correct:**
```tsx
import type { ChatModelConfig } from "api/typesGenerated";
const config: ChatModelConfig = parseConfig(data);
```
## FE3: Reuse before building, and keep PRs single-purpose
- Before writing a component, hook, or helper, search `site/src/components/`
and sibling feature folders for an existing implementation. Reviewers have
repeatedly found new files that were near-copies (or byte-identical copies)
of existing ones.
- Use existing wrapped primitives (Combobox, dialogs, tables) instead of
hand-assembling the underlying pieces they already wrap.
- Delete dead code and unreachable branches instead of carrying them along.
- Keep the PR scoped to one change. Move unrelated cleanups, renames, and
drive-by refactors to separate PRs.
## FE4: Comments must earn their place
- Do not write comments that restate the identifier, the assertion, or the
control flow below them. Reviewers flag these in nearly every AI-authored
PR.
- Keep only comments that capture a non-obvious invariant, external
constraint, or deliberate tradeoff, in 1 to 3 lines.
- Before pushing, re-read every comment your diff adds and delete the ones a
reader would not need.
**Incorrect:**
```tsx
// Track whether the panel is open
const [isOpen, setIsOpen] = useState(false);
```
## FE5: Handle every UI state, and never clobber user state
Every view that renders server data must handle this matrix:
| State | Requirement |
|---------|--------------------------------------------------------------|
| Loading | Show a skeleton or spinner, never a blank or half-valid view |
| Error | Surface the actionable server error, not a generic message |
| Empty | Deliberate empty state with copy, never a blank region |
| Refetch | Keep showing valid data; never reset forms or selections |
- A background refetch must not reinitialize form state or discard in-progress
edits.
- When a mutation partially fails, the UI must reflect what succeeded and what
did not (see FE7 for cache invalidation).
- Render a visible fallback ("Untitled", "N/A") for nullable display data.
## FE6: Accessibility is behavior, not decoration
- Every interactive element must be keyboard-reachable, including the way to
discover why a control is disabled.
- The accessible name must contain the visible label text. Do not replace a
trigger's name with an unrelated `aria-label` (a "Label in Name" violation).
- Check what the primitive does with `aria-*` props before setting them; some
(for example cmdk) silently overwrite `role` and `aria-selected`.
- Preserve focus position across dialogs and route transitions.
- When visually hiding an interactive element, also remove it from the tab
order and accessibility tree, or conditionally render it out of the DOM.
## FE7: React Query discipline
All server data goes through react-query. Never call an `API` function or
`fetch` directly inside a component, and never manage server-data lifecycle
with `useState` + `useEffect`.
- Import query key constants from `api/queries/`. Never re-type a key as a
string literal in a component, story, or test. If the constant is not
exported, export it; do not copy the string.
- `isLoading` means no cached data yet; `isFetching` includes background
refetches. Do not gate on both (`isLoading || isFetching` is just
`isFetching`) and do not blank valid data during a background refetch.
- After a mutation, invalidate every affected query, including on partial
failure paths (for example, when the second of two chained mutations fails).
- Use `mutate()` with `onSuccess`/`onError` callbacks unless you need the
result for control flow. Never wrap `mutateAsync()` in a `try/catch` with an
empty catch block.
**Incorrect (re-typed query key in a story):**
```tsx
parameters: {
queries: [{ key: ["chat-model-configs"], data: [MockChatModelConfig] }],
},
```
**Correct (imported constant):**
```tsx
import { chatModelConfigsKey } from "api/queries/chats";
parameters: {
queries: [{ key: chatModelConfigsKey, data: [MockChatModelConfig] }],
},
```
## FE8: Effects are a last resort
Decide where logic goes before reaching for `useEffect`:
1. Can it be computed from props/state during render? Compute it in render
(or `useMemo` if expensive). Do not mirror it into state.
2. Does it respond to a user action? Put it in the event handler.
3. Is it server data? Use a query or mutation (FE7).
4. Is it synchronizing with an external system (WebSocket, DOM API,
subscription)? This is the only case for `useEffect`.
- Never write an effect that reads state A and calls `setStateB`; derive the
value instead.
- Audit every dependency you add to an effect that owns a connection or
triggers fetches. Past regressions include a dependency change that
disconnected and reconnected the chat WebSocket on every message, and an
effect on `isFetching` that caused an infinite fetch loop.
- Delete effects that only synchronize a ref nobody reads.
## FE9: Fixtures and mocks follow repo conventions
- Represent entities with shared `Mock*` constants in `site/src/testHelpers/`
(for example `MockChatModelConfig` in `testHelpers/chatModels.ts`). When a
story needs a variant, spread the base fixture into a named local constant.
- Compose story query wiring (`{ key, data }`) inline per story so each story
is readable on its own. Share the entity fixture, not a pre-wired query
object.
- Query keys in mocks follow FE7: import the constant.
## FE10: Tests assert observable behavior
- Query by semantic role and accessible name (`getByRole`, `getByLabelText`).
This tests accessibility (FE6) for free.
- Never use `querySelector`, class-name substring matches
(`[class*='flex-col']`), or DOM-geometry assertions. They break silently on
refactors without any user-visible regression.
- Use `data-testid` only when an element has no semantic role or name.
- Keep tests deterministic: no `behavior: "smooth"` scrolling, explicit
locales for `toLocaleString()`, and time passed in as a prop or mock.
AGENTS.md
+1 -1
View File
@@ -11,7 +11,7 @@ Rule #1: If you want exception to ANY rule, YOU MUST STOP and get explicit permi
- Language and area docs: Use [Modern Go](.claude/docs/GO.md), [Testing Patterns and Best Practices](.claude/docs/TESTING.md), [Database Development Patterns](.claude/docs/DATABASE.md), [OAuth2 Development Guide](.claude/docs/OAUTH2.md), [Coder Architecture](.claude/docs/ARCHITECTURE.md), [Troubleshooting Guide](.claude/docs/TROUBLESHOOTING.md), [Documentation Style Guide](.claude/docs/DOCS_STYLE_GUIDE.md), and [Pull Request Description Style Guide](.claude/docs/PR_STYLE_GUIDE.md) when that area is in scope.
- Docs content scope: Use [Coder Docs Content Guidelines](docs/.style/content-guidelines.md) to decide whether a piece of content belongs in `docs/` at all. The Documentation Style Guide above covers prose and formatting; the content guidelines govern scope and routing and supersede the style guide on conflicts.
- Compatibility: `.agents/docs` symlinks to `.claude/docs` for agent runtimes that look there.
- Frontend: Read [Frontend Development Guidelines](site/AGENTS.md) before changing anything under `site/`.
- Frontend: Read [Frontend Development Guidelines](site/AGENTS.md) before changing anything under `site/`. For code under `site/src/`, the [Frontend Patterns](.claude/docs/FRONTEND_PATTERNS.md) rule contract (FE1 to FE10) applies.
- Docs prose: For prose-only edits to existing `docs/` pages, refer to the prose style guide at [`docs/.style/style-guide/`](docs/.style/style-guide/README.md).
For supporting agent-specific guidance, refer to [`.claude/docs/DOCS_STYLE_GUIDE.md`](.claude/docs/DOCS_STYLE_GUIDE.md), which covers structure, research, and content patterns.
- Docs authoring: For new, moved, or restructured `docs/` pages, or when unsure, load the [`write-docs` skill](.claude/skills/write-docs/SKILL.md) first. It points at the canonical content guidelines and the prose style guide above, then walks research, routing, Diátaxis mode, structure, and validation.
site/AGENTS.md
+26
View File
@@ -1,5 +1,31 @@
# Frontend Development Guidelines
## Frontend Non-Negotiables (FE rules)
Read [Frontend Patterns](../.claude/docs/FRONTEND_PATTERNS.md) before changing
anything under `site/src/`. It is the canonical contract behind these rule
IDs; reviewers cite them as FE1 to FE10.
- **FE1**: UI behavior changes ship with Storybook stories whose `play`
function exercises the real interaction. Jest/RTL is for pure logic only.
- **FE2**: No `any`, no `as unknown as`, no avoidable `as` casts. Use
generated types from `api/typesGenerated.ts`.
- **FE3**: Search for an existing component or helper before writing one.
Keep PRs single-purpose.
- **FE4**: No comments that restate identifiers, assertions, or control flow.
- **FE5**: Every view handles loading, error, empty, and refetch states
without clobbering user state.
- **FE6**: Interactive elements stay keyboard-reachable with correct
accessible names.
- **FE7**: All server data through react-query. Import query key constants,
never re-type them as string literals.
- **FE8**: `useEffect` only to synchronize with external systems. Never
derive state or chain fetches in effects.
- **FE9**: Share entity fixtures as `Mock*` constants; compose story query
wiring inline per story.
- **FE10**: Tests query semantic roles and names. No `querySelector` or
class-name assertions.
## TypeScript LSP Navigation (USE FIRST)
When investigating or editing TypeScript/React code, always use the TypeScript language server tools for accurate navigation: