feat(provider): card-based provider & model management with per-model reasoning - #103
Merged
Conversation
…effort
Restructure provider/model configuration around four UX goals:
1. setup no longer forces a model pick — a default is auto-selected
(DefaultEnabled → Recommended → first) for registry providers
2. provider editing works end-to-end, incl. a base_url overwrite bug fix
3. custom (OpenAI-compatible) providers are first-class (name/endpoint/model
+ optional reasoning flag), both in first-run setup and settings
4. capabilities (vision/thinking/reasoning_effort) are model-level, not
provider-level — exposed via a per-model effort control in the chat picker
Backend
- model/registry.go: PickDefaultModel(); custom models flagged reasoning get
standard effort options
- model/chatmodel.go: NewChatModelFromProvider is the single ProviderConfig →
ChatModelConfig map; custom headers / reasoning_effort / thinking / vision
flow into the request
- config/model_state.go: per-model EffortOverrides + ResolveEffort precedence
- config/config.go: relax the empty-Model boot check to a warning
- command/{web,interactive,acp,commands}.go + model/factory.go: every agent
construction path resolves per-model effort before building the model
- web/server.go: setup/complete accepts advanced fields + optional model;
providers CRUD returns name/custom; new POST /api/model-state/effort;
/api/models now emits reasoning_options per model
- model/validate.go: ValidateProvider honors custom headers
- tui/setup.go: drop the model-selection state, keep a model-id step only for
custom providers
Frontend
- SetupView.vue: provider → apikey (no model step); advanced panel (endpoint +
headers); custom provider entry; reasoning flag for custom models
- SettingsDialog.vue: add/edit provider with custom-provider entry; edit
preserves base_url on empty submit; capabilities block removed from the
provider form (model-level now)
- ChatInput.vue: per-model reasoning-effort control next to the model picker,
driven by each model's reasoning_options, remembered per model
- stores/chat.ts + composables/api.ts + types/api.ts: effort overrides load/
save; ProviderAdvanced/ProviderDetail drop the capability fields
i18n (en/ja/ko/zh-Hans/zh-Hant): add custom-provider + effort keys, remove the
now-unused provider-level capability labels
|
Warning Review limit reached
More reviews will be available in 39 minutes and 43 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThe PR adds per-model reasoning-effort overrides, custom-provider configuration, and matching setup/admin flows. It also updates runtime model creation, server APIs, chat effort selection, and removes several exported helpers and standalone modules. ChangesProvider reasoning effort and setup
Helper and module removals
Sequence Diagram(s)Setup validation and completion sequenceDiagram
participant SetupView as SetupView.vue
participant api as web/src/composables/api.ts
participant Server as internal/web/server.go
participant Validate as internal/model/validate.go
participant ProviderConfig as internal/config/config.go
SetupView->>api: setupValidate(provider, api_key, base_url, headers)
api->>Server: POST /api/setup/validate
Server->>Validate: ValidateProviderDetailed(..., headers)
Server-->>SetupView: validation response
SetupView->>api: setupComplete(custom fields, headers)
api->>Server: POST /api/setup/complete
Server->>ProviderConfig: persist provider config + custom model
Server-->>SetupView: resolvedModel
Chat effort override flow sequenceDiagram
participant ChatInput as ChatInput.vue
participant Store as web/src/stores/chat.ts
participant Api as web/src/composables/api.ts
participant Server as internal/web/server.go
participant ModelState as internal/config/model_state.go
ChatInput->>Store: setModelEffort(provider, model, effort)
Store->>Api: POST /api/model-state/effort
Api->>Server: provider, model, effort
Server->>ModelState: SetEffortOverride(provider/model, effort)
Server-->>Api: { effort }
Api-->>Store: persisted override
Store-->>ChatInput: update picker state
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
cnjack
commented
Jun 25, 2026
cnjack
left a comment
cnjack
left a comment
Owner
Author
There was a problem hiding this comment.
Staff Engineer Review — PR #103
CI is green. Full diff read. Six findings below, ranked by severity.
Finding 1 (HIGH) — handleUpdateProvider silently wipes vision, thinking, and provider-level reasoning_effort on every Settings edit
The PR says these ProviderConfig fields are kept as "backward-compatible fallbacks so existing configs still apply." The implementation contradicts that.
handleUpdateProvider unconditionally writes:
pc.Vision = req.Vision // nil when not sent → overwrites existing
pc.Thinking = req.Thinking // nil when not sent → overwrites existing
pc.ReasoningEffort = req.ReasoningEffort // "" when not sent → overwrites existingBut collectProviderAdvanced() in SettingsDialog.vue only sends base_url and headers — never vision, thinking, or reasoning_effort. The ProviderAdvanced type in api.ts also omits them. So every edit via the UI drives all three fields to zero/nil on the server, silently corrupting config values the user had set directly.
Compare to the correct keep-on-empty semantics the same handler uses for api_key and base_url. Fix:
if req.Vision != nil {
pc.Vision = req.Vision
}
if req.Thinking != nil {
pc.Thinking = req.Thinking
}
if req.ReasoningEffort != "" {
pc.ReasoningEffort = req.ReasoningEffort
}Finding 2 (HIGH) — setModelEffort error-revert in the Pinia store is a no-op
web/src/stores/chat.ts:
const next = { ...effortOverrides.value }
// ... mutate next ...
effortOverrides.value = next // optimistic update — now effortOverrides.value IS next
try {
await api.setModelEffort(provider, model, effort)
} catch {
// effortOverrides.value === next is TRUE (same reference)
// so this branch spreads the WRONG (already-failed) state back onto itself.
// The previous state was never captured and cannot be restored.
effortOverrides.value = effortOverrides.value === next
? { ...effortOverrides.value }
: effortOverrides.value
}When the API call fails, the UI keeps showing the failed effort level. Fix:
const prev = { ...effortOverrides.value } // capture before mutation
const next = { ...effortOverrides.value }
// ... mutate next ...
effortOverrides.value = next
try {
await api.setModelEffort(provider, model, effort)
} catch {
effortOverrides.value = prev // actual revert
}Finding 3 (MEDIUM) — Boot-time empty-model check downgraded from hard error to a log warning
RunInteractive, acpAgent.buildAgentSession, and runDoctorMode all take cfg.Model and pass it down to NewChatModelFromProvider without a fallback. If cfg.Model == "", those paths send an empty string as the model name to the OpenAI client, which fails at the provider with something like 400: model is a required parameter — no actionable guidance for the user.
The old hard error at LoadConfig time was correct for these paths. The warning is only safe for the web-server startup path, where handleSetupComplete / handleSetupStatus already resolve a model. Consider keeping the hard error and relaxing it only inside the web-server initialisation.
Finding 4 (MEDIUM) — ResolveEffort reads the model-state JSON from disk on every agent construction
func ResolveEffort(prov, mod, providerEffort string) string {
if state, err := LoadModelState(); err == nil && state != nil { // disk read every call
...
}
return providerEffort
}This is called in 6+ code paths: RunInteractive startup, handleConfig, handleAddModel, acpAgent.buildAgentSession, runDoctorMode, and ModelFactory.GetModel. The factory path is most exposed because GetModel runs for every subagent model request. On networked or slow storage this adds latency to every turn. Cache the loaded state in the server and invalidate it on writes to handleSetModelEffort, or pass the pre-loaded state in as a parameter.
Finding 5 (MEDIUM) — POST /api/providers does not require a model for custom providers
The endpoint validates that a custom provider has a base_url, but not a model:
if isCustom && req.BaseURL == "" {
writeJSON(w, http.StatusBadRequest, ...) // enforced
}
// No equivalent for req.Model
if isCustom && req.Model != "" {
pc.CustomModels = ... // only set if present
}A custom provider added without a model cannot be selected as an active model. The error surfaces only when the user tries to switch to it. Add a server-side check mirroring the client-side validation:
if isCustom && req.Model == "" {
writeJSON(w, http.StatusBadRequest, map[string]string{"error": "model is required for custom providers"})
return
}Finding 6 (LOW) — cleanHeaders trims key whitespace but not value whitespace
for k, v := range in {
k = strings.TrimSpace(k)
if k == "" { continue }
out[k] = v // value not trimmed
}A header value with a trailing space (common in copy-paste) produces an invalid field value per RFC 7230 §3.2.6 and can be rejected or mangled by gateway proxies. Fix: out[k] = strings.TrimSpace(v).
Summary
| # | Severity | Area |
|---|---|---|
| 1 | HIGH | handleUpdateProvider destroys backward-compat vision/thinking/effort fields on edit |
| 2 | HIGH | Store revert on API failure is a no-op; UI state diverges from server |
| 3 | MEDIUM | Empty-model boot warning instead of error; opaque runtime failure downstream |
| 4 | MEDIUM | ResolveEffort disk I/O on every agent build, including subagent factory |
| 5 | MEDIUM | API allows custom provider with no model; error deferred to use time |
| 6 | LOW | Header values not whitespace-trimmed before persistence |
Overall risk: Medium. Findings 1 and 2 are the blockers — one silently corrupts stored config on the first UI edit, the other leaves the UI showing a false effort level after a save failure.
Generated by Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/tui/setup.go (2)
234-250: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAvoid carrying a custom model ID into the next provider selection.
After entering a custom model and Escaping back to the provider list,
selectedModelstays set. A later registry provider submit then skipsPickDefaultModeland savesprovider/<stale-custom-model>.Proposed fix
if sel != nil { p := sel.(providerItem).profile m.selectedProvider = &p + m.selectedModel = "" + m.err = "" return m.advanceAfterProvider() }- modelID := m.selectedModel - if modelID == "" && m.registry != nil { + var modelID string + if m.selectedProvider.NeedURL { + modelID = strings.TrimSpace(m.selectedModel) + } else if m.registry != nil { modelID = m.registry.PickDefaultModel(pID) }Also applies to: 341-348, 439-444
🤖 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 `@internal/tui/setup.go` around lines 234 - 250, The provider/model flow is retaining a stale custom model selection in setup state handling, which causes later provider submissions to reuse the old value instead of picking a default. In the StateCustomModel and related provider-selection paths in setup.go (including the provider submit logic and the back/Esc flow), clear m.selectedModel whenever the user abandons custom model entry or switches providers, and make sure the provider submit path only preserves a custom model when it was explicitly confirmed for the currently selected provider.
298-307: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the unreachable
NeedURLEsc branch.Line 303 can never run because Line 300 already matches every
NeedURLprovider. Move the custom-provider branch before the URL fallback if that is intended, or remove the dead branch.🤖 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 `@internal/tui/setup.go` around lines 298 - 307, The Esc handling in setup.go has an unreachable branch in the switch inside the msg.String() == "esc" block, since the NeedURL condition is already consumed by the first case. Update the logic in the setup state handler so the custom-provider path in the switch on m.selectedProvider is evaluated before the URL fallback if that behavior is intended, or remove the dead NeedURL branch entirely; use the existing StateURL, StateCustomModel, and StateProvider transitions in the same code path.internal/model/factory.go (1)
44-47: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude resolved effort in the model cache key.
The cache is keyed only by
providerModel, so after a user changes the per-model effort override,GetModelcan keep returning the old chat model without the newReasoningEffort.Also applies to: 75-85
🤖 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 `@internal/model/factory.go` around lines 44 - 47, The model cache in factory.GetModel is keyed only by providerModel, so updates to per-model effort overrides can reuse a stale chat model. Update the cache key and lookup in factory.go to include the resolved ReasoningEffort (for example, whatever value is used when constructing the model) so that changes to effort produce a distinct cached entry. Make sure the same resolved effort is used consistently in both the cache read path and the model creation path where the cached model is stored.
🤖 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 `@internal/config/config.go`:
- Around line 31-34: The ReasoningEffort field comment is out of sync with the
accepted values in validReasoningEffort and standardEffortOptions, so update the
documentation on ReasoningEffort to describe the full supported contract rather
than only the narrow set currently listed. Make sure the comment near
ReasoningEffort in the config struct reflects all accepted effort values handled
by internal/web/server.go, including the additional aliases and the emitted
standard options, so readers can rely on the documented behavior.
In `@internal/config/model_state.go`:
- Around line 176-196: SetEffortOverride and ResolveEffort currently collapse an
explicit empty effort into “no override,” so a user cannot store a deliberate
unset value when provider-level ReasoningEffort exists. Update SetEffortOverride
to preserve the override entry even when effort is empty (using presence rather
than deleting on empty), and adjust ResolveEffort to distinguish “override
exists but is empty” from “no override set” when checking the ModelState map for
the given ModelRef.
In `@internal/model/chatmodel.go`:
- Around line 370-373: The vision fallback in the ProviderConfig handling always
defaults nil to true, which overrides registry metadata. Update the vision
resolution logic in the chat model helper around the ProviderConfig.Vision check
so that nil defers to the registry-derived default instead of forcing vision on;
only use the explicit config value when pc.Vision is set, and preserve the
registry’s non-vision setting for models that do not support images.
- Around line 627-638: The vision-disabled collapse in chatmodel.go can leave
ChatMessage.Content empty when UserInputMultiContent contains only images, so
update the non-vision handling in the message conversion path to add a text
fallback instead of returning an empty user message. Use the existing logic in
the msg.UserInputMultiContent loop inside the vision check to preserve text
parts, and if no text remains, populate m.Content with a non-empty fallback
before returning from the conversion method.
In `@internal/web/server.go`:
- Around line 3329-3331: handleUpdateProvider is overwriting existing Vision,
Thinking, and ReasoningEffort values with empty request fields, which wipes
previously stored settings. Update the provider merge logic in
handleUpdateProvider to preserve these fields unless they are explicitly
provided in the request, consistent with the keep-on-empty behavior used for
BaseURL, Name, APIKey, and Headers. Use the existing pc assignment block as the
place to restore or conditionally apply Vision, Thinking, and ReasoningEffort.
In `@web/src/components/SettingsDialog.vue`:
- Around line 682-701: The connection test in validateProviderConnection is
dropping saved custom headers during edits because it only uses h.value and
ignores the placeholder/stored value populated by startEditProvider. Update the
header сборку in validateProviderConnection to mirror collectProviderAdvanced
semantics by falling back to the placeholder or original stored header value
when the edited row is still blank, so edit-time tests send the existing headers
instead of omitting them.
In `@web/src/i18n/locales/ko.ts`:
- Around line 277-278: The new edit labels in the ko locale use inconsistent
terminology compared with the rest of the locale. Update the `edit` and
`editProvider` entries in the `ko` translation object to match the existing
`프로바이더` wording used by nearby keys such as `settings.providers.title` and
`customProvider`, so the terminology stays consistent across the UI.
In `@web/src/i18n/locales/zh-Hans.ts`:
- Around line 277-278: Update the zh-Hans locale entries for edit and
editProvider so they use the same provider terminology as the rest of the file.
In web/src/i18n/locales/zh-Hans.ts, change the strings in the locale object for
edit/editProvider to use 服务商 instead of 提供商, keeping the wording consistent with
symbols like settings.providers.title and customProvider.
In `@web/src/i18n/locales/zh-Hant.ts`:
- Around line 278-279: The zh-Hant locale has inconsistent terminology for
provider-related labels: the edit entries use 供應商 while other keys in this file
use 服務商. Update the translation strings for edit and editProvider in the zh-Hant
locale to use 服務商 so they match the existing wording used by
settings.providers.title and customProvider.
In `@web/src/stores/chat.ts`:
- Around line 839-854: The rollback in setModelEffort is ineffective because the
optimistic map is assigned before the API call, so the catch block only clones
the same failed state instead of restoring the previous one. Capture the prior
effortOverrides.value before building next, assign the optimistic next map, and
in the catch restore the saved previous map if api.setModelEffort fails so the
UI reflects the last persisted state.
---
Outside diff comments:
In `@internal/model/factory.go`:
- Around line 44-47: The model cache in factory.GetModel is keyed only by
providerModel, so updates to per-model effort overrides can reuse a stale chat
model. Update the cache key and lookup in factory.go to include the resolved
ReasoningEffort (for example, whatever value is used when constructing the
model) so that changes to effort produce a distinct cached entry. Make sure the
same resolved effort is used consistently in both the cache read path and the
model creation path where the cached model is stored.
In `@internal/tui/setup.go`:
- Around line 234-250: The provider/model flow is retaining a stale custom model
selection in setup state handling, which causes later provider submissions to
reuse the old value instead of picking a default. In the StateCustomModel and
related provider-selection paths in setup.go (including the provider submit
logic and the back/Esc flow), clear m.selectedModel whenever the user abandons
custom model entry or switches providers, and make sure the provider submit path
only preserves a custom model when it was explicitly confirmed for the currently
selected provider.
- Around line 298-307: The Esc handling in setup.go has an unreachable branch in
the switch inside the msg.String() == "esc" block, since the NeedURL condition
is already consumed by the first case. Update the logic in the setup state
handler so the custom-provider path in the switch on m.selectedProvider is
evaluated before the URL fallback if that behavior is intended, or remove the
dead NeedURL branch entirely; use the existing StateURL, StateCustomModel, and
StateProvider transitions in the same code path.
🪄 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
Run ID: 6ad1e191-9254-4350-961d-da56ebf7063a
📒 Files selected for processing (24)
internal/command/acp.gointernal/command/commands.gointernal/command/interactive.gointernal/command/web.gointernal/config/config.gointernal/config/model_state.gointernal/model/chatmodel.gointernal/model/factory.gointernal/model/registry.gointernal/model/validate.gointernal/tui/setup.gointernal/web/server.goscript/generate_models.goweb/src/components/ChatInput.vueweb/src/components/SettingsDialog.vueweb/src/components/SetupView.vueweb/src/composables/api.tsweb/src/i18n/locales/en.tsweb/src/i18n/locales/ja.tsweb/src/i18n/locales/ko.tsweb/src/i18n/locales/zh-Hans.tsweb/src/i18n/locales/zh-Hant.tsweb/src/stores/chat.tsweb/src/types/api.ts
Comment on lines
+31
to
+34
| // ReasoningEffort controls thinking depth via the OpenAI-compatible | ||
| // "reasoning_effort" parameter. One of "", "low", "medium", "high". | ||
| // Empty ⇒ omit the parameter. | ||
| ReasoningEffort string `json:"reasoning_effort,omitempty"` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
ReasoningEffort doc enumerates a narrower value set than the validator accepts.
This comment lists only "", "low", "medium", "high", but validReasoningEffort in internal/web/server.go (Line 3251) accepts none, minimal, xhigh, and max as well, and standardEffortOptions emits minimal/low/medium/high. Align the doc to avoid confusion about the contract.
🤖 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 `@internal/config/config.go` around lines 31 - 34, The ReasoningEffort field
comment is out of sync with the accepted values in validReasoningEffort and
standardEffortOptions, so update the documentation on ReasoningEffort to
describe the full supported contract rather than only the narrow set currently
listed. Make sure the comment near ReasoningEffort in the config struct reflects
all accepted effort values handled by internal/web/server.go, including the
additional aliases and the emitted standard options, so readers can rely on the
documented behavior.
Comment on lines
+176
to
+196
| func (s *ModelState) SetEffortOverride(ref ModelRef, effort string) { | ||
| if s.EffortOverrides == nil { | ||
| s.EffortOverrides = make(map[string]string) | ||
| } | ||
| key := effortKey(ref) | ||
| if effort == "" { | ||
| delete(s.EffortOverrides, key) | ||
| return | ||
| } | ||
| s.EffortOverrides[key] = effort | ||
| } | ||
|
|
||
| // ResolveEffort returns the effective reasoning effort for a model: the | ||
| // per-model override (from the chat picker) if set, otherwise the provider-level | ||
| // fallback from ProviderConfig. Empty ("") means "send no effort parameter". | ||
| // This is the single place that defines override precedence so every | ||
| // entrypoint (web/TUI/ACP) applies the same value. | ||
| func ResolveEffort(prov, mod, providerEffort string) string { | ||
| if state, err := LoadModelState(); err == nil && state != nil { | ||
| if v := state.GetEffortOverride(ModelRef{Provider: prov, Model: mod}); v != "" { | ||
| return v |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve explicit empty effort overrides.
Line 181 deletes the entry for effort == "", and Line 195 ignores empty values, so a user cannot choose “model default / unset” when the provider config still has ReasoningEffort; it falls back to the provider-level value instead.
Proposed fix
-func (s *ModelState) GetEffortOverride(ref ModelRef) string {
+func (s *ModelState) GetEffortOverride(ref ModelRef) (string, bool) {
if s == nil || s.EffortOverrides == nil {
- return ""
+ return "", false
}
- return s.EffortOverrides[effortKey(ref)]
+ v, ok := s.EffortOverrides[effortKey(ref)]
+ return v, ok
}
// SetEffortOverride records the user's reasoning-effort choice for a model.
// An empty effort clears the override, restoring the default behavior.
func (s *ModelState) SetEffortOverride(ref ModelRef, effort string) {
if s.EffortOverrides == nil {
s.EffortOverrides = make(map[string]string)
}
key := effortKey(ref)
- if effort == "" {
- delete(s.EffortOverrides, key)
- return
- }
s.EffortOverrides[key] = effort
}
@@
func ResolveEffort(prov, mod, providerEffort string) string {
if state, err := LoadModelState(); err == nil && state != nil {
- if v := state.GetEffortOverride(ModelRef{Provider: prov, Model: mod}); v != "" {
+ if v, ok := state.GetEffortOverride(ModelRef{Provider: prov, Model: mod}); ok {
return v
}
}
return providerEffort
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (s *ModelState) SetEffortOverride(ref ModelRef, effort string) { | |
| if s.EffortOverrides == nil { | |
| s.EffortOverrides = make(map[string]string) | |
| } | |
| key := effortKey(ref) | |
| if effort == "" { | |
| delete(s.EffortOverrides, key) | |
| return | |
| } | |
| s.EffortOverrides[key] = effort | |
| } | |
| // ResolveEffort returns the effective reasoning effort for a model: the | |
| // per-model override (from the chat picker) if set, otherwise the provider-level | |
| // fallback from ProviderConfig. Empty ("") means "send no effort parameter". | |
| // This is the single place that defines override precedence so every | |
| // entrypoint (web/TUI/ACP) applies the same value. | |
| func ResolveEffort(prov, mod, providerEffort string) string { | |
| if state, err := LoadModelState(); err == nil && state != nil { | |
| if v := state.GetEffortOverride(ModelRef{Provider: prov, Model: mod}); v != "" { | |
| return v | |
| func (s *ModelState) GetEffortOverride(ref ModelRef) (string, bool) { | |
| if s == nil || s.EffortOverrides == nil { | |
| return "", false | |
| } | |
| v, ok := s.EffortOverrides[effortKey(ref)] | |
| return v, ok | |
| } | |
| func (s *ModelState) SetEffortOverride(ref ModelRef, effort string) { | |
| if s.EffortOverrides == nil { | |
| s.EffortOverrides = make(map[string]string) | |
| } | |
| key := effortKey(ref) | |
| s.EffortOverrides[key] = effort | |
| } | |
| // ResolveEffort returns the effective reasoning effort for a model: the | |
| // per-model override (from the chat picker) if set, otherwise the provider-level | |
| // fallback from ProviderConfig. Empty ("") means "send no effort parameter". | |
| // This is the single place that defines override precedence so every | |
| // entrypoint (web/TUI/ACP) applies the same value. | |
| func ResolveEffort(prov, mod, providerEffort string) string { | |
| if state, err := LoadModelState(); err == nil && state != nil { | |
| if v, ok := state.GetEffortOverride(ModelRef{Provider: prov, Model: mod}); ok { | |
| return v | |
| } | |
| } | |
| return providerEffort | |
| } |
🤖 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 `@internal/config/model_state.go` around lines 176 - 196, SetEffortOverride and
ResolveEffort currently collapse an explicit empty effort into “no override,” so
a user cannot store a deliberate unset value when provider-level ReasoningEffort
exists. Update SetEffortOverride to preserve the override entry even when effort
is empty (using presence rather than deleting on empty), and adjust
ResolveEffort to distinguish “override exists but is empty” from “no override
set” when checking the ModelState map for the given ModelRef.
Comment on lines
+370
to
+373
| vision := true | ||
| if pc.Vision != nil { | ||
| vision = *pc.Vision | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Honor registry-derived vision defaults.
ProviderConfig.Vision == nil is meant to defer to registry metadata, but this helper always treats nil as true. Non-vision registry models will still receive image parts unless users explicitly disable vision in config.
🤖 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 `@internal/model/chatmodel.go` around lines 370 - 373, The vision fallback in
the ProviderConfig handling always defaults nil to true, which overrides
registry metadata. Update the vision resolution logic in the chat model helper
around the ProviderConfig.Vision check so that nil defers to the
registry-derived default instead of forcing vision on; only use the explicit
config value when pc.Vision is set, and preserve the registry’s non-vision
setting for models that do not support images.
Comment on lines
+627
to
+638
| // Vision disabled: collapse to text-only so a non-vision endpoint | ||
| // doesn't 400 on image parts. Text segments are preserved. | ||
| if !vision { | ||
| var text string | ||
| for _, p := range msg.UserInputMultiContent { | ||
| if p.Type == schema.ChatMessagePartTypeText { | ||
| text += p.Text | ||
| } | ||
| } | ||
| m.Content = text | ||
| return m | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid sending empty messages after stripping images.
For image-only multimodal input with vision == false, text remains empty and the request can be rejected as an empty user message. Add a text fallback when all image parts are removed.
Proposed fix
if !vision {
var text string
for _, p := range msg.UserInputMultiContent {
if p.Type == schema.ChatMessagePartTypeText {
+ if text != "" && p.Text != "" {
+ text += "\n"
+ }
text += p.Text
}
}
+ if text == "" {
+ text = "[image omitted: selected model does not support vision]"
+ }
m.Content = text
return m
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Vision disabled: collapse to text-only so a non-vision endpoint | |
| // doesn't 400 on image parts. Text segments are preserved. | |
| if !vision { | |
| var text string | |
| for _, p := range msg.UserInputMultiContent { | |
| if p.Type == schema.ChatMessagePartTypeText { | |
| text += p.Text | |
| } | |
| } | |
| m.Content = text | |
| return m | |
| } | |
| // Vision disabled: collapse to text-only so a non-vision endpoint | |
| // doesn't 400 on image parts. Text segments are preserved. | |
| if !vision { | |
| var text string | |
| for _, p := range msg.UserInputMultiContent { | |
| if p.Type == schema.ChatMessagePartTypeText { | |
| if text != "" && p.Text != "" { | |
| text += "\n" | |
| } | |
| text += p.Text | |
| } | |
| } | |
| if text == "" { | |
| text = "[image omitted: selected model does not support vision]" | |
| } | |
| m.Content = text | |
| return m | |
| } |
🤖 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 `@internal/model/chatmodel.go` around lines 627 - 638, The vision-disabled
collapse in chatmodel.go can leave ChatMessage.Content empty when
UserInputMultiContent contains only images, so update the non-vision handling in
the message conversion path to add a text fallback instead of returning an empty
user message. Use the existing logic in the msg.UserInputMultiContent loop
inside the vision check to preserve text parts, and if no text remains, populate
m.Content with a non-empty fallback before returning from the conversion method.
Comment on lines
+3329
to
+3331
| pc.Vision = req.Vision | ||
| pc.Thinking = req.Thinking | ||
| pc.ReasoningEffort = req.ReasoningEffort |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
handleUpdateProvider unconditionally clears Vision/Thinking/ReasoningEffort.
Unlike BaseURL, Name, APIKey, and Headers — which all use keep-on-empty/merge semantics — these three are assigned directly from the request. Since the PR removes these capability controls from the provider UI, the settings dialog will omit them, so every provider edit silently resets a previously stored vision/thinking/reasoning_effort (e.g. one set via the config file) back to nil/empty.
If the intent is that these are now model-level only and should never be touched here, preserve them instead of overwriting:
🔧 Preserve unless explicitly provided
- pc.Vision = req.Vision
- pc.Thinking = req.Thinking
- pc.ReasoningEffort = req.ReasoningEffort
+ if req.Vision != nil {
+ pc.Vision = req.Vision
+ }
+ if req.Thinking != nil {
+ pc.Thinking = req.Thinking
+ }
+ if req.ReasoningEffort != "" {
+ pc.ReasoningEffort = req.ReasoningEffort
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pc.Vision = req.Vision | |
| pc.Thinking = req.Thinking | |
| pc.ReasoningEffort = req.ReasoningEffort | |
| if req.Vision != nil { | |
| pc.Vision = req.Vision | |
| } | |
| if req.Thinking != nil { | |
| pc.Thinking = req.Thinking | |
| } | |
| if req.ReasoningEffort != "" { | |
| pc.ReasoningEffort = req.ReasoningEffort | |
| } |
🤖 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 `@internal/web/server.go` around lines 3329 - 3331, handleUpdateProvider is
overwriting existing Vision, Thinking, and ReasoningEffort values with empty
request fields, which wipes previously stored settings. Update the provider
merge logic in handleUpdateProvider to preserve these fields unless they are
explicitly provided in the request, consistent with the keep-on-empty behavior
used for BaseURL, Name, APIKey, and Headers. Use the existing pc assignment
block as the place to restore or conditionally apply Vision, Thinking, and
ReasoningEffort.
Comment on lines
+839
to
+854
| async function setModelEffort(provider: string, model: string, effort: string) { | ||
| const key = `${provider}/${model}` | ||
| const next = { ...effortOverrides.value } | ||
| if (effort) { | ||
| next[key] = effort | ||
| } else { | ||
| delete next[key] | ||
| } | ||
| effortOverrides.value = next | ||
| try { | ||
| await api.setModelEffort(provider, model, effort) | ||
| } catch { | ||
| // Revert on failure so the UI doesn't lie about what was applied. | ||
| effortOverrides.value = effortOverrides.value === next ? { ...effortOverrides.value } : effortOverrides.value | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Revert-on-failure is a no-op — the optimistic value is never rolled back.
next is the optimistic map already assigned to effortOverrides.value. In the catch, effortOverrides.value === next is true, so it just shallow-clones the optimistic content back into place — the previous override is never restored. The UI keeps showing the effort that failed to persist, contradicting the comment.
Capture the prior map before mutating and restore it on failure.
🐛 Proposed fix
async function setModelEffort(provider: string, model: string, effort: string) {
const key = `${provider}/${model}`
- const next = { ...effortOverrides.value }
+ const prev = effortOverrides.value
+ const next = { ...prev }
if (effort) {
next[key] = effort
} else {
delete next[key]
}
effortOverrides.value = next
try {
await api.setModelEffort(provider, model, effort)
} catch {
// Revert on failure so the UI doesn't lie about what was applied.
- effortOverrides.value = effortOverrides.value === next ? { ...effortOverrides.value } : effortOverrides.value
+ if (effortOverrides.value === next) effortOverrides.value = prev
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function setModelEffort(provider: string, model: string, effort: string) { | |
| const key = `${provider}/${model}` | |
| const next = { ...effortOverrides.value } | |
| if (effort) { | |
| next[key] = effort | |
| } else { | |
| delete next[key] | |
| } | |
| effortOverrides.value = next | |
| try { | |
| await api.setModelEffort(provider, model, effort) | |
| } catch { | |
| // Revert on failure so the UI doesn't lie about what was applied. | |
| effortOverrides.value = effortOverrides.value === next ? { ...effortOverrides.value } : effortOverrides.value | |
| } | |
| } | |
| async function setModelEffort(provider: string, model: string, effort: string) { | |
| const key = `${provider}/${model}` | |
| const prev = effortOverrides.value | |
| const next = { ...prev } | |
| if (effort) { | |
| next[key] = effort | |
| } else { | |
| delete next[key] | |
| } | |
| effortOverrides.value = next | |
| try { | |
| await api.setModelEffort(provider, model, effort) | |
| } catch { | |
| // Revert on failure so the UI doesn't lie about what was applied. | |
| if (effortOverrides.value === next) effortOverrides.value = prev | |
| } | |
| } |
🤖 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 `@web/src/stores/chat.ts` around lines 839 - 854, The rollback in
setModelEffort is ineffective because the optimistic map is assigned before the
API call, so the catch block only clones the same failed state instead of
restoring the previous one. Capture the prior effortOverrides.value before
building next, assign the optimistic next map, and in the catch restore the
saved previous map if api.setModelEffort fails so the UI reflects the last
persisted state.
Provider & model config (review follow-ups): - Custom model management: edit a provider's custom models end-to-end (add/remove/rename/reasoning). PUT /api/providers replaces custom_models with keep-on-nil semantics, preserves each model's stored context, and refuses to drop the model currently in use. - Custom providers now render the OpenAI icon. Fixed custom-provider detection: MergeConfigProviders flags config-only providers on the registry, surfaced via /api/models + /api/providers (the old !HasProvider check was always false on the merged registry). - Require a model when adding a custom provider; trim header values in cleanHeaders so a pasted token's trailing space can't break auth. - i18n: unify provider terminology (服务商 / 服務商 / 프로바이더) and add custom-model keys across en/ja/ko/zh-Hans/zh-Hant. Repo-wide cleanup (unreferenced code & deps; build stays green): - Remove the internal/prompts package, agent/recovery.go, runner/eventbus.go, web FileViewer.vue, and dead paths across tools/mcp_manager, session, channel, tui, and several web composables. - Drop unused deps: @headlessui/tailwindcss, @types/dompurify, vue-router. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@web/src/utils/providerIcons.ts`:
- Around line 52-60: The provider icon helper still returns raw SVG strings,
which should be replaced with imported icon components instead of bundled
markup. Update iconForProvider to return a component token or equivalent in
providerIcons, then adjust the Vue rendering path to use the returned Heroicons
component directly rather than v-html or inline SVG strings. Make sure custom
provider fallbackOpenai also maps to the imported OpenAI icon component from
`@heroicons/vue/24/outline`.
🪄 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
Run ID: e8d2ebd9-8f4f-4692-be78-2548998aa456
⛔ Files ignored due to path filters (1)
web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (44)
internal/agent/agent.gointernal/agent/recovery.gointernal/automation/scheduler.gointernal/channel/messages.gointernal/handler/web.gointernal/model/chatmodel.gointernal/model/registry.gointernal/prompts/async_env.gointernal/prompts/builder.gointernal/prompts/cache.gointernal/prompts/compact.gointernal/prompts/compact_prompt.mdinternal/remote/ssh.gointernal/remote/ssh_test.gointernal/runner/eventbus.gointernal/session/history.gointernal/session/session.gointernal/team/context.gointernal/tools/mcp_manager.gointernal/tools/mcp_manager_test.gointernal/tools/storage.gointernal/tui/messages.gointernal/tui/setup.gointernal/tui/team_view.gointernal/tui/tui.gointernal/web/server.goweb/package.jsonweb/src/components/ChatInput.vueweb/src/components/FileViewer.vueweb/src/components/ProviderIcon.vueweb/src/components/SettingsDialog.vueweb/src/composables/api.tsweb/src/composables/apiBase.tsweb/src/composables/toolInfo.tsweb/src/composables/useDesktop.tsweb/src/composables/useTheme.tsweb/src/i18n/index.tsweb/src/i18n/locales/en.tsweb/src/i18n/locales/ja.tsweb/src/i18n/locales/ko.tsweb/src/i18n/locales/zh-Hans.tsweb/src/i18n/locales/zh-Hant.tsweb/src/types/api.tsweb/src/utils/providerIcons.ts
💤 Files with no reviewable changes (27)
- internal/prompts/compact_prompt.md
- internal/prompts/cache.go
- internal/prompts/async_env.go
- internal/team/context.go
- web/src/components/FileViewer.vue
- internal/handler/web.go
- internal/tools/storage.go
- internal/tui/team_view.go
- internal/tui/tui.go
- internal/remote/ssh.go
- internal/agent/recovery.go
- web/src/composables/useTheme.ts
- web/src/composables/useDesktop.ts
- internal/tui/messages.go
- internal/tools/mcp_manager_test.go
- internal/runner/eventbus.go
- internal/prompts/compact.go
- web/src/composables/toolInfo.ts
- internal/prompts/builder.go
- web/src/composables/apiBase.ts
- internal/automation/scheduler.go
- internal/session/session.go
- internal/remote/ssh_test.go
- internal/model/chatmodel.go
- internal/tools/mcp_manager.go
- internal/tui/setup.go
- internal/channel/messages.go
✅ Files skipped from review due to trivial changes (3)
- web/src/i18n/locales/zh-Hans.ts
- web/src/i18n/locales/ko.ts
- web/src/i18n/locales/en.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- web/src/composables/api.ts
- web/src/components/SettingsDialog.vue
- web/src/components/ChatInput.vue
- internal/model/registry.go
- web/src/i18n/locales/ja.ts
- web/src/types/api.ts
- web/src/i18n/locales/zh-Hant.ts
- internal/web/server.go
Comment on lines
+52
to
+60
| // known. Callers render a monogram fallback in that case — except for custom | ||
| // (OpenAI-compatible) providers, where passing fallbackOpenai shows the OpenAI | ||
| // mark since that's the wire protocol they speak. | ||
| export function iconForProvider(id: string, fallbackOpenai = false): string | null { | ||
| const key = (id || '').toLowerCase() | ||
| for (const [re, svg] of RULES) { | ||
| if (re.test(key)) return svg | ||
| } | ||
| return null | ||
| return fallbackOpenai ? openai : null |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Replace the SVG-string fallback path with imported icon components.
This change extends the raw SVG/v-html icon flow to custom providers. Please switch this helper to return a component token (or similar) and render imported @heroicons/vue/24/outline icons directly in the Vue layer instead of bundled markup strings. As per coding guidelines, web/**/*.vue: use @heroicons/vue/24/outline exclusively for icons, and do not hand-write inline <svg> icons or v-html SVG path strings.
🤖 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 `@web/src/utils/providerIcons.ts` around lines 52 - 60, The provider icon
helper still returns raw SVG strings, which should be replaced with imported
icon components instead of bundled markup. Update iconForProvider to return a
component token or equivalent in providerIcons, then adjust the Vue rendering
path to use the returned Heroicons component directly rather than v-html or
inline SVG strings. Make sure custom provider fallbackOpenai also maps to the
imported OpenAI icon component from `@heroicons/vue/24/outline`.
Source: Coding guidelines
Replaces the flat provider list with a card-based UI and adds a dedicated
model-authoring flow. Models (built-in + custom) now render uniformly inside a
provider's catalog; custom models are editable/removable, built-in ones toggle
show/hide.
Web UI
- Provider list → cards (brand · name · url · actions + models catalog)
- Edit provider / add custom model open in separate modal windows
(ProviderEditDialog, ModelEditDialog) instead of dense inline expansion
- Custom-model editor: ID / display name / context window / image toggle /
reasoning-effort tier editor (add/remove tiers)
- Test connection returns stateful results (latency + model count on success;
auth/network/server classified on failure)
- Delete guard: if the provider hosts the active model, guide a model switch
instead of erroring
Backend
- handleListModels rebuilds the registry from live config so newly-added
models are selectable immediately; handleUpdateProvider live-publishes
s.cfg/s.registry after save (previously stale until restart)
- New GET /api/providers/{id}/models catalog endpoint: built-in (models.dev)
models for registry providers, brand-fuzzy match for custom endpoints,
live /models probe as last resort; each entry flagged added + custom
- CustomModelConfig gains Attachment + EffortTiers fields (omitempty); registry
merge reads them (TUI inherits via the shared layer)
- Duplicate-id guard: rejects custom models colliding with built-in models
- ValidateProvider returns latency/model_count/error_type; ListProviderModelsLive
Types / i18n
- CustomModelDetail gains custom/context/attachment/effort_tiers; new
ValidateResult, CatalogModel; api.client + providerCatalog()
- zh/en/ja/ko/zh-Hant strings for the new UI
Chore
- Remove stale design/*.pen and redesign .html mockups; add
design/provider-redesign.html (interactive mock)
- gofmt alignment in script/generate_models.go
cnjack
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@internal/config/config.go`:
- Around line 50-53: The Attachment field in the config model cannot distinguish
unset from an explicit false, so model-level image support cannot be persisted
reliably. Update the model config type in config.go to use a tri-state
representation such as *bool, then adjust any serialization/defaulting logic
around Attachment so the model can explicitly inherit, opt in, or opt out while
keeping the registry/provider fallback behavior intact.
In `@internal/model/validate.go`:
- Around line 15-21: The success flag in ValidateResult is serialized as ok, but
ProviderEditDialog.vue expects testResult.valid, so successful validation
responses won’t be recognized. Update the contract by renaming the JSON field in
ValidateResult back to valid, or make the matching frontend/type change at the
same time so both sides use the same symbol consistently.
In `@web/src/components/ModelEditDialog.vue`:
- Around line 99-103: The empty-ID validation in save() is using the
provider-specific message key instead of the model-specific one, so the dialog
shows the wrong error text. Update the error assignment in ModelEditDialog.vue
within save() to use the model required-ID translation key (customModelRequired)
rather than settings.providers.customIdRequired, keeping the rest of the
draft.id trim and empty check unchanged.
In `@web/src/components/SettingsDialog.vue`:
- Around line 145-151: The catalog search state is shared across all provider
cards through catalogSearch, so typing in one card updates every catalog. Update
SettingsDialog.vue to store search text per provider id, matching catalogOpen,
catalogLoading, and catalogModels, and then bind each card’s search input and
filtering logic to its own provider-specific search state instead of the single
ref. Make sure the related catalog expansion/rendering code uses the same
provider key consistently so each card keeps independent search behavior.
- Around line 639-643: The save handler in onProviderSaved currently reloads
configuredProviders and fetches models, but it does not refresh the per-provider
catalog state used by the cards. Update onProviderSaved in SettingsDialog.vue to
rehydrate the saved provider’s catalog immediately after api.listProviders() (or
clear and repopulate all provider catalogs) so newly added or edited providers
are usable without reopening the dialog. Use the existing store.fetchModels flow
as needed, but ensure the catalog data tied to the provider card is refreshed in
the same save path.
🪄 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
Run ID: 5dc35d8a-9367-4c36-91ed-f832aa635330
📒 Files selected for processing (23)
design/automations-redesign.htmldesign/jcode-web.pendesign/jcode.pendesign/landing-page.pendesign/model-and-approval-redesign.htmldesign/nav-actions-redesign.htmldesign/provider-redesign.htmldesign/sidebar-redesign.htmlinternal/config/config.gointernal/model/registry.gointernal/model/validate.gointernal/web/server.goscript/generate_models.goweb/src/components/ModelEditDialog.vueweb/src/components/ProviderEditDialog.vueweb/src/components/SettingsDialog.vueweb/src/composables/api.tsweb/src/i18n/locales/en.tsweb/src/i18n/locales/ja.tsweb/src/i18n/locales/ko.tsweb/src/i18n/locales/zh-Hans.tsweb/src/i18n/locales/zh-Hant.tsweb/src/types/api.ts
💤 Files with no reviewable changes (6)
- design/model-and-approval-redesign.html
- design/sidebar-redesign.html
- design/jcode.pen
- design/automations-redesign.html
- design/landing-page.pen
- design/nav-actions-redesign.html
🚧 Files skipped from review as they are similar to previous changes (4)
- web/src/composables/api.ts
- script/generate_models.go
- internal/model/registry.go
- internal/web/server.go
Comment on lines
+50
to
+53
| // Attachment marks the model as accepting image inputs. When false (the | ||
| // default) the model inherits the provider-level Vision override (if set) or | ||
| // the registry default (allow images). | ||
| Attachment bool `json:"attachment,omitempty"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Attachment can't represent inherit vs. explicit false.
With bool + omitempty, both “unset” and false serialize the same way. The new model UI exposes this as a binary “Supports images” switch, so a text-only custom model on an image-capable provider cannot persist “no image input” at all. This needs a tri-state representation (*bool or similar) before model-level image capability works reliably.
🤖 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 `@internal/config/config.go` around lines 50 - 53, The Attachment field in the
config model cannot distinguish unset from an explicit false, so model-level
image support cannot be persisted reliably. Update the model config type in
config.go to use a tri-state representation such as *bool, then adjust any
serialization/defaulting logic around Attachment so the model can explicitly
inherit, opt in, or opt out while keeping the registry/provider fallback
behavior intact.
Comment on lines
+15
to
+21
| type ValidateResult struct { | ||
| OK bool `json:"ok"` | ||
| LatencyMS int `json:"latency_ms"` | ||
| ModelCount int `json:"model_count"` | ||
| ErrorType string `json:"error_type,omitempty"` // "" | "auth" | "network" | "server" | ||
| Error string `json:"error,omitempty"` | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the validation success flag aligned with the web contract.
ValidateResult serializes success as ok, but ProviderEditDialog.vue reads testResult.valid when rendering the banner. Successful responses will therefore deserialize without a truthy success flag and fall through the failure UI. Rename the JSON field back to valid or update the frontend/type contract in the same change.
🤖 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 `@internal/model/validate.go` around lines 15 - 21, The success flag in
ValidateResult is serialized as ok, but ProviderEditDialog.vue expects
testResult.valid, so successful validation responses won’t be recognized. Update
the contract by renaming the JSON field in ValidateResult back to valid, or make
the matching frontend/type change at the same time so both sides use the same
symbol consistently.
Comment on lines
+99
to
+103
| function save() { | ||
| const id = draft.value.id.trim() | ||
| if (!id) { | ||
| error.value = t('settings.providers.customIdRequired') | ||
| return |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the model-specific required-ID message here.
The empty-ID path currently uses settings.providers.customIdRequired, so this dialog tells the user that a provider ID is missing. Wire this to the model key instead (customModelRequired) so the validation error matches the form.
🤖 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 `@web/src/components/ModelEditDialog.vue` around lines 99 - 103, The empty-ID
validation in save() is using the provider-specific message key instead of the
model-specific one, so the dialog shows the wrong error text. Update the error
assignment in ModelEditDialog.vue within save() to use the model required-ID
translation key (customModelRequired) rather than
settings.providers.customIdRequired, keeping the rest of the draft.id trim and
empty check unchanged.
Comment on lines
+145
to
+151
| // Catalog (browse directory) state, keyed by provider id so each card tracks its | ||
| // own open/search/refresh state independently. The edit/add flow lives in | ||
| // ProviderEditDialog; this tab only owns the inline catalog expansion. | ||
| const catalogOpen = ref<string>('') // provider id whose catalog is expanded, '' = none | ||
| const catalogLoading = ref<string>('') // provider id currently fetching | ||
| const catalogModels = ref<Record<string, CatalogModel[]>>({}) | ||
| const catalogSearch = ref<string>('') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Catalog search state needs to be keyed by provider.
All provider cards bind their search input to the single catalogSearch ref, so typing in one card mirrors into every other card and filters every catalog at once. Store the search text per provider id to match the rest of the catalog state.
Also applies to: 780-785, 1209-1213
🤖 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 `@web/src/components/SettingsDialog.vue` around lines 145 - 151, The catalog
search state is shared across all provider cards through catalogSearch, so
typing in one card updates every catalog. Update SettingsDialog.vue to store
search text per provider id, matching catalogOpen, catalogLoading, and
catalogModels, and then bind each card’s search input and filtering logic to its
own provider-specific search state instead of the single ref. Make sure the
related catalog expansion/rendering code uses the same provider key consistently
so each card keeps independent search behavior.
Comment on lines
+639
to
+643
| // After the dialog saves, refresh the provider list + chat models, then close. | ||
| async function onProviderSaved() { | ||
| configuredProviders.value = await api.listProviders() | ||
| store.fetchModels() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Refresh the provider catalog after a save.
onProviderSaved() reloads configuredProviders, but the model catalog is only fetched in the outer open watcher. A newly added or edited provider therefore renders with an empty catalog until Settings is closed and reopened. Rehydrate the saved provider's catalog here (or clear and repopulate all catalogs) so the card is immediately usable.
🤖 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 `@web/src/components/SettingsDialog.vue` around lines 639 - 643, The save
handler in onProviderSaved currently reloads configuredProviders and fetches
models, but it does not refresh the per-provider catalog state used by the
cards. Update onProviderSaved in SettingsDialog.vue to rehydrate the saved
provider’s catalog immediately after api.listProviders() (or clear and
repopulate all provider catalogs) so newly added or edited providers are usable
without reopening the dialog. Use the existing store.fetchModels flow as needed,
but ensure the catalog data tied to the provider card is refreshed in the same
save path.
…f limit) The golangci-lint action's only-new-issues flag fetches the PR diff from GitHub's API, which rejects diffs over 20k lines (this repo's generated registry + frontend churn exceed that, causing a 406 and failing the job before lint even runs). Switch to --new-from-rev=origin/main, which diffs against the local full checkout (fetch-depth: 0) with no line limit.
This was referenced Jul 4, 2026
cnjack
added a commit
that referenced
this pull request
Jul 12, 2026
… reasoning (#103) * feat(provider): redesign model provider config & per-model reasoning effort Restructure provider/model configuration around four UX goals: 1. setup no longer forces a model pick — a default is auto-selected (DefaultEnabled → Recommended → first) for registry providers 2. provider editing works end-to-end, incl. a base_url overwrite bug fix 3. custom (OpenAI-compatible) providers are first-class (name/endpoint/model + optional reasoning flag), both in first-run setup and settings 4. capabilities (vision/thinking/reasoning_effort) are model-level, not provider-level — exposed via a per-model effort control in the chat picker Backend - model/registry.go: PickDefaultModel(); custom models flagged reasoning get standard effort options - model/chatmodel.go: NewChatModelFromProvider is the single ProviderConfig → ChatModelConfig map; custom headers / reasoning_effort / thinking / vision flow into the request - config/model_state.go: per-model EffortOverrides + ResolveEffort precedence - config/config.go: relax the empty-Model boot check to a warning - command/{web,interactive,acp,commands}.go + model/factory.go: every agent construction path resolves per-model effort before building the model - web/server.go: setup/complete accepts advanced fields + optional model; providers CRUD returns name/custom; new POST /api/model-state/effort; /api/models now emits reasoning_options per model - model/validate.go: ValidateProvider honors custom headers - tui/setup.go: drop the model-selection state, keep a model-id step only for custom providers Frontend - SetupView.vue: provider → apikey (no model step); advanced panel (endpoint + headers); custom provider entry; reasoning flag for custom models - SettingsDialog.vue: add/edit provider with custom-provider entry; edit preserves base_url on empty submit; capabilities block removed from the provider form (model-level now) - ChatInput.vue: per-model reasoning-effort control next to the model picker, driven by each model's reasoning_options, remembered per model - stores/chat.ts + composables/api.ts + types/api.ts: effort overrides load/ save; ProviderAdvanced/ProviderDetail drop the capability fields i18n (en/ja/ko/zh-Hans/zh-Hant): add custom-provider + effort keys, remove the now-unused provider-level capability labels * chore: provider/model config follow-ups + repo-wide dead-code cleanup Provider & model config (review follow-ups): - Custom model management: edit a provider's custom models end-to-end (add/remove/rename/reasoning). PUT /api/providers replaces custom_models with keep-on-nil semantics, preserves each model's stored context, and refuses to drop the model currently in use. - Custom providers now render the OpenAI icon. Fixed custom-provider detection: MergeConfigProviders flags config-only providers on the registry, surfaced via /api/models + /api/providers (the old !HasProvider check was always false on the merged registry). - Require a model when adding a custom provider; trim header values in cleanHeaders so a pasted token's trailing space can't break auth. - i18n: unify provider terminology (服务商 / 服務商 / 프로바이더) and add custom-model keys across en/ja/ko/zh-Hans/zh-Hant. Repo-wide cleanup (unreferenced code & deps; build stays green): - Remove the internal/prompts package, agent/recovery.go, runner/eventbus.go, web FileViewer.vue, and dead paths across tools/mcp_manager, session, channel, tui, and several web composables. - Drop unused deps: @headlessui/tailwindcss, @types/dompurify, vue-router. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(web): card-based provider & model management redesign Replaces the flat provider list with a card-based UI and adds a dedicated model-authoring flow. Models (built-in + custom) now render uniformly inside a provider's catalog; custom models are editable/removable, built-in ones toggle show/hide. Web UI - Provider list → cards (brand · name · url · actions + models catalog) - Edit provider / add custom model open in separate modal windows (ProviderEditDialog, ModelEditDialog) instead of dense inline expansion - Custom-model editor: ID / display name / context window / image toggle / reasoning-effort tier editor (add/remove tiers) - Test connection returns stateful results (latency + model count on success; auth/network/server classified on failure) - Delete guard: if the provider hosts the active model, guide a model switch instead of erroring Backend - handleListModels rebuilds the registry from live config so newly-added models are selectable immediately; handleUpdateProvider live-publishes s.cfg/s.registry after save (previously stale until restart) - New GET /api/providers/{id}/models catalog endpoint: built-in (models.dev) models for registry providers, brand-fuzzy match for custom endpoints, live /models probe as last resort; each entry flagged added + custom - CustomModelConfig gains Attachment + EffortTiers fields (omitempty); registry merge reads them (TUI inherits via the shared layer) - Duplicate-id guard: rejects custom models colliding with built-in models - ValidateProvider returns latency/model_count/error_type; ListProviderModelsLive Types / i18n - CustomModelDetail gains custom/context/attachment/effort_tiers; new ValidateResult, CatalogModel; api.client + providerCatalog() - zh/en/ja/ko/zh-Hant strings for the new UI Chore - Remove stale design/*.pen and redesign .html mockups; add design/provider-redesign.html (interactive mock) - gofmt alignment in script/generate_models.go * ci: use local --new-from-rev instead of only-new-issues (20k-line diff limit) The golangci-lint action's only-new-issues flag fetches the PR diff from GitHub's API, which rejects diffs over 20k lines (this repo's generated registry + frontend churn exceed that, causing a 406 and failing the job before lint even runs). Switch to --new-from-rev=origin/main, which diffs against the local full checkout (fetch-depth: 0) with no line limit. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
4 tasks
This was referenced Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Redesigns the web Settings → Providers tab into a card-based provider/model hub, and makes model capabilities first-class. Supersedes the earlier setup-wizard / per-model-effort work in this branch (kept as earlier commits).
What's new (this iteration)
Card-based provider UI
Dedicated dialogs (not inline expansion)
Stateful test connection
Catalog endpoint + live registry
Custom-model authoring
Earlier commits in this branch (still included)
Backend
Frontend
Test plan
Notes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation