Theme Swapping - #72
aaronsmulktis wants to merge 27 commits into
Conversation
- theme design vars - set default layout - set default theme - use design vars in todo app - update README - add theme & layout to appserver - retain appearance for legacy usage - add theme & layout to registry and session - 44 tests passing
[Improvement] Todo layout
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
97fdbe0 to
5b526c2
Compare
There was a problem hiding this comment.
Looks great overall Aaron. Having layouts and theme as independent variables is a great idea. A few asks:
-
can you make sure you the documentation reflects this new theme/layout approach? We want to make sure the README and hosted documentation 1) removed references to the old appearance, 2) includes example commands with corresponding images of the expected outputs, 3) a list of available themes/layouts in the documentation somewhere.
-
I'm seeing
INFO: ::1:55320 - "GET /.websim.json HTTP/1.1" 404 Not Foundwhen launching the apps viauv run launch.py apps/theme='mono'Do you know what this websim.json is the server is missing? -
the solarized theme doesn't seem to affect the calendar app or messenger or maps or code editor (see image below). Could we make sure at least part of the background is updated with the solarized theme?
Only todo renders from the design tokens in `open_apps.theme`; calendar, messenger, maps, code editor, and the start page each build a `:root` block of their own from `config/apps/<app>/appearance/`, so an `apps/theme=` selection was invisible to them. Add a bridge rather than rewrite five stylesheets: `legacy_theme_css()` emits the theme's token block, aliases the legacy custom properties onto the tokens they should follow, and adds the shared page chrome plus the few per-app selectors whose colors are hard-coded. It returns "" on the `default` theme, so a deployment that never selects one renders exactly as before. Each app emits it after its own stylesheet and per-request, so `reconfigure` theme swaps take effect. Also give every app a `theme: null` field (inherit the global selection, override with `apps.<app>.theme=`), and extend save_screenshots.py with a `theme_<stem>` variation per theme file. Test plan: uv run -m pytest tests/ -> 724 passed, 7 skipped.
The default theme's font-family token was "'Times New Roman', serif", so every app migrated to design tokens (todo, and the online shop) rendered in the browser's serif while the appearance-based apps kept the Arial they had always used. The two looked like different products side by side. Point the token at the same sans stack the appearance configs used. Varying the font deliberately is what the `mono` and `challenging_font` themes are for. Test plan: uv run -m pytest tests/ -> 724 passed, 7 skipped.
mkdocs renders every page it finds under docs/, not just the ones listed in nav. docs/internal-*.md are untracked by design (.gitignore line 201), but a plain `mkdocs build` was rendering them into site/ -- and inlining their full text into site/search/search_index.json and their URLs into site/sitemap.xml, both of which *are* tracked and both of which push to a public remote. The .gitignore rule covers the generated site/internal-* pages themselves but cannot help with the two index files, since those are single tracked blobs that mix internal and public content.
…p layout
Roll the theme/layout split out to every app and delete the `appearance`
group. Previously only todo rendered from design tokens; the other six
apps built their own `:root` block from `config/apps/<app>/appearance/`,
and `legacy_theme_css()` re-pointed those variables at the shared tokens
so a theme selection was at least partly visible. Both are now gone.
`appearance` was conflating three separate things, so each key moved to
where it belongs:
* look -> shared design tokens in `config/apps/theme/` (global)
* structure -> per-app `config/apps/<app>/layout/`
* behaviour -> plain keys in `config/apps/<app>/default.yaml`
(maps zoom/granularity/allow_planning, code editor
mode/highlight/sort_feature, shop enable/credit cards)
One `apps/theme=dark` now themes all seven apps; the dark variation in
`config_parallel_tasks.yaml` drops from six overrides to one.
Themes carry an app-agnostic `assets` block (`tone`, `icon_set`) for the
choices a CSS variable cannot reach -- the start page's raster icons, the
Leaflet tile layer, the CodeMirror stylesheet. Each app maps `tone` onto
its own concrete asset, so a shared theme file never has to grow a key
per app. Without it, `apps/theme=dark` would leave a bright OpenStreetMap
basemap and a light `eclipse` editor pane inside dark chrome.
Fixed along the way:
* `apps.code_editor.theme` silently clobbered CodeMirror's theme. Both
the per-app design-token override and CodeMirror's stylesheet were
named `theme`, and the override won, so the editor initialised with
`theme: 'None'`. CodeMirror's is now `editor_theme`.
* `dark` and `mono` were unusable as global themes -- literal ports of
todo's old local variant, `dark` had a white `color-bg` *and* a white
`color-fg`, `mono` was black on black. Invisible while only todo read
the tokens, broken the moment six more apps did.
* `layers['{{ default_layer }}'].addTo(map)` threw a TypeError on an
unknown layer name, aborting the rest of the script and leaving a
blank map with no landmarks or sidebar wiring. Now falls back.
* `font-size: "{{ base_font_size }}"` in map.html was quoted, so it was
invalid CSS and the setting never applied.
* A missing semicolon in map.html's `.search-btn` swallowed the next
declaration.
Also adds a `colorblind` theme (generalised from the code editor's
`colorblind_access` variant), a greyscale `CartoDB Positron` basemap for
`mono`, and a test that every theme declares the same token vocabulary --
an app uses `var(--token)` without knowing which theme is active, so a
missing token renders as an empty value rather than a loud failure.
Note the reference screenshots need regenerating: `save_screenshots.py`
now captures `theme_*` and `layout_*` variations instead of the old
appearance names.
Test plan: `uv run -m pytest tests/` -> 732 passed, 7 skipped (was 724/7;
+8 new theme/asset tests). ruff unchanged at 292 pre-existing errors with
an identical rule histogram. Served all six routes under `dark` and
`mono` and checked the emitted CSS: correct tokens everywhere, no stale
variables, no unrendered Jinja placeholders; `broken_logos` confirmed
detaching icons from their labels.
There was a problem hiding this comment.
Pull request overview
This PR introduces a shared, design-token based theming system (plus theme “assets” for non-CSS choices) so theme selection can be applied globally or overridden per-app, while separating “layout” (structure) from “theme” (look). It updates several apps, tests, the MCP surface, and documentation to adopt the new apps/theme + per-app layout model.
Changes:
- Add
open_apps.themeutilities to resolve/load themes, render CSS tokens, and expose theme “assets” (tone/icon set) for apps. - Migrate apps/configs away from per-app
appearancegroups to globalapps/themeand per-applayout, updating MCPreconfigure+ variant listing accordingly. - Add/refresh tests and docs to cover precedence/fallback and to reflect the new variation axes.
Reviewed changes
Copilot reviewed 86 out of 87 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_theme.py | New unit tests for theme loading, precedence, assets, and safe token rendering. |
| tests/test_mcp.py | Update MCP variant listing expectations for layout and shared theme. |
| tests/test_apps.py | Update integration fixture overrides to use apps/theme and per-app theme keys. |
| tests/save_screenshots.py | Update screenshot variations to sweep shared themes and selected layouts. |
| src/open_apps/theme.py | New shared theming module (load/resolve/render tokens + theme assets helpers). |
| src/open_apps/mcp/session.py | Update MCP session reconfigure signature to theme/layout/content. |
| src/open_apps/mcp/server.py | Update MCP tools docstrings and params for theme/layout/content. |
| src/open_apps/mcp/registry.py | Teach list_variants that theme is a shared group under config/apps/theme. |
| src/open_apps/mcp/README.md | Document MCP API changes (reconfigure, list_variants) and semantics. |
| src/open_apps/mcp/appserver.py | Update live reconfigure override strings to apps/theme + per-app layout. |
| src/open_apps/apps/todo_app/main.py | Switch todo styling to token-based CSS + per-request theme_style(). |
| src/open_apps/apps/start_page/main.py | Add theme-aware tile colors/icons and emit per-request theme CSS via PageWrapper. |
| src/open_apps/apps/start_page/helper.py | Remove inline font styling and add theme CSS injection support in PageWrapper. |
| src/open_apps/apps/onlineshop_app/templates/html_generator.py | Replace config-driven colors/fonts with theme token CSS and Bootstrap surface overrides. |
| src/open_apps/apps/onlineshop_app/models/global_state.py | Store full apps config node to resolve shared theme from onlineshop templates. |
| src/open_apps/apps/onlineshop_app/main.py | Pass apps config into onlineshop global state for theme resolution. |
| src/open_apps/apps/messenger_app/main.py | Replace appearance-driven CSS vars with token-based component styles + per-request theme block. |
| src/open_apps/apps/map_app/templates/map.html | Convert template styling to design tokens; add themed tile layer fallback; inject theme CSS. |
| src/open_apps/apps/map_app/main.py | Select basemap by theme “tone” and pass rendered theme CSS to template. |
| src/open_apps/apps/codeeditor_app/main.py | Tokenize UI chrome; map shared theme tone to CodeMirror theme; persist in-page override. |
| src/open_apps/apps/calendar_app/main.py | Replace config-generated CSS with token-based styles + per-request layout vars. |
| site/tasks/index.html | Published docs update to include “Goal Variations” section. |
| site/sitemap.xml | Regenerate sitemap (timestamps/conflict cleanup). |
| site/search/search_index.json | Regenerate search index reflecting theme/layout docs changes. |
| site/index.md | Docs update: split appearance into Theme/Layout, update examples and parallel launch docs. |
| site/index.html | Generated HTML reflecting updated site/index.md. |
| site/agents/index.html | Generated HTML: new cluster eval instructions. |
| mkdocs.yml | Exclude internal-*.md from builds to avoid leaking untracked internal docs into site/. |
| docs/index.md | Source docs update mirroring site/index.md theme/layout guidance. |
| config/config.yaml | Add apps/theme: default to Hydra defaults. |
| config/config_parallel_tasks.yaml | Add apps/theme: default and simplify dark variation to a single global theme override. |
| config/apps/todo/layout/kanban_board.yaml | New per-app layout variant for todo. |
| config/apps/todo/layout/default.yaml | New default layout group for todo. |
| config/apps/todo/default.yaml | Replace appearance default with layout default; add per-app theme: null override field. |
| config/apps/todo/appearance/kanban_board.yaml | Remove legacy appearance variant. |
| config/apps/todo/appearance/default.yaml | Remove legacy appearance variant. |
| config/apps/todo/appearance/dark_theme.yaml | Remove legacy appearance variant. |
| config/apps/todo/appearance/challenging_font.yaml | Remove legacy appearance variant. |
| config/apps/todo/appearance/black_and_white.yaml | Remove legacy appearance variant. |
| config/apps/theme/default.yaml | New shared token vocabulary + default assets. |
| config/apps/theme/dark.yaml | New shared dark token palette + dark assets. |
| config/apps/theme/mono.yaml | New shared monochrome token palette + mono assets. |
| config/apps/theme/solarized.yaml | New shared solarized token palette + light assets. |
| config/apps/theme/material.yaml | New shared material-like token palette + light assets. |
| config/apps/theme/bootstrap.yaml | New shared bootstrap-like token palette + light assets. |
| config/apps/theme/challenging_font.yaml | New shared token palette with hard-to-read typography axis. |
| config/apps/theme/colorblind.yaml | New shared colorblind-safe palette generalized from prior editor variant. |
| config/apps/start_page/layout/default.yaml | Move typography/colors to shared theme; keep structure/geometry in layout; add icon sets. |
| config/apps/start_page/layout/clickable_logos.yaml | New/updated layout variant enabling clickable logo headers. |
| config/apps/start_page/layout/broken_logos.yaml | New layout variant to detach icons from tiles (shuffle). |
| config/apps/start_page/default.yaml | Replace appearance default with layout default; add per-app theme: null. |
| config/apps/start_page/appearance/default.yaml | Remove legacy appearance variant. |
| config/apps/start_page/appearance/dark_theme.yaml | Remove legacy appearance variant. |
| config/apps/start_page/appearance/challenging_font.yaml | Remove legacy appearance variant. |
| config/apps/start_page/appearance/black_and_white.yaml | Remove legacy appearance variant. |
| config/apps/onlineshop/layout/default.yaml | New default layout group for onlineshop. |
| config/apps/onlineshop/default.yaml | Replace appearance default with layout default; add per-app theme: null; keep behavior keys. |
| config/apps/onlineshop/appearance/default.yaml | Remove legacy appearance variant. |
| config/apps/onlineshop/appearance/dark_theme.yaml | Remove legacy appearance variant. |
| config/apps/onlineshop/appearance/challenging_font.yaml | Remove legacy appearance variant. |
| config/apps/onlineshop/appearance/black_and_white.yaml | Remove legacy appearance variant. |
| config/apps/messenger/layout/default.yaml | New default layout group for messenger. |
| config/apps/messenger/default.yaml | Replace appearance default with layout default; add per-app theme: null. |
| config/apps/messenger/appearance/default.yaml | Remove legacy appearance variant. |
| config/apps/messenger/appearance/dark_theme.yaml | Remove legacy appearance variant. |
| config/apps/messenger/appearance/challenging_font.yaml | Remove legacy appearance variant. |
| config/apps/messenger/appearance/black_and_white.yaml | Remove legacy appearance variant. |
| config/apps/maps/layout/default.yaml | New default layout group for maps. |
| config/apps/maps/default.yaml | Replace appearance default with layout default; add per-app theme: null; move behavior keys here; add layer_by_tone. |
| config/apps/maps/appearance/default.yaml | Remove legacy appearance variant. |
| config/apps/maps/appearance/dark_theme.yaml | Remove legacy appearance variant. |
| config/apps/maps/appearance/challenging_font.yaml | Remove legacy appearance variant. |
| config/apps/maps/appearance/black_and_white.yaml | Remove legacy appearance variant. |
| config/apps/code_editor/layout/default.yaml | New default layout group for code editor. |
| config/apps/code_editor/default.yaml | Replace appearance default with layout default; add per-app theme: null; define tone→CodeMirror theme mapping. |
| config/apps/code_editor/appearance/default.yaml | Remove legacy appearance variant. |
| config/apps/code_editor/appearance/dark_theme.yaml | Remove legacy appearance variant. |
| config/apps/code_editor/appearance/colorblind_access.yaml | Remove legacy appearance variant (folded into shared colorblind theme). |
| config/apps/code_editor/appearance/challenging_font.yaml | Remove legacy appearance variant. |
| config/apps/code_editor/appearance/black_and_white.yaml | Remove legacy appearance variant. |
| config/apps/calendar/layout/default.yaml | New default layout group for calendar (container width/spacing/button padding). |
| config/apps/calendar/default.yaml | Replace appearance default with layout default; add per-app theme: null. |
| config/apps/calendar/appearance/default.yaml | Remove legacy appearance variant. |
| config/apps/calendar/appearance/dark_theme.yaml | Remove legacy appearance variant. |
| config/apps/calendar/appearance/challenging_font.yaml | Remove legacy appearance variant. |
| config/apps/calendar/appearance/black_and_white.yaml | Remove legacy appearance variant. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
KarenUllrich
left a comment
There was a problem hiding this comment.
@marksibrahim This change will make the results in the paper not reproducible anymore, I am wondering if we should version these changes? Or are we ok with that?
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Good catch. What if I just re-introduce and retain backwards compatibility with the |
…nsmulktis/themes-and-layouts
Anyone with existing `apps/<app>/appearance=...` overrides -- notably the paper's variation grid -- gets a Hydra resolution error after this branch merges, with no pointer to the replacement. Maps every old value onto its theme or layout equivalent, calls out the two renderings that shift (`mono` polarity, `colorblind` scope), and points at the `v1.0-paper` tag for anyone who wants the old tree rather than a port.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical security and compatibility issues, plus live reconfiguration defects, must be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (10)
config/apps/todo/default.yaml:10
- The PR description advertises the single-app override
apps/todo/theme=solarized, but this declaresthemeas a scalar field, so that slash form is a Hydra config-group override and fails becauseconfig/apps/todo/theme/does not exist. Either add support for that public syntax or correct the example to the dotted field override (apps.todo.theme=solarized).
docs/index.md:137 - These replacements do not preserve scope: each old
apps/<app>/appearance=...override targeted one app, while every newapps/theme=...value is global. Following this table changes all apps rather than migrating the requested app. Use dotted per-app fields such asapps.<app>.theme=...for the per-app replacements and document the global forms separately.
src/open_apps/apps/map_app/templates/map.html:295 - The theme block is appended after the map's existing CSS rules, so any
@importproduced byrender_theme_cssis not at the beginning of the<style>block and will be ignored by CSS parsers. Move the theme block before the other rules or emit the import in its own first style/link element.
src/open_apps/apps/onlineshop_app/templates/html_generator.py:16 global_state.apps_configis populated only during the shop's initialset_environment;AppServer.reconfigurenever updates it. This resolver therefore continues to emit the original global theme after a live swap, making the online shop an exception to the claimed per-request behavior. Refresh the shop's global state as part of reconfiguration.
src/open_apps/apps/start_page/helper.py:537render_theme_csscan begin with an@import, but this interpolation places it after all existing rules in the<style>element. Browsers require@importrules to precede style rules, so any theme using the documented nonemptyimport_urlwill have its stylesheet ignored. Emittheme_cssat the start of the style block.
src/open_apps/apps/start_page/main.py:208- This shuffle runs only from
initialize_routes_and_configure_task;AppServer.reconfigure(layout=...)never reruns it. Consequently switching tobroken_logoslive leaves the existing icon mapping unchanged (and the app headers built bycreate_logo_headeralso retain their initialclickable_logostate). Layout-dependent icon/header state needs to be rendered or rebuilt after a live layout change.
src/open_apps/mcp/appserver.py:212 - The
broken_logoslayout setsshuffle_icons, but the icon permutation runs only once ininitialize_routes_and_configure_task(src/open_apps/apps/start_page/main.py:200-213). Replacing the config here does not rerun that side effect, so a live switch tolayout=broken_logosleaves icons attached to their original tiles. Reapply the layout side effect on reconfigure or derive the mapping during rendering.
src/open_apps/mcp/appserver.py:214 - This reconfiguration path only replaces the config and removes the temporary logs; it never calls
random.seedorreset_all_appsdespite documenting a fresh seed and re-seeded state. Thuscontentandseedchanges leave existing SQLite/in-memory state unchanged, and the new seed is not applied to the RNG. Apply the seed and reset/reseed state before returning, or remove that contract.
src/open_apps/mcp/appserver.py:212 - Assigning
self.config.appsonly rebinds the root config (and the FastHTML start-page app); the other route modules keep the original node assigned by theirset_environmentcalls, such astodo_app.app.config. Consequently live theme/layout/content changes are not visible on/todo,/calendar, etc. Update each registered module'sapp.config(and related state) or mutate the shared node in place before returning.
src/open_apps/mcp/session.py:225 reconfigureis advertised as live, but unlikeresetit never reloadsself.page. Theme CSS and layout/content HTML already in the current document therefore remain unchanged, so the nextobserve()still shows the old page after an MCP reconfigure. Reload the current URL and wait for it to settle before returning.
- Files reviewed: 86/87 changed files
- Comments generated: 7
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Removing the parameter changed the published tool schema, so an existing client calling reconfigure(appearance=...) fails argument validation before the call reaches us -- with no alias and no error pointing at the replacement. appearance is back as a deprecated keyword on AppServer/Session/the MCP tool, translated onto the new axes by registry.migrate_appearance() using the same map the docs migration table publishes: five stems became shared themes, three became per-app layouts. Each call raises DeprecationWarning; an appearance that disagrees with a theme/layout passed alongside it raises ValueError rather than silently picking a winner, and an unknown stem raises with the known values listed. list_variants(app, 'appearance') now raises too. The group's directory is gone, so it was falling through to the missing-dir ['default'] fallback -- a client sampling variations would get a silently single-variant sweep. Verified appearance='dark_theme' composes a config identical to theme='dark', and appearance='kanban_board' sets todo.layout=kanban_board. Test plan: uv run -m pytest tests/ -> 766 passed, 7 skipped (was 755/7; +11, including one asserting the reconfigure tool schema still advertises an optional appearance property). ruff unchanged at 6 pre-existing E402.
…ulktis/OpenApps into aaronsmulktis/themes-and-layouts
39851d4 swapped the `path.exists()` fallback for a character check. The character check is the right guard against a name escaping _THEME_DIR, but it is not a substitute: a well-formed name that simply has no yaml (`apps.todo.theme=does_not_exist`) went straight to read_text() and raised FileNotFoundError, against the docstring's promise to degrade gracefully. Keep both guards. Test plan: tests/test_theme.py 28 passed (2 were failing on the branch: TestLoadTheme::test_unknown_theme_falls_back_to_default and TestResolveTheme::test_unknown_per_app_name_degrades_to_default).
`parallel_tasks.app_variations` and save_screenshots' variation grid hold override strings as *data*. A renamed config group there raises when a sweep launches, not at import, so nothing catches it until an allocation is already running -- exactly how the appearance -> theme/layout rename could rot silently. Compose each entry for real. Confirmed the guard bites: reverting one entry to `apps/todo/appearance=dark_theme` fails with "app_variations entry ['apps/todo/appearance=dark_theme'] no longer composes: Could not override 'apps/todo/appearance'." Test plan: uv run -m pytest tests/ -> 768 passed, 7 skipped.
|
Migrating away |
`main` is about to diverge from the tree the arXiv:2511.20766 results were produced against -- PR #72 removes the per-app `appearance` config group in favour of a shared theme plus per-app layout, so the paper's variation overrides stop resolving. Point anyone reproducing our numbers at the `v1.0-paper` tag instead, and say plainly that everything after it is free to move.
The appearance group is gone from config and rendering, so an override written against it is a Hydra composition error rather than a silent no-op -- say so, and make the paper-reproduction path explicit: theme + layout do not reproduce the paper's grid pixel-for-pixel, v1.0-paper does. Also document the one surviving exception, the MCP reconfigure shim, which callers cannot discover from the config tree. Rebuilds site/ from docs/.
Watchman drops a probe file into every watched directory; eight of them were showing up as untracked in every status.
Replaces the per-app
appearancegroup with two orthogonal axes: a sharedtheme(look — design tokens, global) and a per-applayout(structure). All seven apps render from the shared design tokens;
appearanceis gone.
Theme swap — all apps (Hydra group override,
config/apps/theme/):Theme swap — one app (per-app field
apps.<app>.theme;nullinherits theglobal selection):
The two forms differ on purpose:
apps/theme=selects a config group,apps.todo.theme=sets a field. There is noconfig/apps/todo/theme/group,so the slash form scoped to a single app does not resolve.
Layout is per-app and stays a group override:
Themes available:
default,bootstrap,challenging_font,colorblind,dark,material,mono,solarized.Migrating from
appearance: the full table is in thedocs —
dark_theme→apps/theme=dark,black_and_white→apps/theme=mono,colorblind_access→apps/theme=colorblind, andkanban_board/broken_logos/clickable_logos→ the identically-namedlayout. The MCPreconfiguretool keepsappearanceas a deprecated alias mapping onto thenew axes. To reproduce the paper's numbers on the old tree, check out the
v1.0-papertag.