fix(#605): declare the ICU parser, correct the i18n docs, pin test concurrency - #664
Merged
Conversation
…ncurrency Three independent hygiene items left over from PR #599. Items 4 and 5 of the issue are product/sign-off decisions, not code, and are untouched. 1. `@formatjs/icu-messageformat-parser` is imported by the t.rich CI guard in web-ui/app/_lib/i18n-parity.test.ts but resolved only transitively through next-intl -> intl-messageformat. Declared as a devDependency at ^3.5.16, the version already in the tree, so the lockfile gains only the declaration. 2. web-ui/CLAUDE.md claimed `npm run i18n:check` fails on mismatched ICU/tag placeholders. It does not: `i18n:check` runs only scripts/i18n-validate.mjs (key sets, empty/non-string values, forbidden HTML, de===en warnings). The placeholder and rich-text-tag parity check is real but lives in the vitest guard, which CI runs separately. The checklist now names both commands and what each one actually catches — the routines crash that motivated #599 was exactly a placeholder mismatch that passed `i18n:check` with all keys present. 3. `test` pins --test-concurrency=4. Node derives the default from the core count, so a 16-core dev machine ran 16 files at once while a 4-vCPU CI runner runs ~4; the suite starts many short-lived Express servers on localhost and the issue records three consecutive runs each failing a DIFFERENT test, two with connection-level `fetch failed`, all passing in isolation. The pin makes the number the same everywhere. Rationale is in the CI comment next to the step, since package.json cannot carry one. Measured on a 16-core machine: unpinned 36.5s, pinned 48.0s (+31%), both green at 6174 tests. On a 4-vCPU runner the pin is close to a no-op, so the cost lands on large dev machines only. Note this run did NOT reproduce the flake — the pin is prevention based on the issue's evidence, not a fix proven here.
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
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.
Closes #605
Three independent hygiene items left over from PR #599. Items 4 (store "Jetzt installieren" vs. admin "VERBUNDEN") and 5 (
/setup403 gate relaxation) of the issue are product / sign-off decisions rather than code, so they are deliberately untouched here and the issue should stay open for them if this merges — see the note at the bottom.1.
@formatjs/icu-messageformat-parserwas undeclaredThe
t.richCI guard inweb-ui/app/_lib/i18n-parity.test.tsimports it, but it resolved only transitively vianext-intl → intl-messageformat. Declared as adevDependencyat^3.5.16— the version already in the tree — so the lockfile gains nothing but the declaration.No test can cover this one: the package is present either way today, which is exactly why the issue calls it a latent hoisting hazard rather than a live bug.
2.
web-ui/CLAUDE.mdpointed at the wrong gateThe checklist said
npm run i18n:checkfails on mismatched ICU/tag placeholders. It does not.i18n:checkruns onlyscripts/i18n-validate.mjs, which covers key sets, empty/non-string values, forbidden HTML, andde === enwarnings.The placeholder and rich-text-tag parity check is real and is enforced in CI — it just lives in the vitest guard, not in
i18n:check. Verified by runningnpm run i18n:checkon this branch: it reportsOK — 3565 keysplusde === enwarnings and says nothing about placeholders.The checklist now names both commands and what each actually catches. This matters because the routines crash that motivated #599 was precisely a placeholder mismatch that passed
i18n:checkwith all keys present — trusting the wrong gate is the documented failure mode.3.
testnow pins--test-concurrency=4Node derives the default from the core count. This machine reports
availableParallelism = 16, so it ran 16 files at once; a 4-vCPU CI runner runs ~4. The suite starts many short-lived Express servers on localhost, and the issue records three consecutive full runs each failing a different test (runtimeSecretsRoute,conductorTemplateRoutes,inboundWebhookRoutes), two with a connection-levelfetch failed, every one passing in isolation.Since
package.jsoncannot carry a comment, the rationale sits in the CI comment next to the step, matching howtest:pg's--test-concurrency=1is already documented.Measured cost, on a 16-core machine
=4+31 % locally. On a 4-vCPU runner the default is already ~4, so the pin is close to a no-op for CI wall clock — the cost lands on large dev machines only.
Corrected after further measurement (see #666). My first pass here said the flake had not reproduced. It does. On the #566 branch I hit a full 120 000 ms file timeout on
test/devplatform/devPlatformRoutes.test.tsat the default concurrency of 16 — that file runs in 1.77 s and 29/29 green in isolation.I then ran a controlled A/B, same commit, 3 rounds each at
--test-concurrency=4: 1 green / 2 red in both arms. So the pin reduces the flake — it turns a whole-file hang into an occasional single-testECONNREFUSED— but it does not make the suite deterministic on a 16-core machine. The issue's "fully green at 4" was a single run, and so was mine.Victims seen across runs:
builderIssueReporting,devPlatformGates,devWebhooks,devPlatformRoutes— all connection-level (ECONNREFUSED/fetch failed), which supports the localhost socket-contention theory in the issue.CI's 4-vCPU runner is consistently green today, so this does not currently bite there. Read this change as capping the blast radius on large dev machines, not as closing the flake. The contended resource still deserves the hunt the issue proposes as its other option; this change is trivially revertible if you would rather do that first.
Verification
web-ui:i18n:checkOK (3565 keys) · vitest 673 tests / 80 files, all pass · lint 0 errors · typecheck cleanmiddleware: build OK · 6174 tests, 0 fail, 4 skipped with the pin in placemain(3c86f1c8)Note for the reviewer
Closes #605will auto-close the issue on merge, but items 4 and 5 are still open questions. If you want them tracked, either reopen after merge or drop theClosesline and close manually — happy to split them into their own issue instead.