Skip to content

perf: settle only on visible finite animations; collect testIds in one call - #65

Draft
cryptotavares wants to merge 3 commits into
cryptotavares/mmcli-p1-08-abort-refsfrom
cryptotavares/mmcli-p1-10-ext-speedups
Draft

cryptotavares wants to merge 3 commits into
cryptotavares/mmcli-p1-08-abort-refsfrom
cryptotavares/mmcli-p1-10-ext-speedups

Conversation

@cryptotavares

@cryptotavares cryptotavares commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Summary: Reduce browser observation work with a bounded animation wait and one in-page testId collection call. Run both functions in a cached CDP isolated world, so Extension page-global restrictions do not break them.

Stack context: mm CLI v2, Phase 1, P1-10 + P1-12, combined in one PR. Base: cryptotavares/mmcli-p1-08-abort-refs; review #64 first. Followed by #66.

Problem

Browser observations wait for every running animation, including spinners that never finish. The old main-world check also depends on globals that LavaMoat can restrict. Hidden tabs do not provide the animation frames that the check needs.

TestId collection adds three Playwright calls per element: getAttribute, isVisible, and textContent. Extension home therefore needs many round trips before the accessibility snapshot starts.

These costs share the observation budget. The historical proof below records a first Send observation that times out without testIds. This PR changes the browser collection boundary used by observations and PlaywrightPlatformDriver.getTestIds.

Solution

Keep browser orchestration in the daemon. Move DOM inspection into self-contained functions that execute in the main frame's isolated world.

flowchart TD
    A[Browser observation] --> B{Read-only or discovery tool?}
    B -->|no| C[Wait for finite, in-viewport animations]
    B -->|yes| D[Collect testIds in cached isolated world]
    C --> D
    D -->|success| E[Map items to existing output shape]
    D -->|failure or timeout| F[Collect testIds through locator fallback]
    E --> G[Collect accessibility tree]
    F --> G
    G --> H[Return observation]

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

Yellow boxes are new.

callInIsolatedWorld owns the CDP session and execution context. A WeakMap caches them per page. Steady-state calls send one Runtime.callFunctionOn request. Protocol or session failures clear the matching cache entry and retry once. In-page exceptions propagate without a retry.

The isolated world shares the DOM but has separate globals. Page functions travel as source text, so they cannot import helpers or capture module state. Their helpers stay inside each function. The source wrapper supplies __name for keepNames-transformed functions; VM tests check the serialization boundary.

discovery.ts owns the failure policy. Animation timeouts and unreadable pages end the wait. TestId call failures and timeouts use the locator fallback. The server still checks its abort signal between observation stages.

What changes

Condition or boundary Before After
Running animations Every running animation delays collection Only finite animations with non-empty target boxes intersecting the viewport delay collection
Animation wait budget 3000 ms configured wait 1000 ms in-page cap, with 250 ms daemon grace
Hidden page Animation-frame polling only Poll with a 16 ms timer while hidden
TestId collection Three locator calls per element One in-page call in steady state; locator fallback on failure or a 5 s call timeout
TestId output Items carry testId, tag, text, and visibility Same shape, order, limit, and text-preview rules; tag stays element
Page navigation or closed CDP session No shared isolated-world cache Drop the failed entry and retry once; late failures cannot replace a newer entry

Collection includes hidden testIds with visible: false. Visibility follows Playwright's rules, including display: contents, non-empty boxes, and computed visibility. Zero opacity and off-screen position do not hide an otherwise visible item.

The collector visits document matches before open shadow-root matches. It skips empty testIds without consuming the limit. Text remains trimmed and capped at 100 characters; null text maps to undefined at the daemon boundary.

Decisions for reviewers

Decision Reason
Combine P1-10 and P1-12 Both improvements need the same isolated-world execution boundary.
Cache one session and world per page Avoid creating more execution contexts on each call to a long-lived Extension page.
Keep page functions self-contained and exclude them from coverage instrumentation Injected counters or module helpers would break source-text execution. VM tests exercise that boundary directly.
Keep locator fallback and fail-open settling Observation collection remains best-effort when CDP or page inspection fails. The 5 s cap covers the in-page call, not the fallback.
Preserve the separate accessibility snapshot This PR targets animation settling and testIds. P1-14 addresses the remaining snapshot cost identified in the historical proof.

Review guide

Read the files in this order:

# File What to check
1 src/server/create-server.ts snapshotBrowser keeps category-based settling, collection order, and abort checkpoints.
2 src/tools/utils/constants.ts Animation cap, daemon grace, and testId-call budget.
3 src/tools/utils/cdp.ts Per-page caching, stale-entry reset guard, single retry, exception handling, and timeout cleanup.
4 src/tools/utils/in-page.ts Animation eligibility, hidden-page polling, visibility semantics, shadow-root order, and source-text independence.
5 src/tools/utils/discovery.ts Output mapping, locator fallback, and animation failure policy.
6 src/tools/utils/{cdp,in-page,discovery}.test.ts, src/server/create-server.test.ts Cache lifecycle, serialized functions, fallback, timeout, and observation integration.
Testing
  • Fresh check at remote head 50eb3962b30ee8f5e2605dfcf307417e0282f560: CI=true yarn vitest run src/tools/utils/cdp.test.ts src/tools/utils/in-page.test.ts src/tools/utils/discovery.test.ts src/server/create-server.test.ts src/platform/playwright-driver.test.ts src/tools/discovery-tools.test.ts --coverage.enabled=false --maxWorkers=2 --minWorkers=1 passes: 6 files, 317 tests.
  • yarn lint passes. yarn tsc --project tsconfig.build.json --noEmit passes for production sources.
  • Standalone yarn tsc --project tsconfig.json --noEmit fails at the remote head with 11 test diagnostics. One is the new in-page.test.ts:728 type predicate (TS2677); ten come from inherited test code.
  • The server suite emits an expected EACCES log from its read-only log-directory fixture and passes.
  • The Proof and Unit tests sections below retain the original historical evidence. These fresh checks do not repeat the live browser benchmarks or full coverage run.

Breaking changes

None. The constants are internal. Observations may now be taken while an infinite or off-screen animation is still running; that is intended.

Consumer impact

Consumer Change needed Done on branch
Extension test/e2e/playwright/llm-workflow/ None n/a
Mobile tests/llm-workflow/ None (browser-only code paths) n/a

Proof

Tested with a yalc build of this branch in the Extension (e2e build, headless). I ran a fresh before/after bench back to back: before = #64 (7d0c565), after = this branch. Every bench has 5 runs, and the 1-minute host load was 6–9. After a review round I re-checked the final commit (50eb396) and got the same numbers.

Check Before (#64) After
list-testids on home, toolMs p50/p95 (44 items) 53 / 61 1 / 1
list-testids JSON, home + unlock – same items, order, tags and visibility. Only difference: the local chain port in 3 text values, which also changes between two runs of the same build
Click on home (Tokens tab), observationMs p50/p95 562 / 571 515 / 517
Click on Send page, observationMs p50/p95 21 / 32 13 / 13
Click on account list, observationMs p50/p95 529 / 532 513 / 515
First Send click of a session, observationMs 8003 (timeout, no testIds) 43 (11 testIds)
Send → back smoke, 5 runs 5/5, but 4/5 Send opens returned no testIds 5/5, all with testIds (3 runs total)

The remaining ~515 ms on home is the accessibility-tree snapshot: get-state --observe full takes ~509 ms without any settle. The settle and testIds now add about 6 ms together. P1-14 targets that snapshot.

Known gap: the locator fallback logs only through debugWarn, so it is visible only with DEBUG=true. In the recheck it never ran: every list_testids took ~1–2 ms; the fallback takes ~50 ms.

Unit tests
Test Files  51 passed (51)
     Tests  1610 passed (1610)
All files | 97.31 | 94.18 | 96.35 | 97.47
yarn lint → exit 0

New tests:

  • in-page.test.ts:
    • each function is compiled from its own source text in a bare vm context, so a stray import or closure fails the test;
    • a guard checks that toString() has no bundler helpers other than __name;
    • covers the settle predicate (infinite, off-viewport, no target, hidden page) and visibility (display: contents, visibility, zero box), text, limit and shadow-root order.
  • cdp.test.ts: session/world caching, retry after navigation, in-page exceptions, timeout.
  • discovery.test.ts: the single call, plus fallback to locators on failure and on timeout.

Tickets P1-10 + P1-12.

- Animation settle: wait only for running, finite animations whose target
  element is in the viewport (spinners, shimmer and other infinite or
  off-screen animations no longer hold the observation). Cap 1000 ms
  (was 3000 ms). Hidden pages poll with setTimeout (rAF does not fire).
- collectTestIds: one in-page call instead of getAttribute + isVisible +
  textContent per element. Same output shape and order; visibility follows
  Playwright's isVisible (non-empty box, visibility visible,
  display: contents via children; opacity 0 counts as visible). Times out
  after 5 s and falls back to the old per-element locator loop on any
  failure.
- Both in-page functions run in a CDP isolated world of the main frame
  (Page.createIsolatedWorld + Runtime.callFunctionOn), which shares the DOM
  but not page globals, so LavaMoat scuttling cannot break them. One session
  and world per page, recreated once after navigation.

Extension bench (p50): list-testids toolMs 53 -> 1; home click
observationMs 562 -> 515; first Send click observation 8003 ms (timed
out, no testIds) -> 43 ms.

This branch has not been deployed

No deployments
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.

1 participant