Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/vs/sessions/contrib/chat/browser/chat.contribution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ class NewChatInSessionsWindowAction extends Action2 {
});
}

override run(accessor: ServicesAccessor): void {
override async run(accessor: ServicesAccessor, options?: { toSide?: boolean }): Promise<void> {
const sessionsService = accessor.get(ISessionsService);
const sessionsManagementService = accessor.get(ISessionsManagementService);
const activeSession = sessionsService.activeSession.get();
Expand All @@ -201,8 +201,9 @@ class NewChatInSessionsWindowAction extends Action2 {
// Inherit the active session's harness so the new session defaults to
// the kind the user is working in — but only while the folder still
// offers it (see `inheritableSessionTarget`).
sessionsService.openNewSession({
await sessionsService.openNewSession({
folderUri,
toSide: options?.toSide,
...inheritableSessionTarget(sessionsManagementService, activeSession, folderUri),
});
}
Expand Down
19 changes: 16 additions & 3 deletions src/vs/sessions/contrib/sessions/browser/sessionsActions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ import { ISessionsService } from '../../../services/sessions/browser/sessionsSer
import { ChatOriginKind, getChatCapabilities, getGitHubPullRequestRefs, getHighestPriorityPullRequestIcon, getUntitledSessionTitle, IChat, ISession, SessionStatus } from '../../../services/sessions/common/session.js';
import { ISessionsPartService } from '../../../services/sessions/browser/sessionsPartService.js';
import { ISessionsListModelService } from '../../../services/sessions/browser/sessionsListModelService.js';
import { $, append, EventHelper, ModifierKeyEmitter, reset } from '../../../../base/browser/dom.js';
import { $, append, EventHelper, isMouseEvent, ModifierKeyEmitter, reset } from '../../../../base/browser/dom.js';
import { BaseActionViewItem } from '../../../../base/browser/ui/actionbar/actionViewItems.js';
import { Button } from '../../../../base/browser/ui/button/button.js';
import { HoverPosition } from '../../../../base/browser/ui/hover/hoverWidget.js';
Expand Down Expand Up @@ -1270,6 +1270,10 @@ export abstract class CompactButtonActionViewItem extends BaseActionViewItem {
/** Hook invoked right before the action runs (e.g. for telemetry). */
protected onRun(): void { }

protected runAction(_event: MouseEvent | undefined): void {
void this.actionRunner.run(this.action, this._context);
}

protected configureButton(_button: Button): void { }

override render(container: HTMLElement): void {
Expand Down Expand Up @@ -1301,7 +1305,7 @@ export abstract class CompactButtonActionViewItem extends BaseActionViewItem {
return;
}
this.onRun();
this.actionRunner.run(this.action, this._context);
this.runAction(isMouseEvent(e) ? e : undefined);
}));

const buttonLabel = $('span.new-session-button-label', undefined, this.label);
Expand Down Expand Up @@ -1362,7 +1366,7 @@ export abstract class CompactButtonActionViewItem extends BaseActionViewItem {
* Renders the new-session action as the compact "New" pill, shared by the sessions sidebar
* header and the titlebar.
*/
class NewSessionActionViewItem extends CompactButtonActionViewItem {
export class NewSessionActionViewItem extends CompactButtonActionViewItem {

constructor(
action: IAction,
Expand All @@ -1372,6 +1376,7 @@ class NewSessionActionViewItem extends CompactButtonActionViewItem {
@IHoverService hoverService: IHoverService,
@ITelemetryService private readonly telemetryService: ITelemetryService,
@IContextKeyService contextKeyService: IContextKeyService,
@ICommandService private readonly commandService: ICommandService,
) {
super(action, keybindingService, hoverService, contextKeyService);
}
Expand Down Expand Up @@ -1411,6 +1416,14 @@ class NewSessionActionViewItem extends CompactButtonActionViewItem {
protected override onRun(): void {
logSessionsInteraction(this.telemetryService, 'newSession', this.telemetrySource);
}

protected override runAction(event: MouseEvent | undefined): void {
if (event?.altKey) {
this.commandService.executeCommand(NEW_SESSION_ACTION_ID, { toSide: true }).catch(onUnexpectedError);
Comment thread
federicobrancasi marked this conversation as resolved.
} else {
super.runAction(event);
}
}
}

/**
Expand Down
157 changes: 152 additions & 5 deletions src/vs/sessions/contrib/sessions/test/browser/sessionsActions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,24 +8,27 @@ import { constObservable, observableValue } from '../../../../../base/common/obs
import { URI } from '../../../../../base/common/uri.js';
import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js';
import { isIMenuItem, isISubmenuItem, MenuRegistry } from '../../../../../platform/actions/common/actions.js';
import { CommandsRegistry } from '../../../../../platform/commands/common/commands.js';
import { CommandsRegistry, ICommandService } from '../../../../../platform/commands/common/commands.js';
import { IConfigurationService } from '../../../../../platform/configuration/common/configuration.js';
import { TestConfigurationService } from '../../../../../platform/configuration/test/common/testConfigurationService.js';
import { TestInstantiationService } from '../../../../../platform/instantiation/test/common/instantiationServiceMock.js';
import { KeybindingsRegistry } from '../../../../../platform/keybinding/common/keybindingsRegistry.js';
import { workbenchInstantiationService } from '../../../../../workbench/test/browser/workbenchTestServices.js';
import { Menus } from '../../../../browser/menus.js';
import { SESSION_CONVERSATION_SIDE_CHATS_GROUP } from '../../../../browser/sessionConversationGroups.js';
import { SessionView } from '../../../../browser/parts/sessionView.js';
import { ISessionsPartService } from '../../../../services/sessions/browser/sessionsPartService.js';
import { ISessionsService } from '../../../../services/sessions/browser/sessionsService.js';
import { type IOpenNewSessionOptions, type IOpenNewSessionResult, ISessionsService } from '../../../../services/sessions/browser/sessionsService.js';
import { ChatOriginKind, IChat, SessionStatus } from '../../../../services/sessions/common/session.js';
import { IActiveSession, ISessionsManagementService } from '../../../../services/sessions/common/sessionsManagement.js';
import { mock, upcastPartial } from '../../../../../base/test/common/mock.js';

import { SessionConversationActionsContribution } from '../../browser/sessionsActions.js';
import { Action } from '../../../../../base/common/actions.js';
import { NewSessionActionViewItem, type NewSessionButtonStyle, SessionConversationActionsContribution } from '../../browser/sessionsActions.js';
import '../../../chat/browser/chat.contribution.js';
import { NEW_SESSION_ACTION_ID, UNIFIED_WORKSPACE_PICKER_SETTING } from '../../../chat/common/constants.js';
import '../../browser/views/sessionsViewActions.js';
import { createTestSession } from './sessionsListTestUtils.js';
import { UNIFIED_WORKSPACE_PICKER_SETTING } from '../../../chat/common/constants.js';
import { createTestSession, TestCommandService } from './sessionsListTestUtils.js';

suite('Sessions - Actions', () => {

Expand Down Expand Up @@ -240,4 +243,148 @@ suite('Sessions - Actions', () => {
{ title: 'Side chat', group: SESSION_CONVERSATION_SIDE_CHATS_GROUP },
]);
});

test('does not register a separate New Session to the Side command or shortcut', () => {
const commandId = 'workbench.action.sessions.newChatToSide';
assert.deepStrictEqual({
command: CommandsRegistry.getCommand(commandId),
keybindings: KeybindingsRegistry.getDefaultKeybindings().filter(binding => binding.command === commandId),
}, {
command: undefined,
keybindings: [],
});
});

test('New button hover only includes the existing keyboard shortcut', () => {
class TestNewSessionActionViewItem extends NewSessionActionViewItem {
override getHoverContent(keybindingLabel: string | undefined): string {
return super.getHoverContent(keybindingLabel);
}
}

const instantiationService = disposables.add(workbenchInstantiationService(undefined, disposables));
instantiationService.stub(ICommandService, new TestCommandService());
const action = disposables.add(new Action(NEW_SESSION_ACTION_ID, 'New'));
const item = disposables.add(instantiationService.createInstance(
TestNewSessionActionViewItem, action, 'sidebar', constObservable<NewSessionButtonStyle>('default')
));

assert.deepStrictEqual({
withKeybinding: item.getHoverContent('Ctrl+N'),
withoutKeybinding: item.getHoverContent(undefined),
}, {
withKeybinding: 'New Session (Ctrl+N)',
withoutKeybinding: 'New Session',
});
});

for (const source of ['sidebar', 'titleBar'] as const) {
test(`New button in the ${source} opens to the side only on Alt-click`, () => {
const instantiationService = disposables.add(workbenchInstantiationService(undefined, disposables));
const commandService = new TestCommandService();
instantiationService.stub(ICommandService, commandService);
let primaryRuns = 0;
const action = disposables.add(new Action(NEW_SESSION_ACTION_ID, 'New', undefined, true, async () => {
primaryRuns++;
}));
const style = observableValue<NewSessionButtonStyle>('newButtonStyle', 'default');
const item = disposables.add(instantiationService.createInstance(NewSessionActionViewItem, action, source, style));
const container = document.createElement('div');
item.render(container);

const button = container.querySelector<HTMLElement>('.agent-sessions-compact-new-button.monaco-button');
assert.ok(button);
assert.deepStrictEqual({
buttonCount: container.querySelectorAll('.monaco-button').length,
dropdown: container.querySelector('.monaco-button-dropdown'),
popup: button.getAttribute('aria-haspopup'),
label: button.querySelector('.new-session-button-label')?.textContent,
}, {
buttonCount: 1,
dropdown: null,
popup: null,
label: 'New',
});

button.click();
button.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter', keyCode: 13, bubbles: true, cancelable: true }));
button.dispatchEvent(new KeyboardEvent('keydown', { key: ' ', keyCode: 32, bubbles: true, cancelable: true }));
button.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, altKey: true }));
action.enabled = false;
button.click();
button.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, altKey: true }));

assert.deepStrictEqual({ primaryRuns, commands: commandService.calls }, {
primaryRuns: 3,
commands: [{ commandId: NEW_SESSION_ACTION_ID, args: [{ toSide: true }] }],
});

style.set('lightweightWithKeybindingBackground', undefined);
assert.deepStrictEqual({
lightweight: button.classList.contains('lightweight'),
keybindingBackground: button.classList.contains('lightweight-keybinding-background'),
}, { lightweight: true, keybindingBackground: true });
style.set('default', undefined);
assert.deepStrictEqual({
lightweight: button.classList.contains('lightweight'),
keybindingBackground: button.classList.contains('lightweight-keybinding-background'),
}, { lightweight: false, keybindingBackground: false });
});
}

for (const toSide of [undefined, true]) {
for (const scenario of [
{ name: 'workspace', isQuickChat: false, targetAvailable: true },
{ name: 'unavailable provider', isQuickChat: false, targetAvailable: false },
{ name: 'quick chat', isQuickChat: true, targetAvailable: true },
]) {
test(`New Session preserves ${scenario.name} inheritance with toSide=${toSide}`, async () => {
const instantiationService = disposables.add(new TestInstantiationService());
const { session } = createTestSession('active');
const activeSession = upcastPartial<IActiveSession>({
...session,
isQuickChat: constObservable(scenario.isQuickChat),
});
const requests: (IOpenNewSessionOptions | undefined)[] = [];
instantiationService.stub(ISessionsService, new class extends mock<ISessionsService>() {
override readonly activeSession = constObservable(activeSession);
override async openNewSession(options?: IOpenNewSessionOptions): Promise<IOpenNewSessionResult> {
requests.push(options);
return { session: undefined, trustDeclined: false };
}
});
instantiationService.stub(ISessionsManagementService, new class extends mock<ISessionsManagementService>() {
override isNewSessionTargetAvailable(): boolean { return scenario.targetAvailable; }
});

const command = CommandsRegistry.getCommand(NEW_SESSION_ACTION_ID);
assert.ok(command);
await command.handler(instantiationService, toSide ? { toSide } : undefined);

assert.deepStrictEqual(requests, [{
folderUri: scenario.isQuickChat ? undefined : session.workspace.get()?.uri,
toSide,
...(!scenario.isQuickChat && scenario.targetAvailable ? {
providerId: session.providerId,
sessionTypeId: session.sessionType,
} : {}),
}]);
});
}
}

test('New Session propagates opening failures', async () => {
const instantiationService = disposables.add(new TestInstantiationService());
const error = new Error('Opening failed');
instantiationService.stub(ISessionsService, new class extends mock<ISessionsService>() {
override readonly activeSession = constObservable(undefined);
override async openNewSession(): Promise<IOpenNewSessionResult> {
throw error;
}
});
instantiationService.stub(ISessionsManagementService, new class extends mock<ISessionsManagementService>() { });
const command = CommandsRegistry.getCommand(NEW_SESSION_ACTION_ID);
assert.ok(command);
await assert.rejects(async () => command.handler(instantiationService, { toSide: true }), error);
});
});
40 changes: 32 additions & 8 deletions src/vs/sessions/services/sessions/browser/sessionsService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@ export interface IOpenNewSessionOptions extends ICreateNewSessionOptions {
readonly folderUri?: URI;
/** Cancel startup session restoration so this new-session navigation wins. */
readonly cancelRestore?: boolean;

/**
* When `true`, opens the new session (or empty composer slot) to the side
* of the active session in the grid instead of replacing it in place.
*/
readonly toSide?: boolean;
}

/**
Expand Down Expand Up @@ -270,9 +276,10 @@ export interface ISessionsService {

/**
* Insert (or move) a session into the grid positioned next to a target
* session that is already visible.
* session that is already visible. Passing `undefined` operates on the
* empty (new-session) slot.
*/
insertAt(session: ISession, targetSessionId: string, side: 'left' | 'right', activate?: boolean): void;
insertAt(session: ISession | undefined, targetSessionId: string, side: 'left' | 'right', activate?: boolean): void;

/**
* Toggle a session's stickiness in the grid. The session keeps its grid
Expand Down Expand Up @@ -1085,7 +1092,7 @@ export class SessionsService extends Disposable implements ISessionsService {
this._startOpenSession();
try {
const session = this.sessionsManagementService.createNewSession(folderUri, options);
this._activate(session);
this._activateOrInsert(session, options?.toSide);
return { session, trustDeclined: false };
} catch (e) {
// When the folder cannot be resolved (e.g. the active session's
Expand All @@ -1097,11 +1104,11 @@ export class SessionsService extends Disposable implements ISessionsService {

// Without a folder (or when folder resolution failed above): switch to
// the new-session composer view.
// No-op when no session is active (empty new-session placeholder showing).
// No-op when the empty new-session placeholder is active, unless opening to the side.
if (!folderUri) {
this._dismissCustomViewForNavigation(intent);
}
if (this._visibility.activeSession.get() === undefined) {
if (this._visibility.activeSession.get() === undefined && !options?.toSide) {
return { session: undefined, trustDeclined: false };
}
if (!folderUri) {
Expand All @@ -1113,8 +1120,25 @@ export class SessionsService extends Disposable implements ISessionsService {
// active session (first time / after send).
const newSession = this.sessionsManagementService.newSession.get();

this._activate(newSession ?? undefined);
return { session: newSession ?? undefined, trustDeclined: false };
const targetSession = newSession ?? undefined;
this._activateOrInsert(targetSession, options?.toSide);
return { session: targetSession, trustDeclined: false };
}

/** Open or move beside the active session when requested, keeping a single empty slot. */
private _activateOrInsert(session: ISession | undefined, toSide: boolean | undefined): void {
const activeSessionId = this._visibility.activeSession.get()?.sessionId;
const sessionId = session?.sessionId;
if (toSide && activeSessionId !== sessionId) {
const visible = this.visibleSessions.get();
// An empty active slot has no id; fall back to the rightmost session.
const anchorId = activeSessionId ?? visible[visible.length - 1]?.sessionId;
if (anchorId && anchorId !== sessionId) {
this.insertAt(session, anchorId, 'right', true);
return;
}
}
this._activate(session);
}

openQuickChat(options?: ICreateNewSessionOptions): IActiveSession | undefined {
Expand Down Expand Up @@ -1178,7 +1202,7 @@ export class SessionsService extends Disposable implements ISessionsService {
this._onDidToggleSessionStickiness.fire({ session, sticky });
}

insertAt(session: ISession, targetSessionId: string, side: 'left' | 'right', activate: boolean = true): void {
insertAt(session: ISession | undefined, targetSessionId: string, side: 'left' | 'right', activate: boolean = true): void {
this._visibility.insertAt(session, targetSessionId, side, activate);
}

Expand Down
12 changes: 3 additions & 9 deletions src/vs/sessions/services/sessions/browser/visibleSessions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -472,10 +472,10 @@ export class VisibleSessions extends Disposable {
* "open at position" entry points.
*
* - If the slot is not yet visible, a new non-sticky entry is created
* at the computed position. For an `undefined` session (empty slot),
* this is a no-op when an empty slot already exists in the grid.
* at the computed position.
* - If the slot is already visible, it is moved to the computed
* position; its sticky / non-sticky state is preserved.
* position; its sticky / non-sticky state is preserved. This also
* moves the single empty slot when `session` is `undefined`.
*
* When `activate` is `true` (default), the inserted slot also becomes
* the active session. When `false`, the active session is left
Expand All @@ -491,12 +491,6 @@ export class VisibleSessions extends Disposable {
return;
}

// Invariant: at most one empty slot. If inserting the empty slot and
// one already exists, do not add or move another.
if (id === undefined && this._visibleList.includes(undefined)) {
return;
}

let destIdx = side === 'left' ? targetIdx : targetIdx + 1;

const currentIdx = this._visibleList.indexOf(id);
Expand Down
Loading