Repository navigation
fix(ios): keep penalized-route fill on the field it focused - #3201
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Follow-up to the Cubic review in 97a0d43, 045d0ce and 79735a1. The pre-tap lookup now runs inside the text-input probe's issue containment, so a timed-out read no longer fails the fill or ends the runner. The captured input keeps its identifier, and a handle that later resolves to another field, or to nothing, fails closed instead of using the stale point. A secure field whose text equals its placeholder is now left unverified, and the fixture uses named constants. The new XCTests fail with the fixes reverted. 104 runner tests pass locally on iOS 26.5, and |
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
The fix at 79735a1 looks right to me, but the live evidence in the PR body does not cover this commit yet. The one check reported on this PR passed. The live runs in the PR body are on a0a2db4. Commit 97a0d43 then changed the same penalized fill route. A stale handle now resolves to nil instead of re-reading the point, and the pre-tap lookup now goes through probe containment (TextEntry.swift#L238). If the RN field's index-bound handle does not survive the layout move (exists is false, or its identifier changes), the head now returns TEXT_INPUT not-focused or commit-not-observed where a0a2db4 succeeded. Fixtures cannot show this. Could you re-run at 79735a1 (or 045d0ce or later) on the iOS 26.5 RN app with the XCTest channel penalized? The run should cover two cases: fill-replace into the bottom-sheet field on a fresh launch, and fill "" on the login email field. Please paste the CLI JSON responses (ok:true and the resulting snapshot value, with the neighbour unchanged). Please also paste the runner.log lines that show the coordinate-tap synthesized route was taken, with no AGENT_DEVICE_RUNNER_TEXT_INPUT_PROBE_UNAVAILABLE. The cubic-dev-ai P2 thread on the unprotected stale check still applies: #3201 (comment). The stale check reads .exists and .identifier raw, so it lost the ObjC-exception catch the old read had. Wrapping it in the existing containment and returning false on failure restores parity. I did not run XCTest or a device in this review. I also could not tell whether an index-bound handle survives an RN bottom-sheet relayout, or whether two inputs without testIDs could pass the identifier-only check after a re-bind. The focus allowance going from 2 s to 4 s lowers the longest undelayed fill from about 212 to about 181 characters, and I did not see a CHANGELOG entry for that. The next step before merge is the live run at 79735a1 with the responses and runner.log attached. |
|
Two commits since 79735a1:
At 3276a3e: (a) bottom-sheet fill-replace 10/10, needing 2–3 passes each; (b) login Tallies by commit
Failures at a0a2db4 and 002bb98 all returned On how the penalty was forced: a natural trip came on 1 of 13 fresh launches, so I used a local patch that isn't committed. When (a) at 3276a3e: CLI response, snapshot values, runner.log$ agent-device fill @e14 "Coldtwo" --json
{ "success": true, "data": { "x": 313, "y": 427, "text": "Coldtwo", "message": "Filled 7 chars",
"targetKind": "ref", "ref": "e14", "refLabel": "Client",
"resolution": { "source": "ref", "phase": "pre-action", "kind": "exact" } } }The 10 runs took (b) at 3276a3e: CLI responses, runner.log$ agent-device fill 'id=email-input' "" --json
{ "success": true, "data": { "x": 234, "y": 300, "text": "", "message": "Filled 0 chars",
"targetKind": "selector", "selector": "id=email-input",
"resolution": { "source": "runtime", "phase": "pre-action", "kind": "unique" } } }
$ agent-device get attrs 'id=email-input' --json
{ "success": true, "data": { "selector": "id=email-input", "node": { "type": "TextField",
"identifier": "email-input", "placeholder": "Email Address", ... } } } # no value: emptyIn runs 1 and 3 the cold runner also tripped the breaker naturally ( |
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
The code in 3276a3e looks good to me, and the earlier evidence request is now met. Fill on the penalized route now stays on the field it focused, and the text-input probe check at TextEntry.swift:243 now fails closed, so that earlier thread is fixed at this head: #3201 (comment). The slow-fill thread does not apply, so please resolve it: the 4.0 to 5.6 s is the whole fill (focus, clear, commit), and the measured clear phase fits inside the charged budget, so the 30 s watchdog is not reachable on the data shown: #3201 (comment). One limit to know about: the live runs forced the XCTest penalty with a local patch, so the natural bottom-sheet route is shown only by inference, and the wrong-field read path when the pre-tap lookup finds nothing is bounded but not exercised. CI is still queued or running, so I can't say yet whether the iOS Smoke Tests route that drives fill through the runner passes. There are no conflicts. Merge once Smoke Tests and the repo gates report green. |
thymikee
added
the
ready-for-human
|
The new change at c05aba1 looks good, and I have no findings on it. The four clear reads are now charged 3.6 s (4 x 0.9 s) in RunnerTests+TextEntry.swift, which covers the 0.80-0.88 s per read. The route now refuses texts of 129-138 characters before it types the first character. That is the gap the earlier review at 3276a3e left open, and the derived-value tests in SynthesizedTextEntryTests.swift cover that refusal. The live evidence from the author's earlier runs still applies, because the clear loop and the focus logic did not change. I did not see the raw per-read timings, and I did not run the Swift unit tests. I checked the arithmetic by hand and with a small script. The cubic-dev-ai P2 thread on the read cost no longer applies at this head. The 4-5.6 s figure in it was for a whole replacement, not one read, so you can resolve it: #3201 (comment) Smoke Tests is still running and has shown no failure so far. The change only touches the 129-138 character band on the penalized fill route, and smoke fills are short, so I expect no overlap. I can't attribute the result until the job finishes. Please wait for it to finish before merging. |
thymikee
removed
the
ready-for-human
|
At c05aba1 this branch now conflicts with main after today's merges. The code verdict from the earlier review is unchanged, but I removed |
When the XCTest channel is penalized, iOS `fill` taps the daemon's point and types through private synthesis. That route named the field only by the point, but focusing can move the layout (keyboard avoidance, a bottom sheet extending above the keyboard). The commit wait then re-read whatever sat at the point: a fill that landed failed with TEXT_INPUT_COMMIT_NOT_OBSERVED, and `fill ""` cleared the neighbouring field and reported success. The input under the point is now found before the tap, and the target is bound to its identity, the way callstack#3161 binds an element-route target. Reads and clears take that input's handle while it still carries the identity, and otherwise only the focused input or the app-wide identifier match that carries it; a target bound before its tap never re-reads the point. The lookup and the handle check run inside the text-input probe's issue containment, so a read that cannot answer fails the fill closed instead of ending the runner. Finding the input took up to 2 s more on a React Native bottom sheet, so the replacement's focus allowance rises from 2 s to 4 s.
A secure field never exposes its value, so the synthesized replacement's commit wait read nil on every poll and could only expire: every password `fill` on this route failed with TEXT_INPUT_COMMIT_NOT_OBSERVED. The element route leaves such a field unverified, and so does this route now, also when the text equals the field's placeholder. A readable field still needs an exact match here.
In a React Native app launched moments earlier (iOS 26.5 simulator), the
synthesized select-all is often dropped, so the replacement's clear deleted
only the last character and the fill typed after what was left ("Client"
filled with "Coldfour" became "ClieColdfour"). Each clear pass, a select-all
and a delete key, is now followed by a read of the field, and passes repeat
until it reads empty, at most four. A field that still holds text, or whose
read cannot answer, fails with the new TEXT_INPUT_CLEAR_NOT_OBSERVED and
nothing is typed. A secure field gets two unread passes and is typed into
unverified.
The delivery budget charges every pass, and each read 0.9 s (0.80-0.88 s
measured on a penalized React Native bottom sheet), so the route refuses an
undelayed replacement longer than 128 characters before typing any of it.
c05aba1 to
8c9776a
Compare
|
Rebased onto main at 8c9776a. I squashed the review follow-ups (002bb98, 3276a3e, c05aba1) into three commits matching the three bullets in the description, since the follow-ups mostly rewrote the first commit. The one real conflict was #3161. Instead of keeping a parallel identifier check, the pre-tap lookup now binds the target to #3161's At 8c9776a: |
|
I found no blocking problems at 8c9776a. The conflict from the earlier review (#3201 (comment)) is gone, and no conflicts remain. The index-bound handle, the secure-field and placeholder order, and the probe failure path now hold at this head. Not blocking: the comment above awaitSynthesizedReplacementCommit in apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+SynthesizedTextEntry.swift (https://github.com/callstack/agent-device/blob/8c9776a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+SynthesizedTextEntry.swift#L433) still says each poll re-resolves from the refresh point. After this PR the target carries inputAtRefreshPoint, and resolveTextEntryElement trusts that handle first. The comment could say that polls use the input found under the point before the focus tap, and the point is used only when none was found. You can take this or leave it. The four cubic-dev-ai threads are fixed at this head, so please resolve them: the clear read allowance (#3201 (comment)), the index-bound handle (#3201 (comment)), the secure-field check order (#3201 (comment)), and the probe read failure (#3201 (comment)). I did not run XCTest or the live runs. The 127-test pass and the forced-penalty live results at 8c9776a are your report. They name the coordinate-tap replacement route and show no TEXT_INPUT_PROBE_UNAVAILABLE, which covers the re-binding path in this change. Two inputs that are both unidentified and of the same type share one identity, so a handle that re-binds between them can still pass the resolve check. That limit is unchanged from the head already reviewed, and it is not a regression here. Smoke Tests, Repo Guards, Coverage and Integration Tests are still running, and none has failed. The diff touches the runner Swift/ObjC and one docs line, so the iOS smoke lanes are the checks that exercise it. Please make sure CI is green on 8c9776a before it is merged. |
thymikee
added
the
ready-for-human
2 participants
Summary
On the penalized-XCTest route, iOS
filltaps the daemon's point and types through synthesized input. Three problems there (RN 0.86 app, iOS 26.5 simulator):TEXT_INPUT_COMMIT_NOT_OBSERVED, andfill ""cleared the neighbouring field and reported success. The input under the point is now found before the tap and binds the target to its identity, as fix(ios): fill succeeds when app removes input after last character #3161 does on the element route. Reads use that handle, or the focused or identifier match with the same identity, never the point; otherwise the fill fails closed.TEXT_INPUT_CLEAR_NOT_OBSERVEDand types nothing.Runner-only, plus a docs line for the new error (10 files).
Validation
8c9776a:pnpm check:affected --runpasses, and 127 runner XCTests pass (text-entry files, including fix(ios): fill succeeds when app removes input after last character #3161 and fix(ios): report fill into a non-echoing field as unconfirmed #3171). New tests fail without their fixes.8c9776a, iOS 26.5, penalty forced by a local uncommitted patch: bottom-sheet replace 5/5 on fresh launches; loginfill ""3/3, clearing only its target. Password fill ok, unverified.