Merge RB-26 — extract planFileSelection from the upload controller
TE-004: createUploadController performed three inject() calls, an effect()
registration and a window listener before returning, so the real policy
buried inside it — deciding per file whether to reject or start an upload —
was reachable only through a TestBed. planFileSelection in upload.machine.ts
is now that decision as a pure function taking plain {name, type, size}
objects; the controller executes the plan and keeps the one impure step
(crypto.randomUUID()) it can't move. No change to the controller's public
surface.
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:
@@ -127,7 +127,7 @@ Every ticket tracing to a `BIO-` finding, plus every row on agent 07's authorita
|
||||
| **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-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** | open |
|
||||
| **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 |
|
||||
| **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** |
|
||||
|
||||
@@ -0,0 +1,109 @@
|
||||
# RB-26 — move the accept/reject decision into `planFileSelection` (`upload.machine.ts`)
|
||||
|
||||
Status: **implemented** · 2026-08-28 · Source finding: `02-testability.md` TE-004 ·
|
||||
`99-backlog.md` RB-26 · Depends on `implementation/rb-24.md` (moved the upload files into
|
||||
`infrastructure`/`domain`/`application`)
|
||||
|
||||
## What was wrong
|
||||
|
||||
TE-004: `createUploadController` does three `inject()` calls, registers an `effect()`, and
|
||||
adds a `window` focus listener, all before it returns. A spec must run inside a `TestBed`
|
||||
injection context with `UploadAdapter`, `UploadShellService`, and `DestroyRef` all
|
||||
satisfied to reach anything inside it. What sits behind that cost is real policy:
|
||||
`onFileSelected` decides, per file, whether to reject it with reason `'multiple'`, reject
|
||||
it with a `rejectReason` result, or start its upload — a decision over
|
||||
`(categories, categoryId, files)` with no I/O in it. `rejectReason`, the predicate that
|
||||
decision calls, was already exported and spec'd; the decision that calls it was not.
|
||||
|
||||
## What changed
|
||||
|
||||
`libs/shared/src/domain/upload.machine.ts` gains one pure export:
|
||||
|
||||
```ts
|
||||
export function planFileSelection(
|
||||
state: UploadState,
|
||||
categoryId: string,
|
||||
files: { name: string; type: string; size: number }[],
|
||||
): UploadMsg[];
|
||||
```
|
||||
|
||||
It takes plain `{ name, type, size }` objects, not `File` — a spec needs no DOM. The body
|
||||
is the old `onFileSelected` decision, moved: an unknown category plans nothing; too many
|
||||
files for a single-file category plans one `FileRejected` with reason `'multiple'` and
|
||||
skips the per-file checks; otherwise each file is judged by `rejectReason` and plans
|
||||
either a `FileRejected` or a `FileSelected` entry, one entry per input file, in order.
|
||||
|
||||
`libs/shared/src/application/upload-controller.ts`'s `onFileSelected` now maps `selected:
|
||||
File[]` to plain candidates, calls `planFileSelection`, and executes the result: a
|
||||
`FileRejected` entry dispatches as-is; anything else starts the upload for the file at
|
||||
that same array index (`crypto.randomUUID()`, `files.set()`, `shell.upload()` — the three
|
||||
things that must stay impure and stay in the controller). No other method changed.
|
||||
`previewUrlFor` (added by RB-24) is untouched.
|
||||
|
||||
## The `localId` placeholder — a deliberate, contained choice
|
||||
|
||||
An accepted file's planned `FileSelected` entry carries `localId: ''`. A real id needs
|
||||
`crypto.randomUUID()`, and the ticket is explicit that call stays in the controller, not
|
||||
the domain. The controller reads only each entry's `.type` to route it — it dispatches a
|
||||
`FileRejected` entry verbatim, but for a `FileSelected` entry it discards the entry and
|
||||
calls `start(categoryId, selected[i])`, which builds its own message with a real id.
|
||||
The placeholder is therefore never dispatched. This was the only way found to keep the
|
||||
return type exactly `UploadMsg[]` (as the ticket's own code sketch specifies) while still
|
||||
letting the plan carry a per-file, order-preserving "start this one" signal — the
|
||||
`FileRejected` variant carries no file identity (state keys rejections by category only),
|
||||
so position in the returned array is what the controller uses to find the matching
|
||||
original `File`. A discriminated `{ kind: 'reject' | 'start'; msg? }` return would avoid
|
||||
the placeholder but was not built, since the ticket's signature is explicit and the
|
||||
placeholder design meets it without changing behaviour.
|
||||
|
||||
## Behaviour
|
||||
|
||||
Same messages, same order, for the same inputs. Tracing all three original branches:
|
||||
|
||||
- Unknown category: original returns without dispatching; new code calls `planFileSelection`
|
||||
(returns `[]`), then `forEach` over an empty array — no dispatch, no start.
|
||||
- Too many files for a single-file category: original dispatches one `FileRejected`
|
||||
('multiple') and returns; new code gets a one-entry plan and dispatches that one entry —
|
||||
`forEach` never reaches indices past the plan's length, so no file starts.
|
||||
- Per-file loop: original dispatches `FileRejected` or calls `start` for each file, in
|
||||
order; new code's plan has one entry per file, in the same order, and the controller
|
||||
dispatches or starts at each index identically.
|
||||
|
||||
## Testing
|
||||
|
||||
`libs/shared/src/domain/upload.machine.spec.ts` gained a `planFileSelection` describe
|
||||
block: unknown category (plans nothing), the `'multiple'` batch rejection, a passing
|
||||
single file against a single-file category, `rejectReason`'s two reject cases (`'type'`,
|
||||
`'size'`) reached through the plan, the accept case's exact `FileSelected` shape
|
||||
(including the `localId: ''` placeholder), and a mixed multiple-file case asserting
|
||||
order (`['FileSelected', 'FileRejected', 'FileSelected']`).
|
||||
|
||||
**Proved red before green**, per the ticket's instruction not to use `git checkout`:
|
||||
temporarily replaced the function body with a stub returning `[]` unconditionally (an
|
||||
edit, not a revert), ran `ng test shared`, and got:
|
||||
|
||||
```
|
||||
Test Files 1 failed | 23 passed (24)
|
||||
Tests 6 failed | 139 passed (145)
|
||||
```
|
||||
|
||||
The 6 failures were the `'multiple'` rejection, both `rejectReason` cases, the accepted-
|
||||
file shape, and the mixed-order case — every outcome that depends on the real branching,
|
||||
each failing with `expected [] to deeply equal [...]`. The unknown-category case passed
|
||||
even against the stub, since both the stub and the real implementation return `[]` there
|
||||
— expected, not a gap, since that branch has no policy to exercise. A second edit restored
|
||||
the real body; the same run returned to `24 passed / 145 passed`.
|
||||
|
||||
## Scope held
|
||||
|
||||
No change to `createUploadController`'s construction, the `effect()`, or the `window`
|
||||
listener — those are RB-25/RB-27's targets (RB-25 is `UploadShellService`, running
|
||||
concurrently in the same commit window; RB-27 is `upload.adapter.ts`'s XHR closure).
|
||||
Neither file was touched. The controller's public surface (`previewUrlFor`,
|
||||
`onFileSelected`, `onRemove`, `onRetry`, `onDelete`, `onChannelChange`) is unchanged in
|
||||
name and signature, and the organism that calls it (`<app-document-upload>`) needed no
|
||||
change.
|
||||
|
||||
## `npm run ci`
|
||||
|
||||
Result and step count reported in the closing message.
|
||||
Reference in New Issue
Block a user