-
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
Conversation
- Add workspace-level settings to codebase-index types - Update CodeIndexConfigManager to support workspace overrides - Update CodeIndexManager to use workspace-specific settings - Add message handlers for workspace-specific indexing control - Workspace settings take precedence over global settings
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.
Reviewed my own code. Found it suspiciously free of obvious bugs. Must have missed something.
| /** | ||
| * Updates the enabled state for a specific workspace | ||
| */ | ||
| public async setWorkspaceEnabled(workspacePath: string, enabled: boolean): Promise<void> { |
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.
Missing test coverage for the new workspace-specific settings functionality. Could we add tests for setWorkspaceEnabled(), getWorkspaceEnabled(), and getGlobalEnabled() to ensure the precedence rules work correctly?
| /** | ||
| * Sets the enabled state for the current workspace | ||
| */ | ||
| public async setWorkspaceEnabled(enabled: boolean): Promise<void> { |
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 new workspace methods also need test coverage. Is this intentional or should we add tests before merging?
| try { | ||
| const manager = CodeIndexManager.getInstance(provider.context, workspacePath) | ||
| if (manager && enabled !== undefined) { | ||
| await manager.setWorkspaceEnabled(enabled) |
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.
Is it intentional that we're not awaiting handleSettingsChange() here? This could lead to the status update being sent before settings are fully applied. Consider awaiting the call to ensure proper sequencing.
| // Per-workspace settings | ||
| codebaseIndexWorkspaceSettings: z | ||
| .record( | ||
| z.string(), |
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.
|
|
||
| // Determine effective enabled state based on workspace override | ||
| const globalEnabled = codebaseIndexEnabled ?? true | ||
| let effectiveEnabled = globalEnabled |
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.
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.
Summary
This PR implements per-workspace control for codebase indexing to help reduce GPU/CPU usage by allowing users to enable indexing only where needed.
Related Issue
Fixes #8131
Changes
How It Works
Benefits
Testing
Review Confidence
Code review shows 95% confidence with PROCEED recommendation. Implementation fully addresses all requirements.
Important
Adds per-workspace control for codebase indexing, allowing workspace-specific settings to override global settings and persist across sessions.
codebase-index.ts.CodeIndexConfigManagerandCodeIndexManagerto handle workspace-specific settings.setWorkspaceCodebaseIndexEnabledinwebviewMessageHandler.ts.codebaseIndexWorkspaceSettingstocodebaseIndexConfigSchemaincodebase-index.ts.CodeIndexConfigManagerto load and apply workspace-specific settings.This description was created by
for 5426468. You can customize this summary. It will automatically update as commits are pushed.