From c1a36133aeb61d058dedbf214be69f700489d5b4 Mon Sep 17 00:00:00 2001 From: Matt McKay Date: Thu, 10 Sep 2026 21:27:20 +1000 Subject: [PATCH 1/2] fix(options): declare every site option the theme reads, so they reach 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 --- CHANGELOG.md | 28 +++++++ README.md | 35 ++++++++- app/backend/loaders.server.ts | 38 +++++++++- app/components/PageHeaderHistory.tsx | 18 ++++- app/types.ts | 31 +++++--- plugins/git-metadata.mjs | 4 +- .../lectures-favicon.png} | Bin template.yml | 71 +++++++++++++++--- tests/visual/fixture-no-thebe/myst.yml.in | 1 - tests/visual/fixture-rtl/myst.yml.in | 1 - tests/visual/fixture/features.md | 9 ++- tests/visual/fixture/myst.yml.in | 7 +- tests/visual/theme.spec.ts | 43 +++++++++++ 13 files changed, 254 insertions(+), 32 deletions(-) rename public/{favicon.ico => logos/lectures-favicon.png} (100%) diff --git a/CHANGELOG.md b/CHANGELOG.md index f313395dc..46e1d9ccd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,34 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- Every site option the theme reads is now declared in `template.yml`, so it + actually arrives. The MyST CLI validates `site.options` against the + template's declarations and drops every undeclared key, and until now only + the six multilingual options were declared: `twitter`, `favicon`, + `analytics_google`, `analytics_plausible`, `hide_toc`, `hide_search` and the + five `launch_*` launcher keys were read by the theme but never reached it + from `myst.yml`. `favicon` is a `file` option, so the CLI copies it into the + build. A fixture assertion (`site-options` in `theme.spec.ts`) proves a + declared option reaches the rendered page. Options the theme never read + (`hide_outline`, `hide_title_block`, `hide_footer_links`, `outline_maxdepth`) + are dropped from its types rather than declared as dead knobs + ([#173](https://github.com/QuantEcon/quantecon-theme.mystmd/issues/173)). +- The `favicon` option now takes effect at all. Beyond being undeclared, it + was shadowed by a static `public/favicon.ico`, served ahead of the theme's + favicon route on every request. That file is the QuantEcon lectures favicon + (byte-identical to the Sphinx sites'), so it moves to + `public/logos/lectures-favicon.png` and becomes the route's default when no + option is set; a configured favicon is fetched from the content server as + the CLI copied it ([#173](https://github.com/QuantEcon/quantecon-theme.mystmd/issues/173)). +- The page-level `git_metadata` override is a declared option too, written as + a YAML block string (template options are scalar-only). It previously + survived only because the CLI leaves a page's raw `site:` block alone when + nothing in it validates, so a page that also set any declared key (a + translator credit, `hide_search`, and the per-lecture live-compute flag to + come) silently lost its override. An object is still accepted + ([#173](https://github.com/QuantEcon/quantecon-theme.mystmd/issues/173)). + ### Changed - The release bundle's manifest now declares Node `>=20` (it said `>=16`, contradicting CONTRIBUTING.md and every real floor in the repo) and installs diff --git a/README.md b/README.md index 05f1ff062..8a0ef1375 100644 --- a/README.md +++ b/README.md @@ -67,8 +67,9 @@ lecture repo had ever configured one. For running cells without leaving the page, see [Live compute](#live-compute-thebe--jupyterlite) below. The repo/branch/path conventions are configurable under `site.options` in -`myst.yml` (MyST's theme-options section). All keys are optional and the -defaults reproduce the behaviour above, so existing projects need no changes: +`myst.yml` (MyST's theme-options section; see [Site options](#site-options) +for the full list). All keys are optional and the defaults reproduce the +behaviour above, so existing projects need no changes: | Option | Default | Purpose | | ------ | ------- | ------- | @@ -143,6 +144,10 @@ Notes: - The header control renders nothing when no metadata is present, so projects without the plugin are unaffected. +- A page can pin the control by hand with the same shape under `site:` in its + frontmatter, as a YAML block string (`git_metadata: |` then the indented + `last_modified` and `changelog`); it takes precedence over the injected data. + `tests/visual/fixture/features.md` is an example. - The plugin is a silent no-op for untracked files, non-git checkouts, missing `git`, or a `git log` timeout (5s). Shallow CI clones (`fetch-depth: 1`) produce truncated history — use `fetch-depth: 0` when building for deploy. @@ -200,6 +205,32 @@ One trap: a page whose `site:` frontmatter sets any of these keys has its whole `site:` block replaced by the validated keys, so an undeclared `site.git_metadata` on the same page is lost. Keep the two on different pages. +## Site options + +Everything the theme reads from `site.options` is declared in +[`template.yml`](./template.yml). That list is not documentation only: the MyST +CLI validates `site.options` against it and **drops any key the template does +not declare**, so an undeclared option never reaches the theme +([#173](https://github.com/QuantEcon/quantecon-theme.mystmd/issues/173)). +Template options are scalar-only, so the structured ones are written as a YAML +block inside a string (`key: |`), which the theme parses. + +| Option | Scope | Purpose | +| ------ | ----- | ------- | +| `twitter` | site | Handle for the `twitter:site` / `twitter:creator` card meta tags | +| `favicon` | site | Favicon file, relative to `myst.yml`; served at `/favicon.ico` (the QuantEcon lectures favicon when unset) | +| `analytics_google`, `analytics_plausible` | site | Analytics IDs, rendered by `@myst-theme/site` | +| `hide_toc`, `hide_search` | site or page | Hide the contents drawer / the search control | +| `launch_repo_url`, `launch_repo_suffix`, `launch_branch`, `launch_notebooks_path`, `launch_source_path` | site | Notebook launcher conventions ([Launch buttons](#launch-buttons)) | +| `current_language`, `enable_rtl`, `languages`, `language_switcher_label` | site | Multilingual editions ([below](#multilingual-editions)) | +| `translators`, `translators_label` | site or page | Translator credit in the page header | +| `git_metadata` | page | YAML block pinning the "Last changed" control by hand ([Git history](#git-history-in-page-headers)) | + +A page-level value goes under `site:` in the page's frontmatter and is +validated against the same list. Options carry no defaults in `template.yml` +(a declared default would be written into every page's validated block and +override the site-wide value); defaults live in the code that reads each one. + ## Usage with MyST Point your project's `site.template` at a **pinned release** zip: diff --git a/app/backend/loaders.server.ts b/app/backend/loaders.server.ts index 9300b731d..265224a7b 100644 --- a/app/backend/loaders.server.ts +++ b/app/backend/loaders.server.ts @@ -1,3 +1,5 @@ +import fs from "node:fs"; +import path from "node:path"; import fetch from "node-fetch"; import type { SiteManifest } from "myst-config"; import { @@ -131,13 +133,47 @@ export async function getMystSearchJson(): Promise { return await response.json(); } +/** + * The QuantEcon lectures favicon (byte-identical to the Sphinx sites' + * `_static/lectures-favicon.ico`), served when no `favicon` option is set. + * It lives under public/logos/ rather than at public/favicon.ico: a static + * file at that path is served ahead of the `[favicon.ico]` route, which is + * why the `favicon` site option never took effect before #173. + */ +const DEFAULT_FAVICON = "public/logos/lectures-favicon.png"; + +function readDefaultFavicon(): { contentType: string; buffer: Buffer } | null { + // The theme server runs from the bundle directory (`build.start` in + // template.yml), where public/ sits beside build/; the second candidate + // covers being launched from elsewhere. + const candidates = [ + path.resolve(process.cwd(), DEFAULT_FAVICON), + path.resolve(__dirname, "..", DEFAULT_FAVICON), + ]; + for (const file of candidates) { + try { + return { contentType: "image/png", buffer: fs.readFileSync(file) }; + } catch { + // try the next location + } + } + return null; +} + export async function getFavicon(): Promise<{ contentType: string | null; buffer: Buffer; } | null> { // We are always fetching this at run time, so we don't want the rewritten links const config = await getConfig({ rewriteStaticFolder: false }); - const url = config.options?.favicon || "https://mystmd.org/favicon.ico"; + // A declared `file` option: the CLI copied the site's file into its public + // folder and getConfig rewrote the path onto the content server. + const url = config.options?.favicon; + if (!url) return readDefaultFavicon() ?? fetchFavicon("https://mystmd.org/favicon.ico"); + return fetchFavicon(url); +} + +async function fetchFavicon(url: string) { const response = await fetch(url).catch(() => null); if (!response || response.status === 404) return null; return { diff --git a/app/components/PageHeaderHistory.tsx b/app/components/PageHeaderHistory.tsx index b5bf86bc7..059565369 100644 --- a/app/components/PageHeaderHistory.tsx +++ b/app/components/PageHeaderHistory.tsx @@ -1,3 +1,4 @@ +import { parseStructured } from "~/i18n"; import { ChevronDown } from "lucide-react"; import React from "react"; import { usePage } from "./PageProvider"; @@ -67,12 +68,24 @@ const COPY = "text-[0.85rem]"; * * Data sources, in order of precedence: * 1. `site.git_metadata` in the page frontmatter (manual override, and how - * the visual fixture pins deterministic data), then + * the visual fixture pins deterministic data) -- a declared template + * option, so it arrives as a string holding a YAML block (template + * options are scalar-only; see app/i18n.ts) or, from older content, as + * an object; then * 2. `mdast.data.git_metadata` injected at build time by * plugins/git-metadata.mjs. * * Renders nothing when neither is present. */ + +/** The parsed override only counts when it has the plugin's shape. */ +function asGitMetadata(value: unknown): GitMetadata | undefined { + if (!value || typeof value !== 'object' || Array.isArray(value)) return undefined; + const v = value as Partial; + if (!v.last_modified && !Array.isArray(v.changelog)) return undefined; + return { last_modified: v.last_modified, changelog: Array.isArray(v.changelog) ? v.changelog : [] } as GitMetadata; +} + export function PageHeaderHistory({ alignEnd = true }: { alignEnd?: boolean } = {}) { const page = usePage(); // `ms-auto` pushes the control to the end of the header row. When the @@ -89,7 +102,8 @@ export function PageHeaderHistory({ alignEnd = true }: { alignEnd?: boolean } = const frontmatter = page?.frontmatter as any; const meta: GitMetadata | undefined = - frontmatter?.site?.git_metadata ?? (page?.mdast as any)?.data?.git_metadata; + asGitMetadata(parseStructured(frontmatter?.site?.git_metadata)) ?? + (page?.mdast as any)?.data?.git_metadata; const changelog = meta?.changelog ?? []; const lastModified = meta?.last_modified ?? changelog[0]?.date; if (!lastModified) return null; diff --git a/app/types.ts b/app/types.ts index 1de51f27e..e52e5b873 100644 --- a/app/types.ts +++ b/app/types.ts @@ -1,18 +1,29 @@ import type { GitMetadata } from './components/PageHeaderHistory'; export interface TemplateOptions { + // Every key here is declared in template.yml: the CLI drops any + // `site.options` key the template does not declare (#173), so an option + // that is read but not declared never arrives. Keep the two in step. + + // Layout. Both are per-page: Page.tsx merges a page's `site:` block over + // the site-wide options. hide_toc?: boolean; - hide_outline?: boolean; hide_search?: boolean; - hide_footer_links?: boolean; - outline_maxdepth?: number; - hide_title_block?: boolean; + + // Meta / SEO and analytics, passed through to @myst-theme/site. + twitter?: string; // handle for twitter:site / twitter:creator, `@` optional + favicon?: string; // declared `file`: the CLI copies it and rewrites this to its served path + analytics_google?: string; + analytics_plausible?: string; + /** * Page-level override for the "Last changed" header control, normally * injected at build time by plugins/git-metadata.mjs (set under `site:` in - * page frontmatter). + * page frontmatter). Declared as a string holding a YAML block, like the + * lists below, because template options are scalar-only; a real object is + * accepted too (app/i18n.ts `parseStructured`). */ - git_metadata?: GitMetadata; + git_metadata?: string | GitMetadata; // Notebook launcher configuration (set under `site.options` in myst.yml). // Generalises the previously hardcoded Colab launch URLs so @@ -35,10 +46,10 @@ export interface TemplateOptions { // PageContent); the other four are site-wide -- `current_language` and // `enable_rtl` come from the root loader's config, `languages` and the // switcher label from the site manifest -- and a page value is ignored. - // Caution: a page that sets any declared key here has its whole `site:` - // block replaced by the validated keys, so an undeclared `git_metadata` on - // the same page is lost -- keep the two on different pages, as the visual - // fixture does. + // A page that sets any declared key has its whole `site:` block replaced + // by the validated keys, which is why `git_metadata` above is declared too: + // undeclared, it was silently lost from any page that also set one of + // these. current_language?: string; // BCP 47 code of this edition; document `lang`, active switcher entry enable_rtl?: boolean; // dir="rtl" on the document languages?: string | unknown[]; // YAML block of `{code, name, url}`; needs 2+ entries to render diff --git a/plugins/git-metadata.mjs b/plugins/git-metadata.mjs index c3bc06987..c27c6faf4 100644 --- a/plugins/git-metadata.mjs +++ b/plugins/git-metadata.mjs @@ -15,7 +15,9 @@ * The QuantEcon theme renders this as a "Last changed" header control with a * changelog dropdown (app/components/PageHeaderHistory.tsx). Pages can also * set the same shape manually under `site.git_metadata` in their frontmatter, - * which takes precedence over the injected data. + * which takes precedence over the injected data. Write it as a YAML block + * string (`git_metadata: |` followed by the indented YAML): it is a declared + * template option, and the CLI can only declare scalar types (#173). * * Mirrors quantecon-book-theme's get_git_last_modified/get_git_changelog: * per-file `git log --follow`, a hard timeout, and a silent no-op when the diff --git a/public/favicon.ico b/public/logos/lectures-favicon.png similarity index 100% rename from public/favicon.ico rename to public/logos/lectures-favicon.png diff --git a/template.yml b/template.yml index eb160fa55..e543e4a2d 100644 --- a/template.yml +++ b/template.yml @@ -21,18 +21,16 @@ parts: - id: footer description: The site wide footer # Site options (`site.options` in myst.yml). The CLI validates against this -# list and DROPS any key not declared here, and it can only declare scalar -# types -- so the two lists (`languages`, `translators`) are strings holding a -# YAML block; app/i18n.ts parses them. Only `translators` and -# `translators_label` are also read per page (under `site:` in page -# frontmatter); the rest are site-wide. See README "Multilingual editions". +# list and DROPS any key not declared here (#173), so every option the theme +# reads is declared below -- and it can only declare scalar types, so the +# structured ones (`languages`, `translators`, `git_metadata`) are strings +# holding a YAML block; app/i18n.ts `parseStructured` turns them back into +# data. Site-wide unless marked per-page; a per-page value goes under `site:` +# in the page's frontmatter and is validated against this same list. # -# No `default:` on any option, deliberately. The CLI validates every page's -# `site:` frontmatter against this list too, and a declared default is written -# into that validated block even when the page set nothing -- which then -# replaces the page's raw `site:` block, overriding the site-wide value and -# discarding undeclared keys such as `git_metadata`. Defaults live in -# app/i18n.ts instead. +# No `default:` on any option, deliberately. A declared default is written +# into every page's validated `site:` block even when the page set nothing, +# which then overrides the site-wide value. Defaults live in code. options: - id: current_language type: string @@ -64,6 +62,57 @@ options: description: > Accessible name and tooltip of the language switcher, in the edition's own language ("Switch language" when unset). + # --- Meta / SEO --- + - id: twitter + type: string + description: > + Twitter/X handle for the `twitter:site` and `twitter:creator` card meta + tags, with or without the leading `@`. + - id: favicon + type: file + description: > + Site favicon, a path relative to myst.yml. Served at `/favicon.ico` + (the QuantEcon lectures favicon when unset). + # --- Analytics (rendered by @myst-theme/site's Analytics component) --- + - id: analytics_google + type: string + description: Google Analytics measurement ID (e.g. `G-XXXXXXXXXX`). + - id: analytics_plausible + type: string + description: Plausible analytics domain (e.g. `python.quantecon.org`). + # --- Layout (per-page: a page may set either under `site:`) --- + - id: hide_toc + type: boolean + description: Hide the contents drawer and its toolbar toggle. + - id: hide_search + type: boolean + description: Hide the toolbar search control. + # --- Notebook launcher (see README "Launch buttons") --- + - id: launch_repo_url + type: string + description: > + Explicit notebook repository (full URL or `org/repo`); overrides + `` + `launch_repo_suffix`. + - id: launch_repo_suffix + type: string + description: Suffix appended to the source repo to locate the notebook repo (`.notebooks` when unset). + - id: launch_branch + type: string + description: Branch of the notebook repo to launch from (`main` when unset). + - id: launch_notebooks_path + type: string + description: Sub-directory of the notebook repo where the notebooks live. + - id: launch_source_path + type: string + description: Path prefix stripped from the page location (e.g. `lectures/`). + # --- Git history (per-page override of plugins/git-metadata.mjs) --- + - id: git_metadata + type: string + description: > + YAML block with `last_modified` and a `changelog` list of `{hash, + short_hash, author, date, message}`, set under `site:` in a page's + frontmatter to pin the "Last changed" control by hand. Normally injected + at build time by the git-metadata plugin instead. build: # `npm ci`, not `npm install`: the release asset ships a lockfile (release.yml # generates it with `npm install --package-lock-only` and it is in `files` diff --git a/tests/visual/fixture-no-thebe/myst.yml.in b/tests/visual/fixture-no-thebe/myst.yml.in index 803d7b408..862b58e2f 100644 --- a/tests/visual/fixture-no-thebe/myst.yml.in +++ b/tests/visual/fixture-no-thebe/myst.yml.in @@ -14,4 +14,3 @@ site: title: QE Theme No-Thebe Fixture template: __THEME__ options: - logo_text: QE No-Thebe Fixture diff --git a/tests/visual/fixture-rtl/myst.yml.in b/tests/visual/fixture-rtl/myst.yml.in index 25f3bb6ef..99f4c5f82 100644 --- a/tests/visual/fixture-rtl/myst.yml.in +++ b/tests/visual/fixture-rtl/myst.yml.in @@ -16,7 +16,6 @@ site: title: QE Theme RTL Fixture template: __THEME__ options: - logo_text: QE RTL Fixture current_language: fa enable_rtl: true language_switcher_label: تغییر زبان diff --git a/tests/visual/fixture/features.md b/tests/visual/fixture/features.md index 5b7dd678e..d0f18faca 100644 --- a/tests/visual/fixture/features.md +++ b/tests/visual/fixture/features.md @@ -7,8 +7,10 @@ authors: site: # Deterministic stand-in for plugins/git-metadata.mjs output, so the header # history control renders identically on every run (real git dates would - # change with each commit and churn the snapshots). - git_metadata: + # change with each commit and churn the snapshots). A YAML block string, + # because it is a declared template option and those are scalar-only + # (#173); PageHeaderHistory parses it. + git_metadata: | last_modified: '2026-01-15T10:30:00Z' changelog: - hash: 3f9d2c41b8a7e6f5d4c3b2a1908f7e6d5c4b3a29 @@ -26,6 +28,9 @@ site: author: Matt McKay date: '2025-09-20T14:45:00Z' message: Initial features fixture + # A declared per-page option on the same page: before #173 this would have + # replaced the page's `site:` block and silently dropped the override above. + hide_search: false --- # Features diff --git a/tests/visual/fixture/myst.yml.in b/tests/visual/fixture/myst.yml.in index b756e67f6..b0c1d33fa 100644 --- a/tests/visual/fixture/myst.yml.in +++ b/tests/visual/fixture/myst.yml.in @@ -20,7 +20,12 @@ site: title: QE Theme Visual Fixture template: __THEME__ options: - logo_text: QuantEcon Visual Fixture + # Site-wide options declared in template.yml (#173). `twitter` and + # `favicon` are asserted end to end in theme.spec.ts ("site-options"): + # the CLI drops undeclared keys, so this is the fixture's proof that a + # declared one reaches the theme. + twitter: quantecon + favicon: cc-by-sa-4.0-80x15.png # Multilingual editions (#90, #143). Two editions make the toolbar's # language switcher and the hreflang alternates render on every page; the # lists are YAML blocks because template options are scalar-only (see diff --git a/tests/visual/theme.spec.ts b/tests/visual/theme.spec.ts index 693e86915..46f7df340 100644 --- a/tests/visual/theme.spec.ts +++ b/tests/visual/theme.spec.ts @@ -332,6 +332,49 @@ test.describe("QuantEcon theme — visual regression", () => { * translator with a page-level override on `/` and a suppression on * `/lists`; `fixture-rtl` is the Persian edition with `enable_rtl`. */ +test.describe("Site options reach the theme (#173)", () => { + // The CLI validates `site.options` against template.yml and DROPS every key + // the template does not declare, so a theme that reads an undeclared option + // silently runs on its default. These assert, end to end, that a declared + // site-wide option arrives: the fixture sets `twitter` and `favicon` in + // myst.yml.in. The page-level path (`site:` in a page's frontmatter) is + // covered by the `history-open` snapshot above, whose "Last changed" control + // exists only because features.md's `git_metadata` block survives + // validation. + test("site-options", async ({ page, request }, testInfo) => { + test.skip(testInfo.project.name !== "desktop-chrome", "not viewport-dependent"); + await page.goto("/features", { waitUntil: "domcontentloaded" }); + // `twitter:creator`, not `twitter:site`: the article routes' meta() emits + // the former, and under Remix v2 meta semantics the leaf route's tags + // replace the root's (where `twitter:site` lives) rather than merging. + // That gap is Phase 6's (#92); what is asserted here is only that the + // declared option reached the theme at all. + await expect(page.locator('head meta[name="twitter:creator"]')).toHaveAttribute( + "content", + "@quantecon" + ); + // `favicon` is a `file` option: the CLI copies the fixture's PNG into the + // build and the theme serves it at /favicon.ico. Match on the bytes, not + // just the status -- the mystmd default favicon is also a 200. + const served = await request.get("/favicon.ico"); + expect(served.status()).toBe(200); + expect(served.headers()["content-type"]).toContain("image/png"); + const fs = await import("node:fs"); + const fixture = fs.readFileSync("tests/visual/fixture/cc-by-sa-4.0-80x15.png"); + expect(Buffer.from(await served.body()).equals(fixture)).toBe(true); + // And the default when the option is unset (the no-thebe fixture): the + // QuantEcon lectures favicon, byte-identical to the Sphinx sites' one. + // It used to sit at public/favicon.ico, where the static file shadowed the + // route and the option with it. + const noThebe = `http://localhost:${process.env.NO_THEBE_PORT || "3112"}`; + const fallback = await request.get(`${noThebe}/favicon.ico`); + expect(fallback.status()).toBe(200); + expect(fallback.headers()["content-type"]).toContain("image/png"); + const lectures = fs.readFileSync("public/logos/lectures-favicon.png"); + expect(Buffer.from(await fallback.body()).equals(lectures)).toBe(true); + }); +}); + test.describe("Multilingual editions", () => { const rtlBase = `http://localhost:${process.env.RTL_PORT || "3113"}`; From 8d56a86599182027ba0d9c0c58b2b268751e044a Mon Sep 17 00:00:00 2001 From: Matt McKay Date: Thu, 10 Sep 2026 21:35:21 +1000 Subject: [PATCH 2/2] fix(options): harden the favicon default and the git_metadata override 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 --- CHANGELOG.md | 6 +++--- app/backend/loaders.server.ts | 10 ++++++++-- app/components/PageHeaderHistory.tsx | 20 ++++++++++++++++---- 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 46e1d9ccd..8ffb367be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,21 +32,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 declared option reaches the rendered page. Options the theme never read (`hide_outline`, `hide_title_block`, `hide_footer_links`, `outline_maxdepth`) are dropped from its types rather than declared as dead knobs - ([#173](https://github.com/QuantEcon/quantecon-theme.mystmd/issues/173)). + ([#173](https://github.com/QuantEcon/quantecon-theme.mystmd/issues/173)) ([#194](https://github.com/QuantEcon/quantecon-theme.mystmd/pull/194)). - The `favicon` option now takes effect at all. Beyond being undeclared, it was shadowed by a static `public/favicon.ico`, served ahead of the theme's favicon route on every request. That file is the QuantEcon lectures favicon (byte-identical to the Sphinx sites'), so it moves to `public/logos/lectures-favicon.png` and becomes the route's default when no option is set; a configured favicon is fetched from the content server as - the CLI copied it ([#173](https://github.com/QuantEcon/quantecon-theme.mystmd/issues/173)). + the CLI copied it ([#173](https://github.com/QuantEcon/quantecon-theme.mystmd/issues/173)) ([#194](https://github.com/QuantEcon/quantecon-theme.mystmd/pull/194)). - The page-level `git_metadata` override is a declared option too, written as a YAML block string (template options are scalar-only). It previously survived only because the CLI leaves a page's raw `site:` block alone when nothing in it validates, so a page that also set any declared key (a translator credit, `hide_search`, and the per-lecture live-compute flag to come) silently lost its override. An object is still accepted - ([#173](https://github.com/QuantEcon/quantecon-theme.mystmd/issues/173)). + ([#173](https://github.com/QuantEcon/quantecon-theme.mystmd/issues/173)) ([#194](https://github.com/QuantEcon/quantecon-theme.mystmd/pull/194)). ### Changed - The release bundle's manifest now declares Node `>=20` (it said `>=16`, diff --git a/app/backend/loaders.server.ts b/app/backend/loaders.server.ts index 265224a7b..2c6abf00e 100644 --- a/app/backend/loaders.server.ts +++ b/app/backend/loaders.server.ts @@ -142,7 +142,11 @@ export async function getMystSearchJson(): Promise { */ const DEFAULT_FAVICON = "public/logos/lectures-favicon.png"; +// Read once: the file is static, and this runs on every /favicon.ico request. +let defaultFavicon: { contentType: string; buffer: Buffer } | null | undefined; + function readDefaultFavicon(): { contentType: string; buffer: Buffer } | null { + if (defaultFavicon !== undefined) return defaultFavicon; // The theme server runs from the bundle directory (`build.start` in // template.yml), where public/ sits beside build/; the second candidate // covers being launched from elsewhere. @@ -150,14 +154,16 @@ function readDefaultFavicon(): { contentType: string; buffer: Buffer } | null { path.resolve(process.cwd(), DEFAULT_FAVICON), path.resolve(__dirname, "..", DEFAULT_FAVICON), ]; + defaultFavicon = null; for (const file of candidates) { try { - return { contentType: "image/png", buffer: fs.readFileSync(file) }; + defaultFavicon = { contentType: "image/png", buffer: fs.readFileSync(file) }; + break; } catch { // try the next location } } - return null; + return defaultFavicon; } export async function getFavicon(): Promise<{ diff --git a/app/components/PageHeaderHistory.tsx b/app/components/PageHeaderHistory.tsx index 059565369..4bbd597a7 100644 --- a/app/components/PageHeaderHistory.tsx +++ b/app/components/PageHeaderHistory.tsx @@ -78,12 +78,24 @@ const COPY = "text-[0.85rem]"; * Renders nothing when neither is present. */ -/** The parsed override only counts when it has the plugin's shape. */ +/** + * The parsed override only counts when it has the plugin's shape, and only + * changelog entries the renderer can use (an object with string `hash` and + * `date`) are kept, so a malformed hand-written block degrades to fewer rows + * rather than a throw. + */ function asGitMetadata(value: unknown): GitMetadata | undefined { - if (!value || typeof value !== 'object' || Array.isArray(value)) return undefined; + if (!value || typeof value !== "object" || Array.isArray(value)) return undefined; const v = value as Partial; - if (!v.last_modified && !Array.isArray(v.changelog)) return undefined; - return { last_modified: v.last_modified, changelog: Array.isArray(v.changelog) ? v.changelog : [] } as GitMetadata; + const changelog = (Array.isArray(v.changelog) ? v.changelog : []).filter( + (entry): entry is GitChangelogEntry => + !!entry && + typeof entry === "object" && + typeof (entry as any).hash === "string" && + typeof (entry as any).date === "string" + ); + if (!v.last_modified && changelog.length === 0) return undefined; + return { last_modified: v.last_modified, changelog } as GitMetadata; } export function PageHeaderHistory({ alignEnd = true }: { alignEnd?: boolean } = {}) {