feat(web): Profiles tab -- browse, search, create, edit, clone, delet… - #692
feat(web): Profiles tab -- browse, search, create, edit, clone, delet…#692sujoydc wants to merge 4 commits into
Conversation
haofeif
left a comment
haofeif
left a comment
There was a problem hiding this comment.
Thanks for the very thorough write-up — the self-review section made this much faster to audit, and every hardening claim in it held up under execution (details below).
Reviewed at 0948572a (merge-base b6a0520b). Web-only: no Python file changes.
Author claims I reproduced
| Claim | Result |
|---|---|
| 63 new UI tests | Exact. merge-base 177 passed, PR 240 passed -> +63 |
| Full web suite 240/240 | Exact. Test Files 16 passed, Tests 240 passed |
tsc clean, production build clean |
Confirmed. npm run build exit 0 |
| No pre-existing regressions | Confirmed. merge-base 177/177 green; 0 tests regressed |
rewriteFrontmatterName handles $-patterns |
Confirmed for $&, $', $`, $1, $$, both in the typed name and pre-existing in the frontmatter |
| CRLF documents rewrite rather than silently no-op | Confirmed |
Editor loads unresolved /source, never the resolved parse |
Confirmed, and genuinely pinned (mutation below) |
| Truncation contract rendered exactly | Confirmed against live validator output |
| Search order never re-sorted | Confirmed — the fixture is genuinely discriminating |
| Backdrop inert while saving | Confirmed in both modals |
ConfirmModal.confirmationText additive |
Confirmed — gated on !== undefined; no existing caller passes it |
Truncation contract, end-to-end
Rather than trust the copied constant, I drove the real validator to truncation:
300 non-string capabilities: total=100 markers=1 marker_idx=[99] last_sev=error
last_message='Additional validation findings omitted.'
EXACT-MATCH-UI-CONSTANT=True
100 including the marker, exactly one marker, last position, error severity because errors were dropped, and byte-identical to ValidationFindings.OMISSION_MESSAGE. isOmissionMarker requires index === all.length - 1, so the marker text in a non-final position does render as an ordinary finding, as documented.
The secret-leak path is really pinned
I mutated getProfileSource to drop /source (so the editor would read the resolved profile):
Tests 9 failed | 6 passed (15)
Nine editor tests go red. That behaviour is enforced, not just asserted in prose.
Things I suspected and cleared
- Portaled
CustomSelectmenu breaking existing callers.z-[80]clears thez-[60]maximum elsewhere, and the two backdrop closers (AgentPanel:518,FlowsPanel:300) are element-levelonClicks, not document-level handlers, so a click on the portaled menu never reaches them. The pre-existingmemory-graphtest opens the menu and clicks an option, so this is covered by tests that predate the PR. - Mixed line endings from a CRLF clone.
rewriteFrontmatterNamedoes emit a mixed-ending document (---\n...\r\n...---\r\n). I fed it to the real backend parser and it round-trips cleanly (meta={'name': 'newname', 'description': 'd'}), as does${VAR}inside a JSON-valued field. Not a defect — noting it only so it doesn't get re-litigated. - Route ordering. All 14 URLs match
api/main.py, and every literal path is declared before/agents/profiles/{name}. deleteProfileagainst a 204.fetchJSON's empty-body branch already covers it.
What blocks
Two defects, same root cause: a debounced effect whose early return does not invalidate the in-flight request token. The monotonic tokens are correct for the case they were written for (a newer request superseding an older one) but not for "the reason to want this response disappeared".
The first one silently persists the wrong document, so I'm marking this changes-requested — everything else here is in good shape, and both fixes are one-liners I verified.
Both fixes together keep 240 passed and tsc/build clean.
| // Debounced live preview: one render request per quiet burst of config edits. | ||
| useEffect(() => { | ||
| if (previewTimer.current) clearTimeout(previewTimer.current) | ||
| if (!template || !templateSchema) return |
There was a problem hiding this comment.
[P1] A stale template preview can be persisted under a different template
In template mode the create POST body is the preview state (ProfileCreateModal.tsx:471), and Create is gated on preview !== null && !previewLoading (:462) precisely so a mid-debounce render can't be persisted. That gate can be defeated.
Selecting a different template runs setTemplateSchema(null) synchronously (:316). The preview effect then re-runs and hits this early return — which does not bump previewSeq and does not clear preview. When the previous template's in-flight render lands, seq === previewSeq.current still holds, so it calls setPreview(oldContent) and setPreviewLoading(false).
Reproduced (template A's preview released after switching to template B, B's schema still loading):
PROBE selected template shows: aws/stepfunction
PROBE preview pane present: false | contains TEMPLATE-A-BODY: undefined
PROBE create button disabled: false
PROBE POSTed body contains TEMPLATE-A-BODY: true
This is silent rather than merely wrong-looking: the whole Live preview block is gated on (templateSchema || previewError) (:593), so during this window the user sees only "Loading template schema…" (:570) — nothing on screen shows template A's content — while Create is enabled and armed with it. Clicking it writes a profile whose body came from the template the user just navigated away from, under the name they chose, with no error.
Fix — invalidate the token and drop the orphaned render:
if (previewTimer.current) clearTimeout(previewTimer.current)
if (!template || !templateSchema) {
// The reason to want the in-flight render just disappeared: discard it
// so it cannot land as this template's preview (and be persisted).
previewSeq.current++
setPreview(null)
return
}Verified: PROBE create button disabled: true, no stale POST, and the full suite stays at 240 passed. (Leaving previewLoading true here is correct — the next schema load re-arms it.)
| useEffect(() => { | ||
| if (debounceRef.current) clearTimeout(debounceRef.current) | ||
| const q = query.trim() | ||
| if (q === '') { |
There was a problem hiding this comment.
[P2] Clearing the search box restores stale results once the in-flight response lands
Same shape as the P1, without the write. Clearing the box takes this early return after setResults(null), but never bumps searchSeq, so an already-dispatched search still satisfies seq === searchSeq.current at :283 and calls setResults(r). Since rows = results ?? catalog (:299), the list drops back to the filtered set.
Reproduced (response released after the box is cleared):
PROBE search requests in flight: 1
PROBE rows after clear (before response lands): 6
PROBE searchbox value: ""
PROBE rows after stale response lands: 1 ["mid-agentkir"]
The user is left looking at a filtered list with an empty search box and no indication why — including the duplicated_in shadowing warnings that only the catalog rows carry. It is recoverable (type a character and clear again, with nothing in flight), which is why this is a P2 and not a P1.
Fix:
if (q === '') {
searchSeq.current++
setResults(null)
setSearchError(null)
setSearching(false)
return
}Verified: the probe then reports all 6 catalog rows, and the full suite stays at 240 passed.
| } | ||
|
|
||
| /** Extract the frontmatter `name:` value from a rendered document, if any. */ | ||
| export function extractFrontmatterName(content: string): string | null { |
There was a problem hiding this comment.
[nit] extractFrontmatterName can match a name: line in the markdown body
[\s\S]*? isn't bounded to the end of the frontmatter block, so when the frontmatter has no name: the scan continues into the document body:
EXTRACT no-fm-name -> "decoy" // '---\ndescription: d\n---\nbody\nname: decoy\n'
EXTRACT normal -> "real"
EXTRACT quoted -> "real"
EXTRACT crlf -> "real"
Impact today is limited to pre-filling the profile-name box (:343, only when !nameTouched.current), and every shipped template carries a frontmatter name, so I don't think it's reachable — hence nit. Bounding the scan to the block would close it, e.g. match the frontmatter with /^---\r?\n([\s\S]*?)\r?\n---/ first and search name: within that captured group, which is what rewriteFrontmatterName already does correctly.
gutosantos82
left a comment
gutosantos82
left a comment
There was a problem hiding this comment.
PR Review: #692 — feat(web): Profiles tab — browse, search, create, edit, clone, delete profiles
Summary
Adds the Profiles tab to the CAO Web UI (browse/search/create/edit/clone/delete over the #575/#585 APIs) — the final piece of #510. The implementation and its 63 new tests are high quality: we independently reproduced the 240/240 suite, clean tsc/build, and every hardening claim in the PR body. However, maintainer @haofeif has an open CHANGES_REQUESTED at this exact head for a data-integrity race (stale template preview silently persisted), and our review found the root cause is systemic: previewSeq is bumped only when a request is issued, never when the preview is invalidated, so the flagged one-liner alone leaves two more paths to the same silent persistence. Recommendation: fix the token-invalidation class as a whole (all clear sites), not just the two flagged lines, and land regression tests with it.
Blocking (must fix before merge)
- [correctness][verification] web/src/components/ProfileCreateModal.tsx:286-306 (open-reset effect) and :311-314 (template-deselect branch) — 🆕 Both
setPreview(null)without bumpingpreviewSeq.current(the reset also leavespreviewTimerandpreviewLoadinguntouched). The modal stays mounted across close (openprop), so: preview in flight → close → reopen → the stale response still satisfiesseq === previewSeq.current, re-landssetPreview(staleContent)+setProfileName(extractFrontmatterName(stale)), andcanCreate(which checks onlypreview !== null && !previewLoading, nottemplate) enables Create — persisting a body from a template that isn't even selected, with the preview pane not rendered. This is the same silent-persistence class as the maintainer's P1 but a distinct trigger his line-333 fix does not cover. Fix: invalidate the token at every clear site — bumppreviewSeq.current, clearpreviewTimer, resetpreviewLoadingin the reset effect and the deselect branch (or bump the token on every preview-effect run/teardown).
Important (should fix)
- [consistency][correctness] ProfileCreateModal.tsx:309-327 — 🆕
getTemplateSchemais the one fetch in the modal with no staleness guard (no seq token, no cancelled flag, no abort) — unlike the search, preview, andProfileDetail.getProfileeffects beside it. Fast A→B template switch with out-of-order resolution leavestemplateSchema= A's schema andconfig= A's seeded defaults whiletemplate= B: the form renders A's fields under a B selection and the debounced preview renders B with A's config. Same fix norm as the blocking item: token orcancelledflag keyed on the selected template. - [tests] web/src/test/ — 🆕 The monotonic seq-guard machinery the PR headlines has zero test coverage: no test creates two overlapping in-flight requests with reordered resolution, so the
seq !== ...currentdiscard branch is never taken (all search/preview mocks resolve synchronously). The P1/P2 fixes should ship with gate-promise regression tests — (a) search: type, let the request go in flight, clear the box, then resolve → assert results stay null; (b) preview: template A render in flight, switch to B, resolve A → assert no A content in pane and Create does not POST A's body. Otherwise the one-liner fixes land undefended. - [tests] web/src/components/CustomSelect.tsx — 🆕 The shared-component rewrite (portal to
document.body, fixed positioning, flip-up, scroll/resize-to-close, portal-aware outside-click, newinvalidprop) is essentially untested — tests only pick options by testid, and theinvalidred-boundary path on a select is never asserted (the two red-boundary tests hit text/number inputs). This component backs Flows and Agents too; a regression in scroll-close/flip would be invisible to the suite. - [correctness] ProfileCreateModal.tsx (SchemaField object branch) — 🆕 Object/JSON textareas are uncontrolled (
defaultValue) and always receivevalue=undefined(object fields render only in Advanced, which passesundefinedfor object types). Collapse and reopen Advanced (or trigger the error-driven auto-expand) and the textarea remounts empty whilejsonDraftsstill holds — and persists — the typed JSON. Display/state desync: user sees an empty field, the value saves anyway; retyping clobbers the draft. Make the textarea controlled from the draft string. - [conventions] web/README.md — 🆕 The canonical frontend doc (linked from docs/web-ui.md and CODEBASE.md) enumerates every page, supporting component, and file in three places; this PR adds a page (ProfilesPanel) + three components (ProfileCreateModal, ProfileEditorModal, ValidationFindings) and updates none of them. Per the repo's documentation-maintenance rule, update all three enumerations in this PR.
- [conventions] CHANGELOG.md — 🆕 No
[Unreleased] → Addedentry for the new Profiles tab. The #510 arc's API work is already recorded in the changelog; this user-facing UI piece should be too.
Nits (optional)
- [correctness] ProfileCreateModal/ProfileEditorModal save paths — a transport error/timeout on the pre-save
validateProfile(a UX gate; the write route re-validates authoritatively) hard-blocks the save behind a generic "Validation failed". Consider letting transport failures fall through to the server-side validation while still blocking on real 400s. - [consistency] web/src/api.ts:174, 484-495 —
TemplateConfigValidation+validateTemplateConfig()are defined but never referenced anywhere in web/src (the UI usespreviewTemplateinstead). Wire it or drop it. - [consistency] web/src/api.ts:474-476 — the "category/name travels as two path segments, slash must NOT be encoded" comment sits above
getProfileSchema(fixed path, no template arg) but describesgetTemplateSchemabelow it. Move it. - [tests] App.tsx Alt+N renumbering — the flagged-as-risky shortcut shift has no keyboard test (only DOM order is asserted); one test that Alt+2 opens Profiles and Alt+3 opens Agents would pin it.
- [tests] ConfirmModal
typedreset-on-open and the delete/detail error paths (DELETE rejection → snackbar + row retained; detail-panegetProfilefailure) are untested. - [conventions] docs/web-ui.md — the Features paragraph doesn't mention profile management; one clause would do (component detail stays in web/README.md).
- [conventions] commit subject — 77 chars, over the ~72 norm; move the verb enumeration to the body.
- [conventions] ProfilesPanel.tsx — the
{/* Master list */}comment: "master-detail" is a standard pattern name, but the standalone "Master" could read as "List pane" per the project's inclusive-language convention.
Tests
63 new tests across 4 files, counts exact, and assertion quality is deliberately anti-tautological (ordering fixtures defeat both alphabetical and catalog-order coincidences; $&/$'/CRLF adversarial cases assert the actual old bugs; the byte round-trip pins ${VAR} through edit→PUT; the clone test asserts validated bytes equal the POST body; optimistic delete is proven independent of the refetch). The systematic gap is concurrency: the stale-response discard branch is never exercised anywhere, which is precisely where all the known and new race findings live — see the Important items for the two regression tests that should accompany the fixes. Secondary gaps: the CustomSelect rewrite, Alt+N mapping, ConfirmModal reset, and a few error paths.
Verification
Independent dynamic verification at this head (clean checkout):
- ✓ VERIFIED — web suite: 16 files, 240/240 passed (after working around the known npm optional-deps issue for rolldown's native binding, unrelated to the PR).
- ✓ VERIFIED —
npm run build(tsc + vite) exit 0; standalonetscexit 0; only the advisory 823 kB chunk-size notice. - ✓ VERIFIED — maintainer P1 defect chain end-to-end by code reading (early return at :333 leaves token valid → stale
setPreview→canCreateenables →createProfilepersists the stale body), plus the two additional un-flagged instances of the same root cause (open-reset effect, template-deselect branch) now in Blocking. - ✓ VERIFIED — maintainer P2 at ProfilesPanel.tsx:270-276 as described.
- ✓ VERIFIED — the
extractFrontmatterNamenit reproduced empirically (bodyname:returned when frontmatter lacks one). - ✓ Audited the remaining async paths in the diff: ProfileEditorModal's unguarded load is not a practical defect (conditional mount discards stale state); ProfilesPanel detail effect, MemoryGraphView, and useEventFollow use correct guard patterns.
Verdict
Request changes — maintainer @haofeif's CHANGES_REQUESTED stands at this exact head with the P1 data-integrity blocker unaddressed, and the fix should cover the whole token-invalidation class (the two additional preview-clear sites and the unguarded getTemplateSchema fetch), with regression tests, not just the two flagged lines. Everything else about the PR is in strong shape; once the race class is closed and the doc/CHANGELOG drift is patched, this is a clean approve.
Review findings on #692: the monotonic seq tokens guarding the debounced search and preview fetches were bumped only when a new request was issued, never when the reason for the in-flight request disappeared. Every clear path therefore left the old token valid, letting a late response re-land silently: - template switch (preview effect early return): the previous template's render re-armed Create with its body while the pane showed only 'Loading template schema...' (P1, silent wrong-document persistence) - modal close/reopen and template deselect: same class, distinct triggers - search-box clear: stale results restored under an empty box (P2) Also adds the staleness token getTemplateSchema was missing (fast A->B switch with out-of-order resolution left A's schema under B's selection). Regression tests use gated promises to genuinely reorder resolutions -- the seq-discard branch was previously never exercised by any test. All four new tests fail against the unfixed components (mutation-verified). Suite: 244/244, tsc clean, build clean.
Review items on #692: - Object/JSON textareas are controlled from the draft string: an uncontrolled defaultValue remounted EMPTY when Advanced collapsed and reopened while the draft silently persisted into the POST -- the user saw a blank field but the typed JSON still saved - extractFrontmatterName matches the frontmatter block first and scans name: within it, so a 'name:' line in the markdown body can no longer pre-fill the profile-name box (same bounding rewriteFrontmatterName already used) - CustomSelect gains its own suite (portal placement, portal-aware outside-click, flip-up/down positioning, scroll- and resize-to-close, invalid red boundary, disabled options). Writing it exposed a real defect: the resize path reused the scroll handler, whose contains() check throws on a resize event's window target, so the menu never closed on resize -- fixed with an instanceof Node guard - web/README.md (Profiles page section, supporting-components table, project tree), CHANGELOG.md Unreleased/Added, and docs/web-ui.md Features updated per the repo documentation-maintenance rule Suite: 255/255, tsc clean, build clean.
Remaining #692 review nits: - drop validateTemplateConfig + TemplateConfigValidation (defined but never referenced; the UI uses previewTemplate) and move the two-path-segment encoding comment to getTemplateSchema, the method it describes - pre-save validate transport failures fall through to the authoritative server-side validation instead of hard-blocking behind a phantom 'Validation failed'; real 4xx findings still block (both modals) - rename the 'Master list' comment to 'List pane' per the inclusive naming convention - tests: Alt+2/Alt+3 keyboard mapping pinned beyond DOM order, ConfirmModal typed-confirmation reset on reopen, DELETE failure (error snackbar + row retained), detail load failure (list survives, reselect works), and the validate transport fall-through Suite: 260/260, tsc clean, build clean.
PR #692 — reviewer reply draftsAll fixes are in three new commits (no force-push): 2806cfd (blockers), Reply 1 — haofeif's P1 thread (ProfileCreateModal.tsx:333, stale preview persisted)Fixed in 2806cfd, and you were right that the root cause was the token only Your probe scenario is now a regression test: template A's render released Reply 2 — haofeif's P2 thread (ProfilesPanel.tsx:272, stale search restored)Fixed in 2806cfd with exactly your snippet ( Reply 3 — haofeif's nit thread (extractFrontmatterName body match)Fixed in e070c3b using the bounding you suggested: match the frontmatter Reply 4 — top-level reply to haofeif's reviewThanks for the depth here — reproducing every claim in the PR body and then Follow-up commits e070c3b and 33d6477 address the rest of the review Reply 5 — top-level reply to gutosantos82's reviewThanks — the systemic framing was the right call. All three blocking sites On the important items (e070c3b): the JSON textareas are now controlled from Nits (33d6477): dead |
3 participants
Overview
Third and final PR for #510, building the web UI on top of the APIs from #575 (validation service,
/validate,/schema) and #585 (create/update/delete/source endpoints). Adds a Profiles tab where you can browse and search installed profiles, create new ones from a scaffold template or from scratch, edit and clone existing ones, and delete local-store profiles — with the backend validator wired in front of every write.AgentPanelis untouched; it remains the agent-launch picker. Closes #510 once merged.What's in it
Navigation. A Profiles tab between Home and Agents, and the Home dashboard's Profiles stat card now navigates there. This renumbers the Alt+N shortcuts for the tabs after it (one-time shift; the code comment explains the ordering rationale).
List, search, detail. Master-detail layout. The catalog is fetched once on mount — no polling. Search delegates ranking to
GET /agents/profiles/searchwith a 300 ms debounce; results render in server order (the client never re-sorts) and a monotonic token discards stale responses. The detail pane shows source, provider, model, tags, capabilities, and a warning whenduplicated_inreports the name shadowed across directories.Create. One modal, two entry points. From template: pick a scaffold template, fill a form generated from that template's own JSON-Schema, and watch a live preview rendered by
POST /templates/preview(same 300 ms debounce; Create is gated while a render is in flight, and the preview shows the exact document that will be persisted, including the frontmatter name rewrite). From scratch: the form is generated fromGET /agents/profiles/schema— primary fields visible, everything else behind an Advanced expander. Object-valued fields (mcpServers,codexConfig, …) use validated JSON editors rather than bespoke widgets. Frontmatter is emitted as JSON-valued YAML, so no YAML dependency is added.provideris a select fed by the live registry (uninstalled providers labelled but selectable, free-text fallback if the registry call fails);roleis a datalist with the built-in roles plus free entry forsettings.jsoncustom roles;modelstays free text deliberately — there is no registry of valid model IDs to validate against.Edit, clone, delete. Edit opens a raw document editor over
GET /agents/profiles/{name}/sourceand saves via PUT — deliberately not the schema form, because an edit must round-trip the exact stored bytes (env-var placeholders intact; one test pins a${VAR}surviving the full load→edit→PUT cycle). Only local-store profiles get Edit/Delete; built-in, provider, and custom profiles are read-only and offer "Clone to customise", matching the backend's write model. Delete sits behind a type-the-name-to-confirm gate (an optional, additiveconfirmationTextprop on the sharedConfirmModal; existing callers unchanged).Validation.
POST /agents/profiles/validateruns before every save, in both modals and both modes. Errors block client-side; warnings render but allow (and surface again via snackbar after the save). The findings panel renders the truncation contract from #585 precisely: at most 100 findings including one omission marker, exactly once, last, with its severity matching the omitted producer — an error-severity marker is explicitly flagged as hiding errors, and the marker text in any non-final position renders as an ordinary finding. Error findings also paint the specific form control they name (dotted path rooted at the frontmatter key,(root)required-property errors mapped by the quoted name), auto-expanding the Advanced section when the target is hidden. Clone validation runs on the exact rewritten document; one test asserts the validated bytes equal the POST body.Hardening
The branch was reviewed adversarially before opening, with empirical probes against the frontmatter helpers; everything found was fixed here rather than left for review:
rewriteFrontmatterNameuses replacement functions, not strings — a replacement string interprets$-patterns, which corrupted names containing$&/$'and mangled documents whose frontmatter legally contains such text.Tests
63 new UI tests across four files (12 panel, 25 create modal, 15 editor, 11 findings renderer); full web suite 240/240,
tscclean, production build clean. The areas #585's review focused on get the same treatment here: debounce coalescing is asserted by call count, the search-order test uses three rows whose ranked order differs from both alphabetical and catalog order so a re-sort can't pass by coincidence, and the truncation rendering is tested with full 100-finding payloads including marker-severity and marker-position cases.Not in scope (per #510)
Launching agents from the Profiles tab, workflow/team composition, profile rename semantics, and full JSON-Schema modelling of every object-valued field (JSON editors suffice). One known follow-up: making the template live preview directly editable (with dirty-state semantics so form edits don't clobber manual edits) — deferred to keep this PR reviewable; happy to file an issue.