Conversation
WalkthroughAdds a new 🚥 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: 7
🤖 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 50-60: The AVC branch passes onData into withTimeLimit and later
calls the returned callback directly, which can panic if onData is nil; update
the StartScreenCapture call (devices.ScreenCaptureConfig used in the req.Format
== "avc" branch) to ensure onData is non-nil before wrapping it: if onData is
nil, replace it with a no-op handler (e.g., a function that safely ignores
inputs) and then call withTimeLimit on that safe handler, so
targetDevice.StartScreenCapture always receives a valid OnData callback and
runtime panics are prevented.
- Around line 39-47: The code unconditionally calls targetDevice.StartAgent with
devices.StartAgentConfig (OnProgress/Hook) which forces WDA for simulator-native
MP4 recording; change the logic so StartAgent is only invoked for recording
paths that require the agent (e.g., non-simulator or non-native-MP4 dispatch),
and skip StartAgent for the simulator path that uses simctl recordVideo (the
simulator MP4 branch referenced around the simulator handling at lines ~67-88).
Locate uses of targetDevice.StartAgent, devices.StartAgentConfig,
GetShutdownHook and the error return via NewErrorResponse and wrap the
StartAgent call in a conditional based on the dispatch/recorder type so the
native simulator path does not attempt to start the agent. Ensure any
cleanup/error handling still runs when skipped.
In `@devices/android.go`:
- Around line 985-994: The pull failure returns early and skips device cleanup,
leaving the remotePath (/sdcard/mobilecli-rec-*.mp4) on the device; modify the
block around d.runAdbCommand("pull", remotePath, localOutput) so that cleanup is
always attempted before returning—either run d.runAdbCommand("shell", "rm",
remotePath) immediately after a failed pull (ensuring you capture pullOutput and
err first) or use a defer/cleanup helper that calls d.runAdbCommand("shell",
"rm", remotePath) regardless of success; ensure the returned error still
includes the original pull error and pullOutput (preserve existing fmt.Errorf
formatting) while guaranteeing the remove is invoked.
In `@devices/simulator.go`:
- Around line 794-800: The select currently ignores cmd.Wait() errors (in
branches that handle <-sigChan / <-done) so recording failures are treated as
success; capture the result of cmd.Wait() instead of discarding it and propagate
it back to the caller (or return/wrap it) so failures are reported. Concretely,
in the select that uses sigChan, done, cmd and
cmd.Process.Signal(syscall.SIGINT), replace the ignored _ = cmd.Wait() with err
:= cmd.Wait() (or similar), handle that err in both the SIGINT-forwarding branch
and the done-only branch, and return or wrap that error from the surrounding
function so callers receive the recording failure. Ensure Process.Signal call
still runs before waiting and that any error returned from cmd.Wait() is not
swallowed.
In `@pkg/avc2mp4/mp4writer.go`:
- Around line 69-71: The code currently treats timestampUs == 0 as “missing” and
skips such access units; update the logic in the mp4 writer where units are
appended (the block that checks current and current.timestampUs) to only check
that current != nil and always append *current regardless of timestampUs so
timestamp 0 frames are preserved, and apply the same change to the other
identical check handling current/current.timestampUs (the second append site) to
avoid dropping valid frames with timestamp 0.
- Around line 41-44: Guard against non-monotonic timestamps before doing
unsigned/subtraction: ensure you compute durations and PTS using a safe signed
subtraction or clamp negative deltas to zero when computing firstTs :=
units[0].timestampUs, lastTs := units[len(units)-1].timestampUs and duration :=
time.Duration(lastTs-firstTs) * time.Microsecond, and similarly when computing
au.timestampUs-firstTs later; implement a small helper (e.g., safeDelta(a,b)
int64) that returns max(0, a-b) or convert to signed ints before subtracting,
and use that helper in the places referencing firstTs/lastTs and au.timestampUs
to avoid underflow and huge durations.
In `@pkg/avc2mp4/seiparser.go`:
- Around line 24-39: The SEI parsing currently returns immediately if the first
payloadType isn't 5, which skips later payloads; modify the parsing in
seiparser.go to iterate over all SEI payload blocks: wrap the
payloadType/payloadSize decoding and handling in a loop that continues while pos
< len(rbsp), for each block accumulate payloadType (payloadType += 255 while
rbsp[pos]==0xFF), then read payloadSize similarly, then if payloadType == 5
process the user_data_unregistered payload (existing handling for
MOBILENXTIMECODE) otherwise advance pos by payloadSize (with safe bounds checks)
and continue to the next payload; stop when pos reaches end or sizes are
invalid. Ensure you use the same local variables (rbsp, pos, payloadType,
payloadSize) and keep the existing processing logic for type 5.
ℹ️ 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 ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
cli/screenrecord.gocommands/screenrecord.godevices/android.godevices/simulator.gogo.modpkg/avc2mp4/mp4writer.gopkg/avc2mp4/nalparser.gopkg/avc2mp4/seiparser.gopkg/avc2mp4/seiparser_test.goserver/server.go
Comment on lines
+985
to
+994
| // pull the recording from device | ||
| utils.Verbose("Pulling recording from device...") | ||
| pullOutput, err := d.runAdbCommand("pull", remotePath, localOutput) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to pull recording: %w\n%s", err, string(pullOutput)) | ||
| } | ||
|
|
||
| // clean up device | ||
| _, _ = d.runAdbCommand("shell", "rm", remotePath) | ||
|
|
There was a problem hiding this comment.
Remote recording file can leak on pull failure.
At Line 989, early return skips Line 993 cleanup. Failed pulls leave /sdcard/mobilecli-rec-*.mp4 behind.
🧹 Proposed fix
func (d *AndroidDevice) ScreenRecord(localOutput string, timeLimit int) error {
remotePath := fmt.Sprintf("/sdcard/mobilecli-rec-%d.mp4", time.Now().UnixNano())
+ defer func() {
+ _, _ = d.runAdbCommand("shell", "rm", remotePath)
+ }()
@@
pullOutput, err := d.runAdbCommand("pull", remotePath, localOutput)
if err != nil {
return fmt.Errorf("failed to pull recording: %w\n%s", err, string(pullOutput))
}
-
- // clean up device
- _, _ = d.runAdbCommand("shell", "rm", remotePath)
return nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@devices/android.go` around lines 985 - 994, The pull failure returns early
and skips device cleanup, leaving the remotePath (/sdcard/mobilecli-rec-*.mp4)
on the device; modify the block around d.runAdbCommand("pull", remotePath,
localOutput) so that cleanup is always attempted before returning—either run
d.runAdbCommand("shell", "rm", remotePath) immediately after a failed pull
(ensuring you capture pullOutput and err first) or use a defer/cleanup helper
that calls d.runAdbCommand("shell", "rm", remotePath) regardless of success;
ensure the returned error still includes the original pull error and pullOutput
(preserve existing fmt.Errorf formatting) while guaranteeing the remove is
invoked.
Comment on lines
+794
to
+800
| select { | ||
| case <-sigChan: | ||
| // forward SIGINT to simctl so it finalizes the MP4 | ||
| _ = cmd.Process.Signal(syscall.SIGINT) | ||
| <-done | ||
| case <-done: | ||
| } |
There was a problem hiding this comment.
Propagate simctl recordVideo exit errors instead of always succeeding.
At Line 799 and Line 808, the cmd.Wait() error is ignored, so failed recordings can still return success.
🐛 Proposed fix
- select {
- case <-sigChan:
- // forward SIGINT to simctl so it finalizes the MP4
- _ = cmd.Process.Signal(syscall.SIGINT)
- <-done
- case <-done:
- }
+ var waitErr error
+ select {
+ case <-sigChan:
+ // forward SIGINT to simctl so it finalizes the MP4
+ _ = cmd.Process.Signal(syscall.SIGINT)
+ waitErr = <-done
+ case waitErr = <-done:
+ }
@@
signal.Stop(sigChan)
close(sigChan)
- return nil
+ if waitErr != nil {
+ return fmt.Errorf("simctl recordVideo failed: %w", waitErr)
+ }
+ return nil
}Also applies to: 806-809
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@devices/simulator.go` around lines 794 - 800, The select currently ignores
cmd.Wait() errors (in branches that handle <-sigChan / <-done) so recording
failures are treated as success; capture the result of cmd.Wait() instead of
discarding it and propagate it back to the caller (or return/wrap it) so
failures are reported. Concretely, in the select that uses sigChan, done, cmd
and cmd.Process.Signal(syscall.SIGINT), replace the ignored _ = cmd.Wait() with
err := cmd.Wait() (or similar), handle that err in both the SIGINT-forwarding
branch and the done-only branch, and return or wrap that error from the
surrounding function so callers receive the recording failure. Ensure
Process.Signal call still runs before waiting and that any error returned from
cmd.Wait() is not swallowed.
Comment on lines
+41
to
+44
| firstTs := units[0].timestampUs | ||
| lastTs := units[len(units)-1].timestampUs | ||
| duration := time.Duration(lastTs-firstTs) * time.Microsecond | ||
|
|
There was a problem hiding this comment.
Guard against non-monotonic timestamps before unsigned subtraction.
If timestamps ever go backward, lastTs-firstTs and au.timestampUs-firstTs underflow and produce huge duration/PTS values.
💡 Proposed fix
func Convert(avcData []byte, output io.WriteSeeker) (*ConvertResult, error) {
nalus := ParseNALUnits(avcData)
units := groupAccessUnits(nalus)
if len(units) == 0 {
return nil, fmt.Errorf("no access units with timestamps found")
}
+ for i := 1; i < len(units); i++ {
+ if units[i].timestampUs < units[i-1].timestampUs {
+ return nil, fmt.Errorf(
+ "non-monotonic timestamp at access unit %d: %d < %d",
+ i, units[i].timestampUs, units[i-1].timestampUs,
+ )
+ }
+ }
if err := writeMp4(units, output); err != nil {
return nil, err
}Also applies to: 110-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/avc2mp4/mp4writer.go` around lines 41 - 44, Guard against non-monotonic
timestamps before doing unsigned/subtraction: ensure you compute durations and
PTS using a safe signed subtraction or clamp negative deltas to zero when
computing firstTs := units[0].timestampUs, lastTs :=
units[len(units)-1].timestampUs and duration := time.Duration(lastTs-firstTs) *
time.Microsecond, and similarly when computing au.timestampUs-firstTs later;
implement a small helper (e.g., safeDelta(a,b) int64) that returns max(0, a-b)
or convert to signed ints before subtracting, and use that helper in the places
referencing firstTs/lastTs and au.timestampUs to avoid underflow and huge
durations.
Comment on lines
+69
to
+71
| if current != nil && current.timestampUs > 0 { | ||
| units = append(units, *current) | ||
| } |
There was a problem hiding this comment.
Don’t treat timestampUs == 0 as “missing timestamp”.
Line 69 and Line 91 currently drop access units with timestamp 0, which is a valid value and can discard the first frame (or all frames in short captures).
💡 Proposed fix
type accessUnit struct {
nalus []NALUnit
timestampUs uint64
+ hasTimestamp bool
}
@@
if isSlice || isSPS {
- if current != nil && current.timestampUs > 0 {
+ if current != nil && current.hasTimestamp {
units = append(units, *current)
}
current = &accessUnit{}
}
@@
if nalu.Type == nalTypeSEI {
if ts, ok := ParseTimestamp(nalu.Data); ok {
current.timestampUs = ts
+ current.hasTimestamp = true
continue // don't include custom SEI in muxed output
}
}
@@
- if current != nil && current.timestampUs > 0 {
+ if current != nil && current.hasTimestamp {
units = append(units, *current)
}Also applies to: 91-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/avc2mp4/mp4writer.go` around lines 69 - 71, The code currently treats
timestampUs == 0 as “missing” and skips such access units; update the logic in
the mp4 writer where units are appended (the block that checks current and
current.timestampUs) to only check that current != nil and always append
*current regardless of timestampUs so timestamp 0 frames are preserved, and
apply the same change to the other identical check handling
current/current.timestampUs (the second append site) to avoid dropping valid
frames with timestamp 0.
Comment on lines
+24
to
+39
| // parse payloadType (could be multi-byte per H.264 spec) | ||
| payloadType := 0 | ||
| for pos < len(rbsp) && rbsp[pos] == 0xFF { | ||
| payloadType += 255 | ||
| pos++ | ||
| } | ||
| if pos >= len(rbsp) { | ||
| return 0, false | ||
| } | ||
| payloadType += int(rbsp[pos]) | ||
| pos++ | ||
|
|
||
| // user_data_unregistered = 5 | ||
| if payloadType != 5 { | ||
| return 0, false | ||
| } |
There was a problem hiding this comment.
Parse all SEI payload blocks, not just the first one.
At Lines 37-39, parsing stops if the first payload is not user_data_unregistered (type 5). SEI NAL units may carry multiple payloads, so valid MOBILENXTIMECODE data can be skipped.
🔧 Proposed fix
func ParseTimestamp(nalData []byte) (uint64, bool) {
@@
- // parse payloadType (could be multi-byte per H.264 spec)
- payloadType := 0
- for pos < len(rbsp) && rbsp[pos] == 0xFF {
- payloadType += 255
- pos++
- }
- if pos >= len(rbsp) {
- return 0, false
- }
- payloadType += int(rbsp[pos])
- pos++
-
- // user_data_unregistered = 5
- if payloadType != 5 {
- return 0, false
- }
-
- // parse payloadSize
- payloadSize := 0
- for pos < len(rbsp) && rbsp[pos] == 0xFF {
- payloadSize += 255
- pos++
- }
- if pos >= len(rbsp) {
- return 0, false
- }
- payloadSize += int(rbsp[pos])
- pos++
-
- // need at least 16 (UUID) + 8 (timestamp) = 24 bytes
- if payloadSize < 24 || pos+24 > len(rbsp) {
- return 0, false
- }
-
- // check UUID
- var uuid [16]byte
- copy(uuid[:], rbsp[pos:pos+16])
- if uuid != mobilenxTimecodeUUID {
- return 0, false
- }
- pos += 16
-
- // read big-endian uint64 timestamp in microseconds
- ts := binary.BigEndian.Uint64(rbsp[pos : pos+8])
- return ts, true
+ for pos < len(rbsp) {
+ payloadType := 0
+ for pos < len(rbsp) && rbsp[pos] == 0xFF {
+ payloadType += 255
+ pos++
+ }
+ if pos >= len(rbsp) {
+ return 0, false
+ }
+ payloadType += int(rbsp[pos])
+ pos++
+
+ payloadSize := 0
+ for pos < len(rbsp) && rbsp[pos] == 0xFF {
+ payloadSize += 255
+ pos++
+ }
+ if pos >= len(rbsp) {
+ return 0, false
+ }
+ payloadSize += int(rbsp[pos])
+ pos++
+
+ if pos+payloadSize > len(rbsp) {
+ return 0, false
+ }
+
+ if payloadType == 5 && payloadSize >= 24 {
+ payload := rbsp[pos : pos+payloadSize]
+ var uuid [16]byte
+ copy(uuid[:], payload[:16])
+ if uuid == mobilenxTimecodeUUID {
+ return binary.BigEndian.Uint64(payload[16:24]), true
+ }
+ }
+
+ pos += payloadSize
+ }
+ return 0, false
}Also applies to: 41-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/avc2mp4/seiparser.go` around lines 24 - 39, The SEI parsing currently
returns immediately if the first payloadType isn't 5, which skips later
payloads; modify the parsing in seiparser.go to iterate over all SEI payload
blocks: wrap the payloadType/payloadSize decoding and handling in a loop that
continues while pos < len(rbsp), for each block accumulate payloadType
(payloadType += 255 while rbsp[pos]==0xFF), then read payloadSize similarly,
then if payloadType == 5 process the user_data_unregistered payload (existing
handling for MOBILENXTIMECODE) otherwise advance pos by payloadSize (with safe
bounds checks) and continue to the next payload; stop when pos reaches end or
sizes are invalid. Ensure you use the same local variables (rbsp, pos,
payloadType, payloadSize) and keep the existing processing logic for type 5.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
cli/screenrecord.go (1)
40-45: Consider marking--outputas required via Cobra.Using
MarkFlagRequiredwould provide better UX by showing the flag as required in help text and providing a standard Cobra error message, rather than a custom error in RunE.♻️ Suggested improvement
func init() { rootCmd.AddCommand(screenrecordCmd) screenrecordCmd.Flags().StringVarP(&screenrecordOutput, "output", "o", "", "Output MP4 file path") screenrecordCmd.Flags().IntVar(&screenrecordTimeLimit, "time-limit", 0, "Max recording duration in seconds (0 = no limit)") + screenrecordCmd.MarkFlagRequired("output") }Then you can remove the manual validation in RunE (lines 20-22).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/screenrecord.go` around lines 40 - 45, Add Cobra's required flag marker for the output flag in init(): call screenrecordCmd.MarkFlagRequired("output") (handle the returned error if any). This replaces the custom check in screenrecordCmd.RunE that validates screenrecordOutput—remove that manual validation code in RunE so Cobra handles missing --output via its built-in error/help behavior. Ensure you reference the screenrecordCmd, screenrecordOutput, and init() symbols when making the change.commands/screenrecord.go (1)
107-111: Loading entire recording into memory may cause OOM for long captures.
os.ReadFile(tempPath)loads the entire AVC stream into memory. For lengthy recordings, this could exhaust available memory. Consider streaming the conversion ifavc2mp4.Convertsupports anio.Readerinterface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/screenrecord.go` around lines 107 - 111, The code reads the whole temp AVC file into memory via os.ReadFile(tempPath) which can OOM for long recordings; instead open the file with os.Open(tempPath) (remember to defer Close) and stream the file into the converter by passing the io.Reader to the streaming overload of avc2mp4 (e.g., avc2mp4.Convert/ConvertFromReader) rather than a byte slice; update the error handling to surface open/convert errors and remove the os.ReadFile usage so the conversion operates without loading the entire file into memory.
🤖 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 141-160: In screenRecordNative the signal handling is wrong: the
first signal.Reset should be signal.Ignore to prevent the main handler from
triggering, and the second should restore defaults with signal.Reset; update the
calls in screenRecordNative (mirroring the fix for screenRecordMp4) so the first
call uses signal.Ignore(syscall.SIGINT, syscall.SIGTERM) and the later call uses
signal.Reset(syscall.SIGINT, syscall.SIGTERM) to restore normal behavior.
- Around line 78-99: The two identical signal.Reset(syscall.SIGINT,
syscall.SIGTERM) calls mean signals are already defaulted before
StartScreenCapture; change the first call to signal.Ignore(syscall.SIGINT,
syscall.SIGTERM) so signals are ignored during capture and keep the second call
to signal.Reset(...) after targetDevice.StartScreenCapture to restore default
behavior; update the comment above the first call to indicate we are ignoring
Ctrl+C during StartScreenCapture and reference StartScreenCapture and the
surrounding OnData/withTimeLimit/tempFile flow to ensure the ignore spans the
entire capture duration.
- Around line 101-105: The temp file handle created for screen recording is
being closed with tempFile.Close() whose error is ignored and is not guaranteed
to run if StartScreenCapture returns early; change the code to call defer func()
{ if err := tempFile.Close(); err != nil { /* log or handle error */ } }()
immediately after creating tempFile (or otherwise ensure Close runs on all
returns), and ensure any Close error is checked/handled before returning
NewErrorResponse in StartScreenCapture so the file handle is not leaked and
Close failures are not discarded.
---
Nitpick comments:
In `@cli/screenrecord.go`:
- Around line 40-45: Add Cobra's required flag marker for the output flag in
init(): call screenrecordCmd.MarkFlagRequired("output") (handle the returned
error if any). This replaces the custom check in screenrecordCmd.RunE that
validates screenrecordOutput—remove that manual validation code in RunE so Cobra
handles missing --output via its built-in error/help behavior. Ensure you
reference the screenrecordCmd, screenrecordOutput, and init() symbols when
making the change.
In `@commands/screenrecord.go`:
- Around line 107-111: The code reads the whole temp AVC file into memory via
os.ReadFile(tempPath) which can OOM for long recordings; instead open the file
with os.Open(tempPath) (remember to defer Close) and stream the file into the
converter by passing the io.Reader to the streaming overload of avc2mp4 (e.g.,
avc2mp4.Convert/ConvertFromReader) rather than a byte slice; update the error
handling to surface open/convert errors and remove the os.ReadFile usage so the
conversion operates without loading the entire file into memory.
ℹ️ 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 (2)
cli/screenrecord.gocommands/screenrecord.go
commands/screenrecord.go
Outdated
Comment on lines
+78
to
+99
| // prevent main.go's signal handler from calling os.Exit(0) before | ||
| // we finish converting. StartScreenCapture sets up its own handler | ||
| // internally and will return when Ctrl+C is pressed. | ||
| signal.Reset(syscall.SIGINT, syscall.SIGTERM) | ||
|
|
||
| err = targetDevice.StartScreenCapture(devices.ScreenCaptureConfig{ | ||
| Format: "avc", | ||
| Quality: devices.DefaultQuality, | ||
| Scale: devices.DefaultScale, | ||
| FPS: devices.DefaultFramerate, | ||
| OnProgress: func(message string) { | ||
| utils.Verbose(message) | ||
| }, | ||
| OnData: withTimeLimit(func(data []byte) bool { | ||
| _, writeErr := tempFile.Write(data) | ||
| return writeErr == nil | ||
| }, req.TimeLimit), | ||
| }) | ||
|
|
||
| // restore default signal behavior so Ctrl+C during conversion | ||
| // terminates immediately | ||
| signal.Reset(syscall.SIGINT, syscall.SIGTERM) |
There was a problem hiding this comment.
Both signal.Reset calls are identical and comments are misleading.
Lines 81 and 99 both call signal.Reset(syscall.SIGINT, syscall.SIGTERM) which resets signals to their default behavior. The comment on line 97-99 says "restore default signal behavior" but line 81 already did that.
If the intent was to ignore signals during capture, you should use signal.Ignore() on line 81, then signal.Reset() on line 99 to restore.
🐛 Proposed fix to actually ignore signals during capture
// prevent main.go's signal handler from calling os.Exit(0) before
// we finish converting. StartScreenCapture sets up its own handler
// internally and will return when Ctrl+C is pressed.
- signal.Reset(syscall.SIGINT, syscall.SIGTERM)
+ signal.Ignore(syscall.SIGINT, syscall.SIGTERM)
err = targetDevice.StartScreenCapture(devices.ScreenCaptureConfig{
...
})
// restore default signal behavior so Ctrl+C during conversion
// terminates immediately
signal.Reset(syscall.SIGINT, syscall.SIGTERM)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // prevent main.go's signal handler from calling os.Exit(0) before | |
| // we finish converting. StartScreenCapture sets up its own handler | |
| // internally and will return when Ctrl+C is pressed. | |
| signal.Reset(syscall.SIGINT, syscall.SIGTERM) | |
| err = targetDevice.StartScreenCapture(devices.ScreenCaptureConfig{ | |
| Format: "avc", | |
| Quality: devices.DefaultQuality, | |
| Scale: devices.DefaultScale, | |
| FPS: devices.DefaultFramerate, | |
| OnProgress: func(message string) { | |
| utils.Verbose(message) | |
| }, | |
| OnData: withTimeLimit(func(data []byte) bool { | |
| _, writeErr := tempFile.Write(data) | |
| return writeErr == nil | |
| }, req.TimeLimit), | |
| }) | |
| // restore default signal behavior so Ctrl+C during conversion | |
| // terminates immediately | |
| signal.Reset(syscall.SIGINT, syscall.SIGTERM) | |
| // prevent main.go's signal handler from calling os.Exit(0) before | |
| // we finish converting. StartScreenCapture sets up its own handler | |
| // internally and will return when Ctrl+C is pressed. | |
| signal.Ignore(syscall.SIGINT, syscall.SIGTERM) | |
| err = targetDevice.StartScreenCapture(devices.ScreenCaptureConfig{ | |
| Format: "avc", | |
| Quality: devices.DefaultQuality, | |
| Scale: devices.DefaultScale, | |
| FPS: devices.DefaultFramerate, | |
| OnProgress: func(message string) { | |
| utils.Verbose(message) | |
| }, | |
| OnData: withTimeLimit(func(data []byte) bool { | |
| _, writeErr := tempFile.Write(data) | |
| return writeErr == nil | |
| }, req.TimeLimit), | |
| }) | |
| // restore default signal behavior so Ctrl+C during conversion | |
| // terminates immediately | |
| signal.Reset(syscall.SIGINT, syscall.SIGTERM) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commands/screenrecord.go` around lines 78 - 99, The two identical
signal.Reset(syscall.SIGINT, syscall.SIGTERM) calls mean signals are already
defaulted before StartScreenCapture; change the first call to
signal.Ignore(syscall.SIGINT, syscall.SIGTERM) so signals are ignored during
capture and keep the second call to signal.Reset(...) after
targetDevice.StartScreenCapture to restore default behavior; update the comment
above the first call to indicate we are ignoring Ctrl+C during
StartScreenCapture and reference StartScreenCapture and the surrounding
OnData/withTimeLimit/tempFile flow to ensure the ignore spans the entire capture
duration.
Comment on lines
+101
to
+105
| tempFile.Close() | ||
|
|
||
| if err != nil { | ||
| return NewErrorResponse(fmt.Errorf("error during screen capture: %w", err)) | ||
| } |
There was a problem hiding this comment.
Close error is ignored; defer placement may leak file handle on early return.
tempFile.Close() error is discarded. More importantly, if StartScreenCapture returns an error, the file should still be properly closed. Consider using defer tempFile.Close() right after creation, or at minimum check the close error.
🛡️ Proposed fix
tempFile, err := os.CreateTemp("", "screenrecord-*.avc")
if err != nil {
return NewErrorResponse(fmt.Errorf("error creating temp file: %w", err))
}
tempPath := tempFile.Name()
defer os.Remove(tempPath)
+ defer tempFile.Close()
// prevent main.go's signal handler...
signal.Reset(syscall.SIGINT, syscall.SIGTERM)
err = targetDevice.StartScreenCapture(devices.ScreenCaptureConfig{
...
})
// restore default signal behavior...
signal.Reset(syscall.SIGINT, syscall.SIGTERM)
- tempFile.Close()
-
if err != nil {Note: The deferred Close will run after the deferred Remove, but on most systems removing an open file is fine (the data persists until the handle is closed). Alternatively, use a named return and close explicitly before reading.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| tempFile.Close() | |
| if err != nil { | |
| return NewErrorResponse(fmt.Errorf("error during screen capture: %w", err)) | |
| } | |
| tempFile, err := os.CreateTemp("", "screenrecord-*.avc") | |
| if err != nil { | |
| return NewErrorResponse(fmt.Errorf("error creating temp file: %w", err)) | |
| } | |
| tempPath := tempFile.Name() | |
| defer os.Remove(tempPath) | |
| defer tempFile.Close() | |
| // prevent main.go's signal handler... | |
| signal.Reset(syscall.SIGINT, syscall.SIGTERM) | |
| err = targetDevice.StartScreenCapture(devices.ScreenCaptureConfig{ | |
| OnData: onData, | |
| Stop: stopChan, | |
| }) | |
| // restore default signal behavior... | |
| signal.Reset(syscall.SIGINT, syscall.SIGTERM) | |
| if err != nil { | |
| return NewErrorResponse(fmt.Errorf("error during screen capture: %w", err)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commands/screenrecord.go` around lines 101 - 105, The temp file handle
created for screen recording is being closed with tempFile.Close() whose error
is ignored and is not guaranteed to run if StartScreenCapture returns early;
change the code to call defer func() { if err := tempFile.Close(); err != nil {
/* log or handle error */ } }() immediately after creating tempFile (or
otherwise ensure Close runs on all returns), and ensure any Close error is
checked/handled before returning NewErrorResponse in StartScreenCapture so the
file handle is not leaked and Close failures are not discarded.
Comment on lines
+141
to
+160
| func screenRecordNative(record func() error, req ScreenRecordRequest) *CommandResponse { | ||
| // prevent main.go's signal handler from calling os.Exit(0) before | ||
| // the recording tool finishes. ScreenRecord sets up its own handler. | ||
| signal.Reset(syscall.SIGINT, syscall.SIGTERM) | ||
|
|
||
| err := record() | ||
|
|
||
| // restore default signal behavior | ||
| signal.Reset(syscall.SIGINT, syscall.SIGTERM) | ||
|
|
||
| if err != nil { | ||
| return NewErrorResponse(fmt.Errorf("error during screen recording: %w", err)) | ||
| } | ||
|
|
||
| fmt.Fprintf(os.Stderr, "wrote %s\n", req.OutputPath) | ||
|
|
||
| return NewSuccessResponse(ScreenRecordResponse{ | ||
| Output: req.OutputPath, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Same redundant signal.Reset pattern as screenRecordMp4.
Lines 144 and 149 have the same issue as noted in screenRecordMp4 - both calls reset signals to default behavior rather than first ignoring then restoring.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commands/screenrecord.go` around lines 141 - 160, In screenRecordNative the
signal handling is wrong: the first signal.Reset should be signal.Ignore to
prevent the main handler from triggering, and the second should restore defaults
with signal.Reset; update the calls in screenRecordNative (mirroring the fix for
screenRecordMp4) so the first call uses signal.Ignore(syscall.SIGINT,
syscall.SIGTERM) and the later call uses signal.Reset(syscall.SIGINT,
syscall.SIGTERM) to restore normal behavior.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
devices/simulator.go (1)
799-808:⚠️ Potential issue | 🟠 Major
simctl recordVideofailures are still swallowed.Line 803, Line 806, and Line 807 consume
cmd.Wait()without propagating errors, so failed recordings can return success.🐛 Suggested fix
- select { + var waitErr error + select { case <-sigChan: // forward SIGINT to simctl so it finalizes the MP4 _ = cmd.Process.Signal(syscall.SIGINT) - <-done + waitErr = <-done case <-stopChan: _ = cmd.Process.Signal(syscall.SIGINT) - <-done - case <-done: + waitErr = <-done + case waitErr = <-done: } @@ signal.Stop(sigChan) close(sigChan) - return nil + if waitErr != nil { + return fmt.Errorf("simctl recordVideo failed: %w", waitErr) + } + return nil }Also applies to: 816-817
devices/android.go (1)
953-1004:⚠️ Potential issue | 🟡 MinorRemote recording cleanup is still skipped on pull failure.
At Line 998, an early return bypasses cleanup currently done at Line 1003, so
remotePathcan remain on device when pull fails.🧹 Suggested fix
func (d *AndroidDevice) ScreenRecord(localOutput string, timeLimit int, stopChan <-chan struct{}) error { @@ remotePath := fmt.Sprintf("/sdcard/mobilecli-rec-%d.mp4", time.Now().UnixNano()) + defer func() { + _, _ = d.runAdbCommand("shell", "rm", remotePath) + }() @@ pullOutput, err := d.runAdbCommand("pull", remotePath, localOutput) if err != nil { return fmt.Errorf("failed to pull recording: %w\n%s", err, string(pullOutput)) } - - // clean up device - _, _ = d.runAdbCommand("shell", "rm", remotePath) return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@devices/android.go` around lines 953 - 1004, The pull failure return currently skips device cleanup leaving remotePath (variable remotePath) behind; ensure the recording file is always removed by running d.runAdbCommand("shell", "rm", remotePath) before returning on pull errors or, better, add a defer right after remotePath is created (e.g., after remotePath := ...), calling a cleanup wrapper that ignores errors and removes the file via d.runAdbCommand, and then keep the existing error return (use the captured pullOutput and err when returning). Reference the remotePath variable, the d.runAdbCommand("pull", ...) call and the d.runAdbCommand("shell", "rm", remotePath) cleanup so the removal always runs even if pull fails.commands/screenrecord.go (1)
37-45:⚠️ Potential issue | 🟠 MajorAvoid unconditional
StartAgentbefore native recording dispatch.Line 37 starts the agent for all paths. For simulator native MP4 recording (Line 63 onward), this adds an unnecessary WDA dependency and can fail recording before
simctl recordVideois even attempted.🐛 Suggested fix
func ScreenRecordCommand(req ScreenRecordRequest) *CommandResponse { targetDevice, err := FindDeviceOrAutoSelect(req.DeviceID) if err != nil { return NewErrorResponse(fmt.Errorf("error finding device: %w", err)) } - err = targetDevice.StartAgent(devices.StartAgentConfig{ - OnProgress: func(message string) { - utils.Verbose(message) - }, - Hook: GetShutdownHook(), - }) - if err != nil { - return NewErrorResponse(fmt.Errorf("error starting agent: %w", err)) - } + startAgent := func() error { + return targetDevice.StartAgent(devices.StartAgentConfig{ + OnProgress: func(message string) { + utils.Verbose(message) + }, + Hook: GetShutdownHook(), + }) + } @@ case targetDevice.Platform() == "ios" && targetDevice.DeviceType() == "real": + if err := startAgent(); err != nil { + return NewErrorResponse(fmt.Errorf("error starting agent: %w", err)) + } return screenRecordIOSDevice(targetDevice, req)Also applies to: 63-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/screenrecord.go` around lines 37 - 45, The code unconditionally calls targetDevice.StartAgent (with OnProgress and Hook via GetShutdownHook) before deciding whether to use simulator native recording (simctl recordVideo); move or guard the StartAgent call so it only runs for paths that actually require WDA/agent (e.g., physical device or the recording path that uses the agent) and skip it for the simulator native MP4 recording branch (the simctl recordVideo logic). Update the logic around the StartAgent invocation and the simulator branch (the block handling simctl recordVideo) so StartAgent is invoked conditionally (use the device/simulator check on the targetDevice or the recording mode) and preserve the existing OnProgress/Hook handling for the agent-required path; apply the same conditional fix to the duplicate StartAgent usage referenced in the later block.
🧹 Nitpick comments (2)
devices/common.go (1)
82-85: Consider explicit boolean parsing for the environment variable.The current check treats any non-empty value (including
"false","0","no") as truthy, which could confuse users who expect standard boolean semantics. This is a common Go pattern, but worth considering for clarity.♻️ Optional: explicit truthy check
- if os.Getenv("MOBILECLI_REMOTE_ONLY") != "" { + if val := os.Getenv("MOBILECLI_REMOTE_ONLY"); val == "1" || val == "true" { return allDevices, nil }Alternatively, add a brief comment documenting the expected values:
+ // MOBILECLI_REMOTE_ONLY: when set (any non-empty value), skip local device discovery if os.Getenv("MOBILECLI_REMOTE_ONLY") != "" { return allDevices, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@devices/common.go` around lines 82 - 85, The environment check for MOBILECLI_REMOTE_ONLY should parse it as a boolean instead of treating any non-empty string as true; modify the code around the existing os.Getenv usage in devices/common.go (the block that currently returns allDevices when MOBILECLI_REMOTE_ONLY != "") to use os.LookupEnv and strconv.ParseBool (or equivalent) so only explicit truthy values ("1", "t", "T", "true", "TRUE", "True") enable remote-only behavior, handle parse errors by treating them as false (or log a warning), and keep the return of allDevices only when the parsed boolean is true.server/server.go (1)
1060-1066:device.screenrecord.stopcontract and implementation are misaligned.
ScreenRecordStopParamsis defined at Line 1061, buthandleScreenRecordStopat Line 1065 ignoresparams. This makesdeviceIdlook supported while being unused.♻️ Suggested alignment (minimum)
func handleScreenRecordStop(params json.RawMessage) (any, error) { + if len(params) > 0 { + var p ScreenRecordStopParams + if err := json.Unmarshal(params, &p); err != nil { + return nil, fmt.Errorf("invalid parameters: %w. Expected fields: deviceId (optional)", err) + } + _ = p // currently single-session global stop; keep for API contract clarity + } + session, err := recorder.stop() if err != nil { return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/server.go` around lines 1060 - 1066, handleScreenRecordStop currently ignores the incoming params even though ScreenRecordStopParams declares a deviceId, so parse the json.RawMessage into ScreenRecordStopParams inside handleScreenRecordStop, validate that DeviceID is present, and pass it to the recorder stop logic (e.g., call recorder.stop(DeviceID) or select the correct session before calling recorder.stop()). Update recorder.stop usage or its signature if necessary so the deviceId is actually used and remove any misleading unused param behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@devices/remote.go`:
- Around line 272-281: The downloadFile function currently calls http.Get on an
unvalidated URL with no timeout; fix it by parsing and validating downloadURL
(use url.Parse), ensure the scheme is http or https, and reject non-HTTP schemes
or empty hosts, then create an http.Client with a sensible Timeout and a
restrictive Transport (no persistent Dial without timeouts) and use that client
to perform the request (or use context with timeout); additionally, consider
resolving the host/IP and block private/loopback ranges to mitigate SSRF before
issuing the request.
---
Duplicate comments:
In `@commands/screenrecord.go`:
- Around line 37-45: The code unconditionally calls targetDevice.StartAgent
(with OnProgress and Hook via GetShutdownHook) before deciding whether to use
simulator native recording (simctl recordVideo); move or guard the StartAgent
call so it only runs for paths that actually require WDA/agent (e.g., physical
device or the recording path that uses the agent) and skip it for the simulator
native MP4 recording branch (the simctl recordVideo logic). Update the logic
around the StartAgent invocation and the simulator branch (the block handling
simctl recordVideo) so StartAgent is invoked conditionally (use the
device/simulator check on the targetDevice or the recording mode) and preserve
the existing OnProgress/Hook handling for the agent-required path; apply the
same conditional fix to the duplicate StartAgent usage referenced in the later
block.
In `@devices/android.go`:
- Around line 953-1004: The pull failure return currently skips device cleanup
leaving remotePath (variable remotePath) behind; ensure the recording file is
always removed by running d.runAdbCommand("shell", "rm", remotePath) before
returning on pull errors or, better, add a defer right after remotePath is
created (e.g., after remotePath := ...), calling a cleanup wrapper that ignores
errors and removes the file via d.runAdbCommand, and then keep the existing
error return (use the captured pullOutput and err when returning). Reference the
remotePath variable, the d.runAdbCommand("pull", ...) call and the
d.runAdbCommand("shell", "rm", remotePath) cleanup so the removal always runs
even if pull fails.
---
Nitpick comments:
In `@devices/common.go`:
- Around line 82-85: The environment check for MOBILECLI_REMOTE_ONLY should
parse it as a boolean instead of treating any non-empty string as true; modify
the code around the existing os.Getenv usage in devices/common.go (the block
that currently returns allDevices when MOBILECLI_REMOTE_ONLY != "") to use
os.LookupEnv and strconv.ParseBool (or equivalent) so only explicit truthy
values ("1", "t", "T", "true", "TRUE", "True") enable remote-only behavior,
handle parse errors by treating them as false (or log a warning), and keep the
return of allDevices only when the parsed boolean is true.
In `@server/server.go`:
- Around line 1060-1066: handleScreenRecordStop currently ignores the incoming
params even though ScreenRecordStopParams declares a deviceId, so parse the
json.RawMessage into ScreenRecordStopParams inside handleScreenRecordStop,
validate that DeviceID is present, and pass it to the recorder stop logic (e.g.,
call recorder.stop(DeviceID) or select the correct session before calling
recorder.stop()). Update recorder.stop usage or its signature if necessary so
the deviceId is actually used and remove any misleading unused param behavior.
ℹ️ 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 (9)
commands/screenrecord.godevices/android.godevices/common.godevices/remote.godevices/simulator.gorpc/rpc.goserver/dispatch.goserver/recording.goserver/server.go
Comment on lines
+272
to
+281
| func downloadFile(downloadURL, outputPath string) error { | ||
| resp, err := http.Get(downloadURL) | ||
| if err != nil { | ||
| return fmt.Errorf("HTTP GET failed: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode < 200 || resp.StatusCode >= 300 { | ||
| return fmt.Errorf("download returned status %d", resp.StatusCode) | ||
| } |
There was a problem hiding this comment.
Harden downloadFile: validate URL and avoid no-timeout GET.
Line 273 uses http.Get on a server-provided URL without validation and without timeout. That can enable SSRF-style requests from the CLI host and can hang indefinitely.
🔒 Suggested fix
+var downloadHTTPClient = &http.Client{Timeout: 5 * time.Minute}
+
func downloadFile(downloadURL, outputPath string) error {
- resp, err := http.Get(downloadURL)
+ u, err := url.Parse(downloadURL)
+ if err != nil {
+ return fmt.Errorf("invalid download URL: %w", err)
+ }
+ if u.Scheme != "https" || u.Host != "mobilenexthq-artifacts.s3.us-west-2.amazonaws.com" {
+ return fmt.Errorf("download URL must be https://mobilenexthq-artifacts.s3.us-west-2.amazonaws.com/..., got: %s", downloadURL)
+ }
+
+ req, err := http.NewRequest(http.MethodGet, u.String(), nil)
+ if err != nil {
+ return fmt.Errorf("failed to create download request: %w", err)
+ }
+
+ resp, err := downloadHTTPClient.Do(req)
if err != nil {
return fmt.Errorf("HTTP GET failed: %w", err)
}
defer resp.Body.Close()🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 273-273: Uncontrolled data used in network request
The URL of this request depends on a user-provided value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@devices/remote.go` around lines 272 - 281, The downloadFile function
currently calls http.Get on an unvalidated URL with no timeout; fix it by
parsing and validating downloadURL (use url.Parse), ensure the scheme is http or
https, and reject non-HTTP schemes or empty hosts, then create an http.Client
with a sensible Timeout and a restrictive Transport (no persistent Dial without
timeouts) and use that client to perform the request (or use context with
timeout); additionally, consider resolving the host/IP and block
private/loopback ranges to mitigate SSRF before issuing the request.
| return fmt.Errorf("download URL must use HTTPS scheme, got %q", parsed.Scheme) | ||
| } | ||
|
|
||
| resp, err := http.Get(parsed.String()) //nolint:gosec // URL is validated above |
Check failure
Code scanning / CodeQL
Uncontrolled data used in network request Critical
There was a problem hiding this comment.
♻️ Duplicate comments (1)
devices/remote.go (1)
272-303:⚠️ Potential issue | 🟠 MajorHarden
downloadFile: unrestricted HTTPS target + no timeout on Line 281.Line 277 only checks scheme, so arbitrary HTTPS hosts are still allowed; Line 281 uses
http.Getwithout a timeout. This keeps the SSRF/vector and hang risk open.🔒 Proposed fix
+var downloadHTTPClient = &http.Client{ + Timeout: 5 * time.Minute, +} + func downloadFile(downloadURL, outputPath string) error { parsed, err := url.Parse(downloadURL) if err != nil { return fmt.Errorf("invalid download URL: %w", err) } - if parsed.Scheme != "https" { - return fmt.Errorf("download URL must use HTTPS scheme, got %q", parsed.Scheme) + if parsed.Scheme != "https" || parsed.Host != "mobilenexthq-artifacts.s3.us-west-2.amazonaws.com" { + return fmt.Errorf("download URL must be https://mobilenexthq-artifacts.s3.us-west-2.amazonaws.com/..., got: %s", downloadURL) } - resp, err := http.Get(parsed.String()) //nolint:gosec // URL is validated above + req, err := http.NewRequest(http.MethodGet, parsed.String(), nil) + if err != nil { + return fmt.Errorf("failed to create download request: %w", err) + } + + resp, err := downloadHTTPClient.Do(req) if err != nil { return fmt.Errorf("HTTP GET failed: %w", err) }#!/bin/bash # Verify whether download path is still unconstrained and using default client. rg -n -C3 'func downloadFile|parsed\.Scheme|parsed\.Host|http\.Get\(|http\.Client|Timeout|CheckRedirect' devices/remote.goExpected: host allowlisting and a bounded client timeout should both be present, and
http.Get(should not be used here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@devices/remote.go` around lines 272 - 303, The downloadFile function currently accepts any HTTPS host and calls http.Get without a timeout; update downloadFile to validate parsed.Host against an allowlist (or resolve and verify IP ranges) before requesting, replace http.Get(parsed.String()) with a custom http.Client that has a sensible Timeout and no insecure Redirects (set CheckRedirect to prevent following to disallowed hosts), and use client.Get(...) so requests cannot hang or be redirected to attacker-controlled hosts; ensure errors include context and keep existing file write/close behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@devices/remote.go`:
- Around line 272-303: The downloadFile function currently accepts any HTTPS
host and calls http.Get without a timeout; update downloadFile to validate
parsed.Host against an allowlist (or resolve and verify IP ranges) before
requesting, replace http.Get(parsed.String()) with a custom http.Client that has
a sensible Timeout and no insecure Redirects (set CheckRedirect to prevent
following to disallowed hosts), and use client.Get(...) so requests cannot hang
or be redirected to attacker-controlled hosts; ensure errors include context and
keep existing file write/close behavior.
ℹ️ 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 (1)
devices/remote.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.