Skip to content

fix: CodeLens registration leak, cancellation surfaced as error, LanguageModelError codes missing from messages #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
moonolgerd merged 2 commits into from
Mar 7, 2026
Merged

fix: CodeLens registration leak, cancellation surfaced as error, LanguageModelError codes missing from messages #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c16fe06
Initial plan
Copilot Mar 7, 2026
d1c5792
fix: address review comments - registration leak, cancellation, and e…
Copilot Mar 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 7 additions & 4 deletions src/explanationProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,13 @@ export class ExplanationProvider implements vscode.Disposable {
if (err instanceof vscode.LanguageModelError) {
switch (err.code) {
case 'NoPermissions':
throw new Error('Copilot access denied. Please check your GitHub Copilot subscription.');
throw new Error(`Copilot error (${err.code}): access denied. Please check your GitHub Copilot subscription.`);
case 'Blocked':
throw new Error('Copilot request was blocked. Please check your GitHub Copilot settings.');
throw new Error(`Copilot error (${err.code}): request was blocked. Please check your GitHub Copilot settings.`);
case 'NotFound':
throw new Error('Copilot model not found. Please check your GitHub Copilot extension.');
throw new Error(`Copilot error (${err.code}): model not found. Please check your GitHub Copilot extension.`);
case 'RequestFailed':
throw new Error('Copilot request failed. Please try again.');
throw new Error(`Copilot error (${err.code}): request failed. Please try again.`);
default:
throw new Error(`Copilot error (${err.code}): ${err.message}. Please check your GitHub Copilot extension.`);
}
Expand All @@ -93,6 +93,9 @@ export class ExplanationProvider implements vscode.Disposable {
}

result = result.trim();
if (token.isCancellationRequested) {
throw new vscode.CancellationError();
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
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;

Copilot uses AI. Check for mistakes.
}
Comment on lines +96 to +98
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
if (!result) {
throw new Error('Copilot returned an empty explanation. Please try again.');
}
Expand Down
15 changes: 9 additions & 6 deletions src/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,13 @@ export function activate(context: vscode.ExtensionContext): void {
);

// ── Register CodeLens provider for each configured language ──────────────
registerCodeLensProviders(context, codeLensProvider);
// Track ALL registrations in languageRegistrations (not context.subscriptions)
// so the full set can be disposed when sourceDoc.languages changes.
let languageRegistrations: vscode.Disposable[] = [];
registerCodeLensProviders(codeLensProvider, languageRegistrations);

// Dispose the language registrations when the extension is deactivated.
context.subscriptions.push({ dispose: () => languageRegistrations.forEach(d => d.dispose()) });

// Refresh lenses when the user switches to a different editor tab
context.subscriptions.push(
Expand All @@ -43,14 +49,12 @@ export function activate(context: vscode.ExtensionContext): void {
);

// Re-register providers when sourceDoc.languages changes
let languageRegistrations: vscode.Disposable[] = [];
context.subscriptions.push(
vscode.workspace.onDidChangeConfiguration(e => {
if (e.affectsConfiguration('sourceDoc.languages')) {
languageRegistrations.forEach(d => d.dispose());
languageRegistrations = [];
registerCodeLensProviders(context, codeLensProvider, languageRegistrations);
languageRegistrations.forEach(d => context.subscriptions.push(d));
registerCodeLensProviders(codeLensProvider, languageRegistrations);
}
}),
);
Expand Down Expand Up @@ -206,9 +210,8 @@ export function deactivate(): void {
// ── Helpers ───────────────────────────────────────────────────────────────────

function registerCodeLensProviders(
context: vscode.ExtensionContext,
provider: SourceDocCodeLensProvider,
into: vscode.Disposable[] = context.subscriptions as unknown as vscode.Disposable[],
into: vscode.Disposable[],
): void {
const config = vscode.workspace.getConfiguration('sourceDoc');
const languages = config.get<string[]>('languages') ?? [
Expand Down