Skip to content

Conversation

@aharvard
Copy link
Collaborator

@aharvard aharvard commented Dec 16, 2025

Summary

This PR centralizes theme management into a single React context, eliminating scattered localStorage reads and brittle event-based synchronization.

Note

This PR also supports the MCP Apps work by providing a clean, reactive way to get the current theme. See related discussion.

Problem

Theme state was previously managed in multiple places with no single source of truth:

  • ThemeSelector.tsx - read/write localStorage, managed local state
  • App.tsx - listened for Electron IPC, wrote to localStorage, dispatched synthetic storage events
  • MCPUIResourceRenderer.tsx - read directly from localStorage (and ignored system theme preference!)
  • Various components watching StorageEvent which doesn't fire for same-window changes

Solution

Created ThemeContext that provides:

  • userThemePreference: What the user chose ('light' | 'dark' | 'system')
  • setUserThemePreference: Function to change preference
  • resolvedTheme: The actual theme to apply ('light' | 'dark')

Changes

  • New: src/contexts/ThemeContext.tsx - Single source of truth for theme state
  • Updated: ThemeSelector.tsx - Now uses useTheme() hook (simplified from ~150 to ~75 lines)
  • Updated: App.tsx - Wraps with ThemeProvider, removed duplicated theme handling
  • Updated: MCPUIResourceRenderer.tsx - Uses resolvedTheme (fixes bug where system theme was ignored)

Benefits

  • ✅ Single source of truth via React context
  • ✅ Automatic re-renders when theme changes
  • ✅ No more localStorage polling or synthetic storage events
  • ✅ Type-safe theme values
  • ✅ Fixes bug in MCPUIResourceRenderer that ignored system theme preference
  • ✅ ~80 fewer lines of code overall

- Create ThemeContext with single source of truth for theme state
- userThemePreference: what the user chose ('light' | 'dark' | 'system')
- resolvedTheme: the actual theme to apply ('light' | 'dark')
- Update ThemeSelector to use useTheme() hook
- Update App.tsx to wrap with ThemeProvider
- Update MCPUIResourceRenderer to use resolvedTheme (fixes bug where system theme was ignored)
- Remove duplicated theme handling logic from App.tsx

This eliminates brittle localStorage watching and provides type-safe,
reactive theme updates via React context.
Copilot AI review requested due to automatic review settings December 16, 2025 16:41
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR centralizes theme management by introducing a React context to replace scattered localStorage reads and brittle event-based synchronization. The change consolidates theme state into a single source of truth that provides the user's preference and the resolved theme value.

Key changes:

  • Created ThemeContext that manages theme preference and system theme detection
  • Simplified ThemeSelector.tsx by removing duplicated theme logic (~75 lines removed)
  • Fixed bug in MCPUIResourceRenderer.tsx where system theme preference was ignored

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
ui/desktop/src/contexts/ThemeContext.tsx New context provider centralizing theme state management with system theme detection
ui/desktop/src/components/GooseSidebar/ThemeSelector.tsx Simplified to use theme context hook instead of local state and localStorage
ui/desktop/src/components/MCPUIResourceRenderer.tsx Fixed to use resolved theme from context instead of localStorage
ui/desktop/src/App.tsx Wrapped app with ThemeProvider and removed duplicated Electron IPC theme handling

- Use resolved theme value in broadcastThemeChange instead of empty string
- Add validation for IPC payload structure before processing theme-changed events
Copy link
Collaborator

@DOsinga DOsinga left a comment

Choose a reason for hiding this comment

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

Nice! thanks for factoring this out

document.documentElement.classList.remove('dark');
document.documentElement.classList.add('light');
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

we could simplify this to
const to_remove = theme == 'dark'? 'light': 'dark'

}, []);

