diff --git a/docs/internal/admin-placeholder-audit.md b/docs/internal/admin-placeholder-audit.md index 4992a8d..0429b9f 100644 --- a/docs/internal/admin-placeholder-audit.md +++ b/docs/internal/admin-placeholder-audit.md @@ -45,7 +45,7 @@ The line numbers below were captured against commit `~v1.6.0` (post-merge of PR ### Profiles (lines ~5500-5950) -- `5590`, `5745`, `5796` — profile-level actions (save/clone/delete) +- `5590`, `5745`, `5796` — profile-level actions (save/clone/delete) — **DELETE shipped in PR #29 (slice 4c.1):** Profile detail page now has a Delete button → opens type-to-confirm modal → DELETE `/api/profiles/:name`. Save/Clone still pending. - `5927`, `5931`, `5935`, `5939` — pack picker ### Agents (lines ~6500-6630) diff --git a/packages/admin/src/app.tsx b/packages/admin/src/app.tsx index c90c60d..c0b3c6e 100644 --- a/packages/admin/src/app.tsx +++ b/packages/admin/src/app.tsx @@ -4906,6 +4906,234 @@ function ImportManifestWizard({ open, onClose }) { } Object.assign(window, { ImportManifestWizard }); +// ============================================================= +// Delete-profile wizard (v1.7 slice 4c) +// ============================================================= +// Front-end for DELETE /api/profiles/:name. Deleting a profile is +// destructive (it removes the configuration that drives `aqa run +// --profile `) so the modal requires the user to type the +// profile name as confirmation, mirroring the GitHub repo-delete +// confirmation pattern. The server endpoint exists since v1.4. +function DeleteProfileWizard({ open, profileName, onClose, onDeleted }) { + const [confirmText, setConfirmText] = React.useState(''); + const [submitting, setSubmitting] = React.useState(false); + const [error, setError] = React.useState(null); + // Synchronous in-flight guard: `submitting` state doesn't flip until + // the next render, so two rapid clicks on the disabled-soon button + // could both pass the `submitting` check and fire two DELETE + // requests. The ref is mutated inline before any await, so the + // second call short-circuits immediately. Same pattern as the + // kanban's pendingRef (slice 4a iter 4). + const inFlightRef = React.useRef(false); + const toast = useToast(); + + // Reset when modal opens for a new profile (the parent re-uses the + // same component for every profile it renders detail for). + // Include `profileName` so the reset also fires if the modal stays + // mounted across a profile switch (e.g. parent re-uses the wizard + // with a different name) — otherwise the typed confirm text from + // the previous profile would stick around. + // + // CRITICAL: reset `inFlightRef.current` together with `submitting`. + // Otherwise a profile switch mid-flight would re-enable the Delete + // button (submitting=false) while the ref stays true — the user + // could click Delete and the new submit would silently no-op via + // the synchronous guard. Keeping both in sync means the old + // request's finally block becomes a no-op assignment, and the user + // can issue a fresh delete on the new profile. (The old request's + // success path would still toast against the old profileName, + // which is acceptable — the user just changed context.) + React.useEffect(() => { + if (open) { + setConfirmText(''); + setError(null); + setSubmitting(false); + inFlightRef.current = false; + } + }, [open, profileName]); + + const canSubmit = confirmText === profileName && !submitting; + + function handleClose() { + if (submitting) return; + // Reset state synchronously on close — don't rely solely on the + // open-true effect. If the user closes the modal and reopens it + // for the same profile (no `profileName` change to retrigger + // the effect), the first render after reopening would otherwise + // briefly show stale `confirmText` / `error` from the prior + // session before the next effect tick clears them. + setConfirmText(''); + setError(null); + setSubmitting(false); + inFlightRef.current = false; + onClose?.(); + } + + async function handleSubmit() { + if (!canSubmit) return; + if (inFlightRef.current) return; // synchronous double-click guard + inFlightRef.current = true; + setSubmitting(true); + setError(null); + // Capture the profile name we're submitting against. If the parent + // swaps `profileName` while the fetch is in flight (e.g. user + // navigates to a different profile-detail page that reuses this + // wizard mount), the in-flight resolve must NOT mutate the + // wizard's UI state for the OLD profile. The post-fetch code + // checks `submittedName === profileName` before calling any + // setState / onDeleted. This is the "stale closure" guard + // Copilot flagged in iter 7 review. + const submittedName = profileName; + const reqUrl = apiUrl(`/api/profiles/${encodeURIComponent(submittedName)}`); + 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; + } + // Stale-submit guard: if the user switched profiles while this + // fetch was in flight, we still toast/broadcast the *correct* + // event for `submittedName` (the action did happen server-side + // and the user deserves the feedback), but we do NOT mutate + // the wizard's UI state for the NEW profile (setError / + // onDeleted). The wizard belongs to the new profile now. + const stillCurrent = submittedName === profileName; + if (!res.ok) { + const msg = parsed?.error ?? `HTTP ${res.status}`; + // Toast carries the submittedName so the user knows which + // delete attempt failed, even after switching. + toast.push({ + kind: 'error', + title: 'Delete profile failed', + body: `${submittedName}: ${msg}`, + }); + if (stillCurrent) setError(msg); + return; + } + toast.push({ + kind: 'success', + title: 'Profile deleted', + body: submittedName, + }); + // Broadcast the deletion regardless of stale-submit state — + // App-level listener filters PROFILES, and that's true whether + // the user is on the old or new detail page. The event uses + // submittedName, not profileName. + try { + window.dispatchEvent( + new CustomEvent('aqa:profile-deleted', { detail: { name: submittedName } }), + ); + } catch { + // CustomEvent unsupported in this runtime — non-fatal. + } + // onDeleted (which navigates back to /profiles) only makes + // sense when the wizard still belongs to the deleted profile. + // If the user already moved on, don't yank them back. + if (stillCurrent) onDeleted?.(); + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + const full = `Could not reach ${reqUrl} (${msg}). The admin is in mock-data mode or the server is down — the profile was not deleted.`; + // Same stale-submit guard as the success path: toast against + // the submitted name so the user gets the right context, but + // only mutate wizard error state if it still belongs to that + // profile. + toast.push({ + kind: 'error', + title: 'Delete profile failed', + body: `${submittedName}: ${full}`, + }); + if (submittedName === profileName) setError(full); + } finally { + // Only flip submitting/inFlightRef when the wizard still + // belongs to the submitted profile — otherwise a stale resolve + // could re-enable the Delete button for an unrelated profile + // mid-flight (the user's current submit might still be in + // progress on the new profile). + if (submittedName === profileName) { + setSubmitting(false); + inFlightRef.current = false; + } + } + } + + return ( + + 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). + + } + size="md" + footer={ + <> + + + + } + > +
+ {error && ( + + {error} + + )} + + Removing a profile drops every run configuration that uses it. Existing run records, + findings, and audit events are unaffected — only the profile definition is removed. + +
+ + setConfirmText(e.target.value)} + autoFocus + /> +
+ The Delete button stays disabled until the typed text matches the profile name + exactly. Stops accidental clicks on the wrong profile in a list. +
+
+
+ + ); +} +Object.assign(window, { DeleteProfileWizard }); + // ============ shell.jsx ============ // ============================================================= // agentic-qa-kit · admin panel — Shell (Sidebar + TopBar + Palette) @@ -8320,12 +8548,20 @@ function PageScenarioDetail({ id, onNavigate }) { } // ---------------- Profiles ---------------- -function PageProfiles({ onNavigate, onOpenProfile }) { +function PageProfiles({ onNavigate, onOpenProfile, deletedProfiles }) { + // `deletedProfiles` is owned by App (lifted state) so it survives + // route changes. PageProfileDetail dispatches `aqa:profile-deleted` + // BEFORE navigating back here, so a listener on PageProfiles itself + // would miss the event (we weren't mounted at dispatch time). The + // App-level listener catches every dispatch and the Set drips down + // through props. + const deletedNames = deletedProfiles ?? new Set(); + const visible = PROFILES.filter((p) => !deletedNames.has(p.name)); return (
p.execution_mode === 'sandbox').length} sandbox · ${PROFILES.filter((p) => p.execution_mode === 'host').length} host`} + sub={`${visible.length} configured · execution mode mix: ${visible.filter((p) => p.execution_mode === 'sandbox').length} sandbox · ${visible.filter((p) => p.execution_mode === 'host').length} host`} actions={ + + +
+ ); + } + if (!p) { + return ( +
+ + + + The profile you tried to open isn't in the local list. It may have been deleted, + renamed, or the URL is stale.{' '} + + + +
+ ); + } return (
Edit + } /> + setDeleteOpen(false)} + onDeleted={() => { + setDeleteOpen(false); + onNavigate?.('profiles', {}); + }} + />
@@ -10523,7 +10835,12 @@ const ROUTES = { label: 'Profile detail', section: 'Catalog', parent: 'profiles', - render: (ctx) => , + // Do NOT fall back to PROFILES[0].name here: ScreenJumper and other + // entrypoints navigate to `profile-detail` with no params, which would + // otherwise silently render (and let the user delete) the first + // profile. Pass `undefined` and let PageProfileDetail render an + // explicit "no profile selected" state. + render: (ctx) => , }, agents: { label: 'Agents', section: 'Catalog', render: (ctx) => }, @@ -10608,6 +10925,27 @@ function App() { const [mode, setMode] = React.useState('mock'); // mock | live | failed const [lastTick, setLastTick] = React.useState(NOW_REF); const [signedIn, setSignedIn] = React.useState(true); + // Profile deletions broadcast via `aqa:profile-deleted` CustomEvent + // and the set lives at App level (not in PageProfiles) so it + // survives route changes. PageProfileDetail dispatches the event + // before navigating back to /profiles — if the listener lived on + // PageProfiles it would miss the event because PageProfiles isn't + // mounted yet at dispatch time. App is always mounted while the + // user is signed in, so it catches every dispatch. + const [deletedProfiles, setDeletedProfiles] = React.useState(() => new Set()); + React.useEffect(() => { + const handler = (e) => { + const name = e?.detail?.name; + if (typeof name !== 'string') return; + setDeletedProfiles((prev) => { + const next = new Set(prev); + next.add(name); + return next; + }); + }; + window.addEventListener('aqa:profile-deleted', handler); + return () => window.removeEventListener('aqa:profile-deleted', handler); + }, []); // Apply theme React.useEffect(() => { @@ -10666,6 +11004,7 @@ function App() { params: routeParams, theme: tweaks.theme, onTheme: (th) => setTweak('theme', th), + deletedProfiles, }; if (!signedIn) { diff --git a/packages/admin/test/e2e/profile-delete.e2e.ts b/packages/admin/test/e2e/profile-delete.e2e.ts new file mode 100644 index 0000000..0f4c89a --- /dev/null +++ b/packages/admin/test/e2e/profile-delete.e2e.ts @@ -0,0 +1,241 @@ +import { expect, test } from '@playwright/test'; + +/** + * v1.7 slice 4c.1 — Profile Delete confirmation modal wired to + * DELETE /api/profiles/:name. + * + * Covers: + * - Profile detail page has a Delete button (previously absent) + * - Clicking it opens a confirmation modal that requires typing the + * profile name (GitHub-style "type to confirm" anti-footgun pattern) + * - Delete button stays disabled until the typed name matches exactly + * - Happy path: DELETEs /api/profiles/, shows success toast, + * navigates back to the profiles list + * - 4xx/5xx: modal stays open with the server's error message + * + * The server endpoint exists since v1.4 (`DELETE /api/profiles/:name`), + * so this slice is pure UI wiring + e2e. + */ + +async function navigateToProfileDetail(page: import('@playwright/test').Page): Promise { + await page.goto('/'); + await expect(page.locator('.sidebar')).toBeVisible(); + await page + .locator('.nav-item', { hasText: /^Profiles/i }) + .first() + .click(); + await expect(page.locator('.page-title, h1').first()).toContainText(/Profiles/i); + // Click the first profile row to open detail. The prototype renders + // profiles as table rows where the entire row is clickable. + const firstRow = page.locator('table.tbl tbody tr').first(); + await expect(firstRow).toBeVisible(); + const name = await firstRow.locator('td').first().innerText(); + await firstRow.click(); + // Profile detail header shows the name as a mono span. + await expect(page.locator('.page-title .mono, h1 .mono').first()).toBeVisible(); + return name.trim(); +} + +test.describe('Profile delete confirmation', () => { + test('Delete button opens the confirmation modal', async ({ page }) => { + await navigateToProfileDetail(page); + await page.getByTestId('profile-delete-btn').click(); + await expect(page.locator('.modal-title')).toContainText(/Delete profile/i); + await expect(page.getByTestId('profile-delete-confirm')).toBeVisible(); + // Delete is disabled until the user types the name. + await expect(page.getByTestId('profile-delete-submit')).toBeDisabled(); + }); + + test('close+reopen clears the typed confirm text (no stale state flash)', async ({ page }) => { + // Regression test for PR #29 iter 8 (Copilot): + // The reset effect runs only when `open` becomes true OR + // `profileName` changes. Closing and reopening on the SAME + // profile wouldn't fire either trigger, so the first render + // after reopening could briefly show stale `confirmText` / + // `error` from the prior session. handleClose now resets state + // synchronously to avoid the flicker. + const name = await navigateToProfileDetail(page); + await page.getByTestId('profile-delete-btn').click(); + const confirm = page.getByTestId('profile-delete-confirm'); + await confirm.fill(name); // type the exact name → Delete enables + await expect(page.getByTestId('profile-delete-submit')).toBeEnabled(); + // Close via the Cancel button (calls handleClose). + await page.getByRole('button', { name: 'Cancel' }).click(); + await expect(page.locator('.modal-title')).toHaveCount(0); + // Reopen the same profile's delete modal. + await page.getByTestId('profile-delete-btn').click(); + // The confirm input must be empty — no stale value, no enabled + // Delete button (which would mean a misclick could fire DELETE + // without re-typing). + await expect(page.getByTestId('profile-delete-confirm')).toHaveValue(''); + await expect(page.getByTestId('profile-delete-submit')).toBeDisabled(); + }); + + test('typing a non-matching name leaves Delete disabled', async ({ page }) => { + const name = await navigateToProfileDetail(page); + await page.getByTestId('profile-delete-btn').click(); + await page.getByTestId('profile-delete-confirm').fill(`${name}-typo`); + await expect(page.getByTestId('profile-delete-submit')).toBeDisabled(); + }); + + test('exact-match name enables Delete; DELETE fires; toast shown; navigates to /profiles; row drops from list', async ({ + page, + }) => { + // Only intercept DELETEs — let any other HTTP method continue + // to the dev server unmodified so unexpected GET/PUT calls don't + // get silently fulfilled with our `{}` stub. Also match the real + // endpoint's response shape: `DELETE /api/profiles/:name` returns + // `{ok: true}` per @aqa/server's `asResponse({ ok: true })`. + type Req = { url: string; method: string }; + const calls: Req[] = []; + await page.route('**/api/profiles/*', async (route) => { + const method = route.request().method(); + if (method !== 'DELETE') return route.continue(); + calls.push({ url: route.request().url(), method }); + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ ok: true }), + }); + }); + const name = await navigateToProfileDetail(page); + await page.getByTestId('profile-delete-btn').click(); + await page.getByTestId('profile-delete-confirm').fill(name); + await expect(page.getByTestId('profile-delete-submit')).toBeEnabled(); + await page.getByTestId('profile-delete-submit').click(); + // (1) On success the modal closes. + await expect(page.locator('.modal-title')).toHaveCount(0); + // (2) Exactly one DELETE call to /api/profiles/. + expect(calls.length).toBe(1); + expect(calls[0]?.method).toBe('DELETE'); + expect(calls[0]?.url).toContain(`/api/profiles/${encodeURIComponent(name)}`); + // (3) Success toast appears with the right title. + await expect(page.locator('.toast.success', { hasText: 'Profile deleted' })).toBeVisible(); + // (4) After navigating back to /profiles, the just-deleted row + // no longer appears in the list (window event drops it via + // the App-level lifted deletedProfiles set). + // + // Selector: the profile name renders as + // inside a . An earlier version of this test used `td.mono`, + // which matches nothing (the class is on the span, not the td) + // so the count was always 0 and the assertion silently passed + // even if the row was still present (Copilot caught this in + // iter 6 review). Now we target the span directly within a row + // AND assert the table still has other rows so the check isn't + // vacuously satisfied by an empty table. + await expect(page.locator('.page-title, h1').first()).toContainText(/Profiles/i); + const escaped = name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + await expect( + page.locator('table.tbl tbody tr', { + has: page.locator('span.id-link.mono', { hasText: new RegExp(`^${escaped}$`) }), + }), + ).toHaveCount(0); + await expect(page.locator('table.tbl tbody tr')).not.toHaveCount(0); + }); + + test('opening profile-detail with no params shows "no profile selected" (not the first profile)', async ({ + page, + }) => { + // Regression test for PR #29 iter 10 (Copilot): + // The route definition previously defaulted `name` to + // `PROFILES[0].name`, so ScreenJumper (which navigates without + // params) would silently land the user on — and let them delete — + // the first profile. The route now passes `undefined` and the + // page renders an explicit no-selection state. + await page.goto('/'); + await expect(page.locator('.sidebar')).toBeVisible(); + // Open the ScreenJumper and pick "Profile detail" — this is the + // exact code path that exposes the bypass: onNavigate(key) with + // no params. + await page.locator('button[title="Jump to screen"]').click(); + // Each popup row has a numeric-index span and a label span; target + // the label span by exact text and let the click bubble to the + // clickable parent div. + await page + .locator('span') + .filter({ hasText: /^Profile detail$/ }) + .first() + .click(); + // Should land on the no-selection state, NOT a profile name. + await expect(page.locator('.page-title, h1').first()).toContainText(/No profile selected/i); + // Crucially: no Delete button must be present on this state, so + // a stray click can't delete the first profile. + await expect(page.getByTestId('profile-delete-btn')).toHaveCount(0); + // "Back to profiles" returns the user to the list. + await page.getByTestId('profile-detail-back').click(); + await expect(page.locator('.page-title, h1').first()).toContainText(/Profiles/i); + }); + + test('modal close affordances (overlay, X, Escape) are inert while DELETE is in flight', async ({ + page, + }) => { + // Regression test for PR #29 iter 10 (Copilot): + // While `submitting` is true the Cancel button is disabled, but + // earlier the Modal's backdrop and "X" close button (and the + // Escape key handler) still fired onClose — looking broken and + // letting the user dismiss the wizard mid-request. The fix + // passes `onClose={submitting ? undefined : handleClose}`. + // + // We hold the DELETE open by deferring `route.fulfill` until the + // test resolves a promise, giving us a deterministic in-flight + // window in which to assert. + // Definite assignment assertion: the Promise executor runs + // synchronously inside the constructor, so `resolveDelete` is + // guaranteed to be set before the next statement. TS otherwise + // narrows the union to `null` because the assignment is inside + // a closure and refuses to call it. + let resolveDelete!: () => void; + const deleteHeld = new Promise((r) => { + resolveDelete = r; + }); + await page.route('**/api/profiles/*', async (route) => { + if (route.request().method() !== 'DELETE') return route.continue(); + await deleteHeld; + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ ok: true }), + }); + }); + const name = await navigateToProfileDetail(page); + await page.getByTestId('profile-delete-btn').click(); + await page.getByTestId('profile-delete-confirm').fill(name); + await page.getByTestId('profile-delete-submit').click(); + // Button now reads "Deleting…" — confirms we're in the in-flight + // window before we assert close-affordance inertness. + await expect(page.getByTestId('profile-delete-submit')).toContainText(/Deleting/i); + // Try every dismissal vector while in flight. Each one must be a + // no-op: the modal must still be present after the click/keypress. + await page.locator('.overlay').click({ force: true }); + await expect(page.locator('.modal-title')).toContainText(/Delete profile/i); + await page.locator('.modal-head .iconbtn').click(); // the X button + await expect(page.locator('.modal-title')).toContainText(/Delete profile/i); + await page.keyboard.press('Escape'); + await expect(page.locator('.modal-title')).toContainText(/Delete profile/i); + // Release the DELETE — modal should now close on success. + resolveDelete(); + await expect(page.locator('.modal-title')).toHaveCount(0); + }); + + test('4xx keeps modal open and surfaces the server error', async ({ page }) => { + await page.route('**/api/profiles/*', async (route) => { + if (route.request().method() !== 'DELETE') return route.continue(); + await route.fulfill({ + status: 409, + contentType: 'application/json', + body: JSON.stringify({ + error: 'profile is referenced by 3 active runs; cancel them first', + }), + }); + }); + const name = await navigateToProfileDetail(page); + await page.getByTestId('profile-delete-btn').click(); + await page.getByTestId('profile-delete-confirm').fill(name); + await page.getByTestId('profile-delete-submit').click(); + await expect( + page.locator('.modal-body .alert', { hasText: /referenced by 3 active runs/i }), + ).toBeVisible(); + // Modal still open. + await expect(page.locator('.modal-title')).toContainText(/Delete profile/i); + }); +});