clean up css + fix questions carousel reload#295084
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where question carousels were not being properly skipped on reload, and modernizes the question carousel CSS using nesting for better organization and readability.
Changes:
- Mark question carousels as used during chat session deserialization to prevent them from showing as interactive after reload
- Consolidate question carousel CSS from chat.css into chatQuestionCarousel.css
- Refactor CSS to use nesting, improving specificity control and readability
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/common/model/chatModel.ts | Adds logic to mark question carousel parts as used during deserialization, preventing them from being interactive after reload |
| src/vs/workbench/contrib/chat/browser/widget/media/chat.css | Removes legacy question carousel CSS that has been moved to the dedicated carousel CSS file |
| src/vs/workbench/contrib/chat/browser/widget/chatContentParts/media/chatQuestionCarousel.css | Consolidates all carousel CSS and reorganizes using CSS nesting for better maintainability and specificity control |
| background-color: var(--vscode-list-activeSelectionBackground); | ||
| color: var(--vscode-list-activeSelectionForeground); | ||
|
|
||
| .chat-question-label { |
There was a problem hiding this comment.
Typo in CSS class name: should be .chat-question-list-label instead of .chat-question-label. The actual class used in the HTML is chat-question-list-label (defined at line 150), not chat-question-label. This typo will prevent the color override from being applied to selected list items.
| .chat-question-label { | |
| .chat-question-list-label { |
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/media/chatQuestionCarousel.css
Show resolved
Hide resolved
| // Mark any question carousels as used/skipped on reload since they can no longer be interacted with | ||
| const responseContent = raw.response ?? [new MarkdownString(raw.response)]; | ||
| if (Array.isArray(responseContent)) { | ||
| for (const part of responseContent) { | ||
| if (part && 'kind' in part && part.kind === 'questionCarousel' && !part.isUsed) { | ||
| part.isUsed = true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new logic to mark question carousels as used during deserialization lacks test coverage. While this follows the same pattern as confirmation handling (line 1150 in chatModel.ts), it would be valuable to add a test that verifies questionCarousel parts are correctly marked as isUsed=true during deserialization to prevent them from showing as interactive after reload. This would ensure the fix remains robust as the code evolves.
Consider adding a test similar to the pattern in chatModel.test.ts that creates a serializable request with a questionCarousel response part, deserializes it, and asserts that the carousel's isUsed property is true.
2 participants
cleans up a lot of legacy css + css nesting for better specificity and readability
cc @karthiknadig