// Apply theme on initial mount
useEffect(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove the comment - this is not true, we apply the theme when it changes. also everywhere we call setResolvedThem(...) and then applyThemToDocument(...) remove the second call and just rely on this effect

const handleThemeChanged = (_event: unknown, ...args: unknown[]) => {
const themeData = args[0];

// Validate IPC payload structure
Copy link
Collaborator

Choose a reason for hiding this comment

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

that doesn't seem necesary - we should just type the theme data and use it on both sides:

themeData: { mode: string; useSystemTheme: boolean; theme: string }

Copy link
Collaborator

@zanesq zanesq left a comment

Choose a reason for hiding this comment

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

Tested works well thanks!

Copilot AI review requested due to automatic review settings December 16, 2025 18:38
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

@aharvard aharvard merged commit d2358f7 into main Dec 16, 2025
24 checks passed
@aharvard aharvard deleted the feat/centralize-theme-context branch December 16, 2025 18:50
zanesq added a commit that referenced this pull request Dec 16, 2025
…s-predefined-models

* 'main' of github.com:block/goose: (81 commits)
  fix: display shell output as static text instead of spinner (#6041)
  fix : Custom providers with empty API keys show as configured in desktop (#6105)
  Add .agents/skills and ~/.config/agent/skills to skills discovery paths (#6139)
  fix: use instructions for system prompt and prompt for user message in subagents (#6121)
  Fix compaction loop for small models or large input (#5803)
  feat: Centralize theme management with ThemeContext (#6137)
  OpenRouter & Xai streaming (#5873)
  fix: resolve mcp-hermit cleanup path expansion issue (#5953)
  feat: add goose PR reviewer workflow (#6124)
  perf: Avoid repeated MCP queries during streaming responses (#6138)
  Fix YAML serialization for recipes with special characters (#5796)
  Add more posthog analytics (privacy aware) (#6122)
  docs: add Sugar MCP server to extensions registry (#6077)
  Fix tokenState loading on new sessions (#6129)
  bump bedrock dep versions (#6090)
  Don't persist ephemeral extensions when resuming sessions (#5974)
  chore(deps): bump mdast-util-to-hast from 13.2.0 to 13.2.1 in /ui/desktop (#5939)
  chore(deps): bump node-forge from 1.3.1 to 1.3.2 in /documentation (#5898)
  Add Scorecard supply-chain security workflow (#5810)
  Don't show subagent tool when we're a subagent (#6125)
  ...

# Conflicts:
#	crates/goose/src/providers/formats/databricks.rs
aharvard added a commit that referenced this pull request Dec 17, 2025
* main:
  fix: we don't need to warn about tool count when in code mode (#6149)
  deps: upgrade agent-client-protocol to 0.9.0 (#6109)
  fix(providers): fix for gemini-cli on windows to work around cmd's multiline prompt limitations #5911 (#5966)
  More slash commands (#5858)
  fix: MCP UI not rendering due to CallToolResult structure change (#6143)
  fix: display shell output as static text instead of spinner (#6041)
  fix : Custom providers with empty API keys show as configured in desktop (#6105)
  Add .agents/skills and ~/.config/agent/skills to skills discovery paths (#6139)
  fix: use instructions for system prompt and prompt for user message in subagents (#6121)
  Fix compaction loop for small models or large input (#5803)
  feat: Centralize theme management with ThemeContext (#6137)
  OpenRouter & Xai streaming (#5873)
  fix: resolve mcp-hermit cleanup path expansion issue (#5953)
  feat: add goose PR reviewer workflow (#6124)
  perf: Avoid repeated MCP queries during streaming responses (#6138)
  Fix YAML serialization for recipes with special characters (#5796)
  Add more posthog analytics (privacy aware) (#6122)
  docs: add Sugar MCP server to extensions registry (#6077)
dianed-square added a commit that referenced this pull request Dec 17, 2025
* origin/main: (57 commits)
  docs: create/edit recipe button (#6145)
  fix(google): Fix 400 Bad Request error with Gemini 3 thought signatures (#6035)
  fix: we don't need to warn about tool count when in code mode (#6149)
  deps: upgrade agent-client-protocol to 0.9.0 (#6109)
  fix(providers): fix for gemini-cli on windows to work around cmd's multiline prompt limitations #5911 (#5966)
  More slash commands (#5858)
  fix: MCP UI not rendering due to CallToolResult structure change (#6143)
  fix: display shell output as static text instead of spinner (#6041)
  fix : Custom providers with empty API keys show as configured in desktop (#6105)
  Add .agents/skills and ~/.config/agent/skills to skills discovery paths (#6139)
  fix: use instructions for system prompt and prompt for user message in subagents (#6121)
  Fix compaction loop for small models or large input (#5803)
  feat: Centralize theme management with ThemeContext (#6137)
  OpenRouter & Xai streaming (#5873)
  fix: resolve mcp-hermit cleanup path expansion issue (#5953)
  feat: add goose PR reviewer workflow (#6124)
  perf: Avoid repeated MCP queries during streaming responses (#6138)
  Fix YAML serialization for recipes with special characters (#5796)
  Add more posthog analytics (privacy aware) (#6122)
  docs: add Sugar MCP server to extensions registry (#6077)
  ...
katzdave added a commit that referenced this pull request Dec 17, 2025
…icing

* 'main' of github.com:block/goose: (35 commits)
  docs: skills (#6062)
  fix: add conditional configuration for GOOSE_BIN_DIR in PATH (#5940)
  Update dependencies to help in Fedora packaging (#5835)
  fix: make goose reviewer less bad (#6154)
  docs: create/edit recipe button (#6145)
  fix(google): Fix 400 Bad Request error with Gemini 3 thought signatures (#6035)
  fix: we don't need to warn about tool count when in code mode (#6149)
  deps: upgrade agent-client-protocol to 0.9.0 (#6109)
  fix(providers): fix for gemini-cli on windows to work around cmd's multiline prompt limitations #5911 (#5966)
  More slash commands (#5858)
  fix: MCP UI not rendering due to CallToolResult structure change (#6143)
  fix: display shell output as static text instead of spinner (#6041)
  fix : Custom providers with empty API keys show as configured in desktop (#6105)
  Add .agents/skills and ~/.config/agent/skills to skills discovery paths (#6139)
  fix: use instructions for system prompt and prompt for user message in subagents (#6121)
  Fix compaction loop for small models or large input (#5803)
  feat: Centralize theme management with ThemeContext (#6137)
  OpenRouter & Xai streaming (#5873)
  fix: resolve mcp-hermit cleanup path expansion issue (#5953)
  feat: add goose PR reviewer workflow (#6124)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants