feat: accept driver instances instead of {type, ...} config objects - #260
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe change replaces tagged driver configuration objects and allocator classes with concrete 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/driver-mobilecli/src/driver.ts (1)
277-300: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep server ownership through
connect().Line 280 clears a handle created by
prepare()when the server is already reachable. A laterdisconnect()ordispose()cannot stop that process.If RPC connection, device resolution, or agent setup fails after this call starts a server,
this.sessionremains unset.disconnect()then cannot clean up the server.Preserve an existing handle when
ensured.serverProcessis undefined. Use a failure cleanup path for the process started by thisconnect()call.Proposed fix
const ensured = await ensureMobilecliReachable(url, { autoStart: this.autoStart, binaryPath: this.mobilecliPath }); - this.ownedServerProcess = ensured.serverProcess; + if (ensured.serverProcess) { + this.ownedServerProcess = ensured.serverProcess; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/driver-mobilecli/src/driver.ts` around lines 277 - 300, Update connect() to preserve this.ownedServerProcess when ensured.serverProcess is undefined, rather than clearing an existing handle created by prepare(). Track the server process started by this connect() call and clean it up if RPC connection, device resolution, or agent setup fails before this.session is assigned, while retaining the successful connection behavior.packages/driver-mobilecli/src/server.ts (1)
100-106: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle invalid configured binary paths.
resolveMobilecliBinary()returns any nonempty explicit path. If the path does not exist or is not executable,spawn()emits an unhandlederrorevent and can terminate the process. Attach anerrorlistener and rejectstartMobilecliServer()with an actionable error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/driver-mobilecli/src/server.ts` around lines 100 - 106, Update startMobilecliServer and its startMobilecliServer({ binaryPath, port }) spawn flow to attach an error listener to the child process, reject the returned promise with an actionable error when the configured binary cannot be found or executed, and ensure the error event is handled without terminating the process.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/src/test/timeouts.md`:
- Around line 144-150: Add the missing defineConfig import to both standalone
MobileNextDriver examples, alongside the existing MobileNextDriver import, so
each example compiles independently.
In `@packages/driver-mobilenext/src/driver.ts`:
- Around line 527-535: Move the fleetSessionBySerial.delete(deviceId) call in
release so it executes only after fleetClient.releaseDevice(sessionId, deviceId)
completes successfully. Preserve the early return when no session mapping exists
and leave the release logging flow unchanged.
- Around line 218-221: Validate the configured apiUrl in the driver
initialization before constructing FleetApiClient, rejecting any non-HTTPS URL
when an apiKey is used. Keep test-only transport overrides separate from
credentialed requests, and ensure invalid credentialed URLs cannot reach the
FleetApiClient constructor.
In `@packages/mobilewright/src/config.test.ts`:
- Line 202: Remove the duplicate opts declarations in the affected test block,
retaining a single typed declaration for uploadEntry![1]. Ensure all existing
uses continue referencing that one opts variable.
In `@packages/mobilewright/src/config.ts`:
- Line 149: Update the captureGitInfo construction in the configuration merge to
preserve existing user options while enabling commit capture: spread the current
captureGitInfo values and then set commit to true when extra.captureGitInfo is
enabled. Keep unrelated configuration fields unchanged.
In `@packages/protocol/src/driver.ts`:
- Around line 176-180: Update DevicePool.startAllocationForWaiter and the
driver.allocate contract to enforce allocationTimeoutMs independently of
AbortSignal handling: document and require drivers to honor signal, race
driver.allocate against the pool timeout, and ensure any device returned after
timeout is released instead of published.
---
Outside diff comments:
In `@packages/driver-mobilecli/src/driver.ts`:
- Around line 277-300: Update connect() to preserve this.ownedServerProcess when
ensured.serverProcess is undefined, rather than clearing an existing handle
created by prepare(). Track the server process started by this connect() call
and clean it up if RPC connection, device resolution, or agent setup fails
before this.session is assigned, while retaining the successful connection
behavior.
In `@packages/driver-mobilecli/src/server.ts`:
- Around line 100-106: Update startMobilecliServer and its
startMobilecliServer({ binaryPath, port }) spawn flow to attach an error
listener to the child process, reject the returned promise with an actionable
error when the configured binary cannot be found or executed, and ensure the
error event is handled without terminating the process.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9d7e4f7-8bb5-476f-9bb0-28031fea2e8d
📒 Files selected for processing (34)
docs/src/guides/docker.mddocs/src/test/configuration.mddocs/src/test/timeouts.mde2e/mobilewright.config.tse2e/package.jsone2e/tsconfig.jsonpackages/driver-mobilecli/src/driver.tspackages/driver-mobilecli/src/index.tspackages/driver-mobilecli/src/resolve-binary.tspackages/driver-mobilecli/src/server.tspackages/driver-mobilenext/src/driver.tspackages/mobilewright-core/src/device.test.tspackages/mobilewright-core/src/expect.test.tspackages/mobilewright-core/src/locator.test.tspackages/mobilewright-core/src/screen.test.tspackages/mobilewright/src/cli.tspackages/mobilewright/src/config.test.tspackages/mobilewright/src/config.tspackages/mobilewright/src/device-pool/adapters/http-client.test.tspackages/mobilewright/src/device-pool/adapters/http-server.test.tspackages/mobilewright/src/device-pool/adapters/mobilecli-allocator.test.tspackages/mobilewright/src/device-pool/adapters/mobilecli-allocator.tspackages/mobilewright/src/device-pool/adapters/mobilenext-allocator.test.tspackages/mobilewright/src/device-pool/adapters/mobilenext-allocator.tspackages/mobilewright/src/device-pool/allocator-factory.tspackages/mobilewright/src/device-pool/application/device-pool.test.tspackages/mobilewright/src/device-pool/application/device-pool.tspackages/mobilewright/src/device-pool/application/ports.tspackages/mobilewright/src/device-pool/setup.tspackages/mobilewright/src/index.tspackages/mobilewright/src/launchers.tspackages/protocol/src/driver.tspackages/protocol/src/index.tspackages/test/src/fixtures.ts
💤 Files with no reviewable changes (5)
- packages/mobilewright/src/device-pool/adapters/mobilecli-allocator.test.ts
- packages/mobilewright/src/device-pool/adapters/mobilecli-allocator.ts
- packages/mobilewright/src/device-pool/allocator-factory.ts
- packages/mobilewright/src/device-pool/adapters/mobilenext-allocator.test.ts
- packages/mobilewright/src/device-pool/adapters/mobilenext-allocator.ts
| expect(driver.testResult?.environment).toBe('staging'); | ||
| const reporters = config.reporter as Array<[string, unknown]>; | ||
| const uploadEntry = reporters.find(([path]) => String(path).includes('reporter')); | ||
| const opts = uploadEntry![1] as { testResult: { uploadReport: string; name: string; tags: string[]; environment: string } }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate opts declarations.
Line 202 declares const opts multiple times in the same block. TypeScript will reject the test file with a block-scoped variable redeclaration error. Keep one declaration.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/mobilewright/src/config.test.ts` at line 202, Remove the duplicate
opts declarations in the affected test block, retaining a single typed
declaration for uploadEntry![1]. Ensure all existing uses continue referencing
that one opts variable.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/mobilewright/src/device-pool/application/device-pool.ts`:
- Around line 159-182: In the allocation error path around the awaited
driver.allocate call, remove the reserved slot by its slot object identity
rather than the stale slotIndex captured before await. Locate the slot created
for this waiter, find its current index after the await, and splice only that
matching slot while preserving the existing waiter cleanup and timeout handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ce9d7b4-99bb-45ff-ad7f-b5a3ecd11b73
📒 Files selected for processing (5)
docs/src/test/timeouts.mdpackages/driver-mobilenext/src/driver.tspackages/mobilewright/src/config.tspackages/mobilewright/src/device-pool/application/device-pool.tspackages/protocol/src/driver.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/src/test/timeouts.md
- packages/protocol/src/driver.ts
- packages/mobilewright/src/config.ts
- packages/driver-mobilenext/src/driver.ts
1 participant
Summary
defineConfig({ driver })now takes a driver instance (e.g.new MobileNextDriver({ apiKey })) instead of a{ type: 'mobilenext', ... }config object. Omitdriverentirely for theMobilecliDriverdefault.MobilewrightDriver(in@mobilewright/protocol) gainedallocate/release— the separateDeviceAllocator/MobilecliAllocator/MobileNextAllocatorlayer and the threedriver.type-switch sites (allocator-factory.ts,launchers.ts×2) are gone. A driver package now only needs to implement one interface.MobilewrightDriver:prepare()/dispose()for driver-owned lifecycle (mobilecli's local server auto-start/kill now lives insideMobilecliDriveritself), andconfigureReporting()for drivers that want to auto-inject a reporter (mobile-next's upload reporter, previously wired via adriver.type === 'mobilenext'check in coreconfig.ts).url/autoStart/mobilecliPathmoved off the top-level config and intoMobilecliDriver's own constructor options;mobilecliPathis now actually wired through to binary resolution (previously declared but unused).regionoption onMobileNextDriver— confirmed unused anywhere in the codebase.inittemplate, both e2e configs, and three docs pages to the new syntax.e2e/tsconfig.jsonwas missing project references fordriver-mobilecli/driver-mobilenext(needed once the e2e config started importing them directly), ande2e/mobilewright.config.ts'stimeout: 60_000was too short for real cloud device provisioning (~74-120s), causing every test to time out mid-allocation.Test plan
npm run build— cleannpm run lint— cleannpm test— 551 passed, 1 skippede2e/,test:mobilenext): allocation → connect → WebSocket → RPC → real device control all confirmed working; remaining failures in that run are pre-existing e2e test/environment gaps (missing app install on cloud device), unrelated to this change