-
Notifications
You must be signed in to change notification settings - Fork 36.7k
Add chat back button to activity bar #281387
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| /** | ||
| * Context for chat view title actions. | ||
| * Defined locally to avoid layering violations with contrib layer. | ||
| */ | ||
| interface IChatViewTitleActionContext { | ||
| $mid: MarshalledId.ChatViewContext; | ||
| sessionResource: URI; | ||
| } | ||
|
|
||
| /** | ||
| * Internal interface for accessing chat view widget properties. | ||
| * Defined locally because these are internal implementation details not exposed via public interfaces, | ||
| * and importing from contrib layer would violate layering rules. | ||
| */ | ||
| interface IChatViewWithWidget { | ||
| widget?: { | ||
| viewModel?: { | ||
| model?: { | ||
| sessionResource?: URI; | ||
| getRequests?: () => unknown[]; | ||
| }; | ||
| }; | ||
| }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benibenj @bpasero wasn't quite sure what to do here so for the moment I've defined local interfaces.
Was specifically unsure about making some of the private properties for model resources/requests public and/or pulling them out into a separate class/interface, so again just defined an interface locally to make TS happy. Would love input from @roblourens or @justschen on if this feels icky or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not an expert in this area, but I don't think there should be anything chat-specific in AbstractPaneCompositePart. Rather we should figure out how to turn it into a generic feature, which the chat view can enable/configure in some way, or, we figure out how to have a chat-specific IPaneCompositePart which has chat features. We might ask Steven for advice, or maybe Ben understands it well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are 3 approaches
- Have a contributable toolbar to the left of view pane title => I don't think this is a good idea
- When creating a view pane, the title is passed as an option. We could also have a titleAction which is passed in which uses
whencondition for toggling visibility - Change the type of view pane
options.titleandupdateTitle()to bestring | { action: Action; label: string }instead of juststring
Not sure yet which is better. I'm leaning towards 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a "Go Back" button to the Chat view's activity bar when the activity bar is positioned at the Top, Bottom, or Hidden (not in the default sidebar location). The implementation adds a new MenuWorkbenchToolbar to the left of activity bar titles in the AbstractPaneCompositePart class, which displays when viewing the Chat panel in these non-default positions.
Key changes:
- Adds chat-specific toolbar infrastructure to the base pane composite architecture layer
- Makes
IConfigurationServiceaccessible as a protected property in child classes (SidebarPart, PanelPart, AuxiliaryBarPart) - Implements logic to show/hide the toolbar based on activity bar position and chat session state
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/vs/workbench/browser/parts/paneCompositePart.ts |
Adds chat title toolbar infrastructure, including interfaces for chat context, toolbar creation/management methods, and visibility logic |
src/vs/workbench/browser/parts/media/paneCompositePart.css |
Reformats existing CSS selectors to multi-line style and adds new styles for the chat toolbar container |
src/vs/workbench/browser/parts/sidebar/sidebarPart.ts |
Updates constructor to pass IConfigurationService to parent class and access it via this.configurationService |
src/vs/workbench/browser/parts/panel/panelPart.ts |
Updates constructor to pass IConfigurationService to parent class |
src/vs/workbench/browser/parts/auxiliarybar/auxiliaryBarPart.ts |
Updates constructor to pass IConfigurationService to parent class and access it via this.configurationService |
| } | ||
|
|
||
| /* Reduce title label padding when chat toolbar (i.e. chat back button) is present */ | ||
| .monaco-workbench .pane-composite-part > .title .title-label:has(.has-chat-toolbar) { |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CSS selector .title-label:has(.has-chat-toolbar) is logically incorrect. The :has() pseudo-class checks if the element contains a descendant matching the selector, but has-chat-toolbar is a class that should be applied directly to .title-label itself (as seen in line 605 of paneCompositePart.ts: this.titleLabelElement?.classList.toggle('has-chat-toolbar', visible)).
The correct selector should be:
.monaco-workbench .pane-composite-part > .title > .title-label.has-chat-toolbar {
padding-left: 4px !important;
}This will prevent the style from being applied as intended.
| .monaco-workbench .pane-composite-part > .title .title-label:has(.has-chat-toolbar) { | |
| .monaco-workbench .pane-composite-part > .title > .title-label.has-chat-toolbar { |
| protected override onTitleAreaUpdate(compositeId: string): void { | ||
| super.onTitleAreaUpdate(compositeId); | ||
|
|
||
| // Update chat title toolbar visibility and context | ||
| this.updateChatTitleToolbar(compositeId); | ||
|
|
||
| // If title actions change, relayout the composite bar | ||
| this.layoutCompositeBar(); | ||
| } |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chat toolbar visibility might not update reactively when the chat session changes. The updateChatTitleToolbar method is only called from onTitleAreaUpdate, which is triggered when the composite's title area changes. However, changes to the chat session (e.g., when messages are added/removed or when the session resource changes) may not trigger onTitleAreaUpdate.
This could result in the back button appearing or disappearing at incorrect times. For example:
- The back button might not appear when the first message is added to a chat session
- The back button might not disappear when all messages are cleared
Consider adding listeners to chat session changes to ensure the toolbar visibility stays in sync with the actual chat state, or verify that the chat view properly triggers onTitleAreaUpdate when its session changes.
| /** | ||
| * Context for chat view title actions. | ||
| * Defined locally to avoid layering violations with contrib layer. | ||
| */ | ||
| interface IChatViewTitleActionContext { | ||
| $mid: MarshalledId.ChatViewContext; | ||
| sessionResource: URI; | ||
| } | ||
|
|
||
| /** | ||
| * Internal interface for accessing chat view widget properties. | ||
| * Defined locally because these are internal implementation details not exposed via public interfaces, | ||
| * and importing from contrib layer would violate layering rules. | ||
| */ | ||
| interface IChatViewWithWidget { | ||
| widget?: { | ||
| viewModel?: { | ||
| model?: { | ||
| sessionResource?: URI; | ||
| getRequests?: () => unknown[]; | ||
| }; | ||
| }; | ||
| }; | ||
| } |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IChatViewTitleActionContext and IChatViewWithWidget interfaces are duplicated definitions. The IChatViewTitleActionContext interface already exists in src/vs/workbench/contrib/chat/common/chatActions.ts. While duplicating this interface avoids a layering violation (since workbench/browser/parts should not depend on workbench/contrib), this approach creates maintenance risks as the two definitions can drift out of sync.
Consider documenting more clearly that these are intentional duplicates that must be kept in sync with the canonical definitions in the contrib layer, or explore alternative approaches such as:
- Moving the shared interface to a common location accessible to both layers
- Using runtime validation to ensure structural compatibility
| private static readonly CHAT_VIEW_CONTAINER_ID = 'workbench.panel.chat'; | ||
| private static readonly CHAT_VIEW_ID = 'workbench.panel.chat.view.copilot'; | ||
|
|
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded view container and view IDs create tight coupling between the base architecture layer and specific feature implementations in the contrib layer. This violates the layered architecture principle where workbench/browser/parts should be feature-agnostic.
The current implementation makes the core pane composite infrastructure chat-aware, which:
- Breaks architectural boundaries
- Makes it difficult to add similar functionality for other features
- Reduces code reusability
Consider refactoring this to use a more generic extension point mechanism, such as:
- A registry where features can register custom toolbar providers
- A generic callback/hook system for parts to customize their toolbar based on active content
- Making this functionality configurable through contribution points rather than hardcoded
| private static readonly CHAT_VIEW_CONTAINER_ID = 'workbench.panel.chat'; | |
| private static readonly CHAT_VIEW_ID = 'workbench.panel.chat.view.copilot'; | |
| /** | |
| * Registry for pane composite toolbar providers. | |
| * Features can register a toolbar provider for a specific view container or view. | |
| */ | |
| private static readonly toolbarProviderRegistry: Map<string, (composite: PaneComposite) => MenuWorkbenchToolBar | undefined> = new Map(); | |
| /** | |
| * Register a toolbar provider for a specific view container or view. | |
| * @param id The view container or view ID. | |
| * @param provider The provider function. | |
| */ | |
| public static registerToolbarProvider(id: string, provider: (composite: PaneComposite) => MenuWorkbenchToolBar | undefined): void { | |
| AbstractPaneCompositePart.toolbarProviderRegistry.set(id, provider); | |
| } | |
| /** | |
| * Get a toolbar provider for a specific view container or view. | |
| * @param id The view container or view ID. | |
| */ | |
| protected getToolbarProvider(id: string): ((composite: PaneComposite) => MenuWorkbenchToolBar | undefined) | undefined { | |
| return AbstractPaneCompositePart.toolbarProviderRegistry.get(id); | |
| } |
| return undefined; | ||
| } | ||
|
|
||
| const chatViewWithWidget = chatView as unknown as IChatViewWithWidget; |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unsafe type assertion as unknown as IChatViewWithWidget bypasses TypeScript's type safety. This cast assumes internal implementation details of the chat view that are not part of any public contract. If the internal structure of the chat view changes (e.g., property names, nesting levels), this code will silently fail at runtime with undefined access errors.
Consider one of these safer approaches:
- Use optional chaining throughout and handle the undefined case explicitly
- Add runtime type guards to validate the structure before accessing properties
- Define a proper interface that chat views can implement and use
instanceofor a type guard function
|
As this is for January release, I will wait until the November/December agents work is done before reviewing as code and ideas might slightly change due to that work. |
Resolves #281068
Implements support for a back button in the Chat view panel activity bar when in Top, Bottom, and Hidden positions. Specifically adds a
MenuWorkBenchToolbarto the left of activity bar titles in theAbstractPaneCompositePartclass.Position Top:
Position Bottom/Hidden: