fix(windows): Fixed windows Export Issue and early decode Crash#423
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughNormalize and ensure export path exists before writing in the Electron IPC handler; adjust streaming decoder to allow a small, Windows-only tolerance for early-decode gaps when a last-decoded frame exists, emitting a warning instead of throwing; remove some snapshot/dependency triggers in the editor components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/exporter/streamingDecoder.ts (1)
508-516: Windows bypass is a bit too broad (lowkey risky for truly truncated exports)Line 508 now downgrades all early-end failures to warn on Windows. That can mark heavily truncated decodes as successful exports. Consider tolerating only small gaps (known metadata-tail cases) and still failing on large gaps, or returning a
partialExportsignal.nit: tighter guardrail without undoing the Windows fix
- if (isWindows) { + const decodeGapSec = + lastDecodedFrameSec === null ? Number.POSITIVE_INFINITY : requiredEndSec - lastDecodedFrameSec; + const canProceedOnWindows = isWindows && decodeGapSec <= METADATA_TAIL_TOLERANCE_SEC; + if (canProceedOnWindows) { console.warn( `[StreamingVideoDecoder] Decode ended early on Windows at ${decodedAtLabel} (needed ${requiredEndSec.toFixed(3)}s) – proceeding anyway.`, ); // Do not throw on Windows; allow export to complete with the frames we have. } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/streamingDecoder.ts` around lines 508 - 516, The current Windows-only bypass (isWindows) silences all early-end decode errors; change it to only tolerate small known metadata-tail gaps by introducing a max tolerance (e.g. maxToleranceSec) and comparing the missing duration (requiredEndSec - decodedAtLabel) to that threshold: if gap <= maxToleranceSec then log the warning and mark result as partialExport (or set a partialExport flag on the decoder/export result), otherwise keep throwing the Error as before; update the branch around isWindows, decodedAtLabel, and requiredEndSec and ensure callers can handle the partialExport signal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/exporter/streamingDecoder.ts`:
- Around line 508-516: The current Windows-only bypass (isWindows) silences all
early-end decode errors; change it to only tolerate small known metadata-tail
gaps by introducing a max tolerance (e.g. maxToleranceSec) and comparing the
missing duration (requiredEndSec - decodedAtLabel) to that threshold: if gap <=
maxToleranceSec then log the warning and mark result as partialExport (or set a
partialExport flag on the decoder/export result), otherwise keep throwing the
Error as before; update the branch around isWindows, decodedAtLabel, and
requiredEndSec and ensure callers can handle the partialExport signal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c57ada4-2cec-4210-90dd-08870724d3f4
📒 Files selected for processing (2)
electron/ipc/handlers.tssrc/lib/exporter/streamingDecoder.ts
![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: 08aff31351
ℹ️ 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".
On Windows, tolerate small decode gaps (<=3 seconds) to work around driver quirks, allowing export to complete with available frames.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/exporter/streamingDecoder.ts`:
- Around line 513-519: The Windows-only 3s tolerance can silently succeed for
very short requested ranges; change the logic in StreamingVideoDecoder (the
isWindows / lastDecodedFrameSec / decodeGapSec branch inside decodeAll()) to use
a relative cap instead of a fixed 3s (e.g. allowedGap = min(3.0, requiredEndSec
* <small-fraction>) ) and if decodeGapSec > allowedGap return an explicit
partial-decode result (or error) rather than treating it as full success; update
decodeAll()’s return shape to include a partial flag/lastDecodedFrameSec and
adjust callers (videoExporter.ts and gifExporter.ts which await decodeAll()) to
surface a user-visible warning or handle the partial result instead of
proceeding to 100%.
- Around line 504-521: The decodeAll() flow uses requiredEndSec but never
declares it; add a local constant requiredEndSec immediately after the segments
declaration (where segments is created) that computes the end time of the last
segment or 0 if segments is empty (e.g., take segments[segments.length-1].end or
equivalent fallback). This ensures requiredEndSec is defined for the
shouldFailDecodeEndedEarly(...) call and later error message in
StreamingVideoDecoder.
🪄 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: 1ba3f54d-f8db-4d65-8d0b-b458783c5ff2
📒 Files selected for processing (2)
electron/ipc/handlers.tssrc/lib/exporter/streamingDecoder.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/ipc/handlers.ts
Add requiredEndSec calculation to properly handle early decode termination by using the last segment's end time. This addresses issues with export processing on Windows platforms.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/exporter/streamingDecoder.ts`:
- Around line 504-526: The decodeAll method is missing its terminating brace
causing a syntax error and the subsequent computeSegments method to be nested
incorrectly; close the decodeAll method by adding a single `}` just after the
final if-block that handles early decode termination (the block that references
isWindows, lastDecodedFrameSec, and decodeGapSec) so that decodeAll ends before
the declaration of private computeSegments, and then reformat the surrounding
lines (within the StreamingVideoDecoder class) to use consistent indentation
(e.g., 2 or 4 spaces) to avoid mixed tabs/spaces.
- Around line 504-521: Add integration tests that exercise the Windows-specific
tolerance branch around isWindows in streamingDecoder.ts: create tests that mock
navigator.userAgent to a Windows string and invoke the code path where
shouldFailDecodeEndedEarly({... cancelled: this.cancelled, lastDecodedFrameSec,
requiredEndSec, streamDurationSec: this.metadata.streamDuration }) returns true,
then (1) for lastDecodedFrameSec set so decodeGapSec <= 3.0 assert the decoder
does not throw and that console.warn was called, (2) for lastDecodedFrameSec
null or decodeGapSec > 3.0 assert the decoder still throws, and (3) with a
non-Windows userAgent assert the decoder throws when
shouldFailDecodeEndedEarly() is true; use the same entry point used by the
integration tests (the StreamingVideoDecoder or the method that contains the
isWindows check) and ensure navigator.userAgent is reset after each test.
🪄 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: 7f18fb4c-d4c2-45d6-a818-cbbcc0df6941
📒 Files selected for processing (1)
src/lib/exporter/streamingDecoder.ts
Add requiredEndSec calculation to properly handle early decode termination by using the last segment's end time. This addresses issues with export processing on Windows platforms.
org-cyber
force-pushed
the
fix/windows-export-clean
branch
from
d40f40d to
e4d4ce2
Compare
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)
src/lib/exporter/streamingDecoder.ts (1)
252-256:⚠️ Potential issue | 🟠 MajorBase the Windows tolerance on kept span, not absolute media time.
requiredEndSecis an absolute source timestamp, so a clip trimmed to the last 5s of a 60s recording still getsMath.min(3, 60 * 0.1) === 3here. That means Windows can return success after missing most of the visible tail. Sincesrc/lib/exporter/videoExporter.ts:180-230treatsdecodeAll()returning as success, this can quietly ship a truncated export. lowkey safer to size the cap from the kept source span (or at least the final kept segment duration) instead ofrequiredEndSec.💡 One way to size the tolerance from the kept span
@@ const segments = this.splitBySpeed( this.computeSegments(this.metadata.duration, trimRegions), speedRegions, ); const requiredEndSec = segments[segments.length - 1]?.endSec ?? 0; +const keptSourceSpanSec = segments.reduce( + (total, segment) => total + (segment.endSec - segment.startSec), + 0, +); @@ -const maxToleratedGap = Math.min(3.0, requiredEndSec * 0.1); +const maxToleratedGap = Math.min(3.0, keptSourceSpanSec * 0.1);Also applies to: 515-518
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/streamingDecoder.ts` around lines 252 - 256, The Windows-tolerance cap is being computed from absolute media time (using requiredEndSec derived after computeSegments and splitBySpeed), which makes the tolerance too large for clips that only keep a short tail; change the logic to compute the cap from the kept span length (e.g., the duration of the final kept segment or total kept segments) instead of requiredEndSec. Locate where requiredEndSec is computed (after this.splitBySpeed(this.computeSegments(...)) in streamingDecoder.ts) and replace the absolute-end usage with the kept-span duration (finalSegment.endSec - finalSegment.startSec or sum of kept segment durations), and apply the same fix to the analogous logic around lines 515-518 so decodeAll() success won’t silently accept a truncated tail.
🤖 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/exporter/streamingDecoder.ts`:
- Line 225: The JSDoc for the parameter trimRegions is incorrect:
computeSegments treats trimRegions as spans to cut out (regions to discard), not
regions to keep; update the `@param` description for trimRegions in
streamingDecoder.ts (the computeSegments(...) JSDoc) to say these are time
ranges to remove/trim out (e.g., "Array of time regions to discard/trim out from
the input"), ensuring the doc matches the computeSegments implementation and any
mention elsewhere in that function.
---
Outside diff comments:
In `@src/lib/exporter/streamingDecoder.ts`:
- Around line 252-256: The Windows-tolerance cap is being computed from absolute
media time (using requiredEndSec derived after computeSegments and
splitBySpeed), which makes the tolerance too large for clips that only keep a
short tail; change the logic to compute the cap from the kept span length (e.g.,
the duration of the final kept segment or total kept segments) instead of
requiredEndSec. Locate where requiredEndSec is computed (after
this.splitBySpeed(this.computeSegments(...)) in streamingDecoder.ts) and replace
the absolute-end usage with the kept-span duration (finalSegment.endSec -
finalSegment.startSec or sum of kept segment durations), and apply the same fix
to the analogous logic around lines 515-518 so decodeAll() success won’t
silently accept a truncated tail.
🪄 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: 791ea455-e9fa-460b-a00c-1a05c5da0940
📒 Files selected for processing (1)
src/lib/exporter/streamingDecoder.ts
|
The current latest build fixes the export saving isssue when the video being recorded is less that 4,500 frames, this PR fixes the premature decode timeout/termination issue on windows, all tests ran and passed. feel free to check out the pr in your free time or reach out |
org-cyber
changed the title
|
fix ci issues 🙏 |
Address merge conflict markers added during resolution of Windows export fixes, ensuring clean integration of decode termination logic updates.
org-cyber
force-pushed
the
fix/windows-export-clean
branch
from
f4d6ce1 to
5caee9b
Compare
Sorry for the stress, fixed the formatting and lint warnings. Ready for review |
|
did you mean to close this? |
Yeah the CI is failing but it works perfectly, and all the tests pass, so uhhmm ill just use my version of the program, Dont want to add to your work. Really sorry for the stress and delay |
|
@org-cyber no worries, reopen it, i'll fix them in myself after merging it |
Thankyou very much, i Appreciate it Alot |
2 participants
Fixes #428
Description
I fixed two problems that stopped exports from working on Windows:
Export not saving – The latest release was supposed to fix this issue on windows but it still didnt work. The file save dialog appeared, but no file was created. This happened because the file path wasn't formatted correctly for Windows. I added code to clean up the path and make sure the folder exists before writing the file.
Export stopping early – Sometimes exports would fail with a "Decode ended early" error before finishing. On Windows, I made the app log a warning instead of crashing, so the export can finish with the frames it already has. Other operating systems keep the strict check.
Motivation
Windows users couldn't reliably export their recordings. These changes make exports work as expected on Windows without affecting macOS or Linux.
Type of Change
Related Issue(s)
None directly linked, but this addresses reports of export failures on Windows.
Screenshots / Video
No visual changes.
Testing
Recorded a short video and exported as MP4 on Windows 10 – file saved correctly.
Exported to folders with spaces in the name (like C:\My Videos) – worked fine.
Ran unit tests with npm test – 36 tests passed.
Checklist
I have performed a self-review of my code.
I have added any necessary screenshots or videos. (None needed)
I have linked related issue(s) and updated the changelog if applicable. (No issue linked)
Thank you for contributing!
Summary by CodeRabbit