feat: implement pause and resume recording#509
feat: implement pause and resume recording#509Rodriq wants to merge 2 commits intosiddharthvaddem:mainfrom
Conversation
📝 WalkthroughWalkthroughRecording control shifts from a boolean flag to a three-state enum ("recording" | "paused" | "stopped"), updating IPC, preload, main/tray UI, shortcuts, and cursor-telemetry session lifecycle; adds tray pause-toggle hook and a configurable global pause/resume shortcut. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70cbd9027c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // timestamp only if the renderer didn't supply one, so the | ||
| // buffer always has a stable key per session. | ||
| const id = typeof recordingId === "number" ? recordingId : Date.now(); | ||
| cursorTelemetryBuffer.startSession(id); |
There was a problem hiding this comment.
Avoid calling startSession() on every transition to "recording": this path now runs both at initial start and after pause, and startSession() clears the active sample buffer. In a pause→resume flow, all cursor telemetry captured before the pause is dropped, so the final overlay only contains post-resume movement. This breaks cursor/video sync for any recording that is paused at least once.
Useful? React with 👍 / 👎.
| parts.push(key); | ||
| const accelerator = parts.join("+"); | ||
| try { | ||
| globalShortcut.register(accelerator, () => { |
There was a problem hiding this comment.
globalShortcut.register() returns false when an accelerator is unavailable (it does not throw), but this code ignores the return value and only handles exceptions. If the chosen binding is already taken by another app/OS, the UI will appear to save successfully while pause/resume never triggers globally, and because unregisterAll() runs first this can also leave users with no working global pause shortcut.
Useful? React with 👍 / 👎.
![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
🧹 Nitpick comments (1)
electron/main.ts (1)
438-465: 💤 Low valueworks but a couple nits
config: anyis kinda loose — consider typing it asPartial<ShortcutsConfig>or at leastRecord<string, unknown>for a bit more safety- the single-char check on line 451 (
binding.key.length === 1) means special keys like "Space" or "F1" won't register properly. currently fine since togglePauseRecording uses "p", but might bite you if someone customizes it to a function key laterlowkey the silent catch on line 444 is fine for "file doesn't exist" scenarios — fallback to defaults is reasonable.
🔧 optional: slightly tighter typing
- let config: any = {}; + let config: Partial<Record<string, { key?: string; ctrl?: boolean; alt?: boolean; shift?: boolean }>> = {};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/main.ts` around lines 438 - 465, In updateGlobalShortcuts, replace the loose any on config with a tighter type (e.g., Partial<ShortcutsConfig> or Record<string, unknown>) and update the binding.key handling so special keys like "Space" or "F1" are accepted: only uppercase single-character keys (binding.key.length === 1 ? binding.key.toUpperCase() : binding.key) and otherwise use the provided name verbatim; ensure you read binding from the typed config (e.g., const binding = (config.togglePauseRecording as Partial<ShortcutBinding>) || {...}) and keep the existing catch behavior for missing files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@electron/main.ts`:
- Around line 438-465: In updateGlobalShortcuts, replace the loose any on config
with a tighter type (e.g., Partial<ShortcutsConfig> or Record<string, unknown>)
and update the binding.key handling so special keys like "Space" or "F1" are
accepted: only uppercase single-character keys (binding.key.length === 1 ?
binding.key.toUpperCase() : binding.key) and otherwise use the provided name
verbatim; ensure you read binding from the typed config (e.g., const binding =
(config.togglePauseRecording as Partial<ShortcutBinding>) || {...}) and keep the
existing catch behavior for missing files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cbb42301-9d32-48f5-94d8-96002101f847
📒 Files selected for processing (6)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/main.tselectron/preload.tssrc/hooks/useScreenRecorder.tssrc/lib/shortcuts.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/cursorTelemetryBuffer.ts`:
- Around line 151-153: The implementation of startSession now treats repeated
calls with the same recordingId as a no-op via the activeRecordingId check, but
the startSession documentation still states that repeated calls clear active
samples; update the contract to match behavior or change the code to match docs.
Specifically, either adjust the comment/docs for startSession to state that if
recordingId === activeRecordingId the call is a no-op and does not clear
samples, or modify the startSession logic (remove or adjust the
activeRecordingId === recordingId early return in the startSession function) so
repeated calls will clear active samples as documented; reference startSession,
activeRecordingId and recordingId when making the change so the behavior and
docs remain consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ea3d65c-2152-401d-b9f7-5ace34185672
📒 Files selected for processing (2)
src/lib/cursorTelemetryBuffer.test.tssrc/lib/cursorTelemetryBuffer.ts
| if (activeRecordingId === recordingId) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
startSession behavior changed, but contract docs still say the old thing
Line 151 makes same-id calls a no-op (nice), but the startSession docs still describe repeated calls as clearing active samples. lowkey risky for future callers relying on comments over code.
nit: cleaner doc patch
- * Begin a new recording session under the given `recordingId`. Clears
- * any in-progress active samples (without touching already-completed
- * pending batches). Safe to call repeatedly — e.g. a rapid Stop →
- * Record sequence — and the most recent id wins.
+ * Begin a new recording session under the given `recordingId`.
+ * If called with the same `recordingId` as the current active session,
+ * this is a no-op (used by pause/resume flows). If called with a different
+ * id, any in-progress active samples are cleared (without touching
+ * already-completed pending batches).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/cursorTelemetryBuffer.ts` around lines 151 - 153, The implementation
of startSession now treats repeated calls with the same recordingId as a no-op
via the activeRecordingId check, but the startSession documentation still states
that repeated calls clear active samples; update the contract to match behavior
or change the code to match docs. Specifically, either adjust the comment/docs
for startSession to state that if recordingId === activeRecordingId the call is
a no-op and does not clear samples, or modify the startSession logic (remove or
adjust the activeRecordingId === recordingId early return in the startSession
function) so repeated calls will clear active samples as documented; reference
startSession, activeRecordingId and recordingId when making the change so the
behavior and docs remain consistent.
|
@Rodriq this feature already exists. Will be closing this PR |
2 participants
Pull Request Template
Description
Implemented the ability to pause and resume screen recordings. This update modifies the
MediaRecorderstream handling and introduces seamless cursor telemetry synchronization by offsetting the capture time usingaccumulatedDurationMspassed from the Renderer to the Main process. It also adds a new global keyboard shortcut (Cmd/Ctrl+Alt+P) and dynamically updates the system tray menu to reflect the active recording state.Motivation
Users need the flexibility to temporarily pause a recording to skip unwanted segments without having to stop the recording and stitch multiple video files together later. This feature ensures a smoother recording experience while maintaining perfect synchronization between the recorded video and the custom cursor overlay telemetry.
Type of Change
Related Issue(s)
Fixess #508
Screenshots / Video
Added pause resume functionality
Testing
To verify these changes:
npm run devor build).Cmd/Ctrl+Alt+P.Checklist
Thank you for contributing!
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests