fix(chat): guard setVisible against unrendered widget child parts and align ChatEditor lifecycle - #333943
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Inline request inputs no longer receive visibility notifications, and lifecycle regression coverage is missing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes Chat editor/window initialization failures caused by visibility updates before child widgets render.
Changes:
- Guards ChatWidget and QuickChat visibility calls.
- Aligns ChatEditor visibility with the editor lifecycle.
- Adds safe animation-frame handling before list creation.
File summaries
| File | Description |
|---|---|
chatEditor.ts |
Removes eager visibility update. |
chatQuick.ts |
Guards QuickChat visibility calls. |
chatWidget.ts |
Guards unrendered child parts and animation scheduling. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Duplicate test imports prevent the module from compiling.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Duplicate imports in the test file cause TypeScript compilation errors.
Review details
Suppressed comments (1)
src/vs/workbench/contrib/chat/test/browser/widget/chatWidget.test.ts:15
- These imports redeclare bindings already imported on lines 18–28, causing duplicate-identifier errors and preventing this test file from compiling. Keep the new lifecycle import here and use the existing consolidated imports below.
import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../../base/test/common/utils.js';
import { OffsetRange } from '../../../../../../editor/common/core/ranges/offsetRange.js';
import { Range } from '../../../../../../editor/common/core/range.js';
import { acceptAndAwaitSentRequest, ChatWidget, getImmediateSilentSlashCommandPart, layoutChatWidgetForInputHeight } from '../../../browser/widget/chatWidget.js';
import { ChatSendResult, ChatSendResultSent, IChatSendRequestData } from '../../../common/chatService/chatService.js';
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
All Copilot review feedback items have been resolved:
Ready for maintainer review/merge! |
Close #333939
Summary of Changes
Fixes #333939: Opening Chat in a new editor or auxiliary window failed with
"Cannot read properties of undefined (reading 'setVisible')".Root Cause
ChatWidget.setVisible(),this.listWidgetandthis.inputwere directly invoked without verifying whetherrender()had completed instantiating child widgets.dom.getWindow(this.listContainer)inChatWidget.setVisible()lacked nullability validation onthis.listContainer.ChatEditor.createEditor()calledthis.widget.setVisible(true)eagerly during creation instead of leaving visibility management toEditorPanes.setEditorVisible().Fixes
ChatWidget.setVisible()forlistWidget?.setVisible()andinputPartDisposable.value?.setVisible().this.listContainerbefore scheduling animation frames.this.widget.setVisible(true)inChatEditor.createEditor().QuickChat.hide()andQuickChat.show().Verification Steps