v1.7 slice 4c.2: profile edit/save modal wired to PUT /api/profiles/:name#30
Merged
Conversation
…:name
v1.7 slice 4c.2 — first non-destructive CRUD for profiles. Adds an
Edit modal on the profile detail page that submits the commonly-edited
subset of the Profile schema (execution_mode, llm_budget_usd,
parallelism, require_deterministic_replay, packs, tags) and refreshes
the UI without a reload.
Architecture mirrors slice 4c.1's DeleteProfileWizard so the race
conditions we learned about over its nine review iterations don't
need to be re-discovered:
- synchronous in-flight ref guards double-submits before React
re-renders the disabled button (4c.1 iter 4)
- inFlightRef is reset alongside `submitting` when the modal opens
or the source profile swaps (4c.1 iter 5)
- submittedName is captured at fetch start; post-fetch state
mutations are gated on `submittedName === profileName` so a
stale resolve can't write into a different profile's wizard
(4c.1 iter 8)
- handleClose resets state synchronously to prevent stale-state
flicker on close+reopen of the same profile (4c.1 iter 9)
- Modal close affordances (Escape / overlay click / X icon) are
neutralised while submitting via `onClose={submitting ? undefined
: handleClose}` (4c.1 iter 10)
- inline validation (parallelism 1..64 integer, llm_budget_usd ≥ 0)
mirrors the Zod schema so the user gets immediate feedback
The admin's static PROFILES mock uses fictional values that don't
match the Zod schema (execution_mode 'host'/'sandbox', no
parallelism). `deriveProfileForm` coerces those on form open without
mutating the underlying mock array.
UI refresh is driven by a new App-level `updatedProfiles` Map that
listens for `aqa:profile-updated` CustomEvents and lives at the same
lifted-state level as `deletedProfiles`. PageProfileDetail and
PageProfiles merge the override map on top of the static mock, so a
successful PUT is reflected immediately in the detail header and the
list row. The override patch aliases `llm_budget_usd` →
`budget_usd` so the legacy mock-driven display picks up the new value
without us having to migrate every mock-driven UI site in this slice.
Tests (7 new):
- Edit button opens the modal with form pre-filled (verifies the
mock→schema coercion for execution_mode and the default for
parallelism)
- Cancel closes the modal without firing a PUT (route mock
confirms PUT count = 0)
- Out-of-range parallelism disables Save and shows the hint;
in-range re-enables it
- Negative llm_budget_usd disables Save and shows the hint
- Happy path: PUTs a schema-conforming body, toast shown, modal
closes, detail header reflects new execution_mode and budget
(validates the App-level override + alias)
- 4xx keeps modal open and surfaces the server error
- Modal close affordances are inert while PUT is in flight
(overlay click + X click + Escape, with a test-controlled
promise holding the request open)
`toHaveValue` waits after each `fill` in the happy-path test guard
against parallel-worker load races where React's controlled-input
re-render lags behind Playwright's next click.
Verified locally: typecheck clean, lint clean, unit tests 39/39,
profile-edit 7/7, profile-delete 7/7 (combined parallel run 14/14
green).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf21274676
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+5250
to
+5259
| const body = { | ||
| schema_version: '1', | ||
| name: submittedName, | ||
| execution_mode: form.execution_mode, | ||
| llm_usage: Array.isArray(profile?.llm_usage) ? profile.llm_usage : [], | ||
| llm_budget_usd: form.llm_budget_usd, | ||
| parallelism: form.parallelism, | ||
| require_deterministic_replay: form.require_deterministic_replay, | ||
| packs: parseSlugList(form.packs), | ||
| tags: parseSlugList(form.tags), |
There was a problem hiding this comment.
When editing any profile that already has budget_minutes, this full-profile PUT body drops that optional field even though the server/store saves the submitted object as the replacement profile (for example MemoryStore.saveProfile stores profile directly). A user changing only mode/budget would silently remove the profile's wall-clock budget guard from persisted configuration; include the existing profile?.budget_minutes or switch to a server-side merge/validation path.
Useful? React with 👍 / 👎.
| llm_budget_usd: form.llm_budget_usd, | ||
| parallelism: form.parallelism, | ||
| require_deterministic_replay: form.require_deterministic_replay, | ||
| packs: parseSlugList(form.packs), |
There was a problem hiding this comment.
If the user enters an invalid pack slug here (for example uppercase text, spaces, consecutive dashes, or a value over the schema limit), parseSlugList forwards it and Save remains enabled. The /api/profiles/:name handler currently casts req.body as Profile.Profile and stores it without schema validation, so this new UI can persist profiles that @aqa/schemas rejects and later validation/runs will fail; validate the pack list against the slug schema before sending or block Save with an inline error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds the admin profile edit/save flow, wiring the profile detail page to PUT /api/profiles/:name and reflecting saved mock-profile overrides in the detail and list views.
Changes:
- Adds
EditProfileWizardwith editable profile fields, validation, submit guarding, and success/error toasts. - Lifts
updatedProfilesstate toAppviaaqa:profile-updatedevents for immediate UI refresh. - Adds Playwright coverage for opening, validation, save, error handling, cancel, and in-flight close behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
packages/admin/src/app.tsx |
Adds profile edit modal, update event handling, and merged profile overrides in list/detail views. |
packages/admin/test/e2e/profile-edit.e2e.ts |
Adds end-to-end tests for the new profile edit/save workflow. |
| // lives at App level (see `updatedProfiles` in App + the | ||
| // `aqa:profile-updated` event). | ||
| const overrides = name ? updatedProfiles?.get(name) : null; | ||
| const p = isDeleted ? null : rawP ? { ...rawP, ...(overrides || {}) } : null; |
| const v = e.target.value; | ||
| setForm((f) => ({ | ||
| ...f, | ||
| parallelism: v === '' ? 0 : Number.parseInt(v, 10), |
…l parallelism + tick-survivable form state PR #30 iter 2 — addresses 4 review items (2 Codex, 2 Copilot): 1. (Codex) Preserve `budget_minutes` in the PUT body. The wall-clock guard isn't a form field, but the server writes the submitted object as the replacement profile via `MemoryStore.saveProfile`, so omitting the key silently stripped an existing value. The body now forwards it from the source profile when present. 2. (Codex) Validate pack slugs before submit. The server's PUT handler casts `req.body` as `Profile.Profile` without re-running Zod, so without this UI check the user could persist a profile with uppercase / spaced / consecutive-dash / over-length pack slugs. The wizard now mirrors the @aqa/schemas Slug regex (`^[a-z0-9](?:-?[a-z0-9])*$`, max 52 chars) and disables Save with an inline hint when any entry fails. 3. (Copilot) Form state survives App-level re-renders. The reset effect depended on `initial` (memoized from `profile`), but `PageProfileDetail` builds a fresh merged `p` object on every parent render — App's 5-second `lastTick` interval and other state churn would cause the wizard to re-derive `initial`, fire the reset effect, and wipe whatever the user had typed. Re-keyed the reset effect to `[open, profileName]` and read the latest profile via a ref so the wizard resets ONLY on modal-open transitions or genuine profile-name changes. 4. (Copilot) Parallelism input now uses `Number(v)` (not `parseInt(v, 10)`). `parseInt` silently truncated `1.5` → 1 and slipped past the integer check; `Number` preserves the decimal so `Number.isInteger` correctly rejects it. Tests (4 new, 11 total): - Decimal parallelism (1.5) disables Save with the validation hint - Invalid pack slug (uppercase/spaces) disables Save; correcting re-enables - PUT body preserves source `budget_minutes` (injected via `window.PROFILES` mutation before opening the wizard) - User-typed budget survives ~11s of App-level ticks (regression test for the `initial` → `profileName` reset-effect fix) Verified locally with `--workers=2` (matches CI): 11/11 e2e pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
iter 2 on `3bab7b4`: 4 real iter-1 items addressed — 2 from Codex, 2 from Copilot. Codex:
Copilot: Added 4 regression tests (11 e2e total, all green with `--workers=2` matching CI). |
| // over-length) would persist a malformed profile. (Codex review on | ||
| // PR #30.) | ||
| const SLUG_PATTERN = /^[a-z0-9](?:-?[a-z0-9])*$/; | ||
| const MAX_SLUG_LEN = 52; |
| } catch { | ||
| parsed = null; | ||
| } | ||
| const stillCurrent = submittedName === profileName; |
| // lives at App level (see `updatedProfiles` in App + the | ||
| // `aqa:profile-updated` event). | ||
| const overrides = name ? updatedProfiles?.get(name) : null; | ||
| const p = isDeleted ? null : rawP ? { ...rawP, ...(overrides || {}) } : null; |
…se schema modes in detail PR #30 iter 3 — addresses 3 Copilot review items on 3bab7b4: 1. Slug length cap: was 52, should be 64 to match Slug.max(64) in `packages/schemas/src/common.ts:26`. A valid 53–64 char pack slug was being wrongly rejected. (CreatePackWizard's 52 is a tighter UX cap for new pack creation and stays as-is — Profile.packs accepts existing slugs up to the schema limit.) 2. Stale-submit guard is now real. `submittedName === profileName` compared two closure-captured values from the same render, so it was always true inside the in-flight closure — never actually detecting a mid-flight profile swap. The check now reads `profileRef.current?.name` (the always-up-to-date current profile) and compares against the captured `submittedName`. `profileRef` was already in place for the reset-effect fix in iter 2, so this is purely a re-use. 3. Configuration section in PageProfileDetail now lists all four possible execution_mode values. After saving as `agent` / `orchestrator` (schema modes), the override flows into `p.execution_mode`, but the section previously only rendered `sandbox` / `host` radios — leaving no selected mode at all visually. The four-radio version reflects whichever value is current, mock or schema. Tests (2 new, 13 total): - 64-char pack slug is accepted, 65 is rejected with the "exceeds 64" hint - Configuration radios: `host` is checked before save (mock); after saving as `agent`, the `agent` radio is checked and `host` is not — exercises both the override flow and the four-mode list Verified locally: `--workers=2` matches CI, 13/13 e2e pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
iter 3 on `5794f82`: 3 real iter-2 Copilot items addressed.
13/13 e2e green at `--workers=2` (matches CI). Same-pattern stale-submit issue exists in `DeleteProfileWizard` from slice 4c.1; not touching it in this PR for scope discipline since Copilot didn't flag it across 9 iters there — if it comes up later I'll address both. |
Comment on lines
+8964
to
+8968
| const overrides = updatedProfiles ?? new Map(); | ||
| const visible = PROFILES.filter((p) => !deletedNames.has(p.name)).map((p) => { | ||
| const patch = overrides.get(p.name); | ||
| return patch ? { ...p, ...patch } : p; | ||
| }); |
Comment on lines
+5228
to
+5230
| React.useEffect(() => { | ||
| profileRef.current = profile; | ||
| }, [profile]); |
PR #30 iter 4 — 2 Copilot review items addressed on 5794f82: 1. profileRef is now updated during render, not in a post-commit useEffect. A useEffect-driven ref update runs AFTER the render is committed, so if the parent swaps the profile and the in-flight PUT settles before that effect ticks, the stale-submit guard would still read the old profile and run `onSaved` / reset submitting for the wrong session. Render-time assignment closes the race (refs are stable across renders and the assignment is idempotent). 2. PageProfiles header summary now counts every distinct execution_mode present in the visible rows. The previous version hard-counted only `sandbox` + `host`; after saving a profile to a schema mode (`agent`/`orchestrator`) it would silently drop out of the mix even though the table row reflected it. The summary is now derived from a single reduce over `visible`, sorted for stable rendering. Regression test: save the first profile (mock `host`) as `agent`, return to the Profiles list, assert summary contains "1 agent" and "4 sandbox" and no longer contains "host" (1→0 → drops out of the distinct-mode list). 14/14 e2e green at workers=2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
iter 4 on `08dcfae`: 2 real iter-3 Copilot items addressed.
14/14 e2e green at `--workers=2`. |
Comment on lines
+5447
to
+5452
| onChange={(e) => { | ||
| const v = e.target.value; | ||
| setForm((f) => ({ | ||
| ...f, | ||
| llm_budget_usd: v === '' ? null : Number(v), | ||
| })); |
Comment on lines
+5481
to
+5484
| setForm((f) => ({ | ||
| ...f, | ||
| parallelism: v === '' ? 0 : Number(v), | ||
| })); |
…orm state PR #30 iter 5 — addresses 2 Copilot review items on 08dcfae (same root cause for both numeric inputs): The `<input type="number">` browser-level value can transiently be `"-"`, `""`, `"1.5e"`, etc. while the user is mid-edit. The previous onChange did `Number(v)` and stored the result, so those transient values became `NaN` in form state. `value={NaN}` on a controlled input triggers a React warning and produces broken editing behaviour (the cursor jumps, deletion stops responding) before the validator can recover. Switched both `llm_budget_usd` and `parallelism` to RAW STRING form state. `deriveProfileForm` now seeds them as strings (or '' for unset budget); onChange is a plain string passthrough; validation parses with `Number(raw)` and checks finiteness/integer/range at check time; the submit body coerces once at the boundary with `Number(form.parallelism)` and `form.llm_budget_usd === '' ? null : Number(form.llm_budget_usd)`. All 14 existing e2e tests still pass — the `fill('-5')` / `fill('1.5')` / `fill('100')` paths exercise the same hint/disable behaviour via the string-based validator. Verified locally: typecheck clean, lint clean, e2e 14/14 at workers=2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
iter 5 on `77b63b9`: 2 real iter-4 Copilot items addressed (same root cause). Numeric inputs now hold raw strings, not parsed numbers. `` lets the browser transiently hold values like `-`, ``, or `1.5e` mid-edit. The previous `onChange` did `Number(v)` and stored `NaN`, which then fed back through `value={...}` and triggered a React warning + broken editing (cursor jumps, deletion stops responding). Both `llm_budget_usd` and `parallelism` now use raw string form state:
All 14 existing e2e (`fill('-5')`, `fill('1.5')`, `fill('100')`) exercise the same hint/disable behaviour through the string-based validator and still pass. |
| // the legacy alias so the header sub and the list "Budget" cell | ||
| // pick up the new value without us having to migrate every | ||
| // mock-driven UI site to the schema field name in this slice. | ||
| const patch = { ...body, budget_usd: body.llm_budget_usd }; |
Copilot review on PR #30 iter 5: the Edit wizard correctly PUTs `llm_budget_usd: null` when the user blanks the budget, and the submit handler mirrors that into the App override Map as `budget_usd: null`. But the Configuration card on the profile detail page was rendering `<input value={p.budget_usd}>` directly, which produces React's "value prop on input should not be null" warning and a controlled→uncontrolled switch the moment the user saves an unlimited budget. Coalesce null to '' on the read-only display input and add an "Unlimited" placeholder so the empty field is self-documenting. Adds an e2e regression that listens for console errors, blanks the budget, saves, and asserts the legacy display path renders cleanly with no React null-warning fired. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Iter 6 — null-safe legacy budget display (ac7639c)Addresses iter 5 Copilot review (1 real item).
Verification
Iter trend: 4 → 3 → 2 → 2 → 1. Re-requesting Copilot review. |
Comment on lines
+5248
to
+5253
| if (open) { | ||
| setForm(deriveProfileForm(profileRef.current ?? { packs: [], tags: [] })); | ||
| setError(null); | ||
| setSubmitting(false); | ||
| inFlightRef.current = false; | ||
| } |
…tale form flash Copilot review on PR #30 iter 6: the wizard's reset effect is gated on `open`, so a `profileName` change while the modal is closed doesn't refresh the form state. The next time the user opens the wizard for the new profile, the first paint shows the previous profile's form values until the post-commit effect fires and re-derives from the current profile. Re-key the wizard by `p.name` at the PageProfileDetail call site so navigating between profiles forces a fresh mount. The initial useState lazy initializer then seeds form state directly from the current profile — no reliance on a follow-up effect, no stale flash. The 5-second App tick remounts nothing because profileName stays constant within a detail page. Adds an e2e regression that pollutes profile A's wizard form, cancels, navigates to profile B, opens its wizard, and asserts the budget input reflects B's value (not A's polluted sentinel). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Iter 9 — revert
|
| # | Source | Item | Resolution |
|---|---|---|---|
| 1 | Copilot iter 8 | key={p.name} from iter 7 would unmount the wizard mid-PUT on profile change, detaching the in-wizard stale-submit guard (profileRef comparison) and letting the old PUT fire onSaved / a "Profile saved" toast on the new page. |
Reverted the key. Navigation between profiles always passes through PageProfiles (sidebar → Profiles → row click), which unmounts PageProfileDetail and the wizard naturally — so iter 7's "navigating between profiles" test was already covered by the natural unmount/remount, not by the keyed remount. Iter 8's synchronous setForm in handleClose covers close+reopen-on-same-profile. |
Verification
bun run typecheck✓bun run lint✓ (only preexisting unrelated warnings)bunx playwright test profile-edit.e2e.ts --workers=2→ 17/17 ✓ (no test churn — the iter-7 navigation test still passes via the natural unmount path; the iter-8 close+reopen test still passes via handleClose's form reset)
Iter trend: 4 → 3 → 2 → 2 → 1 → 1 → 1 → 1. Re-requesting Copilot review.
Comment on lines
+5347
to
+5350
| const res = await fetch(reqUrl, { | ||
| method: 'PUT', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(body), |
…name Copilot review on PR #30 iter 9: the PUT handler cast `req.body` to a Profile and stored it without schema validation, and did not require the body's `name` to match the path. The admin UI's client-side validation is not a trust boundary — a stale UI bundle, curl, or any non-UI client could persist malformed profiles, and a body that names a different profile would silently create-or-replace the body's name instead of the path's (a path-confusion bug class). Parse with `ProfileSchema.Profile.safeParse(req.body)` and 400 on failure with the existing `formatZodError` formatter, and return a 400 when `req.params.name && profile.name !== req.params.name`. Keep the modal validation as early feedback only. Four new unit tests in api.test.ts cover: schema-conforming happy path, schema rejection (parallelism > 64), name-mismatch rejection, and the route's `profiles:edit` permission stamp. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Iter 10 — server-side Profile validation + name-match (a6646d0)Addresses iter 9 Copilot review (1 real item; security/trust-boundary).
Verification
Iter trend: 4 → 3 → 2 → 2 → 1 → 1 → 1 → 1 → 1 (server-side). Re-requesting Copilot review. |
Comment on lines
+563
to
+571
| // The route name is the canonical identity; a body that names a | ||
| // different profile would silently create-or-replace the body's | ||
| // name instead of the path's, which is a path-confusion class | ||
| // of bug. Reject mismatches with a 400 instead of trusting one | ||
| // side. | ||
| if (req.params.name && profile.name !== req.params.name) { | ||
| return asResponse( | ||
| { | ||
| error: `profile name mismatch: path "${req.params.name}" vs body "${profile.name}"`, |
Comment on lines
+5196
to
+5200
| // the form before the user hits Save. The server PUT handler casts | ||
| // `req.body` as Profile.Profile without re-validating, so without | ||
| // this UI check a typo (uppercase, spaces, consecutive dashes, | ||
| // over-length) would persist a malformed profile. (Codex review on | ||
| // PR #30.) |
…e; refresh stale UI comment Copilot review on PR #30 iter 10 — two real items: 1. Server: the previous mismatch check was gated on `req.params.name && profile.name !== req.params.name`, so a body-only request would persist `body.name` without any path identity — inconsistent with GET/DELETE on the same route which both `notFound('profile')` when the path name is missing. Match that behavior: short-circuit with notFound BEFORE schema parsing when `pathName` is falsy, then enforce the mismatch unconditionally. 2. Admin: the slug-regex comment in app.tsx still said "the server PUT handler casts req.body as Profile.Profile without re- validating" — stale after iter 10's server-side safeParse. Rewrite it to describe the current architecture: server is the trust boundary, the UI check exists for immediate feedback. Adds a unit test that exercises the missing-path-name path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Iter 11 — missing-path-name 404 + stale comment refresh (2702c29)Addresses iter 10 Copilot review (2 real items).
Verification
Iter trend: 4 → 3 → 2 → 2 → 1 → 1 → 1 → 1 → 1 → 2 (server + comment refresh). Re-requesting Copilot review. |
| // Two App-level ticks fire here. Without the fix, the budget | ||
| // input would re-seed to the mock value (5) after the first | ||
| // tick. | ||
| await page.waitForTimeout(11_000); |
… time Copilot review on PR #30 iter 11: the tick-survival regression test slept 11 wall-clock seconds waiting for two App-level 5s intervals to fire — slow and timing-sensitive under parallel- worker CI load. Use Playwright's `page.clock.install()` BEFORE navigation so App's `setInterval` registers against the fake clock, then advance 11 virtual seconds synchronously via `page.clock.runFor(11_000)`. Same two ticks exercised, ~10s faster (12.6s → 1.8s), deterministic, no real timers depended on. Whole profile-edit suite drops from ~35s → ~27s. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Iter 12 — page.clock virtual time for tick-survival test (a198b99)Addresses iter 11 Copilot review (1 perf suggestion).
Verification
Iter trend: 4 → 3 → 2 → 2 → 1 → 1 → 1 → 1 → 1 → 2 → 1 (perf suggestion). Re-requesting Copilot review. |
Comment on lines
+175
to
+179
| // PR #30 iter 2 (Codex): the server's PUT handler doesn't | ||
| // re-validate `req.body`, so without this UI check the user | ||
| // could persist a profile with uppercase / spaced / consecutive- | ||
| // dash pack slugs. The wizard now mirrors the @aqa/schemas Slug | ||
| // regex. |
Copilot review on PR #30 iter 12: the slug-validation e2e test's explanatory comment still claimed the server PUT handler doesn't re-validate `req.body`. Iter 10 made the server the trust boundary (Profile schema safeParse + path/body name-match check), so the comment was contradicting the current implementation. Rewrote to describe the actual architecture: server is the trust boundary; the UI slug check exists for immediate inline feedback before the round-trip. Same fix-shape as iter 11's app.tsx comment rewrite — the test file just escaped that pass. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Iter 13 — refresh stale e2e comment (0b7817c)Addresses iter 12 Copilot review (1 doc item).
Verification
Iter trend: 4 → 3 → 2 → 2 → 1 → 1 → 1 → 1 → 1 → 2 → 1 → 1 (docs-only). Re-requesting Copilot review; expect convergence to zero next pass. |
This was referenced May 19, 2026
lopadova
added a commit
that referenced
this pull request
May 19, 2026
* feat(slice-4c.5): risk edit/save wired to PUT /api/risks/:id
Server:
- PUT /api/risks/:id now schema-validates the body, enforces
body.id === path id, and 404s on missing path id. Same trust-
boundary patterns as PUT /api/profiles/:name (slice 4c.2).
- 5 unit tests added (happy path, schema rejection on bad enum,
id mismatch, missing-path 404, permission)
Admin UI:
- PageRiskEditor's Save button is now wired up: handleSave PUTs a
schema-conforming Risk body (bare-string invariants on legacy
mock rows are coerced to { id, statement } objects so the
server doesn't 400 when the user only edited title/severity).
- Inline UX validation mirrors the schema's min-4-char title; Save
stays disabled until the title is valid (server is still the
trust boundary).
- Architecture lessons carried from PR #30 (Profile Edit):
inFlightRef synchronous double-submit guard, captured
submittedId, render-time riskIdRef, error surfaced as inline
alert with data-testid, header Cancel/Delete disabled during
in-flight Save.
- App-level updatedRisks Map + aqa:risk-updated window event
listener; threaded through ctx like updatedProfiles.
- PageRiskMap merges overrides into the visible list via a memo
keyed on [deletedRisks, updatedRisks] so the matrix and
category views reflect a saved severity/category change.
- PageRiskEditor merges overrides into its useState seed so
a re-opened editor doesn't briefly show stale mock values.
E2E: 4 new tests in risk-edit.e2e.ts, all green with --workers=2
(99 passed, 1 skipped overall).
Server: 58/58. Typecheck clean. Lint preexisting only.
* review(slice-4c.5 iter 1): address Copilot + Codex review feedback
Copilot (1) + Codex P1 (1, same bug):
- Dispatching the full PUT body (with schema-coerced { id, statement }
invariant objects) into updatedRisks would re-merge into the editor
on re-open, but the editor renders r.invariants as bare strings.
Re-opening a previously-saved risk would crash with "Objects are
not valid as a React child". Fix: dispatch only the user-facing
patch (title/category/severity/likelihood) and let the underlying
baseRisk's invariants/id stay untouched. New e2e test exercises
the re-open path and asserts no React child errors appear.
Copilot (1):
- Legacy mock risk and invariant ids use underscores
(risk_cross_tenant_leak, no_raw_query_without_tenant_clause) but
the schema's Slug regex rejects them, so saving an existing mock
risk would 400 on the server's parse step before the id-match
check. Slugify (underscore→dash) at submit time for body.id AND
the path, plus every invariant.id, so the body validates and the
id-match still aligns.
Tests: 5/5 risk-edit e2e green; lint preexisting only.
* review(slice-4c.5 iter 2): revert slugify — surface server's 400 instead
Copilot iter 2 (1 new concern):
- The iter-1 slugify-at-submit (toSlug) fix introduced a worse bug
than the one it solved: editing `risk_cross_tenant_leak` would
PUT `/api/risks/risk-cross-tenant-leak`, server stores under the
dashed id, and later reads/deletes by the displayed underscored
id silently miss the dashed entry → fragmented state.
Reverted to the honest behavior: pass the displayed id through
unchanged. The server's schema-validation will 400 on the legacy
underscored ids and we surface the error inline (the existing
4xx-keeps-page test already covers this). Migrating the mock
fixtures to schema-conforming slugs is a separate hygiene task.
Tests: 5/5 risk-edit e2e green.
* review(slice-4c.5 iter 3): migrate mock risk/invariant ids to dashed slugs
Copilot iter 3 (2, same concern re-raised):
- Mock RISKS fixture used underscored ids (risk_cross_tenant_leak,
no_raw_query_without_tenant_clause, ...) which the schema's Slug
regex rejects, so PUT /api/risks/:id would 400 on schema validation
before reaching the id-match check. Iter 1 tried client-side
slugify; iter 2 reverted that because it fragmented server state
vs displayed ids. The honest, architecturally clean fix is to
migrate the mock fixtures themselves: all 18 risk ids and all
invariant ids are now dashed slugs, FINDINGS.risk_id cross-refs
are updated, and the YAML example in the scenario preview also
uses the new id.
Tests:
- All risk e2e locators that filtered rows by `^risk_` now use
`^risk-` to match the new id shape.
- 100 admin e2e pass (1 skipped); risk-edit 5/5; risk-delete 7/7;
lint preexisting only.
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
v1.7 slice 4c.2 — first non-destructive CRUD for profiles. Adds an Edit modal on the profile detail page that submits the commonly-edited subset of the Profile schema (execution_mode, llm_budget_usd, parallelism, require_deterministic_replay, packs, tags) and refreshes the admin UI without a page reload.
The architecture mirrors slice 4c.1's
DeleteProfileWizardso the same race-condition lessons learned over its nine review iterations are baked in from iter 1 here:useRefguard for double-submitsinFlightRefreset alongsidesubmittingwhen the modal opens or the source profile swaps under ussubmittedNamecaptured at fetch start; post-fetch state mutations gated onsubmittedName === profileNamehandleCloseresets state synchronously to prevent stale-state flicker on close+reopenonClose={submitting ? undefined : handleClose}llm_budget_usd≥ 0) mirrors the Zod schemaUI refresh is driven by a new App-level
updatedProfilesMap listening foraqa:profile-updatedCustomEvents.PageProfileDetailandPageProfilesmerge overrides on top of the staticPROFILESmock so a successful PUT is reflected immediately in the detail header and the list row, without migrating every mock-driven UI site in this slice. The patch aliasesllm_budget_usd→budget_usdfor the legacy display.The admin's static PROFILES mock uses fictional values that don't match the Zod schema (
execution_mode'host'/'sandbox', noparallelism).deriveProfileFormcoerces those on form open without mutating the underlying mock array.Test plan
bun run typecheckcleanbun run lintcleanbun run test— 39/39 unit tests passbunx playwright test profile-edit.e2e.ts— 7/7 passbunx playwright test profile-delete.e2e.ts profile-edit.e2e.ts— 14/14 pass (combined parallel run)7 new e2e tests in
packages/admin/test/e2e/profile-edit.e2e.ts:llm_budget_usddisables Save and shows the validation hint