fix: Implement analysis error investigation fixes#2
Conversation
There was a problem hiding this comment.
Pull request overview
This PR applies a set of robustness fixes to the Source Doc VS Code extension (CodeLens + Copilot explanations), focusing on error handling, edge cases, and lifecycle management.
Changes:
- Improve Copilot
LanguageModelErrormessaging and avoid caching/returning empty explanations. - Add guards for “nothing to explain” and empty documents (range creation).
- Address minor cleanup issues (e.g., double semicolon) and attempt to fix CodeLens registration disposal on language config changes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/extension.ts |
Adjusts language re-registration disposal behavior and adds a no-op guard for explainFile when there are no explainable lines. |
src/explanationProvider.ts |
Adds LanguageModelError code-specific messaging and rejects empty Copilot responses. |
src/codeLensProvider.ts |
Guards file-level lens range creation for empty documents and fixes a stray double semicolon. |
| if (e.affectsConfiguration('sourceDoc.languages')) { | ||
| languageRegistrations.forEach(d => d.dispose()); | ||
| languageRegistrations = []; | ||
| registerCodeLensProviders(context, codeLensProvider, languageRegistrations); | ||
| languageRegistrations.forEach(d => context.subscriptions.push(d)); | ||
| } |
There was a problem hiding this comment.
The language re-registration logic is still leaking/duplicating CodeLens providers: the initial registerCodeLensProviders(context, codeLensProvider) call registers directly into context.subscriptions, but those initial registrations are never captured in languageRegistrations, so they are never disposed when sourceDoc.languages changes. This means removed languages can continue to have a provider registered, and new languages can end up with multiple registrations.
Consider tracking all language registrations in languageRegistrations (including the initial registration) and disposing that set on every languages change, rather than pushing new registrations into context.subscriptions each time.
| result = result.trim(); | ||
| if (!result) { | ||
| throw new Error('Copilot returned an empty explanation. Please try again.'); | ||
| } |
There was a problem hiding this comment.
The new if (!result) throw ... check will treat user cancellation as an error in some cases: if the cancellation token is triggered during streaming before any text is received, the loop breaks, result stays empty, and an error message is thrown/shown. Cancellation should typically short-circuit without surfacing a failure.
Consider explicitly checking token.isCancellationRequested after the streaming loop and throwing a cancellation-specific error (or returning early) so callers can ignore it, while still preventing empty explanations from being cached.
src/explanationProvider.ts
Outdated
| throw new Error('Copilot access denied. Please check your GitHub Copilot subscription.'); | ||
| case 'Blocked': | ||
| throw new Error('Copilot request was blocked. Please check your GitHub Copilot settings.'); | ||
| case 'NotFound': | ||
| throw new Error('Copilot model not found. Please check your GitHub Copilot extension.'); | ||
| case 'RequestFailed': | ||
| throw new Error('Copilot request failed. Please try again.'); |
There was a problem hiding this comment.
For the specific LanguageModelError codes handled here, the thrown messages no longer include the underlying err.code. Elsewhere (and per the extension’s error-handling goals) the code is useful for debugging/support; consider including the code in these user-facing messages as well (e.g., "Copilot error (NoPermissions): …").
| throw new Error('Copilot access denied. Please check your GitHub Copilot subscription.'); | |
| case 'Blocked': | |
| throw new Error('Copilot request was blocked. Please check your GitHub Copilot settings.'); | |
| case 'NotFound': | |
| throw new Error('Copilot 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}): access denied. Please check your GitHub Copilot subscription.`); | |
| case 'Blocked': | |
| throw new Error(`Copilot error (${err.code}): request was blocked. Please check your GitHub Copilot settings.`); | |
| case 'NotFound': | |
| throw new Error(`Copilot error (${err.code}): model not found. Please check your GitHub Copilot extension.`); | |
| case 'RequestFailed': | |
| throw new Error(`Copilot error (${err.code}): request failed. Please try again.`); |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@moonolgerd I've opened a new pull request, #3, to work on those changes. Once the pull request is ready, I'll request review from you. |
…uageModelError codes missing from messages (#3) * Initial plan * fix: address review comments - registration leak, cancellation, and error codes Co-authored-by: moonolgerd <3743184+moonolgerd@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: moonolgerd <3743184+moonolgerd@users.noreply.github.com>
Implements the fixes documented in
analysis-error-investigation.md, addressing LanguageModelError propagation, empty cache issues, double semicolons, disposable leaks on config changes, and empty file guard checks.