Skip to content

feat(slice-4c.1): Profile delete confirmation modal#29

Merged
lopadova merged 11 commits into
mainfrom
task/v1.7-slice-4c-delete-crud
May 19, 2026
Merged

feat(slice-4c.1): Profile delete confirmation modal#29
lopadova merged 11 commits into
mainfrom
task/v1.7-slice-4c-delete-crud

Conversation

@lopadova
Copy link
Copy Markdown
Contributor

Summary

First micro-slice of v1.7 slice 4c (Scenarios/Risks/Profiles CRUD). Adds a Delete action to the Profile detail page wired to the existing DELETE /api/profiles/:name endpoint, behind a GitHub-style "type to confirm" modal.

What changed

Admin (@aqa/admin):

  • New Delete button on PageProfileDetail next to Trigger now / Edit
  • New <DeleteProfileWizard> modal: type-to-confirm input, disabled-until-match Delete button, inline error Alert (with role="alert" from the existing a11y refactor), apiUrl() for the request

Server: no changes — DELETE /api/profiles/:name exists since v1.4.

Test plan

  • bunx playwright test test/e2e/profile-delete.e2e.ts — 4 tests pass (modal opens, mismatch disables submit, exact-match enables + DELETE fires with correct URL, 4xx keeps modal open with inline error)
  • bun --filter @aqa/admin build — Vite 435 KB / 119 KB gzip
  • bun run typecheck — clean
  • bun run lint — clean (4 pre-existing warnings)

Audit doc

  • Line 5590/5745/5796 marked as Delete shipped; Save/Clone still pending in subsequent micro-PRs

What's next in slice 4c

  • Profile Save / Clone (uses PUT/POST that already exist)
  • Risk detail Delete + Edit
  • Scenario detail Delete + Edit + Clone

🤖 Generated with Claude Code

…api/profiles/:name

First micro-slice of v1.7 slice 4c (Scenarios/Risks/Profiles CRUD).
The Profile detail page now has a real Delete action.

## What changed

- Profile detail page (`PageProfileDetail`) gets a new red "Delete"
  button next to "Trigger now" / "Edit" — previously absent
- New `<DeleteProfileWizard>` modal opened from the button:
  - GitHub-style "type the profile name to confirm" anti-footgun
  - Delete button is `disabled` until the typed text matches exactly
  - Calls `DELETE /api/profiles/:name` via `apiUrl()` (honors
    `VITE_AQA_SERVER_URL`)
  - On 2xx: toast + close modal + navigate back to the profiles list
  - On 4xx/5xx: modal stays open with inline `<Alert kind="error"
    role="alert">` showing the server's error message
  - Cancel + Delete both disabled mid-flight
- The server endpoint (`DELETE /api/profiles/:name`) already exists
  since v1.4, so this slice is pure UI wiring + e2e

## Tests

- @aqa/admin 60 Playwright (was 56, +4 for the delete flow): button
  opens modal, non-matching name keeps Delete disabled, exact match
  enables + fires DELETE with the right URL, 4xx keeps modal open
- Lint + typecheck clean

## Audit doc

- Line 5590/5745/5796 (profile-level actions: save/clone/delete) →
  **DELETE shipped**; Save/Clone still pending

## What's next in slice 4c

- Profile **Save** (edit flow): wire the PUT /api/profiles/:name
- Profile **Clone**: POST /api/profiles with copied body
- Risk detail Delete + Edit (DELETE/PUT /api/risks/:id already exist)
- Scenario detail Delete + Edit + Clone (DELETE/PUT /api/scenarios/:id)

Each of those becomes a follow-up micro-PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9ee0354a0a

