fix(desktop): use shared create-profile dialog on Manage Profiles page - #73013
Conversation
The Manage Profiles page had its own local CreateProfileDialog/ RenameProfileDialog copies that predated the shared dialogs in create-profile-dialog.tsx / rename-profile-dialog.tsx. The local create copy lacked the SOUL.md textarea, so New Profile from the sidebar rail and New Profile from Manage Profiles rendered different modals. Delete both local duplicates and reuse the shared self-contained dialogs (they own the createProfile/renameProfile/updateProfileSoul calls), so both entry points show the same modal including SOUL.md.
alt-glitch
added
type/bug
૮ >ﻌ< ა ci reviewran on a14e25a ℹ️ InfoDesktop E2E visual evidence · View test artifacts · View job1 visual diff. inline evidence upload failed. Failed to upload diff-665a0833239e-onboarding-overlay-diff.png with gh image (exit code 1): Error uploading /home/runner/work/_temp/e2e-evidence/diff-665a0833239e-onboarding-overlay-diff.png: step 0 (get upload token): uploadToken not found on repo page — do you have write access to NousResearch/hermes-agent? (or, if NousResearch enforces SAML SSO, authorize at https://github.com/orgs/NousResearch/sso) |
OutThisLife
left a comment
OutThisLife
left a comment
There was a problem hiding this comment.
Fuller pass on apps/desktop/src/app/profiles/index.tsx against the shared profile dialogs and apps/desktop/DESIGN.md. The premise checks out and the de-duplication is the right shape — I just don't want it landing as two-thirds of the class it claims to close.
What's right
- The drift is real. On
main,index.tsxcarries localCreateProfileDialog/RenameProfileDialogcopies (lines 475–707) whileapp/chat/sidebar/profile-switcher.tsximports the shared ones, and the local create copy never got the SOUL.md textarea. - The shared dialogs are self-contained (they own the
createProfile/renameProfile/updateProfileSoulcalls), so collapsing both callbacks into a singleselectAndRefreshis correct and leaves nothing dangling. PROFILE_NAME_RE/isValidProfileNamecollapse to the one exported definition increate-profile-dialog.tsx.- Dropping the toasts doesn't orphan locale keys:
p.created/p.renamedare still consumed by the sharedActionStatusdonelabels.
1. The delete dialog is the third duplicate, and it's the one carrying a bug
This is the same note I left on #69687 (point 6): a hand-rolled delete Dialog next to a shared ConfirmDialog that already owns Enter-to-confirm, the pending→done beat, and inline error. Here it's worse than on webhooks, because profiles doesn't just have the generic ConfirmDialog — it has a purpose-built wrapper sitting in the same folder, and that wrapper's own doc comment claims this page as a caller:
// Thin wrapper over ConfirmDialog: owns the deleteProfile call, inherits
// Enter-to-confirm + busy/done/error from the shared dialog. The single choke
// point for every delete entry point (rail + Profiles view).
export function DeleteProfileDialog({ … })The Profiles view was never switched over, so it missed the fix that landed in f764b0400:
// Deleting the profile the live gateway is on strands it on a dead
// backend. Capture that before the delete; reset *after* the host's
// onDeleted refresh so a refreshActiveProfile racing the (still-dying)
// backend can't clobber the pill back to it.
const wasActive = normalizeProfileKey(profile.name) === normalizeProfileKey($activeGatewayProfile.get())
await deleteProfile(profile.name)
await onDeleted?.()
if (wasActive) {
selectProfile('default')
setActiveProfile('default')
}handleConfirmDelete (index.tsx:151) has none of it — it deletes, toasts, and refreshes the list. Delete the profile you're currently on from Manage Profiles and the gateway plus the statusbar pill stay pointed at a dead backend. That's the same drift class as the SOUL.md symptom, only functional rather than cosmetic, and it's what the PR description means when it says it "removes the drift class."
While it's hand-rolled it also diverges on chrome: variant="outline" Cancel where ConfirmDialog and both shared profile dialogs use ghost, and a deleting ? p.deleting : delete label instead of ActionStatus. All of that disappears with the swap.
Please fold it into this PR:
<DeleteProfileDialog
onClose={() => setPendingDelete(null)}
onDeleted={async () => {
setSelectedName(null)
await refresh()
}}
open={pendingDelete !== null}
profile={pendingDelete}
/>and drop handleConfirmDelete, the deleting state, the deleteProfile import, and the now-unused Dialog/DialogContent/DialogDescription/DialogFooter/DialogHeader/DialogTitle block. p.deleted stays live as the doneLabel.
2. The name field loses live slugging — this aligns by leveling down
The local copies used SanitizedInput with sanitize={slug}, so typing My Profile became my-profile as you went. The shared dialogs have never used it (git log -S SanitizedInput on both files is empty) — they use a plain Input, so the same keystrokes now leave the field invalid, the submit button disabled, and the hint in its error color.
That's a real downgrade for this surface, and the two surfaces end up matching at the worse of the two behaviors. SanitizedInput composes inside Field exactly like Input does (see chat/sidebar/projects/worktree-dialog.tsx:193), so please level up instead — in create-profile-dialog.tsx:
<SanitizedInput
aria-invalid={invalid}
autoFocus
id="new-profile-name"
onValueChange={setName}
placeholder="my-profile"
sanitize={slug}
value={name}
/>and the same in rename-profile-dialog.tsx. Both entry points then get the behavior the Manage page has today, which is what the sanitize primitive exists for: "callers never have to validate-then-reject."
3. Nothing tests the bug class
Same shape as my note on #69077 — the change is correct but untested where it matters. There's no test anywhere under apps/desktop that renders ProfilesView or any of the three profile dialogs, so the invariant this PR exists to establish ("both New Profile entry points render the same modal") is enforced by nothing, which is exactly how the drift got here in the first place.
Per the root AGENTS.md, this wants to be a behavior test, not a source-shape assertion. Rendering ProfilesView with mocked @/hermes, opening create, and asserting the SOUL.md field is present covers it — and after (1), asserting that deleting the active profile lands on default covers the real bug.
Summary
Do (1) and (3) before this lands; (2) is small and belongs in the same pass since it touches the same two files. Logic and de-duplication are otherwise fine, and CI is green — the one visual diff in the E2E evidence is the onboarding-overlay baseline that's also showing on #73074 and #73062, so it isn't yours.
…st the view Addresses review on #73013. 1. Manage Profiles used a hand-rolled delete Dialog next to the shared DeleteProfileDialog in the same folder. That copy missed the active- profile re-home fix (f764b04): deleting the profile the gateway is on stranded it on a dead backend. Switch to the shared dialog, which owns the deleteProfile call and re-homes to default. Drops handleConfirmDelete, the deleting state, and the now-unused Dialog* imports. 2. The name field regressed to a plain Input during the create-dialog dedup, losing live slugging. Level both shared dialogs up to SanitizedInput sanitize={slug} so every entry point gets the behavior Manage Profiles had — the sanitize primitive means callers never validate-then-reject. 3. Nothing rendered ProfilesView, which is how the drift got in. Add a behavior test: create dialog exposes SOUL.md, deleting the active profile re-homes to default, deleting a non-active one does not.
|
Addressed all three in 4430a74. 1. Delete dialog → shared 2. Name field leveled up, not down. Both shared dialogs now use 3. Test the bug class. Added Verified: |
main now labels each panel row's kebab with the row's name
(menuLabel={profile.name}), so the hardcoded "Actions" default this test
relied on no longer exists. The name alone is ambiguous — the row-select
button carries it too — so match the menu trigger via `expanded`.
Neither side conflicts textually, so this only surfaced once main merged in.
There was a problem hiding this comment.
Pull request overview
This PR removes the locally duplicated “Create/Rename Profile” dialogs from the desktop Manage Profiles view and reuses the shared dialog components so both profile entry points (sidebar rail + Manage Profiles page) render the same modal content (including the SOUL.md textarea). It also adds targeted UI tests to prevent this kind of drift from reappearing.
Changes:
- Replaced Manage Profiles’ local create/rename/delete dialog implementations with shared
CreateProfileDialog,RenameProfileDialog, andDeleteProfileDialog. - Updated shared create/rename dialogs to use
SanitizedInputfor live profile-name formatting. - Added
ProfilesViewtests to assert create-dialog parity (SOUL.md present) and correct gateway re-homing behavior on delete.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/desktop/src/app/profiles/index.tsx | Removes local dialog duplicates and uses shared profile dialogs + consolidated refresh/selection callback. |
| apps/desktop/src/app/profiles/create-profile-dialog.tsx | Switches profile name input to SanitizedInput with live sanitization. |
| apps/desktop/src/app/profiles/rename-profile-dialog.tsx | Switches rename input to SanitizedInput with live sanitization. |
| apps/desktop/src/app/profiles/index.test.tsx | Adds coverage for shared create-dialog parity (SOUL.md) and delete re-homing behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <Field htmlFor="rename-profile-name" label={p.newNameLabel}> | ||
| <Input | ||
| <SanitizedInput | ||
| aria-invalid={invalid} | ||
| autoFocus | ||
| id="rename-profile-name" | ||
| onChange={event => setName(event.target.value)} | ||
| onValueChange={setName} | ||
| sanitize={slug} | ||
| value={name} | ||
| /> |
| <SanitizedInput | ||
| aria-invalid={invalid} | ||
| autoFocus | ||
| id="new-profile-name" | ||
| onChange={event => setName(event.target.value)} | ||
| onValueChange={setName} | ||
| placeholder="my-profile" | ||
| sanitize={slug} | ||
| value={name} |
…st the view Addresses review on #73013. 1. Manage Profiles used a hand-rolled delete Dialog next to the shared DeleteProfileDialog in the same folder. That copy missed the active- profile re-home fix (f764b04): deleting the profile the gateway is on stranded it on a dead backend. Switch to the shared dialog, which owns the deleteProfile call and re-homes to default. Drops handleConfirmDelete, the deleting state, and the now-unused Dialog* imports. 2. The name field regressed to a plain Input during the create-dialog dedup, losing live slugging. Level both shared dialogs up to SanitizedInput sanitize={slug} so every entry point gets the behavior Manage Profiles had — the sanitize primitive means callers never validate-then-reject. 3. Nothing rendered ProfilesView, which is how the drift got in. Add a behavior test: create dialog exposes SOUL.md, deleting the active profile re-homes to default, deleting a non-active one does not.
…st the view Addresses review on NousResearch#73013. 1. Manage Profiles used a hand-rolled delete Dialog next to the shared DeleteProfileDialog in the same folder. That copy missed the active- profile re-home fix (80a2d79): deleting the profile the gateway is on stranded it on a dead backend. Switch to the shared dialog, which owns the deleteProfile call and re-homes to default. Drops handleConfirmDelete, the deleting state, and the now-unused Dialog* imports. 2. The name field regressed to a plain Input during the create-dialog dedup, losing live slugging. Level both shared dialogs up to SanitizedInput sanitize={slug} so every entry point gets the behavior Manage Profiles had — the sanitize primitive means callers never validate-then-reject. 3. Nothing rendered ProfilesView, which is how the drift got in. Add a behavior test: create dialog exposes SOUL.md, deleting the active profile re-homes to default, deleting a non-active one does not.
…st the view Addresses review on NousResearch#73013. 1. Manage Profiles used a hand-rolled delete Dialog next to the shared DeleteProfileDialog in the same folder. That copy missed the active- profile re-home fix (f764b04): deleting the profile the gateway is on stranded it on a dead backend. Switch to the shared dialog, which owns the deleteProfile call and re-homes to default. Drops handleConfirmDelete, the deleting state, and the now-unused Dialog* imports. 2. The name field regressed to a plain Input during the create-dialog dedup, losing live slugging. Level both shared dialogs up to SanitizedInput sanitize={slug} so every entry point gets the behavior Manage Profiles had — the sanitize primitive means callers never validate-then-reject. 3. Nothing rendered ProfilesView, which is how the drift got in. Add a behavior test: create dialog exposes SOUL.md, deleting the active profile re-homes to default, deleting a non-active one does not.
Problem
The desktop app has two "New Profile" entry points that rendered different modals:
The Manage Profiles page (
apps/desktop/src/app/profiles/index.tsx) carried its own localCreateProfileDialogandRenameProfileDialogcopies that predated the shared dialogs increate-profile-dialog.tsx/rename-profile-dialog.tsx. The sidebar rail uses the shared ones; Manage Profiles used the stale local copies. The local create copy never got the SOUL.md textarea when it was added to the shared dialog, so the two surfaces drifted.Fix
Delete both local duplicates and reuse the shared self-contained dialogs (they own the
createProfile/renameProfile/updateProfileSoulcalls). Both entry points now render the identical modal, SOUL.md included. This removes the drift class, not just the one SOUL.md symptom.Also collapsed the two identical
onCreated/onRenamedcallbacks into oneselectAndRefresh, and dropped the imports only the removed dialogs used (SanitizedInput,Select*,createProfile,renameProfile,slug,cn, plus the localPROFILE_NAME_RE/isValidProfileName).Behavior note
The Manage page previously popped a toast on create/rename; it now shows the shared dialog's inline "Created" / "Renamed" button status, matching the sidebar. That alignment is the point of the fix.
Verification
tsc(all three tsconfigs) passesNet: 1 file, +12/−290.