FE-1578: Replace fixed sleeps in Playwright tests with condition-based waits - #9529
Conversation
Wait for the state each sleep stood in for: an element attached or visible, the editor DOM reflecting the last keystroke, the document title reflecting the committed page title, or the updateBlockCollectionContents response. Drop sleeps that a following auto-waiting step already covers.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 4 Skipped Deployments
|
There was a problem hiding this comment.
🟡 Changes recommended
Page creation needs a reliable shortname-readiness condition, and the inbox test timeout needs additional headroom.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Replaces fixed Playwright sleeps with condition- and response-based waits to reduce test flakiness and runtime.
Changes:
- Adds DOM, title, visibility, and mutation-response waits.
- Removes obsolete fixed delays.
- Synchronizes notification and editor interactions with observable state.
File summaries
| File | Review |
|---|---|
tests/hash-playwright/tests/features/profile-page.spec.ts |
Waits for editor readiness and bio-save responses; no issues found. |
tests/hash-playwright/tests/features/page-readonly-mode.spec.ts |
Waits for pages-tree, but this does not guarantee the separately queried owner shortname is ready before page creation. |
tests/hash-playwright/tests/features/page-creation.spec.ts |
Adds editor, title, and persistence waits; the page-tree wait can race with owner-shortname readiness and prevent navigation. |
tests/hash-playwright/tests/features/inbox-page.spec.ts |
Replaces polling sleeps with visibility assertions, but the 60-second test timeout lacks sufficient headroom for two 30-second waits plus setup. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ox instead of polling The sidebar's create-page button only navigates to the new page once `useCreatePage` knows the web owner's shortname, which comes from a separate `queryEntitySubgraph` request that renders nothing in the sidebar. Waiting for `pages-tree` alone therefore left a race. A shared helper opens the home page, waits for that request's response and for the page list, and replaces the three call sites. The notifications page mounts a `network-only` query, so reloading it after creating each notification shows the new one at once. Assert with the default expect timeout and drop the test-level timeout, rather than waiting on the 10 s poll.
…page helper `getUser` returns the first root of the `me` subgraph, which is typed as possibly undefined under `noUncheckedIndexedAccess`, so `lint:tsc` failed with TS18048 on the unguarded access. Throw when no user comes back, as the inbox spec does.
PR SummaryLow Risk Overview A new shared helper
Production code is unchanged; only test helpers and assertions. Reviewed by Cursor Bugbot for commit 7223b79. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Dequeued at 22:00 UTC by an unrelated Playwright failure in the queue run (run 33808993925): Generated by Claude Code |
|
Dequeued again at 22:29 UTC by the same test, in run 33811421092: Generated by Claude Code |
Requested by Tim Diekmann · Slack thread
🌟 What is the purpose of this PR?
Before: four Playwright specs under
tests/hash-playwrightpaused with fixedsleep(...)calls — 100 ms between editor keystrokes, 500 ms before clicking buttons, 2 s before a reload, 12 s for a notification poll. On a loaded runner the app can take longer than the sleep, so the next step runs too early and the test fails; in the merge queue that dequeues the whole batch. When the app is faster, the sleep only adds wall time.After: each sleep is replaced by a wait on the state it stood in for — an element attached or visible, the editor's DOM reflecting the last keystroke, the document title reflecting the committed page title, or the
updateBlockCollectionContentsmutation's response. Sleeps that a following auto-waiting step already covered are deleted.🔗 Related links
🚫 Blocked by
🔍 What does this change?
page-creation.spec.tsopenHomeReadyToCreatePage(new shared helper): opens/, waits for the response to the sidebar'squeryEntitySubgraphrequest for the web owner (useCreatePageonly navigates to the new page once that shortname is known, and nothing in the sidebar renders from that query), then forpages-treeto be attached (addPageis a no-op while pages load)expect(em).toHaveText("italics")Enterexpect(blockHandles).toHaveCount(2)/divider/divider/ivisiblehrexpect(blockHandles).toHaveCount(3)expect(blockRegion).toContainText(...)Shift+Enterexpect(hardBreaks).toHaveCount(n)onbr:not(.ProseMirror-trailingBreak)EntertoHaveCount(4)followswaitForResponsefor theupdateBlockCollectionContentsresponse whose returned contents hold 4 blocks, armed before the finalEnterexpect(page).toHaveTitle(...)with the committed name — the document title comes from the refetched page query, so it changes only once the saved title has propagated back to the pagepage-readonly-mode.spec.tsopenHomeReadyToCreatePage, which also supplies the navigation this test was missinginbox-page.spec.tstest.setTimeout(60_000)to fit thempage.reload()after creating each notification, thentoBeVisible()with the default timeout — the notifications query is anetwork-onlyuseQuerymounted by the page, so a reload fetches at once instead of waiting for the poll;test.setTimeoutremovedprofile-page.spec.tswaitForResponsefor theupdateBlockCollectionContentsrequest carrying the bio text, armed before typingtoBeVisibleassertions followLeft as they are: the poll interval in
shared/get-kratos-verification-code.ts, the TOTP window wait inshared/totp-utils.ts, the debounce helper inextension/browser-plugin.spec.ts, and thesleep(500)inentity-editing.spec.ts, which #9526 removes.playwright.config.tsis unchanged: no retries, no timeouts.Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
page-creation,page-readonly-mode,profile-page) aretest.skip'd, so CI exercises onlyinbox-page.spec.ts.page-creation.spec.ts(3 blocks after inserting the divider) take the counts the test already asserted as correct; they are not re-verified here.🐾 Next steps
🛡 What tests cover this?
inbox-page.spec.tsruns in CI.❓ How to test this?
yarn test:integrationintests/hash-playwrightagainst a local stack.inbox-page.spec.tspasses in a few seconds rather than the 24 s it used to sleep.