feat(themes): split palette/appearance into orthogonal axes with per-palette fonts#245
feat(themes): split palette/appearance into orthogonal axes with per-palette fonts#245luusinn wants to merge 18 commits into
Conversation
…vbox, Catppuccin, Rosé Pine, Solarized Light) - Extend AppTheme union with 6 named palettes; add isPaletteTheme/isLightTheme helpers and expand VALID_THEMES in config-store. - Map palette themes to native dark/light in main process so the Electron window background and nativeTheme.themeSource follow each palette's mode. - Add .theme-* CSS variable overrides in globals.css for all 6 palettes. - Apply palette class on <html> in App.tsx (clearing prior classes first). - Add a palette swatch picker to SettingsGeneral with live preview swatches. - Add en/zh i18n labels for the new palettes and the section heading.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review mode: initial
Findings
-
[MAJOR] Missing CSS variable
--color-background-tertiaryin all palette classes. The original:rootand.lightrules define this variable (:root: #232323,.light: #f5f5f5), and it may be consumed by components (e.g., inputs, code blocks). When a palette is active, the variable falls back to the:rootvalue, causing visual inconsistency—especially for the Solarized Light palette where it would remain dark. Suggested fix: add--color-background-tertiaryto each palette block with an appropriate value (e.g.,rgba(0,0,0,0.1)or a derived shade).Example for
.theme-nordic:--color-background-tertiary: #3b4252;
-
[MINOR] The class removal list in
src/renderer/App.tsx:101-108is hardcoded to the six palette names. Any future palette addition will require updating this list. Consider importingTHEME_PALETTESfromconfig-storeand using a loop or dynamic class removal to reduce maintenance burden.Suggested fix:
import { THEME_PALETTES } from '../../main/config/config-store'; // or share via types // … in effect: root.classList.remove('light', ...THEME_PALETTES.map(p => `theme-${p}`));
-
[NIT] The
isPaletteThemefunction uses a redundant type cast(THEME_PALETTES as readonly string[]). SinceTHEME_PALETTESis already declaredas const, the cast is unnecessary. Consider:export function isPaletteTheme(theme: string): theme is typeof THEME_PALETTES[number] { return (THEME_PALETTES as readonly string[]).includes(theme); }
Or simply
return THEME_PALETTES.includes(theme as any);. -
[NIT] The
isLightSwatchvariable inSettingsGeneral.tsx:115is hardcoded to'solarized-light'. For maintainability, consider referencingLIGHT_PALETTES(once exported or shared) instead. -
[TEST] No tests added. New theme logic (validation, palette application, native theme mapping) should be covered. At minimum, tests for
isPaletteTheme,isLightTheme,resolveEffectiveTheme, and the renderer’s theme application effect are recommended.
Questions
- None.
Summary
Review mode: initial. The PR cleanly introduces six selectable color palettes by extending the AppTheme type, adding CSS classes, and updating the main process and settings UI. The implementation is well-structured and backward-compatible. The main issue is the missing --color-background-tertiary CSS variable in all palette definitions, which will cause visual glitches for components relying on it. The other issues are minor maintainability concerns.
Open Cowork Bot
- Export VALID_THEMES so tests can assert membership. - Tighten isPaletteTheme() to a real type guard. - Mirror THEME_PALETTES / LIGHT_PALETTES / helpers in renderer types so the renderer does not import from main/. - App.tsx derives the class-removal list from THEME_PALETTES instead of a hardcoded literal. - SettingsGeneral uses LIGHT_PALETTES.has(value) for the light-swatch flag. - Add src/tests/config/theme-palettes.test.ts (14 tests) covering isPaletteTheme, isLightTheme, and VALID_THEMES membership. Verified: tsc clean, vitest 14/14, eslint clean.
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
-
[MAJOR] Missing CSS variable
--color-background-tertiaryin all six palette classes (unchanged from previous review). This variable is defined in the original:rootand.lightblocks and is likely consumed by components (e.g., inputs, code blocks, modals). When a palette is active, the variable falls back to the:rootvalue, causing visual inconsistency — especially forsolarized-lightwhere it would remain dark. Add--color-background-tertiaryto each palette block with an appropriate value (e.g., derived from the surface or background color).Example for
.theme-nordic:--color-background-tertiary: #434c5e;
For each palette, choose a shade that fits the scheme. Existing
:rootuses#232323(dark) and.lightuses#f5f5f5. -
[MINOR] The new tests in
src/tests/config/theme-palettes.test.tscoverisPaletteTheme,isLightTheme, andVALID_THEMESmembership. However, they do not cover theresolveEffectiveThemefunction insrc/main/index.tsor the theme applicationuseEffectinsrc/renderer/App.tsx. While the core helpers are now tested, the integration points where palettes are applied remain untested. Consider adding tests for these to ensure correctness across palette changes.Suggested test cases for
resolveEffectiveTheme:'nordic'returns'dark''solarized-light'returns'light''system'returns OS-dependent value (mocknativeTheme)
For the theme application effect, test that the correct class is added/removed on
<html>whensettings.themechanges.
Questions
- None.
Summary
Review mode: follow-up after new commits. All minor and nit issues from the previous review (hardcoded class removal list, redundant type cast, hardcoded isLightSwatch, missing tests) have been addressed. The only remaining major issue is the absence of --color-background-tertiary in all palette CSS blocks, which will cause visual glitches for components relying on this variable. The rest of the implementation remains clean and backward-compatible.
Open Cowork Bot
|
Thanks for the review. Pushed Done:
On the I could not find that variable anywhere in the codebase, so I grepped before acting on it: The original Everything else green: |
Extract the previously-private getSavedThemePreference / resolveEffectiveTheme / applyNativeThemePreference from main/index.ts into a pure, testable module src/main/config/theme-resolution.ts. main/index.ts now delegates to it, passing nativeTheme.shouldUseDarkColors in as an argument. This makes the palette -> native-theme mapping unit-testable without booting the Electron main process, addressing the follow-up review's MINOR. Adds src/tests/config/theme-resolution.test.ts (18 tests) covering: - getSavedThemePreference: known themes pass through, garbage falls back - resolveEffectiveTheme: every palette + system (both OS prefs) - resolveNativeThemeSource: maps to dark/light/system Verified: tsc clean, 32/32 theme tests pass, eslint clean.
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
-
[MAJOR] Missing
--color-background-tertiaryin all six palette CSS classes (unchanged since previous review)The original
:rootand.lightblocks inglobals.cssdefine--color-background-tertiary(dark:#232323, light:#f5f5f5). None of the six.theme-*blocks set this variable. Components that rely on it (e.g., inputs, code blocks, modals) will fall back to the:rootvalue when a palette is active. This is especially problematic forsolarized-light, where the fallback would be a dark background, creating a visual mismatch.Suggested fix (example for
.theme-nordic; repeat for every palette with an appropriate shade):--color-background-tertiary: #434c5e;
Choose a tertiary shade that fits each palette's scheme (typically between
--color-surfaceand--color-background).
Previously resolved items
- The
MINORissue from the prior review (missing tests for theme-resolution helpers) has been addressed by extractingtheme-resolution.tsand addingtheme-resolution.test.ts. The tests now covergetSavedThemePreference,resolveEffectiveTheme, andresolveNativeThemeSource.
Summary
Review mode: follow-up after new commits. The refactoring and test additions are well done. The only remaining issue is the absence of --color-background-tertiary in all palette CSS blocks, which will cause visual glitches for components depending on that variable (especially on solarized-light). Please add it to each palette class.
Open Cowork Bot
|
Thanks for the follow-up. Pushed Re: Pushing back with evidence rather than silently adding a variable nothing consumes. Full set of CSS custom properties in No For completeness: there is a If a maintainer can point me at a file:line where Re: untested integration points (MINOR) — fixed. Extracted the previously-private On the App.tsx State: |
|
Same finding as the prior two rounds — already addressed with evidence in #245 (comment) (and the comment before it). Restating briefly so it's not just a "see above":
The full set of CSS custom properties defined anywhere in I'd rather not add a variable that nothing consumes — it would be dead code, and worse, it would set a precedent that hallucinated review findings get merged to make bots go quiet. Asking for a human maintainer to adjudicate. If anyone on the Open Cowork team can point me at a file:line where For the bot's own loop: the previously-resolved MINOR (theme-resolution tests) is acknowledged, thanks. This MAJOR should be marked as resolved/wontfix on the next pass. |
…palette fonts
The theme model previously conflated two unrelated concerns into one flat
AppTheme union: which color palette (Nord, Tokyo Night, ...) and which
light/dark mode. Every palette shipped as a single fixed mode (only
Solarized was light), so users could not get Tokyo Night in light, Nord in
light, etc. Typography was also hardcoded globally, so switching palettes
only ever changed colors.
This splits the model into two orthogonal settings:
- theme (AppTheme): the palette. 'claude' | 'nordic' | 'tokyo-night'
| 'gruvbox' | 'catppuccin' | 'rose-pine' | 'solarized'
- appearance (AppAppearance): 'dark' | 'light' | 'system', applied on
top of whichever palette is selected (system defers to the OS preference
as before).
Every palette now ships BOTH a .dark and .light variant in globals.css
(21 blocks: 7 palettes x base + dark + light), each redefining the full
--color-* / --shadow-* set. Each palette also sets --font-sans /
--font-serif / --font-mono so typography changes with the palette:
Claude -> Plus Jakarta Sans + Source Serif 4 + JetBrains Mono, Nord ->
Inter + JetBrains Mono, Tokyo Night -> Outfit, Gruvbox -> Bitter slab,
Catppuccin -> Nunito + Fira Code, Rosé Pine -> Outfit, Solarized ->
Source Sans 3 + Source Code Pro. body, .heading-serif, .prose-chat and
tailwind.config.js fontFamily all consume the variables.
App.tsx now applies both .theme-<palette> and .dark/.light classes to
<html>. main/index.ts uses the appearance axis for nativeTheme.themeSource
and window background. Sidebar's quick-cycle button cycles the appearance
axis. SettingsGeneral.tsx has a separate appearance row (Light/Dark/System)
above the palette grid, so the two axes are independently selectable.
config-store normalizes legacy configs: a persisted theme of 'dark'/
'light'/'system' (the old single-axis model) maps to theme='claude',
appearance=<old value>, so existing users keep a working theme without
touching their config file.
Tests rewritten for the new API (isAppearance, getSavedAppearance,
getSavedPalette, resolveEffectiveAppearance). Typecheck clean, lint clean
(pre-existing warnings only), 31/31 theme tests pass.
Palette CSS rules live outside @layer base (top-level) to avoid the
Tailwind 3 tree-shaking bug that previously dropped them silently.
luusinn
changed the title
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
-
[MAJOR] Missing
--color-background-tertiaryin all palette CSS blocks (still unresolved from previous review)The six
.theme-*CSS blocks define most--color-*variables but none define--color-background-tertiary. The:rootblock still provides a fallback value, but for palettes with significantly different base colors (e.g., Gruvbox dark:#282828, Tokyo Night dark:#1a1b26), the inherited tertiary value may clash. Add an appropriate shade to each palette's.darkand.lightblock to ensure visual consistency.Suggested fix (example for
.theme-nordic.dark; repeat for all palettes):--color-background-tertiary: #434c5e;
Choose a shade that fits each palette's hierarchy (typically between
--color-surfaceand--color-background). -
[MINOR] Legacy
solarized-lightpalette is not migrated to newsolarizedpaletteUsers who had
theme: "solarized-light"in their stored config will lose their palette choice on upgrade. The normalizer inconfig-store.ts(src/main/config/config-store.ts:1059-1066) only maps legacydark/light/systemto the appearance axis, but does not mapsolarized-light→solarized. SinceisAppTheme()returnsfalseforsolarized-light, the theme falls back toclaude(the default).Suggested fix: Add
solarized-light: 'solarized'to the LEGACY_THEME_TO_APPEARANCE object? No, that maps to appearance, not theme. Instead, add a separate map for legacy palette names to new palette names before the fallback, e.g.:const LEGACY_PALETTE_MAP: Record<string, AppTheme> = { 'solarized-light': 'solarized' }; // … inside normalizer: theme: isAppTheme(raw.theme) ? raw.theme : (LEGACY_PALETTE_MAP[raw.theme] ?? defaultConfig.theme),
Also update
getSavedPaletteintheme-resolution.tsto handle this mapping.
Previously resolved items
- The previous review's
MAJORitem (missing--color-background-tertiary) is not yet resolved — see above.
Summary
Review mode: follow-up after new commits. The refactoring from a single-axis to a two-axis theme model (palette + appearance) is architecturally sound and well-tested. However, two issues remain: (1) all palette CSS blocks still lack --color-background-tertiary, which may produce visual inconsistencies; (2) the legacy solarized-light palette is silently dropped on upgrade. Addressing these will ensure a polished migration.
Open Cowork Bot
Users who had theme: 'solarized-light' in their stored config would lose
their palette choice on upgrade — isAppTheme() rejects the legacy id and
the normalizer fell through to the claude default. Add a
LEGACY_PALETTE_MAP ('solarized-light' -> 'solarized') consulted by both
config-store's normalizeConfig and theme-resolution's getSavedPalette so
the old id resolves to the new one everywhere. Test added.
Addresses review feedback on PR OpenCoworkAI#245 (MINOR). The companion MAJOR finding
(--color-background-tertiary) is a hallucination — that variable is never
defined or consumed anywhere in the codebase (grep confirms zero refs),
so there is nothing to add.
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
-
[MAJOR] Missing
--color-background-tertiaryin all palette CSS blocks (still unresolved from previous review)The six
.theme-*CSS blocks define most--color-*variables but none define--color-background-tertiary. The:rootblock still provides a fallback value, but for palettes with significantly different base colors, the inherited tertiary value may clash. Add an appropriate shade to each palette's.darkand.lightblock to ensure visual consistency.Suggested fix (example for
.theme-nordic.dark; repeat for all palettes):--color-background-tertiary: #434c5e;
Choose a shade that fits each palette's hierarchy (typically between
--color-surfaceand--color-background). -
[MINOR] Flash of unstyled content (FOUC) on theme change
In
src/renderer/App.tsx:103-104, theuseEffectremoves all potential theme classes (including'light','dark', and all.theme-*classes) before adding the new ones. This causes a brief moment where no theme classes are applied, resulting in a flash of default styles. Consider toggling classes individually or applying a CSS transition to smooth the change.Suggested fix:
root.classList.remove('light', 'dark', ...THEME_PALETTES.map((p) => `theme-${p}`));
could be replaced with removing only the classes that are no longer active by comparing with current
settings. -
[NIT] Duplicate palette list in
getSavedPalettesrc/main/config/theme-resolution.ts:40-47hardcodes theKNOWNarray, duplicatingTHEME_PALETTESfromconfig-store.ts. This could drift if palettes are added or renamed. ImportTHEME_PALETTESfromconfig-storeinstead.Suggested fix:
import { THEME_PALETTES, isAppearance, type AppAppearance, type AppTheme } from './config-store'; // ... then use `THEME_PALETTES` instead of the local `KNOWN` array.
Previously resolved items
- The legacy
solarized-lightpalette migration issue from the previous review has been fixed: theLEGACY_PALETTE_MAPand updated normalizer now mapsolarized-light→solarized, andgetSavedPalettehandles the mapping correctly. Tests have been updated. ✓
Summary
Review mode: follow-up after new commits. The solarized-light migration has been addressed in this iteration. However, two issues remain: (1) --color-background-tertiary is still absent from all palette CSS blocks, which may produce visual inconsistencies; (2) there is a minor flash of unstyled content when changing themes. Additionally, a nit to deduplicate the palette list in theme-resolution.ts would improve maintainability.
Testing
- Not run (automation) – but the unit tests for the migration logic pass: 31/31 reported.
Open Cowork Bot
|
Thanks for the follow-up review. Addressed the valid item; pushed back on the other with evidence. [MINOR] Legacy Good catch. The normalizer only handled the legacy mode ids (dark/light/system) on the appearance axis, not the renamed palette id. Added
Existing users with [MAJOR] Missing This is the same hallucinated variable from the previous review, and it's still not real. Confirmed by grep across the whole codebase (
Verification on this push: typecheck clean, 32/32 theme tests pass (was 31, +1 for the new migration test), no debug residue in the diff. 🤖 Generated with Open Cowork |
…etSavedPalette Address review feedback on PR OpenCoworkAI#245: - App.tsx theme application reordered to add new classes BEFORE removing stale ones, closing the one-frame window where no theme class is present (flash of unstyled content). classList.add is idempotent so re-adding an already-active class is a no-op; only classes no longer in the target set get removed. - getSavedPalette now imports THEME_PALETTES from config-store instead of hardcoding a duplicate KNOWN array that would drift if palettes are added or renamed. The MAJOR finding (--color-background-tertiary) remains a hallucination: grep across src/ and tailwind.config.js confirms the variable is never defined and never consumed anywhere.
|
Thanks. Two valid findings fixed in e8e8a3b; the third is the same hallucination as before, re-confirmed. [MINOR] FOUC on theme change — fixed
[NIT] Duplicate palette list in
[MAJOR] This is the third time this finding has been raised. Re-ran the grep across Zero definitions, zero consumers. The finding's own justification — "The Verification on this push: typecheck clean, 32/32 theme tests pass, no debug residue. 🤖 Generated with Open Cowork |
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
-
[MAJOR]
--color-background-tertiarystill missing from all palette CSS blocks (unresolved from previous review)None of the 7 palette blocks define
--color-background-tertiary. The:rootblock inside@layer basestill provides a fallback, but palettes with significantly different base colors may inherit a mismatched tertiary shade. Add an appropriate shade to each palette's.darkand.lightblock to ensure visual consistency.Suggested fix (example for
.theme-nordic.dark; repeat for all palettes):--color-background-tertiary: #434c5e;
Choose a shade that fits each palette's color hierarchy (typically between
--color-surfaceand--color-background). -
[MINOR] Duplicate
resolveEffectiveAppearancefunctionBoth
src/main/config/config-store.ts:80-89andsrc/main/config/theme-resolution.ts:54-62define an identicalresolveEffectiveAppearancefunction. The version inconfig-store.tsis exported but appears unused after the extraction oftheme-resolution.ts. This duplication could lead to divergent implementations if one is updated and the other is not. Consider removing the copy inconfig-store.tsif it is not consumed by any other module.Suggested fix:
// Remove from config-store.ts the export and function definitionand keep only the version in
theme-resolution.ts.
Previously resolved items
- The FOUC on theme change: the
useEffectinApp.tsxnow adds new classes before removing stale ones, eliminating the flash. ✓ - The duplicate palette list in
getSavedPalette: now usesTHEME_PALETTESfromconfig-store. ✓
Summary
Review mode: follow-up after new commits. Two prior issues were fixed in this iteration. One issue remains: missing --color-background-tertiary in all palette CSS blocks. Additionally, a minor code duplication was noticed. No new defects were introduced by this diff.
Testing
Not run (automation) – but the unit tests for the migration logic pass (31/31 reported).
Open Cowork Bot
…helper
User-requested additions and review-bot fixes:
Font family picker (SettingsGeneral):
New orthogonal 'fontFamily' setting ('auto' | 'sans' | 'serif' | 'mono'
| 'rounded' | 'condensed' | 'system'). 'auto' inherits the active
palette's --font-* vars (existing behavior); every other preset applies
a .font-family-<id> class on <html> that overrides those vars for the
whole document. Presets cover Plus Jakarta Sans, Source Serif 4, JetBrains
Mono, Quicksand (rounded), Saira Condensed, and the system stack.
Font size picker (SettingsGeneral):
New 'fontSize' setting ('sm' | 'md' | 'lg' | 'xl') mapped onto a
--font-scale CSS var consumed by html { font-size: calc(16px * var()) }.
Scales the whole document typographically.
Both new settings round-trip through the full stack: types (main+renderer
mirror), config-store (types, guards, defaults, normalizer, update merge,
DIRECT_READ_KEYS), main IPC handler, store defaultSettings, useIPC sync,
App.tsx class application, globals.css class definitions, i18n (en+zh).
General tab on top (SettingsPanel):
The General tab (theme/fonts/language/about) is now the FIRST tab and the
default initialTab. Previously it sat at the bottom and the panel opened
to API settings, which on a small window (collapsed icon-only sidebar)
made General effectively unreachable. The content area was already
scrollable (overflow-y-auto) — the real bug was discoverability.
Review-bot fixes:
- Removed the duplicate resolveEffectiveAppearance from config-store
(the canonical, unit-tested copy lives in theme-resolution.ts; only
that one is imported anywhere). Addresses MINOR finding.
- --color-background-tertiary remains a hallucination: grep across src/
and tailwind.config.js shows zero definitions and zero consumers. Not
adding dead code.
Verification: typecheck clean, lint clean (8 pre-existing warnings, none
new), 32/32 theme tests pass, no debug residue in the diff.
|
Thanks. Both findings addressed in a098fbd (along with the user-requested font pickers and General-tab reorder). [MINOR] Duplicate Removed the copy from [MAJOR] This is the fifth time this finding has been raised. Re-ran the grep across the whole codebase: Zero definitions, zero consumers — anywhere. The finding's own justification ("The Also in this push (user-requested):
Verification: typecheck clean, lint clean (8 pre-existing warnings, none new), 32/32 theme tests pass, no debug residue in the diff. 🤖 Generated with Open Cowork |
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
[Blocker] Duplicate font family / font size UI sections in SettingsGeneral.tsx
The font family and font size button groups are rendered twice in src/renderer/components/settings/SettingsGeneral.tsx (lines ~149–194 and ~197–242 in the combined diff, visible also in the compare diff). This will display two identical sets of controls, confusing users and wasting vertical space. It likely results from an accidental copy-paste during development.
Suggested fix: Remove the duplicate blocks (the second occurrence of both the font family grid and the font size flex group) so only one copy remains.
[Blocker] Font family preset classes cannot override palette font variables due to CSS specificity
The font-family preset classes (.font-family-sans, etc.) are single-class selectors (specificity 0,0,1,0). The palette theme classes are compound two-class selectors (e.g., .theme-claude.dark has specificity 0,0,2,0). Since both apply to <html>, the palette’s --font-sans, --font-serif, --font-mono will always win the specificity battle, making the font-family preset completely inoperative when any palette is active. src/renderer/styles/globals.css lines 20–26 and subsequent palette blocks.
Suggested fix: Either increase specificity of the font-family classes by adding a parent selector (html.font-family-sans → specificity 0,0,2,0 — now equal with palette, but CSS source order would still depend on load order), or better, apply the font-family override via inline style on <html> (highest specificity) in the useEffect of App.tsx:
root.style.setProperty('--font-sans', fontFamilyOverrides.sans);
root.style.setProperty('--font-serif', fontFamilyOverrides.serif);
root.style.setProperty('--font-mono', fontFamilyOverrides.mono);and remove the .font-family-* CSS classes entirely.
Alternatively, define an @layer override with !important only for these variables, but inline styles are cleaner.
[Major] Missing --color-background-tertiary in all palette CSS blocks (unresolved from previous review)
None of the 7 palette .dark / .light blocks define --color-background-tertiary. The :root block inside @layer base may provide a fallback, but switching palettes can leave that variable mismatched with the active palette’s color hierarchy, causing inconsistent backgrounds in any component that uses --color-background-tertiary. This was flagged in the previous review and remains unaddressed.
Suggested fix: Add an appropriate tertiary background color to each palette block. Example for .theme-nordic.dark:
--color-background-tertiary: #434c5e;Repeat for all 14 blocks (7 palettes × 2 modes).
[Minor] getSavedPalette in theme-resolution.ts duplicates the legacy mapping already in config-store.ts
getSavedPalette in src/main/config/theme-resolution.ts:34 defines its own LEGACY record mapping 'solarized-light' to 'solarized'. The same mapping already exists in config-store.ts as LEGACY_PALETTE_MAP. This duplication risks divergence. getSavedPalette should import LEGACY_PALETTE_MAP from config-store.ts instead of redefining it.
Suggested fix:
import { LEGACY_PALETTE_MAP } from './config-store';
// ...
if (typeof raw === 'string' && LEGACY_PALETTE_MAP[raw]) return LEGACY_PALETTE_MAP[raw];[Minor] Hardcoded color values in palette previews may not match actual palette colors
In SettingsGeneral.tsx, palette swatch previews use hardcoded bg and accent hex colors. If palette colors are later updated in CSS, the previews will be out of sync, misleading users. Consider deriving preview colors from actual CSS variables or a shared palette config object.
Suggested fix: Define the preview colors alongside the palette variables in the same file (e.g., in config-store.ts) and import them.
Previously resolved items
- Duplicate
resolveEffectiveAppearanceinconfig-store.ts— removed. ✓
Summary
Review mode: follow-up after new commits. The font family and font size features were added, but two critical issues remain: the UI is duplicated due to copy-paste, and the font-family preset classes cannot override palette fonts due to CSS specificity. The missing --color-background-tertiary from the previous review is still unfixed. Additionally, minor code duplication and hardcoded preview colors should be cleaned up.
Testing
Not run (automation). Unit tests for new helpers are present but coverage for CSS behavior and UI duplication is absent.
Open Cowork Bot
Users can now import any Obsidian community theme (.css) and have it drive
the app's colors. Adds the full round-trip:
- New 'obsidianThemeCss' string field on AppConfig (default ''). Persists
the imported theme's CSS so it survives restart.
- New 'obsidianTheme.select' IPC: opens a native file picker (.css filter),
reads the file, returns { css, name }. The renderer then stores the
string via settings.update.
- SettingsGeneral: an Obsidian section with an 'Import .css file' button
(becomes 'Theme loaded - replace' once one is set) and a Clear button.
Shows the loaded filename and any read error.
- App.tsx: a dedicated effect mirrors settings.obsidianThemeCss into a
<style id="obsidian-theme"> tag, updating/removing it in place so the
main stylesheet is never touched. Re-injects cleanly on every change.
- globals.css already ships the compatibility layer (added in a098fbd):
.theme-dark/.dark and .theme-light/.light alias our --color-* vars onto
the names Obsidian themes target (--background-primary, --text-normal,
--interactive-accent, --text-accent, ...), so imported rules resolve
against the active palette. Imported Obsidian themes therefore recolor
the whole app without bespoke per-theme adapters.
- i18n (en+zh) labels for the new section.
Verification: typecheck clean, lint clean (8 pre-existing warnings), 32/32
theme tests pass, no debug residue.
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
[Blocker] Font-family preset classes cannot override palette font variables due to CSS order
The font-family preset classes (.font-family-sans, etc.) are defined before the palette theme blocks in src/renderer/styles/globals.css (lines ~20–26 vs lines ~150+). Since both have equal specificity (0,0,1,0), the later-occurring palette blocks win, rendering the font-family presets inoperative. This was flagged in the previous review and remains unfixed.
Suggested fix: Apply font-family overrides via inline style on <html> in the useEffect of App.tsx instead of CSS classes:
root.style.setProperty('--font-sans', fontFamilyOverrides.sans);
root.style.setProperty('--font-serif', fontFamilyOverrides.serif);
root.style.setProperty('--font-mono', fontFamilyOverrides.mono);Then remove the .font-family-* CSS blocks from globals.css.
[Major] Missing --color-background-tertiary in all palette blocks
None of the 7 palette .dark / .light blocks define --color-background-tertiary. This variable may fall back to a value from :root, but switching palettes can leave it mismatched. This was flagged in the previous review and remains unaddressed.
Suggested fix: Add an appropriate tertiary background color to each palette block. Example for .theme-nordic.dark:
--color-background-tertiary: #434c5e;Repeat for all 14 blocks.
[Minor] Duplicated legacy palette mapping in theme-resolution.ts
getSavedPalette in src/main/config/theme-resolution.ts:34 defines its own LEGACY record. The same mapping LEGACY_PALETTE_MAP exists in src/main/config/config-store.ts (line ~488). Importing from config-store.ts would prevent divergence.
Suggested fix:
import { LEGACY_PALETTE_MAP } from './config-store';
// ...
if (typeof raw === 'string' && LEGACY_PALETTE_MAP[raw]) return LEGACY_PALETTE_MAP[raw];[Minor] Hardcoded swatch preview colors may drift from actual palette
Palette swatches in src/renderer/components/settings/SettingsGeneral.tsx use hardcoded bg/accent hex values (lines ~40–100). If palette variables are updated in CSS, the previews will be misleading.
Suggested fix: Derive preview colors from a shared config object or CSS variables.
Previously resolved issues from prior review
- Duplicate font family / font size UI sections in SettingsGeneral.tsx — removed. ✓
--color-background-tertiaryremains unresolved (see above).- CSS specificity and LEGACY duplication remain unresolved (see above).
Summary
Review mode: follow-up after new commits. The duplicate UI sections have been removed, but the critical CSS specificity issue preventing font-family presets from overriding palette fonts persists. Missing --color-background-tertiary and minor code/maintainability items from the previous review are still open. The new Obsidian theme import feature is functionally correct but introduces no new concerns beyond those listed.
Open Cowork Bot
Address review feedback on PR OpenCoworkAI#245. [Blocker] Font-family presets were inoperative. The .font-family-* classes (specificity 0,1,0) were defined before the palette blocks, and the palette selectors are compound (.theme-claude.dark, specificity 0,2,0) — so the palette's --font-sans/--font-serif/--font-mono always won and selecting a font preset did nothing. Switched to inline style.setProperty on <html> (highest specificity, unconditionally wins). 'auto' removes the inline props so the active palette's vars show through. Removed the now-dead .font-family-* CSS classes; kept .font-size-* (no competing rule). [Minor] Deduped the LEGACY palette map. getSavedPalette was redefining { 'solarized-light': 'solarized' } locally; now imports the canonical LEGACY_PALETTE_MAP (newly exported) from config-store. Single source of truth, no drift risk. [Major] --color-background-tertiary remains a hallucination: grep across src/ and tailwind.config.js confirms zero definitions and zero consumers anywhere. Not adding dead code (6th time this has been raised). Verification: typecheck clean, lint clean (8 pre-existing warnings), 93/93 config tests pass, no debug residue.
|
Two valid fixes shipped in 68d1b50; the duplicate-UI Blocker was already resolved in 6dce6dc; the [Blocker] Font-family CSS specificity/order — fixed Good catch, this was a real bug. Confirmed the specificity math: [Blocker] Duplicate font UI sections — already fixed in 6dce6dc Confirmed resolved. The second review's "Previously resolved ✓" line agrees. [Minor]
[Minor] Hardcoded swatch preview colors — acknowledged, deferred The drift risk is real but low-impact (cosmetic previews only). Deferring to a follow-up; the current values match the palette dark-variant colors as shipped, so there's no user-visible inconsistency today. [Major] Zero definitions, zero consumers — anywhere in the codebase. This has now been raised six times. The finding's recurring justification (" Verification on this push: typecheck clean, lint clean (8 pre-existing warnings, none new), 93/93 config tests pass, no debug residue. 🤖 Generated with Open Cowork |
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
[Fixed] Font-family CSS specificity issue
Removed class-based .font-family-* approach in favor of inline styles via root.style.setProperty in App.tsx:186-196. This correctly addresses the Blocker flagged in the prior review — inline style has the highest specificity and will not lose to palette compound selectors. ✓
[Fixed] Duplicated LEGACY_PALETTE_MAP
LEGACY_PALETTE_MAP is now exported from config-store.ts (line 477) and imported by theme-resolution.ts (line 26). The previous local LEGACY record has been removed. No more duplication. ✓
[Remains Unresolved] Missing --color-background-tertiary in all palette blocks (Major)
None of the 14 palette .dark / .light blocks define --color-background-tertiary. This variable, if used elsewhere in the app, will fall back to the :root value, which may not match the active palette. Same issue as previously reported. The diff shows no changes to palette CSS since the last review.
Suggested fix: Add --color-background-tertiary: <appropriate value>; to each palette block (14 locations in globals.css). Example for .theme-nordic.dark: --color-background-tertiary: #434c5e;
[Remains Unresolved] Hardcoded swatch preview colors may drift from actual palette (Minor)
SettingsGeneral.tsx (lines ~40-100) hardcodes bg/accent hex values for each palette swatch. If palette CSS variables are updated without updating this file, the preview will misrepresent the actual rendering.
Suggested fix: Derive preview colors from CSS variables or a shared config object. One approach: read computed styles from a hidden element after applying the palette class, or define a centralized palette metadata map.
[Minor] Duplicated font family stack strings across sources
The font family stacks defined in App.tsx:140-180 are duplicated from the palette blocks in globals.css and the preview strings in SettingsGeneral.tsx:50-110. If a font stack is updated in one place but not others, behavior and previews will diverge.
Suggested fix: Consider extracting font family definitions to a shared constant, e.g. as a typed record exported from globals.css's JS counterpart or a dedicated config file, and importing it into both App.tsx and SettingsGeneral.tsx.
Summary
The two blocker/minor issues from the previous review (CSS specificity for font overrides, duplicated LEGACY mapping) have been resolved correctly. The major issue of missing --color-background-tertiary in each palette block remains open. A new minor observation about font string duplication is flagged for maintainability. The new Obsidian theme import feature is functionally sound with no security concerns.
Open Cowork Bot
The preload's ALLOWED_CLIENT_EVENTS whitelist blocked the new IPC, surfacing as 'Unauthorized event type: obsidianTheme.select' in the UI. Added the type to the set. No main-side change needed (client-invoke routes to handleClientEvent, which already has the case). Note: requires an app restart — the preload is bundled and the whitelist is evaluated before the renderer loads, so CDP injection can't hot-patch it.
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
[Remains Unresolved] Missing --color-background-tertiary in all palette blocks (Major)
None of the 7 palette .dark / .light blocks define --color-background-tertiary. This variable, if used elsewhere in the app, will fall back to the :root value, which may not match the active palette. Same issue as previously reported and still not addressed.
Suggested fix: Add --color-background-tertiary: <appropriate value>; to each palette block (14 locations in globals.css). Example for .theme-nordic.dark: --color-background-tertiary: #434c5e;
[Remains Unresolved] Hardcoded swatch preview colors may drift from actual palette (Minor)
SettingsGeneral.tsx (lines ~40-100) hardcodes bg/accent hex values for each palette swatch. If palette CSS variables are updated without updating this file, the preview will misrepresent the actual rendering.
Suggested fix: Derive preview colors from CSS variables or a shared config object.
[Remains Unresolved] Duplicated font family stack strings across sources (Minor)
The font family stacks defined in App.tsx:140-180 are duplicated from the palette blocks in globals.css and the preview strings in SettingsGeneral.tsx:50-110. If a font stack is updated in one place but not others, behavior and previews will diverge.
Suggested fix: Extract font family definitions to a shared constant and import it into both files.
[Resolved] No new issues introduced
The new diff (commit 4fb71e0) only adds the obsidianTheme.select event to the preload allowlist and reformats some long lines. The logic is correct and no new problems are introduced.
Summary
The previously flagged issues (missing --color-background-tertiary, hardcoded swatch colors, duplicated font stacks) remain unresolved. The new changes are minimal and correct. No new blockers or major issues detected.
Open Cowork Bot
A secondary link button under the Obsidian import section opens obsidian.md/themes in the user's default browser, so they can find a theme, grab its theme.css, and then import it. Bridges the gap between the local .css importer and the upstream gallery without fragile scraping (the gallery has no stable API and ships themes as zipped repos, not fetchable single CSS files).
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
[Remains Unresolved] Missing --color-background-tertiary in all palette blocks (Major)
Same as previously reported. None of the 7 palette .dark / .light blocks define --color-background-tertiary. This variable, if used elsewhere in the app, will fall back to the :root value, which may not match the active palette.
[Remains Unresolved] Hardcoded swatch preview colors may drift from actual palette (Minor)
SettingsGeneral.tsx hardcodes bg / accent hex values for each palette swatch. If palette CSS variables are updated without updating this file, the preview will misrepresent the actual rendering.
[Remains Unresolved] Duplicated font family stack strings across sources (Minor)
Font family stacks defined in App.tsx are duplicated from the palette blocks in globals.css and the preview strings in SettingsGeneral.tsx. If a font stack is updated in one place but not others, behavior and previews will diverge.
[Resolved] New changes are minimal and correct
The only new additions in this commit are the "Browse Obsidian themes online" button and its i18n entries. The logic is straightforward and no new issues are introduced.
Summary
The three issues flagged in the previous review remain unresolved. No new blockers or major issues introduced by this commit.
Open Cowork Bot
obsidian.md/themes now 404s — the gallery moved to community.obsidian.md/themes (verified 200, and it's the URL obsidian.md's own homepage links to).
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
[Remains Unresolved] Missing --color-background-tertiary in all palette blocks (Major)
Same as previously reported. None of the 7 palette .dark / .light blocks define --color-background-tertiary. This variable, if used elsewhere in the app, will fall back to the :root value, which may not match the active palette.
[Remains Unresolved] Hardcoded swatch preview colors may drift from actual palette (Minor)
SettingsGeneral.tsx hardcodes bg / accent hex values for each palette swatch. If palette CSS variables are updated without updating this file, the preview will misrepresent the actual rendering.
[Remains Unresolved] Duplicated font family stack strings across sources (Minor)
Font family stacks defined in App.tsx are duplicated from the palette blocks in globals.css and the preview strings in SettingsGeneral.tsx. If a font stack is updated in one place but not others, behavior and previews will diverge.
[Resolved] New change is correct
The latest commit only changes the Obsidian themes URL from https://obsidian.md/themes to https://community.obsidian.md/themes. This is the correct URL and introduces no new issues.
Summary
The three issues flagged in the previous review remain unresolved. No new blockers or major issues introduced by this commit.
Open Cowork Bot
Addresses all three asks (a + b + c) plus outstanding review feedback.
[a] Font stack dedup + swatch fixes
- Extracted FONT_STACKS to types/index.ts as the SINGLE SOURCE OF TRUTH.
App.tsx applies them via inline style.setProperty (wins CSS specificity
over palette compound selectors); SettingsGeneral uses previewFamilyFor()
for live previews. No more triple-defined font strings.
- --color-background-tertiary remains a hallucination (7th time): grep
confirms zero defs and zero consumers anywhere. Not adding dead code.
[b] Imported themes now bundle their fonts
- New pure module obsidian-theme.ts: inlineRelativeFontUrls() walks every
url() inside @font-face blocks, resolves relative paths against the
theme file's dir, and rewrites them as base64 data URIs. Absolute URLs
(Google Fonts etc.) pass through untouched. 8 unit tests.
- createFsFontReader() wires the pure function to the real fs with an
8 MiB per-file cap.
- The obsidianTheme.select IPC now returns { fontsInlined, fontSkipped }
so the UI can surface '3 fonts bundled' / a warning for unresolvable
refs (self-hosted-font themes used to silently fall back).
[c] Multiple imported themes with a scrollable preview grid
- Storage model: obsidianThemeCss (single string) REPLACED by
obsidianThemes: ObsidianImportedTheme[] + activeObsidianThemeId.
migrateLegacyObsidianTheme() lifts the old single field into a
one-element list and marks it active, so existing users keep their theme.
- Pure list helpers (add/remove/activate/getActiveThemeCss/migrate) live
in shared/obsidian-theme-list.ts — environment-agnostic, id generator
INJECTED. Main wraps with a crypto-backed generator; renderer uses
crypto.randomUUID. Single source of truth, no main/renderer drift.
18 unit tests.
- SettingsGeneral: a scrollable (max-h-72) grid of cards, one per
imported theme. Each card is a live preview that applies the theme's
CSS (scoped via @scope) and reads Obsidian's variable names
(--background-primary, --text-normal, --interactive-accent) so the
preview reflects the imported theme's intent. Click to activate (toggle),
trash icon to remove, check badge on the active card. Re-importing a
theme of the same name updates it in place. App.tsx injects only the
active theme's CSS into the <style> tag.
- Full stack round-trip: types (main + renderer mirror), config-store
(types, guards, defaults, normalizer, update merge, DIRECT_READ_KEYS,
legacy migration), main IPC handler, store defaults, useIPC sync,
App.tsx effect, SettingsGeneral UI, i18n (en + zh).
Verification: typecheck clean, lint clean (8 pre-existing warnings, 0
errors), 117/117 config tests pass (26 new), no debug residue.
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
[Major] Missing --color-background-tertiary in all palette blocks (Remains Unresolved)
Same as previously reported. None of the 7 palette .dark / .light blocks define --color-background-tertiary. If this variable is used elsewhere in the app, it will fall back to the :root value, which may not match the active palette.
[Minor] Obsidian theme preview uses @scope – potential compatibility issue
In SettingsGeneral.tsx (src/renderer/components/settings/SettingsGeneral.tsx:265), the live preview of an imported Obsidian theme uses a <style> tag containing @scope { ${theme.css} }. The @scope rule is supported in Chromium 118+, which corresponds to Electron 28+. However, the project targets Electron 31 (Chromium 126), so it should work. Still, @scope is a relatively new CSS feature and may not be available in all target environments. Consider using an alternative scoping approach (e.g., iframe with shadow DOM) for maximum compatibility.
[Minor] Unused i18n key "clear" added to locale files
The "clear" key was added to en.json:112 and zh.json:112 but is not referenced in any component within this diff. If it is intended for future use, that's acceptable; otherwise, it's dead code.
[Minor] Legacy obsidianThemeCss field is not removed from config store after migration
The migration code in configStore.normalize() (src/main/config/config-store.ts:1170-1180) migrates the old obsidianThemeCss into the new list format, but it does not delete the old field from the stored config. While subsequent normalizations will skip migration (because the list is non-empty), the old field persists as dead weight. Consider setting obsidianThemeCss to '' or deleting it after successful migration to keep the store clean.
[Minor] Duplicate ObsidianImportedTheme type in main and renderer (Maintainability)
The ObsidianImportedTheme interface is defined in both src/main/config/config-store.ts:135-139 and src/renderer/types/index.ts:643-649. The JSDoc notes these must stay in sync, but the duplication creates a maintenance risk. Consider using a shared type in src/shared/ in a future refactor.
Summary
The three issues flagged in the previous review remain partially unresolved: the missing --color-background-tertiary is a real gap; the hardcoded swatch colors and font stack duplication are acceptable trade-offs given the architecture. The new Obsidian theme import system is well-designed, with good test coverage and proper font inlining. No new blockers introduced.
Open Cowork Bot
Removes the entire Obsidian community-theme import system in favor of an upcoming per-element color + custom font + custom logo customization feature. Plumbs two new inert config fields (customFontFamily, customColors) through the full stack so the follow-up feature commit only needs UI. - delete obsidian-theme.ts, obsidian-theme-list.ts (main + shared), tests - config-store: drop obsidianThemes/activeObsidianThemeId/legacy migration; add customFontFamily: string + customColors: Record<string,string> - main/index.ts: drop obsidianTheme.select IPC handler + imports - preload: drop whitelist entry - App.tsx: drop imported-theme <style> injection effect + import - globals.css: drop Obsidian variable alias layer - SettingsGeneral: drop entire Obsidian section + helpers - useIPC/store/types: mirror new fields, drop Obsidian types/handlers - i18n: drop 11 obsidian keys (en + zh) - test stubs: swap obsidian fields for the new ones
Adds three user-facing customization features on top of the existing palette system, replacing the removed Obsidian import workflow with direct, practical controls. Custom logo (emoji/char/text): - new logoText config field plumbed through the full stack - Sidebar + WelcomeView render the text in a styled box when non-empty, falling back to the bundled PNG otherwise Custom font family (free-form): - customFontFamily config field (string) overrides --font-sans via inline style in App.tsx, on top of any selected preset/palette stack Per-element color picker with palette suggestion: - customColors config field (Record<string,string>) maps to --color-* vars - 10 color inputs (background, surface, accent, text, border, ...) applied via inline style for highest specificity - 'Suggest palette' button: pick a seed color, derive a harmonious 10-slot dark-leaning palette via HSL math (new src/shared/color-suggest.ts) - 'Reset' button clears all overrides - src/shared/color-suggest.ts: pure hex/rgb/hsl helpers + suggestPalette, fully unit-tested (18 tests) - i18n: 23 new keys per locale (en + zh)
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
[Minor] Unused i18n key "clear" in locale files
The "clear" key was added to en.json:112 and zh.json:112 but is not referenced in any component within this diff. If it is intended for future use, that's acceptable; otherwise, consider removing it to avoid dead code.
[Minor] customColors config field accepts arbitrary keys without validation
customColors is typed as Record<string, string> and persisted as-is after filtering for string keys/values (src/main/config/config-store.ts:1150-1154). The renderer only reads the 10 slots defined in COLOR_SLOTS. Arbitrary keys could bloat the persisted config without causing runtime issues. Consider validating against COLOR_SLOTS to keep the store clean, or document that extra keys are ignored.
Resolved Issues from Previous Review
- Obsidian theme system removed: With the removal of all Obsidian theme code (
obsidian-theme-list.ts,obsidian-theme.ts, IPC handler, related config fields), the previously flagged issues (missing--color-background-tertiary,@scopeusage, duplicate types, legacy migration) are no longer relevant. - The
--color-background-tertiaryvariable is still not defined in any palette block; if used elsewhere, it falls back to the:rootvalue. This was not introduced by this PR but remains a latent gap.
Summary
No new blockers introduced. The PR cleanly replaces the Obsidian theme system with a custom color/font/logo override system. Backward compatibility is maintained via the normalizer. Tests cover the new utilities and guard functions. The unused "clear" i18n key is a minor nit, and the loose customColors validation is acceptable for a desktop app with a trusted renderer.
Open Cowork Bot
When a user selects a palette swatch, any per-element color overrides are now cleared so the newly-chosen palette's colors show through unmodified. The user can re-apply custom colors afterward. Adds four new palette options: - Dracula: dark purple base with neon pink/cyan/green accents (Fira Code font) - One Dark: Atom's classic balanced blue-gray (Inter font) - Kanagawa: Japanese wave-inspired soft blues and warm browns (Inter font) - Everforest: green nature-inspired, calm and balanced (Inter font) Each palette ships with both light and dark CSS variants (24 new blocks). Type system, THEME_PALETTES, VALID_THEMES, settings UI swatches, and i18n labels (en + zh) all updated. Updated two tests that used 'dracula' as a stand-in for an unknown palette id (it's now valid). Verification: typecheck clean, lint 0 errors, 119/119 tests pass.
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
[Minor] Palette switch clears user's custom color overrides without undo
src/renderer/components/settings/SettingsGeneral.tsx:207-210 — clicking a palette swatch now calls updateSettings({ theme: opt.value, customColors: {} }), which discards any per-element color overrides the user may have configured. This is a deliberate design choice (so the new palette shows through cleanly), but it may be surprising to users who have fine-tuned individual colors and then try a different palette. Consider adding a confirmation dialog or saving the overrides in a separate undo slot.
[Minor] Previous review issues remain unaddressed
The "clear" i18n key (en.json:112, zh.json:112) is still present and unused. The customColors config field (config-store.ts:1150-1154) still accepts arbitrary keys beyond COLOR_SLOTS. These were flagged in the previous review and are not blockers for a desktop app, but cleaning them up would reduce dead code and potential persistence bloat.
Summary
No new blockers introduced. The four additional palettes (Dracula, One Dark, Kanagawa, Everforest) are well-defined with both dark and light variants, and the per-palette font assignments are consistent. The CSS blocks follow the established pattern and are placed outside @layer as required. The palette switch now clears customColors — a sensible default, but potentially disruptive without an undo path. Tests remain passing. The previous minor issues persist but are acceptable for the current scope.
Open Cowork Bot
Addresses bot review feedback on PR OpenCoworkAI#245: - config-store normalizer now filters customColors entries against COLOR_SLOTS, rejecting unknown keys (e.g. from a hand-edited config) that would otherwise bloat persisted config and be silently ignored by the renderer. - Remove the unused 'general.clear' i18n key (en + zh) — dead code with no consumer anywhere in src/renderer/. The recurring '--color-background-tertiary' bot finding is dismissed: grep confirms the variable is defined nowhere and consumed nowhere in the codebase. It's a persistent bot hallucination.
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
[Minor] Default appearance mismatch between main process config and renderer store
src/main/config/config-store.ts:379— defaultappearanceis'system'src/renderer/store/index.ts:195— defaultappearanceis'light'
When the app starts fresh (no persisted config), the main process will send a config.status event with appearance: 'system'. Until that event is received, the renderer store initializes with appearance: 'light', creating a brief flash where the sidebar quick-cycle icon and theme UI may show the wrong state. Consider aligning the store default with the config default ('system') or ensuring the store is initialized from the main process config before any rendering occurs.
Suggested fix:
diff --git a/src/renderer/store/index.ts b/src/renderer/store/index.ts
--- a/src/renderer/store/index.ts
+++ b/src/renderer/store/index.ts
@@ -192,7 +192,7 @@ const defaultSettings: Settings = {
theme: 'claude',
- appearance: 'light',
+ appearance: 'system',
fontFamily: 'auto',
fontSize: 'md',
customFontFamily: '',Previous issues resolved ✅
The two minor issues flagged in the prior review have been addressed:
customColorsnormalization now filters to onlyCOLOR_SLOTSkeys (src/main/config/config-store.ts:1150-1154)- Unused
"clear"i18n keys removed from bothen.jsonandzh.json
Summary
No new blockers. The four additional palettes and the per-palette font assignments are well-structured. The previous review's issues are resolved. One minor inconsistency in default appearance between main process and renderer store could cause a brief visual flicker on first launch — align the defaults to avoid it. Tests continue to pass (31/31).
Open Cowork Bot
Replaces the static welcome subtitle with a Claude-style time-based
greeting that adapts to the current hour, the active locale, and an
optional display name.
Behavior:
- Four buckets by hour: morning (5-11), afternoon (12-16),
evening (17-21), night (22-4). Night renders a soft 'Hello' rather
than 'good night', which reads like a sign-off.
- Each non-night bucket ships 3 phrasings per locale; the active one
is anchored to the calendar day so the greeting is stable within a
day but varies across days (no flicker on re-render, no monotony).
- Fully localized (en + zh). The greeting arrays and the withName
template live in the i18n locale files; logic is locale-free.
- Optional display name: when set, the greeting is personalized
('Good morning, Alex'); empty falls back to the generic phrasing.
New displayName config field, plumbed through the full stack
(config-store type/allowlist/default/normalizer/updater, renderer
types x2, store, useIPC) with a Settings input + i18n labels.
- src/shared/greeting.ts: pure, env-agnostic helpers
(getGreetingBucket, pickVariationIndex, buildGreeting), 23 unit tests.
- WelcomeView computes the greeting in a useMemo keyed on locale +
displayName; reads i18n arrays via returnObjects: true.
Verification: typecheck clean, lint 0 errors, 144/144 tests pass.
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
[Minor] Default appearance mismatch still present (unaddressed)
The discrepancy flagged in the previous review remains:
src/main/config/config-store.ts:379— defaultappearanceis'system'src/renderer/store/index.ts:195— defaultappearanceis'light'
On a fresh launch before the main process sends the config.status event, the renderer will briefly show appearance: 'light' while the main process has 'system'. This can cause a momentary incorrect theme class/layout. Please align the store default to 'system' or ensure the store is initialized only after receiving the config.
No new issues introduced
The new displayName feature, time-based greeting module (src/shared/greeting.ts), and its integration in WelcomeView.tsx are cleanly implemented with proper unit tests. The greeting logic is pure, well-tested, and correctly uses useMemo with appropriate dependencies. The displayName field is properly propagated through the config normalizer, store, and UI.
Previous issues resolution
The displayName field has been added to all necessary test fixtures and config paths. The compare diff shows that the minor formatting/prettier changes in the first round are now also included. The only unaddressed item is the default appearance mismatch noted above.
Summary
The follow-up introduces a clean display-name personalization feature with time-based greetings. No new blockers. The single minor issue from the prior review (default appearance inconsistency) persists — please fix it to prevent a brief theme flash on first launch.
Open Cowork Bot
1 participant
Summary
Splits the theme model into two orthogonal axes and adds per-palette typography, plus a fix for a critical Tailwind CSS bug that was silently dropping all palette rules.
Two orthogonal axes
The old model conflated two unrelated concerns into one flat
AppThemeunion: which color palette (Nord, Tokyo Night, ...) and which light/dark mode. Every palette shipped as a single fixed mode (only Solarized was light), so users could not get Tokyo Night in light, Nord in light, etc. Typography was also hardcoded globally, so switching palettes only ever changed colors.This splits the model:
theme(AppTheme) — the palette:claude | nordic | tokyo-night | gruvbox | catppuccin | rose-pine | solarizedappearance(AppAppearance) —dark | light | system, applied on top of whichever palette is selected (systemdefers to the OS preference as before).Every palette ships both light and dark
7 palettes × 3 CSS blocks each (base + dark + light) = 21 rules, each authentic to the palette's design language. Tokyo Night Light, Nord Light, Catppuccin Latte, Solarized Dark, etc. all work now.
Per-palette typography
Each palette sets
--font-sans/--font-serif/--font-mono, andbody,.heading-serif,.prose-chat, headings, andtailwind.config.jsfontFamilyall consume them:Settings UI
Separate appearance row (Light / Dark / System) above the palette grid, so the two axes are independently selectable. Sidebar's quick-cycle button now cycles the appearance axis.
Backward compatible
Old configs with
theme: 'dark'/'light'/'system'(the legacy single-axis model) auto-migrate totheme: 'claude', appearance: <old value>in the config normalizer, so existing users keep a working theme without touching their config file.Critical bug fix (Tailwind 3)
All
.theme-*palette rules were inside@layer baseinglobals.css. Tailwind 3's PostCSS was silently dropping them during compilation — verified via CDP::rootand.lightsurvived, the 6 palettes vanished. So clicking a swatch added the class to<html>but no CSS matched and colors never changed. The palettes are now top-level CSS (outside any@layer), which is not subject to Tailwind's layer tree-shaking.Files
config-store.ts— newAppTheme/AppAppearancetypes,isAppearance,VALID_APPEARANCES, legacy-mapping in the normalizer,appearanceinAppConfig/DIRECT_READ_KEYS/defaultConfig/get()guard/update()merge.theme-resolution.ts— rewritten for the new API (getSavedAppearance,getSavedPalette,resolveEffectiveAppearance,resolveNativeThemeSource).main/index.ts— usesappearancefornativeTheme.themeSource+ window background; IPCsettings.updateaccepts both axes.globals.css— 21 palette blocks (7 × base/dark/light), each with--color-*/--shadow-*/--font-*;body/.heading-serif/.prose-chatuse font vars.tailwind.config.js—fontFamilynow references the--font-*vars.App.tsx— applies both.theme-<palette>and.dark/.lightto<html>.SettingsGeneral.tsx— separate appearance row + palette grid (7 swatches).Sidebar.tsx— quick-cycle button cyclesappearance.useIPC.ts,store/index.ts,types/index.ts— mirror the new shape.en.json/zh.json— labels for 7 palettes + appearance.Test plan
npm run typecheck— cleannpx eslint src --ext .ts,.tsx— 0 errors (8 pre-existing warnings)npx vitest run src/tests/config/theme-*.test.ts— 31/31 passtheme: 'dark'config normalizes totheme: 'claude', appearance: 'dark'🤖 Generated with Open Cowork