fix(ai-provider): sweep sessions + show switch progress + green banner - #83
Merged
KrasimirKralev merged 2 commits intoApr 19, 2026
Conversation
Changing the primary AI provider in Settings (e.g. OpenAI → ClawBox AI) had three user-visible problems: 1. Chat kept responding as the OLD provider after the switch. The configure route updated agents.defaults.model.primary but left every existing session's per-session override pointing at the previous model. OpenClaw's resolver respected those sticky overrides on the next turn, so the chat stayed bound to the old provider until the user cleared the session — or refreshed the page and noticed it still answered wrong. 2. Chat 'froze' during the gateway restart. The chat popup only had skill-install-aware retry logic, so a provider-change gateway restart dropped the WS and the user saw no progress; only the bare retry loop slowly reconnected. 3. No affirmative 'switched to X' feedback — when reconnect finally landed, the chat looked identical to before. The dropdown already shows a green 'Switched chat to X' banner when switching models from within the chat pane (PR ID-Robots#73); provider changes via Settings deserved the same. Changes: - src/app/setup-api/ai-models/configure/route.ts: after 'agents.defaults.model.primary' is written, sweep per-session model overrides via applyModelOverrideToAllAgentSessions with source: 'user' so OpenClaw's resolver keeps the new primary bound across existing sessions. Mirrors the sweep in /setup-api/chat/model from PR ID-Robots#73. Only runs when this call actually set a new primary (skips local-only scopes). - src/components/ChatPopup.tsx: listens for the existing clawbox:primary-ai-configured event (dispatched by SettingsApp after provider configure succeeds) alongside clawbox-skill-installed. Reuses the reloadingSkill overlay + progress bar + quadrupled retry budget — makes the gateway restart visible instead of letting the chat look frozen. Banner label adapts ('Switching AI provider...' vs 'Reloading skills...') via a new reloadReason state; a matching reloadReasonRef escapes the frozen closure captured by the one-time WebSocket hello resolve callback. After reconnect on a provider change, the auto-'my skills were just updated' prompt is skipped (no skill changed, nothing to confirm) and a green 'Switched chat to <label>' banner is pushed using the refreshed chat/model state. - src/app/setup-api/chat/model/route.ts: PROVIDER_LABELS map was missing 'openai-codex', so labelForProvider fell back to the generic 'AI Provider' placeholder — which made the new banner read 'Switched chat to AI Provider' when switching to or from openai-codex. Added 'openai-codex: "OpenAI Codex"'. Post-fix /simplify pass converted the inline fetch().then().then() chain in ChatPopup's reload branch to async/await to match the existing refreshChatModelState pattern in the same file. Semantics unchanged.
Contributor
Author
|
@coderabbitai review |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 29 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes implement AI model provider change synchronization across the system. When a primary AI model is configured, all existing agent sessions are updated with the new provider and model before the gateway restarts. The chat component independently tracks provider changes versus skill reloads and updates the UI accordingly. Changes
Sequence DiagramsequenceDiagram
actor User
participant ConfigAPI as Configure API
participant SessionDB as Session Storage
participant Gateway as Gateway
participant ChatClient as Chat Client
participant WebSocket as WebSocket
User->>ConfigAPI: POST primary model change
ConfigAPI->>SessionDB: applyModelOverrideToAllAgentSessions<br/>(new provider/model)
alt Override Success
SessionDB-->>ConfigAPI: sessions updated
else Override Fails
SessionDB-->>ConfigAPI: error (logged, non-fatal)
end
ConfigAPI->>Gateway: restart gateway
Gateway-->>ChatClient: connection reset
ChatClient->>WebSocket: reconnect
WebSocket-->>ChatClient: hello/connect
alt Provider Change Reload
ChatClient->>ChatClient: fetch updated model state
ChatClient->>ChatClient: append success message<br/>"Switched chat to <label>"
else Skill Reload
ChatClient->>ChatClient: auto-send skill update prompt
end
ChatClient->>User: show updated UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/ChatPopup.tsx (2)
387-416:⚠️ Potential issue | 🟠 MajorDon’t clear the transcript for provider-only reloads.
Line 391 runs before
wasProviderChangeis checked, so a Settings provider switch replaces the current visible chat with only the success banner. Keep the reset for skill changes only.🐛 Proposed fix
if (skillInstalledRef.current) { const wasProviderChange = reloadReasonRef.current === 'provider' skillInstalledRef.current = false reloadReasonRef.current = 'skill' // reset for next reload - setMessages([]) - greetedRef.current = true // prevent auto-greet + if (!wasProviderChange) { + setMessages([]) + greetedRef.current = true // prevent auto-greet + } const evt = skillEventRef.current skillEventRef.current = null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ChatPopup.tsx` around lines 387 - 416, The code clears the chat transcript unconditionally (skillInstalledRef/reloadReasonRef logic) before checking wasProviderChange, causing provider-only switches to wipe messages; modify the flow in the block that handles skillInstalledRef.current so you only call setMessages([]) and set greetedRef.current = true when reloadReasonRef.current is not 'provider' (i.e., when handling actual skill install/uninstall/enable/disable events), leaving the existing success banner behavior for provider changes; ensure you still reset skillInstalledRef.current, reloadReasonRef.current (to 'skill' only when you cleared messages), and clear reloadTimerRef.current / setReloadProgress(100) / setReloadingSkill(false) as currently done.
818-849:⚠️ Potential issue | 🟠 MajorMake provider reload completion independent of a future WebSocket hello.
The Settings event is emitted after configuration, while the configure route restarts the gateway before returning. If the socket has already reconnected by the time this handler runs, this only starts the overlay and no later hello clears it.
🐛 Proposed fix direction
- const providerHandler = makeHandler('provider') + const providerReloadHandler = makeHandler('provider') + const providerHandler = (e: Event) => { + providerReloadHandler(e) + // Provider events can arrive after the gateway restart already completed; + // force a fresh handshake so the provider completion branch always runs. + void connect() + } window.addEventListener('clawbox-skill-installed', skillHandler) window.addEventListener('clawbox:primary-ai-configured', providerHandler) return () => { window.removeEventListener('clawbox-skill-installed', skillHandler) window.removeEventListener('clawbox:primary-ai-configured', providerHandler) if (reloadTimerRef.current) clearInterval(reloadTimerRef.current) } - }, []) + }, [connect])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ChatPopup.tsx` around lines 818 - 849, The providerHandler started via makeHandler('provider') can start the reload overlay even when the gateway/socket has already reconnected, so change providerHandler to complete the reload independently of waiting for a future WebSocket hello: after running the existing makeHandler setup, immediately check the current connection state (e.g. inspect your socket readyState or the app-level connected flag) and if already connected, clear reloadTimerRef, setReloadProgress(100), setReloadingSkill(false), reset retryCountRef and skillInstalledRef/reloadReasonRef as needed; otherwise keep the existing behavior but also subscribe to the connection/hello event to finalize the reload (clear timer and setReloadProgress(100)/setReloadingSkill(false)) when that event arrives. Use the existing symbols makeHandler, providerHandler, skillHandler, reloadTimerRef, setReloadProgress, setReloadingSkill, retryCountRef, skillInstalledRef and reloadReasonRef to locate and update the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ChatPopup.tsx`:
- Around line 421-425: The fetch response from '/setup-api/chat/model' should be
validated before casting and setting state; inside the try block where you call
fetch and parse JSON, check res.ok (and/or validate that parsed object matches
ChatModelState shape and has options array) and handle non-OK or malformed
responses by throwing or returning early (e.g., log error and do not call
setChatModelState). Update the logic around setChatModelState and the subsequent
label derivation (the const label = state.activeLabel ...) to run only when the
response is valid so that later uses like chatModelState.options.map cannot
crash. Ensure you reference the existing symbols:
fetch('/setup-api/chat/model'), setChatModelState, ChatModelState, and
chatModelState.options.map when applying the checks.
---
Outside diff comments:
In `@src/components/ChatPopup.tsx`:
- Around line 387-416: The code clears the chat transcript unconditionally
(skillInstalledRef/reloadReasonRef logic) before checking wasProviderChange,
causing provider-only switches to wipe messages; modify the flow in the block
that handles skillInstalledRef.current so you only call setMessages([]) and set
greetedRef.current = true when reloadReasonRef.current is not 'provider' (i.e.,
when handling actual skill install/uninstall/enable/disable events), leaving the
existing success banner behavior for provider changes; ensure you still reset
skillInstalledRef.current, reloadReasonRef.current (to 'skill' only when you
cleared messages), and clear reloadTimerRef.current / setReloadProgress(100) /
setReloadingSkill(false) as currently done.
- Around line 818-849: The providerHandler started via makeHandler('provider')
can start the reload overlay even when the gateway/socket has already
reconnected, so change providerHandler to complete the reload independently of
waiting for a future WebSocket hello: after running the existing makeHandler
setup, immediately check the current connection state (e.g. inspect your socket
readyState or the app-level connected flag) and if already connected, clear
reloadTimerRef, setReloadProgress(100), setReloadingSkill(false), reset
retryCountRef and skillInstalledRef/reloadReasonRef as needed; otherwise keep
the existing behavior but also subscribe to the connection/hello event to
finalize the reload (clear timer and
setReloadProgress(100)/setReloadingSkill(false)) when that event arrives. Use
the existing symbols makeHandler, providerHandler, skillHandler, reloadTimerRef,
setReloadProgress, setReloadingSkill, retryCountRef, skillInstalledRef and
reloadReasonRef to locate and update the logic.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8bed6678-47b6-490e-99b8-f1ef59b09869
📒 Files selected for processing (3)
src/app/setup-api/ai-models/configure/route.tssrc/app/setup-api/chat/model/route.tssrc/components/ChatPopup.tsx
Comment on lines
+421
to
+425
| try { | ||
| const res = await fetch('/setup-api/chat/model', { cache: 'no-store' }) | ||
| const state = await res.json() as ChatModelState | ||
| setChatModelState(state) | ||
| const label = state.activeLabel ?? state.primary?.label ?? 'the new AI provider' |
There was a problem hiding this comment.
Validate the chat-model response before committing it to state.
A non-OK JSON response like { error: ... } is currently cast to ChatModelState; the next render can crash when it reads chatModelState.options.map.
🛡️ Proposed fix
try {
const res = await fetch('/setup-api/chat/model', { cache: 'no-store' })
- const state = await res.json() as ChatModelState
+ if (!res.ok) throw new Error('Failed to refresh chat model state')
+ const state = await res.json() as ChatModelState
+ if (!Array.isArray(state.options)) {
+ throw new Error('Invalid chat model state response')
+ }
setChatModelState(state)
const label = state.activeLabel ?? state.primary?.label ?? 'the new AI provider'📝 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
| try { | |
| const res = await fetch('/setup-api/chat/model', { cache: 'no-store' }) | |
| const state = await res.json() as ChatModelState | |
| setChatModelState(state) | |
| const label = state.activeLabel ?? state.primary?.label ?? 'the new AI provider' | |
| try { | |
| const res = await fetch('/setup-api/chat/model', { cache: 'no-store' }) | |
| if (!res.ok) throw new Error('Failed to refresh chat model state') | |
| const state = await res.json() as ChatModelState | |
| if (!Array.isArray(state.options)) { | |
| throw new Error('Invalid chat model state response') | |
| } | |
| setChatModelState(state) | |
| const label = state.activeLabel ?? state.primary?.label ?? 'the new AI provider' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ChatPopup.tsx` around lines 421 - 425, The fetch response from
'/setup-api/chat/model' should be validated before casting and setting state;
inside the try block where you call fetch and parse JSON, check res.ok (and/or
validate that parsed object matches ChatModelState shape and has options array)
and handle non-OK or malformed responses by throwing or returning early (e.g.,
log error and do not call setChatModelState). Update the logic around
setChatModelState and the subsequent label derivation (the const label =
state.activeLabel ...) to run only when the response is valid so that later uses
like chatModelState.options.map cannot crash. Ensure you reference the existing
symbols: fetch('/setup-api/chat/model'), setChatModelState, ChatModelState, and
chatModelState.options.map when applying the checks.
…rce hello on provider change Two Major findings on PR ID-Robots#83: 1. Provider switches were wiping the chat transcript. setMessages([]) ran unconditionally in the skillInstalledRef branch before the wasProviderChange check, so a Settings provider change cleared the user's visible history. Skill installs still want the reset (new tools may change the assistant's behavior), but provider changes don't — the conversation semantics are unchanged. Guarded the reset + greetedRef bump behind !wasProviderChange. 2. Provider-reload overlay could get stuck on fast gateway restarts. The configure route restarts the gateway before returning its response, and the primary-ai-configured event fires only after that response lands. If the WS retry loop has already reconnected by then, no future hello triggers the reload branch and the overlay stays up indefinitely. The provider handler now, after seeding state: - closes any currently-open WebSocket (so the resolve-branch fires on a fresh connect rather than relying on a stale hello that missed skillInstalledRef=true) - resets retryCountRef and calls connect() directly This makes the completion independent of any in-flight hello's timing relative to when the event is dispatched.
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
KrasimirKralev
added a commit
to KrasimirKralev/clawbox
that referenced
this pull request
Apr 19, 2026
14 tests had been red on beta since before the Gemma/provider-switch work in PRs ID-Robots#82/ID-Robots#83 landed. Three separate root causes: 1. Test mocks drifted from the configure route's real imports. The route now calls `applyModelOverrideToAllAgentSessions` and `parseFullyQualifiedModel` (added in ID-Robots#83 for session sweeping), plus four functions from `@/lib/llamacpp` and one from `@/lib/local-ai-runtime`. Tests mocked none of these, so the first request-time call to any of them threw and the route returned 500 — producing the "expected 200 to be 500" cascade. Added the missing mocks with real-shape implementations. 2. vitest-under-bun clears mock implementations along with call history in `vi.clearAllMocks()`. Factory defaults set inside `vi.mock(...)` survive `vi.resetModules` but not `mockClear`. Tests passed in isolation, failed in sequence. Re-apply the implementations in `beforeEach` so each test starts with a consistent mock surface. 3. The ai-models-step component test relied on `llamaCppIsActive` defaulting true, but the panel only sets it true when `currentProviderId === "llamacpp"`. Without that prop the "Gemma 4 is already configured" pill never rendered. Added the prop. Extras during simplifier pass: - Mirror real `parseFullyQualifiedModel` logic byte-for-byte (`idx <= 0 || idx === fq.length - 1`) so trailing-slash inputs reject as expected. Fixed drift in `chat-model.test.ts` too. - Extract shared proxy-URL constant via `vi.hoisted` to avoid duplicating the magic string across two mock factories. Result: 93/93 files, 1069/1069 tests pass — CI signal restored.
2 tasks
KrasimirKralev
added a commit
that referenced
this pull request
Apr 19, 2026
…90) 14 tests had been red on beta since before the Gemma/provider-switch work in PRs #82/#83 landed. Three separate root causes: 1. Test mocks drifted from the configure route's real imports. The route now calls `applyModelOverrideToAllAgentSessions` and `parseFullyQualifiedModel` (added in #83 for session sweeping), plus four functions from `@/lib/llamacpp` and one from `@/lib/local-ai-runtime`. Tests mocked none of these, so the first request-time call to any of them threw and the route returned 500 — producing the "expected 200 to be 500" cascade. Added the missing mocks with real-shape implementations. 2. vitest-under-bun clears mock implementations along with call history in `vi.clearAllMocks()`. Factory defaults set inside `vi.mock(...)` survive `vi.resetModules` but not `mockClear`. Tests passed in isolation, failed in sequence. Re-apply the implementations in `beforeEach` so each test starts with a consistent mock surface. 3. The ai-models-step component test relied on `llamaCppIsActive` defaulting true, but the panel only sets it true when `currentProviderId === "llamacpp"`. Without that prop the "Gemma 4 is already configured" pill never rendered. Added the prop. Extras during simplifier pass: - Mirror real `parseFullyQualifiedModel` logic byte-for-byte (`idx <= 0 || idx === fq.length - 1`) so trailing-slash inputs reject as expected. Fixed drift in `chat-model.test.ts` too. - Extract shared proxy-URL constant via `vi.hoisted` to avoid duplicating the magic string across two mock factories. Result: 93/93 files, 1069/1069 tests pass — CI signal restored.
This was referenced May 1, 2026
Merged
8 tasks
6 tasks
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.
Summary
Changing the primary AI provider via Settings (e.g. OpenAI → ClawBox AI) had three visible problems:
agents.defaults.model.primarybut left existing sessions' per-session overrides pointing at the previous model. OpenClaw's resolver respected those sticky overrides on the next turn.Changes
configure/route.ts— after settingagents.defaults.model.primary, sweep per-session overrides viaapplyModelOverrideToAllAgentSessionswithsource: "user"so OpenClaw's resolver keeps the new primary bound across existing sessions. Mirrors PR fix(chat): model-dropdown switches actually stick #73's sweep. Only runs when the call actually set a new primary (skips local-only scopes).ChatPopup.tsx— listens forclawbox:primary-ai-configured(already dispatched by SettingsApp) alongsideclawbox-skill-installed. Reuses the overlay + progress bar + quadrupled retry budget. Banner label adapts ("Switching AI provider..."vs"Reloading skills...") via a newreloadReasonstate + escape-hatchreloadReasonRef(one-time-captured WebSockethelloresolve closure can't see state changes). After reconnect on a provider change, auto-"my skills were updated" prompt is skipped and a green"Switched chat to <label>"banner is pushed using refreshedchat/modelstate.chat/model/route.ts—PROVIDER_LABELSwas missing"openai-codex", solabelForProviderfell back to the generic"AI Provider"placeholder, making the new banner read"Switched chat to AI Provider"when switching to or from openai-codex. Added"openai-codex": "OpenAI Codex"./simplifypassConverted the inline
fetch().then().then()chain in ChatPopup's reload branch toasync/await(matches the existingrefreshChatModelStatepattern in the same file + CLAUDE.md preference). Semantics unchanged.Test plan
Summary by CodeRabbit
New Features
Enhancements