-
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
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 |
|---|---|---|
|
|
@@ -39,18 +39,25 @@ export class ContextProxy { | |
| private stateCache: GlobalState | ||
| private secretCache: SecretState | ||
| private _isInitialized = false | ||
| private _workspacePath: string | undefined | ||
|
|
||
| constructor(context: vscode.ExtensionContext) { | ||
| constructor(context: vscode.ExtensionContext, workspacePath?: string) { | ||
| this.originalContext = context | ||
| this.stateCache = {} | ||
| this.secretCache = {} | ||
| this._isInitialized = false | ||
| this._workspacePath = workspacePath | ||
| } | ||
|
|
||
| public get isInitialized() { | ||
| return this._isInitialized | ||
| } | ||
|
|
||
| // Public getter for workspacePath to allow checking current workspace | ||
| public get workspacePath(): string | undefined { | ||
| return this._workspacePath | ||
| } | ||
|
|
||
| public async initialize() { | ||
| for (const key of GLOBAL_STATE_KEYS) { | ||
| try { | ||
|
|
@@ -290,6 +297,50 @@ export class ContextProxy { | |
| await this.initialize() | ||
| } | ||
|
|
||
| /** | ||
| * 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 { | ||
|
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 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._workspacePath) { | ||
| // If no workspace, fall back to global state | ||
| return this.originalContext.globalState.get<T>(key) ?? defaultValue | ||
| } | ||
|
|
||
| // Create a workspace-specific key | ||
| const workspaceKey = `workspace:${this._workspacePath}:${key}` | ||
| const workspaceValue = this.originalContext.globalState.get<T>(workspaceKey) | ||
|
|
||
| if (workspaceValue !== undefined) { | ||
| return workspaceValue | ||
| } | ||
|
|
||
| // Fall back to global state | ||
| return this.originalContext.globalState.get<T>(key) ?? defaultValue | ||
| } | ||
|
|
||
| /** | ||
| * Update workspace-specific state value | ||
| */ | ||
| public async updateWorkspaceState<T>(key: string, value: T): Promise<void> { | ||
| if (!this._workspacePath) { | ||
| // If no workspace, update global state | ||
| await this.originalContext.globalState.update(key, value) | ||
| return | ||
| } | ||
|
|
||
| // Create a workspace-specific key | ||
| const workspaceKey = `workspace:${this._workspacePath}:${key}` | ||
| await this.originalContext.globalState.update(workspaceKey, value) | ||
| } | ||
|
|
||
| /** | ||
| * Set the workspace path for workspace-specific settings | ||
| */ | ||
| public setWorkspacePath(workspacePath: string | undefined): void { | ||
| this._workspacePath = workspacePath | ||
| } | ||
|
|
||
| private static _instance: ContextProxy | null = null | ||
|
|
||
| static get instance() { | ||
|
|
@@ -300,12 +351,16 @@ export class ContextProxy { | |
| return this._instance | ||
| } | ||
|
|
||
| static async getInstance(context: vscode.ExtensionContext) { | ||
| static async getInstance(context: vscode.ExtensionContext, workspacePath?: string) { | ||
| if (this._instance) { | ||
| // Update workspace path if provided | ||
| if (workspacePath !== undefined) { | ||
| this._instance.setWorkspacePath(workspacePath) | ||
| } | ||
| return this._instance | ||
| } | ||
|
|
||
| this._instance = new ContextProxy(context) | ||
| this._instance = new ContextProxy(context, workspacePath) | ||
| await this._instance.initialize() | ||
|
|
||
| return this._instance | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ import { CloudService, ExtensionBridgeService } from "@roo-code/cloud" | |
| import { TelemetryService, PostHogTelemetryClient } from "@roo-code/telemetry" | ||
|
|
||
| import "./utils/path" // Necessary to have access to String.prototype.toPosix. | ||
| import { getWorkspacePath } from "./utils/path" | ||
| import { createOutputChannelLogger, createDualLogger } from "./utils/outputChannelLogger" | ||
|
|
||
| import { Package } from "./shared/package" | ||
|
|
@@ -97,8 +98,12 @@ export async function activate(context: vscode.ExtensionContext) { | |
| if (!context.globalState.get("allowedCommands")) { | ||
| context.globalState.update("allowedCommands", defaultCommands) | ||
| } | ||
|
|
||
| const contextProxy = await ContextProxy.getInstance(context) | ||
| // Set the workspace path for the ContextProxy to enable workspace-specific settings | ||
| const workspacePath = getWorkspacePath() | ||
| if (workspacePath) { | ||
|
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 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. |
||
| contextProxy.setWorkspacePath(workspacePath) | ||
| } | ||
|
|
||
| // Initialize code index managers for all workspace folders. | ||
| const codeIndexManagers: CodeIndexManager[] = [] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,10 @@ describe("CodeIndexConfigManager", () => { | |
| getSecret: vi.fn().mockReturnValue(undefined), | ||
| refreshSecrets: vi.fn().mockResolvedValue(undefined), | ||
| updateGlobalState: vi.fn(), | ||
| getWorkspaceState: vi.fn().mockReturnValue(undefined), | ||
|
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. 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. |
||
| updateWorkspaceState: vi.fn(), | ||
| setWorkspacePath: vi.fn(), | ||
| workspacePath: undefined, | ||
| } | ||
|
|
||
| configManager = new CodeIndexConfigManager(mockContextProxy) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,11 @@ export class CodeIndexManager { | |
| // Flag to prevent race conditions during error recovery | ||
| private _isRecoveringFromError = false | ||
|
|
||
| // Public getter for configManager to allow workspace-specific settings | ||
| public get configManager(): CodeIndexConfigManager | undefined { | ||
| return this._configManager | ||
| } | ||
|
|
||
| public static getInstance(context: vscode.ExtensionContext, workspacePath?: string): CodeIndexManager | undefined { | ||
| // If workspacePath is not provided, try to get it from the active editor or first workspace folder | ||
| if (!workspacePath) { | ||
|
|
@@ -119,6 +124,10 @@ export class CodeIndexManager { | |
| public async initialize(contextProxy: ContextProxy): Promise<{ requiresRestart: boolean }> { | ||
| // 1. ConfigManager Initialization and Configuration Loading | ||
| 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) | ||
|
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 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? |
||
| } | ||
| this._configManager = new CodeIndexConfigManager(contextProxy) | ||
| } | ||
| // Load configuration once to get current state and restart requirements | ||
|
|
||
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.