From 0c204cd71b86c1e5be849c0e970d8626313c3e87 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Sun, 25 May 2025 14:13:58 -0500 Subject: [PATCH 01/11] feat: Enhance configuration change handling and model dimension checks --- src/services/code-index/config-manager.ts | 103 ++++++++++++++++------ 1 file changed, 77 insertions(+), 26 deletions(-) diff --git a/src/services/code-index/config-manager.ts b/src/services/code-index/config-manager.ts index 0b0bf2e7e8..2942146a5e 100644 --- a/src/services/code-index/config-manager.ts +++ b/src/services/code-index/config-manager.ts @@ -3,6 +3,7 @@ import { ContextProxy } from "../../core/config/ContextProxy" import { EmbedderProvider } from "./interfaces/manager" import { CodeIndexConfig, PreviousConfigSnapshot } from "./interfaces/config" import { SEARCH_MIN_SCORE } from "./constants" +import { getDefaultModelId, getModelDimension } from "../../shared/embeddingModels" /** * Manages configuration state and validation for the code indexing feature. @@ -82,6 +83,8 @@ export class CodeIndexConfigManager { ollamaBaseUrl: codebaseIndexEmbedderBaseUrl, } + const requiresRestart = this.doesConfigChangeRequireRestart(previousConfigSnapshot) + return { configSnapshot: previousConfigSnapshot, currentConfig: { @@ -95,7 +98,7 @@ export class CodeIndexConfigManager { qdrantApiKey: this.qdrantApiKey, searchMinScore: this.searchMinScore, }, - requiresRestart: this._didConfigChangeRequireRestart(previousConfigSnapshot), + requiresRestart, } } @@ -103,7 +106,6 @@ export class CodeIndexConfigManager { * Checks if the service is properly configured based on the embedder type. */ public isConfigured(): boolean { - if (this.embedderProvider === "openai") { const openAiKey = this.openAiOptions?.openAiNativeApiKey const qdrantUrl = this.qdrantUrl @@ -121,43 +123,66 @@ export class CodeIndexConfigManager { /** * Determines if a configuration change requires restarting the indexing process. - * @param prev The previous configuration snapshot - * @returns boolean indicating whether a restart is needed */ - private _didConfigChangeRequireRestart(prev: PreviousConfigSnapshot): boolean { - const nowConfigured = this.isConfigured() // Recalculate based on current state + doesConfigChangeRequireRestart(prev: PreviousConfigSnapshot): boolean { + const nowConfigured = this.isConfigured() - // Check for transition from disabled/unconfigured to enabled+configured - const transitionedToReady = (!prev.enabled || !prev.configured) && this.isEnabled && nowConfigured - if (transitionedToReady) return true + // Handle null/undefined values safely + const prevEnabled = prev?.enabled ?? false + const prevConfigured = prev?.configured ?? false + const prevProvider = prev?.embedderProvider ?? "openai" + const prevModelId = prev?.modelId ?? undefined + const prevOpenAiKey = prev?.openAiKey ?? undefined + const prevOllamaBaseUrl = prev?.ollamaBaseUrl ?? undefined + const prevQdrantUrl = prev?.qdrantUrl ?? undefined + const prevQdrantApiKey = prev?.qdrantApiKey ?? undefined - // If wasn't ready before and isn't ready now, no restart needed for config change itself - if (!prev.configured && !nowConfigured) return false - // If was disabled and still is, no restart needed - if (!prev.enabled && !this.isEnabled) return false + // 1. Transition from disabled/unconfigured to enabled+configured + if ((!prevEnabled || !prevConfigured) && this.isEnabled && nowConfigured) { + return true + } - // Check for changes in relevant settings if the feature is enabled (or was enabled) - if (this.isEnabled || prev.enabled) { - // Check for embedder type change - if (prev.embedderProvider !== this.embedderProvider) return true - if (prev.modelId !== this.modelId) return true // Any model change requires restart + // 2. If was disabled and still is, no restart needed + if (!prevEnabled && !this.isEnabled) { + return false + } - // Check OpenAI settings change if using OpenAI + // 3. If wasn't ready before and isn't ready now, no restart needed + if (!prevConfigured && !nowConfigured) { + return false + } + + // 4. Check for changes in relevant settings if the feature is enabled (or was enabled) + if (this.isEnabled || prevEnabled) { + // Provider change + if (prevProvider !== this.embedderProvider) { + return true + } + + if (this._hasVectorDimensionChanged(prevProvider, prevModelId)) { + return true + } + + // Authentication changes if (this.embedderProvider === "openai") { - if (prev.openAiKey !== this.openAiOptions?.openAiNativeApiKey) return true - // Model ID check moved above + const currentOpenAiKey = this.openAiOptions?.openAiNativeApiKey ?? undefined + if (prevOpenAiKey !== currentOpenAiKey) { + return true + } } - // Check Ollama settings change if using Ollama if (this.embedderProvider === "ollama") { - if (prev.ollamaBaseUrl !== this.ollamaOptions?.ollamaBaseUrl) { + const currentOllamaBaseUrl = this.ollamaOptions?.ollamaBaseUrl ?? undefined + if (prevOllamaBaseUrl !== currentOllamaBaseUrl) { return true } - // Model ID check moved above } - // Check Qdrant settings changes - if (prev.qdrantUrl !== this.qdrantUrl || prev.qdrantApiKey !== this.qdrantApiKey) { + // Qdrant configuration changes + const currentQdrantUrl = this.qdrantUrl ?? undefined + const currentQdrantApiKey = this.qdrantApiKey ?? undefined + + if (prevQdrantUrl !== currentQdrantUrl || prevQdrantApiKey !== currentQdrantApiKey) { return true } } @@ -165,6 +190,32 @@ export class CodeIndexConfigManager { return false } + /** + * Checks if model changes result in vector dimension changes that require restart. + */ + private _hasVectorDimensionChanged(prevProvider: EmbedderProvider, prevModelId?: string): boolean { + const currentProvider = this.embedderProvider + const currentModelId = this.modelId ?? getDefaultModelId(currentProvider) + const resolvedPrevModelId = prevModelId ?? getDefaultModelId(prevProvider) + + // If model IDs are the same and provider is the same, no dimension change + if (prevProvider === currentProvider && resolvedPrevModelId === currentModelId) { + return false + } + + // Get vector dimensions for both models + const prevDimension = getModelDimension(prevProvider, resolvedPrevModelId) + const currentDimension = getModelDimension(currentProvider, currentModelId) + + // If we can't determine dimensions, be safe and restart + if (prevDimension === undefined || currentDimension === undefined) { + return true + } + + // Only restart if dimensions actually changed + return prevDimension !== currentDimension + } + /** * Gets the current configuration state. */ From 8a4d4754ad4db908671f603074fe488739c86b22 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Sun, 25 May 2025 14:17:03 -0500 Subject: [PATCH 02/11] fix: Update embedder creation to use modelId from config --- src/services/code-index/service-factory.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/services/code-index/service-factory.ts b/src/services/code-index/service-factory.ts index 84304a348c..7a95fe9556 100644 --- a/src/services/code-index/service-factory.ts +++ b/src/services/code-index/service-factory.ts @@ -31,12 +31,18 @@ export class CodeIndexServiceFactory { if (!config.openAiOptions?.openAiNativeApiKey) { throw new Error("OpenAI configuration missing for embedder creation") } - return new OpenAiEmbedder(config.openAiOptions) // Reverted temporarily + return new OpenAiEmbedder({ + ...config.openAiOptions, + openAiEmbeddingModelId: config.modelId, + }) } else if (provider === "ollama") { if (!config.ollamaOptions?.ollamaBaseUrl) { throw new Error("Ollama configuration missing for embedder creation") } - return new CodeIndexOllamaEmbedder(config.ollamaOptions) // Reverted temporarily + return new CodeIndexOllamaEmbedder({ + ...config.ollamaOptions, + ollamaModelId: config.modelId, + }) } throw new Error(`Invalid embedder type configured: ${config.embedderProvider}`) @@ -50,11 +56,8 @@ export class CodeIndexServiceFactory { const provider = config.embedderProvider as EmbedderProvider const defaultModel = getDefaultModelId(provider) - // Determine the modelId based on the provider and config, using apiModelId - const modelId = - provider === "openai" - ? (config.openAiOptions?.apiModelId ?? defaultModel) - : (config.ollamaOptions?.apiModelId ?? defaultModel) + // Use the embedding model ID from config, not the chat model IDs + const modelId = config.modelId ?? defaultModel const vectorSize = getModelDimension(provider, modelId) From 85ea51736e16496ec5f479ebfe8ebd72e448af8e Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Sun, 25 May 2025 14:20:23 -0500 Subject: [PATCH 03/11] test: Add unit tests for ServiceFactory embedder and vector store creation --- .../__tests__/service-factory.test.ts | 287 ++++++++++++++++++ 1 file changed, 287 insertions(+) create mode 100644 src/services/code-index/__tests__/service-factory.test.ts diff --git a/src/services/code-index/__tests__/service-factory.test.ts b/src/services/code-index/__tests__/service-factory.test.ts new file mode 100644 index 0000000000..90ce46b97f --- /dev/null +++ b/src/services/code-index/__tests__/service-factory.test.ts @@ -0,0 +1,287 @@ +import { CodeIndexServiceFactory } from "../service-factory" +import { CodeIndexConfigManager } from "../config-manager" +import { CacheManager } from "../cache-manager" +import { OpenAiEmbedder } from "../embedders/openai" +import { CodeIndexOllamaEmbedder } from "../embedders/ollama" +import { QdrantVectorStore } from "../vector-store/qdrant-client" + +// Mock the embedders and vector store +jest.mock("../embedders/openai") +jest.mock("../embedders/ollama") +jest.mock("../vector-store/qdrant-client") + +// Mock the embedding models module +jest.mock("../../../shared/embeddingModels", () => ({ + getDefaultModelId: jest.fn(), + getModelDimension: jest.fn(), +})) + +const MockedOpenAiEmbedder = OpenAiEmbedder as jest.MockedClass +const MockedCodeIndexOllamaEmbedder = CodeIndexOllamaEmbedder as jest.MockedClass +const MockedQdrantVectorStore = QdrantVectorStore as jest.MockedClass + +// Import the mocked functions +import { getDefaultModelId, getModelDimension } from "../../../shared/embeddingModels" +const mockGetDefaultModelId = getDefaultModelId as jest.MockedFunction +const mockGetModelDimension = getModelDimension as jest.MockedFunction + +describe("CodeIndexServiceFactory", () => { + let factory: CodeIndexServiceFactory + let mockConfigManager: jest.Mocked + let mockCacheManager: jest.Mocked + + beforeEach(() => { + jest.clearAllMocks() + + mockConfigManager = { + getConfig: jest.fn(), + } as any + + mockCacheManager = {} as any + + factory = new CodeIndexServiceFactory(mockConfigManager, "/test/workspace", mockCacheManager) + }) + + describe("createEmbedder", () => { + it("should pass model ID to OpenAI embedder when using OpenAI provider", () => { + // Arrange + const testModelId = "text-embedding-3-large" + const testConfig = { + embedderProvider: "openai", + modelId: testModelId, + openAiOptions: { + openAiNativeApiKey: "test-api-key", + }, + } + mockConfigManager.getConfig.mockReturnValue(testConfig as any) + + // Act + factory.createEmbedder() + + // Assert + expect(MockedOpenAiEmbedder).toHaveBeenCalledWith({ + openAiNativeApiKey: "test-api-key", + openAiEmbeddingModelId: testModelId, + }) + }) + + it("should pass model ID to Ollama embedder when using Ollama provider", () => { + // Arrange + const testModelId = "nomic-embed-text:latest" + const testConfig = { + embedderProvider: "ollama", + modelId: testModelId, + ollamaOptions: { + ollamaBaseUrl: "http://localhost:11434", + }, + } + mockConfigManager.getConfig.mockReturnValue(testConfig as any) + + // Act + factory.createEmbedder() + + // Assert + expect(MockedCodeIndexOllamaEmbedder).toHaveBeenCalledWith({ + ollamaBaseUrl: "http://localhost:11434", + ollamaModelId: testModelId, + }) + }) + + it("should handle undefined model ID for OpenAI embedder", () => { + // Arrange + const testConfig = { + embedderProvider: "openai", + modelId: undefined, + openAiOptions: { + openAiNativeApiKey: "test-api-key", + }, + } + mockConfigManager.getConfig.mockReturnValue(testConfig as any) + + // Act + factory.createEmbedder() + + // Assert + expect(MockedOpenAiEmbedder).toHaveBeenCalledWith({ + openAiNativeApiKey: "test-api-key", + openAiEmbeddingModelId: undefined, + }) + }) + + it("should handle undefined model ID for Ollama embedder", () => { + // Arrange + const testConfig = { + embedderProvider: "ollama", + modelId: undefined, + ollamaOptions: { + ollamaBaseUrl: "http://localhost:11434", + }, + } + mockConfigManager.getConfig.mockReturnValue(testConfig as any) + + // Act + factory.createEmbedder() + + // Assert + expect(MockedCodeIndexOllamaEmbedder).toHaveBeenCalledWith({ + ollamaBaseUrl: "http://localhost:11434", + ollamaModelId: undefined, + }) + }) + + it("should throw error when OpenAI API key is missing", () => { + // Arrange + const testConfig = { + embedderProvider: "openai", + modelId: "text-embedding-3-large", + openAiOptions: { + openAiNativeApiKey: undefined, + }, + } + mockConfigManager.getConfig.mockReturnValue(testConfig as any) + + // Act & Assert + expect(() => factory.createEmbedder()).toThrow("OpenAI configuration missing for embedder creation") + }) + + it("should throw error when Ollama base URL is missing", () => { + // Arrange + const testConfig = { + embedderProvider: "ollama", + modelId: "nomic-embed-text:latest", + ollamaOptions: { + ollamaBaseUrl: undefined, + }, + } + mockConfigManager.getConfig.mockReturnValue(testConfig as any) + + // Act & Assert + expect(() => factory.createEmbedder()).toThrow("Ollama configuration missing for embedder creation") + }) + + it("should throw error for invalid embedder provider", () => { + // Arrange + const testConfig = { + embedderProvider: "invalid-provider", + modelId: "some-model", + } + mockConfigManager.getConfig.mockReturnValue(testConfig as any) + + // Act & Assert + expect(() => factory.createEmbedder()).toThrow("Invalid embedder type configured: invalid-provider") + }) + }) + + describe("createVectorStore", () => { + beforeEach(() => { + jest.clearAllMocks() + mockGetDefaultModelId.mockReturnValue("default-model") + }) + + it("should use config.modelId for OpenAI provider", () => { + // Arrange + const testModelId = "text-embedding-3-large" + const testConfig = { + embedderProvider: "openai", + modelId: testModelId, + qdrantUrl: "http://localhost:6333", + qdrantApiKey: "test-key", + } + mockConfigManager.getConfig.mockReturnValue(testConfig as any) + mockGetModelDimension.mockReturnValue(3072) + + // Act + factory.createVectorStore() + + // Assert + expect(mockGetModelDimension).toHaveBeenCalledWith("openai", testModelId) + expect(MockedQdrantVectorStore).toHaveBeenCalledWith( + "/test/workspace", + "http://localhost:6333", + 3072, + "test-key", + ) + }) + + it("should use config.modelId for Ollama provider", () => { + // Arrange + const testModelId = "nomic-embed-text:latest" + const testConfig = { + embedderProvider: "ollama", + modelId: testModelId, + qdrantUrl: "http://localhost:6333", + qdrantApiKey: "test-key", + } + mockConfigManager.getConfig.mockReturnValue(testConfig as any) + mockGetModelDimension.mockReturnValue(768) + + // Act + factory.createVectorStore() + + // Assert + expect(mockGetModelDimension).toHaveBeenCalledWith("ollama", testModelId) + expect(MockedQdrantVectorStore).toHaveBeenCalledWith( + "/test/workspace", + "http://localhost:6333", + 768, + "test-key", + ) + }) + + it("should use default model when config.modelId is undefined", () => { + // Arrange + const testConfig = { + embedderProvider: "openai", + modelId: undefined, + qdrantUrl: "http://localhost:6333", + qdrantApiKey: "test-key", + } + mockConfigManager.getConfig.mockReturnValue(testConfig as any) + mockGetModelDimension.mockReturnValue(1536) + + // Act + factory.createVectorStore() + + // Assert + expect(mockGetModelDimension).toHaveBeenCalledWith("openai", "default-model") + expect(MockedQdrantVectorStore).toHaveBeenCalledWith( + "/test/workspace", + "http://localhost:6333", + 1536, + "test-key", + ) + }) + + it("should throw error when vector dimension cannot be determined", () => { + // Arrange + const testConfig = { + embedderProvider: "openai", + modelId: "unknown-model", + qdrantUrl: "http://localhost:6333", + qdrantApiKey: "test-key", + } + mockConfigManager.getConfig.mockReturnValue(testConfig as any) + mockGetModelDimension.mockReturnValue(undefined) + + // Act & Assert + expect(() => factory.createVectorStore()).toThrow( + "Could not determine vector dimension for model 'unknown-model'. Check model profiles or config.", + ) + }) + + it("should throw error when Qdrant URL is missing", () => { + // Arrange + const testConfig = { + embedderProvider: "openai", + modelId: "text-embedding-3-small", + qdrantUrl: undefined, + qdrantApiKey: "test-key", + } + mockConfigManager.getConfig.mockReturnValue(testConfig as any) + mockGetModelDimension.mockReturnValue(1536) + + // Act & Assert + expect(() => factory.createVectorStore()).toThrow("Qdrant URL missing for vector store creation") + }) + }) +}) From 27d7a069ad3786bde302c5eac039ff3ce774102c Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Sun, 25 May 2025 14:21:35 -0500 Subject: [PATCH 04/11] test: Add comprehensive restart detection tests for configuration changes --- .../__tests__/config-manager.test.ts | 256 +++++++++++++++++- 1 file changed, 254 insertions(+), 2 deletions(-) diff --git a/src/services/code-index/__tests__/config-manager.test.ts b/src/services/code-index/__tests__/config-manager.test.ts index 7b27139943..eddff5213d 100644 --- a/src/services/code-index/__tests__/config-manager.test.ts +++ b/src/services/code-index/__tests__/config-manager.test.ts @@ -75,13 +75,17 @@ describe("CodeIndexConfigManager", () => { }) it("should detect restart requirement when provider changes", async () => { - // Initial state + // Initial state - properly configured mockContextProxy.getGlobalState.mockReturnValue({ codebaseIndexEnabled: true, codebaseIndexQdrantUrl: "http://qdrant.local", codebaseIndexEmbedderProvider: "openai", codebaseIndexEmbedderModelId: "text-embedding-3-large", }) + mockContextProxy.getSecret.mockImplementation((key: string) => { + if (key === "codeIndexOpenAiKey") return "test-openai-key" + return undefined + }) await configManager.loadConfiguration() @@ -91,12 +95,260 @@ describe("CodeIndexConfigManager", () => { codebaseIndexQdrantUrl: "http://qdrant.local", codebaseIndexEmbedderProvider: "ollama", codebaseIndexEmbedderBaseUrl: "http://ollama.local", - codebaseIndexEmbedderModelId: "llama2", + codebaseIndexEmbedderModelId: "nomic-embed-text", }) const result = await configManager.loadConfiguration() expect(result.requiresRestart).toBe(true) }) + + it("should detect restart requirement when vector dimensions change", async () => { + // Initial state with text-embedding-3-small (1536D) + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "openai", + codebaseIndexEmbedderModelId: "text-embedding-3-small", + }) + mockContextProxy.getSecret.mockReturnValue("test-key") + + await configManager.loadConfiguration() + + // Change to text-embedding-3-large (3072D) + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "openai", + codebaseIndexEmbedderModelId: "text-embedding-3-large", + }) + + const result = await configManager.loadConfiguration() + expect(result.requiresRestart).toBe(true) + }) + + it("should NOT require restart when models have same dimensions", async () => { + // Initial state with text-embedding-3-small (1536D) + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "openai", + codebaseIndexEmbedderModelId: "text-embedding-3-small", + }) + mockContextProxy.getSecret.mockImplementation((key: string) => { + if (key === "codeIndexOpenAiKey") return "test-key" + return undefined + }) + + await configManager.loadConfiguration() + + // Change to text-embedding-ada-002 (also 1536D) + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "openai", + codebaseIndexEmbedderModelId: "text-embedding-ada-002", + }) + + const result = await configManager.loadConfiguration() + expect(result.requiresRestart).toBe(false) + }) + + it("should detect restart requirement when transitioning to enabled+configured", async () => { + // Initial state - disabled + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: false, + }) + + await configManager.loadConfiguration() + + // Enable and configure + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "openai", + codebaseIndexEmbedderModelId: "text-embedding-3-small", + }) + mockContextProxy.getSecret.mockReturnValue("test-key") + + const result = await configManager.loadConfiguration() + expect(result.requiresRestart).toBe(true) + }) + + describe("simplified restart detection", () => { + it("should detect restart requirement for API key changes", async () => { + // Initial state + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "openai", + codebaseIndexEmbedderModelId: "text-embedding-3-small", + }) + mockContextProxy.getSecret.mockReturnValue("old-key") + + await configManager.loadConfiguration() + + // Change API key + mockContextProxy.getSecret.mockImplementation((key: string) => { + if (key === "codeIndexOpenAiKey") return "new-key" + return undefined + }) + + const result = await configManager.loadConfiguration() + expect(result.requiresRestart).toBe(true) + }) + + it("should detect restart requirement for Qdrant URL changes", async () => { + // Initial state + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://old-qdrant.local", + codebaseIndexEmbedderProvider: "openai", + codebaseIndexEmbedderModelId: "text-embedding-3-small", + }) + mockContextProxy.getSecret.mockReturnValue("test-key") + + await configManager.loadConfiguration() + + // Change Qdrant URL + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://new-qdrant.local", + codebaseIndexEmbedderProvider: "openai", + codebaseIndexEmbedderModelId: "text-embedding-3-small", + }) + + const result = await configManager.loadConfiguration() + expect(result.requiresRestart).toBe(true) + }) + + it("should handle unknown model dimensions safely", async () => { + // Initial state with known model + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "openai", + codebaseIndexEmbedderModelId: "text-embedding-3-small", + }) + mockContextProxy.getSecret.mockReturnValue("test-key") + + await configManager.loadConfiguration() + + // Change to unknown model + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "openai", + codebaseIndexEmbedderModelId: "unknown-model", + }) + + const result = await configManager.loadConfiguration() + expect(result.requiresRestart).toBe(true) + }) + + it("should handle Ollama configuration changes", async () => { + // Initial state + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "ollama", + codebaseIndexEmbedderBaseUrl: "http://old-ollama.local", + codebaseIndexEmbedderModelId: "nomic-embed-text", + }) + + await configManager.loadConfiguration() + + // Change Ollama base URL + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "ollama", + codebaseIndexEmbedderBaseUrl: "http://new-ollama.local", + codebaseIndexEmbedderModelId: "nomic-embed-text", + }) + + const result = await configManager.loadConfiguration() + expect(result.requiresRestart).toBe(true) + }) + + it("should not require restart when disabled remains disabled", async () => { + // Initial state - disabled but configured + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: false, + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "openai", + }) + mockContextProxy.getSecret.mockImplementation((key: string) => { + if (key === "codeIndexOpenAiKey") return "test-key" + return undefined + }) + + await configManager.loadConfiguration() + + // Still disabled but change other settings + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: false, + codebaseIndexQdrantUrl: "http://different-qdrant.local", + codebaseIndexEmbedderProvider: "ollama", + codebaseIndexEmbedderBaseUrl: "http://ollama.local", + }) + + const result = await configManager.loadConfiguration() + expect(result.requiresRestart).toBe(false) + }) + + it("should not require restart when unconfigured remains unconfigured", async () => { + // Initial state - enabled but unconfigured (missing API key) + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "openai", + }) + mockContextProxy.getSecret.mockReturnValue(undefined) + + await configManager.loadConfiguration() + + // Still unconfigured but change model + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "openai", + codebaseIndexEmbedderModelId: "text-embedding-3-large", + }) + + const result = await configManager.loadConfiguration() + expect(result.requiresRestart).toBe(false) + }) + }) + + describe("getRestartInfo public method", () => { + it("should provide restart info without loading configuration", async () => { + // Setup initial state + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "openai", + codebaseIndexEmbedderModelId: "text-embedding-3-small", + }) + mockContextProxy.getSecret.mockReturnValue("test-key") + + await configManager.loadConfiguration() + + // Create a mock previous config + const mockPrevConfig = { + enabled: true, + configured: true, + embedderProvider: "openai" as const, + modelId: "text-embedding-3-large", // Different model with different dimensions + openAiKey: "test-key", + ollamaBaseUrl: undefined, + qdrantUrl: "http://qdrant.local", + qdrantApiKey: undefined, + } + + const requiresRestart = configManager.doesConfigChangeRequireRestart(mockPrevConfig) + expect(requiresRestart).toBe(true) + }) + }) }) describe("isConfigured", () => { From bfb41d5891d0bcc770e2074e4aba61bf9f48e1b8 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Sun, 25 May 2025 14:22:57 -0500 Subject: [PATCH 05/11] feat: Implement model ID selection logic for provider changes in CodeIndexSettings --- .../components/settings/CodeIndexSettings.tsx | 51 +++++++++++++++++-- 1 file changed, 47 insertions(+), 4 deletions(-) diff --git a/webview-ui/src/components/settings/CodeIndexSettings.tsx b/webview-ui/src/components/settings/CodeIndexSettings.tsx index 8aaaa888bb..64211232ee 100644 --- a/webview-ui/src/components/settings/CodeIndexSettings.tsx +++ b/webview-ui/src/components/settings/CodeIndexSettings.tsx @@ -97,6 +97,30 @@ export const CodeIndexSettings: React.FC = ({ } }, [codebaseIndexConfig, codebaseIndexModels]) + /** + * Determines the appropriate model ID when changing providers + */ + function getModelIdForProvider( + newProvider: EmbedderProvider, + currentProvider: EmbedderProvider | undefined, + currentModelId: string | undefined, + availableModels: CodebaseIndexModels | undefined, + ): string { + if (newProvider === currentProvider && currentModelId) { + return currentModelId + } + + const models = availableModels?.[newProvider] + const modelIds = models ? Object.keys(models) : [] + + if (currentModelId && modelIds.includes(currentModelId)) { + return currentModelId + } + + const selectedModel = modelIds.length > 0 ? modelIds[0] : "" + return selectedModel + } + function validateIndexingConfig(config: CodebaseIndexConfig | undefined, apiConfig: ProviderSettings): boolean { if (!config) return false @@ -210,15 +234,34 @@ export const CodeIndexSettings: React.FC = ({ value={codebaseIndexConfig?.codebaseIndexEmbedderProvider || "openai"} onValueChange={(value) => { const newProvider = value as EmbedderProvider - const models = codebaseIndexModels?.[newProvider] - const modelIds = models ? Object.keys(models) : [] - const defaultModelId = modelIds.length > 0 ? modelIds[0] : "" // Use empty string if no models + const currentProvider = codebaseIndexConfig?.codebaseIndexEmbedderProvider + const currentModelId = codebaseIndexConfig?.codebaseIndexEmbedderModelId + + console.log("[CodeIndexSettings] Provider selection changed:", { + newProvider, + currentProvider, + currentModelId, + providerChanged: newProvider !== currentProvider, + }) + + const modelIdToUse = getModelIdForProvider( + newProvider, + currentProvider, + currentModelId, + codebaseIndexModels, + ) + + console.log("[CodeIndexSettings] Setting new config:", { + provider: newProvider, + modelId: modelIdToUse, + modelChanged: currentModelId !== modelIdToUse, + }) if (codebaseIndexConfig) { setCachedStateField("codebaseIndexConfig", { ...codebaseIndexConfig, codebaseIndexEmbedderProvider: newProvider, - codebaseIndexEmbedderModelId: defaultModelId, + codebaseIndexEmbedderModelId: modelIdToUse, }) } }}> From 53c4755159d0374f03de942cb35fd895e60b8375 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Sun, 25 May 2025 15:10:48 -0500 Subject: [PATCH 06/11] fix: Initialize configuration on constructor to prevent false restart triggers --- src/services/code-index/config-manager.ts | 91 +++++++++++++---------- 1 file changed, 53 insertions(+), 38 deletions(-) diff --git a/src/services/code-index/config-manager.ts b/src/services/code-index/config-manager.ts index 2942146a5e..730f43e3c5 100644 --- a/src/services/code-index/config-manager.ts +++ b/src/services/code-index/config-manager.ts @@ -19,38 +19,18 @@ export class CodeIndexConfigManager { private qdrantApiKey?: string private searchMinScore?: number - constructor(private readonly contextProxy: ContextProxy) {} + constructor(private readonly contextProxy: ContextProxy) { + // Initialize with current configuration to avoid false restart triggers + this._loadAndSetConfiguration() + } /** - * Loads persisted configuration from globalState. + * Private method that handles loading configuration from storage and updating instance variables. + * This eliminates code duplication between initializeWithCurrentConfig() and loadConfiguration(). */ - public async loadConfiguration(): Promise<{ - configSnapshot: PreviousConfigSnapshot - currentConfig: { - isEnabled: boolean - isConfigured: boolean - embedderProvider: EmbedderProvider - modelId?: string - openAiOptions?: ApiHandlerOptions - ollamaOptions?: ApiHandlerOptions - qdrantUrl?: string - qdrantApiKey?: string - searchMinScore?: number - } - requiresRestart: boolean - }> { - const previousConfigSnapshot: PreviousConfigSnapshot = { - enabled: this.isEnabled, - configured: this.isConfigured(), - embedderProvider: this.embedderProvider, - modelId: this.modelId, - openAiKey: this.openAiOptions?.openAiNativeApiKey, - ollamaBaseUrl: this.ollamaOptions?.ollamaBaseUrl, - qdrantUrl: this.qdrantUrl, - qdrantApiKey: this.qdrantApiKey, - } - - let codebaseIndexConfig = this.contextProxy?.getGlobalState("codebaseIndexConfig") ?? { + private _loadAndSetConfiguration(): void { + // Load configuration from storage + const codebaseIndexConfig = this.contextProxy?.getGlobalState("codebaseIndexConfig") ?? { codebaseIndexEnabled: false, codebaseIndexQdrantUrl: "http://localhost:6333", codebaseIndexSearchMinScore: 0.4, @@ -70,6 +50,7 @@ export class CodeIndexConfigManager { const openAiKey = this.contextProxy?.getSecret("codeIndexOpenAiKey") ?? "" const qdrantApiKey = this.contextProxy?.getSecret("codeIndexQdrantApiKey") ?? "" + // Update instance variables with configuration this.isEnabled = codebaseIndexEnabled || false this.qdrantUrl = codebaseIndexQdrantUrl this.qdrantApiKey = qdrantApiKey ?? "" @@ -82,6 +63,40 @@ export class CodeIndexConfigManager { this.ollamaOptions = { ollamaBaseUrl: codebaseIndexEmbedderBaseUrl, } + } + + /** + * Loads persisted configuration from globalState. + */ + public async loadConfiguration(): Promise<{ + configSnapshot: PreviousConfigSnapshot + currentConfig: { + isEnabled: boolean + isConfigured: boolean + embedderProvider: EmbedderProvider + modelId?: string + openAiOptions?: ApiHandlerOptions + ollamaOptions?: ApiHandlerOptions + qdrantUrl?: string + qdrantApiKey?: string + searchMinScore?: number + } + requiresRestart: boolean + }> { + // Capture the ACTUAL previous state before loading new configuration + const previousConfigSnapshot: PreviousConfigSnapshot = { + enabled: this.isEnabled, + configured: this.isConfigured(), + embedderProvider: this.embedderProvider, + modelId: this.modelId, + openAiKey: this.openAiOptions?.openAiNativeApiKey ?? "", + ollamaBaseUrl: this.ollamaOptions?.ollamaBaseUrl ?? "", + qdrantUrl: this.qdrantUrl ?? "", + qdrantApiKey: this.qdrantApiKey ?? "", + } + + // Load new configuration from storage and update instance variables + this._loadAndSetConfiguration() const requiresRestart = this.doesConfigChangeRequireRestart(previousConfigSnapshot) @@ -127,15 +142,15 @@ export class CodeIndexConfigManager { doesConfigChangeRequireRestart(prev: PreviousConfigSnapshot): boolean { const nowConfigured = this.isConfigured() - // Handle null/undefined values safely + // Handle null/undefined values safely - use empty strings for consistency with loaded config const prevEnabled = prev?.enabled ?? false const prevConfigured = prev?.configured ?? false const prevProvider = prev?.embedderProvider ?? "openai" const prevModelId = prev?.modelId ?? undefined - const prevOpenAiKey = prev?.openAiKey ?? undefined - const prevOllamaBaseUrl = prev?.ollamaBaseUrl ?? undefined - const prevQdrantUrl = prev?.qdrantUrl ?? undefined - const prevQdrantApiKey = prev?.qdrantApiKey ?? undefined + const prevOpenAiKey = prev?.openAiKey ?? "" + const prevOllamaBaseUrl = prev?.ollamaBaseUrl ?? "" + const prevQdrantUrl = prev?.qdrantUrl ?? "" + const prevQdrantApiKey = prev?.qdrantApiKey ?? "" // 1. Transition from disabled/unconfigured to enabled+configured if ((!prevEnabled || !prevConfigured) && this.isEnabled && nowConfigured) { @@ -165,22 +180,22 @@ export class CodeIndexConfigManager { // Authentication changes if (this.embedderProvider === "openai") { - const currentOpenAiKey = this.openAiOptions?.openAiNativeApiKey ?? undefined + const currentOpenAiKey = this.openAiOptions?.openAiNativeApiKey ?? "" if (prevOpenAiKey !== currentOpenAiKey) { return true } } if (this.embedderProvider === "ollama") { - const currentOllamaBaseUrl = this.ollamaOptions?.ollamaBaseUrl ?? undefined + const currentOllamaBaseUrl = this.ollamaOptions?.ollamaBaseUrl ?? "" if (prevOllamaBaseUrl !== currentOllamaBaseUrl) { return true } } // Qdrant configuration changes - const currentQdrantUrl = this.qdrantUrl ?? undefined - const currentQdrantApiKey = this.qdrantApiKey ?? undefined + const currentQdrantUrl = this.qdrantUrl ?? "" + const currentQdrantApiKey = this.qdrantApiKey ?? "" if (prevQdrantUrl !== currentQdrantUrl || prevQdrantApiKey !== currentQdrantApiKey) { return true From e556eafa084301f120d4fc3ed84f5ee554241328 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Sun, 25 May 2025 15:11:58 -0500 Subject: [PATCH 07/11] fix: Enhance API key handling and restart logic in CodeIndexConfigManager --- .../__tests__/config-manager.test.ts | 138 ++++++++++++++++++ src/services/code-index/manager.ts | 18 ++- 2 files changed, 154 insertions(+), 2 deletions(-) diff --git a/src/services/code-index/__tests__/config-manager.test.ts b/src/services/code-index/__tests__/config-manager.test.ts index eddff5213d..b0aa024248 100644 --- a/src/services/code-index/__tests__/config-manager.test.ts +++ b/src/services/code-index/__tests__/config-manager.test.ts @@ -320,6 +320,75 @@ describe("CodeIndexConfigManager", () => { }) }) + describe("empty/missing API key handling", () => { + it("should not require restart when API keys are consistently empty", async () => { + // Initial state with no API keys (undefined from secrets) + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "openai", + codebaseIndexEmbedderModelId: "text-embedding-3-small", + }) + mockContextProxy.getSecret.mockReturnValue(undefined) + + await configManager.loadConfiguration() + + // Change an unrelated setting while keeping API keys empty + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "openai", + codebaseIndexEmbedderModelId: "text-embedding-3-small", + codebaseIndexSearchMinScore: 0.5, // Changed unrelated setting + }) + + const result = await configManager.loadConfiguration() + // Should NOT require restart since API keys are consistently empty + expect(result.requiresRestart).toBe(false) + }) + + it("should not require restart when API keys transition from undefined to empty string", async () => { + // Initial state with undefined API keys + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: false, // Start disabled to avoid restart due to enable+configure + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "openai", + }) + mockContextProxy.getSecret.mockReturnValue(undefined) + + await configManager.loadConfiguration() + + // Change to empty string API keys (simulating what happens when secrets return "") + mockContextProxy.getSecret.mockReturnValue("") + + const result = await configManager.loadConfiguration() + // Should NOT require restart since undefined and "" are both "empty" + expect(result.requiresRestart).toBe(false) + }) + + it("should require restart when API key actually changes from empty to non-empty", async () => { + // Initial state with empty API key + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "openai", + }) + mockContextProxy.getSecret.mockReturnValue("") + + await configManager.loadConfiguration() + + // Add actual API key + mockContextProxy.getSecret.mockImplementation((key: string) => { + if (key === "codeIndexOpenAiKey") return "actual-api-key" + return "" + }) + + const result = await configManager.loadConfiguration() + // Should require restart since we went from empty to actual key + expect(result.requiresRestart).toBe(true) + }) + }) + describe("getRestartInfo public method", () => { it("should provide restart info without loading configuration", async () => { // Setup initial state @@ -441,4 +510,73 @@ describe("CodeIndexConfigManager", () => { expect(configManager.currentModelId).toBe("text-embedding-3-large") }) }) + + describe("initialization and restart prevention", () => { + it("should not require restart when configuration hasn't changed between calls", async () => { + // Setup initial configuration - start with enabled and configured to avoid initial transition restart + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "openai", + codebaseIndexEmbedderModelId: "text-embedding-3-small", + }) + mockContextProxy.getSecret.mockImplementation((key: string) => { + if (key === "codeIndexOpenAiKey") return "test-key" + return undefined + }) + + // First load - this will initialize the config manager with current state + await configManager.loadConfiguration() + + // Second load with same configuration - should not require restart + const secondResult = await configManager.loadConfiguration() + expect(secondResult.requiresRestart).toBe(false) + }) + + it("should properly initialize with current config to prevent false restarts", async () => { + // Setup configuration + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: false, // Start disabled to avoid transition restart + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "openai", + codebaseIndexEmbedderModelId: "text-embedding-3-small", + }) + mockContextProxy.getSecret.mockImplementation((key: string) => { + if (key === "codeIndexOpenAiKey") return "test-key" + return undefined + }) + + // Create a new config manager (simulating what happens in CodeIndexManager.initialize) + const newConfigManager = new CodeIndexConfigManager(mockContextProxy) + + // Load configuration - should not require restart since the manager should be initialized with current config + const result = await newConfigManager.loadConfiguration() + expect(result.requiresRestart).toBe(false) + }) + + it("should not require restart when settings are saved but code indexing config unchanged", async () => { + // This test simulates the original issue: handleExternalSettingsChange() being called + // when other settings are saved, but code indexing settings haven't changed + + // Setup initial state - enabled and configured + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexQdrantUrl: "http://qdrant.local", + codebaseIndexEmbedderProvider: "openai", + codebaseIndexEmbedderModelId: "text-embedding-3-small", + }) + mockContextProxy.getSecret.mockImplementation((key: string) => { + if (key === "codeIndexOpenAiKey") return "test-key" + return undefined + }) + + // First load to establish baseline + await configManager.loadConfiguration() + + // Simulate external settings change where code indexing config hasn't changed + // (this is what happens when other settings are saved) + const result = await configManager.loadConfiguration() + expect(result.requiresRestart).toBe(false) + }) + }) }) diff --git a/src/services/code-index/manager.ts b/src/services/code-index/manager.ts index 6922ae90da..42015c2621 100644 --- a/src/services/code-index/manager.ts +++ b/src/services/code-index/manager.ts @@ -99,7 +99,11 @@ export class CodeIndexManager { */ public async initialize(contextProxy: ContextProxy): Promise<{ requiresRestart: boolean }> { // 1. ConfigManager Initialization and Configuration Loading - this._configManager = new CodeIndexConfigManager(contextProxy) + if (!this._configManager) { + this._configManager = new CodeIndexConfigManager(contextProxy) + // For first initialization, load configuration to set up initial state + await this._configManager.loadConfiguration() + } const { requiresRestart } = await this._configManager.loadConfiguration() // 2. Check if feature is enabled @@ -249,10 +253,20 @@ export class CodeIndexManager { * Handles external settings changes by reloading configuration. * This method should be called when API provider settings are updated * to ensure the CodeIndexConfigManager picks up the new configuration. + * If the configuration changes require a restart, the service will be restarted. */ public async handleExternalSettingsChange(): Promise { if (this._configManager) { - await this._configManager.loadConfiguration() + const { requiresRestart } = await this._configManager.loadConfiguration() + + const isFeatureEnabled = this.isFeatureEnabled + const isFeatureConfigured = this.isFeatureConfigured + + // If configuration changes require a restart, restart the service + if (requiresRestart && isFeatureEnabled && isFeatureConfigured) { + this.stopWatcher() + await this.startIndexing() + } } } } From 47e2fd42661a40f47cdf85a458085a8cebb0ed88 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Sun, 25 May 2025 16:22:28 -0500 Subject: [PATCH 08/11] fix: Improve handling of external settings changes and automatic indexing in webviewMessageHandler --- src/core/webview/webviewMessageHandler.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 09c472cc0d..c8fd3608e4 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -1331,7 +1331,18 @@ export const webviewMessageHandler = async (provider: ClineProvider, message: We await updateGlobalState("codebaseIndexConfig", codebaseIndexConfig) try { - await provider.codeIndexManager?.initialize(provider.contextProxy) + if (provider.codeIndexManager) { + await provider.codeIndexManager.handleExternalSettingsChange() + + // If now configured and enabled, start indexing automatically + if (provider.codeIndexManager.isFeatureEnabled && provider.codeIndexManager.isFeatureConfigured) { + if (!provider.codeIndexManager.isInitialized) { + await provider.codeIndexManager.initialize(provider.contextProxy) + } + // Start indexing in background (no await) + provider.codeIndexManager.startIndexing() + } + } } catch (error) { provider.log( `[CodeIndexManager] Error during background CodeIndexManager configuration/indexing: ${error.message || error}`, From 487e74bc7c4161b37901b9b59a92966f5f8b2d11 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Sun, 25 May 2025 16:26:53 -0500 Subject: [PATCH 09/11] fix: Ensure handleExternalSettingsChange only restarts service when manager is initialized --- .../code-index/__tests__/manager.test.ts | 117 ++++++++++++++++++ src/services/code-index/manager.ts | 4 +- 2 files changed, 119 insertions(+), 2 deletions(-) create mode 100644 src/services/code-index/__tests__/manager.test.ts diff --git a/src/services/code-index/__tests__/manager.test.ts b/src/services/code-index/__tests__/manager.test.ts new file mode 100644 index 0000000000..012a9450ee --- /dev/null +++ b/src/services/code-index/__tests__/manager.test.ts @@ -0,0 +1,117 @@ +import * as vscode from "vscode" +import { CodeIndexManager } from "../manager" +import { ContextProxy } from "../../../core/config/ContextProxy" + +// Mock only the essential dependencies +jest.mock("../../../utils/path", () => ({ + getWorkspacePath: jest.fn(() => "/test/workspace"), +})) + +jest.mock("../state-manager", () => ({ + CodeIndexStateManager: jest.fn().mockImplementation(() => ({ + onProgressUpdate: jest.fn(), + getCurrentStatus: jest.fn(), + dispose: jest.fn(), + })), +})) + +describe("CodeIndexManager - handleExternalSettingsChange regression", () => { + let mockContext: jest.Mocked + let manager: CodeIndexManager + + beforeEach(() => { + // Clear all instances before each test + CodeIndexManager.disposeAll() + + mockContext = { + subscriptions: [], + workspaceState: {} as any, + globalState: {} as any, + extensionUri: {} as any, + extensionPath: "/test/extension", + asAbsolutePath: jest.fn(), + storageUri: {} as any, + storagePath: "/test/storage", + globalStorageUri: {} as any, + globalStoragePath: "/test/global-storage", + logUri: {} as any, + logPath: "/test/log", + extensionMode: vscode.ExtensionMode.Test, + secrets: {} as any, + environmentVariableCollection: {} as any, + extension: {} as any, + languageModelAccessInformation: {} as any, + } + + manager = CodeIndexManager.getInstance(mockContext)! + }) + + afterEach(() => { + CodeIndexManager.disposeAll() + }) + + describe("handleExternalSettingsChange", () => { + it("should not throw when called on uninitialized manager (regression test)", async () => { + // This is the core regression test: handleExternalSettingsChange() should not throw + // when called before the manager is initialized (during first-time configuration) + + // Ensure manager is not initialized + expect(manager.isInitialized).toBe(false) + + // Mock a minimal config manager that simulates first-time configuration + const mockConfigManager = { + loadConfiguration: jest.fn().mockResolvedValue({ requiresRestart: true }), + } + ;(manager as any)._configManager = mockConfigManager + + // Mock the feature state to simulate valid configuration that would normally trigger restart + jest.spyOn(manager, "isFeatureEnabled", "get").mockReturnValue(true) + jest.spyOn(manager, "isFeatureConfigured", "get").mockReturnValue(true) + + // The key test: this should NOT throw "CodeIndexManager not initialized" error + await expect(manager.handleExternalSettingsChange()).resolves.not.toThrow() + + // Verify that loadConfiguration was called (the method should still work) + expect(mockConfigManager.loadConfiguration).toHaveBeenCalled() + }) + + it("should work normally when manager is initialized", async () => { + // Mock a minimal config manager + const mockConfigManager = { + loadConfiguration: jest.fn().mockResolvedValue({ requiresRestart: true }), + } + ;(manager as any)._configManager = mockConfigManager + + // Simulate an initialized manager by setting the required properties + ;(manager as any)._orchestrator = { stopWatcher: jest.fn() } + ;(manager as any)._searchService = {} + ;(manager as any)._cacheManager = {} + + // Verify manager is considered initialized + expect(manager.isInitialized).toBe(true) + + // Mock the methods that would be called during restart + const stopWatcherSpy = jest.spyOn(manager, "stopWatcher").mockImplementation() + const startIndexingSpy = jest.spyOn(manager, "startIndexing").mockResolvedValue() + + // Mock the feature state + jest.spyOn(manager, "isFeatureEnabled", "get").mockReturnValue(true) + jest.spyOn(manager, "isFeatureConfigured", "get").mockReturnValue(true) + + await manager.handleExternalSettingsChange() + + // Verify that the restart sequence was called + expect(mockConfigManager.loadConfiguration).toHaveBeenCalled() + expect(stopWatcherSpy).toHaveBeenCalled() + expect(startIndexingSpy).toHaveBeenCalled() + }) + + it("should handle case when config manager is not set", async () => { + // Ensure config manager is not set (edge case) + ;(manager as any)._configManager = undefined + + // This should not throw an error + await expect(manager.handleExternalSettingsChange()).resolves.not.toThrow() + }) + }) +}) diff --git a/src/services/code-index/manager.ts b/src/services/code-index/manager.ts index 42015c2621..73f776409e 100644 --- a/src/services/code-index/manager.ts +++ b/src/services/code-index/manager.ts @@ -262,8 +262,8 @@ export class CodeIndexManager { const isFeatureEnabled = this.isFeatureEnabled const isFeatureConfigured = this.isFeatureConfigured - // If configuration changes require a restart, restart the service - if (requiresRestart && isFeatureEnabled && isFeatureConfigured) { + // If configuration changes require a restart and the manager is initialized, restart the service + if (requiresRestart && isFeatureEnabled && isFeatureConfigured && this.isInitialized) { this.stopWatcher() await this.startIndexing() } From f93e2d7df44804413091171b84526dd70e65d278 Mon Sep 17 00:00:00 2001 From: Daniel <57051444+daniel-lxs@users.noreply.github.com> Date: Sun, 25 May 2025 16:29:38 -0500 Subject: [PATCH 10/11] refactor: remove console logs --- .../src/components/settings/CodeIndexSettings.tsx | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/webview-ui/src/components/settings/CodeIndexSettings.tsx b/webview-ui/src/components/settings/CodeIndexSettings.tsx index 64211232ee..06bac1927d 100644 --- a/webview-ui/src/components/settings/CodeIndexSettings.tsx +++ b/webview-ui/src/components/settings/CodeIndexSettings.tsx @@ -237,13 +237,6 @@ export const CodeIndexSettings: React.FC = ({ const currentProvider = codebaseIndexConfig?.codebaseIndexEmbedderProvider const currentModelId = codebaseIndexConfig?.codebaseIndexEmbedderModelId - console.log("[CodeIndexSettings] Provider selection changed:", { - newProvider, - currentProvider, - currentModelId, - providerChanged: newProvider !== currentProvider, - }) - const modelIdToUse = getModelIdForProvider( newProvider, currentProvider, @@ -251,12 +244,6 @@ export const CodeIndexSettings: React.FC = ({ codebaseIndexModels, ) - console.log("[CodeIndexSettings] Setting new config:", { - provider: newProvider, - modelId: modelIdToUse, - modelChanged: currentModelId !== modelIdToUse, - }) - if (codebaseIndexConfig) { setCachedStateField("codebaseIndexConfig", { ...codebaseIndexConfig, From 1658f580f57b5b869c04dfae883de18c15a00a92 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Sun, 25 May 2025 16:32:14 -0500 Subject: [PATCH 11/11] fix: Load configuration during initialization to ensure correct state and restart requirements --- src/services/code-index/manager.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/services/code-index/manager.ts b/src/services/code-index/manager.ts index 73f776409e..465fa95d0c 100644 --- a/src/services/code-index/manager.ts +++ b/src/services/code-index/manager.ts @@ -101,9 +101,8 @@ export class CodeIndexManager { // 1. ConfigManager Initialization and Configuration Loading if (!this._configManager) { this._configManager = new CodeIndexConfigManager(contextProxy) - // For first initialization, load configuration to set up initial state - await this._configManager.loadConfiguration() } + // Load configuration once to get current state and restart requirements const { requiresRestart } = await this._configManager.loadConfiguration() // 2. Check if feature is enabled