fix(options): declare every site option the theme reads, so they reach it - #194
Merged
Merged
Conversation
…h it The MyST CLI validates `site.options` against template.yml and drops every key the template does not declare, and only the six multilingual options were declared. `twitter`, `favicon`, `analytics_google`, `analytics_plausible`, `hide_toc`, `hide_search` and the five `launch_*` keys were read by the theme but never arrived from myst.yml. All are declared now, with no defaults (a declared default is written into every page's validated `site:` block and overrides the site-wide value); the four keys the theme never read (`hide_outline`, `hide_title_block`, `hide_footer_links`, `outline_maxdepth`) are dropped from its types instead of being declared as dead knobs. The page-level `git_metadata` override is declared too, as a string holding a YAML block (template options are scalar-only), parsed by PageHeaderHistory; an object still works. Undeclared, it survived only because the CLI leaves a page's raw `site:` block alone when nothing in it validates, so any page that also set a declared key lost it silently. The fixture's features page now sets both, which is the regression case. `favicon` had a second blocker: a static public/favicon.ico, served ahead of the `[favicon.ico]` route, shadowed the option and the fallback alike. It is the QuantEcon lectures favicon (byte-identical to the Sphinx sites'), so it moves to public/logos/ and becomes the route's default; a configured favicon is fetched from the content server as the CLI copied it. A `site-options` test asserts end to end that a declared option reaches the rendered page (`twitter:creator`), that the configured favicon is served byte for byte, and that the default is served when unset. Closes #173 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
|
Contributor
🎭 Visual regression results
Details
Skipped testsmobile-chrome › theme.spec.ts › QuantEcon theme — visual regression › without JavaScript › drawer-opens-without-javascript |
There was a problem hiding this comment.
🟡 Changes recommended
A few blocking issues remain around changelog convention compliance (missing PR links) and defensive robustness/performance concerns identified in the updated favicon and git-metadata parsing paths.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a theme contract gap with the MyST CLI by declaring all site.options keys the theme reads in template.yml, ensuring those options aren’t silently dropped during validation. It also unblocks favicon configuration by removing the static-file shadowing behavior and adds an end-to-end visual test to prove options flow through to rendered output.
Changes:
- Declare all theme-consumed
site.optionsintemplate.yml(including scalar-YAML-block workarounds for structured data likegit_metadata). - Make
faviconeffective by serving a default favicon via the/favicon.icoroute when unset, and fetching the configured file when set. - Add a Playwright visual test asserting that declared options (twitter + favicon) reach the rendered page and that favicon bytes match expected fixtures.
File summaries
| File | Description |
|---|---|
| tests/visual/theme.spec.ts | Adds an end-to-end “site options reach the theme” assertion (twitter meta + favicon bytes). |
| tests/visual/fixture/myst.yml.in | Sets declared twitter and favicon options for the main visual fixture. |
| tests/visual/fixture/features.md | Converts site.git_metadata override to YAML-block-string form and adds a declared per-page key alongside it. |
| tests/visual/fixture-rtl/myst.yml.in | Removes unused logo_text from RTL fixture options. |
| tests/visual/fixture-no-thebe/myst.yml.in | Removes unused logo_text from no-thebe fixture options. |
| template.yml | Declares all options the theme reads; documents scalar-only limitation and the “no defaults” rule. |
| README.md | Documents the full site-options surface and the YAML-block-string override form for git_metadata. |
| public/logos/lectures-favicon.png | Adds the new default favicon asset used when favicon is unset. |
| plugins/git-metadata.mjs | Updates documentation to match the new declared-option scalar/YAML-block-string requirement. |
| CHANGELOG.md | Adds Unreleased entries describing the options + favicon + git_metadata behavior changes. |
| app/types.ts | Aligns TS option typing with declared template options; removes previously-typed-but-unused layout knobs. |
| app/components/PageHeaderHistory.tsx | Parses site.git_metadata via parseStructured and validates basic shape before use. |
| app/backend/loaders.server.ts | Implements favicon defaulting and fetch logic when configured via declared favicon option. |
Review details
Suppressed comments (2)
CHANGELOG.md:42
- This new Unreleased entry links only to the issue; CONTRIBUTING.md requires linking each changelog entry to the PR as well, so readers can find the actual change diff.
the CLI copied it ([#173](https://github.com/QuantEcon/quantecon-theme.mystmd/issues/173)).
CHANGELOG.md:49
- This new Unreleased entry links only to the issue; CONTRIBUTING.md requires linking each changelog entry to the PR as well, so readers can find the actual change diff.
([#173](https://github.com/QuantEcon/quantecon-theme.mystmd/issues/173)).
- Files reviewed: 12/13 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot review on #194: the default favicon is read from disk once and cached rather than on every request; the parsed git_metadata override keeps only changelog entries the renderer can use (string hash and date); and the three changelog entries link the PR as CONTRIBUTING requires. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
mmcky
added a commit
that referenced
this pull request
Sep 10, 2026
This was referenced Sep 11, 2026
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.
Closes #173. First step of the remaining #147 sequence: both #92 (meta/SEO) and #114 (the per-lecture live-compute flag) need declared options, and #114's page flag would have collided with the undeclared
git_metadataoverride.What was wrong
The MyST CLI validates
site.optionsagainst the template's declarations and drops every key the template does not declare. Until #174 this theme declared nothing; since then only the six multilingual options. Sotwitter,favicon,analytics_google,analytics_plausible,hide_toc,hide_searchand the fivelaunch_*keys were read by the theme but never arrived frommyst.yml. Verified before the change: the fixture'ssite.optionsreachedconfig.jsonwith only the six declared keys.What changes
template.ymldeclares every option the theme reads (18 in total), grouped and described, with no defaults. The four keys typed but never read anywhere (hide_outline,hide_title_block,hide_footer_links,outline_maxdepth) are removed fromapp/types.tsrather than declared as knobs that do nothing.logo_text, which the fixtures set and nothing reads, is dropped from the fixtures.git_metadatadecision. The page-level override is declared as astringholding a YAML block, the same form #174 used forlanguagesandtranslators, andPageHeaderHistoryparses it with the existingparseStructured(an object still passes through, so older content keeps working). This was the open question in the issue. The alternative, moving the override into the AST data the plugin writes, would have needed the plugin to read page frontmatter and is a plugin-repo change now that the plugin is moving to QuantEcon/quantecon-plugins.mystmd; declaring it keeps the override a theme contract. The fixture's features page now setsgit_metadataand a declared key (hide_search) on the same page, which is exactly the case that used to lose the override silently.faviconhad a second blocker. With the option declared the CLI copies the file and rewrites the path correctly (measured:/cc-by-sa-4.0-80x15-<hash>.png, served by the content server asimage/png), but the theme still returned a 32×32 PNG asimage/x-icon: a staticpublic/favicon.icowas served ahead of the[favicon.ico]route on every request, so neither the option nor the mystmd fallback was ever reachable. That file is byte-identical to the Sphinx lectures'_static/lectures-favicon.ico, so it is the right default and moves topublic/logos/lectures-favicon.png;getFaviconserves it from disk when no option is set and fetches the configured file otherwise.Verification
A new
site-optionstest (desktop-chrome) asserts end to end that a declared site-wide option reaches the rendered page (twitter:creator="@quantecon"), that/favicon.icoon the main fixture is byte-for-byte the configured PNG, and that/favicon.icoon the no-thebe fixture (no option) is byte-for-byte the lectures favicon. The page-level path is covered by the existinghistory-opensnapshot, which only renders because the features page'sgit_metadatablock now survives validation beside a declared key. Full local suite: 37 passed, no visual baseline moved;npm run compile, the 27 unit tests and the plugin end-to-end test pass.One thing surfaced for #92 rather than fixed here: pages emit
twitter:creatorbut nottwitter:site, because the root route's meta (wheretwitter:sitelives) is replaced, not merged, by the article route's meta under Remix v2 semantics. The test asserts the tag pages actually render and says so in a comment.Docs
README gains a "Site options" section with the full table and scope of each option, the git-history section documents the block-string override form, and the CHANGELOG has entries under Unreleased.
🤖 Generated with Claude Code