feat(flow): add an idle condition that waits for the screen to stop moving - #728
feat(flow): add an idle condition that waits for the screen to stop moving#728hubgan wants to merge 24 commits into
Conversation
1fef028 to
9779bd4
Compare
a71b63d to
88670cc
Compare
|
I think this fills the same role as #526, no? In which case, it would be good to settle on one approach. |
@j-piasecki Yeah there's an overlap. Both PR's try to fix the "has the screen stopped moving?" question. Especially for iOS devices. And the #526 is the better approach in my opinion. Additionally this and #727 answer the "Which screen is this?" question. I wonder if that's something worth keeping or if we should close both #727 and #728 PR's. |
|
I'm actually not sure about #526 - on constantly animating screens, it effectively adds 5 seconds of delay to every step. An explicit, surgical step to await idle may make more sense. Screen fingerprinting is also nice, but I don't like the fact that it only works for React Native apps in debug mode. E2E tests are usually run in the release variant. |
88670cc to
e1d9a9a
Compare
1f1ca53 to
5f9507b
Compare
e1d9a9a to
e680fb0
Compare
40fd0cb to
96410fe
Compare
e680fb0 to
151b8f6
Compare
c6fbf40 to
fec70d0
Compare
…oving
`await: { idle: true }` is the one check a selector condition cannot express:
has the screen stopped moving? Flows were substituting fixed `wait:` steps for
it, which either wait too long on every run or too little on the run that
mattered.
It waits on BOTH the UI tree and the rendered pixels, because each is blind to
what the other sees. The tree cannot see presentation-layer motion: an iOS push
or modal dismissal commits its hierarchy up front and then animates a layer for
a few hundred milliseconds, and a cross-fade or scrim moves no node at all — so
a tree-only settle reports a screen that is still sliding and the next tap lands
on a moving target. Measured on a real Android emulator: a page animating a
large element continuously reads as a single static `WebView` node, and the
existing `await-screen-idle` tool called it settled in 465ms, five times out of
five. Pixels alone would not do either — they cannot see a tree still churning
behind an unchanged surface, and anything animated forever (a video, a shimmer)
would make a pixel-only settle unsatisfiable on a screen the tree calls ready.
Unlike that tool, this FAILS on timeout, which is what makes it safe to persist
in a flow: a soft `settled: false` cannot carry a regression verdict on an
unattended replay.
Everything about the design follows from one rule — the absence of evidence is
never evidence. A capture that did not arrive cannot complete the hold. A single
agreeing pair of captures cannot either: every animation that reverses has a
turning point, and two samples straddling it come back identical while the
screen is moving, which on a live 3s cross-fade passed a default-shaped step on
roughly one run in three. Settling takes two consecutive still intervals, so
`minStableMs: 0` still means three reads. A round is never started without the
budget to observe it with, since a round begun with nothing left neither
captures nor reads, and both absences would otherwise be recorded as facts about
the device. And no verdict comes from a latch: a screen that settles and then
moves again, or goes blank, has not settled.
`timeout:` is a real bound. No describe path takes an abort signal, so the tree
read is raced against what is left of the budget — without that, a wedged
ViewInspector RPC ran 2.25s past an 8000ms budget. A read that runs out of step
budget is the step ending, not the source failing, and only the latter is
reported as an environment problem.
The failure modes stay apart because they call for opposite responses: a screen
that never stopped moving is a verdict about the app; a tree that could not be
read is not, and it names the foreground check first, because a backgrounded app
reads identically to an uninstrumented one and relaunching is the wrong repair.
A run whose captures never produced a comparable pair still settles on the tree
and says so in a warning, rather than passing off half a proof as the whole one.
The pixel comparison owns its tolerance rather than borrowing screenshot-diff's.
That one holds a baseline stored across sessions, machines and OS versions
against a live capture and must absorb real drift; this one holds two captures
one poll apart from a single session, where a static screen reads back
byte-identical (measured: zero changed pixels across five consecutive pairs on
an idle simulator). The margin is load-bearing — uniform change is
all-or-nothing, so at the baseline tolerance any cross-fade slower than ~2s
counted zero changed pixels and read as settled mid-animation.
Captures route exactly as the `screenshot` tool routes them, so tvOS and Vega —
which have no simulator-server backend but are perfectly screenshottable through
`xcrun` and the emulator console — are covered rather than written off.
It is deliberately NOT a screen check: a dropped tap leaves the source screen
perfectly idle. It belongs after the element check that names the destination,
never instead of one.
Readiness is not an acceptance criterion. A screen that never stops moving is usually the app working as built — a video, a shimmer, a carousel — so failing the run turns a healthy app into a red test, and the flow's verdict belongs to the identity and outcome checks around the gate rather than to the gate itself. Measured on real devices, the two signals are also not equally sensitive across platforms. The Android tree carries live text and live bounds: a running stopwatch moved its fingerprint on every read, and a fling moved 41 lines with genuine mid-flight positions. The iOS tree saw none of the equivalent motion — a Bluesky feed scrolled for 2.9s with the tree byte-identical across 41 consecutive intervals while the pixels moved in every one. Hard-failing on a signal that different per platform gives one flow file two verdicts: a ticking timestamp or a relative "5s ago" fails a gate on Android that iOS cannot even observe. Both an autoplaying video on iOS and a running stopwatch on Android hard-failed a default-shaped step before this change. So the timeout now reports a warning on a passing step, and the step keeps the part that was always the point: it returns the moment the screen is still, so the following tap resolves its target against a screen that has stopped instead of racing a transition still in flight. That is also why it stays worth placing after every navigation. The warning names what to look at rather than only reporting that the wait gave up, because the benign reading and the bug look identical from here: a stuck spinner is a screen that never finished loading, not a screen that is animating by design. An unreadable window is untouched — it remains `indeterminate`, scored `error`, because the check could not run at all, which is not a verdict about the app and is now idle's only non-passing outcome.
An agent reads the step report, where the outcome is a `warning` on a passing step; the "⚠" is only how the MCP client and the CLI render that field. Naming the glyph pointed the reader at the presentation instead of the thing it can actually check.
78c898b to
5f376c3
Compare
The readiness check compared changed pixels against 0.2% of the whole screen, which no spinner reaches: a stock one covers 0.03-0.15% of a phone display, and it does not move the UI tree either, since it spins in a layer whose box never changes. Both halves of the check therefore agreed a still-loading screen was at rest and passed it clean, with none of the warning both doc surfaces send the author to read. Split the comparison in two. Above the motion fraction the screen is moving, as before. Below it but above a noise floor the change is localized — too small to be the screen moving, too large to be a capture that is not bit-exact — and the settle still completes, because holding a flow for the full timeout over a caret is worse than saying so, but it now says so. Measured on a Chromium app pointed at a page whose only motion is a 40px CSS spinner: 3 runs of 3 warn where 3 of 3 passed silently before. A static page stays clean 4 runs of 4, and a full-screen pulse still reports that the screen never held still.
A tree source that fails has a dedicated hard error. One that HANGS is a different outcome internally, and once any earlier read had succeeded it was never routed there: the step passed and told the author the screen never stopped moving, listing a video, a carousel and live-updating text, on a screen frozen solid. The two are told apart by how much budget the abandoned read had. The last read of a step routinely times out with a couple of hundred milliseconds to its name, and that is the step ending; one abandoned with seconds in hand is a source that wedged. The read still gets the whole remaining budget, since a tree read on a busy screen genuinely takes seconds. Verified against a Chromium app whose renderer enters an infinite loop 400ms into the idle step: 2 runs of 2 now report the source stopped answering, where 4 of 4 used to pass blaming animation.
A settle costs 600ms before any hold is counted: three reads spanning two 200ms polls, plus the budget the closing round needs to start with. Under that, no screen can produce a clean settle however still it is, and the step reported one of two claims about an app that had done nothing — that it never stopped moving, or that it could not be screenshotted. Which one it picked depended on where the budget ran out, so one file gave different verdicts run to run. The parser had a guard for exactly this, but it compared `minStableMs` against `timeout` and only when `minStableMs` was written out — so `timeout: 100` was accepted while the identical `timeout: 100, minStableMs: 250` was rejected. It now checks the effective hold plus what the settle itself costs, and the poll cadence moved next to the defaults it is computed from. Slow sources can still leave a legal step short of reads, so the runner says that too, instead of blaming the app for a screen it barely looked at. Verified on a Chromium app: `timeout: 400`, which warned "never held still" 4 runs of 4 on a completely static page, is now refused at parse with the number to raise it to.
An `idle` step on a screen that rendered no tree content returned an error, which set the run not-ok and skipped every step after it — including the element check that would have named what was actually wrong. The tree read back fine; it was simply empty, which is an observation about the app and not a window that could not be read, and readiness is never this step's to fail a run over. It is also not always a fault: a canvas, a video surface or a splash image renders no accessible content by design. It now warns and goes on, saying which of the two it might be. Verified on a Chromium app pointed at a page with no accessible content: the step passes with the warning and the following step runs, 2 runs of 2, where both used to report `errored: 1` and skip it.
`assert: { idle: true }` gets a tailored explanation of why there is no
assert form. The guard spelling of the same mistake got the generic
"when needs exactly one condition key (exists, visible, hidden, text,
platform)", which never mentions the key the author actually wrote and
leaves them to infer that it is missing from the list on purpose.
It now says the same thing the assert form does: stillness is a wait, a
guard asks what is on the screen now, put `await: { idle: true }` before the
block.
`- idle: true` at the top level of a step returned "unrecognized step kind" with no hint, and the near-miss hint could not find one either — `idle` is not a step kind to be close to. It is also the one near-miss the docs actively produce: every other condition is written with a selector beside it, so `await:` comes along for free, while this one reads like a directive. Say what it should have been.
`timeout: 0.5` was turned into the upper bound for the hold, which asked the author for "an integer between 0 and -0.5" — a range no value satisfies. The two are now checked against each other as a sum, which has an answer for any timeout; this holds that, and the impossible range, in place.
A capture that went missing was also stored as the previous frame, so the round after it had nothing to compare against either — one slow capture cost the settle two intervals, and a backend that was merely intermittently slow ended up reported as one where no screenshot could be read at all. Keep the last frame that actually arrived and compare across the gap instead: the same question, asked over a longer interval. The degraded warning no longer overclaims either — it says the screen could not be screenshotted on enough polls to compare a pair, which is what it knows. Pinned by count rather than wording: with one capture missing out of a still screen's run, the settle now takes five captures where it took six.
The settle asks every capture route for a quarter-scale frame. Chromium's answered through the screenshot path that resizes with `sharp` — an optional dependency nothing in this repo installs — so it silently returned the full-resolution PNG, and a settle decoded a 1800x1226 image into an 8.8MB buffer twice a second on the shared tool-server's only thread. `Page.captureScreenshot`'s own `clip.scale` is applied while rasterizing, so the small frame is the only one that ever exists. Measured on a 900x613 viewport at dpr 2: 1800x1226 and 21-43ms of blocking decode per poll became 450x307 and 2-3ms. It also drops the temp file this route wrote and deleted on every poll. The behaviours the smaller frame has to preserve were re-run against the live app: a static page settles clean, a spinner still reports its localized motion, and a full-screen pulse still reports that the screen never held still.
The suite passed with MIN_STILL_INTERVALS raised to 3 and with the hold clock replaced by `>= 0` — the two constants the step's design argues hardest for were pinned from one side or not at all. Both mutations now fail. Also covered: the default 250ms hold, where the number is actually used rather than only in a parser message; the branch where no tree read ever succeeds, which produces the foreground advice from a standing start; the hold restarting after a failed read and after the screen goes blank; and abort, which had no test anywhere. The capture stub now carries the same three inputs the real one takes — it was dropping the abort signal and the first-capture flag, so neither was exercised inside the loop.
The commit that turned the idle timeout from a failure into a warning updated the skill, the section comments and the tests, and missed the `flow-execute` tool description — the surface an authoring agent actually reads — which still said it FAILS on timeout, with "so it is safe to persist" hung off the claim that was now backwards. It is safe to persist BECAUSE it cannot fail. A test pins both descriptions to that. Also corrected: - the skill now lists all four warnings a passing step can carry, rather than sending the author to look for one they may never get, and says what actually stops a run: a tree source that cannot be read, not merely an empty tree. - it documents that there is no `assert` or `when:` form, that the recorder cannot emit one, and — restoring a line the fail-to-warn commit deleted — that sprinkling it after every step buys nothing now that it cannot fail. - the timeout floor the parser enforces. - MIN_ROUND_BUDGET_MS described the guarantee it does not make: the check runs before the poll sleep, so it is a floor on the wait, not on the round. - both renderers described `warning` as legacy wire-compat for old tool-servers, which now reads as an invitation to delete the field this feature reports through.
Verified on real devices at the capture scale the check uses: a static iOS screen changes 0 pixels between two captures, a live spinner on an iPhone 16 Pro changes 50 of 198k, and a blinking caret on a Pixel 7 changes 50 of 162k. The floor sits an order of magnitude below the movers and clear of the noise, and the residual limit — an indicator smaller than that — is now stated rather than left to be discovered.
…e top
`Page.captureScreenshot`'s `clip` is measured from the top of the document, so
the fixed `{ x: 0, y: 0 }` origin pointed at the top of the page rather than at
the window. On a scrolled document that rectangle is off-screen and Chrome
rasterizes it blank; two blank captures compare as identical, so the pixel half
of the settle voted "still" on every interval of a visibly animating screen —
and did it silently, because the capture succeeded and the tree-only warning
could not fire. Measured on a scrolled page carrying a full-window
black-to-red cross-fade: 5 clean passes out of 5.
Take the origin from `Page.getLayoutMetrics`, a browser-side layout read that
does not need the live main world the cached viewport's `Runtime.evaluate`
does, so it survives the mid-navigation renderer this check runs against. Same
page, same scroll offset: motion detected 5 runs of 5, while an unscrolled page
and one scrolled to a static band both still settle cleanly 3/3, and a page
scrolling inside an inner element is unaffected 3/3.
The Chromium capture test asserted the clip arguments against a mock with no
concept of scroll, which pinned the buggy shape. It now answers the way the
compositor does — content for a rectangle inside the rendered window, blank
white for one outside it — so a wrong origin fails on the colour it returns.
This was the one comparison in the repo that looked at system chrome: `screenshot-diff` ignores the top 6% of the frame, the `snapshot` step opts into that, and the Android flow tree strips com.android.systemui outright, while the settle's comparator masked nothing and leaned entirely on the run-level `pinStatusBar`. That pin has two holes. It lands AFTER the run starts — the simulator repaints the clock and animates the battery fill 100-450ms in — so a fragment beginning with this step compares a real clock against a pinned one; and a nested `tool: flow-execute` clears the pin on its way out without the outer run re-pinning, leaving every later step comparing against a ticking clock. Either way a static, fully-loaded screen was reported as moving, or as carrying "a spinner ... the screen had not finished loading", which is the step's entire product. Mask the same 6% band, on the two platforms that have one: a Chromium window's top band is page content, and Vega and tvOS render with no system chrome, so those keep the whole frame. The masked rows leave the denominator too, so the motion and localized fractions still mean what they say. The two frames the runner was caught comparing on a static iPhone 16 Pro are now cases: 408 changed pixels at y[19..29] (over the 396-pixel motion budget) and the 13-pixel tail at y[19..25] that became the spinner warning. Both read as motion unmasked and still masked, while the same changes one row below the band are still seen. Verified on a live Chromium target that a window whose top 4% is the only thing animating is still detected as moving, 3 runs of 3.
The tvOS arm passed no abort signal to `tvScreenshot`, which forwards it straight to `execFileAsync`. A wedged `xcrun simctl io screenshot` was therefore never killed: the round abandoned the promise, but nothing abandoned the process, and the next poll 200ms later spawned another — one stuck capture becoming a growing pile of subprocesses for the rest of the step. The `screenshot` tool's own tvOS route has always bounded it. Thread the capture's own budget down to the shell-out and combine it with the run's signal, so the subprocess dies with the round that stopped waiting for it. The simulator-server arm deliberately keeps its no-signal ownership (it learns the temp path only from the reply); this route's path is deterministic and internal, and an unbounded process is the worse trade.
A directory run's step renderer skipped everything that was not `fail`/`error`
before reaching the line that prints a warning, while the summary counts
warnings whatever the status. `await: { idle: true }` only ever warns on a step
that PASSED — so `argent flow run <dir>` reported "1 warning" with the text
nowhere on screen, and the warning is the whole of what the step produces.
Single-flow runs and the MCP surface both printed it correctly.
Verified end to end with the CLI against a live Chromium target: a directory
holding a warning flow and a clean one now prints the idle warning under its
step and still counts it in the summary.
The "too few reads to judge anything" guard counted every read that ANSWERED,
including the blank ones — but a blank tree resets both holds and measures no
interval, so it is not one of the three reads a settle takes. A window that was
blank for most of its life therefore sailed past the guard and reached the
motion verdict instead, telling the author that "the screen never held still
... something on it never stops" on the strength of a single measured interval.
Count content-bearing reads for that guard, leaving the reads-that-answered
count to the unreadable-source check above it, which is the question it
actually asks. The warning now says how many reads came back with content,
which is the number the advice ("raise its `timeout:`") acts on.
`assert: { idle: true, visible: X }` reported the mixing error first, and doing
what it said — splitting the two conditions — produced `assert: { idle: true }`,
which is not valid either: idle has no assert form. The assert arm of the
mixing check could never yield a body that subsequently parsed.
Let the assert body go straight to the error that ends the matter, and have
that error name where the other condition belongs, so one message describes the
whole repair. The `await` mixing error is unchanged, and the fix is pinned by
parsing the exact spelling the new message tells the author to write.
The spinner/caret warning's floor was a fraction of frame area, so it grew with the window: 0.005% is ~10 pixels on a phone frame but ~46 on a desktop-sized Chromium one — above every indicator ever measured for it, which turned the warning silently off on exactly the largest windows. Both numbers the comment cited to justify the fraction were phone-sized. What a spinner and a caret have in common is a size in captured pixels, not a share of the frame: 50-66 pixels for a spinner on an iPhone 16 Pro, 57 on a Pixel 5, 50 for a caret on a Pixel 7 — within a factor of two across three frame sizes. Make the floor that count. A static capture pair changed 0 pixels of 237k, so ten still clears the noise it exists for. MOTION_FRACTION stays a fraction: a transition really does move a share of the screen.
Six descriptions of the same check disagreed with it or with each other. - The skill listed four warnings; the implementation raises five. The too-few-reads one is now listed, so the skill and the code comment that enumerates them describe the same set. - The skill said only a source that fails outright or wedges stops the run. There is a third path — one that never answers within the step — whose advice is the opposite: it may merely be slow, so raise the `timeout`. Named, along with what an `error` costs (the run is not ok, later steps skipped). - "the 600ms a settle costs — three reads spanning two 200ms polls" adds to 400. The missing term is the budget the closing round needs to be allowed to start; the skill and the parser error both say so now, and a test pins the boundary the arithmetic claims against the one the parser enforces. - The `flow-execute` description — the surface an authoring agent reads — promised idle "never fails a run" without mentioning that an unreadable tree scores `error` and skips every remaining step. The skill carried the caveat; now both do. - The localized-motion warning said something "kept changing the whole time" while the flag is set by any one interval of the winning hold. It now claims what the flag means: something small was moving while the screen settled. - The spinner's size was stated four different ways. It is the measured one, in the units the floor is now expressed in. The docs test that only phrase-matched two documents now binds the numbers to the parser's own constants and checks the smallest timeout the documented arithmetic allows against the smallest the parser accepts.
- The capture-noise case asserted only `r.ok`, which is true for every idle outcome but an unreadable tree. What it is about is that a +1 drift settles CLEANLY, so it now asserts no warning at all — neither motion nor spinner. - The localized-motion reset when a hold breaks: a spinner that STOPS, then a screen that goes still, must settle without being told something was moving while it did. Deleting the reset now fails. - The `minStableMs` term of the tree-only hold: every case that reached it ran with `minStableMs: 0`, where the term is vacuous. A tree that had only just stopped moving must not be reported as having settled on the hierarchy. - The reads a blank window does not buy, the `idle` step's absent report target, the ten-minute ceiling on `minStableMs`, Vega and tvOS in the warm-capture decision, a Chromium capture that answers with no data, a frame with no pixels to compare, a truncated capture buffer, and the recorder summary for a hand-written idle step — each was deletable with the suite still green. Every case here was checked by making the change it guards against and watching it fail.
One check a selector cannot express
Has the screen stopped moving? Flows were substituting fixed
wait:steps for it, which either wait too long on every run or too little on the run that mattered.Why it reads pixels as well as the tree
Each signal is blind to what the other sees.
The tree cannot see presentation-layer motion. An iOS push or modal dismissal commits its hierarchy up front and then animates a layer for a few hundred milliseconds; a cross-fade or a scrim moves no node at all. Measured on a real Android emulator: a page animating a large element continuously reads as a single static
WebViewnode, and the existingawait-screen-idletool called it settled in 465 ms, five times out of five. On a deterministic Chromium page whose DOM is byte-identical across the animation, that tool returnedsettled: truein 407 ms while this condition failed 8/8.Pixels alone would not do either: they cannot see a tree still churning behind an unchanged surface, and anything animated forever (a video, a shimmer) would make a pixel-only settle unsatisfiable on a screen the tree calls ready.
Unlike that tool, this cannot be silently stepped over. Readiness is not an acceptance criterion — the verdict belongs to the identity and outcome checks around it — so a screen that never settles still passes, carrying a
warningthat says what the green actually bought. That is what makes it safe to persist in a flow: it reports on every run without ever failing one over a video or a shimmer. The only thing that stops a run is a tree source that cannot be read.The rule the design follows
The absence of evidence is never evidence.
minStableMs: 0still means three reads.timeout:is a real boundNo describe path takes an abort signal, so the tree read is raced against what is left of the budget. Without that, a wedged ViewInspector RPC ran 2.25 s past an 8000 ms budget, and a busy Android screen ran 5.4-6.0 s against a 3000 ms one. A read that runs out of step budget is the step ending, not the source failing, and only the latter is reported as an environment problem.
Failure modes stay apart
They call for opposite responses. A screen that never stopped moving is a verdict about the app. A tree that could not be read is not, and it names the foreground check first, because a backgrounded app reads identically to an uninstrumented one and relaunching is the wrong repair. A run whose captures never produced a comparable pair still settles on the tree and says so in a warning, rather than passing off half a proof as the whole one.
Capture routing and tolerance
Captures route exactly as the
screenshottool routes them, so tvOS and Vega, which have no simulator-server backend but are perfectly screenshottable throughxcrunand the emulator console, are covered rather than written off.The pixel comparison owns its tolerance rather than borrowing
screenshot-diff's. That one holds a baseline stored across sessions, machines and OS versions against a live capture and must absorb real drift; this one holds two captures one poll apart from a single session, where a static screen reads back byte-identical (measured: zero changed pixels across five consecutive pairs on an idle simulator). The margin is load-bearing, because uniform change is all-or-nothing: at the baseline tolerance any cross-fade slower than ~2 s counted zero changed pixels and read as settled mid-animation.Not a screen check
A dropped tap leaves the source screen perfectly idle. It belongs after the element check that names the destination, never instead of one. #730 documents the pairing.
Verification
typecheck:tests, lint and prettier clean. Eleven mutations applied to the source, all eleven caught by tests.