robert/todo

Watch

Fork 0

#49 — Tech debt: `callApi`'s untyped request body hides wire-format mismatches #49

New issue

Closed

opened 2026-08-18 13:13:15 +02:00 by lena · 3 comments

lena commented

2026-08-18 13:13:15 +02:00

(Migrated from git.butzei.de)

Tech debt: `callApi`'s untyped request body hides wire-format mismatches

Reported by: Architect / Frontend Engineer (found during #48 User Registration)
Priority: Could — no user-facing symptom by itself, but it's the root cause of a bug class that already hit
production twice
Date: 2026-07-11

Problem

callApi<TResult>(requestName: string, request: {}, ...) (ReactUi/src/api/api.tsx) types its request body
parameter as {} — TypeScript's "any non-null value" type. This means the compiler checks nothing about the
shape of the object passed to any callApi(...) call against the actual C# command/query it targets.

While implementing #48, this let a real bug ship and go undetected: UserSaveDto's wire key is emailAddress
(camelCase of the C# property name), but three separate call sites (SettingsModal.tsx, ForgotPasswordPage.tsx,
and initially the new Register.tsx) sent email instead. Two of the three were pre-existing, shipped bugs —
RequestPasswordResetCommand silently failed on every "forgot password" submission (the anti-enumeration
error-swallowing masked it), and UpdateUserCommand silently blanked the user's stored email on every profile
save. Both had passing tests, because the tests' mocks used the same wrong key as the implementation — internal
consistency, not wire correctness.

Proposal

Type callApi's request parameter against the actual request DTO shape instead of {}, so a call site sending
the wrong field name is a compile error, not a runtime/manual-discovery problem. Options to evaluate:

A discriminated union mapping requestName literal → its expected body type (most precise, but requires
either hand-maintaining the map or generating it from the backend's request types).
At minimum, typing the user:/similar nested DTO fields at each call site against UserSaveDto (etc.) from
dtos.ts, even without fully typing callApi itself.

Acceptance criteria (finalized by PO 2026-07-12)

callApi call sites get compile-time checking against the DTO shape the backend actually expects
A deliberately wrong field name at any callApi(...) call site fails tsc -b, not just at runtime
No behavior change — this is a type-safety improvement only

PO decision: Go with the discriminated-union option (most precise). Build a single
ApiRequestMap (ReactUi/src/api/requests.ts) that hand-maps every requestName literal in use
to its request-body shape and its result type, sourced from the actual C# IRequest<TResult>
record in CqsTodo/Features/**. Bonus beyond the original ask: since the map already knows the
result type per request name, callApi's TResult generic becomes redundant and is dropped —
callApi(requestName, request) return type is inferred automatically, so a mismatched result
type assumption at a call site is now also caught, not just the request body. This is still a
type-only change (no runtime behavior differs) and keeps the map as the single source of truth
instead of splitting body/result across two maps.

ReactUi/src/api/data.tsx's useData hook is unused (zero call sites anywhere in the codebase)
and cannot be typed against ApiRequestMap without either becoming generic over every possible
request name (dead complexity for a hook nobody calls) or reintroducing an escape-hatch {} type
that would undermine the whole point of this ticket. Deleting it is the direct, minimal-scope
consequence of doing this typing correctly, not unrelated cleanup — see 02_software_architect.md
design note in docs/features/done/ once archived.

Blockers

None.

# Tech debt: `callApi`'s untyped request body hides wire-format mismatches **Reported by:** Architect / Frontend Engineer (found during `#48` User Registration) **Priority:** Could — no user-facing symptom by itself, but it's the root cause of a bug class that already hit production twice **Date:** 2026-07-11 ## Problem `callApi<TResult>(requestName: string, request: {}, ...)` (`ReactUi/src/api/api.tsx`) types its request body parameter as `{}` — TypeScript's "any non-null value" type. This means the compiler checks nothing about the shape of the object passed to any `callApi(...)` call against the actual C# command/query it targets. While implementing `#48`, this let a real bug ship and go undetected: `UserSaveDto`'s wire key is `emailAddress` (camelCase of the C# property name), but three separate call sites (`SettingsModal.tsx`, `ForgotPasswordPage.tsx`, and initially the new `Register.tsx`) sent `email` instead. Two of the three were pre-existing, shipped bugs — `RequestPasswordResetCommand` silently failed on every "forgot password" submission (the anti-enumeration error-swallowing masked it), and `UpdateUserCommand` silently blanked the user's stored email on every profile save. Both had passing tests, because the tests' mocks used the same wrong key as the implementation — internal consistency, not wire correctness. ## Proposal Type `callApi`'s `request` parameter against the actual request DTO shape instead of `{}`, so a call site sending the wrong field name is a compile error, not a runtime/manual-discovery problem. Options to evaluate: - A discriminated union mapping `requestName` literal → its expected body type (most precise, but requires either hand-maintaining the map or generating it from the backend's request types). - At minimum, typing the `user:`/similar nested DTO fields at each call site against `UserSaveDto` (etc.) from `dtos.ts`, even without fully typing `callApi` itself. ## Acceptance criteria (finalized by PO 2026-07-12) - [x] `callApi` call sites get compile-time checking against the DTO shape the backend actually expects - [x] A deliberately wrong field name at any `callApi(...)` call site fails `tsc -b`, not just at runtime - [x] No behavior change — this is a type-safety improvement only **PO decision:** Go with the discriminated-union option (most precise). Build a single `ApiRequestMap` (`ReactUi/src/api/requests.ts`) that hand-maps every `requestName` literal in use to its request-body shape **and** its result type, sourced from the actual C# `IRequest<TResult>` record in `CqsTodo/Features/**`. Bonus beyond the original ask: since the map already knows the result type per request name, `callApi`'s `TResult` generic becomes redundant and is dropped — `callApi(requestName, request)` return type is inferred automatically, so a mismatched *result* type assumption at a call site is now also caught, not just the request body. This is still a type-only change (no runtime behavior differs) and keeps the map as the single source of truth instead of splitting body/result across two maps. `ReactUi/src/api/data.tsx`'s `useData` hook is unused (zero call sites anywhere in the codebase) and cannot be typed against `ApiRequestMap` without either becoming generic over every possible request name (dead complexity for a hook nobody calls) or reintroducing an escape-hatch `{}` type that would undermine the whole point of this ticket. Deleting it is the direct, minimal-scope consequence of doing this typing correctly, not unrelated cleanup — see `02_software_architect.md` design note in `docs/features/done/` once archived. ## Blockers None.

lena commented

2026-08-18 13:13:15 +02:00

(Migrated from git.butzei.de)

security (49_typed_callapi_request_bodies_security.md)

Security Review — `#49` Typed `callApi` Request Bodies

Author: Security Agent
Date: 2026-07-12

Pre-review (before implementation)

No new endpoints, no change to authentication/session handling, no change to what data crosses
the wire — this is a frontend-only compile-time typing change. Approved to proceed without a
dedicated deep review; see the design note's Security section.

Final review (after implementation)

Confirmed the implementation matches the pre-review scope: ReactUi/src/api/requests.ts and the
callApi signature change in ReactUi/src/api/api.tsx are type-level only (erased at build
time), and every call site sends the same field names/values as before.

Three behavior changes were introduced, all narrowing rather than widening what the client will
do, so none is a new attack surface:

pushSubscription.ts now throws before calling SubscribeToPushCommand if the browser's
PushSubscriptionJSON is missing endpoint/keys.p256dh/keys.auth, instead of silently
sending a request with those fields undefined (which JSON.stringify would have dropped
from the body entirely). Previously this could reach the backend as a malformed
SubscribeToPushCommand; now it fails on the client before any request is sent — strictly
safer. Code review caught that the original guard fired after the browser had already
created the push subscription, which would've left the device subscribed client-side with no
matching server-side row if the guard tripped; fixed to unsubscribe() before throwing, so a
guard failure now leaves no residual client-side state either.
AcceptInvitePage.tsx now shows the existing "invitation invalid" error state instead of
calling AcceptListInvitationCommand with a possibly-undefined token if the route param is
missing — not a fix for an observed bug (React Router won't render this component without a
non-empty :token path segment in practice), but defense against the compiler-visible
possibility, consistent with treating every request body as untrusted-until-validated.
SettingsModal.tsx's handleProfileSave now requires a valid userId (matching the
userId == null || userId < 0 check already used in CurrentUserAvatar.tsx) before calling
UpdateUserCommand, and shows an error instead of silently doing nothing if the session
expired mid-edit. Same category as #1/#2: a request that could previously be sent with a
bad/null value now either isn't sent (guards 1/2) or is now visibly rejected client-side
instead of a no-op (guard 3) — no case where more data or a wider range of requests is now
sent than before.

No secrets, tokens, or user-controlled data handling changed. No new dependencies added.

Verdict: Approved.

**security** (`49_typed_callapi_request_bodies_security.md`) # Security Review — `#49` Typed `callApi` Request Bodies **Author:** Security Agent **Date:** 2026-07-12 ## Pre-review (before implementation) No new endpoints, no change to authentication/session handling, no change to what data crosses the wire — this is a frontend-only compile-time typing change. Approved to proceed without a dedicated deep review; see the design note's Security section. ## Final review (after implementation) Confirmed the implementation matches the pre-review scope: `ReactUi/src/api/requests.ts` and the `callApi` signature change in `ReactUi/src/api/api.tsx` are type-level only (erased at build time), and every call site sends the same field names/values as before. Three behavior changes were introduced, all narrowing rather than widening what the client will do, so none is a new attack surface: 1. `pushSubscription.ts` now throws before calling `SubscribeToPushCommand` if the browser's `PushSubscriptionJSON` is missing `endpoint`/`keys.p256dh`/`keys.auth`, instead of silently sending a request with those fields `undefined` (which `JSON.stringify` would have dropped from the body entirely). Previously this could reach the backend as a malformed `SubscribeToPushCommand`; now it fails on the client before any request is sent — strictly safer. Code review caught that the original guard fired *after* the browser had already created the push subscription, which would've left the device subscribed client-side with no matching server-side row if the guard tripped; fixed to `unsubscribe()` before throwing, so a guard failure now leaves no residual client-side state either. 2. `AcceptInvitePage.tsx` now shows the existing "invitation invalid" error state instead of calling `AcceptListInvitationCommand` with a possibly-`undefined` token if the route param is missing — not a fix for an observed bug (React Router won't render this component without a non-empty `:token` path segment in practice), but defense against the compiler-visible possibility, consistent with treating every request body as untrusted-until-validated. 3. `SettingsModal.tsx`'s `handleProfileSave` now requires a valid `userId` (matching the `userId == null || userId < 0` check already used in `CurrentUserAvatar.tsx`) before calling `UpdateUserCommand`, and shows an error instead of silently doing nothing if the session expired mid-edit. Same category as `#1`/`#2`: a request that could previously be sent with a bad/null value now either isn't sent (guards 1/2) or is now visibly rejected client-side instead of a no-op (guard 3) — no case where more data or a wider range of requests is now sent than before. No secrets, tokens, or user-controlled data handling changed. No new dependencies added. **Verdict:** Approved.

lena commented

2026-08-18 13:13:15 +02:00

(Migrated from git.butzei.de)

design (49_typed_callapi_request_bodies_design.md)

Design Note — `#49` Typed `callApi` Request Bodies

Author: Software Architect
Date: 2026-07-12

Approach

Add ReactUi/src/api/requests.ts exporting one interface, ApiRequestMap, keyed by every
requestName string literal currently passed to callApi(...) anywhere in ReactUi/src. Each
entry is { request: <body shape>; result: <response shape> }, derived by reading the matching
C# public record XCommand(...) : IRequest<TResult> / XQuery(...) : IRequest<TResult> in
CqsTodo/Features/**. Field types reuse the existing valueTypes.ts aliases and dtos.ts
interfaces wherever one already exists; three new primitive aliases are added to valueTypes.ts
(PasswordResetToken, EmailVerificationToken, NotificationId — all string/number
newtype wrappers on the backend, so a plain alias is correct, matching the existing pattern for
InvitationToken etc.). C# record parameters with a default value (e.g. TodoDueDate? DueDate = null, bool UnreadOnly = false) become optional (?) TS properties, since the wire contract
already treats an omitted key as "use the default."

callApi changes from:

export function callApi<TResult>(requestName: string, request: {}, skipErrorToast?: boolean): Promise<TResult>

to:

export function callApi<K extends ApiRequestName>(
    requestName: K,
    request: ApiRequestMap[K]['request'],
    skipErrorToast?: boolean
): Promise<ApiRequestMap[K]['result']>

TResult is dropped rather than kept alongside K: TypeScript requires either all-or-none
explicit type arguments (no partial lists without defaults), so keeping both would force every
call site to keep writing callApi<TodoDto>(...) even though the correct result type is now
fully determined by requestName — a redundant, driftable annotation the map already makes
unnecessary. All ~40 call sites drop their explicit <TResult> generic; the compiler now derives
and checks it from requestName instead of trusting a hand-written annotation.

ReactUi/src/api/data.tsx (useData hook) has zero call sites in the codebase (verified via
repo-wide grep for api/data imports) and takes requestName: string — incompatible with the
new K extends ApiRequestName constraint without an unsafe cast that would defeat the point of
this change. Deleted rather than kept as dead, untyped code.

Scope check against acceptance criteria

Request body shape checked at every call site — yes, via ApiRequestMap[K]['request'].
Wrong field name fails tsc -b — yes; verified manually during implementation by temporarily
renaming a field at a call site and confirming tsc -b rejects it, then reverting, and pinned
going forward by ReactUi/src/api/requests.typecheck.ts.
No behavior change beyond three narrow, deliberate guards, all fail-closed (each replaces a
previously-possible malformed/no-op request with an explicit rejection, never the reverse):
1. pushSubscription.ts's SubscribeToPushCommand call built its body from
  subscription.toJSON().keys?.p256dh / ?.auth, both string | undefined, against a
  backend command that requires non-null strings — a latent bug the typed request body now
  surfaces as a compile error. Guard added; also unsubscribes the just-created browser-side
  subscription before throwing (code review catch: the guard fires after
  pushManager.subscribe()'s side effect, so without the rollback a failed guard would leave
  the device subscribed client-side with no matching server-side row).
2. AcceptInvitePage.tsx's route token param is typed string | undefined by
  useParams<{token: string}>(), though in practice React Router won't render this component
  without a non-empty :token segment, so this path is not realistically reachable today —
  it's compiler-driven defensive code, not a fix for an observed bug, unlike guard 1. Kept
  because the alternative (a non-null assertion) would silently reintroduce exactly the kind
  of unchecked-assumption bug this ticket exists to eliminate if the route ever changes.
3. SettingsModal.tsx's handleProfileSave needed a userId guard to satisfy
  UpdateUserCommand's non-null userId: UserId (previously userId: number | null compiled
  silently against the untyped {} body). userId can't be the -1 loading sentinel while
  this component is mounted (App.tsx gates the whole route tree on userId !== -1), but it
  can legitimately become null mid-session if the session expires while the modal is open
  (api.tsx's 403 handler calls setUserId(null)) — a real, if rare, reachable path (code
  review catch: an earlier version of this guard used !userId and silently no-op'd instead
  of surfacing an error, which both mishandled -1 per this codebase's own
  userId == null || userId < 0 idiom used in CurrentUserAvatar.tsx and regressed the UX
  from "attempt the call, show an error on failure" to "silently do nothing"). Fixed to match
  that idiom and to surface an error instead of no-op'ing.
4. data.tsx deletion (dead code) — no behavior change since it had no callers.

Known limitation (not fixed this cycle — flagged for PO)

ApiRequestMap is a hand-transcribed snapshot of the backend's request/response contracts, not
generated from them. The backend already exposes a full OpenAPI spec via Swashbuckle
(CqsTodo.WebApi/Program.cs's UseSwagger/AddSwaggerGen), so a codegen step
(openapi-typescript/NSwag) could give the same "wrong field name fails tsc -b" guarantee with
zero hand-maintenance and no drift risk against future backend changes — today, a backend field
rename doesn't fail tsc -b; it only fails once someone manually notices the drift or hits it at
runtime, which is a narrower guarantee than "compile-time checking against the DTO shape the
backend actually expects" could ultimately provide. Out of scope for this Could-tier cycle (it's
materially more work than hand-authoring ~40 map entries); drafted as a follow-up story — see
docs/features/new/51_generate_api_request_map_from_openapi.md.

Security

No new endpoints, no change to what data is sent or to auth/session handling — purely a
compile-time typing change plus the three narrow, fail-closed guards above. Security pre-review:
no concerns, approved to proceed without a dedicated deep review. Final review after
implementation: confirmed scope match, see the security doc for this feature.

**design** (`49_typed_callapi_request_bodies_design.md`) # Design Note — `#49` Typed `callApi` Request Bodies **Author:** Software Architect **Date:** 2026-07-12 ## Approach Add `ReactUi/src/api/requests.ts` exporting one interface, `ApiRequestMap`, keyed by every `requestName` string literal currently passed to `callApi(...)` anywhere in `ReactUi/src`. Each entry is `{ request: <body shape>; result: <response shape> }`, derived by reading the matching C# `public record XCommand(...) : IRequest<TResult>` / `XQuery(...) : IRequest<TResult>` in `CqsTodo/Features/**`. Field types reuse the existing `valueTypes.ts` aliases and `dtos.ts` interfaces wherever one already exists; three new primitive aliases are added to `valueTypes.ts` (`PasswordResetToken`, `EmailVerificationToken`, `NotificationId` — all `string`/`number` newtype wrappers on the backend, so a plain alias is correct, matching the existing pattern for `InvitationToken` etc.). C# record parameters with a default value (e.g. `TodoDueDate? DueDate = null`, `bool UnreadOnly = false`) become optional (`?`) TS properties, since the wire contract already treats an omitted key as "use the default." `callApi` changes from: ```ts export function callApi<TResult>(requestName: string, request: {}, skipErrorToast?: boolean): Promise<TResult> ``` to: ```ts export function callApi<K extends ApiRequestName>( requestName: K, request: ApiRequestMap[K]['request'], skipErrorToast?: boolean ): Promise<ApiRequestMap[K]['result']> ``` `TResult` is dropped rather than kept alongside `K`: TypeScript requires either all-or-none explicit type arguments (no partial lists without defaults), so keeping both would force every call site to keep writing `callApi<TodoDto>(...)` even though the correct result type is now fully determined by `requestName` — a redundant, driftable annotation the map already makes unnecessary. All ~40 call sites drop their explicit `<TResult>` generic; the compiler now derives and checks it from `requestName` instead of trusting a hand-written annotation. `ReactUi/src/api/data.tsx` (`useData` hook) has zero call sites in the codebase (verified via repo-wide grep for `api/data` imports) and takes `requestName: string` — incompatible with the new `K extends ApiRequestName` constraint without an unsafe cast that would defeat the point of this change. Deleted rather than kept as dead, untyped code. ## Scope check against acceptance criteria - Request body shape checked at every call site — yes, via `ApiRequestMap[K]['request']`. - Wrong field name fails `tsc -b` — yes; verified manually during implementation by temporarily renaming a field at a call site and confirming `tsc -b` rejects it, then reverting, and pinned going forward by `ReactUi/src/api/requests.typecheck.ts`. - No behavior change beyond three narrow, deliberate guards, all fail-closed (each replaces a previously-possible malformed/no-op request with an explicit rejection, never the reverse): 1. `pushSubscription.ts`'s `SubscribeToPushCommand` call built its body from `subscription.toJSON().keys?.p256dh` / `?.auth`, both `string | undefined`, against a backend command that requires non-null strings — a latent bug the typed request body now surfaces as a compile error. Guard added; also unsubscribes the just-created browser-side subscription before throwing (code review catch: the guard fires *after* `pushManager.subscribe()`'s side effect, so without the rollback a failed guard would leave the device subscribed client-side with no matching server-side row). 2. `AcceptInvitePage.tsx`'s route `token` param is typed `string | undefined` by `useParams<{token: string}>()`, though in practice React Router won't render this component without a non-empty `:token` segment, so this path is not realistically reachable today — it's compiler-driven defensive code, not a fix for an observed bug, unlike guard 1. Kept because the alternative (a non-null assertion) would silently reintroduce exactly the kind of unchecked-assumption bug this ticket exists to eliminate if the route ever changes. 3. `SettingsModal.tsx`'s `handleProfileSave` needed a `userId` guard to satisfy `UpdateUserCommand`'s non-null `userId: UserId` (previously `userId: number | null` compiled silently against the untyped `{}` body). `userId` can't be the `-1` loading sentinel while this component is mounted (`App.tsx` gates the whole route tree on `userId !== -1`), but it *can* legitimately become `null` mid-session if the session expires while the modal is open (`api.tsx`'s 403 handler calls `setUserId(null)`) — a real, if rare, reachable path (code review catch: an earlier version of this guard used `!userId` and silently no-op'd instead of surfacing an error, which both mishandled `-1` per this codebase's own `userId == null || userId < 0` idiom used in `CurrentUserAvatar.tsx` and regressed the UX from "attempt the call, show an error on failure" to "silently do nothing"). Fixed to match that idiom and to surface an error instead of no-op'ing. 4. `data.tsx` deletion (dead code) — no behavior change since it had no callers. ## Known limitation (not fixed this cycle — flagged for PO) `ApiRequestMap` is a hand-transcribed snapshot of the backend's request/response contracts, not generated from them. The backend already exposes a full OpenAPI spec via Swashbuckle (`CqsTodo.WebApi/Program.cs`'s `UseSwagger`/`AddSwaggerGen`), so a codegen step (openapi-typescript/NSwag) could give the same "wrong field name fails `tsc -b`" guarantee with zero hand-maintenance and no drift risk against future backend changes — today, a backend field rename doesn't fail `tsc -b`; it only fails once someone manually notices the drift or hits it at runtime, which is a narrower guarantee than "compile-time checking against the DTO shape the backend actually expects" could ultimately provide. Out of scope for this Could-tier cycle (it's materially more work than hand-authoring ~40 map entries); drafted as a follow-up story — see `docs/features/new/51_generate_api_request_map_from_openapi.md`. ## Security No new endpoints, no change to what data is sent or to auth/session handling — purely a compile-time typing change plus the three narrow, fail-closed guards above. Security pre-review: no concerns, approved to proceed without a dedicated deep review. Final review after implementation: confirmed scope match, see the security doc for this feature.

lena commented

2026-08-18 13:13:15 +02:00

(Migrated from git.butzei.de)

qa (49_typed_callapi_request_bodies_qa.md)

QA Notes — `#49` Typed `callApi` Request Bodies

Author: QA Agent
Date: 2026-07-12

What was verified

tsc -b is clean across the whole frontend after the change (ReactUi/src/api/requests.ts +
callApi signature change + ~40 call-site updates).
Manually confirmed the acceptance criterion "a deliberately wrong field name fails tsc -b":
temporarily renamed emailAddress → email at the UpdateUserCommand call site in
SettingsModal.tsx, ran tsc -b, got TS2353: Object literal may only specify known properties, and 'email' does not exist in type 'UserSaveDto', then reverted.
ReactUi/src/api/requests.typecheck.ts makes that verification durable instead of a one-off
manual check: it's a compile-only file (not collected by Vitest — no .test./.spec. in the
name) exercising @ts-expect-error against a wrong field name, an unknown request name, and a
missing required field, plus one valid call that must compile clean. If ApiRequestMap ever
regresses back to an untyped {} body, the @ts-expect-error lines stop having anything to
suppress and tsc -b fails on "Unused '@ts-expect-error' directive" — the regression is
caught in CI's frontend build step, same as any other type error.
Full Vitest suite: 36 files / 248 tests passing, unchanged from before this cycle (no new
runtime test needed — this is a compile-time-only guarantee).
npm run lint: 38 problems (30 errors/8 warnings) vs. 42 (33/9) on master before this
change — net reduction (removed now-unused type imports), no new lint findings introduced by
this cycle. Verified by diffing lint output against git stash'd baseline.
Fixed latent bugs this typing work surfaced (see the design note's Scope Check section for
full detail on each): pushSubscription.ts's SubscribeToPushCommand call could previously
send undefined for required key fields if the browser omitted subscription keys (now
guarded, throws instead); AcceptInvitePage.tsx's route token param is typed possibly
undefined by useParams (now guarded, though not realistically reachable given the route
requires a non-empty :token segment — kept as compiler-driven defense, not a fix for an
observed bug); SettingsModal.tsx's profile-save needed a userId guard, which a
four-subagent code review caught was originally written as !userId (silently no-ops instead
of erroring, and doesn't handle the -1 loading sentinel the same way
CurrentUserAvatar.tsx:13 does) — fixed to userId == null || userId < 0 with an explicit
error message, matching that existing idiom.
Updated 5 existing tests whose mocks were incompatible with the new typed callApi signature
(api.test.ts's fake 'TestRequest' name → real 'GetCurrentUserIdQuery';
AcceptInvitePage.test.tsx, InvitePanel.test.tsx, NotificationBell.test.tsx,
pushSubscription.test.ts mock resolved values corrected to match Unit/DTO shapes instead
of undefined) — mechanical fixes, no test intent changed.
Four-subagent /code-review pass (correctness×2, reuse, simplification+efficiency,
altitude+conventions) ran against the full diff; findings addressed: the SettingsModal
!userId guard above; pushSubscription.ts's guard now unsubscribes the just-created browser
subscription before throwing (it previously fired after pushManager.subscribe()'s side
effect, which would've left an orphaned client-side subscription); markNotificationRead's
id parameter now uses the NotificationId type alias instead of a raw number, matching
this codebase's own "no primitive types for domain concepts" convention
(ai/roles/04_frontend_engineer.md); CheckTodoCommand/UncheckTodoCommand in requests.ts
now reuse the existing TodoId type instead of re-declaring its shape inline;
ai/roles/04_frontend_engineer.md's stale reference to the deleted api/data.tsx extraction
pattern was corrected to describe the actual convention (extract only when a request is
reused across 2+ components); the "generate ApiRequestMap from OpenAPI instead of
hand-maintaining it" finding was drafted as a follow-up story,
docs/features/new/51_generate_api_request_map_from_openapi.md, rather than expanded into
this cycle's scope.

Verdict

Approved. No behavior change beyond the three guarded fail-closed fixes noted above (each
replaces a previously-possible malformed/no-op request with an explicit rejection or error
message, never the reverse) — consistent with this codebase's existing error-handling
conventions in the same files.

**qa** (`49_typed_callapi_request_bodies_qa.md`) # QA Notes — `#49` Typed `callApi` Request Bodies **Author:** QA Agent **Date:** 2026-07-12 ## What was verified - `tsc -b` is clean across the whole frontend after the change (`ReactUi/src/api/requests.ts` + `callApi` signature change + ~40 call-site updates). - Manually confirmed the acceptance criterion "a deliberately wrong field name fails `tsc -b`": temporarily renamed `emailAddress` → `email` at the `UpdateUserCommand` call site in `SettingsModal.tsx`, ran `tsc -b`, got `TS2353: Object literal may only specify known properties, and 'email' does not exist in type 'UserSaveDto'`, then reverted. - `ReactUi/src/api/requests.typecheck.ts` makes that verification durable instead of a one-off manual check: it's a compile-only file (not collected by Vitest — no `.test.`/`.spec.` in the name) exercising `@ts-expect-error` against a wrong field name, an unknown request name, and a missing required field, plus one valid call that must compile clean. If `ApiRequestMap` ever regresses back to an untyped `{}` body, the `@ts-expect-error` lines stop having anything to suppress and `tsc -b` fails on "Unused '@ts-expect-error' directive" — the regression is caught in CI's frontend build step, same as any other type error. - Full Vitest suite: 36 files / 248 tests passing, unchanged from before this cycle (no new runtime test needed — this is a compile-time-only guarantee). - `npm run lint`: 38 problems (30 errors/8 warnings) vs. 42 (33/9) on `master` before this change — net reduction (removed now-unused type imports), no new lint findings introduced by this cycle. Verified by diffing lint output against `git stash`'d baseline. - Fixed latent bugs this typing work surfaced (see the design note's Scope Check section for full detail on each): `pushSubscription.ts`'s `SubscribeToPushCommand` call could previously send `undefined` for required key fields if the browser omitted subscription keys (now guarded, throws instead); `AcceptInvitePage.tsx`'s route `token` param is typed possibly `undefined` by `useParams` (now guarded, though not realistically reachable given the route requires a non-empty `:token` segment — kept as compiler-driven defense, not a fix for an observed bug); `SettingsModal.tsx`'s profile-save needed a `userId` guard, which a four-subagent code review caught was originally written as `!userId` (silently no-ops instead of erroring, and doesn't handle the `-1` loading sentinel the same way `CurrentUserAvatar.tsx:13` does) — fixed to `userId == null || userId < 0` with an explicit error message, matching that existing idiom. - Updated 5 existing tests whose mocks were incompatible with the new typed `callApi` signature (`api.test.ts`'s fake `'TestRequest'` name → real `'GetCurrentUserIdQuery'`; `AcceptInvitePage.test.tsx`, `InvitePanel.test.tsx`, `NotificationBell.test.tsx`, `pushSubscription.test.ts` mock resolved values corrected to match `Unit`/DTO shapes instead of `undefined`) — mechanical fixes, no test intent changed. - Four-subagent `/code-review` pass (correctness×2, reuse, simplification+efficiency, altitude+conventions) ran against the full diff; findings addressed: the `SettingsModal` `!userId` guard above; `pushSubscription.ts`'s guard now unsubscribes the just-created browser subscription before throwing (it previously fired after `pushManager.subscribe()`'s side effect, which would've left an orphaned client-side subscription); `markNotificationRead`'s `id` parameter now uses the `NotificationId` type alias instead of a raw `number`, matching this codebase's own "no primitive types for domain concepts" convention (`ai/roles/04_frontend_engineer.md`); `CheckTodoCommand`/`UncheckTodoCommand` in `requests.ts` now reuse the existing `TodoId` type instead of re-declaring its shape inline; `ai/roles/04_frontend_engineer.md`'s stale reference to the deleted `api/data.tsx` extraction pattern was corrected to describe the actual convention (extract only when a request is reused across 2+ components); the "generate `ApiRequestMap` from OpenAPI instead of hand-maintaining it" finding was drafted as a follow-up story, `docs/features/new/51_generate_api_request_map_from_openapi.md`, rather than expanded into this cycle's scope. ## Verdict Approved. No behavior change beyond the three guarded fail-closed fixes noted above (each replaces a previously-possible malformed/no-op request with an explicit rejection or error message, never the reverse) — consistent with this codebase's existing error-handling conventions in the same files.