Conversation
Two things the server and the app depend 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. A build that
emits no script or stylesheet tag at all 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. check-built-assets.sh builds the frontend and runs
that test, and its --self-check rewrites the built index.html into shapes
the parser cannot use (a stylesheet link but no script with a src, a
script but no stylesheet link, an inline-only script) and confirms the
check fails on each, so a check that quietly stopped asserting anything is
distinguishable from a passing one.
Languages other than the default one are loaded with a template-literal
dynamic import through an alias that points outside the frontend root. A
toolchain 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 drives the project's dev server directly and
asserts two separate properties, because only one of them is about
resolution:
1. The module graph resolves the alias and parses the file. Covered by
importing the probe through the server's module runner.
2. A browser is allowed to fetch it. The languages live outside the
frontend root, so they are reachable only if the dev server's
filesystem allow-list covers their directory.
Checking only the first passes while every language 403s in a browser.
Order is load-bearing and is commented as such: once a module is in the
graph the dev server answers from the transform pipeline instead of
reading the file, and the request stops passing through the allow-list.
Measured: 403 for a cold request, 200 for the same request after the
module has been loaded. So the reachability check resolves the path
without loading it and asks before anything else touches a language.
Observed failing on each of: alias removed, yaml plugin removed,
allow-list narrowed to the frontend root, and the application's import
shape changed out from under the probe.
Both run through make check-ui.
(cherry picked from commit 71cd924, without
its Go test file, which landed separately; check-locale-resolution.js
carries the content of commit 919d360 and
the config-path line of commit 1b59fa7;
check-built-assets.sh carries the self-check fixtures of commit
eab6f9d)
Plugin i18n modules register their translations while they are being evaluated, and i18next only attaches its resource-store methods during init. Which of the two happens first is decided by how the bundler groups and orders chunks, so it has to hold in both orders. The check loads the plugin helper without touching the application's i18n bootstrap, so i18next is guaranteed uninitialised, registers a bundle, and then initialises. Two assertions, because either one alone can pass while the feature is broken: registering must not throw, and the translations must actually be present afterwards. A fix that swallowed the error would satisfy the first and leave every plugin string untranslated. The check also asserts that exactly one i18next module is loaded. Without that, the helper and the check can each resolve their own copy and every later assertion silently measures an object nothing under test wrote to. Observed failing on the unguarded call with the same error the browser reports: addResourceBundle is not a function. (cherry picked from commit c27c204; the config path points at vite.config.mts, the name it carries on this branch, per commit 1b59fa7)
(cherry picked from commit 6ef81d3)
border-bottom was declared after the &:hover nested rule. Current Sass hoists trailing declarations above nested rules, so output is unaffected today, but a future Sass release will stop doing that and emit border-bottom last, changing rule order. Move it above the nested rule so source order already matches CSS order. Verified the compiled css is byte-for-byte identical before and after (same filenames, same content, same sha256 across every emitted css file). (cherry picked from commit a181e30)
The 403 route pointed at pages/403, one directory above the module that actually implements it. The real page lives at pages/404/403/index.tsx, a nested sibling of the generic 404 page, matching the same depth-two pattern already used by routes such as Admin/Mcp and Legal/Tos. Because the referenced module never existed, the route always fell through to the router's generic error boundary instead of rendering the real 403 content. Before the migration to Vite, a lookup miss like this failed silently and produced a blank or default fallback. The new router surfaces the mismatch as a visible error boundary, so the same wrong path now breaks loudly instead of quietly. Pointing it at the module that actually exists fixes both behaviors. (cherry picked from commit e50aa01)
go test -run with a pattern matching zero tests exits 0, not an error. If TEST_NAME in this script ever went stale, because the underlying test got renamed or deleted, the check would keep reporting success while testing nothing, forever, silently. Add a guard that lists the package for the exact test name before the first real check runs. If the name is not found, the script now fails loudly and explains why, instead of quietly turning into a no-op. (cherry picked from commit caf88df)
fail() in check-locale-resolution.js and check-plugin-i18n-order.js called process.exit(1) synchronously on an assertion failure. That skips any pending finally block, so the dev server each script starts was never closed once fail() ran. It only looked fine because killing the process tears down the listening socket anyway, but it left no real cleanup path, and it would have silently broken the moment anything meaningful was added after the finally in the future. fail() now throws instead, and is caught once at the top level, so the finally that closes the dev server always runs before the process exits. The top-level catch sets process.exitCode instead of calling process.exit, so the event loop can drain naturally rather than being killed mid-async-work. Both scripts also bound their dev-server-start and module-runner-import calls with a 30 second timeout, so a hang in either one fails the check instead of blocking it forever. Confirmed the fix by perturbing a working copy to force a failure: the new code prints the failure and exits 1, but only after the finally block's cleanup step actually runs; forcing the same failure through the old process.exit(1) shape prints the failure and exits 1 without ever reaching that cleanup step. (cherry picked from commit 98804a0)
The check only ever registered a plugin's translations before i18next.init ran, which exercises the deferred branch in initI18nResource: the listener registered for the 'initialized' event. The immediate branch, which fires synchronously when i18next is already initialised, had no coverage at all. Add a second registration right after init and read the resource bundle back with no i18next event and no other statement in between. The 'initialized' event has already fired for the first scenario at that point, so the new translations only land if the immediate branch actually runs. Without it, this second registration would never appear in the bundle, and every plugin whose module evaluates after i18next.init would render untranslated. (cherry picked from commit 90d1c25)
The three existing self-check fixtures all under-count: one drops the script, one drops the stylesheet, and one has a script tag with no src. None of them would catch a parser that over-matches, counting any link tag as a stylesheet regardless of its rel attribute. Add a fixture with a real script (has a src) and a single non-stylesheet link (rel="manifest") and nothing else. Only a parser that keys off rel="stylesheet" specifically, rather than just the presence of a link tag, correctly rejects this page. (cherry picked from commit 5b89c4b)
withTimeout() wrapped server.listen() but ran before either script could reach its own cleanup path. In check-locale-resolution.js a listen failure threw out of openProbe() before it returned the closable probe, so main()'s try/finally never started and the server openProbe() had already created was never closed. In check-plugin-i18n-order.js the same call sat above the try/finally entirely, so a listen failure skipped past the close() in finally. Vite creates the watcher and websocket server before listen() runs, so either shape leaves the process alive forever on a timeout: the event loop never drains and nothing closes the sockets. Moved the listen call inside a try that always reaches the close: an inline try/catch around openProbe()'s listen for the first file, and the existing try/finally extended to cover listen for the second. A scratch repro mirroring both shapes with a listen that never settles confirms the process now exits on its own once the timeout fires, instead of hanging until something else kills it. (cherry picked from commit 5fcb134)
creating the dev server can hang before listen is ever called, and a hang there leaves live handles that keep the process alive even after the timeout error is reported; bound creation with the same timeout, exit hard once the final catch has reported (all cleanup has run by then), and stop a failing close from replacing the error that actually caused the failure. close() was still unbounded: it awaits plugin buildEnd and closeBundle hooks with no timeout of its own, so a hung close blocked the failure path's own exit, and on the success path could hang the process after the OK line had already printed. Bound every close() call the same way, and exit explicitly on both success and failure, since a leaked handle from any bounded step, close included, must not keep a finished check alive. (cherry picked from commit 62955d7)
the earlier commits bound every known-hangable step; any await this or a future change leaves unbounded could still hang the run, or worse, drain the event loop and exit 0 silently. Preset the exit code to failure, add an unreferenced whole-run watchdog that forces a verdict through any held-alive hang, and keep both explicit exits, so every termination mode ends with the code that matches what was proven. (cherry picked from commit 61e41ce)
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, script/check-built-assets.sh only)
2 of 3 tasks
This branch has not been deployed
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
1 participant
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 #1581. Part of #1578. Step 3 of 3, not required to boot. Depends on #1616 (the Vite cutover, now merged into
dev).Three checks, wired as
make check-ui, for properties that a green build, a working dev server and a compiling binary do not demonstrate. Each one was observed failing on the failure it guards before being relied on.Asset paths,
make check-ui-assets.script/check-built-assets.shbuilds the frontend and runsTestGetStyleResolvesBuiltAssetsfrom step 1. Under--self-checkit rewrites the builtindex.htmlwith four shapes the server cannot use (a stylesheet link but no scriptsrc, a script but no stylesheet link, an inline-only script, and a non-stylesheet<link rel="manifest">that must not be miscounted as a stylesheet), asserts the check fails on each, and restores the real build afterwards, so a check that quietly stopped asserting anything is distinguishable from a passing one. It also fails if the Go test it depends on is ever renamed or removed, instead of silently matching nothing.Non-default locale,
make check-ui-locales.ui/scripts/check-locale-resolution.jsstarts the real dev server from the real config and importsui/scripts/locale-probe.js, which carries a copy of the application's own template-literal@i18nimport, through the server's module runner. It asserts two separate properties, because only one of them is about resolution: the module graph resolves the alias and parses a language file that lives outside the frontend root, and a browser is allowed to fetch it, which depends on the dev server's filesystem allow-list. Checking only the first passes while every language 403s in a browser. It requireszh_CNanden_USto resolve to distinct translated content.Plugin i18n order,
make check-ui-plugin-i18n.ui/scripts/check-plugin-i18n-order.jsloads the plugin helper without touching the application's i18n bootstrap, soi18nextis guaranteed uninitialised, registers a bundle, then initialises, and separately exercises the immediate path where an initialised instance already exists. Two assertions in each branch, because either alone passes while the feature is broken: registering must not throw, and the translations must actually be present afterwards. It also asserts exactly onei18nextmodule is loaded, so the harness cannot silently measure an object nothing under test writes to.All three fail closed. Each check presets its result to failure and only reports success once it has proved it. The two Node scripts bound every server-creation, listen and close call with its own timeout and add an unreferenced whole-run watchdog, so a hang cannot drain the event loop into an accidental success instead of a reported failure.
Small follow-ups.
src/components/Comment/index.scssdeclares its properties before nested rules, clearing the one app-own Sass mixed-declarations warning the step 2 build printed; output is unchanged.pages/403is repointed atpages/404/403, an existing component, so that route renders instead of failing through the error boundary. The comments oncheck-built-assets.shstate the actual constraint (tag shape does not affect parsing; the check fails when a build is present but a required tag is missing).Agentic tooling did the mechanical work in this series; every change was reviewed by a human before being committed.
Verification
On
devat6c00c788, which includes steps 1 and 2, plus these commits:pnpm install --frozen-lockfileunder the pinnedpnpm@9.7.0;make check-uigreen on all three targets;./script/check-built-assets.sh --self-checkreports all four fixtures failing as intended and the real build restored, withTestGetStyleResolvesBuiltAssetspassing again afterwards. The three targets print--- PASS: TestGetStyleResolvesBuiltAssets,OK: zh_CN and en_US resolve at runtime to distinct translated resources, and zh_CN is fetchable over the dev server, andOK: plugin translations registered before i18next.init (deferred) and after i18next.init (immediate) both survive and are present; the self-check reportscheck failed as expectedfor each of the four fixtures and ends with==> self-check passed. The build in this tree no longer prints the Sass mixed-declarations warning that step 2's build printed.None of this adds a CI job; the constraint for wiring one is in the tracking issue.
The commits carry
(cherry picked from commit ...)lines pointing at the branch behind #1567, where these checks were first reviewed.