Problem
PR #832 added native thinking/reasoning support for Ollama. The review identified several follow-up improvements that don't block the feature but should be addressed to prevent subtle bugs and keep the codebase consistent with cross-provider patterns.
Context (who is affected and when)
Affects users of the Ollama provider with native thinking enabled (particularly with models like qwen3 that may emit reasoning via both channels), and future contributors working on the provider settings UI.
Desired behavior
- No duplicate reasoning output when a model emits via both
message.thinking and <think> tags in message.content simultaneously.
- ThinkingBudget's internal state sync effects cannot conflict with Ollama's outer "Enable Thinking" checkbox.
- Code is consistent with cross-provider patterns for guard logic, return types, and test organization.
Acceptance criteria
Given a reasoning model that emits thinking in both message.thinking and <think> content tags simultaneously
When native thinking is enabled
Then reasoning is displayed once, not twice in the UI
And disabling the checkbox fully stops the think parameter from being sent
Given the "Enable Thinking" checkbox is checked and ThinkingBudget is visible
When ThinkingBudget's internal sync effects fire on mount
Then enableReasoningEffort is not inadvertently overwritten by ThinkingBudget
Proposed approach
-
native-ollama.ts:391 — Skip the TagMatcher content pass for a chunk when message.thinking is non-empty, preventing dual-channel double-yield:
if (chunk.message.thinking?.length) {
yield { type: "reasoning", text: chunk.message.thinking }
} else if (chunk.message.content?.length) {
for (const c of matcher.update(chunk.message.content)) yield c
}
-
Ollama.tsx:165 — Proxy setApiConfigurationField passed to ThinkingBudget (match the pattern in OpenAICompatible.tsx:256-278) so ThinkingBudget's internal enableReasoningEffort sync effects cannot override the outer checkbox.
-
native-ollama.ts:292 — Add a comment explaining why "none"/"minimal" map to boolean true (Ollama-specific deviation from the cross-provider spec in ThinkingBudget.tsx:15-19).
-
native-ollama.ts:317 — Return { chatOptions, thinkParam } from buildChatRequestOptions() instead of a positional tuple, consistent with other providers.
-
native-ollama.spec.ts:657, 747 — Move 4 think-param tests into their correct describe("createMessage") / describe("completePrompt") blocks.
Trade-offs / risks
The dual-channel guard changes behavior for models that currently get both channels active simultaneously — but duplicate reasoning is always wrong output, so the change is safe. The TagMatcher fallback continues to serve models that don't use native thinking (e.g. deepseek-r1 with <think> tags).
Identified during review of #832.
Problem
PR #832 added native thinking/reasoning support for Ollama. The review identified several follow-up improvements that don't block the feature but should be addressed to prevent subtle bugs and keep the codebase consistent with cross-provider patterns.
Context (who is affected and when)
Affects users of the Ollama provider with native thinking enabled (particularly with models like qwen3 that may emit reasoning via both channels), and future contributors working on the provider settings UI.
Desired behavior
message.thinkingand<think>tags inmessage.contentsimultaneously.Acceptance criteria
Proposed approach
native-ollama.ts:391— Skip the TagMatcher content pass for a chunk whenmessage.thinkingis non-empty, preventing dual-channel double-yield:Ollama.tsx:165— ProxysetApiConfigurationFieldpassed toThinkingBudget(match the pattern inOpenAICompatible.tsx:256-278) so ThinkingBudget's internalenableReasoningEffortsync effects cannot override the outer checkbox.native-ollama.ts:292— Add a comment explaining why"none"/"minimal"map to booleantrue(Ollama-specific deviation from the cross-provider spec inThinkingBudget.tsx:15-19).native-ollama.ts:317— Return{ chatOptions, thinkParam }frombuildChatRequestOptions()instead of a positional tuple, consistent with other providers.native-ollama.spec.ts:657, 747— Move 4 think-param tests into their correctdescribe("createMessage")/describe("completePrompt")blocks.Trade-offs / risks
The dual-channel guard changes behavior for models that currently get both channels active simultaneously — but duplicate reasoning is always wrong output, so the change is safe. The TagMatcher fallback continues to serve models that don't use native thinking (e.g. deepseek-r1 with
<think>tags).Identified during review of #832.