chore: bundle App fixes — Bug #1 + errorMessages + CI tolerate_failures + fresh Goldens - #129
Merged
Merged
Conversation
Currently the workflow runs every spec under e2e/0*.spec.ts e2e/1*.spec.ts unconditionally and the whole job aborts (set -e) the moment any spec fails. When the only thing that needs regenerating is the wallet header — and an unrelated spec (e.g. 07-send) is flaky on DEV — the regen can't even reach its commit step. The fix that needs the baselines gets stuck behind a problem it has nothing to do with. Two new workflow_dispatch inputs: - `spec_glob` (default unchanged) — narrows the run to specific files. e.g. `e2e/02-create-seed.spec.ts e2e/03-restore-seed.spec.ts e2e/04-unlock-password.spec.ts` only refreshes the wallet headers. - `tolerate_failures` (default false, opt-in) — appends `|| true` to the playwright invocation. `--update-snapshots=all` still writes new PNGs for the specs that ran; the commit step inspects `git status` and commits only files that actually changed. Partial regens are safe. Defaults preserve current behaviour — full suite, fail-fast — so existing routine calls keep working.
… strings The node API emits diagnostic, field-naming error strings (e.g. "account_address is not valid hex", "recipient must be 32 bytes (64 hex chars)", "Failed to broadcast commitment inscription on-chain") that did not match the exact-string lookup table on the app side, so users saw the raw "Serverfehler <status>: ..." fallback instead of the translated message. Adds a second lookup layer SERVER_ERROR_PATTERNS (regex) that runs only when the existing exact-match pass misses. KNOWN_SERVER_ERRORS stays unchanged so the lockstep test with api_remote.rs::error_strings_match_known_app_mapping keeps holding. Patterns are ordered specific-before-generic and intentionally do not overlap with any KNOWN_SERVER_ERRORS string — exact-match always wins. Three new test cases in error-mapping.test.ts cover one variant per pattern, including an address-length variant with a different field name to prove the family (not a specific field) is matched.
The node reports `network` as a display string ("Mainnet", "Mutinynet",
…) — see `node::router::info_handler`. The existing check used a
case-sensitive comparison, so `"Mainnet" !== "mainnet"` evaluated true
and the faucet button rendered on Production. Lowercase the network
name before the comparison so a casing decision on the server can't
accidentally re-enable mint on Mainnet.
Add a WalletScreen unit test for both branches (Mainnet hidden,
Mutinynet visible). Update the `06-balance-zero-empty-banner` e2e
spec to reflect testnet reality (faucet button is the primary CTA
in the empty-wallet banner on Mutinynet); the Mainnet-hidden case
is covered at the unit-test layer since E2E always runs against
the testnet API.
Contributor
[OK] Button-Inventory-Audit — all clearChecked 71 testid(s) in |
…atency The send-flow now does an extra `/api/balance` round-trip before signing (`num_sends` hydration per the thin-client rule in CONTRIBUTING.md), and the matching server-side writes from zk-coins/node #129 / #132 add a few seconds of legitimate per-send latency for the atomic `Account.num_sends` + `commitment_public_key` upsert. Observed wall-time on the bundle PR's CI runs is 180-220 s, which the previous 180 s test cap was treating as a deadlock. Lift the waitFor cap from 150 s to 250 s and the overall test cap from 180 s to 300 s. The race against the inline error banner stays in place so a genuine server-side failure still surfaces with its real message instead of timing out.
…allel-load
The 300 s cap from the previous commit was still insufficient on the
bundle PR's CI. Empirically the send-success spec lands between
180 s and ~5 min on DEV. The variance is driven by:
* Publisher track-tx WS jitter — 30 s timeout per attempt, up to
three attempts under load when Mutinynet block-event delivery
stalls (see `node/src/publisher.rs::TRACK_TX_TIMEOUT_SECS`).
* Playwright `fullyParallel: true` saturates the shared DEV node
with mint + balance traffic from neighbour specs while
07-send-success is its only real-send test, but everything else
keeps hammering /api/balance, /api/info, and (via globalSetup)
/api/mint — node mutex contention pushes the lone send past
its happy path.
Raise the spec cap to 480 s and the waitFor cap to 420 s. Deadlock
still surfaces; well-behaved-but-slow runs no longer mask as a
regression. Follow-ups (out of scope for this PR): (a) tighten the
publisher's track-tx budget, (b) consider serializing the
mutation-heavy specs against the DEV node.
The CI runner's first page.goto('/') after boot occasionally exceeds
Playwright's default 30 s navigation timeout — Cloudflare cold path,
Next.js standalone first-paint, and the WASM bundle handshake all
stack on the very first request. Subsequent navigations in the same
context are fast.
- Raise the context-wide navigation timeout to 90 s for both wallet-
bootstrap contexts. Scoped to globalSetup only so test specs keep
the 30 s default and a genuine app-side regression still surfaces
as a timeout.
- Wrap each wallet bootstrap (clearWalletState + createSeedWallet) in
a single retry on a fresh page so a transient first-paint timeout
doesn't fail the whole CI run.
- Fix a TS strictness regression in 07-send.spec.ts where
textContent() returns string | null and was passed directly to
expect's message slot (string | undefined).
The 480 s cap introduced in 3abecfa was still consistently hit by the real Send through DEV under the suite's parallel load. Both attempts (test + retry) timed out, with the waitFor(420 s) draining before the test cap. Empirically the DEV Send takes 180 s to ~7 min depending on Mutinynet block-time jitter plus neighbouring specs hitting the node. Raising the cap is one half of the calibration; the other half is reducing load: - on the file keeps the 13 in-file tests in a single worker so two 07-send tests can no longer fight each other for DEV bandwidth. - (12 min) and the waitFor budgets raised to 660 s (11 min). Setup overhead (login + balance hydration) lives in the remaining 60 s of headroom. A genuine deadlock still surfaces in 11 min instead of 7. The follow-up to tighten publisher track-tx is unchanged.
Three consecutive E2E runs failed on `07-send-success` at progressively
larger test-timeout caps (180 s -> 300 s -> 480 s -> 720 s). Every time
the DEV node's single proof-gen pipeline was starved by parallel mint
and balance traffic from the rest of the suite (09 / 10 / 11), pushing
the real Send wall-clock above the cap.
Increasing the cap further is the wrong direction. Structural fix:
split the existing E2E step in two.
Step 1: `npx playwright test --grep-invert "send-success"` -- the
parallel-safe bulk of the suite (75 specs) running with the default
fullyParallel + 2 workers.
Step 2: `npx playwright test --grep "send-success" --workers=1` --
the one mutating Send test with exclusive DEV-node bandwidth, no
competing proof-gen / balance traffic from neighbouring specs.
Side fixes:
- Drop `test.describe.configure({ mode: 'serial' })` from the spec.
Serial mode caused a cascade-retry failure where send-no-funds-banner
re-ran on the group retry and failed for an unrelated reason.
- Lower send-success cap back to 6 min (test) + 5 min (waitFor) since
exclusive bandwidth drops the realistic upper bound to ~3-4 min.
globalSetup runs once per playwright invocation, so each step gets a
clean fresh-Alice + fresh-Bob fixture set independent of the other.
ROOT CAUSE of all `07-send-success` failures since PR #104. The test races the success heading against the inline error banner via `Promise.race`, then resolves both: await error.textContent().catch(() => null) The catch handles a *rejected* promise, but Playwright's `Locator.textContent()` does not reject when no element matches — it WAITS for the element to appear, blocking up to the test-timeout. On the success page (the happy path) `send-error` is not in the DOM, so textContent never resolves and the assertion at line 216 sat spinning until the 360 s test cap fired, even though the success heading had rendered minutes earlier. Failure-screenshot evidence from run 26651174055: the page clearly shows "Sent privately" with proof #183 / #184 in both attempt 1 and the retry. The Send succeeded server-side; only the post-race assertion blocked. Fix: tag which branch of the race won, then act only on that branch. The error branch resolves textContent (which is safe because the locator just became visible). The heading branch skips the error check entirely. This was the intent of the race-against-error pattern from the start — the implementation just resolved the error too eagerly. Side-effect: with this fix the previous timeout-budget escalations (180 s -> 300 s -> 480 s -> 720 s -> 360 s) were all chasing the wrong cause. The 360 s cap stays — it's a reasonable upper bound for a real DEV-node Send, but the test now resolves as soon as the success heading appears.
The 07-send-success golden was generated against the broken textContent hang (test never reached the snap() call, so no PNG was ever captured). With the race-winner guard in a15cc24 the test now actually takes a fresh screenshot of the success page and compares it to the on-branch golden, which had drifted with the post-2026-05-23 header/balance layout pass. Includes three other 07-send goldens that the same regen run picked up (confirm-dialog desktop/mobile + recipient-valid-hex) — the maxDiffPixelRatio: 0.01 tolerance had been masking the same drift under the threshold; refreshing them now keeps the diffs at 0 px for any future regression that crosses the threshold. Regen run: 26652625153 against chore/all-app-fixes-bundle.
TaprootFreak
marked this pull request as ready for review
May 29, 2026 17:50
MAJOR (security):
- regenerate-visual-baselines.yml — command injection via
`${{ inputs.spec_glob }}` interpolated into a shell string and
`eval`'d. Replaced with env-var passthrough + array-form CMD,
no eval. Glob expansion still works ($SPEC_GLOB unquoted), but
user-controlled metachars are now data, not parser tokens.
CWE-78.
MINOR:
- errorMessages.ts — doc claim "patterns must NOT overlap with
KNOWN_SERVER_ERRORS" was wrong (overlap exists today, exact-first
shadows it harmlessly). Rewrote the comment to reflect reality
and added the test that the comment now references:
`every KNOWN_SERVER_ERRORS string resolves via the exact-match
table, not via a pattern`. Catches a future refactor that
deletes an exact entry and lets a pattern silently swap the
German copy. Also exported SERVER_ERROR_PATTERNS +
SERVER_ERROR_TO_USER_MESSAGE so the test can introspect them.
- _global-setup.ts::withPageRetry — page leaked on the success
path (no close after `return await step(page)`) and on the
terminal throw. Moved close into a `finally` block.
- ci.yaml::e2e-tests — added `timeout-minutes: 30` so a hung
browser process can't sit on the default 6 h budget.
Subagent re-review NITs: - error-mapping.test.ts had a tautological inner assertion (userMessageFor always returns the exact entry first, so the conditional check could never fire). Line 76's .toBeDefined() already enforces the real lockstep invariant — trim the rest. - SERVER_ERROR_PATTERNS no longer needs export now that the test doesn't consult it. Reverted to module-private. - SERVER_ERROR_TO_USER_MESSAGE stays exported (the test needs it), marked @internal so production callers stick to userMessageFor().
6 tasks
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.
Summary
Single PR that bundles the App-side fixes plus a Goldens refresh, so the App can ship them as one atomic update with green CI.
What's included
ci(regen): add spec_glob + tolerate_failures inputs to baseline regen— from PR ci(regen): add spec_glob + tolerate_failures inputs to baseline regen #115. Makes the baseline-regen workflow robust: partial refreshes (some specs fail) commit the goldens that DID regenerate, instead of all-or-nothing.fix(errorMessages): family pattern matching for diagnostic node error strings— from PR fix(errorMessages): family pattern matching for diagnostic node error strings #124. App now matches the node's diagnostic per-field error strings ("account_address is not valid hex","must be 32 bytes …","Failed to broadcast …on-chain") via regex fallback after the existing exact-match table.fix(wallet): case-insensitive network check for faucet gating— from PR fix(wallet): case-insensitive network check for faucet gating #126. The node reportsnetwork: "Mainnet"(CamelCase); the App's case-sensitive!== 'mainnet'evaluated true → faucet button rendered on Production. Lowercases the comparison so a casing decision on the server can't re-enable mint on Mainnet. Adds unit tests for both branches; updates06-balance-zero-empty-bannerE2E spec to reflect that on testnet the faucet button IS the empty-wallet CTA.test(e2e): regenerate visual baselines— fresh chromium-linux PNGs for the specs whose layout drifted since 2026-05-23. The 07-send goldens picked up a final refresh after the textContent fix below (the success page was unreachable before, so the golden was effectively stale).test(e2e): harden globalSetup against cold-start navigation timeouts— the CI runner's firstpage.goto('/')after boot occasionally exceeded Playwright's default 30 s navigation timeout. Context-wide nav timeout 90 s + single retry on a fresh page, scoped to globalSetup only.ci(e2e): give send-success exclusive DEV-node bandwidth via two-step job— split the E2E step in two. Step 1 runs the parallel-safe bulk of the suite (75 specs, ~3 min). Step 2 runs onlysend-successwith--workers=1, so the one real Send through DEV doesn't compete with parallel mint/balance traffic from 09 / 10 / 11.fix(e2e): guard error.textContent() behind race-winner check— root cause of all07-send-successfailures since PR fix(e2e): 07-send 'send-success' — bump timeouts + race against error #104. The race-against-error pattern resolvederror.textContent()unconditionally, but Playwright'sLocator.textContent()blocks waiting for the element to appear instead of returning null. On the success page (the happy path)send-errordoesn't exist, so the assertion sat spinning until the test cap fired, even though the success heading had rendered minutes earlier. Fix: tag the race winner and only resolvetextContent()on the error branch.Why one bundle PR
Bug #2 (the
prove_account_update failedregression) was fixed in PR #127 and is already merged + deployed; with #127 live, all remaining changes here are independent and small. Bundling them lets us go from "CI red across every App PR" to "develop is green and ready for a Release PR" in a single merge.Test plan
npx vitest run— 323 passed (+3 skipped contract.live)npx next lint,npx prettier --check "src/**/*.{ts,tsx}",npm run buildclean3dc332b