chat: allow collapsing queued chat requests - #335208
chat: allow collapsing queued chat requests#335208lakshaykhattar (LakshayKhattar) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The dequeue path loses collapsed state, with additional accessibility, URI comparison, hover, and CSS issues unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds keyboard-accessible collapsing for queued and steering chat requests with per-message state preservation.
Changes:
- Adds collapse controls and single-line previews.
- Excludes collapsed messages from sticky-scroll headers.
- Adds renderer regression tests.
Review findings:
- Critical: The real dequeue path does not preserve request IDs, so collapsed state is lost when processing begins. Preserve/transfer state and test the actual path, including merged steering.
- Moderate: Compare session URIs using
isEqual, not serialized strings. - Moderate: Include the full truncated preview in the collapsed hover.
- Moderate: Remove the new prohibited
!importantstyling. - Nit: Document keyboard interaction in Chat Accessibility Help.
File summaries
| File | Description |
|---|---|
src/vs/workbench/contrib/chat/test/browser/widget/chatListRenderer.test.ts |
Tests collapse state, layouts, keyboard use, and transitions. |
src/vs/workbench/contrib/chat/browser/widget/media/chat.css |
Styles collapsed request previews. |
src/vs/workbench/contrib/chat/browser/widget/chatListRenderer.ts |
Implements collapse state, controls, and sticky-scroll handling. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Pending view models are recreated on updates. Keep the choice by request ID, | ||
| // including when the same request moves into the transcript. | ||
| this.requestExpansionState.set(element.id, expanded); |
There was a problem hiding this comment.
Fixed in 49af945. The real dequeue path now preserves the pending request ID. Merged steering requests also carry their source IDs into the model/view model, so a collapse choice from any source transfers to the combined prompt; later expansion survives rerenders.
Added service regressions using the public queue/dequeue APIs, including automatic queued/steering dispatch, merged steering, slash commands and retries, and blocked troubleshoot requests. The renderer tests now complement those service tests instead of being the only evidence for the transition. All 101 ChatService tests and 70 renderer tests pass (one existing renderer test remains skipped).
| } | ||
|
|
||
| updateViewModel(viewModel: IChatViewModel | undefined): void { | ||
| if (this.viewModel?.sessionResource.toString() !== viewModel?.sessionResource.toString()) { |
There was a problem hiding this comment.
Fixed in 49af945: updateViewModel now uses the existing isEqual helper for session resources. The session-switch regression still passes as part of the renderer suite.
| templateData.elementDisposables.add(this.hoverService.setupManagedHover(getDefaultHoverDelegate('element'), button.element, () => expanded | ||
| ? localize('collapseRequest', "Collapse Message") | ||
| : localize('expandRequestTooltip', "Expand Message"))); |
There was a problem hiding this comment.
Fixed in 49af945: the collapsed managed hover now includes the full normalized preview, using the localized "Expand Message: {0}" text. Added a regression that verifies the expanded hover and complete collapsed hover content.
| .interactive-item-container.chat-request-collapsed .value > :not(.chat-request-collapse-control) { | ||
| display: none !important; |
There was a problem hiding this comment.
Fixed in 49af945. Removed the new !important rule and moved visibility handling into the renderer. It saves each request child's original display value, hides it on collapse, and restores that value on expansion.
The regression uses a real file attachment and verifies flex -> none -> flex, while the existing height and template-recycling tests continue to pass.
| const button = templateData.elementDisposables.add(new ButtonWithIcon(control, {})); | ||
| button.iconElement.setAttribute('aria-hidden', 'true'); |
There was a problem hiding this comment.
Fixed in 49af945: added a localized paragraph to the existing Chat Accessibility Help explaining Tab/Shift+Tab navigation, Enter/Space activation, and independent queued-message collapse states. Scoped ESLint and hygiene checks pass.
|
lakshaykhattar (@LakshayKhattar) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Long queued prompts take up chat space without a way to collapse them. Add a keyboard-accessible chevron that collapses each queued or steering message into a single-line preview. Each message keeps its own expansion state across rendering updates, queue reordering, and the transition into the transcript. Collapsed messages are excluded from sticky-scroll headers.
The local queue processor preserves request identity. When steering messages merge, the combined prompt inherits an explicit collapse choice from any source message; users can then expand or collapse the combined prompt normally. The collapsed hover exposes the full preview, and Chat Accessibility Help documents the keyboard controls.
Fixes #326123.
Validation:
git diff --checkpassed.To test:
Runtime evidence:
Watch the contributor's recording (MP4, 64 seconds). At approximately 0:48, one queued prompt is collapsed while the other remains expanded; at 0:55, both are collapsed independently while the current response continues streaming. The original recording is hosted as an evidence asset on the contributor's fork.
The recording shows the disclosure UI at commit 53e4074. The dequeue and merged-steering fixes in 49af945 are covered by the automated tests above.