From 76396dfbb7e36088f4d28bf16ae17fb8d720b7f1 Mon Sep 17 00:00:00 2001 From: Connor Peet Date: Sat, 25 Jul 2026 07:44:32 -0700 Subject: [PATCH 1/2] chat: ask side questions from selected responses Reuse the feedback input affordance in the Agents window to create side chats from selected assistant markdown, with accessible pending state and Agent Host context coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/skills/sessions/SKILL.md | 4 + ...source-context-without-copied-history.yaml | 30 +- ...source-context-without-copied-history.yaml | 24 +- .../test/node/e2e/suites/multiChatSuite.ts | 17 +- src/vs/sessions/SESSIONS.md | 73 +++ .../agentFeedbackEditorInputContribution.ts | 179 ++------ .../browser/feedbackInputWidget.ts | 232 ++++++++++ .../media/agentFeedbackEditorInput.css | 25 ++ .../test/browser/feedbackInputWidget.test.ts | 112 +++++ .../browser/btwSlashCommand.contribution.ts | 4 +- .../sessions/contrib/chat/browser/chatView.ts | 7 + .../chat/browser/responseSelectionResolver.ts | 65 +++ .../responseSelectionSideChatController.ts | 221 +++++++++ .../chat/browser/sideChatOrchestration.ts | 44 ++ .../browser/responseSelectionResolver.test.ts | 163 +++++++ ...esponseSelectionSideChatController.test.ts | 420 ++++++++++++++++++ .../browser/actions/chatAccessibilityHelp.ts | 9 +- src/vs/workbench/contrib/chat/browser/chat.ts | 4 + .../chat/browser/widget/chatListWidget.ts | 7 + .../contrib/chat/browser/widget/chatWidget.ts | 4 + .../chatAccessibilityHelp.test.ts | 14 + 21 files changed, 1482 insertions(+), 176 deletions(-) create mode 100644 src/vs/sessions/contrib/agentFeedback/browser/feedbackInputWidget.ts create mode 100644 src/vs/sessions/contrib/agentFeedback/test/browser/feedbackInputWidget.test.ts create mode 100644 src/vs/sessions/contrib/chat/browser/responseSelectionResolver.ts create mode 100644 src/vs/sessions/contrib/chat/browser/responseSelectionSideChatController.ts create mode 100644 src/vs/sessions/contrib/chat/browser/sideChatOrchestration.ts create mode 100644 src/vs/sessions/contrib/chat/test/browser/responseSelectionResolver.test.ts create mode 100644 src/vs/sessions/contrib/chat/test/browser/responseSelectionSideChatController.test.ts diff --git a/.github/skills/sessions/SKILL.md b/.github/skills/sessions/SKILL.md index 122dd30cda1e3..8728a27a7d15c 100644 --- a/.github/skills/sessions/SKILL.md +++ b/.github/skills/sessions/SKILL.md @@ -134,6 +134,10 @@ Whenever the user flags a wrong pattern, rejects an approach, or gives design/ru - **`DetailPanelController` must not hide the detail on an empty editor group in the new-session view**: `_computeTarget` returns `Hidden` when the main editor part is empty (a created session's all-tabs-closed → whole side pane closed). But the new-session (uncreated) view's editor group is *transiently* empty while its Files tab is (re)ensured, and its Files detail is open by default and owned by the layout controller's D3b. Gating the empty-group `Hidden` on `activeSession.isCreated` avoids a transient hide that the D2 visibility listener would otherwise capture as the new-session preference — making every subsequent `cmd+n` open with the side pane hidden. Combined with the editor-visibility reveal gate, the new-session default stays open while a user's explicit hide is still remembered (D3b `_newSessionViewState`). +- **Don't mirror an environment fact onto a shared widget API**: an Agents-window check (e.g. "is this the sessions window?") belongs on `IWorkbenchEnvironmentService.isSessionsWindow`, read directly by the consumer that needs it (e.g. `getChatAccessibilityHelpProvider` in `chatAccessibilityHelp.ts` injecting the service itself). Do not add an `isSessionsWindow`-style property to a shared interface like `IChatWidget` just to thread that fact through — it leaks a sessions-specific concept into shared workbench chat surfaces and every future consumer would need the same plumbing instead of injecting the service once. + +- **A view-lifecycle `setChat`/`setModel`-style hook can be re-invoked for the *same* underlying resource**: `ChatView.setChat` fires again on unrelated status/interactivity observable changes, not only on a genuine view swap. A consumer like `ResponseSelectionSideChatController` that force-dismisses its own transient UI on every call discards an in-progress draft and, worse, clears a pending busy submission mid-flight, letting a duplicate submission race in. Compare the incoming resource against the previously tracked one and only treat a genuine change (or the first call) as dismiss-worthy; a same-resource re-invocation must preserve visible/busy state. + ## Validating Changes You **must** run these checks before declaring work complete: diff --git a/src/vs/platform/agentHost/test/node/e2e/captures/claude-side-chat-receives-bounded-source-context-without-copied-history.yaml b/src/vs/platform/agentHost/test/node/e2e/captures/claude-side-chat-receives-bounded-source-context-without-copied-history.yaml index 6d18438460055..6f472918e9fe0 100644 --- a/src/vs/platform/agentHost/test/node/e2e/captures/claude-side-chat-receives-bounded-source-context-without-copied-history.yaml +++ b/src/vs/platform/agentHost/test/node/e2e/captures/claude-side-chat-receives-bounded-source-context-without-copied-history.yaml @@ -8,11 +8,11 @@ exchanges: - role: user content: Remember the exact token SIDECHAT42 for a later question. Reply with exactly "ready". response: - content: ready + content: |- + I'll remember the token SIDECHAT42. + + ready stopReason: end_turn - usage: - inputTokens: 2686 - outputTokens: 4 - request: model: claude-opus-4.8 system: ${system} @@ -20,12 +20,22 @@ exchanges: - role: user content: Remember the exact token SIDECHAT42 for a later question. Reply with exactly "ready". - role: assistant - content: ready + content: |- + I'll remember the token SIDECHAT42. + + ready - role: user - content: What exact token did I ask you to remember? Reply with only the token. + content: |- + + length=159 + This is a side conversation. Prefer explanation over action; do not make changes or carry out work unless the user explicitly asks. + + Selected text: + + MOONVALE99 + + + Reply with the exact remembered token, then a space, then the exact selected text given to you as context — nothing else. response: - content: SIDECHAT42 + content: SIDECHAT42 MOONVALE99 stopReason: end_turn - usage: - inputTokens: 2 - outputTokens: 10 diff --git a/src/vs/platform/agentHost/test/node/e2e/captures/copilotcli-side-chat-receives-bounded-source-context-without-copied-history.yaml b/src/vs/platform/agentHost/test/node/e2e/captures/copilotcli-side-chat-receives-bounded-source-context-without-copied-history.yaml index 2507af9915fbf..49ee218285b10 100644 --- a/src/vs/platform/agentHost/test/node/e2e/captures/copilotcli-side-chat-receives-bounded-source-context-without-copied-history.yaml +++ b/src/vs/platform/agentHost/test/node/e2e/captures/copilotcli-side-chat-receives-bounded-source-context-without-copied-history.yaml @@ -2,7 +2,7 @@ version: 1 dialect: anthropic exchanges: - request: - model: claude-haiku-4.5 + model: claude-sonnet-5 system: ${system} messages: - role: user @@ -10,11 +10,8 @@ exchanges: response: content: ready stopReason: end_turn - usage: - inputTokens: 9 - outputTokens: 65 - request: - model: claude-haiku-4.5 + model: claude-sonnet-5 system: ${system} messages: - role: user @@ -22,10 +19,17 @@ exchanges: - role: assistant content: ready - role: user - content: What exact token did I ask you to remember? Reply with only the token. + content: |- + + length=159 + This is a side conversation. Prefer explanation over action; do not make changes or carry out work unless the user explicitly asks. + + Selected text: + + MOONVALE99 + + + Reply with the exact remembered token, then a space, then the exact selected text given to you as context — nothing else. response: - content: SIDECHAT42 + content: SIDECHAT42 MOONVALE99 stopReason: end_turn - usage: - inputTokens: 9 - outputTokens: 61 diff --git a/src/vs/platform/agentHost/test/node/e2e/suites/multiChatSuite.ts b/src/vs/platform/agentHost/test/node/e2e/suites/multiChatSuite.ts index 04856a668245f..561638db6e344 100644 --- a/src/vs/platform/agentHost/test/node/e2e/suites/multiChatSuite.ts +++ b/src/vs/platform/agentHost/test/node/e2e/suites/multiChatSuite.ts @@ -47,7 +47,7 @@ export function defineMultiChatTests(context: IAgentHostE2ETestContext): void { return { sessionUri, defaultChatUri: buildDefaultChatUri(sessionUri), workspace }; } - async function createPeer(sessionUri: string, id: string, source?: { chat: string; turnId: string; kind: ChatSourceKind }): Promise { + async function createPeer(sessionUri: string, id: string, source?: { chat: string; turnId: string; kind: ChatSourceKind; selection?: { text: string; responsePartId?: string } }): Promise { const chat = buildChatUri(sessionUri, id); await context.client.call('createChat', { channel: sessionUri, @@ -630,14 +630,17 @@ export function defineMultiChatTests(context: IAgentHostE2ETestContext): void { const { sessionUri, defaultChatUri } = await createSession('side-context'); await driveTurn(defaultChatUri, 'turn-source', 'Remember the exact token SIDECHAT42 for a later question. Reply with exactly "ready".', 1); + const selection = { text: 'MOONVALE99', responsePartId: 'response-part-source-1' }; const sideChatUri = await createPeer(sessionUri, 'side', { kind: ChatSourceKind.SideChat, chat: defaultChatUri, turnId: 'turn-source', + selection, }); await context.client.call('subscribe', { channel: sideChatUri }); - const response = await driveTurn(sideChatUri, 'turn-side', 'What exact token did I ask you to remember? Reply with only the token.', 2); + const question = 'Reply with the exact remembered token, then a space, then the exact selected text given to you as context — nothing else.'; + const response = await driveTurn(sideChatUri, 'turn-side', question, 2); const [sourceState, sideState, session] = await Promise.all([ chatState(defaultChatUri), chatState(sideChatUri), @@ -645,18 +648,20 @@ export function defineMultiChatTests(context: IAgentHostE2ETestContext): void { ]); assert.deepStrictEqual({ - responseIncludesCode: /SIDECHAT42/i.test(response), + responseIncludesRememberedToken: /SIDECHAT42/i.test(response), + responseIncludesSelectedText: /MOONVALE99/i.test(response), sourceTurnCount: sourceState.turns.length, sideTurnCount: sideState.turns.length, origin: session.chats.find(chat => chat.resource === sideChatUri)?.origin, firstMessage: sideState.turns[0]?.message.text, firstAttachments: sideState.turns[0]?.message.attachments ?? [], }, { - responseIncludesCode: true, + responseIncludesRememberedToken: true, + responseIncludesSelectedText: true, sourceTurnCount: 1, sideTurnCount: 1, - origin: { kind: ChatOriginKind.SideChat, chat: defaultChatUri, turnId: 'turn-source' }, - firstMessage: 'What exact token did I ask you to remember? Reply with only the token.', + origin: { kind: ChatOriginKind.SideChat, chat: defaultChatUri, turnId: 'turn-source', selection }, + firstMessage: question, firstAttachments: [], }); }, config.supportsMultipleChats && !!config.supportsSideChats); diff --git a/src/vs/sessions/SESSIONS.md b/src/vs/sessions/SESSIONS.md index fb07c5c2a0fa4..545d9f120f544 100644 --- a/src/vs/sessions/SESSIONS.md +++ b/src/vs/sessions/SESSIONS.md @@ -601,6 +601,79 @@ channel that received `ChatToolCallStart`/`ChatToolCallReady`; confirmations sen to the parent session URI are invalid and will not resolve the SDK permission request. +##### Direct selection invocation (Agents window only) + +Besides typing `/btw`, a user can select assistant markdown text in a chat +response and get an inline "Ask Question" affordance that creates the same +kind of side chat directly from that selection. This is Agents-window-only — +it never appears in the regular workbench chat surface — and reuses +`ISessionsManagementService.createSideChatInSession`/`sendRequest` and +`ISessionsService.openChat`, the identical plumbing `/btw` uses (see +`sideChatOrchestration.ts`'s `createAndSendSideChat`/`openAndSendSideChat` +helpers, shared by both entry points). + +`ResponseSelectionSideChatController` (`contrib/chat/browser/`) is owned by +`ChatView` per chat widget. It listens for `selectionchange` on the widget's +document and resolves the selection via `resolveResponseSelection` +(`responseSelectionResolver.ts`), which only accepts a selection when: +- both selection endpoints fall inside the **same** assistant response + (resolved through `IChatWidget.getElementFromNode`, `isResponseVM`), and +- the selection stays within that response's rendered markdown + (`.chat-markdown-part`), excluding any embedded Monaco editor + (`.monaco-editor`) or tool-invocation UI (`.chat-tool-invocation-part`). + +A resolved selection shows an "Ask Question" input positioned under the +selection, reusing the same visual/input component as the editor's feedback +affordance: `FeedbackInputWidget` (`contrib/agentFeedback/browser/ +feedbackInputWidget.ts`), extracted from `AgentFeedbackInputWidget` so both +consumers share one textarea/action-bar implementation. Submitting creates a +side chat anchored to the **selected response's turn** +(`IChatResponseViewModel.requestId`, not the chat's last turn) with +`selection.text` set to the exact selected text, mirroring `/btw`'s +"inherits model/agent, immutable selection snapshot" semantics. The same +runtime capability/status gate as `/btw` applies before creating the side +chat (`session.capabilities.get().supportsSideChat`, not `Untitled`, not +archived); failing that gate shows a warning notification instead of +creating a partially-supported side chat. + +Submitting does not eagerly dismiss the overlay: it stays visible with a busy +state (`FeedbackInputWidget.setBusy(true, statusLabel)` — disabled input, +hidden action bar, a spinning `Codicon.loading` indicator, `aria-busy` plus an +`aria.status` announcement) while the create/open/send orchestration is +in-flight, and duplicate submission is blocked both by the disabled action and +an explicit `isBusy` guard in `_submit`. Opening the newly-created side chat +naturally dismisses the overlay via `setChat`, which force-dismisses only when +the chat's *resource* actually changes: `ChatView` re-invokes `setChat` for the +same chat on unrelated status/interactivity observable updates, so a +same-resource call must preserve a visible draft and, critically, a pending +busy submission rather than clearing it and letting a duplicate race in. On +failure the busy state clears, the typed question and normal controls are +restored, and the input is refocused so the user can retry without losing +their text — the existing warning/error notification and log call are +unchanged. Escape, scrolling, and selection invalidation are all ignored while +a submission is pending so they cannot race the in-flight request. The +action-bar slot and the spinner that replaces it both size to a shared +`--agent-feedback-line-height` CSS custom property (set once on the widget +root, matching the textarea's line-height) and center via flex, rather than a +hardcoded icon height or a positional transform — this keeps both optically +centered on the input's single line, and still flush to the last line when the +textarea grows multi-line (the row keeps `align-items: flex-end`). + +The `selectionchange` listener ignores events entirely while focus is inside +the "Ask Question" input (`dom.isAncestorOfActiveElement`): focusing the +textarea collapses the browser's native document selection as a side effect, +and without this guard that collapse would dismiss the very input the user +just focused. The captured selection is treated as an immutable snapshot for +that reason — it is not re-read from the live DOM selection on submit. +Escape (or any other dismissal while the input has focus) restores focus to +the source response via `IChatWidget.focusResponseItem(true)` rather than +letting it fall through to the document body. The overlay's position clamps +both horizontally and vertically against the chat widget's own bounds and the +visible viewport, measured after `FeedbackInputWidget.show()`/`autoSize()` so +real dimensions are used; when there isn't room below the selection it flips +above instead, falling back to the nearest in-bounds edge only when neither +placement fully fits. + Agent-host approval levels map to the Copilot SDK allow-all modes before each turn: Default approvals uses `off`, Allow all uses `on`, and Assisted permissions uses `auto`. Assisted permissions only skips a prompt when the SDK's diff --git a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorInputContribution.ts b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorInputContribution.ts index bb34b613afeca..4dc283d84c020 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorInputContribution.ts +++ b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorInputContribution.ts @@ -14,24 +14,22 @@ import { EditorOption } from '../../../../editor/common/config/editorOptions.js' import { Position } from '../../../../editor/common/core/position.js'; import { Range } from '../../../../editor/common/core/range.js'; import { Selection, SelectionDirection } from '../../../../editor/common/core/selection.js'; -import { addStandardDisposableListener, getWindow, isHTMLElement, ModifierKeyEmitter } from '../../../../base/browser/dom.js'; +import { addStandardDisposableListener, getWindow, isHTMLElement } from '../../../../base/browser/dom.js'; import { URI } from '../../../../base/common/uri.js'; import { isEqual } from '../../../../base/common/resources.js'; import { KeyCode } from '../../../../base/common/keyCodes.js'; import { IAgentFeedbackService } from './agentFeedbackService.js'; import { createAgentFeedbackContext } from './agentFeedbackEditorUtils.js'; import { localize, localize2 } from '../../../../nls.js'; -import { ActionBar } from '../../../../base/browser/ui/actionbar/actionbar.js'; -import { Action } from '../../../../base/common/actions.js'; import { Codicon } from '../../../../base/common/codicons.js'; -import { ThemeIcon } from '../../../../base/common/themables.js'; -import { Emitter, Event } from '../../../../base/common/event.js'; +import { Event } from '../../../../base/common/event.js'; import { ISession } from '../../../services/sessions/common/session.js'; import { Action2, MenuId, registerAction2 } from '../../../../platform/actions/common/actions.js'; import { ContextKeyExpr, IContextKey, IContextKeyService, RawContextKey } from '../../../../platform/contextkey/common/contextkey.js'; import { ServicesAccessor } from '../../../../platform/instantiation/common/instantiation.js'; import { ChatContextKeys } from '../../../../workbench/contrib/chat/common/actions/chatContextKeys.js'; import { CHAT_CATEGORY } from '../../../../workbench/contrib/chat/browser/actions/chatActions.js'; +import { FeedbackInputWidget } from './feedbackInputWidget.js'; const addFeedbackAtCurrentLineActionId = 'agentFeedbackEditor.action.addAtCurrentLine'; const agentFeedbackHoverGlyphClassName = 'agent-feedback-glyph'; @@ -40,113 +38,41 @@ const hasAgentFeedbackSessionForEditor = new RawContextKey('agentFeedba /** * The inline "Add Feedback" input shown in the editor when the user selects a * range to comment on. Exported so it can be rendered in a component fixture; - * it only depends on {@link ICodeEditor} for its layout geometry. + * it only depends on {@link ICodeEditor} for its layout geometry. Wraps the + * reusable {@link FeedbackInputWidget} core as an {@link IOverlayWidget}. */ export class AgentFeedbackInputWidget extends Disposable implements IOverlayWidget { private static readonly _ID = 'agentFeedback.inputWidget'; - private static readonly _MIN_WIDTH = 150; - private static readonly _MAX_WIDTH = 400; - // The input should never be wider than the editor itself. Cap it to this - // fraction of the editor width so it doesn't render past the editor bounds - // on narrow editors. - private static readonly _MAX_WIDTH_EDITOR_FRACTION = 0.9; readonly allowEditorOverflow = false; - private readonly _domNode: HTMLElement; - private readonly _inputElement: HTMLTextAreaElement; - private readonly _measureElement: HTMLElement; - private readonly _actionBar: ActionBar; - private readonly _addAction: Action; - private readonly _addAndSubmitAction: Action; + private readonly _core: FeedbackInputWidget; private _position: IOverlayWidgetPosition | null = null; - private _lineHeight = 0; - private readonly _onDidTriggerAdd = this._register(new Emitter()); - readonly onDidTriggerAdd: Event = this._onDidTriggerAdd.event; - - private readonly _onDidTriggerAddAndSubmit = this._register(new Emitter()); - readonly onDidTriggerAddAndSubmit: Event = this._onDidTriggerAddAndSubmit.event; + readonly onDidTriggerAdd: Event; + readonly onDidTriggerAddAndSubmit: Event; constructor( private readonly _editor: ICodeEditor, ) { super(); - this._domNode = document.createElement('div'); - this._domNode.classList.add('agent-feedback-input-widget'); - this._domNode.style.display = 'none'; - - this._inputElement = document.createElement('textarea'); - this._inputElement.rows = 1; - this._inputElement.placeholder = localize('agentFeedback.addFeedback', "Add Feedback"); - this._domNode.appendChild(this._inputElement); - - // Hidden element used to measure text width for auto-growing - this._measureElement = document.createElement('span'); - this._measureElement.classList.add('agent-feedback-input-measure'); - this._domNode.appendChild(this._measureElement); - - // Action bar with add/submit actions - const actionsContainer = document.createElement('div'); - actionsContainer.classList.add('agent-feedback-input-actions'); - this._domNode.appendChild(actionsContainer); - - this._addAction = this._register(new Action( - 'agentFeedback.add', - localize('agentFeedback.add', "Add Feedback"), - ThemeIcon.asClassName(Codicon.plus), - false, - () => { this._onDidTriggerAdd.fire(); return Promise.resolve(); } - )); - - this._addAndSubmitAction = this._register(new Action( - 'agentFeedback.addAndSubmit', - localize('agentFeedback.addAndSubmit', "Add Feedback and Submit"), - ThemeIcon.asClassName(Codicon.send), - false, - () => { this._onDidTriggerAddAndSubmit.fire(); return Promise.resolve(); } - )); - - this._actionBar = this._register(new ActionBar(actionsContainer)); - this._actionBar.push(this._addAction, { icon: true, label: false, keybinding: localize('enter', "Enter") }); - - // Toggle to alt action when Alt key is held - const modifierKeyEmitter = ModifierKeyEmitter.getInstance(); - this._register(modifierKeyEmitter.event(status => { - this._updateActionForAlt(status.altKey); - })); - - // Focus the input when clicking anywhere on the widget that isn't the - // textarea itself or the action bar (e.g. padding around the textarea). - this._register(addStandardDisposableListener(this._domNode, 'mousedown', e => { - const target = e.target as Node | null; - if (target === this._inputElement) { - return; - } - if (actionsContainer.contains(target)) { - return; - } - e.preventDefault(); - this._inputElement.focus(); + this._core = this._register(new FeedbackInputWidget({ + placeholder: localize('agentFeedback.addFeedback', "Add Feedback"), + getMaxContentWidth: () => this._computeContentWidth(), + primaryAction: { + label: localize('agentFeedback.add', "Add Feedback"), + icon: Codicon.plus, + keybindingLabel: localize('enter', "Enter"), + }, + secondaryAction: { + label: localize('agentFeedback.addAndSubmit', "Add Feedback and Submit"), + icon: Codicon.send, + keybindingLabel: localize('altEnter', "Alt+Enter"), + }, })); - - this._lineHeight = 22; - this._inputElement.style.lineHeight = `${this._lineHeight}px`; - } - - private _isShowingAlt = false; - - private _updateActionForAlt(altKey: boolean): void { - if (altKey && !this._isShowingAlt) { - this._isShowingAlt = true; - this._actionBar.clear(); - this._actionBar.push(this._addAndSubmitAction, { icon: true, label: false, keybinding: localize('altEnter', "Alt+Enter") }); - } else if (!altKey && this._isShowingAlt) { - this._isShowingAlt = false; - this._actionBar.clear(); - this._actionBar.push(this._addAction, { icon: true, label: false, keybinding: localize('enter', "Enter") }); - } + this.onDidTriggerAdd = this._core.onDidTriggerPrimary; + this.onDidTriggerAddAndSubmit = this._core.onDidTriggerSecondary; } getId(): string { @@ -154,7 +80,7 @@ export class AgentFeedbackInputWidget extends Disposable implements IOverlayWidg } getDomNode(): HTMLElement { - return this._domNode; + return this._core.domNode; } getPosition(): IOverlayWidgetPosition | null { @@ -162,7 +88,7 @@ export class AgentFeedbackInputWidget extends Disposable implements IOverlayWidg } get inputElement(): HTMLTextAreaElement { - return this._inputElement; + return this._core.inputElement; } setPosition(position: IOverlayWidgetPosition | null): void { @@ -171,75 +97,36 @@ export class AgentFeedbackInputWidget extends Disposable implements IOverlayWidg } show(): void { - this._domNode.style.display = ''; + this._core.show(); } hide(): void { - this._domNode.style.display = 'none'; + this._core.hide(); } clearInput(): void { - this._inputElement.value = ''; - this._updateActionEnabled(); - this._autoSize(); + this._core.clearInput(); } setPlaceholder(placeholder: string): void { - if (this._inputElement.placeholder === placeholder) { - return; - } - this._inputElement.placeholder = placeholder; - this._autoSize(); + this._core.setPlaceholder(placeholder); } autoSize(): void { - this._autoSize(); + this._core.autoSize(); } updateActionEnabled(): void { - this._updateActionEnabled(); - } - - private _updateActionEnabled(): void { - const hasText = this._inputElement.value.trim().length > 0; - this._addAction.enabled = hasText; - this._addAndSubmitAction.enabled = hasText; + this._core.updateActionEnabled(); } - private _autoSize(): void { - const text = this._inputElement.value || this._inputElement.placeholder; - - // Measure the text width using the hidden span - this._measureElement.textContent = text; - const textWidth = this._measureElement.scrollWidth; - - // Clamp width between min and a max that never exceeds the editor width. - // On very narrow editors the max can drop below the nominal minimum, so - // derive an effective minimum that never exceeds the max and apply it - // inline to override the CSS `min-width` (otherwise the textarea would be - // forced back up to its CSS minimum and overflow the editor). - const maxWidth = this._computeMaxWidth(); - const minWidth = Math.min(AgentFeedbackInputWidget._MIN_WIDTH, maxWidth); - const desiredWidth = Math.max(minWidth, textWidth + 10); - const width = Math.min(desiredWidth, maxWidth); - this._inputElement.style.minWidth = `${minWidth}px`; - this._inputElement.style.width = `${width}px`; - - // Reset height to auto then expand to fit all content, with a minimum of 1 line - this._inputElement.style.height = 'auto'; - const newHeight = Math.max(this._inputElement.scrollHeight, this._lineHeight); - this._inputElement.style.height = `${newHeight}px`; - } - - private _computeMaxWidth(): number { + private _computeContentWidth(): number { // The widget sticks to the editor's content left edge, so the space it // has available is the content area width (to the right of the line // numbers/glyph margin), not the full editor width. const layoutInfo = this._editor.getLayoutInfo(); - const contentWidth = Math.max(0, layoutInfo.width - layoutInfo.contentLeft); - return Math.min(AgentFeedbackInputWidget._MAX_WIDTH, contentWidth * AgentFeedbackInputWidget._MAX_WIDTH_EDITOR_FRACTION); + return Math.max(0, layoutInfo.width - layoutInfo.contentLeft); } - } export class AgentFeedbackEditorInputContribution extends Disposable implements IEditorContribution { diff --git a/src/vs/sessions/contrib/agentFeedback/browser/feedbackInputWidget.ts b/src/vs/sessions/contrib/agentFeedback/browser/feedbackInputWidget.ts new file mode 100644 index 0000000000000..6ff8c8bb38b90 --- /dev/null +++ b/src/vs/sessions/contrib/agentFeedback/browser/feedbackInputWidget.ts @@ -0,0 +1,232 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import './media/agentFeedbackEditorInput.css'; +import { addStandardDisposableListener, ModifierKeyEmitter } from '../../../../base/browser/dom.js'; +import { status as announceStatus } from '../../../../base/browser/ui/aria/aria.js'; +import { ActionBar } from '../../../../base/browser/ui/actionbar/actionbar.js'; +import { Action } from '../../../../base/common/actions.js'; +import { Codicon } from '../../../../base/common/codicons.js'; +import { ThemeIcon } from '../../../../base/common/themables.js'; +import { Emitter, Event } from '../../../../base/common/event.js'; +import { Disposable } from '../../../../base/common/lifecycle.js'; + +export interface IFeedbackInputWidgetAction { + readonly label: string; + readonly icon: ThemeIcon; + readonly keybindingLabel: string; +} + +export interface IFeedbackInputWidgetOptions { + readonly placeholder: string; + readonly ariaLabel?: string; + /** Returns the available content width (e.g. editor content width, or a host container's width) to clamp against. */ + readonly getMaxContentWidth: () => number; + readonly primaryAction: IFeedbackInputWidgetAction; + /** When provided, holding Alt swaps the visible action to this one. */ + readonly secondaryAction?: IFeedbackInputWidgetAction; +} + +/** + * Reusable auto-sizing textarea + action bar shared by the editor "Add + * Feedback" overlay and the Agents-window response-selection "Ask Question" + * affordance. Positioning and hosting (overlay widget vs. absolute DOM child) + * are left to the caller; this only owns the DOM structure, sizing and input. + */ +export class FeedbackInputWidget extends Disposable { + + private static readonly _MIN_WIDTH = 150; + private static readonly _MAX_WIDTH = 400; + // The input should never be wider than its host. Cap it to this fraction of + // the available width so it doesn't render past the host bounds when narrow. + private static readonly _MAX_WIDTH_HOST_FRACTION = 0.9; + + private static readonly _LINE_HEIGHT = 22; + + readonly domNode: HTMLElement; + readonly inputElement: HTMLTextAreaElement; + private readonly _measureElement: HTMLElement; + private readonly _actionsContainer: HTMLElement; + private readonly _busyIndicator: HTMLElement; + private readonly _actionBar: ActionBar; + private readonly _primaryAction: Action; + private readonly _secondaryAction: Action | undefined; + private _isShowingSecondary = false; + private _busy = false; + + /** Whether {@link setBusy} is currently active; callers must not submit again while true. */ + get isBusy(): boolean { + return this._busy; + } + + private readonly _onDidTriggerPrimary = this._register(new Emitter()); + readonly onDidTriggerPrimary: Event = this._onDidTriggerPrimary.event; + + private readonly _onDidTriggerSecondary = this._register(new Emitter()); + readonly onDidTriggerSecondary: Event = this._onDidTriggerSecondary.event; + + constructor(private readonly _options: IFeedbackInputWidgetOptions) { + super(); + this.domNode = document.createElement('div'); + this.domNode.classList.add('agent-feedback-input-widget'); + this.domNode.style.display = 'none'; + + this.inputElement = document.createElement('textarea'); + this.inputElement.rows = 1; + this.inputElement.placeholder = _options.placeholder; + this.inputElement.setAttribute('aria-label', _options.ariaLabel ?? _options.placeholder); + this.inputElement.style.lineHeight = `${FeedbackInputWidget._LINE_HEIGHT}px`; + this.domNode.appendChild(this.inputElement); + + // Hidden element used to measure text width for auto-growing + this._measureElement = document.createElement('span'); + this._measureElement.classList.add('agent-feedback-input-measure'); + this.domNode.appendChild(this._measureElement); + + const actionsContainer = document.createElement('div'); + actionsContainer.classList.add('agent-feedback-input-actions'); + actionsContainer.style.height = `${FeedbackInputWidget._LINE_HEIGHT}px`; + this.domNode.appendChild(actionsContainer); + this._actionsContainer = actionsContainer; + + this._busyIndicator = document.createElement('div'); + this._busyIndicator.classList.add('agent-feedback-input-busy-indicator', ...ThemeIcon.asClassNameArray(Codicon.loading), 'codicon-modifier-spin'); + this._busyIndicator.setAttribute('aria-hidden', 'true'); + this._busyIndicator.style.display = 'none'; + this._busyIndicator.style.height = `${FeedbackInputWidget._LINE_HEIGHT}px`; + this.domNode.appendChild(this._busyIndicator); + + this._primaryAction = this._register(new Action( + 'feedbackInput.primary', + _options.primaryAction.label, + ThemeIcon.asClassName(_options.primaryAction.icon), + false, + () => { this._onDidTriggerPrimary.fire(); return Promise.resolve(); } + )); + + this._secondaryAction = _options.secondaryAction ? this._register(new Action( + 'feedbackInput.secondary', + _options.secondaryAction.label, + ThemeIcon.asClassName(_options.secondaryAction.icon), + false, + () => { this._onDidTriggerSecondary.fire(); return Promise.resolve(); } + )) : undefined; + + this._actionBar = this._register(new ActionBar(actionsContainer)); + this._actionBar.push(this._primaryAction, { icon: true, label: false, keybinding: _options.primaryAction.keybindingLabel }); + + if (this._secondaryAction) { + const modifierKeyEmitter = ModifierKeyEmitter.getInstance(); + this._register(modifierKeyEmitter.event(status => this._updateActionForAlt(status.altKey))); + } + + // Focus the input when clicking anywhere on the widget that isn't the + // textarea itself or the action bar (e.g. padding around the textarea). + this._register(addStandardDisposableListener(this.domNode, 'mousedown', e => { + const target = e.target as Node | null; + if (target === this.inputElement || actionsContainer.contains(target)) { + return; + } + e.preventDefault(); + this.inputElement.focus(); + })); + } + + private _updateActionForAlt(altKey: boolean): void { + if (!this._secondaryAction) { + return; + } + if (altKey && !this._isShowingSecondary) { + this._isShowingSecondary = true; + this._actionBar.clear(); + this._actionBar.push(this._secondaryAction, { icon: true, label: false, keybinding: this._options.secondaryAction!.keybindingLabel }); + } else if (!altKey && this._isShowingSecondary) { + this._isShowingSecondary = false; + this._actionBar.clear(); + this._actionBar.push(this._primaryAction, { icon: true, label: false, keybinding: this._options.primaryAction.keybindingLabel }); + } + } + + show(): void { + this.domNode.style.display = ''; + } + + hide(): void { + this.domNode.style.display = 'none'; + } + + clearInput(): void { + this.inputElement.value = ''; + this.updateActionEnabled(); + this.autoSize(); + } + + setPlaceholder(placeholder: string): void { + if (this.inputElement.placeholder === placeholder) { + return; + } + this.inputElement.placeholder = placeholder; + this.inputElement.setAttribute('aria-label', placeholder); + this.autoSize(); + } + + updateActionEnabled(): void { + const hasText = this.inputElement.value.trim().length > 0 && !this._busy; + this._primaryAction.enabled = hasText; + if (this._secondaryAction) { + this._secondaryAction.enabled = hasText; + } + } + + /** + * Toggles an accessible busy state: disables the input/actions, swaps the + * action bar for a spinning loading codicon, and (when turning busy on) + * announces `statusLabel` to screen readers via `aria-busy` + a status + * announcement. Idempotent; a no-op call does not re-announce. + */ + setBusy(busy: boolean, statusLabel?: string): void { + if (this._busy === busy) { + return; + } + this._busy = busy; + this.inputElement.disabled = busy; + this.inputElement.setAttribute('aria-busy', busy ? 'true' : 'false'); + this._actionsContainer.style.display = busy ? 'none' : ''; + this._busyIndicator.style.display = busy ? '' : 'none'; + this.updateActionEnabled(); + if (busy && statusLabel) { + announceStatus(statusLabel); + } + } + + autoSize(): void { + const text = this.inputElement.value || this.inputElement.placeholder; + + // Measure the text width using the hidden span + this._measureElement.textContent = text; + const textWidth = this._measureElement.scrollWidth; + + // Clamp width between min and a max that never exceeds the host width. + // On very narrow hosts the max can drop below the nominal minimum, so + // derive an effective minimum that never exceeds the max and apply it + // inline to override the CSS `min-width` (otherwise the textarea would be + // forced back up to its CSS minimum and overflow the host). + const maxWidth = this._computeMaxWidth(); + const minWidth = Math.min(FeedbackInputWidget._MIN_WIDTH, maxWidth); + const desiredWidth = Math.max(minWidth, textWidth + 10); + const width = Math.min(desiredWidth, maxWidth); + this.inputElement.style.minWidth = `${minWidth}px`; + this.inputElement.style.width = `${width}px`; + + // Reset height to auto then expand to fit all content, with a minimum of 1 line + this.inputElement.style.height = 'auto'; + const newHeight = Math.max(this.inputElement.scrollHeight, FeedbackInputWidget._LINE_HEIGHT); + this.inputElement.style.height = `${newHeight}px`; + } + + private _computeMaxWidth(): number { + return Math.min(FeedbackInputWidget._MAX_WIDTH, this._options.getMaxContentWidth() * FeedbackInputWidget._MAX_WIDTH_HOST_FRACTION); + } +} diff --git a/src/vs/sessions/contrib/agentFeedback/browser/media/agentFeedbackEditorInput.css b/src/vs/sessions/contrib/agentFeedback/browser/media/agentFeedbackEditorInput.css index 197b6293224a8..25833c34b6a69 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/media/agentFeedbackEditorInput.css +++ b/src/vs/sessions/contrib/agentFeedback/browser/media/agentFeedbackEditorInput.css @@ -75,6 +75,8 @@ .agent-feedback-input-widget .agent-feedback-input-actions { display: flex; align-items: center; + justify-content: center; + box-sizing: border-box; margin-left: 2px; flex-shrink: 0; } @@ -85,6 +87,29 @@ height: 16px; } +.agent-feedback-input-widget textarea:disabled { + opacity: 0.6; + cursor: default; +} + +.agent-feedback-input-widget .agent-feedback-input-busy-indicator { + display: inline-flex; + align-items: center; + justify-content: center; + width: 16px; + /* Same shared line-height box as .agent-feedback-input-actions, so the spinner replacing the send action centers identically to it. */ + box-sizing: border-box; + margin-left: 2px; + flex-shrink: 0; + color: var(--vscode-icon-foreground); +} + +@media (prefers-reduced-motion: reduce) { + .agent-feedback-input-widget .agent-feedback-input-busy-indicator { + animation: none; + } +} + .monaco-editor .line-numbers.agent-feedback-glyph { cursor: pointer; z-index: 10; diff --git a/src/vs/sessions/contrib/agentFeedback/test/browser/feedbackInputWidget.test.ts b/src/vs/sessions/contrib/agentFeedback/test/browser/feedbackInputWidget.test.ts new file mode 100644 index 0000000000000..3805477ebaab4 --- /dev/null +++ b/src/vs/sessions/contrib/agentFeedback/test/browser/feedbackInputWidget.test.ts @@ -0,0 +1,112 @@ +/*--------------------------------------------------------------------------------------------- + * 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 { DisposableStore, toDisposable } from '../../../../../base/common/lifecycle.js'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; +import { Codicon } from '../../../../../base/common/codicons.js'; +import { FeedbackInputWidget } from '../../browser/feedbackInputWidget.js'; + +suite('FeedbackInputWidget', () => { + const disposables = ensureNoDisposablesAreLeakedInTestSuite(); + + function createWidget() { + const store = disposables.add(new DisposableStore()); + const widget = store.add(new FeedbackInputWidget({ + placeholder: 'Ask Question', + getMaxContentWidth: () => 400, + primaryAction: { label: 'Ask', icon: Codicon.send, keybindingLabel: 'Enter' }, + })); + return widget; + } + + function actionsContainer(widget: FeedbackInputWidget): HTMLElement { + return widget.domNode.querySelector('.agent-feedback-input-actions')!; + } + + function busyIndicator(widget: FeedbackInputWidget): HTMLElement { + return widget.domNode.querySelector('.agent-feedback-input-busy-indicator')!; + } + + test('setBusy(true) disables the input, hides the action bar, and shows the spinner', () => { + const widget = createWidget(); + + widget.setBusy(true); + + assert.strictEqual(widget.isBusy, true); + assert.strictEqual(widget.inputElement.disabled, true); + assert.strictEqual(widget.inputElement.getAttribute('aria-busy'), 'true'); + assert.strictEqual(actionsContainer(widget).style.display, 'none'); + assert.notStrictEqual(busyIndicator(widget).style.display, 'none'); + }); + + test('setBusy(false) restores the input and action bar', () => { + const widget = createWidget(); + widget.setBusy(true); + + widget.setBusy(false); + + assert.strictEqual(widget.isBusy, false); + assert.strictEqual(widget.inputElement.disabled, false); + assert.strictEqual(widget.inputElement.getAttribute('aria-busy'), 'false'); + assert.strictEqual(actionsContainer(widget).style.display, ''); + assert.strictEqual(busyIndicator(widget).style.display, 'none'); + }); + + test('setBusy is idempotent and does not toggle disabled state again on a repeated call', () => { + const widget = createWidget(); + widget.setBusy(true); + widget.inputElement.disabled = false; // simulate external state to detect a redundant re-apply + + widget.setBusy(true); + + assert.strictEqual(widget.inputElement.disabled, false, 'a no-op setBusy call must not re-apply state'); + }); + + test('disables the primary action while busy even with text present', () => { + const widget = createWidget(); + widget.inputElement.value = 'hello'; + widget.updateActionEnabled(); + + widget.setBusy(true); + + // Re-invoking updateActionEnabled (as callers do on 'input') must keep it disabled while busy. + widget.updateActionEnabled(); + assert.strictEqual((widget as unknown as { _primaryAction: { enabled: boolean } })._primaryAction.enabled, false); + }); + + test('sizes the action bar and busy spinner to the shared single-line height for optical vertical centering', () => { + const widget = createWidget(); + document.body.appendChild(widget.domNode); + disposables.add(toDisposable(() => widget.domNode.remove())); + widget.show(); + + const lineHeight = widget.inputElement.style.lineHeight; + assert.ok(lineHeight.length > 0, 'the input line height must be configured'); + + // Neither box is positioned via a one-off JS transform/top hack; alignment + // comes entirely from the shared flex/line-height CSS pattern below. + assert.strictEqual(actionsContainer(widget).style.transform, ''); + assert.strictEqual(actionsContainer(widget).style.top, ''); + assert.strictEqual(busyIndicator(widget).style.transform, ''); + assert.strictEqual(busyIndicator(widget).style.top, ''); + + // Both boxes resolve to the shared line-height (not the smaller 16px icon size) so they center identically. + const win = document.defaultView!; + assert.strictEqual(win.getComputedStyle(actionsContainer(widget)).height, lineHeight); + const actionsRect = actionsContainer(widget).getBoundingClientRect(); + + widget.setBusy(true); + assert.strictEqual(win.getComputedStyle(busyIndicator(widget)).height, lineHeight); + + // The spinner occupies the exact same box (same top and height) that the + // action bar it replaces did — the optical-centering fix reported as "the + // busy spinner is slightly too low" — not a smaller box flush to the bottom. + const busyRect = busyIndicator(widget).getBoundingClientRect(); + assert.ok(actionsRect.height > 0, 'sanity: the action bar must have measured a non-zero height while visible'); + assert.strictEqual(busyRect.top, actionsRect.top); + assert.strictEqual(busyRect.height, actionsRect.height); + }); +}); diff --git a/src/vs/sessions/contrib/chat/browser/btwSlashCommand.contribution.ts b/src/vs/sessions/contrib/chat/browser/btwSlashCommand.contribution.ts index afb0507a23650..91059d261f8fd 100644 --- a/src/vs/sessions/contrib/chat/browser/btwSlashCommand.contribution.ts +++ b/src/vs/sessions/contrib/chat/browser/btwSlashCommand.contribution.ts @@ -20,6 +20,7 @@ import { ISessionsService } from '../../../services/sessions/browser/sessionsSer import { ISessionsManagementService } from '../../../services/sessions/common/sessionsManagement.js'; import { SessionIsArchivedContext, SessionIsCreatedContext, SessionSupportsSideChatContext } from '../../../common/contextkeys.js'; import { ISideChatSelection, SessionStatus } from '../../../services/sessions/common/session.js'; +import { openAndSendSideChat } from './sideChatOrchestration.js'; function captureSideChatSelection(widget: IChatWidgetService['lastFocusedWidget']): ISideChatSelection | undefined { if (!widget) { @@ -109,8 +110,7 @@ export class BtwSlashCommandContribution extends Disposable implements IWorkbenc return; } - await sessionsService.openChat(session, sideChat.resource); - await sessionsManagementService.sendRequest(session, sideChat, { query: remainder }); + await openAndSendSideChat(sessionsService, sessionsManagementService, session, sideChat, remainder); })); } } diff --git a/src/vs/sessions/contrib/chat/browser/chatView.ts b/src/vs/sessions/contrib/chat/browser/chatView.ts index 350997f51a1d5..2a22c7eccf995 100644 --- a/src/vs/sessions/contrib/chat/browser/chatView.ts +++ b/src/vs/sessions/contrib/chat/browser/chatView.ts @@ -32,6 +32,7 @@ import { NewChatWidget } from './newChatWidget.js'; import { NewChatInSessionWidget } from './newChatInSessionWidget.js'; import { SessionInputBanners } from '../../sessionInputBanners/browser/sessionInputBanners.js'; import { SessionChatInputToolbar } from './sessionChatInputToolbar.js'; +import { ResponseSelectionSideChatController } from './responseSelectionSideChatController.js'; import { ISessionChatPillsDebugService } from './sessionChatInputToolbarDebug.js'; import { AGENT_SESSIONS_SCOPED_INPUT_HISTORY_SETTING } from './sessionsChatHistory.js'; import { activeSessionViewBackground, activeSessionViewForeground, agentsPanelBackground, inactiveSessionViewBackground, inactiveSessionViewForeground } from '../../../common/theme.js'; @@ -118,6 +119,9 @@ export class ChatView extends AbstractChatView { /** Floating status pills (changes, preview, background activity) above the input. */ private readonly _chatPills: SessionChatInputToolbar; + /** Shows an "Ask Question" input when the user selects assistant markdown text. */ + private readonly _selectionSideChatController: ResponseSelectionSideChatController; + /** Reference to the loaded chat model; disposing releases the model. */ private readonly _modelRef = this._register(new MutableDisposable()); @@ -190,6 +194,8 @@ export class ChatView extends AbstractChatView { this._widget.render(this.element); this._widget.setVisible(true); + this._selectionSideChatController = this._register(scopedInstantiationService.createInstance(ResponseSelectionSideChatController, this._widget)); + // Mount the session banners directly above the chat input. this._banners = this._register(instantiationService.createInstance(SessionInputBanners)); this._banners.setActive(this._isActive); @@ -245,6 +251,7 @@ export class ChatView extends AbstractChatView { // Reflect this chat's last-turn changes, status, and background activity. this._chatPills.setChat(chat); + this._selectionSideChatController.setChat(chat); this._banners.setDebugData(undefined); // Reflect read-only (non-interactive) chats: hide the composer and gate diff --git a/src/vs/sessions/contrib/chat/browser/responseSelectionResolver.ts b/src/vs/sessions/contrib/chat/browser/responseSelectionResolver.ts new file mode 100644 index 0000000000000..6176962b92283 --- /dev/null +++ b/src/vs/sessions/contrib/chat/browser/responseSelectionResolver.ts @@ -0,0 +1,65 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import * as dom from '../../../../base/browser/dom.js'; +import { IChatWidget } from '../../../../workbench/contrib/chat/browser/chat.js'; +import { IChatResponseViewModel, isResponseVM } from '../../../../workbench/contrib/chat/common/model/chatViewModel.js'; + +export interface IResolvedResponseSelection { + readonly response: IChatResponseViewModel; + readonly text: string; +} + +/** Ancestor of a valid selection endpoint: rendered assistant markdown. */ +const markdownScopeSelector = '.chat-markdown-part'; +/** Ancestors that exclude an endpoint even inside markdown (embedded code editors, tool UI). */ +const excludedAncestorSelectors = ['.monaco-editor', '.chat-tool-invocation-part']; + +function closestElement(node: Node): HTMLElement | undefined { + return node.nodeType === Node.ELEMENT_NODE ? node as HTMLElement : node.parentElement ?? undefined; +} + +function isAssistantMarkdownEndpoint(node: Node, widgetDomNode: HTMLElement): boolean { + const element = closestElement(node); + if (!element || !widgetDomNode.contains(element) || !element.closest(markdownScopeSelector)) { + return false; + } + return !excludedAncestorSelectors.some(selector => element.closest(selector)); +} + +/** + * Resolves the widget's current native DOM selection to the single assistant + * response it lies entirely within, scoped to rendered markdown only (embedded + * code editors and tool-invocation UI are excluded). Returns `undefined` for an + * empty/collapsed selection, a selection spanning more than one response, or + * one that touches non-markdown content. + */ +export function resolveResponseSelection(widget: IChatWidget): IResolvedResponseSelection | undefined { + const nativeSelection = dom.getWindow(widget.domNode).getSelection(); + const text = nativeSelection?.toString(); + if (!nativeSelection || nativeSelection.isCollapsed || !text?.trim()) { + return undefined; + } + + const { anchorNode, focusNode } = nativeSelection; + if (!anchorNode || !focusNode + || !isAssistantMarkdownEndpoint(anchorNode, widget.domNode) + || !isAssistantMarkdownEndpoint(focusNode, widget.domNode)) { + return undefined; + } + + const anchorElement = closestElement(anchorNode); + const focusElement = closestElement(focusNode); + if (!anchorElement || !focusElement) { + return undefined; + } + const anchorItem = widget.getElementFromNode(anchorElement); + const focusItem = widget.getElementFromNode(focusElement); + if (!anchorItem || anchorItem !== focusItem || !isResponseVM(anchorItem)) { + return undefined; + } + + return { response: anchorItem, text }; +} diff --git a/src/vs/sessions/contrib/chat/browser/responseSelectionSideChatController.ts b/src/vs/sessions/contrib/chat/browser/responseSelectionSideChatController.ts new file mode 100644 index 0000000000000..fcf7f6ca5402b --- /dev/null +++ b/src/vs/sessions/contrib/chat/browser/responseSelectionSideChatController.ts @@ -0,0 +1,221 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import * as dom from '../../../../base/browser/dom.js'; +import { Disposable } from '../../../../base/common/lifecycle.js'; +import { Codicon } from '../../../../base/common/codicons.js'; +import { KeyCode } from '../../../../base/common/keyCodes.js'; +import { localize } from '../../../../nls.js'; +import { ILogService } from '../../../../platform/log/common/log.js'; +import { INotificationService } from '../../../../platform/notification/common/notification.js'; +import { IChatWidget } from '../../../../workbench/contrib/chat/browser/chat.js'; +import { FeedbackInputWidget } from '../../agentFeedback/browser/feedbackInputWidget.js'; +import { ISessionsService } from '../../../services/sessions/browser/sessionsService.js'; +import { IChat, SessionStatus } from '../../../services/sessions/common/session.js'; +import { ISessionsManagementService } from '../../../services/sessions/common/sessionsManagement.js'; +import { IResolvedResponseSelection, resolveResponseSelection } from './responseSelectionResolver.js'; +import { createAndSendSideChat } from './sideChatOrchestration.js'; + +/** + * Agents-window-only controller that shows an "Ask Question" input (reusing + * {@link FeedbackInputWidget}) when the user selects text within a single + * assistant response's rendered markdown, and creates a side chat anchored to + * that response when submitted. Owned by `ChatView` so this affordance never + * appears in the regular workbench chat surface. + */ +export class ResponseSelectionSideChatController extends Disposable { + + private readonly _input: FeedbackInputWidget; + private _resolved: IResolvedResponseSelection | undefined; + private _chat: IChat | undefined; + + constructor( + private readonly _widget: IChatWidget, + @ISessionsManagementService private readonly _sessionsManagementService: ISessionsManagementService, + @ISessionsService private readonly _sessionsService: ISessionsService, + @ILogService private readonly _logService: ILogService, + @INotificationService private readonly _notificationService: INotificationService, + ) { + super(); + + this._input = this._register(new FeedbackInputWidget({ + placeholder: localize('sessions.selectionSideChat.placeholder', "Ask Question"), + ariaLabel: localize('sessions.selectionSideChat.ariaLabel', "Ask a question about the selected response text"), + getMaxContentWidth: () => this._widget.domNode.clientWidth, + primaryAction: { + label: localize('sessions.selectionSideChat.ask', "Ask Question"), + icon: Codicon.send, + keybindingLabel: localize('sessions.selectionSideChat.enter', "Enter"), + }, + })); + this._widget.domNode.appendChild(this._input.domNode); + + this._register(this._input.onDidTriggerPrimary(() => this._submit())); + this._register(dom.addStandardDisposableListener(this._input.inputElement, 'keydown', e => { + if (e.keyCode === KeyCode.Escape) { + e.preventDefault(); + e.stopPropagation(); + this._dismiss(); + return; + } + if (e.keyCode === KeyCode.Enter) { + e.preventDefault(); + e.stopPropagation(); + this._submit(); + } + })); + this._register(dom.addStandardDisposableListener(this._input.inputElement, 'keypress', e => { + e.stopPropagation(); + })); + this._register(dom.addStandardDisposableListener(this._input.inputElement, 'input', () => { + this._input.autoSize(); + this._input.updateActionEnabled(); + })); + + const window = dom.getWindow(this._widget.domNode); + this._register(dom.addDisposableListener(window.document, 'selectionchange', () => this._onSelectionChange())); + // Scrolling the transcript invalidates the widget's pinned position; hide rather than drift. + this._register(dom.addDisposableListener(this._widget.domNode, 'scroll', () => this._dismiss(), true)); + } + + /** + * Tracks which chat the current transcript belongs to, for side-chat + * creation. `ChatView` re-invokes this for the same chat on unrelated + * observable changes, so only force-dismiss on a genuine resource change. + */ + setChat(chat: IChat): void { + const changedChat = !this._chat || this._chat.resource.toString() !== chat.resource.toString(); + this._chat = chat; + if (changedChat) { + this._dismiss(true); + } + } + + private _onSelectionChange(): void { + // The browser collapses the document selection the moment the "Ask + // Question" textarea receives focus (textareas don't participate in + // the Selection API). Ignore selectionchange entirely while focus is + // inside the input so typing doesn't dismiss the widget it just + // captured; a real outside invalidation is handled once focus + // actually leaves (the next selectionchange runs with focus outside). + if (dom.isAncestorOfActiveElement(this._input.domNode)) { + return; + } + // A pending submission owns the overlay until the view changes (see + // `_dismiss`); don't let an incidental selection change reposition or + // swap the captured selection out from under it. + if (this._input.isBusy) { + return; + } + const resolved = resolveResponseSelection(this._widget); + if (!resolved) { + this._dismiss(); + return; + } + this._resolved = resolved; + this._showFor(resolved); + } + + private _showFor(resolved: IResolvedResponseSelection): void { + const nativeSelection = dom.getWindow(this._widget.domNode).getSelection(); + const range = nativeSelection?.rangeCount ? nativeSelection.getRangeAt(0) : undefined; + if (!range) { + return; + } + const selectionRect = range.getBoundingClientRect(); + const containerRect = this._widget.domNode.getBoundingClientRect(); + + this._input.show(); + this._input.autoSize(); + this._input.updateActionEnabled(); + + const gap = 4; + const inputWidth = this._input.domNode.offsetWidth; + const inputHeight = this._input.domNode.offsetHeight; + const viewport = dom.getWindow(this._widget.domNode); + + const maxLeft = Math.max(0, containerRect.width - inputWidth); + const left = Math.max(0, Math.min(selectionRect.left - containerRect.left, maxLeft)); + + // Clamp to whichever is smaller: the widget's own box, or the visible + // viewport below the widget's top edge, so the popup never renders + // past either bound. + const maxTop = Math.max(0, Math.min(containerRect.height, viewport.innerHeight - containerRect.top) - inputHeight); + let top = selectionRect.bottom - containerRect.top + gap; + if (top > maxTop) { + // Not enough room below the selection: prefer placing it above instead. + const aboveTop = selectionRect.top - containerRect.top - inputHeight - gap; + top = aboveTop >= 0 ? aboveTop : maxTop; + } + top = Math.max(0, Math.min(top, maxTop)); + + this._input.domNode.style.top = `${top}px`; + this._input.domNode.style.left = `${left}px`; + } + + /** + * Dismisses the input. While a submission is pending (`_input.isBusy`), + * only a genuine view change (`force`, from {@link setChat}) may dismiss + * it — outside interactions like Escape, scrolling, or selection + * invalidation must not race the in-flight create/open/send. + */ + private _dismiss(force = false): void { + if (!force && this._input.isBusy) { + return; + } + const hadFocus = dom.isAncestorOfActiveElement(this._input.domNode); + this._resolved = undefined; + this._input.setBusy(false); + this._input.hide(); + this._input.clearInput(); + if (hadFocus) { + // Hiding the focused input would otherwise leave focus stranded on + // the body; return it to the transcript it was invoked from. + this._widget.focusResponseItem(true); + } + } + + private _submit(): void { + const resolved = this._resolved; + const chat = this._chat; + const query = this._input.inputElement.value.trim(); + if (!resolved || !chat || !query || this._input.isBusy) { + return; + } + + const found = this._sessionsManagementService.getSessionForChatResource(chat.resource); + if (!found) { + this._notificationService.warn(localize('sessions.selectionSideChat.sessionUnavailable', "A side chat cannot be created from this conversation.")); + return; + } + const { session } = found; + if (session.status.get() === SessionStatus.Untitled || session.isArchived.get() || !session.capabilities.get().supportsSideChat) { + this._notificationService.warn(localize('sessions.selectionSideChat.unsupported', "This conversation does not support side chats.")); + return; + } + + // Keep the overlay visible with a busy state instead of eagerly + // dismissing: opening the created side chat naturally dismisses it via + // `setChat`; on failure the question and normal controls are restored + // below so the user can retry. + this._input.setBusy(true, localize('sessions.selectionSideChat.busy', "Asking question…")); + createAndSendSideChat(this._sessionsManagementService, this._sessionsService, session, chat.resource, resolved.response.requestId, query, { text: resolved.text }) + .then(() => { + // `setChat` (fired by the view change from opening the side + // chat) normally dismisses this overlay already; clear busy + // defensively in case that doesn't happen. + this._input.setBusy(false); + }) + .catch(err => { + this._logService.error('[selectionSideChat] Failed to create side chat', err); + this._notificationService.error(localize('sessions.selectionSideChat.createFailed', "The side chat could not be created.")); + this._input.setBusy(false); + this._input.inputElement.value = query; + this._input.autoSize(); + this._input.updateActionEnabled(); + this._input.inputElement.focus(); + }); + } +} diff --git a/src/vs/sessions/contrib/chat/browser/sideChatOrchestration.ts b/src/vs/sessions/contrib/chat/browser/sideChatOrchestration.ts new file mode 100644 index 0000000000000..28292d5a19add --- /dev/null +++ b/src/vs/sessions/contrib/chat/browser/sideChatOrchestration.ts @@ -0,0 +1,44 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { URI } from '../../../../base/common/uri.js'; +import { ISessionsService } from '../../../services/sessions/browser/sessionsService.js'; +import { IChat, ISession, ISideChatSelection } from '../../../services/sessions/common/session.js'; +import { ISessionsManagementService } from '../../../services/sessions/common/sessionsManagement.js'; + +/** + * Activates `sideChat` through the normal sessions navigation flow, then + * sends `query` on it. Shared by every side-chat entry point (`/btw`, + * response-selection) so they stay consistent about activating the chat + * before sending the first message. + */ +export async function openAndSendSideChat( + sessionsService: ISessionsService, + sessionsManagementService: ISessionsManagementService, + session: ISession, + sideChat: IChat, + query: string, +): Promise { + await sessionsService.openChat(session, sideChat.resource); + await sessionsManagementService.sendRequest(session, sideChat, { query }); +} + +/** + * Creates a side chat branched from `turnId` in `sourceChat`, then opens and + * sends `query` on it via {@link openAndSendSideChat}. + */ +export async function createAndSendSideChat( + sessionsManagementService: ISessionsManagementService, + sessionsService: ISessionsService, + session: ISession, + sourceChat: URI, + turnId: string, + query: string, + selection?: ISideChatSelection, +): Promise { + const sideChat = await sessionsManagementService.createSideChatInSession(session, sourceChat, turnId, selection); + await openAndSendSideChat(sessionsService, sessionsManagementService, session, sideChat, query); + return sideChat; +} diff --git a/src/vs/sessions/contrib/chat/test/browser/responseSelectionResolver.test.ts b/src/vs/sessions/contrib/chat/test/browser/responseSelectionResolver.test.ts new file mode 100644 index 0000000000000..fa5ab6ff75431 --- /dev/null +++ b/src/vs/sessions/contrib/chat/test/browser/responseSelectionResolver.test.ts @@ -0,0 +1,163 @@ +/*--------------------------------------------------------------------------------------------- + * 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 * as dom from '../../../../../base/browser/dom.js'; +import { DisposableStore, toDisposable } from '../../../../../base/common/lifecycle.js'; +import { upcastPartial } from '../../../../../base/test/common/mock.js'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; +import { IChatWidget } from '../../../../../workbench/contrib/chat/browser/chat.js'; +import { IChatResponseViewModel } from '../../../../../workbench/contrib/chat/common/model/chatViewModel.js'; +import { resolveResponseSelection } from '../../browser/responseSelectionResolver.js'; + +function makeResponse(requestId: string): IChatResponseViewModel { + return upcastPartial({ requestId, setVote: () => undefined }); +} + +function stubSelection(store: DisposableStore, anchorNode: Node, focusNode: Node, text: string): void { + const targetWindow = dom.getWindow(anchorNode); + const original = targetWindow.getSelection.bind(targetWindow); + const mutableWindow = targetWindow as typeof targetWindow & { getSelection: () => Selection | null }; + mutableWindow.getSelection = () => ({ + toString: () => text, + isCollapsed: text.length === 0, + anchorNode, + focusNode, + rangeCount: 1, + } as Selection); + store.add(toDisposable(() => { mutableWindow.getSelection = original; })); +} + +suite('resolveResponseSelection', () => { + const disposables = ensureNoDisposablesAreLeakedInTestSuite(); + + function setup() { + const store = disposables.add(new DisposableStore()); + const doc = dom.getActiveDocument(); + const widgetDomNode = doc.createElement('div'); + doc.body.appendChild(widgetDomNode); + store.add(toDisposable(() => widgetDomNode.remove())); + return { store, doc, widgetDomNode }; + } + + test('resolves a plain markdown selection within a single response', () => { + const { store, doc, widgetDomNode } = setup(); + const markdown = doc.createElement('div'); + markdown.classList.add('chat-markdown-part'); + const textNode = doc.createTextNode('hello world'); + markdown.appendChild(textNode); + widgetDomNode.appendChild(markdown); + + const response = makeResponse('turn-1'); + stubSelection(store, textNode, textNode, 'hello world'); + const widget = upcastPartial({ + domNode: widgetDomNode, + getElementFromNode: () => response, + }); + + const resolved = resolveResponseSelection(widget); + assert.ok(resolved); + assert.strictEqual(resolved!.response, response); + assert.strictEqual(resolved!.text, 'hello world'); + }); + + test('rejects a collapsed (empty) selection', () => { + const { store, doc, widgetDomNode } = setup(); + const markdown = doc.createElement('div'); + markdown.classList.add('chat-markdown-part'); + const textNode = doc.createTextNode('hello world'); + markdown.appendChild(textNode); + widgetDomNode.appendChild(markdown); + + stubSelection(store, textNode, textNode, ''); + const widget = upcastPartial({ + domNode: widgetDomNode, + getElementFromNode: () => makeResponse('turn-1'), + }); + + assert.strictEqual(resolveResponseSelection(widget), undefined); + }); + + test('rejects a selection inside an embedded code editor', () => { + const { store, doc, widgetDomNode } = setup(); + const markdown = doc.createElement('div'); + markdown.classList.add('chat-markdown-part'); + const editor = doc.createElement('div'); + editor.classList.add('monaco-editor'); + const textNode = doc.createTextNode('const x = 1;'); + editor.appendChild(textNode); + markdown.appendChild(editor); + widgetDomNode.appendChild(markdown); + + stubSelection(store, textNode, textNode, 'const x = 1;'); + const widget = upcastPartial({ + domNode: widgetDomNode, + getElementFromNode: () => makeResponse('turn-1'), + }); + + assert.strictEqual(resolveResponseSelection(widget), undefined); + }); + + test('rejects a selection inside tool-invocation UI', () => { + const { store, doc, widgetDomNode } = setup(); + const markdown = doc.createElement('div'); + markdown.classList.add('chat-markdown-part'); + const tool = doc.createElement('div'); + tool.classList.add('chat-tool-invocation-part'); + const textNode = doc.createTextNode('ran a tool'); + tool.appendChild(textNode); + markdown.appendChild(tool); + widgetDomNode.appendChild(markdown); + + stubSelection(store, textNode, textNode, 'ran a tool'); + const widget = upcastPartial({ + domNode: widgetDomNode, + getElementFromNode: () => makeResponse('turn-1'), + }); + + assert.strictEqual(resolveResponseSelection(widget), undefined); + }); + + test('rejects a selection spanning two different responses', () => { + const { store, doc, widgetDomNode } = setup(); + const markdown = doc.createElement('div'); + markdown.classList.add('chat-markdown-part'); + const partA = doc.createElement('span'); + const partB = doc.createElement('span'); + const textNode1 = doc.createTextNode('first response'); + const textNode2 = doc.createTextNode('second response'); + partA.appendChild(textNode1); + partB.appendChild(textNode2); + markdown.appendChild(partA); + markdown.appendChild(partB); + widgetDomNode.appendChild(markdown); + + stubSelection(store, textNode1, textNode2, 'first response second response'); + const responseA = makeResponse('turn-1'); + const responseB = makeResponse('turn-2'); + const widget = upcastPartial({ + domNode: widgetDomNode, + getElementFromNode: (node: HTMLElement) => node === partA ? responseA : responseB, + }); + + assert.strictEqual(resolveResponseSelection(widget), undefined); + }); + + test('rejects a selection outside the markdown scope', () => { + const { store, doc, widgetDomNode } = setup(); + const other = doc.createElement('div'); + const textNode = doc.createTextNode('not markdown'); + other.appendChild(textNode); + widgetDomNode.appendChild(other); + + stubSelection(store, textNode, textNode, 'not markdown'); + const widget = upcastPartial({ + domNode: widgetDomNode, + getElementFromNode: () => makeResponse('turn-1'), + }); + + assert.strictEqual(resolveResponseSelection(widget), undefined); + }); +}); diff --git a/src/vs/sessions/contrib/chat/test/browser/responseSelectionSideChatController.test.ts b/src/vs/sessions/contrib/chat/test/browser/responseSelectionSideChatController.test.ts new file mode 100644 index 0000000000000..c83edf87e5df7 --- /dev/null +++ b/src/vs/sessions/contrib/chat/test/browser/responseSelectionSideChatController.test.ts @@ -0,0 +1,420 @@ +/*--------------------------------------------------------------------------------------------- + * 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 * as dom from '../../../../../base/browser/dom.js'; +import { DisposableStore, toDisposable } from '../../../../../base/common/lifecycle.js'; +import { constObservable } from '../../../../../base/common/observable.js'; +import { URI } from '../../../../../base/common/uri.js'; +import { upcastPartial } from '../../../../../base/test/common/mock.js'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; +import { ILogService, NullLogService } from '../../../../../platform/log/common/log.js'; +import { TestInstantiationService } from '../../../../../platform/instantiation/test/common/instantiationServiceMock.js'; +import { INotificationHandle, INotificationService, Severity } from '../../../../../platform/notification/common/notification.js'; +import { TestNotificationService } from '../../../../../platform/notification/test/common/testNotificationService.js'; +import { IChatWidget } from '../../../../../workbench/contrib/chat/browser/chat.js'; +import { IChatResponseViewModel } from '../../../../../workbench/contrib/chat/common/model/chatViewModel.js'; +import { ResponseSelectionSideChatController } from '../../browser/responseSelectionSideChatController.js'; +import { ISessionsService } from '../../../../services/sessions/browser/sessionsService.js'; +import { IChat, ISession, SessionStatus } from '../../../../services/sessions/common/session.js'; +import { ISessionsManagementService } from '../../../../services/sessions/common/sessionsManagement.js'; + +class RecordingNotificationService extends TestNotificationService { + readonly notifications: { severity: Severity; message: string }[] = []; + override warn(message: string): INotificationHandle { + this.notifications.push({ severity: Severity.Warning, message }); + return super.warn(message); + } + override error(error: string | Error): INotificationHandle { + this.notifications.push({ severity: Severity.Error, message: error instanceof Error ? error.message : error }); + return super.error(error); + } +} + +suite('ResponseSelectionSideChatController', () => { + const disposables = ensureNoDisposablesAreLeakedInTestSuite(); + + function setup(options?: { + createSideChatInSession?: ISessionsManagementService['createSideChatInSession']; + sendRequest?: ISessionsManagementService['sendRequest']; + }) { + const store = disposables.add(new DisposableStore()); + const instantiationService = store.add(new TestInstantiationService()); + const doc = dom.getActiveDocument(); + const widgetDomNode = doc.createElement('div'); + doc.body.appendChild(widgetDomNode); + store.add(toDisposable(() => widgetDomNode.remove())); + + const markdown = doc.createElement('div'); + markdown.classList.add('chat-markdown-part'); + const textNode = doc.createTextNode('hello world'); + markdown.appendChild(textNode); + widgetDomNode.appendChild(markdown); + + const response = upcastPartial({ requestId: 'turn-1', setVote: () => undefined }); + const focusResponseItemCalls: boolean[] = []; + const widget = upcastPartial({ + domNode: widgetDomNode, + getElementFromNode: () => response, + focusResponseItem: (lastFocused?: boolean) => { focusResponseItemCalls.push(!!lastFocused); }, + }); + + let containerRect: Partial = { top: 0, left: 0, width: 600, height: 600 }; + widgetDomNode.getBoundingClientRect = () => containerRect as DOMRect; + + const targetWindow = dom.getWindow(widgetDomNode); + const originalGetSelection = targetWindow.getSelection.bind(targetWindow); + const mutableWindow = targetWindow as typeof targetWindow & { getSelection: () => Selection | null }; + let selectionText = ''; + let rangeRect: Partial = { top: 0, bottom: 0, left: 0, right: 0, width: 0, height: 0 }; + mutableWindow.getSelection = () => ({ + toString: () => selectionText, + isCollapsed: selectionText.length === 0, + anchorNode: textNode, + focusNode: textNode, + rangeCount: 1, + getRangeAt: () => ({ + getBoundingClientRect: () => rangeRect as DOMRect, + }), + } as unknown as Selection); + store.add(toDisposable(() => { mutableWindow.getSelection = originalGetSelection; })); + + const setSelection = (text: string, rect?: Partial) => { + selectionText = text; + if (rect) { + rangeRect = rect; + } + doc.dispatchEvent(new Event('selectionchange')); + }; + const setContainerRect = (rect: Partial) => { containerRect = rect; }; + + const sideChat = upcastPartial({ resource: URI.parse('test:///chat/side') }); + const chat = upcastPartial({ resource: URI.parse('test:///chat/source') }); + const session = upcastPartial({ + sessionId: 'session', + resource: URI.parse('test:///session'), + status: constObservable(SessionStatus.Completed), + isArchived: constObservable(false), + capabilities: constObservable({ supportsMultipleChats: true, supportsSideChat: true }), + }); + + const callOrder: string[] = []; + const notificationService = new RecordingNotificationService(); + instantiationService.stub(ISessionsManagementService, upcastPartial({ + getSessionForChatResource: resource => resource.toString() === chat.resource.toString() ? { session, chat } : undefined, + createSideChatInSession: options?.createSideChatInSession ?? (async (_session, _sourceChat, turnId, selection) => { + callOrder.push(`create:${turnId}:${selection?.text}`); + return sideChat; + }), + sendRequest: options?.sendRequest ?? (async (_session, sentChat, sendOptions) => { + callOrder.push(`send:${sentChat.resource.toString()}:${sendOptions.query}`); + }), + })); + instantiationService.stub(ISessionsService, upcastPartial({ + openChat: async (_session, chatUri) => { + callOrder.push(`open:${chatUri.toString()}`); + }, + })); + instantiationService.stub(INotificationService, notificationService); + instantiationService.stub(ILogService, new NullLogService()); + + const controller = store.add(instantiationService.createInstance(ResponseSelectionSideChatController, widget)); + controller.setChat(chat); + + return { controller, setSelection, setContainerRect, callOrder, doc, chat, sideChat, focusResponseItemCalls, notificationService }; + } + + function inputDomNode(controller: ResponseSelectionSideChatController): HTMLElement { + return (controller as unknown as { _input: { domNode: HTMLElement } })._input.domNode; + } + + function inputTextArea(controller: ResponseSelectionSideChatController): HTMLTextAreaElement { + return (controller as unknown as { _input: { inputElement: HTMLTextAreaElement } })._input.inputElement; + } + + function isInputBusy(controller: ResponseSelectionSideChatController): boolean { + return (controller as unknown as { _input: { isBusy: boolean } })._input.isBusy; + } + + function submitViaClick(controller: ResponseSelectionSideChatController, query: string): void { + const textArea = inputTextArea(controller); + textArea.value = query; + textArea.dispatchEvent(new Event('input', { bubbles: true })); + inputDomNode(controller).querySelector('.action-label')!.click(); + } + + function dispatchKey(target: HTMLElement, key: string): void { + const event = new KeyboardEvent('keydown', { key, bubbles: true, cancelable: true }); + Object.defineProperty(event, 'keyCode', { get: () => key === 'Escape' ? 27 : 13 }); + target.dispatchEvent(event); + } + + test('shows the ask-question input for a valid markdown selection', () => { + const { controller, setSelection } = setup(); + assert.strictEqual(inputDomNode(controller).style.display, 'none'); + + setSelection('hello world'); + assert.notStrictEqual(inputDomNode(controller).style.display, 'none'); + }); + + test('hides the input again once the selection is cleared', () => { + const { controller, setSelection } = setup(); + setSelection('hello world'); + assert.notStrictEqual(inputDomNode(controller).style.display, 'none'); + + setSelection(''); + assert.strictEqual(inputDomNode(controller).style.display, 'none'); + }); + + test('creates, opens, and sends a side chat anchored to the response on submit', async () => { + const { controller, setSelection, callOrder, sideChat } = setup(); + setSelection('hello world'); + + const textArea = inputTextArea(controller); + textArea.value = 'what does this mean?'; + textArea.dispatchEvent(new Event('input', { bubbles: true })); + inputDomNode(controller).querySelector('.action-label')!.click(); + + await new Promise(resolve => setTimeout(resolve, 0)); + + assert.deepStrictEqual(callOrder, [ + 'create:turn-1:hello world', + `open:${sideChat.resource.toString()}`, + `send:${sideChat.resource.toString()}:what does this mean?`, + ]); + }); + + test('stays visible and keeps the captured selection when the input steals focus and collapses the native selection', () => { + const { controller, setSelection, callOrder } = setup(); + setSelection('hello world'); + assert.notStrictEqual(inputDomNode(controller).style.display, 'none'); + + const textArea = inputTextArea(controller); + textArea.focus(); + // Focusing the textarea collapses the document Selection as a browser + // side effect; this must not dismiss the widget. + setSelection(''); + assert.notStrictEqual(inputDomNode(controller).style.display, 'none', 'input must stay visible while focused'); + + // The originally-captured selection must still be used on submit, not + // the now-empty native selection. + textArea.value = 'what does this mean?'; + textArea.dispatchEvent(new Event('input', { bubbles: true })); + inputDomNode(controller).querySelector('.action-label')!.click(); + assert.ok(callOrder[0]?.startsWith('create:turn-1:hello world')); + }); + + test('dismisses once focus genuinely leaves the input and the selection is invalid', () => { + const { controller, setSelection } = setup(); + setSelection('hello world'); + + const textArea = inputTextArea(controller); + textArea.focus(); + setSelection(''); + assert.notStrictEqual(inputDomNode(controller).style.display, 'none'); + + textArea.blur(); + setSelection(''); + assert.strictEqual(inputDomNode(controller).style.display, 'none', 'input must dismiss once focus truly leaves it'); + }); + + test('restores focus to the response item when Escape dismisses the focused input', () => { + const { controller, setSelection, focusResponseItemCalls } = setup(); + setSelection('hello world'); + + const textArea = inputTextArea(controller); + textArea.focus(); + dispatchKey(textArea, 'Escape'); + + assert.strictEqual(inputDomNode(controller).style.display, 'none'); + assert.deepStrictEqual(focusResponseItemCalls, [true]); + }); + + test('does not restore focus on dismiss when the input was not focused', () => { + const { setSelection, focusResponseItemCalls } = setup(); + setSelection('hello world'); + setSelection(''); + + assert.deepStrictEqual(focusResponseItemCalls, []); + }); + + test('clamps the overlay vertically within the container bounds when the selection is near the bottom', () => { + const { controller, setSelection, setContainerRect } = setup(); + // A container far shorter than any realistic widget height forces the + // vertical clamp to floor the overlay at the top. + setContainerRect({ top: 0, left: 0, width: 600, height: 20 }); + setSelection('hello world', { top: 10, bottom: 15, left: 0, right: 10, width: 10, height: 5 }); + + const style = inputDomNode(controller).style; + assert.notStrictEqual(style.display, 'none'); + assert.strictEqual(parseFloat(style.top), 0); + }); + + test('stays visible with a busy state while the request is pending, then clears once it settles', async () => { + let resolveCreate!: (chat: IChat) => void; + const pending = new Promise(resolve => { resolveCreate = resolve; }); + const { controller, setSelection, callOrder, sideChat } = setup({ + createSideChatInSession: async (_session, _sourceChat, turnId, selection) => { + callOrder.push(`create:${turnId}:${selection?.text}`); + return pending; + }, + }); + setSelection('hello world'); + submitViaClick(controller, 'what does this mean?'); + + assert.strictEqual(isInputBusy(controller), true, 'input must report busy while the request is pending'); + assert.strictEqual(inputTextArea(controller).disabled, true, 'the textarea must be disabled while pending'); + assert.notStrictEqual(inputDomNode(controller).style.display, 'none', 'the overlay must stay visible while pending, not be dismissed eagerly'); + + resolveCreate(sideChat); + await new Promise(resolve => setTimeout(resolve, 0)); + + assert.deepStrictEqual(callOrder, [ + 'create:turn-1:hello world', + `open:${sideChat.resource.toString()}`, + `send:${sideChat.resource.toString()}:what does this mean?`, + ]); + assert.strictEqual(isInputBusy(controller), false, 'busy clears once the orchestration settles'); + }); + + test('prevents duplicate submission (click and Enter) while a request is pending', async () => { + let resolveCreate!: (chat: IChat) => void; + const pending = new Promise(resolve => { resolveCreate = resolve; }); + let createCalls = 0; + const { controller, setSelection, sideChat } = setup({ + createSideChatInSession: async () => { + createCalls++; + return pending; + }, + }); + setSelection('hello world'); + submitViaClick(controller, 'what does this mean?'); + assert.strictEqual(createCalls, 1); + + // A second click and an Enter keypress while the first request is still + // in flight must not create a second side chat. + inputDomNode(controller).querySelector('.action-label')!.click(); + dispatchKey(inputTextArea(controller), 'Enter'); + assert.strictEqual(createCalls, 1, 'only the first submission must create a side chat'); + + resolveCreate(sideChat); + await new Promise(resolve => setTimeout(resolve, 0)); + }); + + test('ignores Escape and selection-change dismissal while a request is pending', async () => { + let resolveCreate!: (chat: IChat) => void; + const pending = new Promise(resolve => { resolveCreate = resolve; }); + const { controller, setSelection, sideChat } = setup({ + createSideChatInSession: async () => pending, + }); + setSelection('hello world'); + submitViaClick(controller, 'what does this mean?'); + assert.notStrictEqual(inputDomNode(controller).style.display, 'none'); + + dispatchKey(inputTextArea(controller), 'Escape'); + assert.notStrictEqual(inputDomNode(controller).style.display, 'none', 'Escape must not dismiss a pending request'); + + setSelection(''); + assert.notStrictEqual(inputDomNode(controller).style.display, 'none', 'an invalidated selection must not dismiss a pending request'); + + resolveCreate(sideChat); + await new Promise(resolve => setTimeout(resolve, 0)); + }); + + test('restores the entered question and re-enables the input when the side chat fails to create', async () => { + const { controller, setSelection, notificationService } = setup({ + createSideChatInSession: async () => { throw new Error('boom'); }, + }); + setSelection('hello world'); + submitViaClick(controller, 'what does this mean?'); + assert.strictEqual(isInputBusy(controller), true); + + await new Promise(resolve => setTimeout(resolve, 0)); + + assert.strictEqual(isInputBusy(controller), false, 'busy must clear on failure'); + assert.strictEqual(inputTextArea(controller).disabled, false, 'the textarea must be re-enabled on failure'); + assert.strictEqual(inputTextArea(controller).value, 'what does this mean?', 'the entered question must be restored on failure'); + assert.notStrictEqual(inputDomNode(controller).style.display, 'none', 'the overlay must stay visible so the user can retry'); + assert.strictEqual(notificationService.notifications.length, 1); + assert.strictEqual(notificationService.notifications[0].severity, Severity.Error); + }); + + test('same-chat setChat (e.g. a status/interactivity update) preserves a visible draft', () => { + const { controller, setSelection, chat } = setup(); + setSelection('hello world'); + const textArea = inputTextArea(controller); + textArea.value = 'a draft in progress'; + textArea.dispatchEvent(new Event('input', { bubbles: true })); + + // A new IChat object for the same resource (e.g. ChatView re-invoking + // setChat on a status/interactivity observable change) must not + // discard the visible draft. + controller.setChat(upcastPartial({ resource: chat.resource })); + + assert.notStrictEqual(inputDomNode(controller).style.display, 'none', 'input must stay visible on a same-resource setChat'); + assert.strictEqual(textArea.value, 'a draft in progress', 'the typed draft must survive a same-resource setChat'); + }); + + test('same-chat setChat does not clear a pending busy submission', async () => { + let resolveCreate!: (chat: IChat) => void; + const pending = new Promise(resolve => { resolveCreate = resolve; }); + const { controller, setSelection, callOrder, chat, sideChat } = setup({ + createSideChatInSession: async (_session, _sourceChat, turnId, selection) => { + callOrder.push(`create:${turnId}:${selection?.text}`); + return pending; + }, + }); + setSelection('hello world'); + submitViaClick(controller, 'what does this mean?'); + assert.strictEqual(isInputBusy(controller), true); + + // A same-resource setChat (status/interactivity update) must not + // force-dismiss or clear busy while the submission is still pending. + controller.setChat(upcastPartial({ resource: chat.resource })); + assert.strictEqual(isInputBusy(controller), true, 'busy must survive a same-resource setChat'); + assert.strictEqual(inputTextArea(controller).disabled, true); + assert.notStrictEqual(inputDomNode(controller).style.display, 'none'); + + // The still-pending original request must be the one that eventually + // resolves the busy state — a same-resource setChat must not have + // let a second submission race in. + resolveCreate(sideChat); + await new Promise(resolve => setTimeout(resolve, 0)); + + assert.deepStrictEqual(callOrder, [ + 'create:turn-1:hello world', + `open:${sideChat.resource.toString()}`, + `send:${sideChat.resource.toString()}:what does this mean?`, + ]); + assert.strictEqual(isInputBusy(controller), false); + }); + + test('different-resource setChat force-dismisses even while busy', async () => { + let resolveCreate!: (chat: IChat) => void; + const pending = new Promise(resolve => { resolveCreate = resolve; }); + const { controller, setSelection } = setup({ + createSideChatInSession: async () => pending, + }); + setSelection('hello world'); + submitViaClick(controller, 'what does this mean?'); + assert.strictEqual(isInputBusy(controller), true); + + controller.setChat(upcastPartial({ resource: URI.parse('test:///chat/other') })); + + assert.strictEqual(inputDomNode(controller).style.display, 'none', 'a genuine chat change must dismiss even a busy overlay'); + assert.strictEqual(isInputBusy(controller), false); + + // A real click also clears the browser's text selection; reflect that + // here so a `selectionchange` the browser fires asynchronously as + // focus leaves the now-hidden input (which the mocked `getSelection` + // would otherwise still report as "hello world") can't reopen it. + setSelection(''); + + // The now-orphaned request settling afterwards must not reopen the overlay. + resolveCreate(upcastPartial({ resource: URI.parse('test:///chat/side') })); + await new Promise(resolve => setTimeout(resolve, 0)); + assert.strictEqual(inputDomNode(controller).style.display, 'none'); + }); +}); diff --git a/src/vs/workbench/contrib/chat/browser/actions/chatAccessibilityHelp.ts b/src/vs/workbench/contrib/chat/browser/actions/chatAccessibilityHelp.ts index 2ad61a716da03..366dbbb92a48e 100644 --- a/src/vs/workbench/contrib/chat/browser/actions/chatAccessibilityHelp.ts +++ b/src/vs/workbench/contrib/chat/browser/actions/chatAccessibilityHelp.ts @@ -11,6 +11,7 @@ import { AccessibleContentProvider, AccessibleViewProviderId, AccessibleViewType import { IAccessibleViewImplementation } from '../../../../../platform/accessibility/browser/accessibleViewRegistry.js'; import { ContextKeyExpr } from '../../../../../platform/contextkey/common/contextkey.js'; import { IKeybindingService } from '../../../../../platform/keybinding/common/keybinding.js'; +import { IWorkbenchEnvironmentService } from '../../../../services/environment/common/environmentService.js'; import { AccessibilityVerbositySettingId } from '../../../accessibility/browser/accessibilityConfiguration.js'; import { INLINE_CHAT_ID } from '../../../inlineChat/common/inlineChat.js'; import { TerminalContribCommandId } from '../../../terminal/terminalContribExports.js'; @@ -60,7 +61,7 @@ export class AgentChatAccessibilityHelp implements IAccessibleViewImplementation } } -export function getAccessibilityHelpText(type: 'panelChat' | 'inlineChat' | 'quickChat' | 'editsView' | 'agentView', keybindingService: IKeybindingService, supportsFileReferences: boolean): string { +export function getAccessibilityHelpText(type: 'panelChat' | 'inlineChat' | 'quickChat' | 'editsView' | 'agentView', keybindingService: IKeybindingService, supportsFileReferences: boolean, isSessionsWindow: boolean = false): string { const content = []; if (type === 'panelChat' || type === 'quickChat' || type === 'editsView' || type === 'agentView') { content.push(localize('chat.fileChangesDisclosure', 'File change summaries show the total files, additions, and deletions. Focus the disclosure and press Enter or Space to show or hide the individual files.')); @@ -108,6 +109,9 @@ export function getAccessibilityHelpText(type: 'panelChat' | 'inlineChat' | 'qui content.push(localize('chat.previousQuestionCarouselQuestion', 'When a chat question is focused, move to the previous question{0}.', '')); content.push(localize('chat.nextQuestionCarouselQuestion', 'When a chat question is focused, move to the next question{0}.', '')); content.push(localize('chat.focusTip', 'When a tip appears, toggle focus between the tip and the chat input{0}.', '')); + if (isSessionsWindow) { + content.push(localize('sessions.selectionSideChat', 'When you select text within an assistant response, an Ask Question input appears near the selection. Type a question and press Enter to start a new side chat scoped to that selection.')); + } } if (type === 'editsView' || type === 'agentView') { if (type === 'agentView') { @@ -159,6 +163,7 @@ export function getAccessibilityHelpText(type: 'panelChat' | 'inlineChat' | 'qui export function getChatAccessibilityHelpProvider(accessor: ServicesAccessor, editor: ICodeEditor | undefined, type: 'panelChat' | 'inlineChat' | 'quickChat' | 'editsView' | 'agentView'): AccessibleContentProvider | undefined { const widgetService = accessor.get(IChatWidgetService); const keybindingService = accessor.get(IKeybindingService); + const environmentService = accessor.get(IWorkbenchEnvironmentService); const widget = widgetService.lastFocusedWidget; if (!widget) { @@ -172,7 +177,7 @@ export function getChatAccessibilityHelpProvider(accessor: ServicesAccessor, edi const cachedPosition = inputEditor.getPosition(); inputEditor.getSupportedActions(); - const helpText = getAccessibilityHelpText(type, keybindingService, widget.supportsFileReferences); + const helpText = getAccessibilityHelpText(type, keybindingService, widget.supportsFileReferences, environmentService.isSessionsWindow); return new AccessibleContentProvider( type === 'panelChat' ? AccessibleViewProviderId.PanelChat : type === 'inlineChat' ? AccessibleViewProviderId.InlineChat : type === 'agentView' ? AccessibleViewProviderId.AgentChat : AccessibleViewProviderId.QuickChat, { type: AccessibleViewType.Help }, diff --git a/src/vs/workbench/contrib/chat/browser/chat.ts b/src/vs/workbench/contrib/chat/browser/chat.ts index 6221b6a44dc87..239f43057446b 100644 --- a/src/vs/workbench/contrib/chat/browser/chat.ts +++ b/src/vs/workbench/contrib/chat/browser/chat.ts @@ -461,6 +461,10 @@ export interface IChatWidget { getCodeBlockInfosForResponse(response: IChatResponseViewModel): IChatCodeBlockInfo[]; getFileTreeInfosForResponse(response: IChatResponseViewModel): IChatFileTreeInfo[]; getLastFocusedFileTreeForResponse(response: IChatResponseViewModel): IChatFileTreeInfo | undefined; + /** + * Returns the currently rendered chat item containing the node, if any. + */ + getElementFromNode(node: HTMLElement): ChatTreeItem | undefined; clear(targetSessionType?: string): Promise; getViewState(): IChatModelInputState | undefined; lockToCodingAgent(name: string, displayName: string, agentId?: string, agentHostProviderId?: string): void; diff --git a/src/vs/workbench/contrib/chat/browser/widget/chatListWidget.ts b/src/vs/workbench/contrib/chat/browser/widget/chatListWidget.ts index c94a95963f37f..e14c252e618b9 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/chatListWidget.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/chatListWidget.ts @@ -1013,6 +1013,13 @@ export class ChatListWidget extends Disposable { return this._renderer.getTemplateDataForRequestId(requestId); } + /** + * Returns the currently rendered chat item containing the node. + */ + getElementFromNode(node: HTMLElement): ChatTreeItem | undefined { + return this._renderer.getElementFromNode(node); + } + /** * Update renderer options. */ diff --git a/src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts b/src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts index 6472e448939e6..df4676685db5a 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts @@ -3139,6 +3139,10 @@ export class ChatWidget extends Disposable implements IChatWidget { return this.listWidget.getLastFocusedFileTreeForResponse(response); } + getElementFromNode(node: HTMLElement): ChatTreeItem | undefined { + return this.listWidget.getElementFromNode(node); + } + focusResponseItem(lastFocused?: boolean): void { this.listWidget.focusLastItem(lastFocused); } diff --git a/src/vs/workbench/contrib/chat/test/browser/accessibility/chatAccessibilityHelp.test.ts b/src/vs/workbench/contrib/chat/test/browser/accessibility/chatAccessibilityHelp.test.ts index 1ab6da84e2d97..2ec396fb03835 100644 --- a/src/vs/workbench/contrib/chat/test/browser/accessibility/chatAccessibilityHelp.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/accessibility/chatAccessibilityHelp.test.ts @@ -24,4 +24,18 @@ suite('Chat Accessibility Help', () => { unsupported: false, }); }); + + test('only describes the selection side chat affordance in the sessions window', () => { + const keybindingService = { + lookupKeybindings: () => [], + } as unknown as IKeybindingService; + + assert.deepStrictEqual({ + sessionsWindow: getAccessibilityHelpText('agentView', keybindingService, true, true).includes('Ask Question'), + regularWindow: getAccessibilityHelpText('agentView', keybindingService, true, false).includes('Ask Question'), + }, { + sessionsWindow: true, + regularWindow: false, + }); + }); }); From 23bc7e4da14473712cb1eebafe46476617accf33 Mon Sep 17 00:00:00 2001 From: Connor Peet Date: Sat, 25 Jul 2026 08:28:53 -0700 Subject: [PATCH 2/2] chat: address side question review feedback Preserve accessible labels and multiline input behavior, guard stale async UI completion after navigation, and normalize side-chat orchestration APIs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/skills/sessions/SKILL.md | 2 + src/vs/sessions/SESSIONS.md | 27 ++++-- .../browser/feedbackInputWidget.ts | 6 +- .../test/browser/feedbackInputWidget.test.ts | 21 ++++- .../browser/btwSlashCommand.contribution.ts | 2 +- .../responseSelectionSideChatController.ts | 18 ++++ .../chat/browser/sideChatOrchestration.ts | 4 +- ...esponseSelectionSideChatController.test.ts | 92 ++++++++++++++++++- 8 files changed, 158 insertions(+), 14 deletions(-) diff --git a/.github/skills/sessions/SKILL.md b/.github/skills/sessions/SKILL.md index 8728a27a7d15c..eaff5dc16c15e 100644 --- a/.github/skills/sessions/SKILL.md +++ b/.github/skills/sessions/SKILL.md @@ -138,6 +138,8 @@ Whenever the user flags a wrong pattern, rejects an approach, or gives design/ru - **A view-lifecycle `setChat`/`setModel`-style hook can be re-invoked for the *same* underlying resource**: `ChatView.setChat` fires again on unrelated status/interactivity observable changes, not only on a genuine view swap. A consumer like `ResponseSelectionSideChatController` that force-dismisses its own transient UI on every call discards an in-progress draft and, worse, clears a pending busy submission mid-flight, letting a duplicate submission race in. Compare the incoming resource against the previously tracked one and only treat a genuine change (or the first call) as dismiss-worthy; a same-resource re-invocation must preserve visible/busy state. +- **A pending async submission's completion/error handler must no-op after a genuine force-dismiss, not just after a same-resource re-invocation**: even with the same-resource guard above, `ResponseSelectionSideChatController._submit`'s `createAndSendSideChat().then()/.catch()` can still settle *after* the user has genuinely navigated away (a different-resource `setChat`, or any other force-dismiss) — reopening the overlay, restoring the typed query, refocusing the input, or showing a stale error notification for UI the user already dismissed. Capture a `_generation` counter bumped only on a genuine force-dismiss, snapshot it before the async call, and have the settle handlers bail when the counter no longer matches; don't rely solely on resource comparison, since the overlay's own dismissed state (not the chat identity) is what must gate the mutation. + ## Validating Changes You **must** run these checks before declaring work complete: diff --git a/src/vs/sessions/SESSIONS.md b/src/vs/sessions/SESSIONS.md index 545d9f120f544..4f733aec2b74d 100644 --- a/src/vs/sessions/SESSIONS.md +++ b/src/vs/sessions/SESSIONS.md @@ -650,12 +650,16 @@ busy submission rather than clearing it and letting a duplicate race in. On failure the busy state clears, the typed question and normal controls are restored, and the input is refocused so the user can retry without losing their text — the existing warning/error notification and log call are -unchanged. Escape, scrolling, and selection invalidation are all ignored while -a submission is pending so they cannot race the in-flight request. The -action-bar slot and the spinner that replaces it both size to a shared -`--agent-feedback-line-height` CSS custom property (set once on the widget -root, matching the textarea's line-height) and center via flex, rather than a -hardcoded icon height or a positional transform — this keeps both optically +unchanged. A completion/error that settles after a genuine chat navigation +already force-dismissed the overlay (`setChat` with a different chat resource) +is tracked via a submission generation counter bumped on that force-dismiss, +so the stale handler no-ops instead of reopening, refocusing, or mutating the +now-unrelated overlay. Escape, scrolling, and selection invalidation are all +ignored while a submission is pending so they cannot race the in-flight +request. The action-bar slot and the spinner that replaces it both size to the +widget's shared `_LINE_HEIGHT` constant (applied as an inline style to each +element, matching the textarea's line-height) and center via flex, rather than +a hardcoded icon height or a positional transform — this keeps both optically centered on the input's single line, and still flush to the last line when the textarea grows multi-line (the row keeps `align-items: flex-end`). @@ -667,13 +671,22 @@ just focused. The captured selection is treated as an immutable snapshot for that reason — it is not re-read from the live DOM selection on submit. Escape (or any other dismissal while the input has focus) restores focus to the source response via `IChatWidget.focusResponseItem(true)` rather than -letting it fall through to the document body. The overlay's position clamps +letting it fall through to the document body. Plain Enter submits and +prevents the default newline; Shift+Enter and Enter during IME composition +(`e.browserEvent.isComposing`) are left alone so the textarea inserts a +newline or lets composition finish. The overlay's position clamps both horizontally and vertically against the chat widget's own bounds and the visible viewport, measured after `FeedbackInputWidget.show()`/`autoSize()` so real dimensions are used; when there isn't room below the selection it flips above instead, falling back to the nearest in-bounds edge only when neither placement fully fits. +`FeedbackInputWidget.setPlaceholder` only derives `aria-label` from the +placeholder when the widget was constructed without an explicit +`options.ariaLabel`; a caller (like this controller, which sets a dedicated +accessible name) keeps its configured `aria-label` untouched across +placeholder changes. + Agent-host approval levels map to the Copilot SDK allow-all modes before each turn: Default approvals uses `off`, Allow all uses `on`, and Assisted permissions uses `auto`. Assisted permissions only skips a prompt when the SDK's diff --git a/src/vs/sessions/contrib/agentFeedback/browser/feedbackInputWidget.ts b/src/vs/sessions/contrib/agentFeedback/browser/feedbackInputWidget.ts index 6ff8c8bb38b90..ad61578d1d0d7 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/feedbackInputWidget.ts +++ b/src/vs/sessions/contrib/agentFeedback/browser/feedbackInputWidget.ts @@ -55,6 +55,7 @@ export class FeedbackInputWidget extends Disposable { private readonly _secondaryAction: Action | undefined; private _isShowingSecondary = false; private _busy = false; + private readonly _hasExplicitAriaLabel: boolean; /** Whether {@link setBusy} is currently active; callers must not submit again while true. */ get isBusy(): boolean { @@ -69,6 +70,7 @@ export class FeedbackInputWidget extends Disposable { constructor(private readonly _options: IFeedbackInputWidgetOptions) { super(); + this._hasExplicitAriaLabel = _options.ariaLabel !== undefined; this.domNode = document.createElement('div'); this.domNode.classList.add('agent-feedback-input-widget'); this.domNode.style.display = 'none'; @@ -168,7 +170,9 @@ export class FeedbackInputWidget extends Disposable { return; } this.inputElement.placeholder = placeholder; - this.inputElement.setAttribute('aria-label', placeholder); + if (!this._hasExplicitAriaLabel) { + this.inputElement.setAttribute('aria-label', placeholder); + } this.autoSize(); } diff --git a/src/vs/sessions/contrib/agentFeedback/test/browser/feedbackInputWidget.test.ts b/src/vs/sessions/contrib/agentFeedback/test/browser/feedbackInputWidget.test.ts index 3805477ebaab4..638d83fc2eaee 100644 --- a/src/vs/sessions/contrib/agentFeedback/test/browser/feedbackInputWidget.test.ts +++ b/src/vs/sessions/contrib/agentFeedback/test/browser/feedbackInputWidget.test.ts @@ -12,10 +12,11 @@ import { FeedbackInputWidget } from '../../browser/feedbackInputWidget.js'; suite('FeedbackInputWidget', () => { const disposables = ensureNoDisposablesAreLeakedInTestSuite(); - function createWidget() { + function createWidget(ariaLabel?: string) { const store = disposables.add(new DisposableStore()); const widget = store.add(new FeedbackInputWidget({ placeholder: 'Ask Question', + ariaLabel, getMaxContentWidth: () => 400, primaryAction: { label: 'Ask', icon: Codicon.send, keybindingLabel: 'Enter' }, })); @@ -109,4 +110,22 @@ suite('FeedbackInputWidget', () => { assert.strictEqual(busyRect.top, actionsRect.top); assert.strictEqual(busyRect.height, actionsRect.height); }); + + test('setPlaceholder does not overwrite an explicitly configured aria-label', () => { + const widget = createWidget('Ask a question about the selected response text'); + + widget.setPlaceholder('New Placeholder'); + + assert.strictEqual(widget.inputElement.placeholder, 'New Placeholder'); + assert.strictEqual(widget.inputElement.getAttribute('aria-label'), 'Ask a question about the selected response text'); + }); + + test('setPlaceholder keeps the aria-label derived from the placeholder when none was configured', () => { + const widget = createWidget(); + assert.strictEqual(widget.inputElement.getAttribute('aria-label'), 'Ask Question'); + + widget.setPlaceholder('New Placeholder'); + + assert.strictEqual(widget.inputElement.getAttribute('aria-label'), 'New Placeholder'); + }); }); diff --git a/src/vs/sessions/contrib/chat/browser/btwSlashCommand.contribution.ts b/src/vs/sessions/contrib/chat/browser/btwSlashCommand.contribution.ts index 91059d261f8fd..3e3d2156a5bcd 100644 --- a/src/vs/sessions/contrib/chat/browser/btwSlashCommand.contribution.ts +++ b/src/vs/sessions/contrib/chat/browser/btwSlashCommand.contribution.ts @@ -110,7 +110,7 @@ export class BtwSlashCommandContribution extends Disposable implements IWorkbenc return; } - await openAndSendSideChat(sessionsService, sessionsManagementService, session, sideChat, remainder); + await openAndSendSideChat(sessionsManagementService, sessionsService, session, sideChat, remainder); })); } } diff --git a/src/vs/sessions/contrib/chat/browser/responseSelectionSideChatController.ts b/src/vs/sessions/contrib/chat/browser/responseSelectionSideChatController.ts index fcf7f6ca5402b..793e241f9d0df 100644 --- a/src/vs/sessions/contrib/chat/browser/responseSelectionSideChatController.ts +++ b/src/vs/sessions/contrib/chat/browser/responseSelectionSideChatController.ts @@ -30,6 +30,8 @@ export class ResponseSelectionSideChatController extends Disposable { private readonly _input: FeedbackInputWidget; private _resolved: IResolvedResponseSelection | undefined; private _chat: IChat | undefined; + /** Bumped on a genuine chat navigation/force-dismiss so a stale submission's completion/error handler can no-op. */ + private _generation = 0; constructor( private readonly _widget: IChatWidget, @@ -61,6 +63,10 @@ export class ResponseSelectionSideChatController extends Disposable { return; } if (e.keyCode === KeyCode.Enter) { + if (e.browserEvent.isComposing || e.shiftKey) { + // Let IME composition finish, or Shift+Enter insert a newline. + return; + } e.preventDefault(); e.stopPropagation(); this._submit(); @@ -165,6 +171,10 @@ export class ResponseSelectionSideChatController extends Disposable { if (!force && this._input.isBusy) { return; } + if (force) { + // A genuine navigation: bump the generation so a stale submission's completion/error handler no-ops. + this._generation++; + } const hadFocus = dom.isAncestorOfActiveElement(this._input.domNode); this._resolved = undefined; this._input.setBusy(false); @@ -201,8 +211,13 @@ export class ResponseSelectionSideChatController extends Disposable { // `setChat`; on failure the question and normal controls are restored // below so the user can retry. this._input.setBusy(true, localize('sessions.selectionSideChat.busy', "Asking question…")); + const generation = this._generation; createAndSendSideChat(this._sessionsManagementService, this._sessionsService, session, chat.resource, resolved.response.requestId, query, { text: resolved.text }) .then(() => { + // A stale completion after a genuine navigation force-dismissed this overlay must no-op. + if (this._generation !== generation) { + return; + } // `setChat` (fired by the view change from opening the side // chat) normally dismisses this overlay already; clear busy // defensively in case that doesn't happen. @@ -210,6 +225,9 @@ export class ResponseSelectionSideChatController extends Disposable { }) .catch(err => { this._logService.error('[selectionSideChat] Failed to create side chat', err); + if (this._generation !== generation) { + return; + } this._notificationService.error(localize('sessions.selectionSideChat.createFailed', "The side chat could not be created.")); this._input.setBusy(false); this._input.inputElement.value = query; diff --git a/src/vs/sessions/contrib/chat/browser/sideChatOrchestration.ts b/src/vs/sessions/contrib/chat/browser/sideChatOrchestration.ts index 28292d5a19add..a613883c2254c 100644 --- a/src/vs/sessions/contrib/chat/browser/sideChatOrchestration.ts +++ b/src/vs/sessions/contrib/chat/browser/sideChatOrchestration.ts @@ -15,8 +15,8 @@ import { ISessionsManagementService } from '../../../services/sessions/common/se * before sending the first message. */ export async function openAndSendSideChat( - sessionsService: ISessionsService, sessionsManagementService: ISessionsManagementService, + sessionsService: ISessionsService, session: ISession, sideChat: IChat, query: string, @@ -39,6 +39,6 @@ export async function createAndSendSideChat( selection?: ISideChatSelection, ): Promise { const sideChat = await sessionsManagementService.createSideChatInSession(session, sourceChat, turnId, selection); - await openAndSendSideChat(sessionsService, sessionsManagementService, session, sideChat, query); + await openAndSendSideChat(sessionsManagementService, sessionsService, session, sideChat, query); return sideChat; } diff --git a/src/vs/sessions/contrib/chat/test/browser/responseSelectionSideChatController.test.ts b/src/vs/sessions/contrib/chat/test/browser/responseSelectionSideChatController.test.ts index c83edf87e5df7..df5fb9cdcfdec 100644 --- a/src/vs/sessions/contrib/chat/test/browser/responseSelectionSideChatController.test.ts +++ b/src/vs/sessions/contrib/chat/test/browser/responseSelectionSideChatController.test.ts @@ -145,10 +145,14 @@ suite('ResponseSelectionSideChatController', () => { inputDomNode(controller).querySelector('.action-label')!.click(); } - function dispatchKey(target: HTMLElement, key: string): void { - const event = new KeyboardEvent('keydown', { key, bubbles: true, cancelable: true }); + function dispatchKey(target: HTMLElement, key: string, options?: { shiftKey?: boolean; isComposing?: boolean }): KeyboardEvent { + const event = new KeyboardEvent('keydown', { key, shiftKey: options?.shiftKey, bubbles: true, cancelable: true }); Object.defineProperty(event, 'keyCode', { get: () => key === 'Escape' ? 27 : 13 }); + if (options?.isComposing) { + Object.defineProperty(event, 'isComposing', { get: () => true }); + } target.dispatchEvent(event); + return event; } test('shows the ask-question input for a valid markdown selection', () => { @@ -417,4 +421,88 @@ suite('ResponseSelectionSideChatController', () => { await new Promise(resolve => setTimeout(resolve, 0)); assert.strictEqual(inputDomNode(controller).style.display, 'none'); }); + + test('a success that settles after a different-resource setChat does not reopen, refocus, or mutate the overlay', async () => { + let resolveCreate!: (chat: IChat) => void; + const pending = new Promise(resolve => { resolveCreate = resolve; }); + const { controller, setSelection, focusResponseItemCalls } = setup({ + createSideChatInSession: async () => pending, + }); + setSelection('hello world'); + submitViaClick(controller, 'what does this mean?'); + + controller.setChat(upcastPartial({ resource: URI.parse('test:///chat/other') })); + setSelection(''); + const focusCallsAtDismiss = focusResponseItemCalls.length; + + resolveCreate(upcastPartial({ resource: URI.parse('test:///chat/side') })); + await new Promise(resolve => setTimeout(resolve, 0)); + + assert.strictEqual(inputDomNode(controller).style.display, 'none', 'a stale success must not reopen the overlay'); + assert.strictEqual(inputTextArea(controller).value, '', 'a stale success must not mutate the (already cleared) input value'); + assert.deepStrictEqual(focusResponseItemCalls.length, focusCallsAtDismiss, 'a stale success must not refocus the transcript'); + }); + + test('a failure that settles after a different-resource setChat does not reopen, refocus, mutate, or notify', async () => { + let rejectCreate!: (err: Error) => void; + const pending = new Promise((_resolve, reject) => { rejectCreate = reject; }); + const { controller, setSelection, notificationService, focusResponseItemCalls } = setup({ + createSideChatInSession: async () => pending, + }); + setSelection('hello world'); + submitViaClick(controller, 'what does this mean?'); + + controller.setChat(upcastPartial({ resource: URI.parse('test:///chat/other') })); + setSelection(''); + const focusCallsAtDismiss = focusResponseItemCalls.length; + + rejectCreate(new Error('boom')); + await new Promise(resolve => setTimeout(resolve, 0)); + + assert.strictEqual(inputDomNode(controller).style.display, 'none', 'a stale failure must not reopen the overlay'); + assert.strictEqual(inputTextArea(controller).value, '', 'a stale failure must not restore the failed question into the (already cleared) input'); + assert.deepStrictEqual(focusResponseItemCalls.length, focusCallsAtDismiss, 'a stale failure must not refocus the input'); + assert.strictEqual(notificationService.notifications.length, 0, 'a stale failure must not surface a retry notification for an abandoned overlay'); + }); + + test('plain Enter submits and prevents the default newline', () => { + const { controller, setSelection, callOrder } = setup(); + setSelection('hello world'); + const textArea = inputTextArea(controller); + textArea.value = 'what does this mean?'; + textArea.dispatchEvent(new Event('input', { bubbles: true })); + + const event = dispatchKey(textArea, 'Enter'); + + assert.strictEqual(event.defaultPrevented, true, 'plain Enter must prevent the default newline'); + assert.ok(callOrder[0]?.startsWith('create:turn-1:hello world'), 'plain Enter must submit'); + }); + + test('Shift+Enter inserts a newline instead of submitting', () => { + const { controller, setSelection, callOrder } = setup(); + setSelection('hello world'); + const textArea = inputTextArea(controller); + textArea.value = 'what does this mean?'; + textArea.dispatchEvent(new Event('input', { bubbles: true })); + + const event = dispatchKey(textArea, 'Enter', { shiftKey: true }); + + assert.strictEqual(event.defaultPrevented, false, 'Shift+Enter must let the textarea insert a newline'); + assert.deepStrictEqual(callOrder, [], 'Shift+Enter must not submit'); + assert.notStrictEqual(inputDomNode(controller).style.display, 'none', 'Shift+Enter must not dismiss the overlay'); + }); + + test('Enter during IME composition does not submit', () => { + const { controller, setSelection, callOrder } = setup(); + setSelection('hello world'); + const textArea = inputTextArea(controller); + textArea.value = 'what does this mean?'; + textArea.dispatchEvent(new Event('input', { bubbles: true })); + + const event = dispatchKey(textArea, 'Enter', { isComposing: true }); + + assert.strictEqual(event.defaultPrevented, false, 'Enter during IME composition must not be prevented'); + assert.deepStrictEqual(callOrder, [], 'Enter during IME composition must not submit'); + assert.notStrictEqual(inputDomNode(controller).style.display, 'none', 'Enter during IME composition must not dismiss the overlay'); + }); });