fix: parse built asset tags from the document and render every entry stylesheet - #1582
Merged
Merged
Conversation
Two things the server depends on are invisible to the build. Both fail silently, so a green build and a working dev server actively disguise them. The server parses the script and stylesheet paths out of the built index.html and reuses them on every server-rendered page. That parse is coupled to the exact attribute set and attribute order the frontend build writes into those tags. A build that emits a different tag shape still succeeds, the dev server still works, the binary still compiles, and the pages simply render with no scripts and no stylesheet. TestGetStyleResolvesBuiltAssets asserts the parse still finds them. Languages other than the default one are loaded with a template-literal dynamic import through an alias that points outside the frontend root. A bundler that cannot enumerate that pattern still builds and still serves a working app; the resources never arrive, and only for non-default languages, so a smoke test in the default language misses it. check-locale-resolution.js bundles that same import with the project's own configuration, runs it, and requires two languages to resolve to distinct translated content. Both run through make check-ui. check-built-assets.sh --self-check confirms the asset check still fails on tag shapes the parser cannot read, so a check that quietly stopped asserting anything is distinguishable from a passing one. (cherry picked from commit 71cd924, internal/controller/template_controller_test.go only)
GetStyle scraped index.html with regexes matching one exact tag shape: classic scripts with defer first, and stylesheet links with href before rel. Any bundler emitting a different shape returned nothing, and server-rendered pages would load with no JavaScript and no stylesheet while every build step still reported success. The tags are now read from the parsed document, so attribute order, attribute set and quoting no longer matter. header.html emits the scraped paths as script tags itself, and those were classic scripts. A module bundle loaded that way fails on its first import, so fixing only the parsing would have left server-rendered pages broken; the tag is now declared as a module. The self-check fixtures are replaced. The previous two asserted failure on module scripts and on rel-before-href, both of which the parser now accepts, so they would have inverted into false alarms. The replacements cover a stylesheet with no script, a script with no stylesheet, and an inline script with no src. golang.org/x/net moves to a direct requirement, matching its use here. (cherry picked from commit eab6f9d, go.mod and internal/controller/template_controller.go only)
GetStyle returned a single stylesheet path because the previous build emitted exactly one. The current build emits two, so server-rendered pages loaded partially unstyled while every build step still reported success. The stylesheet is now a list, mirroring how script paths are already collected and prefixed, and the template renders one link per entry. This is the same assumption as the tag-shape one fixed earlier: the server encoded a property of one bundler's output, here that there is exactly one entry stylesheet. (cherry picked from commit d66e21b)
The linter configured for this repository flags the slice-building form, so make lint fails on it. (cherry picked from commit 1cfd5da)
Asserting that the parsed stylesheet list is non-empty leaves the exact regression this repository already hit uncovered: a parser that stops at the first stylesheet returns a one element list, satisfies every existing assertion, and silently drops the rest of the page's CSS. Count the declarations again by a cruder method than the parser uses and require the two to agree, so the parser has to be checked against something other than itself. The count is a lower bound: a build that quotes attributes differently drives it to zero and it stops constraining, which is why it supplements the shape-independent assertions rather than replacing them. Verified by reintroducing the truncation and watching this fail. (cherry picked from commit 9232cbd)
The GetStyle comment framed the DOM walk around attribute order, attribute set, and quoting, the same properties the old regex parser depended on, without stating that the new parser ignores all of them. check-built-assets.sh described its self-check as failing when asset tags change shape, but the fixtures test a missing script or stylesheet tag, and the parser accepts any shape as long as the tag is present. Comments now state the actual constraint: tag shape does not affect parsing, and the guarding check fails when a build is present but a required script or stylesheet tag is missing from it. (cherry picked from commit 3bc12e8, internal/controller/template_controller.go only)
robinv8
pushed a commit
that referenced
this pull request
Sep 18, 2026
Fixes #1580. Part of #1578. Step 2 of 3, atomic. Depends on #1582 (the Go asset contract, now merged into `dev`), which is why the server-side parsing does not appear in this diff. Replaces `react-scripts` and `react-app-rewired` with Vite. The configuration preserves the output contracts the server depends on: the build directory stays at `ui/build`, which `ui/static.go` embeds, and emitted assets stay under `static/`, which `internal/router/ui.go` serves as a route. Files keep the `static/js`, `static/css` and `static/media` grouping, which the `analyze` script matches on; the ignore rules for `ui/build`, previously pinned to that layout's directory depth, now ignore everything under `build` except the tracked favicon. Production sourcemaps stay on, and the `REACT_APP_` prefix is retained so `ui/scripts/env.js` remains the single source of truth for configuration shared with the server. That same script's `public_url` value now also drives Vite's `base` configuration, and the manifest link in `ui/index.html` uses Vite's `%BASE_URL%` macro instead of a hardcoded path, so a non-root deployment keeps every asset and manifest reference prefixed the way it did under the previous toolchain's `PUBLIC_URL` wiring. Agentic tooling did the mechanical work in this series; every change was reviewed by a human before being committed. ### The one server-side line `header.html` re-emitted the paths `GetStyle()` finds as classic scripts. An ES module loaded through a classic script tag fails on its first import, so the tag is now declared as a module and carries `crossorigin`, matching the tag Vite's own build emits for the client-rendered entry point. Rewriting the built `index.html` instead was not an option, because `internal/router/ui.go` serves that same file to boot the SPA and it cannot misdeclare its own script type. A `type=module` script fetches in CORS mode, so any CDN origin serving these files needs to send the matching CORS headers, with or without `crossorigin` present; default same-origin deployments are unaffected. The note for CDN users is in the CDN plugin READMEs, apache/answer-plugins#326, per the placement decided on #1567, and the release notes for the version that ships this should carry a short pointer to it. ### Route code splitting Routes loaded pages with `` lazy(() => import(`@/pages/${pagePath}`)) ``. That shape cannot be statically analyzed, so no page received its own chunk and the specifier reached the browser untransformed, leaving every lazily routed page unable to load. Pages are now enumerated with a bounded glob covering the three directory depths routes actually use, excluding component subtrees so their own index files do not become route chunks. A page path with no matching module now rejects with the requested path and the list of known keys, surfacing through the existing route error boundary. ### Behaviour changes, called out deliberately **The custom stylesheet link now derives its href from the build's own base, not a separately computed value.** `PageTags` built the `/custom.css` link from `process.env.PUBLIC_URL`, which the previous toolchain exposed with its trailing slash already stripped; at the default configuration that value was an empty string, resolving to `/custom.css`. `import.meta.env.BASE_URL`, the direct equivalent under the new toolchain, keeps the trailing slash, so substituting it in the same place would resolve the same default configuration to `//custom.css` instead. The trailing slash is stripped explicitly before the substitution, reproducing the previous output at the default configuration and staying correct away from it. The output here is unchanged; only the mechanism it depends on is. **Two routes were dead ends and now fail loudly instead of silently.** `pages/403` and `pages/Admin/UserOverview` both referenced modules that did not exist in the tree, and both failed the same way under the previous toolchain, silently rendering blank. With the glob above, both now report the missing path through the route error boundary. Step 3 repoints `pages/403` at `pages/404/403`, an existing component, so that route renders; `pages/Admin/UserOverview` stays a visible gap because creating the missing page is a content decision. Neither is a regression. **The markdown editor now renders its themed background.** `src/components/Editor/index.scss` read `var(-bs-body-bg)` with a single leading dash. That is not a valid custom property reference, so the declaration was discarded and the editor never received the background it asks for. The previous CSS minifier accepted the invalid value; the current one rejects it outright and fails the build (`[lightningcss minify] Unexpected token Ident("-bs-body-bg")`), which is how it surfaced and why the one-line fix is part of this PR rather than a follow-up. Fixing it changes rendering. ### Dependency removals `react-scripts`, `react-app-rewired`, `customize-cra` and `config-overrides.js` are gone, and `yaml-loader` is replaced by the equivalent Vite plugin, pinned to the schema the previous loader used so bare dates and merge keys keep parsing the same way they did before. The scaffold test the old toolchain generated (`App.test.tsx`) and its jest packages go with it; this project has no test runner and no unit tests, before or after. Three removed packages were already inert before this migration. Both purgecss packages were declared but wired nowhere: no postcss config exists, the overrides file never referenced them, and no script invoked them. `buffer` was aliased and provided as a global, but no application source uses it, and the one dependency requiring it declares `buffer: false` in its own browser field. `sass` and `@types/node` are raised to the versions the toolchain requires; the previous `sass` predates the async compiler API it now calls. `sass` is pinned below the release that begins deprecating `@import`, which this project uses across 30 files. Migrating those to `@use` is a separate concern. Bootstrap's own Sass internals print dozens of dependency deprecation warnings on every build, unrelated to anything in this project's own styles; `vite.config.mts` sets `css.preprocessorOptions.scss.quietDeps: true` to silence those specifically while still surfacing warnings from this project's own stylesheets. One such app-own warning remains in this PR's build output, a mixed-declarations notice from `Comment/index.scss`; the one-line reorder that clears it is a step 3 nit. The eslint config no longer extends `react-app/jest`, which shipped inside `react-scripts` and configured rules for a test suite this project does not have. ### A failure found only by running the built application With the build green and the dev server working, the built application did not boot. React never mounted, the page showed its loading spinner indefinitely, and the browser console was empty. `i18next` attaches its resource-store methods to the instance inside `init()`. The builtin plugins register their translations while their modules are being evaluated. Whether that happens before or after `init` depends on how the bundler groups and orders chunks, so the previously working order was incidental rather than guaranteed. When it inverts, the registration throws while the entry module is still evaluating, which takes the application down before it mounts and produces no console output. Registration now happens immediately only when there is an initialised instance to register into, and otherwise falls to the `initialized` handler the code already installed, which is correct in either order. The check that guards both orders is part of step 3. ### Parity fixes found in review **Bootstrap icon fonts.** The bootstrap-icons stylesheet points at font files under a path the bundler could not resolve on the first migration pass, so the build silently emitted zero font files and every icon rendered as a missing-glyph box. The stylesheet now overrides the package's font-directory variable to a path Vite can resolve; the fonts are emitted, and the boot test below confirms they are served. **Type checking.** `pnpm build` now also runs `tsc --noEmit` before the bundler runs, and is clean today. The previous toolchain ran its checker in the dev server (blocking) and during builds (downgraded to warnings by this project's `TSC_COMPILE_ON_ERROR` setting); none of that carried over when the bundler changed, so until this fix a type error shipped with no signal at all. **Yaml parsing.** The Vite yaml plugin defaults to js-yaml's more permissive schema, which resolves bare dates to JS `Date` objects and enables merge keys; the previous loader's schema kept both as plain strings. The plugin is now pinned to that same schema. **Declared Node range.** The bundler's declared support range is `^20.19.0 || >=22.12.0`. `ui/package.json`'s own `engines.node` allowed `>=20`, which admits versions below that floor, and now matches it exactly. The release workflow's pinned Node, which also sat below the floor, is raised to `20.19.0` to match. That version bump is the only change this PR makes anywhere under `.github/`; it does not add a job. **Typed environment variables.** `import.meta.env.REACT_APP_*` previously typed as `any`, since no `ImportMetaEnv` augmentation existed; a typo'd key would compile clean and only surface as a missing value at runtime. The two keys the app reads this way, `REACT_APP_API_URL` and `REACT_APP_BASE_URL`, are now declared, with `strictImportMetaEnv` enabled so an undeclared key is a type error instead of a silent `any`. ### Verification On `dev` at `ace02c49`, which includes step 1, plus these commits: `pnpm install --frozen-lockfile` under the pinned `pnpm@9.7.0` and `pnpm build` (which now includes `tsc --noEmit`) complete; the build prints three warnings, each once: `front-matter`, a dependency unrelated to this project's own pinned `js-yaml@^4.1.0`, pulls in a legacy `js-yaml@3.x` copy whose `buffer` import gets externalized for browser compatibility; the Sass mixed-declarations notice from `Comment/index.scss` mentioned above; and one chunk over the default size threshold, see the note on chunking below. `go build ./...` and `go vet ./...` pass; `TestGetStyleResolvesBuiltAssets` passes against the Vite output; a search for `react-scripts`, `react-app-rewired`, `customize-cra` and `config-overrides` finds nothing outside the lockfile. The built binary was booted against sqlite3 and loaded in a browser: `/` renders with the client mounted (React's fiber container present on the root element), the module entry script and both entry stylesheets are fetched, and the console shows no errors. The same holds for `/tags`. The server-rendered `/` response itself carries the module entry script and both entry stylesheets, which is `GetStyle()` from step 1 finding them in the Vite output and `header.html` re-emitting them. The three `make check-ui` guards from #1567 (asset paths, non-default locale, plugin i18n order) are not in this PR; they arrive as step 3 and were run green against the equivalent tree there. ### Measurements Captured for #1567 at its head `d06b623`, against `main` at `3b9f137`, same machine, same Node and package manager versions, clean tree and clean install on both sides, five runs per timing metric. They are carried over rather than recaptured: the frontend build inputs here are the same as at that head, minus the step 3 nits and plus the newer locale files on `dev`. | Metric | Before | After | |---|---|---| | Cold production build | 19.87s | 4.77s | | Warm build | 8.75s | 4.87s | | Dev server time to ready | 7104ms | 359ms | | HMR latency | 421ms | 139ms | | Bundle JS, raw / gzip | 3477.60KB / 1198.75KB | 3010.49KB / 1035.57KB | | Bundle total, raw / gzip | 4290.39KB / 1652.24KB | 3763.97KB / 1474.34KB | | Direct dependencies | 75 | 65 | | Packages installed | 1578 | 689 | | Audit findings, critical/high/moderate/low | 4/78/63/13 | 1/50/38/6 | Five things worth stating rather than leaving to be inferred: - Cold and warm builds are within noise of each other, because the new build has no meaningful persistent cache to warm and now also runs a type check on every build, cold or warm alike. The old toolchain had a real warm cache, which is why its two figures differ so much. The comparison to draw is cold against cold. - Dev server time-to-ready is wall clock on both sides, including process spawn. The new tool self-reports a much smaller number that excludes that, and using it would compare two different quantities. - Chunk boundaries differ structurally between the two bundlers, so the bundle rows compare total shipped bytes rather than like-for-like chunks. - HMR latency was measured once, at commit 7200ca3 on the #1567 branch, on both toolchains, and carried forward from there: none of the commits since touch the hot-update path. - The bundle totals include the bootstrap-icons font files the first migration pass had silently dropped, and the cold build includes the restored type check. Both are named costs of parity fixes, already folded into the deltas. ### Not included This PR adds no CI job for the frontend. The project runs no frontend job today, and a build on every push is a cost a maintainer should choose to take on, not one this migration should impose. One constraint carries forward for whoever wires that job later: a bare `go test ./...` reports ok while asserting nothing about the built asset paths, because `TestGetStyleResolvesBuiltAssets` skips when no frontend build is embedded. Any future CI job needs to build the frontend first. The dev server now binds to loopback only by default; reaching it from another device on the network needs an explicit `--host` flag. Create React App's SVG-as-component imports (`import { ReactComponent as X } from './x.svg'`) are not carried over to this configuration. Nothing in this codebase used them. The commits carry `(cherry picked from commit ...)` lines pointing at the branch behind #1567, where these changes were first reviewed and where the regression matrix (subdirectory deploy, OAuth callbacks, absolute CDN `public_url`) was run.
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
2 participants
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.
Fixes #1579. Part of #1578. Step 1 of 3; lands while Create React App still builds, and step 2 (the Vite cutover) depends on it.
Three parts of the Go side had one bundler's output format encoded into them. This PR removes two of them ahead of the toolchain change; the third, the classic script tag in
header.html, is the one line step 2 flips together with the build that emits modules, so the cutover itself stays a frontend-only diff plus that line.GetStyle()matched a literal tag shape. It scrapedindex.htmlwith regexes requiring classic scripts withdeferfirst and stylesheet links withhrefbeforerel. Any other shape returned nothing, and server-rendered pages would load with no JavaScript and no stylesheet while every build step still reported success. The tags are now read from the parsed document (golang.org/x/net/html, already in the module graph and promoted from indirect to direct ingo.mod), so attribute order, attribute set and quoting no longer matter, and the next bundler change cannot reintroduce this. The parser accepts any shape as long as the tag is present; what it guards is a missing script or stylesheet tag.GetStyle()returned a single stylesheet. The current build emits exactly one entry stylesheet; the Vite build emits two, so pages would load partially unstyled. It is now a list, mirroring how script paths were already collected and prefixed, andheader.htmlranges over it. With today's build that is a list of one, and the rendered page is unchanged.header.htmlkeeps its classic script tag.<script defer="defer" src="{{$path}}">is untouched here on purpose; it becomes a module tag in step 2, together with the build that emits modules.Test.
TestGetStyleResolvesBuiltAssetsininternal/controller/template_controller_test.gocalls the realGetStyle()against the real embedded build and requires at least one script source and every declared stylesheet to come back. It skips when no frontend build is embedded, which is a constraint for anyone wiring CI later: a barego test ./...reports ok while asserting nothing about the asset paths unless the frontend was built first. The self-check harness that rewrites the builtindex.htmlinto unparseable shapes and asserts the test fails on each follows in step 3.Agentic tooling did the mechanical work in this series; every change was reviewed by a human before being committed.
Verification
On
devat2c0ced32plus these commits, with the current toolchain:pnpm install --frozen-lockfileandpnpm build(react-scripts) complete;go build ./...andgo vet ./...pass;TestGetStyleResolvesBuiltAssetspasses against the Create React App output (--- PASS, not skipped), and as a negative control the same test fails when the builtindex.htmlis rewritten without a scriptsrc, then passes again once the original is restored. The built index.html at that point carries eight classic<script defer="defer" src="/static/js/...">tags, one inline script and one stylesheet link, and the parser returns all eight sources and the one stylesheet.The commits carry
(cherry picked from commit ...)lines pointing at the branch behind #1567, where this change was first reviewed.