feat: custom JSON themes - #257
Conversation
|
@n00ki is attempting to deploy a commit to the bholmesdev's projects Team on Vercel. A member of the Team first needs to authorize it. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
Adds @hubble.md/theme, Electron ThemeService (custom JSON themes + watch), Appearance settings UI, and wires tokens through desktop UI, terminal, editor, native chrome, and HTML apps. Shared CSS color defaults are removed in favor of runtime applyTheme / injected CSS.
Concerns
- Shared
theme.cssno longer defines any color tokens.apps/wwwonly paints via JSapplyThemeafter the module loads, so first paint (and any non-applyThemeconsumer) loses--background/--foreground/ etc. - Renderer theme tests were gutted to two happy paths; prior coverage for preference/OS edge behavior and no new coverage for rollback, revision gating, or preview restore.
- HTML App CSS likewise drops static color fallbacks and relies entirely on injection + postMessage (works when the desktop pipeline is up; brittle if either path misses).
Verdict
Found: 0 critical, 3 important, 2 suggestions
Request changes
Reviewed by a Warp Factory agent.
| @@ -1,30 +1,4 @@ | |||
| :root { | |||
There was a problem hiding this comment.
:root. apps/www (and any other @hubble.md/ui/theme.css consumer) now has no CSS fallback for --background, --foreground, --primary, etc. until JS runs applyTheme.
apps/www/src/index.css still does color: var(--foreground); background-color: var(--background); at parse time, so first paint is unstyled/wrong. Keep light built-in tokens in CSS (or a tiny inline boot script) and let applyTheme override.
| import App from "./App.tsx"; | ||
| import "./index.css"; | ||
|
|
||
| applyTheme(document.documentElement, HUBBLE_LIGHT_THEME); |
There was a problem hiding this comment.
applyTheme(...) only runs after the module graph loads. With color tokens stripped from theme.css, www has no pre-JS theme. Pair this with CSS defaults (or an inline head script) so first paint is not blank/unstyled.
| it("applies the initial theme state from Electron", async () => { | ||
| const { initTheme, themeStateStore } = await import("./theme"); | ||
| await initTheme(); | ||
|
|
||
| emit(true); | ||
| expect(themeStateStore.get()).toEqual(state); | ||
| expect(document.documentElement.dataset.theme).toBe("builtin:hubble-dark"); | ||
| expect( | ||
| document.documentElement.style.getPropertyValue("--background"), | ||
| ).toBe("#171614"); | ||
| expect(document.documentElement.classList.contains("dark")).toBe(true); | ||
|
|
||
| emit(false); | ||
| expect(document.documentElement.classList.contains("dark")).toBe(false); | ||
| }); | ||
|
|
||
| it("notifies subscribers when the resolved appearance changes", () => { | ||
| const { emit } = mockMatchMedia(false); | ||
| initTheme("system"); | ||
| const listener = vi.fn(); | ||
| const unsubscribe = subscribeTheme(listener); | ||
| it("updates immediately and persists the full selection", async () => { |
There was a problem hiding this comment.
setThemeSettings IPC failure rollback, stale revision drops in acceptThemeState, or previewTheme / restoreThemePreview.
Add tests for those distinct paths before merge.
| :root { | ||
| color-scheme: light dark; | ||
| --brand: oklch(0.426 0.08 159.6); | ||
| --brand-accent: oklch(0.847 0.168 84.7); | ||
| --brand-accent-foreground: oklch(0.327 0.051 164.9); | ||
| --background: oklch(0.995 0.001 95); | ||
| --foreground: oklch(0.27 0.008 150); | ||
| --card: oklch(0.998 0.001 95); | ||
| --card-foreground: var(--foreground); | ||
| --primary: var(--brand); | ||
| --primary-foreground: oklch(0.985 0 0); | ||
| --secondary: oklch(0.968 0.003 95); | ||
| --secondary-foreground: var(--foreground); | ||
| --muted: oklch(0.96 0.003 95); | ||
| --muted-foreground: oklch(0.55 0.01 95); | ||
| --accent: oklch(0.965 0.05 90); | ||
| --accent-foreground: var(--foreground); | ||
| --selected: oklch(0.9 0.12 90); | ||
| --selected-foreground: var(--foreground); | ||
| --destructive: oklch(0.577 0.245 27.325); | ||
| --border: oklch(0.9 0.004 95); | ||
| --input: oklch(0.91 0.004 95); | ||
| --ring: var(--brand-accent); | ||
| --radius: 0.625rem; |
There was a problem hiding this comment.
💡 [SUGGESTION] Static color tokens and the dark prefers-color-scheme block are gone; HTML apps now depend only on main-process themeCss(...) injection + hubble:theme postMessage. Fine when both fire, but a failed/late update leaves apps with no colors.
Keep a minimal built-in light (and optional dark) fallback in this sheet so apps still render if injection/messaging misses.
| const rollback = { | ||
| ...current, | ||
| settings: previous.settings, | ||
| active: activeTheme( | ||
| previous.settings, | ||
| current.themes, | ||
| current.systemAppearance, | ||
| ), | ||
| }; | ||
| themeStateStore.set(rollback); |
There was a problem hiding this comment.
💡 [SUGGESTION] Rollback restores settings/active but does not bump revision. A concurrent main push with the failed settings and a higher revision can re-apply the rejected selection after rollback.
On rollback, either bump a local revision past the failed request or ignore main updates that still match the rejected settings until a newer authoritative state arrives.
There was a problem hiding this comment.
Overview
Follow-up on custom JSON themes (@hubble.md/theme, Electron ThemeService, Appearance UI, desktop/www/runtime wiring).
Prior review
- Addressed: shared
theme.csslight/dark CSS fallbacks + alignment test; renderer tests for IPC rollback, stalerevision, and preview restore; electron system-mode coverage. - Still open (non-blocking): HTML App sheet still has no static color tokens (injection +
hubble:themeonly); optimisticsetThemeSettingsrollback still does not bump/gaterevision.
Concerns
- No new correctness/security blockers in the delta or remaining PR surface.
- Optional hardening only (HTML App CSS fallbacks; rollback vs concurrent main revision).
Verdict
Found: 0 critical, 0 important, 2 suggestions
Approve with nits
Reviewed by a Warp Factory agent.
| --border: oklch(0.9 0.004 95); | ||
| --input: oklch(0.91 0.004 95); | ||
| --ring: var(--brand-accent); | ||
| --radius: 0.625rem; |
There was a problem hiding this comment.
💡 [SUGGESTION] Still open from prior review: static color tokens / dark prefers-color-scheme were removed. Desktop injects themeCss(active) and posts hubble:theme, so the happy path is fine, but a missed injection or late postMessage leaves apps with only --radius.
Keep a minimal built-in light (and optional dark) fallback here so HTML apps still render if those paths miss.
| const rollback = { | ||
| ...current, | ||
| settings: previous.settings, | ||
| active: activeTheme( | ||
| previous.settings, | ||
| current.themes, | ||
| current.systemAppearance, | ||
| ), | ||
| }; | ||
| themeStateStore.set(rollback); |
There was a problem hiding this comment.
💡 [SUGGESTION] Still open: rollback restores settings/active but does not bump revision or ignore main updates that still match the rejected settings. A rare path (write succeeded, IPC error; or another source republishes the failed selection at a higher revision) can re-apply the rejected theme after rollback.
On rollback, bump a local revision past the failed request, or ignore main updates that still equal the rejected settings until newer authoritative state arrives.
1 participant
Summary
@hubble.md/themepackage containing the theme contract, built-in themes, resolution logic, and JSON Schema..jsonthemes from the global<Electron userData>/themesdirectory, watch for file changes, and report invalid themes without disrupting valid ones.https://hubble.md/schemas/theme.jsonand document the theme format.Theme format
Themes are plain JSON files with an optional
$schemadeclaration. The schema remains single-source in the theme package and is exposed through the website without duplicating or prematurely versioning the format.Verification
pnpm checkpnpm build:desktoppnpm test— 467 tests passedpnpm check:react-compiler— zero failurespnpm --filter web build