fix(client): resolve hostContext.theme stuck on "light" when dark mode toggled#1983
fix(client): resolve hostContext.theme stuck on "light" when dark mode toggled#1983mdodell wants to merge 2 commits into
Conversation
…dark mode is toggled Theme had two competing sources of truth — `themeMode` in the preferences store (set by the Settings toggle) and `draftHostContext.theme` baked into the default host context by `buildDefaultWorkspaceHostContext`. When the global theme changed, `loadWorkspaceHostContext` silently skipped the update whenever the store was dirty or a saved config existed, so the stale "light" value always won the resolution chain. Remove `theme` from the default workspace host context so it is resolved at render time from the global preference instead. The playground theme toggle now updates the global preference directly. Closes MCPJam#1982 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
WalkthroughTheme is removed from workspace host-context and client-config builders; 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
|
Nice fix. I think there is still one saved-config path worth covering before merge: if a workspace already has a saved/draft host context with A focused regression would be: set |
… global prefs The theme toggle in HostContextHeader was calling updateThemeMode/setThemeMode which changed the global app shell preference. The intent is for the app-builder theme to be independent — persisting with the workspace host context (like locale and timezone) so MCP apps receive the correct theme via hostContext.theme. MCPAppsRenderer no longer reads configuredHostTheme from draftHostContext directly; theme reaches it through ChatboxHostThemeProvider -> chatboxHostTheme, which PlaygroundMain feeds from extractHostTheme(draftHostContext) ?? globalThemeMode. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/__tests__/mcp-apps-renderer.test.tsx (1)
384-413: ⚡ Quick winStrengthen this regression test with dark-token verification.
Lines 399 and 409 assert
theme: "dark", but the bug contract also requires dark SEP variables. Please asserthostContext.styles.variablesreflects dark-mode tokens in both initialize and update paths.Proposed test augmentation
+import { getHostStyleOrDefault } from "@/lib/host-styles"; ... it("resolves theme from host context provider, ignoring draftHostContext.theme", async () => { Object.assign(mockPlaygroundStoreState, { isPlaygroundActive: true, }); mockPreferencesState.themeMode = "dark"; + const expectedDarkVariables = + getHostStyleOrDefault(mockPreferencesState.hostStyle).resolveStyleVariables( + "dark", + ); mockHostContextStoreState.draftHostContext = { theme: "light", }; ... expect(appBridgeArgsRef.current?.options?.hostContext?.theme).toBe("dark"); + expect( + appBridgeArgsRef.current?.options?.hostContext?.styles?.variables, + ).toMatchObject(expectedDarkVariables); ... expect(mockBridge.setHostContext).toHaveBeenLastCalledWith( expect.objectContaining({ theme: "dark", + styles: expect.objectContaining({ + variables: expect.objectContaining(expectedDarkVariables), + }), }), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/__tests__/mcp-apps-renderer.test.tsx` around lines 384 - 413, The test currently asserts the theme is "dark" but must also verify dark-mode SEP variables are present; update the MCPAppsRenderer test to additionally assert that appBridgeArgsRef.current?.options?.hostContext?.styles?.variables contains the expected dark-mode tokens right after render/initialization, and also that mockBridge.setHostContext was last called with an object whose hostContext.styles.variables reflect the same dark tokens after triggerReady(); reference the MCPAppsRenderer component, appBridgeArgsRef, mockBridge.connect, mockBridge.setHostContext, and triggerReady to locate where to add these two assertions (one for the initial appBridgeArgsRef snapshot and one inside the expect(mockBridge.setHostContext).toHaveBeenLastCalledWith(...)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@mcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/__tests__/mcp-apps-renderer.test.tsx`:
- Around line 384-413: The test currently asserts the theme is "dark" but must
also verify dark-mode SEP variables are present; update the MCPAppsRenderer test
to additionally assert that
appBridgeArgsRef.current?.options?.hostContext?.styles?.variables contains the
expected dark-mode tokens right after render/initialization, and also that
mockBridge.setHostContext was last called with an object whose
hostContext.styles.variables reflect the same dark tokens after triggerReady();
reference the MCPAppsRenderer component, appBridgeArgsRef, mockBridge.connect,
mockBridge.setHostContext, and triggerReady to locate where to add these two
assertions (one for the initial appBridgeArgsRef snapshot and one inside the
expect(mockBridge.setHostContext).toHaveBeenLastCalledWith(...)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3911cc3f-f32a-45ac-aba8-465514c7b7e9
📒 Files selected for processing (5)
mcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/__tests__/mcp-apps-renderer.test.tsxmcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/mcp-apps-renderer.tsxmcpjam-inspector/client/src/components/shared/HostContextHeader.tsxmcpjam-inspector/client/src/components/shared/__tests__/HostContextHeader.test.tsxmcpjam-inspector/client/src/components/ui-playground/PlaygroundMain.tsx
✅ Files skipped from review due to trivial changes (3)
- mcpjam-inspector/client/src/components/shared/tests/HostContextHeader.test.tsx
- mcpjam-inspector/client/src/components/ui-playground/PlaygroundMain.tsx
- mcpjam-inspector/client/src/components/shared/HostContextHeader.tsx
|
Closing this - its fixed in the latest version ( |
…bmit, det-exec, stop, state prune) Reviewer-flagged P1 routing blockers on PR #2196. Every spot in `PlaygroundMain` that previously branched on `isMultiModelMode` to distinguish "compare grid is live" from "true single-pane" was wrong for multi-host mode: host compare fell through the single-pane branch, hitting the hidden root chat session instead of the visible per-card streams. Introduced a unified `isCompareMode = isMultiModelMode || isMultiHostMode` near the gate definitions. Routed the four blockers through it: 1. Submit (#2418) — pre-fix host compare called BOTH `queueBroadcastRequest` AND `sendMessage`, producing a duplicate hidden run plus a broken transcript-handoff baseline. Now the broadcast-only path fires for both modes; `sendMessage` is exclusive to true single-pane. 2. Deterministic execution (#1983) — pre-fix `pendingExecution` in host mode wrote to the hidden root via `setMessages` instead of fanning out via `deterministicExecutionRequest`. Now both modes broadcast. 3. State pruning (#1932) — pre-fix `compareSummaries` / `compareHasMessages` were pruned against model ids only, so a chat- input model change in host compare evicted every host-keyed entry and collapsed the grid to its empty state. activeIds is now the union of `resolvedSelectedModels` and `multiHostColumns`; the effect's deps include both inputs. 4. Stop controls (`use-chat-stop-controls.ts`) — pre-fix the hook took `isMultiModelMode` and called the root `stop()` in host compare, leaving the visible per-card streams running. Renamed the option to `isCompareMode`; `ChatTabV2` keeps its existing semantics by passing `isCompareMode: isMultiModelMode` (no host axis in the chat tab). Also rerouted to `isCompareMode`: - `handleRequireToolApprovalChange` — approval is plumbed via per-card `executionConfig`, so changing it in host compare needs a session generation bump like multi-model does. - The prelude-trace clearing effect — host compare is a compare grid; the single-pane snapshot-clearing logic shouldn't fire under it. - Composer placeholder strings — same chat-tab framing for both compare grids. - Display-mode fullscreen guard. Tightened `isMultiHostMode` to require `resolvedSelectedHosts.length > 1` (valid-but-lower reviewer finding). The picker auto-disables `multiHostEnabled` on the 2 → 1 transition, but a stale localStorage value or a momentarily-unresolved secondary would otherwise let the grid render as a one-column variant of single-pane — routing submit/stop/state through the compare path with no compare value. Left `isMultiModelMode` in place where the branch is model-specific (model-mode added-column effect, `currentCompareMode` tag, model- analytics props, the legacy `multiModelEnabled` prop on the chat input). `multiModelEnabled` analytics tags also stayed model-only — the field name is part of an existing dashboard contract; broadening silently would conflate axes. Regression tests (5 new, `PlaygroundMain.multi-host.test.tsx`): - "submit in multi-host mode broadcasts to the compare cards and does NOT also fire the hidden root sendMessage": drives the captured `ChatInput.onChange` + `onSubmit`, asserts `mockUseChatSession.sendMessage` not called and the latest card receives a broadcast request matching the typed text. - "deterministic tool execution in multi-host mode fans out to the visible cards (not the hidden root session)": rerenders with a populated `pendingExecution`, asserts each card receives the request and `setMessages` is not touched. - "compare state (compareHasMessages) survives a chat-input model change in multi-host mode (host-id entries not pruned)": seeds h-A as having messages via the captured `onHasMessagesChange`, swaps `mockUseChatSession.selectedModel`, asserts the grid section is still visible (`aria-hidden="false"`). - "stop control in multi-host mode bumps the broadcast stop-request and does NOT hit the hidden root stop()": calls captured `ChatInput.stop()`, asserts root `stop` not called and the latest card's `stopRequestId` increased. - "falls back to single-pane when multi-host is persisted on but only one host resolves (length>1 gate)": one resolved host with `multiHostEnabled=true` no longer renders the compare grid. `ChatInput` mock upgraded to capture props (`mockChatInput`) so submit and stop can be driven directly. `act` imported from @testing-library/react for the synchronous setState dispatches. `use-chat-stop-controls.test.tsx` callsites updated for the renamed option. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fixes #1982
hostContext.themeand style variables always resolved to"light"even after toggling dark mode in Settings. The globalthemeModepreference was baked intobuildDefaultWorkspaceHostContext, butloadWorkspaceHostContextsilently skipped updates when the store was dirty or had a saved config — so the stale value always won.Removes
themefrom the default workspace host context so the resolution chain falls through tothemeModedirectly. The playground theme toggle now updates the global preference instead of patchingdraftHostContext.