Skip to content

fix!: review follow-ups: off-screen animations, no late clicks, batchTimeoutMs cap - #71

Merged
cryptotavares merged 4 commits into
cryptotavares/mmcli-p1-30-device-mcp-0-6from
cryptotavares/mmcli-p1-33-review-followups
Oct 29, 2026
Merged

cryptotavares merged 4 commits into
cryptotavares/mmcli-p1-30-device-mcp-0-6from
cryptotavares/mmcli-p1-33-review-followups

Conversation

@cryptotavares

@cryptotavares cryptotavares commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

Summary: Fix the review comments on #62, #64 and #65 in one PR on top of the stack. The approved PRs stay unchanged.

Stack context: mm CLI v2, Phase 1, P1-43. Base: #69, the top of the stack.

Problem

Comment Problem
#65: off-screen animations The browser settle ignored animations whose target was outside the viewport or had zero size. A drawer sliding in, a bottom sheet or an accordion opening from height 0 start that way, so the observation caught them mid-move.
#64: late click click / type checked the request signal only before the target wait. When the request timed out during the wait, the agent got "timed out", but the click still ran once the target appeared.
#64: batchTimeoutMs batchTimeoutMs accepted up to 300 s, but the queue and the CLI stop run_steps after 30 s.
#62: refs after none --observe none does not refresh refs; some refs can then click a different element. Not documented.
#62: reads and animations Reads do not wait for animations, even with --observe full. Not documented.

Solution

flowchart TD
    A[click / type] --> B[Wait for target]
    B --> C{Request timed out?}
    C -->|yes| D[MM_REQUEST_ABORTED, no action]
    C -->|no| E[Click / tap / type]
    E --> F[Mutating observation]
    F --> G[Wait for every running finite animation, max 1 s]
    G --> H[Read tree]

    classDef new fill:#fff3cd,stroke:#d39e00
    class C,D,G new
Loading

Yellow boxes are new or changed.

What changes

Commit Change
fix: wait for animations that start off screen or at zero size animationsSettled waits for every running, finite animation of an element. Infinite animations are still ignored; the 1 s cap is unchanged.
fix: do not click or type after the request timed out IPlatformDriver.click / type take an optional signal. Both drivers call signal.throwIfAborted() after the target wait, right before the action (mobile type also after the focus tap). The tools pass context.signal and return MM_REQUEST_ABORTED.
fix!: cap run_steps batchTimeoutMs at 30 s Schema range 1000–30000, like the cdp timeoutMs.
docs: … README, SKILL.md and CLI help: refs after --observe none, reads and animations, abort limits, batchTimeoutMs.

Decisions for reviewers

Decision Reason
One new PR instead of changing #62/#64/#65 Pushing to an approved PR dismisses its approval, and the restack would change every PR above it.
An action that already started is not cancelled @metamask/device-mcp takes no signal (waitForElement, tapElement, typeText), and Playwright's actionability checks are part of locator.click. The driver checks the signal between steps. Cancelling inside device-mcp needs a device-mcp change (follow-up).
Cap batchTimeoutMs at 30 s instead of raising the request budget for batches Matches the CLI and queue default; long flows can be split into several batches.
Reads stay without the animation wait (docs only) It is a less exact snapshot, not a wrong action. Can be revisited.
The 5 s testId collection cap stays Agreed with the reviewer: watch real usage first.

Breaking changes

run_steps rejects batchTimeoutMs above 30000 (validation error), also when a server raised requestTimeoutMs.

Review guide

# File What to check
1 src/tools/utils/in-page.ts animationsSettled without the viewport/size filter.
2 src/platform/playwright-driver.ts, src/platform/mobile-platform-driver.ts, src/platform/types.ts throwIfAborted() placement.
3 src/tools/interaction.ts Signal passed; abort mapped to MM_REQUEST_ABORTED.
4 src/validation/schemas.ts batchTimeoutMs range and description.
5 README.md, SKILL.md, src/cli/mm.ts Docs.
Testing

yarn test: 51 files, 1809 tests pass. yarn lint passes. Each commit passes the tests on its own.

Extension (e2e fixture wallet, headless), before = #69, after = this branch:

Check Before After
Drawer sliding in from off screen (400 ms): observationMs 515–516 (did not wait) 914–929 (waited)
Accordion from height 0 (400 ms): observationMs 515–516 916–917
Home click observationMs 513–514 512–515
Send / account list click observationMs 12–183 / 14–164 14–212 / 15–164
run_steps click whose target appears 1 s after the 30 s queue timeout clicked not clicked

