Repository navigation
fix(limrun): make Android provider text entry replace the field for fill - #3362
Conversation
The instance's setText inserts at the focused field's cursor and rejects empty text, so the pass-through injector broke fill's replace contract and concatenated the requested text over the old value (#3358). The injector now owns the replacement: tap the target, select-all and delete through the instance's own key events (the same channel the typing rides, mirroring the iOS leg), then setText only the replacing text — and for the empty fill, stop at the clear the SDK tolerates. Strengthen the AndroidTextInjectionRequest target contract to state the replace-and- clear obligation the platform now depends on.
Size Report
Startup median (7 runs, lower is better):
|
…rt closure The Coverage lane's eager-closure budget refused the static edge from android.ts into android-text-entry.ts; load the injector behind an await import on the text path, the same deferral the iOS leg uses for its own fill-path module.
|
I found two problems at c2d19ee. The Limrun Android fill change is not yet shown to work on a real instance, and no test covers the production routing. CI is green with 19 checks and none failing, and there are no conflicts. The new fill path is device-facing: android-text-entry.ts:24 now runs tap, Ctrl+A, Del, then setText through the live instance client. The old setText({x,y}) tapped and typed in one call. Now the tap is a separate call, followed at once by key events, with no wait for focus or the keyboard. If the keys arrive before the field has focus or the IME has attached, Ctrl+A and Del hit the wrong view or nothing. Then fill leaves old text behind or drops its first character. The "first character lost after focus moved from another field" note in the issue is the same risk, and the reporter's 3/3 check covered Ctrl+A, Del and setText, not this sequence. Could you run this on a real Limrun Android instance with the repo CLI on this head and attach the All four tests in android-text-entry.test.ts call I did not run the tests locally. I also did not read |
The injector's own tests call the factory directly; this pins that adbProvider.text routes through it, so a regression to the raw setText pass-through that caused #3358 now fails at the production seam.
|
@thymikee both findings addressed at Session-level routing test — added. Hard-failure claim, with the path you asked about: in the provider branch, Live Limrun run — blocked on this host, recording the command as the docs require. This machine has no On your point about focus timing after the separate tap: the reporter's own direct-client workaround (the 3/3 run) issued |
|
At e4e95f7, the new session-level test covers the production routing: it reads One thing is still missing. The fix depends on how a real Limrun instance handles Not blocking: the comment at android.ts#L83 says the dynamic import protects the import-time closure budget, but the injector is about 25 lines; a static import may be fine. CI is green with 19 checks, and there are no conflicts. If the live run shows replacement and an empty field, no code change is needed. |
|
@thymikee thanks — responding at
Live run: this host is credential-less for Limrun (env, interactive shell, keychain, main checkout — no Not-blocking, static import: tried and gate-refused, not judgment. The Coverage lane's eager-closure rule for |
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
The review asked whether a no-op key clear would let fill "" exit 0. Verification already refuses it on the provider branch; this pins that against the production seam so the soft-success cannot ever grow to cover an un-cleared field.
53dfef4 to
f8f1196
Compare
|
Thanks for the update. At f8f1196, the failure-path test now fails loudly when the field is not cleared, and the cubic-dev-ai thread on the text_mismatch assertion is fixed. CI is green on all 19 checks, but the new commit adds only a provider-seam unit test and no production change, so CI cannot show that the live Limrun route works. The main gap is unchanged from my earlier review. The fix in packages/provider-limrun/src/android-text-entry.ts (https://github.com/callstack/agent-device/blob/f8f1196/packages/provider-limrun/src/android-text-entry.ts#L30) depends on how a real Limrun Android instance handles pressKey('a', ['ctrl']), pressKey('del') and a tap right before them. Every test uses a recording mock, so a wrong key name, a wrong modifier shape, or keys that arrive before focus would still pass. If the keys do not clear a real field, fill still concatenates and #3358 stays open. Please run the 4-case script on this head through the repo CLI (connect limrun --platform android, open Settings) with a host that has LIMRUN_API_KEY, and attach snapshot -i output after each step. The output should show one copy of the text in the Settings search field after two identical fills. It should show the first character kept when focus moves from another field. It should show an empty field after fill @en "". And it should show appended text after type on a non-empty field. If the first character drops, add a short settle after the tap. The cubic-dev-ai thread (#3362 (comment)) is fixed at f8f1196, so you can resolve it. I did not run the tests locally, and I could not check real Limrun key naming, tap-to-key timing or IME behavior. There are no conflicts. The next step before merge is the live Limrun Android run on f8f1196. |
|
Thanks — confirming the state and closing the two things that can be evidenced without the live run: The cubic thread was already resolved ( Key naming and modifier shape are now pinned by the vendored SDK contract, not just the mock.
That removes "typo'd string" as a silent failure mode: an unknown key name is a server-side reject on a request we await, so a wrong name surfaces as Tap-to-key ordering: every step in the injector is a sequentially-awaited request on the single instance-client connection ( The live run: this PR head cannot be driven from my host — I've verified there is no Everything else I can own is done: 19/19 checks green on |
|
I ran the live Limrun Android check on f8f1196 through the repo CLI (
When a fill hit a non-text target, it failed with a verification error instead of reporting success, which is the right behaviour. I did not re-run the old behaviour on main; #3358 already shows it. This covers the gap from my earlier review, so this is ready for human review. |
|
Summary
Closes #3358. On a Limrun Android instance,
fillconcatenated the requested text over the field's old value: the provider-native injector was a one-line pass-through to the instance'ssetText, which inserts at the focused field's cursor and rejects empty text.fillAndroiddelegates replacement to the provider (theAndroidTextInjectionRequest.targetcontract), so the pass-through silently produced the merge, and verification reported it asunconfirmedsoft success.The injector now owns the replacement, matching the contract it already declared and mirroring the Limrun iOS leg: focus the target, select-all and delete through the instance's own key events (the same input channel the typing rides), then
setTextonly the replacing text. The empty fill stops at the clear, which is also the shape the SDK tolerates (setText('')is rejected). No platform routing change: atomically-replacing providers pay nothing extra, and the existing verify/retry stays the generic net.Touched files: the new injector + its test, the provider wiring, the contract doc-comment, and the type re-export it needed.
Validation
2e3a458aa:pnpm check:affected --run— all runnable checks passed (provider-integration covered by GitHub CI).android-text-entry.test.tscases fail (all fill shapes); thetypecase pins that append stays untouched.@limrun/api@0.59.0documentspressKeykeys as case-insensitive plain names ('A','BACK','ENTER', …) and modifiers as exactly'shift','ctrl'/'control','alt'/'option','meta'/'command'/'cmd','sym','fn'/'function'. The injector sends'a'+['ctrl']and'del', which are the documented forms ofKEYCODE_A+META_CTRL_ON andKEYCODE_DEL; an unknown name is a server-side reject on the awaited request, so it fails loudly rather than silently skipping the clear. All injector steps are sequentially awaited on the single instance-client connection, so keys cannot precede the tap's ack.setText) is the workaround the reporter verified 3/3 on the same instance. Residual risk: if an instance IME binds Ctrl+A differently, the old value stays and verification now fails hard (unchanged text → commit-dropped), never silently.