Skip to content

feat: add color scheme customization#2263

Open
kantord wants to merge 6 commits into
mainfrom
customize-colors-in-desktop-app
Open

feat: add color scheme customization#2263
kantord wants to merge 6 commits into
mainfrom
customize-colors-in-desktop-app

Conversation

@kantord
Copy link
Copy Markdown
Member

@kantord kantord commented May 15, 2026

This PR allows for design tokens to be overridden on the user level.

What is included:

  • user-level override using JSON on all operating systems
  • an example theme for debugging that the theme system works
  • technical documentation on the feature
  • a new theme that matches adjusted brand identities

What is only partially included:

  • Adjustments to the tokens and work to simplify tokens and unify them with other codebases
    -things related to the marketing call to action feature: still considering the best approach for those, but in practice theme overriding won't be needed for those, they just need to look good with the default theme. Best course of action might be to ignore the issue.

What is not included:

  • system-level theme override on Mac: [Task] Mac-native theme override #2267
  • documentation site updates on this feature (would make more sense after better unification across code bases)
  • making the new theme with new brand identity default
  • autoreload/live reload for theme changes. A single theme handles both dark and light mode, so live reloading/autoreloading is not needed for any known use case today. A theme override change requires closing and opening the app
  • any GUI for theme picking etc, not clear product requirement for those

"stacklok theme" (NOT made default yet)

after this pr is merged, we need to decide if we want to make this the default theme

recording-2026-05-19_12-26-44.mp4

"demo theme" for testing

Not meant to be particularly pretty, intentionally very different from the default theme to make it easier to spot problems/"stress" test override capability.

recording-2026-05-19_11-22-17.mp4

Copilot AI review requested due to automatic review settings May 15, 2026 18:00
Copy link
Copy Markdown
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

Adds operator-configurable color scheme branding so the Electron main process can load a JSON theme, serialize allowed color tokens to CSS, and expose it to the renderer before React mounts.

Changes:

  • Adds branding config schema, token sanitization/serialization utilities, loader, and tests.
  • Passes generated branding CSS from main to preload via additionalArguments, then injects it in the renderer.
  • Adds a sample test theme, documentation, and a dev launch script for custom-theme verification.

Reviewed changes

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

Show a summary per file
File Description
common/branding/color-tokens.ts Defines allowed color tokens and serializes safe overrides to CSS.
common/branding/__tests__/color-tokens.test.ts Tests token filtering, sanitization, and CSS output.
common/branding/schema.ts Defines the branding JSON schema.
main/src/branding/load.ts Loads branding config and converts color tokens to CSS.
main/src/branding/__tests__/load.test.ts Tests config loading, invalid inputs, and CSS generation.
main/src/branding/paths.ts Resolves the branding config path.
main/src/main-window.ts Loads branding CSS and passes it to the renderer process.
preload/src/api/branding.ts Reads and decodes branding CSS from process arguments.
preload/src/api/__tests__/branding.test.ts Tests preload branding argument handling.
preload/src/preload.ts Exposes the branding API through electronAPI.
renderer/src/renderer.tsx Injects branding CSS before mounting React.
renderer/src/index.css Adds additional themeable color variables.
renderer/src/common/mocks/electronAPI.ts Adds branding to the renderer test Electron API mock.
vitest.setup.ts Adds branding to the global Electron API test stub.
branding-examples/README.md Documents branding examples and usage.
branding-examples/test-theme.json Adds a sample custom color theme.
scripts/start-custom-theme.ts Adds a helper for launching with the sample theme.
package.json Adds the start:customTheme script.

@kantord
Copy link
Copy Markdown
Member Author

kantord commented May 19, 2026

@claude deep review please

@claude
Copy link
Copy Markdown

claude Bot commented May 19, 2026

Claude finished @kantord's task in 5m 19s —— View job


