fix(storybook): fail play errors fast and fix AccountsTab boot race - #72862
Conversation
The AccountsTab expanded-row stories kept timing out in visual regression
CI (3 attempts x 60s per story) despite five prior fix attempts. Reproduced
locally under CPU throttle and traced the full chain:
1. play() starts as soon as React commits the app shell, but the accounts
table renders only after the scene's data loads — several seconds on a
loaded runner. testing-library's default 1s findBy* timeout loses that
race, so `findByTitle('Show more')` threw before the table existed.
2. The generated test still runs postVisit after a play failure (with
context.hasFailure set, meant for failure handling). Our postVisit
ignored the flag and ran the full snapshot flow, whose 60s
waitForSelector outlasted the jest budget — burying the real error
under "Exceeded timeout of 60000 ms".
3. On jest retries the preview answers setCurrentStory with
`storyUnchanged`, which does not re-run loaders or play — so retries
re-tested the same broken page and could never pass.
Fixes: a 15s testing-library asyncUtilTimeout default for all stories
(plus explicit budgets in this file), postVisit honoring hasFailure so
play errors surface immediately, forced story remounts on retries so they
genuinely retry, and a 15s waitForSelector budget instead of 60s.
Verified against the CI storybook build under 4x and 6x CPU throttle:
previously wedged every run at 4x; now 7/7 stories pass repeatedly, and
induced failures report the real error in ~97s instead of ~556s.
Generated-By: PostHog Code
Task-Id: 0c854fe6-a5b4-41de-81ba-e60dcc455a5a
|
Hey @pauldambra! 👋 It looks like your git author email on this PR isn't your
You can fix it for this repo with: git config user.email "you@posthog.com"Or set it globally with |
🤖 CI report✅ Bundle size — no changeUncompressed size of every built Total: 64.68 MiB · no change No file changed by more than 1000 B. Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report ✅ Eager graph — within budgetHow much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy
🟢 Largest files eagerly shipped from
|
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 281.5 KiB | ../node_modules/.pnpm/posthog-js@1.407.1/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 236.0 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 226.1 KiB | ../node_modules/.pnpm/posthog-js@1.407.1/node_modules/posthog-js/dist/module.js |
| 167.1 KiB | src/queries/validators.js |
| 154.3 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 105.8 KiB | src/lib/api.ts |
| 94.0 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.18 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.18 MiB · 17 files | no change | ████░░░░░░ 38.1% of 5.72 MiB |
| Deferred (lazy) | 2.07 MiB · 33 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 714.7 KiB | dist/toolbar/toolbar-app-MKBF64XP.css |
| 545.3 KiB | dist/toolbar/chunk-chunk-HYYFIAXK.js |
| 484.2 KiB | dist/toolbar/chunk-chunk-QS5AHYGW.js |
| 133.6 KiB | dist/toolbar/chunk-chunk-3HAV52CR.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.0 KiB | dist/toolbar/toolbar-app-4WXEV7ZF.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-EL3T2QEA.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-HSIJKX5O.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — no change
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1359.33 MiB · no change
🦔 Hogbox preview · ❌ build failedThe preview didn't come up for commit Previews are optional and never block merging. A failure here is often a hogland or tailnet hiccup rather than anything in your PR, so the check stays green and this comment is the status. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c697780af9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Note 🤖 stamphog reviewed This rewrites shared Storybook CI test-runner logic (global async timeout, retry/remount semantics, and skip-on-failure behavior) that affects every play-based story across the repo — that's CI tooling risk, and the only reviews that scrutinized it (Codex, Graphite) are pinned to an older commit, not the current head; the newer "approval" comments are posted by the PR author's own account, which doesn't count as independent assurance.
Gate mechanics and policy version
Updated in place — this replaces 1 earlier stamphog review(s) on this PR. |
pauldambra
left a comment
pauldambra
left a comment
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
QA Swarm review complete. See inline comment.
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: router (cheap-first pass) + delegated reviewers (qa-team, paul-reviewer, xp-reviewer, security-audit as warranted) Verdict: ✅ APPROVE (round 2 @ 1f6adbc)Round 2 reviewed the two commits that landed in response to round 1 (Codex P1/P2 fixes + constant extraction): zero findings. The router verified against Storybook's bundled source that Key findingsNone this round. ConvergenceNone — single-reviewer round (no delegation). Reviewer summaries
Previous rounds (1)round 1 @ c697780 — ✅ APPROVE: one NIT (redundant Automated by QA Swarm — not a human review |
…etry failures The forced-remount wait in test-runner.ts's preVisit only listened for storyRendered/storyErrored/storyThrewException/playFunctionThrewException. Storybook can emit unhandledErrorsWhilePlaying and still go on to emit storyRendered for the same run, so a real play error was silently treated as a pass. A remount that hit the 30s timeout fell into the same trap — it resolved but wasn't in the failure list, so a still-running remount could race the snapshot flow. Also drops a redundant type cast on context.hasFailure — @storybook/test-runner's exported TestContext already declares that field. Generated-By: PostHog Code Task-Id: 0c854fe6-a5b4-41de-81ba-e60dcc455a5a
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Simplify pass: the remount failure events were listed twice (in-page done listener and the outer retry-failure check) with nothing tying them together, so an edit to one could silently drift from the other. One module-level constant now feeds both, passed into page.evaluate as an argument since the callback crosses the Node-browser boundary. Generated-By: PostHog Code Task-Id: 0c854fe6-a5b4-41de-81ba-e60dcc455a5a
page.evaluate() can reject if the page navigates or its execution context is destroyed mid-remount. The previous .catch(() => undefined) swallowed that into `undefined`, which then read as falsy in the failure check below — silently letting the retry proceed as if the remount had finished cleanly. Generated-By: PostHog Code Task-Id: 0c854fe6-a5b4-41de-81ba-e60dcc455a5a
The 30s remount-wait timer was never cancelled, so after the remount settled via a channel event it still fired ~30s later against the already-resolved promise — harmless but a leaked timer on every retried story. finish() now clears it first. Generated-By: PostHog Code Task-Id: 0c854fe6-a5b4-41de-81ba-e60dcc455a5a
pauldambra
added
the
stamphog
PostHog
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Contained fix to Storybook test-runner flakiness (timeout tuning, retry-remount handling, failure short-circuiting) — test infrastructure, not production/deploy code. The diff confirms both Codex P1/P2 concerns (accepting a retry before the real outcome, timeouts passing silently) and the evaluation-error-swallowing issue were actually fixed in code, not just marked resolved.
- Author wrote 0% of the modified lines and has 10 merged PRs in these paths (familiarity MODERATE).
- 👍 on the PR from hex-security-app[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 125L, 3F substantive — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (125L, 3F, two-areas, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 3817e82 · reviewed head aa45c3a |
The metalyticsLogic view beacon (POST /api/projects/:id/metalytics/) had no default MSW mock, so insight-rendering stories hit an unhandled 405 during play. With the retry now failing on unhandledErrorsWhilePlaying, that surfaced as a hard failure in stories like Paths and FunnelStepsBarChart. Stub it like the neighbouring insights/viewed beacon. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
Pure test/CI-infrastructure fix (storybook preview config, test-runner retry logic, mock handler, and one stories file) with no production code touched; Codex's two flagged issues (retry not waiting for the real failure event, timeout not counted as failure) and Graphite's silent-evaluation-failure concern are all verifiably fixed in the current diff, giving genuine independent-reviewer assurance beyond the author's own claims.
- Author wrote 0% of the modified lines and has 18 merged PRs in these paths (familiarity MODERATE).
- 👍 on the PR from hex-security-app[bot].
- The 'QA Swarm' discussion comment and inline reply were posted by the PR author's own account, not an independent reviewer — treated as author claim, not assurance, per policy; genuine assurance instead comes from the Codex/Graphite bot findings, which the diff shows were substantively addressed.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 128L, 4F substantive — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (128L, 4F, cross-cutting, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 09f143d · reviewed head 58b00f6 |
RepoPickerOpen failed all three attempts on a loaded VR shard: its waits used a local 8s budget (effectively ~7.5s after the 500ms delayed-mount effect), which loses the same full-App boot race this branch fixed for the AccountsTab stories. Now that retries genuinely re-run play, an under-budgeted wait fails hard instead of silently passing off a late-loaded page. Raise the budget to 30s, matching the AccountsTab precedent. Generated-By: PostHog Code Task-Id: 0c854fe6-a5b4-41de-81ba-e60dcc455a5a
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
CI test-runner tooling change, but Codex and Graphite each raised concrete correctness concerns about the new retry/remount logic (race on unhandledErrorsWhilePlaying, timeout not treated as failure, silent swallow of evaluate() errors) and the current diff visibly implements exactly those fixes (REMOUNT_FAILURE_EVENTS handling, 30s timeout treated as failure, evaluate().catch() mapped to a failure event) — independent assurance is satisfied and no unresolved concerns remain.
- Author wrote 0% of the modified lines and has 18 merged PRs in these paths (familiarity MODERATE).
- 👍 on the PR from hex-security-app[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 136L, 5F substantive — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (136L, 5F, cross-cutting, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 0c10618 · reviewed head 6b4add4 |
…meout phase RepoPickerOpen's play waited for `[data-attr="select-github-repository"] input`, but LemonInput spreads data-attr onto the <input> element itself, so the descendant selector never matched — the play function has never succeeded. The masked-retry behavior this branch removes used to re-snapshot the closed picker in update mode and pass anyway; with retries now honest, the story failed every attempt on every runner. Match the input directly. Also make the remount-wait timeout diagnosable: it now records the last storyRenderPhaseChanged phase and reports it (e.g. "last render phase: playing") instead of "unknown error", so a remount that times out points at the story's loading/play rather than at the machinery. Generated-By: PostHog Code Task-Id: 0c854fe6-a5b4-41de-81ba-e60dcc455a5a
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
Test-infrastructure-only change (Storybook test-runner and stories); the diff shows the two Codex P1/P2 correctness issues (accepting a retry on unhandledErrorsWhilePlaying, and not rejecting a timed-out remount) and Graphite's silent-catch concern are all explicitly fixed in the current code (unhandledErrorsWhilePlaying and 'timeout'/'evaluationFailed' are all included in the failure-event check), giving independent agent-reviewer assurance over this CI-tooling change.
- Author wrote 0% of the modified lines and has 18 merged PRs in these paths (familiarity MODERATE).
- 👍 on the PR from hex-security-app[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 158L, 5F substantive — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (158L, 5F, cross-cutting, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 84a0726 · reviewed head 06914e7 |
|
👋 Visual changes detected for this PR. Review and approve in PostHog Visual Review If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix. |
2 updated Run: 98a3edaa-542c-464d-a499-9b8af207cf3d Co-authored-by: pauldambra <984817+pauldambra@users.noreply.github.com>
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
# Conflicts: # products/customer_analytics/frontend/components/Accounts/AccountsTab.stories.tsx
# Conflicts: # products/customer_analytics/frontend/components/Accounts/AccountsTab.stories.tsx
There was a problem hiding this comment.
Storybook test-runner/harness fix for a well-documented flaky-test race; the genuine bot reviewers (Codex, Graphite) raised real concerns about the new retry logic and those are verifiably fixed in the current diff (unhandled-error listening, timeout rejection, evaluation-error handling all present). The author's own "QA Swarm" self-review comment is not counted as independent assurance, but the real Codex/Graphite reviews satisfy that requirement for this CI-tooling change.
- Author wrote 0% of the modified lines and has 605 merged PRs in these paths (familiarity MODERATE).
- 👍 on the PR from hex-security-app[bot].
- Change modifies CI test-runner retry/failure semantics (test infra, not production code) — independent assurance comes from Codex/Graphite bot reviews whose concerns are verified fixed in the diff, not from the author's self-styled 'QA Swarm' comment.
- Visual-review bot flagged snapshot changes; the two updated hashes correspond to an intentional selector fix described in the PR (input[data-attr] vs descendant selector) so this reads as expected, not a red flag.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 132L, 5F substantive — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (132L, 5F, two-areas, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 9922cca · reviewed head 79e3c31 |
1 participant
Problem
The
AccountsTabexpanded-row stories are the most persistent flake in the visual regression suite — five fix attempts since early July (#68802, #69984, #70144, #70243, #71020) and still firing: on 2026-07-22 alone it failed a master run (29904424573), both attempts of a PR run (29903063122), and an unrelated feature branch. Each incident burns 3 attempts × 60s per story (typically 3 stories ≈ 9 minutes of shard time) and fails the shard with an opaqueExceeded timeout of 60000 ms.Reproduced locally by running the CI storybook build artifact under 4× CDP CPU throttle, with request/console/Storybook-channel tracing and a pre-timeout CDP screenshot + CPU profile. The full causal chain:
play()as soon as React commits the app shell, but the accounts table only renders once the scene's data loads — several seconds later on a loaded CI runner. testing-library's default 1sfindBy*timeout loses that race:findByTitle('Show more')throws before the table exists, so the row is never expanded. (Captured screenshot at timeout: healthy page, fully rendered table, row collapsed; CPU profile 98% idle — no wedge, just a wait that can never succeed.)postVisiteven when play failed (context.hasFailure, intended for failure handling). OurpostVisitignored the flag and ran the full snapshot flow — whosewaitForSelector('[data-attr="account-expansion"]', 60000)waited the entire jest budget for an expansion that never happened. Jest then reports a generic timeout instead of theTestingLibraryElementError.setCurrentStorywithstoryUnchanged— loaders and play do not re-run, so attempts 2 and 3 re-snapshot the same broken page and fail identically. That's why incidents always cost the full 3 × 60s.Changes
preview.tsx: set a global testing-libraryasyncUtilTimeoutof 15s, so every story'sfindBy*/waitFortolerates slow scene boots by default (root cause fix, benefits all play stories).test-runner.tspostVisit: honorcontext.hasFailureand skip the snapshot flow, so a failed play surfaces its real error immediately instead of hanging into the jest timeout.test-runner.tspreVisit: on jest retries, force a story remount (re-running loaders and play) so retries genuinely retry, and surface the replayed play function's error if it throws again.AccountsTab.stories.tsx: explicit 30s find budgets in the play functions (belt-and-braces for this known-hot file),waitForSelectorTimeout60s → 15s so genuine failures fail fast, and comments updated to describe the real failure mode.How did you test this code?
Local reproduction harness: served the exact
storybook-buildartifact from the failing CI run and ran theAccountsTabfile with the repo's test-runner under CDP CPU throttling (simulating loaded CI runners; failure reproduced 2/2 runs at 4× before the fix, wedging the same stories as CI):Unable to find an element with the title: Show moreerror, and all 8 retries genuinely remounted and re-ran play (verified via Storybook channel event tracing).preview.tsxchange, since a full local storybook rebuild wasn't feasible in the sandbox): 7/7 stories passed on 3 consecutive runs at 4× throttle and 1 run at 6× throttle — conditions that previously wedged every run.oxlint/oxfmtclean on the three files;pnpm --filter=@posthog/frontend typescript:checkreports no errors in the changed files (pre-existing unrelated errors exist in this sandbox's environment).hogli ci:preflight(no flox python in the sandbox) or rebuild the storybook bundle locally. This PR's own CI run — including the flake-verification job, which re-runs the changed stories 5× — is the authoritative end-to-end check of thepreview.tsxchange.Automatic notifications
Docs update
No user-facing docs affected (CI test infrastructure only).
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
forceRemountchannel event with a 30s in-page settle guard.Created with PostHog Code