refactor: unify error handling across all Robot implementations #245
Conversation
Add consistent error handling pattern to AndroidRobot, IosRobot, and MobileDevice classes using new shared utility functions. Changes: - Add extractCommandError() to extract user-friendly messages from child process errors (stdout/stderr) - Add withActionableError() wrapper for consistent error handling that converts all errors to ActionableError with context - Update all Robot interface method implementations to use the new error handling utilities - Improve error messages with contextual information (e.g., device coordinates, package names, file paths) This ensures all device operations provide clear, actionable error messages regardless of the underlying platform (Android/iOS/simulator). Files changed: - src/robot.ts: Add error handling utilities - src/android.ts: Apply consistent error handling to all methods - src/ios.ts: Apply consistent error handling to all methods - src/mobile-device.ts: Apply consistent error handling to all methods
WalkthroughThis PR introduces standardized error handling across device control modules by implementing a Changes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mobile-device.ts (1)
85-116: Inconsistent error handling compared to other methods.Unlike other methods in this file, the
swipemethod doesn't wrap therunCommandcall (line 115) inwithActionableError. This creates inconsistency with the rest of the implementation where all command executions are wrapped for uniform error handling.🔎 Proposed fix
public async swipe(direction: SwipeDirection): Promise<void> { - const screenSize = await this.getScreenSize(); - const centerX = Math.floor(screenSize.width / 2); - const centerY = Math.floor(screenSize.height / 2); - const distance = 400; // Default distance in pixels - - let startX = centerX; - let startY = centerY; - let endX = centerX; - let endY = centerY; - - switch (direction) { - case "up": - startY = centerY + distance / 2; - endY = centerY - distance / 2; - break; - case "down": - startY = centerY - distance / 2; - endY = centerY + distance / 2; - break; - case "left": - startX = centerX + distance / 2; - endX = centerX - distance / 2; - break; - case "right": - startX = centerX - distance / 2; - endX = centerX + distance / 2; - break; - } - - this.runCommand(["io", "swipe", `${startX},${startY},${endX},${endY}`]); + return withActionableError(async () => { + const screenSize = await this.getScreenSize(); + const centerX = Math.floor(screenSize.width / 2); + const centerY = Math.floor(screenSize.height / 2); + const distance = 400; // Default distance in pixels + + let startX = centerX; + let startY = centerY; + let endX = centerX; + let endY = centerY; + + switch (direction) { + case "up": + startY = centerY + distance / 2; + endY = centerY - distance / 2; + break; + case "down": + startY = centerY - distance / 2; + endY = centerY + distance / 2; + break; + case "left": + startX = centerX + distance / 2; + endX = centerX - distance / 2; + break; + case "right": + startX = centerX - distance / 2; + endX = centerX + distance / 2; + break; + } + + this.runCommand(["io", "swipe", `${startX},${startY},${endX},${endY}`]); + }, `Failed to swipe ${direction}`); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/android.tssrc/ios.tssrc/mobile-device.tssrc/robot.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/robot.ts (1)
src/logger.ts (1)
error(19-21)
src/ios.ts (1)
src/robot.ts (1)
withActionableError(61-71)
src/mobile-device.ts (1)
src/robot.ts (5)
withActionableError(61-71)InstalledApp(10-13)Button(17-17)ScreenElement(26-37)Orientation(73-73)
src/android.ts (1)
src/robot.ts (4)
withActionableError(61-71)InstalledApp(10-13)SwipeDirection(15-15)Orientation(73-73)
🔇 Additional comments (7)
src/robot.ts (2)
42-43: LGTM! Good practice for custom error classes.Setting the
nameproperty helps with error identification and debugging.
46-71: LGTM! Solid error handling implementation.The utilities correctly extract error messages from child process errors and provide consistent error wrapping with contextual information.
src/ios.ts (1)
128-139: LGTM! Proper async/await usage.The
listAppsmethod correctly awaits the async operation withinwithActionableError.src/mobile-device.ts (2)
76-82: LGTM! Proper error wrapping.The method correctly wraps the synchronous operations in
withActionableError.
119-140: LGTM! Consistent error handling across all methods.All these methods correctly apply
withActionableErrorwith appropriate contextual error messages.Also applies to: 144-147, 151-157, 161-164, 168-171, 175-178, 182-185, 189-192, 196-199, 203-206, 210-213, 223-226, 230-242, 246-249, 253-256
src/android.ts (2)
103-116: LGTM! Good error handling and validation.The method wraps the screen size retrieval with proper error handling and includes validation for missing output.
120-133: LGTM! Consistent error handling across all Android operations.All methods correctly apply
withActionableErrorwith contextual error messages. The implementation maintains consistency with the iOS and mobile-device implementations.Also applies to: 146-149, 162-194, 198-235, 302-317, 360-365, 369-372, 376-379, 383-386, 390-393, 447-450, 454-458, 468-474, 478-481
| await withActionableError( | ||
| () => { this.ios("launch", packageName); }, | ||
| `Failed to launch app "${packageName}". Please make sure it exists` | ||
| ); |
There was a problem hiding this comment.
Missing await causes error handling to fail.
The this.ios() method is async (returns Promise<string> as defined on line 94), but it's called without await inside the operation. This means:
- The operation completes immediately without waiting for the command to finish
- Errors from the command won't be caught by
withActionableError - The method may return before the actual operation completes or fails
🔎 Proposed fix
- await withActionableError(
- () => { this.ios("launch", packageName); },
- `Failed to launch app "${packageName}". Please make sure it exists`
- );
+ await withActionableError(
+ async () => { await this.ios("launch", packageName); },
+ `Failed to launch app "${packageName}". Please make sure it exists`
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await withActionableError( | |
| () => { this.ios("launch", packageName); }, | |
| `Failed to launch app "${packageName}". Please make sure it exists` | |
| ); | |
| await withActionableError( | |
| async () => { await this.ios("launch", packageName); }, | |
| `Failed to launch app "${packageName}". Please make sure it exists` | |
| ); |
🤖 Prompt for AI Agents
In src/ios.ts around lines 144-147, the arrow callback passed to
withActionableError uses a block but does not return or await the async
this.ios(...) call, so the operation completes immediately and errors escape
handling; change the callback to return the Promise (either by removing the
braces for an implicit return: () => this.ios("launch", packageName) or
explicitly return/await it: async () => await this.ios("launch", packageName))
so the Promise is awaited and errors are caught by withActionableError.
| await withActionableError( | ||
| () => { this.ios("kill", packageName); }, | ||
| `Failed to terminate app "${packageName}"` | ||
| ); |
There was a problem hiding this comment.
Missing await causes error handling to fail.
Same issue as launchApp - this.ios() is async but not awaited, breaking error handling.
🔎 Proposed fix
- await withActionableError(
- () => { this.ios("kill", packageName); },
- `Failed to terminate app "${packageName}"`
- );
+ await withActionableError(
+ async () => { await this.ios("kill", packageName); },
+ `Failed to terminate app "${packageName}"`
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await withActionableError( | |
| () => { this.ios("kill", packageName); }, | |
| `Failed to terminate app "${packageName}"` | |
| ); | |
| await withActionableError( | |
| async () => { await this.ios("kill", packageName); }, | |
| `Failed to terminate app "${packageName}"` | |
| ); |
🤖 Prompt for AI Agents
In src/ios.ts around lines 152 to 155, the call to this.ios("kill", packageName)
is passed to withActionableError without awaiting it, so thrown/rejected errors
bypass the wrapper; change the lambda to return/await the async call (i.e., make
it an async function that awaits this.ios(...)) so withActionableError receives
the Promise and can handle errors correctly.
| await withActionableError( | ||
| () => { this.ios("install", "--path", path); }, | ||
| `Failed to install app from "${path}"` | ||
| ); |
There was a problem hiding this comment.
Missing await causes error handling to fail.
Same issue as launchApp - this.ios() is async but not awaited, breaking error handling.
🔎 Proposed fix
- await withActionableError(
- () => { this.ios("install", "--path", path); },
- `Failed to install app from "${path}"`
- );
+ await withActionableError(
+ async () => { await this.ios("install", "--path", path); },
+ `Failed to install app from "${path}"`
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await withActionableError( | |
| () => { this.ios("install", "--path", path); }, | |
| `Failed to install app from "${path}"` | |
| ); | |
| await withActionableError( | |
| async () => { await this.ios("install", "--path", path); }, | |
| `Failed to install app from "${path}"` | |
| ); |
🤖 Prompt for AI Agents
In src/ios.ts around lines 160-163, the inline function passed to
withActionableError calls this.ios(...) but does not return/await it, so errors
won't propagate; change the callback to return the promise (e.g., use () =>
this.ios("install", "--path", path) or async () => await this.ios("install",
"--path", path)) so the async call is awaited and withActionableError can catch
failures.
| await withActionableError( | ||
| () => { this.ios("uninstall", "--bundleid", bundleId); }, | ||
| `Failed to uninstall app "${bundleId}"` | ||
| ); |
There was a problem hiding this comment.
Missing await causes error handling to fail.
Same issue as launchApp - this.ios() is async but not awaited, breaking error handling.
🔎 Proposed fix
- await withActionableError(
- () => { this.ios("uninstall", "--bundleid", bundleId); },
- `Failed to uninstall app "${bundleId}"`
- );
+ await withActionableError(
+ async () => { await this.ios("uninstall", "--bundleid", bundleId); },
+ `Failed to uninstall app "${bundleId}"`
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await withActionableError( | |
| () => { this.ios("uninstall", "--bundleid", bundleId); }, | |
| `Failed to uninstall app "${bundleId}"` | |
| ); | |
| await withActionableError( | |
| async () => { await this.ios("uninstall", "--bundleid", bundleId); }, | |
| `Failed to uninstall app "${bundleId}"` | |
| ); |
🤖 Prompt for AI Agents
In src/ios.ts around lines 168 to 171, the call to this.ios(...) is not awaited
inside the withActionableError wrapper so thrown errors bypass the handler;
change the lambda to return and await the promise (i.e. make the arrow function
directly return this.ios(...) and add await before withActionableError), so the
async operation is awaited and errors are propagated into withActionableError
for proper handling.
1 participant
Add consistent error handling pattern to AndroidRobot, IosRobot, and MobileDevice classes using new shared utility functions.
Changes:
This ensures all device operations provide clear, actionable error messages regardless of the underlying platform (Android/iOS/simulator).
Files changed:
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.