Skip to content

Fixed a flaky members import E2E test opening a member detail - #30429

Merged
acburdine merged 1 commit into
mainfrom
claude/e2e-test-failure-check-17f6d9
Sep 1, 2026
Merged

Fixed a flaky members import E2E test opening a member detail#30429
acburdine merged 1 commit into
mainfrom
claude/e2e-test-failure-check-17f6d9

Conversation

@acburdine

Copy link
Copy Markdown
Member

Why

E2E Tests (Main 1/10) went red on main at 0b8af54, with import-custom-fields.test.ts:92 failing all three attempts. It is not new: the same test failed once and passed on retry in 3 of the 12 preceding main runs — about a quarter of attempts — and this run simply lost every retry.

From the failing run's Playwright trace, the failure is not about the imported value. After the row click, the member-detail chunk loads but GET /members/<id>/ is never issued, and the URL timeline reads:

211714  #/members?filter=label:[import-2026-09-01-14-15]   modal close navigates
211774  #/members                                          test goto strips it
211930  click member row
212012  #/members?filter=label:[import-2026-09-01-14-15]   app rewrites, replacing the detail route

The members list mirrors its filter state into the URL from an effect (use-members-filter-state.ts), and that write is a navigation of its own. One landing right after the click replaces the detail route with the list again, so the assertion waits out its timeout on the screen it started from.

What

openMemberByName re-clicks the row when the detail screen does not appear, instead of waiting out a timeout on a screen the app navigated away from. Three attempts, 5s each, against a 60s CI test timeout on a test that normally runs in ~13s.

The app bug stays open

A member opened straight after an import can send the reader back to the list — this only stops the test from being red. Two fixes to the URL sync were tried and rejected:

  • Merging the adopt and normalize effects so an externally-changed query is not overwritten. currentQuery lags the last-written query by one render after every setFilters, so the adopt branch fires on the app's own writes and resets the draft: the composite custom-field filter cascade loses its operator selection and custom-field-filter-value never renders. Failed an address filter matches on the chosen part only 2/2, which passes 3/3 without the change.
  • Guarding the write with useNavigation().state !== 'idle'. Needs a data router; every MemoryRouter-based harness in apps/admin then throws, failing the members unit suite, and no unit test could be made to fail without the guard.

The real fix looks like making the URL the single source of truth for committed filters and keeping in-progress predicates local, so no background write can preempt a navigation.

Testing

Locally, against the dev stack:

  • Members import with custom fields — 4 passed
  • custom-field-filter-* tests, run individually — pass
  • admin unit suite — 512 passed; e2e lint 0 errors; e2e test:types clean

🤖 Generated with Claude Code

no ref

The members list writes its own query string back after something changes it —
closing the import modal applies the import's label filter — and that write is
a navigation of its own. One landing right after a member row is clicked
replaces the detail route with the list again: the detail chunk loads, the
member is never fetched, and the assertion waits out its timeout on the list.

`import-custom-fields.test.ts` hit this on roughly a quarter of attempts and
lost all three retries on 0b8af54, taking the E2E shard red. Re-click the row
when the detail screen does not appear, rather than waiting out a timeout on a
screen the app navigated away from.

The app-side race is real and stays open: a member opened straight after an
import can send the reader back to the list.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@acburdine
acburdine requested a review from 9larsons as a code owner September 1, 2026 15:06
@acburdine
acburdine requested a review from rob-ghost September 1, 2026 15:06
@nx-cloud

nx-cloud Bot commented Sep 1, 2026

Copy link
Copy Markdown

🤖 Nx Cloud AI Fix

Ensure the fix-ci command is configured to always run in your CI pipeline to get automatic fixes in future runs. For more information, please see https://nx.dev/ci/features/self-healing-ci


View your CI Pipeline Execution ↗ for commit 8a6913a

Command Status Duration Result
nx run @tryghost/e2e:test:fixtures ✅ Succeeded 5s View ↗
nx run-many --target=build --projects=tag:publi... ✅ Succeeded <1s View ↗
nx run ghost-monorepo:lint:boundaries ✅ Succeeded <1s View ↗
nx run-many -t lint -p @tryghost/e2e ✅ Succeeded 5s View ↗
nx run @tryghost/admin:build ✅ Succeeded 9s View ↗

💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗


☁️ Nx Cloud last updated this comment at 2026-09-01 15:12:33 UTC

@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Team

Run ID: aa726cb4-fe8b-46f0-89eb-8c43bccbd87f

📥 Commits

Reviewing files that changed from the base of the PR and between 733862a and 8a6913a.

📒 Files selected for processing (1)
  • e2e/helpers/pages/admin/members/members-list-page.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📜 Recent review details
⏰ Context from checks skipped due to timeout. (6)
  • GitHub Check: Build Ghost-CLI archive
  • GitHub Check: Coverage
  • GitHub Check: Stripe fixture checks
  • GitHub Check: Build Docker Images
  • GitHub Check: Build E2E Public App Assets
  • GitHub Check: Detect Tinybird changes
🧰 Additional context used
📓 Path-based instructions (6)
Review fixture/page-object lifecycle, concurrency, reset timing, reusable

⚙️ CodeRabbit configuration file

Files:

  • e2e/helpers/pages/admin/members/members-list-page.ts
Review lens: "where does this data become trusted?"

⚙️ CodeRabbit configuration file

Files:

  • e2e/helpers/pages/admin/members/members-list-page.ts
Prioritise concrete correctness, security, data-integrity, compatibility,

⚙️ CodeRabbit configuration file

Files:

  • e2e/helpers/pages/admin/members/members-list-page.ts
Follow the locator priority in the E2E writing guide; do not copy generated

📄 CodeRabbit inference engine (e2e/AGENTS.md)

Files:

  • e2e/helpers/pages/admin/members/members-list-page.ts
Type-safe boundaries: Fail only if the PR:

📄 CodeRabbit inference engine (Custom checks)

Files:

  • e2e/helpers/pages/admin/members/members-list-page.ts
Always use `pnpm`, never npm or Yarn.

📄 CodeRabbit inference engine (e2e/AGENTS.md)

Files:

  • e2e/helpers/pages/admin/members/members-list-page.ts
🔇 Additional comments (1)
e2e/helpers/pages/admin/members/members-list-page.ts (1)

11-13: LGTM!

Also applies to: 26-28, 47-47, 60-86


Walkthrough

MembersListPage now detects the member detail screen with a public locator. openMemberByName waits up to five seconds for detail navigation and retries the member-link click up to three times when navigation returns to the list. The final timeout is rethrown.

Suggested reviewers: 9larsons, rob-ghost

Merge Risk: ⚪ Minimal · up to 8a691

This change makes the members import test retry the row click when the detail view does not open, without changing product behavior. No actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: fixing flaky E2E navigation when opening a member detail screen.
Description check ✅ Passed The description directly explains the flaky navigation, the retry-based fix, the unresolved application bug, and the testing performed.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Type-Safe Boundaries ✅ Passed PASS. The PR changes only the E2E page helper e2e/helpers/pages/admin/members/members-list-page.ts, which is test support and is exempt under the check. The added code uses typed Playwright `Locator…
New Files Are Typescript ✅ Passed PASS: The pull-request commit changes only the pre-existing e2e/helpers/pages/admin/members/members-list-page.ts file. The parent-to-HEAD tree comparison reports zero added files and zero added `.js…
Full details: Type-Safe Boundaries

Explanation

PASS. The PR changes only the E2E page helper e2e/helpers/pages/admin/members/members-list-page.ts, which is test support and is exempt under the check. The added code uses typed Playwright Locator values, numeric constants, and a caught-and-rethrown error. It adds no boundary-data consumption, any, unchecked assertion, @ts-ignore, or @ts-nocheck. The existing downloadPath as string is unchanged and therefore is not introduced by this PR.

Full details: New Files Are Typescript

Explanation

PASS: The pull-request commit changes only the pre-existing e2e/helpers/pages/admin/members/members-list-page.ts file. The parent-to-HEAD tree comparison reports zero added files and zero added .js, .jsx, .cjs, or .mjs files.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/e2e-test-failure-check-17f6d9

Comment @coderabbitai help to get the list of available commands.

@acburdine
acburdine merged commit e87001d into main Sep 1, 2026
51 checks passed
@acburdine
acburdine deleted the claude/e2e-test-failure-check-17f6d9 branch September 1, 2026 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant