-
Notifications
You must be signed in to change notification settings - Fork 37.9k
Add unit tests for thinking content part and fix issue with tools/md rendered out of order #289784
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds comprehensive unit tests for ChatThinkingContentPart and fixes an ordering issue where markdown/thinking content was rendered before tool invocations when the content arrived while the thinking section was collapsed.
Changes:
- Added 1043 lines of unit tests covering lazy rendering, display modes (Collapsed, CollapsedPreview, FixedScrolling), state management, content updates, and DOM structure
- Fixed ordering bug by making
setupThinkingContaineruse lazy rendering when collapsed, ensuring markdown and tool items render in the order they arrive - Introduced discriminated union type
ILazyItemwith variantsILazyToolItemandILazyThinkingItemto handle both tool and thinking items in the lazy rendering queue
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatThinkingContentPart.test.ts | New test file with comprehensive coverage of ChatThinkingContentPart functionality, including regression tests for the ordering bug |
| src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatThinkingContentPart.ts | Refactored lazy rendering system to support both tool and thinking items, ensuring correct DOM ordering when content arrives while collapsed |
Comments suppressed due to low confidence (2)
src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatThinkingContentPart.test.ts:951
- The comment on line 951 is confusing. It says "When the id is the same, hasSameContent returns true (other.id !== this.id is false)" but the test name and assertion expect false to be returned. Looking at the implementation (line 946 in the source:
return other?.id !== this.id), when the IDs are the same,other.id !== this.idevaluates to false, so the method returns false. The comment should be clarified to: "When the id is the same, other.id !== this.id is false, so hasSameContent returns false"
// When the id is the same, hasSameContent returns true (other.id !== this.id is false)
src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatThinkingContentPart.test.ts:724
- The test name says "BUG: markdown renders before tools" but this PR fixes that bug, so the test should now be passing and verifying the correct behavior. Consider updating the test name to reflect that this test now validates the fix works correctly, rather than documenting the bug's existence. For example: "markdown via updateThinking should preserve order with lazy tool items (regression test)"
test('markdown via updateThinking should preserve order with lazy tool items (BUG: markdown renders before tools)', () => {
| // BUG: Currently markdown is always first because it's not lazy | ||
| assert.ok(tool1Index < markdownIndex, | ||
| `BUG: Tool1 (index ${tool1Index}) should come BEFORE markdown (index ${markdownIndex}) ` + | ||
| `because tool1 was appended first. Current DOM order indicates markdown is eagerly ` + | ||
| `placed first regardless of arrival order.`); |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment on lines 800-804 suggests the bug still exists ("BUG: Currently markdown is always first"), but this PR fixes the bug. Since the assertion expects tool1 to come before markdown (and should now pass with the fix), update this comment to reflect that this is now the correct behavior and the test validates the fix works. For example: "With the fix, tool1 should come before markdown because it was appended first."
This issue also appears in the following locations of the same file:
- line 951
- line 724
…rendered out of order (microsoft#289784)
No description provided.