Add agent feedback input widget with submission functionality#297702
Add agent feedback input widget with submission functionality#297702
Conversation
…and service integration
There was a problem hiding this comment.
Pull request overview
Adds an in-editor “agent feedback” input overlay for Agent Sessions, including action-bar controls to add feedback and optionally submit it immediately, plus related styling/theme updates.
Changes:
- Extend the feedback overlay widget with an action bar (Add / Add+Submit) and Alt-key affordance.
- Add
addFeedbackAndSubmitto the feedback service to wait for attachment propagation and then submit. - Introduce a dedicated theme color/CSS variable for the widget border and update widget layout CSS.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/sessions/contrib/agentFeedback/browser/media/agentFeedbackEditorInput.css | Flex-layout updates for textarea + action bar; uses new border color token. |
| src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackService.ts | Adds service API to add feedback and submit after attachment update. |
| src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorInputContribution.ts | Implements action bar UI, Enter/Alt+Enter behavior, and widget disposal path. |
| src/vs/sessions/common/theme.ts | Registers agentFeedbackInputWidget.border color token. |
| build/lib/stylelint/vscode-known-variables.json | Adds the new CSS variable to the allowlist. |
| } else { | ||
| // This should not normally happen, but if the widget isn't found, wait a bit to give it a chance to initialize before submitting. | ||
| await new Promise(resolve => setTimeout(resolve, 100)); | ||
| } | ||
|
|
||
| await this._commandService.executeCommand('agentFeedbackEditor.action.submit'); |
There was a problem hiding this comment.
executeCommand('agentFeedbackEditor.action.submit') resolves the target session by looking at the current active editor (see AgentFeedbackEditorAction.run). Since this method awaits attachment updates first, the user could switch editors in the meantime and the submit command may target a different session or do nothing. Consider invoking submit in a way that’s bound to sessionResource (e.g. extend the command to accept an optional sessionResource arg, or call widget.acceptInput(...) directly for the resolved widget).
| } else { | |
| // This should not normally happen, but if the widget isn't found, wait a bit to give it a chance to initialize before submitting. | |
| await new Promise(resolve => setTimeout(resolve, 100)); | |
| } | |
| await this._commandService.executeCommand('agentFeedbackEditor.action.submit'); | |
| // Submit input directly on the resolved widget to avoid relying on the active editor. | |
| await widget.acceptInput(); | |
| } else { | |
| // This should not normally happen, but if the widget isn't found, wait a bit to give it a chance to initialize before submitting. | |
| await new Promise(resolve => setTimeout(resolve, 100)); | |
| // Fall back to the command-based submission, which will use the current active editor. | |
| await this._commandService.executeCommand('agentFeedbackEditor.action.submit'); | |
| } |
| // Toggle to alt action when Alt key is held | ||
| const modifierKeyEmitter = ModifierKeyEmitter.getInstance(); | ||
| modifierKeyEmitter.event(status => { | ||
| this._updateActionForAlt(status.altKey); | ||
| }); |
There was a problem hiding this comment.
The ModifierKeyEmitter listener isn’t disposed. Because ModifierKeyEmitter is a singleton, this subscription will survive after the overlay widget is removed and can keep calling into a disposed ActionBar, causing leaks and potential errors. Store the returned IDisposable and dispose it in AgentFeedbackInputWidget.dispose() (or use a DisposableStore inside the widget).
| this._editor.applyFontInfo(this._inputElement); | ||
| this._editor.applyFontInfo(this._measureElement); | ||
| this._lineHeight = this._editor.getOption(EditorOption.lineHeight); | ||
| this._lineHeight = 22; |
There was a problem hiding this comment.
this._lineHeight = 22 hard-codes a line height that can easily be smaller than the editor’s actual font/lineHeight settings (e.g. accessibility font sizes), leading to clipped or overlapping text in the textarea. Prefer deriving this from this._editor.getOption(EditorOption.lineHeight) (or computed font metrics) and only clamp to a minimum if needed.
| this._lineHeight = 22; | |
| const editorLineHeight = this._editor.getOption(EditorOption.lineHeight); | |
| this._lineHeight = Math.max(22, editorLineHeight); |
| const sessionResource = this._sessionResource; | ||
| this._hide(); | ||
| this._editor.focus(); | ||
| this._agentFeedbackService.addFeedbackAndSubmit(sessionResource, model.uri, selection, text); |
There was a problem hiding this comment.
addFeedbackAndSubmit(...) returns a Promise but it’s invoked without await/void/.catch(...). If executeCommand rejects, this can become an unhandled rejection (the sessions workbench installs a global handler for these). Consider awaiting it (making _addFeedbackAndSubmit async) or explicitly handling errors (e.g. void ...catch(onUnexpectedError)).
| this.addFeedback(sessionResource, resourceUri, range, text); | ||
|
|
||
| // Wait for the attachment contribution to update the chat widget's attachment model | ||
| const widget = this._chatWidgetService.getWidgetBySessionResource(sessionResource); | ||
| if (widget) { | ||
| const attachmentId = 'agentFeedback:' + sessionResource.toString(); | ||
| const hasAttachment = () => widget.attachmentModel.attachments.some(a => a.id === attachmentId); | ||
|
|
||
| if (!hasAttachment()) { | ||
| await Event.toPromise( | ||
| Event.filter(widget.attachmentModel.onDidChange, () => hasAttachment()) | ||
| ); | ||
| } | ||
| } else { | ||
| // This should not normally happen, but if the widget isn't found, wait a bit to give it a chance to initialize before submitting. | ||
| await new Promise(resolve => setTimeout(resolve, 100)); |
There was a problem hiding this comment.
addFeedbackAndSubmit only waits for the attachment to exist. If the session already had an agentFeedback attachment, hasAttachment() will be true immediately and this can submit before the async attachment update includes the newly added feedback item (dropping the latest comment from the submitted payload). Consider capturing the new feedback id returned by addFeedback(...) and waiting until the attachment model reflects that id / expected count, and add a timeout to avoid waiting forever if the attachment update never arrives.
| this.addFeedback(sessionResource, resourceUri, range, text); | |
| // Wait for the attachment contribution to update the chat widget's attachment model | |
| const widget = this._chatWidgetService.getWidgetBySessionResource(sessionResource); | |
| if (widget) { | |
| const attachmentId = 'agentFeedback:' + sessionResource.toString(); | |
| const hasAttachment = () => widget.attachmentModel.attachments.some(a => a.id === attachmentId); | |
| if (!hasAttachment()) { | |
| await Event.toPromise( | |
| Event.filter(widget.attachmentModel.onDidChange, () => hasAttachment()) | |
| ); | |
| } | |
| } else { | |
| // This should not normally happen, but if the widget isn't found, wait a bit to give it a chance to initialize before submitting. | |
| await new Promise(resolve => setTimeout(resolve, 100)); | |
| const timeout = (ms: number) => new Promise<void>(resolve => setTimeout(resolve, ms)); | |
| // Wait for the attachment contribution to update the chat widget's attachment model | |
| const widget = this._chatWidgetService.getWidgetBySessionResource(sessionResource); | |
| if (widget) { | |
| const attachmentId = 'agentFeedback:' + sessionResource.toString(); | |
| const hasAttachment = () => widget.attachmentModel.attachments.some(a => a.id === attachmentId); | |
| const hadAttachment = hasAttachment(); | |
| this.addFeedback(sessionResource, resourceUri, range, text); | |
| if (hadAttachment) { | |
| // Attachment already existed: wait for the next attachment model change after addFeedback | |
| const waitForChange = Event.toPromise(widget.attachmentModel.onDidChange); | |
| await Promise.race([waitForChange, timeout(2000)]); | |
| } else { | |
| // Attachment did not exist before: wait until it appears (or timeout) | |
| if (!hasAttachment()) { | |
| const waitForAttachment = Event.toPromise( | |
| Event.filter(widget.attachmentModel.onDidChange, () => hasAttachment()) | |
| ); | |
| await Promise.race([waitForAttachment, timeout(2000)]); | |
| } | |
| } | |
| } else { | |
| // This should not normally happen, but if the widget isn't found, wait a bit to give it a chance to initialize before submitting. | |
| this.addFeedback(sessionResource, resourceUri, range, text); | |
| await timeout(100); |
| const attachmentId = 'agentFeedback:' + sessionResource.toString(); | ||
| const hasAttachment = () => widget.attachmentModel.attachments.some(a => a.id === attachmentId); | ||
|
|
There was a problem hiding this comment.
The attachment id prefix is duplicated as a string literal here ('agentFeedback:'). Since this id is already defined/exported as ATTACHMENT_ID_PREFIX in agentFeedbackAttachment.ts, consider importing/reusing it to prevent accidental drift if the id format changes.
Copilot Generated Description:Introduce an agent feedback input widget with an action bar, including submission functionality and styling adjustments. Fixes include setting a fixed line height, adjusting padding, and updating border transparency. Additional UX tweaks enhance the overall design.