fix(waveform): stop re-decoding audio-less recordings, and unify "no camera" (#348) - #349
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change centralizes camera-source resolution, supports camera-less export and project reopening, improves no-audio waveform handling, and adds deterministic media-link registry synchronization in tests. ChangesCamera-less project handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Asset
participant CameraResolver
participant SceneBuilder
participant NativeCompositor
participant CompositorDecoder
Asset->>CameraResolver: provide asset camera track
CameraResolver-->>SceneBuilder: resolved path and offset
CameraResolver-->>NativeCompositor: resolved path and offset
SceneBuilder->>CompositorDecoder: serialized clip sources
NativeCompositor->>CompositorDecoder: active camera source and offset
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Two test files failed intermittently in the full suite and passed every time in isolation, a different one on each run: cursorSidecar.test.ts ENOTEMPTY: directory not empty, rmdir '…/recordings' mediaLinksRegistry.test expected "warn" to be called at least once One cause, not two. `findMediaLinksByFingerprint` refreshes a drifted path with a write it deliberately does not await — a lookup must not pay for a write it does not need — and nothing could wait for it either. So the work outlived the call that started it: a caller removing the directory raced the write into recreating an entry between `fs.rm`'s recursive walk and its final rmdir, and a test asserting on the write's outcome was asserting on a coin flip behind a fixed 50 ms. The comment above that refresh already documents an earlier round of this same race, reported then as `mkdir ENOENT` after 1628 passing tests. That round stopped the failure becoming an unhandled rejection; it left the work un-awaitable, which is what came back. `whenRegistryIdle` drains the queues `withWriteLock` already keeps, so the two tests wait instead of guessing — the teardown before `fs.rm`, and `withoutUnhandledRejections` before its assertions. It loops rather than awaiting once because a drained write can have queued another behind it, and it never throws: the tails swallow their outcome, so this means "the writes are done", not "the writes succeeded". Deliberately not papered over with a retrying `fs.rm`. That pattern already lives in this file as `rmBestEffort` and is right for the one test that races removal on purpose; for these two the race was accidental, so it is removed rather than tolerated. The new case asserts the refresh is durably on disk once the drain resolves. Neutering `whenRegistryIdle` to a no-op turns it red — without that, it would pass just as well with the whole drain deleted.
Issue #348: a project recorded with no camera and no microphone. Recording a screen demo with both off is the default for most users, and the failure lands at reopen — the recording exists on disk and the user cannot get to it. The crash itself is not reproduced here, and this does not claim to fix it. What is measured is a cost that only a mic-less recording pays, on every single project open. The Windows helper muxes an audio stream only when a source is enabled (`main.cpp:835`), so with mic and system audio off the MP4 has no audio stream at all — confirmed with ffprobe on real captures, and 22 of the 30 camera-less recordings on this machine are in that state, the largest being 175 MB of 14-minute capture. `get-audio-peaks` already answers correctly for such a file. Its three replies mean different things: peaks, `peaks: null` for "no native ffmpeg on this host", and `success: false` for "ffmpeg ran and found nothing to decode". Only the middle one is a gap the browser pipelines exist to cover. The renderer treated all three the same and fell through, so after ffmpeg had answered in ~2s the renderer spent a 175 MB copy into OPFS plus a full Chromium decode re-discovering it. The main process already documents the intent — "degrade quietly: the renderer draws no waveform, which is correct" — and the caller now honours it. Empty peaks rather than a throw, so the answer caches like any other result and is never recomputed. Failures cache too, which is why the map is nullable and lookups go through `has()`: a file with no audio fails deterministically, so retrying it is pure cost, and on a host with no ffmpeg that retry is the whole-file browser decode. Caching only successes meant a recording WITH a mic paid for its waveform once while one WITHOUT paid, and threw away, the same work on every mount. Nothing is decoded before `durationSec` is known either. It is not a hint: it is what routes the work to the cheap native tier, and `ClipWaveform` cannot draw a bar without it, so starting early bought nothing and cost a full-file read. Measured on the real app against a real camera-less 175 MB project: expensive browser decodes on open, 1 -> 0. The reopen regression test the issue asks for lives with the other getProject cases. It asserts the project still appears in `listProjects` — a document that throws there is dropped by the skip-on-error catch and presents as "my project vanished" rather than as an error — and that the relinker does not hand its webcam to an asset that never had one.
Five producers of a webcam path, five spellings of "no camera": sceneDescription.ts "" honours visible sceneDescription.ts clipHasCamera a second copy of the same test NativeCompositorOverlay undefined honours visible NativeCompositorOverlay "" honours visible ExportDialog.tsx ?? asset.originalPath IGNORES visible CliExportRunner.tsx ?? asset.originalPath IGNORES visible The export pair is issue #265's defect shape: substituting the screen recording makes "no camera" indistinguishable from "the camera happens to be this file". It only ever worked because both fields were filled from the same variable, so the two strings matched byte for byte and `webcam_is_real`'s `eq_ignore_ascii_case` caught it. Any producer deriving one of them differently — a separator, a case, a resolved path, all routine on Windows — would have re-opened #265, and the reconciliation would still have been a draw gate downstream of five producers. They now route through `assetCameraSource`, in the module that already owns camera resolution and is import-safe from the main process. `""` is the one sentinel. A hidden camera counts as no camera, which the preview and the scene already did and the two export producers did not — the same project rendered two ways depending on who asked. The Rust change is a prerequisite, not a tidy-up: `timeline_walk` opened the webcam decoder with a bare `?`, so the moment the export producers stopped sending the screen path, exporting any camera-less project would have failed outright. It now falls back to the screen exactly as `live.rs` already does; `set_has_webcam` has already decided not to draw the PiP, and the decoder exists only because `compose_frame` samples two streams unconditionally. Verified by ablation on the rebuilt addon, same recording both ways: with the fallback stats {"frames":72,…}, valid MP4 without the fallback EXPORT FAILED: open_input: -22 (Invalid argument) The `hasAudio: true` comment went with it. It claimed recordings from this app always carry a decodable audio track; a capture with no mic and no system audio has no audio stream at all. The value stays optimistic because nothing populates `asset.audio` yet and every consumer downstream degrades on a stream-less file, but the reason is now the true one. Note for anyone building from this commit: the native addon must be rebuilt with it. `build:win` compiles it from the same sources, so releases stay in step; a dev tree with a stale `.node` will fail camera-less export.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@crates/compositor/src/timeline_walk.rs`:
- Around line 178-190: The webcam decoder cache in the fallback branch of the
timeline decoder setup is keyed by the empty camera path, causing camera-less
clips with different screen sources to share a decoder. Use a distinct cache key
incorporating clip.screen, or a typed camera/screen key, while preserving reuse
for the same source; add a regression test covering two camera-less clips with
different screen files.
In `@src/hooks/useAudioPeaks.test.ts`:
- Around line 98-112: Update the test around useAudioPeaks so it waits for the
failed decode to settle, using a local console.warn spy or another
post-rejection observable, before unmounting the first hook. Then retain the
remount assertions and streamingCalls count to verify the permanent failure
cache prevents retries, and add coverage for the new behavior in the same test
package if needed.
In `@src/lib/ai-edition/timeline/camera.test.ts`:
- Line 128: Update the test description in the camera test case to use double
quotes instead of single quotes, without changing the test wording or behavior.
- Around line 119-145: Add a test in the assetCameraSource describe block
covering an asset with a visible cameraTrack whose sourcePath is empty, and
assert it returns the no-camera result { path: "", offsetSec: 0 }. Reuse the
existing asset fixture and preserve the current coverage for hidden, missing,
and valid camera sources.
🪄 Autofix
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: 29691bf3-6bce-4aaa-9d96-bc0e1198d0f9
📒 Files selected for processing (13)
crates/compositor/src/timeline_walk.rselectron/ai-edition/document-service.test.tselectron/media/cursorSidecar.test.tselectron/media/mediaLinksRegistry.test.tselectron/media/mediaLinksRegistry.tssrc/cli/CliExportRunner.tsxsrc/components/ai-edition/ExportDialog.tsxsrc/components/ai-edition/NativeCompositorOverlay.tsxsrc/hooks/useAudioPeaks.test.tssrc/hooks/useAudioPeaks.tssrc/lib/ai-edition/timeline/camera.test.tssrc/lib/ai-edition/timeline/camera.tssrc/native/sceneDescription.ts
| expect(assetCameraSource(asset)).toEqual({ path: "/cam-1.mp4", offsetSec: 0.3 }); | ||
| }); | ||
|
|
||
| it('says "no camera" with an empty path — NEVER the screen recording', () => { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use double quotes.
Replace the single-quoted test description with double quotes.
As per coding guidelines, **/*.{ts,tsx,js,jsx,mts,cts,json} requires double quotes.
🤖 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/ai-edition/timeline/camera.test.ts` at line 128, Update the test
description in the camera test case to use double quotes instead of single
quotes, without changing the test wording or behavior.
Source: Coding guidelines
…opened
Review catch, and a regression this branch introduced. Every camera-less clip
now carries the SAME webcam path (the empty sentinel), so caching the fallback
decoder under it made the second camera-less clip reuse the first one's
decoder — the screen file of a different clip.
That is not cosmetic even though the PiP is not drawn: `webcam_available_
duration` clamps `source_end_sec` a few lines below, unconditionally. A 60s
clip following a clip whose source is 41s long was cut to 41s.
Before this branch the export producers sent the screen path as the webcam
path, so the key was per-clip by accident and the collision could not happen.
Unifying on `""` removed that accident, which is exactly the class of thing
the unification was supposed to prevent — one shared sentinel needs one
deliberate key, not an inherited one.
Verified by ablation on the rebuilt addon, two camera-less clips, 3s of a 41s
file then 60s of an 866s file:
keyed by the opened file videoDurationS 63 both clips full length
keyed by the raw path videoDurationS 44.25
"clip #1 raccourci de 18.767s
(fin demandée 60.000s, fin disponible 41.233s)"
Two test gaps from the same review:
- The no-audio cache test waited on `streamingCalls`, which only proves the
decode STARTED. The remounts could then join the still-pending in-flight
promise and the test would pass even with the failure cache broken. It now
waits on the hook's own warning, the first observable after the rejection
settles, and asserts it fired exactly once.
- `assetCameraSource` has a `!cam.sourcePath` branch with no coverage. A
parsed document cannot reach it (`cameraTrackSchema` requires a non-empty
path) but the accessor takes an asset, not a parse result.
Not taken: the review also asked for double quotes in a test title that Biome
formats with single quotes because the string contains double quotes. Biome
owns that and passes.
EtienneLescot
force-pushed
the
claude/bug-correction-e2e-test-1a54e1
branch
from
August 12, 2026 10:45
da1cdc1 to
6aa3d9e
Compare
Contributor
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 `@crates/compositor/src/timeline_walk.rs`:
- Around line 185-202: The webcam decoder source, cache key, and webcam
visibility must remain consistent in the timeline composition flow. Update the
logic around webcam_decs insertion and webcam_is_real so a failed non-empty
clip.webcam either propagates the decoder error for real camera paths or falls
back atomically to clip.screen with its screen key and false webcam flag; ensure
later clips cannot reuse a decoder cached under the failed webcam path, and add
a regression test covering two screens sharing an unopenable webcam path.
🪄 Autofix
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: c06eb58d-3e8a-49cf-821f-872d15e8372c
📒 Files selected for processing (3)
crates/compositor/src/timeline_walk.rssrc/hooks/useAudioPeaks.test.tssrc/lib/ai-edition/timeline/camera.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/ai-edition/timeline/camera.test.ts
- src/hooks/useAudioPeaks.test.ts
…ther Second review pass, on the fix from the previous commit. The fallback I added fired on ANY `Decoder::open` failure, not just the empty path it was written for, and that made two things disagree with each other. A non-empty webcam path that cannot be opened — a camera file the user moved or deleted — fell back to the screen, but `webcam_key` stayed the webcam path (so the decoder and its cache key named different files) and `webcam_is_real` stayed true (so the screen was drawn into the webcam's own PiP box). The second half is issue #265 exactly, reintroduced by a fallback meant for a different case. Source, key and `has_webcam` now come from one decision: no camera -> open the screen as a stand-in, key by the screen, no PiP a camera -> open it, key by it, draw it; a failure propagates which is what happened before this branch added a fallback at all, so a missing camera file is still a loud error rather than a wrong picture. It also drops the `match`: the fallback is expressed by choosing the source, not by catching an error after the fact. Verified on the rebuilt addon: two camera-less clips, 3s of a 41s file then 60s of an 866s file -> videoDurationS 63, both clips full length one clip whose non-empty webcam path does not exist -> refused: open_input: -2 (No such file or directory) `live.rs` computes `has_real_webcam` the same way and has the same blind spot for a non-empty path that will not open. Out of scope here — the live path never gained this fallback — but it is the same shape and worth its own look.
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.
Closes #348 — partially. Read the first section before the rest.
The crash is not reproduced, and this does not claim to fix it
Reopening a camera-less, mic-less project works on this machine. That was checked, not assumed:
migrate + documentSchema.parsereplayed over all 158 real projects on this machine (schema v4→v7, 34 of them camera-less): zero failures.status=ready error=null doc=loaded, preview painting, no error card.That last run is worth keeping, because it pins the on-disk shape this whole PR is about:
22 of the 30 camera-less recordings on this machine are in that state. The Windows helper muxes an audio stream only when a source is enabled (
wgc-capture/src/main.cpp:835), so "no mic + no system audio" means no audio track, not a silent one.So: what follows is a measured cost and a real latent defect on that path, not a proven fix for the reporter's crash. Issue #348 should stay open until the questions at the bottom are answered.
What is fixed
1. A mic-less recording paid for its missing waveform on every open
get-audio-peaksdistinguishes three replies, and only one is a reason to fall through:success+ peakssuccess+peaks: nullsuccess: falseThe renderer treated all three alike. So for a file with no audio, ffmpeg answered in ~2s and the renderer then spent a 175 MB copy into OPFS plus a full Chromium decode re-discovering it — on every project open, since a module-scope cache starts empty each launch. The main process already documents the intent (
"degrade quietly: the renderer draws no waveform, which is correct"); the caller now honours it.Failures are cached too. A recording with a mic paid for its waveform once; one without paid, and threw away, the same work on every mount. That asymmetry is the interesting part of this report.
Measured on the real app, real project: expensive browser decodes on open, 1 → 0.
2. Five spellings of "no camera"
visible?sceneDescription.ts""sceneDescription.ts(clipHasCamera)NativeCompositorOverlay.tsxundefinedNativeCompositorOverlay.tsx""ExportDialog.tsx?? asset.originalPathCliExportRunner.tsx?? asset.originalPathThe export pair is #265's defect shape. It only ever worked because both fields came from the same variable, so the strings matched byte for byte and
webcam_is_real'seq_ignore_ascii_casecaught it — a separator, a case or a resolved path would have re-opened #265. All six now route throughassetCameraSource, and a hidden camera counts as no camera everywhere instead of in three places out of five.The Rust change is a prerequisite, not a tidy-up.
timeline_walkopened the webcam decoder with a bare?, so the moment producers stopped sending the screen path, every camera-less export would have failed. Proven by ablation on the rebuilt addon, same recording both ways:build:wincompiles it from the same sources so releases stay in step, but a dev tree with a stale.nodewill fail camera-less export.3. Two flaky tests (unrelated, surfaced by these runs)
cursorSidecarfailing withENOTEMPTYandmediaLinksRegistryfailing onexpected "warn" to be calledwere one defect:findMediaLinksByFingerprintrefreshes a drifted path with a write nobody could await. Its own comment documents an earlier round of the same race.whenRegistryIdledrains the queueswithWriteLockalready keeps.Verification
tsc(app + tests) and Biome clean.Every non-trivial claim above has an ablation behind it: the peaks tests go red without the fix, the new registry-drain test goes red with the drain no-op'd, and the export dies without the Rust fallback.
Still unknown — needs the reporter
Deliberately not in this PR
hasAudio: trueis still hardcoded in three producers. Nothing populatesasset.audio, and every consumer degrades on a stream-less file, so the value stays — but the comment claiming recordings always carry audio is gone, because ffprobe says otherwise.projectStorewritesstatus: "error"that nothing reads, and there is no error boundary anywhere insrc/. That is why this report arrived with no error text, and it is worth its own issue.Summary by CodeRabbit
Bug Fixes
Reliability