fix: was missing tunnel when listing apps on iOS #140
Conversation
|
Warning Rate limit exceeded@gmegidish has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughEnsure the iOS tunnel is started before device operations by calling startTunnel() from Reboot, LaunchWda, ListApps, InstallApp, and UninstallApp. StartTunnel() now delegates to TunnelManager.StartTunnel(), treats an "already running" sentinel error as non-fatal (logged), and wraps other failures. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🤖 Fix all issues with AI agents
In @devices/ios.go:
- Around line 228-241: The code currently detects the "already running" tunnel
by comparing err.Error() to a string; create a sentinel error (e.g., var
ErrTunnelAlreadyRunning = errors.New("tunnel is already running")) in
tunnel-manager.go and return that sentinel from StartTunnel() when appropriate,
then update the check in devices/ios.go to use errors.Is(err,
ErrTunnelAlreadyRunning) (or errors.As if you prefer a typed error) instead of
string comparison; this replaces brittle string matching with a stable sentinel
and ensures proper error detection across packages.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
devices/ios.go
🧰 Additional context used
🧬 Code graph analysis (1)
devices/ios.go (1)
utils/logger.go (1)
Verbose(19-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
devices/ios.go (5)
116-121: LGTM!The tunnel initialization for
Reboot()follows the consistent pattern used across other device operations, with proper error handling and context.
375-380: LGTM!Adding tunnel initialization here ensures
LaunchWdaworks correctly when called directly (not throughStartAgent). The redundant call when invoked viaStartAgentis handled gracefully by the "already running" check instartTunnel().
600-605: Core fix for the PR objective.This addresses the missing tunnel initialization that was causing
ListAppsto fail on iOS 17+ devices. The pattern is consistent with other device operations.
695-700: LGTM!Tunnel initialization for
InstallAppfollows the consistent pattern and ensures iOS 17+ compatibility.
723-728: LGTM!Tunnel initialization for
UninstallAppcompletes the consistent pattern across all device operations requiring tunnel connectivity.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
devices/ios.go (1)
223-244: Fix build: importerrorsand qualifyios.ErrTunnelAlreadyRunning.
Current code won’t compile (undefined: errors/undefined: ErrTunnelAlreadyRunning).Proposed fix
import ( "context" + "errors" "fmt" "io" "strconv" "strings" "time" @@ func (d *IOSDevice) startTunnel() error { @@ err := d.tunnelManager.StartTunnel() if err != nil { // check if it's the "already running" error, which is fine - if errors.Is(err, ErrTunnelAlreadyRunning) { + if errors.Is(err, ios.ErrTunnelAlreadyRunning) { utils.Verbose("Tunnel already running for this device") return nil } return fmt.Errorf("failed to start tunnel: %w", err) }devices/ios/tunnel-manager.go (1)
60-107: StartTunnelWithCallback returns success before the initial tunnel update completes.The
UpdateTunnelscall runs inside a goroutine, soStartTunnelWithCallback()returnsnilimmediately—before the tunnel has actually started. Callers expect the function name and synchronous return semantics to mean "tunnel is ready after this call returns," but startup failures are only reported asynchronously via theonProcessDiedcallback. This breaks downstream "ensure tunnel then proceed" flows and hides startup errors from callers that check the return value.Since
UpdateTunnelsis a synchronous blocking call that completes when the update finishes, it should be called before the goroutine launches so thatStartTunnelWithCallback()only returnsnilafter the initial tunnel setup succeeds.
🤖 Fix all issues with AI agents
In @devices/ios.go:
- Around line 241-243: Replace the fixed sleep after StartTunnel() with a
polling loop that calls tunnelMgr.FindTunnel(d.Udid) until it returns a valid
tunnel (e.g., tunnelInfo.Udid != "") or a timeout is reached; locate the call to
StartTunnel() and the subsequent time.Sleep(1 * time.Second) and instead
repeatedly call tunnelMgr.FindTunnel(d.Udid) with a short interval (e.g.,
100-200ms) and an overall timeout, returning an error if the tunnel isn't ready
in time and proceeding only when FindTunnel reports the tunnel is available.
In @devices/ios/tunnel-manager.go:
- Line 16: Add the missing import for the standard errors package so
ErrTunnelAlreadyRunning compiles: in devices/ios/tunnel-manager.go add `import
"errors"` (or include "errors" in the existing import block) so the var
ErrTunnelAlreadyRunning = errors.New("tunnel is already running") can reference
errors.
🧹 Nitpick comments (1)
devices/ios.go (1)
113-120: Avoid double-wrappingfailed to start tunnelat call sites.
SincestartTunnel()already returnsfmt.Errorf("failed to start tunnel: %w", err), wrapping again in callers will likely produce duplicated context.Example adjustment (pattern)
err := d.startTunnel() if err != nil { - return fmt.Errorf("failed to start tunnel: %w", err) + return err }Also applies to: 368-381, 598-606, 693-701, 721-729
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
devices/ios.godevices/ios/tunnel-manager.go
🧰 Additional context used
🧬 Code graph analysis (1)
devices/ios.go (2)
devices/ios/tunnel-manager.go (1)
ErrTunnelAlreadyRunning(16-16)utils/logger.go (1)
Verbose(19-23)
🪛 GitHub Actions: PR #140
devices/ios/tunnel-manager.go
[error] 16-16: Go build failed: undefined: errors in devices/ios/tunnel-manager.go:16
🪛 GitHub Check: security
devices/ios/tunnel-manager.go
[failure] 16-16:
undefined: errors
devices/ios.go
[failure] 234-234:
undefined: ErrTunnelAlreadyRunning
[failure] 234-234:
undefined: errors
🪛 GitHub Check: test
devices/ios/tunnel-manager.go
[failure] 16-16:
undefined: errors
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
devices/ios.go (1)
117-121: Receiver type mismatch: value receiver calling pointer method.
Reboot()uses a value receiver(d IOSDevice)(line 114), butstartTunnel()is defined with a pointer receiver(d *IOSDevice). While Go allows this call (it takes the address automatically), it's inconsistent with other methods that also callstartTunnel()(e.g.,LaunchWdaat line 369 also uses value receiver). This works but is a code smell—consider using pointer receivers consistently for methods that mutate state or call pointer-receiver methods.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
devices/ios.godevices/ios/tunnel-manager.go
🧰 Additional context used
🧬 Code graph analysis (1)
devices/ios.go (2)
devices/ios/tunnel-manager.go (1)
ErrTunnelAlreadyRunning(17-17)utils/logger.go (1)
Verbose(19-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
🔇 Additional comments (7)
devices/ios/tunnel-manager.go (2)
17-18: LGTM! Well-designed sentinel error.Using a sentinel error for "tunnel already running" is idiomatic Go and enables callers to use
errors.Is()for reliable comparison, which is exactly whatstartTunnel()indevices/ios.gonow does.
61-63: LGTM! Consistent with the new sentinel error pattern.Returning the sentinel
ErrTunnelAlreadyRunninghere properly enables the caller to distinguish this non-fatal condition from actual errors.devices/ios.go (5)
224-245: LGTM! Clean error handling with sentinel error.The use of
errors.Is(err, ios.ErrTunnelAlreadyRunning)is correct and properly treats "already running" as a non-fatal condition. The early return for devices not requiring tunnel (iOS < 17) is efficient.One minor note: the 1-second sleep (line 243) is a timing workaround. If tunnel readiness becomes unreliable, consider polling for tunnel availability instead.
377-381: LGTM! Tunnel ensured before WDA operations.The tunnel is properly started before obtaining the enhanced device connection. This aligns with the PR objective.
599-606: Core fix: tunnel now established before listing apps.This addresses the PR objective—
ListApps()was missing thestartTunnel()call, causing failures on iOS 17+ devices that require a tunnel for device communication.
697-701: LGTM! Tunnel ensured for app installation.Consistent with the other device operations that require tunnel for iOS 17+.
725-729: LGTM! Tunnel ensured for app uninstallation.Consistent with the other device operations that require tunnel for iOS 17+.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @commands/commands.go:
- Around line 70-75: deviceCache is a global map accessed concurrently and
iterated without synchronization, causing a data race and nondeterministic
selection; add a sync.RWMutex (e.g., deviceCacheMu) to protect it, wrap the
iteration in the shown snippet with deviceCacheMu.RLock()/RUnlock(), and change
every location that writes or mutates deviceCache (the places that assign or
delete from deviceCache) to use deviceCacheMu.Lock()/Unlock(); additionally, if
you need a deterministic "first online device" choose a stable selection
strategy (e.g., sort keys or use an explicit priority field) instead of relying
on map iteration order.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
commands/commands.godocs/jsonrpc_README.mdserver/server.go
🧰 Additional context used
🧬 Code graph analysis (1)
server/server.go (1)
commands/apps.go (4)
AppRequest(8-11)TerminateAppCommand(35-53)ListAppsRequest(56-58)ListAppsCommand(61-73)
🪛 markdownlint-cli2 (0.18.1)
docs/jsonrpc_README.md
148-148: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
148-148: Hard tabs
Column: 1
(MD010, no-hard-tabs)
149-149: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
149-149: Hard tabs
Column: 1
(MD010, no-hard-tabs)
150-150: Unordered list indentation
Expected: 4; Actual: 2
(MD007, ul-indent)
150-150: Hard tabs
Column: 1
(MD010, no-hard-tabs)
151-151: Unordered list indentation
Expected: 4; Actual: 2
(MD007, ul-indent)
151-151: Hard tabs
Column: 1
(MD010, no-hard-tabs)
154-154: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
154-154: Hard tabs
Column: 1
(MD010, no-hard-tabs)
155-155: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
155-155: Hard tabs
Column: 1
(MD010, no-hard-tabs)
156-156: Unordered list indentation
Expected: 4; Actual: 2
(MD007, ul-indent)
156-156: Hard tabs
Column: 1
(MD010, no-hard-tabs)
264-264: Hard tabs
Column: 1
(MD010, no-hard-tabs)
265-265: Hard tabs
Column: 1
(MD010, no-hard-tabs)
272-272: Hard tabs
Column: 1
(MD010, no-hard-tabs)
273-273: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (5)
server/server.go (3)
191-194: LGTM!New method dispatch entries follow the established pattern and correctly route to the new handlers.
476-483: LGTM!Parameter structs are consistent with existing definitions like
AppsLaunchParamsand follow the same JSON tag conventions.
742-783: LGTM!Both handlers follow established patterns in this file:
handleAppsTerminatemirrorshandleAppsLaunchwith required params validationhandleAppsListcorrectly makes params optional to support device auto-selection viaFindDeviceOrAutoSelectdocs/jsonrpc_README.md (2)
147-157: Documentation accurately reflects the new API methods.The new method entries for
apps_terminateandapps_listfollow the existing documentation style and correctly describe the parameters. The static analysis warnings about tabs and indentation apply to the entire file's pre-existing style, which you've correctly maintained for consistency.
259-274: LGTM!Curl examples for the new methods are consistent with existing examples and provide clear usage patterns.
Summary by CodeRabbit
Bug Fixes
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.