WIP: Memory Optimization#474
Conversation
Documents 7 identified memory issues across recording and export pipelines with concrete real-world impact numbers, proposed fixes, testing plan, and rollback strategy. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a PRD describing seven memory issues and implements fixes across IPC, exporter, renderer, and UI: caps/trim capture output buffers, release decoded audio chunks and canvas/readback resources, lower export decode backpressure with memory tiers, add memory-tier export settings and persistence, and update related tests and cleanup calls. ChangesMemory Optimization PRD & Runtime Fixes
Sequence Diagram(s)sequenceDiagram
participant UI as User/UI
participant VideoEditor as VideoEditor
participant Exporter as ModernVideoExporter
participant NativeCapture as nativeFrameCapture
participant FFmpeg as FFmpeg
UI->>VideoEditor: user selects export + memory tier
VideoEditor->>Exporter: start export (memoryUsage)
Exporter->>NativeCapture: capture frames (may use fallback canvas)
Exporter->>FFmpeg: spawn ffmpeg (stderr collected)
FFmpeg->>Exporter: stderr data
Exporter->>Exporter: trim stderr buffer if > MAX_CAPTURE_OUTPUT_BUFFER_LENGTH
Exporter->>NativeCapture: releaseNativeFrameCaptureResources() on cleanup
Exporter->>Exporter: apply backpressure profile (getExportBackpressureProfile(memoryUsage))
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
🤖 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 `@docs/PRD-memory-optimization.md`:
- Around line 46-49: The per-issue and combined memory-savings numbers are
inconsistent; pick a single baseline scenario (e.g., 30-minute 4K recording on
4-core) and recompute all per-issue estimates and the aggregated “combined
worst-case” total to match that same scenario, then update the per-issue bullets
(the lines listing “5-minute/10-minute/30-minute” wasted MB for Issue 1 and the
similar entries mentioned), the summary table, and the combined-figure paragraph
so every value is derived from the same baseline and the combined total is the
true sum/union for that scenario.
- Around line 153-159: Two fenced code blocks containing the flow diagrams
titled "User hits Record" and "User hits Export" are missing language
identifiers; add a language tag (e.g., text) after the opening triple backticks
for both blocks so they become ```text. Update the block that starts with "User
hits Record" and the block that starts with "User hits Export" (also noted
around lines 165-172) to include the language identifier to satisfy MD040 and
keep the docs lint-clean.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 58323c55-14c2-4565-a8cc-4a3df7d1fa28
📒 Files selected for processing (1)
docs/PRD-memory-optimization.md
gurpreetkaits
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/PRD-memory-optimization.md (1)
46-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIssue 1 memory estimates are internally inconsistent and likely mis-scaled.
Line 46–48 values (55 → 220 → 660 MB) don’t scale proportionally with duration, but those numbers are reused in Line 185 and Line 258 for prioritization and totaling. Please recompute Issue 1 with one consistent formula and update all downstream references (including Line 266 total) from that same basis.
Also applies to: 185-186, 258-259
🤖 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 `@docs/PRD-memory-optimization.md` around lines 46 - 48, Issue 1’s memory estimates are mis-scaled (the values at Lines 46–48 are doubled inconsistently); recompute using a consistent per-minute wasted-memory rate derived from the 5-minute example (55 MB wasted → 11 MB/min), then recalc and replace the 5/10/30-minute wasted and peak values accordingly and propagate those corrected numbers to every downstream reference mentioned (Issue 1 occurrences at Lines 185–186, 258–259 and the total at Line 266) so all references use the same formula and totals.
🤖 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.
Duplicate comments:
In `@docs/PRD-memory-optimization.md`:
- Around line 46-48: Issue 1’s memory estimates are mis-scaled (the values at
Lines 46–48 are doubled inconsistently); recompute using a consistent per-minute
wasted-memory rate derived from the 5-minute example (55 MB wasted → 11 MB/min),
then recalc and replace the 5/10/30-minute wasted and peak values accordingly
and propagate those corrected numbers to every downstream reference mentioned
(Issue 1 occurrences at Lines 185–186, 258–259 and the total at Line 266) so all
references use the same formula and totals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b2886174-8b4b-4465-9f74-b34d913f4706
📒 Files selected for processing (1)
docs/PRD-memory-optimization.md
…256KB to prevent unbounded growth during recording/export 2. Cursor telemetry cleanup — activeCursorSamples cleared after snapshot instead of accumulating forever 3. Audio encoder cleanup — channel chunk arrays freed after merging into the final buffer 4. Reduced backpressure limits — smaller decode/pending frame queues to lower peak memory during export 5. Frame renderer cleanup — background/composite canvases properly nulled on dispose 6. Native frame capture cleanup — fallback canvas/context released when export ends
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/exporter/exportTuning.test.ts (1)
58-82: ⚡ Quick winAdd a matching conservative-path assertion for WebCodecs.
Line 58 onward now validates FFmpeg conservative limits, but the WebCodecs conservative values were also changed in
getExportBackpressureProfile()and aren’t directly asserted here. Please add one test case forencodeBackend: "webcodecs"under low-core or extreme workload to lockmaxDecodeQueue: 6andmaxPendingFrames: 12.🤖 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/lib/exporter/exportTuning.test.ts` around lines 58 - 82, Add a parallel assertion for the WebCodecs conservative path by calling getExportBackpressureProfile with encodeBackend: "webcodecs" for a low-core or extreme workload (mirror the breezeLowCoreProfile/breezeHeavyProfile setup) and assert that the returned profile.name is "breeze-conservative" and that profile.maxDecodeQueue === 6 and profile.maxPendingFrames === 12 (also include any other conservative-specific fields you expect, e.g., profile.maxInFlightNativeWrites if applicable) so the WebCodecs conservative limits are explicitly tested.electron/ipc/register/export.ts (1)
197-204: ⚡ Quick winConsider extracting trimming logic to a shared utility.
The trimming logic here is duplicated from
state.tslines 17-22, where it exists as a helper functiontrimCaptureOutputBuffer. Since both files implement identical buffer-trimming behavior, extracting this to a shared utility (e.g.,electron/ipc/utils/bufferTrimming.ts) would improve maintainability and ensure consistent trimming logic across all capture outputs.♻️ Proposed consolidation approach
Create a shared utility:
// electron/ipc/utils/bufferTrimming.ts import { MAX_CAPTURE_OUTPUT_BUFFER_LENGTH, CAPTURE_OUTPUT_BUFFER_TRIM_TARGET } from '../constants'; export function trimCaptureOutputBuffer(buffer: string): string { if (buffer.length <= MAX_CAPTURE_OUTPUT_BUFFER_LENGTH) { return buffer; } return buffer.slice(buffer.length - CAPTURE_OUTPUT_BUFFER_TRIM_TARGET); }Then update this file:
-import { - MAX_CAPTURE_OUTPUT_BUFFER_LENGTH, - CAPTURE_OUTPUT_BUFFER_TRIM_TARGET, -} from "../constants"; +import { trimCaptureOutputBuffer } from "../utils/bufferTrimming"; ffmpegProcess.stderr.on("data", (chunk: Buffer) => { session.stderrOutput += chunk.toString(); - if (session.stderrOutput.length > MAX_CAPTURE_OUTPUT_BUFFER_LENGTH) { - session.stderrOutput = session.stderrOutput.slice( - session.stderrOutput.length - CAPTURE_OUTPUT_BUFFER_TRIM_TARGET, - ); - } + session.stderrOutput = trimCaptureOutputBuffer(session.stderrOutput); });And similarly update
state.tsto import from the shared utility.🤖 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 `@electron/ipc/register/export.ts` around lines 197 - 204, Duplicate buffer-trimming logic in the ffmpeg stderr handler should be extracted to a shared utility and reused: create a new function trimCaptureOutputBuffer (e.g., in electron/ipc/utils/bufferTrimming.ts) that imports MAX_CAPTURE_OUTPUT_BUFFER_LENGTH and CAPTURE_OUTPUT_BUFFER_TRIM_TARGET and performs the slice logic, then replace the inline trimming inside the ffmpegProcess.stderr.on("data", ...) callback (where session.stderrOutput is appended and trimmed) to call trimCaptureOutputBuffer(session.stderrOutput); also update state.ts to import and use the same trimCaptureOutputBuffer so both places share the single implementation.
🤖 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/ipc/register/export.ts`:
- Around line 40-43: Several handlers append process output without bounds (see
electron/ipc/export/native-video.ts stderr accumulation and multiple
stdout/stderr buffers in electron/ipc/register/recording.ts); update each
accumulation point (e.g., variables like stderrBuffer, stdoutBuffer or functions
handling 'data' events such as onStdout/onStderr) to trim using
MAX_CAPTURE_OUTPUT_BUFFER_LENGTH and CAPTURE_OUTPUT_BUFFER_TRIM_TARGET: after
appending new chunk, if buffer.length > MAX_CAPTURE_OUTPUT_BUFFER_LENGTH, set
buffer = buffer.slice(buffer.length - CAPTURE_OUTPUT_BUFFER_TRIM_TARGET) (or
equivalent) so buffers never grow unbounded; reuse the constants imported from
electron/ipc/register/export.ts (or the constants module) and apply the same
pattern at all listed locations.
---
Nitpick comments:
In `@electron/ipc/register/export.ts`:
- Around line 197-204: Duplicate buffer-trimming logic in the ffmpeg stderr
handler should be extracted to a shared utility and reused: create a new
function trimCaptureOutputBuffer (e.g., in electron/ipc/utils/bufferTrimming.ts)
that imports MAX_CAPTURE_OUTPUT_BUFFER_LENGTH and
CAPTURE_OUTPUT_BUFFER_TRIM_TARGET and performs the slice logic, then replace the
inline trimming inside the ffmpegProcess.stderr.on("data", ...) callback (where
session.stderrOutput is appended and trimmed) to call
trimCaptureOutputBuffer(session.stderrOutput); also update state.ts to import
and use the same trimCaptureOutputBuffer so both places share the single
implementation.
In `@src/lib/exporter/exportTuning.test.ts`:
- Around line 58-82: Add a parallel assertion for the WebCodecs conservative
path by calling getExportBackpressureProfile with encodeBackend: "webcodecs" for
a low-core or extreme workload (mirror the
breezeLowCoreProfile/breezeHeavyProfile setup) and assert that the returned
profile.name is "breeze-conservative" and that profile.maxDecodeQueue === 6 and
profile.maxPendingFrames === 12 (also include any other conservative-specific
fields you expect, e.g., profile.maxInFlightNativeWrites if applicable) so the
WebCodecs conservative limits are explicitly tested.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f8e7c59-d8ca-4bc3-bb28-c63515c5ed35
📒 Files selected for processing (18)
electron/ipc/constants.tselectron/ipc/cursor/telemetry.tselectron/ipc/register/export.tselectron/ipc/state.tselectron/native/bin/darwin-arm64/recordly-native-cursor-monitorelectron/native/bin/darwin-arm64/recordly-screencapturekit-helperelectron/native/bin/darwin-arm64/recordly-system-cursorselectron/native/bin/darwin-arm64/recordly-window-listelectron/native/bin/darwin-x64/recordly-native-cursor-monitorelectron/native/bin/darwin-x64/recordly-screencapturekit-helperelectron/native/bin/darwin-x64/recordly-system-cursorselectron/native/bin/darwin-x64/recordly-window-listsrc/lib/exporter/audioEncoder.tssrc/lib/exporter/exportTuning.test.tssrc/lib/exporter/exportTuning.tssrc/lib/exporter/modernFrameRenderer.tssrc/lib/exporter/modernVideoExporter.tssrc/lib/exporter/nativeFrameCapture.ts
✅ Files skipped from review due to trivial changes (3)
- electron/ipc/constants.ts
- src/lib/exporter/modernFrameRenderer.ts
- src/lib/exporter/audioEncoder.ts
| import { | ||
| MAX_CAPTURE_OUTPUT_BUFFER_LENGTH, | ||
| CAPTURE_OUTPUT_BUFFER_TRIM_TARGET, | ||
| } from "../constants"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify buffer constants are defined and check their values
echo "=== Checking buffer size constants ==="
rg -n 'MAX_CAPTURE_OUTPUT_BUFFER_LENGTH|CAPTURE_OUTPUT_BUFFER_TRIM_TARGET' electron/ipc/constants.ts
echo -e "\n=== Checking for other potential unbounded buffer accumulations ==="
# Look for patterns where stderr/stdout is accumulated without bounds
rg -n -C3 'stderr.*\+=' --type=ts -g '!*.test.ts' electron/
echo -e "\n=== Checking for other .on\(\"data\" patterns that might accumulate ==="
# Find other data event handlers that might need similar treatment
rg -n -B2 -A5 '\.on\("data"' --type=ts electron/ipc/ | rg -A5 'stderrOutput|stdoutOutput|OutputBuffer'Repository: webadderallorg/Recordly
Length of output: 4356
Buffer constants verified, but other data handlers lack bounds checking.
The buffer size constants MAX_CAPTURE_OUTPUT_BUFFER_LENGTH (256 KB) and CAPTURE_OUTPUT_BUFFER_TRIM_TARGET (128 KB) are correctly defined in electron/ipc/constants.ts with appropriate values per the PRD. However, verification identified that other data event handlers throughout the codebase accumulate output without bounds:
electron/ipc/export/native-video.ts(line 326): stderr accumulation without size limitselectron/ipc/register/recording.ts(lines 305, 309, 496, 499, 1115, 1118): multiple stdout/stderr handlers appending to buffers without bounds checking
Apply the same buffer protection pattern (using the defined constants) to these locations to prevent unbounded memory growth.
🤖 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 `@electron/ipc/register/export.ts` around lines 40 - 43, Several handlers
append process output without bounds (see electron/ipc/export/native-video.ts
stderr accumulation and multiple stdout/stderr buffers in
electron/ipc/register/recording.ts); update each accumulation point (e.g.,
variables like stderrBuffer, stdoutBuffer or functions handling 'data' events
such as onStdout/onStderr) to trim using MAX_CAPTURE_OUTPUT_BUFFER_LENGTH and
CAPTURE_OUTPUT_BUFFER_TRIM_TARGET: after appending new chunk, if buffer.length >
MAX_CAPTURE_OUTPUT_BUFFER_LENGTH, set buffer = buffer.slice(buffer.length -
CAPTURE_OUTPUT_BUFFER_TRIM_TARGET) (or equivalent) so buffers never grow
unbounded; reuse the constants imported from electron/ipc/register/export.ts (or
the constants module) and apply the same pattern at all listed locations.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/exporter/exportTuning.ts (1)
162-163: ⚡ Quick winAsymmetric reduction between backends—verify this is intentional.
The WebCodecs conservative profile reduces
maxDecodeQueueto 6 (vs. 4 for ffmpeg) andmaxPendingFramesto 12 (vs. 8 for ffmpeg). If this asymmetry reflects genuine differences in pipeline buffering needs between backends, consider documenting the rationale inline. If the values were chosen arbitrarily, consider aligning them or explaining why WebCodecs requires larger buffers on constrained systems.📝 Example inline comment
if (isLowCoreSystem || isExtremeWorkload) { return { name: "webcodecs-conservative", maxEncodeQueue, + // WebCodecs requires slightly larger decode/pending buffers than ffmpeg + // due to async frame readiness checks. Empirically validated on 4-core systems. maxDecodeQueue: 6, maxPendingFrames: 12, maxInFlightNativeWrites: 1, }; }🤖 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/lib/exporter/exportTuning.ts` around lines 162 - 163, The WebCodecs export tuning sets maxDecodeQueue = 6 and maxPendingFrames = 12 which differs from the ffmpeg profile (4 and 8); verify this asymmetry is intentional and either align the values or add an inline comment documenting the rationale. Locate the settings for maxDecodeQueue and maxPendingFrames in exportTuning (the WebCodecs profile) and either change them to match the ffmpeg profile or add a concise comment explaining why WebCodecs needs larger buffers (e.g., pipeline/backpressure differences, OS constraints, or observed throughput), referencing both "WebCodecs" and "ffmpeg" in the comment for clarity.
🤖 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.
Nitpick comments:
In `@src/lib/exporter/exportTuning.ts`:
- Around line 162-163: The WebCodecs export tuning sets maxDecodeQueue = 6 and
maxPendingFrames = 12 which differs from the ffmpeg profile (4 and 8); verify
this asymmetry is intentional and either align the values or add an inline
comment documenting the rationale. Locate the settings for maxDecodeQueue and
maxPendingFrames in exportTuning (the WebCodecs profile) and either change them
to match the ffmpeg profile or add a concise comment explaining why WebCodecs
needs larger buffers (e.g., pipeline/backpressure differences, OS constraints,
or observed throughput), referencing both "WebCodecs" and "ffmpeg" in the
comment for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c0636671-5664-4ded-b5d5-baf0cd70691d
📒 Files selected for processing (7)
electron/ipc/cursor/telemetry.tselectron/ipc/register/export.tssrc/lib/exporter/audioEncoder.tssrc/lib/exporter/exportTuning.test.tssrc/lib/exporter/exportTuning.tssrc/lib/exporter/modernFrameRenderer.tssrc/lib/exporter/modernVideoExporter.ts
✅ Files skipped from review due to trivial changes (3)
- src/lib/exporter/modernVideoExporter.ts
- src/lib/exporter/audioEncoder.ts
- src/lib/exporter/modernFrameRenderer.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- electron/ipc/register/export.ts
- electron/ipc/cursor/telemetry.ts
- src/lib/exporter/exportTuning.test.ts
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 (2)
src/components/video-editor/VideoEditor.tsx (2)
4849-4905:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSame stale-closure bug in
handleStartExportFromDropdown.Line 4872 reads
exportMemoryUsageinto theExportSettingsobject, but the dependency array (lines 4893–4905) omits it. As a result the dropdown’s "Start export" button will keep packing the initialexportMemoryUsagevalue intosettings.memoryUsage, defeating the new UI control.🛠️ Proposed fix
videoPath, exportFormat, exportEncodingMode, + exportMemoryUsage, exportQuality,🤖 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/video-editor/VideoEditor.tsx` around lines 4849 - 4905, handleStartExportFromDropdown constructs ExportSettings with memoryUsage set from exportMemoryUsage but the hook dependency array for useCallback omits exportMemoryUsage, causing a stale closure; add exportMemoryUsage to the dependency array for handleStartExportFromDropdown so the callback updates when exportMemoryUsage changes (verify the symbol names handleStartExportFromDropdown and exportMemoryUsage are used).
2136-2144:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd missing
setExportMemoryUsagecall when loading a project.
applyLoadedProjectrestores export settings includingexportEncodingMode,exportBackendPreference,exportPipelineModel,exportQuality, etc., but omitssetExportMemoryUsage. SincenormalizeProjectEditorinprojectPersistence.ts(line 1021) includesexportMemoryUsage: normalizeExportMemoryUsage(editor.exportMemoryUsage), the field is persisted but silently discarded on load, causing the preference to reset each time a project is opened.Add
setExportMemoryUsage(normalizedEditor.exportMemoryUsage);alongside the other export setting calls at lines 2136–2144.🤖 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/video-editor/VideoEditor.tsx` around lines 2136 - 2144, applyLoadedProject restores multiple export settings but omits the persisted exportMemoryUsage; update applyLoadedProject to call setExportMemoryUsage(normalizedEditor.exportMemoryUsage) alongside the other setters (e.g., setExportEncodingMode, setExportQuality) so the normalized exportMemoryUsage produced by normalizeProjectEditor is applied when a project is loaded.
🧹 Nitpick comments (2)
src/components/video-editor/VideoEditor.tsx (1)
833-1027: ⚡ Quick winEditor presets do not capture/apply
exportMemoryUsage.
captureEditorPresetSnapshot(lines 833–936) snapshotsexportEncodingMode,exportBackendPreference,exportPipelineModel,exportQuality,mp4FrameRate, etc., andapplyEditorPresetSnapshot(lines 977–1027) writes each back via setters — butexportMemoryUsageis omitted from both. If the new field is meant to behave like the other export settings (i.e. become part of a preset), please add it to both functions and toEditorPresetSnapshot. If presets are deliberately scoped to exclude memory tuning, a short comment near the encoding fields would prevent the next refactor from re-introducing the inconsistency.🤖 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/video-editor/VideoEditor.tsx` around lines 833 - 1027, The preset snapshot and apply logic currently omit exportMemoryUsage; update the EditorPresetSnapshot type to include exportMemoryUsage, add exportMemoryUsage to the returned object in captureEditorPresetSnapshot (function captureEditorPresetSnapshot) alongside the other export fields, and setExportMemoryUsage(snapshot.exportMemoryUsage) in applyEditorPresetSnapshot (function applyEditorPresetSnapshot); if intentional to exclude it instead, add a brief comment near exportEncodingMode/exportBackendPreference explaining why exportMemoryUsage is deliberately omitted.src/lib/exporter/exportTuning.ts (1)
132-154: 💤 Low value**
hightier equalsbalancedfor the _BALANCED and _BALANCED_PLUS profiles.For
BREEZE_BALANCED,BREEZE_BALANCED_PLUS,WEBCODECS_BALANCED, andWEBCODECS_BALANCED_PLUS, thehighentry is byte-identical to thebalancedentry. If that's intentional (i.e., these profiles are already at the desired ceiling and onlylowshould claw back), consider a brief comment so a future contributor doesn't assume it's a copy-paste oversight and start growing thehighnumbers. If it isn't intentional, thehightier currently provides no benefit overbalancedon multi-core / non-extreme workloads.🤖 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/lib/exporter/exportTuning.ts` around lines 132 - 154, The high tier equals balanced for BREEZE_BALANCED, BREEZE_BALANCED_PLUS, WEBCODECS_BALANCED, and WEBCODECS_BALANCED_PLUS which looks like a copy/paste oversight or an intentional ceiling; either add a short clarifying comment above each of these constants stating “high intentionally equals balanced to cap scaling” (so future contributors don’t change it) OR update the high entries in those MemoryTieredProfile objects (BREEZE_BALANCED, BREEZE_BALANCED_PLUS, WEBCODECS_BALANCED, WEBCODECS_BALANCED_PLUS) to the intended higher values if the intent was to provide a stronger high tier. Ensure the comment references the profile name and reasoning if you choose the comment approach.
🤖 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 `@src/components/video-editor/VideoEditor.tsx`:
- Line 4338: handleExport's closure captures exportMemoryUsage but it's not in
the useCallback dependency array, causing stale values; update the useCallback
that defines handleExport to include exportMemoryUsage in its dependency list
(the same array currently covering settings, exportOptions, etc.) so that
memoryUsage = settings.memoryUsage ?? exportMemoryUsage reflects updates to the
"Memory" toggle group; locate the handleExport function in VideoEditor.tsx and
add exportMemoryUsage to its dependencies alongside the other referenced
symbols.
---
Outside diff comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 4849-4905: handleStartExportFromDropdown constructs ExportSettings
with memoryUsage set from exportMemoryUsage but the hook dependency array for
useCallback omits exportMemoryUsage, causing a stale closure; add
exportMemoryUsage to the dependency array for handleStartExportFromDropdown so
the callback updates when exportMemoryUsage changes (verify the symbol names
handleStartExportFromDropdown and exportMemoryUsage are used).
- Around line 2136-2144: applyLoadedProject restores multiple export settings
but omits the persisted exportMemoryUsage; update applyLoadedProject to call
setExportMemoryUsage(normalizedEditor.exportMemoryUsage) alongside the other
setters (e.g., setExportEncodingMode, setExportQuality) so the normalized
exportMemoryUsage produced by normalizeProjectEditor is applied when a project
is loaded.
---
Nitpick comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 833-1027: The preset snapshot and apply logic currently omit
exportMemoryUsage; update the EditorPresetSnapshot type to include
exportMemoryUsage, add exportMemoryUsage to the returned object in
captureEditorPresetSnapshot (function captureEditorPresetSnapshot) alongside the
other export fields, and setExportMemoryUsage(snapshot.exportMemoryUsage) in
applyEditorPresetSnapshot (function applyEditorPresetSnapshot); if intentional
to exclude it instead, add a brief comment near
exportEncodingMode/exportBackendPreference explaining why exportMemoryUsage is
deliberately omitted.
In `@src/lib/exporter/exportTuning.ts`:
- Around line 132-154: The high tier equals balanced for BREEZE_BALANCED,
BREEZE_BALANCED_PLUS, WEBCODECS_BALANCED, and WEBCODECS_BALANCED_PLUS which
looks like a copy/paste oversight or an intentional ceiling; either add a short
clarifying comment above each of these constants stating “high intentionally
equals balanced to cap scaling” (so future contributors don’t change it) OR
update the high entries in those MemoryTieredProfile objects (BREEZE_BALANCED,
BREEZE_BALANCED_PLUS, WEBCODECS_BALANCED, WEBCODECS_BALANCED_PLUS) to the
intended higher values if the intent was to provide a stronger high tier. Ensure
the comment references the profile name and reasoning if you choose the comment
approach.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 692dbb25-5b87-4cd3-8140-759d81fd4d0a
📒 Files selected for processing (11)
electron/ipc/register/recording.tssrc/components/video-editor/ExportSettingsMenu.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/editorPreferences.tssrc/components/video-editor/projectPersistence.tssrc/i18n/locales/en/settings.jsonsrc/lib/exporter/exportTuning.test.tssrc/lib/exporter/exportTuning.tssrc/lib/exporter/index.tssrc/lib/exporter/modernVideoExporter.tssrc/lib/exporter/types.ts
✅ Files skipped from review due to trivial changes (1)
- src/i18n/locales/en/settings.json
| settings.encodingMode ?? | ||
| exportEncodingMode) | ||
| : (settings.encodingMode ?? exportEncodingMode); | ||
| const memoryUsage = settings.memoryUsage ?? exportMemoryUsage; |
There was a problem hiding this comment.
Stale closure: exportMemoryUsage is missing from handleExport's dependency list.
memoryUsage is computed from settings.memoryUsage ?? exportMemoryUsage, but exportMemoryUsage is not listed in the useCallback dependency array at lines 4655–4734. The callback will keep capturing the initial exportMemoryUsage value, so changes to the new "Memory" toggle group will not take effect for the MP4 path unless the user explicitly passes settings.memoryUsage and another dep changes. Add exportMemoryUsage to the dependency list.
🛠️ Proposed fix
exportEncodingMode,
+ exportMemoryUsage,
exportBackendPreference,
exportPipelineModel,🤖 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/video-editor/VideoEditor.tsx` at line 4338, handleExport's
closure captures exportMemoryUsage but it's not in the useCallback dependency
array, causing stale values; update the useCallback that defines handleExport to
include exportMemoryUsage in its dependency list (the same array currently
covering settings, exportOptions, etc.) so that memoryUsage =
settings.memoryUsage ?? exportMemoryUsage reflects updates to the "Memory"
toggle group; locate the handleExport function in VideoEditor.tsx and add
exportMemoryUsage to its dependencies alongside the other referenced symbols.
1 participant
Summary
Summary by CodeRabbit