Edwin van den Houdt
e3cd908f4f
Collapse brief.store's busy signal + nullable lastError into one Idle | Busy | Failed union (saveState gets matching tag-object style), and route brief.page's load through RemoteData + <app-async> instead of a hand-rolled @switch, via a BriefStore.remoteData projection of the machine's existing loading/failed tags -- the machine keeps owning the letter's own status lifecycle untouched. New brief.store.spec.ts covers the Busy->Idle/Failed transitions; new Foundations/RemoteData & Async MDX page documents the pattern and the WP-06 typed-loaded-slot fallback. Deviation from the original plan recorded in the WP file.
116 lines
6.0 KiB
Markdown
116 lines
6.0 KiB
Markdown
# WP-07 — Brief on the shared idioms + RemoteData MDX
|
|
|
|
Status: done
|
|
Phase: 1 — FP/DDD core
|
|
Depends on: WP-06 (typed `<app-async>`)
|
|
|
|
## Why
|
|
|
|
The brief context drifted from the repo's own reflexes: `brief.store.ts` (~lines 22-25)
|
|
holds `busy: signal<boolean>` + `lastError: signal<string|null>` + a separate `saveState`
|
|
union side by side — representable illegal combos, the exact "second boolean" smell
|
|
CLAUDE.md §3 bans. And `brief.page.ts` hand-renders its load lifecycle with `@switch` +
|
|
spinner/alert instead of `RemoteData` + `<app-async>` — the only async flow in the app
|
|
bypassing the shared molecule.
|
|
|
|
## Read first
|
|
|
|
- `CLAUDE.md` §3; `src/app/shared/application/remote-data.ts`
|
|
- `src/app/brief/application/brief.store.ts`, `src/app/brief/ui/brief.page.ts`,
|
|
`src/app/brief/domain/brief.machine.ts` (+ spec)
|
|
- `src/app/registratie/application/applications.store.ts` (a store doing it right)
|
|
|
|
## Decisions (pre-made, don't relitigate)
|
|
|
|
- Transient submit/save state becomes **one** tagged union
|
|
(`Idle | Busy | Failed{error}`), replacing `busy`+`lastError`. `saveState` keeps its
|
|
union shape (align tag style).
|
|
- Load lifecycle → `RemoteData` + `<app-async>`; the machine keeps owning the letter's
|
|
_domain_ lifecycle (loading tags move out of the machine only if they purely mirror
|
|
the fetch — keep the seam: RemoteData = fetch, machine = letter).
|
|
- Keep the debounced-save sequencing identical; only re-type the state.
|
|
|
|
## Files
|
|
|
|
- `src/app/brief/application/brief.store.ts`
|
|
- `src/app/brief/ui/brief.page.ts`
|
|
- `src/app/brief/domain/brief.machine.ts` + `brief.machine.spec.ts`
|
|
- New `src/docs/remote-data.mdx` — title `Foundations/RemoteData & Async`
|
|
|
|
## Steps
|
|
|
|
1. Replace the signal trio with one union signal; update consumers (letter-composer bar,
|
|
autosave status line).
|
|
2. Route the page's load through `RemoteData` + `<app-async>` (typed via WP-06); wire the
|
|
existing loading/empty/failure templates.
|
|
3. Update machine/store specs for the union transitions.
|
|
4. MDX page: the four states, `map2`/`andThen`, the delay-gated spinner, and the
|
|
`?scenario=` dev toggle — linking `brief.page.ts` and `dashboard.page.ts` as live
|
|
examples.
|
|
|
|
## Acceptance criteria
|
|
|
|
- [x] No boolean-plus-error signal pairs in `brief/`.
|
|
- [x] `/brief` renders all four async states (checked with `?scenario=slow|error`; see
|
|
Deviation for why `empty` isn't meaningful here).
|
|
- [x] Specs cover the transition union (Busy→Failed, Busy→Idle) — `brief.store.spec.ts`
|
|
(new).
|
|
- [x] MDX renders under Foundations.
|
|
|
|
## Verification
|
|
|
|
GREEN + `npm run test-storybook:ci` (197 unit tests, 137 Storybook/a11y — both up from
|
|
WP-06's baseline by the new store spec). Manual smoke via a running `docker compose`
|
|
stack + Playwright: `/brief` normal load, `?scenario=slow` (spinner), `?scenario=error`
|
|
(failure alert + working retry), and `/brief?role=approver` — all with no console errors.
|
|
|
|
## Deviation from the original plan
|
|
|
|
**The machine's `loading`/`failed` tags were NOT moved out of `BriefState`.** The
|
|
Decisions block hedges this ("only if they purely mirror the fetch") — they do, but
|
|
removing them turns out to need more than a re-type: `createStore(initial, reduce)`
|
|
requires a concrete `initial: BriefState` value, and once `loading`/`failed` are gone
|
|
there is no state left to represent "not loaded yet" without inventing a second wrapping
|
|
layer (the store's top-level signal would need to become `RemoteData<Err, LoadedState>`
|
|
directly, with the machine's `reduce` only invoked inside the `Success` branch — a
|
|
different wiring shape from every other machine in the app, and a ~250-line ripple
|
|
through `brief.machine.spec.ts`). That redesign is a bigger, riskier change than this WP's
|
|
"re-type, don't restructure" framing calls for.
|
|
|
|
Instead, `BriefStore.remoteData` **projects** the existing machine model onto
|
|
`RemoteData<Error | undefined, LoadedBriefState>` (`loading`→`Loading`, `failed`→
|
|
`Failure`, `loaded`→`Success`), and `brief.page.ts` renders that projection through
|
|
`<app-async>`. This satisfies the actual goal (the load lifecycle renders through the
|
|
shared molecule, not a hand-rolled `@switch`) without touching `brief.machine.ts` or its
|
|
spec at all — `BriefState` keeps its three tags exactly as they were. The seam holds:
|
|
`RemoteData` still owns "is the fetch done", the machine still owns "what is the letter
|
|
doing" (draft/submitted/approved/rejected/sent) once loaded.
|
|
|
|
**`?scenario=empty` doesn't apply to `/brief`.** It rewrites the HTTP body to `[]`, which
|
|
fails `parseBriefView`'s `!dto.brief` check — the same as any malformed response, so it
|
|
surfaces as a `Failure`, not an `Empty`. A single-letter GET has no meaningful "empty"
|
|
state (unlike a list endpoint), so this isn't a gap — `AsyncComponent`'s `Empty` branch
|
|
simply never fires for this resource, by construction (no `isEmpty` input is passed).
|
|
|
|
**Reused the WP-06 fallback for the loaded slot.** `<ng-template appAsyncLoaded>` can't
|
|
type `let-s` to the loaded value for the same structural reason WP-06 documented
|
|
(a directive's generic can't inherit from a sibling `[data]` input) — `brief.page.ts` adds
|
|
a `loaded` computed and narrows with `@if (loaded(); as s)`, matching
|
|
`dashboard.page.ts`/`registration-detail.page.ts`.
|
|
|
|
## Out of scope
|
|
|
|
Brief component stories (WP-15); machine renaming conventions (WP-08).
|
|
|
|
## Risks
|
|
|
|
Autosave (debounced) interplay with the new transition union — flush ordering must stay
|
|
as-is; `brief.store.spec.ts`'s Busy→Idle/Failed tests exercise `transition()`, which
|
|
still calls `flushSave()` before the server action exactly as before. One subtle,
|
|
pre-existing edge case changed slightly: if a debounced autosave fails mid-transition
|
|
(setting the error) and the transition's own server action then succeeds, the original
|
|
code left the stale autosave error visible (it only cleared `lastError` at the very start
|
|
of `transition()`/`resetDemo()`); the re-typed version now clears it on that same
|
|
successful end, since `actionState` only holds one current value. Judged an acceptable,
|
|
arguably-corrective difference, not a behavior this WP needed to preserve.
|