Skip to content

fix(scroll): an overscroll bounce at the edge is not movement - #2984

Merged
thymikee merged 3 commits into
callstack:mainfrom
okwasniewski:oskar/scroll-edge-bounce-is-not-movement
Sep 25, 2026
Merged

thymikee merged 3 commits into
callstack:mainfrom
okwasniewski:oskar/scroll-edge-bounce-is-not-movement

Conversation

@okwasniewski

@okwasniewski okwasniewski commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Closes #2884.

observeScrollMovement credits moved on the first capture that differs from the pre-gesture baseline. At the end of a list iOS rubber-bands past the edge, so that first capture lands mid-bounce with every row shifted by a few points, reads as changed, and the content then springs back to exactly the pre-gesture tree. The next command's stabilization sees the surface identical to its baseline (post_gesture_snapshot_stale_accept, matchedPreGestureBaseline: true) while the scroll had already answered moved, and an agent scrolling until at-edge never stops.

The baseline now carries its nodes, and a changed reading against a baseline that already ended in the scrolled direction (readScrollEdgeState(baseline.nodes, edge): a container with no hidden content) is credited only once the surface holds still and still differs. A bounce that settles back to the baseline takes the existing settled path and answers at-edge; a bounce that never settles expires as surface-unsettled. A scroll whose baseline still hid content keeps paying one read, so the one-capture cost claim holds everywhere except at the edge. Keyed on typed edge state and the shared at-rest comparator, not on diagnostics text. The decision is logged as scroll_movement_edge_rest_required.

Touched: 2 source files, 1 test file.

Validation

Tested commit 4549d4006 (review follow-up: edge state resolved at the swipe point through readScrollEdgeState's target; nested-list cases pinned).

  • pnpm check:affected --run: passed (an earlier run flagged the poll loop's complexity; the per-reading verdict is now its own function).
  • New tests: bounce that springs back is at-edge in 3 reads; content that changes at the edge and holds still is moved in 2 reads; a bounce that never settles is unobserved / surface-unsettled. The existing one-read moved case (baseline with hidden content) is unchanged.
  • Live, iPhone 17 Pro / iOS 26.2 simulator, CLI at this head, --debug: open com.apple.Preferences, scroll bottom, then twice snapshot + scroll down 0.75. Both scrolls answered movement: at-edge with scroll_movement_edge_rest_required then scroll_movement_observed attempts: 3 in the request log, i.e. the first read differed from the baseline and the surface settled back. On main that first read is the moved answer the issue reports.

Review in cubic

At the end of a list iOS rubber-bands past the edge, so the first
post-gesture capture lands mid-bounce with every row shifted and read as
moved, then the content sprang back to the pre-gesture tree. A change
against a baseline that already ended in the scrolled direction is now
credited only once the surface holds still and still differs; a bounce
that settles back answers at-edge. Closes callstack#2884.
Copilot AI lite review requested due to automatic review settings September 25, 2026 15:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread src/daemon/scroll-movement.ts Outdated
Comment thread src/daemon/scroll-movement.ts Outdated
The rest requirement resolved the edge state without the swipe point, so a
nested screen could answer for a scroller the gesture never ran in. The
baseline check now passes the swipe midpoint through readScrollEdgeState,
the same selection the edge verdict uses, and the poll loop doc names the
edge exception.
Copilot AI review requested due to automatic review settings September 25, 2026 16:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 3 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread src/daemon/scroll-movement.ts Outdated
@okwasniewski

Copy link
Copy Markdown
Contributor Author

[claude-fable-5-1] responding on behalf of Oskar

CI note: Smoke Tests on this head failed at accept deep-link confirmation: alert accept answered main thread execution timed out from the runner watchdog, 14 of 15 tests passed. That is the alert activation path on the runner (#2956 / #2491), not the host-side scroll movement this PR changes; the earlier run on this PR died on the xcrun probe timeout (#2940). No rerun rights here.

@thymikee

Copy link
Copy Markdown
Member

Reviewed at 510eb60. The code looks right.

Not blocking: the inline comment above the readScrollEdgeState(baseline.nodes, edge, { point: swipe.midpoint }) call at https://github.com/callstack/agent-device/blob/510eb60/src/daemon/scroll-movement.ts#L333 still describes the outer/inner list relationship backwards even though the code and the sibling test are correct, and claimMoved's own edge check a few lines down at https://github.com/callstack/agent-device/blob/510eb60/src/daemon/scroll-movement.ts#L446 resolves its container without the swipe midpoint while baselineEndsInDirection resolves with it, so the two calls could in principle pick different containers on a nested-list screen — passing swipe.midpoint through claimMoved's call would make one selection rule govern every edge decision in this module, but both points can be taken or left as they don't change behavior on the cases this PR covers.

Smoke Tests was still running at review time with nothing to attribute against this diff; scroll-movement.ts and scroll-edge-state.ts sit on the scroll-command route that suite exercises, so worth a look if it fails, but there's no failure evidence yet. I could not reproduce the author's iPhone 17 Pro / iOS 26.2 live scroll run myself and I'm relying on the PR body's description of it rather than a raw log, and I did not confirm pnpm check:affected independently since CI hadn't finished. The one open item is Smoke Tests finishing green.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 25, 2026
Copilot AI review requested due to automatic review settings September 25, 2026 16:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@okwasniewski

Copy link
Copy Markdown
Contributor Author

[claude-fable-5-1] responding on behalf of Oskar

CI note for 4549d4006: Smoke Tests failed at wait for Agent Device Tester (the fixture launch landmark, wait timed out for text) before any scroll ran; the same step failed on #2981's first run with no scroll code changed. Everything else is green. No rerun rights here.

@thymikee
thymikee merged commit bce6f52 into callstack:main Sep 25, 2026
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

iOS scroll reports movement: moved at the list edge when the content did not move

3 participants