Repository navigation
Delete a profile from the profile picker with Ctrl+D - #270
apoapostolov wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Profile deletion can race with a concurrent opener and remove a live writer’s directory because the owner probe does not maintain exclusive protection through deletion.
3 open issues
What changed in this PR
Adds confirmed profile deletion to OptChat’s TUI profile picker, addressing #269 while preserving non-TUI behavior and keeping Pi session files.
Changes:
- Adds a custom profile picker with Ctrl+D deletion and confirmation.
- Adds profile-folder removal, ownership checks, and saved-pointer cleanup.
- Updates documentation and tests for picker interactions and deletion flows.
| File | Description |
|---|---|
test/support.ts |
Adds helpers for driving the custom profile picker. |
test/profile-delete.test.ts |
Tests deletion, pointer handling, and refusal cases. |
test/profile-busy.test.ts |
Adapts picker tests and covers confirmed and declined deletion. |
test/menu.test.ts |
Tests Ctrl+D, Enter, Escape, and the create row. |
src/profiles.ts |
Adds profile deletion and saved-pointer cleanup. |
src/menu.ts |
Implements the custom picker and deletion shortcut. |
src/index.ts |
Coordinates confirmation, active-profile refusal, and picker reopening. |
README.md |
Documents the shortcut and deletion behavior. |
Review effort: Balanced
|
Thanks for this! The feature is sound and the refusals work. A few things before we can merge:
It also needs a rebase onto main (#278 landed). The only conflict is About the failing test you saw: it passes on clean main and only fails when provider API keys are set in the environment, because If you'd rather not do the rework, let us know and we'll take it over and keep you as author. #267 gets its own small fix after this one. |
0ed2344 to
dff607c
Compare
| try { | ||
| // Renamed to a hidden sibling before the rmSync: a delete that fails on Windows then cannot leave half a profile in the picker (listProfiles() hides dot names). | ||
| const reserved = join(dataHome(), 'profiles', `.deleting-${name}-${Date.now()}`); | ||
| renameSync(dir, reserved); |
| /** The select list and, with it, the j/k movement both menus share: rows styled once, a delete shortcut can pre-empt a key. */ | ||
| function sharedList(items: SelectItem[], theme: Theme, close: (value?: string) => void, redraw = () => {}, | ||
| shortcut?: (data: string, current: string | undefined) => boolean) { | ||
| const list = new SelectList(items, items.length, { |
| rmSync(reserved, { recursive: true, force: true, maxRetries: 3 }); | ||
| // listProfiles() already hides a name with no folder, but without this a profile created again under the same name would inherit the stale pointer. | ||
| const last = join(dataHome(), 'last-profile'); | ||
| try { if (readFileSync(last, 'utf8').trim() === name) unlinkSync(last); } catch {} |
Rebased onto main (jonaslsaa#278); its two picker-fallthrough tests now use the profilePickerAnswers helper. After the review: - Ctrl+D goes through the keybindings argument (keys.matches(data, 'app.session.delete')), so it fires in kitty, Ghostty and WezTerm encodings and follows the user's own bindings, not a raw 0x04. - deleteProfile takes the real lock (lockProfile(dir, 'deleting'), delete in try, release in finally) and drops the isWindows/lstat owner probe. - The folder is renamed to profiles/.deleting-NAME-TIMESTAMP before rmSync, so a failed delete cannot leave half a profile in the picker. - profilePicker and optchatMenu share one list block: same styling and j/k handling instead of a copy. - The three picker tests are one, the helper no longer swallows unexpected openings or a dead try/catch, and the README line is shorter.
…anup order, picker scrolling, shorter README
dff607c to
ca82e92
Compare


Closes #269.
What changed
/optchat profilenow deletes the highlighted profile with Ctrl+D, the keyapp.session.deleteuses in Pi's own session list, and confirms before touching anything.Then:
Three files carry the feature:
deleteProfile()insrc/profiles.ts: refuses a profile the lock cannot release, removes the folder, and clearslast-profilewhen it named that profile (lastProfile()hides a missing name, but a profile created again under the same name would otherwise inherit the stale pointer).profilePicker()insrc/menu.ts: the profile list as a custom component, so a bare key press reaches it.+ Create profileis not a profile, so Ctrl+D there does nothing. It also acceptsj/k, like the/optchatmenu already does.chooseProfile()insrc/index.ts: picks, then loops. A delete gets a confirmation, a refusal fromdeleteProfile()becomes a notification, and the picker reopens either way.Why the picker had to change shape
ctx.ui.selectanswers one question and hands control back, so it cannot see a key that means "delete the row I am on". The picker is nowctx.ui.custom, the same asoptchatMenuand the settings page. Non-TUI hosts keep returningundefinedbefore any dialog is shown, so RPC and SDK sessions behave exactly as before.Two refusals worth calling out
deleteProfile()asks the lock who owns it, so the removal becomesProfile already running: …. Deleting under a live process would leave it writing into a folder that no longer exists.Evidence
npm run checkclean.npm test: 376 tests, 374 pass, 1 fail, 1 skipped. The failure is pre-existing onmain(agents.test.ts, "a task may name its own model and thinking level…"), same onf724d31without this branch.test/profile-delete.test.ts(folder and pointer removal, another-name pointer survives, a held profile refused then deletable after release, missing profile, invalid name) and threetest/menu.test.tscases for the picker's Enter, Ctrl+D and Escape.profile-busy.test.tsflows that drove the oldselectnow drive the picker throughctx.ui.customviaprofilePickerAnswers()intest/support.ts, which renders the component and walks to a row before pressing the key. Two of them cover the delete path end to end: confirmed deletes, declined keeps the profile and reopens the picker.OPTCHAT_HOMEpointed at a throwaway directory: Ctrl+D ondemoshowed the confirmation, confirming deleted the folder and the notification saidDeleted demo.; Ctrl+D onkeepthen Escape leftkeepon disk and returned to the list.Risk
The change is a feature path with no migration: nothing existing changes shape,
config.jsonand the profile layout are untouched, and a user who never presses Ctrl+D sees the same list with one extra hint line. No version bump, on the assumption that releases are cut on this repo.Pairs with #267
Deleting a profile is the easy way to leave a Pi session bound to a folder that no longer exists, which is #267. This PR cannot see those sessions, so the recovery proposed there (resume as plain Pi, warn once, keep
--optchat-profile <name>loud) is the other half of the story. The delete does refuse the two cases it can see: a profile another Pi holds, and the profile the current session serves.