Fix click-sound export parity for Lightning and Legacy#580
Open
crbender wants to merge 4 commits into
Open
Conversation
Unifies preview/export cursor click selection, captures extension audio events during export, and routes Lightning audio planning to offline mix when click-sound capture is active. Also adds guardrails and regression tests for selector behavior, export parity, and overlay decode caching. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds export-time capture of extension-triggered audio (cursor click sounds), a selector to pick a single click telemetry point for emission, refactors renderers to emit cursor:click earlier using the selector, caches decoded overlay audio, and merges captured audio regions into exporter audio processing and native planning. ChangesExtension Audio Export Parity for Click Sounds
Sequence DiagramsequenceDiagram
participant Component as Component/Renderer
participant ExtHost as ExtensionHost
participant Selector as Click Selector
participant Exporter as Exporter
participant Processor as AudioProcessor
Exporter->>ExtHost: hasEventListeners("cursor:click")?
ExtHost-->>Exporter: true/false
rect rgba(0, 128, 255, 0.5)
note right of Exporter: Export capture lifecycle
Exporter->>ExtHost: beginExportAudioCapture()
loop Frame decode & render
Exporter->>Component: render(timeMs)
Component->>Selector: selectCursorClickForEmission(telemetry, timeMs, lastEmitted)
Selector-->>Component: CursorClickTelemetryPoint | null
alt Click selected
Component->>ExtHost: emitEvent("cursor:click", cursorData)
activate ExtHost
ExtHost->>ExtHost: set currentEventContext = timeMs
ExtHost->>ExtHost: invoke handlers
ExtHost->>ExtHost: clear context
deactivate ExtHost
end
end
Exporter->>ExtHost: endExportAudioCapture()
Exporter->>ExtHost: drainExportAudioRegions(durationMs)
ExtHost-->>Exporter: AudioRegion[]
end
Exporter->>Exporter: getAudioRegionsForExport() = merge configured + captured
Exporter->>Processor: process(mergedAudioRegions)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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. ✨ Finishing Touches🧪 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 |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/exporter/modernFrameRenderer.ts (1)
3276-3284:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
post-videoandpost-webcamin the compositing gate.
compositeExtensions()still runs both of those hook phases, butshouldCompositeExtensionFrame()cannot be activated by them. An extension that only registerspost-videoorpost-webcamwill never composite in Modern export.Proposed fix
private shouldCompositeExtensionFrame(): boolean { return ( extensionHost.hasCursorEffects() || + extensionHost.hasRenderHooks("post-video") || extensionHost.hasRenderHooks("post-zoom") || extensionHost.hasRenderHooks("post-cursor") || + extensionHost.hasRenderHooks("post-webcam") || extensionHost.hasRenderHooks("post-annotations") || extensionHost.hasRenderHooks("final") || extensionHost.hasEventListeners("cursor:click") ); }🤖 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/modernFrameRenderer.ts` around lines 3276 - 3284, The compositing gate in shouldCompositeExtensionFrame() omits the "post-video" and "post-webcam" render hook checks causing extensions that only implement those hooks to never trigger compositing; update shouldCompositeExtensionFrame() to also OR in extensionHost.hasRenderHooks("post-video") and extensionHost.hasRenderHooks("post-webcam") so compositeExtensions() can run for those phases (keep the existing checks for hasCursorEffects, post-zoom, post-cursor, post-annotations, final, and the cursor:click event listener).src/lib/exporter/videoExporter.ts (1)
586-590:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t return
audioMode: "none"when extension capture is enabled.At
Line 586, the earlynonebranch ignoresthis.extensionAudioCaptureEnabled. For extension-only audio cases, this can misclassify the export as audio-less and skip the intended audio-handling path.Suggested fix
if ( !videoInfo.hasAudio && sourceAudioFallbackPaths.length === 0 && - audioRegions.length === 0 + audioRegions.length === 0 && + !this.extensionAudioCaptureEnabled ) { return { audioMode: "none" }; }🤖 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/videoExporter.ts` around lines 586 - 590, The early-return branch that sets audioMode to "none" uses the condition on videoInfo.hasAudio, sourceAudioFallbackPaths, and audioRegions but omits this.extensionAudioCaptureEnabled; update the conditional in the block (the check around videoInfo.hasAudio && sourceAudioFallbackPaths.length === 0 && audioRegions.length === 0) to also require that this.extensionAudioCaptureEnabled is false (i.e., only classify as "none" when extension audio capture is not enabled), so extension-only audio cases fall through to the audio-handling path.
🤖 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/lib/extensions/extensionHost.ts`:
- Line 965: The no-op disposer currently returns an empty block `() => {}` which
trips the lint rule; replace it with a concise expression-bodied no-op such as
`() => void 0` (i.e., change the returned noop disposer from `() => {}` to `()
=> void 0`) so the disposer (the noop returned in extensionHost.ts) no longer
contains an empty block statement.
---
Outside diff comments:
In `@src/lib/exporter/modernFrameRenderer.ts`:
- Around line 3276-3284: The compositing gate in shouldCompositeExtensionFrame()
omits the "post-video" and "post-webcam" render hook checks causing extensions
that only implement those hooks to never trigger compositing; update
shouldCompositeExtensionFrame() to also OR in
extensionHost.hasRenderHooks("post-video") and
extensionHost.hasRenderHooks("post-webcam") so compositeExtensions() can run for
those phases (keep the existing checks for hasCursorEffects, post-zoom,
post-cursor, post-annotations, final, and the cursor:click event listener).
In `@src/lib/exporter/videoExporter.ts`:
- Around line 586-590: The early-return branch that sets audioMode to "none"
uses the condition on videoInfo.hasAudio, sourceAudioFallbackPaths, and
audioRegions but omits this.extensionAudioCaptureEnabled; update the conditional
in the block (the check around videoInfo.hasAudio &&
sourceAudioFallbackPaths.length === 0 && audioRegions.length === 0) to also
require that this.extensionAudioCaptureEnabled is false (i.e., only classify as
"none" when extension audio capture is not enabled), so extension-only audio
cases fall through to the audio-handling 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e1416d8d-e81b-46ed-b361-2b4fa83acdc5
📒 Files selected for processing (13)
src/components/video-editor/VideoPlayback.tsxsrc/lib/exporter/audioEncoder.test.tssrc/lib/exporter/audioEncoder.tssrc/lib/exporter/frameRenderer.test.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/modernFrameRenderer.test.tssrc/lib/exporter/modernFrameRenderer.tssrc/lib/exporter/modernVideoExporter.nativeStaticLayout.test.tssrc/lib/exporter/modernVideoExporter.tssrc/lib/exporter/videoExporter.tssrc/lib/extensions/cursorClickSelection.test.tssrc/lib/extensions/cursorClickSelection.tssrc/lib/extensions/extensionHost.ts
- replace empty no-op block in extensionHost disposer\n- include post-video and post-webcam hooks in modern compositing gate\n- prevent legacy audioPlan from returning none when extension capture is enabled Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/exporter/modernFrameRenderer.test.ts (1)
757-772: ⚡ Quick winAdd a matching
post-webcamcompositing test.This new case pins the
post-videobranch, but the siblingpost-webcambranch added insrc/lib/exporter/modernFrameRenderer.tsis still uncovered. A second assertion here would keep the full gate change from partially regressing.🤖 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/modernFrameRenderer.test.ts` around lines 757 - 772, Test only covers the "post-video" branch of compositing; add a parallel assertion that pins the "post-webcam" branch by mocking extensionHost.hasRenderHooks to return true when phase === "post-webcam" and asserting shouldCompositeExtensionFrame() is true. Modify the test block around createRenderer() / renderer.shouldCompositeExtensionFrame() to include a second mock (or a separate it() case) that mirrors the existing setup but uses hasRenderHooks.mockImplementation((phase: string) => phase === "post-webcam"), and ensure you restore that spy along with the others (hasCursorEffectsSpy, hasRenderHooksSpy, hasEventListenersSpy) so the test suite remains isolated.
🤖 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/lib/exporter/videoExporter.ts`:
- Around line 589-590: The current guard uses extensionAudioCaptureEnabled
(derived from extensionHost.hasEventListeners("cursor:click")) to decide the
no-audio fast path, which is wrong; change the logic in videoExporter.ts so that
the "none" audio path is determined solely from drained/captured audio regions
(audioRegions after draining) or an explicit "captured audio exists" flag
populated when playSound() events actually produce audio, and remove
extensionAudioCaptureEnabled from this guard. Specifically, stop using
extensionAudioCaptureEnabled (and the hasEventListeners call) as proof of
captured audio and instead check audioRegions.length (or a boolean set when
audio frames/regions are appended during drain) to choose the audio plan.
---
Nitpick comments:
In `@src/lib/exporter/modernFrameRenderer.test.ts`:
- Around line 757-772: Test only covers the "post-video" branch of compositing;
add a parallel assertion that pins the "post-webcam" branch by mocking
extensionHost.hasRenderHooks to return true when phase === "post-webcam" and
asserting shouldCompositeExtensionFrame() is true. Modify the test block around
createRenderer() / renderer.shouldCompositeExtensionFrame() to include a second
mock (or a separate it() case) that mirrors the existing setup but uses
hasRenderHooks.mockImplementation((phase: string) => phase === "post-webcam"),
and ensure you restore that spy along with the others (hasCursorEffectsSpy,
hasRenderHooksSpy, hasEventListenersSpy) so the test suite remains isolated.
🪄 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: bb8faf89-e469-4076-998d-c8c2452d58c5
📒 Files selected for processing (4)
src/lib/exporter/modernFrameRenderer.test.tssrc/lib/exporter/modernFrameRenderer.tssrc/lib/exporter/videoExporter.tssrc/lib/extensions/extensionHost.ts
- use captured regions only for legacy audioPlan none-fast-path\n- add post-webcam compositing gate test in modernFrameRenderer Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolve audioEncoder conflicts by preserving both the offline mix soft limiter and capped overlay decode cache, with merged test coverage for both behaviors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Author
|
Resolved the merge conflicts by keeping both changes in :\n- preserved capped overlay decode caching\n- preserved offline soft-limiter logic\n- merged test coverage for both paths\n\nPushed in commit ; PR should now be conflict-free. |
Author
|
Conflict resolution update: merged both sides of the audioEncoder conflicts and kept all intended behavior.
Pushed in commit |
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.
Description
This PR closes the gap where click sounds worked in the editor, but were missing or inconsistent in exports, especially on Lightning.
Main focus here is parity plus reliability. We now use the same click selection semantics in preview and both export paths, capture extension-triggered click audio during export, and force Lightning onto the audio path that can actually mix those captured events.
What changed
extensionHost(beginExportAudioCapture,drainExportAudioRegions,endExportAudioCapture) and wiredplaySoundcapture for export-time click events.cursorClickSelection.ts) and moved preview, legacy renderer, and modern renderer to the same selection behavior.click,double-click,right-click,middle-click) so one physical click does not retrigger frommouseup.cursor:clickemits before hook/effect execution.hasEventListeners("cursor:click")) so event-only extensions still run in export.offline-render-fallbackand is not dropped on no-source-audio recordings.MAX_CACHED_AUDIO_REGION_PATHS) to avoid unbounded growth.Why this should fix the issue
Testing
cursorClickSelectionframeRenderer/modernFrameRenderercursor click parity testsmodernVideoExporter.nativeStaticLayoutaudio-plan testsaudioEncoderoverlay-cache testsRelated issues
Closes #572
Closes #573
Closes #574
Closes #575
Closes #576
Closes #577
Closes #578
Related follow-up: #579 (docs and author guidance)
Summary by CodeRabbit
New Features
Performance Improvements
Bug Fixes
Tests