refactor(shared): extract uploadOutcome from the XHR load closure (RB-27)
UploadAdapter.xhrUpload built new XMLHttpRequest() directly and put the
actual decisions inside its load listener: 2xx-vs-not, JSON.parse of the
body with a fallback, and ProblemDetails mapping via parseError. None of
it was reachable without stubbing the XHR global, so it had no spec
(TE-005; file LH 5/64, BRH 3/57).
Extract uploadOutcome(status, responseText): Result<string, {
documentId }>, a pure function next to genericError/parseError. It holds
the 2xx check, the JSON.parse-with-fallback, and the ProblemDetails
mapping. The load listener is now a two-line dispatch into it.
Abort-vs-error disambiguation stays where it is: it decides whether a
response exists at all, before uploadOutcome would even run, and the
proposed signature has no field for "aborted". It is already a one-line
ternary with no DOM-only logic to extract.
Add upload.adapter.spec.ts: plain describe/it, no DOM, no XHR stub,
covering a 2xx success, a 2xx unparseable body, a non-2xx ProblemDetails
body, a non-2xx non-ProblemDetails body, and the 200/300 boundary.
Verified red by editing uploadOutcome down to one line (an Edit, not
git checkout): 4 of 5 new specs failed. Re-applied with a second Edit.
Coverage for upload.adapter.ts: LH 5/64 -> 12/65, BRH 3/57 -> 7/59.
Skip TE-005's optional half (moving the currentScenario() branch into
KeepaliveTransport.send()): it needs a second file, upload-shell.
service.ts, and this ticket's own scope fences it to upload.adapter.ts
and its spec. The dev simulator's behaviour is unchanged.
Mark RB-27 implemented in 99-backlog.md and add its implementation note,
including a batch 5 close-out.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -101,7 +101,7 @@ Every ticket tracing to a `BIO-` finding, plus every row on agent 07's authorita
|
||||
16-row "Compliance review required" list, carries it — regardless of priority.
|
||||
|
||||
| ID | Module | Category | Description | Baseline metric improved | Effort | Risk | Priority | CD batch # | Depends on | Compliance | Status |
|
||||
| --------- | -------------------------------- | ------------- | ------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------- | ------ | -------- | -------- | ---------- | ---------- | ------------ | -------- |
|
||||
| --------- | -------------------------------- | ------------- | ------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------- | ------ | -------- | -------- | ---------- | ---------- | ------------ | --------------- |
|
||||
| **RB-01** | backend/Program.cs + Data | security | Add an owner/capability check to `GET /uploads/{id}/content` and `/uploads/status`; 404 not 403 | §3c Data 75.5% branch vs 99.0% line (BL-005) | S | Low | **P1** | 1 | — | **SIGN-OFF** | **done** |
|
||||
| **RB-02** | backend/Program.cs + Data | privacy | Stop concatenating the BSN into `AuthzAudit.Resource`; assert on **values** in the test | §3c Data 75.5% branch (BL-005) | S | Low | **P1** | 1 | — | **SIGN-OFF** | **done** |
|
||||
| **RB-03** | backend/Contracts | privacy | `MaskTail(a.Owner, 3)` in `ToAdminSummaryDto` — both cross-owner lists inherit it | §3a bhp/behandeling 91.6%/81.5%; §7 Mapping row | S | Low | **P1** | 1 | — | **SIGN-OFF** | **done** |
|
||||
@@ -128,7 +128,7 @@ Every ticket tracing to a `BIO-` finding, plus every row on agent 07's authorita
|
||||
| **RB-24** | libs/shared/upload | ADR conform. | Move `upload/` into `infrastructure`/`domain`/`application`; **delete** the depcruise carve-out | BL-010; §7 "+1 adapter outside `infrastructure/`", "8 of 9 machines in `domain/`"; §3b shared/domain 0% reach | M | Med | P2 | 5 | — | **SIGN-OFF** | **done** |
|
||||
| **RB-25** | libs/shared/upload | testability | `UPLOAD_TRANSPORT` injection token (the `SESSION_PORT` shape) instead of `inject(KeepaliveTransport)` | §3a upload 52.0%/50.0%; §3b file unreached, non-`ui/` | S | Low | P2 | 5 | RB-24 | **SIGN-OFF** | **done** |
|
||||
| **RB-26** | libs/shared/upload | testability | Move the accept/reject decision to `planFileSelection` in `upload.machine.ts` | §3a upload 52.0%/50.0%; §4a module max CC 27 | S | Low | P2 | 5 | RB-24 | **SIGN-OFF** | **done** |
|
||||
| **RB-27** | libs/shared/upload | testability | Extract `uploadOutcome(status, responseText)` out of the XHR closure | file LH 5/64 (**7.8% line**), BRH 3/57 (**5.3% branch**) | S–M | Low | P2 | 5 | RB-25 | **SIGN-OFF** | open |
|
||||
| **RB-27** | libs/shared/upload | testability | Extract `uploadOutcome(status, responseText)` out of the XHR closure | file LH 5/64 (**7.8% line**), BRH 3/57 (**5.3% branch**) | S–M | Low | P2 | 5 | RB-25 | **SIGN-OFF** | **implemented** |
|
||||
| **RB-28** | libs/beheer + ssp/brief | testability | `BLOB_PRESENTER` token; the 3 commands' success paths become assertable | §3a beheer/application **40.5% branch — worst FE**; brief.store BRH 32/64 | S–M | Low | P2 | 5 | — | **SIGN-OFF** | **done** |
|
||||
| **RB-29** | backend/Domain | testability | Thread the existing `at` through `LetterHtml.ResolveAuto` instead of reading `UtcNow` | §3c Domain 82.0% branch; §4b `LetterHtml.cs` CC 21 | S | Low | P2 | 5 | — | — | **done** |
|
||||
| **RB-30** | backend/Data + Domain | testability | Extract 5 brief guards into `Domain/Letters/BriefRules.cs`; add `tests/Domain/BriefRuleTests.cs` | §3c Data **75.5% branch** (BL-005); §4b `BriefStore.cs` CC 17, `ToDto` CC 16 | M | Med | P2 | 5 | — | **SIGN-OFF** | **done** |
|
||||
|
||||
@@ -0,0 +1,193 @@
|
||||
# RB-27 — `uploadOutcome` extracted from the XHR `load` closure
|
||||
|
||||
Status: **implemented** · 2026-08-28 · Source finding: `02-testability.md` TE-005 ·
|
||||
`99-backlog.md` RB-27, "Merges" table row for RB-25/26/27 · Depends on
|
||||
`implementation/rb-24.md` (the move that put this file at its current path) and
|
||||
`implementation/rb-25.md` (handoff paragraph read before deciding the optional half)
|
||||
|
||||
## What was wrong
|
||||
|
||||
`libs/shared/src/infrastructure/upload.adapter.ts`'s `xhrUpload` constructs
|
||||
`new XMLHttpRequest()` directly and attaches its `load` listener inline. The listener
|
||||
body held the actual decisions: 2xx-vs-not, `JSON.parse` of the response body with a
|
||||
fallback to a generic error, and (on a non-2xx status) ProblemDetails mapping via the
|
||||
un-exported `parseError`. None of it is reachable without stubbing the XHR global, so
|
||||
the interpretation logic had no spec.
|
||||
|
||||
TE-005's baseline citation: **LH 5 / LF 64 (7.8% line), BRH 3 / BRF 57 (5.3% branch)**.
|
||||
The file was counted "reached" in the module total only because another spec imports
|
||||
it — essentially nothing in it executed.
|
||||
|
||||
## What changed
|
||||
|
||||
One function extracted from the `load` listener, in the same file:
|
||||
|
||||
```ts
|
||||
export function uploadOutcome(
|
||||
status: number,
|
||||
responseText: string,
|
||||
): Result<string, { documentId: string }> {
|
||||
if (status < 200 || status >= 300) return err(parseError(responseText));
|
||||
try {
|
||||
return ok({ documentId: JSON.parse(responseText).documentId });
|
||||
} catch {
|
||||
return err(genericError());
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
placed next to `genericError`/`parseError` (below the class, above the dev
|
||||
`simulateUpload`). It contains exactly the 2xx-vs-not check, the `JSON.parse`-with-
|
||||
fallback, and the ProblemDetails mapping — the three decisions TE-005 names. The `load`
|
||||
listener is now a two-line dispatch:
|
||||
|
||||
```ts
|
||||
xhr.addEventListener('load', () => {
|
||||
const outcome = uploadOutcome(xhr.status, xhr.responseText);
|
||||
outcome.ok ? resolve(outcome.value) : reject(outcome.error);
|
||||
});
|
||||
```
|
||||
|
||||
`Result`, `ok`, `err` are imported from `@shared/kernel/fp` (the repo's one `Result`
|
||||
type, already used the same way by `libs/shared`'s other infrastructure adapters).
|
||||
`parseError` and `genericError` are untouched — `uploadOutcome` calls them exactly as
|
||||
the old listener body did, so their own behavior (ProblemDetails detail extraction,
|
||||
generic fallback) is unchanged.
|
||||
|
||||
## Abort-vs-error: left as a separate, smaller concern
|
||||
|
||||
TE-005 names abort-vs-error disambiguation in the same sentence as the extraction
|
||||
target, but its proposed signature — `uploadOutcome(status: number, responseText:
|
||||
string)` — has no way to express "the request was aborted before any response
|
||||
arrived." That is a real, structural mismatch, not an oversight to route around:
|
||||
|
||||
- `uploadOutcome` runs inside the `load` listener, which fires only when the browser
|
||||
received a complete HTTP response — it has a `status` and a `responseText` by
|
||||
construction.
|
||||
- The `abort` listener fires instead of `load` when `xhr.abort()` was called
|
||||
client-side. There is no HTTP response at that point — no status, no body — so
|
||||
folding it into `uploadOutcome`'s signature would mean inventing a fake status (e.g.
|
||||
`0`) to stand for "not actually a response," which trades one implicit convention for
|
||||
another and makes the pure function's contract lie about what it receives.
|
||||
|
||||
The existing code already expresses this as the smallest form it can take:
|
||||
|
||||
```ts
|
||||
xhr.addEventListener('abort', () => (aborted ? reject(UPLOAD_ABORTED) : reject(genericError())));
|
||||
```
|
||||
|
||||
one ternary, deciding between two sentinels based on which native event fired and
|
||||
whether `cancel()` was called first — not on response content. It is not a second
|
||||
`uploadOutcome`-shaped decision hiding in a closure; it is a one-line dispatch already.
|
||||
Extracting it into its own named function would add a call site and an import for a
|
||||
single ternary with no reachable-only-via-DOM logic left inside it. Left in place, as
|
||||
DoD point 2 allows.
|
||||
|
||||
## Spec added, verified red
|
||||
|
||||
`libs/shared/src/infrastructure/upload.adapter.spec.ts` (new file) — plain
|
||||
`describe`/`it`, no `TestBed`, no DOM, no XHR stub, matching the DoD's explicit
|
||||
"that is the entire point." Five cases:
|
||||
|
||||
1. 2xx status with a valid JSON body → `{ ok: true, value: { documentId } }`.
|
||||
2. 2xx status with an unparseable body → falls back to the generic `UPLOAD_FAILED`
|
||||
text (the `JSON.parse`-with-fallback branch).
|
||||
3. Non-2xx status with a ProblemDetails body → the `detail` field, via `parseError`.
|
||||
4. Non-2xx status with a body that is not ProblemDetails-shaped → falls back to the
|
||||
generic text.
|
||||
5. The 200/300 boundary: 299 is success, 300 is not.
|
||||
|
||||
`UPLOAD_FAILED`'s text is not exported (unchanged by this ticket), so the spec holds
|
||||
its own copy of the Dutch string as a local constant with a comment pointing at the
|
||||
source — the same trade every other spec makes when asserting against `$localize`
|
||||
constants that never leave their module ($localize`strings are English-first prose
|
||||
only where the source is`nl`, so this is the source text as written, not a stand-in).
|
||||
|
||||
**Red-proof.** Edited `uploadOutcome`'s body down to a single line —
|
||||
`return ok({ documentId: JSON.parse(responseText).documentId });`, dropping the
|
||||
status check and the try/catch — with an `Edit` (not `git checkout`). Ran
|
||||
`ng test shared`. Result: 4 of the 5 new specs failed:
|
||||
|
||||
```
|
||||
SyntaxError: Unexpected token 'o', "not json" is not valid JSON
|
||||
❯ uploadOutcome libs/shared/src/infrastructure/upload.adapter.ts:168:32
|
||||
|
||||
AssertionError: expected { ok: true, value: { …(1) } } to deeply equal { ok: false, …(1) }
|
||||
- Expected "error": "Document is al aan een aanvraag gekoppeld.", "ok": false,
|
||||
+ Received "ok": true, "value": { "documentId": undefined },
|
||||
|
||||
SyntaxError: Unexpected token 'I', "Internal S"... is not valid JSON
|
||||
|
||||
AssertionError: expected true to be false // Object.is equality
|
||||
```
|
||||
|
||||
(only the plain 2xx-valid-JSON case still passed, as expected of a mutant that always
|
||||
reports success). Re-applied the real body with a second `Edit`; `git diff` against
|
||||
HEAD shows only the intended net change — the red edit left no trace. Re-ran:
|
||||
163/163 green.
|
||||
|
||||
## Coverage, `upload.adapter.ts`
|
||||
|
||||
| Metric | Before (TE-005 baseline) | After |
|
||||
| -------- | ------------------------ | ---------------------- |
|
||||
| Lines | LH 5 / LF 64 (7.8%) | LH 12 / LF 65 (18.5%) |
|
||||
| Branches | BRH 3 / BRF 57 (5.3%) | BRH 7 / BRF 59 (11.9%) |
|
||||
|
||||
(`LF`/`BRF` grew by one line and two branches because `uploadOutcome` is new source;
|
||||
`npm run test:coverage`'s shared run, `coverage/shared/lcov.info`, narrowed to this
|
||||
file's `SF:` block.) The jump is real but modest in absolute percentage: `uploadOutcome`
|
||||
itself is now fully exercised (`FNDA:6,uploadOutcome`, both branches of the status
|
||||
check hit, both the try and the catch path hit), but the class methods
|
||||
(`categoriesResource`, `status`, `deleteDocument`, `xhrUpload`'s own body,
|
||||
`simulateUpload`) remain unreached — they need DI/XHR/timers to test and are
|
||||
out of this ticket's scope, exactly as TE-005 scopes it ("extract the interpretation,
|
||||
not the transport").
|
||||
|
||||
## Optional scenario-branch move: not taken
|
||||
|
||||
TE-005 suggests, as an explicitly optional second half, moving the `currentScenario()`
|
||||
branch from `xhrUpload` up into `KeepaliveTransport.send()`
|
||||
(`libs/shared/src/application/upload-shell.service.ts`) so `xhrUpload` becomes
|
||||
transport-only. RB-25's handoff confirms the seam is available (`KeepaliveTransport`
|
||||
is still unexported, `send()` is still an unchanged one-liner) but not required.
|
||||
|
||||
This ticket does not take that half, for a reason RB-25's handoff does not settle:
|
||||
the ticket's own **Scope** section restricts this ticket to `upload.adapter.ts` and its
|
||||
spec only ("RB-24, RB-25, RB-26, RB-28 have all already merged — nothing else in the
|
||||
upload module is in flight, so you have the folder to yourself"). Moving the scenario
|
||||
branch requires editing `upload-shell.service.ts` too — exporting `simulateUpload` (or
|
||||
moving it) out of `upload.adapter.ts` and importing it into the application-layer
|
||||
`send()` — which is a second file, outside the stated scope. Doing it anyway would also
|
||||
widen this single-file ticket's diff for an explicitly optional half the ticket itself
|
||||
says to skip when it "complicates the diff." The dev simulator's behavior is therefore
|
||||
byte-for-byte unchanged: `xhrUpload` still checks `currentScenario()` first and still
|
||||
delegates to the untouched `simulateUpload` for `upload-slow`/`upload-fail`, verified by
|
||||
inspection (the only edit inside `xhrUpload` is the `load`-listener dispatch) and by the
|
||||
full `shared` suite staying green, including `upload-shell.service.spec.ts`'s existing
|
||||
scenario-adjacent assertions.
|
||||
|
||||
## Verification
|
||||
|
||||
- `npm run lint`: clean.
|
||||
- `npm run dep:check`: unaffected — the only new import is `@shared/kernel/fp`, already
|
||||
the repo's shared `Result` module, imported the same way by other `libs/shared`
|
||||
infrastructure adapters (no new import direction).
|
||||
- `npm test` / `ng test shared`: 163/163, across 26 spec files — 5 of those tests are
|
||||
the new `upload.adapter.spec.ts`, the other 158 across 25 pre-existing files are
|
||||
unchanged by this ticket.
|
||||
- `npm run ci`: result and step count reported in the implementing agent's final answer.
|
||||
|
||||
## Batch 5 close-out
|
||||
|
||||
Batch 5 (RB-25 through RB-30) is now fully implemented. For `libs/shared/upload`
|
||||
(moved to its layered home by RB-24) specifically: `upload.machine.ts` (domain) has its
|
||||
own spec and `planFileSelection` extracted by RB-26; `upload-shell.service.ts`
|
||||
(application) has a full spec covering `upload()`/`cancel()`/`delete()`/
|
||||
`pollReturning()` via the `UPLOAD_TRANSPORT` token RB-25 added; `upload-controller.ts`
|
||||
(application) was already spec'd before this batch; `upload.adapter.ts`
|
||||
(infrastructure) now has `uploadOutcome` as a pure, spec'd seam, though the class's
|
||||
HTTP-bound methods (categories/status/delete/the XHR transport itself) remain
|
||||
untested by design — XHR is the one boundary this batch deliberately does not
|
||||
abstract, per TE-005's own instruction. End to end, every layer of the upload module
|
||||
that can hold pure logic now does, and has a spec proving it; what is left uncovered is
|
||||
exactly the DOM/network edge the module exists to wrap, not logic hiding behind it.
|
||||
@@ -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. 492 frontend behaviours across
|
||||
**is** the suite, reshaped for a business reader. 497 frontend behaviours across
|
||||
9 contexts; 261 backend behaviours across 42 test
|
||||
classes.
|
||||
|
||||
@@ -929,6 +929,14 @@ classes.
|
||||
- failed then retried returns to queued
|
||||
- UploadRemoved drops the upload
|
||||
|
||||
#### uploadOutcome
|
||||
|
||||
- resolves a 2xx response with a valid JSON body to the document id
|
||||
- falls back to the generic error when a 2xx body is not valid JSON
|
||||
- maps a non-2xx ProblemDetails body to its detail
|
||||
- falls back to the generic error for a non-2xx body without a ProblemDetails detail
|
||||
- treats status 200-299 as success and everything else as failure
|
||||
|
||||
#### withIdempotencyKey / currentIdempotencyKey
|
||||
|
||||
- threads the key to every read made inside the wrapped fn
|
||||
|
||||
@@ -0,0 +1,35 @@
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { uploadOutcome } from './upload.adapter';
|
||||
|
||||
/** Matches the un-exported UPLOAD_FAILED fallback text in upload.adapter.ts. */
|
||||
const UPLOAD_FAILED = 'Uploaden is niet gelukt. Probeer het opnieuw.';
|
||||
|
||||
describe('uploadOutcome', () => {
|
||||
it('resolves a 2xx response with a valid JSON body to the document id', () => {
|
||||
const outcome = uploadOutcome(200, JSON.stringify({ documentId: 'doc-1' }));
|
||||
expect(outcome).toEqual({ ok: true, value: { documentId: 'doc-1' } });
|
||||
});
|
||||
|
||||
it('falls back to the generic error when a 2xx body is not valid JSON', () => {
|
||||
const outcome = uploadOutcome(201, 'not json');
|
||||
expect(outcome).toEqual({ ok: false, error: UPLOAD_FAILED });
|
||||
});
|
||||
|
||||
it('maps a non-2xx ProblemDetails body to its detail', () => {
|
||||
const outcome = uploadOutcome(
|
||||
409,
|
||||
JSON.stringify({ detail: 'Document is al aan een aanvraag gekoppeld.', status: 409 }),
|
||||
);
|
||||
expect(outcome).toEqual({ ok: false, error: 'Document is al aan een aanvraag gekoppeld.' });
|
||||
});
|
||||
|
||||
it('falls back to the generic error for a non-2xx body without a ProblemDetails detail', () => {
|
||||
const outcome = uploadOutcome(500, 'Internal Server Error');
|
||||
expect(outcome).toEqual({ ok: false, error: UPLOAD_FAILED });
|
||||
});
|
||||
|
||||
it('treats status 200-299 as success and everything else as failure', () => {
|
||||
expect(uploadOutcome(299, JSON.stringify({ documentId: 'd' })).ok).toBe(true);
|
||||
expect(uploadOutcome(300, JSON.stringify({ detail: 'x' })).ok).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -9,6 +9,7 @@ import { currentScenario } from '@shared/infrastructure/scenario';
|
||||
import { currentSubject } from '@shared/infrastructure/subject';
|
||||
import { environment } from '@shared/environments/environment';
|
||||
import { DocumentCategory } from '@shared/domain/upload.machine';
|
||||
import { Result, err, ok } from '@shared/kernel/fp';
|
||||
|
||||
/** Answer-derived query params that affect which categories the server presents. */
|
||||
export interface CategoryParams {
|
||||
@@ -128,15 +129,8 @@ export class UploadAdapter {
|
||||
if (e.lengthComputable) onProgress(Math.round((e.loaded / e.total) * 100));
|
||||
});
|
||||
xhr.addEventListener('load', () => {
|
||||
if (xhr.status >= 200 && xhr.status < 300) {
|
||||
try {
|
||||
resolve({ documentId: JSON.parse(xhr.responseText).documentId });
|
||||
} catch {
|
||||
reject(genericError());
|
||||
}
|
||||
} else {
|
||||
reject(parseError(xhr.responseText));
|
||||
}
|
||||
const outcome = uploadOutcome(xhr.status, xhr.responseText);
|
||||
outcome.ok ? resolve(outcome.value) : reject(outcome.error);
|
||||
});
|
||||
xhr.addEventListener('error', () => reject(genericError()));
|
||||
xhr.addEventListener('abort', () =>
|
||||
@@ -160,6 +154,24 @@ export class UploadAdapter {
|
||||
const UPLOAD_FAILED = $localize`:@@upload.failed:Uploaden is niet gelukt. Probeer het opnieuw.`;
|
||||
const genericError = (): string => UPLOAD_FAILED;
|
||||
|
||||
/**
|
||||
* Pure interpretation of one finished XHR `load` event: 2xx-vs-not, `JSON.parse`
|
||||
* of the body with a fallback to a generic error, and (on a non-2xx status)
|
||||
* ProblemDetails mapping via `parseError`. No DOM and no XHR — the listener that
|
||||
* calls this only reads `xhr.status`/`xhr.responseText` and dispatches the result.
|
||||
*/
|
||||
export function uploadOutcome(
|
||||
status: number,
|
||||
responseText: string,
|
||||
): Result<string, { documentId: string }> {
|
||||
if (status < 200 || status >= 300) return err(parseError(responseText));
|
||||
try {
|
||||
return ok({ documentId: JSON.parse(responseText).documentId });
|
||||
} catch {
|
||||
return err(genericError());
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Demo-only (dev): the real XHR POST finishes instantly for metadata, so progress
|
||||
* and failure can't otherwise be shown. Drives the progress bar over ~2.5s, then
|
||||
|
||||
Reference in New Issue
Block a user