From 02d41536df4dde4a0437b627140869cb7c10f62f Mon Sep 17 00:00:00 2001 From: Edwin van den Houdt Date: Fri, 4 Sep 2026 18:27:09 +0200 Subject: [PATCH] refactor(brief): move the action lifecycle into the machine (RD-12) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The action lifecycle (Idle | Busy | Failed) lived in an imperative store-level signal, set from ten call sites outside the reducer. The reducer could not enforce which action transitions are legal. Add `action` to `BriefState.Loaded`, driven by three new messages (ActionStarted, ActionFinished, ActionFailed) and handled in `reduce`. Replace every `actionState.set(...)` call in `brief.store.ts` with the matching `dispatch`. `BriefLoaded` resets `action` to Idle, so a fresh load clears a stale action error instead of letting it outlive the reload. `busy` and `lastError` stay as `computed`s on the store with a byte-identical public signature — they are the render seam for four components and two page templates, and the union belongs in the machine, not the components. `revealBigNummer` still sets only `Failed`, never `Busy` — an existing asymmetry, not changed here. `SaveState`, `org-template.store.ts`, and `pendingPublish` are out of scope (RD-13, RD-14). Co-Authored-By: Claude Sonnet 5 --- .../src/app/brief/application/brief.store.ts | 40 +++-- .../app/brief/domain/brief.machine.spec.ts | 33 ++++ .../ssp/src/app/brief/domain/brief.machine.ts | 22 ++- .../RD-12-brief-action-in-machine.md | 163 ++++++++++++++++++ docs/project/readable-codebase/README.md | 2 +- libs/shared/docs/behaviour-spec.mdx | 7 +- 6 files changed, 248 insertions(+), 19 deletions(-) create mode 100644 docs/project/readable-codebase/RD-12-brief-action-in-machine.md diff --git a/apps/ssp/src/app/brief/application/brief.store.ts b/apps/ssp/src/app/brief/application/brief.store.ts index 15b166a..83c04bf 100644 --- a/apps/ssp/src/app/brief/application/brief.store.ts +++ b/apps/ssp/src/app/brief/application/brief.store.ts @@ -1,7 +1,7 @@ import { Injectable, computed, inject, signal } from '@angular/core'; import { Result } from '@shared/kernel/fp'; import { createStore } from '@shared/application/store'; -import { ActionState, SaveState } from '@shared/application/action-state'; +import { SaveState } from '@shared/application/action-state'; import { createHistory } from '@shared/application/history'; import { createDebouncedSave } from '@shared/application/debounced-save'; import { fromLoadLifecycle } from '@shared/application/remote-data'; @@ -41,11 +41,16 @@ export class BriefStore implements PendingSave { readonly model = this.store.model; - private actionState = signal({ tag: 'Idle' }); - readonly busy = computed(() => this.actionState().tag === 'Busy'); + /** The one-shot action lifecycle now lives on the machine's `Loaded.action` (RD-12); + these stay as plain `computed`s so the render seam (four `busy = input(...)` + components, two page templates) keeps a byte-identical boolean/string API. */ + readonly busy = computed(() => { + const s = this.model(); + return s.tag === 'Loaded' && s.action.tag === 'Busy'; + }); readonly lastError = computed(() => { - const s = this.actionState(); - return s.tag === 'Failed' ? s.error : null; + const s = this.model(); + return s.tag === 'Loaded' && s.action.tag === 'Failed' ? s.action.error : null; }); /** Surfaced autosave state for the indicator + aria-live region. */ @@ -212,7 +217,9 @@ export class BriefStore implements PendingSave { if (r.ok) { this.saveState.set({ tag: 'Saved' }); } else { - this.actionState.set({ tag: 'Failed', error: r.error }); + // The autosave failure legitimately surfaces in two places: the small save + // indicator below (kept as-is) and the action error line (RD-12). + this.store.dispatch({ tag: 'ActionFailed', error: r.error }); this.saveState.set({ tag: 'Error' }); } } @@ -224,19 +231,19 @@ export class BriefStore implements PendingSave { /** Demo "start over": recreate the brief server-side and load the fresh view. */ async resetDemo() { - this.actionState.set({ tag: 'Busy' }); + this.store.dispatch({ tag: 'ActionStarted' }); this.debouncedSave.cancel(); const r = await this.adapter.reset(); this.saveState.set({ tag: 'Idle' }); if (r.ok) { - this.actionState.set({ tag: 'Idle' }); + this.store.dispatch({ tag: 'ActionFinished' }); this.orgTemplate.set(r.value.orgTemplate); this.caseContext.set(r.value.caseContext); this.history.clear(); this.rejectionSnapshot.set(null); this.store.dispatch({ tag: 'BriefLoaded', ...r.value }); } else { - this.actionState.set({ tag: 'Failed', error: r.error }); + this.store.dispatch({ tag: 'ActionFailed', error: r.error }); } } @@ -249,13 +256,13 @@ export class BriefStore implements PendingSave { letter in a new tab via `BLOB_PRESENTER.open` — see its doc comment for why the object URL is never revoked. */ async previewLetter() { - this.actionState.set({ tag: 'Busy' }); + this.store.dispatch({ tag: 'ActionStarted' }); const r = await this.previewAdapter.preview(); if (!r.ok) { - this.actionState.set({ tag: 'Failed', error: r.error }); + this.store.dispatch({ tag: 'ActionFailed', error: r.error }); return; } - this.actionState.set({ tag: 'Idle' }); + this.store.dispatch({ tag: 'ActionFinished' }); this.blobPresenter.open(r.value); } @@ -269,7 +276,8 @@ export class BriefStore implements PendingSave { async revealBigNummer() { const r = await this.revealAdapter.reveal(true); if (!r.ok) { - this.actionState.set({ tag: 'Failed', error: r.error }); + // Never sets Busy — an existing asymmetry (RD-12), not fixed here. + this.store.dispatch({ tag: 'ActionFailed', error: r.error }); return; } this.caseContext.update((c) => (c ? { ...c, bigNummer: r.value } : c)); @@ -278,15 +286,15 @@ export class BriefStore implements PendingSave { // A transition: flush any pending save, call the server (authoritative), then mirror // the returned status through the pure reducer's guarded transition. private async transition(action: () => Promise>) { - this.actionState.set({ tag: 'Busy' }); + this.store.dispatch({ tag: 'ActionStarted' }); this.debouncedSave.cancel(); await this.flushSave(); const r = await action(); if (!r.ok) { - this.actionState.set({ tag: 'Failed', error: r.error }); + this.store.dispatch({ tag: 'ActionFailed', error: r.error }); return; } - this.actionState.set({ tag: 'Idle' }); + this.store.dispatch({ tag: 'ActionFinished' }); this.applyServerStatus(r.value); } diff --git a/apps/ssp/src/app/brief/domain/brief.machine.spec.ts b/apps/ssp/src/app/brief/domain/brief.machine.spec.ts index 1593c83..230e74a 100644 --- a/apps/ssp/src/app/brief/domain/brief.machine.spec.ts +++ b/apps/ssp/src/app/brief/domain/brief.machine.spec.ts @@ -265,6 +265,39 @@ describe('brief.machine reduce', () => { expect(sent.tag === 'Loaded' && sent.brief.status).toEqual({ tag: 'sent', sentAt: 't3' }); }); + // --- RD-12: the action lifecycle lives on `Loaded.action`, driven by three msgs --- + + it('ActionStarted moves a loaded brief to Busy', () => { + const s = reduce(loaded(), { tag: 'ActionStarted' }); + expect(s.tag === 'Loaded' && s.action).toEqual({ tag: 'Busy' }); + }); + + it('ActionFailed carries the error', () => { + const s = reduce(loaded(), { tag: 'ActionFailed', error: 'niet gelukt' }); + expect(s.tag === 'Loaded' && s.action).toEqual({ tag: 'Failed', error: 'niet gelukt' }); + }); + + it('ActionFinished returns to Idle', () => { + const busy = reduce(loaded(), { tag: 'ActionStarted' }); + const s = reduce(busy, { tag: 'ActionFinished' }); + expect(s.tag === 'Loaded' && s.action).toEqual({ tag: 'Idle' }); + }); + + it('BriefLoaded resets a stale action error to Idle', () => { + const failed = reduce(loaded(), { tag: 'ActionFailed', error: 'niet gelukt' }); + const reloaded = reduce(failed, { + tag: 'BriefLoaded', + brief: briefWith({ tag: 'draft' }), + availablePassages: lib, + decisions, + }); + expect(reloaded.tag === 'Loaded' && reloaded.action).toEqual({ tag: 'Idle' }); + }); + + it('an action message is a no-op when the brief is not loaded', () => { + expect(reduce(initialLoading(), { tag: 'ActionStarted' })).toEqual(initialLoading()); + }); + it('a status transition replaces decisions with the fresh server value', () => { const submitted = loaded({ tag: 'submitted', submittedBy: 'u1', submittedAt: 't' }); const staleApprover: BriefDecisions = { diff --git a/apps/ssp/src/app/brief/domain/brief.machine.ts b/apps/ssp/src/app/brief/domain/brief.machine.ts index 1834265..cf824da 100644 --- a/apps/ssp/src/app/brief/domain/brief.machine.ts +++ b/apps/ssp/src/app/brief/domain/brief.machine.ts @@ -36,6 +36,10 @@ import { passagesForBesluit } from './besluit'; * structurally impossible (a pasted `{{…}}` is caught by the linter as `malformed`). */ +/** The one-shot action lifecycle (submit/approve/reject/send/preview/reveal/reset), + owned by the reducer instead of an imperative store-level signal (RD-12). */ +export type BriefActionState = { tag: 'Idle' } | { tag: 'Busy' } | { tag: 'Failed'; error: string }; + export type BriefState = | { tag: 'Loading' } | { @@ -43,6 +47,7 @@ export type BriefState = brief: Brief; availablePassages: readonly LibraryPassage[]; decisions: BriefDecisions; + action: BriefActionState; } | { tag: 'Failed'; reason: string }; @@ -65,7 +70,10 @@ export type BriefMsg = | { tag: 'Approved'; by: string; at: string; decisions: BriefDecisions } // submitted → approved | { tag: 'Rejected'; by: string; at: string; comments: string; decisions: BriefDecisions } // submitted → rejected | { tag: 'Sent'; at: string; decisions: BriefDecisions } // approved → sent - | { tag: 'Seed'; state: BriefState }; + | { tag: 'Seed'; state: BriefState } + | { tag: 'ActionStarted' } // a one-shot action (submit/approve/preview/…) began + | { tag: 'ActionFinished' } // it completed successfully + | { tag: 'ActionFailed'; error: string }; // it failed, carrying the message to show /** Edits are allowed only in these statuses; editing a rejected letter reopens it. */ function isEditable(status: BriefStatus): boolean { @@ -193,6 +201,9 @@ export function reduce(s: BriefState, m: BriefMsg): BriefState { brief: m.brief, availablePassages: m.availablePassages, decisions: m.decisions, + // A fresh load clears a stale action error rather than letting it outlive + // the reload (RD-12, decision 4). + action: { tag: 'Idle' }, }; case 'BriefLoadFailed': return { tag: 'Failed', reason: m.reason }; @@ -260,6 +271,15 @@ export function reduce(s: BriefState, m: BriefMsg): BriefState { case 'Sent': return transition(s, 'approved', () => ({ tag: 'sent', sentAt: m.at }), m.decisions); + // The action lifecycle (RD-12): a no-op unless a brief is loaded, since there is + // nothing to attach the action state to otherwise. + case 'ActionStarted': + return s.tag === 'Loaded' ? { ...s, action: { tag: 'Busy' } } : s; + case 'ActionFinished': + return s.tag === 'Loaded' ? { ...s, action: { tag: 'Idle' } } : s; + case 'ActionFailed': + return s.tag === 'Loaded' ? { ...s, action: { tag: 'Failed', error: m.error } } : s; + default: return assertNever(m); } diff --git a/docs/project/readable-codebase/RD-12-brief-action-in-machine.md b/docs/project/readable-codebase/RD-12-brief-action-in-machine.md new file mode 100644 index 0000000..bc2ff2f --- /dev/null +++ b/docs/project/readable-codebase/RD-12-brief-action-in-machine.md @@ -0,0 +1,163 @@ +# RD-12 — Move the brief's action lifecycle into the machine + +Status: done +Source: PLAN.md 1b#2a + +## Why + +`brief.store.ts` keeps the action lifecycle in a store-level signal, set imperatively from +about ten places entirely outside the reducer: + +```ts +private actionState = signal({ tag: 'Idle' }); +readonly busy = computed(() => this.actionState().tag === 'Busy'); +readonly lastError = computed(() => { … }); +``` + +So the machine cannot enforce which action transitions are legal, and `ActionState` has two +producers and **zero** consumers that keep the union — both stores immediately collapse it +back to a boolean plus a nullable string, the exact shape its own doc comment says it exists +to remove. + +Move it into the machine's `Loaded` state and the reducer owns it, like every other state +change in this house. + +## Read first + +- `libs/shared/src/application/action-state.ts` — 9 lines, both types. **Only `ActionState` + is in scope**; `SaveState` is RD-14's. +- `apps/ssp/src/app/brief/application/brief.store.ts` — `actionState` at 44, `busy` at 45, + `lastError` at 46, and the setter sites in `flushSave` (207), `resetDemo` (226), + `previewLetter` (251), `revealBigNummer` (269) and `transition` (280) +- `apps/ssp/src/app/brief/domain/brief.machine.ts` — the `Loaded` variant (PascalCase since + RD-11) and `reduce` +- `apps/ssp/src/app/brief/ui/brief.page.ts:50-126` — the `` wrapper. Decision 2 + depends on it. + +## Decisions (pre-made, don't relitigate) + +1. **`action` becomes a field on `BriefState.Loaded`**, carrying the same three cases + (`Idle | Busy | Failed{error}`), driven by three new messages — `ActionStarted`, + `ActionFinished`, `ActionFailed` — handled in `reduce`. The imperative + `actionState.set(...)` calls become `dispatch(...)`. + +2. **This is safe because every action trigger is template-gated, and that was verified, not + assumed.** `brief.page.ts:55` opens ``, which renders only when + `remoteData()` is `Success` — i.e. when the machine is `Loaded`. All three entry points sit + inside it: the reset button (`:83`), `previewLetter` (`:101`, `:120`) and `revealBigNummer` + (`:102`). `transition` backs submit/approve/reject/send, reachable only from the same + surface, and `flushSave` runs from the debounced autosave, which only fires while editing a + loaded brief. + + **If you add an action trigger outside that slot, this design breaks.** Do not add one. + +3. **`busy` and `lastError` stay as `computed`s on the store.** They are the render seam, not + a second encoding: four components take `busy = input(...)` — `behandel-scherm`, + `letter-composer`, `org-template-editor`, `rejection-comments` — and two pages read + `store.busy()`/`store.lastError()` directly. A boolean is right at that boundary; the union + is right in the machine. **Do not push the union down into the components** — it would churn + four components and their stories for no gain. + +4. **`BriefLoaded` resetting `action` to `Idle` is intended.** A fresh load clears a stale + action error, which is a small behaviour _improvement_: today a failed action's message can + outlive a reload. Let the reducer do it, and say so in a comment. + +5. **`flushSave` sets both `saveState` and `actionState` today. Keep both.** The autosave + failure legitimately surfaces in two places — the small save indicator and the action error + line. Only the `actionState` half becomes a dispatch here; leave `saveState` exactly as it + is. + +6. **Do not touch `org-template.store.ts`, `pendingPublish`, or `SaveState`.** RD-13 folds + org-template (including `pendingPublish`, the one genuine illegal-state pair), and RD-14 + moves `SaveState` and deletes `action-state.ts`. `action-state.ts` therefore still exists + after this ticket, exporting only `SaveState` plus an `ActionState` that brief no longer + imports. + +## Files + +- `apps/ssp/src/app/brief/domain/brief.machine.ts` (+ `.spec.ts`) +- `apps/ssp/src/app/brief/application/brief.store.ts` (+ `.spec.ts`) + +Not `action-state.ts` (RD-14 deletes it). Not `org-template.store.ts` (RD-13). No UI files. + +## Steps + +1. Add `action` to `BriefState.Loaded` and the three messages to `BriefMsg`; handle them in + `reduce`, including the `BriefLoaded` reset from decision 4. +2. Add reducer spec cases (see Acceptance). +3. Replace each `actionState.set(...)` in `brief.store.ts` with the matching `dispatch`. +4. Re-point `busy` and `lastError` at the machine's `Loaded.action`, keeping their public + signatures identical so no UI file changes. +5. Run `npm run gen:behaviour-spec` — new `it()` titles otherwise fail the drift check. +6. Update this ticket's `Status:` to `done` and the README's RD-12 row to `done`. +7. Commit all of it together. + +## Acceptance criteria + +Dry-run against the tree before handover, with the measured baselines: `brief.store.ts` has +**14** `actionState` occurrences and `brief.machine.ts` has **0** action messages; both must +invert. `saveState` is **5** and must stay 5. The `busy`/`lastError` declarations are **2** and +must stay 2. + +```bash +B=apps/ssp/src/app/brief +git grep -c "actionState" -- $B/application/brief.store.ts # MUST return nothing +git grep -n "ActionState" -- $B # MUST return nothing +git grep -c "ActionStarted\|ActionFinished\|ActionFailed" -- $B/domain/brief.machine.ts # >= 3 +``` + +The render seam is unchanged, so no UI file was touched: + +```bash +git diff --name-only HEAD | grep -c "brief/ui/" || true # MUST be 0 +git grep -c "readonly busy\|readonly lastError" -- $B/application/brief.store.ts # still 2 +``` + +`SaveState` and org-template are untouched (decision 6): + +```bash +git diff --name-only HEAD | grep -cE "action-state|org-template" || true # MUST be 0 +git grep -c "saveState" -- $B/application/brief.store.ts # unchanged: still 5 +``` + +New reducer cases: + +``` +- ActionStarted moves a loaded brief to Busy +- ActionFailed carries the error +- ActionFinished returns to Idle +- BriefLoaded resets a stale action error to Idle +- an action message is a no-op when the brief is not loaded +``` + +```bash +npm run ci # exits 0 +``` + +## Verification + +`npm run ci`. No story, no `.mdx`, no `libs/shared/src/ui/**`, so `--full` is not required. + +If you do run the full gate, pass `timeout: 600000` on the Bash call — it takes about 8 +minutes and the harness backgrounds anything longer than 120s, which would end your turn with +the work uncommitted. + +## Out of scope + +- `org-template.store.ts` and `pendingPublish` — RD-13. +- `SaveState`, and deleting `action-state.ts` — RD-14. +- The four `busy = input(...)` components and their stories (decision 3). +- The `NO_SUBORGS`/`NO_TABLES`-should-be-`Empty` finding — optional RD-34. + +## Risks + +- **Decision 2 is the load-bearing assumption.** It holds today because of one + ``. Re-read `brief.page.ts:50-126` and confirm before you start; + if any trigger has moved outside that slot since this ticket was written, stop and say so + rather than adding a guard that changes behaviour. +- **`revealBigNummer` sets only `Failed`, never `Busy`.** Do not "fix" that asymmetry here — + it is existing behaviour, and changing it is a separate decision. +- **Keep `busy`/`lastError` signatures byte-identical.** They are read from two page templates; + a renamed or re-typed member turns a pure refactor into a UI change. +- **`behaviour-spec.mdx` drift** from the new spec titles. Run `gen:behaviour-spec` in the same + commit. diff --git a/docs/project/readable-codebase/README.md b/docs/project/readable-codebase/README.md index 8aa4961..2b2e9f0 100644 --- a/docs/project/readable-codebase/README.md +++ b/docs/project/readable-codebase/README.md @@ -106,7 +106,7 @@ two. Note that RD-15 exists because 22 abandoned agent worktrees are still on di | RD-09 | Teach the effect map: ARCHITECTURE §2d + fp-tea (2 docs, no generator) | 08 | | done | | RD-10 | `WizardStatus` to a payload-carrying `WizardPhase` | 08 | yes | done | | RD-11 | Fold the lifecycle projection into `remote-data.ts`; PascalCase 3 machines | 01 | | done | -| RD-12 | `ActionState` becomes `action` on `BriefState.Loaded` | 11 | | todo | +| RD-12 | `ActionState` becomes `action` on `BriefState.Loaded` | 11 | | done | | RD-13 | Same for org-template, folding `pendingPublish` in | 12 | | todo | | RD-14 | Move `SaveState` to `debounced-save.ts`; delete `action-state.ts` | 13 | | todo | | RD-15 | Remove 22 abandoned agent worktrees (4.7 GB) | 01 | | todo | diff --git a/libs/shared/docs/behaviour-spec.mdx b/libs/shared/docs/behaviour-spec.mdx index 9b596e3..d647d16 100644 --- a/libs/shared/docs/behaviour-spec.mdx +++ b/libs/shared/docs/behaviour-spec.mdx @@ -20,7 +20,7 @@ tested where._ Every bullet below is a real test name from the suite — an `it()` title (frontend) or a test method name (backend), read as a sentence. Nothing here is hand-written prose: this page -**is** the suite, reshaped for a business reader. 519 frontend behaviours across +**is** the suite, reshaped for a business reader. 524 frontend behaviours across 9 contexts; 261 backend behaviours across 42 test classes. @@ -260,6 +260,11 @@ classes. - approve fires only from submitted - reject fires from submitted, carrying comments - send fires only from approved +- ActionStarted moves a loaded brief to Busy +- ActionFailed carries the error +- ActionFinished returns to Idle +- BriefLoaded resets a stale action error to Idle +- an action message is a no-op when the brief is not loaded - a status transition replaces decisions with the fresh server value #### diffBlocks