-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: add per-workspace control for codebase indexing #8138
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ import { changeLanguage, t } from "../../i18n" | |
| import { Package } from "../../shared/package" | ||
| import { type RouterName, type ModelRecord, toRouterName } from "../../shared/api" | ||
| import { MessageEnhancer } from "./messageEnhancer" | ||
| import { CodeIndexManager } from "../../services/code-index/manager" | ||
|
|
||
| import { | ||
| type WebviewMessage, | ||
|
|
@@ -2569,6 +2570,29 @@ export const webviewMessageHandler = async ( | |
| }) | ||
| break | ||
| } | ||
| case "setWorkspaceCodebaseIndexEnabled": { | ||
| const { workspacePath, enabled } = message | ||
| try { | ||
| const manager = CodeIndexManager.getInstance(provider.context, workspacePath) | ||
| if (manager && enabled !== undefined) { | ||
| await manager.setWorkspaceEnabled(enabled) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it intentional that we're not awaiting |
||
|
|
||
| // Send updated status back to webview | ||
| const status = manager.getCurrentStatus() | ||
| provider.postMessageToWebview({ | ||
| type: "indexingStatusUpdate", | ||
| values: { | ||
| ...status, | ||
| workspaceEnabled: manager.getWorkspaceEnabled(), | ||
| globalEnabled: manager.getGlobalEnabled(), | ||
| }, | ||
| }) | ||
| } | ||
| } catch (error) { | ||
| console.error("Failed to set workspace codebase index enabled state:", error) | ||
| } | ||
| break | ||
| } | ||
| case "startIndexing": { | ||
| try { | ||
| const manager = provider.getCurrentWorkspaceCodeIndexManager() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,15 @@ export class CodeIndexConfigManager { | |
| private searchMinScore?: number | ||
| private searchMaxResults?: number | ||
|
|
||
| constructor(private readonly contextProxy: ContextProxy) { | ||
| // Per-workspace settings | ||
| private workspaceSettings: Record<string, { enabled: boolean }> = {} | ||
| private currentWorkspacePath: string | undefined | ||
|
|
||
| constructor( | ||
| private readonly contextProxy: ContextProxy, | ||
| workspacePath?: string, | ||
| ) { | ||
| this.currentWorkspacePath = workspacePath | ||
| // Initialize with current configuration to avoid false restart triggers | ||
| this._loadAndSetConfiguration() | ||
| } | ||
|
|
@@ -61,6 +69,7 @@ export class CodeIndexConfigManager { | |
| codebaseIndexEmbedderModelId, | ||
| codebaseIndexSearchMinScore, | ||
| codebaseIndexSearchMaxResults, | ||
| codebaseIndexWorkspaceSettings, | ||
| } = codebaseIndexConfig | ||
|
|
||
| const openAiKey = this.contextProxy?.getSecret("codeIndexOpenAiKey") ?? "" | ||
|
|
@@ -72,8 +81,20 @@ export class CodeIndexConfigManager { | |
| const mistralApiKey = this.contextProxy?.getSecret("codebaseIndexMistralApiKey") ?? "" | ||
| const vercelAiGatewayApiKey = this.contextProxy?.getSecret("codebaseIndexVercelAiGatewayApiKey") ?? "" | ||
|
|
||
| // Load workspace settings | ||
| this.workspaceSettings = codebaseIndexWorkspaceSettings ?? {} | ||
|
|
||
| // Determine effective enabled state based on workspace override | ||
| const globalEnabled = codebaseIndexEnabled ?? true | ||
| let effectiveEnabled = globalEnabled | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be helpful to add JSDoc comments explaining the precedence rules (workspace settings override global settings). This is a key behavior that should be documented. |
||
|
|
||
| if (this.currentWorkspacePath && this.workspaceSettings[this.currentWorkspacePath]) { | ||
| // Workspace setting takes precedence over global setting | ||
| effectiveEnabled = this.workspaceSettings[this.currentWorkspacePath].enabled | ||
| } | ||
|
|
||
| // Update instance variables with configuration | ||
| this.codebaseIndexEnabled = codebaseIndexEnabled ?? true | ||
| this.codebaseIndexEnabled = effectiveEnabled | ||
| this.qdrantUrl = codebaseIndexQdrantUrl | ||
| this.qdrantApiKey = qdrantApiKey ?? "" | ||
| this.searchMinScore = codebaseIndexSearchMinScore | ||
|
|
@@ -409,6 +430,47 @@ export class CodeIndexConfigManager { | |
| return this.codebaseIndexEnabled | ||
| } | ||
|
|
||
| /** | ||
| * Updates the enabled state for a specific workspace | ||
| */ | ||
| public async setWorkspaceEnabled(workspacePath: string, enabled: boolean): Promise<void> { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage for the new workspace-specific settings functionality. Could we add tests for |
||
| if (!this.contextProxy) { | ||
| return | ||
| } | ||
|
|
||
| // Get current config | ||
| const currentConfig = this.contextProxy.getGlobalState("codebaseIndexConfig") ?? {} | ||
|
|
||
| // Update workspace settings | ||
| const workspaceSettings = currentConfig.codebaseIndexWorkspaceSettings ?? {} | ||
| workspaceSettings[workspacePath] = { enabled } | ||
|
|
||
| // Save updated config | ||
| await this.contextProxy.updateGlobalState("codebaseIndexConfig", { | ||
| ...currentConfig, | ||
| codebaseIndexWorkspaceSettings: workspaceSettings, | ||
| }) | ||
|
|
||
| // Reload configuration to apply changes | ||
| await this.loadConfiguration() | ||
| } | ||
|
|
||
| /** | ||
| * Gets the enabled state for a specific workspace | ||
| */ | ||
| public getWorkspaceEnabled(workspacePath: string): boolean | undefined { | ||
| const config = this.contextProxy?.getGlobalState("codebaseIndexConfig") ?? {} | ||
| return config.codebaseIndexWorkspaceSettings?.[workspacePath]?.enabled | ||
| } | ||
|
|
||
| /** | ||
| * Gets the global enabled state (without workspace override) | ||
| */ | ||
| public getGlobalEnabled(): boolean { | ||
| const config = this.contextProxy?.getGlobalState("codebaseIndexConfig") ?? {} | ||
| return config.codebaseIndexEnabled ?? true | ||
| } | ||
|
|
||
| /** | ||
| * Gets whether the code indexing feature is properly configured | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -118,7 +118,7 @@ export class CodeIndexManager { | |
| public async initialize(contextProxy: ContextProxy): Promise<{ requiresRestart: boolean }> { | ||
| // 1. ConfigManager Initialization and Configuration Loading | ||
| if (!this._configManager) { | ||
| this._configManager = new CodeIndexConfigManager(contextProxy) | ||
| this._configManager = new CodeIndexConfigManager(contextProxy, this.workspacePath) | ||
| } | ||
| // Load configuration once to get current state and restart requirements | ||
| const { requiresRestart } = await this._configManager.loadConfiguration() | ||
|
|
@@ -419,4 +419,40 @@ export class CodeIndexManager { | |
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Sets the enabled state for the current workspace | ||
| */ | ||
| public async setWorkspaceEnabled(enabled: boolean): Promise<void> { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new workspace methods also need test coverage. Is this intentional or should we add tests before merging? |
||
| if (!this._configManager || !this.workspacePath) { | ||
| return | ||
| } | ||
|
|
||
| await this._configManager.setWorkspaceEnabled(this.workspacePath, enabled) | ||
|
|
||
| // Reload configuration to apply changes | ||
| await this.handleSettingsChange() | ||
| } | ||
|
|
||
| /** | ||
| * Gets the enabled state for the current workspace | ||
| */ | ||
| public getWorkspaceEnabled(): boolean | undefined { | ||
| if (!this._configManager || !this.workspacePath) { | ||
| return undefined | ||
| } | ||
|
|
||
| return this._configManager.getWorkspaceEnabled(this.workspacePath) | ||
| } | ||
|
|
||
| /** | ||
| * Gets the global enabled state (without workspace override) | ||
| */ | ||
| public getGlobalEnabled(): boolean { | ||
| if (!this._configManager) { | ||
| return true | ||
| } | ||
|
|
||
| return this._configManager.getGlobalEnabled() | ||
| } | ||
| } | ||
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.
Could we add path validation here? The generic
z.string()for workspace paths might accept invalid file paths. Consider using a custom validator or at least documenting the expected format.