refactor: remove progress service dependency and optimize chat session forking#306207
refactor: remove progress service dependency and optimize chat session forking#306207DonJayamanne wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the chat “Fork Conversation” action to remove the progress service dependency and reduce duplicate contributed-session forks by deduplicating in-flight fork operations.
Changes:
- Removed
IProgressServicedependency from chat fork action handling. - Added in-flight fork deduplication (
pendingForkmap) for contributed chat sessions. - Routed contributed-session forking through an action-instance helper to reuse/dedupe fork execution.
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/chat/browser/actions/chatForkActions.ts:253
- In
forkContributedChatSession(...), the deferred navigation path usessetTimeout(async () => { await chatWidgetService.openSession(...) })without any error handling; ifopenSessionrejects this can surface as an unhandled promise rejection. Consider wrapping the callback in try/catch (and report/log the error), or returning/awaiting a promise that captures failures.
async function forkContributedChatSession(sourceSessionResource: URI, request: IChatSessionRequestHistoryItem | undefined, openForkedSessionImmediately: boolean, chatSessionsService: IChatSessionsService, chatWidgetService: IChatWidgetService) {
const cts = new CancellationTokenSource();
try {
const forkedItem = await chatSessionsService.forkChatSession(sourceSessionResource, request, cts.token);
| private async forkContributedChatSession(sourceSessionResource: URI, request: IChatSessionRequestHistoryItem | undefined, openForkedSessionImmediately: boolean, chatSessionsService: IChatSessionsService, chatWidgetService: IChatWidgetService) { | ||
| const pendingKey = `${sourceSessionResource.toString()}@${request?.id ?? 'full'}`; | ||
| const pending = this.pendingFork.get(pendingKey); | ||
| if (pending) { | ||
| return pending; | ||
| } | ||
|
|
||
| const forkPromise = forkContributedChatSession(sourceSessionResource, request, openForkedSessionImmediately, chatSessionsService, chatWidgetService); | ||
| this.pendingFork.set(pendingKey, forkPromise); | ||
| try { | ||
| await forkPromise; | ||
| } finally { | ||
| this.pendingFork.delete(pendingKey); | ||
| } | ||
| } |
There was a problem hiding this comment.
pendingFork is cleared once forkContributedChatSession(...) resolves, but when openForkedSessionImmediately is false the helper schedules openSession via setTimeout without awaiting it. This means the dedupe window ends before navigation happens, so a user can still trigger another /fork and create another session before the first forked session opens. Consider keeping the pending entry until the deferred openSession completes (e.g., have the helper return a promise that includes the deferred open, or delay deletion until after the scheduled open finishes).
This issue also appears on line 250 of the same file.
No description provided.