fix: guard Quick Search "Open in Search View" against unmaterialized tree element (fixes #326646)#326652
Conversation
…tree element (fixes #326646) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot could not run the full agentic suite for this review because it was automatically requested on a bot-authored pull request. Request a review from Copilot under Reviewers to retry with the full agentic suite. Improved support for bot-authored pull requests is coming soon.
Adds a defensive guard in Quick Access text search selection logic to avoid errors when the async tree hasn’t materialized the target element yet.
Changes:
- Add an explanatory comment describing the “Tree element not found” failure mode in the async tree.
- Guard focusing/revealing a match with
hasNodeto avoid acting on non-materialized elements.
| if (currentElem && viewer?.hasNode(currentElem)) { | ||
| viewer.setFocus([currentElem], getSelectionKeyboardEvent()); | ||
| viewer.setSelection([currentElem], getSelectionKeyboardEvent()); | ||
| viewer.reveal(currentElem); |
There was a problem hiding this comment.
Fixed in d910a70: replaced the optional-chaining condition with an explicit currentElem && viewer && viewer.hasNode(currentElem) guard so viewer is narrowed to non-undefined before setFocus/setSelection/reveal.
Generated by errors-fix-driver · 770.3 AIC · ⌖ 34.2 AIC · ⊞ 72.9K
| const viewer: WorkbenchCompressibleAsyncDataTree<ISearchResult, RenderableMatch> | undefined = viewlet?.getControl(); | ||
| if (currentElem) { | ||
| // The async data tree may not have materialized `currentElem` yet (e.g. its parent | ||
| // file match has not been expanded/loaded), in which case selecting or revealing it | ||
| // throws `TreeError [SearchView] Tree element not found`. Guard on `hasNode` before | ||
| // touching the tree and fall back to focusing the search widget otherwise. | ||
| if (currentElem && viewer?.hasNode(currentElem)) { | ||
| viewer.setFocus([currentElem], getSelectionKeyboardEvent()); | ||
| viewer.setSelection([currentElem], getSelectionKeyboardEvent()); | ||
| viewer.reveal(currentElem); |
There was a problem hiding this comment.
Fixed in d910a70: the condition now uses an explicit currentElem && viewer && viewer.hasNode(currentElem) check, narrowing viewer to a non-undefined type inside the block before the tree method calls.
Generated by errors-fix-driver · 770.3 AIC · ⌖ 34.2 AIC · ⊞ 72.9K
…ow viewer before tree calls Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
| // The async data tree may not have materialized `currentElem` yet (e.g. its parent | ||
| // file match has not been expanded/loaded), in which case selecting or revealing it | ||
| // throws `TreeError [SearchView] Tree element not found`. Guard on `hasNode` before | ||
| // touching the tree and fall back to focusing the search widget otherwise. | ||
| if (currentElem && viewer && viewer.hasNode(currentElem)) { |
There was a problem hiding this comment.
The explicit fallback is already present locally: the else branch of this same if calls viewlet.searchAndReplaceWidget.focus(). The guard change (currentElem && viewer && viewer.hasNode(currentElem)) simply routes the "node not yet materialized" case into that existing else, so the unmaterialized-node path now degrades to focusing the search widget — exactly what the comment describes. No rewording or additional else handling is needed.
Generated by errors-fix-driver · 476.2 AIC · ⌖ 24.2 AIC · ⊞ 72.9K
Summary
TreeError [SearchView] Tree element not foundis thrown when a user moves a Quick Search (Go to File / text search quick access) result into the full Search view via the "Open in Search View" / "More" / "See More Files" affordances.TextSearchQuickAccess.moveToSearchViewlettransfers its search model to the Search view and then callsviewer.setSelection([currentElem])on the view'sWorkbenchCompressibleAsyncDataTree. Because that tree lazy-loads its nodes,currentElem(often a deepMatchunder a not-yet-expanded file, or a file match beyond the visible limit) may not be materialized in the tree yet, sosetSelectionthrows. The impact is an unhandled error surfaced to telemetry and a failed "open in search view" gesture.Fixes #326646
Recommended reviewer:
@osortegaCulprit Commit
88056f2@andreamahWorkbenchCompressibleAsyncDataTreefor lazy loading AI results (#228886)moveToSearchViewletkept callingsetSelection/revealwith an element reference that a sync tree always contained but the async tree only contains once its parent chain has been loaded, so the pre-existing call became able to throwTree element not found.Code Flow
sequenceDiagram participant User as Quick Search pick participant QA as TextSearchQuickAccess.moveToSearchViewlet participant View as SearchView.replaceSearchModel participant Tree as CompressibleAsyncDataTree participant Model as CompressedObjectTreeModel User->>QA: accept "Open in Search View" / "More" QA->>View: replaceSearchModel(model) (queues async refresh) Note over View: setInput + queued updateChildren;<br/>deep nodes not yet materialized QA->>Tree: setSelection([currentElem]) Tree->>Model: map element -> node (getNode/getCompressedNode) Note over Model: ⚠️ element not in nodes map Note over Model: 💥 TreeError [SearchView] Tree element not foundAffected Files
src/vs/base/browser/ui/tree/compressedObjectTreeModel.tsthrow new TreeError(this.user, 'Tree element not found: ...')(from stack)src/vs/workbench/contrib/search/browser/quickTextSearch/textSearchQuickAccess.tsviewer.setSelection([currentElem])called on a lazy async tree with an element (matches[limit]L256 / deepelementL291) that may not be materializedsrc/vs/base/browser/ui/tree/asyncDataTree.tshasNode()reports whether an element has been materialized in the async treeRepro Steps
setSelectionthrowsTreeError [SearchView] Tree element not found. Timing-dependent: larger result sets and collapsed files increase the likelihood.How the Fix Works
Chosen approach —
textSearchQuickAccess.ts→moveToSearchViewlet(): guard thesetFocus/setSelection/revealblock onviewer?.hasNode(currentElem). When the element has not been materialized in the async tree, fall back to focusing the search widget — the exact behavior the existingelsebranch already provides for the "no element" case. This checks a documented precondition of the async-data-tree API before invoking it, at the site that produces the invalid call (fix at the caller that makes the unsupported call, not at the shared tree crash site). It reuses the samehasNodematerialization check already used elsewhere in this view (RefreshTreeController.refreshTree,searchView.tsL2769), so it introduces no new contract and notry/catchthat would hide errors from telemetry.Lifecycle pattern: async-init race (stale/unmaterialized cached reference) — the consumer holds a
RenderableMatchthat the freshly-input async tree has not yet loaded.Producer site:
src/vs/workbench/contrib/search/browser/searchView.ts:469—replaceSearchModelcallssetInputand only queues (does not synchronously complete) the tree refresh, so deep nodes are absent when control returns to the caller.Why this fix: after this change,
textSearchQuickAccess.ts:220only callssetSelection/revealwhenhasNodeconfirms the element exists, soCompressedObjectTreeModel.getCompressedNodecan no longer receive an absent element from this path.Alternatives considered: awaiting a full recursive tree materialization/
expandToinsidereplaceSearchModelbefore returning — rejected because it would force-expand collapsed file matches (changing UX) and broaden the change well beyond the failing path for a gesture whose graceful degradation (focus the widget) already exists.Recommended Owner
@osortega— current Search View owner permicrosoft/vscode-engineeringtriage/working-areas.md(the culprit-commit author's change is effectively pre-existing; area owner selected as the live, authoritative owner of Search View UI / quick search).errors-fix-driver — cycle 1
Trigger: cron_check_failed · Head:
d910a70fea1(d910a70)textSearchQuickAccess.ts:224— optional chaining doesn't narrowviewer(in scope)d910a70(replied + resolved)textSearchQuickAccess.ts:227— same narrowing concern (in scope)d910a70(replied + resolved)Linux / Electron(prior SHA)Push: yes —
d910a70· Copilot rerequested: okReady gate: ci pending after push → not marking ready this cycle