-
Notifications
You must be signed in to change notification settings - Fork 199
fix(ollama): always respond to model refresh, surface errors, and use form-edited base URL #878
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1221,23 +1221,48 @@ export const webviewMessageHandler = async ( | |
| case "requestOllamaModels": { | ||
| // Specific handler for Ollama models only. | ||
| const { apiConfiguration: ollamaApiConfig } = await provider.getState() | ||
| // Prefer the baseUrl/apiKey from the message values (which reflect | ||
| // the user's unsaved edits in the settings form) over the saved | ||
| // state, so the refresh uses the URL the user is actually looking | ||
| // at — not the stale one from before they started editing. | ||
| const baseUrl = message.values?.baseUrl ?? ollamaApiConfig.ollamaBaseUrl | ||
| const apiKey = message.values?.apiKey ?? ollamaApiConfig.ollamaApiKey | ||
| const logBaseUrl = baseUrl || "http://localhost:11434" | ||
| const ollamaOptions = { | ||
| provider: "ollama" as const, | ||
| baseUrl, | ||
| apiKey, | ||
| } | ||
| try { | ||
| const ollamaOptions = { | ||
| provider: "ollama" as const, | ||
| baseUrl: ollamaApiConfig.ollamaBaseUrl, | ||
| apiKey: ollamaApiConfig.ollamaApiKey, | ||
| } | ||
| // Flush cache and refresh to ensure fresh models. | ||
| // Refresh the cache before reading the models. Keep this error | ||
| // separate from the read below so diagnostics identify which | ||
| // cache operation failed. | ||
| await flushModels(ollamaOptions, true) | ||
| } catch (error) { | ||
| const errorMsg = error instanceof Error ? error.message : String(error) | ||
| provider.log(`[requestOllamaModels] Failed to refresh model cache for ${logBaseUrl}: ${errorMsg}`) | ||
| provider.postMessageToWebview({ | ||
| type: "ollamaModels", | ||
| ollamaModels: {}, | ||
| error: errorMsg, | ||
| }) | ||
| break | ||
| } | ||
|
|
||
| try { | ||
| const ollamaModels = await getModels(ollamaOptions) | ||
|
|
||
| if (Object.keys(ollamaModels).length > 0) { | ||
| provider.postMessageToWebview({ type: "ollamaModels", ollamaModels: ollamaModels }) | ||
| } | ||
| // Always post a response so the webview refresh status can | ||
| // transition out of "loading" — even when no models are found. | ||
| provider.postMessageToWebview({ type: "ollamaModels", ollamaModels }) | ||
| } catch (error) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both |
||
| // Silently fail - user hasn't configured Ollama yet | ||
| console.debug("Ollama models fetch failed:", error) | ||
| const errorMsg = error instanceof Error ? error.message : String(error) | ||
| provider.log(`[requestOllamaModels] Failed to read models for ${logBaseUrl}: ${errorMsg}`) | ||
| provider.postMessageToWebview({ | ||
| type: "ollamaModels", | ||
| ollamaModels: {}, | ||
| error: errorMsg, | ||
| }) | ||
| } | ||
| break | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -219,7 +219,13 @@ const ApiOptions = ({ | |
| }, | ||
| }) | ||
| } else if (selectedProvider === "ollama") { | ||
| vscode.postMessage({ type: "requestOllamaModels" }) | ||
| vscode.postMessage({ | ||
| type: "requestOllamaModels", | ||
| values: { | ||
| baseUrl: apiConfiguration?.ollamaBaseUrl, | ||
| apiKey: apiConfiguration?.ollamaApiKey, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| }, | ||
| }) | ||
| } else if (selectedProvider === "lmstudio") { | ||
| requestLmStudioModels(apiConfiguration?.lmStudioBaseUrl) | ||
| } else if (selectedProvider === "vscode-lm") { | ||
|
|
@@ -243,6 +249,7 @@ const ApiOptions = ({ | |
| apiConfiguration?.openAiBaseUrl, | ||
| apiConfiguration?.openAiApiKey, | ||
| apiConfiguration?.ollamaBaseUrl, | ||
| apiConfiguration?.ollamaApiKey, | ||
| apiConfiguration?.lmStudioBaseUrl, | ||
| apiConfiguration?.litellmBaseUrl, | ||
| apiConfiguration?.litellmApiKey, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mockGetModelsis asserted with the right options, butmockFlushModelsisn’t checked here. IfflushModelsused the stale saved-state URL whilegetModelsused the form-edited one, this test would still pass. Worth adding: