fix: rewrite asset URLs in route stylesheets, not just those in _assets/ - #151
Conversation
The #139 rewriter enumerated a single directory, `public/build/_assets`. Remix also emits route and shared-chunk CSS into the build root, `_shared/` and `routes/`, so those files were never in scope and shipped in v2.3.1 still carrying absolute `/myst_assets_folder/_assets/plotly-*.svg` references — the same defect #138 described, in the files that fix did not cover. Walk the build directory instead, and compute the prefix per stylesheet from its own location: a stylesheet in `_assets/` still gets `./`, one in the build root gets `./_assets/`, and one in `routes/` gets `../_assets/`. The existence guard now resolves from each stylesheet's own directory too — that assumption is what silently failed here, so it should be the thing being checked. Verified against a real `npm run prod:build`: 78 asset URLs rewritten across 7 stylesheets, zero absolute references remain anywhere under `public/build`, and an independent resolver confirms all 78 exist at the paths the stylesheets now name. Re-running the script is a no-op. Refs #150 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🎭 Visual regression results
Details
Skipped testsmobile-chrome › theme.spec.ts › QuantEcon theme — visual regression › launch-colab |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #150, a residual case of the #138 defect. The post-build script scripts/relative-css-asset-urls.mjs (added in #139) rewrites Remix's absolute ${publicPath}_assets/… url() references in built stylesheets to be relative, so they resolve in myst build --html output and under a baseurl. The prior version only enumerated the _assets/ directory, missing route- and shared-chunk CSS that Remix emits into the build root, _shared/, and routes/. Four such stylesheets shipped in v2.3.1 still carrying an absolute --jp-icon-plotly reference.
The fix walks the whole build tree and derives each stylesheet's _assets/ prefix from its own location (./, ./_assets/, or ../_assets/) via path.relative, and validates each rewritten target relative to that stylesheet's own directory.
Changes:
- Recursively walk
public/buildfor all.cssfiles instead of only_assets/. - Compute the relative
_assets/prefix per stylesheet based on its depth, replacing the hardcoded./. - Resolve the existence guard from each stylesheet's own directory; add a CHANGELOG entry.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
scripts/relative-css-asset-urls.mjs |
Adds a recursive stylesheet walk and per-location prefix computation, and resolves the existence check from each stylesheet's directory. |
CHANGELOG.md |
Documents the fix under [Unreleased] / Fixed, referencing #139, #138, and #150. |
I reviewed the path-prefix logic against all four layouts (build root, _assets/, _shared/, routes/), the updated URL/existence regexes, the cross-platform path.sep→/ normalization, and the remix.config.prod.js values the script reads. The logic is correct and self-consistent, and the CHANGELOG links are accurate. I found no concrete issues to flag. Note this is a release-affecting build-pipeline step with subtle cross-platform path manipulation and no automated regression test guarding it.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Move the [Unreleased] entries under ## [2.4.0] - 2026-09-04 with a headline blockquote, add the compare link, re-point the [Unreleased] compare base at v2.4.0, and bump package.json (npm version 2.4.0 --no-git-tag-version). template.yml is stamped by release.yml at build time. Minor rather than patch: #155 adds the site footer part and back-to-top styling alongside its fixes. The release carries three commits that have been on main since 2026-09-03: - #151 completes the static-build asset fix that v2.3.1 began. #139 covered the stylesheets in _assets/, but Remix also emits route and shared-chunk CSS, and four such files shipped in 2.3.1 still pointing --jp-icon-plotly at an absolute /myst_assets_folder/ URL. A production build now emits zero absolute asset URLs and all 78 references resolve. - #155 brings lecture content typography into line with the Sphinx builds and fixes inline code rendering wrapped in literal backticks — a regression that reached every code span on every page. - #161 bumps fast-uri 3.1.5 to 3.1.7. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #150. Found while verifying the published v2.3.1 artifact — #139 fixed the
_assets/stylesheets, but Remix emits others that were never in the rewriter's scope, and four of them shipped in v2.3.1 still carrying absolute/myst_assets_folder/…URLs.What was still broken
scripts/relative-css-asset-urls.mjsenumerated one directory:Route and shared-chunk CSS lands in
public/build/,public/build/_shared/andpublic/build/routes/, soreaddirSyncnever saw it. Unzipping the releasedquantecon-theme.zip:public/build/_assets/public/build/public/build/_shared/public/build/routes/All four are the same reference,
:root{--jp-icon-plotly: url(/myst_assets_folder/_assets/plotly-6TYK7N2P.svg)}. The SVG is present in the bundle — only the reference is wrong, so it resolves undermyst startand 404s inmyst build --htmloutput and under abaseurl.#139's scoping note ("only the KaTeX stylesheet is affected;
app.cssandthebe-core.cssemit nourl()references") was measured against the_assets/set, which is exactly the set that was in scope — so the gap was invisible from inside it.Why it isn't a one-line glob change
The old substitution hardcoded
url($1./, correct only for stylesheets sitting inside_assets/. The newly-included files reference_assets/from a different depth:_assets/*.css./plotly-*.svgroot-*.css./_assets/plotly-*.svg_shared/*.css,routes/*.css../_assets/plotly-*.svgSo the script now walks the build directory and derives each stylesheet's prefix from its own location via
path.relative. The existence guard resolves from that stylesheet's directory too — the previous guard resolved everything against_assets/, so even had these files been in scope it would have validated them against the wrong base. That assumption is the thing that failed, so it is now the thing being checked.Verification
Against a real
npm run prod:build(not just the Playwright suite, which only exercises themyst startroute where the absolute form happens to work):[css-assets] rewrote 78 asset URL(s) in 7 stylesheet(s)— was 60 in 1.myst_assets_folderreferences remain anywhere underpublic/build.root-*.css→./_assets/…,_shared/androutes/→../_assets/….url()and resolving it from its own stylesheet's directory reports 78 checked, 0 broken.rewrote 0 asset URL(s) in 0 stylesheet(s), exit 0).npm run compileclean ·npm run test:unit20/20.No behaviour change under
myst start:routes/x.css→../_assets/yresolves to/myst_assets_folder/_assets/y, the same path it named before.Not urgent
The visible symptom is a missing plotly icon on statically built sites that render plotly outputs — not the maths-wide degradation #138 caused. This does not warrant re-tagging v2.3.1; it can ride the next release.
🤖 Generated with Claude Code