fix(chat session picker): add action bar actions and improve dropdown handling#310064
fix(chat session picker): add action bar actions and improve dropdown handling#310064DonJayamanne wants to merge 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the chat session picker dropdown to support a dedicated action bar section and separates command actions from the main option list.
Changes:
- Adds an
actionBarActionProviderto supply actions shown in the dropdown action bar. - Refactors command actions into a new
getDropdownActionBarActions()method, returning early fromgetDropdownActions().
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/browser/chatSessions/chatSessionPickerActionItem.ts | Adds action bar actions provider and moves command actions out of the main dropdown list |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 3
| // if locked, show the current option only | ||
| const currentOption = this.delegate.getCurrentOption(); | ||
| if (currentOption?.locked) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
The comment says 'if locked, show the current option only' but the implementation returns no action-bar actions. Please update the comment to reflect the actual behavior (e.g., that action-bar actions are hidden/disabled when locked) to avoid misleading future maintenance.
| tooltip: command.tooltip ?? command.title, | ||
| label: command.title, | ||
| run: () => { | ||
| this.commandService.executeCommand(command.command, ...args); |
There was a problem hiding this comment.
Consider returning the executeCommand(...) result from run. This allows callers/UI plumbing to observe completion/failures (and chain/await the promise) instead of always treating the action as synchronous.
| this.commandService.executeCommand(command.command, ...args); | |
| return this.commandService.executeCommand(command.command, ...args); |
| for (const command of (group.commands ?? [])) { | ||
| const args = command.arguments ? [...command.arguments] : []; | ||
| const sessionResource = this.delegate.getSessionResource(); | ||
| if (sessionResource) { | ||
| args.unshift(sessionResource); | ||
| } |
There was a problem hiding this comment.
this.delegate.getSessionResource() is called once per command. If it’s stable for the duration of this method, compute it once before the loop and reuse it to avoid repeated calls (minor, but simplifies the loop and avoids potential extra work).
See below for a potential fix:
const sessionResource = this.delegate.getSessionResource();
// Add commands at the end in a separate section (only if there are options)
for (const command of (group.commands ?? [])) {
const args = command.arguments ? [...command.arguments] : [];
Screenshot ChangesBase: Changed (1)
|
| Before | After |
|---|---|
 |
 |
blocks-ci screenshots changed
Replace the contents of test/componentFixtures/blocks-ci-screenshots.md with:
Updated blocks-ci-screenshots.md
<!-- auto-generated by CI — do not edit manually -->
#### editor/codeEditor/CodeEditor/Dark
#### editor/codeEditor/CodeEditor/Light
Before
No separator before the command
After
