Conversation
- remove explorer - bring in output - add keyboard shortcut for sessions Co-authored-by: Copilot <copilot@github.com>
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @benibenjMatched files:
|
There was a problem hiding this comment.
Pull request overview
Updates view/container registration to better tailor the Sessions window’s View menu and available panels by introducing a window “enablement” concept and applying it across key contributions.
Changes:
- Replace
WindowVisibility/windowVisibilitywithWindowEnablement/windowEnablementacross views and containers. - Filter
ViewDescriptorService.viewContainers(and related lookups) to only expose containers enabled for the current window type (Editor vs Sessions). - Simplify Sessions window contributions by removing the custom Sessions logs container, enabling Output in Sessions, and adding a Sessions keyboard shortcut.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/services/views/browser/viewDescriptorService.ts | Filters exposed containers by window enablement and updates lookups/visibility gating accordingly. |
| src/vs/workbench/contrib/terminal/browser/terminal.contribution.ts | Marks Terminal container/view as enabled in both Editor and Sessions windows. |
| src/vs/workbench/contrib/output/browser/output.contribution.ts | Enables Output container/view in both Editor and Sessions windows. |
| src/vs/workbench/common/views.ts | Introduces WindowEnablement and renames descriptor properties to windowEnablement. |
| src/vs/sessions/sessions.common.main.ts | Removes Sessions-specific logs contribution import. |
| src/vs/sessions/contrib/sessions/browser/sessions.contribution.ts | Uses windowEnablement and adds a keybinding for the Sessions container. |
| src/vs/sessions/contrib/logs/browser/logs.contribution.ts | Removes Sessions-specific logs container implementation. |
| src/vs/sessions/contrib/files/browser/files.contribution.ts | Migrates view/container registration to windowEnablement. |
| src/vs/sessions/contrib/chatDebug/browser/chatDebug.contribution.ts | Migrates view/container registration to windowEnablement. |
| src/vs/sessions/contrib/chat/browser/chat.contribution.ts | Migrates view/container registration to windowEnablement. |
| src/vs/sessions/contrib/changes/browser/changes.contribution.ts | Migrates view/container registration to windowEnablement. |
Copilot's findings
Comments suppressed due to low confidence (2)
src/vs/workbench/common/views.ts:78
- The JSDoc for
WindowEnablementand its members still describes “visible” semantics. Since the enum/property were renamed to enablement, consider updating these comments to consistently use “enabled” terminology to avoid confusion about whether this controls registration/availability vs. view visibility state.
This issue also appears on line 329 of the same file.
/**
* Specifies in which window a view or view container should be visible.
*/
export const enum WindowEnablement {
/**
* Visible only in the editor window
*/
Editor = 1,
/**
* Visible only in agent sessions window
*/
Sessions = 2,
/**
* Visible in both editor and agent sessions windows
*/
Both = 3,
src/vs/workbench/common/views.ts:333
- This JSDoc refers to
WindowEnablement.Workbench, butWindowEnablementonly definesEditor,Sessions, andBoth. Please update the default description to the correct enum value (likelyEditor) and align the wording with the new enablement concept.
/**
* Specifies in which window this view should be visible.
* Defaults to WindowEnablement.Workbench (main workbench only).
*/
readonly windowEnablement?: WindowEnablement;
- Files reviewed: 11/11 changed files
- Comments generated: 3
| @@ -286,14 +292,14 @@ export class ViewDescriptorService extends Disposable implements IViewDescriptor | |||
| getViewContainerByViewId(viewId: string): ViewContainer | null { | |||
| // Check if the view itself should be visible in current workspace | |||
There was a problem hiding this comment.
The comment says this checks whether the view should be “visible in current workspace”, but the new logic is checking windowEnablement (window-specific availability). Consider updating the comment to reflect the window enablement filtering being applied here.
| // Check if the view itself should be visible in current workspace | |
| // Check if the view is enabled in the current window/context before resolving its container |
| return null; | ||
| } | ||
| return viewContainer; | ||
| return this.viewContainers.find(vc => vc.id === id) ?? null; |
There was a problem hiding this comment.
getViewContainerById currently calls this.viewContainers (which filters viewContainersRegistry.all) and then does a linear search. This causes a full filter allocation on every lookup. Consider using viewContainersRegistry.get(id) followed by an enablement check (or caching the enabled containers list) to avoid repeated full scans/allocations.
| return this.viewContainers.find(vc => vc.id === id) ?? null; | |
| const viewContainer = this.viewContainersRegistry.get(id); | |
| return viewContainer && this.isViewContainerEnabled(viewContainer) ? viewContainer : null; |
| private readonly _onDidChangeViewContainers = this._register(new Emitter<{ added: ReadonlyArray<{ container: ViewContainer; location: ViewContainerLocation }>; removed: ReadonlyArray<{ container: ViewContainer; location: ViewContainerLocation }> }>()); | ||
| readonly onDidChangeViewContainers = this._onDidChangeViewContainers.event; | ||
| get viewContainers(): ReadonlyArray<ViewContainer> { return this.viewContainersRegistry.all; } | ||
| get viewContainers(): ReadonlyArray<ViewContainer> { return this.viewContainersRegistry.all.filter(vc => this.isViewContainerEnabled(vc)); } |
There was a problem hiding this comment.
The new window enablement filtering (e.g., viewContainers filtering by windowEnablement) is behaviorally important for Sessions vs Editor windows, but there don’t appear to be unit tests covering the inclusion/exclusion rules. Please add a focused test in the existing viewDescriptorService.test.ts suite that registers containers/views with different windowEnablement values and asserts they’re filtered correctly when isSessionsWindow is toggled.
Co-authored-by: Copilot copilot@github.com