From e0de3232da2f5e071d9533a4b7de6c4deedcbd32 Mon Sep 17 00:00:00 2001 From: Rob Lourens Date: Tue, 31 Mar 2026 21:37:02 -0700 Subject: [PATCH] agentHost: Disable folders from disconnected remotes (#306989) * agentHost: Disable folders from disconnected remotes * Address comments * add tests Co-authored-by: Copilot --------- Co-authored-by: Copilot --- .../chat/browser/sessionWorkspacePicker.ts | 109 +++++- .../browser/sessionWorkspacePicker.test.ts | 336 ++++++++++++++++++ 2 files changed, 437 insertions(+), 8 deletions(-) create mode 100644 src/vs/sessions/contrib/chat/test/browser/sessionWorkspacePicker.test.ts diff --git a/src/vs/sessions/contrib/chat/browser/sessionWorkspacePicker.ts b/src/vs/sessions/contrib/chat/browser/sessionWorkspacePicker.ts index 8c5fc0d81ba..395ddcf86ef 100644 --- a/src/vs/sessions/contrib/chat/browser/sessionWorkspacePicker.ts +++ b/src/vs/sessions/contrib/chat/browser/sessionWorkspacePicker.ts @@ -8,7 +8,7 @@ import { SubmenuAction, toAction } from '../../../../base/common/actions.js'; import { Codicon } from '../../../../base/common/codicons.js'; import { Emitter, Event } from '../../../../base/common/event.js'; import { MarkdownString } from '../../../../base/common/htmlContent.js'; -import { Disposable, DisposableStore } from '../../../../base/common/lifecycle.js'; +import { Disposable, DisposableStore, MutableDisposable } from '../../../../base/common/lifecycle.js'; import { URI, UriComponents } from '../../../../base/common/uri.js'; import { Schemas } from '../../../../base/common/network.js'; import { localize } from '../../../../nls.js'; @@ -21,6 +21,7 @@ import { IOutputService } from '../../../../workbench/services/output/common/out import { IQuickInputService, IQuickPickItem } from '../../../../platform/quickinput/common/quickInput.js'; import { IStorageService, StorageScope, StorageTarget } from '../../../../platform/storage/common/storage.js'; import { IUriIdentityService } from '../../../../platform/uriIdentity/common/uriIdentity.js'; +import { autorun } from '../../../../base/common/observable.js'; import { renderIcon } from '../../../../base/browser/ui/iconLabel/iconLabels.js'; import { ThemeIcon } from '../../../../base/common/themables.js'; import { ISessionWorkspace } from '../../sessions/common/sessionData.js'; @@ -80,6 +81,7 @@ export class WorkspacePicker extends Disposable { private _triggerElement: HTMLElement | undefined; private readonly _renderDisposables = this._register(new DisposableStore()); + private readonly _connectionStatusListener = this._register(new MutableDisposable()); get selectedProject(): IWorkspaceSelection | undefined { return this._selectedWorkspace; @@ -125,7 +127,10 @@ export class WorkspacePicker extends Disposable { this._onDidSelectWorkspace.fire(restored); } } + this._watchConnectionStatus(); })); + + this._watchConnectionStatus(); } /** @@ -177,6 +182,10 @@ export class WorkspacePicker extends Disposable { const delegate: IActionListDelegate = { onSelect: (item) => { this.actionWidgetService.hide(); + if (item.selection && this._isProviderUnavailable(item.selection.providerId)) { + // Workspace belongs to an unavailable remote — ignore selection + return; + } if (item.remoteProvider && item.browseActionIndex === undefined) { // Disconnected remote host — show options menu after widget hides this._showRemoteHostOptionsDelayed(item.remoteProvider); @@ -307,15 +316,19 @@ export class WorkspacePicker extends Disposable { const providersWithWorkspaces = allProviders.filter(p => recentWorkspaces.some(w => w.providerId === p.id)); for (let pi = 0; pi < providersWithWorkspaces.length; pi++) { const provider = providersWithWorkspaces[pi]; + const isOffline = this._isProviderUnavailable(provider.id); const providerWorkspaces = recentWorkspaces.filter(w => w.providerId === provider.id); for (let i = 0; i < providerWorkspaces.length; i++) { const { workspace, providerId } = providerWorkspaces[i]; const selection: IWorkspaceSelection = { providerId, workspace }; const selected = this._isSelectedWorkspace(selection); + const description = i === 0 + ? (isOffline ? localize('workspacePicker.providerOffline', "{0} (Offline)", provider.label) : provider.label) + : (isOffline ? localize('workspacePicker.offline', "Offline") : undefined); items.push({ kind: ActionListItemKind.Action, label: workspace.label, - description: i === 0 ? provider.label : undefined, + description, group: { title: '', icon: workspace.icon }, item: { selection, checked: selected || undefined }, onRemove: () => this._removeRecentWorkspace(selection), @@ -329,9 +342,11 @@ export class WorkspacePicker extends Disposable { for (const { workspace, providerId } of recentWorkspaces) { const selection: IWorkspaceSelection = { providerId, workspace }; const selected = this._isSelectedWorkspace(selection); + const isOffline = this._isProviderUnavailable(providerId); items.push({ kind: ActionListItemKind.Action, label: workspace.label, + description: isOffline ? localize('workspacePicker.offlineSingle', "Offline") : undefined, group: { title: '', icon: workspace.icon }, item: { selection, checked: selected || undefined }, onRemove: () => this._removeRecentWorkspace(selection), @@ -552,6 +567,57 @@ export class WorkspacePicker extends Disposable { dom.append(this._triggerElement, renderIcon(Codicon.chevronDown)).classList.add('sessions-chat-dropdown-chevron'); } + /** + * Returns whether the given provider is a remote that is currently unavailable + * (disconnected or still connecting). + * Returns false for providers without connection status (e.g. local providers). + */ + private _isProviderUnavailable(providerId: string): boolean { + const provider = this.sessionsProvidersService.getProviders().find(p => p.id === providerId); + if (!provider?.connectionStatus) { + return false; + } + return provider.connectionStatus.get() !== RemoteAgentHostConnectionStatus.Connected; + } + + /** + * Watch connection status observables from all remote providers. + * When a remote disconnects, clear the selection if it belongs to that + * provider. When a remote reconnects, try to restore a stored workspace. + */ + private _watchConnectionStatus(): void { + const remoteProviders = this.sessionsProvidersService.getProviders().filter(p => p.connectionStatus !== undefined); + if (remoteProviders.length === 0) { + this._connectionStatusListener.clear(); + return; + } + + this._connectionStatusListener.value = autorun(reader => { + for (const provider of remoteProviders) { + provider.connectionStatus!.read(reader); + } + + // If the current selection belongs to an unavailable provider, clear it + if (this._selectedWorkspace && this._isProviderUnavailable(this._selectedWorkspace.providerId)) { + this._selectedWorkspace = undefined; + this._updateTriggerLabel(); + this._onDidChangeSelection.fire(); + } + + // If no selection, try to restore the previously checked workspace + // (only the checked entry, not any fallback, to avoid unexpected switches) + if (!this._selectedWorkspace) { + const restored = this._restoreCheckedWorkspace(); + if (restored) { + this._selectedWorkspace = restored; + this._updateTriggerLabel(); + this._onDidChangeSelection.fire(); + this._onDidSelectWorkspace.fire(restored); + } + } + }); + } + private _isSelectedWorkspace(selection: IWorkspaceSelection): boolean { if (!this._selectedWorkspace) { return false; @@ -573,14 +639,23 @@ export class WorkspacePicker extends Disposable { } private _restoreSelectedWorkspace(): IWorkspaceSelection | undefined { + // Try the checked entry first + const checked = this._restoreCheckedWorkspace(); + if (checked) { + return checked; + } + + // Fall back to the first resolvable recent workspace from a connected provider try { const providers = this._getActiveProviders(); const providerIds = new Set(providers.map(p => p.id)); const storedRecents = this._getStoredRecentWorkspaces(); - // Find the checked entry for an active provider for (const stored of storedRecents) { - if (!stored.checked || !providerIds.has(stored.providerId)) { + if (!providerIds.has(stored.providerId)) { + continue; + } + if (this._isProviderUnavailable(stored.providerId)) { continue; } const uri = URI.revive(stored.uri); @@ -589,10 +664,28 @@ export class WorkspacePicker extends Disposable { return { providerId: stored.providerId, workspace }; } } + return undefined; + } catch { + return undefined; + } + } + + /** + * Restore only the checked (previously selected) workspace if its provider + * is currently available. Does not fall back to other workspaces. + * Used by the connection status watcher to avoid unexpected workspace switches. + */ + private _restoreCheckedWorkspace(): IWorkspaceSelection | undefined { + try { + const providers = this._getActiveProviders(); + const providerIds = new Set(providers.map(p => p.id)); + const storedRecents = this._getStoredRecentWorkspaces(); - // No checked entry found — fall back to the first resolvable recent workspace for (const stored of storedRecents) { - if (!providerIds.has(stored.providerId)) { + if (!stored.checked || !providerIds.has(stored.providerId)) { + continue; + } + if (this._isProviderUnavailable(stored.providerId)) { continue; } const uri = URI.revive(stored.uri); @@ -650,8 +743,8 @@ export class WorkspacePicker extends Disposable { if (p.providerId === providerId && this.uriIdentityService.extUri.isEqual(URI.revive(p.uri), uri)) { return undefined; } - // Clear checked from other entries for the same provider when marking checked - if (checked && p.providerId === providerId) { + // Clear checked from all other entries when marking checked + if (checked && p.checked) { return { ...p, checked: false }; } return p; diff --git a/src/vs/sessions/contrib/chat/test/browser/sessionWorkspacePicker.test.ts b/src/vs/sessions/contrib/chat/test/browser/sessionWorkspacePicker.test.ts new file mode 100644 index 00000000000..0b5c3500c17 --- /dev/null +++ b/src/vs/sessions/contrib/chat/test/browser/sessionWorkspacePicker.test.ts @@ -0,0 +1,336 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import { Codicon } from '../../../../../base/common/codicons.js'; +import { Emitter, Event } from '../../../../../base/common/event.js'; +import { Disposable, DisposableStore } from '../../../../../base/common/lifecycle.js'; +import { ISettableObservable, observableValue } from '../../../../../base/common/observable.js'; +import { URI } from '../../../../../base/common/uri.js'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; +import { IActionWidgetService } from '../../../../../platform/actionWidget/browser/actionWidget.js'; +import { RemoteAgentHostConnectionStatus, IRemoteAgentHostService } from '../../../../../platform/agentHost/common/remoteAgentHostService.js'; +import { IClipboardService } from '../../../../../platform/clipboard/common/clipboardService.js'; +import { TestInstantiationService } from '../../../../../platform/instantiation/test/common/instantiationServiceMock.js'; +import { IQuickInputService } from '../../../../../platform/quickinput/common/quickInput.js'; +import { IStorageService, StorageScope, StorageTarget } from '../../../../../platform/storage/common/storage.js'; +import { TestStorageService } from '../../../../../workbench/test/common/workbenchTestServices.js'; +import { IPreferencesService } from '../../../../../workbench/services/preferences/common/preferences.js'; +import { IOutputService } from '../../../../../workbench/services/output/common/output.js'; +import { IUriIdentityService } from '../../../../../platform/uriIdentity/common/uriIdentity.js'; +import { extUri } from '../../../../../base/common/resources.js'; +import { ISessionsProvidersService } from '../../../sessions/browser/sessionsProvidersService.js'; +import { ISessionsManagementService } from '../../../sessions/browser/sessionsManagementService.js'; +import { ISessionChangeEvent, ISessionsProvider } from '../../../sessions/browser/sessionsProvider.js'; +import { ISessionWorkspace } from '../../../sessions/common/sessionData.js'; +import { WorkspacePicker, IWorkspaceSelection } from '../../browser/sessionWorkspacePicker.js'; + +// ---- Storage key (must match the one in sessionWorkspacePicker.ts) ---------- +const STORAGE_KEY_RECENT_WORKSPACES = 'sessions.recentlyPickedWorkspaces'; + +// ---- Mock providers --------------------------------------------------------- + +function createMockProvider(id: string, opts?: { + connectionStatus?: ISettableObservable; +}): ISessionsProvider { + return { + id, + label: `Provider ${id}`, + icon: Codicon.remote, + sessionTypes: [], + connectionStatus: opts?.connectionStatus, + browseActions: [], + resolveWorkspace: (uri: URI): ISessionWorkspace => ({ + label: uri.path.substring(1) || uri.path, + icon: Codicon.folder, + repositories: [{ uri, workingDirectory: undefined, detail: undefined, baseBranchName: undefined, baseBranchProtected: undefined }], + requiresWorkspaceTrust: false, + }), + onDidChangeSessions: Event.None, + getSessions: () => [], + createNewSession: () => { throw new Error('Not implemented'); }, + createNewSessionFrom: () => { throw new Error('Not implemented'); }, + setSessionType: () => { throw new Error('Not implemented'); }, + getSessionTypes: () => [], + renameSession: async () => { }, + setModel: () => { }, + archiveSession: async () => { }, + unarchiveSession: async () => { }, + deleteSession: async () => { }, + setRead: () => { }, + getUntitledSession: () => undefined, + sendRequest: async () => { throw new Error('Not implemented'); }, + }; +} + +class MockSessionsProvidersService extends Disposable { + declare readonly _serviceBrand: undefined; + + private readonly _onDidChangeProviders = this._register(new Emitter()); + readonly onDidChangeProviders: Event = this._onDidChangeProviders.event; + readonly onDidChangeSessions: Event = Event.None; + readonly onDidReplaceSession = Event.None; + + private _providers: ISessionsProvider[] = []; + + setProviders(providers: ISessionsProvider[]): void { + this._providers = providers; + this._onDidChangeProviders.fire(); + } + + getProviders(): ISessionsProvider[] { + return this._providers; + } + + resolveWorkspace(providerId: string, repositoryUri: URI): ISessionWorkspace | undefined { + const provider = this._providers.find(p => p.id === providerId); + return provider?.resolveWorkspace(repositoryUri); + } +} + +// ---- Test helpers ----------------------------------------------------------- + +function seedStorage(storageService: IStorageService, entries: { uri: URI; providerId: string; checked: boolean }[]): void { + const stored = entries.map(e => ({ + uri: e.uri.toJSON(), + providerId: e.providerId, + checked: e.checked, + })); + storageService.store(STORAGE_KEY_RECENT_WORKSPACES, JSON.stringify(stored), StorageScope.PROFILE, StorageTarget.MACHINE); +} + +function createTestPicker( + disposables: DisposableStore, + providersService: MockSessionsProvidersService, + storageService?: IStorageService, +): WorkspacePicker { + const instantiationService = disposables.add(new TestInstantiationService()); + const storage = storageService ?? disposables.add(new TestStorageService()); + + instantiationService.stub(IActionWidgetService, { isVisible: false, hide: () => { }, show: () => { } }); + instantiationService.stub(IStorageService, storage); + instantiationService.stub(IUriIdentityService, { extUri }); + instantiationService.stub(ISessionsProvidersService, providersService); + instantiationService.stub(ISessionsManagementService, { + activeProviderId: observableValue('activeProviderId', undefined), + }); + instantiationService.stub(IRemoteAgentHostService, {}); + instantiationService.stub(IQuickInputService, {}); + instantiationService.stub(IClipboardService, {}); + instantiationService.stub(IPreferencesService, {}); + instantiationService.stub(IOutputService, {}); + + return disposables.add(instantiationService.createInstance(WorkspacePicker)); +} + +// ---- Assertion helpers ------------------------------------------------------ + +function assertSelectedProvider(picker: WorkspacePicker, expectedProviderId: string | undefined, message?: string): void { + assert.strictEqual(picker.selectedProject?.providerId, expectedProviderId, message); +} + +// ---- Tests ------------------------------------------------------------------ + +suite('WorkspacePicker - Connection Status', () => { + + const disposables = new DisposableStore(); + let providersService: MockSessionsProvidersService; + + setup(() => { + providersService = new MockSessionsProvidersService(); + disposables.add(providersService); + }); + + teardown(() => { + disposables.clear(); + }); + + ensureNoDisposablesAreLeakedInTestSuite(); + + test('restore skips unavailable (disconnected) provider', () => { + const remoteStatus = observableValue('status', RemoteAgentHostConnectionStatus.Disconnected); + const remoteProvider = createMockProvider('remote-1', { connectionStatus: remoteStatus }); + const localProvider = createMockProvider('local-1'); + + const storage = disposables.add(new TestStorageService()); + seedStorage(storage, [ + { uri: URI.file('/remote/project'), providerId: 'remote-1', checked: true }, + { uri: URI.file('/local/project'), providerId: 'local-1', checked: false }, + ]); + + providersService.setProviders([remoteProvider, localProvider]); + const picker = createTestPicker(disposables, providersService, storage); + + // The checked entry is from a disconnected provider — should fall back to local + assertSelectedProvider(picker, 'local-1'); + }); + + test('restore skips connecting provider', () => { + const remoteStatus = observableValue('status', RemoteAgentHostConnectionStatus.Connecting); + const remoteProvider = createMockProvider('remote-1', { connectionStatus: remoteStatus }); + const localProvider = createMockProvider('local-1'); + + const storage = disposables.add(new TestStorageService()); + seedStorage(storage, [ + { uri: URI.file('/remote/project'), providerId: 'remote-1', checked: true }, + { uri: URI.file('/local/project'), providerId: 'local-1', checked: false }, + ]); + + providersService.setProviders([remoteProvider, localProvider]); + const picker = createTestPicker(disposables, providersService, storage); + + assertSelectedProvider(picker, 'local-1'); + }); + + test('restore picks connected remote provider', () => { + const remoteStatus = observableValue('status', RemoteAgentHostConnectionStatus.Connected); + const remoteProvider = createMockProvider('remote-1', { connectionStatus: remoteStatus }); + + const storage = disposables.add(new TestStorageService()); + seedStorage(storage, [ + { uri: URI.file('/remote/project'), providerId: 'remote-1', checked: true }, + ]); + + providersService.setProviders([remoteProvider]); + const picker = createTestPicker(disposables, providersService, storage); + + assertSelectedProvider(picker, 'remote-1'); + }); + + test('disconnect clears selection from that provider', () => { + const remoteStatus = observableValue('status', RemoteAgentHostConnectionStatus.Connected); + const remoteProvider = createMockProvider('remote-1', { connectionStatus: remoteStatus }); + + const storage = disposables.add(new TestStorageService()); + seedStorage(storage, [ + { uri: URI.file('/remote/project'), providerId: 'remote-1', checked: true }, + ]); + + providersService.setProviders([remoteProvider]); + const picker = createTestPicker(disposables, providersService, storage); + assertSelectedProvider(picker, 'remote-1'); + + // Disconnect + remoteStatus.set(RemoteAgentHostConnectionStatus.Disconnected, undefined); + assertSelectedProvider(picker, undefined, 'Selection should be cleared after disconnect'); + }); + + test('reconnect restores the same workspace', () => { + const remoteStatus = observableValue('status', RemoteAgentHostConnectionStatus.Connected); + const remoteProvider = createMockProvider('remote-1', { connectionStatus: remoteStatus }); + + const storage = disposables.add(new TestStorageService()); + seedStorage(storage, [ + { uri: URI.file('/remote/project'), providerId: 'remote-1', checked: true }, + ]); + + providersService.setProviders([remoteProvider]); + const picker = createTestPicker(disposables, providersService, storage); + assertSelectedProvider(picker, 'remote-1'); + + // Disconnect — clears selection + remoteStatus.set(RemoteAgentHostConnectionStatus.Disconnected, undefined); + assertSelectedProvider(picker, undefined, 'Should clear on disconnect'); + + // Reconnect — should restore + remoteStatus.set(RemoteAgentHostConnectionStatus.Connected, undefined); + assertSelectedProvider(picker, 'remote-1', 'Should restore after reconnect'); + assert.strictEqual( + picker.selectedProject?.workspace.repositories[0]?.uri.path, + '/remote/project', + 'Should restore the same workspace URI', + ); + }); + + test('disconnect does not auto-select another provider workspace', () => { + const remoteStatus = observableValue('status', RemoteAgentHostConnectionStatus.Connected); + const remoteProvider = createMockProvider('remote-1', { connectionStatus: remoteStatus }); + const localProvider = createMockProvider('local-1'); + + const storage = disposables.add(new TestStorageService()); + seedStorage(storage, [ + { uri: URI.file('/remote/project'), providerId: 'remote-1', checked: true }, + { uri: URI.file('/local/project'), providerId: 'local-1', checked: false }, + ]); + + providersService.setProviders([remoteProvider, localProvider]); + const picker = createTestPicker(disposables, providersService, storage); + assertSelectedProvider(picker, 'remote-1'); + + // Disconnect remote + remoteStatus.set(RemoteAgentHostConnectionStatus.Disconnected, undefined); + + // Should NOT auto-select local workspace — should remain empty + assertSelectedProvider(picker, undefined, 'Should not auto-select another provider on disconnect'); + }); + + test('checked is globally unique after persist', () => { + const localProvider = createMockProvider('local-1'); + const remoteStatus = observableValue('status', RemoteAgentHostConnectionStatus.Connected); + const remoteProvider = createMockProvider('remote-1', { connectionStatus: remoteStatus }); + + const storage = disposables.add(new TestStorageService()); + seedStorage(storage, [ + { uri: URI.file('/remote/project'), providerId: 'remote-1', checked: true }, + { uri: URI.file('/local/project'), providerId: 'local-1', checked: false }, + ]); + + providersService.setProviders([remoteProvider, localProvider]); + const picker = createTestPicker(disposables, providersService, storage); + + // Select the local workspace + const localWorkspace: IWorkspaceSelection = { + providerId: 'local-1', + workspace: localProvider.resolveWorkspace(URI.file('/local/project')), + }; + picker.setSelectedWorkspace(localWorkspace, false); + + // Verify storage: only the local entry should be checked + const raw = storage.get(STORAGE_KEY_RECENT_WORKSPACES, StorageScope.PROFILE); + assert.ok(raw, 'Storage should have recent workspaces'); + const stored = JSON.parse(raw!) as { providerId: string; checked: boolean }[]; + const checkedEntries = stored.filter(e => e.checked); + assert.strictEqual(checkedEntries.length, 1, 'Only one entry should be checked'); + assert.strictEqual(checkedEntries[0].providerId, 'local-1', 'The local entry should be checked'); + }); + + test('onDidSelectWorkspace fires on reconnect restore', () => { + const remoteStatus = observableValue('status', RemoteAgentHostConnectionStatus.Connected); + const remoteProvider = createMockProvider('remote-1', { connectionStatus: remoteStatus }); + + const storage = disposables.add(new TestStorageService()); + seedStorage(storage, [ + { uri: URI.file('/remote/project'), providerId: 'remote-1', checked: true }, + ]); + + providersService.setProviders([remoteProvider]); + const picker = createTestPicker(disposables, providersService, storage); + + const selected: IWorkspaceSelection[] = []; + disposables.add(picker.onDidSelectWorkspace(w => selected.push(w))); + + // Disconnect then reconnect + remoteStatus.set(RemoteAgentHostConnectionStatus.Disconnected, undefined); + remoteStatus.set(RemoteAgentHostConnectionStatus.Connected, undefined); + + assert.strictEqual(selected.length, 1, 'onDidSelectWorkspace should fire once on reconnect'); + assert.strictEqual(selected[0].providerId, 'remote-1'); + assert.strictEqual(selected[0].workspace.repositories[0]?.uri.path, '/remote/project', 'Event should carry the correct workspace URI'); + }); + + test('local provider is never treated as unavailable', () => { + const localProvider = createMockProvider('local-1'); + + const storage = disposables.add(new TestStorageService()); + seedStorage(storage, [ + { uri: URI.file('/local/project'), providerId: 'local-1', checked: true }, + ]); + + providersService.setProviders([localProvider]); + const picker = createTestPicker(disposables, providersService, storage); + + assertSelectedProvider(picker, 'local-1', 'Local provider workspace should always be selectable'); + }); +});