From a27b3d45a78723c7654311438d45b9c4d5fe2fdf Mon Sep 17 00:00:00 2001 From: kno Date: Thu, 12 Mar 2026 07:21:05 +0100 Subject: [PATCH 1/4] fix: persist SCM repository visibility state across restarts SCM repository visibility was not properly restored on restart due to two issues in the onDidAddRepository restoration logic: 1. Repositories with isHidden (e.g., Copilot worktrees) were not in the saved state but triggered the index === -1 path, which made ALL repos visible and reset the didSelectRepository flag. 2. Genuinely new repos (not in previous state) also triggered the same path, making all existing repos visible instead of only adding the new repo. The fix: - Skip the restoration logic for isHidden repos (they are still tracked internally but don't affect visibility restoration) - New repos are added as visible without changing existing repos' visibility state --- .../contrib/scm/browser/scmViewService.ts | 36 +-- .../scm/test/browser/scmViewService.test.ts | 219 ++++++++++++++++++ 2 files changed, 237 insertions(+), 18 deletions(-) create mode 100644 src/vs/workbench/contrib/scm/test/browser/scmViewService.test.ts diff --git a/src/vs/workbench/contrib/scm/browser/scmViewService.ts b/src/vs/workbench/contrib/scm/browser/scmViewService.ts index d7b3583901c7c8..73c0207365f9ef 100644 --- a/src/vs/workbench/contrib/scm/browser/scmViewService.ts +++ b/src/vs/workbench/contrib/scm/browser/scmViewService.ts @@ -359,30 +359,30 @@ export class SCMViewService implements ISCMViewService { let removed: Iterable = Iterable.empty(); if (this.previousState && !this.didFinishLoadingRepositories.get()) { + // Hidden repositories are not part of the saved state, so skip + // the restoration logic for them. They are still added to the + // internal list but should not affect the visibility restoration. + if (repository.provider.isHidden) { + this.insertRepositoryView(this._repositories, repositoryView); + this._onDidChangeRepositories.fire({ added: Iterable.empty(), removed: Iterable.empty() }); + return; + } + const index = this.previousState.all.indexOf(getProviderStorageKey(repository.provider)); if (index === -1) { - // This repository is not part of the previous state which means that it - // was either manually closed in the previous session, or the repository - // was added after the previous session. In this case, we should select - // all of the repositories. - const added: ISCMRepository[] = []; - - this.insertRepositoryView(this._repositories, repositoryView); - + // This repository is not part of the previous state which means + // it was added after the previous session. Add it as visible + // without changing the visibility of existing repositories. if (this.selectionModeConfig.get() === ISCMRepositorySelectionMode.Multiple || !this._repositories.find(r => r.selectionIndex !== -1)) { - // Multiple selection mode or single selection mode (select first repository) - this._repositories.forEach((repositoryView, index) => { - if (repositoryView.selectionIndex === -1) { - added.push(repositoryView.repository); - } - repositoryView.selectionIndex = index; - }); - - this._onDidChangeRepositories.fire({ added, removed: Iterable.empty() }); + const maxSelectionIndex = this.getMaxSelectionIndex(); + this.insertRepositoryView(this._repositories, { ...repositoryView, selectionIndex: maxSelectionIndex + 1 }); + this._onDidChangeRepositories.fire({ added: [repositoryView.repository], removed: Iterable.empty() }); + } else { + this.insertRepositoryView(this._repositories, repositoryView); + this._onDidChangeRepositories.fire({ added: Iterable.empty(), removed: Iterable.empty() }); } - this.didSelectRepository = false; return; } diff --git a/src/vs/workbench/contrib/scm/test/browser/scmViewService.test.ts b/src/vs/workbench/contrib/scm/test/browser/scmViewService.test.ts new file mode 100644 index 00000000000000..40b3bb4050ef4f --- /dev/null +++ b/src/vs/workbench/contrib/scm/test/browser/scmViewService.test.ts @@ -0,0 +1,219 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import * as assert from 'assert'; +import { DisposableStore } from '../../../../../base/common/lifecycle.js'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; +import { IConfigurationService } from '../../../../../platform/configuration/common/configuration.js'; +import { TestConfigurationService } from '../../../../../platform/configuration/test/common/testConfigurationService.js'; +import { IContextKeyService } from '../../../../../platform/contextkey/common/contextkey.js'; +import { MockContextKeyService } from '../../../../../platform/keybinding/test/common/mockKeybindingService.js'; +import { IStorageService, StorageScope, StorageTarget } from '../../../../../platform/storage/common/storage.js'; +import { ISCMProvider, ISCMService } from '../../common/scm.js'; +import { SCMService } from '../../common/scmService.js'; +import { ISCMViewServiceState, SCMViewService } from '../../browser/scmViewService.js'; +import { workbenchInstantiationService } from '../../../../test/browser/workbenchTestServices.js'; +import { TestStorageService } from '../../../../test/common/workbenchTestServices.js'; +import { Event } from '../../../../../base/common/event.js'; +import { URI } from '../../../../../base/common/uri.js'; +import { ITextModel } from '../../../../../editor/common/model.js'; +import { observableValue } from '../../../../../base/common/observable.js'; +import { Command } from '../../../../../editor/common/languages.js'; + +function createMockProvider(id: string, rootUri: URI, options?: { isHidden?: boolean }): ISCMProvider { + return { + id, + providerId: 'git', + label: 'Git', + name: 'Git', + rootUri, + groups: [], + onDidChangeResourceGroups: Event.None, + onDidChangeResources: Event.None, + isHidden: options?.isHidden, + inputBoxTextModel: {} as ITextModel, + contextValue: observableValue('contextValue', undefined), + count: observableValue('count', undefined), + commitTemplate: observableValue('commitTemplate', ''), + artifactProvider: observableValue('artifactProvider', undefined), + historyProvider: observableValue('historyProvider', undefined), + acceptInputCommand: undefined, + actionButton: observableValue('actionButton', undefined), + statusBarCommands: observableValue('statusBarCommands', undefined), + getOriginalResource: () => Promise.resolve(null), + dispose: () => { }, + } as ISCMProvider; +} + +function getProviderStorageKey(provider: ISCMProvider): string { + return `${provider.providerId}:${provider.label}${provider.rootUri ? `:${provider.rootUri.toString()}` : ''}`; +} + +suite('SCMViewService - Visibility Persistence', () => { + + const disposables = new DisposableStore(); + + function createViewServiceWithState(previousState?: ISCMViewServiceState): { viewService: SCMViewService; scmService: SCMService; storageService: TestStorageService } { + const instantiationService = disposables.add(workbenchInstantiationService(undefined, disposables)); + const storageService = disposables.add(new TestStorageService()); + + if (previousState) { + storageService.store('scm:view:visibleRepositories', JSON.stringify(previousState), StorageScope.WORKSPACE, StorageTarget.MACHINE); + } + + instantiationService.stub(IStorageService, storageService); + + const configurationService = new TestConfigurationService({ + scm: { + repositories: { + selectionMode: 'multiple', + sortOrder: 'discovery time', + explorer: false, + } + } + }); + instantiationService.stub(IConfigurationService, configurationService); + + const contextKeyService = disposables.add(new MockContextKeyService()); + instantiationService.stub(IContextKeyService, contextKeyService); + + const scmService = instantiationService.createInstance(SCMService); + instantiationService.stub(ISCMService, scmService); + + const viewService = disposables.add(instantiationService.createInstance(SCMViewService)); + + return { viewService, scmService, storageService }; + } + + teardown(() => disposables.clear()); + ensureNoDisposablesAreLeakedInTestSuite(); + + test('isHidden repos do not break visibility restoration', () => { + // Set up previous state: 3 repos, repo2 is hidden + const provider1 = createMockProvider('1', URI.file('/repo1')); + const provider2 = createMockProvider('2', URI.file('/repo2')); + const provider3 = createMockProvider('3', URI.file('/repo3')); + + const previousState: ISCMViewServiceState = { + all: [ + getProviderStorageKey(provider1), + getProviderStorageKey(provider2), + getProviderStorageKey(provider3) + ], + visible: [0, 2], // repo1 and repo3 are visible, repo2 is hidden + sortKey: 'discoveryTime' as any + }; + + const { viewService, scmService } = createViewServiceWithState(previousState); + + // Register an isHidden repo FIRST (before the known repos) + disposables.add(scmService.registerSCMProvider( + createMockProvider('hidden-1', URI.file('/hidden-repo'), { isHidden: true }) + )); + + // Then register the known repos + disposables.add(scmService.registerSCMProvider(provider1)); + disposables.add(scmService.registerSCMProvider(provider2)); + disposables.add(scmService.registerSCMProvider(provider3)); + + // repo2 should still be hidden, the isHidden repo should not + // have caused all repos to become visible + assert.deepStrictEqual( + viewService.visibleRepositories.map(r => r.provider.rootUri?.toString()), + [provider1.rootUri!.toString(), provider3.rootUri!.toString()] + ); + }); + + test('new repo does not reset visibility of existing repos', () => { + // Set up previous state: 3 repos, repo2 is hidden + const provider1 = createMockProvider('1', URI.file('/repo1')); + const provider2 = createMockProvider('2', URI.file('/repo2')); + const provider3 = createMockProvider('3', URI.file('/repo3')); + + const previousState: ISCMViewServiceState = { + all: [ + getProviderStorageKey(provider1), + getProviderStorageKey(provider2), + getProviderStorageKey(provider3) + ], + visible: [0, 2], // repo1 and repo3 are visible, repo2 is hidden + sortKey: 'discoveryTime' as any + }; + + const { viewService, scmService } = createViewServiceWithState(previousState); + + // Register the known repos + disposables.add(scmService.registerSCMProvider(provider1)); + disposables.add(scmService.registerSCMProvider(provider2)); + disposables.add(scmService.registerSCMProvider(provider3)); + + // Register a NEW repo that wasn't in the previous state + const provider4 = createMockProvider('4', URI.file('/repo4')); + disposables.add(scmService.registerSCMProvider(provider4)); + + // repo2 should still be hidden, repo4 should be visible + const visibleUris = viewService.visibleRepositories.map(r => r.provider.rootUri?.toString()); + assert.ok(visibleUris.includes(provider1.rootUri!.toString()), 'repo1 should be visible'); + assert.ok(!visibleUris.includes(provider2.rootUri!.toString()), 'repo2 should be hidden'); + assert.ok(visibleUris.includes(provider3.rootUri!.toString()), 'repo3 should be visible'); + assert.ok(visibleUris.includes(provider4.rootUri!.toString()), 'repo4 (new) should be visible'); + }); + + test('visibility state is correctly restored when repos arrive in order', () => { + const provider1 = createMockProvider('1', URI.file('/repo1')); + const provider2 = createMockProvider('2', URI.file('/repo2')); + const provider3 = createMockProvider('3', URI.file('/repo3')); + + const previousState: ISCMViewServiceState = { + all: [ + getProviderStorageKey(provider1), + getProviderStorageKey(provider2), + getProviderStorageKey(provider3) + ], + visible: [0, 2], + sortKey: 'discoveryTime' as any + }; + + const { viewService, scmService } = createViewServiceWithState(previousState); + + disposables.add(scmService.registerSCMProvider(provider1)); + disposables.add(scmService.registerSCMProvider(provider2)); + disposables.add(scmService.registerSCMProvider(provider3)); + + assert.deepStrictEqual( + viewService.visibleRepositories.map(r => r.provider.rootUri?.toString()), + [provider1.rootUri!.toString(), provider3.rootUri!.toString()] + ); + }); + + test('visibility state is correctly restored when hidden repo arrives first', () => { + const provider1 = createMockProvider('1', URI.file('/repo1')); + const provider2 = createMockProvider('2', URI.file('/repo2')); + const provider3 = createMockProvider('3', URI.file('/repo3')); + + const previousState: ISCMViewServiceState = { + all: [ + getProviderStorageKey(provider1), + getProviderStorageKey(provider2), + getProviderStorageKey(provider3) + ], + visible: [0, 2], + sortKey: 'discoveryTime' as any + }; + + const { viewService, scmService } = createViewServiceWithState(previousState); + + // Register the hidden repo first + disposables.add(scmService.registerSCMProvider(provider2)); + disposables.add(scmService.registerSCMProvider(provider1)); + disposables.add(scmService.registerSCMProvider(provider3)); + + // repo2 should still be hidden regardless of registration order + const visibleUris = viewService.visibleRepositories.map(r => r.provider.rootUri?.toString()); + assert.ok(visibleUris.includes(provider1.rootUri!.toString()), 'repo1 should be visible'); + assert.ok(!visibleUris.includes(provider2.rootUri!.toString()), 'repo2 should be hidden'); + assert.ok(visibleUris.includes(provider3.rootUri!.toString()), 'repo3 should be visible'); + }); +}); From 445313c40ca359e5fd42b099804e63292d18df54 Mon Sep 17 00:00:00 2001 From: kno Date: Mon, 4 May 2026 12:20:22 +0200 Subject: [PATCH 2/4] fix: address PR review comments - Replace `as any` casts with ISCMRepositorySortKey.DiscoveryTime in tests - Fix misleading comment about visibility in single-select mode - Fix bug where new repo registered before known repos loses visibility during state restoration (preserve new repos in the first-visible reset) - Add test for new-repo-before-known-repos registration order - Fix SCMMenus.dispose() to properly clean up titleMenu and menus map - Fix SCMService to dispose inputHistory and emitters - Fix test disposal ordering and Event.debounce subscription init --- src/vs/workbench/contrib/scm/browser/menus.ts | 5 ++ .../contrib/scm/browser/scmViewService.ts | 14 ++-- .../contrib/scm/common/scmService.ts | 6 ++ .../scm/test/browser/scmViewService.test.ts | 65 ++++++++++++++++--- 4 files changed, 76 insertions(+), 14 deletions(-) diff --git a/src/vs/workbench/contrib/scm/browser/menus.ts b/src/vs/workbench/contrib/scm/browser/menus.ts index d783fd55814449..c91be5cc413d7c 100644 --- a/src/vs/workbench/contrib/scm/browser/menus.ts +++ b/src/vs/workbench/contrib/scm/browser/menus.ts @@ -408,6 +408,11 @@ export class SCMMenus implements ISCMMenus, IDisposable { } dispose(): void { + this.titleMenu.dispose(); + for (const [, item] of this.menus) { + item.dispose(); + } + this.menus.clear(); this.disposables.dispose(); } } diff --git a/src/vs/workbench/contrib/scm/browser/scmViewService.ts b/src/vs/workbench/contrib/scm/browser/scmViewService.ts index 73c0207365f9ef..6443c8c8e5d292 100644 --- a/src/vs/workbench/contrib/scm/browser/scmViewService.ts +++ b/src/vs/workbench/contrib/scm/browser/scmViewService.ts @@ -242,7 +242,7 @@ export class SCMViewService implements ISCMViewService { @IStorageService private readonly storageService: IStorageService, @IWorkspaceContextService private readonly workspaceContextService: IWorkspaceContextService ) { - this.menus = instantiationService.createInstance(SCMMenus); + this.menus = this.disposables.add(instantiationService.createInstance(SCMMenus)); const explorerEnabledConfig = observableConfigValue('scm.repositories.explorer', false, this.configurationService); this.graphShowIncomingChangesConfig = observableConfigValue('scm.graph.showIncomingChanges', true, this.configurationService); @@ -372,8 +372,9 @@ export class SCMViewService implements ISCMViewService { if (index === -1) { // This repository is not part of the previous state which means - // it was added after the previous session. Add it as visible - // without changing the visibility of existing repositories. + // it was added after the previous session. In multi-select mode + // (or if no repository is selected yet), add it as visible. In + // single-select mode with a selection, add it but not visible. if (this.selectionModeConfig.get() === ISCMRepositorySelectionMode.Multiple || !this._repositories.find(r => r.selectionIndex !== -1)) { const maxSelectionIndex = this.getMaxSelectionIndex(); this.insertRepositoryView(this._repositories, { ...repositoryView, selectionIndex: maxSelectionIndex + 1 }); @@ -398,8 +399,11 @@ export class SCMViewService implements ISCMViewService { if (!this.didSelectRepository) { removed = [...this.visibleRepositories]; this._repositories.forEach(r => { - r.focused = false; - r.selectionIndex = -1; + // Preserve visibility of repos not in previousState (new repos) + if (this.previousState!.all.indexOf(getProviderStorageKey(r.repository.provider)) !== -1) { + r.focused = false; + r.selectionIndex = -1; + } }); this.didSelectRepository = true; diff --git a/src/vs/workbench/contrib/scm/common/scmService.ts b/src/vs/workbench/contrib/scm/common/scmService.ts index 5bdf2e5f9aa70a..7dfb3b6d132055 100644 --- a/src/vs/workbench/contrib/scm/common/scmService.ts +++ b/src/vs/workbench/contrib/scm/common/scmService.ts @@ -423,6 +423,12 @@ export class SCMService implements ISCMService { return repository; } + dispose(): void { + this.inputHistory.dispose(); + this._onDidAddProvider.dispose(); + this._onDidRemoveProvider.dispose(); + } + getRepository(id: string): ISCMRepository | undefined; getRepository(resource: URI): ISCMRepository | undefined; getRepository(idOrResource: string | URI): ISCMRepository | undefined { diff --git a/src/vs/workbench/contrib/scm/test/browser/scmViewService.test.ts b/src/vs/workbench/contrib/scm/test/browser/scmViewService.test.ts index 40b3bb4050ef4f..3fff1f2708bdeb 100644 --- a/src/vs/workbench/contrib/scm/test/browser/scmViewService.test.ts +++ b/src/vs/workbench/contrib/scm/test/browser/scmViewService.test.ts @@ -11,7 +11,7 @@ import { TestConfigurationService } from '../../../../../platform/configuration/ import { IContextKeyService } from '../../../../../platform/contextkey/common/contextkey.js'; import { MockContextKeyService } from '../../../../../platform/keybinding/test/common/mockKeybindingService.js'; import { IStorageService, StorageScope, StorageTarget } from '../../../../../platform/storage/common/storage.js'; -import { ISCMProvider, ISCMService } from '../../common/scm.js'; +import { ISCMProvider, ISCMRepositorySortKey, ISCMService } from '../../common/scm.js'; import { SCMService } from '../../common/scmService.js'; import { ISCMViewServiceState, SCMViewService } from '../../browser/scmViewService.js'; import { workbenchInstantiationService } from '../../../../test/browser/workbenchTestServices.js'; @@ -56,8 +56,9 @@ suite('SCMViewService - Visibility Persistence', () => { const disposables = new DisposableStore(); function createViewServiceWithState(previousState?: ISCMViewServiceState): { viewService: SCMViewService; scmService: SCMService; storageService: TestStorageService } { - const instantiationService = disposables.add(workbenchInstantiationService(undefined, disposables)); - const storageService = disposables.add(new TestStorageService()); + const serviceDisposables = new DisposableStore(); + const instantiationService = workbenchInstantiationService(undefined, serviceDisposables); + const storageService = serviceDisposables.add(new TestStorageService()); if (previousState) { storageService.store('scm:view:visibleRepositories', JSON.stringify(previousState), StorageScope.WORKSPACE, StorageTarget.MACHINE); @@ -76,13 +77,22 @@ suite('SCMViewService - Visibility Persistence', () => { }); instantiationService.stub(IConfigurationService, configurationService); - const contextKeyService = disposables.add(new MockContextKeyService()); + const contextKeyService = serviceDisposables.add(new MockContextKeyService()); instantiationService.stub(IContextKeyService, contextKeyService); const scmService = instantiationService.createInstance(SCMService); + serviceDisposables.add(scmService); instantiationService.stub(ISCMService, scmService); - const viewService = disposables.add(instantiationService.createInstance(SCMViewService)); + const viewService = instantiationService.createInstance(SCMViewService); + + // Subscribe so that Event.debounce initializes its internal subscription, + // preventing a disposal error when the emitter is disposed without listeners. + serviceDisposables.add(viewService.onDidChangeVisibleRepositories(() => { })); + + // Add viewService first so it is disposed before its dependencies + disposables.add(viewService); + disposables.add(serviceDisposables); return { viewService, scmService, storageService }; } @@ -103,7 +113,7 @@ suite('SCMViewService - Visibility Persistence', () => { getProviderStorageKey(provider3) ], visible: [0, 2], // repo1 and repo3 are visible, repo2 is hidden - sortKey: 'discoveryTime' as any + sortKey: ISCMRepositorySortKey.DiscoveryTime }; const { viewService, scmService } = createViewServiceWithState(previousState); @@ -139,7 +149,7 @@ suite('SCMViewService - Visibility Persistence', () => { getProviderStorageKey(provider3) ], visible: [0, 2], // repo1 and repo3 are visible, repo2 is hidden - sortKey: 'discoveryTime' as any + sortKey: ISCMRepositorySortKey.DiscoveryTime }; const { viewService, scmService } = createViewServiceWithState(previousState); @@ -173,7 +183,7 @@ suite('SCMViewService - Visibility Persistence', () => { getProviderStorageKey(provider3) ], visible: [0, 2], - sortKey: 'discoveryTime' as any + sortKey: ISCMRepositorySortKey.DiscoveryTime }; const { viewService, scmService } = createViewServiceWithState(previousState); @@ -200,7 +210,7 @@ suite('SCMViewService - Visibility Persistence', () => { getProviderStorageKey(provider3) ], visible: [0, 2], - sortKey: 'discoveryTime' as any + sortKey: ISCMRepositorySortKey.DiscoveryTime }; const { viewService, scmService } = createViewServiceWithState(previousState); @@ -216,4 +226,41 @@ suite('SCMViewService - Visibility Persistence', () => { assert.ok(!visibleUris.includes(provider2.rootUri!.toString()), 'repo2 should be hidden'); assert.ok(visibleUris.includes(provider3.rootUri!.toString()), 'repo3 should be visible'); }); + + test('new repo registered before known repos remains visible after restoration', () => { + const provider1 = createMockProvider('1', URI.file('/repo1')); + const provider2 = createMockProvider('2', URI.file('/repo2')); + const provider3 = createMockProvider('3', URI.file('/repo3')); + + const previousState: ISCMViewServiceState = { + all: [ + getProviderStorageKey(provider1), + getProviderStorageKey(provider2), + getProviderStorageKey(provider3) + ], + visible: [0, 2], + sortKey: ISCMRepositorySortKey.DiscoveryTime + }; + + const { viewService, scmService } = createViewServiceWithState(previousState); + + // Register a NEW repo (not in previousState) BEFORE the known repos + const provider4 = createMockProvider('4', URI.file('/repo4')); + disposables.add(scmService.registerSCMProvider(provider4)); + + // Then register the known repos + disposables.add(scmService.registerSCMProvider(provider1)); + disposables.add(scmService.registerSCMProvider(provider2)); + disposables.add(scmService.registerSCMProvider(provider3)); + + // provider4 (new) should remain visible even though it registered first + assert.deepStrictEqual( + viewService.visibleRepositories.map(r => r.provider.rootUri?.toString()).sort(), + [ + provider1.rootUri!.toString(), + provider3.rootUri!.toString(), + provider4.rootUri!.toString() + ].sort() + ); + }); }); From 85a17da8960e9da02175cf91232f09a8bddb0ea9 Mon Sep 17 00:00:00 2001 From: kno Date: Mon, 4 May 2026 13:24:16 +0200 Subject: [PATCH 3/4] feat: keep repository visibility context menu open --- src/vs/base/browser/ui/menu/menu.ts | 26 +++++++++++++++ src/vs/platform/actions/common/actions.ts | 32 ++++++++++++++++++- src/vs/platform/actions/common/menuService.ts | 4 ++- .../contextview/browser/contextMenuHandler.ts | 5 +++ .../contrib/scm/browser/scmViewPane.ts | 2 +- 5 files changed, 66 insertions(+), 3 deletions(-) diff --git a/src/vs/base/browser/ui/menu/menu.ts b/src/vs/base/browser/ui/menu/menu.ts index bbadbf1d73b4cd..a2f9b3c65ef174 100644 --- a/src/vs/base/browser/ui/menu/menu.ts +++ b/src/vs/base/browser/ui/menu/menu.ts @@ -25,6 +25,7 @@ import { DisposableStore } from '../../../common/lifecycle.js'; import { isLinux, isMacintosh } from '../../../common/platform.js'; import { ScrollbarVisibility, ScrollEvent } from '../../../common/scrollable.js'; import * as strings from '../../../common/strings.js'; +import { hasKey } from '../../../common/types.js'; import { AnchorAlignment, layout, LayoutAnchorPosition } from '../../../common/layout.js'; export const MENU_MNEMONIC_REGEX = /\(&([^\s&])\)|(^|[^&])&([^\s&])/; @@ -237,6 +238,22 @@ export class Menu extends ActionBar { parent: this }; + // When a keepOpen action runs, refresh the checked/enabled state of all items + this._register(this.onDidRun(e => { + if (e.action && hasKey(e.action, 'keepOpen') && e.action.keepOpen) { + for (const item of this.viewItems) { + if (item instanceof BaseMenuActionViewItem) { + // Re-evaluate the action's state from context keys + if (hasKey(item.action, 'refreshState')) { + (item.action as { refreshState(): void }).refreshState(); + } + // Update the visual state + item.refreshState(); + } + } + } + })); + this.mnemonics = new Map>(); // Scroll Logic @@ -700,6 +717,15 @@ class BaseMenuActionViewItem extends BaseActionViewItem { return this.mnemonic; } + /** + * Refreshes the checked and enabled visual state of this menu item. + * Used when a keepOpen action runs and the menu stays visible. + */ + refreshState(): void { + this.updateChecked(); + this.updateEnabled(); + } + protected applyStyle(): void { const isSelected = this.element && this.element.classList.contains('focused'); const fgColor = isSelected && this.menuStyle.selectionForegroundColor ? this.menuStyle.selectionForegroundColor : this.menuStyle.foregroundColor; diff --git a/src/vs/platform/actions/common/actions.ts b/src/vs/platform/actions/common/actions.ts index c2dcf733944351..cfb329908a1016 100644 --- a/src/vs/platform/actions/common/actions.ts +++ b/src/vs/platform/actions/common/actions.ts @@ -25,6 +25,12 @@ export interface IMenuItem { group?: 'navigation' | string; order?: number; isHiddenByDefault?: boolean; + /** + * When true, the context menu will not close when this item is triggered. + * This is useful for toggle actions in menus where the user may want to + * change multiple items without having to reopen the menu. + */ + keepOpen?: boolean; } export interface ISubmenuItem { @@ -575,6 +581,9 @@ export class MenuItemAction implements IAction { readonly alt: MenuItemAction | undefined; private readonly _options: IMenuActionOptions | undefined; + private readonly _contextKeyService: IContextKeyService; + private readonly _precondition: ContextKeyExpression | undefined; + private readonly _toggledCondition: ContextKeyExpression | undefined; readonly id: string; readonly label: string; @@ -583,6 +592,11 @@ export class MenuItemAction implements IAction { readonly enabled: boolean; readonly checked?: boolean; + /** + * When true, the context menu should not close when this item is triggered. + */ + keepOpen: boolean = false; + constructor( item: ICommandAction, alt: ICommandAction | undefined, @@ -590,8 +604,11 @@ export class MenuItemAction implements IAction { readonly hideActions: IMenuItemHide | undefined, readonly menuKeybinding: IAction | undefined, @IContextKeyService contextKeyService: IContextKeyService, - @ICommandService private _commandService: ICommandService + @ICommandService private _commandService: ICommandService, ) { + this._contextKeyService = contextKeyService; + this._precondition = item.precondition; + this.id = item.id; this.label = MenuItemAction.label(item, options); this.tooltip = (typeof item.tooltip === 'string' ? item.tooltip : item.tooltip?.value) ?? ''; @@ -600,10 +617,12 @@ export class MenuItemAction implements IAction { let icon: ThemeIcon | undefined; + this._toggledCondition = undefined; if (item.toggled) { const toggled = ((item.toggled as { condition: ContextKeyExpression }).condition ? item.toggled : { condition: item.toggled }) as { condition: ContextKeyExpression; icon?: Icon; tooltip?: string | ILocalizedString; title?: string | ILocalizedString; }; + this._toggledCondition = toggled.condition; this.checked = contextKeyService.contextMatchesRules(toggled.condition); if (this.checked && toggled.tooltip) { this.tooltip = typeof toggled.tooltip === 'string' ? toggled.tooltip : toggled.tooltip.value; @@ -629,6 +648,17 @@ export class MenuItemAction implements IAction { } + /** + * Re-evaluates the `checked` and `enabled` state from the current context keys. + * Used when the menu stays open after this action runs (keepOpen). + */ + refreshState(): void { + (this as unknown as { enabled: boolean }).enabled = !this._precondition || this._contextKeyService.contextMatchesRules(this._precondition); + (this as unknown as { checked: boolean | undefined }).checked = this._toggledCondition + ? this._contextKeyService.contextMatchesRules(this._toggledCondition) + : undefined; + } + run(...args: unknown[]): Promise { let runArgs: unknown[] = []; diff --git a/src/vs/platform/actions/common/menuService.ts b/src/vs/platform/actions/common/menuService.ts index f5b7046bbb9674..65bd29708b3aaa 100644 --- a/src/vs/platform/actions/common/menuService.ts +++ b/src/vs/platform/actions/common/menuService.ts @@ -292,7 +292,9 @@ class MenuInfo extends MenuInfoSnapshot { if (isMenuItem) { // MenuItemAction const menuKeybinding = createConfigureKeybindingAction(this._commandService, this._keybindingService, item.command.id, item.when); - (activeActions ??= []).push(new MenuItemAction(item.command, item.alt, options, menuHide, menuKeybinding, this._contextKeyService, this._commandService)); + const menuItemAction = new MenuItemAction(item.command, item.alt, options, menuHide, menuKeybinding, this._contextKeyService, this._commandService); + menuItemAction.keepOpen = !!item.keepOpen; + (activeActions ??= []).push(menuItemAction); } else { // SubmenuItemAction const groups = new MenuInfo(item.submenu, this._hiddenStates, this._collectContextKeysForSubmenus, this._commandService, this._keybindingService, this._contextKeyService).createActionGroups(options); diff --git a/src/vs/platform/contextview/browser/contextMenuHandler.ts b/src/vs/platform/contextview/browser/contextMenuHandler.ts index b0f447fd1d39cc..e32e9ba469e77e 100644 --- a/src/vs/platform/contextview/browser/contextMenuHandler.ts +++ b/src/vs/platform/contextview/browser/contextMenuHandler.ts @@ -15,6 +15,7 @@ import { IKeybindingService } from '../../keybinding/common/keybinding.js'; import { INotificationService } from '../../notification/common/notification.js'; import { ITelemetryService } from '../../telemetry/common/telemetry.js'; import { defaultMenuStyles } from '../../theme/browser/defaultStyles.js'; +import { MenuItemAction } from '../../actions/common/actions.js'; export interface IContextMenuHandlerOptions { @@ -153,6 +154,10 @@ export class ContextMenuHandler { this.telemetryService.publicLog2('workbenchActionExecuted', { id: e.action.id, from: 'contextMenu' }); } + if (e.action instanceof MenuItemAction && e.action.keepOpen) { + return; + } + this.contextViewService.hideContextView(false); } diff --git a/src/vs/workbench/contrib/scm/browser/scmViewPane.ts b/src/vs/workbench/contrib/scm/browser/scmViewPane.ts index a2887d47f25215..776d1efc38f53c 100644 --- a/src/vs/workbench/contrib/scm/browser/scmViewPane.ts +++ b/src/vs/workbench/contrib/scm/browser/scmViewPane.ts @@ -976,7 +976,7 @@ class RepositoryVisibilityAction extends Action2 { f1: false, precondition: ContextKeyExpr.or(ContextKeys.RepositoryVisibilityCount.notEqualsTo(1), ContextKeys.RepositoryVisibility(repository).isEqualTo(false)), toggled: ContextKeys.RepositoryVisibility(repository).isEqualTo(true), - menu: { id: Menus.Repositories, group: '0_repositories' } + menu: { id: Menus.Repositories, group: '0_repositories', keepOpen: true } }); this.repository = repository; } From 87c995923a0b9d9615a9a85251b2a9a37b684efa Mon Sep 17 00:00:00 2001 From: aruizdesamaniego-sh Date: Tue, 12 May 2026 07:57:16 +0200 Subject: [PATCH 4/4] fix(scm): correct removed event and single-select restoration when new repo registers first - Only include repos from previousState in the removed event (not new repos that remain visible), fixing spurious UI state disposal in SCMViewPane - Clear all repos during the reset block so single-select mode correctly restores the saved selection even when a new repo registered earlier - Re-apply visibility to new repos after the first restored repo is placed, merged into a single event to avoid bouncing add/removed deltas - Add assertion that new repos never appear in removed events during startup Co-Authored-By: Claude Sonnet 4.6 --- .../contrib/scm/browser/scmViewService.ts | 30 ++++++++++++++----- .../scm/test/browser/scmViewService.test.ts | 13 ++++++++ 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/src/vs/workbench/contrib/scm/browser/scmViewService.ts b/src/vs/workbench/contrib/scm/browser/scmViewService.ts index 6443c8c8e5d292..422e2d46e9b86a 100644 --- a/src/vs/workbench/contrib/scm/browser/scmViewService.ts +++ b/src/vs/workbench/contrib/scm/browser/scmViewService.ts @@ -357,6 +357,7 @@ export class SCMViewService implements ISCMViewService { } satisfies ISCMRepositoryView; let removed: Iterable = Iterable.empty(); + let newReposToReAdd: ISCMRepositoryView[] = []; if (this.previousState && !this.didFinishLoadingRepositories.get()) { // Hidden repositories are not part of the saved state, so skip @@ -397,13 +398,18 @@ export class SCMViewService implements ISCMViewService { } else { // First visible repository if (!this.didSelectRepository) { - removed = [...this.visibleRepositories]; + newReposToReAdd = this._repositories.filter(r => + r.selectionIndex !== -1 && + this.previousState!.all.indexOf(getProviderStorageKey(r.repository.provider)) === -1 + ); + + removed = [...this.visibleRepositories].filter(r => + this.previousState!.all.indexOf(getProviderStorageKey(r.provider)) !== -1 + ); + this._repositories.forEach(r => { - // Preserve visibility of repos not in previousState (new repos) - if (this.previousState!.all.indexOf(getProviderStorageKey(r.repository.provider)) !== -1) { - r.focused = false; - r.selectionIndex = -1; - } + r.focused = false; + r.selectionIndex = -1; }); this.didSelectRepository = true; @@ -415,7 +421,17 @@ export class SCMViewService implements ISCMViewService { // Multiple selection mode or single selection mode (select first repository) const maxSelectionIndex = this.getMaxSelectionIndex(); this.insertRepositoryView(this._repositories, { ...repositoryView, selectionIndex: maxSelectionIndex + 1 }); - this._onDidChangeRepositories.fire({ added: [repositoryView.repository], removed }); + + if (newReposToReAdd.length > 0 && this.selectionModeConfig.get() === ISCMRepositorySelectionMode.Multiple) { + const addedRepos: ISCMRepository[] = [repositoryView.repository]; + for (const r of newReposToReAdd) { + r.selectionIndex = this.getMaxSelectionIndex() + 1; + addedRepos.push(r.repository); + } + this._onDidChangeRepositories.fire({ added: addedRepos, removed }); + } else { + this._onDidChangeRepositories.fire({ added: [repositoryView.repository], removed }); + } } else { // Single selection mode (add subsequent repository) this.insertRepositoryView(this._repositories, repositoryView); diff --git a/src/vs/workbench/contrib/scm/test/browser/scmViewService.test.ts b/src/vs/workbench/contrib/scm/test/browser/scmViewService.test.ts index 3fff1f2708bdeb..8c4af7a19d6595 100644 --- a/src/vs/workbench/contrib/scm/test/browser/scmViewService.test.ts +++ b/src/vs/workbench/contrib/scm/test/browser/scmViewService.test.ts @@ -244,6 +244,13 @@ suite('SCMViewService - Visibility Persistence', () => { const { viewService, scmService } = createViewServiceWithState(previousState); + const removedDuringRestore: string[] = []; + disposables.add(viewService.onDidChangeRepositories(e => { + for (const r of e.removed) { + removedDuringRestore.push(r.provider.rootUri?.toString() ?? ''); + } + })); + // Register a NEW repo (not in previousState) BEFORE the known repos const provider4 = createMockProvider('4', URI.file('/repo4')); disposables.add(scmService.registerSCMProvider(provider4)); @@ -262,5 +269,11 @@ suite('SCMViewService - Visibility Persistence', () => { provider4.rootUri!.toString() ].sort() ); + + // provider4 should never appear in a removed event during restoration + assert.deepStrictEqual( + removedDuringRestore.filter(uri => uri === provider4.rootUri!.toString()), + [] + ); }); });