ℹ️ 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 +8558 to +8560
onDeleted={() => {
setDeleteOpen(false);
onNavigate?.('profiles', {});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Remove deleted profiles from the list after success

When the DELETE succeeds, this callback only closes the modal and navigates to profiles, but PageProfiles still renders the static PROFILES array, so the just-deleted profile immediately reappears in the list and can be opened/deleted again even though the server removed it. In the successful-delete path, update the local profile collection or refetch the list before returning to the profiles page so the UI matches the backend state.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an admin-side “Delete profile” action on the Profile detail screen, implemented as a GitHub-style type-to-confirm modal that calls the existing DELETE /api/profiles/:name endpoint. This is part of v1.7 slice 4c.1 (Profiles CRUD micro-slice) and is backed by new Playwright E2E coverage plus an internal placeholder-audit update.

Changes:

  • Added DeleteProfileWizard modal (type-to-confirm, disabled-until-match submit, inline error alert + toast) and wired it to DELETE /api/profiles/:name.
  • Added a new Delete button to PageProfileDetail that opens the modal and navigates back to Profiles on success.
  • Added Playwright E2E tests covering modal open/disable/enable, DELETE request firing, and 4xx error surfacing; updated internal audit doc entry.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
packages/admin/test/e2e/profile-delete.e2e.ts New Playwright E2E suite for the profile delete confirmation flow.
packages/admin/src/app.tsx Implements the delete modal and wires a new Delete action into the Profile detail page.
docs/internal/admin-placeholder-audit.md Marks the profile Delete placeholder as shipped for slice 4c.1.

Comment on lines +68 to +69
// On success the modal closes (we navigate back to /profiles).
await expect(page.locator('.modal-title')).toHaveCount(0);
Comment thread packages/admin/src/app.tsx Outdated
setError(null);
setSubmitting(false);
}
}, [open]);
Comment thread packages/admin/src/app.tsx Outdated
open={open}
onClose={handleClose}
title="Delete profile"
sub={`This permanently removes the "${profileName}" profile. \`aqa run --profile ${profileName}\` will start failing immediately. The action cannot be undone from the admin (you'd need to re-create the profile from scratch).`}
…tronger e2e, profileName dep

PR #29 iter 1 review fixes (Codex P2 + 3 Copilot items):

## Real fixes

1. **Codex P2 — deleted profile reappears in list** (app.tsx:8560):
   `PageProfiles` rendered the static `PROFILES` constant directly, so
   after a successful delete the user returned to the list and saw
   the just-deleted profile still there (and could open/delete it
   again). Fixed via a window-level `aqa:profile-deleted` CustomEvent:
   `DeleteProfileWizard` broadcasts on success; `PageProfiles`
   subscribes and maintains a `deletedNames` Set that filters
   `PROFILES` before render. Works in both mock-mode (no real
   backend) and live-mode (a real refetch would also drop the entry).

2. **Copilot — happy-path test gaps** (e2e:69): the test asserted only
   modal-close + correct URL. Now also asserts the success toast
   appears with the right title AND that the just-deleted row drops
   out of the Profiles table after the post-delete navigation.

3. **Copilot — effect deps incomplete** (app.tsx:4931): the reset
   effect depended only on `open`. If a parent reuses the same
   `DeleteProfileWizard` mount across two different profiles (same
   component, different `profileName` prop), the confirm text from
   the previous profile would stick around. Added `profileName` to
   deps so the reset fires on either trigger.

4. **Copilot — Markdown backticks rendered literally** (app.tsx:4981):
   the modal's `sub` prop was a template-literal string with
   `\`aqa run...\`` backticks, but `<Modal>` renders `sub` as plain
   React children (no Markdown parsing). The backticks appeared
   literally in the UI. Switched `sub` to a JSX fragment with
   `<code>` for the command + `<span className="mono">` for the
   profile name — matches the rest of the admin's code styling.

Tests: @aqa/admin 60 Playwright (unchanged count; happy-path test
upgraded with 2 new assertions). Lint + typecheck clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lopadova lopadova requested a review from Copilot May 19, 2026 08:48
@lopadova
Copy link
Copy Markdown
Contributor Author

