Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds FPS and AVC support for screen capture, streams capture bytes via OnData, adds OnProgress hooks, ensures DeviceKit is installed on Android, updates server headers/validation for mjpeg vs avc, and introduces mutex protection for WDA session access. Changes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
devices/android.go (1)
694-700: Potential nil pointer dereference onconfig.OnData.Unlike
OnProgresswhich is guarded with nil checks (lines 643, 668),OnDatais called without a nil check. If a caller passes a config without anOnDatacallback, this will panic.if n > 0 { // Send bytes to callback, break if it returns false + if config.OnData == nil { + continue + } if !config.OnData(buffer[:n]) { break } }Alternatively, validate
OnDatais not nil at the start of the function, similar to the format validation.
🧹 Nitpick comments (3)
devices/ios.go (1)
348-363: Remove unnecessaryif trueblock.The
if true { ... }construct is always executed and serves no purpose. This appears to be leftover scaffolding or debugging code.Apply this diff to remove the unnecessary conditional:
- // assuming everything went well if we reached this point - if true { - portMjpeg, err := findAvailablePort() - if err != nil { - return fmt.Errorf("failed to find available port for mjpeg: %w", err) - } + // Set up MJPEG port forwarding + portMjpeg, err := findAvailablePort() + if err != nil { + return fmt.Errorf("failed to find available port for mjpeg: %w", err) + } - portForwarderMjpeg := ios.NewPortForwarder(d.ID()) - err = portForwarderMjpeg.Forward(portMjpeg, 9100) - if err != nil { - return fmt.Errorf("failed to forward port for mjpeg: %w", err) - } + portForwarderMjpeg := ios.NewPortForwarder(d.ID()) + err = portForwarderMjpeg.Forward(portMjpeg, 9100) + if err != nil { + return fmt.Errorf("failed to forward port for mjpeg: %w", err) + } - mjpegUrl := fmt.Sprintf("http://localhost:%d/", portMjpeg) - d.mjpegClient = mjpeg.NewWdaMjpegClient(mjpegUrl) - utils.Verbose("Mjpeg client set up on %s", mjpegUrl) - } + mjpegUrl := fmt.Sprintf("http://localhost:%d/", portMjpeg) + d.mjpegClient = mjpeg.NewWdaMjpegClient(mjpegUrl) + utils.Verbose("Mjpeg client set up on %s", mjpegUrl)server/server.go (1)
716-724: Consider logging the JSON marshal error.The
json.Marshalerror is silently ignored. While it's unlikely to fail for a simple map, logging would aid debugging if issues arise.- statusJSON, _ := json.Marshal(map[string]string{"message": message}) + statusJSON, err := json.Marshal(map[string]string{"message": message}) + if err != nil { + log.Printf("Failed to marshal progress message: %v", err) + return + }cli/screenshot.go (1)
89-117: Default constants are named for MJPEG but used for AVC too.The defaults
DefaultMJPEGScaleandDefaultMJPEGFramerateare applied regardless of whether the format is "mjpeg" or "avc". If AVC has different optimal defaults, consider introducingDefaultAVCScale/DefaultAVCFramerateconstants, or rename the current ones to be format-agnostic (e.g.,DefaultScale,DefaultFramerate).This is a minor naming concern for now since the values may work for both formats.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
cli/device.go(2 hunks)cli/screenshot.go(4 hunks)commands/dump.go(1 hunks)commands/info.go(1 hunks)commands/input.go(7 hunks)commands/orientation.go(3 hunks)commands/screenshot.go(2 hunks)commands/url.go(2 hunks)devices/android.go(5 hunks)devices/common.go(10 hunks)devices/ios.go(4 hunks)devices/simulator.go(6 hunks)devices/wda/longpress.go(1 hunks)devices/wda/requests.go(1 hunks)devices/wda/send-keys.go(1 hunks)devices/wda/tap.go(1 hunks)devices/wda/types.go(1 hunks)server/server.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (13)
cli/device.go (1)
devices/common.go (1)
StartAgentConfig(32-34)
commands/screenshot.go (1)
devices/common.go (1)
StartAgentConfig(32-34)
commands/info.go (1)
devices/common.go (1)
StartAgentConfig(32-34)
commands/dump.go (1)
devices/common.go (1)
StartAgentConfig(32-34)
commands/orientation.go (1)
devices/common.go (1)
StartAgentConfig(32-34)
devices/ios.go (2)
devices/common.go (3)
StartAgentConfig(32-34)ScreenCaptureConfig(22-29)DefaultMJPEGFramerate(18-18)utils/logger.go (1)
Verbose(19-23)
devices/wda/requests.go (1)
devices/wda/types.go (1)
WdaClient(9-13)
commands/url.go (1)
devices/common.go (1)
StartAgentConfig(32-34)
devices/common.go (4)
devices/android.go (1)
GetAndroidDevices(370-385)utils/logger.go (1)
Verbose(19-23)devices/ios.go (1)
ListIOSDevices(89-107)devices/simulator.go (1)
GetSimulators(114-160)
commands/input.go (1)
devices/common.go (1)
StartAgentConfig(32-34)
server/server.go (1)
devices/common.go (2)
StartAgentConfig(32-34)ScreenCaptureConfig(22-29)
devices/simulator.go (2)
devices/common.go (3)
StartAgentConfig(32-34)ScreenCaptureConfig(22-29)DefaultMJPEGFramerate(18-18)devices/wda/mjpeg/types.go (1)
NewWdaMjpegClient(7-11)
cli/screenshot.go (2)
commands/commands.go (1)
NewErrorResponse(26-31)devices/common.go (5)
StartAgentConfig(32-34)DefaultMJPEGScale(16-16)DefaultMJPEGFramerate(18-18)ScreenCaptureConfig(22-29)DefaultMJPEGQuality(14-14)
🪛 GitHub Check: test
devices/wda/requests.go
[failure] 146-146:
fmt.Println call has possible Printf formatting directive %v
⏰ 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 (30)
devices/wda/types.go (1)
12-12: LGTM!The
sessionIdfield addition properly supports session caching. The unexported field is appropriately scoped for internal session management.devices/wda/send-keys.go (1)
7-7: LGTM!Consistent with the session caching refactor. The
GetOrCreateSessionusage follows the same pattern established in other WDA operations.devices/wda/longpress.go (1)
9-9: LGTM!Consistent session management refactor. The cached session approach is properly applied.
commands/orientation.go (2)
33-33: LGTM!The migration to config-based
StartAgentis correctly applied. The empty config struct is appropriate for this use case.
63-63: LGTM!Consistent application of the config-based API. The change aligns with the refactor across the codebase.
cli/device.go (1)
51-51: LGTM!The config-based
StartAgentAPI is correctly applied in the CLI context.commands/screenshot.go (1)
57-57: LGTM!The config-based API is correctly applied. The empty config struct is appropriate for this command context.
commands/url.go (1)
26-26: LGTM!The config-based API migration is correctly applied. The change is consistent with the broader refactoring effort.
devices/wda/tap.go (1)
9-9: Session management refactor verified and approved.The migration from
CreateSessiontoGetOrCreateSessionis correctly implemented. The method properly caches the session ID on first call and returns the cached value on subsequent calls, eliminating the need for deferred session cleanup. The refactoring is applied consistently acrosstap.go,send-keys.go, andlongpress.gowith appropriate error handling in each case.commands/info.go (1)
20-20: LGTM!The migration to the new config-based API is correct. Passing an empty
StartAgentConfig{}is appropriate here since progress callbacks aren't needed for this command.commands/dump.go (1)
27-27: LGTM!Correct migration to the config-based
StartAgentAPI.devices/ios.go (2)
242-242: LGTM!The signature change to accept
StartAgentConfigaligns with the new config-based API pattern. The progress callbacks are properly checked for nil before invocation.
645-660: LGTM!The
StartScreenCapturerefactoring correctly extracts configuration from the struct, applies sensible defaults for FPS, and routes the callbacks appropriately.commands/input.go (2)
7-7: LGTM!The new import is required for accessing
StartAgentConfigtype.
63-63: LGTM!All
StartAgentcall sites in this file have been correctly updated to use the new config-based API. The emptyStartAgentConfig{}is appropriate for these command handlers since they don't require progress callbacks.server/server.go (2)
533-536: LGTM!The migration to the config-based
StartAgentConfigAPI is correctly applied here, passing an empty config since no progress callback is needed for the device info handler.
726-751: LGTM!The refactoring to use
StartAgentConfigandScreenCaptureConfigwith callbacks is well-structured. TheOnDatacallback properly returnsfalseon write errors to stop streaming, and the progress callback is correctly wired to both agent startup and screen capture.devices/common.go (4)
21-34: LGTM!The new configuration structs are well-designed with clear documentation. Making callbacks optional (nil-able) provides flexibility for callers who don't need progress reporting.
57-57: LGTM!Interface method signature correctly updated to use the new config struct.
67-67: LGTM!Interface method signature correctly updated to use
ScreenCaptureConfig.
78-80: LGTM on timing instrumentation.The verbose timing logs are useful for diagnosing performance issues during device enumeration. Consistent use of
time.Since(startTime)pattern is clean.Also applies to: 95-97, 106-108, 118-120, 167-167, 201-201
devices/android.go (3)
387-395: LGTM!The signature change to accept
StartAgentConfigis correctly implemented. The method body remains unchanged since Android doesn't require an agent for online devices.
638-641: LGTM!Format validation correctly extended to support both "mjpeg" and "avc" formats, aligning with the PR objective.
658-673: LGTM on AVC server support.Clean implementation of server class selection based on format. The conditional logic clearly differentiates between MJPEG and AVC servers with appropriate progress messages.
cli/screenshot.go (4)
13-16: LGTM!Package-level variables for new CLI flags are correctly declared.
60-67: LGTM!Help text and format validation correctly updated to include "avc" as a valid format option.
78-87: LGTM!Progress callback correctly writes to stderr, which is appropriate for CLI tools that output binary data to stdout.
142-143: LGTM!New CLI flags for
--scaleand--fpsare correctly wired with sensible defaults (0 triggers the package defaults).devices/simulator.go (2)
304-320: LGTM!Progress callbacks are properly guarded with nil checks before invocation. The
defer os.Remove(file)is correctly placed after the download succeeds.
458-557: LGTM!The
StartAgentmethod is correctly refactored to useStartAgentConfig. Progress callbacks are appropriately placed at key stages (launching, waiting) with nil guards.
| func (s *SimulatorDevice) StartScreenCapture(config ScreenCaptureConfig) error { | ||
| mjpegPort, err := s.getWdaMjpegPort() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get MJPEG port: %w", err) | ||
| } | ||
|
|
||
| // configure mjpeg framerate | ||
| err = s.wdaClient.SetMjpegFramerate(DefaultMJPEGFramerate) | ||
| fps := config.FPS | ||
| if fps == 0 { | ||
| fps = DefaultMJPEGFramerate | ||
| } | ||
| err = s.wdaClient.SetMjpegFramerate(fps) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if config.OnProgress != nil { | ||
| config.OnProgress("Starting video stream") | ||
| } | ||
|
|
||
| mjpegClient := mjpeg.NewWdaMjpegClient(fmt.Sprintf("http://localhost:%d", mjpegPort)) | ||
| return mjpegClient.StartScreenCapture(format, callback) | ||
| return mjpegClient.StartScreenCapture(config.Format, config.OnData) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
#!/bin/bash
# Find the ScreenCaptureConfig definition and related code
fd -t f "simulator.go" | head -20Repository: mobile-next/mobilecli
<shell_result>
Repository: mobile-next/mobilecli
Exit code: 0
stdout:
devices/simulator.go
</shell_result>
Script executed:
#!/bin/bash
# Search for ScreenCaptureConfig definition
rg -n "type ScreenCaptureConfig" -A 10Repository: mobile-next/mobilecli
<shell_result>
Repository: mobile-next/mobilecli
Exit code: 0
stdout:
devices/common.go:22:type ScreenCaptureConfig struct {
devices/common.go-23- Format string
devices/common.go-24- Quality int
devices/common.go-25- Scale float64
devices/common.go-26- FPS int
devices/common.go-27- OnProgress func(message string) // optional progress callback
devices/common.go-28- OnData func([]byte) bool // data callback - return false to stop
devices/common.go-29-}
devices/common.go-30-
devices/common.go-31-// StartAgentConfig contains configuration for agent startup operations
devices/common.go-32-type StartAgentConfig struct {
</shell_result>
Script executed:
#!/bin/bash
# Search for WdaMjpegClient and its StartScreenCapture method
rg -n "WdaMjpegClient" -A 5 -B 2 --type go | head -60Repository: mobile-next/mobilecli
<shell_result>
Repository: mobile-next/mobilecli
Exit code: 0
stdout:
devices/simulator.go-669- }
devices/simulator.go-670-
devices/simulator.go:671: mjpegClient := mjpeg.NewWdaMjpegClient(fmt.Sprintf("http://localhost:%d", mjpegPort))
devices/simulator.go-672- return mjpegClient.StartScreenCapture(config.Format, config.OnData)
devices/simulator.go-673-}
devices/simulator.go-674-
devices/simulator.go-675-func findWdaProcessForDevice(deviceUDID string) (int, string, error) {
devices/simulator.go-676- cmd := exec.Command("/bin/ps", "-o", "pid,command", "-E", "-ww", "-e")
--
devices/wda/mjpeg/types.go-1-package mjpeg
devices/wda/mjpeg/types.go-2-
devices/wda/mjpeg/types.go:3:type WdaMjpegClient struct {
devices/wda/mjpeg/types.go-4- baseURL string
devices/wda/mjpeg/types.go-5-}
devices/wda/mjpeg/types.go-6-
devices/wda/mjpeg/types.go:7:func NewWdaMjpegClient(baseURL string) *WdaMjpegClient {
devices/wda/mjpeg/types.go:8: return &WdaMjpegClient{
devices/wda/mjpeg/types.go-9- baseURL: baseURL,
devices/wda/mjpeg/types.go-10- }
devices/wda/mjpeg/types.go-11-}
--
devices/wda/mjpeg/screencapture.go-8-)
devices/wda/mjpeg/screencapture.go-9-
devices/wda/mjpeg/screencapture.go:10:func (c *WdaMjpegClient) StartScreenCapture(format string, callback func([]byte) bool) error {
devices/wda/mjpeg/screencapture.go-11-
devices/wda/mjpeg/screencapture.go-12- client := &http.Client{
devices/wda/mjpeg/screencapture.go-13- Timeout: 0, // No timeout for long-lived streaming requests
devices/wda/mjpeg/screencapture.go-14- }
devices/wda/mjpeg/screencapture.go-15-
--
devices/wda/mjpeg/health.go-1-package mjpeg
devices/wda/mjpeg/health.go-2-
devices/wda/mjpeg/health.go:3:// func (c *WdaMjpegClient) StartScreenCapture(format string, callback func([]byte) bool) error {
devices/wda/mjpeg/health.go-4-
devices/wda/mjpeg/health.go:5:func (c *WdaMjpegClient) CheckHealth() error {
devices/wda/mjpeg/health.go-6- err := c.StartScreenCapture("mjpeg", func(data []byte) bool {
devices/wda/mjpeg/health.go-7- // Just read some data and stop the stream
devices/wda/mjpeg/health.go-8- return false
devices/wda/mjpeg/health.go-9- })
devices/wda/mjpeg/health.go-10-
--
devices/ios.go-34- tunnelManager *ios.TunnelManager
devices/ios.go-35- wdaClient *wda.WdaClient
devices/ios.go:36: mjpegClient *mjpeg.WdaMjpegClient
devices/ios.go-37-}
devices/ios.go-38-
devices/ios.go-39-func (d IOSDevice) ID() string {
devices/ios.go-40- return d.Udid
devices/ios.go-41-}
--
devices/ios.go-359-
devices/ios.go-360- mjpegUrl := fmt.Sprintf("http://localhost:%d/", portMjpeg)
devices/ios.go:361: d.mjpegClient = mjpeg.NewWdaMjpegClient(mjpegUrl)
devices/ios.go-362- utils.Verbose("Mjpeg client set up on %s", mjpegUrl)
devices/ios.go-363- }
devices/ios.go-364-
devices/ios.go-365- return nil
devices/ios.go-366-}
</shell_result>
Script executed:
#!/bin/bash
# Search for SetMjpegFramerate method
rg -n "SetMjpegFramerate" -A 5 -B 2 --type goRepository: mobile-next/mobilecli
<shell_result>
Repository: mobile-next/mobilecli
Exit code: 0
stdout:
devices/simulator.go-660- fps = DefaultMJPEGFramerate
devices/simulator.go-661- }
devices/simulator.go:662: err = s.wdaClient.SetMjpegFramerate(fps)
devices/simulator.go-663- if err != nil {
devices/simulator.go-664- return err
devices/simulator.go-665- }
devices/simulator.go-666-
devices/simulator.go-667- if config.OnProgress != nil {
--
devices/ios.go-649- fps = DefaultMJPEGFramerate
devices/ios.go-650- }
devices/ios.go:651: err := d.wdaClient.SetMjpegFramerate(fps)
devices/ios.go-652- if err != nil {
devices/ios.go-653- return err
devices/ios.go-654- }
devices/ios.go-655-
devices/ios.go-656- if config.OnProgress != nil {
--
devices/wda/appium-settings.go-23-}
devices/wda/appium-settings.go-24-
devices/wda/appium-settings.go:25:func (c *WdaClient) SetMjpegFramerate(framerate int) error {
devices/wda/appium-settings.go-26- err := c.SetAppiumSettings(map[string]interface{}{
devices/wda/appium-settings.go-27- "mjpegServerFramerate": framerate,
devices/wda/appium-settings.go-28- })
devices/wda/appium-settings.go-29- if err != nil {
devices/wda/appium-settings.go-30- return fmt.Errorf("failed to set mjpeg framerate: %w", err)
</shell_result>
Script executed:
#!/bin/bash
# Search for Android device screen capture implementation
rg -n "StartScreenCapture" -A 15 --type go | grep -A 15 "android"Repository: mobile-next/mobilecli
<shell_result>
Repository: mobile-next/mobilecli
Exit code: 0
stdout:
devices/common.go-79- androidDevices, err := GetAndroidDevices()
devices/common.go-80- utils.Verbose("GetAndroidDevices took %s", time.Since(startTime))
devices/common.go-81- if err != nil {
devices/common.go-82- utils.Verbose("Warning: Failed to get Android devices: %v", err)
--
devices/android.go:638:func (d *AndroidDevice) StartScreenCapture(config ScreenCaptureConfig) error {
devices/android.go-639- if config.Format != "mjpeg" && config.Format != "avc" {
devices/android.go-640- return fmt.Errorf("unsupported format: %s, only 'mjpeg' and 'avc' are supported", config.Format)
devices/android.go-641- }
devices/android.go-642-
devices/android.go-643- if config.OnProgress != nil {
devices/android.go-644- config.OnProgress("Installing DeviceKit")
devices/android.go-645- }
devices/android.go-646-
devices/android.go-647- utils.Verbose("Ensuring DeviceKit is installed...")
devices/android.go-648- err := d.EnsureDeviceKitInstalled()
devices/android.go-649- if err != nil {
devices/android.go-650- return fmt.Errorf("failed to ensure DeviceKit is installed: %v", err)
devices/android.go-651- }
devices/android.go-652-
devices/android.go-653- appPath, err := d.GetAppPath("com.mobilenext.devicekit")
--
devices/ios.go:645:func (d IOSDevice) StartScreenCapture(config ScreenCaptureConfig) error {
devices/ios.go-646- // configure mjpeg framerate
devices/ios.go-647- fps := config.FPS
devices/ios.go-648- if fps == 0 {
devices/ios.go-649- fps = DefaultMJPEGFramerate
devices/ios.go-650- }
devices/ios.go-651- err := d.wdaClient.SetMjpegFramerate(fps)
devices/ios.go-652- if err != nil {
devices/ios.go-653- return err
devices/ios.go-654- }
devices/ios.go-655-
devices/ios.go-656- if config.OnProgress != nil {
devices/ios.go-657- config.OnProgress("Starting video stream")
devices/ios.go-658- }
</shell_result>
Script executed:
#!/bin/bash
# Get full Android StartScreenCapture implementation
sed -n '638,750p' devices/android.goRepository: mobile-next/mobilecli
<shell_result>
Repository: mobile-next/mobilecli
Exit code: 0
stdout:
func (d *AndroidDevice) StartScreenCapture(config ScreenCaptureConfig) error {
if config.Format != "mjpeg" && config.Format != "avc" {
return fmt.Errorf("unsupported format: %s, only 'mjpeg' and 'avc' are supported", config.Format)
}
if config.OnProgress != nil {
config.OnProgress("Installing DeviceKit")
}
utils.Verbose("Ensuring DeviceKit is installed...")
err := d.EnsureDeviceKitInstalled()
if err != nil {
return fmt.Errorf("failed to ensure DeviceKit is installed: %v", err)
}
appPath, err := d.GetAppPath("com.mobilenext.devicekit")
if err != nil {
return fmt.Errorf("failed to get app path: %v", err)
}
var serverClass string
var progressMsg string
if config.Format == "mjpeg" {
serverClass = "com.mobilenext.devicekit.MjpegServer"
progressMsg = "Starting MJPEG server"
} else {
serverClass = "com.mobilenext.devicekit.AvcServer"
progressMsg = "Starting AVC server"
}
if config.OnProgress != nil {
config.OnProgress(progressMsg)
}
utils.Verbose("Starting %s with app path: %s", serverClass, appPath)
cmdArgs := append([]string{"-s", d.getAdbIdentifier()}, "exec-out", fmt.Sprintf("CLASSPATH=%s", appPath), "app_process", "/system/bin", serverClass, "--quality", fmt.Sprintf("%d", config.Quality), "--scale", fmt.Sprintf("%.2f", config.Scale), "--fps", fmt.Sprintf("%d", config.FPS))
utils.Verbose("Running command: %s %s", getAdbPath(), strings.Join(cmdArgs, " "))
cmd := exec.Command(getAdbPath(), cmdArgs...)
stdout, err := cmd.StdoutPipe()
if err != nil {
return fmt.Errorf("failed to create stdout pipe: %v", err)
}
if err := cmd.Start(); err != nil {
return fmt.Errorf("failed to start %s: %v", serverClass, err)
}
// Read bytes from the command output and send to callback
buffer := make([]byte, 65536)
for {
n, err := stdout.Read(buffer)
if err != nil {
break
}
if n > 0 {
// Send bytes to callback, break if it returns false
if !config.OnData(buffer[:n]) {
break
}
}
}
_ = cmd.Process.Kill()
return nil
}
func (d *AndroidDevice) installPackage(apkPath string) error {
output, err := d.runAdbCommand("install", apkPath)
if err != nil {
return fmt.Errorf("failed to install package: %v\nOutput: %s", err, string(output))
}
if strings.Contains(string(output), "Success") {
return nil
}
return fmt.Errorf("installation failed: %s", string(output))
}
func (d *AndroidDevice) EnsureDeviceKitInstalled() error {
packageName := "com.mobilenext.devicekit"
appPath, err := d.GetAppPath(packageName)
if err != nil {
return fmt.Errorf("failed to check if %s is installed: %v", packageName, err)
}
if appPath != "" {
// already installed, we have a path to .apk
return nil
}
utils.Verbose("DeviceKit not installed, downloading and installing...")
downloadURL, err := utils.GetLatestReleaseDownloadURL("mobile-next/devicekit-android")
if err != nil {
return fmt.Errorf("failed to get download URL: %v", err)
}
utils.Verbose("Downloading APK from: %s", downloadURL)
tempDir, err := os.MkdirTemp("", "devicekit-android-*")
if err != nil {
return fmt.Errorf("failed to create temp directory: %v", err)
}
defer func() { _ = os.RemoveAll(tempDir) }()
apkPath := filepath.Join(tempDir, "devicekit.apk")
if err := utils.DownloadFile(downloadURL, apkPath); err != nil {
return fmt.Errorf("failed to download APK: %v", err)
}
</shell_result>
Script executed:
#!/bin/bash
# Get full iOS StartScreenCapture implementation
sed -n '645,700p' devices/ios.goRepository: mobile-next/mobilecli
<shell_result>
Repository: mobile-next/mobilecli
Exit code: 0
stdout:
func (d IOSDevice) StartScreenCapture(config ScreenCaptureConfig) error {
// configure mjpeg framerate
fps := config.FPS
if fps == 0 {
fps = DefaultMJPEGFramerate
}
err := d.wdaClient.SetMjpegFramerate(fps)
if err != nil {
return err
}
if config.OnProgress != nil {
config.OnProgress("Starting video stream")
}
return d.mjpegClient.StartScreenCapture(config.Format, config.OnData)
}
func findAvailablePort() (int, error) {
for port := 8100; port <= 8199; port++ {
if utils.IsPortAvailable("localhost", port) {
return port, nil
}
}
return 0, fmt.Errorf("no available ports found in range 8101-8199")
}
func (d IOSDevice) DumpSource() ([]ScreenElement, error) {
return d.wdaClient.GetSourceElements()
}
func (d IOSDevice) InstallApp(path string) error {
log.SetLevel(log.WarnLevel)
device, err := d.getEnhancedDevice()
if err != nil {
return fmt.Errorf("failed to get enhanced device connection: %w", err)
}
svc, err := zipconduit.New(device)
if err != nil {
return fmt.Errorf("zipconduit failed: %w", err)
}
defer func() { _ = svc.Close() }()
err = svc.SendFile(path)
if err != nil {
return fmt.Errorf("failed to install app: %w", err)
}
return nil
}
func (d IOSDevice) UninstallApp(packageName string) (*InstalledAppInfo, error) {
log.SetLevel(log.WarnLevel)
</shell_result>
Based on my verification of the codebase, the review comment is accurate and identifies a real issue. Here are my findings:
Confirmed Facts:
-
Android implementation (devices/android.go:675) explicitly uses both
QualityandScale:"--quality", fmt.Sprintf("%d", config.Quality), "--scale", fmt.Sprintf("%.2f", config.Scale), "--fps" -
iOS and Simulator implementations both ignore
QualityandScale, only applyingFPS:- Simulator calls
mjpegClient.StartScreenCapture(config.Format, config.OnData)without these parameters - iOS makes the identical call
- Neither passes Quality/Scale to the MJPEG client
- Simulator calls
-
WdaMjpegClient.StartScreenCapture method signature only accepts
formatandcallbackparameters—no support for Quality or Scale
This is indeed an inconsistency where Android dynamically configures quality and scale, but iOS and Simulator implementations ignore these configuration fields.
config.Quality and config.Scale are not used for simulator screen capture.
The ScreenCaptureConfig includes Quality and Scale fields, but the simulator implementation only applies FPS via SetMjpegFramerate. These values are ignored when calling mjpegClient.StartScreenCapture. Unlike the Android implementation (which passes --quality and --scale to DeviceKit), the iOS and simulator implementations have no mechanism to apply these settings.
Consider documenting this platform limitation or removing these fields from ScreenCaptureConfig for iOS-based implementations to prevent caller confusion.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/server.go (1)
681-690: Consider consistent naming with Go conventions.The function name uses
JsonRpcbut the existing structs in this file useJSONRPC(e.g.,JSONRPCRequest,JSONRPCResponse). For consistency, consider renaming tonewJSONRPCNotification.-// newJsonRpcNotification creates a JSON-RPC notification message -func newJsonRpcNotification(message string) map[string]interface{} { +// newJSONRPCNotification creates a JSON-RPC notification message +func newJSONRPCNotification(message string) map[string]interface{} {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
devices/android.go(3 hunks)devices/simulator.go(2 hunks)server/server.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
devices/android.go (1)
utils/logger.go (1)
Verbose(19-23)
devices/simulator.go (1)
devices/common.go (1)
DefaultMJPEGFramerate(18-18)
⏰ 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 (10)
server/server.go (3)
707-720: LGTM!The format defaulting, validation, and platform restriction logic is well-structured. The sequence (default → validate → platform check) ensures proper error handling before proceeding.
733-742: LGTM!The content types are appropriate:
multipart/x-mixed-replacefor MJPEG streaming andvideo/h264for AVC. The streaming headers are correctly applied.
744-764: No action needed—OnProgresscallback is safely handled when nil.The
OnProgresscallback is properly guarded with nil checks in all implementations. Each device type (iOS, Android, Simulator) checksif config.OnProgress != nilbefore invoking the callback in bothStartAgentandStartScreenCapture. This safely handles the AVC format path whereprogressCallbackremains nil.devices/android.go (6)
787-789: LGTM!The format validation correctly supports both "mjpeg" and "avc" formats with a clear error message.
792-792: Progress messages updated to generic "Agent" terminology.The progress messages now use "Installing Agent" and "Starting Agent" instead of the more specific "Installing DeviceKit" and "Starting MJPEG server". While this aligns with the agent-based flow described in the PR objectives, the generic terminology is less informative about the actual operation being performed.
Also applies to: 814-814
806-811: LGTM!The dynamic server class selection correctly maps the format to the appropriate server class. This is safe because the format has already been validated at lines 787-789.
817-817: LGTM!The verbose logging now includes the dynamic server class name, making it more informative for debugging.
828-828: LGTM!The error message now includes the dynamic server class name, providing better diagnostic information.
864-913: LGTM! Well-structured DeviceKit installation flow.The
EnsureDeviceKitInstalledmethod is comprehensive and well-implemented:
- Checks if DeviceKit is already installed before attempting download
- Properly handles temporary file cleanup with
defer- Verifies installation after completion
- Provides clear error messages at each step
devices/simulator.go (1)
356-358: InstallWebDriverAgent progress callback is clear and safeThe added
onProgress("Installing WebDriverAgent")block is correctly nil-guarded and gives a precise status update at the right phase. No issues from a correctness or UX standpoint.
| utils.Verbose("Starting MJPEG server with app path: %s", appPath) | ||
| cmdArgs := append([]string{"-s", d.getAdbIdentifier()}, "exec-out", fmt.Sprintf("CLASSPATH=%s", appPath), "app_process", "/system/bin", "com.mobilenext.devicekit.MjpegServer", "--quality", fmt.Sprintf("%d", config.Quality), "--scale", fmt.Sprintf("%.2f", config.Scale)) | ||
| utils.Verbose("Starting %s with app path: %s", serverClass, appPath) | ||
| cmdArgs := append([]string{"-s", d.getAdbIdentifier()}, "exec-out", fmt.Sprintf("CLASSPATH=%s", appPath), "app_process", "/system/bin", serverClass, "--quality", fmt.Sprintf("%d", config.Quality), "--scale", fmt.Sprintf("%.2f", config.Scale), "--fps", fmt.Sprintf("%d", config.FPS)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ScreenCaptureConfig struct definition and FPS field
rg -n -A 15 'type ScreenCaptureConfig struct' --type=go
# Search for FPS validation or default value assignment
rg -n -C 3 'FPS\s*[=:]' --type=go
# Search for FPS usage in validation/config
rg -n -B 2 -A 2 'FPS' --type=go | head -100Repository: mobile-next/mobilecli
Length of output: 3721
🏁 Script executed:
#!/bin/bash
# Search for DefaultMJPEGFramerate definition
rg -n 'DefaultMJPEGFramerate' --type=go -B 2 -A 2
# Search for MjpegServer or AvcServer class definitions (might be Java)
fd -e java | xargs rg -l 'MjpegServer\|AvcServer' 2>/dev/null || echo "No Java files with MjpegServer/AvcServer found"
# Check if there's any FPS validation or bounds checking in Android code
rg -n -B 5 -A 5 'config.FPS\|screencaptureFPS' devices/android.goRepository: mobile-next/mobilecli
Length of output: 970
Add FPS validation in Android screen capture to match iOS/Simulator implementation.
The Android implementation at line 818 passes config.FPS directly to the command without validating for a zero value, unlike iOS and Simulator which both default to DefaultMJPEGFramerate (30) when FPS is 0. Add validation here to handle config.FPS == 0:
fps := config.FPS
if fps == 0 {
fps = DefaultMJPEGFramerate
}
cmdArgs := append([]string{"-s", d.getAdbIdentifier()}, "exec-out", fmt.Sprintf("CLASSPATH=%s", appPath), "app_process", "/system/bin", serverClass, "--quality", fmt.Sprintf("%d", config.Quality), "--scale", fmt.Sprintf("%.2f", config.Scale), "--fps", fmt.Sprintf("%d", fps))🤖 Prompt for AI Agents
In devices/android.go around line 818, the code passes config.FPS directly into
the adb command which allows a zero FPS value; update the logic to mirror
iOS/Simulator by validating FPS and substituting DefaultMJPEGFramerate (30) when
config.FPS == 0, then use that validated fps variable when building cmdArgs so
the "--fps" flag never receives 0.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes / Validation
✏️ Tip: You can customize this high-level summary in your review settings.