diff --git a/apps/ssp/src/app/brief/application/org-template.store.ts b/apps/ssp/src/app/brief/application/org-template.store.ts index daf40a7..2834d5d 100644 --- a/apps/ssp/src/app/brief/application/org-template.store.ts +++ b/apps/ssp/src/app/brief/application/org-template.store.ts @@ -1,6 +1,6 @@ import { Injectable, computed, effect, inject, signal } from '@angular/core'; import { createStore } from '@shared/application/store'; -import { ActionState, SaveState } from '@shared/application/action-state'; +import { SaveState } from '@shared/application/action-state'; import { createDebouncedSave } from '@shared/application/debounced-save'; import { fromLoadLifecycle } from '@shared/application/remote-data'; import { UploadAdapter, uploadContentUrl } from '@shared/infrastructure/upload.adapter'; @@ -13,6 +13,7 @@ import { SubOrgSummary, } from '@brief/domain/org-template'; import { + OrgTemplateActionState, OrgTemplateMsg, OrgTemplateState, initial, @@ -47,17 +48,23 @@ export class OrgTemplateStore implements PendingSave { readonly subOrgs = signal([]); readonly selectedSubOrgId = signal(null); - private actionState = signal({ tag: 'Idle' }); - readonly busy = computed(() => this.actionState().tag === 'Busy'); + /** The one-shot action lifecycle and the publish impact-confirm gate now live on + the machine's `Loaded.action` as one four-variant union (RD-13); these stay as + plain `computed`s so the render seam (the editor organism's `input()`s, the + page template) keeps a byte-identical boolean/string API. */ + private action = computed(() => this.loaded()?.action ?? { tag: 'Idle' }); + readonly busy = computed(() => this.action().tag === 'Busy'); readonly lastError = computed(() => { - const s = this.actionState(); - return s.tag === 'Failed' ? s.error : null; + const a = this.action(); + return a.tag === 'Failed' ? a.error : null; }); + /** The publish impact-confirm gate (PRD §7h: show N affected letters before POST). + Before RD-13 this was an independent boolean, so it could be `true` at the same + time `busy` was `true` — representable and meaningless. It is now derived from + the same union `busy` reads, so the two are mutually exclusive by construction. */ + readonly pendingPublish = computed(() => this.action().tag === 'ConfirmingPublish'); readonly saveState = signal({ tag: 'Idle' }); - /** The publish impact-confirm gate (PRD §7h: show N affected letters before POST). */ - readonly pendingPublish = signal(false); - readonly remoteData = computed(() => fromLoadLifecycle(this.model())); private loaded = computed(() => { @@ -165,60 +172,64 @@ export class OrgTemplateStore implements PendingSave { this.store.dispatch({ tag: 'DraftSaved', savedDraft: draft }); } else { this.saveState.set({ tag: 'Error' }); - this.actionState.set({ tag: 'Failed', error: r.error }); + this.store.dispatch({ tag: 'ActionFailed', error: r.error }); } } // --- publish (impact-confirm) / rollback / proefbrief --- + // RD-13: `requestPublish`/`cancelPublish` are the only two commands here that do + // NOT guard on `loaded()` — as dispatches they no-op outside `Loaded` by + // construction (the reducer's own guard), so behaviour is unchanged. requestPublish() { - this.pendingPublish.set(true); + this.store.dispatch({ tag: 'PublishRequested' }); } cancelPublish() { - this.pendingPublish.set(false); + this.store.dispatch({ tag: 'PublishCancelled' }); } async confirmPublish() { const s = this.loaded(); if (!s) return; - this.pendingPublish.set(false); - this.actionState.set({ tag: 'Busy' }); + // ActionStarted overwrites `action` straight to Busy, so ConfirmingPublish and + // Busy are never simultaneously true (RD-13). + this.store.dispatch({ tag: 'ActionStarted' }); this.debouncedSave.cancel(); await this.flushSave(); // publish the saved draft — flush any pending edit first const r = await this.adapter.publish(s.subOrgId); 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' }); await this.selectSubOrg(s.subOrgId); // reload: new version, history, unsentBriefs = 0 } async rollback(version: number) { const s = this.loaded(); if (!s) return; - this.actionState.set({ tag: 'Busy' }); + this.store.dispatch({ tag: 'ActionStarted' }); this.debouncedSave.cancel(); const r = await this.adapter.rollback(s.subOrgId, version); 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.store.dispatch({ tag: 'DraftLoaded', view: r.value }); // old version copied into draft } async proefbrief() { const s = this.loaded(); if (!s) return; - this.actionState.set({ tag: 'Busy' }); + this.store.dispatch({ tag: 'ActionStarted' }); this.debouncedSave.cancel(); await this.flushSave(); // the proefbrief renders the server's draft const r = await this.adapter.proefbrief(s.subOrgId); 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); } diff --git a/apps/ssp/src/app/brief/domain/org-template.machine.spec.ts b/apps/ssp/src/app/brief/domain/org-template.machine.spec.ts index 1ceb8f8..e94519a 100644 --- a/apps/ssp/src/app/brief/domain/org-template.machine.spec.ts +++ b/apps/ssp/src/app/brief/domain/org-template.machine.spec.ts @@ -160,4 +160,44 @@ describe('org-template.machine', () => { expect(switched.upload.uploads).toHaveLength(0); expect(switched.subOrgId).toBe('cibg-vakbekwaamheid'); }); + + // --- the action lifecycle + publish impact-confirm gate, folded into one union (RD-13) --- + + it('PublishRequested moves a loaded template to ConfirmingPublish', () => { + const s = expectTag(reduce(loaded(), { tag: 'PublishRequested' }), 'Loaded'); + expect(s.action).toEqual({ tag: 'ConfirmingPublish' }); + }); + + it('PublishCancelled returns to Idle', () => { + const confirming = reduce(loaded(), { tag: 'PublishRequested' }); + const s = expectTag(reduce(confirming, { tag: 'PublishCancelled' }), 'Loaded'); + expect(s.action).toEqual({ tag: 'Idle' }); + }); + + it('ActionStarted from ConfirmingPublish goes to Busy, so confirming and busy cannot coexist', () => { + const confirming = expectTag(reduce(loaded(), { tag: 'PublishRequested' }), 'Loaded'); + expect(confirming.action.tag).toBe('ConfirmingPublish'); + const s = expectTag(reduce(confirming, { tag: 'ActionStarted' }), 'Loaded'); + expect(s.action).toEqual({ tag: 'Busy' }); + }); + + it('ActionFailed carries the error', () => { + const busy = reduce(loaded(), { tag: 'ActionStarted' }); + const s = expectTag(reduce(busy, { tag: 'ActionFailed', error: 'mislukt' }), 'Loaded'); + expect(s.action).toEqual({ tag: 'Failed', error: 'mislukt' }); + }); + + it('DraftLoaded resets a stale action error to Idle', () => { + const failed = reduce(loaded(), { tag: 'ActionFailed', error: 'mislukt' }); + const s = expectTag(reduce(failed, { tag: 'DraftLoaded', view: view() }), 'Loaded'); + expect(s.action).toEqual({ tag: 'Idle' }); + }); + + it('an action message is a no-op when the template is not loaded', () => { + expect(reduce({ tag: 'Loading' }, { tag: 'PublishRequested' })).toEqual({ tag: 'Loading' }); + expect(reduce({ tag: 'Loading' }, { tag: 'ActionStarted' })).toEqual({ tag: 'Loading' }); + expect(reduce({ tag: 'Loading' }, { tag: 'ActionFailed', error: 'x' })).toEqual({ + tag: 'Loading', + }); + }); }); diff --git a/apps/ssp/src/app/brief/domain/org-template.machine.ts b/apps/ssp/src/app/brief/domain/org-template.machine.ts index 2682973..8c63953 100644 --- a/apps/ssp/src/app/brief/domain/org-template.machine.ts +++ b/apps/ssp/src/app/brief/domain/org-template.machine.ts @@ -9,6 +9,13 @@ import { UploadMsg, UploadState, initialUpload, reduceUpload } from '@shared/dom * `dirty` tracks unsaved edits (the store debounce-saves them). The logo upload is * the composable upload sub-machine folded in, exactly like the wizards fold * `reduceUpload` — its `UploadComplete`/`UploadRemoved` also mutate `draft.logoDocumentId`. + * + * `action` (RD-13) owns the one-shot action lifecycle AND the publish impact-confirm + * gate as ONE four-variant union, replacing two independent store-level signals + * (`actionState` + `pendingPublish`). Before RD-13, `pendingPublish === true && busy + * === true` was representable and meaningless — the confirm dialog could show while a + * publish was already in flight. A single field with one tag at a time makes that + * combination unrepresentable. */ /** The org-identity text fields editable directly on the letter canvas. */ @@ -21,6 +28,17 @@ export type OrgTemplateTextField = | 'signatureRole' | 'signatureClosing'; +/** The one-shot action lifecycle (publish/rollback/proefbrief), plus the publish + impact-confirm gate, owned by the reducer instead of two independent store-level + signals (RD-13). `ConfirmingPublish` is a variant of this SAME union, so + "confirming a publish while one is already in flight" is unrepresentable — no + state can ever carry both at once. */ +export type OrgTemplateActionState = + | { tag: 'Idle' } + | { tag: 'ConfirmingPublish' } + | { tag: 'Busy' } + | { tag: 'Failed'; error: string }; + export type OrgTemplateState = | { tag: 'Loading' } | { tag: 'Failed'; reason: string } @@ -34,6 +52,7 @@ export type OrgTemplateState = dirty: boolean; /** Logo upload sub-state (single file, `org-logo` category). */ upload: UploadState; + action: OrgTemplateActionState; }; export const initial: OrgTemplateState = { tag: 'Loading' }; @@ -47,7 +66,12 @@ export type OrgTemplateMsg = /** Carries the draft that was saved: clears `dirty` only if no edit landed during the round-trip (reference-equal), so a concurrent edit keeps its pending save. */ | { tag: 'DraftSaved'; savedDraft: OrgTemplate } - | { tag: 'Upload'; msg: UploadMsg }; + | { tag: 'Upload'; msg: UploadMsg } + | { tag: 'PublishRequested' } // opens the publish impact-confirm gate + | { tag: 'PublishCancelled' } // closes it without publishing + | { tag: 'ActionStarted' } // a one-shot action (publish/rollback/proefbrief) began + | { tag: 'ActionFinished' } // it completed successfully + | { tag: 'ActionFailed'; error: string }; // it failed, carrying the message to show /** Edit the loaded draft; a no-op in any non-loaded state (illegal by construction). */ function editDraft(s: OrgTemplateState, f: (draft: OrgTemplate) => OrgTemplate): OrgTemplateState { @@ -72,6 +96,9 @@ export function reduce(s: OrgTemplateState, m: OrgTemplateMsg): OrgTemplateState // Keep the loaded logo category across sub-org switches (it's the same // `org-logo` category, loaded once); drop only any in-flight/finished uploads. upload: s.tag === 'Loaded' ? { ...s.upload, uploads: [], rejections: {} } : initialUpload, + // A fresh load clears a stale action error rather than letting it outlive + // the reload (RD-13, same as brief's RD-12). + action: { tag: 'Idle' }, }; case 'FieldEdited': return editDraft(s, (d) => ({ ...d, [m.field]: m.value })); @@ -96,6 +123,22 @@ export function reduce(s: OrgTemplateState, m: OrgTemplateMsg): OrgTemplateState } return { ...s, upload }; } + + // The action lifecycle (RD-13): a no-op unless a template is loaded, since there + // is nothing to attach the action state to otherwise. `ConfirmingPublish` and + // `Busy` are variants of one field, so ActionStarted overwriting it to `Busy` is + // what makes the two mutually exclusive by construction — not by convention. + case 'PublishRequested': + return s.tag === 'Loaded' ? { ...s, action: { tag: 'ConfirmingPublish' } } : s; + case 'PublishCancelled': + return s.tag === 'Loaded' ? { ...s, action: { tag: 'Idle' } } : s; + 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-13-org-template-action.md b/docs/project/readable-codebase/RD-13-org-template-action.md new file mode 100644 index 0000000..5ffba00 --- /dev/null +++ b/docs/project/readable-codebase/RD-13-org-template-action.md @@ -0,0 +1,166 @@ +# RD-13 — Fold org-template's action lifecycle and `pendingPublish` into one union + +Status: done +Source: PLAN.md 1b#2a and 1b#6 + +## Why + +`org-template.store.ts` repeats the pattern RD-12 removed from brief — an `actionState` +signal set imperatively from 13 places — and adds the arc's **one genuine illegal-state +pair**: + +```ts +private actionState = signal({ tag: 'Idle' }); // Idle | Busy | Failed +readonly pendingPublish = signal(false); // independent boolean +``` + +Nothing prevents `pendingPublish === true` _and_ `busy === true` at the same time. That state +is representable and meaningless: the UI would show the publish-impact confirmation while a +publish is already in flight. Two independent signals cannot express "these are mutually +exclusive"; one union can. + +## Read first + +- `docs/project/readable-codebase/RD-12-brief-action-in-machine.md` — the same migration, + already done and green for brief. Copy its shape. +- `apps/ssp/src/app/brief/application/org-template.store.ts` — `actionState` at 50, `busy` at + 51, `lastError` at 52, `saveState` at 56, `pendingPublish` at 59, and the publish flow at + 174-193 +- `apps/ssp/src/app/brief/domain/org-template.machine.ts` — the `Loaded` variant at 27-35 + (PascalCase since RD-11) +- `apps/ssp/src/app/brief/ui/org-template-editor/org-template-editor.component.ts:245,282` and + `org-template.page.ts:59` — the render seam that must not change + +## Decisions (pre-made, don't relitigate) + +1. **One four-variant union on `OrgTemplateState.Loaded`:** + + ```ts + action: { tag: 'Idle' } | { tag: 'ConfirmingPublish' } | { tag: 'Busy' } | { tag: 'Failed'; error: string } + ``` + + `ConfirmingPublish` is the fourth variant that absorbs `pendingPublish`. This is the whole + point of the ticket: after it, "confirming" and "busy" are mutually exclusive **by + construction**, not by convention. + +2. **`requestPublish` and `cancelPublish` become dispatches.** They are the only two commands + in this store that do **not** guard on `loaded()` today — they just set the boolean. As + messages (`PublishRequested`, `PublishCancelled`) they no-op outside `Loaded`, which is the + correct behaviour and means you do not add a guard that changes anything. + +3. **The other commands keep their existing `const s = this.loaded(); if (!s) return;` + guards** — `confirmPublish` (181), `rollback` (197), and the two at 158 and 211. Do not + remove them; they are stronger than brief's template gate and remain correct. + +4. **`pendingPublish`, `busy` and `lastError` all stay as store members with byte-identical + public signatures.** `pendingPublish` becomes + `computed(() => this.action().tag === 'ConfirmingPublish')` rather than a `signal`. The + render seam must not move: `org-template-editor.component.ts:282` takes + `pendingPublish = input(false)`, `:245` renders on it, `org-template.page.ts:59` passes it, + and two story args set it. **No file under `brief/ui/` may change.** + +5. **`flushSave` sets both `saveState` and `actionState`** (lines 161-169). Convert only the + `actionState` half. `saveState` must still number 5 occurrences. + +6. **`action-state.ts` still exists after this ticket.** RD-14 moves `SaveState` into + `debounced-save.ts` and deletes the file. Do not delete it here, and do not touch + `SaveState`. + +7. **Do not revisit `NO_SUBORGS`.** `org-template.store.ts:29,129` dispatches `LoadFailed` for + what is semantically `Empty`. That is a real finding and it is optional RD-34, not this + ticket. + +## Files + +- `apps/ssp/src/app/brief/domain/org-template.machine.ts` (+ `.spec.ts`) +- `apps/ssp/src/app/brief/application/org-template.store.ts` + +Not `action-state.ts` (RD-14). Not `brief.machine.ts` or `brief.store.ts` (RD-12, done). No UI +files. + +## Steps + +1. Add the four-variant `action` field to `OrgTemplateState.Loaded` and the messages to + `OrgTemplateMsg`: `PublishRequested`, `PublishCancelled`, `ActionStarted`, + `ActionFinished`, `ActionFailed`. +2. Handle them in `reduce`, each a no-op outside `Loaded`. `DraftLoaded` resets `action` to + `Idle`, matching RD-12's deliberate reset. +3. Add reducer spec cases (see Acceptance), including the mutual-exclusion case. +4. Replace the 13 `actionState.set(...)` and 4 `pendingPublish.set(...)` sites with dispatches. +5. Re-point `busy`, `lastError` and `pendingPublish` at `Loaded.action`, keeping signatures + identical. +6. Run `npm run gen:behaviour-spec` — new spec titles otherwise fail the drift check. +7. Update this ticket's `Status:` to `done` and the README's RD-13 row to `done`. +8. Commit all of it together. + +## Acceptance criteria + +Measured baselines, dry-run before handover. Commands are scoped to **this ticket's two +files**, never to the `brief/` directory — `action-state.ts` and other files legitimately +still reference these names. + +```bash +S=apps/ssp/src/app/brief/application/org-template.store.ts +M=apps/ssp/src/app/brief/domain/org-template.machine.ts + +git grep -c "actionState" -- $S # was 13 -> MUST return nothing +git grep -cw "ActionState" -- $S # MUST return nothing (word-anchored: a new + # OrgTemplateActionState would contain the old name) +git grep -c "pendingPublish" -- $S # was 4 (a signal) -> now exactly 1 (a computed) +git grep -c "saveState" -- $S # unchanged: still 5 +git grep -c "readonly busy\|readonly lastError" -- $S # unchanged: still 2 +git grep -c "ConfirmingPublish" -- $M # >= 1 +``` + +The render seam did not move: + +```bash +git diff --name-only 8e5f48c | grep -c "brief/ui/" || true # MUST be 0 +``` + +New reducer cases, the third being the point of the ticket: + +``` +- PublishRequested moves a loaded template to ConfirmingPublish +- PublishCancelled returns to Idle +- ActionStarted from ConfirmingPublish goes to Busy, so confirming and busy cannot coexist +- ActionFailed carries the error +- DraftLoaded resets a stale action error to Idle +- an action message is a no-op when the template 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 run the full gate anyway, pass `timeout: 600000` on the Bash call — it takes about 8 +minutes, and the harness backgrounds anything over 120s, which would end your turn with the +work uncommitted. + +If `dotnet test` fails with `SQLite Error 1: 'no such table: …'`, that is the stale +`bigregister.db` artifact documented in this README's Troubleshooting section. It is unrelated +to your change. + +## Out of scope + +- `SaveState` and deleting `action-state.ts` — RD-14. +- `NO_SUBORGS` becoming `Empty` — optional RD-34 (decision 7). +- Any file under `brief/ui/`, and the four `busy = input(...)` components. +- `brief.machine.ts` / `brief.store.ts` — RD-12 already did those. + +## Risks + +- **The mutual-exclusion case is the acceptance test that matters.** If your reducer lets + `ConfirmingPublish` and `Busy` coexist in any way, the ticket has not achieved its purpose + even if every grep passes. +- **`pendingPublish` changes from a `signal` to a `computed`.** Anything that _writes_ it must + become a dispatch. A leftover `.set()` call will not compile, which is the desired outcome. +- **Keep `busy`/`lastError`/`pendingPublish` signatures byte-identical.** All three are read + from a page template; renaming or re-typing one turns a pure refactor into a UI change and + breaks two stories. +- **`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 d0d3d3c..80560a4 100644 --- a/docs/project/readable-codebase/README.md +++ b/docs/project/readable-codebase/README.md @@ -107,7 +107,7 @@ two. Note that RD-15 exists because 22 abandoned agent worktrees are still on di | 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 | | done | -| RD-13 | Same for org-template, folding `pendingPublish` in | 12 | | todo | +| RD-13 | Same for org-template, folding `pendingPublish` in | 12 | | done | | 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 | | RD-16 | `parseDashboardView` returns `BigProfile`; delete `DashboardView` | 01 | | todo | diff --git a/libs/shared/docs/behaviour-spec.mdx b/libs/shared/docs/behaviour-spec.mdx index d647d16..11698eb 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. 524 frontend behaviours across +**is** the suite, reshaped for a business reader. 530 frontend behaviours across 9 contexts; 261 backend behaviours across 42 test classes. @@ -308,6 +308,12 @@ classes. - a completed logo upload sets logoDocumentId + dirty - removing the logo clears logoDocumentId + dirty - DraftLoaded (sub-org switch) keeps the loaded logo category, drops uploads +- PublishRequested moves a loaded template to ConfirmingPublish +- PublishCancelled returns to Idle +- ActionStarted from ConfirmingPublish goes to Busy, so confirming and busy cannot coexist +- ActionFailed carries the error +- DraftLoaded resets a stale action error to Idle +- an action message is a no-op when the template is not loaded #### parseOrgTemplateAdminView