-
Notifications
You must be signed in to change notification settings - Fork 3.7k
fix: autocomplete performance issues #8364
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 issues found across 29 files
Prompt for AI agents (all 4 issues)
Understand the root cause of the following 4 issues and fix them.
<file name="extensions/vscode/src/extension/VsCodeExtension.ts">
<violation number="1" location="extensions/vscode/src/extension/VsCodeExtension.ts:638">
Register the `onDidChangeActiveTextEditor` listener with the extension context so it is disposed during deactivation to avoid leaking the handler.</violation>
</file>
<file name="core/autocomplete/snippets/getAllSnippets.ts">
<violation number="1" location="core/autocomplete/snippets/getAllSnippets.ts:131">
This debug console.log runs on every file read from the recently opened cache, so autocomplete will emit a log per file on each invocation; this adds synchronous logging overhead and floods the console, undermining the performance improvements you're targeting. Please drop or gate the log before merging.</violation>
</file>
<file name="core/autocomplete/util/HelperVars.ts">
<violation number="1" location="core/autocomplete/util/HelperVars.ts:150">
fullSuffixLines should return the cached array; pointing to fullSuffixLines causes infinite recursion at runtime.</violation>
</file>
<file name="extensions/vscode/src/autocomplete/RecentlyVisitedRangesService.ts">
<violation number="1" location="extensions/vscode/src/autocomplete/RecentlyVisitedRangesService.ts:64">
Map#get returns undefined for unknown keys, so this guard never runs and the file security check is skipped, letting flagged files through.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
|
|
||
| this.ide.onDidChangeActiveTextEditor((filepath) => { | ||
| void this.core.invoke("files/opened", { uris: [filepath] }); | ||
| vscode.window.onDidChangeActiveTextEditor((editor) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Register the onDidChangeActiveTextEditor listener with the extension context so it is disposed during deactivation to avoid leaking the handler.
Prompt for AI agents
Address the following comment on extensions/vscode/src/extension/VsCodeExtension.ts at line 638:
<comment>Register the `onDidChangeActiveTextEditor` listener with the extension context so it is disposed during deactivation to avoid leaking the handler.</comment>
<file context>
@@ -634,8 +635,12 @@ export class VsCodeExtension {
- this.ide.onDidChangeActiveTextEditor((filepath) => {
- void this.core.invoke("files/opened", { uris: [filepath] });
+ vscode.window.onDidChangeActiveTextEditor((editor) => {
+ if (editor) {
+ this.core.invoke("files/opened", {
</file context>
| // Create a promise that resolves to a snippet or null | ||
| const readPromise = new Promise<AutocompleteCodeSnippet | null>( | ||
| (resolve) => { | ||
| console.log( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This debug console.log runs on every file read from the recently opened cache, so autocomplete will emit a log per file on each invocation; this adds synchronous logging overhead and floods the console, undermining the performance improvements you're targeting. Please drop or gate the log before merging.
Prompt for AI agents
Address the following comment on core/autocomplete/snippets/getAllSnippets.ts at line 131:
<comment>This debug console.log runs on every file read from the recently opened cache, so autocomplete will emit a log per file on each invocation; this adds synchronous logging overhead and floods the console, undermining the performance improvements you're targeting. Please drop or gate the log before merging.</comment>
<file context>
@@ -128,6 +128,9 @@ const getSnippetsFromRecentlyOpenedFiles = async (
// Create a promise that resolves to a snippet or null
const readPromise = new Promise<AutocompleteCodeSnippet | null>(
(resolve) => {
+ console.log(
+ `read file - getAllSnippets getSnippetsFromRecentlyOpenedFiles - ${fileUri}`,
+ );
</file context>
| let securityCheck = this.securityChecks.get( | ||
| event.textEditor.document.fileName, | ||
| ); | ||
| if (securityCheck === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Map#get returns undefined for unknown keys, so this guard never runs and the file security check is skipped, letting flagged files through.
Prompt for AI agents
Address the following comment on extensions/vscode/src/autocomplete/RecentlyVisitedRangesService.ts at line 64:
<comment>Map#get returns undefined for unknown keys, so this guard never runs and the file security check is skipped, letting flagged files through.</comment>
<file context>
@@ -39,95 +42,120 @@ export class RecentlyVisitedRangesService {
+ let securityCheck = this.securityChecks.get(
+ event.textEditor.document.fileName,
+ );
+ if (securityCheck === null) {
+ securityCheck = isSecurityConcern(event.textEditor.document.fileName);
+ this.securityChecks.set(
</file context>
| if (securityCheck === null) { | |
| if (securityCheck === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 issues found across 35 files
Prompt for AI agents (all 4 issues)
Understand the root cause of the following 4 issues and fix them.
<file name="extensions/vscode/src/extension/VsCodeExtension.ts">
<violation number="1" location="extensions/vscode/src/extension/VsCodeExtension.ts:637">
Ensure the Disposable returned by onDidChangeActiveTextEditor is registered in context.subscriptions so the listener is cleaned up on deactivate.</violation>
</file>
<file name="core/tools/implementations/readFileRange.integration.vitest.ts">
<violation number="1" location="core/tools/implementations/readFileRange.integration.vitest.ts:200">
`readFileRangeImpl` still obtains file ranges via `readRangeInFile`, which calls `ide.readFile(uri)` without range options. This assertion will always fail because no second argument is ever passed to `readFile`, breaking the test.</violation>
</file>
<file name="extensions/vscode/src/autocomplete/RecentlyVisitedRangesService.ts">
<violation number="1" location="extensions/vscode/src/autocomplete/RecentlyVisitedRangesService.ts:46">
The feature-flag branch now always runs, so when the flag returns undefined numSurroundingLines becomes undefined, producing NaN slice bounds and empty snippets for most users.</violation>
<violation number="2" location="extensions/vscode/src/autocomplete/RecentlyVisitedRangesService.ts:65">
securityChecks never caches results because Map.get returns undefined, so the isSecurityConcern guard is skipped and sensitive files are now stored.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
|
|
||
| this.ide.onDidChangeActiveTextEditor((filepath) => { | ||
| void this.core.invoke("files/opened", { uris: [filepath] }); | ||
| vscode.window.onDidChangeActiveTextEditor((editor) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure the Disposable returned by onDidChangeActiveTextEditor is registered in context.subscriptions so the listener is cleaned up on deactivate.
Prompt for AI agents
Address the following comment on extensions/vscode/src/extension/VsCodeExtension.ts at line 637:
<comment>Ensure the Disposable returned by onDidChangeActiveTextEditor is registered in context.subscriptions so the listener is cleaned up on deactivate.</comment>
<file context>
@@ -634,8 +634,12 @@ export class VsCodeExtension {
- this.ide.onDidChangeActiveTextEditor((filepath) => {
- void this.core.invoke("files/opened", { uris: [filepath] });
+ vscode.window.onDidChangeActiveTextEditor((editor) => {
+ if (editor) {
+ this.core.invoke("files/opened", {
</file context>
|
|
||
| // Verify correct 0-based conversion | ||
| expect(mockIde.readRangeInFile).toHaveBeenCalledWith("file:///test.txt", { | ||
| expect(mockIde.readFile).toHaveBeenCalledWith("file:///test.txt", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readFileRangeImpl still obtains file ranges via readRangeInFile, which calls ide.readFile(uri) without range options. This assertion will always fail because no second argument is ever passed to readFile, breaking the test.
Prompt for AI agents
Address the following comment on core/tools/implementations/readFileRange.integration.vitest.ts at line 200:
<comment>`readFileRangeImpl` still obtains file ranges via `readRangeInFile`, which calls `ide.readFile(uri)` without range options. This assertion will always fail because no second argument is ever passed to `readFile`, breaking the test.</comment>
<file context>
@@ -197,7 +197,7 @@ test("readFileRangeImpl handles normal ranges correctly", async () => {
// Verify correct 0-based conversion
- expect(mockIde.readRangeInFile).toHaveBeenCalledWith("file:///test.txt", {
+ expect(mockIde.readFile).toHaveBeenCalledWith("file:///test.txt", {
start: { line: 1, character: 0 }, // 2 - 1
end: { line: 3, character: Number.MAX_SAFE_INTEGER }, // 4 - 1
</file context>
| let securityCheck = this.securityChecks.get( | ||
| event.textEditor.document.fileName, | ||
| ); | ||
| if (securityCheck === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
securityChecks never caches results because Map.get returns undefined, so the isSecurityConcern guard is skipped and sensitive files are now stored.
Prompt for AI agents
Address the following comment on extensions/vscode/src/autocomplete/RecentlyVisitedRangesService.ts at line 65:
<comment>securityChecks never caches results because Map.get returns undefined, so the isSecurityConcern guard is skipped and sensitive files are now stored.</comment>
<file context>
@@ -39,95 +43,120 @@ export class RecentlyVisitedRangesService {
+ let securityCheck = this.securityChecks.get(
+ event.textEditor.document.fileName,
+ );
+ if (securityCheck === null) {
+ securityCheck = isSecurityConcern(event.textEditor.document.fileName);
+ this.securityChecks.set(
</file context>
| if (securityCheck === null) { | |
| if (securityCheck === undefined) { |
| ); | ||
|
|
||
| if (recentlyVisitedRangesNumSurroundingLines) { | ||
| if (true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The feature-flag branch now always runs, so when the flag returns undefined numSurroundingLines becomes undefined, producing NaN slice bounds and empty snippets for most users.
Prompt for AI agents
Address the following comment on extensions/vscode/src/autocomplete/RecentlyVisitedRangesService.ts at line 46:
<comment>The feature-flag branch now always runs, so when the flag returns undefined numSurroundingLines becomes undefined, producing NaN slice bounds and empty snippets for most users.</comment>
<file context>
@@ -39,95 +43,120 @@ export class RecentlyVisitedRangesService {
);
- if (recentlyVisitedRangesNumSurroundingLines) {
+ if (true) {
this.isEnabled = true;
this.numSurroundingLines = recentlyVisitedRangesNumSurroundingLines;
</file context>
| if (true) { | |
| if (recentlyVisitedRangesNumSurroundingLines != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 issues found across 35 files
Prompt for AI agents (all 5 issues)
Understand the root cause of the following 5 issues and fix them.
<file name="core/tools/implementations/readFileRange.ts">
<violation number="1" location="core/tools/implementations/readFileRange.ts:49">
Switching to the shared readRangeInFile helper causes readFileRange to prepend a newline for single-line ranges; for startLine === endLine the helper returns "\n<line>" instead of just the requested line, which is a functional regression for this tool.</violation>
</file>
<file name="core/autocomplete/context/ImportDefinitionsService.ts">
<violation number="1" location="core/autocomplete/context/ImportDefinitionsService.ts:26">
Commenting out the onDidChangeActiveTextEditor handler stops warming this cache when the user switches files, so import definition lookups now return undefined instead of the cached definitions.</violation>
</file>
<file name="core/autocomplete/snippets/getAllSnippets.ts">
<violation number="1" location="core/autocomplete/snippets/getAllSnippets.ts:131">
This new console.log will fire for every recently opened file read, spamming the console and adding overhead in production usages. Please remove it or gate it behind the existing logging/instrumentation mechanism.</violation>
</file>
<file name="extensions/vscode/src/autocomplete/RecentlyVisitedRangesService.ts">
<violation number="1" location="extensions/vscode/src/autocomplete/RecentlyVisitedRangesService.ts:46">
Restore the feature-flag guard before enabling recently visited ranges so numSurroundingLines isn't set to undefined when the flag is missing.</violation>
<violation number="2" location="extensions/vscode/src/autocomplete/RecentlyVisitedRangesService.ts:65">
Ensure the security check cache initializes when the lookup returns undefined so isSecurityConcern runs at least once per file.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| // Use the IDE's readRangeInFile method with 0-based range (IDE expects 0-based internally) | ||
| const content = await extras.ide.readRangeInFile(resolvedPath.uri, { | ||
| // readRangeInFile expects 0-based range indexes | ||
| const content = await readRangeInFile(extras.ide, resolvedPath.uri, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching to the shared readRangeInFile helper causes readFileRange to prepend a newline for single-line ranges; for startLine === endLine the helper returns "\n" instead of just the requested line, which is a functional regression for this tool.
Prompt for AI agents
Address the following comment on core/tools/implementations/readFileRange.ts at line 49:
<comment>Switching to the shared readRangeInFile helper causes readFileRange to prepend a newline for single-line ranges; for startLine === endLine the helper returns "\n<line>" instead of just the requested line, which is a functional regression for this tool.</comment>
<file context>
@@ -44,8 +45,8 @@ export const readFileRangeImpl: ToolImpl = async (args, extras) => {
- // Use the IDE's readRangeInFile method with 0-based range (IDE expects 0-based internally)
- const content = await extras.ide.readRangeInFile(resolvedPath.uri, {
+ // readRangeInFile expects 0-based range indexes
+ const content = await readRangeInFile(extras.ide, resolvedPath.uri, {
start: {
line: startLine - 1, // Convert from 1-based to 0-based
</file context>
| ); | ||
| }); | ||
| console.log("new import definitions service"); | ||
| // ide.onDidChangeActiveTextEditor((filepath) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting out the onDidChangeActiveTextEditor handler stops warming this cache when the user switches files, so import definition lookups now return undefined instead of the cached definitions.
Prompt for AI agents
Address the following comment on core/autocomplete/context/ImportDefinitionsService.ts at line 26:
<comment>Commenting out the onDidChangeActiveTextEditor handler stops warming this cache when the user switches files, so import definition lookups now return undefined instead of the cached definitions.</comment>
<file context>
@@ -21,15 +22,16 @@ export class ImportDefinitionsService {
- );
- });
+ console.log("new import definitions service");
+ // ide.onDidChangeActiveTextEditor((filepath) => {
+ // this.cache
+ // .initKey(filepath)
</file context>
| // Create a promise that resolves to a snippet or null | ||
| const readPromise = new Promise<AutocompleteCodeSnippet | null>( | ||
| (resolve) => { | ||
| console.log( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new console.log will fire for every recently opened file read, spamming the console and adding overhead in production usages. Please remove it or gate it behind the existing logging/instrumentation mechanism.
Prompt for AI agents
Address the following comment on core/autocomplete/snippets/getAllSnippets.ts at line 131:
<comment>This new console.log will fire for every recently opened file read, spamming the console and adding overhead in production usages. Please remove it or gate it behind the existing logging/instrumentation mechanism.</comment>
<file context>
@@ -128,6 +128,9 @@ const getSnippetsFromRecentlyOpenedFiles = async (
// Create a promise that resolves to a snippet or null
const readPromise = new Promise<AutocompleteCodeSnippet | null>(
(resolve) => {
+ console.log(
+ `read file - getAllSnippets getSnippetsFromRecentlyOpenedFiles - ${fileUri}`,
+ );
</file context>
| let securityCheck = this.securityChecks.get( | ||
| event.textEditor.document.fileName, | ||
| ); | ||
| if (securityCheck === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure the security check cache initializes when the lookup returns undefined so isSecurityConcern runs at least once per file.
Prompt for AI agents
Address the following comment on extensions/vscode/src/autocomplete/RecentlyVisitedRangesService.ts at line 65:
<comment>Ensure the security check cache initializes when the lookup returns undefined so isSecurityConcern runs at least once per file.</comment>
<file context>
@@ -39,95 +43,120 @@ export class RecentlyVisitedRangesService {
+ let securityCheck = this.securityChecks.get(
+ event.textEditor.document.fileName,
+ );
+ if (securityCheck === null) {
+ securityCheck = isSecurityConcern(event.textEditor.document.fileName);
+ this.securityChecks.set(
</file context>
| if (securityCheck === null) { | |
| if (securityCheck === undefined) { |
| ); | ||
|
|
||
| if (recentlyVisitedRangesNumSurroundingLines) { | ||
| if (true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restore the feature-flag guard before enabling recently visited ranges so numSurroundingLines isn't set to undefined when the flag is missing.
Prompt for AI agents
Address the following comment on extensions/vscode/src/autocomplete/RecentlyVisitedRangesService.ts at line 46:
<comment>Restore the feature-flag guard before enabling recently visited ranges so numSurroundingLines isn't set to undefined when the flag is missing.</comment>
<file context>
@@ -39,95 +43,120 @@ export class RecentlyVisitedRangesService {
);
- if (recentlyVisitedRangesNumSurroundingLines) {
+ if (true) {
this.isEnabled = true;
this.numSurroundingLines = recentlyVisitedRangesNumSurroundingLines;
</file context>
Description
TODO Make sure to revert changes from #8391
[ What changed? Feel free to be brief. ]
AI Code Review
@continue-reviewChecklist
Screen recording or screenshot
[ When applicable, please include a short screen recording or screenshot - this makes it much easier for us as contributors to review and understand your changes. See this PR as a good example. ]
Tests
[ What tests were added or updated to ensure the changes work as expected? ]
Summary by cubic
Cut down expensive file reads in autocomplete by building prefix/suffix from already-loaded lines, deferring snippet reads, and pruning by tokens on line arrays. Removes IDE.onDidChangeActiveTextEditor to simplify event handling and avoid unnecessary work.
Refactors
Migration