fix: CodeLens registration leak, cancellation surfaced as error, LanguageModelError codes missing from messages#3
Conversation
…rror codes Co-authored-by: moonolgerd <3743184+moonolgerd@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes three robustness bugs in the extension's CodeLens provider lifecycle and Copilot error handling.
Changes:
extension.ts: Fixes a CodeLens provider registration leak where the initialregisterCodeLensProviders()call was bypassing thelanguageRegistrationstracking array, causing initial registrations to survive extension deactivation and accumulate duplicates on config changes. The fix tracks all registrations inlanguageRegistrationsand uses a closure-capturing wrapper disposable for deactivation cleanup.explanationProvider.ts: Fixes cancellation being surfaced as an empty-response error (by adding atoken.isCancellationRequestedcheck after streaming), and addserr.codetoLanguageModelErrormessages for all specific cases (NoPermissions,Blocked,NotFound,RequestFailed) to match the existing default case.package-lock.json: Version bump from0.2.1to0.3.0to reflect accumulated fixes.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/extension.ts |
Fixes registration leak by removing context parameter from registerCodeLensProviders and tracking all registrations in languageRegistrations with a wrapper disposable |
src/explanationProvider.ts |
Adds cancellation check after streaming and includes err.code in all LanguageModelError messages |
package-lock.json |
Version bump from 0.2.1 to 0.3.0 |
| if (token.isCancellationRequested) { | ||
| throw new vscode.CancellationError(); | ||
| } |
There was a problem hiding this comment.
The new cancellation check (lines 96-98) handles two sub-scenarios: (1) token cancelled before any chunks arrive (result is empty), and (2) token cancelled mid-stream after some chunks have already been accumulated (result is non-empty). The existing test at line 176 only covers the first sub-scenario. The second sub-scenario — where chunks have been partially received before cancellation — is the key case that this PR was introduced to fix (previously it would have thrown the empty-response error only if result was empty), but it has no dedicated test to verify that partial content is discarded and CancellationError is thrown. Adding a test with a fake model that yields chunks and a token cancelled after the first chunk would verify the intended fix.
|
|
||
| result = result.trim(); | ||
| if (token.isCancellationRequested) { | ||
| throw new vscode.CancellationError(); |
There was a problem hiding this comment.
The new CancellationError thrown when token.isCancellationRequested is true after streaming (line 96-98) propagates to callers that do not specifically handle it. In runExplain (extension.ts ~line 310), the generic catch (err) block catches the CancellationError and shows a user-visible error message like "Source Doc: Canceled". In explainFile, the cancellation errors accumulate in the results array and cause a misleading "N line(s) failed" error message to appear at the end.
The callers already guard against cancellation (the if (!token.isCancellationRequested) checks after explain() returns), but they don't guard against CancellationError being thrown. The fix should either check for err instanceof vscode.CancellationError in both catch blocks and silently swallow it, or re-check token.isCancellationRequested in the callers' catch blocks before showing the error.
| throw new vscode.CancellationError(); | |
| // Let callers handle cancellation via the token rather than an exception. | |
| // Do not cache partial results or surface an "empty explanation" error. | |
| return result; |
Three robustness issues in the CodeLens registration lifecycle and Copilot error handling, surfaced by PR review.
Changes
extension.ts— CodeLens provider registration leakInitial
registerCodeLensProviderscall pushed disposables directly intocontext.subscriptions, bypassinglanguageRegistrations. OnsourceDoc.languagesconfig change, only the subsequent registrations were disposed — the initial set survived indefinitely, and re-registrations accumulated duplicates.Now all registrations (initial and subsequent) are tracked exclusively in
languageRegistrations. A single wrapper disposable handles deactivation cleanup viacontext.subscriptions:explanationProvider.ts— Cancellation surfaced as empty-response errorIf the cancellation token fired mid-stream before any chunks arrived,
resultstayed empty and the!resultguard threw a user-visible error. Added an explicit cancellation check before the empty-response guard:explanationProvider.ts—LanguageModelErrorcode absent from messagesSpecific
LanguageModelErrorcases (NoPermissions,Blocked,NotFound,RequestFailed) omittederr.codefrom their messages, unlike the default case. All cases now include the code for debuggability:✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.