feat: add progress notifications on wda installation #109
Conversation
WalkthroughRefactors device APIs to accept configuration structs: Changes
Sequence Diagram(s)sequenceDiagram
participant Client as CLI / Server HTTP
participant Device as Device (Android/iOS/Simulator)
participant Agent as WebDriverAgent
Client->>Device: StartAgent(StartAgentConfig{OnProgress})
Note right of Device: report progress via OnProgress
Device->>Device: Install/Launch agent steps
Device->>Agent: Wait for ready
Agent-->>Device: Ready
Device-->>Client: StartAgent returns
Client->>Device: StartScreenCapture(ScreenCaptureConfig{Format,OnProgress,OnData})
Note right of Device: OnProgress("Starting video stream")
Device->>Device: Initialize MJPEG / stream client
Device-->>Client: Frame bytes via OnData
Note over Client,Device: Progress messages interleaved via OnProgress
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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/android.go (1)
684-690: Potential nil dereference ifconfig.OnDatais nil.If a caller invokes
StartScreenCapturewithout settingOnData, line 686 will panic. Consider adding a nil check or validatingOnDatais not nil at the start of the function.+ if config.OnData == nil { + return fmt.Errorf("OnData callback is required") + } + if config.OnProgress != nil { config.OnProgress("Installing DeviceKit") }Alternatively, add the validation at function entry (around line 639).
devices/simulator.go (1)
651-669: Add nil check forconfig.OnDatabefore passing to mjpegClient.The
OnDatacallback is documented without an "optional" designation (unlikeOnProgress), but it's passed tomjpegClient.StartScreenCapturewithout a nil check. At line 39 ofdevices/wda/mjpeg/screencapture.go, the callback is invoked directly:if !callback(buffer[:n]). In Go, invoking a nil function value causes a panic. If a caller passes nil forOnData, this will crash at runtime.The same issue exists in
devices/ios.go:656anddevices/android.go:686(which callsconfig.OnDatadirectly without a guard).
🧹 Nitpick comments (2)
server/server.go (2)
717-720: Consider handling errors in progress callback.The
json.MarshalandWriteerrors are ignored, while theOnDatacallback (line 740) properly checks write errors. If progress updates are truly non-critical and best-effort, this is acceptable; otherwise, consider at least logging failures.Apply this diff if you'd like to add minimal error logging:
progressCallback := func(message string) { - statusJSON, _ := json.Marshal(map[string]string{"message": message}) + statusJSON, err := json.Marshal(map[string]string{"message": message}) + if err != nil { + log.Printf("Error marshaling progress message: %v", err) + return + } mimeMessage := fmt.Sprintf("--BoundaryString\r\nContent-Type: application/json\r\nContent-Length: %d\r\n\r\n%s\r\n", len(statusJSON), statusJSON) - _, _ = w.Write([]byte(mimeMessage)) + if _, err := w.Write([]byte(mimeMessage)); err != nil { + log.Printf("Error writing progress message: %v", err) + }
734-752: Inconsistent error logging on line 742.The
OnDatacallback usesfmt.Printlnwhile the rest of the file consistently useslog.Printf(e.g., line 195). Consider usinglog.Printffor consistency.Apply this diff:
OnData: func(data []byte) bool { _, writeErr := w.Write(data) if writeErr != nil { - fmt.Println("Error writing data:", writeErr) + log.Printf("Error writing data: %v", writeErr) return false }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cli/device.go(2 hunks)cli/screenshot.go(1 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(4 hunks)devices/common.go(3 hunks)devices/ios.go(4 hunks)devices/simulator.go(7 hunks)server/server.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
commands/input.go (1)
devices/common.go (1)
StartAgentConfig(30-32)
cli/device.go (1)
devices/common.go (1)
StartAgentConfig(30-32)
commands/orientation.go (1)
devices/common.go (1)
StartAgentConfig(30-32)
commands/screenshot.go (1)
devices/common.go (1)
StartAgentConfig(30-32)
commands/url.go (1)
devices/common.go (1)
StartAgentConfig(30-32)
devices/android.go (2)
devices/common.go (2)
StartAgentConfig(30-32)ScreenCaptureConfig(21-27)utils/logger.go (1)
Verbose(19-23)
commands/dump.go (1)
devices/common.go (1)
StartAgentConfig(30-32)
commands/info.go (1)
devices/common.go (1)
StartAgentConfig(30-32)
server/server.go (1)
devices/common.go (2)
StartAgentConfig(30-32)ScreenCaptureConfig(21-27)
cli/screenshot.go (2)
devices/common.go (4)
StartAgentConfig(30-32)ScreenCaptureConfig(21-27)DefaultMJPEGQuality(13-13)DefaultMJPEGScale(15-15)commands/commands.go (1)
NewErrorResponse(26-31)
⏰ 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). (3)
- GitHub Check: build_on_windows
- GitHub Check: build_on_macos
- GitHub Check: build_on_linux
🔇 Additional comments (18)
devices/common.go (2)
20-32: Well-designed config structs for extensibility.The config struct pattern is a good approach for adding optional callbacks and parameters. The inline documentation clearly explains the callback semantics (
OnProgressis optional,OnDatareturns false to stop).
55-65: All implementations ofControllableDevicehave been properly updated.Verified that
StartAgentandStartScreenCaptureaccept the new config structs across all device implementations:
- StartAgent: SimulatorDevice, IOSDevice, AndroidDevice all accept
StartAgentConfigand useconfig.OnProgressfor notifications- StartScreenCapture: SimulatorDevice, AndroidDevice, IOSDevice all accept
ScreenCaptureConfigand properly extract fields (Format, Quality, Scale, OnProgress, OnData) for their operationsThe interface changes are complete and consistent.
devices/simulator.go (2)
304-320: Progress callbacks correctly guarded with nil checks.The nil checks before invoking
onProgressproperly handle the case when callers don't provide a progress callback.
458-557: StartAgent implementation looks correct.Progress notifications are well-placed at key stages: WebDriverAgent installation, launching, and waiting for startup. The config is properly threaded through to
InstallWebDriverAgent.commands/orientation.go (2)
33-36: Correct migration to new StartAgent API.Using an empty
StartAgentConfig{}is appropriate here since CLI commands don't need progress callbacks.
63-66: Consistent usage with OrientationGetCommand.commands/screenshot.go (1)
57-60: Correct migration to new StartAgent API.Using an empty
StartAgentConfig{}is consistent with the other command implementations.devices/android.go (2)
387-395: StartAgent correctly accepts config struct.Even though Android devices don't require an agent for online devices, the method signature correctly matches the interface. The unused parameter is acceptable since the interface requires this signature.
638-645: Good format validation and nil check for OnProgress.The format validation ensures only "mjpeg" is accepted, providing a clear error message for unsupported formats.
commands/dump.go (1)
27-27: LGTM!The update to use
devices.StartAgentConfig{}correctly adapts to the new configuration-driven API. Passing an empty config is appropriate here since this command doesn't require progress reporting.commands/url.go (1)
5-6: LGTM!The import addition and
StartAgentcall update correctly align with the new configuration-driven API.Also applies to: 26-26
commands/input.go (1)
7-7: LGTM!All
StartAgentcalls are consistently updated to use the new configuration-driven API across all input command handlers.Also applies to: 63-63, 89-89, 115-115, 141-141, 167-167, 208-208
cli/device.go (1)
7-7: LGTM!The import and
StartAgentcall update correctly adapt to the new API. The empty config is appropriate for this info command.Also applies to: 51-51
commands/info.go (1)
20-20: LGTM!The
StartAgentcall correctly uses the new configuration struct.devices/ios.go (1)
242-244: Good progress notification integration.The
StartAgentmethod now properly emits progress notifications at key stages (starting tunnel, launching WDA, waiting for agent). The nil checks before invokingOnProgressare correctly implemented.Also applies to: 286-294, 317-343
cli/screenshot.go (1)
73-100: Well-structured progress and data callbacks.Good implementation:
- Progress messages correctly go to stderr, keeping stdout clean for binary data
OnDataproperly returnsfalseon write errors to stop the capture gracefully- Error messages in
OnDataare written to stderrserver/server.go (2)
533-533: LGTM: Config struct migration.The empty config is appropriate here since device info retrieval doesn't require progress notifications.
726-731: LGTM: Structured config with progress callback.The agent is correctly started with progress notifications before screen capture begins.
| func (d IOSDevice) StartScreenCapture(config ScreenCaptureConfig) error { | ||
| // configure mjpeg framerate | ||
| err := d.wdaClient.SetMjpegFramerate(DefaultMJPEGFramerate) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return d.mjpegClient.StartScreenCapture(format, callback) | ||
| if config.OnProgress != nil { | ||
| config.OnProgress("Starting video stream") | ||
| } | ||
|
|
||
| return d.mjpegClient.StartScreenCapture(config.Format, config.OnData) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify if Quality and Scale from config should be utilized.
ScreenCaptureConfig includes Quality and Scale fields, but only Format and OnData are passed to mjpegClient.StartScreenCapture. If these fields are intentionally unused for iOS MJPEG capture, consider documenting this. Otherwise, they may need to be applied.
🏁 Script executed:
#!/bin/bash
# Check if mjpegClient.StartScreenCapture or SetMjpegFramerate methods accept quality/scale parameters
ast-grep --pattern $'func ($_, $_) StartScreenCapture($$$) {
$$$
}'
# Also check the wda client for any quality/scale settings
rg -n "Quality|Scale" --type go -C 3Length of output: 9091
I need to search for the mjpegClient interface and implementation to determine if quality/scale can be configured for iOS.
🏁 Script executed:
#!/bin/bash
# Find mjpegClient definition and its StartScreenCapture method signature
rg -n "type.*mjpegClient|interface.*mjpegClient" --type go
# Find mjpegClient.StartScreenCapture method definition
ast-grep --pattern 'func ($_, $_) StartScreenCapture($$$) {
$$$
}'
# Search for mjpegClient initialization to see what type it is
rg -n "mjpegClient.*=" --type go -B 2 -A 2
# Look for quality/scale related methods in wda or mjpeg packages
rg -n "SetQuality|SetScale|Quality|Scale" devices/wda/ --type goLength of output: 691
🏁 Script executed:
#!/bin/bash
# Alternative: search more broadly for mjpeg-related interfaces
rg -n "StartScreenCapture" --type go -B 3 -A 6Length of output: 5468
🏁 Script executed:
#!/bin/bash
# Find mjpegClient definition and its type
rg -n "mjpegClient" --type go -B 2 -A 2
# Search for mjpeg client implementations or interfaces
fd -e go | xargs grep -l "mjpeg" | head -20Length of output: 1178
Let me check if wdaClient has methods to configure quality or scale for the MJPEG stream:
🏁 Script executed:
#!/bin/bash
# Search for wdaClient interface and its available methods
rg -n "type WdaClient|func.*wdaClient" --type go -A 3
# Look for SetMjpeg methods on wdaClient
rg -n "SetMjpeg|SetQuality|SetScale" --type go devices/wda/
# Check WdaClient definition
fd -e go devices/wda/ | head -10Length of output: 613
🏁 Script executed:
#!/bin/bash
# Find all methods on WdaClient
rg -n "func \(c \*WdaClient\)" --type go
# Also check the appium-settings file for all SetMjpeg methods
cat devices/wda/appium-settings.goLength of output: 2880
Unused Quality and Scale parameters in iOS StartScreenCapture implementation need resolution.
The iOS (and Simulator) StartScreenCapture method receives Quality and Scale from ScreenCaptureConfig but does not use them. Unlike Android, which passes these parameters to its external MJPEG server (devices/android.go:663), the iOS implementation only passes Format and OnData to mjpegClient.StartScreenCapture (devices/ios.go:656).
The WdaClient interface provides only SetMjpegFramerate for MJPEG configuration; there are no SetMjpegQuality or SetMjpegScale methods. Either:
- These fields should be removed from the iOS/Simulator code paths, or
- If WDA supports quality/scale configuration through Appium settings, add corresponding methods to
WdaClientand invoke them inStartScreenCapture.
🤖 Prompt for AI Agents
In devices/ios.go around lines 645-656, StartScreenCapture currently sets only
the MJPEG framerate and ignores ScreenCaptureConfig.Quality and .Scale; either
remove those fields for iOS/simulator or (preferred) extend WdaClient with
SetMjpegQuality(int) and SetMjpegScale(float64) (and implement them) and call
them here before starting the mjpeg client, only invoking them when
config.Quality/Scale are non-zero/nonnull; preserve the existing framerate call,
keep the OnProgress message, and then call
mjpegClient.StartScreenCapture(config.Format, config.OnData).
There was a problem hiding this comment.
Actionable comments posted: 0
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)
746-802: Ensure MJPEG child process is reaped and guard OnData in Android StartScreenCapture
StartScreenCapturedoes a good job of:
- Validating
config.Format == "mjpeg".- Emitting progress messages for installing DeviceKit and starting the MJPEG server.
- Passing
config.Qualityandconfig.Scaledown to the JavaMjpegServer.- Streaming stdout chunks into
config.OnData.Two things are worth tightening up:
Child process reaping
cmd.Start()is called, butcmd.Wait()is never invoked. Even though youKillthe process at the end, not waiting for it can leave zombies around in a long-lived server. A simple defer afterStartfixes this: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 MJPEG server: %v", err) }
- // Ensure the MJPEG process is properly reaped
- defer func() { _ = cmd.Wait() }()
@@
- _ = cmd.Process.Kill()
- return nil
- _ = cmd.Process.Kill()
- return nil
}This preserves the existing behavior (including killing the process when the callback asks to stop) while ensuring the OS reclaims the child. 2. **OnData nil-guard** The implementation assumes `config.OnData` is non-nil. If a caller ever omits it, this will panic on the first chunk. A small upfront check makes the contract explicit: ```diff func (d *AndroidDevice) StartScreenCapture(config ScreenCaptureConfig) error { + if config.OnData == nil { + return fmt.Errorf("StartScreenCapture: OnData callback is required") + } if config.Format != "mjpeg" { return fmt.Errorf("unsupported format: %s, only 'mjpeg' is supported", config.Format) }Together these changes would make the Android MJPEG capture path more robust in long-running and misconfiguration scenarios.
♻️ Duplicate comments (2)
devices/ios.go (1)
638-650: Clarify / honor Quality & Scale and guard against nil OnData in iOS screen capture
StartScreenCapturenow usesScreenCaptureConfigbut still only consumesFormatandOnData;QualityandScaleremain unused on iOS/simulator, unlike Android where they’re forwarded to the MJPEG server. If WDA truly has no knobs for these, consider either documenting that they’re ignored on iOS/simulator or exposing platform-specific configuration so callers don’t assume they apply uniformly. Also,config.OnDatais required formjpegClient.StartScreenCaptureto function; a defensive nil-check with a clear error would make misuse fail early instead of panicking later.Example tweak:
func (d IOSDevice) StartScreenCapture(config ScreenCaptureConfig) error { + if config.OnData == nil { + return fmt.Errorf("StartScreenCapture: OnData callback is required") + } // configure mjpeg framerate err := d.wdaClient.SetMjpegFramerate(DefaultMJPEGFramerate) if err != nil { return err } @@ - return d.mjpegClient.StartScreenCapture(config.Format, config.OnData) + return d.mjpegClient.StartScreenCapture(config.Format, config.OnData) }devices/simulator.go (1)
654-672: Align simulator screen capture config semantics with iOS and guard OnDataSimilar to the iOS implementation,
StartScreenCapturefor simulators usesconfig.Formatandconfig.OnDatabut ignoresQualityandScale. If those fields are intended to be Android-specific, consider documenting that they are no-ops on iOS/simulator; otherwise, you might later want to plumb them through if WDA ever exposes corresponding controls. Also, sinceconfig.OnDatais required formjpegClient.StartScreenCaptureto do anything useful, a small upfront nil-check (returning a clear error) would prevent accidental panics.Example:
func (s *SimulatorDevice) StartScreenCapture(config ScreenCaptureConfig) error { + if config.OnData == nil { + return fmt.Errorf("StartScreenCapture: OnData callback is required") + } mjpegPort, err := s.getWdaMjpegPort() if err != nil { return fmt.Errorf("failed to get MJPEG port: %w", err) } @@ - mjpegClient := mjpeg.NewWdaMjpegClient(fmt.Sprintf("http://localhost:%d", mjpegPort)) - return mjpegClient.StartScreenCapture(config.Format, config.OnData) + mjpegClient := mjpeg.NewWdaMjpegClient(fmt.Sprintf("http://localhost:%d", mjpegPort)) + return mjpegClient.StartScreenCapture(config.Format, config.OnData) }
🧹 Nitpick comments (2)
devices/android_display_test.go (1)
74-110: Consider adding a case where first display is OFF and second is ONCurrent
Test_parseDisplayIdFromDumpsysStatecovers “first ON” and invalid input. An extra case where the first display is OFF and a later one is ON would document howparseDisplayIdFromDumpsysStatebehaves when multiple displays exist and only a later one is active.server/server.go (1)
681-767: Streaming progress notifications through the MJPEG response is coherent with the new config modelThe
handleScreenCapturechanges cleanly wire:
- A
progressCallbackthat wraps messages as JSON-RPC notifications and writes them asapplication/jsonparts in the samemultipart/x-mixed-replacestream.StartAgent(devices.StartAgentConfig{OnProgress: progressCallback})so WDA/DeviceKit startup progress is surfaced to clients.StartScreenCapture(devices.ScreenCaptureConfig{Format, Quality, Scale, OnProgress: progressCallback, OnData: ...})so both setup progress and MJPEG bytes flow through one HTTP response.Headers and flushing behavior look appropriate for long-lived streaming. One minor nit:
fmt.Println("Error writing data:", writeErr)writes to stdout; if you care about log routing, consider using the existing logging approach (e.g.,log.Printforutils.Verbose/utils.Info) instead, but this is purely cosmetic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cli/screenshot.go(2 hunks)devices/android.go(6 hunks)devices/android_display_test.go(1 hunks)devices/ios.go(4 hunks)devices/simulator.go(7 hunks)server/server.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
devices/android.go (2)
devices/common.go (2)
StartAgentConfig(30-32)ScreenCaptureConfig(21-27)utils/logger.go (1)
Verbose(19-23)
devices/simulator.go (2)
devices/common.go (2)
StartAgentConfig(30-32)ScreenCaptureConfig(21-27)devices/wda/mjpeg/types.go (1)
NewWdaMjpegClient(7-11)
devices/ios.go (1)
devices/common.go (3)
StartAgentConfig(30-32)ScreenCaptureConfig(21-27)DefaultMJPEGFramerate(17-17)
server/server.go (1)
devices/common.go (2)
StartAgentConfig(30-32)ScreenCaptureConfig(21-27)
⏰ 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 (11)
devices/android_display_test.go (2)
5-38: Display ID parsing tests match implementation behaviorThe
Test_parseDisplayIdFromCmdDisplaytable cases accurately reflectparseDisplayIdFromCmdDisplay’s logic (first ON entry wins; invalid input yields empty string) and will guard regressions on the cmd-display path.
40-72: Viewport-based display parsing tests are well targeted
Test_parseDisplayIdFromDumpsysViewportdirectly exercises the regex path forisActive=trueINTERNAL viewports and thelocal:prefix stripping, which is exactly whatparseDisplayIdFromDumpsysViewportimplements.devices/ios.go (1)
242-359: StartAgent progress callbacks are wired sensibly for iOS real devicesThe new
StartAgent(config StartAgentConfig)wiring usesOnProgressat the most relevant phases (tunnel start, WDA launch, agent wait) and avoids sending progress in the “WDA not installed” error path, addressing the earlier misleading message concern. Callers that don’t care about progress can still pass a zero-value config without behavior changes.cli/screenshot.go (1)
73-101: CLI screencapture integration with config-based APIs looks solidThe transition to
StartAgent(devices.StartAgentConfig{OnProgress: ...})andStartScreenCapture(devices.ScreenCaptureConfig{...})is consistent with the new device interface. Progress is funneled cleanly throughutils.Verbose, and theOnDatacallback correctly stops the stream on stdout write errors. This path should behave like before while adding useful progress output.devices/android.go (3)
59-97: ANDROID_HOME existence check is a safe improvementThe additional
os.Statcheck before returningANDROID_HOMEavoids constructing tool paths from a non-existent SDK directory and lets the function fall back to well-known defaults instead. This is a backward-compatible robustness win.
174-287: Multi-display screenshot flow is reasonable with sane fallbacksThe new
getDisplayCount,parseDisplayIdFrom*,getFirstDisplayId, andcaptureScreenshotfunctions add proper support for multi-display devices:
getDisplayCountusesdumpsys SurfaceFlinger --display-idand falls back to assuming a single display on error.getFirstDisplayIdsensibly preferscmd display get-displays, thendumpsys displayvia viewport and state parsers.TakeScreenshotkeeps backward compatibility (single-display/older Android → defaultscreencap) while targeting the first active display when multiple are present.Combined with the new tests in
devices/android_display_test.go, this should make screenshot behavior much less surprising on multi-display setups while remaining safe on older or quirky devices.
495-503: Android StartAgent ignoring progress config is acceptableGiven Android doesn’t require a separate agent process, updating the signature to
StartAgent(config StartAgentConfig)while returning early for offline devices and otherwise doing nothing is fine. It keeps the interface consistent across platforms without introducing unnecessary work on Android.server/server.go (2)
76-91: CORS preflight max-age header is fineAdding
Access-Control-Max-Age: 86400to the CORS middleware is a straightforward optimization for browsers and doesn’t affect non-CORS clients.
519-545: device_info now uses the config-based StartAgent API consistentlyCalling
targetDevice.StartAgent(devices.StartAgentConfig{})keeps the JSON-RPCdevice_infopath in sync with the new device interface without changing existing behavior, since no progress callback is required here.devices/simulator.go (2)
304-362: Simulator WDA installation progress is integrated cleanly
InstallWebDriverAgent(onProgress)now reports"Downloading WebDriverAgent"when a progress callback is provided, which ties nicely into the broader StartAgent progress story. The rest of the install pipeline is unchanged, so this adds observability without altering behavior.
452-560: Simulator StartAgent progress covers all key phases
StartAgent(config StartAgentConfig)for simulators now:
- Waits for boot completion with a
"Waiting for Simulator to boot"message in the Booting state.- Emits
"Installing WebDriverAgent on Simulator"and passesconfig.OnProgressdown intoInstallWebDriverAgent.- Announces
"Starting WebDriverAgent"before launch and"Waiting for agent to start"beforeWaitForAgent.This gives clients good visibility into what’s happening while remaining backward compatible when
OnProgressis nil.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.