feat(chrome): add device-wide theme preference and config command - #131
feat(chrome): add device-wide theme preference and config command#131gfiorav wants to merge 5 commits into
Conversation
|
Thanks for the PR! It looks like this branch has a merge conflict with the base branch right now. When you get a chance, could you rebase onto (or merge in) the latest base branch, resolve the conflict, and push? Once GitHub shows the PR as mergeable again, it'll be picked back up for review. Noted for lavish-axi#131 at |
1dd4b75 to
5b6befb
Compare
|
Thanks for the work here — solid execution and test coverage. But I think this approach adds more complexity/maintenance than I want, and the bigger issue is it breaks portability: with the injected I'd like to rescope this along these lines instead:
Can you rework the PR along these lines? Happy to discuss specifics if useful. |
9cdaa3f to
e72f02f
Compare
The editor chrome had only a dark theme. Add a cohesive light counterpart keyed by data-theme on :root, defaulting to the OS preference and tracking live OS changes with no reload. - src/chrome.css: keep the luxury palette as the dark :root default, add a :root[data-theme=light] block with paper/ink tokens, route hover/scrim/ banner/divider surfaces through new --hover/--scrim/--banner-*/--accent- divider semantic tokens so each theme sets a fitting value, style the theme switch. - src/server.js: inline a no-FOUC head script that resolves data-theme from localStorage before paint, add a three-state System/Light/Dark segmented control to the top bar with sun/moon/system icons. - src/chrome-client.js: own the runtime theme logic - read the stored preference, wire the toggle, apply data-theme, and listen to the prefers-color-scheme media query so System follows live OS changes. System is stored by removing the localStorage key. - tests: server tests for the head script, toggle markup, light token block, and theme-aware surfaces; chrome-client harness extended with localStorage/matchMedia/documentElement and five theme-logic tests. - .prettierignore: ignore the environment-injected .opencode dir so pnpm run check passes in worktrees that include it. - AGENTS.md: document the chrome theme system. The user-authored artifact inside the iframe is never themed by the chrome.
Replace the in-browser System/Light/Dark switch (and its per-tab localStorage toggle) with a single device-wide preference set from the CLI. - Add `lavish-axi config [theme [system|light|dark]]` that reads and writes a top-level `config` object in `~/.lavish-axi/state.json`. - `SessionStore.readState` now preserves `config` across every session write (the previous readState stripped it and would have wiped a theme write the first time any session was touched). - `createChromeHtml` accepts `themePref` from `getThemePreference` and embeds it as a `data-theme-pref` attribute on `<html>`; the inline head script resolves `data-theme` from that attribute before paint. - `chrome-client.js` reads the attribute and tracks live OS changes while the preference is `system`. The three-state segmented control and the localStorage write are gone. - The annotation-card / SDK UI still follows the resolved chrome theme (sourced from the device preference, not from the localStorage toggle). - Artifact content theming becomes **generation guidance**, not a runtime override. Lavish no longer injects a content-theme stylesheet, a no-FOUC boot script, or an opt-out mechanism: served artifacts stay byte-identical to their on-disk form (apart from the SDK script tag), restoring direct-open portability that the previous iteration broke. - A new `src/appearance.js` exposes `resolveSystemAppearance` (osascript on macOS, AppsUseLightTheme registry on Windows, gsettings on Linux) and `buildThemeDirective` (which turns the stored preference into a short directive for the agent to bake into the artifact's own CSS, Tailwind theme, or DaisyUI `data-theme`). - For `system`, the OS appearance is resolved at CLI time on the device and folded into a concrete directive; when the platform cannot be queried the directive falls back to a `prefers-color-scheme` hint, since a statically baked theme cannot live-track later OS toggles. An unset preference emits no directive, so a vanilla install never nags the agent. - `home` and `design` CLI outputs surface a `theme_preference` block with the current preference and directive so agents pick the theme up while writing HTML. - Docs: AGENTS.md and README describe the new control surface and explicitly drop the previous own/opt-out framing.
…torage test harness
e72f02f to
ed0ff76
Compare
|
@kunchenguid Human gfiorav here: I agree with the fact that this is too much to maintain. I can't shake the idea that lavish should support two very good themes (light and dark) and give the agent a host of well curated components it can use (tables, headers, option selector, etc). Artifact-driven themes are nice, but I think they don't scale as well as a more practical (and more opinionated) approach. Just a few thoughts. I'm having the robot gfiorav rework the PR as per your suggestions. |
|
what happened withi this? shall i try to redo a PR for light mode? |
Yes, please try again. I lost interest (too many things going on) but it would be nice to have. Kun provided some guidance on what he think the solution should be, so take his pointers into account |
|
will do. thanks! |
|
@gfiorav, @kunchenguid did the redo, it ended up as two PRs: #203 and #204. same feature, same pixels, the only difference is how the css does it (attribute selector + pick whichever you prefer, close the other and i'll clean up :) thanks |
Intent
The developer was finishing a theming rework on Lavish Editor's
fm/lavish-theme-q4branch per a revised plan from the maintainer, who had rejected the original content-injection approach (PR #131). The goal was to add alavish-axi config theme <system|light|dark>command that persists instate.json, have the editor chrome source its theme from that device preference, and surface atheme_preference: { preference, directive }block in the home and design CLI outputs as generation guidance for the agent (resolvingsystemto a concrete light/dark directive at CLI time, or null when unset). They applied a provided WIP diff, added an appearance resolver with tests, updatedAGENTS.mdand the README to document the newconfigcommand, ranpnpm run checkto green, and were about to ship through the no-mistakes gate to open a PR againstmain.What Changed
lavish-axi config theme <system|light|dark>backed by a top-levelconfig.themeinstate.json(getThemePreference/setThemePreference), with a newsrc/appearance.jsresolver that detects current OS appearance viaosascript(macOS), registry (Windows), orgsettings(Linux).createChromeHtmlnow embeds<html data-theme-pref="...">plus a no-FOUC inline head script that appliesdata-themebefore paint,chrome-client.jsreapplies it on load and live-followsprefers-color-schemewhile the preference issystem, andchrome.cssdefines a light counterpart on:root[data-theme="light"]with theme-aware--hover,--scrim,--banner-*, and--accent-dividertokens.theme_preference: { preference, directive }block in the home and design CLI outputs as agent generation guidance -light/darkcarry a concrete "bake this look into the artifact" directive,systemis resolved at CLI time to a concrete light/dark directive, and unset installs emit no directive so a vanilla install never nags the agent. AGENTS.md and README updated to document the new command and the runtime-vs-content theming model.Risk Assessment
✅ Low: The change is well-bounded: it adds a new
configsubcommand, a device-wideconfig.themefield in state.json, a no-FOUC inline script that keys the chrome offdata-theme-pref, and a small OS-appearance resolver; the substantive correctness and dead-code concerns from previous review rounds were addressed by the auto-fixes, and the remaining user-ignored items are documented product decisions, so this can merge without further review.Testing
Built the project, ran the full 495-test suite (all pass), and exercised the newlavish-axi config theme <system|light|dark>command end-to-end: setting and reading each value, validating rejected inputs, confirming thetheme_preferenceblock appears in both home and design output for every preference, verifying the no-FOUCdata-theme-prefattribute on the served chrome HTML, screenshotting the chrome in both light and dark themes (with and without the More menu open), and confirming the system preference live-followsprefers-color-schemevia DevTools emulation while explicit light/dark stay pinned. The appearance resolver and theme-directive builder were also exercised directly. All behavior matches the spec from the agent session./var/folders/y8/wy53333j43ncsk_r9djvh5mh0000gn/T/no-mistakes-evidence/01KWMZRQ30SY51PNVKHK4H13KZ/chrome-light-1280.png)/var/folders/y8/wy53333j43ncsk_r9djvh5mh0000gn/T/no-mistakes-evidence/01KWMZRQ30SY51PNVKHK4H13KZ/chrome-light-menu.png)/var/folders/y8/wy53333j43ncsk_r9djvh5mh0000gn/T/no-mistakes-evidence/01KWMZRQ30SY51PNVKHK4H13KZ/chrome-dark-1280.png)/var/folders/y8/wy53333j43ncsk_r9djvh5mh0000gn/T/no-mistakes-evidence/01KWMZRQ30SY51PNVKHK4H13KZ/chrome-dark-menu.png)Evidence: Test artifact used in the chrome screenshots
Evidence: state.json after `lavish-axi config theme dark` (config preserved with session)
Evidence: state.json after `lavish-axi config theme light` (config preserved with session)
Evidence: state.json after `lavish-axi config theme system` (config preserved with session)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 6 issues found → auto-fixed (2) ✅
src/server.js:800- chromeIcons.sun and chromeIcons.system are defined but never referenced anywhere in the source or tests. AGENTS.md explicitly states 'There is no in-browser theme switch', so these appear to be speculative additions for a future feature that does not exist.test/chrome-client-queue.test.js:142- The new test harness additions include a localStorage mock that references a lavish-axi:theme key (line 258) that has no counterpart in chrome-client.js (which only uses sessionStorage), plus setSystemTheme/themePreference/resolvedTheme helpers that are not called by any test. This reads like abandoned infrastructure - a previous approach that stored the preference in localStorage was apparently replaced by the server-injected data-theme-pref attribute, but the leftover mock and helpers were never removed. Carries the highest confusion risk because the lavish-axi:theme key is actively misleading.src/chrome.css:465- .share-overlay still uses a hardcoded rgba(15, 17, 21, 0.72) background even though the change introduces a theme-aware --scrim token (overridden in :root[data-theme=light]). In light mode the share dialog will sit on a dark scrim while the ended-session overlay and the layout-issue banner get proper light variants. The new AGENTS.md note says 'do not reintroduce hardcoded dark-only colors', so the author may want to migrate this too (likely with a slightly different opacity, e.g. a new --scrim-modal token).src/cli.js:141- readHomeThemePreference returns the value from getThemePreference, which falls back to 'system' when no preference is stored via normalizeThemePreference(undefined). For an unset / default install, the home output's theme_preference.preference is therefore the string 'system' rather than null - the same value the field would carry if the user had explicitly set theme to system. The agent can still disambiguate via the directive field (null vs a concrete OS-resolved string), but the symmetric API would be cleaner: pass the raw value through so preference mirrors the user's actual choice (null when unset, 'light'/'dark'/'system' when set), and let getThemePreference continue to supply the chrome with a defaulted 'system'.src/chrome-client.js:802- applyResolvedTheme(resolveTheme(themePref)) at the end of chrome-client.js re-runs the same computation the inline head script already performed (server.js:876). The accompanying comment 'recompute here so the device preference wins' is misleading - the device preference is already applied before paint. The real reason for the call is that the surrounding block also wires up the matchMedia change listener, and a single apply call sites the listener registration naturally. Either drop the call (the attribute is already set) or update the comment so the next reader doesn't think the inline script is unreliable.test/cli-output.test.js:1465- The 'unset' case in the new test calls createHomeOutput with no themePreference/themeDirective, so the function default of {preference: null, directive: null} is asserted. In the real run() flow, themePreference is always supplied by readHomeThemePreference(), which returns 'system' (not null) when unset - so the actual home output for a vanilla install will carry {preference: 'system', directive: null}, not the null/null pair the test pins. The test currently guards the function default rather than the shipped behavior, which is mildly misleading if a future reader cross-references the test against the run() wiring.🔧 Fix: Remove unused chrome theme icons and dead localStorage test harness
3 issues (1 warning, 2 infos) still open:
test/chrome-client-queue.test.js:142- The new test infrastructure (systemMediaQueryList, mediaQueryListeners, matchMedia, documentElement, getAttribute) was added so the chrome-client.js OS-appearance change handler can be exercised, but no test in this file actually invokes a registered change handler. That means the new behavior at src/chrome-client.js:806-812 (where themePref === 'system' re-resolves on matchMedia('change')) ships without direct coverage. The previoussetSystemThemehelper that drove this path was deleted in 22c4749; without a replacement, a regression that prevented live OS tracking would only be caught by a manual end-to-end check.src/cli.js:159- For themePref === 'system', readHomeThemeDirective calls await resolveSystemAppearance() and then passes the result to buildThemeDirective(raw, { resolvedAppearance }). If the first attempt returns null (timeout, missing binary, gsettings not installed, etc.), buildThemeDirective falls through to its own default resolve(platform) call inside the if (themePref === 'system') branch, spawning a second osascript/reg/gsettings process. On a host where the OS query consistently fails, this doubles the latency of the home output (two 750ms timeouts worst case). A simple fix is to pass the first attempt's result as a non-null placeholder (e.g. resolvedAppearance: null is enough - buildThemeDirective already handles that) and not duplicate the work, or to short-circuit buildThemeDirective to skip the inner resolve when the caller already passed a hint even if null.test/chrome-client-queue.test.js:142- The new matchMedia mock declares a mediaQueryListeners array and a removeEventListener method on systemMediaQueryList, but no test in the file reads from mediaQueryListeners or invokes removeEventListener. The harness returns systemMediaQueryList for the prefers-color-scheme query and the chrome-client.js script does call addEventListener('change', handler), but the registered handlers are never invoked. Either drop the array/removeEventListener (chrome-client.js only calls addEventListener) or add a test that fires the listeners to exercise the change path - otherwise this is dead state carried alongside the new theming work.🔧 Fix: Dedupe OS appearance resolution and prune dead test mock
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
pnpm test(495/495 pass)pnpm run buildsucceedsnode dist/cli.mjs configreturns{ config: {} }for unsetnode dist/cli.mjs config themereturns{ config: { theme: "system" } }for unsetnode dist/cli.mjs config theme darkwritesconfig.theme=darkto state.jsonnode dist/cli.mjs config theme lightwritesconfig.theme=lightto state.jsonnode dist/cli.mjs config theme systemwritesconfig.theme=systemto state.jsonnode dist/cli.mjs config theme sepiareturns VALIDATION_ERROR with allowed-values hintnode dist/cli.mjs config whateverreturns VALIDATION_ERROR for unknown subcommandnode dist/cli.mjshome output includestheme_preference: { preference, directive }for unset/light/dark/systemnode dist/cli.mjs designoutput includestheme_preference: { preference, directive }plus updatedtheme_usagenode dist/cli.mjs config --helpshows the new help blockServed chrome HTML at/session/<key>embeds<html data-theme-pref="...">and the no-FOUC inline head scriptchrome-devtools-axireportsdata-theme-pref=dark / data-theme=darkafterconfig theme darkchrome-devtools-axireportsdata-theme-pref=light / data-theme=lightafterconfig theme lightchrome-devtools-axi emulate --color-scheme darkflips chrome from light to dark when pref issystemchrome-devtools-axi emulate --color-scheme lightflips chrome from dark to light when pref issystemExplicitdarkpreference stays dark even after--color-scheme light(no live-follow when not system)SessionStore.upsertSession+queuePromptsafter a config write preserves the config (regression)buildThemeDirectivereturns null for null/undefined/sepia/blue preferencesbuildThemeDirectivereturns null for unset (vanilla install never nags the agent)buildThemeDirective("system", { resolvedAppearance: "light" })returns the light-mode-OS directivebuildThemeDirective("system", { resolvedAppearance: "dark" })returns the dark-mode-OS directivebuildThemeDirective("system", { resolvedAppearance: null })returns the prefers-color-scheme fallbackresolveSystemAppearance("darwin", exec=>"true")returnsdark;exec=>"false"returnslightresolveSystemAppearance("freebsd")returns null on unsupported platformsdist/chrome.cssdefines:root{...color-scheme:dark;--bg:var(--ink-900);...}and:root[data-theme="light"]{...color-scheme:light;--bg:#f1ece3;...}with the matching theme-aware tokens (--hover,--scrim,--banner-bg/-fg/-border,--accent-divider)Theme-aware surfaces (.menu-item:hover,.more-button:hover,.ended-overlay,.layout-issue-banner,.send-caret) route through CSS variables instead of hardcoded dark-only colors✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.