Set up frontend test tooling and cover the review workspace #11

Closed
opened 2026-07-28 04:52:52 +00:00 by claude-bot · 1 comment

Context

The frontend has no test tooling configured. The review workspace holds the most
intricate client logic in the app — conflict handling, optimistic updates, the
front/back toggle, and tab state — and it is entirely unverified.

Scope

Vitest + React Testing Library setup, plus component tests for the highest-risk UI.

Implementation notes

  • Add Vitest, React Testing Library, and jsdom; wire a test script and make sure it
    runs under the same Node version CI uses.
  • Mock the API layer at the fetch/client boundary so tests do not need a live backend.
  • Provide a TanStack Query wrapper fixture with retries disabled and a fresh
    QueryClient per test — a shared client leaks cache between tests and produces
    false passes.
  • Priority components:
    • Review workspace conflict handling. A 409 must surface the conflict banner and
      block further submission until reload. This is the single most important client
      behaviour to pin down, because failing open here means silent data loss.
    • Decision form: validation, submit, disabled state during flight, success update.
    • Photo browser: status filters, empty state, loading state, error state.
    • Notes inline edit: dirty state, cancel discards, save persists.
    • Auth gate: unauthenticated renders login, authenticated renders shell.
  • Assert on user-visible behaviour and accessible roles rather than implementation details.

Done when

  • npm test runs green locally and in CI
  • Conflict handling, decision submit, and filter behaviour are covered
  • Tests share no state and pass in any order
  • No test requires a running backend

References

  • frontend/src/pages/ReviewWorkspace, PhotoBrowser
  • frontend/src/api/, frontend/src/hooks/useAuth
  • Node/npm run on the dev server at 10.1.1.14 per AGENTS.md
## Context The frontend has no test tooling configured. The review workspace holds the most intricate client logic in the app — conflict handling, optimistic updates, the front/back toggle, and tab state — and it is entirely unverified. ## Scope Vitest + React Testing Library setup, plus component tests for the highest-risk UI. ## Implementation notes - Add Vitest, React Testing Library, and jsdom; wire a `test` script and make sure it runs under the same Node version CI uses. - Mock the API layer at the fetch/client boundary so tests do not need a live backend. - Provide a TanStack Query wrapper fixture with retries disabled and a fresh `QueryClient` per test — a shared client leaks cache between tests and produces false passes. - Priority components: - **Review workspace conflict handling.** A 409 must surface the conflict banner and block further submission until reload. This is the single most important client behaviour to pin down, because failing open here means silent data loss. - Decision form: validation, submit, disabled state during flight, success update. - Photo browser: status filters, empty state, loading state, error state. - Notes inline edit: dirty state, cancel discards, save persists. - Auth gate: unauthenticated renders login, authenticated renders shell. - Assert on user-visible behaviour and accessible roles rather than implementation details. ## Done when - [ ] `npm test` runs green locally and in CI - [ ] Conflict handling, decision submit, and filter behaviour are covered - [ ] Tests share no state and pass in any order - [ ] No test requires a running backend ## References - `frontend/src/pages/ReviewWorkspace`, `PhotoBrowser` - `frontend/src/api/`, `frontend/src/hooks/useAuth` - Node/npm run on the dev server at `10.1.1.14` per `AGENTS.md`
claude-bot added this to the v0.2.0 milestone 2026-07-28 04:52:52 +00:00
Author

Done in baac843. 34 new tests; 50 passing across 6 files.

Tooling was already in place

Vitest + RTL + jsdom, npm test wired and green in CI, src/test/render.tsx giving a fresh QueryClient per test with retries off, and setup.ts running cleanup() + vi.restoreAllMocks() after each. So the work went entirely into the client logic that decides whether a reviewer's work survives.

Two defects, both the same shape

1. patchNotes and patchFlags had no onError at all.

A 409 on either left the reviewer with the editor still open, their text still on screen, and nothing anywhere to say the save had not happened. Notes are where a reviewer records what a relative told them about a photograph — frequently the only surviving record of who is in it — so a reviewer who types it, saves, sees no error and navigates away has lost it. This is exactly the "failing open here means silent data loss" the issue names, and it was on the notes panel rather than the decision form.

2. A conflict did not block further submission.

The issue asks for this explicitly and nothing did it. Every button stayed live, carrying the same review_version the server had already rejected, failing the same way each time — with the reason two panels away in a banner.

Conflict handling is now one shared shape (conflictVersion() + a ConflictProps pair) across the decision form, notes and the rescan flag. The reruns stay enabled: they carry no review_version and cannot conflict, so disabling them would let a conflict in one panel silently remove a capability in another for no reason.

Three accessibility defects, found by asserting on roles

The issue asks for assertions on accessible roles rather than implementation details, and doing that surfaced:

  • the decision form's four <label>s were associated with no control at all — getByLabelText("Precision") simply fails, and so does a screen reader
  • the notes textarea had no accessible name
  • the tab bar was styled <button>s, so which tab is selected was invisible to a screen reader — and the Notes panel has its own History control, making getByRole("button", {name: "History"}) genuinely ambiguous. Now role="tablist" / role="tab" / aria-selected.

Coverage