Deep review — color scheme customization (PR #2263)

  • Read core branding code (schema, loader, color-tokens, paths)
  • Read main/preload/renderer integration points
  • Read modified UI tokens & components
  • Read tests for branding code
  • Assess security (CSS injection, path traversal, schema validation)
  • Assess architecture & design choices

Nice piece of work — clean main/preload/renderer separation, robust sanitization, decent tests, and the :root:not(.dark) specificity trick is well chosen. Comments below are mostly about visual regressions surfaced by the token migration and a few hardening / polish notes.


🔴 Visual regressions to fix

1. Textarea border disappears in light moderenderer/src/common/components/ui/textarea.tsx:13

border-input and the new bg-input both resolve to --input (oklch(0.922 0 0) in light mode — renderer/src/index.css:105). The border is the same color as the background, so it visually vanishes. Previously bg-white provided contrast against the gray border.

dark:bg-card rescues this in dark mode, but light mode now has a borderless textarea. Either:

  • revert the light-mode bg (e.g. bg-card or keep white), or
  • introduce a distinct token (e.g. --input-background semantically separated from the existing border-only meaning).

Fix this →

2. text-warning-foreground is unreadable in dark moderenderer/src/common/components/custom-socket-banner.tsx:35

--warning-foreground is defined as oklch(0.2 0.05 80) (dark olive) in both :root and .dark blocks (renderer/src/index.css:124 and :184). Combined with bg-warning/15 (mostly-transparent yellow tint over the page bg), in dark mode this renders dark text on a near-black background.

This usage is the first time text-warning-foreground appears outside a high-contrast bg-warning surface, which is why the missing dark override only surfaces here. Two ways to address:

  • Override --warning-foreground in .dark { ... } to a light value (e.g. oklch(0.96 0.02 80)), OR
  • Don't pair text-warning-foreground with a transparent warning tint — use text-warning or text-foreground on these alert surfaces.

The cloud-ui parity should mirror whatever choice you make there. Fix this →


🟡 Design / architecture

3. Single-window branding propagationmain/src/main-window.ts:176

additionalArguments is set only on the main window. If you ever add a secondary window (a modal popup, a tray quickwin, an MCP-server log window), its preload will see no branding and the renderer will fall back to defaults. Two reasonable hardenings:

  • Centralize a getCommonWebPreferences() helper that always injects the brand arg, or
  • Switch to a synchronous IPC channel (ipcRenderer.sendSync from preload) — heavier, but immune to "forgot to add the arg" mistakes.

Not blocking — only a concern when more windows arrive.

4. Schema doesn't include chart colorscommon/branding/color-tokens.ts:21-60

--chart-1--chart-5 are defined in index.css but absent from COLOR_TOKEN_KEYS, so operators can't theme them. If that's intentional (cloud-ui parity, chart palettes are off-limits), document it in branding-examples/README.md so it's not a surprise. Otherwise, add them to the allowlist.

5. loadBrandingConfig runs on every createMainWindow callmain/src/main-window.ts:164

createMainWindow is also invoked from recreateMainWindowForShutdown. The file read is small so the cost is negligible, but you could cache it at first call (one-shot read at boot is what the comment in load.ts:14-16 actually claims). If you ever switch to fs.watch, the cache becomes a published reload point. Minor nit.

6. Schema strips unknown nested keys silently

Top-level uses .loose() (forward-compat), but design_tokens and design_tokens.colors use Zod's default "strip" mode. An operator typo like design_tokens.color (singular) → design_tokens.colors is silently ignored with no warning. Consider logging at warn level if the parsed shape has fewer effective color keys than the input file. Pure polish, not blocking.


🟢 Nits / polish

7. Buffer.from(encoded, 'base64') try/catch is unreachablepreload/src/api/branding.ts:22-26

Node's Buffer.from(..., 'base64') never throws on invalid base64 — it silently discards garbage. The catch block is dead but harmless. Either drop the try/catch or add a comment explaining the defensive intent.

8. MAX_VALUE_LENGTH = 100common/branding/color-tokens.ts:69

Fits any reasonable color value; the limit is fine. color(display-p3 ...) with all four args fits in ~60 chars, so 100 leaves headroom. No change needed.

9. No coverage assertion that all COLOR_TOKEN_KEYS survive emissioncommon/branding/__tests__/color-tokens.test.ts

A it.each(COLOR_TOKEN_KEYS)('accepts known key %s', ...) test would catch accidental drops during future refactors. Optional.

10. scripts/start-custom-theme.ts:18 uses spawn(..., { shell: true }) with an array — works on bash/zsh, but shell: true with an args[] is platform-dependent on Windows cmd.exe quoting. Either pass a single command string or drop shell: true. Cosmetic.

11. branding-examples/test-theme.json sets avatar-background — currently a silent no-op until index.css adds the token. The intent is documented in color-tokens.ts:18-20; just confirming this is by design.


✅ Things done well

  • CSS-injection allowlist + value-pattern sanitization (UNSAFE_VALUE_PATTERN) is conservative and well-tested. Length cap is a nice belt-and-braces.
  • :root:not(.dark) selector specificity trick is correctly motivated by the comment at color-tokens.ts:105-109.
  • Base64 transport over additionalArguments correctly sidesteps shell-quoting concerns.
  • setTextContent (not innerHTML) on the <style> element preserves the no-eval guarantee on the renderer side.
  • Loader returns null on every failure mode and logs at warn — surfaces broken configs without blocking app boot. Good failure design.
  • ENOENT is a quiet path (silent fallback to defaults) while other read errors are loud. Matches operator expectations.
  • The chat/UI cleanup pass (replacing text-red-500 / dark:text-red-400 with text-destructive etc.) is consistent and the right direction.
  • Solid test coverage across color-tokens, load, and the preload arg parser. Tests are focused and easy to read.

Out of scope (acknowledged in the PR description)

  • macOS system-theme integration → [Task] Mac-native theme override #2267
  • Hot-reload (no fs.watch) → restart required is fine for MVP; mention in user docs when public.
  • Branding app_name / logo_url are parsed but not applied — fine as forward-compat.

| Branch: customize-colors-in-desktop-app

@github-actions github-actions Bot added size/XL and removed size/L labels May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants