azaion/ui
1
0
Fork 0
mirror of https://github.com/azaion/ui.git synced 2026-06-21 10:31:10 +00:00
Code Issues Packages Projects Releases Wiki Activity

[AZ-460] [AZ-462] [AZ-466] [AZ-475] Batch 4 - destructive UX/forms/overlay/save

Browse Source 
AZ-466 — Destructive UX policy + ConfirmDialog a11y + no-alert (4pts):
  src/components/ConfirmDialog.test.tsx (8 fast),
  tests/destructive_ux.test.tsx (4 fast, AdminPage class-delete drift),
  e2e/tests/destructive_ux.e2e.ts. New static checks STC-SEC7 (alert
  allowlist) + STC-SEC8 (destructive-surfaces gated/drift) wired through
  scripts/check-banned-deps.mjs reading tests/security/banned-deps.json.

AZ-475 — Numeric form input rejection (2pts):
  tests/form_hygiene.test.tsx (3 fast). Documents two SettingsPage drifts:
  silent zero coercion via parseInt(v)||0 and labels missing htmlFor.

AZ-462 — Overlay membership at in-window edges (2pts):
  tests/overlay_membership.test.tsx (6 fast). Documents getTimeWindowDetections
  strict < drift; AC-1 boundary tests are it.fails(); AC-2 / control PASS.
  Mocks HTMLCanvasElement.getContext to capture strokeRect.

AZ-460 — Annotation save URL + payload contract (2pts):
  tests/annotations_endpoint.test.tsx (6 fast),
  e2e/tests/annotations_endpoint.e2e.ts. AC-1 URL canary PASSes; AC-2
  payload missing 4 fields documented as it.fails(); AC-3 manual-draw
  PASS, AI-suggestion-accept + bulk-edit-save QUARANTINE skip.

Test infrastructure:
  - tests/setup.ts: NoopResizeObserver + NoopEventSource JSDOM polyfills.
  - tests/msw/handlers/annotations.ts: doubly-prefixed paths matching
    production calls (e.g. /api/annotations/annotations).
  - tests/msw/handlers/flights.ts: plural /aircrafts paths.

Verification: bun run test:fast → 80 passed, 13 skipped (14 files).
scripts/run-tests.sh --static-only → 24/24 PASS (was 22; +STC-SEC7/SEC8).
Per-batch self-review verdict: PASS_WITH_WARNINGS. Cumulative review
of batches 04-06 due after batch 6 per implement/SKILL.md Step 14.5.
Report: _docs/03_implementation/batch_04_report.md.

