feat: more information during screenrecord#180
Conversation
WalkthroughThis pull request extends the screen recording functionality with silent mode support and progress callbacks. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
commands/screenrecord.go (1)
197-207:⚠️ Potential issue | 🟡 MinorAvoid “recording ended” messaging before capture error checks.
progress.recordingEnded()currently runs beforeerris evaluated, so a failed capture can still print “Screen recording ended, please wait while finalizing video”. That message is misleading on failure.Suggested fix
- progress.recordingEnded() - - if req.StopChan == nil { - signal.Reset(syscall.SIGINT, syscall.SIGTERM) - } - - tempFile.Close() - - if err != nil { - return NewErrorResponse(fmt.Errorf("error during screen capture: %w", err)) - } + if err != nil { + progress.stopTicker() + if !progress.silent { + fmt.Fprintln(os.Stderr) + } + if req.StopChan == nil { + signal.Reset(syscall.SIGINT, syscall.SIGTERM) + } + tempFile.Close() + return NewErrorResponse(fmt.Errorf("error during screen capture: %w", err)) + } + + progress.recordingEnded() + if req.StopChan == nil { + signal.Reset(syscall.SIGINT, syscall.SIGTERM) + } + tempFile.Close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/screenrecord.go` around lines 197 - 207, The progress.recordingEnded() call is executed before checking err, causing a misleading success message on failure; modify the flow in the function that performs the capture so that you first handle the error (if err != nil return NewErrorResponse(...)) and only after a successful capture call progress.recordingEnded(); ensure tempFile.Close() and the signal.Reset(req.StopChan == nil) logic still run as needed (use defer or explicit close/reset) so resources and signal handlers are properly cleaned up even on error; update references: progress.recordingEnded(), err, tempFile.Close(), req.StopChan and signal.Reset to reflect this reordered logic.
🤖 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 303-305: The progress wrapper is being initialized with
resp.ContentLength directly (see the struct fields reader/total/onProgress),
which can be -1 for unknown lengths and results in negative totals; update the
initialization in both locations (the blocks around the reader: resp.Body,
total: resp.ContentLength, onProgress: onProgress and the similar block at lines
~415-417) to coerce non-positive ContentLength to 0 (e.g., total :=
resp.ContentLength; if total < 0 { total = 0 }) or otherwise handle unknown
length explicitly so downstream progress calculations never see a negative
total.
---
Outside diff comments:
In `@commands/screenrecord.go`:
- Around line 197-207: The progress.recordingEnded() call is executed before
checking err, causing a misleading success message on failure; modify the flow
in the function that performs the capture so that you first handle the error (if
err != nil return NewErrorResponse(...)) and only after a successful capture
call progress.recordingEnded(); ensure tempFile.Close() and the
signal.Reset(req.StopChan == nil) logic still run as needed (use defer or
explicit close/reset) so resources and signal handlers are properly cleaned up
even on error; update references: progress.recordingEnded(), err,
tempFile.Close(), req.StopChan and signal.Reset to reflect this reordered logic.
ℹ️ 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 (3)
cli/screenrecord.gocommands/screenrecord.godevices/remote.go
| reader: resp.Body, | ||
| total: resp.ContentLength, | ||
| onProgress: onProgress, |
There was a problem hiding this comment.
Handle unknown content length to avoid negative progress totals.
When resp.ContentLength == -1, totalMB becomes negative and user-facing progress output can be incorrect.
Suggested fix
- pr := &progressReader{
+ totalBytes := resp.ContentLength
+ if totalBytes < 0 {
+ totalBytes = 0
+ }
+ pr := &progressReader{
reader: resp.Body,
- total: resp.ContentLength,
+ total: totalBytes,
onProgress: onProgress,
deadline: deadline,
}Also applies to: 415-417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@devices/remote.go` around lines 303 - 305, The progress wrapper is being
initialized with resp.ContentLength directly (see the struct fields
reader/total/onProgress), which can be -1 for unknown lengths and results in
negative totals; update the initialization in both locations (the blocks around
the reader: resp.Body, total: resp.ContentLength, onProgress: onProgress and the
similar block at lines ~415-417) to coerce non-positive ContentLength to 0
(e.g., total := resp.ContentLength; if total < 0 { total = 0 }) or otherwise
handle unknown length explicitly so downstream progress calculations never see a
negative total.
1 participant
No description provided.