Android (device-mcp 0.6.0, real funds, benign taps): all smoke and ref checks pass, same as #69.

iOS (device-mcp 0.6.0, real funds, benign taps): all smoke and ref checks pass, same as #69. (A first attempt hit the stale idb companion after a simulator reboot, dtuhidd connection ... invalidated; fixed by restarting the companion. See MetaMask/device-mcp#43 and #70.)

The browser settle ignored animations whose target was outside the
viewport or had an empty box. A drawer sliding in from the edge, a bottom
sheet sliding up, or an accordion opening from height 0 starts in that
state, so the settle ended on the first frame while the element was still
moving (checked in Chromium: drawer, modal and accordion settled at 0 ms;
a fade-in waited 408 ms).

animationsSettled now waits for every running, finite animation of an
element. Infinite animations (spinners, the fox, skeleton shimmer) are
still ignored, and the 1 s cap is unchanged.

Review: #65 (comment)
clickTool and typeTool checked the request signal only before the target
wait. When the queue timed out during the wait, the agent got "timed out"
but the click or type still ran once the target appeared (most likely in
run_steps, where the whole batch shares one 30 s queue timeout).

IPlatformDriver.click and type take an optional signal. Both drivers call
signal.throwIfAborted() after the target wait, right before the action
(mobile type also checks between the focus tap and typeText). The tools pass
context.signal and return MM_REQUEST_ABORTED when the signal is aborted.

An action that already started is not cancelled. @metamask/device-mcp takes
no signal, so its waitForElement, tapElement and typeText run until they end;
the driver checks between them.

Review: #64 (comment)
batchTimeoutMs accepted up to 300 s, but run_steps has no timeoutMs, so with
the default request limit the queue and the CLI stop the call after 30 s. A
larger batchTimeoutMs could never return its partial results. The schema now
accepts 1000-30000 ms, like the cdp timeoutMs. The deadline is checked
between steps, so partial results need a value below the request limit (the
schema description says so).

BREAKING CHANGE: run_steps rejects batchTimeoutMs above 30000 with a
validation error, also when the server's requestTimeoutMs was raised. Split
longer flows into several batches.

Review: #64 (comment)
- --observe none does not refresh refs. Role/name, DOM-path and mobile label
  refs can match a different element after the screen changed: target by
  testId or run describe-screen first.
- Reads and describe-screen do not wait for animations, so chaining a
  --observe none mutation with a read can catch a screen mid-animation.
- A click or type that timed out during the target wait does not act. An
  action already in progress is not stopped: Playwright's checks inside
  locator.click/fill, and on mobile the device-mcp tap and typeText.
- run_steps batchTimeoutMs: range, checked between steps, 30 s request limit.

Review: #62 (comment)
#62 (comment)
#64 (comment)
#64 (comment)
@cryptotavares
cryptotavares added this pull request to stack #63 October 29, 2026 11:44
@cryptotavares
cryptotavares marked this pull request as ready for review October 29, 2026 12:04
@cryptotavares
cryptotavares requested a review from a team as a code owner October 29, 2026 12:04
@cryptotavares
cryptotavares merged commit f80bddc into main Oct 29, 2026
27 checks passed
@cryptotavares
cryptotavares deleted the cryptotavares/mmcli-p1-33-review-followups branch October 29, 2026 13:52
cryptotavares added a commit that referenced this pull request Oct 29, 2026
> **Summary:** Dispose session-owned platform drivers after consumer
cleanup, including failure and shutdown paths. Mobile drivers forward
disposal to their own device backend so lifecycle-capable backends
release their companions automatically. One server-owned lifecycle
retries failed disposals. Only the current session's failures fail a
call.

Companion implementation:
MetaMask/device-mcp#43.

## Problem

Core calls the consumer's `cleanup()` method to close a testing session.
That contract does not release device-backend resources. Mobile cleanup
can close the app while leaving its IDB companion running.

The consumer can also clear its driver during cleanup. Looking up the
driver afterward loses the resource owner. Cleanup errors create another
gap: releasing the backend must still run when closing the app fails.

```mermaid
sequenceDiagram
    participant C as Core
    participant S as Consumer session manager
    participant D as Mobile driver
    C->>S: cleanup
    S->>S: Close app and clear driver
    S-->>C: Cleanup result
    Note over C,D: No captured driver or disposal hook releases backend resources
```

## Solution

Introduce one server-owned cleanup lifecycle and an optional
platform-driver disposal contract.

