refactor: converge Maestro input handling#623
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b46dfbb2cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Review — converge Maestro input handling
This is the highest compat-risk change in the batch, precisely because it's the one with the biggest speed win (native wait+fill instead of snapshot-tap + per-char type). Worth being deliberate here.
Main concern — coalescing tapOn + inputText without a trailing Enter. In Maestro the semantics are "tap the target, then type into whatever currently has focus." Folding that into fill(tapSelector, text) assumes the tap target is the editable field. That holds for the common case, but it diverges from Maestro when:
tapOnhits a button/affordance that reveals or focuses a different input (e.g. a "Search" entry point that pushes a screen with a separate field — and notesearch/queryare inisLikelyTextEntrySelector, so this exact case will mis-coalesce), or- the real input's id/label contains none of the heuristic keywords, so we under-coalesce and silently stay on the slow path.
isLikelyTextEntrySelector is a reasonable gate but it's a string heuristic — it will both over- and under-match. Since the original code only did this coalescing when an Enter followed (a much stronger signal that this is a submit-a-field flow), dropping that guard widens the blast radius. Suggestions, in rough order of preference:
- Gate the no-Enter coalescing on the runtime-resolved node actually being editable (you already have an
is editablepredicate), and fall back to snapshot tap +typewhenfillcan't resolve an editable target. That keeps the fast path for true fields and preserves Maestro behavior otherwise. - If runtime gating is too invasive for this PR, at minimum record the heuristic + its known divergences in the compatibility debt map (#621) so it's tracked.
eraseText → type('\b'.repeat(n)). This leans on the native type handler interpreting \b as a delete keystroke. Maestro's eraseText deletes N characters from the focused field's cursor. Please confirm both Android and iOS native type actually treat \b as backspace/delete — the new test only asserts the parsed command shape, not that runtime deletion happens. If it's not guaranteed on both platforms this silently diverges.
pasteText → type. Drops clipboard/paste semantics in favor of synthesized keystrokes. Probably an acceptable divergence, but it's a divergence — worth a line in the debt map.
The "known gaps" note (focused input with no preceding tap stays on type) is a sensible scope cut. Once the editability gating question is resolved this is a strong net win.
Generated by Claude Code
|
Addressed in 70b33f2. The no-Enter tapOn + inputText path now stays as Maestro tap + focused type, optional tapOn is excluded from the wait/fill fast path, and the remaining Enter-only fast path plus paste/type divergences are called out in the PR body. Removed the duplicate provider-backed Android scenario owned by PR #620. |
|
1 participant
Summary
Converges Maestro text-entry replay for the narrower submit-field path: non-optional text-entry-looking
tapOn+inputText+pressKey: Enternow coalesces into nativewait+fill, with Enter preserved through the Maestro keyboard-enter runtime path.No-Enter
tapOn+inputTextremains on the Maestro tap + focusedtypepath so a tap that reveals or focuses a different field does not get rewritten intofill(tapSelector, text). OptionaltapOnalso stays on the Maestro runtime path so its skip semantics are preserved.Removed the duplicate provider-backed Android native-input scenario; PR #620 owns the umbrella provider guard.
Touched-file count: 4.
Known gaps: the remaining Enter fast path still uses the text-entry selector-name heuristic rather than runtime editability gating. Focused
inputText,pasteText, anderaseTextwithout a preceding text-entry target still use the existing focused-fieldtypebehavior because there is no target available for nativefill;pasteTextis synthesized text entry rather than clipboard paste.Validation
PATH=/Users/thymikee/.nvm/versions/node/v24.13.0/bin:$PATH NODE_OPTIONS="--import /private/tmp/rolldown-wasi-shim.mjs" /Users/thymikee/.nvm/versions/node/v24.13.0/bin/pnpm exec vitest run src/compat/maestro/__tests__/replay-flow.test.tspassed: 29 tests.PATH=/Users/thymikee/.nvm/versions/node/v24.13.0/bin:$PATH NODE_OPTIONS="--import /private/tmp/rolldown-wasi-shim.mjs" /Users/thymikee/.nvm/versions/node/v24.13.0/bin/pnpm exec vitest run src/daemon/handlers/__tests__/session-replay-vars.test.ts -t "coalesces Maestro text-entry tapOn"passed: 1 test, 65 skipped.PATH=/Users/thymikee/.nvm/versions/node/v24.13.0/bin:$PATH NODE_OPTIONS="--import /private/tmp/rolldown-wasi-shim.mjs" /Users/thymikee/.nvm/versions/node/v24.13.0/bin/pnpm exec vitest run test/integration/provider-scenarios/android-test-suite.test.ts --project provider-integrationpassed: 2 tests./Users/thymikee/.nvm/versions/node/v24.13.0/bin/pnpm typecheckpassed./Users/thymikee/.nvm/versions/node/v24.13.0/bin/pnpm check:fallow --base origin/mainpassed.PATH=/Users/thymikee/.nvm/versions/node/v24.13.0/bin:$PATH /Users/thymikee/.nvm/versions/node/v24.13.0/bin/pnpm formatpassed; unrelated formatter churn was restored, and the four touched files were formatted directly withoxfmt.