feat: multimodal-safe model switch, persisted model, composer drafts, desktop links - #145
Merged
Merged
Conversation
… desktop links Four web/desktop UX fixes: 1. Strip image parts for text-only models. NewChatModelFromProvider now derives the vision flag from registry modalities (explicit pc.Vision still wins; unknown/custom models keep the permissive default), so switching from a multimodal to a text-only model no longer 400s with "messages.content.type 参数非法". Adds RegistryModel.SupportsImageInput and dedups the three modality loops (web health/models, acp caps). 2. Friendlier run errors in web. SummarizeRunError unwraps eino NodeRunError / go-openai API errors into a one-line summary plus the raw detail; WebDoneData carries both and the timeline renders the summary with a collapsible details block (and an image-support hint on 400 content.type rejections). 3. Persist web/desktop model switches. handleSwitchModel now writes cfg.Model + SaveConfig (cfgMu discipline, rollback on failure), so a restart resumes the selected model — matching the TUI picker. 4. Remember composer drafts per conversation. drafts.ts persists unsent input keyed by session id in localStorage; ChatInput restores on conversation switch/remount and clears on send. 5. Open external links in the system browser on desktop. A delegated capture-phase click interceptor routes external http(s) anchors via tauri-plugin-opener (new tab in browser mode); same-origin and loopback sidecar links are untouched. Tests: vision derivation + error summarizer unit tests; go test ./... passes; golangci-lint clean; web tsc + vite build pass.
📝 WalkthroughWalkthroughThe PR makes chat-model construction provider-aware, derives vision support from static registry metadata, summarizes agent errors with optional details, persists per-session composer drafts, and routes external HTTP(S) links through the desktop opener. ChangesModel capabilities and provider wiring
Run-error delivery
Composer draft persistence
External link handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ChatInput
participant drafts
participant localStorage
ChatInput->>drafts: readDraft(sessionId)
drafts->>localStorage: get session draft
localStorage-->>drafts: draft text
drafts-->>ChatInput: restore composer text
ChatInput->>drafts: writeDraft(sessionId, text)
drafts->>localStorage: save or remove session draft
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/model/error_summary.go`:
- Around line 66-68: Update apiMessagePattern to enable dotall matching with
(?s) and prefix the message tag with a greedy .* so multiline messages are
supported and FindAllStringSubmatch can select the final message: occurrence in
nested errors. Preserve the existing trailing-content capture and last-match
extraction behavior.
In `@internal/web/models.go`:
- Around line 125-149: Move the eng.applyModelSwitch call to after the
configuration persistence and persistErr failure return in the model-switch
handler. Ensure it runs only after config.SaveConfig succeeds, while preserving
the existing rollback and HTTP 500 behavior for save failures.
In `@web/src/components/ChatInput.tsx`:
- Around line 308-309: Move the draftLiveRef.current assignment out of the
ChatInput render body and into a useEffect that depends on currentSessionId and
input. Keep the ref initialized with the existing initial draft value, and
ensure the effect updates both sessionId and text after commit.
🪄 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: 5c652ab1-3154-4863-a620-7c80f2ec749d
📒 Files selected for processing (21)
internal/command/acp.gointernal/command/interactive.gointernal/command/web.gointernal/command/workflow.gointernal/handler/web.gointernal/model/chatmodel.gointernal/model/chatmodel_vision_test.gointernal/model/error_summary.gointernal/model/error_summary_test.gointernal/model/factory.gointernal/model/registry.gointernal/tools/subagent_model_routing_test.gointernal/web/models.gointernal/web/server.goweb/src/app/store.tsweb/src/app/wsBridge.tsweb/src/components/ChatInput.tsxweb/src/lib/drafts.tsweb/src/lib/useDesktop.tsweb/src/lib/ws.tsweb/src/main.tsx
Comment on lines
+66
to
+68
| // apiMessagePattern captures the trailing "message: …" segment produced by | ||
| // go-openai's APIError.Error(); the last occurrence wins for nested wraps. | ||
| apiMessagePattern = regexp.MustCompile(`message: (.+?)\s*$`) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix apiMessagePattern to support multiline messages and properly extract the last occurrence.
The current pattern message: (.+?)\s*$ has two flaws:
- The
.wildcard does not match newlines by default in Go. If an API message spans multiple lines (e.g., formatted JSON or a bulleted list), the regex will fail entirely, causing the summary to fall back to the noisy full error text. - The
\s*$anchor forces the match to the end of the text. Consequently,FindAllStringSubmatchwill only ever yield a single match, meaning the "last occurrence wins for nested wraps" logic does not work as intended (it will instead capture the firstmessage:and everything after it).
To fix both issues, use (?s) for multiline matching and prefix with a greedy .* to scan up to the last message: tag.
🐛 Proposed fix
- // apiMessagePattern captures the trailing "message: …" segment produced by
- // go-openai's APIError.Error(); the last occurrence wins for nested wraps.
- apiMessagePattern = regexp.MustCompile(`message: (.+?)\s*$`)
+ // apiMessagePattern captures the trailing "message: …" segment produced by
+ // go-openai's APIError.Error(). Using (?s) supports multiline messages,
+ // and the greedy .* ensures the last occurrence wins for nested wraps.
+ apiMessagePattern = regexp.MustCompile(`(?s).*message:\s*(.*)$`)📝 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
| // apiMessagePattern captures the trailing "message: …" segment produced by | |
| // go-openai's APIError.Error(); the last occurrence wins for nested wraps. | |
| apiMessagePattern = regexp.MustCompile(`message: (.+?)\s*$`) | |
| // apiMessagePattern captures the trailing "message: …" segment produced by | |
| // go-openai's APIError.Error(). Using (?s) supports multiline messages, | |
| // and the greedy .* ensures the last occurrence wins for nested wraps. | |
| apiMessagePattern = regexp.MustCompile(`(?s).*message:\s*(.*)$`) |
🤖 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/error_summary.go` around lines 66 - 68, Update
apiMessagePattern to enable dotall matching with (?s) and prefix the message tag
with a greedy .* so multiline messages are supported and FindAllStringSubmatch
can select the final message: occurrence in nested errors. Preserve the existing
trailing-content capture and last-match extraction behavior.
Comment on lines
125
to
+149
| eng.applyModelSwitch(ag, req.Provider, req.Model) | ||
|
|
||
| // Persist the selection so a restart resumes on this model — matches the | ||
| // TUI model picker, which writes cfg.Model on every switch. In-place on the | ||
| // shared cfg under cfgMu (same discipline as handleSetSmallModel). | ||
| s.cfgMu.Lock() | ||
| s.mu.Lock() | ||
| var persistErr error | ||
| if s.cfg != nil { | ||
| prevModel := s.cfg.Model | ||
| s.cfg.Model = req.Provider + "/" + req.Model | ||
| if err := config.SaveConfig(s.cfg); err != nil { | ||
| // Keep memory consistent with disk: a failed save must not leave the | ||
| // live config advertising a value that won't survive a restart. | ||
| s.cfg.Model = prevModel | ||
| persistErr = err | ||
| } | ||
| } | ||
| s.mu.Unlock() | ||
| s.cfgMu.Unlock() | ||
| if persistErr != nil { | ||
| writeJSON(w, http.StatusInternalServerError, map[string]string{"error": "failed to save config: " + persistErr.Error()}) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent state inconsistency on config save failure.
Currently, eng.applyModelSwitch is called before persisting the configuration. If config.SaveConfig fails, the API returns an HTTP 500 error and skips the WebSocket broadcast, leading the client to believe the switch failed. However, the agent's internal state has already been updated to the new model, causing a mismatch between the UI, the config, and the active session.
Move the eng.applyModelSwitch call after the persistence block to ensure the session state is only updated if the config is successfully saved.
💡 Proposed fix
if err != nil {
writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()})
return
}
- eng.applyModelSwitch(ag, req.Provider, req.Model)
// Persist the selection so a restart resumes on this model — matches the
// TUI model picker, which writes cfg.Model on every switch. In-place on the
// shared cfg under cfgMu (same discipline as handleSetSmallModel).
s.cfgMu.Lock()
s.mu.Lock()
var persistErr error
if s.cfg != nil {
prevModel := s.cfg.Model
s.cfg.Model = req.Provider + "/" + req.Model
if err := config.SaveConfig(s.cfg); err != nil {
// Keep memory consistent with disk: a failed save must not leave the
// live config advertising a value that won't survive a restart.
s.cfg.Model = prevModel
persistErr = err
}
}
s.mu.Unlock()
s.cfgMu.Unlock()
if persistErr != nil {
writeJSON(w, http.StatusInternalServerError, map[string]string{"error": "failed to save config: " + persistErr.Error()})
return
}
+
+ eng.applyModelSwitch(ag, req.Provider, req.Model)
// Track in recent models.📝 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
| eng.applyModelSwitch(ag, req.Provider, req.Model) | |
| // Persist the selection so a restart resumes on this model — matches the | |
| // TUI model picker, which writes cfg.Model on every switch. In-place on the | |
| // shared cfg under cfgMu (same discipline as handleSetSmallModel). | |
| s.cfgMu.Lock() | |
| s.mu.Lock() | |
| var persistErr error | |
| if s.cfg != nil { | |
| prevModel := s.cfg.Model | |
| s.cfg.Model = req.Provider + "/" + req.Model | |
| if err := config.SaveConfig(s.cfg); err != nil { | |
| // Keep memory consistent with disk: a failed save must not leave the | |
| // live config advertising a value that won't survive a restart. | |
| s.cfg.Model = prevModel | |
| persistErr = err | |
| } | |
| } | |
| s.mu.Unlock() | |
| s.cfgMu.Unlock() | |
| if persistErr != nil { | |
| writeJSON(w, http.StatusInternalServerError, map[string]string{"error": "failed to save config: " + persistErr.Error()}) | |
| return | |
| } | |
| // Persist the selection so a restart resumes on this model — matches the | |
| // TUI model picker, which writes cfg.Model on every switch. In-place on the | |
| // shared cfg under cfgMu (same discipline as handleSetSmallModel). | |
| s.cfgMu.Lock() | |
| s.mu.Lock() | |
| var persistErr error | |
| if s.cfg != nil { | |
| prevModel := s.cfg.Model | |
| s.cfg.Model = req.Provider + "/" + req.Model | |
| if err := config.SaveConfig(s.cfg); err != nil { | |
| // Keep memory consistent with disk: a failed save must not leave the | |
| // live config advertising a value that won't survive a restart. | |
| s.cfg.Model = prevModel | |
| persistErr = err | |
| } | |
| } | |
| s.mu.Unlock() | |
| s.cfgMu.Unlock() | |
| if persistErr != nil { | |
| writeJSON(w, http.StatusInternalServerError, map[string]string{"error": "failed to save config: " + persistErr.Error()}) | |
| return | |
| } | |
| eng.applyModelSwitch(ag, req.Provider, req.Model) | |
| // Track in recent models. |
🤖 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/models.go` around lines 125 - 149, Move the eng.applyModelSwitch
call to after the configuration persistence and persistErr failure return in the
model-switch handler. Ensure it runs only after config.SaveConfig succeeds,
while preserving the existing rollback and HTTP 500 behavior for save failures.
Comment on lines
+308
to
+309
| const draftLiveRef = useRef({ sessionId: currentSessionId, text: input }) | ||
| draftLiveRef.current = { sessionId: currentSessionId, text: input } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid mutating refs during the render phase.
React requires render functions to be pure. Mutating draftLiveRef.current directly in the component body can cause unpredictable behavior in Strict Mode or Concurrent React if a render is aborted or restarted. Wrap the assignment in a useEffect to ensure it only mutates after the DOM is committed.
♻️ Proposed fix
const draftLiveRef = useRef({ sessionId: currentSessionId, text: input })
- draftLiveRef.current = { sessionId: currentSessionId, text: input }
+ useEffect(() => {
+ draftLiveRef.current = { sessionId: currentSessionId, text: input }
+ }, [currentSessionId, input])📝 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
| const draftLiveRef = useRef({ sessionId: currentSessionId, text: input }) | |
| draftLiveRef.current = { sessionId: currentSessionId, text: input } | |
| const draftLiveRef = useRef({ sessionId: currentSessionId, text: input }) | |
| useEffect(() => { | |
| draftLiveRef.current = { sessionId: currentSessionId, text: input } | |
| }, [currentSessionId, input]) |
🤖 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/ChatInput.tsx` around lines 308 - 309, Move the
draftLiveRef.current assignment out of the ChatInput render body and into a
useEffect that depends on currentSessionId and input. Keep the ref initialized
with the existing initial draft value, and ensure the effect updates both
sessionId and text after commit.
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.
概述
四个 web/desktop 体验修复（相互独立但同域，故同 PR）：
1. 多模态 → 文本模型切换自动过滤图片
问题：从多模态模型切到纯文本模型后，历史消息中的 image parts 原样提交，provider 400：
messages.content.type 参数非法，取值范围 ['text']。修复：
NewChatModelFromProvider新增 provider 参数，vision 解析顺序：显式pc.Vision覆盖 → registry modalities → 默认开启（未知/自定义模型保持原行为）。模型切换本就会重建 chatModel，已有的toOpenAIMessage剥离逻辑随之对整个历史生效。新增RegistryModel.SupportsImageInput()并去重 web/acp 三处 modality 循环。2. web 运行错误美化
问题：
[NodeRunError] error, status code: 400, ... node path: [node_1, ChatModel]原文直接刷在时间线。修复：新增
model.SummarizeRunError—— 剥离 eino/go-openai 包装，输出单行摘要（API error 400: …，400 图片拒绝时附加 "this model may not support image input" 提示），原文进WebDoneData.detail。前端透传后由 jcode-uiMessage组件渲染为摘要 + 可折叠 details。3. 模型切换持久化到 config
问题：web/desktop 切换模型只写内存，重启回退。
修复：
handleSwitchModel切换成功后按handleSetSmallModel的锁纪律（cfgMu→mu、失败回滚）写cfg.Model+SaveConfig，与 TUI 选择器行为对齐。4. 输入框草稿按会话记忆
新增
web/src/lib/drafts.ts（localStorage 按 session id，try/catch 防御 hardened webview），ChatInput懒初始化恢复、会话切换 flush/恢复、发送后清除、卸载兜底。重启 app 草稿保留。5. desktop 外链走系统浏览器
问题：desktop 内点聊天链接直接在 app webview 里导航（
target=_blank则因 Tauri 默认拒绝新窗口成为死链）。修复：
initExternalLinks()委托捕获阶段点击监听，外部 http(s) 链接经tauri-plugin-opener走系统浏览器（浏览器模式新开标签页）；同源与 loopback（Bearer 鉴权的 sidecar API）放行。插件与 capability 已存在，零 Rust 改动。测试
go test ./...通过（26 包）golangci-lint0 issuesmake lint-web（tsc × 3）+make build-web通过Summary by CodeRabbit
New Features
Bug Fixes