File Covers
ReviewWorkspacePage.test.tsx (20) conflict banner incl. the no-version case (null, never -1#75), blocking, reload restores, ordinary errors stay inline, decision payload/version/disabled-in-flight/rationale cleared, notes dirty-cancel-save + conflict + inline failure, rescan conflict, front/back toggle incl. no-back-scan, tabs, photo-not-found, and evidence-fetch-failure ≠ no-evidence (#78)
PhotoBrowserPage.test.tsx (10) filter round-trips to the server, All drops the parameter rather than sending status= (which the typed enum from #8 would now 422), initial state read from the URL so a filtered view is linkable, active marking, the three empty states kept distinct, grid rendering, click and keyboard navigation
App.test.tsx (4) the auth gate's three states — treating isLoading as unauthenticated flashes the sign-in page at every signed-in reviewer on every reload; and that a 401 is not retried, since each retry delays the sign-in page and spends the auth rate limit (#68) on a request the user did not make

AuthGate is now exported so it can be tested without mounting a second BrowserRouter inside the test's router.

Verified load-bearing

Mutation: setting blocked: false and deleting the notes onError fails exactly the two tests written for them, and nothing else.

Done when

  • npm test runs green locally and in CI
  • Conflict handling, decision submit, and filter behaviour are covered
  • Tests share no state and pass in any order — fresh QueryClient per test, fetch reset after each
  • No test requires a running backend — everything is stubbed at fetch

tsc --noEmit clean, npm run build clean. Run locally in a node:22-slim container matching CI's image, since this machine has no Node.

Done in baac843. 34 new tests; **50 passing** across 6 files. ## Tooling was already in place Vitest + RTL + jsdom, `npm test` wired and green in CI, `src/test/render.tsx` giving a **fresh `QueryClient` per test with retries off**, and `setup.ts` running `cleanup()` + `vi.restoreAllMocks()` after each. So the work went entirely into the client logic that decides whether a reviewer's work survives. ## Two defects, both the same shape **1. `patchNotes` and `patchFlags` had no `onError` at all.** A 409 on either left the reviewer with the editor still open, their text still on screen, and nothing anywhere to say the save had not happened. Notes are where a reviewer records what a relative told them about a photograph — frequently the only surviving record of who is in it — so a reviewer who types it, saves, sees no error and navigates away has lost it. This is exactly the "failing open here means silent data loss" the issue names, and it was on the *notes* panel rather than the decision form. **2. A conflict did not block further submission.** The issue asks for this explicitly and nothing did it. Every button stayed live, carrying the same `review_version` the server had already rejected, failing the same way each time — with the reason two panels away in a banner. Conflict handling is now one shared shape (`conflictVersion()` + a `ConflictProps` pair) across the decision form, notes and the rescan flag. **The reruns stay enabled**: they carry no `review_version` and cannot conflict, so disabling them would let a conflict in one panel silently remove a capability in another for no reason. ## Three accessibility defects, found by asserting on roles The issue asks for assertions on accessible roles rather than implementation details, and doing that surfaced: - the decision form's four `<label>`s were associated with no control at all — `getByLabelText("Precision")` simply fails, and so does a screen reader - the notes textarea had no accessible name - the tab bar was styled `<button>`s, so *which tab is selected* was invisible to a screen reader — and the Notes panel has its own **History** control, making `getByRole("button", {name: "History"})` genuinely ambiguous. Now `role="tablist"` / `role="tab"` / `aria-selected`. ## Coverage | File | Covers | |---|---| | `ReviewWorkspacePage.test.tsx` (20) | conflict banner incl. the no-version case (`null`, never `-1` — #75), blocking, reload restores, ordinary errors stay inline, decision payload/version/disabled-in-flight/rationale cleared, notes dirty-cancel-save + conflict + inline failure, rescan conflict, front/back toggle incl. no-back-scan, tabs, photo-not-found, and evidence-fetch-failure ≠ no-evidence (#78) | | `PhotoBrowserPage.test.tsx` (10) | filter round-trips to the server, `All` drops the parameter rather than sending `status=` (which the typed enum from #8 would now 422), initial state read from the URL so a filtered view is linkable, active marking, the three empty states kept distinct, grid rendering, click and keyboard navigation | | `App.test.tsx` (4) | the auth gate's **three** states — treating `isLoading` as unauthenticated flashes the sign-in page at every signed-in reviewer on every reload; and that a 401 is not retried, since each retry delays the sign-in page and spends the auth rate limit (#68) on a request the user did not make | `AuthGate` is now exported so it can be tested without mounting a second `BrowserRouter` inside the test's router. ## Verified load-bearing Mutation: setting `blocked: false` and deleting the notes `onError` fails **exactly** the two tests written for them, and nothing else. ## Done when - [x] `npm test` runs green locally and in CI - [x] Conflict handling, decision submit, and filter behaviour are covered - [x] Tests share no state and pass in any order — fresh `QueryClient` per test, `fetch` reset after each - [x] No test requires a running backend — everything is stubbed at `fetch` `tsc --noEmit` clean, `npm run build` clean. Run locally in a `node:22-slim` container matching CI's image, since this machine has no Node.
Sign in to join this conversation.
No description provided.