Skip to content

The dismissal test waits for the frame, not the popup - #430

Open
serenakeyitan wants to merge 1 commit into
mainfrom
fix/dismissal-test-uiopen-race
Open

The dismissal test waits for the frame, not the popup#430
serenakeyitan wants to merge 1 commit into
mainfrom
fix/dismissal-test-uiopen-race

Conversation

@serenakeyitan

@serenakeyitan serenakeyitan commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Fixes #429. Flagged as an unrelated flake in #428; this is its own issue and its own PR.

dismissal spares the drag and the artifact pill failed about one run in four. It was never the drag or the pill — every failure was the test's own first assertion:

  ✗ dismissal spares the drag and the artifact pill
    precondition: a word click did not open the composer

The race

Closing a card lands in two steps. React unmounts .tdoc-popup at commit; the frame — which is what actually decides whether a click dismisses or acts — hears about it one useEffect and one postMessage later:

// shell/src/document-shell.jsx
useEffect(() => {
  const open = Boolean((openCommentId || openClusterKey || composer) && !reanchorId);
  bridge.send({ type: 'tdoc:uiOpen', open });
}, [...]);

The preceding test ends by clicking .tdoc-popup button.x. This one measures geometry and clicks a word a few milliseconds after that. When the mousedown lands inside the gap, shellUiOpen in server/frame-probe.js is still true, so the frame reads the click as a dismissal — swallowClick = true, mousedown returns early, mouseup posts tdoc:cleared instead of reporting a word selection. No composer, failed precondition.

openCount() only ever watched the shell's half of a two-part state change. Same shape as the Move anchor test, where the banner renders before the frame has been told it is re-anchoring.

The fix

data-tdoc-selecting, data-tdoc-editing and data-tdoc-interaction-mode are already mirrored onto <html> in the frame, and tests already wait on them. shellUiOpen was the one piece of frame state that was not, so nothing could wait for it. Mirror it as data-tdoc-ui-open and gate the test on the frame's own copy:

const frameKnows = (open) => frame.waitForFunction(
  (want) => document.documentElement.hasAttribute('data-tdoc-ui-open') === want, open);

Three gates: before the opening click, after it, and after the close between step 1 and step 2 (replacing a 200ms sleep). No blanket waits added.

A second defect in the same test

Step 2 never clicked the pill. boundingBox() is already in main-frame coordinates for an element inside an iframe; the test added the doc-frame offset a second time. Measured live:

docframe box y=48
pill via frame.locator().boundingBox()          y=252.98   <- already absolute
pill via frame.evaluate(getBoundingClientRect)  y=204.98
click landed at y=316; elementFromPoint there = a plain <p>

48px below the pill, on prose. A click on prose while a card is open also dismisses, so openCount() === 0 passed and the assertion has been vacuous since #403 — the pill path it is named after was never exercised. Dropping the doubled offset puts the click on the pill centre, and the new frameKnows(true) gate is what keeps that real click deterministic.

Testing

Baseline on clean origin/main, 10 runs: 2 failures, both the same assertion.

After, 8 consecutive runs of node test/browser-editing.test.js: 8/8 green, 18 passed 0 failed each.

node test/run.js (the offline merge gate): PASS — all 75 suites green, including dismiss-rule.test.js, whose four source-shape guards still hold.

🤖 Generated with Claude Code

@serenakeyitan @claude
The dismissal test waits for the frame, not the popup
74701fc 
`dismissal spares the drag and the artifact pill` failed about one run in
four, always on its own first assertion: `precondition: a word click did
not open the composer`.

The frame decides whether a click dismisses or acts, from its own
`shellUiOpen` copy of the state — and the shell only sends that in an
effect, after React has already unmounted the card. The preceding test
closes the composer with the X button and this one clicks a word a few
milliseconds later; when that mousedown lands in the gap, the frame still
believes something is open, reads the click as a dismissal, and posts
`tdoc:cleared` instead of a word selection. `openCount()` was watching the
shell's half of a two-part state change.

Mirror `shellUiOpen` onto `<html>` as `data-tdoc-ui-open`, next to
`data-tdoc-selecting`, `data-tdoc-editing` and `data-tdoc-interaction-mode`
which are already there, and gate the test on the frame's own copy instead
of on a timeout.

Also: step 2 never clicked the pill. `boundingBox()` is already in
main-frame coordinates for an element inside an iframe, and the test added
the doc-frame offset to it a second time, putting the click 48px low, on
prose. Prose dismisses too, so the assertion passed without exercising the
path it is named after.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

Preview

Open this: https://pr-430-tdoc-preview.jyshi1107.workers.dev/ — the homepage, this PR's code
carrying the landing version in this PR's checkout.
Or a document: https://pr-430-tdoc-preview.jyshi1107.workers.dev/d/conway-life/v/2

This link is unique to this PR. New commits update the same URL. It is not tdoc.dev.

Preview has no Durable Object — concurrent comments use the KV fallback. Data expires in 14 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yayashuxue yayashuxue Awaiting requested review from yayashuxue yayashuxue is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

The dismissal test races the frame's copy of the open state

1 participant

@serenakeyitan