feat(idb_client): installation + setup #130
Conversation
WalkthroughAdds a new asynchronous fb-idb-based iOS client wrapper module using per-call Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant IdbWrapper as IdbClientWrapper
participant fbIdb as fb-idb Client
participant Device as iOS Device
Caller->>IdbWrapper: tap(x, y)
Note over IdbWrapper: Build per-call client\nClient.build(address, logger)
IdbWrapper->>fbIdb: perform tap via client
fbIdb->>Device: inject touch event
Device-->>fbIdb: success/failure
fbIdb-->>IdbWrapper: result
IdbWrapper-->>Caller: True/False or bytes/dict as appropriate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
🧹 Nitpick comments (3)
minitap/mobile_use/context.py (1)
18-101: IDB client integration into context looks consistent with existing ADB patternThe new
idb_clientfield andget_idb_client()accessor are consistent with the existingadb_clientflow and keep the public context API symmetrical across Android/iOS. No functional concerns from this change alone.If you later want to eliminate the
# type: ignore, you could use a small helper or property that asserts non-Nonebefore returning, but that’s optional and in line with existing code.minitap/mobile_use/clients/idb_client.py (2)
107-115: list_apps error handling is fine; consider surfacing errors if callers need to distinguish “no apps” vs failureFunctionally,
list_apps()wrapsself.client.list_apps()and returns an empty list on failure, which is reasonable for a first pass and matches your pattern of returning falsy values on errors.If, later, you need to distinguish “0 installed apps” from “IDB request failed”, you might want to either re‑raise or return a structured result (e.g.
tuple[bool, list[InstalledAppInfo]]or a small dataclass). For now this is optional.
134-152: Button mapping logic is reasonable; just relies on resolving HIDButtonType correctlyThe
button()method’s mapping from string names to HID button enums is straightforward and case-insensitive:
- HOME, LOCK, SIDE_BUTTON, APPLE_PAY, SIRI → enum values
- Unknown strings return
Falsewithout touching IDB.Once the
HIDButtonTypeimport is corrected, this logic is fine. If you anticipate more button types, consider pulling the map out to a module-level constant so it’s easier to extend and test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
minitap/mobile_use/clients/idb_client.py(1 hunks)minitap/mobile_use/context.py(3 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
minitap/mobile_use/context.py (1)
minitap/mobile_use/clients/idb_client.py (1)
IdbClientWrapper(23-168)
minitap/mobile_use/clients/idb_client.py (1)
minitap/mobile_use/utils/logger.py (2)
get_logger(118-147)error(88-89)
🪛 GitHub Actions: Feature Integration 🧩
minitap/mobile_use/clients/idb_client.py
[error] 6-6: "HIDButtonType" is unknown import symbol (reportAttributeAccessIssue)
[error] 40-40: Argument of type "MobileUseLogger" cannot be assigned to parameter "logger" of type "Logger"
[error] 51-51: "tap" is not a known attribute of "None" (reportOptionalMemberAccess)
[error] 55-55: "swipe" is not a known attribute of "None" (reportOptionalMemberAccess)
[error] 59-59: "screenshot" is not a known attribute of "None" (reportOptionalMemberAccess)
[error] 67-67: Expression of type "None" cannot be assigned to parameter of type "list[str]"
[error] 67-67: Expression of type "None" cannot be assigned to parameter of type "dict[str, str]"
[error] 70-70: "launch" is not a known attribute of "None" (reportOptionalMemberAccess)
[error] 81-81: "terminate" is not a known attribute of "None" (reportOptionalMemberAccess)
[error] 91-91: Object of type "BufferedReader[_BufferedReaderStream]" cannot be used with "async with" because it does not correctly implement aenter
[error] 91-91: Object of type "BufferedReader[_BufferedReaderStream]" cannot be used with "with" because it does not correctly implement aexit
[error] 92-92: "AsyncIterator[InstalledArtifact]" is not awaitable
[error] 92-92: "install" is not a known attribute of "None" (reportOptionalMemberAccess)
[error] 101-101: "uninstall" is not a known attribute of "None" (reportOptionalMemberAccess)
[error] 110-110: "list_apps" is not a known attribute of "None" (reportOptionalMemberAccess)
[error] 119-119: "text" is not a known attribute of "None" (reportOptionalMemberAccess)
[error] 128-128: "key" is not a known attribute of "None" (reportOptionalMemberAccess)
[error] 148-148: "button" is not a known attribute of "None" (reportOptionalMemberAccess)
[error] 156-156: "clear_keychain" is not a known attribute of "None" (reportOptionalMemberAccess)
[error] 164-164: "open_url" is not a known attribute of "None" (reportOptionalMemberAccess)
🔇 Additional comments (3)
pyproject.toml (1)
16-38: fb-idb dependency may break installs on non-macOS; consider guarding or making optionalAdding
fb-idb==1.1.7as a hard dependency meanspip installcan fail on platforms where fb-idb isn’t supported (commonly non-macOS environments). Given the project targets both Windows and Linux hosts in other parts of the codebase, you may want to:
- Move
fb-idbinto an optional extra (e.g.ios), or- Add a platform marker so it only installs where supported.
Also, please confirm
1.1.7is the intended version (no known regressions or security issues for your use case).minitap/mobile_use/clients/idb_client.py (2)
116-133: Text and key helpers look good once client/connection typing is fixedThe
text()andkey()wrappers correctly map through to the underlying client and normalize success/failure tobool. After you address theClient | Nonetyping and logger issues, these methods themselves don’t need additional changes.
87-97: Path.open() cannot be used as async context manager; Client.install() return type needs verificationThe first issue is confirmed:
async with bundle_path.open("rb")will crash at runtime since pathlib'sPath.open()is synchronous without async context manager support.However, the claim that
Client.install()returns anAsyncIteratorcould not be verified via the fb-idb GitHub repository or the codebase. All otherself.clientmethods in this file use the sameawaitpattern (tap,swipe,screenshot,launch,terminate,uninstall,list_apps,text,key,button), and none report issues. This requires manual verification against your fb-idb version's actual API.Additionally, the suggested diff has a syntax error:
open("rb", "rb")should beopen("rb"), and removing@require_connectionlacks justification since it's consistently used as a safety check throughout the class.Fix the Path.open() issue and verify Client.install() behavior:
Replace the async with statement:
with bundle_path.open("rb") as f: await self.client.install(bundle=f)Then verify against your fb-idb library version whether
install()needsasync foriteration instead ofawait.
charlestheprogrammer
force-pushed
the
feat/idb-client-setup
branch
4 times, most recently
from
89298e6 to
f04983d
Compare
charlestheprogrammer
force-pushed
the
feat/idb-client-setup
branch
from
f04983d to
bad4866
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
minitap/mobile_use/clients/idb_client.py (1)
46-192: Consider adding method docstrings for better API documentation.While the class-level docstring clearly explains the decorator pattern, individual methods would benefit from docstrings describing their parameters, return values, and behavior. This would improve maintainability and make the API more discoverable for future developers.
Example for the
tapmethod:@with_idb_client async def tap(self, client: Client, x: int, y: int, duration: float | None = None): """Tap at the specified coordinates on the device screen. Args: client: IDB client instance (injected by decorator) x: X coordinate y: Y coordinate duration: Optional tap duration in seconds """ await client.tap(x=x, y=y, duration=duration)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
minitap/mobile_use/clients/idb_client.py(1 hunks)minitap/mobile_use/context.py(3 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- minitap/mobile_use/context.py
🧰 Additional context used
🧬 Code graph analysis (1)
minitap/mobile_use/clients/idb_client.py (1)
minitap/mobile_use/utils/logger.py (2)
get_logger(118-147)error(88-89)
🔇 Additional comments (4)
minitap/mobile_use/clients/idb_client.py (3)
14-22: LGTM! Decorator pattern correctly addresses lifecycle management.The
@with_idb_clientdecorator elegantly solves the Client.build lifecycle issue by creating a fresh client context per operation, ensuring proper cleanup after each call completes. Thelogger.loggeraccess correctly passes the underlyinglogging.Loggerto fb-idb.
158-174: LGTM! Decorator pattern now correctly applied to all methods.Both
clear_keychain()andopen_url()now use the@with_idb_clientdecorator, addressing the previous concern about missing connection guards. This ensures consistent lifecycle management across all methods.
138-156: The HIDButtonType enum values in fb-idb are: APPLE_PAY, HOME, LOCK, SIDE_BUTTON, SIRI. The code at lines 142-146 correctly uses these bare enum member names without any prefix. No changes are needed.pyproject.toml (1)
37-37: No action needed; fb-idb==1.1.7 is valid and secure.The latest fb-idb version is 1.1.7 (released Mar 3, 2022), and no known vulnerabilities have been found in safety scans. The pinned dependency is acceptable as-is.
charlestheprogrammer
force-pushed
the
feat/idb-client-setup
branch
from
ba9e87c to
c7ea43a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
minitap/mobile_use/clients/idb_client.py (1)
9-10: Verify HIDButtonType import and enum member names against fb-idbYou now import
HIDButtonTypefromidb.common.typesand map string names like"HOME"/"LOCK"toHIDButtonType.HOME/HIDButtonType.LOCK, etc. Given earlier feedback about enum member naming and location, please double‑check against the exact fb-idb 1.1.7 API that:
idb.common.types.HIDButtonTypeis the correct enum type expected byclient.button(...), and- The enum actually exposes the members you use (
HOME,LOCK,SIDE_BUTTON,APPLE_PAY,SIRI).Otherwise this will fail at runtime with an
AttributeErroror a mismatched value. If you’ve already validated this against your environment or fb-idb docs, feel free to ignore; just worth confirming explicitly.#!/bin/bash # Quick runtime check of HIDButtonType in your environment python - << 'PY' from idb.common.types import HIDButtonType print("HIDButtonType members:", [m.name for m in HIDButtonType]) PYAlso applies to: 263-281
🧹 Nitpick comments (3)
minitap/mobile_use/clients/idb_client.py (3)
17-28: Port selection helper is fine; just be aware of the race window
_find_available_portis a straightforward scan-and-bind helper and is fine for local tooling. There is the usual small race window between detecting a free port and startingidb_companionon it (another process could bind it first), but for dev use this is typically acceptable; consider documenting that it’s best-effort rather than strongly reserved.
81-151: Improve idb_companion process lifecycle to avoid potential I/O blocking and destructor surprisesSpawning a long‑lived
idb_companionwith bothstdoutandstderrpiped but never read (except on early failure) risks those pipes filling and eventually blocking the child process. Also,__del__does synchronouswait()calls, which can block unpredictably during GC or interpreter shutdown.Consider:
- Redirecting
stdouttoDEVNULL(and optionallystderrtoo) unless you plan to stream logs somewhere.- Treating
cleanup()/ the async context manager as the primary shutdown path, and simplifying__del__to a best‑effort.terminate()without blocking waits.For example:
- self.companion_process = subprocess.Popen( - cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True - ) + self.companion_process = subprocess.Popen( + cmd, + stdout=subprocess.DEVNULL, + stderr=subprocess.PIPE, + text=True, + )This keeps error output available on startup failure while avoiding unconsumed stdout buildup.
Also applies to: 163-169
171-205: Align return types and error‑handling semantics across wrapper methodsRight now, methods differ in behavior:
tap,swipe, andscreenshotdon’t wrap calls intry/exceptand have no explicit return annotations (implicitly returningNone/bytes), while- Most others (
launch,terminate,install,uninstall,text,key,button,clear_keychain,open_url) catchException, log, and returnbool.For callers this is a bit inconsistent: some operations can raise, others only return
False. It would be cleaner to pick a single convention (e.g., “all methods either returnboolor specific data and never raise, except truly unexpected failures” or “let fb-idb errors bubble up”) and annotate signatures accordingly.In practice that probably means either:
- Adding
try/exceptand-> booltotap/swipe, and-> byteswith guarded behavior toscreenshot, or- Dropping broad
except Exceptionin the others and letting the caller handle errors consistently.Also applies to: 245-299
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
minitap/mobile_use/clients/idb_client.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
minitap/mobile_use/clients/idb_client.py (3)
minitap/mobile_use/utils/logger.py (5)
get_logger(118-147)debug(76-77)info(79-80)warning(85-86)error(88-89)minitap/mobile_use/servers/device_hardware_bridge.py (1)
wait(199-201)minitap/mobile_use/clients/device_hardware_client.py (1)
get(11-13)
🔇 Additional comments (1)
minitap/mobile_use/clients/idb_client.py (1)
31-40: Overall design of per‑call Client.build and context‑managed companion looks solidThe
with_idb_clientdecorator pattern (per‑callClient.buildscoped in an async context manager) together with theIdbClientWrapperasync context manager foridb_companionyields a clean lifecycle: no leaked long‑lived clients, clear separation between local and external companions, and simple call‑site usage.From a high level this structure looks good and addresses the earlier connection‑lifecycle concerns.
Also applies to: 42-80, 301-317
charlestheprogrammer
force-pushed
the
feat/idb-client-setup
branch
from
c7ea43a to
52e9175
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
minitap/mobile_use/clients/idb_client.py (5)
30-36: Consider clearer decorator pattern for client injection.The decorator injects
clientas the first argument afterself, but all decorated methods still explicitly declareclient: Clientin their signatures. While this works functionally, it's unconventional and could confuse maintainers who might expectclientto be passed by callers.Two clearer alternatives:
- Remove
clientfrom method signatures and access via closure (requires refactoring methods to not take client parameter)- Use kwargs injection: Modify decorator to inject as
client=clientand update method signaturesCurrent pattern works but may cause confusion during maintenance.
105-111: Replace fixed sleep with connection readiness check.The 2-second sleep (line 105) assumes the companion will be ready within that timeframe, which may be too short on slow systems or wastefully long on fast ones. Consider polling the gRPC endpoint with retry/timeout instead:
# After starting process for _ in range(10): # ~5 second total timeout if self.companion_process.poll() is not None: # Process died break try: # Try to connect async with Client.build(address=self.address, logger=logger.logger) as test_client: # Connection successful break except Exception: await asyncio.sleep(0.5) else: # Timeout or process died ...This ensures the companion is actually ready to accept requests.
149-158: Document limitations of synchronous del cleanup.The
__del__method performs synchronous process cleanup (terminate/wait/kill) while the proper asynccleanup()method exists. Since__del__cannot useawait, it may not properly clean up if the process is slow to terminate, and it has no way to handle the async operations gracefully.Add a docstring warning:
def __del__(self): """Emergency cleanup in destructor. Prefer explicit cleanup() or context manager. Note: Performs synchronous cleanup only. May not fully clean up resources if companion process doesn't terminate quickly. """And consider adding a warning log if this path is taken:
def __del__(self): if self.companion_process is not None: logger.warning( f"Companion process for {self.udid} cleaned up in __del__. " "Prefer explicit cleanup() or async context manager." ) # ... rest of cleanup
178-184: Use async file I/O to avoid blocking the event loop.Lines 182-183 perform synchronous file writes in an async method, which blocks the event loop. For large screenshots, this can cause noticeable delays.
Use
asyncio.to_threador theaiofileslibrary:Option 1: Using asyncio.to_thread (no additional dependencies)
@with_idb_client async def screenshot(self, client: Client, output_path: str | None = None): screenshot_data = await client.screenshot() if output_path: await asyncio.to_thread(self._write_screenshot, output_path, screenshot_data) return screenshot_data def _write_screenshot(self, path: str, data: bytes): with open(path, "wb") as f: f.write(data)Option 2: Using aiofiles (cleaner)
import aiofiles @with_idb_client async def screenshot(self, client: Client, output_path: str | None = None): screenshot_data = await client.screenshot() if output_path: async with aiofiles.open(output_path, "wb") as f: await f.write(screenshot_data) return screenshot_data
212-222: Consider async file handle for install method.Line 216 opens the app bundle file synchronously. While the actual reading happens in
client.install(), opening the file handle blocks briefly. For consistency and to avoid any potential blocking:@with_idb_client async def install(self, client: Client, app_path: str) -> bool: try: bundle_path = Path(app_path) # Open file in thread pool def open_file(): return open(bundle_path, "rb") f = await asyncio.to_thread(open_file) try: async for _ in client.install(bundle=f): pass finally: await asyncio.to_thread(f.close) return True except Exception as e: logger.error(f"Failed to install: {e}") return FalseOr use
aiofilesfor cleaner syntax. This is optional since the file open operation is typically fast.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
minitap/mobile_use/clients/idb_client.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
minitap/mobile_use/clients/idb_client.py (1)
minitap/mobile_use/utils/logger.py (5)
get_logger(118-147)debug(76-77)info(79-80)warning(85-86)error(88-89)
🔇 Additional comments (1)
minitap/mobile_use/clients/idb_client.py (1)
263-272: No issues found—enum member names are correct.The official HIDButtonType enum in fb-idb defines: APPLE_PAY, HOME, LOCK, SIDE_BUTTON, and SIRI. All five members used in the code (lines 264–268) match exactly. The import from
idb.common.typesis correct, and noBUTTON_prefix exists in the actual enum definition.
| @with_idb_client | ||
| async def tap(self, client: Client, x: int, y: int, duration: float | None = None): | ||
| await client.tap(x=x, y=y, duration=duration) | ||
|
|
||
| @with_idb_client | ||
| async def swipe( | ||
| self, client: Client, x_start: int, y_start: int, x_end: int, y_end: int, delta: int = 10 | ||
| ): | ||
| await client.swipe(p_start=(x_start, y_start), p_end=(x_end, y_end), delta=delta) | ||
|
|
||
| @with_idb_client | ||
| async def screenshot(self, client: Client, output_path: str | None = None): | ||
| screenshot_data = await client.screenshot() | ||
| if output_path: | ||
| with open(output_path, "wb") as f: | ||
| f.write(screenshot_data) | ||
| return screenshot_data |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add consistent error handling to tap, swipe, and screenshot methods.
The tap, swipe, and screenshot methods lack try-except blocks, unlike all other API methods (launch, terminate, install, etc.) which catch exceptions, log errors, and return False or empty results. This inconsistency can surprise callers who expect uniform error handling.
Add exception handling to match the pattern:
@with_idb_client
async def tap(self, client: Client, x: int, y: int, duration: float | None = None):
- await client.tap(x=x, y=y, duration=duration)
+ try:
+ await client.tap(x=x, y=y, duration=duration)
+ return True
+ except Exception as e:
+ logger.error(f"Failed to tap: {e}")
+ return False
@with_idb_client
async def swipe(
self, client: Client, x_start: int, y_start: int, x_end: int, y_end: int, delta: int = 10
):
- await client.swipe(p_start=(x_start, y_start), p_end=(x_end, y_end), delta=delta)
+ try:
+ await client.swipe(p_start=(x_start, y_start), p_end=(x_end, y_end), delta=delta)
+ return True
+ except Exception as e:
+ logger.error(f"Failed to swipe: {e}")
+ return False
@with_idb_client
async def screenshot(self, client: Client, output_path: str | None = None):
- screenshot_data = await client.screenshot()
- if output_path:
- with open(output_path, "wb") as f:
- f.write(screenshot_data)
- return screenshot_data
+ try:
+ screenshot_data = await client.screenshot()
+ if output_path:
+ with open(output_path, "wb") as f:
+ f.write(screenshot_data)
+ return screenshot_data
+ except Exception as e:
+ logger.error(f"Failed to take screenshot: {e}")
+ return b"" # Return empty bytes on error🤖 Prompt for AI Agents
In minitap/mobile_use/clients/idb_client.py around lines 168-184, the tap, swipe
and screenshot methods lack the same try/except error handling used elsewhere;
wrap each method body in a try/except Exception as e, log the exception with the
instance logger (e.g. self.logger.exception or self.logger.error with the
exception), and return the same failure sentinel used by other methods: for tap
and swipe return False on exception, and for screenshot return an empty bytes
object (b"") on exception; keep successful behavior unchanged and preserve the
method signatures.
There was a problem hiding this comment.
Agreed with the rabbit: you can actually include this logic in your custom wrapper so that the logic is not duplicated
There was a problem hiding this comment.
@plfavreau, sounds good! Here's the complete implementation with consistent error handling for the three methods:
@with_idb_client
async def tap(self, client: Client, x: int, y: int, duration: float | None = None) -> bool:
try:
await client.tap(x=x, y=y, duration=duration)
return True
except Exception as e:
logger.error(f"Failed to tap at ({x}, {y}): {e}")
return False
@with_idb_client
async def swipe(
self, client: Client, x_start: int, y_start: int, x_end: int, y_end: int, delta: int = 10
) -> bool:
try:
await client.swipe(p_start=(x_start, y_start), p_end=(x_end, y_end), delta=delta)
return True
except Exception as e:
logger.error(f"Failed to swipe from ({x_start}, {y_start}) to ({x_end}, {y_end}): {e}")
return False
@with_idb_client
async def screenshot(self, client: Client, output_path: str | None = None) -> bytes:
try:
screenshot_data = await client.screenshot()
if output_path:
with open(output_path, "wb") as f:
f.write(screenshot_data)
return screenshot_data
except Exception as e:
logger.error(f"Failed to take screenshot: {e}")
return b""This brings these methods in line with the error handling pattern used throughout the rest of the wrapper class.
There was a problem hiding this comment.
I mean, in the wrapper itself we can abstract this try / catch logic since this is the same one every single time
That way we can actually save a lot of lines / verbosity
There was a problem hiding this comment.
@plfavreau, excellent point! We can definitely abstract the try/catch logic into the decorator. Here's an enhanced version that handles exceptions generically:
def with_idb_client(func):
@wraps(func)
async def wrapper(self, *args, **kwargs):
try:
async with Client.build(address=self.address, logger=logger.logger) as client:
return await func(self, client, *args, **kwargs)
except Exception as e:
method_name = func.__name__
logger.error(f"Failed to {method_name}: {e}")
# Infer default error return value from type annotation
return_type = func.__annotations__.get('return')
if return_type == bool:
return False
elif return_type == bytes:
return b""
elif return_type == list or (hasattr(return_type, '__origin__') and return_type.__origin__ is list):
return []
elif return_type == dict or (hasattr(return_type, '__origin__') and return_type.__origin__ is dict):
return {}
return None
return wrapperThen all the methods become much simpler:
@with_idb_client
async def tap(self, client: Client, x: int, y: int, duration: float | None = None) -> bool:
await client.tap(x=x, y=y, duration=duration)
return True
@with_idb_client
async def swipe(
self, client: Client, x_start: int, y_start: int, x_end: int, y_end: int, delta: int = 10
) -> bool:
await client.swipe(p_start=(x_start, y_start), p_end=(x_end, y_end), delta=delta)
return True
@with_idb_client
async def screenshot(self, client: Client, output_path: str | None = None) -> bytes:
screenshot_data = await client.screenshot()
if output_path:
with open(output_path, "wb") as f:
f.write(screenshot_data)
return screenshot_dataThis approach:
- Eliminates all the repetitive try/catch blocks
- Automatically infers the correct error return value from type annotations
- Provides consistent error logging with the method name
- Keeps the method implementations focused on their core logic
| @with_idb_client | ||
| async def screenshot(self, client: Client, output_path: str | None = None): | ||
| screenshot_data = await client.screenshot() | ||
| if output_path: | ||
| with open(output_path, "wb") as f: | ||
| f.write(screenshot_data) | ||
| return screenshot_data |
There was a problem hiding this comment.
Is the returned screenshot_data base64 ?
It'd would be a good idea to indicate it in the variable name
There was a problem hiding this comment.
It's the PNG content received in bytes not base64, I'll include this in docstring
| @with_idb_client | ||
| async def tap(self, client: Client, x: int, y: int, duration: float | None = None): | ||
| await client.tap(x=x, y=y, duration=duration) | ||
|
|
||
| @with_idb_client | ||
| async def swipe( | ||
| self, client: Client, x_start: int, y_start: int, x_end: int, y_end: int, delta: int = 10 | ||
| ): | ||
| await client.swipe(p_start=(x_start, y_start), p_end=(x_end, y_end), delta=delta) | ||
|
|
||
| @with_idb_client | ||
| async def screenshot(self, client: Client, output_path: str | None = None): | ||
| screenshot_data = await client.screenshot() | ||
| if output_path: | ||
| with open(output_path, "wb") as f: | ||
| f.write(screenshot_data) | ||
| return screenshot_data |
There was a problem hiding this comment.
Could you try to type the expected return statement (python -> symbol, same as you did for the launch command for instance) for all the methods of the class ?
| await client.launch( | ||
| bundle_id=bundle_id, args=args or [], env=env or {}, foreground_if_running=True | ||
| ) |
There was a problem hiding this comment.
That comment applies for all the commands :
What is the return type of client.launch() ? is it an IDB custom type ? Possibly, it could include information about how the command execution went. In that case it would be truly useful to return that object, as agents will need as much context as possible about the commands execution. (It drastically improves the whole orchestration reliability, it better guides agents internal reasoning 👍 )
There was a problem hiding this comment.
launch returns None, but I can return the list of all apps installed to give the agent some context. I can investigate for other cases
| @with_idb_client | ||
| async def button(self, client: Client, button_type: str) -> bool: | ||
| try: | ||
| button_map = { | ||
| "HOME": HIDButtonType.HOME, | ||
| "LOCK": HIDButtonType.LOCK, | ||
| "SIDE_BUTTON": HIDButtonType.SIDE_BUTTON, | ||
| "APPLE_PAY": HIDButtonType.APPLE_PAY, | ||
| "SIRI": HIDButtonType.SIRI, | ||
| } | ||
| button_enum = button_map.get(button_type.upper()) | ||
| if not button_enum: | ||
| return False |
There was a problem hiding this comment.
You can actually save a lot of lines here, by either typing button_type as HIDButtonType directly if possible, or if client.button() method only accepts the given subset of HIDButtonType, then you can declare a custom Literal type for this purpose 👍
e.g: ButtonType = Literal[HIDButtonType.HOME, HIDButtonType.LOCK, ... ]
So that you don't event need the mapper ;)
plfavreau
left a comment
plfavreau
left a comment
There was a problem hiding this comment.
So cool !!
Well done for the lifecycle management, that's also a good thing that you already thought about remote communication
I left a few comments to improve some points. Otherwise this is perfect ! 👍
plfavreau
left a comment
plfavreau
left a comment
There was a problem hiding this comment.
LGTM !
I do not approve because we won't merge this on main directly. We will merge from your second PR instead where I did some changes related to the setup of IDB
🚀 What's new?
Describe the purpose of your pull request. What problem does it solve? What feature does it add? Link to any relevant issues!
🤔 Type of Change
What kind of change is this? Mark with an
x✅ Checklist
Before you submit, please make sure you've done the following. If you have any questions, we're here to help!
ruff check .andruff format .pass).💬 Any questions or comments?
Have a question or need some help? Join us on Discord!
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.