feat(scroll): find off-screen targets in one command with --until - #2436
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
The new loop must distinguish failed observation from end of content. Both captureUntilNodes and captureRuntimeScrollNodes discard capture quality and turn missing nodes into an empty array; the vertical edge analyzer then reports no room, producing scroll_until_edge_reached without a readable capture. Preserve the capture failure before matching or edge detection, and add regressions through both execution routes for missing/sparse captures. The web amount conversion also needs a live-browser check: verify observed scrolling for a relative amount and an explicit pixel distance, including the paced path. The reported iOS run is useful, but does not cover this separate behavior change. Please itemize the 7.7 kB unpacked growth and explain whether more of the existing scroll-pass machinery can be reused, or why a smaller design was rejected. The iOS CI failure happened while waiting for the test app before scrolling, so it looks unrelated to --until rather than evidence against the local measurements. |
97dbeef to
17956a5
Compare
|
All four addressed. Rebased onto main; the branch is linear on Failed observation vs end of contentYou were right, and it was worse than a reporting problem. Both Both routes now classify the capture before the selector match and before the edge analyzer, and refuse rather than continue. The classifier is
Sparseness reuses the signals absence assertions already trust ( Truncation is deliberately not refused. A truncated tree is real and readable with its tail missing, and refusing it would fail large screens where the target is plainly in view. There is a test pinning that. Regressions, both routes and both shapes: Chasing this also turned up a bug in my own error builders: the typed fields were nested one level too deep, so Web amount, live browserFair — the iOS run said nothing about this path. Wikipedia through the managed backend, reading
The paced path is covered on both branches, and the split sums to the requested distance rather than drifting. The 7.7 kBMeasured as an A/B:
Two things carry it, and new logic is the smaller one. About 3.8 kB is genuinely new: the daemon's until executor plus the capture refusal, the flag through the registry, the compatibility assertion, and the help and guidance text. The loop itself is roughly thirty lines. The rest is that Reuse, and the smaller designsReused rather than reimplemented: the edge signal ( Two smaller designs I did try and back out of: Static imports instead of lazy. Measured at roughly 1.5 kB smaller, because nothing gets split into its own chunk. It makes Extracting the sparse predicate into its own module so both callers get it without the The iOS CI failureAgreed, and confirmed. It failed at step 7 in Worth noting the shape: |
|
The new refusal check at 17956a5 still misses the backend's sparse verdict. BackendSnapshotResult carries it as quality, while scrollUntilCaptureRefusal reads snapshotQuality; the daemon test uses that wrong field too. The in-process route can also drop top-level quality when it selects result.snapshot. Preserve the canonical quality annotation on both routes and test real backend-shaped sparse results, proving they fail before matching, edge analysis or scrolling. The added web measurements and size explanation address those earlier requests. |
|
Right on both counts, and the daemon test was the worse half: it asserted through Fixed at The classifier now reads the verdict under every spelling a capture can arrive in. Asking each route to normalize first is exactly what produced this bug, so the one place that asks the question is the place that understands the shapes. The in-process route no longer picks a level. It was doing Tests use real backend shapes and prove the ordering. Each route now covers a missing capture and a sparse one, with a gesture spy asserting zero scrolls, and the sparse fixtures deliberately have content below the fold so an edge verdict would be wrong there too:
Live re-check on the simulator after the change, since the classifier now runs against real captures and a wrong field would refuse everything: |
3740017 to
09320ad
Compare
|
Due-diligence pass on the design. A large part of it was not needed, and the reason is worth stating plainly: I designed for two live execution routes without proving both were reachable. The in-process scroll route is dead codeI built Proved empirically rather than by reading. I put a tripwire throw at the top of the in-process
The public typed client's So the "two paths must agree" justification, and everything built to serve it, was scaffolding for a caller that does not exist. What that removedThe in-process
Every remaining byte lands in the daemon bundle where the feature lives, instead of duplicating shared modules into standalone chunks. The earlier itemization is obsolete: there is no longer a chunk-splitting line, and the growth is the daemon route (+3.8 kB, the loop and the selector pipeline it now reaches), help and guidance text (+0.8 kB), the registry tier and flag (+0.6 kB), the compatibility assertion (+0.3 kB), the MCP guide (+0.2 kB) and the web conversion (+0.1 kB). What stayed, and whyThe The VerificationUnit 9445, provider-integration 199, and typecheck, lint, format, layering, fallow, di-seams, integration-progress, mcp-metadata, build, package, production-exports, bundle-owner-files, command-docs, depgraph all green. Re-verified live on the simulator after the rework: one-pass arrival, the already-visible no-op, end-of-content after two passes, the edge rejection, and the clamped-amount message. Separate finding
|
|
The quality-field fix and single daemon implementation look sound at 09320ad. One regression fixture still needs correcting: climbingHierarchy calls the stateful targetTop() twice for one rectangle, producing inverted bounds on the first capture and advancing the target twice. Read it once, use that value for both bounds, and assert the intended arrival pass count so the test proves repeated scrolling on valid geometry. Also remove the duplicated edge-rejection, unavailable-capture and missing-capture tests left in scroll-runtime.test.ts. The reported live evidence and revised size explanation address the earlier gaps. |
|
Dug for second-order size wins. Found a small one, and found that my earlier attribution was wrong. Correcting that first, because it changes what is worth cutting. The selector pipeline costs 35 bytes, not 3.8 kBI claimed the growth was mostly Same for the rest of the module's dependencies, each measured by stubbing it out and rebuilding:
Everything the feature needs was already in the daemon bundle. There is no dependency to remove. Where the bytes actually areStubbing the whole Net across the branch, user-facing prose is about 2,400 B — roughly 40% of the growth, and it is the part that fixes the behavior the original report was about. What I trimmedProse that was stated more than once on the same surface. The 0.8 cap appeared in the gestures topic, the dogfood example and the command description; the top/bottom rule appeared in both the command description and the +6,499 → +6,252 B. Modest, and it is what is left once the fat is gone. What I looked at and did not cutThe nested-snapshot branch in the capture reader. A live iOS run never hits it, but a one-platform sample is not proof. I asked the type system instead: the runtime capture result does carry The error hints. They are the longest literals, and they are what an agent acts on instead of re-planning. Tightened the wording, kept every action.
My read is that the remaining growth is the feature, not overhead. Gates green, and re-verified live after the trims: one-pass arrival, end-of-content after two passes, the edge rejection, and the clamped-amount message. |
75b7740 to
d34bfeb
Compare
|
Both correct. Fixed at The climbing fixture was broken. The test now asserts the arrival count. The row starts two screens below the fold and climbs one per capture, so it arrives on the third: exactly two gestures. Removed the duplicated cases in One note on my own verification. A first pass reported an unrelated Android gesture-admission failure, both cases timing out at 5000 ms. Other worktrees on this machine were running suites concurrently; alone it passes. Flagging it so the run is not mistaken for a signal from this branch. |
|
The fixture and duplicate-test findings are addressed at d34bfeb. The test now uses valid bounds and proves two scroll passes; no remaining code findings, and the reported iOS/browser checks cover the changed behavior. The red iOS job timed out compiling the unchanged snapshot-bridge test before scrolling; it looks unrelated and needs a green rerun before merge. |
…canonical predicates
d34bfeb to
52665cf
Compare
|
All four blockers fixed, the safe drops applied, the nit taken. Rebased onto main at Blockers1. The command-runtime executor is gone, not moved. You are right that it was not a pure move, and right that it was dead: That also retires the disagreement you named — there is now one scroll executor, and it knows 2.
3. 4. Contract and schema aligned. Safe drops
Nit
The shared loopAgreed, and agreed it is not this PR. Filed as #2468 with your framing: one VerificationUnit 9474, provider-integration 203, and typecheck, lint, format, layering, fallow, di-seams, integration-progress, mcp-metadata, build, package, production-exports, command-docs, bundle-owner-files, depgraph. Re-verified live on a simulator after the predicate change: one-pass arrival, the already-visible no-op, end-of-content after two passes, the edge rejection, the clamped-amount message, and |
|
Looks good at 52665cf. The duplicate executor is gone, capture quality uses the shared sparse check, and --until uses the same visibility predicate as is visible. No remaining code blockers; the reported simulator/browser evidence covers the changed paths, and the shared edge-loop follow-up stays in #2468. All completed checks pass; iOS smoke is still running. One nonblocking cleanup: refresh the PR body to reflect the single executor and latest validation counts, since its earlier implementation description is now stale. |
|
Why
User feedback from a side-by-side run against Claude's built-in iOS tool: agent-device won, and the one recurring complaint was that the agent "scrolls too little to find an element", called out as the agent's fault rather than the tool's.
Investigating that turned up three things, and the agent was the least of them.
Our own guidance taught the slow loop. The skill's routing card and the MCP server guide both prescribe bare
scroll <direction> --settle. The off-screen refusal hint went further and told the agent to scroll "in small steps", warning that a single large scroll overshoots. The agent followed our instructions exactly.scroll bottomalready existed and nothing pointed at it. It runs a scroll-and-check loop server-side in one request. The string appeared in exactly one agent-visible place: the command's own description. Not in the skill, not in the MCP guide, not in the snapshot legend that prints[off-screen below], not in the refusal hint.A larger
amountbuys almost nothing, and said so dishonestly. One gesture is clamped to the viewport axis minus 10% padding at each end, in both the TypeScript planner and the Swift runner. Default travel is 0.65 of the viewport on iOS, 0.6 elsewhere, and the ceiling is 0.8. Soscroll down 3does not scroll three viewports, it scrolls 0.8 of one, and the success message reported "Scrolled down by 3". The dogfood help topic usedscroll down 3as its worked example.What this changes
scroll <direction> --until <selector>. Repeats scroll-and-check passes until the selector matches a node that is on screen, then stops. One request instead of the scroll-then-look-again loop, and because it checks between passes it stops on the target rather than sailing past it, which retires the accuracy-versus-distance tradeoff the old hint was working around.An unreadable capture is refused before either check runs, as
scroll_until_capture_unreadablewith ano-captureorsparse-treecause. Coercing a missing tree to[]would otherwise reach the edge analyzer as "no room below", so a failed read would report end-of-content.It stops three ways. Matched is the success. Running out of scrollable content and spending the 12-pass budget are separate typed failures (
scroll_until_edge_reached,scroll_until_pass_limit) with different hints, because the corrective action differs. End-of-content reuses the same signalscroll top/scroll bottomalready trust, so both stop in the same place. Horizontal scrolls have no such analyzer and are bounded by the budget alone.--untilis refused on thetop/bottomdirections, which already carry a stop condition, rather than letting one silently win.Honest distance reporting. The message now names the travel the planner honored:
Scrolled down by 3 of the viewport (700px). The result JSON already carried the honored pixels; only the prose lied.Web
amountwas in the wrong unit.amountis a viewport fraction everywhere else, and the browser backend passed it straight through as CSS pixels, soscroll down 0.5travelled half a pixel. It now scales the default step the way the Linux pointer backend scales its wheel clicks.Guidance. The skill, the MCP server guide, the snapshot legend, the workflow card, the gestures topic, and both off-screen refusal hints now name the one-command path. The dogfood example is no longer a number that does nothing.
Shape
scrollreaches a device in exactly one place (ADR 0019, the daemon's generic route), so the feature lives in one module beside it:src/daemon/scroll-until.tsowns the pass loop, the arrival check, the capture-readability check and both failure shapes. No new package surface, no shared-module indirection, and nothing to keep two callers in agreement.The
untilplan tier admits on the same facts the edge tier does, so it shares thatusedeclaration rather than duplicating one. The end-of-content signal is the onescroll top/scroll bottomalready trust, reached through a smallcanScrollFurtherAtEdgeexport on the existingscroll-edge-statesubpath.scrollmoved out ofgestures.tsinto its own runtime module first, as a pure move with its tests, since that file was already past the threshold the repo sets for adding behavior.Validation
Live on an iOS Simulator against Settings, using the built CLI on this branch, reaching the
Developerrow from the top of the list. Three runs each.scroll down+snapshot -i, repeated until foundscroll down --until 'label=Developer'The command count is the part that matters more than the clock: the old path also makes the agent read two full snapshots it does not need.
Web
amountverified in a live browser against Wikipedia, readingwindow.scrollYbetween commands and resetting to the top each time:scroll downscroll down 0.6scroll down 0.5scroll down 1.2scroll down --pixels 250scroll down --pixels 250 --duration-ms 300scroll down 1.2 --duration-ms 300Also confirmed live on this branch: an already-visible target costs zero gestures, the edge rejection prints its hint, a missing selector reports end-of-content after 2 passes rather than burning the 12-pass budget, and
scroll down 3now reportsScrolled down by 3 of the viewport (700px)on an 874-point viewport.Gates, all green: unit (9442 tests), provider-integration (199), typecheck, lint, format, layering, fallow, di-seams, integration-progress, build, package, replay-compat, daemon-wire-compat, mcp-metadata, command-docs, agent-guidance, gate-manifest, depgraph, bundle-owner-files, freerange, fixture-cache, production-exports.
production-exportspasses but prints 62 repo-wide unused exports as advisory output. None of them come from this branch, checked against its JSON.