Merge RB-22 — tolerate a 404 on GET /brief with a one-shot reset
CQ-007 expand half. BriefStore.load() treats a 404 as 'no brief yet' and calls the existing reset() command once. load()'s error channel becomes the BriefLoadFailure union, because runResult folds the HTTP status away and the store needs it. Today's backend never 404s, so the branch is a no-op until RB-23 lands the contract half. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> # Conflicts: # docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md # libs/shared/docs/behaviour-spec.mdx
This commit is contained in:
@@ -0,0 +1,139 @@
|
||||
# RB-22 — `BriefStore.load()` tolerates a 404, calling `reset()` exactly once
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `04-cqrs-light.md` CQ-007 ·
|
||||
`99-backlog.md` RB-22, "Tickets that were rejected and split" · `implementation/rb-17.md`
|
||||
(the `runResult`/`runSubmit` seam this store already sits on)
|
||||
|
||||
This is the **expand** half of an expand/contract pair. RB-23 (backend: `GET /brief` 404s
|
||||
when absent; `BriefStore.GetOrCreate` splits into `Get` + `ResetAndCreate`) ships after this
|
||||
ticket, in a later merge. Today's backend never 404s `GET /brief`, so this ticket's new
|
||||
branch is dead code in the running app — provably backend-frontend-safe by construction.
|
||||
|
||||
## What was wrong
|
||||
|
||||
CQ-007 flags `GET /brief` (`Program.cs:603` → `BriefStore.GetOrCreate`,
|
||||
`Data/BriefStore.cs:50`) as the one endpoint in the backend where a GET performs a
|
||||
persisted write, breaking the read/write split every other endpoint respects. The fix is
|
||||
split across both sides of the seam because the FE must be ready to receive a 404 before
|
||||
the backend can safely start sending one. This ticket is the FE half: `BriefStore.load()`
|
||||
(`apps/ssp/src/app/brief/application/brief.store.ts`) had no notion of "no brief exists
|
||||
yet" — every adapter failure, 404 included, dispatched `BriefLoadFailed` and showed the
|
||||
generic error banner. `BriefAdapter.load()` (`brief.adapter.ts`) also had no way to tell
|
||||
the store a failure was specifically an HTTP 404: it folded every failure through the
|
||||
shared `runResult` helper (RB-17), which keeps only a human-readable string and throws
|
||||
away the HTTP status.
|
||||
|
||||
The ticket read as filed against the current code: `BriefStore.load()` is exactly where
|
||||
CQ-007 says it is, `BriefAdapter.load()` is exactly the read `runResult` call RB-17 pointed
|
||||
at it, and nothing about either file was factually wrong. Nothing to flag here.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| -------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
|
||||
| `apps/ssp/src/app/brief/infrastructure/brief.adapter.ts` | `load()`'s error channel becomes `BriefLoadFailure` (`{tag:'notFound'} \| {tag:'error', reason:string}`) instead of a plain `string`. `load()` no longer routes through the shared `runResult` — it does its own try/catch so it can read the thrown value's HTTP `status` before folding it away, via the new local `isHttpNotFound` predicate. Every other method (`save`/`submit`/`approve`/`reject`/`send`/`reset`) is untouched, still on `runSubmit`. |
|
||||
| `apps/ssp/src/app/brief/application/brief.store.ts` | `load()` branches on `BriefLoadFailure`: `notFound` (and not already recovered) calls the existing `reset()` command directly and applies its result; every other failure (including a repeated `notFound`) dispatches `BriefLoadFailed` as before. Extracted `applyLoadedView` (the success-path body shared by `load()` and the new recovery path) and added `recoverFromMissingBrief`. |
|
||||
| `apps/ssp/src/app/brief/application/brief.store.spec.ts` | New `describe('BriefStore.load — 404 tolerance (RB-22)')` with the two required cases. Six pre-existing `load:` fakes' explicit `Result<string, BriefView>` return-type annotations updated to `Result<BriefLoadFailure, BriefView>` (they only ever produce the `ok: true` branch, so this is a type-only change); two shared `ok(v)` test helpers that build fakes for both `load` and `save` had their return-type annotation dropped in favour of `as const` inference, since one helper now serves two different error-channel types. |
|
||||
| `libs/shared/docs/behaviour-spec.mdx` | Regenerated (`npm run gen:behaviour-spec`) — picks up the two new `it()` titles. |
|
||||
|
||||
No `backend/` file was touched — `Program.cs` and `Data/BriefStore.cs` are RB-23's, per the
|
||||
ticket's explicit scope.
|
||||
|
||||
## How the once-only bound is structural
|
||||
|
||||
`BriefStore` gains one field: `private hasRecoveredFromMissingBrief = false`. `load()`
|
||||
takes the recovery branch only when `r.error.tag === 'notFound' && !this.hasRecoveredFromMissingBrief`,
|
||||
and the branch's first statement sets the flag before doing anything else. A second 404 —
|
||||
whether from a second `load()` call, or in principle from `reset()` itself somehow also
|
||||
404ing — falls through to the plain `BriefLoadFailed` branch instead, on every subsequent
|
||||
call, for the life of the store instance. This is a field on the singleton store, not a
|
||||
comment: nothing in the reachable call graph can flip it back to `false`.
|
||||
|
||||
The loop CQ-007's proposed change warns about ("the reset's own load must not be able to
|
||||
loop") is not merely bounded, it is **structurally absent**: `recoverFromMissingBrief`
|
||||
calls `this.adapter.reset()` and applies its `BriefView` result directly (the same
|
||||
`applyLoadedView` the success path uses) — it never calls `this.load()` again. There is no
|
||||
recursive edge from the recovery path back into `load()` for the once-only flag to have to
|
||||
stop; the flag exists only to stop a **second, separate** `load()` invocation (e.g. a
|
||||
caller retrying navigation) from reaching `reset()` again.
|
||||
|
||||
## Judgement calls
|
||||
|
||||
- **`load()` no longer uses `runResult`, only for this one method.** `runResult`
|
||||
(`libs/shared/src/application/submit.ts`, RB-17) intentionally keeps only a string —
|
||||
every other read in the app is fine with that. This is the first read that needs one
|
||||
more bit (the HTTP status) than `runResult` exposes, so `load()` does its own
|
||||
try/catch instead, matching `runResult`'s shape (`problemDetail(e, fallback)` on the
|
||||
non-404 path) but adding the 404 branch first. `libs/shared/src/application/submit.ts`
|
||||
itself is untouched — changing a shared helper used by many call sites for one adapter's
|
||||
need was out of scope and unjustified.
|
||||
- **404 detection reads `(e as {status?:unknown}).status === 404`, not
|
||||
`SwaggerException.isSwaggerException`.** The generated client throws a plain
|
||||
`SwaggerException` for `GET /brief` today (no OpenAPI 404 response is declared for it
|
||||
yet), but throws the parsed `ProblemDetails` object instead for an endpoint whose spec
|
||||
**does** declare a 404 (both shapes carry a `status` field). Checking `status` alone,
|
||||
not the `SwaggerException` type, means this predicate keeps working unchanged once
|
||||
RB-23 regenerates the client with a documented 404 response for `briefGET()` — no
|
||||
follow-up FE edit needed for detection to keep working.
|
||||
- **`BriefLoadFailure` is a new exported type, not a sentinel string.** CLAUDE.md's
|
||||
default reflex is a discriminated union over a second flag; a magic string
|
||||
(`'__not_found__'`) compared by identity would have kept `load()`'s signature at
|
||||
`Result<string, BriefView>` and touched fewer test lines, but it is exactly the kind
|
||||
of stringly-typed control flow the union tool exists to avoid. The touched-test cost
|
||||
was six type annotations plus two helper signatures, all in the one already-scoped
|
||||
spec file — judged worth it for the correct shape.
|
||||
`BriefLoadFailure` is exported.
|
||||
- **On a repeated 404, the store shows `BRIEF_LOAD_FAILED`** (the same generic banner
|
||||
text `load()` already used for every other failure), not a distinct "still missing"
|
||||
message. No new user-facing copy was needed or added, so no new `$localize` id and no
|
||||
`messages.en.xlf` change — confirmed by diffing for `$localize` occurrences: both
|
||||
hits in the diff are unchanged context lines, not new additions.
|
||||
- **`resetDemo()` (the "start over" button) was left untouched**, even though it
|
||||
duplicates part of the same apply-a-fresh-view logic now factored into
|
||||
`applyLoadedView`. It also manages `actionState`/`saveState`/`rejectionSnapshot` that
|
||||
`recoverFromMissingBrief` correctly does not touch (an automatic recovery on first
|
||||
load is not a user-initiated "start over" action), and refactoring it was not asked
|
||||
for by this ticket.
|
||||
|
||||
## Verification
|
||||
|
||||
- **Verified red without the fix.** Temporarily replaced `load()`'s body (via `Edit`,
|
||||
not `git checkout`) with the pre-fix shape — every failure, `notFound` included,
|
||||
dispatches `BriefLoadFailed` straight away, no `reset()` call — and reran the spec
|
||||
file. Both new tests failed:
|
||||
`expected "vi.fn()" to be called 1 times, but got 0 times` on `reset`, for both "a 404
|
||||
drives exactly one reset()" and "a second 404 does not drive a second reset()"; the
|
||||
other 18 tests in the file stayed green. Restored the real fix with a second `Edit`
|
||||
and reran: all 20 tests in the file green, 30/30 across both touched spec files.
|
||||
- `npm run ci` (foreground, no background/Monitor): **green**, exit 0 — lint,
|
||||
typecheck, `dep:check` (341 + 226 modules, 0 violations), `format:check`,
|
||||
`check:tokens`, `check:seam`, tests (ssp 260/260, behandelportal 37/37, shared
|
||||
138/138, beheer 23/23 — 458 total), `ng build --localize` (both apps), `npm audit`
|
||||
(0 vulnerabilities), backend `dotnet test` (260/260 — the known
|
||||
`OpenZaakIntegrationTests.Admin_cases_…` container-dependent failure did not
|
||||
reproduce on this run, matching the standing caveat that it needs a live OpenZaak
|
||||
container and is not this ticket's bug), backend dependency audit clean, `gen:snippets`
|
||||
drift clean, `gen:behaviour-spec` drift clean once the regenerated file was staged (the
|
||||
local gate diffs the working tree against the index, so it necessarily shows a diff
|
||||
before the file is staged/committed — the same documented, expected behaviour RB-17
|
||||
recorded, not a defect).
|
||||
- `npx prettier --check` on every touched file (including the reformatted
|
||||
`99-backlog.md` table and the regenerated `behaviour-spec.mdx`): clean.
|
||||
|
||||
## What RB-23 must do
|
||||
|
||||
Once `GET /brief` in `Program.cs` returns a real 404 (no `ProblemDetails` body is
|
||||
required — `BriefAdapter.load()`'s `isHttpNotFound` only reads the HTTP `status`, not the
|
||||
response body), and `BriefStore.GetOrCreate` splits into `Get` (query) + the existing
|
||||
`ResetAndCreate` (already there, already used by `POST /brief/reset`), this ticket's
|
||||
`notFound` branch stops being dead code and starts running on first-ever page load for any
|
||||
owner with no persisted brief. Run `npm run gen:api` as part of RB-23 so `briefGET()`
|
||||
regenerates with a documented `status === 404` branch throwing the parsed `ProblemDetails`
|
||||
(matching the shape most other endpoints already use) — the detection predicate here
|
||||
already tolerates that shape and needs no FE follow-up change. Two things worth
|
||||
re-verifying once RB-23 lands, not fixing preemptively here: first, that the resulting
|
||||
double round-trip (404, then `reset()`) is an acceptable UX cost on a first visit, per
|
||||
CQ-007's own framing of this as its least certain finding; second, that this store's
|
||||
`hasRecoveredFromMissingBrief` field — private to one store instance, reset only by a full
|
||||
reload — is still the right lifetime for the once-only guard once a real 404 can occur in
|
||||
production traffic, not only in a test's fake adapter.
|
||||
Reference in New Issue
Block a user