fix(maestro): unify the scrollable-ancestor walks and fix Android scroll-container selection - #1592
Merged
Merged
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
Member
Author
|
Code review is clean on the behavior-preserving helper substitution and the divergence tests are meaningful. One sequencing correction before merge: the PR body’s statement that #1589 should merge cleanly is not true at the current exact heads. A three-way merge produces conflicts in |
thymikee
force-pushed
the
claude/sleepy-dubinsky-e6427b
branch
from
August 4, 2026 17:17
b5a83fe to
3173b62
Compare
Member
Author
|
Rebased onto The PR body's "should merge cleanly" claim was wrong — thanks for catching it, and I have corrected the body. More importantly, the divergence framing in the original commit was also wrong, and reviewing the comment is what surfaced it. I had compared the predicates over type strings harvested from fixture text rather than over what each platform actually emits at the call site.
So the third walk now collapses onto the shared helper as well, the explanatory comment is deleted, and the test is rewritten to pin the classification over each platform's real vocabulary, with the Android rows as the regression guard. This makes the PR a behavior change on Android, not the behavior-preserving substitution you reviewed — the swipe now starts inside the list rather than at screen centre. That is the intended fix, but it is the one thing worth a device check before merge, since the conformance oracle demonstrably does not cover scroll-container selection. Green at 🤖 Addressed by Claude Code |
thymikee
changed the title
thymikee
force-pushed
the
claude/sleepy-dubinsky-e6427b
branch
from
August 4, 2026 17:19
3173b62 to
bb23268
Compare
fallow reported three structurally identical "walk up the parent chain to the nearest scrollable ancestor" implementations as clone groups (dup:1b401a24, dup:ce01e1de). Two of the three predicates classify identically, one does not. snapshot-policy.ts's isScrollableNode is logically identical to contracts' isScrollableNodeLike -- same six type patterns, same `=== 'table'` equality, same role/subrole fallback, and neither normalizes the type first. The walks match too, so findScrollableAncestorRect collapses onto findNearestScrollableAncestor with `(n) => Boolean(n.rect)`. runtime-port-geometry.ts's isScrollableSnapshotType does NOT agree. It equality-matches the NORMALIZED type, so over 227 node-type strings harvested from the repo's fixtures and tests it disagrees in both directions: Android ListView/GridView/RecyclerView, HorizontalScrollView, AXScrollBar and role-only scrollables clip but are not swipe containers, while XCUIElementTypeTable and AXTable are swipe containers but do not clip (the clip predicate compares 'table' against the unnormalized type, so prefixed forms miss). Only bare `table` satisfies both. It stays separate, with the divergence and the reason each call site needs its own answer written down where it can be read. The new test is load-bearing rather than decorative: replacing isScrollableSnapshotType with the contracts predicate leaves `pnpm maestro:conformance` at 46/46 and the pre-existing maestro suite at 206/206 green. The oracle does not cover scroll-container selection, so nothing else in the repo fails on that collapse. resolveRootViewport is deliberately left alone -- it resembles contracts' resolveViewportRect but lacks its third "largest containing rect of any node" fallback, so it is a real divergence and not the next dedup.
…ntilVisible
The divergence note added in the previous commit was wrong, and it was
covering for a bug rather than describing a design.
Grounding the comparison at the call site instead of in fixture text
changes the answer. `node.type` is never normalized on the way in -- it
carries the raw platform string -- so the domain of each predicate is
exactly what each platform emits:
iOS `elementTypeName` returns 31 fixed short names ("Table",
"ScrollView", "CollectionView", ...), never "XCUIElementType*".
Android `attrs.className`, fully qualified.
macOS role-mapped short names; outside Maestro's platform union.
Over all 31 iOS names the two predicates agree on every single one. The
claimed `XCUIElementTypeTable` / `AXTable` divergence was measured on
strings the runner cannot produce; the real emission is "Table", which
both predicates accept. The role/subrole arm is macOS-helper-only, so it
is inert for Maestro entirely.
What remains is Android, one-directional, and a defect: matching a
normalized type for EQUALITY recognizes bare `android.widget.ScrollView`
and silently misses HorizontalScrollView, NestedScrollView, RecyclerView,
ListView and GridView. `scrollUntilVisible` therefore selected no
container and fell back to a screen-centred swipe inside essentially
every RecyclerView-backed list -- contradicting the function's own
documented intent, and contradicting the existing Android test that
expects `android.widget.ScrollView` to be selected.
So the third walk collapses onto the shared helper too: the substring
predicate is also the better fit for Android's open class-name space,
where an allow-list would keep missing NestedScrollView and every custom
subclass. All three walks now share
`@agent-device/contracts/snapshot`, and the explanatory comment is gone
because there is nothing left to explain.
The test is rewritten to pin the classification over the vocabulary each
platform actually emits, with the Android rows as the regression guard.
thymikee
force-pushed
the
claude/sleepy-dubinsky-e6427b
branch
from
August 4, 2026 19:25
bb23268 to
c7c43ab
Compare
Member
Author
Android live validation (emulator, API 36,
|
| selected viewport | swipe from | swipe to | start in list | end in list | |
|---|---|---|---|---|---|
pre-fix (HEAD~1) |
ScrollView {0,136,1080,2201} |
(540, 1236) | (540, 356) | ✅ | ❌ no |
| fixed | RecyclerView {0,606,1080,1731} |
(540, 1471) | (540, 779) | ✅ | ✅ |
The pre-fix drag terminates at y=356, above the list (which starts at 606) — in the collapsing app-bar region. That is the "screen-centred swipe in an unrelated nested gesture surface" the function's own doc comment warns about.
End-to-end on device
before: Screen reader, TalkBack, Display, Display size and text, Color and motion …
target present: False
run: agent-device replay flow.yaml --maestro --platform android -> Replayed 2 steps
after: target revealed: True, rect {x:63, y:2224, w:512, h:71}, inside the RecyclerView
scrollUntilVisible + assertVisible: "Text-to-speech output" passes. Repeated 5×, consistent.
Limits — stated rather than papered over
- Both builds pass this flow (pre-fix 7.3s / 8.0s, fixed 7.9s / 9.2s). On this screen the pre-fix aim still started inside the list, so it worked; the fix corrects the aim, it does not flip a pass/fail here, and it is not a speed change. A functional break needs a partial-height list with a competing gesture surface next to it, which stock Settings does not offer.
- Only
RecyclerViewis live-validated. I could not find a liveNestedScrollView/ListView/GridView/HorizontalScrollViewsurface on this image — stock Settings is RecyclerView throughout, and React Native reports its ScrollView as plainandroid.widget.ScrollView. Those four remain covered by the unit table only; they follow from the same substring rule on the raw class name, but that is an inference, not a device observation. - The aim numbers are computed, not sniffed.
geteventcaptures nothing because the Android helper injects throughUiAutomationat the InputManager level, so there is no kernel input event to read. The table above is the realresolveMaestroScrollableGesturerun against the real device snapshot, with the geometry file swapped betweenHEADandHEAD~1— not a re-implementation.
🤖 Addressed by Claude Code
Member
Author
|
Reviewed c7c43ab: clean and ready. The shared predicate fixes the real Android selection failure, not merely the duplicate walk. The RecyclerView regression goes red with the old normalized equality predicate, the daemon replay route is live-validated on Android, and all exact-head checks are green. Residual: the device run directly covers RecyclerView; the remaining Android container spellings are pinned by the same classification table. |
thymikee
added
the
ready-for-human
|
This was referenced Aug 5, 2026
thymikee
added a commit
that referenced
this pull request
Aug 5, 2026
"Is this the Application/Window root" was written nine times: three spellings normalizing `type|role|subrole`, five lowercasing `type` alone, and one comparing the normalized type for EQUALITY. Two of the nine sat in `contracts/snapshot-visibility.ts` itself, disagreeing with each other. Measured before collapsing, using #1592's method — ground the comparison in what each backend ACTUALLY emits, not in fixture strings. Over the 31 names iOS's `elementTypeName` can return, the 18 fully-qualified class names Android emits, and the 24 mapped/raw forms the macOS helper produces, the nine agreed on 71 of 73. The two exceptions are macOS window subroles, and the only spelling that disagreed is maestro's `===`, whose platform union is `android | ios` — so it can never see them. The duplication was textual, not behavioral, which is what made the collapse safe. `isViewportRootNode` reads role and subrole because the macOS helper is the only backend populating them and the only one able to emit a window whose `type` does not say so: `normalizedSnapshotType` returns the raw subrole for a non-standard window, so an `AXWindow` with subrole `AXSystemDialog` or `AXUnknown` reads as neither from `type` alone. Those two shapes are the whole behavioral delta of this change, at the six call sites that were type-only, and they are windows by role. `snapshot-viewport-root.test.ts` pins the predicate over those three emitted vocabularies. Red evidence: reverting the canonical definition to the type-only spelling fails 2 of 5 cells, to the equality spelling 4 of 5. Also drops two kernel re-declarations this made visible: maestro's local `containsPoint` and `rectsOverlap` were character-identical to `@agent-device/kernel/rect`'s `containsPoint` and `isRectVisibleInViewport`, in a file that already imports from that module. And `resolveViewportRect` loses three `as Rect` casts that only existed because `.filter()` cannot narrow `node.rect` — one `flatMap` states the same thing honestly. Deliberately NOT in this change: the three viewport RESOLVERS still diverge, and on Android that is a live defect rather than duplication. Filed separately with the measurement.
thymikee
added a commit
that referenced
this pull request
Aug 5, 2026
"Is this the Application/Window root" was written nine times: three spellings normalizing `type|role|subrole`, five lowercasing `type` alone, and one comparing the normalized type for EQUALITY. Two of the nine sat in `contracts/snapshot-visibility.ts` itself, disagreeing with each other. Measured before collapsing, using #1592's method — ground the comparison in what each backend ACTUALLY emits, not in fixture strings. Over the 31 names iOS's `elementTypeName` can return, the 18 fully-qualified class names Android emits, and the 24 mapped/raw forms the macOS helper produces, the nine agreed on 71 of 73. The two exceptions are macOS window subroles, and the only spelling that disagreed is maestro's `===`, whose platform union is `android | ios` — so it can never see them. The duplication was textual, not behavioral, which is what made the collapse safe. `isViewportRootNode` reads role and subrole because the macOS helper is the only backend populating them and the only one able to emit a window whose `type` does not say so: `normalizedSnapshotType` returns the raw subrole for a non-standard window, so an `AXWindow` with subrole `AXSystemDialog` or `AXUnknown` reads as neither from `type` alone. Those two shapes are the whole behavioral delta of this change, at the six call sites that were type-only, and they are windows by role. `snapshot-viewport-root.test.ts` pins the predicate over those three emitted vocabularies. Red evidence: reverting the canonical definition to the type-only spelling fails 2 of 5 cells, to the equality spelling 4 of 5. Also drops two kernel re-declarations this made visible: maestro's local `containsPoint` and `rectsOverlap` were character-identical to `@agent-device/kernel/rect`'s `containsPoint` and `isRectVisibleInViewport`, in a file that already imports from that module. And `resolveViewportRect` loses three `as Rect` casts that only existed because `.filter()` cannot narrow `node.rect` — one `flatMap` states the same thing honestly. Deliberately NOT in this change: the three viewport RESOLVERS still diverge, and on Android that is a live defect rather than duplication. Filed separately with the measurement.
thymikee
added a commit
that referenced
this pull request
Aug 5, 2026
…1613) * refactor(contracts): one viewport-root predicate for the whole repo "Is this the Application/Window root" was written nine times: three spellings normalizing `type|role|subrole`, five lowercasing `type` alone, and one comparing the normalized type for EQUALITY. Two of the nine sat in `contracts/snapshot-visibility.ts` itself, disagreeing with each other. Measured before collapsing, using #1592's method — ground the comparison in what each backend ACTUALLY emits, not in fixture strings. Over the 31 names iOS's `elementTypeName` can return, the 18 fully-qualified class names Android emits, and the 24 mapped/raw forms the macOS helper produces, the nine agreed on 71 of 73. The two exceptions are macOS window subroles, and the only spelling that disagreed is maestro's `===`, whose platform union is `android | ios` — so it can never see them. The duplication was textual, not behavioral, which is what made the collapse safe. `isViewportRootNode` reads role and subrole because the macOS helper is the only backend populating them and the only one able to emit a window whose `type` does not say so: `normalizedSnapshotType` returns the raw subrole for a non-standard window, so an `AXWindow` with subrole `AXSystemDialog` or `AXUnknown` reads as neither from `type` alone. Those two shapes are the whole behavioral delta of this change, at the six call sites that were type-only, and they are windows by role. `snapshot-viewport-root.test.ts` pins the predicate over those three emitted vocabularies. Red evidence: reverting the canonical definition to the type-only spelling fails 2 of 5 cells, to the equality spelling 4 of 5. Also drops two kernel re-declarations this made visible: maestro's local `containsPoint` and `rectsOverlap` were character-identical to `@agent-device/kernel/rect`'s `containsPoint` and `isRectVisibleInViewport`, in a file that already imports from that module. And `resolveViewportRect` loses three `as Rect` casts that only existed because `.filter()` cannot narrow `node.rect` — one `flatMap` states the same thing honestly. Deliberately NOT in this change: the three viewport RESOLVERS still diverge, and on Android that is a live defect rather than duplication. Filed separately with the measurement. * test(contracts): enumerate the macOS emitter's real vocabulary Review found the table claimed to pin "the vocabulary each backend actually emits" while omitting most of it. `normalizedSnapshotType` has three output classes and only two were represented: 1. thirteen roles mapped to fixed short names — six were missing (StaticText, TextField, TextArea, MenuBarItem, Menu, MenuItem); 2. AXWindow, whose output is the SUBROLE unless it is AXStandardWindow; 3. the `default:` arm, `subrole ?? role`, emitting the raw AX-prefixed value for every unmapped role. All three are now enumerated, and the table asserts its own completeness against the emitter's fixed-output set — a role added to that switch without being added here fails, which is the emitter-drift protection the docblock was promising but not delivering. Re-measuring over the complete tables also corrected the header's own numbers. The claim was "71 of 73 agree, 2 disagree"; over 75 names it is 71 agree and FOUR disagree, because AXSystemDialog and AXUnknown were absent from the old table. Those two are the behavioral delta of this PR — an AXWindow whose subrole is emitted as the type, invisible to the six type-only spellings and named exactly by `role` — so the incomplete table had been hiding the very rows that justify reading role/subrole. The other two (AXFloatingWindow, AXSystemFloatingWindow) remain inert: only the `===` spelling misses them and its platform union is `android | ios`. * test(contracts): derive the macOS fixed-output set from the emitter Two test-validity defects from review, both real. The raw-fallback row `{ type: 'AXSearchField', role: 'AXTextField', subrole: 'AXSearchField' }` was unreachable: the `AXTextField` arm returns `TextField` whatever the subrole, so no emitter run can produce it. Replaced with `{ type: 'AXSortButton', role: 'AXCell', subrole: 'AXSortButton' }` — a subrole on a genuinely unmapped role, which is what the `subrole ?? role` default arm actually emits. `MACOS_FIXED_OUTPUTS` was a hand-kept twin compared against a hand-kept table, which is circular: a new mapped Swift role is absent from BOTH, so they agree and the gate stays green. The "emitter-drift protection" the docblock promised did not exist. The set is now parsed out of `normalizedSnapshotType` in SnapshotTraversal.swift, so the comparison is against the emitter rather than against a copy of the table's own assumptions. `case "AXWindow"` returns a subrole expression rather than a literal and is deliberately outside the literal-return set. Red evidence: adding `case "AXDisclosureTriangle": return "DisclosureTriangle"` to the Swift switch fails with `expected [ 'DisclosureTriangle' ] to deeply equal []`; 6 pass once reverted. The parser throws rather than silently matching nothing if the function is renamed or moved. * chore: restore maestro conformance corpus to main 45 corpus YAMLs carried an unrelated quote-style churn ("Button" -> 'Button'). They were already modified in the worktree when this branch started and a `git add -A` swept them into the predicate commit. Nothing in this PR reads them. Restored verbatim to main.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fallowreported three structurally identical "walk up the parent chain until you find a scrollable ancestor" implementations as clone groups (dup:1b401a24,dup:ce01e1de), each with a different scrollable predicate. Verifying whether the predicates actually agree turned up a real Android bug, so this ends up being a consolidation plus a behavior fix, not a pure refactor.Grounding the comparison at the call site
node.typeis never normalized on the way in — it carries the raw platform string — so each predicate's real domain is exactly what each platform emits:elementTypeName(RunnerTests+Snapshot.swift)Table,ScrollView,CollectionView, … — neverXCUIElementType*attrs.className(ui-hierarchy.ts)androidx.recyclerview.widget.RecyclerViewScrollArea; outside Maestro'sandroid | iosunionOver all 31 iOS names the predicates agree on every one.
role/subroleis emitted only by the macOS helper, so that arm is inert for Maestro entirely.The Android bug
The geometry predicate equality-matched a normalized type against four values. That recognizes bare
android.widget.ScrollViewand silently misses every other Android scroll container:android.widget.ScrollViewandroid.widget.HorizontalScrollViewandroidx.core.widget.NestedScrollViewandroidx.recyclerview.widget.RecyclerViewandroid.widget.ListViewandroid.widget.GridViewSo
scrollUntilVisibleselected no container and fell back to a screen-centred swipe inside essentially every RecyclerView-backed list. That contradictsresolveMaestroScrollableGesture's own doc comment — "avoids beginning a screen-centred swipe in an unrelated nested gesture surface" — and contradicts the existing Android test that expectsandroid.widget.ScrollViewto be selected. It's an oversight, not a design.The substring predicate is also the better fit for Android's open class-name space: an allow-list would keep missing
NestedScrollViewand every custom subclass.What changed
All three walks now share
@agent-device/contracts/snapshot:snapshot-policy.ts—findScrollableAncestorRect+isScrollableNodedeleted; the visibility walk callsfindNearestScrollableAncestorwithBoolean(ancestor.rect).runtime-port-geometry.ts—isScrollableSnapshotType+findNearestScrollableContainerdeleted; theincludeSelfcase is a two-linefindScrollContainerhelper. No explanatory comment, because there is nothing left to explain.__tests__/scroll-container-classification.test.ts— 43 tests pinning the classification over each platform's real vocabulary (the full 31-name iOS set, common Android classes), through observable behavior rather than internal predicates. The Android rows are the regression guard, plus an explicit assertion that a RecyclerView swipe now starts at the list's centre instead of the screen's.Reviewer notes
scrollUntilVisiblenow starts its swipe inside the list. That is the intended fix, but the conformance oracle demonstrably does not cover scroll-container selection — I verified this by mutating the predicate and watchingmaestro:conformancestay 46/46 green and the whole pre-existing maestro suite stay green. A device check on an Android flow is the one thing worth doing before merge.resolveRootViewportis deliberately untouched. It resembles contracts'resolveViewportRectbut lacks that function's third "largest containing rect of any node" fallback — a real divergence, not a pending dedup.4f8dc3f31), keeping itsbuildSnapshotNodeMap/findSnapshotAncestor/isUsefulVisibilityAnchorcutover alongside this change. The newpackages/selectorsworkspace needs apnpm install.Verification
Green:
maestro:conformance46/46 ·test:maestro-compat240/240 ·test:integration:provider149/149 ·typecheck·lint·format:check·check:layering·check:fallow·check:production-exports.The provider suite is flaky under CPU contention on this machine — a run with the changes failed 7 tests on timeouts, and a clean
origin/mainbaseline failed 1. Failing set changes run to run, all timeouts, all in unrelated areas (doctor, record, text). Re-runs of both were clean.