From e221834f6ec652e316f80c0ed2c81d17ae3e4e26 Mon Sep 17 00:00:00 2001 From: Edwin van den Houdt Date: Fri, 4 Sep 2026 20:33:56 +0200 Subject: [PATCH] refactor: add successOr, sweep remaining inline unwraps (RD-17) Eight sites hand-rolled `rd.tag === 'Success' ? rd.value : fallback`. Six take the new `successOr(rd, fallback)`, one takes the existing `successOf`, and one (`big-profile.store.ts`) uses the existing `map`, since it returns a RemoteData rather than an unwrapped value. Co-Authored-By: Claude Sonnet 5 --- .../app/behandeling/ui/beoordeling.page.ts | 6 +- .../app/behandeling/ui/werkvoorraad.page.ts | 6 +- .../application/big-profile.store.ts | 10 +- .../app/registratie/ui/admin-cases.page.ts | 6 +- .../ui/dashboard/mijn-aanvragen.section.ts | 8 +- .../registratie-wizard.component.ts | 7 +- .../RD-17-successor-sweep.md | 160 ++++++++++++++++++ docs/project/readable-codebase/README.md | 2 +- libs/beheer/src/ui/audit.page.ts | 6 +- libs/shared/docs/behaviour-spec.mdx | 7 +- .../src/application/feature-flags.store.ts | 7 +- .../src/application/remote-data.spec.ts | 14 +- libs/shared/src/application/remote-data.ts | 10 ++ 13 files changed, 212 insertions(+), 37 deletions(-) create mode 100644 docs/project/readable-codebase/RD-17-successor-sweep.md diff --git a/apps/behandelportal/src/app/behandeling/ui/beoordeling.page.ts b/apps/behandelportal/src/app/behandeling/ui/beoordeling.page.ts index cf8971e..8922f77 100644 --- a/apps/behandelportal/src/app/behandeling/ui/beoordeling.page.ts +++ b/apps/behandelportal/src/app/behandeling/ui/beoordeling.page.ts @@ -1,5 +1,6 @@ import { Component, computed, inject } from '@angular/core'; import { ActivatedRoute } from '@angular/router'; +import { successOf } from '@shared/application/remote-data'; import { PageShellComponent } from '@shared/layout/page-shell/page-shell.component'; import { AlertComponent } from '@shared/ui/alert/alert.component'; import { ButtonComponent } from '@shared/ui/button/button.component'; @@ -73,10 +74,7 @@ export class BeoordelingPage { protected retryText = $localize`:@@beoordeling.retry:Opnieuw proberen`; protected rows = detailRows; - protected readonly view = computed(() => { - const rd = this.store.view(); - return rd.tag === 'Success' ? rd.value : undefined; - }); + protected readonly view = computed(() => successOf(this.store.view())); constructor() { void this.store.load(this.id); diff --git a/apps/behandelportal/src/app/behandeling/ui/werkvoorraad.page.ts b/apps/behandelportal/src/app/behandeling/ui/werkvoorraad.page.ts index ce8ed3f..b52e8f6 100644 --- a/apps/behandelportal/src/app/behandeling/ui/werkvoorraad.page.ts +++ b/apps/behandelportal/src/app/behandeling/ui/werkvoorraad.page.ts @@ -5,6 +5,7 @@ import { ButtonComponent } from '@shared/ui/button/button.component'; import { SkeletonComponent } from '@shared/ui/skeleton/skeleton.component'; import { ASYNC } from '@shared/ui/async/async.component'; import { AccessStore } from '@shared/application/access.store'; +import { successOr } from '@shared/application/remote-data'; import { WerkvoorraadStore } from '@behandeling/application/werkvoorraad.store'; import { WerkvoorraadListComponent } from '@behandeling/ui/werkvoorraad-list/werkvoorraad-list.component'; @@ -56,10 +57,7 @@ export class WerkvoorraadPage { protected access = inject(AccessStore); protected canBeoordelen = computed(() => this.access.can('aanvraag:beoordelen')); - protected items = computed(() => { - const rd = this.store.items(); - return rd.tag === 'Success' ? rd.value : []; - }); + protected items = computed(() => successOr(this.store.items(), [])); protected heading = $localize`:@@werkvoorraad.heading:Werkvoorraad`; protected intro = $localize`:@@werkvoorraad.intro:Aanvragen die op beoordeling wachten.`; diff --git a/apps/ssp/src/app/registratie/application/big-profile.store.ts b/apps/ssp/src/app/registratie/application/big-profile.store.ts index c590e96..81c3255 100644 --- a/apps/ssp/src/app/registratie/application/big-profile.store.ts +++ b/apps/ssp/src/app/registratie/application/big-profile.store.ts @@ -52,10 +52,12 @@ export class BigProfileStore { ); /** Specialisms/notes stay a separate stream (they have their own empty state). */ - readonly aantekeningen = computed>(() => { - const rd = fromResource(this.aantekeningenRes, (v) => !v || v.length === 0); - return rd.tag === 'Success' ? { tag: 'Success', value: rd.value ?? [] } : rd; - }); + readonly aantekeningen = computed>(() => + map( + fromResource(this.aantekeningenRes, (v) => !v || v.length === 0), + (v) => v ?? [], + ), + ); // --- Optimistic herregistratie state, shared with the dashboard ----------- private pending = signal(false); diff --git a/apps/ssp/src/app/registratie/ui/admin-cases.page.ts b/apps/ssp/src/app/registratie/ui/admin-cases.page.ts index 8f83fad..cc051ac 100644 --- a/apps/ssp/src/app/registratie/ui/admin-cases.page.ts +++ b/apps/ssp/src/app/registratie/ui/admin-cases.page.ts @@ -6,6 +6,7 @@ import { DataBlockComponent } from '@shared/ui/data-block/data-block.component'; import { DataRowComponent } from '@shared/ui/data-row/data-row.component'; import { ASYNC } from '@shared/ui/async/async.component'; import { AccessStore } from '@shared/application/access.store'; +import { successOr } from '@shared/application/remote-data'; import { formatDatumNl } from '@shared/kernel/datum'; import { Aanvraag } from '@registratie/domain/aanvraag'; import { TYPE_LABELS, statusLabel, referentie } from '@registratie/domain/aanvraag-view'; @@ -78,10 +79,7 @@ export class AdminCasesPage { protected access = inject(AccessStore); protected canManage = computed(() => this.access.can('cases:manage')); - protected cases = computed(() => { - const rd = this.store.cases(); - return rd.tag === 'Success' ? rd.value : []; - }); + protected cases = computed(() => successOr(this.store.cases(), [])); protected heading = $localize`:@@adminCases.heading:Aanvragen beheren`; protected intro = $localize`:@@adminCases.intro:Alle aanvragen in het register. Een aanvraag verwijderen kan niet ongedaan worden gemaakt.`; diff --git a/apps/ssp/src/app/registratie/ui/dashboard/mijn-aanvragen.section.ts b/apps/ssp/src/app/registratie/ui/dashboard/mijn-aanvragen.section.ts index 7fce4b6..feabd9e 100644 --- a/apps/ssp/src/app/registratie/ui/dashboard/mijn-aanvragen.section.ts +++ b/apps/ssp/src/app/registratie/ui/dashboard/mijn-aanvragen.section.ts @@ -6,6 +6,7 @@ import { HeadingComponent } from '@shared/ui/heading/heading.component'; import { ApplicationListComponent } from '@shared/ui/application-list/application-list.component'; import { ApplicationLinkComponent } from '@shared/ui/application-link/application-link.component'; import { ASYNC } from '@shared/ui/async/async.component'; +import { successOr } from '@shared/application/remote-data'; import { AanvragenStore } from '@registratie/application/aanvragen.store'; import { Aanvraag, AanvraagType } from '@registratie/domain/aanvraag'; import { @@ -88,10 +89,9 @@ export class MijnAanvragenSection { protected submittedRow = submittedRow; - protected aanvragen = computed(() => { - const rd = this.store.aanvragen(); - return rd.tag === 'Success' ? sortForDashboard(rd.value) : []; - }); + protected aanvragen = computed(() => + sortForDashboard(successOr(this.store.aanvragen(), [])), + ); protected concepten_ = computed(() => concepten(this.aanvragen())); protected ingediend_ = computed(() => ingediend(this.aanvragen())); diff --git a/apps/ssp/src/app/registratie/ui/registratie-wizard/registratie-wizard.component.ts b/apps/ssp/src/app/registratie/ui/registratie-wizard/registratie-wizard.component.ts index 3f64c28..50a1d4c 100644 --- a/apps/ssp/src/app/registratie/ui/registratie-wizard/registratie-wizard.component.ts +++ b/apps/ssp/src/app/registratie/ui/registratie-wizard/registratie-wizard.component.ts @@ -21,7 +21,7 @@ import { ASYNC } from '@shared/ui/async/async.component'; import { AddressFieldsComponent } from '@registratie/ui/address-fields/address-fields.component'; import { createStore } from '@shared/application/store'; import { whenTag } from '@shared/kernel/fp'; -import { RemoteData } from '@shared/application/remote-data'; +import { RemoteData, successOr } from '@shared/application/remote-data'; import { RegistratieLookupStore } from '@registratie/application/registratie-lookup.store'; import { DuoLookupDto, PolicyQuestionDto } from '@registratie/contracts/duo-diplomas.dto'; import { @@ -516,10 +516,7 @@ export class RegistratieWizardComponent { inside it too: ``'s own context can't inherit a generic from the sibling [data] input (Angular only infers a structural directive's type parameter from an input on that same node). */ - protected duoData = computed(() => { - const rd = this.lookupRd(); - return rd.tag === 'Success' ? rd.value : null; - }); + protected duoData = computed(() => successOr(this.lookupRd(), null)); readonly jaNee = JA_NEE; diff --git a/docs/project/readable-codebase/RD-17-successor-sweep.md b/docs/project/readable-codebase/RD-17-successor-sweep.md new file mode 100644 index 0000000..6a27daa --- /dev/null +++ b/docs/project/readable-codebase/RD-17-successor-sweep.md @@ -0,0 +1,160 @@ +# RD-17 — Add `successOr`, and sweep the remaining inline unwraps + +Status: done +Source: PLAN.md 2.3 + +## Why + +`successOf` landed with the dashboard refactor and removed the repeated +`rd.tag === 'Success' ? rd.value : undefined` from five sites. **Eight more inline unwraps +remain**, and they do not all want the same helper — the fallbacks genuinely differ. One +helper is missing, and one site hand-rolls a function that already exists. + +## Already in the working tree (an interrupted session did steps 1-2) + +`git status` shows four modified files. Do not redo this work; check it, then continue. + +- `remote-data.ts` — `successOr` exists at 119 with the three-parameter signature and a doc + comment. Done. +- `remote-data.spec.ts` — `successOr` cases mirror `successOf`'s. Done. +- `behandeling/ui/beoordeling.page.ts` — converted to `successOf`. Done. +- `behandeling/ui/werkvoorraad.page.ts` — converted to `successOr(rd, [])`. Done. + +Six sites in the decision-2 table remain. + +## Read first + +- `libs/shared/src/application/remote-data.ts` — `successOf` at 109, `map` at 78, and the + `// #region showcase:fold` marker at 50-69. `successOr` goes next to `successOf`, well clear + of that region. +- `libs/shared/src/application/remote-data.spec.ts` — `successOf` already has cases; mirror + them. + +## Decisions (pre-made, don't relitigate) + +1. **Add `successOr` with three type parameters, not two:** + + ```ts + export function successOr(rd: RemoteData, fallback: F): T | F { + return rd.tag === 'Success' ? rd.value : fallback; + } + ``` + + The third parameter is load-bearing. Call sites pass `[]` and `null`, neither of which is + assignable to `T`, so a two-parameter `successOr(rd, fallback: T): T` would not + compile at those sites. + +2. **The eight in-scope sites, and which helper each takes:** + + | Site | Today | Becomes | + | ------------------------------------------------------- | -------------------------------------------------- | ------------------------------------- | + | `behandeling/ui/beoordeling.page.ts:78` | `? rd.value : undefined` | `successOf(rd)` | + | `behandeling/ui/werkvoorraad.page.ts:61` | `? rd.value : []` | `successOr(rd, [])` | + | `registratie/ui/admin-cases.page.ts:83` | `? rd.value : []` | `successOr(rd, [])` | + | `beheer/src/ui/audit.page.ts:99` | `? rd.value : []` | `successOr(rd, [])` | + | `shared/application/feature-flags.store.ts:27` | `? rd.value : []` | `successOr(rd, [])` | + | `registratie/ui/dashboard/mijn-aanvragen.section.ts:93` | `? sortForDashboard(rd.value) : []` | `sortForDashboard(successOr(rd, []))` | + | `registratie/ui/registratie-wizard/…component.ts:521` | `? rd.value : null` | `successOr(rd, null)` | + | `registratie/application/big-profile.store.ts:57` | `? { tag: 'Success', value: rd.value ?? [] } : rd` | **`map(rd, (v) => v ?? [])`** | + +3. **The last row is the interesting one.** `big-profile.store.ts:57` re-wraps a `Success` + and passes everything else through — that is exactly `map`, which has existed in this file + since before the arc started. Use the existing function; do not reach for `successOr` + there, because the site returns a `RemoteData`, not an unwrapped value. + +4. **`mijn-aanvragen.section.ts` folds a map into its unwrap.** `sortForDashboard(successOr(rd, []))` + is equivalent because `sortForDashboard([])` is `[]`, and it reads better than nesting + `map`. Keep the sort outside. + +5. **Leave the two boolean predicates alone.** `access.store.ts:36` + (`&& rd.value.includes(capability)`) and `feature-flags.store.ts:51` + (`&& (rd.value.find(…)?.enabled ?? false)`) answer a yes/no question rather than unwrapping + a value. `successOr(rd, []).includes(x)` would work but allocates an array to answer a + boolean, and reads no better. Not a win. + +6. **Four sites inside `remote-data.ts` itself are not call sites** — they are the bodies of + `map`, `andThen`, `successOf` and the new `successOr`. Obviously do not rewrite a function + in terms of itself. + +7. **Leave the six spec-file occurrences alone.** `aanvragen.store.spec.ts` and + `admin-cases.store.spec.ts` use `s.tag === 'Success' && s.value.map(…)` inside `expect(…)`. + That is an assertion idiom; replacing it would obscure what the test checks. + +## Files + +- `libs/shared/src/application/remote-data.ts` (+ `.spec.ts`) — the new helper +- The eight files in decision 2 + +## Steps + +1. Add `successOr` next to `successOf` per decision 1, with a doc comment saying when to + reach for it rather than `successOf` or `map`. +2. Add spec cases mirroring `successOf`'s. +3. Convert the eight sites per the table. One file at a time; let the type-checker confirm + each. +4. Run `npm run gen:behaviour-spec` — new spec titles otherwise fail the drift check. +5. Update this ticket's `Status:` to `done` and the README's RD-17 row to `done`. +6. Commit all of it together. + +## Acceptance criteria + +Measured baselines, dry-run before handover. The ternary form appears **11** times in the +working tree; **4** of those are the bodies of `map`, `andThen`, `successOf` and `successOr` +inside `remote-data.ts` and must survive, so the target is exactly 4. + +```bash +git grep -c "tag === 'Success' ?" -- apps libs | awk -F: '{s+=$NF} END {print s}' # is 11 -> MUST be 4 +git grep -c "tag === 'Success' ?" -- libs/shared/src/application/remote-data.ts # MUST still be 4 +``` + +The new helper exists and is used: + +```bash +git grep -c "export function successOr" -- libs/shared/src/application/remote-data.ts # MUST be 1 +git grep -l "successOr" -- apps libs | wc -l # >= 7 +``` + +The two predicates and the specs are untouched (decisions 5 and 7): + +```bash +git grep -c "tag === 'Success' &&" -- apps libs | awk -F: '{s+=$NF} END {print s}' # unchanged: 8 +``` + +`big-profile.store.ts` uses the existing `map`, not a new helper (decision 3): + +```bash +git grep -n "map(" -- apps/ssp/src/app/registratie/application/big-profile.store.ts # >= 3 (2 existing + the new one) +``` + +```bash +npm run ci # exits 0 +``` + +## Verification + +`npm run ci`. This edits no story and no `.mdx`, but it **does** edit +`libs/shared/src/application/remote-data.ts`, which is not under `libs/shared/src/ui/**` — so +`--full` is not required by the README's rule. + +Verified for you: the `// #region showcase:fold` marker sits at lines 50-69, well above +`successOf` at 109, so adding a function there cannot cause snippet drift. If you move +anything inside that region, run `npm run gen:snippets` in the same commit. + +## Out of scope + +- The two boolean predicates (decision 5). +- The six spec-file assertions (decision 7). +- `remote-data.ts`'s own three internal uses (decision 6). +- Adding any further combinator. `successOf`, `successOr`, `map`, `map2` and `andThen` cover + every site here; a sixth would be speculative. + +## Risks + +- **Do not give `successOr` two type parameters.** Call sites pass `[]` and `null`; a + `fallback: T` signature fails to compile at exactly the sites this ticket exists to fix. +- **`big-profile.store.ts:57` is a `map`, not a `successOr`.** It returns a `RemoteData`. Using + `successOr` there would change the member's type and break its consumers. +- **`behaviour-spec.mdx` drift** from the new spec titles. Run `gen:behaviour-spec` in the + same commit. +- **Watch the `?? []` inside `big-profile.store.ts:57`.** The value being mapped is + nullable; the `?? []` must move inside the `map` callback, not disappear. diff --git a/docs/project/readable-codebase/README.md b/docs/project/readable-codebase/README.md index 3618dec..3ef5e9e 100644 --- a/docs/project/readable-codebase/README.md +++ b/docs/project/readable-codebase/README.md @@ -111,7 +111,7 @@ two. Note that RD-15 exists because 22 abandoned agent worktrees are still on di | RD-14 | Move `SaveState` to `debounced-save.ts`; delete `action-state.ts` | 13 | | done | | RD-15 | Remove 22 abandoned agent worktrees (4.7 GB) | 01 | | done | | RD-16 | ~~`parseDashboardView` returns `BigProfile`~~ — DROPPED, see PLAN.md 2.2 | 01 | | n/a | -| RD-17 | `successOf`/`successOr` sweep — 10 sites, 8 files | 01 | | todo | +| RD-17 | `successOf`/`successOr` sweep — 10 sites, 8 files | 01 | | done | | RD-18 | Ticket-reference sweep, frontend — 181 refs, 100 files | 01 | | todo | | RD-19 | Ticket-reference sweep, backend — 370 refs, 86 files | 01 | | todo | | RD-20 | `wizard-errors.ts` + spec, adopted by all 3 wizards | 02 | | todo | diff --git a/libs/beheer/src/ui/audit.page.ts b/libs/beheer/src/ui/audit.page.ts index d88f301..505a702 100644 --- a/libs/beheer/src/ui/audit.page.ts +++ b/libs/beheer/src/ui/audit.page.ts @@ -5,6 +5,7 @@ import { AlertComponent } from '@shared/ui/alert/alert.component'; import { ButtonComponent } from '@shared/ui/button/button.component'; import { ASYNC } from '@shared/ui/async/async.component'; import { AccessStore } from '@shared/application/access.store'; +import { successOr } from '@shared/application/remote-data'; import { AuditStore } from '@beheer/application/audit.store'; /** @@ -94,10 +95,7 @@ export class AuditPage { protected access = inject(AccessStore); protected canRead = computed(() => this.access.can('cases:manage')); - protected entries = computed(() => { - const rd = this.store.entries(); - return rd.tag === 'Success' ? rd.value : []; - }); + protected entries = computed(() => successOr(this.store.entries(), [])); protected heading = $localize`:@@audit.heading:Auditlog`; protected intro = $localize`:@@audit.intro:Toegangs- en inzagebeslissingen (autorisatie en het tonen van afgeschermde gegevens). Vastgelegd zonder persoonsgegevens.`; diff --git a/libs/shared/docs/behaviour-spec.mdx b/libs/shared/docs/behaviour-spec.mdx index 11698eb..dd037a7 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. 530 frontend behaviours across +**is** the suite, reshaped for a business reader. 532 frontend behaviours across 9 contexts; 261 backend behaviours across 42 test classes. @@ -968,6 +968,11 @@ classes. - unwraps a Success value - is undefined for every other state +#### successOr + +- unwraps a Success value +- is the fallback for every other state + #### upload lifecycle messages - queued → progress → complete diff --git a/libs/shared/src/application/feature-flags.store.ts b/libs/shared/src/application/feature-flags.store.ts index 5f57b5f..925ca73 100644 --- a/libs/shared/src/application/feature-flags.store.ts +++ b/libs/shared/src/application/feature-flags.store.ts @@ -1,5 +1,5 @@ import { Injectable, computed, inject, signal } from '@angular/core'; -import { RemoteData } from '@shared/application/remote-data'; +import { RemoteData, successOr } from '@shared/application/remote-data'; import { runSubmit } from '@shared/application/submit'; import { Result, ok, err } from '@shared/kernel/fp'; import { FeatureFlag } from '@shared/domain/feature-flag'; @@ -22,10 +22,7 @@ export class FeatureFlagStore { readonly flags = this.state.asReadonly(); /** The resolved list (empty until loaded) — for the admin toggle UI. */ - readonly all = computed(() => { - const rd = this.state(); - return rd.tag === 'Success' ? rd.value : []; - }); + readonly all = computed(() => successOr(this.state(), [])); constructor() { void this.load(); diff --git a/libs/shared/src/application/remote-data.spec.ts b/libs/shared/src/application/remote-data.spec.ts index 323ddbd..ff7eaea 100644 --- a/libs/shared/src/application/remote-data.spec.ts +++ b/libs/shared/src/application/remote-data.spec.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest'; -import { RemoteData, fromLoadLifecycle, map2, map, successOf } from './remote-data'; +import { RemoteData, fromLoadLifecycle, map2, map, successOf, successOr } from './remote-data'; import { loading, failure, empty, success } from '../testing/remote-data'; const loadingRd: RemoteData = loading(); @@ -34,6 +34,18 @@ describe('successOf', () => { }); }); +describe('successOr', () => { + it('unwraps a Success value', () => { + expect(successOr(ok(2), 0)).toBe(2); + }); + + it('is the fallback for every other state', () => { + expect(successOr(loadingRd, [])).toEqual([]); + expect(successOr(failureRd, [])).toEqual([]); + expect(successOr(empty(), null)).toBeNull(); + }); +}); + describe('fromLoadLifecycle', () => { it('maps Loading → Loading', () => { expect(fromLoadLifecycle({ tag: 'Loading' })).toEqual(loading()); diff --git a/libs/shared/src/application/remote-data.ts b/libs/shared/src/application/remote-data.ts index 6f35d15..093e277 100644 --- a/libs/shared/src/application/remote-data.ts +++ b/libs/shared/src/application/remote-data.ts @@ -109,3 +109,13 @@ export function andThen( export function successOf(rd: RemoteData): T | undefined { return rd.tag === 'Success' ? rd.value : undefined; } + +/** Unwrap a Success value, or a caller-supplied `fallback` for every other state. + Reach for this over `successOf` when `undefined` is not a usable value at the + call site (e.g. a list the template iterates, which wants `[]`). Reach for + `map` instead when the site needs to stay a `RemoteData`, not an unwrapped + value. Three type parameters: `fallback` need not be assignable to `T` + (an empty array is not a `T[]` at the type level, only at the value level). */ +export function successOr(rd: RemoteData, fallback: F): T | F { + return rd.tag === 'Success' ? rd.value : fallback; +}