ui/_docs/03_implementation/reviews/batch_07_review.md

Code Review Report

Batch: 7 — AZ-471, AZ-473, AZ-478, AZ-479 Date: 2026-05-11 Verdict: PASS Mode: Full (per-batch invocation by /implement)

Inputs

• Task specs:
  • _docs/02_tasks/todo/AZ-471_test_canvas_bbox.md (5 ACs, 5 pts)
  • _docs/02_tasks/todo/AZ-473_test_photo_mode.md (3 ACs, 2 pts)
  • _docs/02_tasks/todo/AZ-478_test_network_resilience.md (3 ACs, 3 pts)
  • _docs/02_tasks/todo/AZ-479_test_bundle_fcp_soak.md (4 ACs, 3 pts)
• Changed files (9 total, all under Blackbox Tests OWNED scope):
  • tests/canvas_editor.test.tsx
  • tests/photo_mode.test.tsx
  • tests/network_resilience.test.tsx
  • e2e/tests/canvas_bbox.e2e.ts
  • e2e/tests/photo_mode.e2e.ts
  • e2e/tests/network_resilience.e2e.ts
  • e2e/tests/perf_fcp.e2e.ts
  • e2e/tests/perf_annotation_memory_soak.e2e.ts
  • scripts/run-tests.sh (one new function static_check_bundle_size + one run_static row STC-PERF01)

Findings

#	Severity	Category	File:Line	Title
—	—	—	—	None

No Critical, High, Medium, or Low findings.

Phase Walkthrough

Phase 1 — Context Loading

