feat(launch): make the Launch control opt-in and explicitly configured - #224
Conversation
Launch appeared on every page of any project that set `project.github`, opening Colab on a notebook repository whose name the theme guessed: the source repository, minus any `.myst` suffix, plus `.notebooks`. No option turned it off, so a site without a notebooks repository sent readers to a repository that does not exist -- lecture-wasm's pages link to QuantEcon/lecture-wasm.notebooks, a 404, on every page. The control now renders only when `launch_notebook_repo` names the notebook repository and `launch_colab` is true. Nothing is derived from `project.github`, so a site gets Launch only once it has said where the notebooks are and what may open them. The option group is split along that seam: `launch_notebook_*` says where the notebook lives, and the service keys say what can open it, because one source serves every service. The names are flat only because template options are scalar-only; each is the nested path it becomes once mystmd supports structured options, so that migration is mechanical. The toolbar and overflow slots carry `empty:hidden`, so a site with no launch configured takes no gap where the control would have been. BREAKING CHANGE: `launch_repo_url`, `launch_repo_suffix`, `launch_branch`, `launch_notebooks_path` and `launch_source_path` are replaced, and a site that relied on the guessed repository must now name it explicitly to keep its Launch link. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UZLpDVYu1YBZHQfwkRRJj7
- README's "Launch Notebooks" section still opened by stating the `.notebooks` suffix convention as the default, contradicting the opt-in explanation twenty lines below it. A reader of the first paragraph would conclude Launch works straight from `project.github`. - An option set to an empty or blank string reaches the theme as one: the CLI validates it as a string and passes it through. An empty `launch_notebook_branch` therefore skipped the `main` default and built `blob//<path>`, a 404 on every page; a blank `launch_notebook_repo` passed the render gate and linked to `github//`. Blank now counts as unset, and the branch is trimmed like the other path options, so a copied `/main/` works too. - The new "no gap where the control would have been" assertion could not fail. An empty `<li>` is a zero-width flex item whether or not it is displayed, so measuring its width proved nothing -- and Playwright treats a zero-size element as hidden, so `toBeHidden()` was blind in the same way. It now asserts the computed `display`, which does fail when the collapse is removed, and covers the mobile overflow menu by opening it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UZLpDVYu1YBZHQfwkRRJj7
|
🎭 Visual regression resultsDetails
Skipped testsmobile-chrome › theme.spec.ts › QuantEcon theme — visual regression › without JavaScript › drawer-opens-without-javascript |
…he guess The "Other `site` keys" table credited `project.github` with "the notebook repo derivation", which this branch removes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UZLpDVYu1YBZHQfwkRRJj7
There was a problem hiding this comment.
🔵 Needs a closer look
It is a breaking configuration change with cross-repository migration impact, and the author explicitly flags stale rtl.png visual baselines (including -darwin ones that cannot be regenerated in CI) that need a maintainer refresh before merge.
Pull request overview
This PR makes the toolbar Launch control opt-in and explicitly configured, closing #205. Previously the control appeared on every page of any project that set project.github, linking to a notebook repository whose name was guessed (source repo, minus .myst, plus .notebooks) — which 404s on sites without such a repository (e.g. lecture-wasm). Now Launch renders only when launch_notebook_repo names a repository and launch_colab is true, with nothing derived from project.github. This is a breaking change: the five launch_* options are replaced by the launch_notebook_* group plus the launch_colab service flag, and launch_repo_suffix and the .myst rule are removed. The deferred notebook-existence failsafe is split out to #223.
Changes:
- Rework
launchUrls.ts/LaunchButton.tsxso the notebook repo is always configured (never derived), blank strings count as unset, and the branch is trimmed/defaulted tomain. - Rename/redeclare the launch options across
template.yml,app/types.tsand all docs; addlaunch_colab. - Add
qe-launch-slot empty:hiddento the toolbar and mobile slots so an unconfigured site takes no gap; rewrite unit tests and add alaunch-absent-without-configvisual test onfixture-no-thebe.
File summaries
| File | Description |
|---|---|
| app/components/toolbar/launchUrls.ts | New repo-based LaunchConfig; trims whitespace/slashes; branch falls back to main on blank; removes suffix/derivation logic. |
| app/components/toolbar/LaunchButton.tsx | Drops useProjectManifest; gates render on trimmed launch_notebook_repo + launch_colab; wires new config keys. |
| app/components/toolbar/Toolbar.tsx | Adds qe-launch-slot/empty:hidden to the desktop Launch slot to collapse when empty. |
| app/components/toolbar/MobileActionsMenu.tsx | Same collapse behavior for the mobile overflow Launch slot. |
| app/components/PageHeaderHistory.tsx | Comment-only: clarifies commit links keep project.github as-is (.myst included). |
| app/types.ts | Replaces the five launch_* fields with the launch_notebook_* group + launch_colab; updates docblock. |
| template.yml | Declares the new options; removes launch_repo_url/launch_repo_suffix. |
| README.md / docs/launch.md / docs/configuration.md | Rewrites the launch documentation, option tables, and Sphinx-key mapping; removes self-contradiction. |
| tests/unit/launch-urls.test.mjs | Rewrites unit tests for the new option shape, blank/slash branch handling, and prefix edge cases. |
| tests/visual/theme.spec.ts | Pins the exact Colab URL; adds launch-absent-without-config asserting no control/link and a collapsed slot (computed display). |
| tests/visual/fixture/myst.yml.in | Adds launch_notebook_repo + launch_colab so the present-control test has both axes configured. |
| CHANGELOG.md | Adds a Breaking "Changed" entry under Unreleased. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
#221 and #222 both landed `### Added` entries under `## [Unreleased]` while this was open, which conflicted textually with this branch's `### Changed` entry while agreeing in substance. Resolved by keeping all three, ordered the way Keep a Changelog orders them: Added (the default site footer, the `{raw}` rewriter) before Changed (the breaking launch change). `tests/visual/theme.spec.ts` auto-merged as a union of both sides: the `Site footer` block from #221 and this branch's launch tests are all present, and the unit suite is unchanged from main except for this branch's own rewrite of `launch-urls.test.mjs`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`fixture-rtl` names no launch options, so with Launch now opt-in its toolbar loses the control. The committed baseline still carried it, and the suite could not catch that: one ~20px icon on a 1280x2396 page is under the `maxDiffPixelRatio: 0.01` the rtl snapshot allows, so the comparison passed while the baseline described a page the theme can no longer render -- which would mask a later regression in that area. Refreshed with `--update-snapshots=all`, since the default mode rewrites only baselines whose comparison actually fails. Only the desktop baseline moves, and its dimensions are unchanged at 1280x2396: an icon left the toolbar without the page reflowing. On mobile the control sits inside the closed `MobileActionsMenu`, so it was never visible in the full-page screenshot and those pixels are identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/update-snapshots |
1 similar comment
|
/update-snapshots |
|
🎭 Refreshed visual baselines in 52557ca:
|
#221, #222 and #224 all landed entries under `## [Unreleased]` while this was open, which conflicted textually with this branch's `### Fixed` entry while agreeing in substance. Resolved by keeping all four, in the order Keep a Changelog uses: Added (default site footer, `{raw}` rewriter), Changed (breaking launch change), then Fixed (output image centring). The Fixed entry also gains its PR link alongside the issue link, which is what CONTRIBUTING.md asks for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fixture notebook gains a stored `image/png` cell, so /notebook grows from 1094 to 1551px on desktop and from 1203 to 1548px on mobile. Both darwin baselines were size mismatches against that and failed outright, so the default update mode was sufficient: it rewrites only a baseline whose comparison fails, which is why the other twelve darwin baselines are untouched. (A sub-threshold change needs `=all` instead, as the rtl baseline did in #224.) The heights land a few pixels off the linux set's 1552/1553, which is the reason these baselines are platform-suffixed at all -- the CI font stack wraps slightly differently, so neither platform's pixels can satisfy the other. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(notebooks): centre images in cell outputs A plot produced by a lecture's code cells sat against the left edge of the content column with all the spare width beside it: 0px to its left and 232px to its right, for a 568px figure in the 800px column. The lecture builds centre the same plots. A plot output is a bare `<img>` that upstream renders with neither class nor style, inside a container marked only by `data-name`, so the container attribute is what there is to select on; `text-align` on the container cannot move it, because Preflight makes the image a block. Auto side margins on the image do, matching the lecture builds' own `.cell_output img` rule. Both container names are covered: the stored outputs are re-rendered into `active-outputs-container` when a reader starts live compute, and the plot should not jump left at that moment. Only images are centred, so tables and text outputs stay left-aligned as they are on the lecture sites. The fixture notebook gains a cell with a stored `image/png` output -- wider than the mobile column and narrower than the desktop one, so the same page exercises both centring and capping -- and a DOM assertion measures the gaps. The pixel suite alone could not catch this: the plot is mostly white, so the shift moves 0.36% of the page, under the suite's 1% tolerance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UZLpDVYu1YBZHQfwkRRJj7 * test: refresh CI visual snapshots (linux baselines) * test: compare page widths numerically in output-image-centred The overflow check was a strict boolean on `scrollWidth > clientWidth`. Both values are integers rounded from sub-pixel layout, so a page that fits exactly can still report one more pixel of scrollWidth than clientWidth -- and `expect(false).toBe(false)` names neither value when it fails. It now compares them with the same 1px allowance as the gap assertions beside it, and returns the two widths so a failure reports them. Page overflow itself is guarded by the 1280/1300/1328px sweep across both text directions in `outline-within-viewport`; this line is a sanity check that the page holding a centred image still fits, which is why it keeps `clientWidth` as its subject rather than the viewport width -- the latter includes the scrollbar and would tolerate roughly 15px more. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: refresh the local notebook baseline for the stored plot output The fixture notebook gains a stored `image/png` cell, so /notebook grows from 1094 to 1551px on desktop and from 1203 to 1548px on mobile. Both darwin baselines were size mismatches against that and failed outright, so the default update mode was sufficient: it rewrites only a baseline whose comparison fails, which is why the other twelve darwin baselines are untouched. (A sub-threshold change needs `=all` instead, as the rtl baseline did in #224.) The heights land a few pixels off the linux set's 1552/1553, which is the reason these baselines are platform-suffixed at all -- the CI font stack wraps slightly differently, so neither platform's pixels can satisfy the other. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Matt McKay <mmcky@users.noreply.github.com>
#221, #222, #224 and #225 all landed entries under `## [Unreleased]` while this was open. This branch's entry is `### Added` like two of theirs, so it folds into that list rather than forming a second section: Added (default site footer, `{raw}` rewriter, this checklist), then main's Changed and Fixed sections unchanged. The entry also gains its PR link alongside the issue link, which is what CONTRIBUTING.md:225-227 asks for. `docs/configuration.md` auto-merged, and was checked rather than assumed: main's launch rows from #224 and this branch's `project.description`, `project.keywords` and `project.math` rows are all present, with no row duplicated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#221, #222, #224, #225 and #226 all landed entries under `## [Unreleased]` while this was open. This branch adds one `### Added` entry and one `### Fixed` entry, so each folds into the matching section rather than opening a second: Added (default site footer, `{raw}` rewriter, migration checklist, this canonical link), Changed (the breaking launch change), Fixed (output image centring, these base-URL head links). Both entries also gain their PR link beside the issue link, which is what CONTRIBUTING.md:225-227 asks for. `README.md`, `docs/configuration.md`, `template.yml` and `tests/visual/theme.spec.ts` auto-merged. Each was checked rather than assumed: both sides' content is present in all four. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#221, #222, #224, #225, #226 and #227 all landed entries under `## [Unreleased]` while this was open. This branch adds one `### Added` entry, so it folds into that list rather than opening a second section, and it gains its PR link beside the issue link, which is what CONTRIBUTING.md:225-227 asks for. `docs/layout.md`, `styles/quantecon.css` and `tests/visual/theme.spec.ts` auto-merged. Each was checked rather than assumed: the branch's h4 indent rule sits beside main's output-image centring rule, the branch's outline assertions beside main's footer, launch, centring and canonical tests, and the branch's h2-to-h4 documentation beside main's default-footer section. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#221, #222, #224, #225, #226, #227 and #228 all landed `[Unreleased]` entries while this was open. This branch adds a `### Fixed` entry, so it folds into main's existing Fixed section (output image centring, base-URL head links) rather than opening a second one, and it gains its PR link beside the issue link per CONTRIBUTING.md:225-227. `docs/configuration.md`, `docs/index.md` and `docs/layout.md` auto-merged, and each was checked rather than assumed: layout.md holds this branch's Header and Widths rewrites beside #228's h2-to-h4 outline text and #221's default-footer section; index.md holds #226's Pages-table row beside this branch's rewritten closing paragraph; configuration.md holds the rows from all four PRs that touched its table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #205, implementing the 2026-09-11 decision. Breaking change — see the CHANGELOG entry.
Problem, re-verified before the work
Built main at
d7ec6cdand servedfixture-rtl(which setsproject.githuband no launch options — the shape of a lecture repo with no notebooks repository): one Launch control, linking to a repository name guessed from the source one. On the branch: zero. That guess is what putsQuantEcon/lecture-wasm.notebooks, a 404, on every live lecture-wasm page.Change
Launch renders only when
launch_notebook_reponames the repository andlaunch_colabis true. Nothing is derived fromproject.github;launch_repo_suffixand the.mystrule are gone.launch_repo_urllaunch_notebook_reponb_repository_urllaunch_branchlaunch_notebook_branch(defaultmain)nb_branchlaunch_notebooks_pathlaunch_notebook_dirnb_path_to_notebookslaunch_source_pathlaunch_notebook_source_dirpath_to_docslaunch_repo_suffixlaunch_colablaunch_buttons.colab_urlThe
launch_notebook_*group says where the notebook lives; the service key says what can open it — one source serves every service, so a service is not a property of the source. Flat names are the nested paths they become under QuantEcon/mystmd#112.The toolbar and overflow slots carry
qe-launch-slot+empty:hidden, so a site with no launch takes no gap where the control sat.Deferred check split out
Per @mmcky, the notebook-existence failsafe is now #223 rather than part of this PR. This PR alone removes every dead link we can currently point at: lecture-wasm and the four unconfigured translations get no Launch at all, and the three repos naming a missing notebooks repository are being fixed Sphinx-side and simply will not set
launch_notebook_repo.Adversarial review, and what it found
Four lenses (correctness, config/docs, UI/accessibility, regressions), each finding independently refuted by two skeptics. 9 findings survived, reducing to four distinct defects — all fixed in the second commit:
.notebooksconvention as the default — twenty lines above the text saying nothing is derived. Fixed.launch_notebook_branch: ""skipped themaindefault and builtblob//<path>— a 404 on every page — and a blanklaunch_notebook_repopassed the render gate and linked togithub//. Blank now counts as unset, and the branch is trimmed like the other path options (so a pasted/main/works).<li>is a zero-width flex item whether displayed or not, so measuring width proved nothing. My first fix —toBeHidden()— was blind the same way, because Playwright calls a zero-size element hidden. I mutation-tested it: withempty:hiddenstripped from both slots, the old assertion still passed; the new one (computeddisplay) fails on both projects, and passes again when restored. It now also opens the mobile overflow menu, which is the only place the mobile case is really testable.rtl.pngbaselines were stale — now refreshed, see below.Three findings were verified and refuted. Not silently capped: 9 further findings were low severity or over the five-per-lens verification cap and were not put through refutation.
Merged with main
#221 (the default site footer) and #222 (the
{raw}rewriter) both landed while this was open, and both added an### Addedblock directly under## [Unreleased], which conflicted textually with this branch's### Changedentry. Merged main in at098c2e2cf, keeping all three entries in the order Keep a Changelog uses: Added (default site footer,{raw}rewriter) before Changed (this breaking launch entry).tests/visual/theme.spec.tsauto-merged as a union of both sides, verified rather than assumed: all four tests are present (launch-colab,launch-absent-without-configfrom here;default-footer-without-part,declared-part-replaces-defaultfrom #221), and the 47-line delta against this branch matches #221's own diff stat for that file exactly. Typecheck is clean on the merged tree and the unit suite is unchanged from main apart from this branch's own rewrite oflaunch-urls.test.mjs.Visual baselines —
-darwinrefreshed herefixture-rtldeclares no launch options, so it loses its Launch icon.The important part is that the suite did not fail, and could not: one ~20px icon on a 1280×2396 page is far under the
maxDiffPixelRatio: 0.01that thertlsnapshot allows. I confirmed that directly — the unmodified suite was 42 passed / 0 failed while the committed baseline still contained a control the theme can no longer render. Green CI was masking a stale baseline, which would go on to hide a genuine regression in that area.-darwin— refreshed in7bce0d6a8with--update-snapshots=all, which is what rewrites a baseline whose comparison never fails; the default mode would have left it untouched. Only the desktop baseline moves (155,691 → 155,301 bytes, dimensions unchanged at 1280×2396: an icon left the toolbar without the page reflowing). The mobile baseline is byte-identical, because on mobile the control sits inside the closedMobileActionsMenuand was never visible in a full-page screenshot. The other 12 darwin baselines are untouched, so=allcaused no collateral churn, and a clean non-updating re-run is 42 passed / 0 failed.-linux— refreshed in52557ca47by/update-snapshots, which the workflow runs withMODE=all. Onlydesktop-chrome-linux/rtl.pngmoved (145,402 → 144,954 bytes), mirroring darwin exactly. The mobile baseline matched a fresh full re-capture byte-for-byte, which also settles its 2680px height as genuine CI font-wrapping rather than a stale file. One wrinkle worth knowing for future refreshes: that commit is pushed bygithub-actions[bot], so its ownpull_requestruns arrive gated asaction_requiredwith no jobs started — which presents as "no checks reported" rather than as a pending run. They were approved, and CI is green on the tip.Tests
tests/unit/launch-urls.test.mjsrewritten for the new options: both repo spellings, themaindefault, blank/slash-wrapped branches,dir,source_dir, a source dir that is only a prefix of a longer name, and the combined case.launch-colabnow asserts the exact URL built from the fixture's ownlaunch_notebook_repo(it previously matched a pattern, because the repo came fromgithub).launch-absent-without-configis new, onfixture-no-thebe: no control, no Colab link anywhere, and the slot collapsed — mutation-proven, as above.Review
Copilot raised one comment, on the changelog entry linking the issue but not the PR. Addressed in
098c2e2cf: CONTRIBUTING.md:227 does ask for a PR link, so the entry now carries both. Its supporting claim that the PR link is "followed by every released entry" is not accurate, and the reply records that — issue-only entries appear across five released versions (four in 2.6.1, three in 2.3.1, one each in 2.5.0, 2.4.0 and 2.0.0), so the practice in the file is mixed and CONTRIBUTING is what is being followed.Out of scope, flagged not done
https://gitlab.com/O/ris silently rewritten into a GitHub Colab link, andhttps://github.com/O/r/tree/main/notebookskeeps its extra segments. This is pre-existing behaviour (the oldlaunch_repo_urlwent through the same code) and Launch control cannot be turned off and links to a nonexistent notebooks repo on sites without one #205 explicitly scopes it out: "Out of scope (F076, latent, no lecture site affected): a sub-path inside a repository URL, and non-GitHub repository URLs." Raising rather than fixing, since narrowing it is a maintainer call.PLAN.mdstill names the five removed options. Left deliberately — Decide how the CC BY-SA licence footer survives the cutover: a theme default or a per-repo footer.md #203/Pages emit no rel=canonical, and the favicon link 404s on sites served under a sub-path #207/The "On this page" outline leaves out h4 subsections that the Sphinx panel lists and expands #208 all record that PLAN.md gets a closing note in Close out PLAN.md now that the parity plan is complete #218 rather than row edits.jb2branch adding the two options) are outside this repo.Rebase note
Open PR #195 also edits
template.yml,app/types.ts,docs/configuration.mdandtests/visual/theme.spec.ts. The conflicts are adjacent-hunk only — #195 insertsenable_live_computeimmediately above the launcher block intemplate.ymland a row above the launch rows indocs/configuration.md— so whichever lands second rebases cheaply.🤖 Generated with Claude Code