Skip to content

Commit e5da708

Browse files
docs: write down how to model a feature's state, and lint the checkable part (#14472)
Writes down the state-modelling conventions we keep re-explaining in review, and adds one lint rule for the part that is mechanically checkable. ### Why Reviews of anything with a beginning and an end — a wizard, an upload, an onboarding flow — keep converging on the same handful of points: several independent booleans that must agree, values stored that could be derived, and effects used to keep two pieces of internal state in sync rather than to talk to something outside the app. Each time it gets re-argued from scratch on the PR, which is slow for the reviewer and unpleasant for the author, who reasonably asks why nobody said so earlier. `docs/guidance/state-and-effects.md` is glob-loaded on `src/**/*.ts` and `src/**/*.vue`, so it applies automatically rather than needing to be found. ### What it says One state value as a discriminated union instead of N booleans, named events rather than assignment scattered across call sites, a pure transition function so one place owns the rules, `computed` for anything derivable, and effects reserved for synchronising outward. Plus the Vue mapping — state to Pinia, transition to a pure function, derivation to `computed`, synchronisation to `watch`. It deliberately does **not** recommend a state-machine library. There is no XState or `createMachine` precedent anywhere in `src/`, and a hand-rolled union with a pure `reduce` gets the same guarantees without the dependency. ### The lint rule `no-restricted-syntax` flagging `getBoundingClientRect`, `getComputedStyle` and `querySelector*` inside a `computed`. A derivation that measures the DOM runs a layout read on every recompute and cannot be unit-tested without a browser. It ships at **`warn`, not `error`**, because there are exactly four pre-existing instances and this PR does not fix them: | File | Count | | ---- | ----- | | `src/components/maskeditor/BrushCursor.vue` | 2 | | `src/components/breadcrumb/SubgraphBreadcrumb.vue` | 1 | | `src/components/topbar/WorkflowTabs.vue` | 1 | Promote to `error` once those derive from stores instead. I verified the count against `main` at the current head rather than trusting the number I first wrote. ### Scope Docs and lint config only — no runtime code, no behaviour change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent a8483a8 commit e5da708

3 files changed

Lines changed: 186 additions & 0 deletions

File tree