All 4 task specs read; ACs catalogued; module-layout.md consulted for OWNED / READ-ONLY / FORBIDDEN envelopes. Every changed source file lives under tests/**, e2e/**, or scripts/run-tests.sh — the OWNED scope of the Blackbox Tests cross-cutting component (epic AZ-455). Adding the bundle-size gate to scripts/run-tests.sh is intentional ownership: the script is the test runner / static profile orchestrator, owned by the test infrastructure (AZ-456 / AZ-481), not by any feature component. No production-source file under src/** was modified.

Phase 2 — Spec Compliance

Task	AC	Test	Today	Drift documented
AZ-471	AC-1 (FT-P-39 manual draw geometry)	tests/canvas_editor.test.tsx + e2e/tests/canvas_bbox.e2e.ts	PASS — fast asserts canonical canvas-coordinate quad; e2e drives a real pointer drag and inspects the save POST	—
AZ-471	AC-2 (FT-P-40 8-handle resize)	tests/canvas_editor.test.tsx	PASS — 8 sub-tests, one per handle, each asserting the opposite anchor is invariant	—
AZ-471	AC-3 (FT-P-41 Ctrl+click multi-select)	same	it.fails()	drift — handleMouseDown early-returns into draw mode on Ctrl+button-0 before the multi-select branch runs
AZ-471	AC-4 (FT-P-42 Ctrl+wheel zoom-around-cursor)	same	it.fails()	drift — handleWheel updates zoom only; pan is not adjusted to keep the cursor invariant
AZ-471	AC-5 (FT-P-43 Ctrl+drag pan on empty canvas)	same	it.fails()	drift — same early Ctrl-gate as AC-3; empty-canvas Ctrl+drag enters draw mode instead of pan
AZ-473	AC-1 (FT-P-48 PhotoMode switch sets filter)	tests/photo_mode.test.tsx	PASS — toggling mode updates the rendered class list	—
AZ-473	AC-2 (FT-P-49 auto-select on out-of-range)	same	PASS — switching to a mode where the prior selectedClassNum is out-of-window reselects the first valid class	—
AZ-473	AC-3 (FT-P-50 yoloId on the wire)	tests/photo_mode.test.tsx + e2e/tests/photo_mode.e2e.ts	PASS — fast covers P=0/20/40 against MSW; e2e companion repeats all three modes against the real annotations/ service	—
AZ-478	AC-1 (NFT-RES-03 offline at boot)	tests/network_resilience.test.tsx + e2e/tests/network_resilience.e2e.ts	it.fails() (fast) + test.fail (e2e) + control PASS asserting current redirect behaviour	drift — <App> boot redirects silently to /login on any /api/* failure; no in-DOM error indicator is rendered
AZ-478	AC-2 (NFT-RES-09 tainted-canvas fallback)	tests/network_resilience.test.tsx	it.fails() + control PASS asserting page does not crash	drift — AnnotationsPage.handleDownload calls canvas.toBlob without try/catch; the SecurityError surfaces as an unhandled rejection with no fallback UI
AZ-478	AC-3 (NFT-RES-10 SSE disconnect indicator)	tests/network_resilience.test.tsx + e2e/tests/network_resilience.e2e.ts	it.fails() (fast) + test.fail (e2e) + control PASS asserting the error path fires but no banner renders	drift — no SSE consumer (AnnotationsSidebar, FlightsPage) wires createSSE's onError to a connection-lost banner
AZ-479	AC-1 (NFT-PERF-01 / NFT-RES-LIM-01 bundle ≤ 2 MB gzipped)	scripts/run-tests.sh static_check_bundle_size (STC-PERF01)	PASS — sums gzipped JS in dist/assets/, asserts ≤ 2 097 152 bytes	—
AZ-479	AC-2 (NFT-RES-LIM-04 mission-planner exclusion)	scripts/run-tests.sh static_check_dist_no_mission_planner (STC-S5, pre-existing)	PASS	—
AZ-479	AC-3 (NFT-PERF-10 FCP /flights ≤ 3 s)	e2e/tests/perf_fcp.e2e.ts	gated — runs in suite-e2e profile only; chromium-only; warmup + 5 measurement runs; median asserted ≤ 3000 ms	—
AZ-479	AC-4 (NFT-RES-LIM-05 30-min annotation soak)	e2e/tests/perf_annotation_memory_soak.e2e.ts	gated — RUN_LONG_RUNNING=1 chromium-only; baseline at t=60 s, final at t=1800 s, ≤ 1.10× growth	—

Every AC has at least one assertion; every documented drift is paired with a control PASS test that pins the current production drift (so the drift is observable today and the contract test flips automatically once the production fix lands).

Phase 3 — Test Coverage Hygiene

• 3 fast files / 5 e2e files / 1 static-runner edit / 0 production-source files modified.
• Total fast tests added: 25.
  • AZ-471: 15 (1 + 8 sub-tests + 1 + 1 + 1 + 3 controls/setup variants).
  • AZ-473: 5 (1 + 1 + 3 — one per mode P=0/20/40).
  • AZ-478: 7 (3 fail-cases + 3 control snapshots + 1 service-worker check).
• Total e2e tests added: 8 across 5 files (AZ-471 manual draw; AZ-473 yoloId × 3 modes; AZ-478 offline boot + SSE disconnect; AZ-479 FCP + annotation soak).
• 1 new static check added (STC-PERF01); existing STC-S5 mission-planner exclusion covers AZ-479 AC-2.
• All it.fails() and test.fail placements paired with a control test or with explanatory comments documenting the drift and the condition that flips them green. No it.skip is used to hide a failure.

Phase 4 — Hygiene & Drift

• 0 files added to src/ — production code untouched (pure blackbox test batch).
• 0 files added to _docs/ (no new lessons surfaced from this batch — the existing LESSONS.md URL-stub entry was already followed in tests/network_resilience.test.tsx's tainted-canvas test, validating the rule).
• The tests/setup.ts MSW boundary (onUnhandledRequest: 'error') is preserved — every new test seeds its own handlers explicitly (e.g., tests/canvas_editor.test.tsx adds the /api/admin/auth/refresh handler in beforeEach so the AuthProvider mount does not surface as an unhandled MSW request).
• tests/network_resilience.test.tsx installs scoped process.on('unhandledRejection') handlers around AC-2 and AC-3 that match ONLY the expected drift signatures (SecurityError from toBlob and the auth refresh failure on offline boot). Any other rejection still throws.
• The new static_check_bundle_size function in scripts/run-tests.sh mirrors the gzip + find + awk pattern that scripts/run-performance-tests.sh already uses for the same threshold — no new measurement methodology, just a different gate point so every commit is checked instead of only the on-demand perf script.

Phase 5 — Static + Lint

• bun run test:fast — 25 files / 150 passed / 13 skipped / 13.77 s wall.
• ./scripts/run-tests.sh --static-only — 25 / 25 static checks PASS / 12.14 s wall (added STC-PERF01; no other regressions). Build succeeded; gzipped initial JS bundle currently fits under the 2 MB budget.
• ReadLints clean on all 9 changed files.
• tsc --noEmit -p tsconfig.test.json succeeded as part of STC-T1.
• Standalone bunx tsc --noEmit against the 5 new e2e files (out-of-tree of tsconfig.test.json) also clean.

Phase 6 — Self-Review

• Test rigs re-read end-to-end for naming clarity, AAA shape, and proper teardown of every globally mutated handle (HTMLElement.prototype.clientWidth/Height, Element.prototype.getBoundingClientRect, requestAnimationFrame, URL.createObjectURL/revokeObjectURL, HTMLCanvasElement.prototype.{getContext,toBlob}, globalThis.Image, globalThis.EventSource, navigator.serviceWorker, process.on('unhandledRejection')).
• The canvas-spy in tests/canvas_editor.test.tsx captures lineWidth along with each strokeRect call so the selection-ring contract for AC-3 multi-select is observable from a pure JSDOM canvas mock without inspecting React state.
• tests/photo_mode.test.tsx AC-3 reuses the AC-1/AC-5 canvas mocks from AZ-471 to drive a draw inside <AnnotationsPage>, then asserts the wire payload — same fixture surface, no duplication of canvas-instrumentation logic.
• e2e/tests/perf_fcp.e2e.ts issues one warmup navigation (recorded as fcp-warmup-ms, not gated) and 5 measured runs; median is taken from the sorted list. The annotation channel makes the result self-explanatory in CI logs without parsing test names.
• e2e/tests/perf_annotation_memory_soak.e2e.ts reads performance.memory.usedJSHeapSize at t=60 s and t=1800 s, asserts the ratio is in (0.4, 1.10]. The lower bound flags a suspicious GC reclaim that would otherwise trivially "pass" the upper bound — it does not block on noise.
• Long comments in every test body explain why each it.fails() / test.fail exists and what condition will flip it green; future readers can tell intentional-drift from regression at a glance.

Phase 7 — Architecture Compliance

• No layer-direction violations. Tests are leaves of the import graph; they import production sources but no production source imports them.
• No new cyclic dependencies (verified via tsc --noEmit and bun run build in the static profile).
• src/features/annotations/CanvasEditor.tsx, src/components/DetectionClasses.tsx, src/features/annotations/AnnotationsPage.tsx, src/api/sse.ts, src/auth/AuthContext.tsx, and the SSE consumers are all exercised but not modified.
• New static check STC-PERF01 runs after STC-B1 (vite build) in the same run-tests.sh block, so the build is a precondition by ordering — no separate trigger needed.
• STC-S6 (no WS/GraphQL/gRPC/SSR deps), STC-S13 (no client-side persistence libs), STC-N3 (no service worker registration) all re-confirm.

Summary

PASS — the batch lands four blackbox-test tasks (15 ACs total) with zero production-code edits, every drift paired with a runnable control test, full static + fast suite green, and one new commit-time static gate (STC-PERF01) that promotes the bundle-size budget from on-demand perf script to the per-commit lane.