Fix settings saving logic to ensure the saved settings are used (#3976)

* feat: Enhance configuration change handling and model dimension checks

* fix: Update embedder creation to use modelId from config

* test: Add unit tests for ServiceFactory embedder and vector store creation

* test: Add comprehensive restart detection tests for configuration changes

* feat: Implement model ID selection logic for provider changes in CodeIndexSettings

* fix: Initialize configuration on constructor to prevent false restart triggers

* fix: Enhance API key handling and restart logic in CodeIndexConfigManager

* fix: Improve handling of external settings changes and automatic indexing in webviewMessageHandler

* fix: Ensure handleExternalSettingsChange only restarts service when manager is initialized

* refactor: remove console logs

* fix: Load configuration during initialization to ensure correct state and restart requirements
This commit is contained in:
Daniel 2025-05-25 17:25:20 -05:00 • committed by GitHub
parent 977fa26e01
commit 6a8fb599c1
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 990 additions and 73 deletions

View file

@ -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}`,

View file

@ -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)
})
})
})

View file

@ -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<vscode.ExtensionContext>
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()
})
})
})

View file

@ -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<typeof OpenAiEmbedder>
const MockedCodeIndexOllamaEmbedder = CodeIndexOllamaEmbedder as jest.MockedClass<typeof CodeIndexOllamaEmbedder>
const MockedQdrantVectorStore = QdrantVectorStore as jest.MockedClass<typeof QdrantVectorStore>
// Import the mocked functions
import { getDefaultModelId, getModelDimension } from "../../../shared/embeddingModels"
const mockGetDefaultModelId = getDefaultModelId as jest.MockedFunction<typeof getDefaultModelId>
const mockGetModelDimension = getModelDimension as jest.MockedFunction<typeof getModelDimension>
describe("CodeIndexServiceFactory", () => {
let factory: CodeIndexServiceFactory
let mockConfigManager: jest.Mocked<CodeIndexConfigManager>
let mockCacheManager: jest.Mocked<CacheManager>
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")
})
})
})

View file

@ -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
}
// Check OpenAI settings change if using OpenAI
if (this.embedderProvider === "openai") {
if (prev.openAiKey !== this.openAiOptions?.openAiNativeApiKey) return true
// Model ID check moved above
// 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
}
// Check Ollama settings change if using Ollama
if (this.embedderProvider === "ollama") {
if (prev.ollamaBaseUrl !== this.ollamaOptions?.ollamaBaseUrl) {
if (this._hasVectorDimensionChanged(prevProvider, prevModelId)) {
return true
}
// Authentication changes
if (this.embedderProvider === "openai") {
const currentOpenAiKey = this.openAiOptions?.openAiNativeApiKey ?? ""
if (prevOpenAiKey !== currentOpenAiKey) {
return true
}
// Model ID check moved above
}
// Check Qdrant settings changes
if (prev.qdrantUrl !== this.qdrantUrl || prev.qdrantApiKey !== this.qdrantApiKey) {
if (this.embedderProvider === "ollama") {
const currentOllamaBaseUrl = this.ollamaOptions?.ollamaBaseUrl ?? ""
if (prevOllamaBaseUrl !== currentOllamaBaseUrl) {
return true
}
}
// 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.
*/

View file

@ -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<void> {
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()
}
}
}
}

View file

@ -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)

View file

@ -97,6 +97,30 @@ export const CodeIndexSettings: React.FC<CodeIndexSettingsProps> = ({
}
}, [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<CodeIndexSettingsProps> = ({
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,
})
}
}}>