fix: optimize webm duration patching to prevent crash on long recordings#77
Merged
Conversation
📝 WalkthroughWalkthroughThis PR streams large WebM duration patches through a temp file path with in-memory fallback, adds tests for the patching flow, and introduces a ChangesWebM Duration Patch Optimization
Recording Saving State UI
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/hooks/useScreenRecorder.ts (1)
304-315: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
savingis set even for cancel/discard runs in the browser-recorder path.
finalizeRecordingunconditionally callssetSaving(true), unlikefinalizeNativeWindowsRecording/finalizeNativeMacRecording, which skip it whendiscardis true (Lines 433-435, 537-539). SincecancelRecording/restartRecordingsetdiscardRecordingId.currentbefore triggering the recorder'sstopevent (which invokes this function asynchronously), the discard intent is already knowable here. As written, canceling or restarting a browser-based recording will show the "Saving..." spinner and block minimize/close/settings for as long asactiveScreenRecorder.recordedBlobPromisetakes to resolve, even though nothing is actually being saved.🐛 Proposed fix
finalizingRecordingId.current = activeRecordingId; - setSaving(true); + const isDiscardRun = discardRecordingId.current === activeRecordingId; + if (!isDiscardRun) { + setSaving(true); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useScreenRecorder.ts` around lines 304 - 315, The browser recorder path in finalizeRecording is setting saving for discard flows, unlike finalizeNativeWindowsRecording and finalizeNativeMacRecording. Update finalizeRecording to check discardRecordingId.current against activeRecordingId before calling setSaving(true), and skip the saving state when the run was canceled or restarted. Use the existing discardRecordingId.current and activeRecordingId values in finalizeRecording so cancelRecording/restartRecording do not trigger the "Saving..." spinner or block controls.src/components/launch/LaunchWindow.tsx (1)
1057-1084: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDisable restart/cancel while
savingis true. Native finalization keepsrecordingset, so this group stays clickable during the in-flight save. Restart can then callstartRecording()after a no-op finalize and race the current recording teardown; cancel just does nothing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/launch/LaunchWindow.tsx` around lines 1057 - 1084, Disable the restart and cancel controls in the recording action group while saving is in progress, since `recording` stays true during native finalization and the buttons remain clickable. Update the button logic in `LaunchWindow` for `restartRecording` and `cancelRecording` to respect `saving` (and keep `togglePaused` behavior unchanged), so `startRecording()` cannot race an active finalize and cancel cannot fire during the in-flight save.
🧹 Nitpick comments (1)
src/components/launch/LaunchWindow.tsx (1)
86-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated disabled-state utility classes across HUD button constants.
hudGroupClasses,hudIconBtnClasses,hudAuxIconBtnClasses, andwindowBtnClasseseach repeatdisabled:opacity-30 disabled:cursor-not-allowed disabled:pointer-events-none. Consider extracting a sharedhudDisabledClassesfragment to avoid drift if the disabled styling needs to change later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/launch/LaunchWindow.tsx` around lines 86 - 95, The HUD button class constants repeat the same disabled-state utilities, so extract the shared disabled fragment into a reusable `hudDisabledClasses` value and compose it into `hudGroupClasses`, `hudIconBtnClasses`, `hudAuxIconBtnClasses`, and `windowBtnClasses`. Keep the existing styling behavior unchanged while centralizing the repeated `disabled:opacity-30 disabled:cursor-not-allowed disabled:pointer-events-none` classes to avoid future drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/recording/webm-duration.test.ts`:
- Around line 73-76: Hoist the WebmElementMock type declaration out of the first
it block so it’s available to the later tests in webm-duration.test.ts. Move the
interface to the surrounding describe scope (or another shared scope near the
test helpers) and keep the existing getSectionById and getValue members
unchanged so all test cases can reference the same mock type.
In `@electron/recording/webm-duration.ts`:
- Around line 130-156: The initial header write in the optimized patch flow can
reject before the existing cleanup runs, leaving the ReadStream and WriteStream
open. Move the ws.write(patchedBytes) promise into the same try/finally block
used around pipeline in the webm-duration patching logic so rs and ws are always
destroyed on any failure; keep the outer catch for logging and temp-file
cleanup, and use the existing createReadStream, createWriteStream, and pipeline
flow as the anchor points.
---
Outside diff comments:
In `@src/components/launch/LaunchWindow.tsx`:
- Around line 1057-1084: Disable the restart and cancel controls in the
recording action group while saving is in progress, since `recording` stays true
during native finalization and the buttons remain clickable. Update the button
logic in `LaunchWindow` for `restartRecording` and `cancelRecording` to respect
`saving` (and keep `togglePaused` behavior unchanged), so `startRecording()`
cannot race an active finalize and cancel cannot fire during the in-flight save.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 304-315: The browser recorder path in finalizeRecording is setting
saving for discard flows, unlike finalizeNativeWindowsRecording and
finalizeNativeMacRecording. Update finalizeRecording to check
discardRecordingId.current against activeRecordingId before calling
setSaving(true), and skip the saving state when the run was canceled or
restarted. Use the existing discardRecordingId.current and activeRecordingId
values in finalizeRecording so cancelRecording/restartRecording do not trigger
the "Saving..." spinner or block controls.
---
Nitpick comments:
In `@src/components/launch/LaunchWindow.tsx`:
- Around line 86-95: The HUD button class constants repeat the same
disabled-state utilities, so extract the shared disabled fragment into a
reusable `hudDisabledClasses` value and compose it into `hudGroupClasses`,
`hudIconBtnClasses`, `hudAuxIconBtnClasses`, and `windowBtnClasses`. Keep the
existing styling behavior unchanged while centralizing the repeated
`disabled:opacity-30 disabled:cursor-not-allowed disabled:pointer-events-none`
classes to avoid future drift.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 50301293-1342-4ace-8620-73b1b057bd65
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
electron/recording/webm-duration.test.tselectron/recording/webm-duration.tssrc/components/launch/LaunchWindow.test.tsxsrc/components/launch/LaunchWindow.tsxsrc/hooks/useScreenRecorder.tssrc/i18n/locales/ar/launch.jsonsrc/i18n/locales/en/launch.jsonsrc/i18n/locales/es/launch.jsonsrc/i18n/locales/fr/launch.jsonsrc/i18n/locales/it/launch.jsonsrc/i18n/locales/ja-JP/launch.jsonsrc/i18n/locales/ko-KR/launch.jsonsrc/i18n/locales/pt-BR/launch.jsonsrc/i18n/locales/ru/launch.jsonsrc/i18n/locales/tr/launch.jsonsrc/i18n/locales/vi/launch.jsonsrc/i18n/locales/zh-CN/launch.jsonsrc/i18n/locales/zh-TW/launch.json
- Hoist WebmElementMock interface to describe scope in webm-duration.test.ts - Wrap ws.write inside try/finally to ensure streams are always destroyed on failure - Disable pause/restart/cancel HUD buttons while saving to prevent race conditions - Skip setSaving(true) for cancelled/restarted recordings (discardRecordingId guard) - Extract hudDisabledClasses constant to reduce repetition in LaunchWindow.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/recording/webm-duration.ts`:
- Around line 130-140: The finalization flow currently writes the patched header
to the writable stream before calling pipeline(), which leaves a gap where a
write error can go unhandled and crash the process. Update the logic around the
tmpPath write in webm-duration.ts to use fs.writeFile() for the patched header
first, then use pipeline() only for appending the remaining bytes from
createReadStream(filePath, { start: clusterOffset }) into the same output path.
Keep the existing temp-file finalization behavior intact while ensuring all
stream errors are covered by the pipeline path.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ae0c307-9095-46b7-9f59-5a454a04e087
📒 Files selected for processing (4)
electron/recording/webm-duration.test.tselectron/recording/webm-duration.tssrc/components/launch/LaunchWindow.tsxsrc/hooks/useScreenRecorder.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/hooks/useScreenRecorder.ts
- electron/recording/webm-duration.test.ts
- src/components/launch/LaunchWindow.tsx
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.
Summary
Implemented a hybrid WebM duration patching algorithm to avoid out-of-memory crashes when saving long recordings on Windows and macOS. For files under 2MB, in-memory parsing is used. For files over 2MB, an optimized chunk-based parsing and streaming approach reads the first 2MB chunk, locates the first Cluster element, truncates the parsed segment children to output the duration-patched header, and then streams the rest of the original Cluster data directly from disk to the target file.
Also added a
savingloading state during recording completion, which displays a visual spinner and "Saving..." text on the HUD record button and disables settings, tray layout controls, minimize, and close actions.Related issue
Closes #41
Refs #74
Type of change
Release impact
Desktop impact
Screenshots / video
N/A (HUD overlay visual changes and testing validation results documented in walkthrough.md).
Testing
electron/recording/webm-duration.test.tsfor small/large files.npm run test.Summary by CodeRabbit