Repository navigation
fix(interaction): refuse a target with no usable centre - #3842
faresrafat3 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
A selector that resolves to a target with no usable centre was dispatched at the node's origin, and the command reported success. The touch-point layer now uses the kernel's `isPositiveFiniteRect` guard, so a target with no extent, or one carrying the failed-read `CGRectInfinite` sentinel, refuses with the existing typed reason `target_bounds_invalid` and no tap is dispatched. The layer had no area rule of its own: `normalizeRect` refuses only a negative width or height, so a zero-area rect passed through it, and so did the sentinel. The kernel's `isPositiveFiniteRect` refuses both. The new fixtures publish the capture the issue describes: a zero-area node is `hittable: false`. ADR 0011 records the guard's rule and names the shapes the suite keeps, and the runtime-selector and runtime-ref contract suites prove the refusal in the in-process runtime harness, under the errorTaxonomy cell, before any dispatch. Fixes callstack#3841
|
The code change in 58b739c looks right, but it is not ready to merge: it needs live device proof. CI shows one green check, and the device lanes that exercise this route were skipped for this run, so green here does not cover the changed press/click path. There are no conflicts. This change alters which targets the runtime refuses for press, click and fill by selector and @ref on mobile (interaction-touch-point.ts). It comes from a physical iPhone SpringBoard report (#1296/#3841), and the contributor guide asks for a live run of a changed device-facing route. The PR body says the seven device lanes were skipped and live evidence is still owed. Fixtures and the contract harness only prove the fix against hand-written snapshots, so we do not yet know that the real capture yields a 0x0 or placeholder rect for the reported node, or that the real daemon refuses it. Please run this on a real iOS device or a simulator, on SpringBoard page 2 with the page-1 icons still in the tree at 0x0 and I read the diff and the new tests against the old normalizeRect/resolveRectCenter path, but I did not run the tests or the base tree. I also cannot confirm that |
58b739c to
3ea91a3
Compare
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
|
|
The earlier review of 58b739c asked for live proof of the new refusal. At 3ea91a3 the code is unchanged in intent, but that proof is still missing. The one reported check passes. The seven device lanes were skipped, and they exercise the interaction route this diff touches, so green does not cover the changed press and click path. No other reviewer has looked at this head yet, and I did not run the tests or re-trace the Swift capture files, so the claim that real iOS capture gives the zero-area node is not yet confirmed. The change in https://github.com/callstack/agent-device/blob/3ea91a3/packages/selectors/src/interaction-touch-point.ts#L37 alters which targets press, click and fill refuse by selector and @ref on mobile. The only proof now is hand-written snapshots in the in-process contract harness. The reply argues from the capture code and asks if the nightly replay-ios lane would count. It counts only if a case reaches a zero-area node. Otherwise, please paste a live run from a simulator or device. A maintainer can do this on macOS if your host cannot. The run needs these five items, on a home screen whose page-1 icons are still in the tree at 0x0 with hittable:false.
Please paste the command lines and JSON in the PR. That live run is what must happen before merge. |
|
Understood — and your answer settles the question I asked: the nightly The state that produces it. From the original report in #3841: on SpringBoard home page 2, the page-1 icons stay in the tree with The five items, as commands. On a Mac with the branch checked out: Item 2 must show A script that runs all of it and prints what each step must show is in the repository now: What I cannot do. This host is Linux with no macOS, no Xcode, no attached iPhone and no device-farm credentials, so I cannot run the lane myself. That is why the body says the device lanes were skipped and the live evidence is still owed. One thing I can offer, if the simulator will not give us the state. The zero-area node does not have to come from SpringBoard: it only has to exist in the tree. The repository's own test app could carry a view with no area — a zero-size element that stays in the accessibility tree — and a replay or Maestro flow that presses it. That would make the nightly lane reach the refused path on every run, which is a stronger answer than a one-off paste, and it is the answer to "does green cover the changed press and click path". I would rather you did not have to wait for a device to be convinced. Say the word and I will add it as a second commit on the branch, with the same fail-closed shape as the rest of the change: the flow asserts the refusal, and it fails if the node is not refused. One correction to keep the record honest. The reply above argues from the capture code — the iOS runner publishes a zero-area rect for a node that is not laid out, the runner's own invariant requires that node to be non-hittable, and the guard refuses it on both the selector and the The head is still |
|
Thanks, the page-2 recipe is clear and it matches #3841. At 3ea91a3 the code verdict does not change: the live run is still the open item before merge, and a maintainer will run it on macOS. One correction: A zero-size view in the test app with a flow that asserts the refusal would be useful regression coverage. Should it replace the one-off SpringBoard paste? That is the maintainer's call. It proves the guard on a real capture only if the lane runs on a device or simulator, so please hold it until the maintainer answers. |
The review asked for live evidence on a real device, and noted that `device-evidence.sh` was not in this pull request — so push it or drop the mention. This pushes it. The script runs the evidence run and prints what each step must show; it decides nothing. A human reads the output and pastes it into the pull request. Every command in it appears in the repository's own CLI help. The zero-size test-app view and the flow that asserts the refusal are not here: the review asked for them to be held until the maintainer answers whether that should replace the one-off SpringBoard paste, so they are held.
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
|
Pushed, and the head moved again: The script runs the evidence run and prints what each step must show; it decides nothing, so a human reads the output and pastes what belongs in the pull request. Its steps are the five items you asked for, in the order you asked for them:
It also presses an icon that is on the current page in the same session, because the proof has two halves: the guard refuses the node with no usable centre, and it does not break the normal path. That last step is the one I would not want skipped — a guard that refuses everything would satisfy steps 2 to 5 alone. The home screen has to be on page 2 while page 1 is still in the tree, which is the state #3841 reported. The script says so at the top, and it takes The zero-size view is held, not forgotten. It is not in this commit and it will not be in the next one either, because you asked for it to wait for your answer on whether it should replace the one-off SpringBoard paste. To be explicit about what I would do in each case: if it should replace the paste, I will add the view and the flow to the test app and make the flow assert the refusal, and the SpringBoard steps come out of the script; if it should sit beside the paste, I will add both and keep the script as it is now. Either way the view only proves the guard on a real capture if the lane runs on a device or a simulator, so it is worth having your answer before I write it. On the live run itself: agreed, and I am not asking anyone to take my word for it. The lane that would cover it is the changed press/click path, and the seven device lanes were skipped on this run, so green does not cover the change. The script is there so that the run is one command and its output is pasted, rather than a paragraph describing a run. |
|
Thanks for the update. The refusal change itself is unchanged since 3ea91a3, so the code review carries over, and the one check reported on 314516d is green. The new file does not yet give us the live evidence this PR owes, and it blocks merge. In device-evidence.sh, Could we drop the script and post the steps as a checklist in a PR comment? 1) Open SpringBoard on the device. 2) Snapshot and show the 0x0 node. 3) Press by selector and by @ref and show Not blocking: the script sits at the repository root, hard-codes the fork URL and the Still missing is a live run on a real iOS capture (SpringBoard page 2 or similar) that reaches the refusal route. It must show the press-by-selector and press-by-@ref JSON with error.code COMMAND_FAILED, details.reason I did not run the script. The no-op result comes from reading Before merge, remove device-evidence.sh from the tree. Then a maintainer runs the live iOS refusal and normal-tap evidence and the results go into the PR. |
Summary
A selector that resolved to a target with no usable centre was dispatched at
the node's origin, and the command reported success. The touch-point layer now
uses the kernel's
isPositiveFiniteRectguard, so a target with no extent — orone carrying the failed-read
CGRectInfinitesentinel — refuses with thetyped reason
target_bounds_invalid, and no tap is dispatched.The layer had no area rule:
normalizeRectrefuses only a negativewidth or height, so a zero-area rect passed through — and so did the sentinel.
9 files: the guard, its unit and command-level tests, the shared fixture, two
contract scenarios with their coverage manifests, and the ADR.
rect-center.tsand the reason vocabulary are untouched; no new reason wasadded.
Fixes #3841
Validation
Base
507ef2376621d78aa76cf531bedc5abdf30e7a5a, head3ea91a37c335c58af975322378758da8f13dafdf.pnpm check:affected --run --head <head>— all runnable checks passed(
exactCommit=true): format, lint, typecheck, layering, fallow, build, vitest.vitest --project unit-core packages/selectors src/commands/interaction—81 files, 695 tests passed.
vitest --project interaction-contract— 10 files, 80 tests passed.the unit cases fail naming
point: {x:0,y:0}, both contract scenarios failwith
Missing expected rejection.Unresolved risks. The device lane was not run: the change is in the
interaction path, so the repository's gate skipped seven device lanes —
web-smoke,replay-ios,replay-ios-device,replay-macos,replay-linux,linux-command-evidence,replay-android. The live evidenceis still owed. The full
unit-corelane is red on this host for reasons thatreproduce on the base tree.