AGENTS.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ See @docs/guidance/\*.md for file-type-specific conventions (auto-loaded by glob
44

55
- `docs/guidance/engineering.md` — general engineering guidelines, project philosophy, code-review checklist, external resource links
66
- `docs/guidance/vue-components.md` — Vue 3 Composition API best practices
7+
- `docs/guidance/state-and-effects.md` — modelling a feature's state: one discriminated union, named events, a pure transition, effects reserved for synchronising outward
78
- `docs/guidance/typescript.md` — TypeScript type-safety rules
89
- `docs/guidance/vitest.md` — Vitest unit/component test conventions
910
- `docs/guidance/playwright.md` — Playwright E2E conventions and API-mock typing table

docs/guidance/state-and-effects.md

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
---
2+
globs:
3+
- 'src/**/*.ts'
4+
- 'src/**/*.vue'
5+
---
6+
7+
# State, Effects and Workflows
8+
9+
How to represent a multi-step process — onboarding, a wizard, an upload, a
10+
checkout, anything with a beginning and an end — in a reactive framework.
11+
12+
> **Events initiate work. One explicit state represents the workflow. Pure
13+
> derivations shape the UI. Effects only synchronise with external systems.**
14+
15+
This applies to any framework; the Vue mapping is at the bottom.
16+
17+
## 1. One state, not several booleans
18+
19+
Hold the **minimum authoritative facts** needed to render and continue. Anything
20+
computable from them is not state.
21+
22+
Avoid independent booleans for what is really one value. Prefer a discriminated
23+
union, so invalid combinations are structurally impossible rather than merely
24+
unlikely.
25+
26+
```ts
27+
// ✗ four facts that must agree, and nothing makes them
28+
const steps = ref<Step[]>([])
29+
const stepIdx = ref(-1)
30+
const waiting = ref(false)
31+
const active = ref<Flow | null>(null)
32+
33+
// ✓ one fact
34+
type FlowState =
35+
| { phase: 'idle' }
36+
| { phase: 'awaiting'; flow: Flow; steps: Step[]; idx: number }
37+
| { phase: 'showing'; flow: Flow; steps: Step[]; idx: number }
38+
| { phase: 'finished'; flow: Flow; outcome: Outcome }
39+
```
40+
41+
**The tell:** if ending the workflow means resetting four variables in the right
42+
order, it was one state and you gave it four variables.
43+
44+
## 2. Change state through named events and a pure transition
45+
46+
```ts
47+
type FlowEvent =
48+
| { type: 'started'; flow: Flow; steps: Step[] }
49+
| { type: 'advanced' }
50+
| { type: 'skipped' }
51+
52+
function reduceFlow(state: FlowState, event: FlowEvent): FlowState // pure switch
53+
```
54+
55+
Invalid transitions become harmlessan event that means nothing in the current
56+
phase returns the state untouchedand valid ones become reviewable in one
57+
place. This is the same reason most frameworks recommend a reducer once
58+
state-update logic gets complex.
59+
60+
It is also exhaustively testable: every state crossed with every event, including
61+
the pairs that should do nothing.
62+
63+
## 3. Async orchestration belongs in commands
64+
65+
Do **not** build chains where each step is an effect reacting to the last:
66+
67+
```
68+
saving changed → effect saves → saved changed → effect invalidates
69+
→ invalidated changed → effect navigates
70+
```
71+
72+
Nobody can read that as one sequence, and the middle of it is reachable from
73+
places you did not intend. Give the whole causal sequence to one function:
74+
75+
```ts
76+
async function startFlow(id: string) {
77+
const data = await load(id)
78+
if (!data) return false
79+
dispatch({ type: 'started', flow: id, steps: build(data) })
80+
await settle()
81+
dispatch({ type: 'stepEntered', idx: 0 })
82+
return true
83+
}
84+
```
85+
86+
## 4. Derive everything derivable
87+
88+
Do not store `isOpening`, `canTransition`, `isLast`, or a second copy of data
89+
that already exists. If a `computed` can name it, do not put it in a `ref` and
90+
keep it in sync by hand — the sync is where the bugs live.
91+
92+
## 5. Reserve effects for synchronising with the outside
93+
94+
**Good effects** — one-way, outward, and they write nothing anything else reads:
95+
96+
- state changed → emit telemetry
97+
- state changed → write a setting
98+
- component disposed → abort a request or unsubscribe
99+
- external socket event → _dispatch an event_ (not: assign state directly)
100+
101+
**Bad effects:**
102+
103+
- pointer target changed → recalculate steps
104+
- success changed → advance the workflow
105+
- an effect that writes state a second effect reads
106+
107+
If two effects communicate through shared state, that is a transition wearing a
108+
disguise. Put it in the reducer.
109+
110+
## 6. Do not inspect the DOM for something a store already knows
111+
112+
Avoid `getBoundingClientRect`, `getComputedStyle`, and `document.querySelector`
113+
for positions and sizes that a store holds — especially inside a `computed`,
114+
where every recompute becomes a layout read.
115+
116+
For canvas-anchored UI: `layoutStore` holds node bounds and `useTransformState`
117+
mirrors the camera, both reactive, so a screen rect is a **derivation** rather
118+
than a measurement. Floating UI accepts a
119+
[virtual element](https://floating-ui.com/docs/virtual-elements) for exactly
120+
this.
121+
122+
Two caveats worth knowing rather than discovering:
123+
124+
- `layoutStore` bounds are the node's **body box**. The element renders one
125+
`NODE_TITLE_HEIGHT` above `position` and the resize tracker subtracts it, so
126+
anything deriving a rect must add it back — and a collapsed node is exactly one
127+
title tall, which is zero without it.
128+
- Node ids are **graph-local**. An id resolved against one graph names a
129+
different node in another, so anything holding one across a workflow or
130+
subgraph change must pin the graph it resolved against.
131+
132+
## 7. Reach for a formal state machine when it earns it
133+
134+
Parallel regions, nested states, timed transitions, cancellation, replay — at
135+
that point a library (XState or equivalent) buys real guarantees.
136+
137+
Below it, a discriminated union plus a pure reducer is the same model without the
138+
dependency, and the migration between them is mechanical. **There is currently no
139+
XState in this repo**; introducing it is a deliberate decision, not a default.
140+
141+
Note that two genuinely parallel regions — say a wizard's progress and a
142+
long-running job's outcome — should be two small states, not one product type.
143+
144+
## Vue mapping
145+
146+
| Concern | Where it goes |
147+
| --------------------- | --------------------------------------------- |
148+
| Workflow state | `ref` / `shallowRef` in a Pinia store |
149+
| Transition | pure function, its own module, no Vue imports |
150+
| Derived UI | `computed` |
151+
| External sync | `watch` / `watchEffect` |
152+
| The workflow itself | store method (a command) |
153+
| Complex state machine | XState or equivalent |
154+
155+
**Mental model:** user or external event → command performs async work → typed
156+
event → pure state transition → reactive derived UI. Effects sit _beside_ this
157+
loop, only to synchronise with external systems.

eslint.config.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -302,6 +302,34 @@ export default defineConfig([
302302
]
303303
}
304304
},
305+
// A layout read inside a derivation runs on every recompute, and a derivation
306+
// that measures the DOM cannot be tested without one. See
307+
// docs/guidance/state-and-effects.md.
308+
//
309+
// 'warn' rather than 'error' because four pre-existing instances remain, in
310+
// BrushCursor.vue, WorkflowTabs.vue and SubgraphBreadcrumb.vue. Promote to
311+
// 'error' once those are derived from stores instead.
312+
{
313+
files: ['src/**/*.ts', 'src/**/*.vue'],
314+
ignores: ['**/*.test.ts', '**/*.spec.ts'],
315+
rules: {
316+
'no-restricted-syntax': [
317+
'warn',
318+
{
319+
selector:
320+
"CallExpression[callee.name='computed'] CallExpression[callee.property.name='getBoundingClientRect']",
321+
message:
322+
'Do not measure the DOM inside a computed - every recompute becomes a layout read. Derive from a store instead. See docs/guidance/state-and-effects.md.'
323+
},
324+
{
325+
selector:
326+
"CallExpression[callee.name='computed'] CallExpression[callee.property.name=/^(getComputedStyle|querySelector|querySelectorAll)$/]",
327+
message:
328+
'Do not inspect the DOM inside a computed. Derive from a store instead. See docs/guidance/state-and-effects.md.'
329+
}
330+
]
331+
}
332+
},
305333
{
306334
files: ['**/*.spec.ts'],
307335
ignores: ['browser_tests/tests/**/*.spec.ts', 'apps/*/e2e/**/*.spec.ts'],

0 commit comments

Comments
 (0)