feat(slice-4c.7+4c.8 admin): scenario edit/clone via shared YAML-textarea wizard#37
Merged
Merged
Conversation
…area wizard Closes the admin half of the previously server-only slices 4c.7 (PUT) and 4c.8 (POST) with a single shared modal that's pragmatic for v1.7 mock-data mode and ready for live-data mode. UI: - ScenarioYamlWizard: mode='edit' | 'clone'. The textarea is seeded with a schema-conforming Scenario stub (built from the current id; cloned with an empty id forcing the user to choose). Client parses YAML for early UX feedback (`yaml` package imported as a new admin dependency); the server is the actual trust boundary. - Edit button + Clone button wired into PageScenarioDetail header alongside the existing Delete. - Inline UX hints: same-as-source id (clone), collision id (clone), path/body id mismatch (edit). Submit stays disabled until the hints clear. - App-level updatedScenarios + createdScenarios Maps with aqa:scenario-updated / aqa:scenario-created events; threaded through ctx. PageScenarios appends created clones to the tree (and removes deleted ids). Tombstone-clear on re-create (lesson from PR #31 iter 1). - Test-only window.__aqaYamlParse hook exposed so e2e and the wizard itself share a stable parse symbol. E2E: 6 new tests in scenario-yaml.e2e.ts (Edit happy/seeded/id- mismatch, Clone seeded/same-source/collision/happy/4xx). Admin e2e total: 113 passed, 1 skipped. Lint preexisting only.
There was a problem hiding this comment.
Pull request overview
Adds admin-side Scenario Edit and Clone flows using a shared YAML-textarea modal, plus E2E coverage, to complete slices 4c.7-admin and 4c.8-admin.
Changes:
- Introduces
ScenarioYamlWizard(edit/clone) that client-parses YAML for UX hints and submits PUT/POST to the server. - Adds Edit/Clone actions to
PageScenarioDetailand lifts scenario create/update state toAppvia CustomEvents. - Adds Playwright E2E tests for the new wizard flows and introduces the
yamlclient dependency.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/admin/src/app.tsx | Implements the YAML wizard, wires Edit/Clone buttons, and adds App-level scenario created/updated tracking. |
| packages/admin/test/e2e/scenario-yaml.e2e.ts | Adds E2E coverage for scenario edit/clone wizard behaviors and error cases. |
| packages/admin/package.json | Adds yaml dependency for client-side parsing. |
| bun.lock | Locks the new yaml dependency. |
Comment on lines
+5469
to
+5471
| return [ | ||
| 'schema_version: "1"', | ||
| `id: ${id}`, |
Comment on lines
+12728
to
+12734
| // v1.7 slice 4c.7-admin / 4c.8-admin — Scenario edits broadcast | ||
| // via `aqa:scenario-updated` and clones via `aqa:scenario-created`. | ||
| // Same lifted-state reasoning as profile and risk: App-level Map | ||
| // survives route changes and seeds both the Scenarios list and the | ||
| // detail-page rendering for cloned-but-new ids. | ||
| const [updatedScenarios, setUpdatedScenarios] = React.useState(() => new Map()); | ||
| const [createdScenarios, setCreatedScenarios] = React.useState(() => new Map()); |
Comment on lines
+12737
to
+12744
| const id = e?.detail?.id; | ||
| const patch = e?.detail?.patch; | ||
| if (typeof id !== 'string' || !patch || typeof patch !== 'object') return; | ||
| setUpdatedScenarios((prev) => { | ||
| const next = new Map(prev); | ||
| next.set(id, { ...(prev.get(id) || {}), ...patch }); | ||
| return next; | ||
| }); |
![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: 5d1b3ff26b
ℹ️ 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".
| ? `body id "${parsedBody.id}" does not match the path "${scenarioId}"` | ||
| : null; | ||
| const uxError = parseError || sameAsSourceError || collisionError || idMismatchError; | ||
| const canSubmit = !submitting && uxError === null && parsedBody !== null; |
There was a problem hiding this comment.
canSubmit only requires parsedBody !== null, while every UX guard (sameAsSourceError, collisionError, idMismatchError) is skipped when parsedBody.id is missing. In clone mode the seeded YAML starts with id: blank, so Save is enabled and sends a body with no valid id, which predictably fails server-side schema validation (400) instead of being prevented client-side. This breaks the documented “force user to choose id” flow and creates a guaranteed failing submit path.
Useful? React with 👍 / 👎.
|
|
||
| React.useEffect(() => { | ||
| if (open) { | ||
| setYamlText(buildScenarioStubYaml(mode === 'clone' ? '' : scenarioId)); |
There was a problem hiding this comment.
Edit mode seeds the YAML id directly from scenarioId, but the built-in scenario catalog uses dotted/underscored ids (for example api.tenant.cross_tenant_search), while server validation parses Scenario.id as Slug (lowercase alnum + dashes only). Because edit mode also treats id changes as mismatches, users cannot convert these ids to a valid slug, so PUT requests from the default catalog will always 400 against the schema.
Useful? React with 👍 / 👎.
Copilot (3): - Clone mode let users POST a null id: the stub generated `id: ` (empty), YAML parsed that as `id: null`, and the existing UX checks didn't fire on null. Added an explicit "choose a new id" warning so the submit is blocked before the server's Slug rejection. New e2e test covers it. - updatedScenarios was set in App state but never consumed: the YAML preview on the Spec tab was always the static mock. The spec EditorYAML now stringifies the override (or the created body for clones) into its `lines` prop, so a successful save is visible immediately. New e2e test covers the round-trip. - Prototype-pollution defense: aqa:scenario-updated / aqa:scenario-created carry patches derived from free-form user-edited YAML. Spread-merging them into App state would allow keys like `__proto__` / `constructor` / `prototype` to mutate Object's prototype. Added `safeMergeObject` that strips those keys before merging; applied to both event handlers. Also expose `window.__aqaYamlStringify` (mirrors __aqaYamlParse) so the spec preview's override-render goes through a stable shared symbol. Tests: 8/8 scenario-yaml e2e green; lint preexisting only.
Comment on lines
+5540
to
+5546
| let parsedBody = null; | ||
| let parseError = null; | ||
| try { | ||
| parsedBody = window.__aqaYamlParse?.(yamlText); | ||
| } catch (e) { | ||
| parseError = e instanceof Error ? e.message : String(e); | ||
| } |
Comment on lines
+5597
to
+5600
| const msg = parsed?.error ?? `HTTP ${res.status}`; | ||
| const fullMsg = `${submittedSourceId}: ${msg}`; | ||
| toast.push({ kind: 'error', title: `Save scenario failed`, body: fullMsg }); | ||
| setError(msg); |
Comment on lines
+9847
to
+9851
| const existingIds = React.useMemo(() => { | ||
| const s = new Set(); | ||
| // Re-derive from the PageScenarios mock list (kept inline there | ||
| // for v1.7; a future slice can promote both to a shared module). | ||
| const mockIds = [ |
| if (typeof id !== 'string' || !patch || typeof patch !== 'object') return; | ||
| setUpdatedScenarios((prev) => { | ||
| const next = new Map(prev); | ||
| next.set(id, safeMergeObject(prev.get(id), patch)); |
…opilot
Copilot iter-2 (4):
- YAML parsing in render path → useMemo on yamlText so unrelated
state changes (submitting toggles, scenarioId prop, …) don't
re-trigger an O(yaml-size) parse on every render.
- Error toast/body in clone mode now reports the NEW id being
created (the request's actual subject), not the source id. Fixes
the misleading "<source>: id already exists" string when the
server 409s on a collision with the new id.
- SCENARIO_FIXTURES promoted to a module-level const; both
PageScenarios and PageScenarioDetail.existingIds now consume the
same list, so adding/removing a mock id no longer needs a
parallel edit in two places.
- aqa:scenario-updated now REPLACES rather than MERGES the
override (the patch is the full PUT body, not a delta). Removing
an optional field in the YAML now actually drops it from the
override / preview.
Tests: 15/15 scenario-{yaml,delete} e2e green; lint preexisting only.
Comment on lines
+5551
to
+5574
| // Clone mode forces the user to pick an id — YAML parses `id:` | ||
| // (empty) as `null`, so without this guard canSubmit would be true | ||
| // and the user could POST a body with `id: null`. The server would | ||
| // 400 on the Slug check, but failing earlier (and with a clearer | ||
| // hint) is better UX. | ||
| const cloneEmptyIdError = | ||
| mode === 'clone' && (parsedBody?.id == null || parsedBody.id === '') | ||
| ? 'choose a new id for the clone' | ||
| : null; | ||
| const sameAsSourceError = | ||
| mode === 'clone' && parsedBody?.id && parsedBody.id === scenarioId | ||
| ? 'new id is the same as the source' | ||
| : null; | ||
| const collisionError = | ||
| mode === 'clone' && parsedBody?.id && existingIds?.has?.(parsedBody.id) | ||
| ? `id "${parsedBody.id}" already exists` | ||
| : null; | ||
| const idMismatchError = | ||
| mode === 'edit' && parsedBody?.id && parsedBody.id !== scenarioId | ||
| ? `body id "${parsedBody.id}" does not match the path "${scenarioId}"` | ||
| : null; | ||
| const uxError = | ||
| parseError || cloneEmptyIdError || sameAsSourceError || collisionError || idMismatchError; | ||
| const canSubmit = !submitting && uxError === null && parsedBody !== null; |
Comment on lines
+5615
to
+5628
| const newId = parsed?.scenario?.id ?? parsedBody.id; | ||
| toast.push({ | ||
| kind: 'success', | ||
| title: mode === 'edit' ? 'Scenario saved' : `Scenario "${newId}" created`, | ||
| body: newId, | ||
| }); | ||
| try { | ||
| window.dispatchEvent( | ||
| new CustomEvent(mode === 'edit' ? 'aqa:scenario-updated' : 'aqa:scenario-created', { | ||
| detail: | ||
| mode === 'edit' | ||
| ? { id: submittedSourceId, patch: parsedBody } | ||
| : { id: newId, scenario: parsedBody }, | ||
| }), |
| const original = await textarea.inputValue(); | ||
| await textarea.fill(original.replace(`id: ${id}`, 'id: changed-by-user')); | ||
| await expect(page.getByTestId('scenario-yaml-uxerr')).toContainText( | ||
| /body id "changed-by-user".*path "/i, |
…ver response Copilot iter-3 (2 of 3 actionable): - canSubmit could become true when YAML parsed to a non-object (scalar/array/null) or when __aqaYamlParse was undefined and returned undefined. Added explicit bodyShapeError + missingIdError guards; canSubmit now requires parsedBody to be a plain object with a non-empty string id. cloneEmptyIdError still takes priority in clone mode for the friendlier "choose a new id" copy. - aqa:scenario-updated/created now broadcast the SERVER's response body (which has Zod-applied defaults like invariant_refs:[], cleanup:[], probe/oracle defaults) instead of the client's parsedBody. updatedScenarios / createdScenarios stay accurate to what's actually stored even when users omit optional fields. (Third comment was a false-positive regex reading — the existing id-mismatch test asserts `body id "changed-by-user".*path "` which does match the rendered message; the test passes.) Tests: 8/8 scenario-yaml e2e green; lint preexisting only.
Comment on lines
+12849
to
+12857
| // If the user cloned into a previously-deleted id, lift the | ||
| // tombstone so the new row resolves (same lesson as Profile | ||
| // Clone in PR #31 iter 1). | ||
| setDeletedScenarios((prev) => { | ||
| if (!prev.has(id)) return prev; | ||
| const next = new Set(prev); | ||
| next.delete(id); | ||
| return next; | ||
| }); |
…ore its first user
Copilot iter-4 (1):
- The aqa:scenario-created effect calls setDeletedScenarios (to lift
the tombstone when a clone reuses a deleted id), but
setDeletedScenarios was declared AFTER that effect in the App
body. JS TDZ for const means reading the binding before its
declaration throws ReferenceError. Reordered: deletedScenarios
state + its delete-event effect now precede the
edit/create-event effect.
Tests: 15/15 scenario-{yaml,delete} e2e green.
Comment on lines
+10071
to
+10075
| '# scenario.schema.json v1', | ||
| `id: ${sid}`, | ||
| 'risk_ref: risk-cross-tenant-leak', | ||
| 'description: |', | ||
| ' Query /api/orders/search as tenant A with a payload that bypasses', |
Comment on lines
+5541
to
+5546
| const { parsedBody, parseError } = React.useMemo(() => { | ||
| let p = null; | ||
| let err = null; | ||
| try { | ||
| p = window.__aqaYamlParse?.(yamlText); | ||
| } catch (e) { |
…+ debounce parse Copilot iter-5 (2): - Static fallback YAML preview (and Outline sidebar) used pre-schema field names (risk_ref/probes/oracle) and "schema- validated" badge was misleading. Replaced with schema-conforming field names (risk_refs/steps/oracles, plus schema_version/title/cleanup/tags) so the preview matches what the server would accept. - YAML parse on every keystroke was on the render path. Added a 150ms debounce (separate debouncedYaml state) so fast typers stay responsive; UX-error alert and submit-button settle a tick after the user pauses. Tests: 8/8 scenario-yaml e2e green.
Comment on lines
+9669
to
+9720
| id: 'api.tenant.cross_tenant_search', | ||
| pack: 'api', | ||
| oracle: 'cross_tenant', | ||
| last_status: 'failed', | ||
| }, | ||
| { | ||
| id: 'api.tenant.cross_tenant_invoice', | ||
| pack: 'api', | ||
| oracle: 'cross_tenant', | ||
| last_status: 'failed', | ||
| }, | ||
| { | ||
| id: 'auth.jwt.replay_after_logout', | ||
| pack: 'security-owasp', | ||
| oracle: 'authn', | ||
| last_status: 'failed', | ||
| }, | ||
| { id: 'api.idor.invoice_pdf', pack: 'api', oracle: 'authz', last_status: 'succeeded' }, | ||
| { | ||
| id: 'security.rate_limit.search', | ||
| pack: 'security-owasp', | ||
| oracle: 'rate_limit', | ||
| last_status: 'failed', | ||
| }, | ||
| { | ||
| id: 'agentic.tool_budget.runaway', | ||
| pack: 'security-agentic', | ||
| oracle: 'budget', | ||
| last_status: 'failed', | ||
| }, | ||
| { id: 'data.pii.logs', pack: 'security-owasp', oracle: 'pii_scan', last_status: 'failed' }, | ||
| { | ||
| id: 'business.order.total_rounding', | ||
| pack: 'core', | ||
| oracle: 'invariant', | ||
| last_status: 'succeeded', | ||
| }, | ||
| { id: 'security.csrf.admin', pack: 'security-owasp', oracle: 'csrf', last_status: 'failed' }, | ||
| { | ||
| id: 'ui.xss.reflected_search', | ||
| pack: 'web-ui-laravel', | ||
| oracle: 'xss_scan', | ||
| last_status: 'succeeded', | ||
| }, | ||
| { | ||
| id: 'security.prompt_injection.search_rag', | ||
| pack: 'security-agentic', | ||
| oracle: 'llm_judge', | ||
| last_status: 'failed', | ||
| }, | ||
| { | ||
| id: 'migrations.rollback.smoke', |
Comment on lines
+5591
to
+5596
| const missingIdError = | ||
| parseError == null && | ||
| isPlainObject(parsedBody) && | ||
| (typeof parsedBody.id !== 'string' || parsedBody.id.length === 0) | ||
| ? 'id must be a non-empty string' | ||
| : null; |
Comment on lines
+106
to
+107
| // Type the well-known mock id `api.idor.invoice_pdf` — collision. | ||
| await textarea.fill(seeded.replace(/^id:.*/m, 'id: api.idor.invoice_pdf')); |
Comment on lines
+5520
to
+5526
| React.useEffect(() => { | ||
| if (open) { | ||
| setYamlText(buildScenarioStubYaml(mode === 'clone' ? '' : scenarioId)); | ||
| setError(null); | ||
| setSubmitting(false); | ||
| inFlightRef.current = false; | ||
| } |
…ent slug check + edit seeds from override
Copilot iter-6 (4):
- SCENARIO_FIXTURES ids migrated from dot-separated
(api.tenant.cross_tenant_search) to Slug-conforming dashed
(api-tenant-cross-tenant-search). Without this, Edit/Clone PUT/
POST against the real server would 400 on Scenario.id's Slug
regex before reaching id-match / collision logic. Tree grouping
switched from id.split('.')[0] to an explicit `category` field
on each fixture. Leaf label uses the full id (no truncation).
- Client-side slug validation added: parsedBody.id must match
Slug regex (lowercase alnum + dashes, ≤ 64). The hint reads
"id must be lowercase letters/digits with dashes between
(Slug)". E2E test asserts dots are rejected and a valid slug
re-enables submit.
- Edit mode now seeds the textarea from the App-level
updatedScenarios/createdScenarios override (yamlStringify) when
available; falls back to the stub on first edit. Re-opening
Edit after a save shows the saved body — preventing the
previous "save the stub on top of the real body" trap. E2E test
covers the round-trip.
- Updated the collision e2e to use the slug-conforming
api-idor-invoice-pdf (previously used a non-conforming dotted id).
Tests: 10/10 scenario-yaml + 7/7 scenario-delete e2e green.
Comment on lines
+11
to
+13
| // Expose the YAML parser on `window` so the test-only ScenarioYamlWizard | ||
| // (and the e2e tests) can drive client-side parsing through a stable | ||
| // symbol. Mirrors __aqaApiUrl / __aqaNavigate. |
Comment on lines
+5568
to
+5572
| const [debouncedYaml, setDebouncedYaml] = React.useState(yamlText); | ||
| React.useEffect(() => { | ||
| const h = setTimeout(() => setDebouncedYaml(yamlText), 150); | ||
| return () => clearTimeout(h); | ||
| }, [yamlText]); |
… accuracy Copilot iter-7 (2): - The `__aqaYamlParse` window-hook comment claimed "test-only ScenarioYamlWizard", but the wizard is now wired into PageScenarioDetail's production flow. Comment corrected. - debouncedYaml lagged yamlText for up to 150ms whenever the modal reseeded (open/scenarioId/mode/persistedBody change), briefly showing stale warnings or a stale-disabled submit. The seed effect now also calls setDebouncedYaml(seeded) so parsedBody and uxError reflect the new session immediately; only typing is debounced. Tests: 10/10 scenario-yaml e2e green.
Comment on lines
+12957
to
+12960
| const onUpdate = (e) => { | ||
| const id = e?.detail?.id; | ||
| const patch = e?.detail?.patch; | ||
| if (typeof id !== 'string' || !patch || typeof patch !== 'object') return; |
Comment on lines
+12972
to
+12976
| const onCreate = (e) => { | ||
| const id = e?.detail?.id; | ||
| const scenario = e?.detail?.scenario; | ||
| if (typeof id !== 'string' || !scenario || typeof scenario !== 'object') return; | ||
| setCreatedScenarios((prev) => { |
Comment on lines
+10156
to
+10159
| try { | ||
| return window.__aqaYamlStringify?.(fromState).split('\n') ?? []; | ||
| } catch { | ||
| /* fall through to mock preview */ |
Comment on lines
+9822
to
+9827
| createdRows.push({ | ||
| id, | ||
| // Mock-display defaults: pack-prefix splits cleanly enough for | ||
| // the tree grouping; oracle/last_status are best-effort. | ||
| pack: typeof sc?.tags?.find?.((t) => /^pack:/i.test(t)) === 'string' | ||
| ? sc.tags.find((t) => /^pack:/i.test(t)).split(':').slice(1).join(':') |
…factors
Copilot iter-8 (4):
- aqa:scenario-updated / aqa:scenario-created guards previously
accepted arrays via typeof === 'object'; tightened to isPlain-
Object so arrays/Maps/Sets can't slip into App-level overrides
and break downstream consumers (yamlStringify, collision check).
- The override-stringify path called .split('\n') directly on the
result of an optional-chained __aqaYamlStringify?.(…) — a
missing hook would throw. Chained the split via an intermediate
variable guarded by typeof === 'string'.
- PageScenarios' createdRows loop iterated `tags` twice to find
the `pack:` tag. Hoisted to a single local.
- createdRows now also copies through the user-supplied
`category` field (falling back to 'misc') so cloned scenarios
group correctly in the tree.
Tests: 17/17 scenario-yaml + scenario-delete e2e green.
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
Closes the admin half of slices 4c.7 (Scenario Edit) and 4c.8 (Scenario Clone) — both previously delivered server-only — using a shared YAML-textarea modal.
Admin
E2E
Test plan
🤖 Generated with Claude Code