Skip to content

Commit 3725ebd

Browse files
committed
feat: add proactive embedder validation on provider switch
- Validate embedder connection when switching providers - Prevent misleading 'Indexed' status when embedder is unavailable - Show immediate error feedback for invalid configurations - Add comprehensive test coverage for validation flow This ensures users get immediate feedback when configuring embedders, preventing confusion when providers like Ollama are not accessible.
1 parent 2a53db1 commit 3725ebd

File tree

3 files changed

+237
-2
lines changed

3 files changed

+237
-2
lines changed

src/core/webview/webviewMessageHandler.ts

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1830,8 +1830,12 @@ export const webviewMessageHandler = async (
18301830
const settings = message.codeIndexSettings
18311831

18321832
try {
1833-
// Save global state settings atomically (without codebaseIndexEnabled which is now in global settings)
1833+
// Check if embedder provider has changed
18341834
const currentConfig = getGlobalState("codebaseIndexConfig") || {}
1835+
const embedderProviderChanged =
1836+
currentConfig.codebaseIndexEmbedderProvider !== settings.codebaseIndexEmbedderProvider
1837+
1838+
// Save global state settings atomically (without codebaseIndexEnabled which is now in global settings)
18351839
const globalStateConfig = {
18361840
...currentConfig,
18371841
codebaseIndexQdrantUrl: settings.codebaseIndexQdrantUrl,
@@ -1880,7 +1884,28 @@ export const webviewMessageHandler = async (
18801884

18811885
// Then handle validation and initialization
18821886
if (provider.codeIndexManager) {
1883-
await provider.codeIndexManager.handleSettingsChange()
1887+
// If embedder provider changed, perform proactive validation
1888+
if (embedderProviderChanged) {
1889+
try {
1890+
// Force handleSettingsChange which will trigger validation
1891+
await provider.codeIndexManager.handleSettingsChange()
1892+
} catch (error) {
1893+
// Validation failed - the error state is already set by handleSettingsChange
1894+
provider.log(
1895+
`Embedder validation failed after provider change: ${error instanceof Error ? error.message : String(error)}`,
1896+
)
1897+
// Send validation error to webview
1898+
await provider.postMessageToWebview({
1899+
type: "indexingStatusUpdate",
1900+
values: provider.codeIndexManager.getCurrentStatus(),
1901+
})
1902+
// Exit early - don't try to start indexing with invalid configuration
1903+
break
1904+
}
1905+
} else {
1906+
// No provider change, just handle settings normally
1907+
await provider.codeIndexManager.handleSettingsChange()
1908+
}
18841909

18851910
// Auto-start indexing if now enabled and configured
18861911
if (provider.codeIndexManager.isFeatureEnabled && provider.codeIndexManager.isFeatureConfigured) {
@@ -1898,6 +1923,11 @@ export const webviewMessageHandler = async (
18981923
provider.log(
18991924
`Code index initialization failed during settings save: ${error instanceof Error ? error.message : String(error)}`,
19001925
)
1926+
// Send error status to webview
1927+
await provider.postMessageToWebview({
1928+
type: "indexingStatusUpdate",
1929+
values: provider.codeIndexManager.getCurrentStatus(),
1930+
})
19011931
// Don't re-throw - settings are already saved successfully
19021932
}
19031933
} else {
Lines changed: 203 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,203 @@
1+
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"
2+
import { CodeIndexManager } from "../manager"
3+
import * as vscode from "vscode"
4+
import { ContextProxy } from "../../../core/config/ContextProxy"
5+
6+
// Mock dependencies
7+
vi.mock("vscode", () => ({
8+
workspace: {
9+
workspaceFolders: [{ uri: { fsPath: "/test/workspace" } }],
10+
},
11+
ExtensionContext: vi.fn(),
12+
EventEmitter: vi.fn().mockImplementation(() => ({
13+
fire: vi.fn(),
14+
event: vi.fn(),
15+
dispose: vi.fn(),
16+
})),
17+
Uri: {
18+
joinPath: vi.fn().mockImplementation((base, ...paths) => ({
19+
fsPath: [base?.fsPath || base, ...paths].join("/"),
20+
})),
21+
file: vi.fn().mockImplementation((path) => ({ fsPath: path })),
22+
},
23+
}))
24+
25+
vi.mock("../../../utils/path", () => ({
26+
getWorkspacePath: vi.fn(() => "/test/workspace"),
27+
}))
28+
29+
vi.mock("fs/promises", () => ({
30+
default: {
31+
readFile: vi.fn().mockRejectedValue(new Error("File not found")),
32+
},
33+
}))
34+
35+
// Track mock instances
36+
let mockStateManagerInstance: any
37+
let mockServiceFactoryInstance: any
38+
39+
vi.mock("../state-manager", () => ({
40+
CodeIndexStateManager: vi.fn().mockImplementation(() => {
41+
mockStateManagerInstance = {
42+
onProgressUpdate: vi.fn(),
43+
getCurrentStatus: vi.fn().mockReturnValue({ state: "Idle", error: null }),
44+
dispose: vi.fn(),
45+
setSystemState: vi.fn(),
46+
}
47+
return mockStateManagerInstance
48+
}),
49+
}))
50+
51+
vi.mock("../service-factory", () => ({
52+
CodeIndexServiceFactory: vi.fn().mockImplementation(() => {
53+
mockServiceFactoryInstance = {
54+
createServices: vi.fn().mockReturnValue({
55+
embedder: { embedderInfo: { name: "ollama" } },
56+
vectorStore: {},
57+
scanner: {},
58+
fileWatcher: {
59+
onDidStartBatchProcessing: vi.fn(),
60+
onBatchProgressUpdate: vi.fn(),
61+
watch: vi.fn(),
62+
stopWatcher: vi.fn(),
63+
dispose: vi.fn(),
64+
},
65+
}),
66+
validateEmbedder: vi.fn().mockResolvedValue({ valid: true }), // Default to valid
67+
}
68+
return mockServiceFactoryInstance
69+
}),
70+
}))
71+
72+
vi.mock("../search-service", () => ({
73+
CodeIndexSearchService: vi.fn().mockImplementation(() => ({
74+
searchIndex: vi.fn(),
75+
})),
76+
}))
77+
78+
vi.mock("../orchestrator", () => ({
79+
CodeIndexOrchestrator: vi.fn().mockImplementation(() => ({
80+
state: "Idle",
81+
startIndexing: vi.fn(),
82+
stopWatcher: vi.fn(),
83+
clearIndexData: vi.fn(),
84+
})),
85+
}))
86+
87+
describe("Proactive Embedder Validation", () => {
88+
let manager: CodeIndexManager
89+
let mockContext: vscode.ExtensionContext
90+
let mockContextProxy: ContextProxy
91+
92+
beforeEach(() => {
93+
// Clear all instances before each test
94+
;(CodeIndexManager as any).instances.clear()
95+
96+
mockContext = {
97+
subscriptions: [],
98+
extensionPath: "/test/extension",
99+
globalState: {
100+
get: vi.fn(),
101+
update: vi.fn(),
102+
},
103+
secrets: {
104+
get: vi.fn(),
105+
store: vi.fn(),
106+
},
107+
globalStorageUri: { fsPath: "/test/global-storage" },
108+
} as any
109+
110+
mockContextProxy = {
111+
getValue: vi.fn().mockImplementation((key) => {
112+
if (key === "codebaseIndexConfig") {
113+
return {
114+
codebaseIndexEnabled: true,
115+
codebaseIndexQdrantUrl: "http://localhost:6333",
116+
codebaseIndexEmbedderProvider: "ollama",
117+
codebaseIndexEmbedderModelId: "nomic-embed-text",
118+
}
119+
}
120+
return undefined
121+
}),
122+
setValue: vi.fn(),
123+
getSecret: vi.fn().mockImplementation((key) => {
124+
if (key === "codeIndexOpenAiKey") return "test-key"
125+
return undefined
126+
}),
127+
storeSecret: vi.fn(),
128+
getGlobalState: vi.fn().mockImplementation((key) => {
129+
if (key === "codebaseIndexConfig") {
130+
return {
131+
codebaseIndexEnabled: true,
132+
codebaseIndexQdrantUrl: "http://localhost:6333",
133+
codebaseIndexEmbedderProvider: "ollama",
134+
codebaseIndexEmbedderModelId: "nomic-embed-text",
135+
}
136+
}
137+
return undefined
138+
}),
139+
setGlobalState: vi.fn(),
140+
refreshSecrets: vi.fn().mockResolvedValue(undefined),
141+
} as any
142+
143+
// Create manager instance
144+
manager = CodeIndexManager.getInstance(mockContext)!
145+
})
146+
147+
afterEach(() => {
148+
vi.clearAllMocks()
149+
CodeIndexManager.disposeAll()
150+
})
151+
152+
it("should validate embedder when provider changes during handleSettingsChange", async () => {
153+
// Initialize manager first
154+
await manager.initialize(mockContextProxy)
155+
156+
// Mock the config manager to simulate a provider change that requires restart
157+
const configManager = (manager as any)._configManager
158+
vi.spyOn(configManager, "loadConfiguration").mockResolvedValue({ requiresRestart: true })
159+
vi.spyOn(configManager, "isFeatureEnabled", "get").mockReturnValue(true)
160+
vi.spyOn(configManager, "isFeatureConfigured", "get").mockReturnValue(true)
161+
162+
// Clear all previous calls to track new calls
163+
mockServiceFactoryInstance.validateEmbedder.mockClear()
164+
165+
// Call handleSettingsChange
166+
try {
167+
await manager.handleSettingsChange()
168+
} catch (error) {
169+
// We expect this to potentially throw, but we're mainly interested in whether validation was called
170+
}
171+
172+
// Verify validation was called during handleSettingsChange
173+
expect(mockServiceFactoryInstance.validateEmbedder).toHaveBeenCalled()
174+
})
175+
176+
it("should start indexing when validation succeeds after provider change", async () => {
177+
// Initialize manager first
178+
await manager.initialize(mockContextProxy)
179+
180+
// Mock the config manager to simulate a provider change that requires restart
181+
const configManager = (manager as any)._configManager
182+
vi.spyOn(configManager, "loadConfiguration").mockResolvedValue({ requiresRestart: true })
183+
vi.spyOn(configManager, "isFeatureEnabled", "get").mockReturnValue(true)
184+
vi.spyOn(configManager, "isFeatureConfigured", "get").mockReturnValue(true)
185+
186+
// Mock the service factory to simulate validation success
187+
mockServiceFactoryInstance.validateEmbedder.mockResolvedValue({
188+
valid: true,
189+
})
190+
191+
// Mock startIndexing
192+
const startIndexingSpy = vi.spyOn(manager, "startIndexing").mockResolvedValue(undefined)
193+
194+
// Call handleSettingsChange should succeed
195+
await manager.handleSettingsChange()
196+
197+
// Verify indexing was started
198+
expect(startIndexingSpy).toHaveBeenCalled()
199+
200+
// Verify no error state was set
201+
expect(mockStateManagerInstance.setSystemState).not.toHaveBeenCalledWith("Error", expect.any(String))
202+
})
203+
})

src/services/code-index/manager.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,8 @@ export class CodeIndexManager {
296296
} catch (error) {
297297
// Error state already set in _recreateServices
298298
console.error("Failed to recreate services:", error)
299+
// Re-throw the error so the caller knows validation failed
300+
throw error
299301
}
300302
}
301303
}

0 commit comments

Comments
 (0)