Skip to content

get errors for a notebook URI #1247

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+26 −7
Open

get errors for a notebook URI #1247

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2b5125c
get problems within notebooks
amunger Oct 6, 2025
b351e28
filter severity
amunger Oct 6, 2025
21581d0
diagnostics for notebook URI
amunger Oct 6, 2025
4c376fb
remove extra filter
amunger Oct 6, 2025
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
33 changes: 26 additions & 7 deletions src/extension/tools/node/getErrorsTool.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ import { ILogService } from '../../../platform/log/common/logService';
import { IPromptPathRepresentationService } from '../../../platform/prompts/common/promptPathRepresentationService';
import { IWorkspaceService } from '../../../platform/workspace/common/workspaceService';
import { getLanguage } from '../../../util/common/languages';
import { isJupyterNotebookUri } from '../../../util/common/notebooks';
import { isLocation } from '../../../util/common/types';
import { coalesce } from '../../../util/vs/base/common/arrays';
import { CancellationToken } from '../../../util/vs/base/common/cancellation';
import { Disposable } from '../../../util/vs/base/common/lifecycle';
import { URI } from '../../../util/vs/base/common/uri';
Expand All @@ -25,7 +27,6 @@ import { DiagnosticContext, Diagnostics } from '../../prompts/node/inline/diagno
import { ToolName } from '../common/toolNames';
import { ICopilotTool, ToolRegistry } from '../common/toolsRegistry';
import { checkCancellation, formatUriForFileWidget, resolveToolInputPath } from './toolUtils';
import { coalesce } from '../../../util/vs/base/common/arrays';

interface IGetErrorsParams {
// Note that empty array is not the same as absence; empty array
Expand Down Expand Up @@ -62,14 +63,17 @@ class GetErrorsTool extends Disposable implements ICopilotTool<IGetErrorsParams>
throw new Error(`Invalid input path ${filePath}`);
}

let diagnostics = range
? findDiagnosticForSelectionAndPrompt(this.languageDiagnosticsService, uri, new Range(...range), undefined)
: this.languageDiagnosticsService.getDiagnostics(uri);

diagnostics = diagnostics.filter(d => d.severity <= DiagnosticSeverity.Warning);
let diagnostics: vscode.Diagnostic[] = [];
if (isJupyterNotebookUri(uri)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be used for all notebooks, not just Jupyter?
I think you can use INotebookService.hasSupportedNotebook to check if Uri is a notebook document

diagnostics = this.getNotebookCellDiagnostics(uri);
} else {
diagnostics = range
? findDiagnosticForSelectionAndPrompt(this.languageDiagnosticsService, uri, new Range(...range), undefined)
: this.languageDiagnosticsService.getDiagnostics(uri);
}

return {
diagnostics,
diagnostics: diagnostics.filter(d => d.severity <= DiagnosticSeverity.Warning),
uri,
};
});
Expand Down Expand Up @@ -140,6 +144,21 @@ class GetErrorsTool extends Disposable implements ICopilotTool<IGetErrorsParams>
return uris.map(formatUriForFileWidget).join(', ');
}

private getNotebookCellDiagnostics(uri: URI) {
const notebook = this.workspaceService.notebookDocuments
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest using findNotebook function from notebooks.ts (that does what you're doing here and you also don't need to do the uri.toString)

.find((doc: { uri: URI }) => doc.uri.toString() === uri.toString());
Copy link
Preview

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

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

The URI comparison using toString() is inefficient and may not handle URI normalization correctly. Consider using URI's built-in comparison methods like uri.toString() === doc.uri.toString() with proper URI normalization, or use VS Code's URI comparison utilities if available.

Suggested change
.find((doc: { uri: URI }) => doc.uri.toString() === uri.toString());
.find((doc: { uri: URI }) => URI.equals(doc.uri, uri));

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's not a real function

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the better approach is to use isEqual function found in resources.ts

if (!notebook) {
this.logService.error(`Notebook not found: ${uri.toString()}, could not retrieve diagnostics`);
return [];
}

return notebook.getCells()
.flatMap((cell) => {
const uri = cell.document.uri;
return this.languageDiagnosticsService.getDiagnostics(uri);
});
}

async provideInput(promptContext: IBuildPromptContext): Promise<IGetErrorsParams | undefined> {
const seen = new Set<string>();

Expand Down
Loading