Repository navigation
fix: guard unlockFromCodingAgent against uninitialized input part (fixes #314442) - #314520
vs-code-engineering[bot] wants to merge 2 commits into
Conversation
|
Base:
|
|
Copilot resolve the merge conflicts in this pull request |
There was a problem hiding this comment.
Pull request overview
Guards ChatWidget.unlockFromCodingAgent() against being called before the widget input part is initialized, addressing a startup/session-load crash in the Chat/Agents workbench flow (fixes #314442).
Changes:
- Adds an early-return in
unlockFromCodingAgent()wheninputPartDisposable.valueis not yet set.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts | Adds a guard in unlockFromCodingAgent() to avoid dereferencing the input part before render/initialization. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 1
a2662fd to
129524a
Compare
#314442) The inputEditor getter crashes when inputPartDisposable.value is undefined (widget not yet rendered). Add early return guard matching existing pattern in renderWelcomeViewContentIfNeeded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
129524a to
f709889
Compare
| // Only touch the input editor if the active input part has been rendered. | ||
| // `this.input` may resolve to either the main or inline input part, both of which | ||
| // use non-null assertions internally and would throw before being initialized. | ||
| const activeInputPart = this.viewModel?.editing && this.configurationService.getValue<string>('chat.editRequests') !== 'input' | ||
| ? this.inlineInputPartDisposable.value | ||
| : this.inputPartDisposable.value; | ||
| if (activeInputPart) { | ||
| this.inputEditor.updateOptions({ placeholder: undefined }); | ||
| } |
There was a problem hiding this comment.
Landed this cycle: updateOptions is now called via activeInputPart.inputEditor directly (the same null-checked part the guard resolves), so it targets the correct editor (inline vs main) and no longer routes through the this.inputEditor/this.input getter that could throw before initialization.
Generated by errors-fix-driver · 1.7K AIC · ⌖ 99.6 AIC · ⊞ 72.9K
There was a problem hiding this comment.
Addressed: the update now uses activeInputPart.inputEditor.updateOptions(...) (same resolved part as the guard), so this.inputEditor is no longer evaluated before the input part is rendered.
Generated by errors-fix-driver · 1.2K AIC · ⌖ 55.9 AIC · ⊞ 72.9K
There was a problem hiding this comment.
Addressed by the merge with origin/main this cycle: the update now targets activeInputPart.inputEditor directly, so the guarded part and the editor being updated are the same and the uninitialized-getter path is avoided.
Generated by errors-fix-driver · 1.2K AIC · ⌖ 164.8 AIC · ⊞ 72.9K
| // Only touch the input editor if the active input part has been rendered. | ||
| // `this.input` may resolve to either the main or inline input part, both of which | ||
| // use non-null assertions internally and would throw before being initialized. | ||
| const activeInputPart = this.viewModel?.editing && this.configurationService.getValue<string>('chat.editRequests') !== 'input' | ||
| ? this.inlineInputPartDisposable.value | ||
| : this.inputPartDisposable.value; | ||
| if (activeInputPart) { | ||
| this.inputEditor.updateOptions({ placeholder: undefined }); | ||
| } |
There was a problem hiding this comment.
Addressed in this cycle's merge-conflict resolution. The placeholder reset now goes through an editor reference owned by the null-checked part — activeInputPart.inputEditor.updateOptions(...) — exactly the coupling you suggested. It no longer re-evaluates the throwing this.inputEditor getter chain (which dereferences this.inputPartDisposable.value! and throws before returning, so optional chaining on its result would not have helped). The condition and the updated object are now the same activeInputPart.
Generated by errors-fix-driver · 1.7K AIC · ⌖ 143.5 AIC · ⊞ 72.9K
There was a problem hiding this comment.
Addressed: the call now routes through activeInputPart.inputEditor, which is the exact part checked in the guard, so a truthy activeInputPart guarantees the editor being updated is initialized.
Generated by errors-fix-driver · 1.2K AIC · ⌖ 55.9 AIC · ⊞ 72.9K
| // Only touch the input editor if the active input part has been rendered. | ||
| // `this.input` may resolve to either the main or inline input part, both of which | ||
| // use non-null assertions internally and would throw before being initialized. | ||
| const activeInputPart = this.viewModel?.editing && this.configurationService.getValue<string>('chat.editRequests') !== 'input' |
There was a problem hiding this comment.
Thanks for the suggestion. This PR's scope is bounded by #314442: guarding unlockFromCodingAgent against an uninitialized input part. Centralizing the chat.editRequests 'input' literal into a shared constant is a broader readability refactor that touches code beyond the changed lines, so I'm leaving it for a separate follow-up. Note the same 'input' comparison already exists in the input/inlineInputPart getters, so a fix should be done consistently across all sites. Feel free to open a tracking issue.
Generated by errors-fix-driver · 1.7K AIC · ⌖ 143.5 AIC · ⊞ 72.9K
| if (activeInputPart) { | ||
| this.inputEditor.updateOptions({ placeholder: undefined }); | ||
| } |
There was a problem hiding this comment.
Addressed in the latest merge-with-main resolution. The placeholder reset now goes through the null-checked active input part directly — activeInputPart.inputEditor.updateOptions({ placeholder: undefined }) — instead of the this.inputEditor getter chain (which can throw via this.input's non-null assertions before the part is rendered). The editor is now accessed on the same part the readiness condition guards, so there's no unguarded access path.
Generated by errors-fix-driver · 1.4K AIC · ⌖ 35.2 AIC · ⊞ 72.9K
There was a problem hiding this comment.
Addressed: rather than restoring the optional chaining on this.inputEditor, the call now uses activeInputPart.inputEditor.updateOptions(...), which is only reached when the resolved active input part exists — so it is safe without optional chaining.
Generated by errors-fix-driver · 1.2K AIC · ⌖ 55.9 AIC · ⊞ 72.9K
| const activeInputPart = this.viewModel?.editing && this.configurationService.getValue<string>('chat.editRequests') !== 'input' | ||
| ? this.inlineInputPartDisposable.value | ||
| : this.inputPartDisposable.value; |
There was a problem hiding this comment.
Thanks for the suggestion. This PR's scope is bounded by #314442: guarding unlockFromCodingAgent against an uninitialized input part. The 'chat.editRequests' === 'input' literal is shared with the existing input/inlineInputPart getters, so centralizing it into a shared constant/helper should be done consistently across all call sites in a dedicated follow-up rather than partially here. Leaving it out of scope for this fix.
Generated by errors-fix-driver · 1.4K AIC · ⌖ 35.2 AIC · ⊞ 72.9K
| if (activeInputPart) { | ||
| this.inputEditor.updateOptions({ placeholder: undefined }); | ||
| } |
There was a problem hiding this comment.
Addressed in this cycle's merge-with-main conflict resolution. The placeholder reset no longer goes through the this.inputEditor getter (which throws internally via this.input's non-null assertion when the part is uninitialized). It now updates options on the already null-checked part directly — activeInputPart.inputEditor.updateOptions({ placeholder: undefined }) — so the getter chain that produced Cannot read properties of undefined (reading 'inputEditor') is never re-evaluated. activeInputPart and this.input resolve via identical selection logic, so the null-check on activeInputPart is a sufficient guarantee for the direct activeInputPart.inputEditor access.
Generated by errors-fix-driver · 1.4K AIC · ⌖ 78.7 AIC · ⊞ 72.9K
There was a problem hiding this comment.
Addressed: the editor update now uses activeInputPart.inputEditor, the same part validated by the guard, so its truthiness does guarantee the editor being updated is initialized.
Generated by errors-fix-driver · 1.2K AIC · ⌖ 55.9 AIC · ⊞ 72.9K
| // Only touch the input editor if the active input part has been rendered. | ||
| // `this.input` may resolve to either the main or inline input part, both of which | ||
| // use non-null assertions internally and would throw before being initialized. | ||
| const activeInputPart = this.viewModel?.editing && this.configurationService.getValue<string>('chat.editRequests') !== 'input' | ||
| ? this.inlineInputPartDisposable.value | ||
| : this.inputPartDisposable.value; | ||
| if (activeInputPart) { | ||
| this.inputEditor.updateOptions({ placeholder: undefined }); | ||
| } |
There was a problem hiding this comment.
Addressed in this cycle's merge-conflict resolution: the placeholder reset now calls activeInputPart.inputEditor.updateOptions(...) directly on the already-null-checked part, rather than the this.inputEditor getter chain (which resolves through inputPartDisposable.value! and would throw before the part is rendered). The editor instance updated is exactly the one guarded by the if (activeInputPart) check.
Generated by errors-fix-driver · 1.3K AIC · ⌖ 56.5 AIC · ⊞ 72.9K
There was a problem hiding this comment.
Addressed in this cycle's merge with main: the call now goes through activeInputPart.inputEditor.updateOptions(...), so the guard and the actual update reference the same resolved input part rather than the throwing this.inputEditor getter.
Generated by errors-fix-driver · 1.2K AIC · ⌖ 55.9 AIC · ⊞ 72.9K
| const activeInputPart = this.viewModel?.editing && this.configurationService.getValue<string>('chat.editRequests') !== 'input' | ||
| ? this.inlineInputPartDisposable.value | ||
| : this.inputPartDisposable.value; |
There was a problem hiding this comment.
Thanks for the suggestion. This PR's scope is bounded by #314442: guarding unlockFromCodingAgent against an uninitialized input part. The inline active-input-part resolution mirrors the existing get input() getter; extracting a shared helper and centralizing the 'input' mode string is a worthwhile cleanup but falls outside this crash fix, so I'm leaving it for a separate follow-up. Feel free to open a tracking issue if you'd like it addressed.
Generated by errors-fix-driver · 1.3K AIC · ⌖ 56.5 AIC · ⊞ 72.9K
There was a problem hiding this comment.
Thanks — the active-input-part selection is intentionally inlined here to mirror the existing get input() logic in this file. Extracting a shared helper is a reasonable cleanup but falls outside the scope of #314442, so I'm leaving it for a separate follow-up.
Generated by errors-fix-driver · 1.2K AIC · ⌖ 55.9 AIC · ⊞ 72.9K
| // Clear all state related to locking unconditionally so the widget cannot | ||
| // remain stuck in a locked state if this is called before the input part renders. | ||
| this._lockedAgent = undefined; |
There was a problem hiding this comment.
Addressed: after the merge with main there is no early return — all lock/context-key state is cleared unconditionally at the top of unlockFromCodingAgent, and only the input-editor updateOptions call is guarded behind activeInputPart. The "unconditionally" comment now matches the code.
Generated by errors-fix-driver · 1.2K AIC · ⌖ 55.9 AIC · ⊞ 72.9K
| const activeInputPart = this.viewModel?.editing && this.configurationService.getValue<string>('chat.editRequests') !== 'input' | ||
| ? this.inlineInputPartDisposable.value | ||
| : this.inputPartDisposable.value; |
There was a problem hiding this comment.
Thanks — this is a valid readability point, but it's out of scope for this PR, which is bounded by #314442 (guarding unlockFromCodingAgent against an uninitialized input part). The 'input' config comparison mirrors the existing get input() logic elsewhere in this file; extracting/centralizing it is a separate refactor better handled in its own follow-up.
Generated by errors-fix-driver · 1.2K AIC · ⌖ 55.9 AIC · ⊞ 72.9K
| const activeInputPart = this.viewModel?.editing && this.configurationService.getValue<string>('chat.editRequests') !== 'input' | ||
| ? this.inlineInputPartDisposable.value | ||
| : this.inputPartDisposable.value; | ||
| if (activeInputPart) { | ||
| this.inputEditor.updateOptions({ placeholder: undefined }); | ||
| } |
There was a problem hiding this comment.
Addressed in this cycle's merge with main: the placeholder reset now calls activeInputPart.inputEditor.updateOptions(...) instead of this.inputEditor, so the throwing this.inputEditor getter chain is no longer evaluated. The editor is only touched when the resolved active input part actually exists.
Generated by errors-fix-driver · 1.2K AIC · ⌖ 55.9 AIC · ⊞ 72.9K
| const activeInputPart = this.viewModel?.editing && this.configurationService.getValue<string>('chat.editRequests') !== 'input' | ||
| ? this.inlineInputPartDisposable.value | ||
| : this.inputPartDisposable.value; | ||
| if (activeInputPart) { | ||
| this.inputEditor.updateOptions({ placeholder: undefined }); | ||
| } |
There was a problem hiding this comment.
Addressed in the latest push: the placeholder reset now calls activeInputPart.inputEditor.updateOptions(...) directly on the already-resolved input part, instead of the this.inputEditor getter chain (this.input → inputPart → non-null assertion). Since activeInputPart is guaranteed non-null inside the guard, the throwing getter chain is no longer re-evaluated, closing this concern.
Generated by errors-fix-driver · 1.8K AIC · ⌖ 57 AIC · ⊞ 72.9K
| const activeInputPart = this.viewModel?.editing && this.configurationService.getValue<string>('chat.editRequests') !== 'input' | ||
| ? this.inlineInputPartDisposable.value | ||
| : this.inputPartDisposable.value; |
| if (activeInputPart) { | ||
| this.inputEditor.updateOptions({ placeholder: undefined }); | ||
| } |
There was a problem hiding this comment.
Addressed in the latest push: the code now uses activeInputPart.inputEditor.updateOptions(...) directly rather than this.inputEditor, so the guarded, already-resolved input part is used and the potentially-throwing this.inputEditor getter is never re-evaluated.
Generated by errors-fix-driver · 1.8K AIC · ⌖ 57 AIC · ⊞ 72.9K
| const activeInputPart = this.viewModel?.editing && this.configurationService.getValue<string>('chat.editRequests') !== 'input' | ||
| ? this.inlineInputPartDisposable.value | ||
| : this.inputPartDisposable.value; |
| const activeInputPart = this.viewModel?.editing && this.configurationService.getValue<string>('chat.editRequests') !== 'input' | ||
| ? this.inlineInputPartDisposable.value | ||
| : this.inputPartDisposable.value; | ||
| if (activeInputPart) { | ||
| this.inputEditor.updateOptions({ placeholder: undefined }); | ||
| } |
| const activeInputPart = this.viewModel?.editing && this.configurationService.getValue<string>('chat.editRequests') !== 'input' | ||
| ? this.inlineInputPartDisposable.value | ||
| : this.inputPartDisposable.value; |
Bryan Chen (bryanchen-d)
assigned
Justin Chen (justschen) and unassigned
Bryan Chen (bryanchen-d)
🔧 Error Fix
Summary
Cannot read properties of undefined (reading 'inputEditor')e59c74b0-5708-93a6-a810-5b7e891eaf6f530cb5deFixes #314442
Recommended reviewer:
@justschenCulprit Commit
d88ce17dby@justschen— "fix forking not working in claude + agents" introduced theunlockFromCodingAgentmethod that accessesthis.inputEditorwithout guarding for uninitialized input part.Affected Files
src/vs/workbench/contrib/chat/browser/widget/chatWidget.tsHow the Fix Works
unlockFromCodingAgentclears all lock state unconditionally (so the widget can never get stuck locked), then only touches the input editor when the active input part has actually been rendered. It resolves the active input part (inlineInputPartDisposablewhen editing, otherwiseinputPartDisposable) and updates that part's own editor directly —activeInputPart.inputEditor.updateOptions(...)— so the throwingthis.inputEditorgetter chain is never re-evaluated before the part exists.Recommended Owner
@justschen— primary author ofunlockFromCodingAgentand the widget lock/unlock state logic inchatWidget.ts.errors-fix-driver — cycle 19
Trigger: cron_merge_conflict · Head: new SHA assigned by the push handler (two pushes this cycle: merge resolution + review fix).
chatWidget.ts(unlockFromCodingAgent)origin/main: preserved the input-part guard and adopted main'seditable: !this._readOnlyrenderer option. Conflict markers cleared, merge committed and pushed.chatWidget.ts:2268— guard did not actually protect thethis.inputEditorgetter accessactiveInputPart.inputEditor.updateOptions(...)directly on the resolved, guarded part, so the throwingthis.input/this.inputEditorchain is never re-evaluated. This also matches the PR description. Replied + resolved 10 threads (SCKcg,SChzk,SAFw1,SAUtw,SAZM-,SAiXu,SAp1F,SAzeR,SBMA-,SBRdu) — per-cycle resolve cap (10) reached.SBWhX,SBc6t,SBl6w,SBr3y,SBz4F,SB7k7,SCCaK)SAiX0,SAzeY,SBRd0,SBWhc,SBl60,SBr30,SB7lE,SCCaI,SCCaF,SCKcl,SChzn) — extract helper / avoid magic string'input'/ reword commentPush: yes — merge-conflict resolution +
activeInputPart.inputEditorfix (squashed by push handler, new SHA assigned) · Copilot rerequested: okReady gate: pushes just landed → CI pending, Copilot has not yet reviewed the new head, and 18 review threads remain unresolved (resolve cap) → not marking ready this cycle.