Add AI tools integration - #2025
Conversation
|
🤖 Integration tests ❌ 2 of 35 test jobs failed for |
|
🤖 Integration tests ❌ 2 of 35 test jobs failed for |
|
🤖 Integration tests ✅ all 35 test jobs passed for |
|
🤖 Integration tests ✅ all 35 test jobs passed for |
| // In both cases we expect the workspace to be reloaded and the extension will | ||
| // be activated again. | ||
| // be activated again. The AI tools setup affordance is added as a | ||
| // viewsWelcome entry (see package.json) since it works without a folder. |
There was a problem hiding this comment.
stale comment? no viewsWelcome in package.json
There was a problem hiding this comment.
Yes, whoops! Will remove
| // Detect install state on activation and, if installed, auto-apply any | ||
| // available update; otherwise prompt the user (once) to install the tools. | ||
| // Non-blocking so it doesn't delay activation. | ||
| aiToolsCommands.initializeCommand()(); |
There was a problem hiding this comment.
Can initializeCommand() throw? (try/catch)
There was a problem hiding this comment.
Also it's running in remote mode, even though commands are gated to not run
There was a problem hiding this comment.
Yeah it seems it can if window.withProgress throws, but it probably shouldn't. I'll update this to show an error toast instead 👍
| loggerManager | ||
| ), | ||
| telemetry.registerCommand( | ||
| "databricks.logs.show", |
There was a problem hiding this comment.
looks like duplicate of existing "databricks.internal.showOutput" command
There was a problem hiding this comment.
and has no enablement (will appear in remote mode)
There was a problem hiding this comment.
I'll remove this then 👍
| // before the CLI call — passing an empty `--agents` list would make | ||
| // the CLI act on every detected agent, which is not what was chosen. | ||
| if (agents.length > 0 && cliAgents.length === 0) { | ||
| this.telemetry.recordEvent(Events.AITOOLS_INSTALL, { |
There was a problem hiding this comment.
Here AITOOLS_INSTALL is reported as success (Cursor specific case), but where do we install it?
addCursorPlugin() call before seems to only open a modal
There was a problem hiding this comment.
Installing the cursor plugin via the modal opened by addCursorPlugin is not observable by this extension. In this case (agents.length > 0 && cliAgents.length === 0) only cursor is selected so we don't need to run the CLI, therefore we pass success: true since there is no other success signal we can use
|
|
||
| it("hangs on a stdin-reading process without closeStdin", async () => { | ||
| const raced = await Promise.race([ | ||
| cancellableExecFile("cat", []).then(() => "completed"), |
There was a problem hiding this comment.
minor: "cat" process seems to stay alive/leaking
| setIsCursor(value: boolean) { | ||
| commands.executeCommand( | ||
| "setContext", | ||
| "databricks.context.isCursor", |
There was a problem hiding this comment.
No function reads "databricks.context.isCursor" key. is it needed?
There was a problem hiding this comment.
and no "when" clause is checking it in package.json
There was a problem hiding this comment.
This must have been left behind at some point. I'll remove 👍
| * for available updates, and caching the resolved install location. | ||
| */ | ||
| export class AiToolsManager implements Disposable { | ||
| private disposables: Disposable[] = []; |
There was a problem hiding this comment.
nit: nothing is pushed in disposables
There was a problem hiding this comment.
yeah I know, I was just following the boilerplate. I can remove it 👍
| description: version, | ||
| collapsibleState: TreeItemCollapsibleState.None, | ||
| }, | ||
| agents !== undefined && { |
There was a problem hiding this comment.
agents !== undefined is always true as it defaults to [] in model
There was a problem hiding this comment.
good catch. This should be agents.length > 0
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
| const items: ConfigurationTreeItem[] = [ | ||
| { | ||
| label: "AI tools", | ||
| id: TREE_ICON_ID, | ||
| description: description ?? "", | ||
| tooltip: `AI tools installed (${installLocation})`, | ||
| contextValue: getContextValue(state), | ||
| iconPath: icon, | ||
| collapsibleState: TreeItemCollapsibleState.Collapsed, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
In case installLocation !== undefined (maybe cached) but detectError is true. it will show collapsed node and when user clicks it will expand to empty node (as getChildren returns [] in case of detectError)
is this intentional?
There was a problem hiding this comment.
Yeah probably it's better to just show the error state in all cases and ignore any cached state
| "url": "https://github.com/databricks/databricks-vscode.git" | ||
| }, | ||
| "activationEvents": [ | ||
| "onStartupFinished", |
There was a problem hiding this comment.
Why do we need this? it makes extension activate for all cases, even when user opens some unrelated (to databricks) project. We should try to avoid it as it makes VSCode UX slow
There was a problem hiding this comment.
Also it will force activating all dependent extensions (python, yaml, debugger, etc..)
3 participants
Changes
Integrate the
databricks aitoolsfamily of commands into the extension.AI toolsrow to the configuration paneTests
Added tests pass