test(remote-react-components): settle the tree before parking the pointer by mfal · Pull Request #2985 · mittwald/flow

mfal · 2026-08-28T08:44:42Z

What & why

Two visual scenarios diverged between Local and Remote, on firefox (dark
theme) — found while resolving the forward merge in #2981, but not caused by
it: both reproduce on the parent commit of either release line.

Test	`main` @ `83d6086`	`next` @ `b171f12`
`List items` (`List-items-filtered`)	fails, 782 px	fails, 782 px
`List date range filter` (`…-filtered`)	fails, 529 px	passes (timing)

In both, the keyboard focus ring on the toggled filter value is present in the
reference and absent in the output. It went unnoticed because
run-visual-tests is an opt-in label, not a required check.

Root cause

testScreenshot parked the pointer before the Remote tree had applied the
interaction.

setNeutralPointerPosition calls hover(), which emits a pointermove, and
react-aria's useFocusVisible takes any pointer event as the current
interaction modality — but only pointerdown/mousedown notify its
subscribers. So nothing re-renders and elements already carrying
data-focus-visible keep it. The pending remote render then lands, recomputes
isFocusVisible() against the now-pointer modality, and drops the attribute.

Local renders synchronously, before the pointer ever moves, and keeps the ring.
Remote applies an interaction a thread round trip late and renders after it.
Both environments pass the same description to testScreenshot, so they share
one reference file — hence a ~1% diff rather than an obvious failure.

Measured at capture time, same element, Remote:

data-focused="true"   data-focus-visible=<gone>   matches(:focus-visible)=true

The browser still considers the element focus-visible, so this is react-aria's
attribute being recomputed, not native focus state. Reordering confirms it:
settle-then-hover keeps the ring, hover-then-settle loses it.

The fix

Wait for the document to stop mutating before parking the pointer — a
MutationObserver quiet period (100 ms quiet, 2 s cap). That gives Remote the
ordering Local already has: last render, then pointer. It observes the whole
document rather than the container, because overlays render in portals outside
it and the remote tree's own mirror — whose mutations are exactly what has to
settle — lives outside it too.

No baselines were touched. The ring is genuine keyboard state; the reference
was right and the output was wrong.

