feat(copilotcli): Perf improvement via resolveChatSessionItem API#312010
feat(copilotcli): Perf improvement via resolveChatSessionItem API#312010DonJayamanne merged 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a lazy “resolve” path for chat session items so expensive details (timing/changes/badge) can be fetched on-demand when an item becomes visible, reducing unnecessary RPC/work for session types like copilotcli.
Changes:
- Adds
resolveChatSessionItemto the proposed chat sessions API (new controller method; legacy provider method is deprecated and bridged). - ExtHost/MainThread plumbing for resolve support, including capability advertisement and a per-item resolve cache to avoid repeated resolves.
- Adds/updates mocks and tests to cover capability toggling, caching, invalidation on updates, and stale in-flight results.
Show a summary per file
| File | Description |
|---|---|
| src/vscode-dts/vscode.proposed.chatSessionsProvider.d.ts | Adds proposed API surface for resolving session items (new controller hook + deprecated provider hook). |
| src/vs/workbench/contrib/chat/common/chatSessionsService.ts | Extends internal controller/service interfaces with resolveChatSessionItem. |
| src/vs/workbench/contrib/chat/browser/chatSessions/chatSessions.contribution.ts | Implements IChatSessionsService.resolveChatSessionItem by delegating to the registered controller. |
| src/vs/workbench/api/common/extHost.protocol.ts | Extends main/ext host RPC shape with supportsResolve registration, capability updates, and $resolveChatSessionItem. |
| src/vs/workbench/api/common/extHostChatSessions.ts | Bridges deprecated provider resolve into controller resolve and adds capability update signaling + $resolveChatSessionItem handler. |
| src/vs/workbench/api/browser/mainThreadChatSessions.ts | Adds supportsResolve gating, per-item resolve caching/invalidation, and capability update handling. |
| src/vs/workbench/api/test/browser/mainThreadChatSessions.test.ts | Adds tests for resolve behavior, caching semantics, invalidation, and capability advertisement. |
| src/vs/workbench/contrib/chat/test/common/mockChatSessionsService.ts | Updates mock to satisfy new resolveChatSessionItem service method. |
| src/vs/workbench/contrib/chat/test/common/chatService/mockChatService.ts | Implements getMetadataForSession for tests that now depend on it during resolve flows. |
Copilot's findings
- Files reviewed: 8/9 changed files
- Comments generated: 3
| resolveChatSessionItem: provider.resolveChatSessionItem | ||
| ? async (item, token) => { | ||
| const resolved = await provider.resolveChatSessionItem!(item, token); | ||
| if (resolved) { |
There was a problem hiding this comment.
When bridging the deprecated provider hook, consider validating that the returned resolved.resource matches item.resource before calling collection.add(resolved). If a provider accidentally returns a different resource, this will add a new item while the originally requested item remains unresolved (and $resolveChatSessionItem will still return the old item), potentially leading to duplicates and hard-to-debug UI state. Logging a warning and ignoring mismatched resources would make this more robust.
| if (resolved) { | |
| if (resolved) { | |
| if (resolved.resource.toString() !== item.resource.toString()) { | |
| this._logService.warn(`ExtHostChatSessions. Ignoring resolved chat session item for ${chatSessionType} because the provider returned a different resource. Requested: ${item.resource.toString()}, resolved: ${resolved.resource.toString()}`); | |
| return; | |
| } |
| $provideChatSessionProviderOptions(providerHandle: number, token: CancellationToken): Promise<IChatSessionProviderOptions | undefined>; | ||
| $provideHandleOptionsChange(providerHandle: number, sessionResource: UriComponents, updates: Record<string, string | IChatSessionProviderOptionItem | undefined>, token: CancellationToken): Promise<void>; | ||
| $forkChatSession(providerHandle: number, sessionResource: UriComponents, request: IChatSessionRequestHistoryItemDto | undefined, token: CancellationToken): Promise<Dto<IChatSessionItem>>; | ||
| $resolveChatSessionItem(providerHandle: number, sessionResource: UriComponents, token: CancellationToken): Promise<Dto<IChatSessionItem> | undefined>; |
There was a problem hiding this comment.
$resolveChatSessionItem uses the parameter name providerHandle, but this value is actually the chat session item controller handle (the same handle passed to $registerChatSessionItemController). Renaming the parameter to controllerHandle here would match the rest of the interface (e.g. $provideChatSessionInputState) and reduce confusion between provider vs controller handles.
| $resolveChatSessionItem(providerHandle: number, sessionResource: UriComponents, token: CancellationToken): Promise<Dto<IChatSessionItem> | undefined>; | |
| $resolveChatSessionItem(controllerHandle: number, sessionResource: UriComponents, token: CancellationToken): Promise<Dto<IChatSessionItem> | undefined>; |
| // Drop any cached `undefined` results so a newly-installed handler can be invoked. | ||
| if (supportsResolve) { | ||
| this._resolveCache.clear(); | ||
| } |
There was a problem hiding this comment.
When supportsResolve is toggled off, _resolveCache is currently left intact. Clearing the cache on both enable and disable would avoid retaining per-resource promises/results unnecessarily (and prevents any chance of stale resolved data being held after capability is removed).
| // Drop any cached `undefined` results so a newly-installed handler can be invoked. | |
| if (supportsResolve) { | |
| this._resolveCache.clear(); | |
| } | |
| // Drop cached resolve results whenever the capability changes to avoid retaining stale data. | |
| this._resolveCache.clear(); |
No description provided.