Skip to content

fix: select iOS simulator by installed app#1376

Draft
thymikee wants to merge 2 commits into
mainfrom
agent/fix-1335
Draft

fix: select iOS simulator by installed app#1376
thymikee wants to merge 2 commits into
mainfrom
agent/fix-1335

Conversation

@thymikee

Copy link
Copy Markdown
Member

Summary

Prefer the unique booted iOS simulator that contains the requested app when open has no explicit device selector.
Return actionable errors instead of silently falling back to name ordering when zero or multiple simulators match.

Validation

Automated: focused resolver/platform/open tests, lint, typecheck, build, Fallow, provider integration, and serial reruns of parallel timeout cases.

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.8 MB 1.8 MB +653 B
JS gzip 577.8 kB 578.1 kB +299 B
npm tarball 692.4 kB 692.7 kB +316 B
npm unpacked 2.4 MB 2.4 MB +653 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.9 ms 27.2 ms +0.3 ms
CLI --help 57.3 ms 58.0 ms +0.8 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/context.js +1.0 kB +318 B
dist/src/cli.js -1.7 kB -315 B
dist/src/runtime.js +907 B +254 B
dist/src/tv-remote.js -99 B -52 B
dist/src/internal/daemon.js +98 B +38 B

@thymikee

Copy link
Copy Markdown
Member Author

Review — fix: select iOS simulator by installed app

Reviewed the full diff plus surrounding resolution code. This is a clean, well-scoped fix. Recommend merge with a couple of optional nits below.

What it does

When open <app> has no explicit device selector and 2+ iOS simulators are booted, it probes each booted sim for the requested app and selects the unique match. Zero matches → actionable APP_NOT_INSTALLED; multiple matches → AMBIGUOUS_MATCH with a --udid hint. Previously resolveDevice silently picked the alphabetically-first booted sim (src/kernel/device.ts:291-293), so this replaces a silent wrong-device pick with a correct pick or a clear error.

Strengths

  • Tightly scoped blast radius. findBootedAppleSimulatorWithApp bails early (bootedSimulators.length < 2 → undefined) before any simctl call, so the common single-sim path is untouched and pays zero extra cost. Explicit selectors and lease-backed resolution (allowLocalSimulatorFallback === false) also short-circuit. Good instinct given the perf sensitivity here.
  • Correct new lookup semantics. findIosSimulatorInstalledApp deliberately does not reuse resolveIosApp's bundle-id pass-through — it verifies the bundle id is actually installed. The doc comment earns its place by explaining exactly that difference.
  • The cache-key change is the subtle-but-right part. Stripping appleSimulatorAppTarget from the key means the app-aware pick made at open time is reused by every later resolution in the request scope, then pinned via session.device for follow-up commands. That's why the app target only needs to be threaded at the single open call site rather than all ~12 resolveTargetDevice callers. The invariant is documented and covered by the "reuses an app-aware selection" test.
  • Conventions followed: lazy import('../platforms/apple/core/apps.ts') mirrors the existing findBootableIosSimulator pattern; registered error codes; kernel matchesDeviceSelector/isIosFamily reused rather than re-implemented; distinct candidate lists per error branch (all booted sims for not-installed vs. only matches for ambiguous) is a nice touch.
  • Test coverage is thorough: unique-match, cache reuse, zero-match, ambiguous, explicit-selector-skips-probe, plus a focused unit test for findIosSimulatorInstalledApp (bundle id, alias, stopped-sim no-probe).

Minor / optional

  1. Option name vs. usage (naming nit). session-open.ts passes appleSimulatorAppTarget unconditionally, so on an Android open the call reads resolveTargetDevice({ platform: 'android' }, { appleSimulatorAppTarget: 'Demo' }) (as the updated runtime test now asserts). It's a harmless no-op since only the Apple path reads it, but the apple-prefixed name on an Android call is mildly confusing. Consider a platform-neutral openAppTarget, or only passing it for Apple platforms.
  2. Latent ordering assumption. The cache-key stripping is correct because open performs the app-aware resolution first in its scope. If a future code path resolved a device earlier in the same scope without an app target, it would poison the cache and the later app-aware call would reuse the wrong sim. Worth a one-line note on the assumption, but not actionable now.
  3. Bare-domain edge (very low risk). A positional like open example.com isn't a deep link, so with 2+ booted sims it'd be probed as a bundle id and surface APP_NOT_INSTALLED. Not worth guarding unless it comes up — open targets are bundle-id / app-name / deep-link in practice.

Complexity is appropriate — nothing over-engineered. LGTM. 👍

@thymikee

Copy link
Copy Markdown
Member Author

P1 — The fresh open route bypasses the app-aware selection. runRequestWithinScope opens the target-resolution cache before createRequestExecutionScope; the latter calls resolveRequestExecutionLockKeys, whose advisory fresh-open path first performs a generic resolveTargetDevice(bindingReq.flags). Because this PR removes appleSimulatorAppTarget from the cache key, handleOpenCommand later receives that generic cached simulator and never reaches findBootedAppleSimulatorWithApp. With two booted simulators and the app only on the second, the motivating failure therefore remains.

Please make the cache/order preserve app-aware selection on the real request route and add a regression that covers generic advisory resolution before app-aware resolution—preferably through fresh open request binding. After the code fix, also provide live two-simulator evidence with the app installed on only one; the current PR documents none.

@thymikee

Copy link
Copy Markdown
Member Author

Re-reviewed exact head 4583bf5: the P1 is fixed. Advisory lock selection and handleOpenCommand now use the same app-target options, so the first request-cache entry is app-aware and dispatch correctly reuses it. The fresh-open regression is non-vacuous: reverting either call site produces a generic first resolution and fails the asserted app-aware binding. Existing resolver tests cover unique, absent, and ambiguous matches. I found no remaining code issue, and all reported checks are green.

Readiness still needs practical evidence: please run a fresh open with two booted iOS simulators where the requested app is installed on only one, and show that the session binds to that simulator. The current tests mock resolution/simctl and do not replace the required live proof. No ready-for-human label yet.

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