Conversation
WalkthroughThis change introduces end-to-end screen recording functionality across the codebase. It adds a new CLI subcommand 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (2)
commands/screenrecord.go (2)
102-108: Starting the agent is unnecessary for Android and may cause errors for unimplemented device types.
StartAgentis always called beforeRecordVideo. For Android, this is a no-op. For iOS, simulator, and remote devices,RecordVideoreturns "not yet implemented" anyway, so starting the agent is wasted effort (or could fail, masking the real error). This is acceptable for now, but consider guarding with a platform check or moving agent start into the device'sRecordVideoimplementation when iOS support is added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/screenrecord.go` around lines 102 - 108, The code always calls targetDevice.StartAgent(...) before RecordVideo which is unnecessary for Android (no-op) and may fail or mask errors for unimplemented types (iOS/simulator/remote); update the logic to avoid starting the agent unconditionally—either add a platform/type check before calling StartAgent (e.g., only call StartAgent for device types that require it) or remove the StartAgent call here and move agent startup into each device's RecordVideo implementation (so StartAgent and GetShutdownHook are invoked only when needed).
58-123: Recording provides no progress feedback to the user during long operations.
RecordVideois called without anOnProgresscallback (Line 111), so the user sees no output during recording (which can last up to 5 minutes). The Android implementation supportsOnProgressfor "Recording video" and "Pulling recorded video" messages. Consider wiring it up, e.g., viafmt.Fprintf(os.Stderr, ...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/screenrecord.go` around lines 58 - 123, ScreenRecordCommand currently calls targetDevice.RecordVideo(devices.RecordVideoConfig{...}) without providing an OnProgress callback, so long recordings show no feedback; update the devices.RecordVideoConfig passed in ScreenRecordCommand to set OnProgress to a function that writes progress messages to stderr (e.g., fmt.Fprintf(os.Stderr, "...")), and if there is a separate pull step (e.g., PullRecordedVideo) also wire an OnProgress callback there—use the symbols ScreenRecordCommand, targetDevice.RecordVideo, and devices.RecordVideoConfig to locate and update the call site.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commands/screenrecord.go`:
- Around line 27-56: parseBitRate currently accepts zero and negative values
(e.g., "-5M"), which are passed to adb; after parsing the numeric value in
parseBitRate (both in the "M" and "K" branches and the plain-number branch),
validate that the resulting bit-rate (num or computed value) is > 0 and return
an error like "bit-rate must be positive" if not; you can either perform the
check in each branch after computing the int value or consolidate by converting
all branches to a single positive-int validation before returning.
In `@devices/android.go`:
- Around line 1264-1290: RecordVideo's local signal handling (sigChan +
signal.Notify) creates a race with the global shutdown and is Windows-unsafe;
remove the local signal registration and instead accept a context.Context (or
use the existing shutdown context) in RecordVideo so cancellation is driven by
main's hook, then when cancelling wait for cmd.Wait() to finish before pulling
the file; also replace the unhandled call to cmd.Process.Signal(syscall.SIGINT)
in the interrupt path with a platform-safe attempt that checks the error and
falls back to cmd.Process.Kill() on failure (or when runtime.GOOS == "windows")
so Windows users can stop recording cleanly.
---
Nitpick comments:
In `@commands/screenrecord.go`:
- Around line 102-108: The code always calls targetDevice.StartAgent(...) before
RecordVideo which is unnecessary for Android (no-op) and may fail or mask errors
for unimplemented types (iOS/simulator/remote); update the logic to avoid
starting the agent unconditionally—either add a platform/type check before
calling StartAgent (e.g., only call StartAgent for device types that require it)
or remove the StartAgent call here and move agent startup into each device's
RecordVideo implementation (so StartAgent and GetShutdownHook are invoked only
when needed).
- Around line 58-123: ScreenRecordCommand currently calls
targetDevice.RecordVideo(devices.RecordVideoConfig{...}) without providing an
OnProgress callback, so long recordings show no feedback; update the
devices.RecordVideoConfig passed in ScreenRecordCommand to set OnProgress to a
function that writes progress messages to stderr (e.g., fmt.Fprintf(os.Stderr,
"...")), and if there is a separate pull step (e.g., PullRecordedVideo) also
wire an OnProgress callback there—use the symbols ScreenRecordCommand,
targetDevice.RecordVideo, and devices.RecordVideoConfig to locate and update the
call site.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (8)
cli/flags.gocli/screenrecord.gocommands/screenrecord.godevices/android.godevices/common.godevices/ios.godevices/remote.godevices/simulator.go
| func parseBitRate(s string) (int, error) { | ||
| s = strings.TrimSpace(s) | ||
| if s == "" { | ||
| return 0, fmt.Errorf("empty bit-rate string") | ||
| } | ||
|
|
||
| upper := strings.ToUpper(s) | ||
|
|
||
| if strings.HasSuffix(upper, "M") { | ||
| num, err := strconv.ParseFloat(upper[:len(upper)-1], 64) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("invalid bit-rate: %w", err) | ||
| } | ||
| return int(num * 1_000_000), nil | ||
| } | ||
|
|
||
| if strings.HasSuffix(upper, "K") { | ||
| num, err := strconv.ParseFloat(upper[:len(upper)-1], 64) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("invalid bit-rate: %w", err) | ||
| } | ||
| return int(num * 1_000), nil | ||
| } | ||
|
|
||
| num, err := strconv.Atoi(s) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("invalid bit-rate: %w", err) | ||
| } | ||
| return num, nil | ||
| } |
There was a problem hiding this comment.
No validation that parsed bit-rate is positive.
parseBitRate successfully parses negative values (e.g., "-5M" → -5000000) and zero. These are passed directly to adb screenrecord --bit-rate, which will either reject them with a confusing error or produce unexpected behavior. Consider adding a positivity check after parsing.
Proposed fix
num, err := strconv.Atoi(s)
if err != nil {
return 0, fmt.Errorf("invalid bit-rate: %w", err)
}
+ if num <= 0 {
+ return 0, fmt.Errorf("bit-rate must be a positive value, got %d", num)
+ }
return num, nil
}The same check should be applied after the M and K suffix branches as well, or consolidated after all branches:
+func parseBitRate(s string) (int, error) {
+ // ... existing parsing logic ...
+ // After computing the final int value:
+ if result <= 0 {
+ return 0, fmt.Errorf("bit-rate must be a positive value")
+ }
+ return result, nil
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commands/screenrecord.go` around lines 27 - 56, parseBitRate currently
accepts zero and negative values (e.g., "-5M"), which are passed to adb; after
parsing the numeric value in parseBitRate (both in the "M" and "K" branches and
the plain-number branch), validate that the resulting bit-rate (num or computed
value) is > 0 and return an error like "bit-rate must be positive" if not; you
can either perform the check in each branch after computing the int value or
consolidate by converting all branches to a single positive-int validation
before returning.
| // set up signal handling so Ctrl+C stops recording gracefully | ||
| sigChan := make(chan os.Signal, 1) | ||
| signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM) | ||
| defer signal.Stop(sigChan) | ||
|
|
||
| if err := cmd.Start(); err != nil { | ||
| return fmt.Errorf("failed to start screenrecord: %w", err) | ||
| } | ||
|
|
||
| // wait for either process exit or signal | ||
| done := make(chan error, 1) | ||
| go func() { | ||
| done <- cmd.Wait() | ||
| }() | ||
|
|
||
| select { | ||
| case <-sigChan: | ||
| // user interrupted — send SIGINT to adb so screenrecord finalizes the mp4 | ||
| utils.Verbose("Interrupt received, stopping recording...") | ||
| _ = cmd.Process.Signal(syscall.SIGINT) | ||
| <-done // wait for process to finish writing | ||
| case err := <-done: | ||
| if err != nil { | ||
| // screenrecord may exit non-zero on some devices; log but continue | ||
| utils.Verbose("screenrecord exited with: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look for all signal handling in the codebase
rg -n 'signal\.Notify' --type=go -C3Repository: mobile-next/mobilecli
Length of output: 1461
🏁 Script executed:
# Check for GetShutdownHook usage
rg -n 'GetShutdownHook\|shutdown' --type=goRepository: mobile-next/mobilecli
Length of output: 47
🏁 Script executed:
# Look at the RecordVideo function context around the signal handling
fd -e go | xargs grep -l "RecordVideo\|signal.Notify" | head -5Repository: mobile-next/mobilecli
Length of output: 159
🏁 Script executed:
# Check the specific file context
wc -l devices/android.goRepository: mobile-next/mobilecli
Length of output: 88
🏁 Script executed:
# Check the ScreenRecordCommand to see if it uses shutdown hooks
rg -n 'ScreenRecordCommand\|GetShutdownHook' --type=go -C5Repository: mobile-next/mobilecli
Length of output: 47
🏁 Script executed:
# Look at the specific signal handling line in android.go to see exact error handling
sed -n '1280,1290p' devices/android.goRepository: mobile-next/mobilecli
Length of output: 478
🏁 Script executed:
# Search for Windows-specific handling or comments about Windows
rg -n 'windows\|Windows\|SIGINT' --type=go | grep -i windowsRepository: mobile-next/mobilecli
Length of output: 47
🏁 Script executed:
# Check if there's any build tags or Windows-specific signal handling
rg -n '//\s*\+build\|GOOS' --type=goRepository: mobile-next/mobilecli
Length of output: 47
🏁 Script executed:
# Look for where RecordVideo is called from
rg -n 'RecordVideo' --type=goRepository: mobile-next/mobilecli
Length of output: 722
🏁 Script executed:
# Check if there's a shutdown hook pattern in the codebase
rg -n 'shutdown\|Shutdown' --type=go | head -20Repository: mobile-next/mobilecli
Length of output: 47
🏁 Script executed:
# Look at main.go to see the full shutdown pattern
cat -n main.go | head -60Repository: mobile-next/mobilecli
Length of output: 1522
🏁 Script executed:
# Check what ShutdownHook does
rg -n 'NewShutdownHook\|type.*ShutdownHook' --type=go -A 15Repository: mobile-next/mobilecli
Length of output: 47
🏁 Script executed:
# Look at SetShutdownHook in commands package
rg -n 'SetShutdownHook' --type=go -C 5Repository: mobile-next/mobilecli
Length of output: 2619
🏁 Script executed:
# Check if the hook kills child processes or just cleans up
rg -n 'Shutdown\(\)' --type=go -C 3 | head -40Repository: mobile-next/mobilecli
Length of output: 1665
🏁 Script executed:
# Look at the full RecordVideo function to understand the complete flow
sed -n '1241,1311p' devices/android.goRepository: mobile-next/mobilecli
Length of output: 2260
🏁 Script executed:
# Search for how other RecordVideo implementations (iOS, simulator) handle signals
sed -n '1174,1250p' devices/ios.goRepository: mobile-next/mobilecli
Length of output: 2818
🏁 Script executed:
# Check if there's any cleanup registered in ScreenRecordCommand
cat -n commands/screenrecord.go | head -150Repository: mobile-next/mobilecli
Length of output: 4187
🏁 Script executed:
# Check if context.Context is used elsewhere in RecordVideo interface or similar functions
rg -n 'context.Context' --type=go | head -20Repository: mobile-next/mobilecli
Length of output: 257
🏁 Script executed:
# Verify Windows behavior of syscall.SIGINT - check documentation comments
rg -n 'windows\|Windows\|signal' devices/android.go -C 2Repository: mobile-next/mobilecli
Length of output: 47
Process-wide signal interception creates a race condition that can prevent file download, and graceful stop is broken on Windows.
Two critical issues with the signal handling:
-
Race condition with process shutdown:
signal.Notifyin RecordVideo registers for SIGINT/SIGTERM while main.go already has a process-wide handler registered for the same signals. In Go, both handlers receive the signal simultaneously. When Ctrl+C is pressed, main's signal handler (line 41 of main.go) callshook.Shutdown()andos.Exit(0)immediately, terminating the process before RecordVideo can complete the file pull (lines 1292–1296). This leaves the recording on the device or partially downloaded. -
Windows incompatibility:
cmd.Process.Signal(syscall.SIGINT)is not supported on Windows and silently fails (the error is discarded). Users on Windows cannot gracefully stop recording early and must wait for the full--time-limitduration, or kill the process (which may corrupt the MP4).
Fix: Remove the local signal handler in RecordVideo and instead pass a context.Context (tied to the main shutdown hook) to allow cancellation. Or, coordinate with main's handler to prevent process exit while the goroutine is still pulling the file. For Windows, fallback to cmd.Process.Kill() if Signal() fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@devices/android.go` around lines 1264 - 1290, RecordVideo's local signal
handling (sigChan + signal.Notify) creates a race with the global shutdown and
is Windows-unsafe; remove the local signal registration and instead accept a
context.Context (or use the existing shutdown context) in RecordVideo so
cancellation is driven by main's hook, then when cancelling wait for cmd.Wait()
to finish before pulling the file; also replace the unhandled call to
cmd.Process.Signal(syscall.SIGINT) in the interrupt path with a platform-safe
attempt that checks the error and falls back to cmd.Process.Kill() on failure
(or when runtime.GOOS == "windows") so Windows users can stop recording cleanly.
1 participant
No description provided.