Merge RB-25 — add the UPLOAD_TRANSPORT injection token
TE-003: UploadShellService documented UploadTransport as the swap seam, then bound the concrete, unexported KeepaliveTransport class directly, so a spec could not fake it. UPLOAD_TRANSPORT copies the SESSION_PORT shape; the default factory returns the same instance, so runtime behaviour is unchanged. upload-shell.service.ts goes from 0% to 88.57% line coverage across 16 new specs for upload(), cancel(), delete() and pollReturning(). 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:
@@ -126,7 +126,7 @@ Every ticket tracing to a `BIO-` finding, plus every row on agent 07's authorita
|
||||
| **RB-22** | ssp/brief | CQRS-light | _(expand)_ `BriefStore.load()` tolerates a 404 by calling the existing `reset()` once | BL-003; §7 Backend CQRS-light row | S | Low | P2 | 4 | — | **SIGN-OFF** | **done** |
|
||||
| **RB-23** | backend/Program.cs + Data | CQRS-light | _(contract)_ `GET /brief` 404s when absent; `GetOrCreate` → `Get` | BL-003; §7 Backend CQRS-light row | S | Med | P2 | 4 | RB-22 | **SIGN-OFF** | **done** |
|
||||
| **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** | open |
|
||||
| **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-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** | open |
|
||||
|
||||
@@ -0,0 +1,130 @@
|
||||
# RB-25 — `UPLOAD_TRANSPORT` injection token replaces `inject(KeepaliveTransport)`
|
||||
|
||||
Status: **implemented** · 2026-08-28 · Source finding: `02-testability.md` TE-003 ·
|
||||
`99-backlog.md` RB-25, "Merges" table row for RB-25/26/27 · Depends on
|
||||
`implementation/rb-24.md` (the move that put this file at its current path)
|
||||
|
||||
## What was wrong
|
||||
|
||||
`libs/shared/src/application/upload-shell.service.ts` defines `export interface
|
||||
UploadTransport` and documents it as the swap seam for upload transport. It then binds
|
||||
`private transport: UploadTransport = inject(KeepaliveTransport)` — the concrete class,
|
||||
which is `@Injectable` but not exported. A spec cannot reference the class to override its
|
||||
provider, and cannot provide against the interface either, because an interface is not a
|
||||
DI token. The port existed on paper only.
|
||||
|
||||
The consequence TE-003 measures: `upload()`, `delete()`, `cancel()`, and `pollReturning()`
|
||||
— the methods that translate transport and adapter outcomes into `UploadMsg`s — had no
|
||||
spec at all. `libs/shared/upload` sat at 52.0% line / 50.0% branch, and
|
||||
`upload-shell.service.ts` was one of the two unreached non-`ui/` files.
|
||||
|
||||
## What changed
|
||||
|
||||
One file plus one new spec, exactly as scoped:
|
||||
|
||||
- `libs/shared/src/application/upload-shell.service.ts`: added
|
||||
|
||||
```ts
|
||||
export const UPLOAD_TRANSPORT = new InjectionToken<UploadTransport>('UPLOAD_TRANSPORT', {
|
||||
providedIn: 'root',
|
||||
factory: () => inject(KeepaliveTransport),
|
||||
});
|
||||
```
|
||||
|
||||
copied verbatim from TE-003's own fix, placed directly under the `KeepaliveTransport`
|
||||
class it wraps. `UploadShellService.transport` now reads
|
||||
`inject(UPLOAD_TRANSPORT)` instead of `inject(KeepaliveTransport)`. This is the same
|
||||
interface-plus-token shape as `SessionPort`/`SESSION_PORT`
|
||||
(`libs/shared/src/application/session.port.ts`), the repo's one other explicit port.
|
||||
`KeepaliveTransport` itself is untouched: still a private, unexported `@Injectable`, and
|
||||
still the default factory's target — a real app gets the exact same singleton instance
|
||||
it always did.
|
||||
|
||||
- `libs/shared/src/application/upload-shell.service.spec.ts` (new): a recording fake
|
||||
`UploadTransport` (records every `send()` call, exposes `resolveDone`/`rejectDone` per
|
||||
call so a test drives the returned `Promise` by hand) provided against `UPLOAD_TRANSPORT`,
|
||||
plus a fake `UploadAdapter` (a plain object with `vi.fn()` for `status`/`deleteDocument`)
|
||||
provided against the already-exported `UploadAdapter` class. 16 specs across all four
|
||||
target methods:
|
||||
- `upload()` — `UploadQueued` carries the transport's `backgroundSyncAvailable`;
|
||||
`onProgress` → `UploadProgress`; a resolved transport → `UploadComplete`; a rejected
|
||||
transport → `UploadFailed` with the rejection reason; a rejection with the
|
||||
`UPLOAD_ABORTED` sentinel dispatches nothing.
|
||||
- `cancel()` — calls the stored cancel function for an in-flight upload and forgets it
|
||||
(a second `cancel()` on the same id is a no-op); an unknown id is a no-op.
|
||||
- `delete()` — `UploadDeleting` then `UploadDeleteComplete` on success;
|
||||
`UploadDeleteFailed` with the server's `detail` on a ProblemDetails rejection; falls
|
||||
back to an empty reason when the rejection carries no `detail`.
|
||||
- `pollReturning()` — skips the adapter call entirely for an empty upload list;
|
||||
dispatches `BackgroundUploadsReturned` filtered to only the items the server reports
|
||||
`complete` with a `documentId`; dispatches nothing when nothing has arrived.
|
||||
|
||||
No other file changed. `UploadAdapter`, `upload.machine.ts`, and `upload-controller.ts`
|
||||
are untouched, per the ticket's file-scope fence (RB-26 and RB-28 are concurrently in
|
||||
adjacent files).
|
||||
|
||||
## Verification
|
||||
|
||||
- **Coverage, `upload-shell.service.ts`** (`npm run test:coverage` narrowed to `shared`,
|
||||
read from `coverage/shared/lcov.info`):
|
||||
|
||||
| Metric | Before | After |
|
||||
| --------- | ------ | -------------- |
|
||||
| Lines | 0% | 88.57% (31/35) |
|
||||
| Branches | 0% | 85.00% (17/20) |
|
||||
| Functions | 0% | 87.50% (14/16) |
|
||||
|
||||
"Before" is 0% across the board: no spec file for this service existed prior to this
|
||||
ticket (confirmed by `grep -rln UploadShellService --include=*.spec.ts`, which returns
|
||||
only the new spec), matching TE-003's "unreached" classification. The remaining
|
||||
uncovered lines are the `KeepaliveTransport` class body (`send()`, its `inject`) and the
|
||||
`UPLOAD_TRANSPORT` factory closure itself — both require a real `XMLHttpRequest`/real DI
|
||||
resolution to exercise and are intentionally out of this ticket's scope: TE-003's fix is
|
||||
the seam, not a rewrite of the transport it wraps.
|
||||
|
||||
- **Red-proof.** Edited `upload()`'s success branch from
|
||||
`dispatch({ type: 'UploadComplete', localId: req.localId, documentId })` to
|
||||
`dispatch({ type: 'UploadFailed', localId: req.localId, reason: 'BROKEN-FOR-RED-PROOF' })`,
|
||||
ran `ng test shared`. Result: 1 failed / 150 passed, with
|
||||
|
||||
```
|
||||
AssertionError: expected "vi.fn()" to be called with arguments: [ { type: 'UploadComplete', …(2) } ]
|
||||
Received:
|
||||
1st vi.fn() call: [{ "backgroundSync": false, "localId": "l1", "type": "UploadQueued" }]
|
||||
2nd vi.fn() call: [{ "localId": "l1", "reason": "BROKEN-FOR-RED-PROOF", "type": "UploadFailed" }]
|
||||
```
|
||||
|
||||
at `upload-shell.service.spec.ts:79` (the `UploadComplete` assertion). Re-applied the
|
||||
original line with a second edit (not `git checkout`); `git diff` against HEAD shows
|
||||
only the intended token change — the red edit left no trace. Re-ran: 151/151 green.
|
||||
|
||||
- `npm run ci`: result and step count in the final answer.
|
||||
|
||||
## Judgement call
|
||||
|
||||
- **The fake `UploadAdapter` is a plain object, not a class extending `UploadAdapter`.**
|
||||
`UploadAdapter` is exported and already usable as a DI token (it always was — TE-003's
|
||||
gap was specific to `KeepaliveTransport`, not `UploadAdapter`), so `delete()` and
|
||||
`pollReturning()` (which never touch `this.transport`) were technically fakeable before
|
||||
this ticket by providing a fake `UploadAdapter`. Nobody had written that spec, though,
|
||||
and `upload()`/`cancel()` still needed `UPLOAD_TRANSPORT` regardless (they populate and
|
||||
drain the `inflight` map via `transport.send()`). The spec fakes both seams together so
|
||||
all four methods are exercised as one coherent suite, per the ticket's own framing
|
||||
("provide a recording fake transport and assert the message translation in `upload()`,
|
||||
`delete()`, `cancel()` and `pollReturning()`").
|
||||
|
||||
## Handoff to RB-27
|
||||
|
||||
RB-27 extracts `uploadOutcome(status, responseText)` out of the XHR closure in
|
||||
`libs/shared/src/infrastructure/upload.adapter.ts`'s `xhrUpload` — a different file,
|
||||
untouched by this ticket. The token makes RB-27's optional half (moving the
|
||||
`currentScenario()` branch into `KeepaliveTransport.send()`) no easier and no harder than
|
||||
before: `KeepaliveTransport` is still unexported and its `send()` body is unchanged, one
|
||||
line (`inject(UploadAdapter)`, `return this.adapter.xhrUpload(req, onProgress)`). If RB-27
|
||||
takes that optional move, it can inject `UPLOAD_TRANSPORT` in its own spec to assert the
|
||||
scenario branch without touching this file — the seam is there and provided-in-root, but
|
||||
RB-27 does not need to change anything here to use it.
|
||||
|
||||
## `npm run ci`
|
||||
|
||||
Result and step count reported in the final answer.
|
||||
Reference in New Issue
Block a user