Skip to content

fix(chat): guard setVisible against unrendered widget child parts and align ChatEditor lifecycle - #333943

Open
Zain Nadeem (zainnadeem786) wants to merge 4 commits into
microsoft:mainfrom
zainnadeem786:fix/chat-editor-setvisible-undefined
Open

fix(chat): guard setVisible against unrendered widget child parts and align ChatEditor lifecycle#333943
Zain Nadeem (zainnadeem786) wants to merge 4 commits into
microsoft:mainfrom
zainnadeem786:fix/chat-editor-setvisible-undefined

Conversation

@zainnadeem786

Copy link
Copy Markdown
Contributor

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

  1. In ChatWidget.setVisible(), this.listWidget and this.input were directly invoked without verifying whether render() had completed instantiating child widgets.
  2. dom.getWindow(this.listContainer) in ChatWidget.setVisible() lacked nullability validation on this.listContainer.
  3. ChatEditor.createEditor() called this.widget.setVisible(true) eagerly during creation instead of leaving visibility management to EditorPanes.setEditorVisible().

Fixes

  • Added optional chaining in ChatWidget.setVisible() for listWidget?.setVisible() and inputPartDisposable.value?.setVisible().
  • Added null-check guard for this.listContainer before scheduling animation frames.
  • Removed redundant eager this.widget.setVisible(true) in ChatEditor.createEditor().
  • Added defensive optional chaining in QuickChat.hide() and QuickChat.show().

Verification Steps

  1. Open Chat panel and trigger "Open Chat in Editor" via Command Palette or editor action.
  2. Trigger "Open Chat in New Window" / auxiliary window.
  3. Verify that chat editors initialize cleanly without modal error dialogs.

Copilot AI balanced review requested due to automatic review settings September 2, 2026 07:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts
Comment thread src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread src/vs/workbench/contrib/chat/test/browser/widget/chatWidget.test.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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

@zainnadeem786

Copy link
Copy Markdown
Contributor Author

All Copilot review feedback items have been resolved:

  • Inline input visibility forwarding applied.
  • Added regression tests for setVisible prior to render().
  • Cleaned up imports and updated test DOM nodes to use mainWindow.document.

Ready for maintainer review/merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Opening Chat in a new editor/window fails with "Cannot read properties of undefined (reading 'setVisible')"

3 participants