WebShop Python Rewrite - #78
Open
aaronsmulktis wants to merge 20 commits into
Open
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>
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.
`git worktree add .claude/worktrees/<topic>` puts a full checkout inside the repo. It showed up as untracked in the main checkout, so a stray `git add -A` would have committed the entire nested tree.
The shop was a port of Princeton's WebShop that needed OpenJDK 21 and a
~1000-product dataset downloaded from Google Drive by `setup.sh`. Products
were searched through a Lucene index built by pyserini, which is a JNI
binding and hence the JDK. None of that reaches an offline eval node, so the
app shipped disabled and could not be launched from a clean checkout.
Three things made the rewrite cheap. `get_reward`/`get_goals` -- the whole
WebShop scoring model, including its spaCy POS-tagging and thefuzz attribute
matching -- were imported but never called. No task in config/tasks targets
the shop. So nothing depended on the dataset's annotations or on the state
shape, and the schema was free to change.
* Catalog seeds from `config/apps/onlineshop/content/` like every other app
(40 products, 8 categories), so `content` is a real variation axis.
* Search is SQLite FTS5 with its built-in `bm25()`, weighted title > options
> bullets > description. Option values are indexed, matching the document
the original built for Lucene. No JDK, no new dependency.
* Persistence is one SQLite file of ordinary relational tables. Order lines
were keyed by a stringified Python tuple ("('B07YPYJ32Z', '{}')") read back
with ast.literal_eval; `/onlineshop_all` now returns plain nested JSON.
* Rendering is FastHTML on the shared design tokens, so `apps/theme=` applies,
plus a new `layout` group: default, grid, compact_table.
* Product imagery is a generated SVG swatch keyed on the sku. The old cards
pointed at Amazon CDN URLs, which is egress the eval nodes do not have.
Enabled by default now that it has no setup step. Drops pyserini, spacy,
thefuzz, gdown and faiss-cpu (the last two already unused) -- 33 packages
including pyjnius. setup.sh keeps only the Java 21 install, moved to the map
app which is the actual OpenTripPlanner consumer.
Test plan: uv run -m pytest tests/ -> 772 passed, 6 skipped (was 724/7; the
onlineshop test no longer skips on missing Java). tests/test_onlineshop.py
adds 47 covering search ranking and FTS operator injection, cart merge/split
on options, checkout, the reward state shape, all three layouts, per-app and
global themes, german content, and absence of external hosts.
Not verified: reference screenshots under tests/generated_screenshots for the
onlineshop_* routes still show the old Bootstrap UI and need regenerating;
Chromium would not start in this sandbox.
Three things found while using the shop: * Cart lines showed a unit price and a quantity but no line total. Added "$89.00 each x 3 = $267.00". * Two lines of the same product differ *only* by their options, which were rendered as a small muted caption -- so a cart holding a fine-nib and a broad-nib pen read as duplicated rather than as two variants. Options are now chips, and a deselected line says so. (The merge itself was correct: identical options merge in every path -- urlencoded, multipart, in-process.) * No way back to the landing page from the cart or an item. Added a "Shop" link to the shop bar, plus a cart count. It lives there rather than on the header logo because `clickable_logo` defaults to false and is a start-page variation axis; navigation must not depend on a difficulty knob. Also name the tables explicitly. fastlite derives them from the class name and singularises, so `Order` became `order` -- a SQL reserved word, breaking `SELECT * FROM order` for anything reading the database directly. That defeats the point of the schema, which is to be readable by other apps and by humans. Now products / cart_items / orders / order_items, as documented. Test plan: uv run -m pytest tests/test_onlineshop.py -> 51 passed (4 new, covering line totals, option chips, per-page navigation, and table naming). Verified against a live launch with sqlite3: unquoted joins across all four tables now work.
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.
The installation page still described the old shop as an optional extra requiring OpenJDK 21, a gdown dataset and setup_pyserini.sh. Replace that with what is actually true -- the shop needs no setup and is on by default -- and add the parts a user needs after launching: * how to enable/disable it and vary layout, content and theme; * how to find the SQLite database for the run they were just clicking through (`ls -t log_outputs/*/databases/onlineshop.db | head -1`) and query it; * a short architecture note: catalog seeded from Hydra content, FTS5 + bm25() for search, four relational tables, FastHTML on the shared design tokens, generated SVG imagery, `/onlineshop_all` as the reward surface. Kept a collapsed note on the old JDK/Lucene setup, since the Google Drive links it used are now dead and anyone following an older guide will get stuck. Added a matching section to the README. Verified every documented override composes (layout x3, content x3, theme global and per-app, enable, products_per_page) and the sqlite3 snippets run against a live launch.
Thumbnails were a coloured square with the product's initials, which made a listing scan like a spreadsheet. Replace with 35 line-art glyphs -- headphones, skillet, tent, fountain pen -- picked by keyword from the title with a per-category fallback, so a product added to the config without a matching keyword still gets something appropriate rather than a blank. Still generated rather than fetched, for the same reason as before: the original hotlinked Amazon CDN URLs, which eval nodes cannot reach and tests/test_no_egress.py forbids. Deterministic, no dependency, ~2kB each, and hue stays keyed on the sku so a product looks the same on every page. Keyword order is load-bearing and was wrong on first pass: "LED Desk Lamp" matched the standing-desk entry because it contains "desk". Ordering is now commented and pinned by tests, along with "Desk Organizer" and the "Backpacking Tent" / "backpack" collision. Test plan: uv run -m pytest tests/test_onlineshop.py -> 62 passed (11 new). Audited all 40 products: 34 distinct glyphs, zero falling through to the category default.
Replace the generated product catalog with an optional importer for the WebShop item dump, so no model-written product copy ships in the repo. `scripts/fetch_webshop.py` downloads the dump from its HuggingFace mirror and converts it into a `webshop` content pack. The output is gitignored: the records are scraped Amazon listings, so they stay on the machine that downloaded them rather than being redistributed from here. Field names are read from candidate lists because the dump has been re-exported several times -- the current one uses name/full_description/small_description/ pricing -- and `--inspect` prints the real schema when they drift again. Image URLs are dropped on import, including the ones hidden in `customization_options` sibling keys, and the script aborts if any survive. `content/default.yaml` is now chrome only. With an empty catalog the start page leaves the shop unregistered and drops its tile, so the app is absent rather than an empty storefront. `content/fixture.yaml` carries the small mechanical catalog the tests need. Also fixes two things real titles exposed: * Glyph keywords matched as bare substrings, so "pen" fired on "Open Toe Sandal" and "tent" on "content". They now match whole words with an optional plural, and the table is widened against real titles: keyword coverage goes from 41/200 to 113/200, the rest taking a category default. * WebShop's coarse `category` field put nearly everything in one slug, so the mapping reads the `product_category` breadcrumb deepest-crumb-first. `scripts/render_glyph_sheet.py` renders a catalog's line art to one page, tinting the products that fell back, so coverage can be eyeballed without launching the app. Test plan: 799 passed, 6 skipped. Verified against the real dump -- 47 pages over 200 scraped products reference no external host and the seeded database holds no URLs. ruff clean on both scripts; mkdocs build --strict clean (site/ left unbuilt).
aaronsmulktis
requested review from
marksibrahim
and
a lite review from Copilot
September 2, 2026 18:15
meta-cla
Bot
added
the
CLA Signed
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness/security issues in the updated code paths (inverted Java gating for map planning, and insufficient CSS value sanitization when injecting theme CSS), plus fragile src.open_apps... imports added/modified in changed regions that can break packaged installs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR rewrites the OnlineShop app in Python and introduces shared design-token theming, while updating app gating/variation tooling and documentation so the shop is only exposed when a real catalog is present.
Changes:
- Add shared design-token theming (
apps/theme=...) with per-app overrides, and bridge legacy appearance-based apps onto the token system. - Rewrite OnlineShop to seed products/state from Hydra content packs (including an optional WebShop downloader) and generate inline SVG thumbnails (no external imagery/egress).
- Update MCP reconfigure/variant listing, screenshot tooling, tests, and docs to match the new theme/layout/content model and catalog gating.
File summaries
| File | Description |
|---|---|
| tests/test_theme.py | Adds unit tests for theme loading, resolution precedence, and CSS token rendering. |
| tests/test_onlineshop.py | Adds end-to-end tests for rewritten shop routes, search/cart/checkout behavior, gating, and no-egress guarantees. |
| tests/test_mcp.py | Updates variant-listing expectations to include layout + shared theme group. |
| tests/test_apps.py | Updates start-page expectations: shop route/tile absent when catalog is empty. |
| tests/save_screenshots.py | Updates screenshot variation generation to include shared themes and new shop routes/category URLs. |
| src/open_apps/theme.py | Introduces theme resolution + token rendering + legacy bridge CSS. |
| src/open_apps/mcp/session.py | Extends MCP session reconfigure API to accept theme/layout. |
| src/open_apps/mcp/server.py | Extends MCP tool schema/docs for theme/layout-aware reconfigure. |
| src/open_apps/mcp/registry.py | Adds shared theme variant discovery and supports layout group listing. |
| src/open_apps/mcp/README.md | Documents updated MCP tool signatures and variant groups. |
| src/open_apps/mcp/appserver.py | Implements live reconfigure overrides for theme/layout/content/appearance. |
| src/open_apps/apps/todo_app/main.py | Migrates todo styling to design tokens and injects per-request theme CSS. |
| src/open_apps/apps/start_page/main.py | Gates shop registration/tile on presence of a catalog; injects legacy theme CSS into start page. |
| src/open_apps/apps/start_page/helper.py | Extends PageWrapper to accept and emit theme CSS last in <style>. |
| src/open_apps/apps/messenger_app/main.py | Injects legacy theme bridge style per-request. |
| src/open_apps/apps/map_app/templates/map.html | Adds theme_css injection point to map HTML template. |
| src/open_apps/apps/map_app/main.py | Passes legacy theme CSS into the map template context. |
| src/open_apps/apps/codeeditor_app/main.py | Injects legacy theme bridge style per-request. |
| src/open_apps/apps/calendar_app/main.py | Injects legacy theme bridge style per-request. |
| src/open_apps/apps/onlineshop_app/templates/html_generator.py | Removes legacy HTML generator (shop rewrite no longer uses it). |
| src/open_apps/apps/onlineshop_app/search_engine/run_indexing.sh | Removes Lucene/pyserini indexing script (search rewritten). |
| src/open_apps/apps/onlineshop_app/search_engine/convert_product_file_format.py | Removes pyserini conversion utility (search rewritten). |
| src/open_apps/apps/onlineshop_app/routes/products.py | Removes legacy FastAPI routes (shop rewrite). |
| src/open_apps/apps/onlineshop_app/routes/orders.py | Removes legacy FastAPI orders route (shop rewrite). |
| src/open_apps/apps/onlineshop_app/routes/cart.py | Removes legacy FastAPI cart route (shop rewrite). |
| src/open_apps/apps/onlineshop_app/models/order.py | Removes legacy order model (shop rewrite). |
| src/open_apps/apps/onlineshop_app/models/global_state.py | Removes global-state model (shop rewrite). |
| src/open_apps/apps/onlineshop_app/models/cart.py | Removes legacy cart model (shop rewrite). |
| src/open_apps/apps/onlineshop_app/engine/normalize.py | Removes legacy reward/normalization utilities (shop rewrite). |
| src/open_apps/apps/onlineshop_app/engine/goal.py | Removes legacy reward goal logic (shop rewrite). |
| src/open_apps/apps/onlineshop_app/engine/engine.py | Removes Lucene-backed engine (shop rewrite). |
| setup.sh | Refocuses setup on optional Java 21 for map planning only. |
| setup_pyserini.sh | Removes pyserini setup script (no longer needed). |
| setup_javapath.sh | Updates JAVA_HOME paths to map app’s Java install location. |
| scripts/render_glyph_sheet.py | Adds dev utility to render all shop glyphs for a content pack. |
| scripts/fetch_webshop.py | Adds optional downloader/converter to build a gitignored webshop content pack from HF mirror. |
| README.md | Documents shop catalog build flow, theming/layout axes, SVG imagery, and no-egress approach. |
| docs/installation.md | Updates install guide: Java only for maps; adds detailed shop catalog build instructions. |
| docs/index.md | Documents new layout axis and theme-native apps. |
| docs/agents.md | Updates agent setup notes: Java optional for maps; shop no longer needs JDK. |
| config/config.yaml | Adds apps/theme: default to global defaults. |
| config/config_parallel_tasks.yaml | Switches todo dark variation to apps.todo.theme=dark. |
| config/apps/todo/layout/default.yaml | Adds todo layout group (structure-only). |
| config/apps/todo/layout/kanban_board.yaml | Adds todo kanban layout group (structure-only). |
| config/apps/todo/default.yaml | Switches todo defaults from appearance→layout and adds per-app theme override field. |
| config/apps/todo/appearance/default.yaml | Removes todo appearance variants (migrated to theme/layout). |
| config/apps/todo/appearance/dark_theme.yaml | Removes todo appearance variants (migrated to theme/layout). |
| config/apps/todo/appearance/challenging_font.yaml | Removes todo appearance variants (migrated to theme/layout). |
| config/apps/todo/appearance/black_and_white.yaml | Removes todo appearance variants (migrated to theme/layout). |
| config/apps/todo/appearance/kanban_board.yaml | Removes todo appearance variant (replaced by layout group). |
| config/apps/theme/default.yaml | Adds default shared token theme. |
| config/apps/theme/dark.yaml | Adds dark shared token theme. |
| config/apps/theme/mono.yaml | Adds mono shared token theme. |
| config/apps/theme/solarized.yaml | Adds solarized shared token theme. |
| config/apps/theme/material.yaml | Adds material shared token theme. |
| config/apps/theme/bootstrap.yaml | Adds bootstrap shared token theme. |
| config/apps/theme/challenging_font.yaml | Adds challenging-font shared token theme. |
| config/apps/start_page/default.yaml | Adds per-app theme override field for start page. |
| config/apps/onlineshop/layout/default.yaml | Adds shop layout group (structure-only). |
| config/apps/onlineshop/layout/grid.yaml | Adds grid shop layout variant. |
| config/apps/onlineshop/layout/compact_table.yaml | Adds compact-table shop layout variant. |
| config/apps/onlineshop/default.yaml | Switches shop defaults to layout group; defines new DB path and theme override; enables by default. |
| config/apps/onlineshop/content/default.yaml | Makes default pack chrome-only with empty catalog and adds category strip labels. |
| config/apps/onlineshop/content/german.yaml | Translates chrome category labels for German variation. |
| config/apps/onlineshop/content/fixture.yaml | Adds small deterministic fixture catalog for tests (plus seeded cart/orders). |
| config/apps/onlineshop/appearance/default.yaml | Removes shop appearance variants (migrated to theme/layout). |
| config/apps/onlineshop/appearance/dark_theme.yaml | Removes shop appearance variants (migrated to theme/layout). |
| config/apps/onlineshop/appearance/challenging_font.yaml | Removes shop appearance variants (migrated to theme/layout). |
| config/apps/onlineshop/appearance/black_and_white.yaml | Removes shop appearance variants (migrated to theme/layout). |
| config/apps/messenger/default.yaml | Adds per-app theme override field for messenger. |
| config/apps/maps/default.yaml | Adds per-app theme override field for maps. |
| config/apps/code_editor/default.yaml | Adds per-app theme override field for code editor. |
| config/apps/calendar/default.yaml | Adds per-app theme override field for calendar. |
| .gitignore | Updates ignores for map Java install and gitignored generated webshop catalog content pack. |
| pyproject.toml | Removes pyserini/spacy/thefuzz/gdown deps now that the shop rewrite drops those features/dependencies. |
Review details
Suppressed comments (1)
src/open_apps/apps/start_page/main.py:175
- Map route-planning gating is inverted: this disables planning when Java 21 is available, and leaves it on when Java is missing. The condition should be negated so planning is only turned off when Java 21 is not present.
- Files reviewed: 75/77 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+15
to
+16
| from src.open_apps.frontend import local_hdrs | ||
| from src.open_apps.theme import legacy_theme_style |
Comment on lines
+13
to
+15
| from src.open_apps.apps.start_page.helper import create_logo_header | ||
| from src.open_apps.frontend import local_hdrs | ||
| from src.open_apps.theme import legacy_theme_style |
| from fasthtml.common import * | ||
| import requests | ||
| import json | ||
| from src.open_apps.theme import legacy_theme_css |
Comment on lines
+13
to
+15
| from src.open_apps.apps.start_page.helper import create_logo_header | ||
| from open_apps.frontend import local_hdrs | ||
| from open_apps.theme import legacy_theme_style |
Comment on lines
+11
to
+13
| from src.open_apps.apps.start_page.helper import create_logo_header | ||
| from src.open_apps.frontend import local_hdrs | ||
| from src.open_apps.theme import theme_style |
Keep the source catalog's image URLs and render them when asked, instead of discarding them at import. `apps.onlineshop.product_images` picks between `glyphs` (default) and `hotlink`. Under `hotlink` a product with several images becomes a CSS-only carousel -- one hidden radio per slide, one clickable dot per image -- so the controls are real elements an agent can click with no JavaScript and no new dependency. A single image renders without controls, and a product with none falls back to its glyph. The glyph also ships inside every carousel as the fallback: each <img> has an onerror that reveals it, so a failed fetch degrades to line art rather than a broken-image icon. One failure takes the whole carousel down deliberately -- the common case is "no network", where all of them fail, and a half-populated carousel is a worse observation than a consistent one. Default is `glyphs`, and an unrecognised value falls back to it, because hotlinking is only safe when the network is really there: * the eval nodes have no outbound network, so the images do not arrive while the page still returns 200 and a screenshot-scored agent gets graded on an observation that quietly lost its imagery; * TestNoEgress fails under `hotlink`, by design; * for the WebShop catalog the URLs point at Amazon's CDN. The importer's URL guard is now field-scoped rather than whole-record: text fields still must not contain a URL, since those render under either mode, while `images` is the one place a URL is legitimate. Amazon's transparent spacer gif is filtered out of the image list, and images are capped at four to match the carousel dot rules. Test plan: 809 passed, 6 skipped. Verified both modes over the real 200- product catalog -- `glyphs` emits no product-media markup and references no external host, `hotlink` emits 34 slides against m.media-amazon.com. Browser rendering unverified: chromium crashes in this sandbox, so the carousel is checked at the markup level (checked-radio adjacency, unique radio group per sku, glyph sibling present) rather than visually.
Six app modules imported their siblings through `src.open_apps...`, which only resolves because `open_apps/__init__.py` pushes the repo root onto sys.path on package import. That shim works, but it means the same module is loaded twice under two names -- `src.open_apps.theme` and `open_apps.theme` are distinct objects with separate globals -- so identity checks and any module-level state silently diverge depending on which spelling a caller used. Import them the way the package is installed instead. The shim stays: it is a public compatibility path and something outside this repo may still use the old spelling. Also guard `render_theme_css` against a theme token containing `<` or `>`, which could otherwise close the <style> block it is rendered into. No shipped theme has such a value, so this is defensive only -- the neighbouring checks already cover the property name and the @import URL. Test plan: 809 passed, 6 skipped. Confirmed no module remains loaded under the `src.` name after importing the apps, and that the theme guard drops an injected `</style><script>` value while leaving quoted font stacks intact.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes:
apps/onlineshop/content=fixtureRunning it:
uv run scripts/fetch_webshop.py(seeds products from HF)uv run launch.py apps/onlineshop/content=webshop