-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: Add per-workspace codebase indexing control #7512
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-specific storage support to ContextProxy - Update CodeIndexConfigManager to check workspace settings before global - Add message handlers for workspace-specific indexing settings - Add UI support with workspace override toggle in CodeIndexPopover - Add translation keys for new UI elements - Update tests to support new workspace-specific methods Fixes #7511
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.
Reviewing my own code because apparently I trust no one, not even myself.
| const contextProxy = await ContextProxy.getInstance(context) | ||
| // Set the workspace path for the ContextProxy to enable workspace-specific settings | ||
| const workspacePath = getWorkspacePath() | ||
| if (workspacePath) { |
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 this intentional? The workspace path is set after getting the ContextProxy instance. Could this cause issues if multiple workspaces try to initialize simultaneously? Consider setting the workspace path as part of the getInstance call or ensuring thread safety.
| * Get workspace-specific state value | ||
| * Falls back to global state if workspace value doesn't exist | ||
| */ | ||
| public getWorkspaceState<T>(key: string, defaultValue?: T): T | 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.
The workspace state operations don't have error handling. What happens if VSCode storage operations fail? Consider wrapping these in try-catch blocks to handle potential failures gracefully.
| if (!this._configManager) { | ||
| // Ensure the ContextProxy has the workspace path set for workspace-specific settings | ||
| if (this.workspacePath && contextProxy.workspacePath !== this.workspacePath) { | ||
| contextProxy.setWorkspacePath(this.workspacePath) |
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 workspace path mismatch check only updates the path but doesn't handle potential conflicts. Should this be more defensive? What if two different CodeIndexManager instances are trying to use different workspace paths?
| await this.initialize() | ||
| } | ||
|
|
||
| /** |
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.
Consider adding more detailed JSDoc comments for these new public methods. It would help future developers understand the behavior, especially around edge cases like what happens when workspace path is undefined.
| </div> | ||
|
|
||
| {/* Workspace-specific override */} | ||
| {cwd && ( |
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 UI addition! Consider adding a visual indicator to show which setting (global vs workspace) is currently active. This would help users understand the override hierarchy better.
| getSecret: vi.fn().mockReturnValue(undefined), | ||
| refreshSecrets: vi.fn().mockResolvedValue(undefined), | ||
| updateGlobalState: vi.fn(), | ||
| getWorkspaceState: vi.fn().mockReturnValue(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.
Good that you updated the mocks! Consider adding specific test cases for the workspace override scenarios - like testing what happens when workspace setting exists vs when it doesn't, and verifying the fallback behavior.
|
The issue needs proper scoping |
This PR implements per-workspace codebase indexing control as requested in issue #7511.
Summary
Users working with multiple VS Code windows experience high GPU usage when indexing is enabled globally. This PR adds granular control to enable/disable indexing for specific projects/workspaces.
Changes
ContextProxywith methods to get/set workspace-specific stateCodeIndexConfigManagerto check workspace settings before falling back to global settingsWebviewMessageandExtensionMessageinterfacesCodeIndexPopoverReact component to include workspace override checkboxesHow it works
Testing
Fixes #7511
Important
Adds per-workspace codebase indexing control, allowing users to manage indexing settings for individual workspaces, with updates to
ContextProxy,webviewMessageHandler, andCodeIndexPopover.ContextProxyto support workspace-specific state withgetWorkspaceState()andupdateWorkspaceState()methods.webviewMessageHandlerto handle workspace-specific indexing settings.CodeIndexConfigManagerto prioritize workspace settings over global settings.CodeIndexPopoverto include UI elements for workspace-specific settings.config-manager.spec.tsandmanager.spec.tsto cover new workspace-specific functionality.WebviewMessageandExtensionMessageinterfaces.settings.json.This description was created by
for cfc5529. You can customize this summary. It will automatically update as commits are pushed.