refactor(shared): add UPLOAD_TRANSPORT injection token (RB-25)
UploadShellService injected the concrete KeepaliveTransport class instead of a token. The class was not exported, so a spec could not fake it, and could not provide against the UploadTransport interface either, since an interface is not a DI token. The port existed only on paper. Add UPLOAD_TRANSPORT, an InjectionToken with a default factory that resolves the same KeepaliveTransport singleton, following the SessionPort/SESSION_PORT shape. UploadShellService now injects the token. Runtime behaviour is unchanged. Add upload-shell.service.spec.ts: a recording fake transport plus a fake UploadAdapter exercise upload(), delete(), cancel() and pollReturning(), the four methods the missing seam left unreachable. Coverage for upload-shell.service.ts goes from 0% to 88.6% line / 85% branch. Mark RB-25 done in 99-backlog.md and add its implementation note. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -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