Add ability to fork conversations (from Chat or with /fork) #294773
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new “fork conversation” capability for Chat sessions, accessible via both a context menu action and a /fork slash command.
Changes:
- Adds
/forkslash command that triggers a new fork-conversation workbench action. - Introduces
workbench.action.chat.forkConversationto clone chat session state into a new session. - Minor styling tweak to chat widget toolbar/layout.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/browser/widget/media/chat.css | Adds spacing (gap) for a chat UI element. |
| src/vs/workbench/contrib/chat/browser/chat.contribution.ts | Registers /fork slash command and wires up fork action registration. |
| src/vs/workbench/contrib/chat/browser/actions/chatForkActions.ts | Implements the fork action: cloning session data and opening a new session. |
| setTimeout(async () => { | ||
| try { | ||
| await chatWidgetService.openSession(newSessionResource, ChatViewPaneTarget); | ||
| } finally { | ||
| modelRef.dispose(); | ||
| } | ||
| }, 0); |
There was a problem hiding this comment.
The async callback passed to setTimeout can produce an unhandled promise rejection if openSession throws/rejects (since nothing observes the promise returned by the async function). Wrap the async work so rejections are handled (e.g., schedule a non-async callback and explicitly void the promise with a .catch(...) using the repo’s standard unexpected-error handler).
| async run(accessor: ServicesAccessor, ...args: unknown[]) { | ||
| const chatWidgetService = accessor.get(IChatWidgetService); | ||
| const chatService = accessor.get(IChatService); | ||
| const forkedTitlePrefix = localize('chat.forked.titlePrefix', "Forked: "); |
There was a problem hiding this comment.
Using a localized prefix for startsWith(...) and for determining whether to re-prefix titles is brittle across locale changes (and can lead to double-prefixing or failure to detect already-forked titles in a different language). Prefer a non-localized, stable marker (e.g., a model/session metadata flag), or at least a non-localized internal prefix check while keeping the displayed title localized.
| cleanData.customTitle = chatModel.title.startsWith(forkedTitlePrefix) | ||
| ? chatModel.title | ||
| : localize('chat.forked.title', "Forked: {0}", chatModel.title); |
There was a problem hiding this comment.
Using a localized prefix for startsWith(...) and for determining whether to re-prefix titles is brittle across locale changes (and can lead to double-prefixing or failure to detect already-forked titles in a different language). Prefer a non-localized, stable marker (e.g., a model/session metadata flag), or at least a non-localized internal prefix check while keeping the displayed title localized.
| forkedData.customTitle = chatModel.title.startsWith(forkedTitlePrefix) | ||
| ? chatModel.title | ||
| : localize('chat.forked.title', "Forked: {0}", chatModel.title); |
There was a problem hiding this comment.
Using a localized prefix for startsWith(...) and for determining whether to re-prefix titles is brittle across locale changes (and can lead to double-prefixing or failure to detect already-forked titles in a different language). Prefer a non-localized, stable marker (e.g., a model/session metadata flag), or at least a non-localized internal prefix check while keeping the displayed title localized.
| forkedData.customTitle = chatModel.title.startsWith(forkedTitlePrefix) | |
| ? chatModel.title | |
| : localize('chat.forked.title', "Forked: {0}", chatModel.title); | |
| forkedData.customTitle = localize('chat.forked.title', "Forked: {0}", chatModel.title); |
| targetIndex = isRequestVM(entry) ? Math.max(0, requestIndex - 1) : requestIndex; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| if (targetIndex < 0) { | ||
| const requestIndex = chatModel.getRequests().findIndex(r => r.id === targetRequestId); | ||
| targetIndex = isRequestItem ? Math.max(0, requestIndex - 1) : requestIndex; |
There was a problem hiding this comment.
The action tooltip says “Fork conversation from this point”, but when invoked on a request the logic subtracts 1 and effectively forks before the selected request. Either adjust the tooltip/label to reflect “fork up to before this request” (checkpoint semantics), or change the indexing logic so selecting a request includes that request in the fork.
| targetIndex = isRequestVM(entry) ? Math.max(0, requestIndex - 1) : requestIndex; | |
| break; | |
| } | |
| } | |
| } | |
| if (targetIndex < 0) { | |
| const requestIndex = chatModel.getRequests().findIndex(r => r.id === targetRequestId); | |
| targetIndex = isRequestItem ? Math.max(0, requestIndex - 1) : requestIndex; | |
| targetIndex = requestIndex; | |
| break; | |
| } | |
| } | |
| } | |
| if (targetIndex < 0) { | |
| const requestIndex = chatModel.getRequests().findIndex(r => r.id === targetRequestId); | |
| targetIndex = requestIndex; |
| targetIndex = isRequestVM(entry) ? Math.max(0, requestIndex - 1) : requestIndex; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| if (targetIndex < 0) { | ||
| const requestIndex = chatModel.getRequests().findIndex(r => r.id === targetRequestId); | ||
| targetIndex = isRequestItem ? Math.max(0, requestIndex - 1) : requestIndex; |
There was a problem hiding this comment.
The action tooltip says “Fork conversation from this point”, but when invoked on a request the logic subtracts 1 and effectively forks before the selected request. Either adjust the tooltip/label to reflect “fork up to before this request” (checkpoint semantics), or change the indexing logic so selecting a request includes that request in the fork.
| targetIndex = isRequestVM(entry) ? Math.max(0, requestIndex - 1) : requestIndex; | |
| break; | |
| } | |
| } | |
| } | |
| if (targetIndex < 0) { | |
| const requestIndex = chatModel.getRequests().findIndex(r => r.id === targetRequestId); | |
| targetIndex = isRequestItem ? Math.max(0, requestIndex - 1) : requestIndex; | |
| targetIndex = requestIndex; | |
| break; | |
| } | |
| } | |
| } | |
| if (targetIndex < 0) { | |
| const requestIndex = chatModel.getRequests().findIndex(r => r.id === targetRequestId); | |
| targetIndex = requestIndex; |
| this._store.add(slashCommandService.registerSlashCommand({ | ||
| command: 'fork', | ||
| detail: nls.localize('fork', "Fork conversation into a new chat session"), |
There was a problem hiding this comment.
The localization key 'fork' is very generic and increases the risk of key collisions/ambiguity in NLS files. Use a more specific key (e.g., 'chat.slashCommand.fork.detail') to keep localization identifiers scoped and descriptive.
1 participant
Forking.mov