fix(scroll): answer with the movement a directional scroll observed - #2771
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
Reviewed at 8a8bd46. The code is not ready, and the live validation is still missing.
This changes the device-facing directional scroll path on iOS and Android: it adds a post-gesture capture and a new Is the scope proportional to the problem? The PR adds a module, a new dispatch seam in CI was still queued or running at review time. Smoke Tests assert live |
8a8bd46 to
b4e88b6
Compare
|
Reviewed at b4e88b6, as a follow-up to the 8a8bd46 review. The main defect from that review is still there; nothing in b4e88b6 touches it.
The PR changes the device-facing directional scroll path on both iOS and Android, adding a post-gesture capture and the new Carried over from the 8a8bd46 review, still open: The Android smoke job failed at step 47, Next step: the comparability gate needs to run before every movement verdict, with a drifted-and-changed test proving it; then the live iOS and Android runs above need to show the numbers they're supposed to show, and CI needs a clean rerun on this commit. |
b4e88b6 to
ec02962
Compare
|
Both findings were right; the first was a real bug. Pushed as 1. Lineage gate ran only on the quiet path — fixed. 2. The dispatch seam — deleted, and you were right. 3. Scope / "one definition of moved". The movement claim is the daemon's existing definition — it calls 4. Live run — done, and it found something. iPhone Duo (iOS 27.1), tester @ But The refusal is additionally unproven on-device in another way: On size: the report still reads +5.0 kB unpacked; the seam removal is most of the reduction available, the rest is the module. |
|
Reviewed ec02962. This is still the only layer in the stack that changes a device answer: it adds a post-gesture capture and a new The two live runs from the earlier review are still missing. The Android emulator failed with For Android: build with One question: a chained CI on ec02962 is cancelled or queued after the force-push, with no failed logs. This diff changes the scroll steps in Smoke Tests, so we still need a finished run on this commit. Next step: the two device runs above, then green CI. |
ec02962 to
cc4d6b9
Compare
cc4d6b9 to
7be7922
Compare
7be7922 to
e533710
Compare
e533710 to
55797fc
Compare
|
The rename since ec02962 is the only change on this delta; the Android evidence gap from that review is still open. This is a follow-up on the earlier review. The direction path adds a post-gesture capture and a new scroll_no_progress refusal, on both iOS and Android: https://github.com/callstack/agent-device/blob/55797fc/src/daemon/scroll-runtime.ts#L140. Nobody has shown the Android helper capture gives a comparable pair against a stored snapshot, and nobody has shown the refusal firing on a device. Without that, directional scroll on Android could answer unobserved or refuse wrongly, and it would ship unnoticed. Can you run All 16 checks were queued when I looked, and none had failed. Smoke Tests and Integration Tests exercise this route (scroll-runtime.ts, generic-runtime-execution.ts, and the contracts use declaration all changed), so they need a finished green run on 55797fc. I did not run tests or devices; this review covers only the logical delta from the range-diff. The rebase moved the base from 15ae636 to a26d175, so I did not re-review the base layers from #2770 and #2776. Is the chained-scroll baseline carry-forward that you mentioned as a required follow-up tracked anywhere, since it isn't part of this stack? The next thing this needs is an Android helper-backed run showing movement:'moved', one on-device run showing the scroll_no_progress refusal, and a finished green run of Smoke and Integration on 55797fc. |
55797fc to
745a88f
Compare
|
Both device runs are done, and your Android question is what surfaced a false Android, with the helper path proven first —
{ "reason": "scroll_no_progress", "direction": "down", "hiddenContentAt": "bottom",
"containerRect": { "x": 303.7, "y": 244.3, "width": 320, "height": 92.3 },
"requestedPixels": 502 }
Your second Android probe is what caught a bug. Chasing why On a chained CI: 🤖 Generated with Apex by Callstack |
745a88f to
35d4071
Compare
|
Rebased onto
The refusal was re-verified on devices, on both platforms — Your Android note then let me prove the
with the app's own row geometry identical across the gesture in both runs ( Everything else on #2770 / #2776 / #2772 is answered in their threads; the chained- 🤖 Generated with Apex by Callstack |
|
Reviewed 745a88f. The scroll-container gate from the earlier review (#2771 (comment)) is fixed, and this looks ready for human review on the code. Not blocking: could #2776 also require a within-container difference for horizontal scrolls? The Android and iOS device results are from the author's report on 745a88f. Smoke Tests and Bundle Size were cancelled or queued after the restack. This diff changes The head has since moved to 35d4071, a rebase onto main in which this commit is patch-identical, so the review above still applies. |
|
CI note, so the red Smoke is not read as this layer's: the failing assertion is That is #2784: This layer's diff is 19 files and touches neither the bridge nor snapshot warning assembly, and 14 of 15 items in that job passed — including the scroll assertions this layer owns. Rerun triggered; the lane should settle when #2818 lands. 🤖 Generated with Apex by Callstack |
Part of stack #2777. Layers below: #2770 (behavior-neutral readers) and #2776 (the observation module). This layer wires
scroll <direction>to it — the only commit here that changes an answer on a device.The bug (#2714)
scroll down 0.75reported the travel its gesture plan had computed. Nothing between the gesture and the response looked at the screen, so the distance was a restatement of the request — on every retry, until a later assertion failed for a reason no log explained.What this layer changes
resolveBoundScrollRuntime's direction arm asksplanScrollMovement({device, flags, session})before the dispatcher runs, thenexecuteDirectionScrolldispatches once, asks #2776 what movement that gesture may claim, and answers:movement: 'moved', one capturescroll_no_progressmovement: 'at-edge', no distancemovement: 'unchanged'movement: 'unobserved'captureSnapshotis declaredconditionalon the direction use — notpreferred(ADR 0019 §2 reserves that for optimizations, and this observation is what makes the answer honest) and notrequired(a runtime that cannot read a screen still scrolls, and answers the response it answered before). One wire test per side of that parity.Callers that already own the read are not charged twice:
--settle, and replay'spostGestureStabilization: false, decline with a typed reason, as does a non-swipe device.Review round 2 — what your two findings became
The lineage gate ran only on the quiet path. Fixed, and it was a real defect:
pollForSurfaceVerdictclassified first, so a baseline and a post-gesture read from different capture lineages could answermovedon a pair this module itself calls incomparable. The gate now runs on every capture, before the changed/unchanged classifier — including themovedpath. The deferred loop rebases on a producer change because its question is whether the surface settles; a pre-gesture comparison has no such option, so the claim is withheld. Test: acomparisonKey-drifted baseline whose frames genuinely differ →unobservedin one capture; its pair is asnapshot -ibaseline, same expectation. Red/green: deleting the gate turns exactly those two tests red.The
GenericExecutionDispatchseam. Gone, and you were right that it was the wrong seam:resolveGenericRuntimeExecutionalready hadsessionandreq, so the plan is computed there (session+flags, two lines) and handed to the closure as data.runtime-admission.tsand the five unrelated leaf files (back,focus,orientation,system-button,tv-remote) are untouched by this stack now — 30 lines and one shared-type widening deleted. It is also more correct: the flags and device it reads are pre-effect facts, so reading them pre-seam removes the pretense that the execution closure is where they belong.Two more things fell out of the same pass: the
capture!non-null assertion is gone (the capture is bound before it is used), andexecuteDirectionScrollnow dispatches once and returns once instead of three near-copies ofscrollOnce(…) → scrollResult(…). The four hand-rolled capture inputs in this file (movement read, edge rest-poll, edge state,--until) collapsed into onescrollCaptureInput(context, scope?), which also makes explicit a decision that was previously only an idiom repeated three times: a scroll read carries the session's app and the request's execution metadata, and deliberately none of the caller's snapshot flags, sosnapshot -ion the same request cannot change what a stop condition reads.Validation
pnpm check:affected --run→ all runnable checks passed on the stack head882256121(lint, format, typecheck across all packages + examples, layering, fallow, the vitest and node-test stages). At this layer's head (35d4071af): the scroll wire suites (136 files / 696 tests related toscroll-runtime.ts) andtscclean, andpnpm check:fallow --base fix/scroll-movement-observationreports no issues.movedasserts exactly one capture and every decline asserts zero; the harness runs the real ADR 0014 transition (expireRefFrame) between resolve and execute, so a claim that depends on the wrong moment fails rather than passing by accident.Live verification — iPhone Duo simulator (iOS 27.1),
Agent Device Tester@/automationBuilt from this branch (
pnpm build && pnpm clean:daemon), unscopedsnapshotfirst,--verbosefor the request ndjson.scroll down 0.75(your control)movement: 'moved'—Scrolled down by 0.75 of the viewport (502px); ndjsonscroll_movement_observed {"movement":"moved","attempts":1,"durationMs":359,"requestedPixels":502}(second sample 344 ms). The one-capture cost claim holds on a device.scroll top→snapshot→scroll up 0.75movement: 'at-edge'—Scrolled up and no hidden content above was detected; ndjsonattempts:2, durationMs:886. The quiet pair forms, no distance is claimed, and a legitimate end-of-content no-op is not refused.snapshot -i→scroll down 0.75(your ask)movement: 'unobserved',reason: baseline-presentation-drift. The gate fires on a device, not only in fixtures.scroll down 0.75(no capture between)movement: 'unobserved',reason: stored-surface-not-current— see below.The gap, escalated by the live run
The refusal did not fire on the issue's own usage pattern. Repeat
scroll down 0.75without an intervening capture and every call after the first answersunobserved: the first scroll's own gesture is a device side effect, so the stored tree is no longer current when the second one plans. The distance is still reported next to thatunobserved, which is the disclosure rather than a lie — but #2714's agent was repeating the command, so this design as shipped answers honestly once and then goes blind exactly where the report needed it.I had filed this as a known gap; the run says it is the follow-up that has to land, and its shape is now concrete: hand the observation's own post-gesture capture forward as the next scroll's baseline, keyed on the side-effect lineage counter in
ref-frame.ts(readSessionRuntimeRevision), so a bare chain keeps a current baseline while any other command invalidates it. Refusing it is what makes the guard correct, so the cache — not a looser freshness rule — is the only right answer. Tracked as the next unit; I did not want to grow this stack by a session-state change while it is under review.scroll_no_progressnow fires on real devices, on both platforms. My earlier note said the refusal was unreachable because/automationhonors the synthesized swipe — that was about the list itself, not about what sits on top of it. With the tester's own confirmation sheet open (tap Open automation alert on/automation), the baseline is taken with the sheet up and the gesture lands on the sheet, so the analyzer resolves a container that still reports hidden content below while the contents never shift:{ "reason": "scroll_no_progress", "direction": "down", "hiddenContentAt": "bottom", "containerRect": { "x": 303.7, "y": 244.3, "width": 320, "height": 92.3 }, "requestedPixels": 502 }success: false,COMMAND_FAILED, messagescroll down moved nothing: the container still reports hidden content below and its contents never shifted, and the hint names the raw drag (swipe x1 y1 x2 y2 started inside the list) because the refusal knows where the gesture ran. The request log shows both reads on one producer (snapshot_capture {"backend":"xctest"}twice, same fallback route), so the pair was comparable rather than lucky.On Android the same refusal fires on the tester's
/catalogafter a settled baseline:COMMAND_FAILED — scroll down moved nothing: the container still reports hidden content below and its contents never shifted. So the #2714 loop can no longer repeat itself silently: the first wasted scroll answersmoved-grade honesty, and the second one fails the command.Android: run, with the helper path proven first.
pnpm build:android,pnpm clean:daemon, the development build installed viapnpm test-app:android,adb reverse tcp:8081, andsnapshot -i --jsonon the tester's/catalogbefore any scroll claim:helperVersionequalspackage.json's 0.21.12 (the stale 0.20.11-dev helper was replaced on the spot), so this is not the UIAutomator fallback. Thensnapshot→scroll down 0.75:movement: 'moved'as predicted, but on two reads rather than one: Android's first post-gesture read still showed the pre-gesture surface (a RecyclerView still settling), so the loop polled again and took the change on the second. The one-capture claim is iOS-specific; on Android the honest figure is two reads / ~440 ms. My earlierCan't find service: activitywas my own error — I had used the emulator beforesys.boot_completed.What the Android run found is fixed in #2776.
/catalogalso gave memovement: 'moved'for a scroll that had not scrolled: flipping only a system status-bar icon (dumpsys battery set level) between the baseline and the post-gesture read was enough, because the classifier had been asked about the whole surface and the helper's tree carries the status bar.movedis now confined to the container the swipe ran inside, and that shape answersunobserved/change-outside-container.🤖 Generated with Apex by Callstack