[JacobStephens2]

Summary

Site browser tests now wait for the state an action should produce, rather than reading it once straight after a click, key press or navigation.

 await heading.click();
-assert.equal(await content.isVisible(), true);
+await content.waitFor({ state: "visible" });

 await link.click();
-assert.equal(new URL(page.url()).hash, href);
-assert.ok(await page.locator(href).evaluate(top < 40));
+await page.waitForURL(url => url.hash === href);
+await waitForScrolledIntoView(page, href);

 await heading.click();
-assert.equal(await heading.evaluate(n => n.closest("details").open), true);
+await openFold(heading).waitFor({ state: "attached" });   // details[open] ancestor

site/tests/pages.test.cjs
  waitUntil(read, message)      # Node-side poll, Playwright's 30s default (DEFAULT_TIMEOUT_MS)
  waitForScrolledIntoView       # scroll position: no locator state expresses it
  openFold(heading)             # the heading's fold, matched only while open
  unfoldGuide(page)             # click each guide h2, wait for it to open
site/tests/README.md            # documents the wait-after-action rule

• No isVisible() remains in the suite. The custom timeout: 2000/5000 on waits are gone, so every wait uses the default.
• Playing a video now waits for its iframe before the next key press or layout measurement. The keyboard test checks scrollY only after the Space key's player appears.
• waitUntil exists because page.waitForFunction never re-checks on a javaScriptEnabled: false page. locator.waitFor does poll there.
• No retries were added. site/package.json is unchanged.

Part of #729.

Closes #1326

Evidence

• Before: the suite on origin/main passed one local run. It relied on snapshot reads after actions, and I did not reproduce a flake locally. Probe of the alternative polling API on a page without JavaScript, where the DOM changes at 500ms:

  page.waitForFunction(...)                  -> Timeout 3000ms exceeded (default and polling: 50)
  locator.waitFor({ state: "visible" })      -> ok after ~940ms
  locator("details[open]").waitFor(detached) -> ok after ~1020ms
  

• After: 10 consecutive local runs of npm test in site/ at c4aaff3:

  run 1..10: rc=0  # pass 50  # fail 0   (61s–80s each)
  

  scripts/affected-tests origin/main passes: npm test --prefix site (50/50), cargo test --test share_card --test site_deploy, cargo test --bin thirdshift prompts_page.

Merge Danger

Door: two-way

Test-only change, plus one README paragraph. A revert restores the old assertions.

Blast Radius: tests

A wait that should fail now takes up to 30s to time out instead of failing at once. It still fails. Shipped site pages and the binary are untouched.

Review

The review used origin/main (09d6d40) as the fixed point, because the local main ref was stale. Both reviewers read both changed files (site/tests/README.md, site/tests/pages.test.cjs), so no changed file went unread. Addressed: Standards 1 (the copied 30s default is now the named DEFAULT_TIMEOUT_MS) and Standards 3 (scrolledIntoView → waitForScrolledIntoView). Neither axis found behaviour that is wrong.

Unaddressed findings

Standards

• 2. Possible Speculative Generality: waitUntil has one caller. Declined. site/tests/README.md names waitUntil as the suite's tool for "a reading no locator expresses", so it is the documented seam for future non-locator waits, not a hypothetical one.
• 3 (second half). openFold(x).waitFor({ state: "detached" }) reads indirectly as "wait for the fold to close". Declined. One locator for the open state, attached or detached, avoids a second helper for its negation, and the openFold comment says it matches "only while it is open".
• 4. Mild Duplicated Code: two fold xpaths, and unfoldGuide/checkSectionToggles both loop over main h2. Declined. The two locators differ on purpose. checkSectionToggles needs the fold whether open or closed, to focus its summary and find its content. openFold must match only an open one. The loops do different things to each heading.
• 5. Lost failure context in the page-link loops (waitForURL without path) and comments replacing messages. Declined as diagnostics only. Locator timeouts name their target, for example locator('#prompts-title').locator('xpath=ancestor::details[1][@open]') from the failure seen during development. A URL timeout's stack line points to the test and the loop.
• 6. click({ timeout: 2000 }) is the last explicit timeout (site/tests/pages.test.cjs:210). Declined. It bounds an action, not a state check. The issue asks for "visibility or state check[s]" to use the default timeout, and the README rule covers waits.

Spec

• Minor deviation: waitUntil/waitForScrolledIntoView are new, not "a polling helper already used in the suite". Declined. The suite had none (git show origin/main:site/tests/pages.test.cjs | rg -n "waitFor|poll"), and the only Playwright option, page.waitForFunction, times out without JavaScript. Both reviewers reproduced that with the probe in Evidence. Scroll position has no locator state.
• Scope creep: the README paragraph. Kept. AGENTS.md sends site-test setup and conventions to site/tests/README.md, and the paragraph records the rule this issue introduces.
• Scope creep: openFold(#prompts-title).waitFor({ state: "detached" }). Kept. It waits for the Prompts click to take effect before checking that Command is still open. Without it, that check would race the click this issue is about.
• Remaining click({ timeout: 2000 }) and the copied 30000. The constant is addressed above. The click timeout is declined as under Standards 6.

Built with claude · claude-opus-5-5 · high

Security review outcome

No unaddressed introduced findings.