fix(stakes): fix dialog races causing e2e false positives - #434
Open
yhabib wants to merge 3 commits into
Open
Conversation
Two timing-dependent bugs were producing most of the E2E failures in CI. Both are user-facing, not test artifacts. Navigation blocker race: the four neuron detail action views returned to the summary via setTimeout(onSuccess). onSuccess triggers a router navigation, which NavigationBlockerDialog intercepts while isProcessing is still true. A zero-delay timer does not guarantee React has committed and flushed the effect that disarms the blocker, so under load the navigation was silently swallowed and the dialog stayed open after a successful mutation. The parent now flags success and navigates from an effect, relying on React running child effects before parent effects. Staking wizard reset: state was reset on a 300ms timer after close, but the effect cleanup clears that timer when the modal reopens first, so the reset never ran and the wizard reopened stuck on the previous success screen with no amount input. Reset on open as well, and use useEffectEvent so the mutation object no longer re-arms the timer on every render. Trace decoding confirmed the ledger transfer returned Ok and claimOrRefresh succeeded in the failing runs, so the on-chain work was never the problem.
The E2E job did all the work on one runner with no caching: npm ci (~70s), a full Vite build and a cargo release build inside dfx deploy (~47s), and a Playwright browser install (~25s) on every run. npm ci ran in five separate jobs per PR, and the backend compiled twice. - Shard E2E into parallel chromium and mobile jobs. Previously both projects shared one runner and ran nearly back to back. - Cache npm, cargo and Playwright browsers via workspace-local dirs, so they are reachable both from actions/cache and from inside the mounted container. - Add a concurrency group with cancel-in-progress for pull_request only. merge_group is deliberately excluded, since cancelling those would drop a merge. - Drop e2e retries from 2 to 1. Every spec is a describe.serial block, so a retry re-runs the whole block including its expensive beforeAll; two retries meant one stuck test could burn ~15 minutes before reporting. Snapshot artifacts are now per project, so update-snapshots.sh downloads by pattern instead of by exact name.
|
✅ No security or compliance issues detected. Reviewed everything up to e1153e8. Security Overview
Detected Code Changes
|
📊 Build Bundle StatsThe latest build generated the following assets: |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses two user-facing timing/race issues in the staking and neuron-action dialogs that were causing E2E false positives, and reduces CI E2E runtime by improving test synchronization and parallelizing/caching the E2E workflow.
Changes:
- Fix neuron detail action “return to summary” navigation race by deferring navigation to a parent effect after the navigation blocker is disarmed.
- Fix staking wizard state reset race by resetting on open as well as (delayed) on close, using
useEffectEventto avoid timer re-arming across renders. - Speed up and stabilize CI E2E runs via better waits, Playwright project sharding, dependency/browser caching, reduced retries, and PR-only concurrency cancellation.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/governance-app-frontend/tests/e2e/advanced-following.spec.ts | Replaces a fixed sleep with a visibility-based wait after staking wizard completion. |
| src/governance-app-frontend/src/features/stakes/components/stakingWizard/StakingWizardModal.tsx | Resets wizard state on open and (delayed) on close to avoid reopening on a stale success step. |
| src/governance-app-frontend/src/features/stakes/components/neuronDetail/NeuronDetailModal.tsx | Removes timer-based “go back” and introduces parent-controlled success navigation sequencing. |
| src/governance-app-frontend/src/features/stakes/components/neuronDetail/NeuronDetailMaturityModeView.tsx | Calls onSuccess() directly; parent now handles safe navigation timing. |
| src/governance-app-frontend/src/features/stakes/components/neuronDetail/NeuronDetailIncreaseStakeView.tsx | Calls onSuccess() directly; parent now handles safe navigation timing. |
| src/governance-app-frontend/src/features/stakes/components/neuronDetail/NeuronDetailIncreaseDelayView.tsx | Calls onSuccess() directly; parent now handles safe navigation timing. |
| src/governance-app-frontend/src/features/stakes/components/neuronDetail/NeuronDetailDissolveView.tsx | Calls onSuccess() directly; parent now handles safe navigation timing. |
| src/governance-app-frontend/playwright.config.ts | Lowers CI retries to 1 with rationale tied to describe.serial cost. |
| scripts/update-snapshots.sh | Downloads multiple per-shard snapshot artifacts via --pattern and searches recursively. |
| .gitignore | Ignores workspace-local CI cache directory .ci-cache/. |
| .github/workflows/pr_checks.yml | Adds PR-only concurrency cancellation to supersede in-flight runs for the same PR. |
| .github/workflows/pipeline_dev.yaml | Adds PR-only concurrency cancellation to supersede in-flight runs for the same PR. |
| .github/workflows/e2e.yml | Shards E2E per Playwright project, adds caches, and uploads per-project artifacts/reports. |
| .github/workflows/check.yaml | Adds workspace-local npm/cargo cache dirs and restores caches in relevant jobs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot review feedback. The success screen can disappear mid-close-animation once the wizard state resets, so it is not a reliable signal that the dialog has closed. Wait on staking-wizard-dialog with an explicit timeout instead. Also fixed the stale single-artifact name in the update-snapshots.sh early-exit note now that snapshots are uploaded per project.
artkorotkikh-dfinity
approved these changes
Aug 7, 2026
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.
Motivation
Most of the E2E failures in CI came from two timing bugs in the app, not from the tests. Both are user facing: a neuron action dialog could stay open after a successful mutation, and the staking wizard could reopen stuck on the previous success screen. Decoding the traces from failing runs showed the ledger transfer returned
OkandclaimOrRefreshsucceeded, so the on-chain part always worked. While in there, also cut the E2E runtime.Changes
setTimeout(onSuccess), but a zero-delay timer does not guarantee React has flushed the effect that disarmsNavigationBlockerDialog, so under load the navigation was swallowed. The parent now flags success and navigates from an effect, which runs after the blocker is disarmed.useEffectEventso the mutation object no longer re-arms the timer on every render.stakeNeurone2e helper with a wait for the wizard to actually close.actions/cacheand inside the mounted container.cancel-in-progressforpull_requestonly.merge_groupis excluded on purpose, cancelling those would drop a merge.describe.serialblock, so a retry re-runs the whole block including its slowbeforeAll.