-
Notifications
You must be signed in to change notification settings - Fork 2.9k
refactor: ♻️ Refactor CodeBaseIndexer out of core #5894
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?
refactor: ♻️ Refactor CodeBaseIndexer out of core #5894
Conversation
Your cubic subscription is currently inactive. Please reactivate your subscription to receive AI reviews and use cubic. |
✅ Deploy Preview for continuedev canceled.
|
@Patrick-Erichsen related to #5564 I've performed a refactor of CodeBaseIndxer.ts and core.ts to refactor the indexing functions out of core. I'm going to note a few questions in a self review for you. |
@@ -244,6 +244,7 @@ export class VsCodeExtension { | |||
this.consoleView, | |||
this.configHandler, | |||
this.verticalDiffManager, | |||
// This is expected to exist, but it's no longer used? Can I remove it? | |||
this.core.continueServerClientPromise, |
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.
@Patrick-Erichsen this is no longer used anywhere in core. I can't see anywhere else it is used as well. Note in CodeBaseIndexer I just created a self contained new instance of this class. I'm not sure if this was meant to be a singleton or it's used in some other way? If it's no longer used we can refactor it out of here, and remove the initalization from core.ts.
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.
Nice catch @chezsmithy - I think this is safe to remove and would be appreciated if you could do so.
cc @sestinj for a sanity check however.
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.
I just poked around, seems like we might use it in extensions/vscode/src/commands.ts
?
"continue.giveAutocompleteFeedback": async () => {
const feedback = await vscode.window.showInputBox({
ignoreFocusOut: true,
prompt:
"Please share what went wrong with the last completion. The details of the completion as well as this message will be sent to the Continue team in order to improve.",
});
if (feedback) {
const client = await continueServerClientPromise;
const completionsPath = getDevDataFilePath(
"autocomplete",
LOCAL_DEV_DATA_VERSION,
);
const lastLines = await readLastLines.read(completionsPath, 2);
client.sendFeedback(feedback, lastLines);
}
},
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.
Ah we should just remove this
this.codebaseIndexerPromise = new Promise( | ||
async (resolve) => (codebaseIndexerResolve = resolve), | ||
); | ||
|
||
let continueServerClientResolve: (_: any) => void | 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.
@Patrick-Erichsen note this is no longer used anywhere in core. I've moved initalization into CodeBaseIndexer rather than relying on this instance. Should we just remove it?
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.
Yep, let's remove this one too, thanks for calling it out 👍
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.
@sestinj cc on this question along with the other.
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 is a great cleanup, will make the callTools
logic we originally discussed less dependent on core
and this is just a solid cleanup in general. Nice we we can remove the awkward codebaseIndexerResolve
things.
Going to wait to merge until I double check with @sestinj on that one comment but otherwise good to go 🚀
f5d7f91
to
f6d114d
Compare
All good to go from me except I think we should just completely remove |
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.
all looks great. as we're removing the continueServerClient, let's take out the "continue.giveAutocompleteFeedback"
command in VS Code
Description
Refector CodeBaseIndexer out of core.ts in a way that it can be independently passed to tools which might need to perform reindexing. Not in #5564 we would now pass core (this.codeBaseIndexer) to the tools implementations rather than the the entire core object.
Checklist
Screenshots
[ For visual changes, include screenshots. Screen recordings are particularly helpful, and appreciated! ]
Tests
I've manually tested the following in debug:
I've added new tests for CodeBaseIndexer.ts to cover the changes.