fix(ollama): always respond to model refresh, surface errors, and use form-edited base URL#878
fix(ollama): always respond to model refresh, surface errors, and use form-edited base URL#878navedmerchant wants to merge 1 commit into
Conversation
navedmerchant
requested review from
JamesRobert20,
edelauna,
hannesrudolph and
taltas
as code owners
📝 WalkthroughWalkthroughThe Ollama refresh handler now accepts current form credentials, always returns model data, and surfaces errors. The settings UI adds a refresh button with loading, success, and error states, with tests covering backend responses and frontend interactions. ChangesOllama model refresh
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
webview-ui/src/components/settings/providers/__tests__/Ollama.spec.tsx (1)
232-355: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing coverage for the "preserve models on refresh error" scenario.
Given the model-list-wipe issue flagged in
Ollama.tsx(settingollamaModelsto{}on any error response), consider adding a test that: loads models successfully first, then triggers a refresh that fails, and asserts the previously loaded models remain inollamaModels/the picker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/components/settings/providers/__tests__/Ollama.spec.tsx` around lines 232 - 355, Add coverage in the “Ollama Component - refresh models” tests for preserving previously loaded models after a failed refresh: complete one successful refresh, trigger another refresh, dispatch an error response, and assert the existing model remains visible in the picker or otherwise unchanged. Use the existing dispatchMessage helper and refresh-button flow, and verify the error state is shown without clearing the prior model.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webview-ui/src/components/settings/providers/__tests__/Ollama.spec.tsx`:
- Around line 74-80: Update the `vi.mock("`@src/components/ui`", ...)` setup in
`Ollama.spec.tsx` to preserve the real barrel exports via `vi.importActual()`
and override only `Button`; ensure `ModelPicker` and `ThinkingBudget` continue
receiving their actual UI dependencies while the test-specific button behavior
remains.
---
Nitpick comments:
In `@webview-ui/src/components/settings/providers/__tests__/Ollama.spec.tsx`:
- Around line 232-355: Add coverage in the “Ollama Component - refresh models”
tests for preserving previously loaded models after a failed refresh: complete
one successful refresh, trigger another refresh, dispatch an error response, and
assert the existing model remains visible in the picker or otherwise unchanged.
Use the existing dispatchMessage helper and refresh-button flow, and verify the
error state is shown without clearing the prior model.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c7ef33f-2b62-4f1b-a1ac-fee40638bb54
📒 Files selected for processing (5)
src/core/webview/__tests__/webviewMessageHandler.spec.tssrc/core/webview/webviewMessageHandler.tswebview-ui/src/components/settings/ApiOptions.tsxwebview-ui/src/components/settings/providers/Ollama.tsxwebview-ui/src/components/settings/providers/__tests__/Ollama.spec.tsx
| // Stub the shared Button so we can assert onClick/disabled without its styling deps. | ||
| vi.mock("@src/components/ui", () => ({ | ||
| Button: ({ children, onClick, disabled, className }: any) => ( | ||
| <button onClick={onClick} disabled={disabled} className={className} data-testid="refresh-button"> | ||
| {children} | ||
| </button> | ||
| ), |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "from \"`@src/components/ui`\"" webview-ui/src/components/settings -A1 -g '!**/__tests__/**'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 3680
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Ollama provider\n'
ast-grep outline webview-ui/src/components/settings/providers/Ollama.tsx --view expanded || true
printf '\n## Test file outline\n'
ast-grep outline webview-ui/src/components/settings/providers/__tests__/Ollama.spec.tsx --view expanded || true
printf '\n## Ollama.tsx relevant lines\n'
nl -ba webview-ui/src/components/settings/providers/Ollama.tsx | sed -n '1,220p'
printf '\n## Ollama.spec.tsx relevant lines\n'
nl -ba webview-ui/src/components/settings/providers/__tests__/Ollama.spec.tsx | sed -n '1,420p'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 438
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Search for sibling components used by Ollama/provider test path\n'
rg -n "ModelPicker|ThinkingBudget|Checkbox|VSCodeTextField|refresh-model|refreshModels|loading" webview-ui/src/components/settings/providers webview-ui/src/components/settings -g '!**/__tests__/**' -A3 -B3Repository: Zoo-Code-Org/Zoo-Code
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Direct usage of settings subcomponents inside Ollama-related files\n'
rg -n "ModelPicker|ThinkingBudget|Checkbox|VSCodeTextField" webview-ui/src/components/settings -g '!**/__tests__/**' -A2 -B2
printf '\n## Ollama.tsx imports and JSX\n'
nl -ba webview-ui/src/components/settings/providers/Ollama.tsx | sed -n '1,220p'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba webview-ui/src/components/settings/ModelPicker.tsx | sed -n '1,260p'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## ModelPicker.tsx\n'
awk '{printf "%d:%s\n", NR, $0}' webview-ui/src/components/settings/ModelPicker.tsx | sed -n '1,260p'
printf '\n## UI barrel imports in settings subtree\n'
rg -n "from \"`@src/components/ui`\"" webview-ui/src/components/settings/ModelPicker.tsx webview-ui/src/components/settings/ThinkingBudget.tsx webview-ui/src/components/settings/providers/Ollama.tsx -A2 -B2Repository: Zoo-Code-Org/Zoo-Code
Length of output: 10832
Mock only Button here. ModelPicker and ThinkingBudget also import symbols from @src/components/ui, so this full barrel mock can turn those dependencies into undefined and make the render brittle. Use vi.importActual() and override just Button.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webview-ui/src/components/settings/providers/__tests__/Ollama.spec.tsx`
around lines 74 - 80, Update the `vi.mock("`@src/components/ui`", ...)` setup in
`Ollama.spec.tsx` to preserve the real barrel exports via `vi.importActual()`
and override only `Button`; ensure `ModelPicker` and `ThinkingBudget` continue
receiving their actual UI dependencies while the test-specific button behavior
remains.
github-actions
Bot
added
the
awaiting-review
|
@roomote Please review this PR |
|
I saw the mention, but I could not start work on this PR with the current Roomote GitHub setup. |
2 participants
Related GitHub Issue
Closes: #877
Description
This PR fixes the Ollama model refresh flow so the settings panel always gives the user feedback and uses the URL they're actually looking at. The key implementation details:
Extension handler (
webviewMessageHandler.ts)requestOllamaModelscase now prefersbaseUrl/apiKeyfrom the incoming messagevalues(which reflect the user's unsaved form edits) over the saved extension state, falling back to state only when values are absent. This lets a user validate a URL before saving it.ollamaModelsresponse back to the webview — even when no models are found — so the UI can transition out of the loading state instead of hanging forever.provider.log(...)and an emptyollamaModelsresponse is posted with the error message included, so the webview can display it directly. The previousconsole.debugsilent swallow is removed.Webview (
ApiOptions.tsx)requestOllamaModelsmessage dispatched on provider switch now includesvalues: { baseUrl, apiKey }from the currentapiConfiguration, matching the new handler contract.Ollama settings component (
Ollama.tsx)useEventhook fromreact-usewith a nativewindow.addEventListener("message", ...)inside auseEffect(with proper cleanup), removing thereact-usedependency from this component.refreshStatusstate machine (idle→loading→success|error) and arefreshErrorstate to hold the backend error message.Buttoncomponent) that shows a spinner while loading, disables itself during refresh, and displays success / error status text below the button. The error text prefers the backend-provided message and falls back to a localized generic error string.loadingstate, so unsolicitedollamaModelsmessages (e.g. from mount refresh) don't clobber the status display.Test Procedure
Unit tests — extension handler (
src/core/webview/__tests__/webviewMessageHandler.spec.ts)cd src && npx vitest run core/webview/__tests__/webviewMessageHandler.spec.tsbaseUrl/apiKeyfrom messagevaluestake precedence over saved state.Unit tests — Ollama component (
webview-ui/src/components/settings/providers/__tests__/Ollama.spec.tsx)cd webview-ui && npx vitest run src/components/settings/providers/__tests__/Ollama.spec.tsxdescribe("Ollama Component - refresh models")block verifies: idle button render, correctrequestOllamaModelspayload withvalueson click, loading/disabled state, success state on models arrival, error state on empty models, backend error message display, and no-op for unsolicited messages when not loading.Manual testing
Pre-Submission Checklist
Screenshots / Videos
N/A — the refresh button and status text use existing shared UI primitives (
Button, codicon icons, VS Code CSS variables) with no new visual design.Documentation Updates
settings:providers.refreshModels.*).Additional Notes
The
react-useuseEventimport was removed fromOllama.tsxin favor of a nativeuseEffect-based listener. This is a minor cleanup that reduces the component's dependency surface and makes the listener lifecycle explicit. The corresponding test mock forreact-usewas also removed.Get in Touch
navedmerchant (GitHub) / navedmerchant on Discord
Summary by CodeRabbit
New Features
Bug Fixes
Tests