Skip to content

fix(client): resolve hostContext.theme stuck on "light" when dark mode toggled#1983

Closed
mdodell wants to merge 2 commits into
MCPJam:mainfrom
mdodell:mdodell/fix-dark-mode-host-context-theme
Closed

fix(client): resolve hostContext.theme stuck on "light" when dark mode toggled#1983
mdodell wants to merge 2 commits into
MCPJam:mainfrom
mdodell:mdodell/fix-dark-mode-host-context-theme

Conversation

@mdodell

@mdodell mdodell commented Apr 30, 2026

Copy link
Copy Markdown

Fixes #1982

hostContext.theme and style variables always resolved to "light" even after toggling dark mode in Settings. The global themeMode preference was baked into buildDefaultWorkspaceHostContext, but loadWorkspaceHostContext silently skipped updates when the store was dirty or had a saved config — so the stale value always won.

Removes theme from the default workspace host context so the resolution chain falls through to themeMode directly. The playground theme toggle now updates the global preference instead of patching draftHostContext.

…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>
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Apr 30, 2026
@chelojimenez

chelojimenez commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@coderabbitai

coderabbitai Bot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

Theme is removed from workspace host-context and client-config builders; buildDefaultWorkspaceHostContext/buildDefaultWorkspaceClientConfig no longer accept or set a theme field. WorkspaceClientConfigSync stops reading theme from the preferences store. HostContextHeader refactors toggle logic to compute a local newTheme and pass it to patchHostContext. Chat/App renderer logic now resolves theme from the host-theme provider (chatboxHostTheme ?? themeMode) instead of draft host context. Tests were updated to reflect that host-context drafts no longer contain theme and theme resolution uses the host-theme provider.


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.

❤️ Share
Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

@Genmin

Genmin commented May 1, 2026

Copy link
Copy Markdown

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 theme: "light", removing theme from the default host context does not clear that saved value. In the playground, both MCPAppsRenderer and PlaygroundMain still prefer extractHostTheme(draftHostContext) over the shared themeMode, so Settings dark mode can still send/render light for that saved workspace case.

A focused regression would be: set themeMode = "dark", isPlaygroundActive = true, and draftHostContext = { theme: "light" }; assert the MCP Apps bridge hostContext has theme: "dark" and dark SEP variables. The corresponding runtime fix is to resolve MCP Apps/playground rendering theme from the active chat/global preference (chatboxHostTheme ?? themeMode) rather than from draftHostContext.theme, while keeping the other host-context fields unchanged.

… 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>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
mcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/__tests__/mcp-apps-renderer.test.tsx (1)

384-413: ⚡ Quick win

Strengthen this regression test with dark-token verification.

Lines 399 and 409 assert theme: "dark", but the bug contract also requires dark SEP variables. Please assert hostContext.styles.variables reflects 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

📥 Commits

Reviewing files that changed from the base of the PR and between 419b473 and 09fa9e4.

📒 Files selected for processing (5)
  • mcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/__tests__/mcp-apps-renderer.test.tsx
  • mcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/mcp-apps-renderer.tsx
  • mcpjam-inspector/client/src/components/shared/HostContextHeader.tsx
  • mcpjam-inspector/client/src/components/shared/__tests__/HostContextHeader.test.tsx
  • mcpjam-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

@mdodell

mdodell commented May 12, 2026

Copy link
Copy Markdown
Author

Closing this - its fixed in the latest version (2.4.3).

@mdodell mdodell closed this May 12, 2026
chelojimenez added a commit that referenced this pull request May 20, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] dark/light mode switch has no effect, sending incorrect variables to MCP Apps

3 participants