1. **Capture the owner.** Read `getPlatformDriver()` before consumer
cleanup can clear the driver.
2. **Keep cleanup order.** Run consumer app/session cleanup first.
Dispose the captured driver afterward, even when cleanup throws.
3. **Retain failed disposals in the server.** `createServer` owns one
`DriverLifecycle`. Tools reach it through
`ToolContext.cleanupSession()`. A later cleanup or daemon stop retries
the retained drivers.
4. **Fail only for the current session.** Consumer cleanup and the
current driver's disposal decide the result. Drivers from earlier
sessions are retried best-effort and in parallel. Their failures go to
the daemon log and do not fail the call.
5. **Use the lifecycle everywhere.** Apply it to cleanup, forced
relaunch, failed launch, daemon stop, and server startup rollback.

```mermaid
flowchart TD
    T[cleanup / launch --force / failed launch] -->|ToolContext.cleanupSession| L[DriverLifecycle, one per server]
    S[Daemon stop] --> L
    R[Startup rollback] --> L
    L --> B[Capture session-owned driver]
    B --> C[Run consumer cleanup]
    C -->|success or failure| D[Dispose captured driver]
    C -->|in parallel| P[Retry older retained drivers, best-effort]
    P -->|failure| W[Keep retained, write WARN to daemon log]
    D --> E{Cleanup or current disposal failed?}
    E -->|no| F[Return consumer cleanup result]
    E -->|yes| G[Retain the failed current driver]
    G --> H[Report current session errors to caller]
    G -.->|next cleanup or stop| P
    W -.->|next cleanup or stop| P
    D --> I[Mobile driver delegates to its backend]

    classDef new fill:#fff3cd,stroke:#d39e00
    classDef stop fill:#f8d7da,stroke:#c82333
    class L,B,D,P,W,G,I new
    class H stop
```

Yellow boxes are new. Red boxes stop successful cleanup completion.

## What changes

| Call or condition | Before | After |
|---|---|---|
| `mm cleanup` | Call consumer cleanup | Capture the driver, clean up
the consumer, then dispose the driver |
| Consumer clears its driver | Core loses access after cleanup | The
captured driver remains available for disposal |
| Consumer cleanup throws | Propagate the cleanup error | Attempt
disposal, then propagate the original or combined errors |
| Current driver disposal fails | No core disposal lifecycle | Fail the
call and retain the driver in the server lifecycle; the next cleanup or
daemon stop retries it |
| Driver from an earlier session still fails | n/a | Retry it
best-effort, in parallel, on every cleanup; log a `[WARN]` line in the
daemon log; do not fail the current cleanup or launch |
| Forced relaunch | Clean up the active session before launch | Also
await old-driver disposal before launching |
| Consumer launch rejects | Return the classified launch error | Attempt
partial-session cleanup and preserve the original launch error |
| Daemon stop or startup rollback | Clean up consumer resources | Also
attempt captured driver disposal and a final best-effort retry of
retained drivers |
| Older backend has no disposal hook | Existing mobile operations |
Mobile disposal completes without a backend call |
| Tool context built outside the daemon | n/a |
`cleanupSession(manager)` falls back to per-manager retention |

## Decisions for reviewers

| Decision | Reason |
|---|---|
| Keep `IPlatformDriver.dispose()` optional | Browser drivers and
existing consumer implementations remain compatible. |
| Capture before consumer cleanup | Consumers can clear their driver
reference as part of resetting session state. |
| The server owns retry state (`DriverLifecycle`), not a map keyed by
the session manager | Retry state must not depend on which wrapper of
the session manager a caller holds. On the Phase 1 stack each request
gets a new wrapper, so a key-based map loses failed disposals after the
request. Cleanup, stop, and rollback must also share one retry list. |
| Optional `ToolContext.cleanupSession()` | Tools use the server
lifecycle without knowing about wrappers. Contexts built by tests or
other hosts still work through the fallback. |
| Preserve launch errors after best-effort rollback | Consumer error
codes remain the actionable launch result, even if rollback also fails.
The rollback error goes to the debug log. |
| Only the current session can fail a call; older retained drivers are
retried best-effort | A driver that keeps failing would otherwise fail
every later cleanup and block `launch --force` permanently. Each retry
can take about 10 s, so serial retries also push cleanup toward the 30 s
request timeout. Parallel retries and daemon-log warnings keep the retry
without blocking the user. |
| Support older device-mcp versions structurally | Core can merge
independently. Companion teardown activates when a consumer supplies a
lifecycle-capable backend. |
| Leave unexposed drivers with the consumer | Core cannot capture a
driver that the consumer never returns. Consumers must dispose those
partial-launch resources locally. |

