Repository navigation
fix(ios): settle the first capture after an open that saw the launch in flight - #3356
Conversation
|
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
I reviewed #3356 at c57cbd2. The code looks right to me, but I cannot approve it until the live iOS check is confirmed. I did not run the simulator validation, and the three-run table is the author's own result. The author also says the CI layout shift (y=319) was not reproduced. I did not run the unit, runtime or integration tests, so I judged the regression tests from the diff and the old code. I did not measure the added latency of the first capture after an unobservable open. The table reports about 2 to 2.7 s, and a run that reaches the unobservable open path and shows that range would settle it. All 21 checks pass, and there are no conflicts. Nothing else blocks the merge once that run is in. Not blocking, and you can take or leave these: "Closes #3354" overstates the scope, because that issue also asks for a typed failure when a moved target cannot be confirmed, a bounded check, and an updated cell in packages/contracts/src/interaction-guarantees.ts, and the PR body says observable opens and layout shifts after an open are not covered, so "Refs #3354" fits better, and the guarantee cell at https://github.com/callstack/agent-device/blob/c57cbd2/packages/contracts/src/interaction-guarantees.ts#L168 could name the open-time mark; also POST_OPEN_IN_FLIGHT_OBSERVATION in https://github.com/callstack/agent-device/blob/c57cbd2/src/daemon/deferred-interaction-outcome.ts#L116 is a one-use alias for the literal 'unobservable' with a long doc block, so inlining it and shortening the comment would read better. Is there a smaller design? I could not find one. It reuses the existing quiet-window loop and the postGestureStabilization field through one small owner function, and marking inside markDeferredInteractionOutcome would not work because that call runs before the session is published. The earlier thread on the commands.md settle limit is fixed at this head, so you can resolve it: #3356 (comment) |
|
Addressed in
|
|
This PR is ready on the code. The earlier review at c57cbd2 asked for the settle after an in-flight open to be tied to the unobservable open, and at 1fb2bce that is done: the commands page now says so and states that probe-failed and the other values are unaffected, so the cubic-dev-ai P2 thread on website/docs/docs/commands.md can be resolved (#3356 (comment)). Not blocking: the doc comment on markPostOpenStabilization in src/daemon/deferred-interaction-outcome.ts#L112 says "unobservable" means the launch was still in flight, but the same value is also returned when target discovery finds no running process (packages/platform-apple/src/snapshot-observability.ts#L155), so a one-line rewording to "launch still in flight or no process found" would match the code; take it or leave it. CI is green, and the diff touches daemon open completion, deferred-outcome marking and the iOS world fixtures, so the integration lane does exercise the changed route. The simulator run is your own report; I did not run it or read its logs, and I did not reproduce the CI layout shift (General at y=319 instead of 406), so the fix is judged by its mechanism and the regression test. I also did not run the unit, runtime or integration tests; from the diff, the new assertions would fail without markPostOpenStabilization. The PR has merge conflicts with main. Please rebase onto main to clear them; since #3355 also changes the TAP_OUTCOME_NOT_OBSERVED_GAP trackingIssue line, expect a one-line conflict in interaction-guarantees.ts. Then re-run pnpm check:affected, because I did not re-check the patch against the new base. |
…in flight (#3354) An iOS app open whose post-open observation came back `unobservable` returns on a fixed settle while the launch may still be laying out. A selector click resolved from that first capture tapped a point the target had already left. The published session now carries the existing post-gesture stabilization mark, so its next capture runs the same bounded quiet-window loop a scroll does. Fresh sessions are covered: the mark is placed after the session is published. Other verdicts, Android, and warm clicks are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… settle Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… mark in the tap gap Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1fb2bce to
601fff5
Compare
|
Addressed in
The live simulator run is from |
|
I reviewed 601fff5 and found nothing to fix. The conflict from the earlier review (#3356 (comment)) is resolved after the rebase, and all 21 checks pass. The earlier live simulator validation ran on 1fb2bce. I did not re-run it on 601fff5, because the change since then is comment and guarantee text only. I did not run tests locally, and I did not review the upstream commits the rebase pulled in. The cubic-dev-ai thread on the commands.md wording no longer applies, so please resolve it: the new bullet limits the settle to unobservable launches and says every other value, including probe-failed, reads once (#3356 (comment)). Nothing else blocks this PR. |
… read the app (#3367) CI evidence across 30 runs of 01-settings.ad: 61 iOS opens reported 54 probe-failed, 7 observable, 0 unobservable, so the unobservable-only mark from #3356 never fired. Every stale-tap failure followed a probe-failed open. Mark the next capture for any defined verdict other than observable, mirroring the open-policy fixed-delay condition; an open with no verdict (physical device) still does not mark. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… read the app (#3367) CI evidence across 30 runs of 01-settings.ad: 61 iOS opens reported 54 probe-failed, 7 observable, 0 unobservable, so the unobservable-only mark from #3356 never fired. Every stale-tap failure followed a probe-failed open. Mark the next capture for any defined verdict other than observable, mirroring the open-policy fixed-delay condition; an open with no verdict (physical device) still does not mark. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… read the app (#3376) * fix(daemon): settle the first capture after any iOS open that did not read the app (#3367) CI evidence across 30 runs of 01-settings.ad: 61 iOS opens reported 54 probe-failed, 7 observable, 0 unobservable, so the unobservable-only mark from #3356 never fired. Every stale-tap failure followed a probe-failed open. Mark the next capture for any defined verdict other than observable, mirroring the open-policy fixed-delay condition; an open with no verdict (physical device) still does not mark. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * docs(open): scope the post-open settle to opens that report a verdict Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(daemon): describe the post-open mark by what the verdict proves, not its cause Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
Summary
When an iOS
openreportspostOpenObservation: unobservable, the session's next capture now reuses the existing post-gesture quiet-window loop, so a selector click right after the open waits for two matching reads (bounded) before it picks its tap point. On CI, that first capture caughtGeneralat y=319 instead of the settled y=406.observable,probe-failed,app-unidentified,not-eligible, Android, and warm clicks are unchanged.I rejected a runner-side frame guard: it adds a query to every warm click.
Completes #3354 as rescoped. Layout shifts that don't follow an open,
observableopens, and the typed failure for a moved target moved to #3367.Validation
pnpm check:affected --runpassed on601fff53a, rebased onto main.1fb2bced2, iPhone 17 Pro Max simulator (iOS 27.0),01-settings.adwith--retries 0 --debug, 5/5 passed:observableunobservableThe unobservable path adds about 1.8 s to the first click after such an open. These runs did not reproduce the CI layout shift itself.
🤖 Generated with Claude Code