Add Waveform as Background of the Trim Track - Easier to size the Trim#647
Open
davideme wants to merge 19 commits into
Open
Add Waveform as Background of the Trim Track - Easier to size the Trim#647davideme wants to merge 19 commits into
davideme wants to merge 19 commits into
Conversation
- Add RowWaveform component (refactored from TrimWaveform) that decodes audio via AudioContext, caches peaks per URL, and renders a faint waveform canvas behind any timeline row using useTimelineContext() for zoom/pan alignment - Wire RowWaveform as background of the trim row in TimelineEditor - Add showTrimWaveform boolean to EditorState / ProjectEditorState with default true; persists across saves/loads - Add Timeline Settings accordion section in SettingsPanel (always visible) with a Waveform toggle to enable/disable the visualization - Bump hint text opacity from 0.12 to 0.25 so "Press T to add trim" remains readable over the waveform background Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add "Timeline" as a dedicated SettingsPanelMode with AudioWaveform icon - Waveform toggle now lives exclusively in the Timeline panel (not in every panel) - Default showTrimWaveform to false so trim row starts with visible hint text - Guard AccordionItem render with activePanelMode === "timeline" to prevent bleed Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace AudioWaveform icon with Brackets on the Timeline panel tab and section header - Rename toggle label from "Waveform" to "Show Audio Waveform on Trim Track" - Make toggle full-width to accommodate the longer label Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The 0.25 bump was a workaround for hint visibility over the waveform background, but is now stale — waveform defaults to off and hints only appear on empty rows anyway. Restores consistency with all other row hints. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Move fetch/decode/cache logic out of RowWaveform into a standalone useAudioPeaks hook. RowWaveform is now a pure canvas renderer that accepts pre-computed peaks as a prop. TimelineEditor calls the hook and passes peaks down, skipping decoding entirely when waveform is off. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…loading Add a single exported loadFileAsArrayBuffer in streamingDecoder.ts that owns the IPC-vs-fetch branching for local and remote video URLs. StreamingVideoDecoder.loadSourceFile and useAudioPeaks both delegate to it, removing the duplicated readBinaryFile/fetch logic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e with useRef - Move computePeaks into audioPeaksWorker.ts — runs off the main thread with zero-copy channel buffer transfer - Replace module-level peaksCache Map with a useRef scoped to the hook instance — no global mutable state, toggle off/on still instant Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add timeline.title and timeline.waveform keys to every settings.json locale file. Replace all hardcoded "Timeline" and "Show Audio Waveform on Trim Track" strings in SettingsPanel with t() calls. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
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 optional trim-track waveform rendering: editor state and persistence for ChangesAudio Waveform Visualization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 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. 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 |
![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: b7d7c08906
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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: 3
🧹 Nitpick comments (2)
src/lib/exporter/streamingDecoder.ts (1)
189-193: ⚡ Quick winSanitize filename extraction before creating
FileLine 189 currently splits the raw URL string; URLs with query/hash (or
data:) can produce kinda cursed filenames likeclip.mp4?token=...or a giant data URI. nit: cleaner to parse withURLand fall back to"video".Proposed patch
- const filename = videoUrl.split(/[\\/]/).pop() || "video"; + const filename = (() => { + try { + const parsed = new URL(videoUrl, window.location.href); + if (parsed.protocol === "blob:" || parsed.protocol === "data:") return "video"; + const name = decodeURIComponent(parsed.pathname.split("/").pop() ?? ""); + return name || "video"; + } catch { + const name = (videoUrl.split(/[\\/]/).pop() ?? "").split(/[?#]/)[0]; + return name || "video"; + } + })();🤖 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/streamingDecoder.ts` around lines 189 - 193, The filename extraction from videoUrl is unsafe: update the logic in streamingDecoder (where filename and File are created) to parse the URL using the URL constructor when possible, strip any search and hash (e.g., use pathname's last segment) and fall back to a safe default "video" for data: URIs or invalid URLs; ensure you handle exceptions from new URL(videoUrl) and trim/remove query params so that the File is created with a clean filename before calling new File([...], filename, ...).src/i18n/locales/zh-TW/settings.json (1)
13-13: pre-existing: malformed nested speed objectnot part of your changes, but there's an empty
"speed": {}nested inside the zoom object. speed is already defined properly at lines 30-35, so this nested one is kinda cursed and should probably be removed at some point.🤖 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/i18n/locales/zh-TW/settings.json` at line 13, Remove the accidental empty nested "speed": {} property inside the "zoom" object in the zh-TW locale so it doesn't shadow or duplicate the proper "speed" definition; locate the nested "speed" key inside the "zoom" object (the one set to {}) and delete that entry, leaving the correctly defined top-level "speed" object (lines where "speed" is properly defined) intact.
🤖 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/timeline/RowWaveform.tsx`:
- Around line 37-50: The effect currently returns early when peaks is
null/empty, leaving previous drawing visible; modify the useEffect in
RowWaveform (the block using canvasRef, peaks, canvasSize) to obtain the canvas
and its 2D context first and call ctx.clearRect(0, 0, canvasSize.w,
canvasSize.h) (or canvas.width/height cleared at devicePixelRatio scale) before
returning when peaks is falsy, so the canvas is always cleared even if there's
nothing to draw.
In `@src/hooks/useAudioPeaks.ts`:
- Around line 67-87: On cache miss and on failure clear stale waveform state
immediately: when cacheRef.current.get(videoUrl) returns falsy, call
setPeaks(null) before starting the async work (so old peaks don't linger), and
inside the catch block call setPeaks(null) if the operation was not cancelled
(check the cancelled flag). Update the logic around loadFileAsArrayBuffer /
getAudioCtx().decodeAudioData / computePeaksInWorker to ensure cancelled is
respected before setting peaks and that cacheRef.current.set(videoUrl, p) only
happens on success.
In `@src/i18n/locales/zh-TW/settings.json`:
- Line 210: The translation for the "waveform" key uses inconsistent terminology
("修剪軌道") versus the rest of the trim feature which uses "剪輯"; change the value
of the "waveform" entry so it matches the existing terminology (e.g., update
"waveform": "在修剪軌道上顯示音訊波形" to "在剪輯軌道上顯示音訊波形" or "在剪輯區域上顯示音訊波形") to keep
terminology consistent with other trim strings such as those under the trim
section.
---
Nitpick comments:
In `@src/i18n/locales/zh-TW/settings.json`:
- Line 13: Remove the accidental empty nested "speed": {} property inside the
"zoom" object in the zh-TW locale so it doesn't shadow or duplicate the proper
"speed" definition; locate the nested "speed" key inside the "zoom" object (the
one set to {}) and delete that entry, leaving the correctly defined top-level
"speed" object (lines where "speed" is properly defined) intact.
In `@src/lib/exporter/streamingDecoder.ts`:
- Around line 189-193: The filename extraction from videoUrl is unsafe: update
the logic in streamingDecoder (where filename and File are created) to parse the
URL using the URL constructor when possible, strip any search and hash (e.g.,
use pathname's last segment) and fall back to a safe default "video" for data:
URIs or invalid URLs; ensure you handle exceptions from new URL(videoUrl) and
trim/remove query params so that the File is created with a clean filename
before calling new File([...], filename, ...).
🪄 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: aafe51ee-a32e-423c-88c3-0e0f72d4a4f8
📒 Files selected for processing (23)
src/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/editorDefaults.tssrc/components/video-editor/projectPersistence.tssrc/components/video-editor/timeline/Row.tsxsrc/components/video-editor/timeline/RowWaveform.tsxsrc/components/video-editor/timeline/TimelineEditor.tsxsrc/hooks/audioPeaksWorker.tssrc/hooks/useAudioPeaks.tssrc/hooks/useEditorHistory.tssrc/i18n/locales/ar/settings.jsonsrc/i18n/locales/en/settings.jsonsrc/i18n/locales/es/settings.jsonsrc/i18n/locales/fr/settings.jsonsrc/i18n/locales/it/settings.jsonsrc/i18n/locales/ja-JP/settings.jsonsrc/i18n/locales/ko-KR/settings.jsonsrc/i18n/locales/ru/settings.jsonsrc/i18n/locales/tr/settings.jsonsrc/i18n/locales/vi/settings.jsonsrc/i18n/locales/zh-CN/settings.jsonsrc/i18n/locales/zh-TW/settings.jsonsrc/lib/exporter/streamingDecoder.ts
Rename to BackgroundWaveform to reflect its visual role rather than structural placement. Remove the redundant wrapper div — Row already provides relative overflow-hidden, so the canvas can sit absolute inset-0 directly and observe itself via ResizeObserver. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Reset peaks to null immediately when an uncached URL is set, so the previous video's waveform is not shown during decode. Also clear on decode failure so stale data does not persist indefinitely. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use 剪輯 (editing/clip) instead of 修剪 (trimming) to match the term already used in the trim section throughout zh-TW strings. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Move the peaks null-check after clearRect so a previously drawn waveform is erased when peaks transitions to null (e.g. on URL change or decode failure) rather than lingering on screen. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add docstrings to all undocumented functions and methods across the files touched in this branch: getAudioCtx, Row component, and all StreamingVideoDecoder methods (loadSourceFile, computeSegments, getExportMetrics, splitBySpeed, getDemuxer, cancel, destroy, withTimeout). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/hooks/useAudioPeaks.ts (1)
15-43: 💤 Low valueWorker can't be aborted on cancellation — resource waste on rapid URL changes.
lowkey risky: if
videoUrlchanges or component unmounts mid-computation, thecancelledflag in the effect prevents state updates, but the worker keeps chugging along until it finishes (then terminates). rapid video switches = orphaned workers stacking up until they each complete.for typical usage it's probably fine, but if someone scrubs through a video list quickly with large files, you'll have multiple workers burning CPU concurrently.
one option: return an abort handle from this function so the effect cleanup can terminate the worker early.
💡 Optional: return abort handle
-function computePeaksInWorker(audioBuffer: AudioBuffer): Promise<Float32Array> { - return new Promise((resolve, reject) => { +function computePeaksInWorker( + audioBuffer: AudioBuffer, + signal?: AbortSignal, +): Promise<Float32Array> { + return new Promise((resolve, reject) => { const worker = new Worker(new URL("./audioPeaksWorker.ts", import.meta.url), { type: "module", }); + + signal?.addEventListener("abort", () => { + worker.terminate(); + reject(new DOMException("Aborted", "AbortError")); + }); // ... rest unchangedThen in the effect:
const abortController = new AbortController(); // pass abortController.signal to computePeaksInWorker return () => { cancelled = true; abortController.abort(); };🤖 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/useAudioPeaks.ts` around lines 15 - 43, computePeaksInWorker currently spawns a Worker that cannot be terminated from the caller; change its API to accept an optional AbortSignal (e.g., computePeaksInWorker(audioBuffer: AudioBuffer, signal?: AbortSignal): Promise<Float32Array>) and inside the function keep a reference to the Worker and wire signal.addEventListener('abort', ...) to terminate the worker and reject the promise with an AbortError/DOMException; also guard against posting the message if already aborted. In the effect that calls computePeaksInWorker (where you currently use a cancelled flag), create an AbortController, pass controller.signal into computePeaksInWorker, and call controller.abort() in the cleanup so the worker is terminated immediately on URL change or unmount. Ensure worker.terminate() is called both on normal completion, error, and on abort to avoid orphaned workers.src/lib/exporter/streamingDecoder.ts (1)
183-196: 💤 Low valuefilename extraction is kinda cursed for data: URLs
line 190 splits on
/or\and grabs the last segment. for normal paths and blob URLs this works fine, but data: URLs would yield something like"mp4;base64,AAAA..."as the filename - not ideal.also, line 191 creates the blob without a MIME type, so
blob.typeis always""and you'll always fall back to"application/octet-stream". web-demuxer probably doesn't care since it does format detection anyway, but worth noting if this util gets used elsewhere.lowkey fine as-is since data: URLs for full videos are super rare, but a quick sanitization or fallback would be cleaner:
💡 Optional: handle data: URL filenames more gracefully
- const filename = videoUrl.split(/[\\/]/).pop() || "video"; + const isDataUrl = videoUrl.startsWith("data:"); + const filename = isDataUrl + ? "video" + : videoUrl.split(/[\\/]/).pop() || "video";🤖 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/streamingDecoder.ts` around lines 183 - 196, The filename logic in loadSourceFile produces bad names for data: URLs and the Blob is created without a MIME type; update loadSourceFile to detect data: URLs (videoUrl.startsWith("data:")), parse out the MIME type (the part between "data:" and the first ";"), derive a safe filename (e.g., "video" + extension from the MIME or fallback "video") by sanitizing out params like "base64,..." and commas, and create the Blob with that MIME via new Blob([buffer], { type: mime || "" }) so blob.type is set before constructing the File(filename, { type: ... }); keep the existing behavior for non-data URLs (videoUrl split) and ensure the File uses the computed filename and blob.type.
🤖 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/hooks/useAudioPeaks.ts`:
- Around line 15-43: computePeaksInWorker currently spawns a Worker that cannot
be terminated from the caller; change its API to accept an optional AbortSignal
(e.g., computePeaksInWorker(audioBuffer: AudioBuffer, signal?: AbortSignal):
Promise<Float32Array>) and inside the function keep a reference to the Worker
and wire signal.addEventListener('abort', ...) to terminate the worker and
reject the promise with an AbortError/DOMException; also guard against posting
the message if already aborted. In the effect that calls computePeaksInWorker
(where you currently use a cancelled flag), create an AbortController, pass
controller.signal into computePeaksInWorker, and call controller.abort() in the
cleanup so the worker is terminated immediately on URL change or unmount. Ensure
worker.terminate() is called both on normal completion, error, and on abort to
avoid orphaned workers.
In `@src/lib/exporter/streamingDecoder.ts`:
- Around line 183-196: The filename logic in loadSourceFile produces bad names
for data: URLs and the Blob is created without a MIME type; update
loadSourceFile to detect data: URLs (videoUrl.startsWith("data:")), parse out
the MIME type (the part between "data:" and the first ";"), derive a safe
filename (e.g., "video" + extension from the MIME or fallback "video") by
sanitizing out params like "base64,..." and commas, and create the Blob with
that MIME via new Blob([buffer], { type: mime || "" }) so blob.type is set
before constructing the File(filename, { type: ... }); keep the existing
behavior for non-data URLs (videoUrl split) and ensure the File uses the
computed filename and blob.type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3476c007-a32c-42ef-aa56-6c177737488c
📒 Files selected for processing (3)
src/components/video-editor/timeline/Row.tsxsrc/hooks/useAudioPeaks.tssrc/lib/exporter/streamingDecoder.ts
- computePeaksInWorker now accepts an AbortSignal; terminates the worker immediately on abort and rejects with AbortError so orphaned workers cannot outlive the effect cleanup - useAudioPeaks wires an AbortController into the effect; controller.abort() fires alongside the existing cancelled flag on URL change or unmount - loadSourceFile handles data: URLs by parsing the MIME type, deriving a safe filename (e.g. video.mp4), and setting Blob type correctly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/exporter/streamingDecoder.ts (1)
191-207: ⚡ Quick winPreserve MIME/type (and filename) metadata in
loadSourceFilefor non-data:URLs
src/lib/exporter/streamingDecoder.ts(lines 191-208) only populatesmimefordata:URLs; for everything else,mimestays"", so the createdBlob/Fileends up asapplication/octet-stream(andblob:/object URLs will often have an extension-less “UUID” filename). That metadata loss likely won’t break demuxing (web-demuxer is byte/probe driven rather thanfile.type/extension driven), but it’s kinda cursed for observability and could bite future logic—plumb through the fetchedContent-Type(and better filename) fromloadFileAsArrayBufferinstead of reconstructing a genericFilehere.🤖 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/streamingDecoder.ts` around lines 191 - 207, The code in loadSourceFile/streamingDecoder.ts currently only sets mime for data: URLs, leaving mime empty for fetched URLs and causing Blobs/Files to default to application/octet-stream; update the data flow so loadFileAsArrayBuffer (or the function that returns buffer) also returns the fetched Content-Type and an inferred filename, then in the block that sets mime/filename use the returned contentType when videoUrl is not a data: URL (e.g., mime = returnedContentType || mime) and prefer the returned filename (fallback to the existing basename/ext logic); ensure the final new File uses blob.type from that mime (or "application/octet-stream" if still missing).
🤖 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/streamingDecoder.ts`:
- Around line 191-207: The code in loadSourceFile/streamingDecoder.ts currently
only sets mime for data: URLs, leaving mime empty for fetched URLs and causing
Blobs/Files to default to application/octet-stream; update the data flow so
loadFileAsArrayBuffer (or the function that returns buffer) also returns the
fetched Content-Type and an inferred filename, then in the block that sets
mime/filename use the returned contentType when videoUrl is not a data: URL
(e.g., mime = returnedContentType || mime) and prefer the returned filename
(fallback to the existing basename/ext logic); ensure the final new File uses
blob.type from that mime (or "application/octet-stream" if still missing).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36b80ce6-2abf-49f9-bd25-a0404b021bbd
📒 Files selected for processing (2)
src/hooks/useAudioPeaks.tssrc/lib/exporter/streamingDecoder.ts
Change return type to { data, contentType } so the fetched Content-Type
header is available to callers. loadSourceFile uses it to set the Blob
MIME type for https:/blob: URLs instead of always falling back to
application/octet-stream. useAudioPeaks destructures only { data }.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eSourceFile Restore loadSourceFile as a thin router (local vs remote) and extract the IPC and fetch paths into dedicated private methods, each containing the original logic from main. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ile/loadRemoteSourceFile Make loadLocalSourceFile and loadRemoteSourceFile static so they can be called without an instance. loadSourceFile applies withTimeout around each static call. loadFileAsArrayBuffer delegates to the same static methods and extracts ArrayBuffer + contentType from the blob, eliminating the duplicated IPC/fetch branching logic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Biome had reformatted two pre-existing VideoFrame constructor calls from single-line to multi-line style when --write was run on the file. Restore them to match main so the diff only reflects our intentional refactoring. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.
Pull Request Template
Description
Add the waveform of the audio track as background of the Trim Track. The feature is disabled by default and can be enabled via a Timeline Panel.
Motivation
In other tools I was used to cut/trim when seeing the section with the waveform, as I can see the silence.
Type of Change
Screenshots / Video
Video (if applicable):
Screen.Recording.2026-05-23.at.17.47.31.mov
Testing
Checklist
Summary by CodeRabbit
New Features
Improvements