feat: adding screencapture settings method #153
Conversation
WalkthroughAdds per-device TCP connection tracking and a method to send length-prefixed JSON-RPC screen-capture configuration (bitrate and optional frame rate) from server handlers to live iOS AVC streams; wires a new server RPC route and validates parameters. 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. ✨ Finishing touches
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
devices/ios.go (1)
695-737: Change StartScreenCapture to use pointer receiver.Line 695 uses a value receiver, so mutations to
screenCaptureConn(line 718) modify only a copy.SendScreenCaptureConfiguration(which uses a pointer receiver) will seeniland fail. Additionally, thesync.Mutexfield is unsafe to copy.🔧 Proposed fix
-func (d IOSDevice) StartScreenCapture(config ScreenCaptureConfig) error { +func (d *IOSDevice) StartScreenCapture(config ScreenCaptureConfig) error {
🤖 Fix all issues with AI agents
In `@devices/ios.go`:
- Around line 48-52: The build fails due to missing standard library imports
used by sync.Mutex, json.Marshal, and binary.BigEndian; update the import block
in ios.go to include "sync", "encoding/json", and "encoding/binary" so
references to sync.Mutex, json.Marshal, and binary.BigEndian resolve; ensure the
imports are added to the existing grouped import statement near the top of
ios.go so the file compiles.
In `@server/server.go`:
- Around line 295-299: The request struct ScreenCaptureSetConfigurationParams
uses snake_case JSON tags causing clients sending camelCase (deviceId/frameRate)
to be ignored; change the tags to camelCase (json:"deviceId",
json:"frameRate,omitempty") and ensure the bitrate tag matches your existing
convention (e.g., json:"bitrate" or json:"bitRate" to be consistent), and update
the corresponding screen-capture response struct that exposes
DeviceID/Bitrate/FrameRate (the struct used in the response near the screen
capture handlers) to use the same camelCase JSON field names so request and
response schemas align.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
devices/ios.go (1)
698-724: Change StartScreenCapture to use a pointer receiver to fix state mutations.Line 698 uses a value receiver, so mutations to
screenCaptureConnandscreenCaptureConnLockat lines 722–724 modify a copy ofIOSDevice. This breaksSendScreenCaptureConfiguration(line 809), which uses a pointer receiver and expects to read the stored connection—it will always findnil. Additionally,AndroidDeviceandSimulatorDeviceboth use pointer receivers for the same method, making this an inconsistency.Fix
-func (d IOSDevice) StartScreenCapture(config ScreenCaptureConfig) error { +func (d *IOSDevice) StartScreenCapture(config ScreenCaptureConfig) error {
🤖 Fix all issues with AI agents
In `@devices/ios.go`:
- Around line 735-740: The defer that currently calls conn.Close() and then
clears d.screenCaptureConn should handle the Close error per project convention
by discarding it (use blank assignment) rather than ignoring it; update the
defer closure that references conn, d.screenCaptureConnLock, and
d.screenCaptureConn to call the close as a blank assignment (e.g., _ =
conn.Close()), and make the same change for the other identical conn.Close()
occurrence related to screen capture teardown that also manipulates
d.screenCaptureConn or uses conn in a defer.
| defer func() { | ||
| conn.Close() | ||
| d.screenCaptureConnLock.Lock() | ||
| d.screenCaptureConn = nil | ||
| d.screenCaptureConnLock.Unlock() | ||
| }() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and look at the specified lines
git ls-files | grep -E "devices/ios\.go"Repository: mobile-next/mobilecli
Length of output: 79
🏁 Script executed:
# Read the file around the specified lines (735-740)
if [ -f "devices/ios.go" ]; then
wc -l devices/ios.go
sed -n '730,745p' devices/ios.go
fiRepository: mobile-next/mobilecli
Length of output: 469
🏁 Script executed:
# Check if linter configuration exists
git ls-files | grep -E "(\.golangci|\.eslintrc|linter|lint)" | head -20Repository: mobile-next/mobilecli
Length of output: 78
🏁 Script executed:
# Search for utils.Verbose function
find . -type f -name "*.go" | head -20 | xargs grep -l "func.*Verbose\|utils.Verbose" 2>/dev/null | head -5Repository: mobile-next/mobilecli
Length of output: 84
🏁 Script executed:
# Check for any errcheck configuration
grep -r "errcheck" . --include="*.yml" --include="*.yaml" --include="*.toml" --include="*.cfg" 2>/dev/null | head -10Repository: mobile-next/mobilecli
Length of output: 158
🏁 Script executed:
# Get exact lines 735-740
sed -n '735,740p' devices/ios.goRepository: mobile-next/mobilecli
Length of output: 208
🏁 Script executed:
# Check the golangci.yml errcheck configuration
cat .golangci.yml | grep -A 5 "errcheck:"Repository: mobile-next/mobilecli
Length of output: 156
🏁 Script executed:
# Find utils package and Verbose function
find . -type f -name "*.go" -path "*/utils/*" | head -10Repository: mobile-next/mobilecli
Length of output: 270
🏁 Script executed:
# Search for utils.Verbose function definition
rg "func.*Verbose" --type go -A 2Repository: mobile-next/mobilecli
Length of output: 423
🏁 Script executed:
# Check what type conn is by looking at context around the defer
sed -n '715,740p' devices/ios.goRepository: mobile-next/mobilecli
Length of output: 817
🏁 Script executed:
# Search for Close() error handling patterns in Go files
rg "Close\(\)" --type go -A 1 -B 1 | head -50Repository: mobile-next/mobilecli
Length of output: 1492
🏁 Script executed:
# Look for conn.Close() specifically
rg "conn\.Close\(\)" --type go -A 1 -B 1Repository: mobile-next/mobilecli
Length of output: 303
🏁 Script executed:
# Check how errors are handled in defer blocks
rg "defer.*Close" --type go -A 2 | head -60Repository: mobile-next/mobilecli
Length of output: 2134
🏁 Script executed:
# Check project conventions for handling errors in defer
rg "_.*:=" --type go | grep -i close | head -20Repository: mobile-next/mobilecli
Length of output: 47
Fix conn.Close() error handling to match project conventions.
Line 736 ignores the error from conn.Close(), which fails lint (errcheck). The project standard for handling Close() errors in defer blocks is to use blank assignment, not logging. Apply this throughout the defer block.
🔧 Suggested fix
defer func() {
- conn.Close()
+ _ = conn.Close()
d.screenCaptureConnLock.Lock()
d.screenCaptureConn = nil
d.screenCaptureConnLock.Unlock()
}()Additionally, line 762 has an identical unhandled conn.Close() that should be fixed the same way.
🧰 Tools
🪛 GitHub Check: lint
[failure] 736-736:
Error return value of conn.Close is not checked (errcheck)
🤖 Prompt for AI Agents
In `@devices/ios.go` around lines 735 - 740, The defer that currently calls
conn.Close() and then clears d.screenCaptureConn should handle the Close error
per project convention by discarding it (use blank assignment) rather than
ignoring it; update the defer closure that references conn,
d.screenCaptureConnLock, and d.screenCaptureConn to call the close as a blank
assignment (e.g., _ = conn.Close()), and make the same change for the other
identical conn.Close() occurrence related to screen capture teardown that also
manipulates d.screenCaptureConn or uses conn in a defer.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.