feat: delete sessions from the session picker - #3670
Conversation
🦋 Changeset detectedLatest commit: e3db926 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![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: 45fdd9eb5f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| private async deleteCurrentSessionFromPicker(session: SessionRow): Promise<void> { | ||
| // Tear down before deleting so no events from the dying session reach the | ||
| // UI; on success start fresh — the same destination as createNewSession. | ||
| this.hideSessionPicker(); |
There was a problem hiding this comment.
Calling hideSessionPicker() immediately restores and focuses the editor while the subsequent close, delete, and replacement-session creation are still pending. If the user submits input during that window, ensureSession() starts a lazy creation because closeSession() has already unloaded the current session, while this method later calls createNewSession() independently; the two creations can replace/close each other mid-dispatch, losing the submitted prompt and leaving an extra session. Keep the picker or another blocking state mounted until the replacement session is ready, or serialize this creation with ensureSessionPromise.
Useful? React with 👍 / 👎.
| // Tear down before deleting so no events from the dying session reach the | ||
| // UI; on success start fresh — the same destination as createNewSession. | ||
| this.hideSessionPicker(); | ||
| await this.closeSession('deleting session'); |
There was a problem hiding this comment.
If Session.close() rejects, this await is outside the recovery try, even though closeSession() has already unloaded the session and hideSessionPicker() has restored the editor. The outer handler only displays a deletion error, so deletion is never attempted and the TUI is left detached from the old session until another action happens to create one. Include the close step in the recovery path so a close-RPC failure also reattaches or deliberately starts a fresh session.
Useful? React with 👍 / 👎.
| } | ||
| await this.harness.deleteSession(session.id); | ||
| const requestToken = ++this.sessionPickerScopeRequestToken; | ||
| await this.fetchSessions(this.state.sessionsScope); |
There was a problem hiding this comment.
When deletion succeeds but this refresh fails, fetchSessions() catches the error internally and retains the pre-delete state.sessions, so execution continues to remount the picker and announce success while still showing the deleted row. Selecting or deleting that stale row then produces further errors. Make refresh failure observable here or remove the deleted row locally before remounting.
Useful? React with 👍 / 👎.
45fdd9e to
28050eb
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28050ebddc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (matchesKey(data, Key.ctrl('x'))) { | ||
| const selected = this.list.selected(); | ||
| if (selected !== undefined && this.onDeleteRequest !== undefined) { | ||
| this.deleteState = { session: selected, phase: 'confirm' }; |
There was a problem hiding this comment.
When Enter has already started the asynchronous onSelect flow, the picker remains interactive, so pressing Ctrl+X and confirming can run deletion concurrently with resumeSession/switchToSession. For the current row, the selection flow can call hideSessionPicker() while deletion is still replacing the session; for another row, deletion can close the newly resumed Session while switchToSession() is initializing it, leaving the TUI attached to a closed/deleted session. Fresh evidence beyond the earlier input-lock finding is that handleInput permits this delete path after an in-flight selection; lock the picker when either lifecycle action starts or serialize both operations.
Useful? React with 👍 / 👎.
| const styled = | ||
| state.phase === 'confirm' | ||
| ? currentTheme.boldFg('warning', `Delete session "${label}"? [y/N]`) | ||
| : currentTheme.fg('textMuted', `Deleting session "${label}"…`); | ||
| return truncateToWidth(styled, width, ELLIPSIS); |
There was a problem hiding this comment.
When a generated session title is long or the terminal is narrow, truncating the entire confirmation string removes the trailing ? [y/N], leaving the armed dialog with no visible indication of how to confirm. Reserve space for the fixed prefix and suffix, truncate only label, and then append the confirmation suffix so the destructive-action controls remain discoverable.
AGENTS.md reference: apps/kimi-code/AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
28050eb to
964372f
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 964372f569
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| private async deleteSessionFromPicker(session: SessionRow): Promise<void> { | ||
| try { | ||
| await this.waitForLazyCreation(); |
There was a problem hiding this comment.
If a Ctrl+A scope fetch is already running when the user presses Ctrl+X and confirms, this path leaves its request token valid until after the non-current deletion, and the current-session path does not invalidate it until the picker closes. The scope request can therefore finish and call remountSessionPicker, replacing the locked picker with a fresh interactive instance while close/delete/replacement creation is still running; selecting another session from that instance races createNewSession() and can close or replace the newly selected session. Invalidate the outstanding scope request before the first await, or preserve the lifecycle lock across remounts.
Useful? React with 👍 / 👎.
| this.hideSessionPicker(); | ||
| return; | ||
| } | ||
| await this.createNewSession(); |
There was a problem hiding this comment.
When deleting the current session succeeds but createSessionFromCurrentState() or post-create setup fails, createNewSession() catches the error and returns normally, so this code still hides the picker even though no usable replacement was established. The deleted session ID and transcript remain displayed; if the next prompt succeeds through lazy creation, that path does not clear the old transcript, mixing the deleted session's history with the new session. Make replacement creation report success here or explicitly reset the detached-session UI on failure.
Useful? React with 👍 / 👎.
| ); | ||
| const prefix = state.phase === 'confirm' ? 'Delete session "' : 'Deleting session "'; | ||
| const suffix = state.phase === 'confirm' ? '"? [y/N]' : '"…'; | ||
| const labelBudget = Math.max(1, width - visibleWidth(prefix) - visibleWidth(suffix)); |
There was a problem hiding this comment.
The new label-only truncation still floors labelBudget at one column, so at widths of 24 columns or less the composed confirmation is wider than the terminal and the final whole-line clamp truncates the trailing [y/N]. This is fresh evidence that the previously reported issue remains for narrow terminals despite being fixed for long titles at wider widths; truncate the prefix as necessary so the confirmation suffix is always retained.
AGENTS.md reference: apps/kimi-code/AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
964372f to
8111965
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 811196538b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }); | ||
| await this.switchToSession(resumed, `Resumed session (${resumed.id}).`); | ||
| } catch { | ||
| await this.createNewSession(); |
There was a problem hiding this comment.
If closing or deleting the current session fails, and resuming it also fails, createNewSession() can catch a creation failure and return normally; this branch then hides the picker even though closeSession() already unloaded the session. Because the failure branch retains the old session ID and transcript, the next prompt lazily creates a different session without clearing that transcript, mixing the old history into the new session's UI. Fresh evidence beyond the earlier recovery comments is the new fallback's swallowed failure followed by the unconditional picker close; make creation report whether recovery succeeded and only restore input after a usable session exists.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Ctrl+X on a picker row arms an inline [y/N] confirmation; deleting another session refreshes the list in place, deleting the current session closes it and starts a fresh one, and a failed delete of the current session reattaches to it.
8111965 to
e3db926
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3db92625d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| this.setAppState({ sessionId: '' }); | ||
| this.clearTranscriptAndRedraw(); | ||
| await this.createNewSession(); |
There was a problem hiding this comment.
When deletion or close fails, resumeSession() can succeed but switchToSession() can still reject after setSession(resumed) has attached the resumed object, for example when syncRuntimeState() fails. This catch then clears only the displayed session ID; if createNewSession() also fails and swallows that error, the picker closes with this.session still pointing to the partially initialized session and without its event subscription, so the next prompt is dispatched into an inconsistent session. Fresh evidence beyond the earlier recovery finding is that this catch handles post-attachment switchToSession() failures as well as resume failures; unload the partially attached session before attempting the fallback creation.
Useful? React with 👍 / 👎.
| // a failed refetch must not resurrect it in the remounted list. | ||
| this.state.sessions = this.state.sessions.filter((row) => row.id !== session.id); | ||
| const requestToken = ++this.sessionPickerScopeRequestToken; | ||
| await this.fetchSessions(this.state.sessionsScope); |
There was a problem hiding this comment.
When a user deletes a row before all backend pages have been loaded and this refresh fails, fetchSessions() clears sessionsNextCursor before its try and retains the locally cached rows. The remounted picker consequently sets hasMore to false, making every not-yet-loaded session unreachable until the dialog is reopened and a later refresh succeeds. Fresh evidence beyond the earlier stale-row report is that removing the deleted row locally does not preserve the pagination state; preserve the prior cursor on failure or make the refresh failure observable instead of remounting.
AGENTS.md reference: apps/kimi-code/AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
| const styled = | ||
| state.phase === 'confirm' | ||
| ? currentTheme.boldFg('warning', head + suffix) | ||
| : currentTheme.fg('textMuted', head + suffix); |
There was a problem hiding this comment.
Whenever deletion confirmation is shown, this styles the entire prompt with warning. The normative TUI dialog spec distinguishes destructive action text, which uses the error token, from the [y/N] confirmation keys, which use bold warning; coloring head + suffix together removes that danger-state distinction. Style the destructive head with error and reserve bold warning for the confirmation suffix.
AGENTS.md reference: apps/kimi-code/AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
1 participant
Related Issue
N/A — internal feature request (TUI session deletion). External-PR rule does not apply.
Problem
The TUI offers no way to delete a session:
/sessionsonly lists and resumes, so unwanted sessions accumulate forever and the only remedy is deleting files by hand. The engine already implements hot deletion (close live instance → stop turns → remove session dir → index tombstone, with tests), and the SDK exposes it viaKimiHarness.deleteSession— the TUI is the only missing piece.What changed
Ctrl+Xon a row arms an inline warning-coloredDelete session "title"? [y/N]line;yconfirms,n/Esccancels, every other key is ignored. While the delete runs the picker locks input. The hint line gainsCtrl+X delete.Ctrl+Xwas chosen because printable chars feed the fuzzy search,Ctrl+Dis the exit shortcut, andBackspaceedits the query.KimiTUI): deleting another session deletes it, drops the row locally, then refetches and remounts the list in place (picker stays open,Session deleted.status). Deleting the current session keeps the picker mounted (input locked) while it tears down UI subscriptions, deletes, and starts a fresh session via the existingcreateNewSession()flow — so a submitted prompt can never race the swap. If closing or deleting the current session fails, the TUI reattaches to it (the engine aborts the delete and keeps the session) or falls back to a new session, surfacing the error after the switch.onDeleteRequest(session)and clears its delete state when the returned promise settles — rejections included, so it can never get stuck in the deleting state.The approach mirrors the provider-manager in-list destructive-action pattern (
D+ inline[y/N], already in the TUI design spec) and reuses the engine-tested hot-delete path — no SDK or REST surface changes.Checklist
/approve).gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.