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}`, diff --git a/src/services/code-index/__tests__/config-manager.test.ts b/src/services/code-index/__tests__/config-manager.test.ts index 7b27139943..b0aa024248 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,329 @@ 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("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 + 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", () => { @@ -189,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/__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/__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") + }) + }) +}) diff --git a/src/services/code-index/config-manager.ts b/src/services/code-index/config-manager.ts index 0b0bf2e7e8..730f43e3c5 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. @@ -18,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, @@ -69,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 ?? "" @@ -81,6 +63,42 @@ 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) return { configSnapshot: previousConfigSnapshot, @@ -95,7 +113,7 @@ export class CodeIndexConfigManager { qdrantApiKey: this.qdrantApiKey, searchMinScore: this.searchMinScore, }, - requiresRestart: this._didConfigChangeRequireRestart(previousConfigSnapshot), + requiresRestart, } } @@ -103,7 +121,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 +138,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 - 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 ?? "" + const prevOllamaBaseUrl = prev?.ollamaBaseUrl ?? "" + const prevQdrantUrl = prev?.qdrantUrl ?? "" + const prevQdrantApiKey = prev?.qdrantApiKey ?? "" - // 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 + } + + // 3. If wasn't ready before and isn't ready now, no restart needed + if (!prevConfigured && !nowConfigured) { + return false + } - // Check OpenAI settings change if using OpenAI + // 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 ?? "" + 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 ?? "" + 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 ?? "" + const currentQdrantApiKey = this.qdrantApiKey ?? "" + + if (prevQdrantUrl !== currentQdrantUrl || prevQdrantApiKey !== currentQdrantApiKey) { return true } } @@ -165,6 +205,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. */ diff --git a/src/services/code-index/manager.ts b/src/services/code-index/manager.ts index 6922ae90da..465fa95d0c 100644 --- a/src/services/code-index/manager.ts +++ b/src/services/code-index/manager.ts @@ -99,7 +99,10 @@ 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) + } + // Load configuration once to get current state and restart requirements const { requiresRestart } = await this._configManager.loadConfiguration() // 2. Check if feature is enabled @@ -249,10 +252,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 and the manager is initialized, restart the service + if (requiresRestart && isFeatureEnabled && isFeatureConfigured && this.isInitialized) { + this.stopWatcher() + await this.startIndexing() + } } } } 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) diff --git a/webview-ui/src/components/settings/CodeIndexSettings.tsx b/webview-ui/src/components/settings/CodeIndexSettings.tsx index 8aaaa888bb..06bac1927d 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,21 @@ 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 + + const modelIdToUse = getModelIdForProvider( + newProvider, + currentProvider, + currentModelId, + codebaseIndexModels, + ) if (codebaseIndexConfig) { setCachedStateField("codebaseIndexConfig", { ...codebaseIndexConfig, codebaseIndexEmbedderProvider: newProvider, - codebaseIndexEmbedderModelId: defaultModelId, + codebaseIndexEmbedderModelId: modelIdToUse, }) } }}>