mirror of
https://github.com/azaion/ui.git
synced 2026-06-21 11:41:11 +00:00
[AZ-509] [AZ-510] [AZ-511] [AZ-512] Cycle 3 new-task: epic + 3 task specs
Recovers cycle 3 Step 9 (New Task) outputs that were left uncommitted at the prior session boundary. Adds AZ-509 epic to dependencies table and the three task specs (AZ-510 auth bootstrap consolidation, AZ-511 classColors carve-out, AZ-512 admin edit detection class). Advances autodev state to step 10 (Implement) in_progress. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -95,3 +95,22 @@
|
||||
- **AZ-498 — contract**: produces/consumes `_docs/02_document/contracts/satellite-provider/tiles.md` (v1.0.0, draft).
|
||||
- **AZ-499 — out-of-band**: the compromised key `335799082893fad97fa36118b131f919` must be revoked at the OpenWeatherMap dashboard before AZ-499 closes. AC-7 captures that as a deliverable.
|
||||
- **AZ-499 — gap fix**: adds a new `owm_key_in_source` banned-deps kind that covers `src/` AND `mission-planner/`, closing the source-scan gap left by AZ-482's `dist/`-only scan.
|
||||
|
||||
---
|
||||
|
||||
## Epic AZ-509 — Auth bootstrap + classColors carve-out + admin class edit (cycle 3)
|
||||
|
||||
| Task | Name | Epic | Complexity | Depends on |
|
||||
|------|------|------|-----------|------------|
|
||||
| AZ-510 | Auth bootstrap refresh consolidation (B3 / P3) | AZ-509 | 3 | None |
|
||||
| AZ-511 | classColors carve-out to dedicated component (F3) | AZ-509 | 3 | AZ-485 (barrels), AZ-486 (endpoints) |
|
||||
| AZ-512 | Admin — edit existing detection class (P12 / F10) | AZ-509 | 3 | None in UI; cross-workspace: `admin/` PATCH `/api/admin/classes/{id}` (verify-or-block at impl) |
|
||||
|
||||
### Notes (AZ-509)
|
||||
|
||||
- **Epic AZ-509** is the cycle-3 umbrella. User priority: fixes first — implementation order C → D → B (AZ-510 → AZ-511 → AZ-512).
|
||||
- **Three independent tasks**: no inter-task hard dependencies. The implement skill (Step 10) may parallelise within the cycle's batch plan, but the user's stated preference is fixes-first ordering — the batch plan should sequence AZ-510 → AZ-511 → AZ-512 within the cycle.
|
||||
- **AZ-510** consolidates two divergent refresh paths onto the working POST + credentials shape. Closes long-standing Finding B3 against Vision principle P3. UI-only; no backend coordination.
|
||||
- **AZ-511** moves `src/features/annotations/classColors.ts` → `src/class-colors/` with a barrel and clears the F3-pending STC-ARCH-01 exemption. Closes the "5 coupled places" lesson (LESSONS.md 2026-05-12). Depends on AZ-485 (per-component barrel pattern) and AZ-486 (endpoint builders) only as historical baseline — they're long-landed.
|
||||
- **AZ-512 — cross-workspace prerequisite**: requires `PATCH /api/admin/classes/{id}` in the `admin/` sibling service. The task spec carries a BLOCKING verification gate at implementation time; if the endpoint is absent, the implementer surfaces Choose A/B/C/D (file admin/ ticket as hard prereq / ship UI form against MSW stub for review only / drop AZ-512 from cycle 3). No silent workaround permitted.
|
||||
- **Total complexity**: 9 points across 3 tasks (3+3+3). All within the 2–5 point per-PBI budget.
|
||||
|
||||
@@ -0,0 +1,145 @@
|
||||
# Consolidate AuthContext bootstrap onto POST refresh + /users/me chain
|
||||
|
||||
**Task**: AZ-510_auth_bootstrap_consolidation
|
||||
**Name**: Auth bootstrap refresh consolidation
|
||||
**Description**: Replace the broken `GET /api/admin/auth/refresh` bootstrap path in `AuthContext.tsx` with the same `POST /api/admin/auth/refresh` (credentials-included) path the 401-retry already uses, chaining `GET /api/admin/users/me` to fetch the user shape. Closes the long-standing Finding B3 logged against Architecture Vision principle P3.
|
||||
**Complexity**: 3 points
|
||||
**Dependencies**: None (POST refresh path already lives in `api/client.ts:88` and is exercised by tests)
|
||||
**Component**: 02_auth (primary); 03_shared-ui (Header.test.tsx MSW handlers); 01_api-transport (no source change, but tests reference `api/client.ts`)
|
||||
**Tracker**: AZ-510
|
||||
**Epic**: AZ-509
|
||||
|
||||
## Problem
|
||||
|
||||
The SPA has two refresh-token paths and they disagree:
|
||||
|
||||
- **Bootstrap (broken)** — `src/auth/AuthContext.tsx:24` issues `GET /api/admin/auth/refresh` WITHOUT `credentials: 'include'`. The `Secure HttpOnly` refresh cookie set by `POST /api/admin/auth/login` is therefore never sent on the bootstrap call; the server cannot recognise the session; the request fails; the `.catch(() => {})` swallows the error; `setLoading(false)` resolves to "no user"; `ProtectedRoute` redirects to `/login`. A returning user with a perfectly valid refresh cookie is silently bounced to login on every page load.
|
||||
|
||||
- **401-retry (works)** — `src/api/client.ts:88` issues `POST /api/admin/auth/refresh` WITH `credentials: 'include'`. This path runs only when a subsequent authenticated request hits a 401; it does NOT run on bootstrap because line 73's `if (res.status === 401 && accessToken)` short-circuits when `accessToken` is null (which it always is on cold boot).
|
||||
|
||||
The broken path was flagged in the architecture documentation review (Architecture Vision principle P3 — "bearer in memory, refresh in HttpOnly cookie") and again in `_docs/02_document/architecture_compliance_baseline.md` as downstream item B3. Step 4 (Testability) chose to leave it for a behaviour cycle because the fix changes the bootstrap response handling, not just hardcoded strings — outside the testability-revision allowed-changes list.
|
||||
|
||||
Observable failure mode today: every page reload by an authenticated user shows a brief `/login` redirect followed by a forced re-login. Operators have learned to ignore it; the behaviour normalises a UX regression that violates P3.
|
||||
|
||||
## Outcome
|
||||
|
||||
- A returning user with a valid refresh cookie loads any URL (`/`, `/flights`, `/dataset`, …) and lands on the intended route without redirecting through `/login`.
|
||||
- A returning user with an expired/invalid refresh cookie sees `/login` exactly once — no flash of the protected shell, no infinite redirect loop.
|
||||
- The `GET /api/admin/auth/refresh` request disappears from network traces in the bootstrap window.
|
||||
- `POST /api/admin/auth/refresh` (with credentials) followed by `GET /api/admin/users/me` (with bearer) appears in network traces on every successful bootstrap.
|
||||
- Existing MSW tests pass against the new code path; no test handler relies on the deprecated GET bootstrap.
|
||||
|
||||
## Scope
|
||||
|
||||
### Included
|
||||
|
||||
- `src/auth/AuthContext.tsx` — rewrite the `useEffect` mount handler to:
|
||||
1. `await fetch(getApiBase() + endpoints.admin.authRefresh(), { method: 'POST', credentials: 'include' })` — direct call (not `api.post()`, because `api.post` does not carry `credentials: 'include'` and adding it there would change every callsite's CORS posture).
|
||||
2. On `!res.ok` → set `user: null` + `loading: false` + return.
|
||||
3. On success → `setToken(data.token)`, then `api.get<AuthUser>('/api/admin/users/me')` to fetch the user shape, `setUser(authUser)`, `setLoading(false)`.
|
||||
4. On the `/users/me` failure path → `setToken(null)`, `setUser(null)`, `setLoading(false)`. Do not throw silently — a 401 here is a genuine "refresh succeeded but the user record is gone" edge case worth surfacing through console.error.
|
||||
- Tests (in-task; not deferred to a separate `test-spec sync` ticket):
|
||||
- `src/auth/AuthContext.test.tsx` — update bootstrap tests to assert `POST /api/admin/auth/refresh` then `GET /api/admin/users/me`. Drop GET-bootstrap expectations.
|
||||
- `src/auth/ProtectedRoute.test.tsx` — same MSW handler swap.
|
||||
- `src/components/Header.test.tsx` — same MSW handler swap (the test fires a full app render that exercises bootstrap).
|
||||
- New i18n strings: NONE (the user-visible behaviour change is the absence of the spurious redirect, not new copy).
|
||||
- A small note added to `_docs/02_document/components/02_auth/description.md` recording that bootstrap and 401-retry now share a single wire shape.
|
||||
|
||||
### Excluded
|
||||
|
||||
- Refresh-cookie rotation backend changes — server keeps its existing rotate-on-refresh policy unchanged.
|
||||
- SSE bearer-rotation hardening (ADR-008 consequences) — separate ticket scope; the `?token=...` query-string refresh problem is not addressed here.
|
||||
- Changing `api.post` to default `credentials: 'include'` — out of scope; would expand the test matrix to every POST callsite.
|
||||
- Embedding the user payload in the POST refresh response — would be a backend wire-contract change; the chained `/users/me` GET is intentional and matches existing semantics.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
**AC-1: Bootstrap uses POST refresh with credentials**
|
||||
Given a fresh app mount (no in-memory bearer)
|
||||
When `AuthProvider` renders
|
||||
Then exactly one outbound request is made to `POST /api/admin/auth/refresh` with `credentials: 'include'`; no `GET /api/admin/auth/refresh` request occurs.
|
||||
|
||||
**AC-2: Successful refresh chains to /users/me**
|
||||
Given the POST refresh returns 200 with `{ token: '<bearer>' }`
|
||||
When the response resolves
|
||||
Then `setToken('<bearer>')` is called, then `GET /api/admin/users/me` is requested with `Authorization: Bearer <bearer>`; on its 200 response the returned `AuthUser` is exposed via `useAuth().user`; `loading` flips to `false`.
|
||||
|
||||
**AC-3: Failed refresh shows /login without flash**
|
||||
Given the POST refresh returns 401 (no valid cookie) or a network error occurs
|
||||
When the response is handled
|
||||
Then `setUser(null)` + `setLoading(false)` are called; `ProtectedRoute` renders the spinner during the in-flight bootstrap and then renders `/login` exactly once; no protected route component renders even momentarily; no second redirect fires.
|
||||
|
||||
**AC-4: /users/me failure after refresh success clears the bearer**
|
||||
Given the POST refresh returns 200 but the subsequent `GET /users/me` returns 401 or fails
|
||||
When the failure is handled
|
||||
Then `setToken(null)` is called, `setUser(null)` + `setLoading(false)` are called, the user lands on `/login`, and `console.error` carries a diagnostic message identifying the edge case (refresh OK / user GET failed).
|
||||
|
||||
**AC-5: Returning user is not bounced through /login**
|
||||
Given a refresh cookie that the backend considers valid
|
||||
When the user reloads any protected URL (e.g. `/flights`)
|
||||
Then no `/login` route is rendered (verified via a Playwright e2e check or via the React-Router history not containing a `/login` entry); the user sees the protected route immediately after the bootstrap spinner.
|
||||
|
||||
**AC-6: No regression in the 401-retry path**
|
||||
Given an authenticated session with an expired bearer (`accessToken` non-null but server-side expired)
|
||||
When the user makes any API call from a feature page
|
||||
Then the existing `api/client.ts:73` 401-retry path is unchanged, calls `POST /api/admin/auth/refresh` with credentials, rotates the bearer, and replays the original request — behaviour identical to today.
|
||||
|
||||
## Non-Functional Requirements
|
||||
|
||||
**Performance**: bootstrap latency added by the chained `/users/me` GET is observable but acceptable — both calls hit the same nginx, same auth, same machine in prod; budget: under 200 ms p95 for the chain on the suite dev compose stack.
|
||||
|
||||
**Compatibility**: no change to the backend contract. The chained `/users/me` GET already exists and is the only source of user shape today; tests prove it.
|
||||
|
||||
**Reliability**: every failure mode (refresh 401, refresh network error, refresh 200 + users/me 401, refresh 200 + users/me network error) must resolve `loading` to `false` and put the user on `/login`. No path may leave `loading: true` indefinitely.
|
||||
|
||||
## Unit Tests
|
||||
|
||||
| AC Ref | What to Test | Required Outcome |
|
||||
|--------|--------------|------------------|
|
||||
| AC-1 | `AuthContext` mount with no prior bearer | exactly one POST `/api/admin/auth/refresh` is made; no GET refresh |
|
||||
| AC-2 | POST refresh 200 → users/me 200 | bearer set + user set + `loading: false` |
|
||||
| AC-3 | POST refresh 401 | `setUser(null)` + `loading: false` + no further requests |
|
||||
| AC-3 | POST refresh network error (MSW `HttpResponse.error()`) | same as 401 case |
|
||||
| AC-4 | POST refresh 200 → users/me 401 | `setToken(null)` + `setUser(null)` + `loading: false`; console.error called |
|
||||
| AC-6 | request → 401 → POST refresh 200 → replay → 200 | unchanged 401-retry behaviour (regression guard) |
|
||||
|
||||
## Blackbox Tests
|
||||
|
||||
| AC Ref | Initial Data/Conditions | What to Test | Expected Behavior | NFR References |
|
||||
|--------|------------------------|--------------|-------------------|----------------|
|
||||
| AC-1 | Browser with valid refresh cookie | Reload `/flights` | DevTools Network panel shows POST `/api/admin/auth/refresh` followed by GET `/users/me` — no GET refresh | — |
|
||||
| AC-5 | Browser with valid refresh cookie | Reload `/flights` | `/flights` renders directly; no `/login` is visible at any point | — |
|
||||
| AC-3 | Browser with expired refresh cookie | Reload `/` | Spinner briefly visible; then `/login`; no flash of the protected shell | Reliability |
|
||||
|
||||
## Constraints
|
||||
|
||||
- The `getApiBase()` helper is the ONLY source for the base URL — do not bypass it.
|
||||
- The new bootstrap path must NOT use `api.post()` because that helper does not carry `credentials: 'include'`. Direct `fetch(..., { method: 'POST', credentials: 'include' })` is intentional; the comment in `api/client.ts:88` documents the same pattern.
|
||||
- The MSW test handlers must run against the **production** code paths — no `vi.mock('api/client')` or equivalent allowed.
|
||||
- `setToken(null)` must precede `setUser(null)` on every failure path so that an in-flight component re-render does not see a partial state where `user: null` but `accessToken: <stale-bearer>`.
|
||||
|
||||
## Risks & Mitigation
|
||||
|
||||
**Risk 1: POST refresh response shape varies across environments**
|
||||
- *Risk*: The 401-retry path assumes `{ token }`; production may also return `{ token, user }` (unverified). If so, the chained `/users/me` GET is wasted work.
|
||||
- *Mitigation*: Inspect the live response shape during implementation; if `user` is present, skip the chained GET. The contract is single-source in the backend Admin API spec — verify there first, not by guessing.
|
||||
|
||||
**Risk 2: Tests assume GET-bootstrap fail-soft behaviour**
|
||||
- *Risk*: Some current tests may assert the broken behaviour as the expected outcome ("when bootstrap fails the user lands on /login"). Re-pointing those tests at the POST path may surface assertion bugs that have been masking real regressions.
|
||||
- *Mitigation*: Read each test's assertions before swapping the handler; if the test was asserting the broken behaviour as a feature, replace the assertion with the AC-3 behaviour from this spec. Do not preserve a test that documents the bug.
|
||||
|
||||
**Risk 3: Bootstrap latency regression**
|
||||
- *Risk*: Two sequential GETs on every page load is more network than one. For very slow refresh cookies (e.g., over slow links), the user perceives a longer spinner.
|
||||
- *Mitigation*: NFR Performance budget (200 ms p95 on dev compose) is the gate. If a real-world deployment exceeds it, the next iteration may embed user in the POST refresh response (Excluded scope above).
|
||||
|
||||
**Risk 4: Concurrent `<StrictMode>` double-mount fires bootstrap twice**
|
||||
- *Risk*: React 18+ StrictMode dev mode mounts effects twice; two concurrent POST refresh requests could race the cookie rotation (the backend rotates on every refresh).
|
||||
- *Mitigation*: Add a module-scoped in-flight guard (a `Promise<void> | null` ref) so the second mount awaits the first. The guard is small enough to live inside `AuthContext.tsx` without a new helper.
|
||||
|
||||
## References
|
||||
|
||||
- `src/auth/AuthContext.tsx:23-31` — broken bootstrap path being replaced.
|
||||
- `src/api/client.ts:88-98` — working POST refresh path that informs the new bootstrap.
|
||||
- `_docs/02_document/components/02_auth/description.md` — component spec; F2 (two refresh paths) is the documented finding this task closes.
|
||||
- `_docs/02_document/architecture_compliance_baseline.md` — downstream item B3 (will move to RESOLVED).
|
||||
- `_docs/02_document/architecture.md` Architecture Vision P3 — "bearer in memory, refresh in HttpOnly cookie".
|
||||
@@ -0,0 +1,167 @@
|
||||
# Carve classColors.ts out of 06_annotations into its own component dir
|
||||
|
||||
**Task**: AZ-511_classcolors_carve_out
|
||||
**Name**: classColors carve-out to dedicated component (closes F3)
|
||||
**Description**: Move `src/features/annotations/classColors.ts` to its own component directory `src/class-colors/` with a barrel; update the four consumer import paths to go through the barrel; remove the STC-ARCH-01 F3-pending exemption; clean up the five coupled documentation/script callouts. Closes the High Architecture baseline finding F3 and eliminates the carry-forward exemption surface logged in `LESSONS.md` ("5 coupled places").
|
||||
**Complexity**: 3 points
|
||||
**Dependencies**: AZ-485 (Public API barrels + STC-ARCH-01) — the F3 exemption only exists because AZ-485 landed; this task lives on top of that boundary.
|
||||
**Component**: 11_class-colors (gains a physical home); 06_annotations (loses the misplaced file from its owns-glob); 03_shared-ui (consumer); plus three doc/script artifacts.
|
||||
**Tracker**: AZ-511
|
||||
**Epic**: AZ-509
|
||||
|
||||
## Problem
|
||||
|
||||
Baseline finding **F3** (`_docs/02_document/architecture_compliance_baseline.md`): `src/features/annotations/classColors.ts` is a Layer 0 / 1 shared kernel logically owned by component `11_class-colors`, but it physically sits inside `06_annotations`'s owns-glob. Re-exporting it through the `06_annotations` barrel would create a runtime circular import:
|
||||
|
||||
```
|
||||
AnnotationsPage → DetectionClasses (03_shared-ui) → 06_annotations barrel → AnnotationsPage
|
||||
```
|
||||
|
||||
So after AZ-485 landed the per-component barrel architecture, F3 became visible. The workaround documented in `_docs/02_document/module-layout.md` Layout Rule #3 leaves the file in place and adds an exemption regex to `scripts/check-arch-imports.mjs` so consumers can deep-import `'../features/annotations/classColors'` without tripping STC-ARCH-01.
|
||||
|
||||
The exemption is correct but expensive — it lives in **five coupled places**, captured as a lesson on 2026-05-12:
|
||||
|
||||
1. `scripts/check-arch-imports.mjs` — `EXEMPT_RE` allowing the deep import.
|
||||
2. `tests/architecture_imports.test.ts` — fixture asserting the exemption holds.
|
||||
3. `src/features/annotations/index.ts` — 7-line carry-over comment block explaining why classColors is NOT re-exported here.
|
||||
4. `_docs/02_document/components/11_class-colors/description.md` — Caveats §7 "Physical location is misplaced today" + Module Inventory's "physical location pending refactor" suffix.
|
||||
5. `_docs/02_document/module-layout.md` — Layout Rule #3 exemption clause + Per-Component Mapping for `11_class-colors` ("Directories: none today...") + Verification Needed #1 + `shared/class-colors` proposed section + `06_annotations` Owns clause ("EXCEPT `classColors.ts`").
|
||||
|
||||
Every contributor reading any one of those touches the exemption — and the lesson explicitly warns that the carry-over **never silently drifts** because each touchpoint is enforced (static check, unit test, doc, layout rule). The cost is real ongoing tax; closing F3 removes all of it at once.
|
||||
|
||||
## Outcome
|
||||
|
||||
- `classColors.ts` lives at its logical layer (`src/class-colors/classColors.ts`) with a proper barrel (`src/class-colors/index.ts`); consumers import from the barrel (`'../class-colors'` or `'../../class-colors'`) like every other component.
|
||||
- The STC-ARCH-01 exemption regex disappears from `scripts/check-arch-imports.mjs` and from the architecture test fixture; running `bun run --bun scripts/check-arch-imports.mjs --mode=arch-imports` finds zero deep imports anywhere in `src/`.
|
||||
- The five coupled doc/script callouts above are simplified: each reflects the new physical home; none reference an exemption.
|
||||
- `bun run build` succeeds with no runtime circular-import warnings (the original concern is gone because `class-colors` is no longer a subtree of `06_annotations`).
|
||||
- `architecture_compliance_baseline.md` F3 row reads **CLOSED** with the task and commit reference, mirroring the AZ-485 → F4 and AZ-486 → F7 patterns.
|
||||
|
||||
## Scope
|
||||
|
||||
### Included
|
||||
|
||||
**Source changes**
|
||||
|
||||
- Create directory `src/class-colors/` containing:
|
||||
- `classColors.ts` — exact byte-for-byte copy of `src/features/annotations/classColors.ts` (12-color palette, 12 fallback names, `getClassColor`, `getPhotoModeSuffix`, `getClassNameFallback`, `FALLBACK_CLASS_NAMES` — no behaviour change).
|
||||
- `index.ts` — re-exports the four public symbols: `getClassColor`, `getClassNameFallback`, `getPhotoModeSuffix`, `FALLBACK_CLASS_NAMES`.
|
||||
- Delete `src/features/annotations/classColors.ts`.
|
||||
- Update 4 consumer imports (currently shown by `rg classColors src/`):
|
||||
- `src/components/DetectionClasses.tsx` — `from '../features/annotations/classColors'` → `from '../class-colors'`.
|
||||
- `src/features/annotations/CanvasEditor.tsx` — `from './classColors'` → `from '../../class-colors'`.
|
||||
- `src/features/annotations/AnnotationsSidebar.tsx` — `from './classColors'` → `from '../../class-colors'`.
|
||||
- `src/features/annotations/AnnotationsPage.tsx` — `from './classColors'` → `from '../../class-colors'`.
|
||||
- Drop the "classColors symbols are NOT re-exported here" comment block from `src/features/annotations/index.ts` (lines 5-12 of the current file).
|
||||
|
||||
**Script + test changes**
|
||||
|
||||
- Remove the F3-pending exemption from `scripts/check-arch-imports.mjs` (the `EXEMPT_RE` entry covering `features/annotations/classColors`).
|
||||
- Update `tests/architecture_imports.test.ts` so the fixture asserting the exemption is either deleted (preferred) or rewritten to assert "no exemptions remain". Whichever shape, the test must still pass and continue to catch regressions.
|
||||
|
||||
**Documentation changes**
|
||||
|
||||
- `_docs/02_document/module-layout.md`:
|
||||
- Layout Rule #3 — drop the "One F3-pending exemption" clause.
|
||||
- Per-Component Mapping for `11_class-colors` — `Directories: src/class-colors/**` (not "none today"); `Public API exported from src/class-colors/index.ts` (not "no barrel today").
|
||||
- Verification Needed #1 — mark as RESOLVED with task reference.
|
||||
- `## Shared / Cross-Cutting` → `### shared/class-colors` block — remove the workaround note about READ-ONLY for `06_annotations` tasks.
|
||||
- Per-Component Mapping for `06_annotations` — drop the "EXCEPT `classColors.ts`" clause from Owns.
|
||||
- `_docs/02_document/components/11_class-colors/description.md` — Caveats §7 "Physical location is misplaced today" → rewrite as "Physical location: `src/class-colors/`" with the historical note moved to a single line citing the closing task; Module Inventory path updated.
|
||||
- `_docs/02_document/architecture_compliance_baseline.md` — F3 row gets the CLOSED marker (same shape as F4, F7), with task + commit hash placeholder for the implementer to fill at merge time.
|
||||
|
||||
### Excluded
|
||||
|
||||
- Moving `CanvasEditor.tsx` (Finding F2 — different cross-feature edge; separate task).
|
||||
- Creating `src/shared/` (Finding F6 — distinct decision; deliberately NOT used as the target so this task doesn't pre-empt F6 design).
|
||||
- Changing the `classColors.ts` API surface — pure file move + import-path updates. The dead `??` guard noted in `11_class-colors/description.md` §5 stays dead; the redundancy with `DetectionClass.photoMode` stays unaddressed; both are Step 4/5 review items, not this task.
|
||||
- Renaming any of the four exported symbols.
|
||||
- Adding `localization` for the suffix strings (Step 4 i18n item; separate concern).
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
**AC-1: File physically lives at new location**
|
||||
Given the repository after the task lands
|
||||
When `ls src/class-colors/`
|
||||
Then it contains `classColors.ts` and `index.ts`; running `find src/features/annotations -name classColors.ts` returns no results.
|
||||
|
||||
**AC-2: Consumers import via barrel**
|
||||
Given the four consumer files (`DetectionClasses.tsx`, `CanvasEditor.tsx`, `AnnotationsSidebar.tsx`, `AnnotationsPage.tsx`)
|
||||
When their imports are inspected
|
||||
Then each imports from `'../class-colors'` or `'../../class-colors'` (the barrel), not from `'.../classColors'` (the file).
|
||||
|
||||
**AC-3: Architecture static check has zero exemptions**
|
||||
Given the codebase after the task lands
|
||||
When `bun run --bun scripts/check-arch-imports.mjs --mode=arch-imports` runs
|
||||
Then the exit code is 0; the `EXEMPT_RE` block in the script contains no entry for `classColors`; `tests/architecture_imports.test.ts` passes without referencing a classColors exemption.
|
||||
|
||||
**AC-4: Build succeeds with no circular-import warnings**
|
||||
Given the codebase after the task lands
|
||||
When `bun run build` runs
|
||||
Then it succeeds; Vite output contains no "Circular dependency" warnings involving `class-colors`, `annotations`, or `DetectionClasses`.
|
||||
|
||||
**AC-5: Full test suite green**
|
||||
Given the codebase after the task lands
|
||||
When `bun run test` runs
|
||||
Then all previously-passing tests still pass — including `tests/detection_classes.test.tsx` (AZ-472), `tests/architecture_imports.test.ts`, and any test that imports a consumer file.
|
||||
|
||||
**AC-6: Documentation is consistent**
|
||||
Given the codebase after the task lands
|
||||
When the 5 coupled doc/script touchpoints are inspected
|
||||
Then `module-layout.md`, `11_class-colors/description.md`, `architecture_compliance_baseline.md`, `src/features/annotations/index.ts`, and `scripts/check-arch-imports.mjs` all reflect the new physical home; no surviving reference describes classColors as "physically misplaced", "F3-pending", or "exempt".
|
||||
|
||||
## Non-Functional Requirements
|
||||
|
||||
**Compatibility**: zero runtime behaviour change. Bundle size is identical (same exported symbols, same implementation). Bundle composition shifts by one chunk boundary but tree-shaking preserves dead-code-elimination semantics.
|
||||
|
||||
**Reliability**: the structural fix removes a long-standing risk that a new contributor accidentally re-introduces the circular import by re-exporting classColors from the 06_annotations barrel. After this task lands, that re-export becomes legal but no longer creates a cycle (because class-colors is its own component).
|
||||
|
||||
## Unit Tests
|
||||
|
||||
| AC Ref | What to Test | Required Outcome |
|
||||
|--------|--------------|------------------|
|
||||
| AC-1 | `import { getClassColor } from '../class-colors'` | resolves to the new file; `getClassColor(0)` returns the same hex as today |
|
||||
| AC-2 | Static scan of import declarations in the 4 consumers | every import is via barrel; no file-path import remains |
|
||||
| AC-3 | Architecture test fixture (`tests/architecture_imports.test.ts`) | passes after the F3 exemption fixture is removed |
|
||||
| AC-5 | All existing classColors-touching tests | unchanged assertions, all green |
|
||||
|
||||
## Blackbox Tests
|
||||
|
||||
| AC Ref | Initial Data/Conditions | What to Test | Expected Behavior | NFR References |
|
||||
|--------|------------------------|--------------|-------------------|----------------|
|
||||
| AC-4 | Clean clone, `bun install` complete | `bun run build` | succeeds; no circular-import warnings | Reliability |
|
||||
| AC-2 + AC-3 | Clean clone, `bun install` complete | `bun run --bun scripts/check-arch-imports.mjs --mode=arch-imports` | exit 0; no exemption block matches classColors | — |
|
||||
| AC-5 | Clean clone, `bun install` complete | `bun run test` | full suite passes | — |
|
||||
|
||||
## Constraints
|
||||
|
||||
- The file move must be a single atomic commit (or one PR's worth of commits). Splitting "move file" from "update imports" creates a broken intermediate state where neither path works.
|
||||
- The new directory name is `src/class-colors/` — kebab-case, matching every other component dir established by AZ-485. Do NOT use `src/classColors/` (camel-case) or `src/shared/class-colors/` (opens F6).
|
||||
- The barrel must re-export ALL four current public symbols. Dropping `FALLBACK_CLASS_NAMES` (currently used by `DetectionClasses.tsx` for the empty-state fallback row) would break the consumer.
|
||||
- The `EXEMPT_RE` regex literal in `scripts/check-arch-imports.mjs` may be a single combined pattern — read the script first to understand its shape before editing.
|
||||
|
||||
## Risks & Mitigation
|
||||
|
||||
**Risk 1: A consumer was missed**
|
||||
- *Risk*: A test file, story, or sample (`tests/**`, `e2e/**`, `_docs/02_document/modules/*.md`) imports `classColors` from the old path and breaks after the move.
|
||||
- *Mitigation*: Before deletion, `rg "features/annotations/classColors" .` from the repo root. Every match outside `_docs/` is a consumer that must be updated. Doc references inside `_docs/` are addressed in the documentation changes above.
|
||||
|
||||
**Risk 2: Vite hot-module resolution caches the old path**
|
||||
- *Risk*: After the move, a stale dev-server HMR session continues to resolve `'../features/annotations/classColors'` from cache.
|
||||
- *Mitigation*: Cold-restart `bun run dev` after the move. CI is unaffected.
|
||||
|
||||
**Risk 3: A circular import resurfaces from a different direction**
|
||||
- *Risk*: A future contributor re-introduces a circle by importing something from `06_annotations` inside `src/class-colors/classColors.ts`. The new physical separation doesn't make all circles impossible.
|
||||
- *Mitigation*: Out of scope for this task. The general "no cross-component deep imports" rule (STC-ARCH-01) is already in place and now applies to `class-colors` symmetrically; that's the standing protection.
|
||||
|
||||
**Risk 4: The architecture test fixture deletion loses regression coverage**
|
||||
- *Risk*: The current `tests/architecture_imports.test.ts` fixture asserts that the exemption WORKS. Deleting the fixture removes that regression check; if a future change accidentally re-introduces a similar exemption, the test won't catch it.
|
||||
- *Mitigation*: Replace the fixture with a stronger assertion: "no `EXEMPT_RE` entries match any path under `src/`". That keeps the safety net while removing the F3-specific coupling.
|
||||
|
||||
## References
|
||||
|
||||
- `_docs/02_document/architecture_compliance_baseline.md` — F3 (High / Architecture); to be marked CLOSED on completion.
|
||||
- `_docs/02_document/module-layout.md` — Layout Rule #3, Per-Component Mapping `11_class-colors`, `06_annotations`, Verification Needed #1, `## Shared / Cross-Cutting` → `### shared/class-colors`.
|
||||
- `_docs/02_document/components/11_class-colors/description.md` — Caveats §7, Module Inventory.
|
||||
- `_docs/LESSONS.md` — 2026-05-12 architecture lesson on the 5-coupled-places exemption pattern.
|
||||
- `_docs/02_tasks/done/AZ-485_refactor_public_api_barrels.md` — establishes the per-component barrel pattern this task extends.
|
||||
@@ -0,0 +1,184 @@
|
||||
# Admin: edit existing detection class (inline form + PATCH wiring)
|
||||
|
||||
**Task**: AZ-512_admin_edit_detection_class
|
||||
**Name**: Admin — edit existing detection class
|
||||
**Description**: Re-introduce the "edit detection class" affordance the WPF→React port lost. Wire an inline edit form on each Detection Class row in the Admin page, calling `PATCH /api/admin/classes/{id}` with the editable fields, refreshing classes via the existing read endpoint. Closes Architecture Vision principle **P12** ("admin can edit existing detection classes — add + edit + delete is the full CRUD surface").
|
||||
**Complexity**: 3 points
|
||||
**Dependencies**: None in the UI workspace. Cross-workspace hard prerequisite: `admin/` sibling service must expose `PATCH /api/admin/classes/{id}` — verification step BLOCKS implementation if absent (see Risks).
|
||||
**Component**: 08_admin (primary)
|
||||
**Tracker**: AZ-512
|
||||
**Epic**: AZ-509
|
||||
|
||||
## Problem
|
||||
|
||||
`AdminPage.tsx` today supports only two of the three CRUD operations for detection classes:
|
||||
|
||||
- **Add** — `handleAddClass` POSTs `endpoints.admin.classes()` with `{ name, shortName, color, maxSizeM }`.
|
||||
- **Delete** — `handleDeleteClass(id)` DELETEs `endpoints.admin.class(id)`.
|
||||
- **Edit** — **missing**. Operators wanting to fix a typo in a class name, recolour a class, or adjust its `maxSizeM` must delete the class (orphaning every detection that references it) and recreate it. That's a destructive workaround for a routine maintenance action.
|
||||
|
||||
This was confirmed as a user-visible gap during Step 4.5 (Architecture Vision finalisation, 2026-05-10): Vision principle **P12** was elevated to a binding constraint expressly because the verification log (`_docs/02_document/04_verification_log.md` F10) showed the modern UI was a regression vs the legacy WPF page, which supported in-place edit. The principle has been on the books since but no cycle has scheduled the work.
|
||||
|
||||
The endpoint builder `endpoints.admin.class(id)` already exists (used today by DELETE) and matches the conventional PATCH target for an item-by-id mutation. The `api.patch()` helper exists in `api/client.ts`. The piece that doesn't exist (or isn't verified to exist) is the backend route handler.
|
||||
|
||||
## Outcome
|
||||
|
||||
- An admin user looking at the Detection Classes table can click any row (or a per-row pencil affordance) and see the row swap to an inline edit form populated with the current values.
|
||||
- Edits to `name`, `shortName`, `color`, and `maxSizeM` are sent via `PATCH /api/admin/classes/{id}`; on 200 the row re-renders with the updated values; on 4xx/5xx an inline error message appears next to the form.
|
||||
- A Cancel button on the form discards local edits and reverts the row.
|
||||
- Validation: `name` is required; `maxSizeM` is a positive number; `color` is a hex string from the standard color input.
|
||||
- All new user-visible strings are added to both `en.json` and `ua.json` per principle P6.
|
||||
- Closes P12. `_docs/02_document/04_verification_log.md` F10 moves to RESOLVED.
|
||||
- No regression in add or delete; no change to the rest of the Admin page (users, aircrafts, AI/GPS settings).
|
||||
|
||||
## Scope
|
||||
|
||||
### Included
|
||||
|
||||
- `src/features/admin/AdminPage.tsx`:
|
||||
- Add `editingId: number | null` and `editForm: { name, shortName, color, maxSizeM }` state.
|
||||
- Add row-click (or pencil-icon click) handler that sets `editingId` and seeds `editForm` from the current row.
|
||||
- Replace the read-only row markup with the editable form markup when `c.id === editingId`.
|
||||
- Add `handleUpdateClass()` that calls `api.patch(endpoints.admin.class(c.id), editForm)`, on success re-fetches classes from `endpoints.annotations.classes()` (mirrors `handleAddClass`'s refresh pattern), clears `editingId`, surfaces errors inline (no `alert()`).
|
||||
- Add `handleCancelEdit()` that clears `editingId` and `editForm`.
|
||||
- Wire keyboard convenience: `Enter` in the form submits; `Escape` cancels.
|
||||
- New i18n strings in `en.json` + `ua.json` under `admin.classes.*`: `edit` (button/title), `save`, `cancel`, `nameRequired`, `maxSizeMustBePositive`, `updateFailed`.
|
||||
- Update `_docs/02_document/components/08_admin/description.md` to record the new affordance (one paragraph in the relevant section).
|
||||
|
||||
### Excluded
|
||||
|
||||
- Fixing the missing ConfirmDialog on class **DELETE** (Finding B4 — separate task; do NOT bundle even though the same file is being touched. Scope discipline.).
|
||||
- Editing `photoMode` for an existing class — `photoMode` is a class-creation property today; mutating it after creation has cross-detection implications (`yoloId = classId + photoModeOffset`) that need backend rules; out of scope.
|
||||
- Bulk edit / multi-select edit — single-row edit only.
|
||||
- Renaming the underlying API endpoint or changing its wire shape.
|
||||
- Adding edit affordances to **users** or **aircrafts** in this page — separate concerns.
|
||||
- Refactoring `AdminPage.tsx` to extract per-section components — Step 8 refactor candidate, not this task.
|
||||
|
||||
## Cross-Workspace Verification (BLOCKING gate)
|
||||
|
||||
Before implementing the form, the implementer MUST verify the backend endpoint exists:
|
||||
|
||||
1. Read `../admin/` source (or the service's OpenAPI/Swagger surface) to confirm `PATCH /api/admin/classes/{id}` is routed and accepts `{ name?, shortName?, color?, maxSizeM? }`.
|
||||
2. If the endpoint exists → proceed with implementation per the AC below.
|
||||
3. If the endpoint is missing → **STOP**. Surface to the user via Choose A/B/C/D:
|
||||
- **A**: File a hard-prerequisite ticket on the `admin/` workspace, pause AZ-512 until that lands.
|
||||
- **B**: Implement only the UI form, mock-stubbed against MSW in tests, mark the cycle's Step 11 (Run Tests) as "blocked on admin/ PATCH" and ship a draft PR for review.
|
||||
- **C**: Drop AZ-512 from cycle 3, defer to a future cycle once `admin/` work is scheduled.
|
||||
|
||||
Do not invent a workaround that bypasses the missing endpoint.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
**AC-1: Edit affordance is visible on every class row**
|
||||
Given the Admin page is loaded for an admin user
|
||||
When the Detection Classes table renders
|
||||
Then each row displays an edit affordance (pencil icon or click-to-edit cue) alongside the existing delete affordance.
|
||||
|
||||
**AC-2: Clicking edit opens the inline form pre-populated**
|
||||
Given a class row is in read-only state
|
||||
When the user activates its edit affordance
|
||||
Then the row replaces its read-only cells with editable `name`, `shortName`, `color`, `maxSizeM` inputs; the inputs are seeded with the row's current values; Save and Cancel buttons are visible; no other row enters edit mode simultaneously.
|
||||
|
||||
**AC-3: Save sends PATCH and refreshes the list**
|
||||
Given the inline form has valid edits
|
||||
When the user clicks Save (or presses Enter inside the form)
|
||||
Then exactly one `PATCH /api/admin/classes/{id}` request is made with body `{ name, shortName, color, maxSizeM }`; on 200 the classes list re-fetches and the row re-renders in read-only state with the new values; the form closes.
|
||||
|
||||
**AC-4: Cancel discards edits**
|
||||
Given the inline form has unsaved edits
|
||||
When the user clicks Cancel (or presses Escape inside the form)
|
||||
Then no network request is made; the form closes; the row reverts to its previous read-only values.
|
||||
|
||||
**AC-5: Validation prevents invalid submits**
|
||||
Given the inline form has `name === ''` OR `maxSizeM <= 0` OR `maxSizeM` is non-numeric
|
||||
When the user clicks Save
|
||||
Then NO network request is made; an inline error message appears next to the offending field with the appropriate i18n key (`admin.classes.nameRequired` / `admin.classes.maxSizeMustBePositive`); focus moves to the offending field.
|
||||
|
||||
**AC-6: Backend error is surfaced**
|
||||
Given the PATCH request fails with 4xx or 5xx
|
||||
When the response is handled
|
||||
Then an inline error message appears under the form using the `admin.classes.updateFailed` i18n key; the form stays open with the user's edits intact; no alert() is used (Finding B4 anti-pattern).
|
||||
|
||||
**AC-7: i18n parity**
|
||||
Given the en.json and ua.json bundles after the task lands
|
||||
When the AZ-465 i18n parity test runs
|
||||
Then every new admin.classes.* key exists in both bundles with non-empty values; t() coverage is preserved.
|
||||
|
||||
**AC-8: Existing add + delete behaviour is unchanged**
|
||||
Given the Admin page after the task lands
|
||||
When an admin user adds a new class or deletes an existing class
|
||||
Then the network requests and UI behaviour are byte-identical to today (regression guard).
|
||||
|
||||
## Non-Functional Requirements
|
||||
|
||||
**Performance**: editing a row triggers exactly two requests in the success path — `PATCH` then `GET classes` (the existing refresh pattern). No additional polling, no debounced auto-save.
|
||||
|
||||
**Compatibility**: the wire contract is additive — `PATCH /api/admin/classes/{id}` accepting `{ name?, shortName?, color?, maxSizeM? }` is the assumed shape. If the live endpoint requires every field, the form's `editForm` already carries every field (seeded from the row), so the request body is always complete — no compatibility variance.
|
||||
|
||||
**Accessibility**: the inline form must be keyboard-navigable; Tab moves between inputs; Enter submits; Escape cancels. The edit affordance must have an accessible name (`aria-label={t('admin.classes.edit')}`) when implemented as an icon-only button.
|
||||
|
||||
## Unit Tests
|
||||
|
||||
| AC Ref | What to Test | Required Outcome |
|
||||
|--------|--------------|------------------|
|
||||
| AC-2 | Click the edit affordance on row N | row N renders the inline form with seeded values; other rows unchanged |
|
||||
| AC-3 | Submit valid form | one PATCH call to `/api/admin/classes/{id}` with the expected body; row re-renders with new values |
|
||||
| AC-3 | Submit via Enter key | same as Save button |
|
||||
| AC-4 | Click Cancel | no network call; row reverts |
|
||||
| AC-4 | Press Escape in form | same as Cancel button |
|
||||
| AC-5 | Empty name, click Save | no PATCH; inline error visible |
|
||||
| AC-5 | Negative maxSizeM, click Save | no PATCH; inline error visible |
|
||||
| AC-6 | PATCH returns 500 | form stays open; inline error visible; no alert() |
|
||||
| AC-7 | i18n keys exist in both bundles | passes the existing AZ-465 parity assertion |
|
||||
| AC-8 | Add + delete unchanged | full re-run of the existing AdminPage tests |
|
||||
|
||||
## Blackbox Tests
|
||||
|
||||
| AC Ref | Initial Data/Conditions | What to Test | Expected Behavior | NFR References |
|
||||
|--------|------------------------|--------------|-------------------|----------------|
|
||||
| AC-2 + AC-3 | Logged in as admin; classes table has ≥ 3 rows | Click edit on row 2; change name; Save | DevTools shows one PATCH; row 2's name updates in place | Performance |
|
||||
| AC-4 | Same | Click edit on row 2; change name; Cancel | No PATCH; row 2 unchanged | — |
|
||||
| AC-5 | Same | Click edit on row 2; clear name; Save | No PATCH; inline error visible next to name input | — |
|
||||
| AC-6 | Same; backend stubbed to return 500 on PATCH | Click edit on row 2; change name; Save | Inline error visible; form stays open | Reliability |
|
||||
| AC-7 | Switch language between en and ua | Click edit on any row | Form labels + error messages render in the active language | — |
|
||||
|
||||
## Constraints
|
||||
|
||||
- Use the existing `endpoints.admin.class(id)` builder. Do not introduce a new endpoint helper for PATCH — the URL is the same as DELETE and that's the wire-contract single-source-of-truth invariant established by AZ-486.
|
||||
- Use the existing `api.patch()` helper. Do not call `fetch()` directly.
|
||||
- Render the inline form **inside the same `<tr>`** as the row being edited — do NOT open a modal or a side drawer. The legacy WPF behaviour (per `_docs/legacy/wpf-era.md` §10 and `_docs/ui_design/`) is in-row inline edit.
|
||||
- Every new visible string MUST exist in both `en.json` and `ua.json` (P6 enforcement); the AZ-465 i18n parity test will fail otherwise.
|
||||
- Do not use `alert()` or `window.confirm()` for errors (Finding B4 anti-pattern); inline messages only.
|
||||
|
||||
## Risks & Mitigation
|
||||
|
||||
**Risk 1: Backend endpoint does not exist** *(highest)*
|
||||
- *Risk*: `PATCH /api/admin/classes/{id}` may not be implemented in `../admin/`; the form would 404 in production.
|
||||
- *Mitigation*: The Cross-Workspace Verification gate above is BLOCKING. The implementer must verify before writing the form. If missing, the gate's Choose A/B/C/D forces a decision; we do not paper over with a stub.
|
||||
|
||||
**Risk 2: PATCH semantics — full body vs partial body**
|
||||
- *Risk*: The backend may treat PATCH as full-body (replace, like PUT) rather than partial (merge). If so, an undocumented absent field could be silently nulled.
|
||||
- *Mitigation*: Always send the complete `editForm` (every field from the seeded row). This is the safer default regardless of backend semantics. Document the decision in the implementation report.
|
||||
|
||||
**Risk 3: Two rows in edit mode simultaneously**
|
||||
- *Risk*: Subtle UI bug — clicking "edit" on row 3 while row 2 is still in edit mode could leave both open if state is per-row.
|
||||
- *Mitigation*: Use a single `editingId: number | null` state (NOT per-row) so opening one row's editor automatically closes any other. AC-2 explicitly asserts this.
|
||||
|
||||
**Risk 4: Cancel after partial save (network in-flight)**
|
||||
- *Risk*: User clicks Save, then Cancel before the PATCH resolves. Race condition between server-side success and client-side cancel.
|
||||
- *Mitigation*: Disable the form (or at least Save + Cancel buttons) while a PATCH is in flight, with a spinner indicator. The 200 response always wins — the form closes; no further action on Cancel.
|
||||
|
||||
**Risk 5: i18n drift introduced by missed keys**
|
||||
- *Risk*: A new error string in en.json without the matching ua.json key breaks AZ-465's parity test.
|
||||
- *Mitigation*: Add all six new keys to BOTH bundles in the same commit. Run `bun run test tests/i18n_parity.test.ts` (or whatever the AZ-465 test path is) locally before marking the task done.
|
||||
|
||||
## References
|
||||
|
||||
- `_docs/02_document/architecture.md` — Architecture Vision principle P12.
|
||||
- `_docs/02_document/04_verification_log.md` — F10 (Class edit affordance missing).
|
||||
- `_docs/02_document/components/08_admin/description.md` — current Admin page surface.
|
||||
- `src/features/admin/AdminPage.tsx` — implementation target.
|
||||
- `src/api/endpoints.ts:30` — `endpoints.admin.class(id)` (existing PATCH/DELETE target).
|
||||
- `src/api/client.ts:106` — `api.patch()` helper.
|
||||
- `_docs/02_tasks/done/AZ-466_test_destructive_ux.md` — Finding B4 / no-alert anti-pattern enforced via `<DestructiveButton>` and static check.
|
||||
- `_docs/02_tasks/done/AZ-465_test_i18n.md` — i18n parity test that protects AC-7.
|
||||
Reference in New Issue
Block a user