iter 2 on 037f73c: all 4 iter-1 items addressed — (1) Codex P2: deleted profile drops from list via window event + filter (test asserts row missing post-delete); (2) Copilot: happy-path test now asserts success toast + post-nav row removal; (3) Copilot: effect deps now include profileName; (4) Copilot: subtitle backticks replaced with JSX <code>. 60 e2e tests, lint+typecheck clean.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment on lines +4944 to +4951
async function handleSubmit() {
if (!canSubmit) return;
setSubmitting(true);
setError(null);
const reqUrl = apiUrl(`/api/profiles/${encodeURIComponent(profileName)}`);
try {
const res = await fetch(reqUrl, { method: 'DELETE' });
const text = await res.text();
Comment thread packages/admin/src/app.tsx Outdated
Comment on lines +8565 to +8567
function PageProfileDetail({ name, onNavigate }) {
const p = profileByName(name) || PROFILES[0];
const [deleteOpen, setDeleteOpen] = React.useState(false);
PR #29 iter 2 review fixes (Copilot — 2 real new items + 1 Codex stale re-flag):

## Real fixes

1. **Silent fallback to PROFILES[0] for unknown route param**
   (Copilot app.tsx:8567): `PageProfileDetail` did
   `profileByName(name) || PROFILES[0]`, so a stale/typoed URL
   would silently display the first profile in the list. With a
   destructive Delete action on this page, that meant a misclick
   could DELETE the wrong profile. Fixed: when the profile isn't
   found, render a clear not-found page with a warning Alert
   ("No such profile") and a Back-to-Profiles button. The Delete
   action is unreachable until the right profile is selected.

2. **Synchronous double-click guard for Delete** (Copilot
   app.tsx:4951): `setSubmitting(true)` is async, so two rapid
   clicks could both pass the `submitting` check before the
   button re-renders disabled and fire two DELETE requests.
   Added an `inFlightRef` mutated inline before the await — same
   pattern as the kanban's pendingRef from slice 4a iter 4. The
   second click short-circuits immediately.

## Stale re-flag (no action)

Codex re-flagged "remove deleted profiles from list" — already
fixed in iter 2 commit 037f73c via the `aqa:profile-deleted` window
event + `deletedNames` filter in PageProfiles. Verified the event
broadcasts and the listener filters correctly with the
strengthened happy-path test (asserts the row drops out).

Tests: @aqa/admin 60 Playwright (unchanged count; all still pass).
Lint + typecheck clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lopadova lopadova requested a review from Copilot May 19, 2026 09:05
@lopadova
Copy link
Copy Markdown
Contributor Author

iter 3 on fda116b: 2 real iter-2 items addressed — (1) Copilot: PageProfileDetail no longer silently falls back to PROFILES[0] for unknown route param — renders a clear not-found state with Back button, preventing destructive misclicks; (2) Copilot: inFlightRef synchronous guard prevents rapid double-click from firing two DELETE requests (same pattern as kanban pendingRef). Codex's P2 re-flag was stale (fix shipped in iter 2). 60 e2e tests, lint+typecheck clean.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment thread packages/admin/src/app.tsx Outdated
Comment on lines +8487 to +8506
function PageProfiles({ onNavigate, onOpenProfile }) {
// Subscribe to delete events so the just-deleted profile drops out
// of the rendered list. In mock-data mode the PROFILES array is a
// static constant — without this subscription, navigating back to
// /profiles after a successful delete would still show the entry.
// In a live deployment with a real backend the page would refetch;
// the event-driven local state makes both behaviors match.
const [deletedNames, setDeletedNames] = React.useState(() => new Set());
React.useEffect(() => {
const handler = (e) => {
const name = e?.detail?.name;
if (typeof name !== 'string') return;
setDeletedNames((prev) => {
const next = new Set(prev);
next.add(name);
return next;
});
};
window.addEventListener('aqa:profile-deleted', handler);
return () => window.removeEventListener('aqa:profile-deleted', handler);
…nges

PR #29 iter 3 review fix (Copilot — 1 real new item + 3 stale re-flags):

## Real fix

**CustomEvent timing race** (Copilot app.tsx:8506): the
`aqa:profile-deleted` listener used to live on `PageProfiles`, but
the event is dispatched from `PageProfileDetail` (still mounted)
right BEFORE we navigate back to `/profiles`. So when `PageProfiles`
finally mounts, its `useEffect` listener was never on the page to
catch the dispatch — the `deletedNames` Set initialized empty and
the row reappeared.

The happy-path test passed only by accident (Playwright was
matching during a transient state).

Lifted the state + listener to `App`, which is always mounted while
the user is signed in:

- `App` owns `deletedProfiles: Set<string>` and the listener
- The set rides on `ctx` and is passed through to both
  `PageProfiles` (filters the rendered list) and
  `PageProfileDetail` (treats just-deleted profiles as not-found
  so a stale link doesn't accidentally re-target the mock row)

The same architecture is reusable for future delete actions on
risks, scenarios, etc — App-level lifted-state for transient mock
mutations.

## Stale re-flags (no action)

Copilot also re-flagged: silent fallback to PROFILES[0] (fixed in
iter 3 with not-found state), submitting double-click race (fixed
in iter 3 with inFlightRef). Codex's "remove deleted profile from
list" was the same comment now correctly addressed by this iter's
architecture fix.

Tests: @aqa/admin 60 Playwright (unchanged count; all 4
profile-delete tests still pass, including the row-drops-from-list
assertion which now works reliably across the route change).
Lint + typecheck clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lopadova lopadova requested a review from Copilot May 19, 2026 09:19
@lopadova
Copy link
Copy Markdown
Contributor Author

iter 4 on b3b60c7: 1 real iter-3 item (timing race) addressed — deletedProfiles Set + listener lifted to App so it survives route changes. PageProfileDetail dispatches event before nav, App listener catches it (App is always mounted), and the Set drips down to both PageProfiles (filters list) and PageProfileDetail (treats deleted names as not-found for stale links). Architecture is reusable for future delete actions on risks/scenarios. Other 3 comments stale re-flags. 60 e2e tests still pass.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

if (open) {
setConfirmText('');
setError(null);
setSubmitting(false);
PR #29 iter 4 review fix (Copilot — 1 real new item + 2 stale re-flags):

## Real fix

**`submitting` vs `inFlightRef` desync on mid-flight profile
switch** (Copilot app.tsx:4940): the reset effect for the modal
(triggered by `open` or `profileName` changing) set `submitting`
to `false` but didn't reset `inFlightRef.current`. If a parent
re-used the same `DeleteProfileWizard` mount with a new
`profileName` while a DELETE was still in-flight, the UI would
re-enable the Delete button (submitting=false) but a click would
silently no-op via the synchronous ref guard — confusing dead
button.

Now the effect resets `inFlightRef.current` together with
`submitting`. The in-flight request's `finally` block becomes a
no-op assignment (setting false on an already-false ref) and the
user can issue a fresh delete against the new profile. The old
request's success path still toasts against the old
`profileName` — acceptable because the user changed context.

A full fix would use `AbortController` to actually cancel the
old request, but that adds material complexity for an edge case
(parents would have to remount the modal on profile switch
instead of re-using it, which they currently do). The state-sync
approach handles the symptom without scope creep.

## Stale re-flags (no action)

Codex re-flagged "remove deleted profile" (fixed iter 4 with App-
level lifted state) and Copilot re-flagged "submitting state
async race" (fixed iter 3 with inFlightRef).

Tests: @aqa/admin 60 Playwright (unchanged count; all 4
profile-delete tests still pass). Lint + typecheck clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lopadova lopadova requested a review from Copilot May 19, 2026 09:29
@lopadova
Copy link
Copy Markdown
Contributor Author

iter 5 on c34d1b1: 1 real iter-4 item — reset effect now resets inFlightRef alongside submitting so a mid-flight profile switch doesn't leave the Delete button visually enabled but functionally dead via the sync guard. Codex P2 and 1 Copilot comment were stale re-flags of items addressed in iters 3-4.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

const calls: Req[] = [];
await page.route('**/api/profiles/*', async (route) => {
calls.push({ url: route.request().url(), method: route.request().method() });
await route.fulfill({ status: 200, contentType: 'application/json', body: '{}' });
… shape

PR #29 iter 5 review fix (Copilot — 1 real new item + 3 stale re-flags):

The happy-path e2e route mock intercepted ALL `**/api/profiles/*`
requests with a `{}` response body regardless of HTTP method. Two
problems Copilot caught:

1. The blanket match would silently fulfill unexpected GET/PUT
   calls during the test, masking any regression that introduced
   extra fetches.
2. The empty `{}` body diverges from the actual `DELETE
   /api/profiles/:name` response shape, which is `{ok: true}` per
   `@aqa/server`'s `asResponse({ ok: true })` (api.ts:303).

Now the mock only intercepts DELETE (other methods get
`route.continue()`) and the response body matches the real
endpoint's shape. The assertion that "exactly one DELETE call"
fires is now stricter — it counts ALL intercepted calls (which
are guaranteed DELETE), not a filtered subset.

Other 3 iter-5 comments were stale re-flags of items addressed
in earlier iters: Codex P2 deleted-row (lifted state iter 4),
Copilot async race (inFlightRef iter 3), Copilot ref/submitting
desync (sync in reset effect iter 5).

Tests: @aqa/admin 60 Playwright, all pass. Lint clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lopadova
Copy link
Copy Markdown
Contributor Author

iter 6 on 3a9a281: 1 real iter-5 item — route mock now DELETE-only with real {ok:true} response shape. 3 stale re-flags.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

// no longer appears in the list (window event drops it).
await expect(page.locator('.page-title, h1').first()).toContainText(/Profiles/i);
await expect(
page.locator('table.tbl tbody tr td.mono', { hasText: new RegExp(`^${name}$`) }),
PR #29 iter 6 review fix (Copilot — 1 real new item + 3 stale re-flags):

**Vacuous selector** (Copilot e2e:93): the "row drops from list"
assertion used `page.locator('table.tbl tbody tr td.mono', ...)`,
but in PageProfiles the profile name is rendered as a
`<span class="id-link mono">` INSIDE a plain `<td>` — there are
no `td.mono` elements anywhere, so `.toHaveCount(0)` always
passed regardless of whether the row was actually there. The
strengthened test from iter 2 was therefore a false positive.

Fixed to:
1. Target rows that CONTAIN a `span.id-link.mono` with the exact
   profile name as text (using a regex-escaped name to handle
   any future special chars).
2. Additionally assert that the table still has OTHER rows, so
   the "deleted row not present" check isn't satisfied vacuously
   by an empty table.

Other 3 iter-6 comments were stale re-flags addressed earlier:
- Codex P2 deleted-row (lifted state iter 4)
- Copilot async submitting race (inFlightRef iter 3)
- Copilot ref/submitting desync (sync in effect iter 5)

Tests: @aqa/admin 60 Playwright, all 4 profile-delete tests pass
with the now-meaningful row assertion. Lint clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lopadova
Copy link
Copy Markdown
Contributor Author

iter 7 on d5a4354: 1 real iter-6 item — fixed the vacuous row-removal selector (td.mono matched nothing because the class lives on a span inside the td). Test now targets span.id-link.mono within rows AND asserts table not empty to prevent vacuous-pass on empty state. Other 3 stale re-flags.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment on lines +4962 to +5003
async function handleSubmit() {
if (!canSubmit) return;
if (inFlightRef.current) return; // synchronous double-click guard
inFlightRef.current = true;
setSubmitting(true);
setError(null);
const reqUrl = apiUrl(`/api/profiles/${encodeURIComponent(profileName)}`);
try {
const res = await fetch(reqUrl, { method: 'DELETE' });
const text = await res.text();
let parsed = null;
try {
parsed = text ? JSON.parse(text) : null;
} catch {
parsed = null;
}
if (!res.ok) {
const msg = parsed?.error ?? `HTTP ${res.status}`;
setError(msg);
toast.push({ kind: 'error', title: 'Delete profile failed', body: msg });
return;
}
toast.push({
kind: 'success',
title: 'Profile deleted',
body: profileName,
});
// Broadcast the deletion so the list page (or any other view
// showing this profile) can drop it from its local state.
// Without this, navigating back to /profiles would still show
// the just-deleted entry (in mock-data mode the PROFILES array
// is a static module constant; in live mode the page would
// refetch — the event makes both cases consistent).
try {
window.dispatchEvent(
new CustomEvent('aqa:profile-deleted', { detail: { name: profileName } }),
);
} catch {
// CustomEvent unsupported in this runtime — non-fatal.
}
onDeleted?.();
} catch (e) {
PR #29 iter 7 review fix (Copilot — 1 real new item + 3 stale re-flags):

**Stale-closure race after mid-flight profile switch** (Copilot
app.tsx:5003): an in-flight DELETE fetch for the previous
`profileName` could still resolve after the user navigated to a
different profile-detail page (same wizard mount, new
`profileName` prop). The post-fetch code would then call
`setError(...)`, `setResult(...)`, and `onDeleted?.()` against the
NEW profile context — wrong error in the new modal, or
unexpectedly yanking the user back to /profiles when the OLD
request succeeds.

Implemented the capture-name pattern Copilot suggested:

- `const submittedName = profileName` captured at submit start
- After the fetch resolves, check `submittedName === profileName`
  before calling `setError` / `onDeleted` (i.e. mutating the
  wizard's UI state)
- Always emit the toast and the `aqa:profile-deleted` window
  event using `submittedName` (the action DID happen server-side
  and the App-level listener that filters PROFILES doesn't care
  which wizard fired it; the user deserves the feedback)
- Same guard in the catch branch + finally block — only flip
  `submitting` / `inFlightRef.current` back to false when the
  wizard still belongs to the submitted profile, otherwise the
  stale resolve could clobber a fresh submit's state on the new
  profile

A full fix would use `AbortController` to actually cancel the old
request, but the toast/broadcast-and-skip-state-mutation pattern
preserves the right user-visible outcome without adding the abort
plumbing.

Other 3 iter-7 comments were stale re-flags (Codex P2 deleted-row,
Copilot async submit race, Copilot ref/submitting desync — all
addressed in iters 3-5).

Tests: @aqa/admin 60 Playwright, all pass. Lint clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lopadova lopadova requested a review from Copilot May 19, 2026 10:07
@lopadova
Copy link
Copy Markdown
Contributor Author

iter 8 on 0ba0eb8: 1 real iter-7 item — captured submittedName at fetch start; post-fetch code only mutates wizard state (setError/setResult/onDeleted/setSubmitting/inFlightRef) when submittedName === profileName. Toast and aqa:profile-deleted event still fire with the submittedName so the user gets correct feedback even after a profile switch. Other 3 stale re-flags.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment on lines +4957 to +4960
function handleClose() {
if (submitting) return;
onClose?.();
}
// Delete is disabled until the user types the name.
await expect(page.getByTestId('profile-delete-submit')).toBeDisabled();
});

…ession test

PR #29 iter 8 review fixes (Copilot — 2 real new items + 4 stale re-flags):

## Real fixes

1. **Stale state flash on close+reopen** (Copilot app.tsx:4960):
   the reset effect only fires when `open` becomes true OR
   `profileName` changes. Closing and reopening on the SAME
   profile wouldn't trigger either, so the first render after
   reopening would briefly show the previous session's
   `confirmText` / `error` / `submitting` / `inFlightRef.current`.
   `handleClose` now resets all four synchronously before calling
   the parent's `onClose` — no flicker, no chance for a misclick
   to fire DELETE without re-typing.

2. **Missing close+reopen e2e** (Copilot e2e:48): added a
   regression test that opens the modal, types the exact name
   (which enables Delete), closes via Cancel, reopens, and
   asserts the confirm input is empty and Delete is disabled.

## Stale re-flags (no action)

Codex P2 deleted-row (lifted state iter 4), Copilot async submit
race (inFlightRef iter 3), Copilot ref/submitting desync (iter 5),
Copilot stale-closure race (submittedName guard iter 8).

Tests: @aqa/admin 61 Playwright (60 existing + 1 new close+reopen).
Lint clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lopadova lopadova requested a review from Copilot May 19, 2026 10:16
@lopadova
Copy link
Copy Markdown
Contributor Author

iter 9 on dfd0cf3: 2 real iter-8 items — (1) handleClose resets confirmText/error/submitting/inFlightRef synchronously to prevent stale-state flicker on close+reopen of the same profile; (2) regression e2e: open → type name → Cancel → reopen → assert empty input and disabled Delete. 4 stale re-flags.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment thread packages/admin/src/app.tsx Outdated
Comment on lines +8622 to +8641
const rawP = profileByName(name);
// Treat just-deleted profiles as not-found so a stale link
// (e.g. a notification linking to a profile a colleague just
// removed) doesn't render the underlying mock row.
const isDeleted = deletedProfiles?.has(name);
const p = isDeleted ? null : rawP;
const [deleteOpen, setDeleteOpen] = React.useState(false);
// Now that this page has a destructive Delete action, a stale or
// typoed route param (e.g. /profiles/<deleted-name>) must NOT
// silently render the first profile in the list — otherwise the
// user could delete the wrong record. Show a clear not-found
// state instead.
if (!p) {
return (
<div className="page" data-screen-label="14 Profile detail (not found)">
<PageHeader title="Profile not found" sub={`No profile with name "${name}".`} />
<Alert kind="warning" title="No such profile">
<span style={{ fontSize: 12.5 }}>
The profile you tried to open isn't in the local list. It may have been deleted,
renamed, or the URL is stale.{' '}
Comment thread packages/admin/src/app.tsx Outdated
return (
<Modal
open={open}
onClose={handleClose}
lopadova and others added 2 commits May 19, 2026 14:00
…g DELETE

PR #29 iter 10 — address two items from Copilot review on dfd0cf3:

1. ScreenJumper bypass (app.tsx:10807):
   The profile-detail route defaulted `name` to PROFILES[0].name when
   params were missing. ScreenJumper navigates with no params, so a
   user opening the screen picker and jumping to "Profile detail"
   would silently land on (and could delete) the first profile.
   Now the route passes params.name unchanged and PageProfileDetail
   renders an explicit "no profile selected" state with an Info alert
   and a Back-to-profiles button (separate from the existing
   "profile not found" state for unknown names).

2. Modal close affordances during in-flight DELETE (app.tsx:5065):
   While submitting, the Cancel button was disabled but the Modal's
   backdrop click, X iconbtn, and Escape handler all still fired
   onClose — looking broken and letting the user dismiss the wizard
   mid-request. Now onClose is set to undefined while submitting so
   all three vectors no-op until the request resolves.

Tests:
- New e2e: ScreenJumper jump to "Profile detail" lands on the
  no-selection state, never the first profile; the Delete button is
  not rendered in that state.
- New e2e: while DELETE is held in flight (route handler awaits a
  test-controlled promise), overlay click + X click + Escape are
  all no-ops; releasing the request closes the modal as expected.

Verified locally: typecheck clean, lint clean, unit tests 39/39,
e2e profile-delete 7/7.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n-flight test

CI typecheck failed because TS narrows `let resolveDelete: T | null = null`
to `null` (the assignment lives inside the Promise executor closure and
doesn't update the outer type), making the later `resolveDelete?.()` call
fail with TS2349 "Type 'never' has no call signatures".

The Promise executor runs synchronously, so the resolver is guaranteed
to be set before we use it. Switch to `let resolveDelete!: () => void`
(definite-assignment assertion) and a direct call.

Note: local Bun run had buffered output that masked the error; only the
isolated `bun run typecheck` in packages/admin surfaces it. Verified
now locally.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lopadova
Copy link
Copy Markdown
Contributor Author

iter 10 on d7af60d (9f34c67 had a CI typecheck regression — see follow-up commit): 2 real iter-9 items addressed —

  1. ScreenJumper bypass (app.tsx:10807): the profile-detail route previously defaulted name to PROFILES[0].name, so the bottom-left ScreenJumper (which navigates with no params) would silently land the user on the first profile — within striking distance of the new destructive Delete button. Now the route passes params.name unchanged and PageProfileDetail renders an explicit "no profile selected" state (Info alert + Back-to-profiles button, separate from the existing "not found" state for unknown names). The Delete button is not rendered on either state.

  2. Modal close affordances during in-flight DELETE (app.tsx:5065): while submitting, the Cancel button was disabled but the Modal's backdrop click, X iconbtn, and Escape handler all still fired onClose — looking inconsistent and letting the user dismiss the wizard mid-request. Now onClose={submitting ? undefined : handleClose} makes all three vectors inert until the request resolves.

Tests:

  • New e2e: opening ScreenJumper → "Profile detail" lands on the no-selection state, never the first profile; Delete button absent.
  • New e2e: while DELETE is held in flight (test-controlled promise), overlay click + X click + Escape are all no-ops; releasing the request closes the modal as expected.

Follow-up commit d7af60d fixes a TS2349 (Type 'never' has no call signatures) that local Bun output had buffered/masked — TS narrows let resolveDelete: T | null = null to null because the assignment lives in the Promise executor closure. Switched to definite-assignment let resolveDelete!: () => void. All CI green now (typecheck/lint/unit/e2e).

@lopadova lopadova merged commit a0d4df0 into main May 19, 2026
15 checks passed
@lopadova lopadova deleted the task/v1.7-slice-4c-delete-crud branch May 19, 2026 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants