feat(slice-4c.3): profile clone wizard + POST /api/profiles#31
Merged
Conversation
Server: - POST /api/profiles handler with @aqa/schemas Profile validation - 409+EEXIST on name collision, 201 on create, requires profiles:edit - 4 unit tests added (48/48 server tests pass) Admin UI: - CloneProfileWizard with source name read-only + slug-validated new name - Inline collision check vs PROFILES + createdProfiles + self - Same architecture as EditProfileWizard: inFlightRef, captured submittedSource, render-time profileRef, sync handleClose reset, overlay-inertness during submit - App-level createdProfiles Map + aqa:profile-created window event so the cloned profile shows up in PageProfiles list and resolves on its new detail page without a reload - PageProfileDetail gets Clone button between Edit and Delete; onCloned navigates to the new profile's detail page E2E: 10 new tests in profile-clone.e2e.ts, all green with --workers=2
![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: a536d39c61
ℹ️ 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
+11788
to
+11791
| setCreatedProfiles((prev) => { | ||
| const next = new Map(prev); | ||
| next.set(name, withMockAliases); | ||
| return next; |
There was a problem hiding this comment.
When a clone is created with a name that was previously deleted (explicitly allowed by existingNames), this handler adds the new profile to createdProfiles but leaves the old name in deletedProfiles. Because PageProfiles and PageProfileDetail both treat names in deletedProfiles as hidden/not found, the newly created profile becomes inaccessible immediately after a successful POST. This breaks the “reuse deleted name” flow and can reproduce by deleting a profile, cloning another profile into that same name, then navigating to its detail page.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds “Profile Clone” capability end-to-end: a new server POST /api/profiles endpoint with create-only semantics, plus an admin Clone modal that POSTs a cloned Profile and updates the mock-driven UI via a created-profiles map/event so the new profile appears immediately without reload.
Changes:
- Server: implement
POST /api/profileswith@aqa/schemasProfile validation, 409/EEXIST on name collision, andprofiles:editpermission requirement. - Admin: add
CloneProfileWizard, wire a new Clone button on Profile detail, and propagate newly created profiles viaaqa:profile-created+ App-levelcreatedProfiles. - E2E: add a Playwright suite covering clone UX, validation/collision behavior, success navigation/list update, and error/in-flight close affordances.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/server/src/api.ts | Adds POST /api/profiles route with schema validation and 409-on-exist create-only semantics. |
| packages/server/test/api.test.ts | Adds unit tests for POST /api/profiles (201/400/409 + permission). |
| packages/admin/src/app.tsx | Implements Clone wizard, adds Clone action to profile detail, and merges created profiles into list/detail views. |
| packages/admin/test/e2e/profile-clone.e2e.ts | Adds Playwright coverage for clone modal UX + POST behavior + navigation/list update. |
Comment on lines
+5666
to
+5670
| const res = await fetch('/api/profiles', { | ||
| method: 'POST', | ||
| headers: { 'content-type': 'application/json' }, | ||
| body: JSON.stringify(body), | ||
| }); |
Comment on lines
+5598
to
+5601
| // not collide with an existing profile (mock or already-created). | ||
| // Case-insensitive comparison so a slug-cased "smoke" can't be | ||
| // cloned as "Smoke" → server-side casing would then conflict on | ||
| // the next case-folded lookup. |
Comment on lines
+9353
to
+9354
| // in either the mock list, the created-via-clone Map, or as a | ||
| // freshly-edited override. Deleted names are eligible for re-use. |
Comment on lines
+551
to
+555
| const existing = await ctx.store.loadProfile(profile.name); | ||
| if (existing) { | ||
| return asResponse( | ||
| { | ||
| error: `profile "${profile.name}" already exists; PUT /api/profiles/${encodeURIComponent(profile.name)} to update or pick a different name`, |
Comment on lines
+11782
to
+11787
| // Mirror the schema field into the legacy mock alias so | ||
| // PageProfiles' table (which reads `p.budget_usd` directly) | ||
| // formats correctly without each row needing schema-aware | ||
| // accessors. Same trick the EditProfileWizard submit handler | ||
| // uses for `updatedProfiles`. | ||
| const withMockAliases = { ...profile, budget_usd: profile.llm_budget_usd }; |
Copilot (5):
- Use apiUrl('/api/profiles') in CloneProfileWizard so the POST lands
on VITE_AQA_SERVER_URL when the admin is hosted separately
- Fix stale comments on the slug-collision-check and existingNames
memo to match the actual (case-sensitive, mock+created-only) behavior
- Make fmtRelative return em-dash on null/undefined so a freshly-
cloned row with no last_run_at doesn't render "NaNmo ago"
- Stamp last_run_at=null on createdProfiles entries (defense in depth
alongside the fmtRelative fix)
- Atomic create at the store layer: add Store.createProfile(profile)
returning { created: boolean }; POST /api/profiles now delegates to
it instead of load+save, closing the TOCTOU window where two
concurrent POSTs for the same name could both observe "missing"
and overwrite each other
Codex (P1):
- When the broadcast aqa:profile-created event names a previously-
deleted profile, the App-level handler now clears the tombstone
from deletedProfiles so the new row is visible in the list and its
detail page resolves (instead of being hidden by the deleted-name
filter)
Tests:
- New unit test: concurrent POST /api/profiles for the same name
yields exactly [201, 409] (verifies createProfile atomicity)
- New e2e test: delete profile A, clone profile B into A's name —
modal closes, new detail page resolves, Profiles list shows the
re-created row (no tombstone hiding it)
- Server tests: 49/49; admin e2e: 88/89 + 1 skipped; typecheck clean;
lint only preexisting warnings
Comment on lines
88
to
+93
| async saveProfile(_p: Profile.Profile): Promise<void> { | ||
| this.notImpl('saveProfile'); | ||
| } | ||
| async createProfile(_p: Profile.Profile): Promise<{ created: boolean }> { | ||
| this.notImpl('createProfile'); | ||
| } |
…file CRUD Copilot iter-2 review (1): - The existing "every method throws not implemented" test only asserted saveRun/loadRun; extend it to cover the full Profile CRUD surface (listProfiles, loadProfile, saveProfile, createProfile, deleteProfile) so the scaffold contract stays accurately enforced as new Store methods are added. Store tests: 6/6 pass; typecheck clean; lint preexisting only.
| > | ||
| <div className="col gap-12"> | ||
| {error && ( | ||
| <Alert kind="danger" title="Clone failed" data-testid="profile-clone-error"> |
Contributor
Author
There was a problem hiding this comment.
Addressed in commit e7b0a2a (iter 3): switched to kind="error" and moved the data-testid onto the inner <span> so it lands on actual DOM. See packages/admin/src/app.tsx:5754-5759.
Comment on lines
+5756
to
+5757
| <Alert kind="danger" title="Clone failed" data-testid="profile-clone-error"> | ||
| <span style={{ fontSize: 12 }}>{error}</span> |
Contributor
Author
There was a problem hiding this comment.
Addressed in commit e7b0a2a (iter 3): switched to kind="error" and moved the data-testid onto the inner <span> so it lands on actual DOM. See packages/admin/src/app.tsx:5754-5759.
Comment on lines
+98
to
+107
| s.saveProfile({ | ||
| schema_version: '1', | ||
| name: 'p', | ||
| execution_mode: 'orchestrator', | ||
| llm_usage: [], | ||
| packs: [], | ||
| tags: [], | ||
| parallelism: 1, | ||
| require_deterministic_replay: false, | ||
| }), |
Contributor
Author
There was a problem hiding this comment.
Addressed in commit e7b0a2a (iter 3): both literals now include llm_budget_usd: null. See packages/store/test/store.test.ts:103 (saveProfile) and :118 (createProfile).
Comment on lines
+112
to
+121
| s.createProfile({ | ||
| schema_version: '1', | ||
| name: 'p', | ||
| execution_mode: 'orchestrator', | ||
| llm_usage: [], | ||
| packs: [], | ||
| tags: [], | ||
| parallelism: 1, | ||
| require_deterministic_replay: false, | ||
| }), |
Contributor
Author
There was a problem hiding this comment.
Addressed in commit e7b0a2a (iter 3): both literals now include llm_budget_usd: null. See packages/store/test/store.test.ts:103 (saveProfile) and :118 (createProfile).
…ffold test
Copilot iter-3 review (4 issues, 1 fix):
- CloneProfileWizard's error Alert used kind="danger", but Alert only
recognizes error/warning/success/info/ai — "danger" silently dropped
the role="alert" live-region semantics and the styling. Switched to
kind="error" so screen readers announce the failure.
- The Alert component destructures only {kind, title, children, icon}
and doesn't forward unknown props to the root DOM. The
data-testid="profile-clone-error" was therefore silently dropped.
Moved the testid onto the inner <span> that wraps the error text,
which is the actual content under test.
- Two test profiles in packages/store/test/store.test.ts omitted the
Profile contract's `llm_budget_usd` field. Added llm_budget_usd:null
to both saveProfile() and createProfile() literals so the scaffold
test stays accurately schema-conforming.
Tests: 11/11 clone e2e; 6/6 store; typecheck clean.
4 tasks
lopadova
added a commit
that referenced
this pull request
May 19, 2026
…area wizard (#37) * feat(slice-4c.7+4c.8 admin): scenario edit/clone via shared YAML-textarea 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. * chore(slice-4c.7-admin): bun.lock for new yaml dependency * review(slice-4c.7+4c.8-admin iter 1): address Copilot review feedback 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. * review(slice-4c.7+4c.8-admin iter 2): perf + correctness fixes from Copilot 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. * review(slice-4c.7+4c.8-admin iter 3): tighten canSubmit + persist server 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. * review(slice-4c.7+4c.8-admin iter 4): move deletedScenarios state before 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. * review(slice-4c.7+4c.8-admin iter 5): align spec preview with schema + 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. * review(slice-4c.7+4c.8-admin iter 6): scenario fixtures to Slug + client 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. * review(slice-4c.7+4c.8-admin iter 7): debounce sync on seed + comment 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. * review(slice-4c.7+4c.8-admin iter 8): tighten event guards + small refactors 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
v1.7 slice 4c.3 — Profile Clone, the third micro-PR in slice 4c (after Delete #29 and Edit #30).
Server
POST /api/profiles:@aqa/schemasProfilevalidationcode: EEXISTon name collisionprofiles:editAdmin
CloneProfileWizard— source name read-only, slug-validated new name, inline collision check vs mock list + already-created clones + selfinFlightRefsynchronous double-submit guardsubmittedSourceat fetch start; stale-submit guard compares to render-timeprofileRef.current?.nameafter the awaited POSThandleClosefor cancel + reopen on the same sourcecreatedProfilesMap +aqa:profile-createdwindow event so the cloned profile appears in the Profiles list and resolves on its new detail page without a reloadPageProfileDetail: Clone button between Edit and Delete;onClonednavigates to the new profileE2E
profile-clone.e2e.ts, all green with--workers=2Test plan
bun run --filter @aqa/server test(48/48)bun run --filter @aqa/admin test:e2e -- --workers=2(87 passed, 1 skipped)bun run typecheckbun run lint(only 4 preexisting warnings in auth/adapters tests)🤖 Generated with Claude Code