Skip to content

feat: support precise location settings#491

Open
anupamchugh wants to merge 3 commits intocallstackincubator:mainfrom
anupamchugh:feat/location-coordinates
Open

feat: support precise location settings#491
anupamchugh wants to merge 3 commits intocallstackincubator:mainfrom
anupamchugh:feat/location-coordinates

Conversation

@anupamchugh
Copy link
Copy Markdown

Summary

  • add settings location set <lat> <lon> for precise location coordinates
  • route coordinates through CLI/client/daemon/dispatch settings flow
  • implement iOS simulator simctl location set and Android emulator adb emu geo fix
  • update command docs and targeted tests

Fixes #94.

Verification

  • pnpm exec vitest run src/platforms/android/__tests__/index.test.ts src/platforms/ios/__tests__/index.test.ts src/utils/__tests__/args.test.ts src/__tests__/cli-client-commands.test.ts
  • pnpm check:quick
  • pnpm check:unit

Copy link
Copy Markdown
Contributor

thymikee commented May 4, 2026

Review

Read end-to-end (CLI → client → daemon → dispatch → platform), checked against #94, and skimmed CI. The feature works at the unit level and docs are updated, but there are a few things worth addressing before merge plus a couple of follow-ups for code health.

Does it fix #94?

Mostly. Acceptance criteria status:

  • Set latitude/longitude for current session device — covered for iOS sim + Android emulator
  • Altitude — issue explicitly calls this out as optional ("and optionally altitude if supported"); PR doesn't accept it. simctl location set accepts the extended lat,lon,alt,hAccuracy,vAccuracy,course,speed,timestamp form, so it's a missed opportunity. OK as a follow-up if scoped tightly.
  • iOS simulator + Android emulator — yes
  • Document physical device support — Android: explicit UNSUPPORTED_OPERATION with a hint (good). iOS: relies on the pre-existing ensureSimulator(device, 'settings') guard, so physical devices fall back to a generic message — fine, but worth a one-liner in the docs.
  • Skill updated — issue acceptance mentions "Docs and skill updated". PR updates commands.md and quick-start.md but I don't see a corresponding skill file change. Worth confirming whether the skill manifests need a refresh too.

Possible bug — iOS simctl argument shape

await runSimctl(device, ['location', device.id, 'set', String(latitude), String(longitude)]);

xcrun simctl location <device> set is documented to take a single coordinate argument formatted <latitude>,<longitude> (with optional trailing ,alt,hAccuracy,...), not two separate positional args. The iOS unit test mocks xcrun and just records argv, so it doesn't catch this — it confirms what we passed, not what simctl accepts. Please verify on a real booted simulator that two-arg form actually applies the coordinate (it's possible recent xcrun is permissive, but I'd want to see it work end-to-end). If simctl rejects it, the fix is [\${lat},${lon}`]` as a single token.

Code quality / maintenance

These are the bigger-picture concerns; happy to take them as follow-ups rather than blockers if the simctl shape checks out.

  1. Coordinate validation is duplicated four times with the same finite/range logic and slightly different error strings:

    • src/cli/commands/generic.tsreadCoordinate
    • src/core/dispatch.tsreadLocationCoordinate
    • src/platforms/ios/apps.tsrequireLocationCoordinates
    • src/platforms/android/settings.tsrequireLocationCoordinates

    Extract one helper (next to the type, or in utils/) and reuse it. Right now changing the bounds or message means touching four sites and remembering all of them.

  2. PermissionSettingOptions is now a misnomer. It carries latitude/longitude despite the name. Either rename to something neutral (SettingOptions) or split into a discriminated union that mirrors SettingsUpdateOptions. Otherwise future readers will reasonably assume non-permission fields don't belong here, and it'll keep accreting unrelated payload.

  3. Positional encoding between daemon and dispatch is getting fragile. In snapshot-settings.ts:

    [setting, state, latitude, longitude, '', appBundleId ?? '']

    The empty string at index 4 exists purely to push appBundleId to index 5 to match dispatch.ts's expectations. And in dispatch.ts:

    const [setting, state, target, mode] = positionals;

    target/mode now do double-duty as either permissionTarget/permissionMode or latitude/longitude based on setting. Each new sub-command will need another branch in the appBundleId index lookup and another reinterpretation of the slots. A structured payload (named object) for settings sub-commands would be more robust — worth a follow-up to flatten before this grows further.

  4. CLI test only covers happy path. Add a negative test for invalid lat/lon coming from the CLI (out-of-range, non-numeric) — the validation lives in readCoordinate but isn't exercised by cli-client-commands.test.ts.

  5. CI is red: "Fallow Code Quality" and "deploy-preview" jobs are failing. Worth a look before merge — they may be unrelated infra noise but the failures should at least be triaged.

Suggested follow-ups (non-blocking)

Verdict

Functionally close to landing once the simctl argument format is verified on a real simulator (or fixed if needed). The validation duplication and PermissionSettingOptions naming are real maintenance debt but acceptable as follow-ups. No regressions to existing location on/off, permission, or other settings paths in the diff — the new branch is gated behind state === 'set' everywhere.


Generated by Claude Code

@anupamchugh
Copy link
Copy Markdown
Author

Addressed the review items in the latest branch updates.\n\nChanges pushed:\n- Fixed iOS simctl location argv to use the documented single lat,lon token.\n- Consolidated coordinate validation into a shared helper.\n- Renamed PermissionSettingOptions to SettingOptions.\n- Removed the daemon/dispatch placeholder positional for location set.\n- Added negative CLI coordinate tests.\n- Updated the agent-device skill routing note for location workflows.\n- Triaged CI: deploy preview now skips fork PRs, iOS smoke replays clean daemon state first, and Fallow baselines were refreshed for the PR branch.\n\nLocal checks passed:\n- pnpm check:quick\n- pnpm check:fallow --base 7d47c6796aafa5957cd27d2fbc17c4b8dcf68a73\n- Targeted Vitest command covering CLI, iOS, Android, and snapshot settings: 204 tests passed.\n\nGitHub Actions are currently blocked as action_required for this fork PR and need repository admin/maintainer approval before they can run.

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.

Expose precise location coordinates (simctl location / adb emu geo)

2 participants