refactor: move Wave 5 touch commands to platform runtime - #1987
Conversation
Size Report
npm unpacked components
Startup median (7 runs, lower is better):
Top changed chunks:
Top changed packed files
|
Deep code-quality reviewThe cutover itself is the right shape and the deletions are real ( But the daemon-side landing does not follow the pattern the four previous waves established, and the cost is visible in the diff. My main objection is structural, and I think it collapses a lot of this code. 1. (blocker) The admission seam is cast away, and six defensive wrappers are the debt
const bindSelected = admission.bind as unknown as (
device: DeviceInfo,
selectedUse: RuntimeUseDeclaration,
) => Promise<BoundTouchOperations>; // BoundTouchOperations = { operations: Partial<PlatformRuntimeOperations> }This is the only async function executeTapPoint(runtime, input) {
if (!runtime.operations.tapPoint) return requiredOperation('tapPoint');
return await runtime.operations.tapPoint(input);
}
// ×6, plus requiredOperation(): neverCompare the two prior waves, which needed none of this: // src/daemon/type-text-runtime.ts
/** What the executor needs: only the bound operation, however broad the bind that carried it. */
type BoundTypeTextOperations = Readonly<{
operations: Readonly<{ typeText: TypeTextRuntimeOperations['typeText'] }>;
}>;
...
const backendResult = await runtime.operations.typeText(...); // no guard, no cast
The cast exists because switch (plan.kind) {
case 'open': { const runtime = await bind(params.device, openApplicationRuntimeUse); ... }
case 'open-apply-runtime-hints': { const runtime = await bind(params.device, openApplicationWithRuntimeHintApplyUse); ... }with the comment "Only the exact bind differs per plan, so each 2. (blocker) A second capture-intent builder, three files from the one that says it is the only one
The new 3. (blocker) Ref support is encoded three times, and web asserts a fact it cannot knowOne invariant, three channels:
if (input.target.kind === 'ref' && interactor.tapRef) return await interactor.tapRef(...);
if (input.target.kind === 'ref') throw new AppError('UNSUPPORTED_OPERATION', 'Bound tap target requires a point...');copy-pasted across Worse, the union itself is loose: Concretely wrong today: 4. The interaction backend became a conditional-spread pyramid, and optional-chains inside its own guard
5. The Apple move dropped every "why" comment and inlined the magic numbers
const stepCost = (step.durationMs ?? 0) + (step.pauseMs ?? 0) + 250;
if (current.length > 0 && (current.length >= maxSteps || cost + stepCost > 20_000)) {Those two numbers were While there: these ~130 lines of sequence mechanics ( 6. Duplicated dispatch preamble, and a conditional identity wrapper
And 7.
|
|
Request changes: this head is not merge-ready. It conflicts with current main/#1955, and the exact-head Coverage run has two deterministic PR-owned failures: |
284b4b1 to
2f7369d
Compare
|
Addressed the review on
Validation: Merge-readiness evidence still pending: exact-head GitHub Coverage/Bundle Size and live device/cloud proof for the five commands. This update is published and reported, not a merge-ready claim; I did not wait for CI. |
|
Follow-up pushed in |
Live evidence + the §9
|
| command | iOS simulator | Pixel_7_review | web |
|---|---|---|---|
click |
✅ Home → Catalog (tree changes) | ✅ Home → Catalog | ✅ @ref → example.com (2 nodes) → IANA (33 nodes) |
press |
✅ registers as tap; longpress counter stays 1 |
✅ In cart: 0 → 1 |
— |
press --count 3 |
✅ In cart: 0 → 3 (fused sequence) |
✅ In cart: 1 → 4 |
— |
press --hold-ms 800 |
✅ registers as longpress (Long presses: 0 → 1) — the fused hold branch |
— | — |
press --double-tap |
✅ dispatched, +1 (fixture Pressable coalesces, so it cannot discriminate) |
— | — |
longpress |
✅ Last input: longpress, counter 0 → 1 |
✅ Last input: longpress, counter 0 → 1 |
— |
fill |
✅ ×2 — hidden-keyboard start and shown-keyboard/@ref; text reads back; debounced filter commits 12 results → 0 results |
✅ IME-helper path; lamp reads back; filter commits 12 → 0 |
— |
hover |
✅ refusal cell | ✅ both routes |
The press/longpress discrimination is the strong one: the same control, press → Last input: tap
with the long-press counter unchanged, longpress → counter increments. press --hold-ms 800 routing
to the longpress branch is the direct live proof of the fused holdMs > 0 step kind that moved into
platform-apple.
Web hover confirms the D2 design end to end: hover @e2 returns Hovered @e2 (no coordinates — the
DOM-handle route chosen inside the web owner) while hover text="Learn more" returns
Hovered text="Learn more" (297, 174) (the point route). One operation, owner-selected route, exactly
as the amended native-ref description claims.
2. §9 measurement: the preferred declaration is not supported by the numbers
Instrument: summed daemon-side phase durations from the --debug request ndjson (CLI wall clock is
unusable here — it swings 86–665 ms for the same nominal path on Node startup alone). Every sample
asserts request_success, and each is tagged with its actual path: cap= counts snapshot_capture,
fb= counts ios_direct_selector_tap_fallback. A/B is click id="…" (direct-eligible) versus the same
click with --count 1, which disqualifies the fast path via hasNonDefaultClickOptions while keeping
the semantics identical (one tap). n = 8 pairs each, alternating.
| target | direct (cap=0, fb=0) |
runtime (cap=1, fb=0) |
delta |
|---|---|---|---|
| Home, ~32 nodes | 900 ms median [885–1013] | 731 ms median [714–769] | +170 ms (+23.2%) slower |
| Catalog, ~46 nodes | 1270 ms median [1232–1366] | 1198 ms median [1180–1253] | +72 ms (+6.0%) slower |
The direct path skips the snapshot capture entirely (cap=0 on every sample, confirmed) and is still
slower on both trees. Its fused runner query costs more than capture-plus-coordinate-tap.
The gap narrows as the tree grows (+23.2% → +6.0%), which is the honest caveat: the fast path's
benefit scales with capture cost, so on a large real-world tree — the Bluesky-class app it was designed
for — it plausibly crosses over. What I can state is that on every target I could measure it is a
regression, so the recorded measurement §9 requires does not exist yet. Either measure on a
large-tree real app and record the crossover, or retire the declaration per §9 ("a preferred operation
declared without a measurement is speculative surface and is rejected in review").
A second, timing-independent finding sharpens this. React Native text= selectors make the direct path
fall back: ios_direct_selector_tap_fallback with error: "selector matched multiple elements", because
it counts raw XCTest matches before hittability can pick a winner, and RN wraps text so a visually
unique string is several raw elements. The cleanest case is the fixture's inert surface — a 4-node
screen where text="Tapping this text changes nothing on screen." is unique in the AX tree and the
direct path still saw multiple raw matches. Every instrumented attempt on that selector fell back,
and the 12 timed runs were uniformly slow (832–950 ms), consistent with all of them paying the failed
attempt plus the full runtime path. id= selectors on unique accessibility identifiers succeeded
(id="inert-target", id="inert-title", id="home-title", id="catalog-title", all fb=0).
So the fast path helps only for id=-shaped selectors, and on text= — the shape agents most often
author — it is pure added latency before the runtime path does the work anyway.
Caveats: one device, warm runner, one RN fixture, trees of 32/46 nodes.
3. Defect: Android hover refusal emits a focus hint (regression against legacy)
$ agent-device hover 'id="catalog-search"' # Pixel_7_review
Error (UNSUPPORTED_OPERATION): hover is not supported on this device
Hint: focus is supported on Android emulators and physical devices.
The user asked about hover and is told about focus. Root cause is one line —
packages/platform-android/src/runtime.ts:202 sets hover: focusKindUnavailable, reusing the fact
object built for the retired focus bucket, which carries its own focus-worded hint at line 65.
touchRuntimeOperationFacts defaults the correct text with input.hover.hint ?? HOVER_UNAVAILABLE_HINT,
so an owner that supplies any hint suppresses the right one. Apple, HarmonyOS and Linux pass a
hint-less unavailable and therefore print the correct hover text; Android is the only owner affected.
This is a regression, not a pre-existing wart: on main (checked at a pre-#1987 baseline)
unsupportedHintForDevice('hover', android) returns undefined, so legacy refused with no hint.
The PR replaces "no hint" with "a hint about the wrong command."
Suggested fix: give Android a hover-specific hint-less fact so the contract default applies. Worth
considering making it structural rather than a one-line fix — HOVER_UNAVAILABLE_HINT being a
default is what let an unrelated fact silently win; hover's hint could be set unconditionally on the
unavailable branch, which would make this defect unrepresentable.
Minor, non-blocking: the refusal message moved from "hover is not supported on this platform"
(legacy dispatch) to "…on this device". More accurate under per-device facts — flagging only
because the unit record committed to carrying that string through unchanged.
4. Not verified
fill on web (no suitable field on the pages used), HarmonyOS (no nova 14 attached), and Vega
(fact-only refusal cells). The --double-tap cell dispatched but the fixture cannot discriminate it.
|
Request changes at |
|
Addressed the remaining review on
Size disposition: the reported Validation: Live evidence remains the linked |
|
Addressed the remaining guarantee blocker in |
|
Still not merge-ready at Exact-head CI also needs owner action: Coverage has five eager-closure budget failures, and Android Smoke fails in the alert-cancellation interaction flow. The exact-head web live leg remains pending. |
e809d49 to
444a986
Compare
|
Addressed at
The Android artifact shows the migrated click succeeded, the alert was visible through wait/snapshot/get, and the later
|
|
Code review is clean at Not yet merge-ready: the exact-head iOS Smoke job is still running, and the PR still records the exact-head web live leg as pending after the sparse-provider admission changes. Finish those two evidence items before merge; no further code finding from this review. |
|
Summary
Moves the Wave 5 touch cluster from legacy capability and dispatch routing to request-bound platform runtime operations for
click,press,fill,longpress, andhover.Part of #1739.
Validation
Exact head
444a98633:pnpm check:affected --runpassed: 543 related test files / 4,293 tests plus every runnable format, lint, typecheck, layering, fallow, build/package, Node integration, provider integration, conformance, and structural/model gatemaestro-direct-selectorormaestro-non-hittable-fallbackpath with no new public response metadata{ source: 'direct-ios', kind: 'not-observed' }Live device evidence from predecessor
e809d492c(this follow-up changes only internal path classification, contract tests, and closure pins):click,press,fill, andlongpresscommitted the expected app-state changes;hoverreturned the canonical typed refusalhoverrefusalThe prior Android Smoke failure occurred after the migrated
clicksuccessfully opened and exposed the native alert.alert dismissthen closed the dialog without the fixture receiving its cancel callback; currentmainpassed the identical alert flow. This is isolated from touch dispatch and is left to exact-head CI rather than widening this PR.The preceding size report at
e809d492cwas+6.2 kBnpm unpacked (+1.3 kBgzip), entirely in JS/dist; Apple runner source, macOS helper source, and Android helper artifacts were unchanged. Exact-head Bundle Size and device lanes remain authoritative.Docs and skills are unchanged because the public command surface and semantics are unchanged.