fix(chat): every reveal of the chat view handles its own rejection - #77
Merged
Conversation
Follow-up to #76, which fixed the two instances in the code it introduced and left the older ones alone to keep that review diff readable. `executeCommand` returns a Thenable, so a bare call in a void context turns any rejection into an unhandled promise rejection in the extension host — attributed to nothing, which is the part that makes it useless. SIX SITES, NOT SEVEN. The brief for this change listed the `levelcode.ai.focus` registration as needing a `return`. It does not: `() => vscode.commands.executeCommand(…)` is a concise arrow body, so it already returns the thenable and VS Code already reports its failures. Left alone. The other six are all BACKGROUND reveals — addSelection, addContext, resumeSession, the editor panel's onDidDispose, the sessions view's newSession, and handleLaunch. In each the accompanying work has already succeeded by the time the reveal runs, so failing that work because the panel would not come forward would be worse than the panel not coming forward. They route through one `focusChatView(why)` helper that logs and never rejects. Logged, not swallowed: the complaint was "attributed to nothing", so `.catch(() => {})` would answer the letter of it and none of the substance. `why` names the caller in the log. ONE GUARD FOR THE WHOLE CLASS. Rather than six assertions naming six functions, the test scans every occurrence of the call and accepts only returned / awaited / concise-arrow / helper-wrapped / inline-handled forms. That covers the seventh site and the eighth nobody has written yet — which matters, because this pattern reached six copies precisely because nothing was watching for it. Verified non-vacuous against develop: the same scan reports exactly the six bare sites there (lines 423, 468, 1055, 2298, 2435, 2531) and zero here. Bypasses, each reverted and confirmed to fail: - a single site returned to a bare call; the switch-case site returned to a bare call - the focus command wrapped in a block body so it stops returning (proves the guard protects the site that was already correct) - the helper swallowing silently; the log dropping its caller attribution One assertion deliberately relaxed after bypassing it: pinning `.then(undefined, …)` over `.catch(…)` failed a refactor with no behavioural difference. It now accepts either and still fails when the handler stops logging. 23 tests in chatSurface, 32 suites green.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens levelcode-ai chat view “reveal/focus” calls so background reveals never create unhandled promise rejections in the extension host, and adds a test guard to prevent regressions.
Changes:
- Introduces
focusChatView(why)to wrapvscode.commands.executeCommand('levelcodeAi.chat.focus')for fire-and-forget reveals and log failures with caller attribution. - Routes six background reveal sites through the shared helper instead of issuing bare
executeCommandcalls. - Adds a test that scans all
executeCommand('levelcodeAi.chat.focus')call sites and enforces that each occurrence is handled (returned/awaited/helper-wrapped/inline-handled).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| extensions/levelcode-ai/extension.js | Adds focusChatView() and replaces background bare focus calls with helper-wrapped calls to avoid unhandled rejections. |
| extensions/levelcode-ai/test/chatSurface.test.js | Updates existing expectation and adds a repo-guarding test to enforce all focus calls are handled. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #76, which fixed the two instances in the code it introduced and deliberately left the older ones alone to keep that review diff readable.
vscode.commands.executeCommandreturns a Thenable, so a bare call in a void context turns any rejection into an unhandled promise rejection in the extension host — attributed to nothing, which is the part that makes it useless.Six sites, not seven
The follow-up note listed the
levelcode.ai.focusregistration as needing areturn. It doesn't:That's a concise arrow body, so it already returns the thenable and VS Code already reports its failures. Left alone — but now covered by the guard below, which is arguably more useful than changing it would have been.
The other six are all background reveals
addSelection,addContext,resumeSession, the editor panel'sonDidDispose, the sessions view'snewSession, andhandleLaunch.In every one, the accompanying work — a selection added, a session resumed, a login launched — has already succeeded by the time the reveal runs. Failing that work because the panel wouldn't come forward would be worse than the panel not coming forward. So they route through one helper that logs and never rejects:
Logged, not swallowed. The complaint was "attributed to nothing", so
.catch(() => {})would answer the letter of it and none of the substance —whynames the caller.One guard for the whole class
Rather than six assertions naming six functions, the test scans every occurrence of the call and accepts only the handled forms: returned, awaited, concise-arrow, helper-wrapped, or inline-handled. That covers the seventh site and the eighth nobody has written yet — which matters, because this pattern reached six copies precisely because nothing was watching for it.
Verified non-vacuous against
develop: the same scan reports exactly the six bare sites there and zero here.Bypasses
Each reverted and confirmed to fail:
One assertion deliberately relaxed after bypassing it. I'd pinned
.then(undefined, …)over.catch(…)— but those are equivalent here, so the pin would have failed a refactor that changes nothing. It now accepts either form and still fails when the handler stops logging (verified both directions).23 tests in
chatSurface, 32 suites green.