## Review guide

Read the files in this order:

| # | File | What to check |
|---|---|---|
| 1 | `src/server/cleanup-session.ts` | `DriverLifecycle`: capture
order, error aggregation, retained disposal ownership, fallback for
contexts without a lifecycle |
| 2 | `src/platform/mobile-platform-driver.ts`, `src/platform/types.ts`
| Optional hook, concurrent disposal, and retry behavior |
| 3 | `src/server/create-server.ts`, `src/types/http.ts` | One lifecycle
per server, `ToolContext.cleanupSession`, shutdown and startup rollback
|
| 4 | `src/tools/cleanup.ts`, `src/tools/launch.ts` | Cleanup
integration, forced relaunch ordering, and launch-error preservation |
| 5 | `src/server/session-manager.ts` | Consumer ownership contract |
| 6 | Changed and new tests, `vitest.config.mts` | Lifecycle boundaries,
server-level retry, compatibility, and automatically raised coverage
thresholds |
| 7 | README, agent reference, changelog | Release adoption and consumer
partial-launch responsibilities |

<details>
<summary>Testing</summary>

- `yarn build` passes.
- `yarn lint` passes, including ESLint, formatting, constraints,
dependency checks, and changelog validation.
- `yarn test` passes after merging `main` (Phase 1 stack, #58–#71): 53
files and 1,835 tests. Typechecking and configured coverage thresholds
pass (97.53 / 94.91 / 96.73 / 97.68).
- The package-type check (`attw --pack`) passes for CommonJS, ESM, and
bundler resolution.
- Core's normal test command raises coverage thresholds automatically.
This branch includes that generated update.
- Lifecycle tests cover driver capture before reset, consumer errors,
retained retries, older backends, forced relaunch, failed launch, and
server teardown.
- Server integration tests: a failed `/tool/cleanup` is retried by the
next `/tool/cleanup`, and by `server.stop()`. Both tests fail when the
retry state is keyed by a per-request session-manager wrapper.
- A stuck driver from an earlier session does not fail a later
`/tool/cleanup` or `launch --force`. It is retried on every cleanup, in
parallel with other retained drivers, and `server.stop()` logs it.
- Merged `main` after the Phase 1 stack landed. Conflicts were "keep
both sides": in `mobile-platform-driver.ts`, keep `#excludeDevOverlay`,
`#windowSize` and `#disposing`; in `platform/types.ts`, keep
`dispose?()` and the `click` signal docs; in `create-server.ts` and
`types/http.ts`, keep `signal` and the removal of `knowledgeStore`, and
add `driverLifecycle` and `cleanupSession`.

| Live check with both built branches | Result |
|---|---|
| Cleanup A while B uses another simulator | B continues returning its
expected device ID |
| Cleanup A while B uses the same simulator | B continues returning its
expected device ID |
| Query the disposed backend | Rejects with `IDB backend has been
disposed` |
| Compare companion processes afterward | Both pre-existing companions
remain; all test-owned companions are removed |
| Mobile app on the Phase 1 stack + device-mcp#43: launch/cleanup,
forced relaunch ×2, kill companion, kill daemon, simulator reboot | The
session companion is released on every path; no companion accumulates
(details in device-mcp#43) |

The live checks read the screen and tap non-wallet elements. They do not
submit transactions or exercise wallet flows.

</details>

<details>
<summary>Docs, adoption, and boundaries</summary>

- `README.md` documents session-owned drivers, fresh instances per
launch, the server-owned retry, and consumer-managed partial-launch
failures.
- `SKILL.md` clarifies that `--shutdown` stops the worktree daemon.
Driver disposal does not stop Metro or shut down simulators.
- `CHANGELOG.md` records the lifecycle hook, cleanup entry points, and
server-owned retries.
- Publish the linked device-mcp implementation before consumers adopt
it. This PR keeps the existing device-mcp dependency range.
- The Mobile consumer disposes the iOS driver in its adapter cleanup,
which covers a failed launch where the provider resets its state before
core can see the driver. Disposal is idempotent with core's.
- Consumers that call their own `cleanup()` directly must also manage
driver disposal. Drivers hidden during partial launch remain the
consumer's responsibility.
- Separate companions isolate connection teardown. Sessions targeting
the same simulator still share app state.

</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants