Repository navigation
fix(mobile): reobserve targets after keyboard transitions - #1297
shamilovtim wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Heads up: you’ve reached your flex budget. Increase your flex budget or wait for usage to reset.
View guided diff | Re-trigger cubic
okwasniewski
left a comment
There was a problem hiding this comment.
Thanks @shamilovtim. The diagnosis in #1296 is right: settled() treats "same rect as in the pre-action snapshot" as proof a control is at rest, and a keyboard-driven relayout can land after that first snapshot. The target_still_moving half is a good fix. The keyboard half needs a different shape before we can take it.
What I checked: merged onto current main (9837386, agent-device 0.21.29) with no conflicts. pnpm check passes, and so does the mobile unit suite (356 tests). Mutation check: if I revert surface.ts, all three reobserves after %s cases fail. If I revert errors.ts, the dispatched: 'no' case fails. So both tests catch the regression. I did not run this on a device.
Must fix
-
The keyboard fix depends on which action ran, so it only covers some of the cases.
surface.ts:1270(fill/clear) andsurface.ts:1328(typeText) drop the pre-action index, but the action that actually brings the keyboard up is a tap or focus on the field. I ran your regression withtap,focus, andpress 'a'on the textbox in place of the entry action, and all three still send the tap to the old ref (@e4, expected@e42). The same gap is in the character path ofpressFocusedKey(surface.ts:1337) and ofperformpress(surface.ts:1242), as cubic noted. A dismissal (Enter,dismissKeyboard) moves controls just as late. Adding more action kinds to the list would only patch over this, and we don't merge timing workarounds. Please base the decision on what the snapshots show instead of on the action kind. The engine already reads keyboard state off every capture (measuredKeyboard/keyboardShowing). Store it next to the pre-action index (themarkAction(before)call sites already pass that index), and insettled()skip the unchanged-rect shortcut when the keyboard state changed between the pre-action capture and the target's capture. That one rule covers tap, focus, fill, type, press, Enter, and dismiss, and it removes theaction.kind === 'fill' || ...conditional along with thetypeTextspecial case. Before you rework this, please confirm from your downstream logs that the stale first frame already reports the keyboard as visible (iOS reports its final frame immediately). If it doesn't, tell us, because then the missing piece belongs in agent-device and not here. -
The fix adds latency without data to show the cost. Right now every fill or type followed quickly by a test action waits out the rest of
transition, even when nothing moves. The keyboard-state signal from point 1 limits that wait to the cases where the keyboard actually changed. If you keep any unconditional wait, please run the mobile benchmark deterministic suite (apps/mobile-benchmark) and compare main against the branch. -
Some doc comments are now out of date. The
staleOrJSDoc (errors.ts:275) still says "Keyed on the reason alone", and the module table (errors.ts:11) lists onlydetails.reason. Please state thetarget_still_moving+dispatched: 'no'rule in both places, and remove the inline comment aterrors.ts:283. TheDEFAULT_TRANSITION_MSandsettled()JSDoc insurface.tsalso still say unmoved controls are acted on at once.
Nits
- The
staleOrcondition is fine as it is. If it grows, a smallrefusedBeforeDispatch(details)predicate next toSTALE_REF_REASONSwould read better than the inline||chain. - Docs (
options.ts:107,docs/reference/mobile.mdx:221, bothsetup.mdcopies): once point 1 is done this sentence becomes "a control the keyboard moved also waits out the budget". Keep it to one sentence that tells users what they'll see, and leave out how the engine does it. - The changeset will need rewording to match point 1.
For the maintainer
- The
target_still_moving+dispatched: 'no'-> retryableNODE_STALEmapping is correct on its own, and it is what agent-device's own hint asks callers to do: re-observe and retry. It probably also clears the 0.12.3 refusals @tadeumaia reported in the #1296 thread. If the keyboard rework takes a while, it could land first. - Overlap: #1253 (thymikee) moves press readiness into agent-device on Android, and #1300 (same author) changes
relocated()right next to this code. Agree on which layer owns motion readiness before point 1 is rebuilt.
This was an automated review.
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
|
Reworked in 828b1e8; evidence is recorded at head ef7f427. A fresh controlled public iOS reproduction does confirm the requested premise: on-screen keyboard nodes are present at 483 ms while the button is still at its pre-focus y418.67, and it moves to y258.67 at 785 ms. The key plane is inside the viewport and a screenshot confirms visible software keys. The PR links the component and observations. A fresh video was captured and is attached. This is a controlled focus-driven workload, not a claim of deterministic reproduction in the original app. The adapter now retains captured keyboard state beside the index and on the target binding. An unchanged rectangle skips the remaining transition wait only when keyboard state also stayed the same. The fill/clear conditional and typeText exception are removed. Tap, focus, character entry, Enter and dismissal are covered by the same rule. The comments, user-facing sentences and changeset were updated; no new unconditional wait remains. Same focused regressions: PR base 17 failed/4 passed, candidate 21 passed. All 366 mobile tests and pnpm check pass. The same built-CLI diagnostic passes on base and candidate with one worker and zero retries; those native runs demonstrate compatibility, not a deterministic live failure or performance claim. The full unmodified benchmark was not run: the local generated host needed UIScene adaptation for the installed iOS 27 runtime. For overlap, the boundary follows #1253's stated split: native readiness owns eligible opt-in Android presses, while this PR changes adapter transition policy. Please confirm that boundary in re-review. #1300 handles a target lost after refresh; a combined disposable checkout passes 367 mobile tests after resolving the binding-argument overlap. A separate local merge with current main d8c597b passes 374 mobile tests and pnpm check, preserving its viewport changes. The PR body recommends landing #1300 first and retaining its stale check when rebasing this PR. Required fork workflows still await maintainer approval. video.mp4 |
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Heads up: you’ve reached your flex budget. Increase your flex budget or wait for usage to reset.
View guided diff | Re-trigger cubic
ef7f427 to
530e530
Compare
|
Rebased |
okwasniewski
left a comment
There was a problem hiding this comment.
Thanks @shamilovtim. The rework does what the last review asked. The decision now comes from what the snapshots show, not from which action ran. Every capture keeps keyboardVisible, and so does every binding. settled() (surface.ts:1196) uses the unchanged-rect shortcut only when the keyboard state before the action matches the target's capture. The per-action conditional and the typeText special case are gone. Thanks also for checking on a device that the first stale frame already shows the keyboard.
Status of the earlier asks at 4a092a5:
- Base the decision on observed state: done. I removed the
keyboardVisiblecomparison and 17 tests failed: tap, focus, fill, clear, type, character press, focused character, Enter, focused Enter and dismiss, for both the measured and the tree keyboard signal. - Latency: resolved. Nothing waits unconditionally any more. The rest of the budget is spent only when the keyboard actually changed.
- Out-of-date doc comments: done. I made the
target_still_movingmapping ignoredispatchedand theunknown/undefinedtests failed.
pnpm checkand the mobile unit suite pass (374 tests).
Must fix
- Please remove
packages/mobile/tests/fixtures/keyboard-signal/README.md. No test uses it. It is a 99-line write-up of a one-off device check, and it describes a local iOS 27 host adaptation. Evidence like this belongs in the PR description, which already links to it at a fixed commit.
Nits
- The changeset describes how the engine works ("captured keyboard state", "transition budget"). Write it from the user's side instead, for example: "A test step on a control the keyboard moves now waits for it to land instead of tapping where it was. Moving-target refusals are retried when agent-device sent no input."
I approved the fork CI runs for this head.
This was an automated review.
|
[Codex] responding on behalf of shamilovtim Addressed both requests in 17b7d31: removed the unused keyboard-signal README and rewrote the changeset from the user's perspective. The existing PR-description permalink still resolves to the original diagnostic evidence.
Ready for re-review. GitHub denied this account permission to submit a formal review request ( |
What and why
target_still_movingto retryableNODE_STALEonly withdispatched: 'no'. Missing/unknown dispatch stays nonretryable.flowchart LR A[Target captured] --> B{New or moved target, or keyboard changed?} B -->|Yes| C[Wait remaining budget and refresh] B -->|No| D[Act immediately]#1253 keeps native readiness ownership for eligible opt-in Android presses; this change belongs to the adapter transition policy. #1300 handles losing the target after refresh. Merge #1300 first, then retain its stale check when rebasing this PR. A disposable combined checkout passed 367 mobile tests after resolving the binding-argument overlap.
Issue
Fixes #1296.
Verified
Ran it locally: yes. Head
530e53046a9d790f8891629443b9c383568e873f.Public controlled fixture and capture evidence: visible software-keyboard nodes precede the button moving from y418.67 to y258.67. The original consumer raw capture was not retained; this verifies the signal in a fresh controlled reproduction.
d8c597b93f77d8: 1/1 pass, 3.71s body828b1e8: 1/1 pass, 4.24s bodyBoth live runs pass, so they establish compatibility, not a deterministic native failure or performance claim. The video run also passed; media remains local because upstream attachment upload requires repository write access. The diagnostic host used a local generated UIScene adaptation for iOS 27; no full unmodified benchmark or Android device pass is claimed.
Rebased onto current main
d8c597b, preserving its viewport fix: all 374 mobile tests andpnpm checkpass. A fresh built-CLI run with agent-device 0.21.29 also passes, one worker and zero retries. Unchanged-state fast-path tests assert no pending timer. Independent review found no remaining issues.The reviewed runtime head
4a092a550passed the spec, docs and mobile gates, including iOS and Android deterministic suites. The documentation-only follow-up17b7d310removes the one-off diagnostic README and rewrites the changeset for users; the evidence permalink above remains available at its original commit.pnpm check, all 374 mobile unit tests and an independent review pass for this follow-up. No new device run was needed for these documentation edits.