Also includes the previously-untracked
_docs/03_implementation/cumulative_review_batches_01-03_report.md
generated at the start of this session before batch 4 began.

Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
2026-05-11 04:15:01 +03:00
parent 2051088706
commit 1dd25edee3
20 changed files with 1812 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files
_docs/03_implementation/batch_04_report.md
+228
View File
@@ -0,0 +1,228 @@
# Batch Report
**Batch**: 04
**Tasks**: AZ-466 (Destructive UX policy + ConfirmDialog + no-alert), AZ-475 (Numeric form hygiene), AZ-462 (Overlay window membership), AZ-460 (Annotation save URL + payload contract)
**Date**: 2026-05-11
**Cycle**: Phase A baseline, Step 6 — Implement Tests
**Total complexity**: 10 pts (4 + 2 + 2 + 2)
## Task Results
| Task | Status | Files Modified | Tests | AC Coverage | Issues |
|------|--------|---------------|-------|-------------|--------|
| AZ-466_test_destructive_ux | Done | 2 created (1 ConfirmDialog unit + 1 cross-component); 1 e2e created; 1 modified (`tests/security/banned-deps.json` adds `alert_calls` + `destructive_surfaces`); 1 modified (`scripts/check-banned-deps.mjs` + `scripts/run-tests.sh` add STC-SEC7 / STC-SEC8) | 8 fast `ConfirmDialog.test.tsx` (7 pass, 1 skipped); 4 fast `tests/destructive_ux.test.tsx` (3 pass + 1 skip QUARANTINE incl. 2 `it.fails()`); 2 e2e `e2e/tests/destructive_ux.e2e.ts` (both `test.fail`); 2 new static checks (PASS) | 5 / 5 ACs covered | 5 documented drifts: ConfirmDialog missing 4 a11y attrs (`role="dialog"`, `aria-modal`, `aria-labelledby`, `aria-describedby`); no focus trap; AdminPage class-delete bypasses ConfirmDialog (file in `destructive_surfaces.drift`); `alert()` allowlist seeded with 4 production callsites (Phase B drains it) |
| AZ-475_test_form_hygiene | Done | 1 created (`tests/form_hygiene.test.tsx`) | 3 fast (2 pass, including 1 control + 1 `it.fails()` per AC) | 2 / 2 ACs covered | 2 documented drifts: `<label>` lacks `htmlFor`; `parseInt(v) \|\| 0` silently coerces empty/non-numeric to 0 and PUTs |
| AZ-462_test_overlay_membership | Done | 1 created (`tests/overlay_membership.test.tsx`) | 6 fast (5 pass, including 2 `it.fails()` for AC-1 inclusive boundary) | 3 / 3 ACs covered | 1 documented drift: `getTimeWindowDetections` uses strict `<` instead of `<=`; AC-1 boundary tests are `it.fails()` until production lifts the operator |
| AZ-460_test_annotations_endpoint | Done | 1 created (`tests/annotations_endpoint.test.tsx`); 1 e2e created (`e2e/tests/annotations_endpoint.e2e.ts`); 1 modified (`tests/msw/handlers/annotations.ts` doubly-prefixed paths); 1 modified (`tests/msw/handlers/flights.ts` plural `aircrafts` paths) | 6 fast (4 pass, 2 skipped QUARANTINE, including 1 `it.fails()` for AC-2 payload shape); 3 e2e (1 skip-on-no-seed, 2 `test.fail` for AC-2) | 3 / 3 ACs covered | 2 documented drifts: save body sends only `{mediaId, time, detections}` instead of the 6-field wire contract `{Source, WaypointId, videoTime, mediaId, detections, status}`; AI-suggestion-accept and bulk-edit-save entry points wholly absent in production (`it.skip` QUARANTINE) |
## AC Test Coverage: All covered (13 / 13 ACs across the four tasks)
### AZ-466 — Destructive UX policy (5 ACs, 14 scenarios)
| Scenario | Where | Profile | Status |
|----------|-------|---------|--------|
| AC-1 / FT-P-04 (ConfirmDialog `role="dialog"` + aria-modal) | `src/components/ConfirmDialog.test.tsx` | fast | `it.fails()` — both attrs missing |
| AC-1 / FT-P-05 (ConfirmDialog labeled by title via aria-labelledby + described by message) | same | fast | `it.fails()` — neither attr today |
| AC-1 / FT-P-06 (Escape key closes dialog) | same | fast | PASS — production already calls onClose on Escape |
| AC-1 / focus-trap (Tab cycles within dialog) | same | fast | `it.skip` QUARANTINE — no trap implemented |
| AC-1 / control: dialog renders (positive sanity) | same | fast | PASS |
| AC-1 / control: confirm/cancel callbacks fire | same | fast | PASS |
| AC-1 / control: hidden when closed | same | fast | PASS |
| AC-2 / FT-P-26 (Delete → Confirm → DELETE fires) | `tests/destructive_ux.test.tsx` + `e2e/tests/destructive_ux.e2e.ts` | fast + e2e | `it.fails()` (fast) + `test.fail` (e2e) — AdminPage bypasses ConfirmDialog |
| AC-2 / FT-N-07 (Delete → Cancel → no DELETE) | same | fast + e2e | `it.fails()` + `test.fail` |
| AC-2 / control: production today deletes immediately | `tests/destructive_ux.test.tsx` | fast | PASS — pins drift |
| AC-3 / no `alert()` outside allowlist | `scripts/run-tests.sh::STC-SEC7` → `check-banned-deps.mjs --kind=alert_calls` | static | PASS (allowlist enforced; new alerts FAIL) |
| AC-4 / FT-P-27 (every destructive surface gated or in drift list) | `STC-SEC8` → `--kind=destructive_surfaces` | static | PASS (3 files: 2 gated, 1 drift) |
| AC-4 / runtime mirror (one example via class-delete) | `tests/destructive_ux.test.tsx` | fast | covered by AC-2 above |
| AC-5 / NFT-SEC-07 (no `alert()` in `src/`) | `STC-SEC7` (allowlist) | static | PASS — static check is the gating signal |
**AC summary**:
- AC-1 ConfirmDialog a11y → 4 `it.fails()` + 1 `it.skip` + 4 controls; FT-P-06 (Escape) PASS.
- AC-2 Delete-confirm-cancel happy path → `it.fails()` + control + e2e companion (`test.fail`).
- AC-3 / AC-5 No `alert()` → STC-SEC7 with 4-entry allowlist (Phase B drains).
- AC-4 Destructive surfaces enumeration → STC-SEC8 file-level heuristic (3 files: `MediaList.tsx` and `FlightsPage.tsx` gated; `AdminPage.tsx` in drift).
### AZ-475 — Numeric form input rejection (2 ACs, 3 scenarios)
| Scenario | Where | Profile | Status |
|----------|-------|---------|--------|
| AC-1 / FT-N-11 (clear → validation error + no PUT) | `tests/form_hygiene.test.tsx` | fast | `it.fails()` — silent zero today |
| AC-1 / control: production silently coerces empty input to 0 and PUTs | same | fast | PASS — pins drift |
| AC-2 / FT-N-12 (non-numeric → validation error + no PUT) | same | fast | `it.fails()` — same coercion path |
**AC summary**:
- AC-1 Empty input rejection → `it.fails()` + control proving `defaultCameraWidth: 0` PUTs today.
- AC-2 Non-numeric rejection → `it.fails()` (the `<input type="number">` path swallows non-numeric chars; the helper sets the value via dispatchEvent to force the React state).
### AZ-462 — Overlay membership at in-window edges (3 ACs, 6 scenarios)
| Scenario | Where | Profile | Status |
|----------|-------|---------|--------|
| AC-1 / FT-P-14 (annotation EXACTLY on lower bound IS rendered) | `tests/overlay_membership.test.tsx` | fast | `it.fails()` — strict `<` excludes boundary |
| AC-1 / FT-P-15 (annotation EXACTLY on upper bound IS rendered) | same | fast | `it.fails()` — same drift |
| AC-1 / control: strict `<` excludes the boundary today | same | fast | PASS — pins drift |
| AC-2 / FT-N-01 (annotation BEFORE lower bound NOT rendered) | same | fast | PASS |
| AC-2 / FT-N-02 (annotation AFTER upper bound NOT rendered) | same | fast | PASS |
| AC-2 / positive control: annotation INSIDE the window IS rendered | same | fast | PASS — proves test apparatus would observe a render |
**AC summary**:
- AC-1 Inclusive boundary → 2 `it.fails()` + control proving exclusion today.
- AC-2 Strict exclusion outside the window → 2 PASS + positive control (apparatus sanity).
- AC-3 Canvas-output assertion (not React state) → satisfied by mocking `HTMLCanvasElement.prototype.getContext` to capture every `strokeRect` call.
### AZ-460 — Annotation save URL + payload contract (3 ACs, 6 scenarios)
| Scenario | Where | Profile | Status |
|----------|-------|---------|--------|
| AC-1 / FT-P-07 (URL canary: `/api/annotations/annotations`) | `tests/annotations_endpoint.test.tsx` + `e2e/tests/annotations_endpoint.e2e.ts` | fast + e2e | PASS (fast) — production already POSTs the doubly-prefixed URL; e2e gated by suite stack |
| AC-2 / FT-P-08 (required-fields: Source, WaypointId, videoTime, mediaId, detections, status) | same | fast + e2e | `it.fails()` + `test.fail` — production sends only `{mediaId, time, detections}` |
| AC-2 / control: production sends partial body (`{mediaId, detections}`) | `tests/annotations_endpoint.test.tsx` | fast | PASS — pins drift |
| AC-3 / manual-draw / select-existing entry point | same + e2e | fast + e2e | PASS — exercises the only wired entry point |
| AC-3 / AI-suggestion-accept entry point | same | fast | `it.skip` QUARANTINE — no production path today |
| AC-3 / bulk-edit-save entry point | same | fast | `it.skip` QUARANTINE — no production path today |
**AC summary**:
- AC-1 URL canary → PASS for the only wired save path; e2e companion gated.
- AC-2 Required fields → `it.fails()` for the missing 4 fields; control pins the partial-body drift.
- AC-3 Multi-entry-point coverage → 1 PASS for manual-draw + 2 `it.skip` QUARANTINE for unimplemented paths (test shape documented in skip comments).
## Code Review Verdict: PASS_WITH_WARNINGS
Self-review walked inline per `.cursor/skills/code-review/SKILL.md` phases 1–7.
- **Phase 1 (Context)**: 4 task specs re-read; `_docs/02_document/module-layout.md` Blackbox Tests envelope respected; reuses helpers from AZ-456 (`tests/helpers/{render,auth}.ts`) and fixtures (`seed_users`, `seed_flights`). No new shared helpers introduced — the form-hygiene file inlines a small `inputForLabel(...)` DOM-traversal helper because SettingsPage's labels lack `htmlFor` (drift documented in the test header).
- **Phase 2 (Spec compliance)**: every AC across the four task specs has at least one test (running, `it.fails()`, or `it.skip` with QUARANTINE reason). Drift handling uniform with batches 1–3: `it.fails()` for documented production drift (attribute/operator/payload-field exists in spec but absent in code); `it.skip` for behavior wholly absent (AI-suggestion-accept save, bulk-edit save, focus trap inside ConfirmDialog).
- **Phase 3 (Code quality)**: `check-banned-deps.mjs`'s new `checkDestructiveSurfaces` is a single function with one responsibility (file-level heuristic comparing `gated` ∪ `drift` against the live filesystem); `tests/security/banned-deps.json` `alert_calls` and `destructive_surfaces` sections each have an `ac:` field, a `scope:` field, an explicit `match:` description, and inline `$*_comment` hooks for code review; the test files use Arrange/Act/Assert structure consistently; no bare `catch` blocks; no error suppression.
- **Phase 4 (Security)**: no new secrets in test fixtures (reuses AZ-457's `test-bearer-default`); the AZ-466 changes strengthen security posture (every `alert()` and every destructive surface is now allowlisted and code-review-visible); the new static checks fail-closed on additions; the `check-banned-deps.mjs` walks files and runs ripgrep / regex over them — no execution of test inputs.
- **Phase 5 (Performance)**: fast suite **5.5 s wall-clock** for 80 + 13-skipped tests across 14 files (was 4.4 s for 57 + 9 skipped in batch 3 — +1.1 s for 23 new tests, well under the 5 min budget). Static profile **~16 s** for 24 checks (was 12 s for 22 in batch 3; +4 s primarily from the two new STC-SEC7 / STC-SEC8 checks reading `tests/security/banned-deps.json`). The `it.fails()` tests each consume ~1 s waiting for the assertion to NOT match — same shape as batches 1–3, acceptable.
- **Phase 6 (Cross-task consistency)**: the four tasks touch **disjoint** subsystems (ConfirmDialog + AdminPage destructive UX vs SettingsPage form hygiene vs CanvasEditor overlay vs AnnotationsPage save). Shared surface = `tests/helpers/`, `tests/fixtures/`, `tests/msw/`, `tests/security/banned-deps.json` — all consumed read-only or strictly extended (new sections, never modifying existing ones). No contract collisions; no duplicate symbols.
- **Phase 7 (Architecture compliance)**:
- Test files import only public seams:
- `src/components/ConfirmDialog.test.tsx`: `ConfirmDialog` default export.
- `tests/destructive_ux.test.tsx`: `AdminPage` default export.
- `tests/form_hygiene.test.tsx`: `SettingsPage` default export.
- `tests/overlay_membership.test.tsx`: `CanvasEditor` default export + `AnnotationSource`/`AnnotationStatus`/etc. enums (public types).
- `tests/annotations_endpoint.test.tsx`: `AnnotationsPage` default export + `FlightProvider` (public symbol on `FlightContext.tsx`) + public enums.
- No imports of `*.internal.*` files, no reaching into other components' private files.
- E2E tests don't import any production modules — Playwright primitives only (consistent with batches 1–3).
- No new cyclic module dependencies introduced.
- Test setup: `tests/setup.ts` gained two no-op JSDOM polyfills (`ResizeObserver` and `EventSource`). These are environment polyfills (not production code workarounds), and per-test installations of richer stubs (e.g. `tests/sse_lifecycle.test.tsx`'s EventSource fake) override + restore — verified by re-running batch 3's SSE suite alongside the new tests with no regressions.
### Findings
1. **Low / Maintainability / Drift** — AZ-466 AC-1 four ConfirmDialog a11y attributes (`role="dialog"`, `aria-modal`, `aria-labelledby`, `aria-describedby`) are missing today; FT-P-04 / FT-P-05 are `it.fails()`. The Escape handler exists (FT-P-06 PASSes), but no focus trap (`it.skip` QUARANTINE). **Recommendation**: file `feat(confirm-dialog): a11y attrs + focus trap` in Phase B. Touches one file (`src/components/ConfirmDialog.tsx`); should also localize the title via `t()` if the existing copy is hard-coded.
2. **Low / Maintainability / Drift** — AZ-466 AC-4 `AdminPage.handleDeleteClass` calls `api.delete` without ConfirmDialog. The file is recorded in `tests/security/banned-deps.json::destructive_surfaces.drift` to keep the static check passing while making the gap visible in code review. **Recommendation**: `feat(admin): gate class-delete via ConfirmDialog` — moves `src/features/admin/AdminPage.tsx` from `drift` to `gated` and flips FT-P-26 / FT-N-07 from `it.fails()` to PASS.
3. **Low / Maintainability / Drift** — AZ-466 AC-3 / AC-5 `alert()` allowlist contains 4 callsites (`MediaList.tsx`, `FlightsPage.tsx`, `JsonEditorDialog.tsx`, `flightPlan.tsx`). Each is a per-feature blocker dialog or validation message that should migrate to a non-blocking toast or an inline error. **Recommendation**: 4 small Phase B tasks (one per file), each removing one allowlist entry — measurable progress.
4. **Low / Maintainability / Drift** — AZ-475 AC-1 silent-zero coercion AND `<label>` without `htmlFor`. Two related drifts in the same file (`SettingsPage.tsx`). **Recommendation**: combined Phase B task `feat(settings): numeric input validation + label association` that lands a `useNumericField()` hook (or equivalent) and adds `id`/`htmlFor` so screen readers and `getByLabelText` both work.
5. **Low / Maintainability / Drift** — AZ-462 AC-1 strict `<` in `getTimeWindowDetections` → boundary annotations are dropped. **Recommendation**: one-character production change (`<` → `<=`) + flip FT-P-14/15 from `it.fails()` to PASS. Confirm with the suite annotations service that `lowerBound` and `upperBound` are inclusive on the wire.
6. **Low / Architecture / Drift** — AZ-460 AC-2 save body shape (4 missing fields). The fields touch the wire contract; the suite annotations service must be checked to see what it expects today. **Recommendation**: a Phase B task `feat(annotations-save): emit Source/WaypointId/videoTime/status` that lifts the body shape. May require a coordinated change with the annotations service if the server today happily accepts the partial body.
7. **Low / Architecture / Drift** — AZ-460 AC-3 only one save entry point exists. The AI-suggestion-accept and bulk-edit-save paths are documented in `it.skip` QUARANTINE comments with the test shape they should take when the production paths land. **Recommendation**: 2 Phase B feature tasks (AI-accept, bulk-edit) — the test side is ready to be activated by removing the `.skip`.
8. **Low / Architecture / Drift (test infrastructure)** — `tests/msw/handlers/annotations.ts` and `tests/msw/handlers/flights.ts` both gained doubly-prefixed / plural paths (`/api/annotations/annotations`, `/api/flights/aircrafts`) to match what production callers actually use. The single-prefix paths are kept for backward compatibility with batch 1–3 tests. **Recommendation**: Phase B tracker entry `chore(test-infra): drop the single-prefix annotation/flight paths` once production has been confirmed to use only the doubly-prefixed/plural shapes everywhere.
9. **Low / Architecture / Drift (test infrastructure)** — `tests/msw/handlers/admin.ts` `/api/admin/users` returns `paginate(seedUsers)` while `AdminPage` reads it as a flat `User[]`. The destructive-UX test override returns `[]` (flat) to keep AdminPage from crashing. **Recommendation**: confirm whether the suite admin service emits a flat array or a paginated payload, then align the MSW default with production. Either way, file as `chore(admin-handler): align msw with prod /admin/users shape`.
10. **Low / Architecture / Interpretation (carried over from batches 1–3)** — Test helpers (`tests/helpers/{render,auth,sse-mock}.ts`) and the polyfills in `tests/setup.ts` import / patch production accessors. Reaffirmed per the batch-1 / 2 / 3 rule: "Black-box discipline applies to test bodies, not to test setup helpers / composition-root wrappers / consumer-pattern mirrors". The polyfills are JSDOM environment plumbing (no-op stubs for browser APIs JSDOM doesn't ship), not production-code workarounds.
## Auto-Fix Attempts: 0
## Stuck Agents: None
## Files Changed (10)
### Created — `src/` (1)
```
src/components/ConfirmDialog.test.tsx # AZ-466 fast — 8 tests (1 skipped)
```
### Created — `tests/` (3)
```
tests/destructive_ux.test.tsx # AZ-466 fast — 4 tests (1 skipped)
tests/form_hygiene.test.tsx # AZ-475 fast — 3 tests
tests/overlay_membership.test.tsx # AZ-462 fast — 6 tests
tests/annotations_endpoint.test.tsx # AZ-460 fast — 6 tests (2 skipped)
```
### Created — `e2e/tests/` (2)
```
e2e/tests/destructive_ux.e2e.ts # AZ-466 e2e — 2 scenarios (both test.fail)
e2e/tests/annotations_endpoint.e2e.ts # AZ-460 e2e — 3 scenarios (1 skip-on-no-seed, 1 test.fail)
```
### Modified (5)
```
tests/setup.ts # JSDOM polyfills: NoopResizeObserver, NoopEventSource
tests/security/banned-deps.json # New sections: alert_calls (4-entry allowlist) + destructive_surfaces (2 gated, 1 drift)
scripts/check-banned-deps.mjs # New checkDestructiveSurfaces; allowlist support in checkSourceTree; main() routing
scripts/run-tests.sh # Add STC-SEC7 (no-alert) + STC-SEC8 (destructive surfaces)
tests/msw/handlers/annotations.ts # Add doubly-prefixed annotation/settings/classes handlers (production shape)
tests/msw/handlers/flights.ts # Add plural /api/flights/aircrafts handlers (production shape)
_docs/_autodev_state.md # Batch 4 sub_step pointer + notes
```
(File count = 4 created in `tests/` + 1 created in `src/` + 2 created in `e2e/tests/` + 5 modified + 2 MSW handlers modified = 14 file touches; uniqueness count is 12 — `tests/msw/handlers/annotations.ts` and `tests/msw/handlers/flights.ts` are extensions of existing files.)
## Verification Run (host)
```
$ bun run test:fast
✓ tests/infrastructure.test.ts (5 tests) 53ms
✓ src/api/client.test.ts (9 tests) 61ms
✓ tests/sse_lifecycle.test.tsx (9 tests | 1 skipped) 74ms
✓ src/auth/AuthContext.test.tsx (4 tests) 249ms
✓ src/components/Header.test.tsx (6 tests | 1 skipped) 302ms
✓ src/components/ConfirmDialog.test.tsx (8 tests | 1 skipped) 285ms
✓ tests/wire_contract.test.ts (11 tests | 2 skipped) 8ms
✓ tests/i18n.test.tsx (4 tests | 2 skipped) 4ms
✓ tests/annotations_endpoint.test.tsx (6 tests | 2 skipped) 523ms
✓ src/auth/ProtectedRoute.test.tsx (12 tests | 3 skipped) 1101ms
✓ mission-planner/src/test/jsonImport.test.ts (6 tests) 5ms
✓ tests/overlay_membership.test.tsx (6 tests) 2137ms
✓ tests/form_hygiene.test.tsx (3 tests) 2351ms
✓ tests/destructive_ux.test.tsx (4 tests | 1 skipped) 2342ms
Test Files 14 passed (14)
Tests 80 passed | 13 skipped (93)
$ ./scripts/run-tests.sh --static-only
[run-tests] static profile PASSED — 24/24 checks (was 22 in batch 3; +2 from batch 4: STC-SEC7, STC-SEC8)
$ ./scripts/run-tests.sh
[run-tests] static profile : ran (PASS)
[run-tests] fast profile : ran (PASS)
[run-tests] e2e profile : skipped (host)
[run-tests] exit code : 0
```
E2E profile not exercised in this batch — same Risk 4 as batches 1–3 (requires `docker compose -f e2e/docker-compose.suite-e2e.yml up -d` plus parent-suite `:test` images). The new e2e companion files (`e2e/tests/destructive_ux.e2e.ts`, `e2e/tests/annotations_endpoint.e2e.ts`) will run on the suite stack.
## Next Batch
Remaining: **14 test-implementation tasks** in `_docs/02_tasks/todo/`:
- AZ-461 (detection endpoints sync/async/long-video, 2pts)
- AZ-463 (flight selection persistence + memory soaks, 3pts)
- AZ-464 (bulk-validate URL + body + UI sync, 2pts)
- AZ-469 (browser support + responsive variants, 2pts)
- AZ-470 (panel-width debounced PUT + rehydration, 2pts)
- AZ-471 (CanvasEditor draw/resize/multi-select/zoom/pan, 5pts)
- AZ-472 (DetectionClasses load + hotkeys + click + fallback, 3pts)
- AZ-473 (PhotoMode switch + auto-select + yoloId wire, 2pts) — soft dep on AZ-472
- AZ-474 (Tile-split + YOLO parser + auto-zoom + indicator, 3pts)
- AZ-476 (Upload 501 MB → 413 → user-visible error, 2pts)
- AZ-477 (Settings save 500/network resilience, 3pts)
- AZ-478 (Network offline + SSE disconnect + tainted-canvas, 3pts)
- AZ-479 (Bundle ≤2 MB + mission-planner excluded + FCP + soak, 3pts)
- AZ-480 (Prod image nginx:alpine + 500M + 9 routes + edge RAM, 3pts)
All carry **Component**: `Blackbox Tests` and **Dependencies**: `AZ-456` (✓ done). Soft cross-dep: AZ-473 needs AZ-472's DetectionClasses fixtures.
Suggested next batch (4 tasks, ~9 pts, dependency-disjoint at the file level): AZ-461 (detection endpoints, 2pts); AZ-464 (bulk-validate URL/body/sync, 2pts); AZ-470 (panel-width debounced PUT, 2pts); AZ-472 (DetectionClasses load + hotkeys, 3pts). Together they touch the detect/ endpoints, bulk dataset endpoints, useResizablePanel hook, and the DetectionClasses component — disjoint at the file level.
A cumulative cross-batch review (batches 04–06) is due **after batch 6** per `implement/SKILL.md` Step 14.5 (every 3 batches). Today's per-batch self-review is recorded above; the cumulative pass will compare batches 04–06 against architecture findings F1–F9 (the same baseline used by the batches 01–03 cumulative review).
Recommendation: continue in a new conversation. Batch 4 added 6 new files + 2 new static checks + 23 new fast tests + 2 new e2e files; the next batch will load distinct task specs (detect endpoints, bulk-validate, resizable-panel, DetectionClasses).
_docs/03_implementation/cumulative_review_batches_01-03_report.md
+200
View File
@@ -0,0 +1,200 @@
# Cumulative Code Review Report
**Batches**: 01–03 (AZ-456, AZ-457/459/465/481, AZ-458/467/468/482)
**Date**: 2026-05-11
**Cycle**: Phase A baseline, Step 6 — Implement Tests
**Mode**: cumulative (`/code-review` cumulative mode, all 7 phases)
**Trigger**: implement skill Step 14.5 — every K=3 batches
**Scope (changed files since baseline `729ad1c`)**: 60 paths
- `tests/**` (33 created): MSW server + 9 handler files, 8 fixture files, 4 helper files, 5 test files (`infrastructure`, `wire_contract`, `i18n`, `sse_lifecycle`), `setup.ts`, `i18n-allowlist.json`, `security/banned-deps.json`
- `src/**` (4 created): `api/client.test.ts`, `auth/AuthContext.test.tsx`, `auth/ProtectedRoute.test.tsx`, `components/Header.test.tsx`
- `e2e/**` (15 created): `playwright.config.ts`, `docker-compose.suite-e2e.yml`, OWM + tile stubs (Dockerfile + server), runner Dockerfile + entrypoint, fixture SQL, 5 e2e test files
- `scripts/**` (3 created + 2 modified): `check-banned-deps.mjs`, `check-i18n-coverage.mjs`, `check-ci-image-labels.mjs`; modified `run-tests.sh` and `run-performance-tests.sh`
- root config (3 created + 3 modified): `vitest.config.ts`, `tsconfig.test.json`, `tests/security/banned-deps.json` source-of-truth; modified `package.json`, `bun.lock`, `tsconfig.json`
**Verdict**: **PASS_WITH_WARNINGS**
---
## Phase 1 — Context
Inputs re-read:
- Task specs in current cycle's done/: AZ-456, AZ-457, AZ-458, AZ-459, AZ-465, AZ-467, AZ-468, AZ-481, AZ-482
- `_docs/02_document/architecture.md` + Architecture Vision (P1–P12)
- `_docs/02_document/module-layout.md` (`Blackbox Tests` envelope, the `Imports from` clarification commit `496b089`)
- `_docs/02_document/architecture_compliance_baseline.md` (F1–F9)
- `_docs/00_problem/restrictions.md`, `_docs/01_solution/solution.md`
- All three batch reports (`batch_01_report.md`, `batch_02_report.md`, `batch_03_report.md`)
## Phase 2 — Spec Compliance
Per-batch coverage already verified inline. Aggregated:
- AZ-456: 8/8 ACs
- AZ-457: 4/4 ACs (FT-P-01 / NFT-SEC-04 with `it.fails()` drift carry-overs)
- AZ-458: 3/3 ACs (AC-2 bearer rotation + annotation-status SSE drifts; e2e gated)
- AZ-459: 4/4 ACs (`it.fails()` for the 3 documented enum drifts; `verification_pending` skips for CombatReadiness + MediaType value-set)
- AZ-465: 4/4 ACs (FT-P-24/25 quarantined — detector + persistence not in production yet)
- AZ-467: 4/4 ACs (FT-P-32 spinner a11y `it.fails()`; FT-P-33 timeout + FT-N-03/05 RBAC `it.skip` quarantine)
- AZ-468: 3/3 ACs (FT-P-30/31 `it.fails()`; FT-N-09 `it.skip` quarantine)
- AZ-481: 3/3 ACs (image.title DRIFT reported; not blocking)
- AZ-482: 6/6 ACs (all PASS — deny-list checker is future-proofing)
**Total: 39/39 ACs covered**, with explicit drift / quarantine markers on every gap. No silent fail.
No `## Contract` sections in the test specs (test tasks consume contracts but don't redefine them); contract verification is delegated to AZ-459 (enum spec snapshot) and exercised in `tests/wire_contract.test.ts`.
## Phase 3 — Code Quality
Spot-checks across the new test infrastructure:
- Helper functions each carry a single responsibility — `seedBearer/clearBearer` (token state), `seedNavigateToLogin` (login redirect spy), `renderWithProviders` (composition root for tests), `createFakeEventSource/simulateSseStream` (SSE doubles), `jsonResponse/paginate/sse` (MSW response shorthand). Names match what each function does.
- No bare `catch` / `try` swallowing across new files.
- Arrange / Act / Assert pattern preserved across all `*.test.{ts,tsx}` files (verified via spot-check in `AuthContext.test.tsx`, `client.test.ts`, `wire_contract.test.ts`, `sse_lifecycle.test.tsx`, `Header.test.tsx`, `ProtectedRoute.test.tsx`).
- Test files do not narrate trivial code in comments; comments are reserved for `it.fails()` drift rationale and `it.skip` quarantine reason — both required for traceability.
- No `console.log` / `console.error` left in test bodies (only in `tests/setup.ts` for MSW logger config).
- Test helpers do not import each other circularly; helpers form a flat dependency tree (`render` → `i18n`, `auth` → `client`, `navigate` → `client`, `sse-mock` standalone).
No Phase 3 findings.
## Phase 4 — Security Quick-Scan
- No real secrets in fixtures: `tests/fixtures/seed_users.ts` uses placeholder argon2 hashes; bearer tokens use the `'test-bearer-default'` constant; OWM and tile stub URLs are stub-only (`/_owm/_health`, `/_tile/...`).
- No `eval`, no `shell=True`, no `subprocess` in scripts beyond `bun`/`tsc`/`vite` invocations.
- The static check refactoring in batch 3 (`scripts/check-banned-deps.mjs`) reads the deny-list from `tests/security/banned-deps.json` — JSON-only data input, regex applied to file paths and contents. No execution of file contents. No shell metachars passed to `child_process` (the script uses `node:fs`).
- AZ-482 explicitly strengthens posture: SEC-09 (OWM key) now also enforced against `dist/`; SEC-13 catches dropped legacy integrations (WhatsApp/Telegram/D-Bus/libsignal); SEC-14 anti-criterion catches accidental concurrent-edit reconcile.
- `tests/setup.ts` opts MSW into `'error'` on unhandled requests — drift in test wiring fails loudly rather than silently masking production calls.
No Phase 4 findings.
## Phase 5 — Performance
Wall-clock progression (host runs):
| Batch | Fast tests | Fast wall-clock | Static checks | Static wall-clock |
|-------|-----------|-----------------|---------------|-------------------|
| 01 | 11 | ~3 s | 13 | ~26 s |
| 02 | 38 + 4 skipped | ~3 s | 19 | ~13 s |
| 03 | 57 + 9 skipped | ~4.4 s | 22 | ~12 s |
- Per-test wall-clock budget remains well under the 5-minute target (`solution.md` perf budget).
- The dominant cost is `STC-T1` (`tsc --noEmit`) + `STC-B1` (`vite build`) at ~8 s combined; both unchanged across batches.
- No new pathological patterns: no nested loops on per-test setup, no synchronous file I/O in test bodies, fixtures preloaded once per process.
- The MSW handler set has grown from 0 → 9 handler files; handlers are O(1) match by URL pattern (msw v2.x trie), no N+1 risk introduced.
No Phase 5 findings.
## Phase 6 — Cross-Batch Consistency
Key cumulative concern: helpers / fixtures / static-check IDs / handler routes must not collide or duplicate across batches.
**Symbol audit** (across all batches):
- `tests/helpers/auth.ts` — `seedBearer`, `clearBearer` (1 producer, 4 consumers: `client.test.ts`, `AuthContext.test.tsx`, `ProtectedRoute.test.tsx`, `Header.test.tsx`)
- `tests/helpers/navigate.ts` — `seedNavigateToLogin` (1 producer, 1 consumer: `client.test.ts`)
- `tests/helpers/render.tsx` — `renderWithProviders` + screen/waitFor re-exports (1 producer, 4 consumers)
- `tests/helpers/sse-mock.ts` — `createFakeEventSource`, `simulateSseStream` (1 producer, 1 consumer: `sse_lifecycle.test.tsx`)
- `tests/msw/server.ts` — `server` (1 producer, 5 consumers)
- `tests/msw/helpers.ts` — `jsonResponse`, `errorResponse`, `noContent`, `paginate`, `latency`, `sse`, `dropResponse` (1 producer, multi-consumer)
- `tests/fixtures/seed_users.ts` — `opAlice`, `opBob`, `adminCarol`, `integratorDave`, `seedUsers`, `seedPermissions` (1 producer, multi-consumer; the same four user objects are reused across `ProtectedRoute.test.tsx` and `Header.test.tsx` with consistent IDs/permissions — no divergent definitions)
- `tests/fixtures/seed_flights.ts` — `seedFlights`, `liveGpsFlightId` — used by `Header.test.tsx` and `sse_lifecycle.test.tsx` consistently
**No duplicate symbol** across batches. **No fixture redefinition** (no second `opAlice` with different role/permissions; no second `liveGpsFlightId` constant).
**Static check IDs** (22 across `scripts/run-tests.sh`):
`STC-S1, S2, S5, S6, S13, N2, N3, N4, N5, SEC1, SEC1B, SEC2, SEC3, SEC4, SEC13, SEC14, FN15, FP22, FP23, CI11, T1, B1` — all unique, none reused. Naming convention: `STC-<topic-prefix><number>` consistently applied.
**MSW handler routes** (9 handler files, ~50 routes total):
Each handler file owns a disjoint URL prefix (`/admin/...`, `/flights/...`, `/annotations/...`, `/detect/...`, `/loader/...`, `/resource/...`, `/_owm/...`, `/tiles/...`). No overlap; no duplicate route definitions. Spot-checked `index.ts` to confirm `defaultHandlers` is the union without duplicates.
**Drift handling pattern uniformity**:
- `it.fails()` — used when the production element exists but the asserted attribute / behavior is missing today (e.g., FT-P-01 `credentials: 'include'`, FT-P-30/31 dropdown a11y, FT-P-32 spinner a11y, AC-2 bearer rotation re-deps).
- `it.skip` + `// QUARANTINE: ...` — used when the production capability is wholly absent (FT-N-09 Escape handler, FT-P-33 timeout fallback, FT-N-03/05 RBAC, FT-P-09/10 annotation-status SSE, FT-P-24/25 i18n detector + persistence).
- Both patterns include a control test asserting the gap, so the absence is provably demonstrated rather than tacitly assumed.
This pattern is uniform across batches 1–3. The `verification_pending` skip in AZ-459 is a third pattern (`it.skip` for "spec is provisional") — consistent within its task.
No Phase 6 findings beyond the carried-over interpretation note (see Phase 7 / Findings below).
## Phase 7 — Architecture Compliance
**Per-import inspection of test files** (cross-component edges):
| Test file | Cross-component imports | Verdict |
|-----------|-------------------------|---------|
| `src/api/client.test.ts` | `tests/msw/server`, `tests/helpers/auth`, `tests/helpers/navigate` | OK — only test infrastructure |
| `src/auth/AuthContext.test.tsx` | `tests/msw/server`, `tests/helpers/render`, `src/api/client` (`api`, `getToken`, `setToken` — public testability accessors landed by AZ-454/Step 4), `tests/helpers/auth` | OK |
| `src/auth/ProtectedRoute.test.tsx` | `tests/msw/server`, `tests/msw/helpers`, `tests/helpers/render`, `tests/helpers/auth`, `tests/fixtures/seed_users` | OK |
| `src/components/Header.test.tsx` | `tests/msw/server`, `tests/msw/helpers`, `tests/helpers/render`, `tests/helpers/auth`, `tests/fixtures/seed_flights`, `tests/fixtures/seed_users` | OK |
| `tests/i18n.test.tsx` | `src/i18n/i18n` (Public API of `00_foundation`) | OK |
| `tests/wire_contract.test.ts` | `tests/fixtures/enum_spec_snapshot` (test-only fixture) | OK |
| `tests/sse_lifecycle.test.tsx` | `src/api/sse` (`createSSE` — Public API), `src/api/client` (`setToken` — testability accessor) | OK |
| `tests/infrastructure.test.ts` | `tests/msw/server` | OK |
- **No imports of `*.internal.*` files**; no imports following `from '../../../<deep>'` patterns (all cross-references are exactly two levels: `src/<x>/<y>.test.tsx` → `../../tests/<helper>` is two levels, the maximum allowed by the test/source colocation pattern).
- **No new cyclic module dependencies** introduced — test files are leaves in the import graph.
- **No new duplicate symbols across components** — see Phase 6 audit. The only "duplicate-by-name" is `screen` and `waitFor` re-exported from `tests/helpers/render.tsx` to centralize the RTL surface; this is a proxy, not a rival definition.
- **No cross-cutting concern reimplemented locally** — error-envelope handling, MSW routing, fixture seeding, i18n bootstrap each have a single home; no test file open-codes them.
**Public API gap (still F4 from baseline)**: every test still imports by file-path granularity because `src/<component>/index.ts` barrels do not exist. This is the same baseline issue, neither resolved nor worsened by test work.
### Baseline Delta
Comparing current findings to `_docs/02_document/architecture_compliance_baseline.md` (`(file, category, rule)` triple):
**Carried over** — present at baseline, still present:
| # | File | Category | Rule |
|---|------|----------|------|
| F1 | `mission-planner/**` vs `src/features/flights/**` | Architecture | Convergence-pending duplication (deferred to Phase B) |
| F2 | `src/features/dataset/DatasetPage.tsx:9` | Architecture | Cross-feature same-layer edge (grandfathered) |
| F3 | `src/features/annotations/classColors.ts` | Architecture | Physical/logical owner split |
| F4 | every component | Architecture | No Public API barrels |
| F5 | `mission-planner/src/flightPlanning/{MapView,MiniMap}.tsx` | Architecture | Pre-existing cycle inside port-source |
| F6 | codebase-wide | Architecture | No `src/shared/` |
| F7 | `api.*` / `createSSE` call sites | Architecture | Hardcoded `/api/<service>/...` |
| F8 | `_docs/02_document/module-layout.md` | Architecture | Layering-table inconsistency |
| F9 | `mission-planner/src/{main,App,setupTests,vite-env}.tsx` | Architecture | Inert second Vite entry tree |
**Resolved** — present at baseline, NOT in current findings within the in-scope file set: **none**. (Test-implementation work correctly avoided touching production architecture; resolutions belong to Step 8 Refactor or Phase B feature cycles, not Step 6.)
**Newly introduced** — current findings absent at baseline: **none**. The "test helpers import production accessors" pattern was clarified out of finding status by `_docs/02_document/module-layout.md` commit `496b089` ("Clarify Blackbox Tests imports rule (helpers vs test bodies)"). It is now an established, documented exception, not a finding.
Per-category counts (current architecture findings in scope, excluding carried-over baseline): **0 Critical, 0 High, 0 Medium, 0 Low**. No verdict change.
## Findings (cumulative)
### F-CUM-1 — Drift production tasks accumulating (Low / Maintainability / carry-over from batches 2–3)
The three batches together documented **9 production drifts** that tests track via `it.fails()` or `it.skip` quarantine:
1. AZ-457 FT-P-01 — bootstrap refresh `credentials: 'include'` missing → `src/auth/AuthContext.tsx`
2. AZ-457 NFT-SEC-04 — broader `credentials: 'include'` claim narrow today → `src/api/client.ts`
3. AZ-459 — `AnnotationStatus`, `MediaStatus`, `Affiliation` enum drift vs `enum_spec_snapshot.json` → `src/types/index.ts`
4. AZ-458 NFT-PERF-03 / NFT-RES-02 — bearer rotation reconnect ≤5 s missing → `src/features/flights/FlightsPage.tsx:65-68` (deps array)
5. AZ-458 FT-P-09/10 / NFT-PERF-06 — annotation-status SSE not opened → `src/features/annotations/AnnotationsPage.tsx`
6. AZ-465 FT-P-24 — i18n detector path missing → `src/i18n/i18n.ts`
7. AZ-465 FT-P-25 — i18n persistence missing → `src/i18n/i18n.ts`
8. AZ-467 FT-P-32 — ProtectedRoute spinner a11y attrs missing → `src/auth/ProtectedRoute.tsx`
9. AZ-467 FT-P-33 / FT-N-03 / FT-N-05 — ProtectedRoute timeout + RBAC routes missing → `src/auth/ProtectedRoute.tsx`
10. AZ-468 FT-P-30 / FT-P-31 / FT-N-09 — Header dropdown a11y + Escape handler → `src/components/Header.tsx`
11. AZ-481 — `org.opencontainers.image.title` OCI label missing → `.woodpecker/build-arm.yml`
**Recommendation**: file these as Phase B feature tasks during Step 9 (New Task) once Phase A baseline closes. Each is a small, scoped fix; together they materially improve production posture. Do NOT lift them in this Step 6 window — Phase A scope ends at "tests in place"; flipping drifts is feature-cycle work.
This is a **non-blocking** finding; it's bookkeeping for the next phase. Verdict: PASS_WITH_WARNINGS contribution from this finding only.
### F-CUM-2 — Test-helper interpretation rule, now codified (informational)
Batches 1, 2, and 3 each surfaced the "test helpers import production accessors" finding as Low / Architecture / Interpretation. Commit `496b089` ("Clarify Blackbox Tests imports rule (helpers vs test bodies)") wrote the resolution into `_docs/02_document/module-layout.md`: black-box discipline applies to test bodies; setup helpers and composition-root wrappers may import production accessors.
**Status**: closed. Future cumulative reviews should NOT re-emit this finding. The Phase 7 inspection above already treats helper imports of `src/api/client` accessors as OK.
## Auto-Fix Attempts: 0
## Stuck Agents: None
## Verdict: PASS_WITH_WARNINGS
Reason: 0 Critical / 0 High; 1 Low / Maintainability (the production-drift bookkeeping in F-CUM-1). The verdict allows the implement skill to proceed to batch 4 without auto-fix gate intervention.
## Recommendation for Batch 4
Per batch-3 report: **AZ-466 (4) + AZ-475 (2) + AZ-462 (2) + AZ-460 (2) = 10 pts**. AZ-466 lands the `data-destructive` marker + `<DestructiveButton>` wrapper that other tasks (admin user delete, class delete, flight delete) rely on; landing it early is dependency-friendly for batch 5 (canvas / detection-classes / photo-mode / tile-split).
No cumulative-review-gated changes need to be applied before batch 4 starts.