feat(slice-4a): wire Findings kanban status-change to POST /api/findings/:id/status#27
Merged
Merged
Conversation
…ngs/:id/status
v1.7 slice 4a — Findings page actions per audit doc 4a.
The Kanban view's terminal-transition confirm modal was previously
client-side-only: `doMove` flipped local state and toasted, with the
reason textarea unwired. Now:
- `doMove` POSTs to `/api/findings/:id/status` (the v1.4 endpoint that
was already implemented but unused by the UI)
- Body shape: `{status, reason}` (matches server contract; rejected
with 400 if either is missing)
- Optimistic UI update happens AFTER the server confirms, so a
4xx/5xx leaves the card in its original column with a clear error
- The reason textarea is now controlled (`value`/`onChange`) and
Confirm is disabled until the reason is non-empty (server requires
it; matching client-side prevents the round-trip on empty input)
- Inline `<Alert kind="error">` surfaces the server's error message
without closing the modal, so the user can fix the reason + retry
- Cancel is disabled while a request is in flight to prevent
abandoning a mid-flight POST
- Non-terminal moves (drag to `draft`) also POST so the change lands
in the audit chain, with a default reason indicating drag-origin
Adds test ids on cards (`kanban-card-<id>`) and columns
(`kanban-col-<key>`) plus a 4-test Playwright e2e suite covering:
modal opens on terminal-drop, reason-required disable/enable,
happy-path 200 + modal close + correct request body, 4xx keeps
modal open and shows server error.
Audit doc updated:
- line 3837/3840 (kanban card actions) marked **DONE**
- slice 4a checklist item marked **SHIPPED**
Tests: @aqa/admin 44 Playwright (40 existing + 4 new). Lint +
typecheck clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR wires the admin Findings kanban status transition flow to the existing server status endpoint and adds e2e coverage for terminal drag/drop confirmation behavior.
Changes:
- Adds POST-based status changes from the kanban, with required reason input and inline error display.
- Adds Playwright coverage for terminal status transitions and server error handling.
- Updates the internal placeholder audit document to mark the kanban slice as shipped.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
packages/admin/src/app.tsx |
Wires kanban status moves to /api/findings/:id/status, adds controlled reason/error state, and test IDs. |
packages/admin/test/e2e/findings-status.e2e.ts |
Adds Playwright tests for terminal drag/drop confirmation and request handling. |
docs/internal/admin-placeholder-audit.md |
Marks the kanban placeholder audit item and slice 4a checklist as shipped. |
Comment on lines
+4221
to
+4222
| Required. Recorded as <code>finding.status_changed</code> event in the audit chain. | ||
| Confirm is disabled until you provide a non-empty reason. |
| - `3521`, `3524` — verify / reject row actions | ||
| - `3609` — open detail | ||
| - `3837`, `3840` — kanban card actions | ||
| - ~~`3837`, `3840` — kanban card actions~~ **DONE (slice 4a, PR #27):** the kanban confirm-transition modal is now wired to `POST /api/findings/:id/status` with required reason input + error alert + disabled-until-reason submit. Drag-and-drop opens the modal; happy-path closes it and moves the card; 4xx/5xx keeps the modal open with the server's error message. 4 Playwright e2e tests cover the flow. |
| Doing all 81 in one PR is unreviewable. Plan: **one PR per page** so each is a manageable review surface. | ||
|
|
||
| 1. **slice 4a — Findings page actions** (verify, reject, mark-fixed, mark-duplicate row actions). Needs `PATCH /api/findings/:id/status` (already exists in `packages/server` from v1.4). Just wire the UI buttons. | ||
| 1. ~~**slice 4a — Findings page actions** (verify, reject, mark-fixed, mark-duplicate row actions). Needs `PATCH /api/findings/:id/status` (already exists in `packages/server` from v1.4). Just wire the UI buttons.~~ **SHIPPED (PR #27).** Kanban terminal-transition modal wired to `POST /api/findings/:id/status`. Drag-and-drop, required-reason capture, error-on-fail, optimistic UI only after server confirmation. Remaining row actions in clusters/list views (verify/reject inline buttons) deferred to a follow-up PR if/when those views grow real row actions — today the kanban is the canonical status-change surface. |
Comment on lines
+4038
to
+4043
| // Non-terminal moves (only `draft` today) don't require a reason — | ||
| // we still POST to the server so the change lands in the audit | ||
| // chain, but the body uses a default reason. If the server | ||
| // rejects, we surface a toast and revert nothing (the local | ||
| // state never actually flipped). | ||
| void doMove(f.id, col.key, '(non-terminal move from kanban drag)'); |
Comment on lines
+4074
to
+4075
| setItems((prev) => prev.map((x) => (x.id === id ? { ...x, status } : x))); | ||
| toast.push({ title: 'Status updated', body: `${id} → ${status}`, kind: 'success' }); |
Comment on lines
+4203
to
+4205
| <Alert kind="error" title="Status change failed"> | ||
| {submitError} | ||
| </Alert> |
PR #27 iter 1 review fixes (Copilot — all 6 items real): 1. **app.tsx:4222 + audit doc** — UI hint and audit-doc bullet claimed the kanban writes a `finding.status_changed` event to the audit chain. The server's `updateFindingStatus` only mutates the store today; the `EventKind` schema doesn't define that kind yet. Reworded both to say "persisted to the store" + explicitly call out the missing audit-event write as a v1.7.x follow-up. 2. **app.tsx:4205 a11y** — the shared `Alert` component is a plain div, so a server error appearing after submit was invisible to screen-reader users. Added role logic: `role="alert"` for `kind="error"|"warning"` (implicit aria-live="assertive"), `role="status"` for `kind="success"` (polite), no live region for decorative info/ai banners. 3. **app.tsx:4075 — happy-path test didn't assert card moved** The 200 path test now asserts the dropped card actually appears in the target column with the expected `data-finding-status`, not just that the modal closed. 4. **app.tsx:4043 — non-terminal drag untested** The non-terminal path (drag to `draft`) bypasses the confirm modal but still POSTs to the server. Added two new e2e tests: one for the happy-path (modal stays closed, default reason forwarded), one for the failure path (4xx leaves the card in its original column without moving). Tests: @aqa/admin 46 Playwright (was 44, +2 for non-terminal path). Lint + typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
iter 2 on |
| data-testid="kanban-confirm-submit" | ||
| disabled={submitting || reason.trim() === ''} | ||
| onClick={async () => { | ||
| const ok = await doMove(confirm.finding.id, confirm.toCol.key, reason.trim()); |
Comment on lines
+4049
to
+4052
| // we still POST to the server so the change lands in the audit | ||
| // chain, but the body uses a default reason. If the server | ||
| // rejects, we surface a toast and revert nothing (the local | ||
| // state never actually flipped). |
| // chain, but the body uses a default reason. If the server | ||
| // rejects, we surface a toast and revert nothing (the local | ||
| // state never actually flipped). | ||
| void doMove(f.id, col.key, '(non-terminal move from kanban drag)'); |
… CI typecheck PR #27 iter 2 review fixes (Copilot — 3 real new items + 1 CI failure): ## CI typecheck failure (workspace-wide tsc was stricter than admin's own check) Lines 120-121 of the new findings-status.e2e.ts hit TS2339 "Property 'status' does not exist on type 'never'" because a `let posted: Body | null = null` closed-over by a `page.route` callback narrows to `null` (literal) in the outer scope, then `null?.status` resolves to `never` under the workspace TS config. Worked around with a `const captured = { value: ... }` wrapper — TS doesn't narrow object properties through closures. ## Real new issues 1. **Schema-invariant transitions (Copilot app.tsx:4190)**: the Finding schema requires `duplicate_of` for `status='duplicate'` and `verification.deterministic === true` with attempts >= 1 for `status='verified'`, but the wizard only collects `{status, reason}`. Dragging a card to either column would persist a schema-invalid finding. Until those extra fields are wired (a v1.7.x follow-up that needs both the wizard and the server endpoint extended), the modal now surfaces a clear `<Alert kind="warning">` for Duplicate/Verified targets explaining the gap and that re-validation may fail. The user can still proceed (the status mutation will land in the store) and complete the missing fields via the YAML editor. New regression tests: dragging to Duplicate shows the warning; same for Verified. 2. **Comment alignment (Copilot app.tsx:4052)**: the doMove docstring claimed the POST "lands the change in the audit chain". The current store implementation only mutates `status` on the finding record — appending `finding.status_changed` to the audit chain is a separate v1.7.x follow-up. Comment now reflects that. 3. **State isolation (Copilot app.tsx:4053)**: the non-terminal drag path shared `submitting`/`submitError` state with the terminal-transition modal. A slow / failing drag-to-draft request could disable a separately-opened terminal modal or show an unrelated error inside it. `doMove` now takes a `setOnModal` boolean — terminal transitions route state into the modal; non-terminal moves stay toast-only. ## Stale re-flags (no action needed) Three iter-2 comments re-flagged items already addressed in this push: non-terminal drag untested (covered by 2 new tests), happy-path success-state assertion (added), Alert a11y (role attribute added in iter 2). Tests: @aqa/admin 48 Playwright (was 46, +2 schema-invariant warnings). Lint + typecheck clean (locally + workspace-wide). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
iter 3 on |
Comment on lines
+4082
to
+4085
| const res = await fetch(`/api/findings/${encodeURIComponent(id)}/status`, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ status, reason: reasonText }), |
| // non-terminal POST shouldn't disable a different terminal modal | ||
| // the user might open while the drag-to-draft request is in | ||
| // flight. Failures surface via toast only. | ||
| void doMove(f.id, col.key, '(non-terminal move from kanban drag)', false); |
| Required by the server. Confirm is disabled until you provide a non-empty reason. | ||
| Note: status transitions are persisted to the store today, but the server-side | ||
| hook that appends a <code>finding.status_changed</code> event to the audit chain | ||
| is a follow-up — until then the reason text is stored on the finding record only. |
| - `3521`, `3524` — verify / reject row actions | ||
| - `3609` — open detail | ||
| - `3837`, `3840` — kanban card actions | ||
| - ~~`3837`, `3840` — kanban card actions~~ **DONE (slice 4a, PR #27):** the kanban confirm-transition modal is now wired to `POST /api/findings/:id/status` with required reason input + error alert + disabled-until-reason submit. Drag-and-drop opens the modal (terminal columns) or POSTs directly with a default reason (non-terminal `draft` column); happy-path closes the modal and moves the card; 4xx/5xx keeps the modal open with the server's error message and leaves the card in its original column. 6 Playwright e2e tests cover the flow (4 terminal + 2 non-terminal). **Not yet wired:** the server-side hook that appends a `finding.status_changed` event to the audit chain — `MemoryStore.updateFindingStatus` today only mutates the finding record; the EventKind schema doesn't yet define `finding.status_changed`. Tracked as a v1.7.x follow-up. |
…e audit alignment PR #27 iter 3 review fixes (Copilot — 4 real new items + 2 stale re-flags): ## Real fixes 1. **Hard-coded relative URL** (Copilot app.tsx:4085): the fetch used `/api/findings/...` which works when admin and server share an origin but breaks the documented deployment where admin runs on Vite dev and @aqa/server is at a separate origin (spec calls for `VITE_AQA_SERVER_URL`). New `apiUrl(path)` helper composes the base from `import.meta.env.VITE_AQA_SERVER_URL` (falling back to relative paths for the same-origin case) and is exposed on `window.__aqaApiUrl` for direct e2e introspection. New regression test asserts the helper's composition rules. 2. **Racing transitions** (Copilot app.tsx:4056): non-terminal drag left the card draggable while the POST was in flight, so two drags in quick succession on the same card could submit competing transitions (last response wins, possibly inconsistent with server state). Now a per-finding `pending` Set guards doMove — cards in flight are non-draggable and carry `data-finding-pending="true"` for UI/test introspection. A second drag on a pending card surfaces a "skipped" toast and the POST is not re-issued. New regression test holds a POST open and asserts the pending attribute flips correctly. 3. **Modal subtitle ↔ field-hint inconsistency** (Copilot app.tsx:4273): the modal subtitle still said "will be logged to the audit chain" while my iter-2 field hint correctly said the audit-chain hook is a follow-up. Aligned: subtitle now carries the v1.7.x-follow-up caveat; field-hint defers to the subtitle. 4. **Audit doc test count** (Copilot audit-doc:32): said "6 Playwright tests" but the slice now ships 10 (the iter-2 non-terminal pair, the iter-3 schema-invariant pair, plus the apiUrl helper + pending-lock additions in this push). Updated. ## Stale re-flags (no action needed) - app.tsx:4102 happy-path success-state assertion (added in iter 2) - app.tsx:4232 Alert a11y role (added in iter 2) Tests: @aqa/admin 50 Playwright (was 48, +1 apiUrl helper, +1 pending-lock). Lint + typecheck clean (locally + workspace). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
iter 4 on |
Comment on lines
+4106
to
+4122
| if (pending.has(id)) { | ||
| toast.push({ | ||
| kind: 'warning', | ||
| title: 'Status change skipped', | ||
| body: `${id} already has a transition in flight — wait for it to land before retrying.`, | ||
| }); | ||
| return false; | ||
| } | ||
| if (setOnModal) { | ||
| setSubmitting(true); | ||
| setSubmitError(null); | ||
| } | ||
| setPending((prev) => { | ||
| const next = new Set(prev); | ||
| next.add(id); | ||
| return next; | ||
| }); |
| }} | ||
| title={`Confirm transition to ${confirm?.toCol.label}`} | ||
| sub={`Moving "${confirm?.finding?.title}" to a terminal status. Provide a reason — this will be logged to the audit chain.`} | ||
| sub={`Moving "${confirm?.finding?.title}" to a terminal status. Provide a reason — it will be persisted on the finding record. (The matching audit-chain event is a v1.7.x follow-up.)`} |
Comment on lines
+4297
to
+4302
| for this transition to be schema-valid. Today the API only persists{' '} | ||
| <code>status</code> and <code>reason</code> — the extra fields aren't yet wired | ||
| through the wizard or the server endpoint, so the resulting finding may fail | ||
| re-validation. Tracked as a v1.7.x follow-up. You can still proceed; the change | ||
| will land in the store and you can fill in the missing fields via the YAML editor. | ||
| </span> |
Comment on lines
+4089
to
+4092
| // The server today only mutates `status` on the finding record — | ||
| // appending a `finding.status_changed` event to the audit chain is | ||
| // a v1.7.x follow-up (EventKind enum + store wiring). This function | ||
| // POSTs the change but does NOT claim audit-chain coverage yet. |
…nce copy PR #27 iter 4 review fixes (Copilot — 4 real new items + 3 stale re-flags): ## Real fixes 1. **Re-entrancy guard was stale-state** (Copilot app.tsx:4122): `if (pending.has(id))` checked React state which doesn't update within the same tick. Two rapid `doMove` calls in the same microtask would both see `pending` empty and both fire POSTs. Added a synchronous `pendingRef.current` (a Set mutated inline before the await) for the gate check; the React state still tracks the same set so the data-finding-pending attribute and draggable toggle continue working. New e2e holds the first POST open, fires the Confirm click, and asserts exactly ONE network POST despite the rapid path. 2-3. **Reason isn't actually persisted server-side** (Copilot app.tsx:4240 + app.tsx:4302): the modal subtitle and the schema-invariant warning both claimed the reason "will be persisted on the finding record" / "Today the API only persists status and reason". Verified in `MemoryStore.updateFindingStatus` (memory.ts:117) — the reason parameter is prefixed with `_` (intentionally unused) and dropped on the floor. Only `status` is mutated. Both copy locations now say the reason is **required by the endpoint** for non-emptiness but **dropped by the store**, making both server-side reason persistence AND the audit-chain hook explicit v1.7.x follow-ups. Audit doc updated to match. 4. **PR description audit-chain wording** (Copilot app.tsx:4092 ← actually a PR-description nit): in this commit's body and the audit-doc bullet, the non-terminal drag is described as "persists to the store" rather than "lands in the audit chain" — aligning with the actual server behavior. ## Stale re-flags (no action needed) - app.tsx:4144 happy-path success-state assertion (added iter 2) - app.tsx:4281 Alert a11y role (added iter 2) - app.tsx:4080 non-terminal background race (per-finding pending lock added iter 3) Tests: @aqa/admin 51 Playwright (was 50, +1 synchronous re-entrancy guard). Lint + typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
iter 5 on |
Comment on lines
+256
to
+257
| // Wait for callCount to settle. | ||
| await page.waitForTimeout(150); |
Comment on lines
+4163
to
+4166
| const msg = e instanceof Error ? e.message : String(e); | ||
| const full = `Could not reach /api/findings/${id}/status (${msg}). The admin is in mock-data mode or the server is down — the status change was not persisted.`; | ||
| if (setOnModal) setSubmitError(full); | ||
| toast.push({ kind: 'error', title: 'Status change failed', body: full }); |
… test PR #27 iter 5 review fixes (Copilot — 2 real new items + 5 stale re-flags): ## Real fixes 1. **Network-error message hardcoded the URL** (Copilot app.tsx:4166): the fetch call used `apiUrl(...)` (which honors `VITE_AQA_SERVER_URL`) but the catch-branch error message was built from the raw template `/api/findings/${id}/status`. In a deployment with a separate server origin, the toast would point at a URL different from where the actual request was sent — making debugging harder. Refactored to resolve the URL once (`reqUrl = apiUrl(...)`) and reuse the same string in fetch + error message. 2. **Flaky `waitForTimeout(150)`** (Copilot e2e:257): the rapid double-submit regression test used a fixed-duration wait to settle `callCount`, which races on slow CI. Replaced with a deterministic signal — wait for the modal to close (which only happens after the held response resolves and `setItems` lands). `callCount` is settled by the time the modal disappears. ## Stale re-flags (no action needed) Copilot's iter-5 review repeated 5 items already addressed: happy-path success assertion (iter 2), Alert a11y role (iter 2), non-terminal background race / pending lock (iter 3), warning copy "Today the API only persists status and reason" (iter 5 — text already says "status on the finding record / reason ... dropped by the store"), PR-description audit-chain wording. Tests: @aqa/admin 51 Playwright (unchanged count, one test now deterministic-wait instead of timeout). Lint + typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
iter 6 on |
lopadova
added a commit
that referenced
this pull request
May 19, 2026
…owing PR #28 iter 1 review fixes (Copilot — 2 real items + CI typecheck): ## Workspace TS narrowing (CI) `let posted: T | null = null` inside a `page.route` closure narrows to `null` in the outer scope under the workspace TS config, so `posted?.yaml` becomes a `never` property access. Same pattern + fix as PR #27 iter 2: use a `const captured = { value: ... }` wrapper — TS doesn't narrow object properties through closures. Applied to both `happy-path` and `409 duplicate` tests. ## Real iter-1 fixes 1. **2xx + empty/non-JSON body treated as success** (Copilot app.tsx:4735): a 200 response with empty body left `parsed = null` (falsy), so `setResult(null)` kept the wizard in form-state while the success toast fired. The user saw a success notification but the modal didn't transition to the result panel — confusing. Now the wizard explicitly checks for the documented `{pack: {...}}` shape in the response. If it's missing (empty body, non-JSON, or unexpected shape) we treat it as an integration error and surface a clear message. New regression test: a 200 with empty body shows the error alert and does NOT show the result panel. 2. **File-input UX** (Copilot app.tsx:4716): after successfully reading a file into the textarea, the wizard didn't clear a stale "could not read file" error from a prior selection. Additionally, the `<input type="file">` value persisted, so re-selecting the same file didn't fire `onChange` in Chrome/Edge. Both fixed: `setError(null)` on success, and `fileInput.value = ''` in a `finally` block so re-selection works reliably. Tests: @aqa/admin 56 Playwright (was 55, +1 for 2xx-without-pack). Lint + typecheck clean (locally + workspace-wide). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
lopadova
added a commit
that referenced
this pull request
May 19, 2026
…#28) * feat(slice-4b): admin Import-manifest wizard + POST /api/packs/import v1.7 slice 4b — wires the "Import manifest" placeholder button on the Packs page to a real wizard that POSTs a YAML pack manifest to a new server endpoint, which parses + validates + installs into the store. ## Server side (@aqa/server) - New `POST /api/packs/import` endpoint (`packs:install` permission) - Body: `{ yaml: string, force?: boolean }` - Parses YAML server-side (the existing `POST /api/packs` accepts pre-parsed JSON; this endpoint accepts raw `pack.yaml` text so the admin doesn't need to YAML-parse client-side) - Validates against `@aqa/schemas/PackManifest` (canonical Zod schema) - On duplicate name without `force=true`, returns 409 with `code: 'EEXIST'` (reuses the structured-error pattern from PR #26) - Adds `yaml` workspace dep to `@aqa/server` - 8 new unit tests: happy-path 201, missing body.yaml, YAML parse error, schema-invalid manifest, 409 duplicate, force=true overwrite, non-boolean force rejected, permission requirement ## Admin UI (@aqa/admin) - New `<ImportManifestWizard>` modal opened from the (previously silent) "Import manifest" button - YAML textarea + native file picker for loading a `pack.yaml` from disk; selecting a file fills the textarea, submit still requires explicit click - Force-overwrite checkbox surfaces the 409-retry path - Result panel shows installed name + version + next-step guidance (matching pack directory still needs to exist on disk for `aqa run` to discover it) - All errors render inline as `<Alert kind="error" role="alert">` (a11y carried over from slice 4a) - 4 Playwright e2e tests: open/disabled-submit, happy-path 201, schema-validation 400 keeps wizard open, 409 → toggle force → retry succeeds ## Audit doc - Lines 6850/6853 marked **DONE** (Install pack via PR #26 + Import manifest via this PR) - Slice 4b checklist item marked **SHIPPED** ## Tests - @aqa/server: 34 unit (was 26, +8 import tests) - @aqa/admin: 55 Playwright (51 existing + 4 import wizard) - Lint + typecheck clean Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(slice-4b): 2xx-without-pack as error, file-input cleanup, TS narrowing PR #28 iter 1 review fixes (Copilot — 2 real items + CI typecheck): ## Workspace TS narrowing (CI) `let posted: T | null = null` inside a `page.route` closure narrows to `null` in the outer scope under the workspace TS config, so `posted?.yaml` becomes a `never` property access. Same pattern + fix as PR #27 iter 2: use a `const captured = { value: ... }` wrapper — TS doesn't narrow object properties through closures. Applied to both `happy-path` and `409 duplicate` tests. ## Real iter-1 fixes 1. **2xx + empty/non-JSON body treated as success** (Copilot app.tsx:4735): a 200 response with empty body left `parsed = null` (falsy), so `setResult(null)` kept the wizard in form-state while the success toast fired. The user saw a success notification but the modal didn't transition to the result panel — confusing. Now the wizard explicitly checks for the documented `{pack: {...}}` shape in the response. If it's missing (empty body, non-JSON, or unexpected shape) we treat it as an integration error and surface a clear message. New regression test: a 200 with empty body shows the error alert and does NOT show the result panel. 2. **File-input UX** (Copilot app.tsx:4716): after successfully reading a file into the textarea, the wizard didn't clear a stale "could not read file" error from a prior selection. Additionally, the `<input type="file">` value persisted, so re-selecting the same file didn't fire `onChange` in Chrome/Edge. Both fixed: `setError(null)` on success, and `fileInput.value = ''` in a `finally` block so re-selection works reliably. Tests: @aqa/admin 56 Playwright (was 55, +1 for 2xx-without-pack). Lint + typecheck clean (locally + workspace-wide). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(slice-4b): concise Zod errors, code assertions, apiUrl in Create-pack, audit doc PR #28 iter 2 review fixes (Copilot — 6 real new items + 2 stale re-flags): ## Real fixes 1. **Verbose Zod error.message** (Copilot api.ts:456): the schema-invalid response surfaced `validated.error.message`, which is a multi-line JSON-ish dump (often 5KB+) and unreadable in the admin's inline alert. New `formatZodError(err)` helper walks `err.issues` and produces a concise `path: message; path: message` list ("applies_when.sut_type: Required" etc). Falls back to a truncated `message` if `issues` is missing. Test asserts the error is not a multi-line dump. 2. **Test code-field assertions** (Copilot api.test.ts:214 + 228): the YAML-parse-failure and schema-invalid-manifest tests asserted only the status + error string, not the structured `code` field. Both now assert `code === 'EINVAL'` to lock the contract. 3. **CreatePackWizard didn't use apiUrl()** (Copilot app.tsx:4739): ImportManifestWizard correctly used `apiUrl(...)` to honor VITE_AQA_SERVER_URL, but CreatePackWizard still hard-coded the relative path. Now both pack wizards go through `apiUrl()`, keeping the deployment story consistent. 4. **`POST POST` in audit doc** (Copilot audit-doc:57): the wizard description double-prefixed "POSTs to `POST /api/packs/import`". Reworded to "calls `POST /api/packs/import`". 5. **Endpoint-consolidation note** (Copilot api.ts:466): the pre-existing `POST /api/packs` route doesn't validate or conflict-check (MemoryStore.installPack silently overwrites), while `/api/packs/import` does both. Consolidating onto a shared validate-then-install helper is the right architectural move, but it'd change long-standing behavior in the JSON route that callers may depend on — out of scope for this slice. Added an inline NOTE on the `POST /api/packs` handler tracking it as a v1.7.x follow-up and steering callers toward `/api/packs/import` for safety guarantees. ## Stale re-flags (no action) `app.tsx:4746` (2xx-without-pack as error) and `app.tsx:4721` (file-input clear-error + reset-value) were both addressed in iter 2's c8fede7 push — Copilot's iter-2 review re-flagged them based on stale line numbers. Tests: @aqa/server 34 (unchanged count; 2 tests upgraded to assert `code`). @aqa/admin 56 Playwright. Lint + typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First sub-slice of v1.7 slice 4 (admin placeholder button audit). Wires the Findings kanban's terminal-transition confirm modal — which was previously client-side state + toast only — to the real
POST /api/findings/:id/statusendpoint that's been in@aqa/serversince v1.4 but never connected to the UI.What changed
doMove(FindingsKanban) now POSTs to/api/findings/:id/statuswith{status, reason}<Alert kind="error">surfaces server errors without closing the modalTest plan
bunx playwright test test/e2e/findings-status.e2e.ts— 4 tests pass (modal opens on terminal drop, reason-required gate, happy-path 200 with correct request body, 4xx keeps modal open + shows server error)bun --filter @aqa/admin build— Vite 425 KB / 115 KB gzipbun --filter @aqa/admin test(Playwright suite) — 44 e2e tests pass (40 existing + 4 new)bun run lint— clean (4 pre-existing warnings, 0 errors)bun run typecheck— cleanAudit doc
Out of scope
Up next
POST /api/packs/importendpoint🤖 Generated with Claude Code