Second fix: the tooltip scenario (was #2991)

Tooltip had kept the scheduled run red in both environments since 2026-08-27,
for a related reason — folded in here because both fixes touch testScreenshot.

The firefox-linux baseline for Tooltip - visible shows the button with no
tooltip; its three siblings all show one. Tooltip was the only visual
scenario that hovered before capturing, and the pointer park un-hovers the
trigger. react-aria then keeps the tooltip painted for closeDelay — 500ms by
default — so the capture usually still caught it. A race, not a guarantee.

6bf57c7 (fix(CodeBlock): …, #2945) lost that race: update-screenshots runs
the whole suite with --update and then git add -A, so it committed the
tooltip-less frame in a PR that only touched CodeBlock.

The scenario now opens the tooltip through a controlled isOpen instead of
hovering. That removes the race outright rather than working around it: no timer
is involved, and the pointer park is irrelevant. The rendered output is
identical to what the three intact baselines already show, so no baseline
changes in this PR — verified 3× per browser against the committed files.

What it drops is coverage that hovering opens a tooltip. That is behaviour, not
appearance, and a screenshot could only ever assert it by racing the 500ms delay
— which is how the wrong frame got committed. It belongs in a browser test.

Tooltip-visible-firefox-linux.png stays wrong; it only affects the firefox
visual run, which is not a required check, and it is now the single baseline that
needs regenerating rather than four. #2995 makes that sweep safe to run.

Two rows in the root AGENTS.md Common failures table come with it: the unscoped
update-screenshots sweep, and vitest run --update <file>, where --update
takes an optional value and swallows the positional filter, so the run updates
every baseline and only the test count (354, not 2) gives it away.

Also: a guard in the required job

ScreenshotPreamble.browser.test.tsx asserts the ordering rule itself: a DOM
mutation scheduled inside the settle wait's quiet window must have landed by the
time prepareForScreenshot returns. It fails without the settle wait and passes
with it, on both browsers, with no keyboard navigation, focus, menus or anything
else platform-dependent.

It asserts the rule rather than a scenario on purpose. A scenario only
reproduces this bug while its round trip happens to outlast the pointer park,
and that margin is incidental — see the second commit: the first version of
this guard tabbed from the sorting menu to the filter button, which CI rejected
because WebKit on Linux does not move focus there the way macOS does; removing
the tab then made the round trip land early enough that the test passed with the
fix reverted. Three variants behaved that way. A test that silently guards
nothing is worse than no test.

The focus ring in the real output stays covered by the visual suite, which is
what caught this in the first place.

Verification

Check	Result
`List.browser.test.tsx`, firefox, before	1 failed / 17 passed — `List items (Remote)`, 782 px
`List.browser.test.tsx`, firefox, after	18 passed
9 keyboard-heavy visual files, firefox / webkit	54 passed / 44 passed
New guard, fix disabled	fails on webkit and firefox
New guard, fix enabled	passes 3/3 on webkit and firefox
Whole `browser*` project, both browsers	10 passed
`Tooltip` visual vs. untouched baselines, firefox / webkit	3/3 passed each
`Tooltip` + `List` visual, firefox / webkit	20 passed / 20 passed
`test:compile`, eslint, prettier	clean

For the reviewer

Title is test:, not fix:. The change is entirely test-harness code, so
a consumer-facing fix: changelog entry would be misleading. It routes to
main either way, so it still forward-merges into next as intended.
run-visual-tests is on this PR. The change alters when every screenshot
is taken, and local runs only ever compare -darwin. If any Linux baseline was
itself captured mid-update, this is where it shows.
run-visual-tests stays red on one baseline. The required main check runs
the visual suite too, but only on webkit (--browser.name=webkit) — which is
exactly why both bugs here went unnoticed: both were firefox-only. main
should now pass, since no baseline changes. The firefox run still fails on
Tooltip-visible-firefox-linux.png, the frame fix(CodeBlock): balance the padding and unify the editor line colors #2945 committed with no tooltip
in it. Regenerate it once ci: commit only baselines that reproduce, and only baselines #2995 is in, or merge as is — that check is not
required.
Separate defect, left open. The run also emits an intermittent unhandled
TypeError: listener is not a function
(props-for-element.mjs → useControlledHostValueProps.ts:34). It is not
the cause here: running List items alone reproduces the 782 px diff with no
such error, and the stack shows onChangeFromProps is a function
(eventListenerCallbackWrapper), so the optional-call guard did its job.

The lead: usePropsForRemoteElement's "all listeners allowed" branch wraps
every entry of eventListeners with no truthiness check, unlike the
eventProps branch which guards if (listener). Since
RemoteReceiver.updateProperty assigns updateObject[property] = value, a
removed listener leaves the key with undefined, Object.entries still yields
it, and the resulting callable wrapper throws exactly that message when
invoked. Unproven — a probe on the mutation stream did not fire in the runs
where the error also did not reproduce. A fix would belong in the fork
(mfal/remote-dom, publish/mittwald) or a patches/ entry, not in this PR.

Checklist

PR title is a Conventional Commit and matches the base branch above
pnpm lint is clean (eslint, stylelint, prettier on the changed files);
browser tests pass in both browsers
Generated code is committed — this change touches no generated artifact
User-facing strings added to both de-DE and en-US — n/a, no UI text
Docs updated if a public API changed — n/a; no snapshot updates, the
references were already correct

🤖 Generated with Claude Code

…nter Two visual scenarios diverged between `Local` and `Remote` — `List items` (`List-items-filtered`, 782 px) and `List date range filter` (529 px), both on firefox, both a keyboard focus ring present in the reference and absent in the output. Neither was caused by the sync merge in #2981; both reproduce on the parent commits of either release line. `testScreenshot` parked the pointer before the `Remote` tree had applied the interaction. `setNeutralPointerPosition` calls `hover()`, which emits a `pointermove`, and react-aria's `useFocusVisible` takes any pointer event as the current interaction modality — but only `pointerdown`/`mousedown` notify its subscribers. So nothing re-renders and elements already carrying `data-focus-visible` keep it. The pending remote render then lands, recomputes `isFocusVisible()` against the now-`pointer` modality, and drops the attribute. `Local` renders synchronously, before the pointer ever moves, and keeps the ring. `Remote` applies an interaction a thread round trip late and renders after it. Both environments pass the same description to `testScreenshot`, so they share one reference file and the divergence read as a ~1% pixel diff. Wait for the document to stop mutating before parking the pointer, which gives `Remote` the ordering `Local` already has: last render, then pointer. The whole document, not the container — overlays render in portals outside it, and the remote tree's own mirror, whose mutations are exactly what has to settle, lives outside it too. The ring is genuine keyboard state, so no baseline changes: all 18 List scenarios pass against the committed references on both browsers and themes. Also add `KeyboardFocusRing.browser.test.tsx`, which asserts the same thing on the DOM instead of on pixels. This shipped unnoticed because `run-visual-tests` is an opt-in label, and it only ever showed in the one browser that renders the dark theme; the guard runs in the required browser job and names the cause when it fails. It needs the full `List items` tree — a smaller list settles faster than the pointer park and cannot catch the race. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

github-actions · 2026-08-28T08:45:46Z

Coverage Report for ./packages/components/

Status	Category	Percentage	Covered / Total
🔵	Lines	76%	627 / 825
🔵	Statements	75.91%	643 / 847
🔵	Functions	77.95%	145 / 186
🔵	Branches	66.66%	298 / 447

File Coverage

No changed files found.

Generated in workflow #6401 for commit 0538857 by the Vitest Coverage Report Action

github-actions · 2026-08-28T08:53:00Z

🚀 Preview Deployment

Preview environments are ready:

Type	URL
docs	pr-2985.docs.review.flow-components.de
storybook	pr-2985.storybook.review.flow-components.de

Images:

docs: ghcr.io/mittwald/flow/docs:pr-2985
storybook: ghcr.io/mittwald/flow/storybook:pr-2985

…enario The `KeyboardFocusRing` guard added in the previous commit was fragile in two ways, and CI caught the first: it tabbed from the sorting menu to the filter button, which does not move focus the same way in WebKit on Linux as it does on macOS, so the menu never opened and both environments failed on a missing locator. Removing the tab exposed the second, worse problem. A scenario only reproduces this bug while its round trip happens to outlast the pointer park, and that margin is incidental: opening the menu by clicking its own trigger made the round trip land early enough that the test passed with the fix reverted. Three variants behaved that way — a test that silently guards nothing is worse than no test. So assert the ordering rule directly instead. A pending DOM mutation, scheduled inside the settle wait's quiet window, must have landed by the time `prepareForScreenshot` returns. That fails without the settle wait and passes with it, deterministically, on both browsers — no keyboard navigation, no focus, no menus, nothing platform-dependent. It also documents the bound honestly: the quiet window only extends for mutations arriving inside it, so it covers work continuously in flight, not a render scheduled after an arbitrary idle gap. The focus ring in the real output stays covered by the visual suite, which is what caught this in the first place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

github-actions · 2026-08-28T09:03:14Z

❌ Visual Regression Tests Failed

The visual test step did not pass. If snapshots differ from the committed baselines, download the visual-diffs artifact from this run to inspect the actual/diff images (the artifact is absent if the run failed before comparison, e.g. during install or browser setup).

If the differences are intentional, update the baselines by adding the update-screenshots label to the PR.

Run details

…while capturing The `Tooltip` scenario has kept the scheduled visual run red in both environments since 2026-08-27, and it is not a rendering regression: the `firefox-linux` baseline for `Tooltip - visible` shows the button with no tooltip. The three baselines beside it all show one. It is the only visual scenario that hovers before capturing, and `testScreenshot` then parks the pointer, un-hovering the trigger. react-aria keeps a tooltip painted for `closeDelay` — 500ms by default — so the capture normally still catches it. That is a race, not a guarantee: on a slow runner the frame comes out tooltip-less. The scenario passed by outrunning a timer it does not control. because the `update-screenshots` label runs the whole suite with `--update` and then `git add -A`, it also committed the tooltip-less frame as this baseline — one stray file in an otherwise unrelated PR. Local and Remote share a baseline per description, which is why one bad file fails both. So stop parking the pointer for a scenario whose subject is a hover state. `keepPointerPosition` leaves it where the scenario put it, and a tooltip held open by a pointer that never leaves has no closing window at all. The trigger is then captured hovered, which is what a user actually sees with a tooltip open, so all four baselines change; the two `-darwin` ones are regenerated here. Also document the two traps behind this in the root AGENTS.md: the unscoped `update-screenshots` sweep, and `vitest run --update <file>` — where `--update` takes an optional value and swallows the positional filter, so the run updates every baseline and only the test count (354, not 2) gives it away. Both cost real time while diagnosing this, and the second nearly re-committed the very frames #2985 fixes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

`visual-diffs` has never existed. `.vitest-attachments` is a dot directory, and `actions/upload-artifact` skips hidden paths unless told otherwise, so every run logged "No files were found with the provided path" and uploaded nothing — including runs whose failure was a plain pixel mismatch with reference, actual and diff images sitting right there on disk. The failure comment points reviewers at that artifact anyway, and vitest truncates the step log before its own failure summary, so a red visual check offered no reference image, no diff, and no list of which scenarios failed. Diagnosing #2985 meant reading a snapshot's byte size and its git history to work out what had changed. `include-hidden-files: true` is the whole fix. Note the scheduled visual workflow uploads nothing at all — its failures stay just as opaque. Left alone here; it needs the upload step added, not a flag. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

`--update` writes every baseline it can and *then* the suite still fails on anything a rewritten snapshot cannot fix — a scenario that never captures a stable frame, most often. The job stopped at that step, so one unstable scenario anywhere discarded the baselines already written for every other one, and the label looked like it had done nothing at all. That is not hypothetical: it is what the sweep on #2985 did. It ran the full suite, wrote the two `-linux` tooltip baselines the PR needs, then exited 1 and committed nothing, leaving a required check red with no way to fix it from the PR. So the update run is allowed to be red, and the verification added in the previous commit becomes the actual gate: a baseline is committed if re-running its scenario reproduces it, whatever else the suite did. A scenario that fails for reasons a snapshot cannot fix no longer blocks the ones that do. The summary distinguishes the cases, because "red and wrote nothing" must not read as "everything was already correct". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Replaces the `keepPointerPosition` approach from the previous commit, which fixed the race but cost four new baselines — and the two `-linux` ones can only be produced by CI, which left `main` red with no way to fix it from the PR: the sweep that would write them discards everything it wrote if any scenario anywhere is unstable. Opening the tooltip through a controlled `isOpen` removes the race outright instead of working around it. There is no timer involved at all: the tooltip is open because the test says so, and the pointer park is irrelevant. The rendered output is what the three intact baselines already show — trigger in its resting state, tooltip below it — so they stay valid, verified 3× per browser against the committed files. `keepPointerPosition` goes with it; nothing needs it any more. What this drops is coverage that hovering *opens* a tooltip. That is behaviour, not appearance, and a screenshot could only ever assert it by racing react-aria's 500ms `closeDelay` — which is how the wrong frame got committed in the first place. It belongs in a browser test. `Tooltip-visible-firefox-linux.png` stays wrong: it is the frame #2945 captured with no tooltip in it. It only affects the firefox visual run, which is not a required check, and it is now the single baseline that needs regenerating rather than four. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

mfal requested a review from a team August 28, 2026 08:44