Sign up
View stargazersSign up

refactor(server): device hosts own their device operations#21835

merged1
main
device/host-owns-device-ops
opened20d agobyJJulius Marminge
9 files changed
View on GitHubTwo-way sync enabledPR last imported 19d ago

DeviceService lists, boots, attaches streams to, shuts down, and screenshots devices by calling expo-device-hub's HTTP API directly. That ties every device host to running expo-device-hub, which blocks hosts whose devices live somewhere else, such as EAS Simulator sessions.

This moves those calls behind a devices field on DeviceHostReady (list, boot, attach, shutdown, screenshot):

  • DeviceHubOperations.ts is the hub-backed implementation, moved unchanged out of DeviceService: the same routes, timeouts, boot-error classification, AVD listing (a failed emulator -list-avds stays a warning with androidDiscoveryIncomplete, as #19974 made it), and iOS shutdown fallback. The local and SSH hosts both build their operations from it.
  • DeviceService calls ready.devices.* and no longer needs an HttpClient.
  • The SSH host's platformAvailability is hoisted unchanged so its hub operations can keep re-probing Android before listing AVDs.

No behavior change. Part of built-in EAS Simulator support (stack #21847). Layer 1 of 6: the EAS host in #21844 supplies its own operations.

Testing: the device and MCP device suites pass (14 files, 106 tests), and the server typecheck passes. The existing DeviceService tests still drive the hub routes through a fake hub, now via the host's operations, so they cover the moved code.

Done with Claude Opus 5.5 in Claude Code, running in T3 Code.

🤖 Generated with Claude Code

Checks

1 failing · 21 successful · 10 skipped · 2 neutral
Some checks failed

Failing1

dc960a8
Ggithub-actionsBot20d ago
Synced from GitHub

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ProviderMetricMain baselineThis PRImpactPR ceiling
CodexTotal thread wire5.0 KiB5.0 KiB+7 B (+0.1%)6.8 KiB✅
CodexThread snapshot wire3.8 KiB3.8 KiB0 B (0.0%)4.9 KiB✅
CodexLive turn WebSocket wire1.2 KiB1.2 KiB+7 B (+0.6%)2.0 KiB✅
CodexLive turn WebSocket decoded20.9 KiB20.9 KiB0 B (0.0%)29.3 KiB✅
CodexLive turn messages220 (0.0%)8✅
ClaudeTotal thread wire5.0 KiB5.0 KiB+7 B (+0.1%)6.8 KiB✅
ClaudeThread snapshot wire3.8 KiB3.8 KiB0 B (0.0%)4.9 KiB✅
ClaudeLive turn WebSocket wire1.2 KiB1.2 KiB+7 B (+0.6%)2.0 KiB✅
ClaudeLive turn WebSocket decoded21.2 KiB21.2 KiB0 B (0.0%)29.3 KiB✅
ClaudeLive turn messages220 (0.0%)8✅

Baseline: 9482773 · PR result: dc960a8 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

Added 1 commit

  • refactor(server): device hosts own their device operationsdc960a8
apps/server/src/device/DeviceHubOperations.tsline 50Open thread
……@@ -0,0 +1,247 @@
47+ readonly hostId: DeviceHostId;
48+ readonly origin: string;
49+ readonly http: HttpClient.HttpClient;
50+ readonly run: DeviceHostReady["run"];
MmacroscopeappBoton line 5020d ago

This factory takes HttpClient.HttpClient, an Effect service dependency, as an explicit parameter. Could the hub operations acquire it from the Effect environment instead, with the host construction exposing that requirement through its make/layer? Keep the origin and host ID as explicit configuration.

Posted via Macroscope — Effect Service Conventions

JJulius Marminge20d ago

The host's make is where the requirement lives: LocalDeviceHost.make and SshDeviceHost.make both yield* HttpClient.HttpClient from the environment and hand it to the operations they build for each running hub (LocalDeviceHost.ts:274, SshDeviceHost.ts:129). makeDeviceHubOperations is an internal helper that is called synchronously from toReady, not a service with a layer, so making it effectful would only move the same yield* into toReady's three call sites and the test fixtures. Keeping the client as an explicit input of the helper, with the hosts exposing the requirement, matches how it was wired before the move out of DeviceService. Leaving as is.

MmacroscopeappBot20d ago
Synced from GitHub

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This PR moves device discovery and lifecycle behavior into a new host-owned operations abstraction and rewires local and SSH production paths around it. The implementation largely extracts existing hub behavior, but the shared architectural boundary and all device operations are materially affected.

You can add or adjust custom eligibility rules. Learn more.

CcoderabbitaiBot20d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Team
  • Run ID: 81bd132b-46a6-4860-b2ba-11a26dbd1f2e
📥 Commits

Reviewing files that changed from the base of the PR and between 94827737ea018902dd2e932cddb51643365dc446 and dc960a82411acc13530c0d2da75fa7d3f4633262.

📒 Files selected for processing: 9
  • apps/server/src/device/DeviceActions.test.ts
  • apps/server/src/device/DeviceHost.ts
  • apps/server/src/device/DeviceHubOperations.ts
  • apps/server/src/device/DeviceMultiHost.test.ts
  • apps/server/src/device/DeviceService.test.ts
  • apps/server/src/device/DeviceService.ts
  • apps/server/src/device/LocalDeviceHost.ts
  • apps/server/src/device/SshDeviceHost.ts
  • docs/internals/devices.md

Limit details: You’ve used all 10 included reviews currently available.

📝 Walkthrough

Device operations now belong to the ready host interface. Local and SSH hosts provide hub-backed operations, and DeviceService delegates device discovery and lifecycle actions through that interface.

Changes: Device operation ownership

Layer / File(s)Summary
Define and implement host device operations
apps/server/src/device/DeviceHost.ts, apps/server/src/device/DeviceHubOperations.ts, apps/server/src/device/DeviceActions.test.ts		DeviceHostReady exposes a device operations interface. The hub-backed implementation provides listing, boot, attach, shutdown, and screenshot operations.
Wire host implementations
apps/server/src/device/LocalDeviceHost.ts, apps/server/src/device/SshDeviceHost.ts, apps/server/src/device/DeviceMultiHost.test.ts, docs/internals/devices.md		Local and SSH hosts provide hub-backed device operations. Multi-host tests and the architecture documentation reflect host-owned operations.
Delegate service operations to the host
apps/server/src/device/DeviceService.ts, apps/server/src/device/DeviceService.test.ts		DeviceService delegates discovery and device lifecycle actions to the ready host. Tests configure device operations on the host fixture.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Refactor

Sequence Diagram(s)

mermaid
1sequenceDiagram
2 participant DeviceService
3 participant DeviceHostReady
4 participant DeviceHubOperations
5 participant expo-device-hub
6 DeviceService->>DeviceHostReady: delegate device operation
7 DeviceHostReady->>DeviceHubOperations: invoke operation
8 DeviceHubOperations->>expo-device-hub: send hub request
9 expo-device-hub-->>DeviceHubOperations: return hub response
10 DeviceHubOperations-->>DeviceService: return operation result

Suggested reviewers: tris203, maria-rcks

Merge Risk: ⚪ Minimal · up to dc960

The device-operation refactor is mergeable after normal checks; no concrete behavior regression was identified.

Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check nameStatusExplanationResolution
Description checkWarningThe description clearly explains the problem, implementation, scope, and verification results. However, it does not provide the required Scope and approval section or explicit maintainer approval or a…Add a Scope and approval section. Include the triaged issue or discussion with explicit maintainer approval, or explain why this focused refactor qualifies for an exemption. Preserve the existing verification details and state any checks no…
Passed Passed checks: 3 passed
Check nameStatusExplanation
Title checkPassedThe title clearly and concisely describes the main refactor: device hosts now own device operations.
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description clearly explains the problem, implementation, scope, and verification results. However, it does not provide the required Scope and approval section or explicit maintainer approval or an exemption rationale.

Resolution

Add a Scope and approval section. Include the triaged issue or discussion with explicit maintainer approval, or explain why this focused refactor qualifies for an exemption. Preserve the existing verification details and state any checks not run if applicable.

  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Usage-based review receipt

Note

This review was completed with usage-based billing: files reviewed beyond your plan's included limits are billed at $0.25/file. View usage-based billing.

Comment @coderabbitai help to get the list of available commands.