feat: add audio hooks/music timeline support and sync with upstream timeline updates#427
Open
imAaryash wants to merge 13 commits intosiddharthvaddem:mainfrom
Open
feat: add audio hooks/music timeline support and sync with upstream timeline updates#427imAaryash wants to merge 13 commits intosiddharthvaddem:mainfrom
imAaryash wants to merge 13 commits intosiddharthvaddem:mainfrom
Conversation
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 end-to-end background-music and audio-hook support: new Electron audio picker IPC, audio library/types, editor state/persistence, Settings UI for music/hooks with preview, timeline/music/hook items and visibility, playback preview/triggering, and export-time mixing/triggering of hook/background audio. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: a966428c93
ℹ️ 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".
![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: 10
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 (1)
346-403:⚠️ Potential issue | 🟠 MajorAdd
webcamSizePresetback tocurrentProjectSnapshot.This memo depends on
webcamSizePreset, but the field never gets serialized into the snapshot payload. That makes unsaved-change detection lie here: webcam size edits can be missed, and projects with a non-default webcam size can show as dirty immediately after load.🧩 Minimal fix
return createProjectSnapshot(currentProjectMedia, { wallpaper, backgroundMusicPath, backgroundMusicRegions, backgroundMusicVolume, audioHooks, audioHooksVolume, shadowIntensity, showBlur, motionBlurAmount, borderRadius, padding, cropRegion, zoomRegions, trimRegions, speedRegions, annotationRegions, hookRegions, aspectRatio, webcamLayoutPreset, webcamMaskShape, + webcamSizePreset, webcamPosition, exportQuality, exportFormat, gifFrameRate,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/VideoEditor.tsx` around lines 346 - 403, The snapshot payload returned by createProjectSnapshot is missing the webcamSizePreset field, so add webcamSizePreset to the object passed into createProjectSnapshot (the same place where wallpaper, backgroundMusicPath, etc. are listed) so the memo for currentProjectSnapshot correctly serializes webcam size changes; update the object in the return of the createProjectSnapshot call in VideoEditor.tsx (where createProjectSnapshot is invoked) to include webcamSizePreset.src/components/video-editor/SettingsPanel.tsx (1)
1-18:⚠️ Potential issue | 🟡 Minorimports need sorting per biome
pipeline's complaining about import organization. quick fix:
npx biome check --write src/components/video-editor/SettingsPanel.tsxor let your IDE's organize imports do the thing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/SettingsPanel.tsx` around lines 1 - 18, The import statements in SettingsPanel.tsx are not sorted per Biome rules; run the Biome formatter or organize imports so external packages and named imports are alphabetized and grouped (e.g., ensure "Block" import and the "lucide-react" named icons like Bug, Crop, Download, Film, Image, Lock, Music, Palette, Search, Sparkles, Star, Trash2, Unlock, Upload, X are ordered per your project's Biome configuration). Fix by running the suggested command (npx biome check --write src/components/video-editor/SettingsPanel.tsx) or using your IDE's "Organize Imports" to reorder and format the imports.
🧹 Nitpick comments (5)
src/components/video-editor/projectPersistence.test.ts (1)
34-55: Coverage gap: new audio persistence fields are only validated in empty/default state.At Line 35 and Line 55 (mirrored at Line 116 and Line 136), fixtures only use empty regions and disabled hooks. that won’t catch regressions in actual hook/music timeline serialization (especially overlapping hooks, which this PR explicitly enables). nit: cleaner if we add one non-empty fixture case with overlapping
hookRegions+ non-default volumes.Also applies to: 115-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/projectPersistence.test.ts` around lines 34 - 55, Add a second non-default fixture to projectPersistence.test.ts that exercises audio persistence with real data: populate backgroundMusicRegions and hookRegions with overlapping regions, set audioHooks flags (e.g., zoom/trim/speed true as appropriate), and use non-default volumes (backgroundMusicVolume and audioHooksVolume not zero/0.35) so serialization/deserialization tests validate non-empty timelines; update the existing assertions that currently only test empty arrays to also validate the new fixture round-trip (look for the fixture object used in serialize/deserialize tests and the fields backgroundMusicRegions, hookRegions, audioHooks, backgroundMusicVolume, audioHooksVolume and add mirrored cases where the other tests currently use only empty/default state).electron/ipc/handlers.ts (1)
768-776: Use i18n keys here instead of hardcoded dialog strings.Line 768 and Lines 772-776 are hardcoded English, while nearby file dialogs are localized via
mainT(...). This will feel inconsistent in non-English locales.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 768 - 776, Replace hardcoded dialog strings in the file dialog options (the title property and the filter name values under filters) with i18n lookups using mainT(...); specifically update the title ("Select audio") and filter names ("Audio Files", "All Files") to call mainT with appropriate keys (e.g. mainT('dialogs.selectAudio'), mainT('dialogs.audioFiles'), mainT('dialogs.allFiles')) so the dialog is localized while leaving defaultPath: app.getPath("music") unchanged; ensure the chosen i18n keys exist or add them to the locale files and use the same keys where other file dialogs in the codebase use mainT.src/components/video-editor/SettingsPanel.tsx (3)
1374-1374: nit: magic number 80the slice limit works fine but extracting
const MAX_VISIBLE_TRACKS = 80would make the intent clearer. same applies to line 1548. totally optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/SettingsPanel.tsx` at line 1374, Extract the magic number 80 into a clearly named constant (e.g., const MAX_VISIBLE_TRACKS = 80) and use it instead of the literal in the filteredMusicLibrary.slice call(s) (replace filteredMusicLibrary.slice(0, 80) and the similar occurrence around line with the same symbol with filteredMusicLibrary.slice(0, MAX_VISIBLE_TRACKS)); declare the constant near the top of the SettingsPanel component or file so intent is clear and reuse is straightforward.
709-746: minor: potential stale setState on unmountthere's a small race window where if the component unmounts while
await getAssetPathis in flight,setPreviewingMusicTrackIdcould fire on an unmounted component. same pattern in the hook preview handler.not a huge deal in practice (react will just warn), but you could add a mounted ref check if you want to be squeaky clean:
🛡️ optional guard pattern
const mountedRef = useRef(true); useEffect(() => () => { mountedRef.current = false; }, []); // then in handler: if (!mountedRef.current) return; setPreviewingMusicTrackId(trackId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/SettingsPanel.tsx` around lines 709 - 746, The handler handleMusicLibraryPreview can call setPreviewingMusicTrackId after an awaited getAssetPath while the component may have unmounted; add a mountedRef (useRef<boolean>) set to true and flipped to false in a useEffect cleanup, then check mountedRef.current before calling setPreviewingMusicTrackId (and before any other setState or toast calls) inside handleMusicLibraryPreview and the similar preview handler to avoid updating state on an unmounted component.
726-729: hardcoded error stringsthese toast messages are the only strings not going through
t(). kinda inconsistent with the rest of the i18n setup.♻️ use translation keys
- toast.error("Unable to preview this track"); + toast.error(t("audio.previewError"));and same pattern for the hook preview error messages.
Also applies to: 741-744, 775-778, 790-793
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/SettingsPanel.tsx` around lines 726 - 729, Replace hardcoded toast messages in SettingsPanel.tsx with i18n translation keys using the existing t() function: where audio.play().catch(() => { stopMusicPreview(); toast.error("Unable to preview this track"); }) (and the similar hook preview error handlers) appear, call toast.error(t('settings.preview_unable_to_play')) or similarly named keys instead of raw strings; ensure t is imported/available in the component (useTranslation hook) and add corresponding keys to your translation files for the new entries (e.g., settings.preview_unable_to_play, settings.preview_hook_error) so all preview error toasts use t() consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/preload.ts`:
- Around line 70-72: The open-audio-file-picker IPC call is exposed without
validating the caller; update the main process IPC handler for
"open-audio-file-picker" to verify event.sender before proceeding (similar to
how "save-project-file" uses isTrustedProjectPath) — check the event.sender or
its webContents/id against your trusted window list or call
isTrustedProjectPath-equivalent and return/reject if not trusted, so only
known/trusted renderer windows can trigger the handler (refer to the
"open-audio-file-picker" channel, the ipcRenderer.invoke call in preload.ts, and
the existing "save-project-file"/isTrustedProjectPath logic to mirror the
validation approach).
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 1396-1583: The pipeline flagged formatting drift in the
SettingsPanel component JSX (SettingsPanel.tsx around the music/hooks JSX blocks
and handlers like handleHookLibraryPreview, onMusicTrackSelect,
onHookTimelineAdd), so run the project's formatter on that file and commit the
changes: execute npx biome check --write
src/components/video-editor/SettingsPanel.tsx (or the equivalent biome format
command), review the resulting edits to the JSX/indentation, stage and commit
them, and push so CI no longer fails.
- Around line 211-213: The props selectedZoomInDuration, selectedZoomOutDuration
and onZoomDurationChange are dead — remove them from the SettingsPanel props
interface and any type declarations in the SettingsPanel component (search for
SettingsPanel and its props/interface) and also remove them from where
SettingsPanel is instantiated (VideoEditor) so you no longer pass these unused
props; ensure Typescript types and any prop spreading remain consistent after
deletion.
In `@src/components/video-editor/timeline/Item.tsx`:
- Around line 166-179: The JSX branches for isMusic and isHook always render the
literal strings "Music" and "Hook", hiding the actual clip label passed from
TimelineEditor; update the isMusic and isHook branches in Item.tsx to render the
incoming label variable (e.g., label or clip.label used elsewhere in this
component) instead of the hardcoded text while keeping the Music2 and Sparkles
icons and the existing classes so stacked SFX/hook clips show their real labels.
In `@src/components/video-editor/timeline/TimelineEditor.tsx`:
- Around line 791-805: The hint string for the hook row is hardcoded; replace
hint="Use hook sounds from Audio settings" with a localized call (e.g.
hint={t('timeline.hookRow.hint')} or your project’s i18n key) so the Row with
id={HOOK_ROW_ID} uses translation like the neighboring rows; update the
translation keys/file to include the new string and keep usage consistent with
other rows that call t(...), and ensure hookItems, selectedHookId, and
onSelectHook behavior is unchanged.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 190-196: hookSoundLayers is only component state and must be
persisted to the project's audio config so projects keep their layer assignments
across load/save; update initialization and setters to read/write
project.audioConfig.hookSoundLayers. Specifically: when mounting/init (where
hookSoundLayers is created from DEFAULT_HOOK_SOUND_ASSETS) load from
project.audioConfig.hookSoundLayers if present (use hookSoundLayers and
setHookSoundLayers), and when users change layers call setHookSoundLayers and
also update project.audioConfig.hookSoundLayers so the value is included in
saveProject; also ensure loadProject populates the component state from
project.audioConfig.hookSoundLayers and that preview/export routines read from
project.audioConfig.hookSoundLayers (or from the authoritative hookSoundLayers
state) so reopen/preview/export produce the same audio.
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 1186-1194: The useEffect for resetting music lists
backgroundMusicPath in its deps but never references it, triggering
exhaustive-deps; update the effect body (the useEffect that accesses
backgroundMusicRef and calls backgroundMusic.pause()/backgroundMusic.currentTime
= 0) to also reference backgroundMusicPath in the guard (e.g., check if
backgroundMusicPath is falsy alongside backgroundMusic) so the dependency is
intentional and lint-clean while preserving the reset behavior.
- Around line 1212-1222: The code is seeking the looped audio directly to the
video's currentTime which can exceed the audio duration and cause clamp/loop
jitter; update both places that set backgroundMusic.currentTime to compute a
wrappedTime = (backgroundMusic.duration > 0 &&
isFinite(backgroundMusic.duration)) ? (currentTime % backgroundMusic.duration) :
currentTime, guard against NaN/zero duration, and assign
backgroundMusic.currentTime = wrappedTime instead of the raw currentTime (apply
the same modulo logic in the pause branch and the playing-sync branch for
backgroundMusic).
In `@src/lib/audioLibrary.json`:
- Around line 1-365: The audioLibrary.json manifest is failing biome formatting
(biome check) starting at the top of the file; fix by running the Biome
formatter (or the repo's prep script that regenerates the manifest) against
audioLibrary.json to apply the expected formatting and JSON normalization
(ensure the top-level array and objects remain valid JSON—no trailing commas or
extra characters), then commit the formatted file so biome check passes.
In `@src/lib/exporter/audioEncoder.ts`:
- Around line 312-364: playHookSound currently clamps audioHooksVolume above
zero (uses Math.max(0.01, ...) and sets initial gain to 0.0001), causing a
slider value of 0 to still produce sound; change both the file-backed branch and
synth branch to respect true zero: for file-backed nodes set gain.gain.value =
Math.min(1, Math.max(0, audioHooksVolume)); for the oscillator path compute peak
= Math.min(0.22, Math.max(0, audioHooksVolume * 0.22)) and if peak === 0 simply
skip creating/starting the oscillator (or create it but set gain to 0 and do not
schedule ramps) to avoid exponential/linear ramps from a non-zero floor; update
any other occurrences in playHookSound (and the similar code block at the other
range noted) that use Math.max(..., 0.01) or initial 0.0001 to use 0 so mute =
truly silent (refer to playHookSound, audioHooksVolume, audioContext,
destinationNode, HOOK_DURATIONS, HOOK_FREQUENCIES, disconnectHookNode,
activeHookNodes).
---
Outside diff comments:
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 1-18: The import statements in SettingsPanel.tsx are not sorted
per Biome rules; run the Biome formatter or organize imports so external
packages and named imports are alphabetized and grouped (e.g., ensure "Block"
import and the "lucide-react" named icons like Bug, Crop, Download, Film, Image,
Lock, Music, Palette, Search, Sparkles, Star, Trash2, Unlock, Upload, X are
ordered per your project's Biome configuration). Fix by running the suggested
command (npx biome check --write src/components/video-editor/SettingsPanel.tsx)
or using your IDE's "Organize Imports" to reorder and format the imports.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 346-403: The snapshot payload returned by createProjectSnapshot is
missing the webcamSizePreset field, so add webcamSizePreset to the object passed
into createProjectSnapshot (the same place where wallpaper, backgroundMusicPath,
etc. are listed) so the memo for currentProjectSnapshot correctly serializes
webcam size changes; update the object in the return of the
createProjectSnapshot call in VideoEditor.tsx (where createProjectSnapshot is
invoked) to include webcamSizePreset.
---
Nitpick comments:
In `@electron/ipc/handlers.ts`:
- Around line 768-776: Replace hardcoded dialog strings in the file dialog
options (the title property and the filter name values under filters) with i18n
lookups using mainT(...); specifically update the title ("Select audio") and
filter names ("Audio Files", "All Files") to call mainT with appropriate keys
(e.g. mainT('dialogs.selectAudio'), mainT('dialogs.audioFiles'),
mainT('dialogs.allFiles')) so the dialog is localized while leaving defaultPath:
app.getPath("music") unchanged; ensure the chosen i18n keys exist or add them to
the locale files and use the same keys where other file dialogs in the codebase
use mainT.
In `@src/components/video-editor/projectPersistence.test.ts`:
- Around line 34-55: Add a second non-default fixture to
projectPersistence.test.ts that exercises audio persistence with real data:
populate backgroundMusicRegions and hookRegions with overlapping regions, set
audioHooks flags (e.g., zoom/trim/speed true as appropriate), and use
non-default volumes (backgroundMusicVolume and audioHooksVolume not zero/0.35)
so serialization/deserialization tests validate non-empty timelines; update the
existing assertions that currently only test empty arrays to also validate the
new fixture round-trip (look for the fixture object used in
serialize/deserialize tests and the fields backgroundMusicRegions, hookRegions,
audioHooks, backgroundMusicVolume, audioHooksVolume and add mirrored cases where
the other tests currently use only empty/default state).
In `@src/components/video-editor/SettingsPanel.tsx`:
- Line 1374: Extract the magic number 80 into a clearly named constant (e.g.,
const MAX_VISIBLE_TRACKS = 80) and use it instead of the literal in the
filteredMusicLibrary.slice call(s) (replace filteredMusicLibrary.slice(0, 80)
and the similar occurrence around line with the same symbol with
filteredMusicLibrary.slice(0, MAX_VISIBLE_TRACKS)); declare the constant near
the top of the SettingsPanel component or file so intent is clear and reuse is
straightforward.
- Around line 709-746: The handler handleMusicLibraryPreview can call
setPreviewingMusicTrackId after an awaited getAssetPath while the component may
have unmounted; add a mountedRef (useRef<boolean>) set to true and flipped to
false in a useEffect cleanup, then check mountedRef.current before calling
setPreviewingMusicTrackId (and before any other setState or toast calls) inside
handleMusicLibraryPreview and the similar preview handler to avoid updating
state on an unmounted component.
- Around line 726-729: Replace hardcoded toast messages in SettingsPanel.tsx
with i18n translation keys using the existing t() function: where
audio.play().catch(() => { stopMusicPreview(); toast.error("Unable to preview
this track"); }) (and the similar hook preview error handlers) appear, call
toast.error(t('settings.preview_unable_to_play')) or similarly named keys
instead of raw strings; ensure t is imported/available in the component
(useTranslation hook) and add corresponding keys to your translation files for
the new entries (e.g., settings.preview_unable_to_play,
settings.preview_hook_error) so all preview error toasts use t() consistently.
🪄 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: 11160942-dcee-4322-a6a5-34e7d517d266
⛔ Files ignored due to path filters (38)
public/audio/hooks/annotation.mp3is excluded by!**/*.mp3public/audio/hooks/blur.wavis excluded by!**/*.wavpublic/audio/hooks/speed.mp3is excluded by!**/*.mp3public/audio/hooks/trim.wavis excluded by!**/*.wavpublic/audio/hooks/zoom.wavis excluded by!**/*.wavpublic/audio/library/hooks/openscreen-sfx-alarm-001.mp3is excluded by!**/*.mp3public/audio/library/hooks/openscreen-sfx-alert.mp3is excluded by!**/*.mp3public/audio/library/hooks/openscreen-sfx-beep-001.wavis excluded by!**/*.wavpublic/audio/library/hooks/openscreen-sfx-camera-001.wavis excluded by!**/*.wavpublic/audio/library/hooks/openscreen-sfx-camera-004.mp3is excluded by!**/*.mp3public/audio/library/hooks/openscreen-sfx-click-001.wavis excluded by!**/*.wavpublic/audio/library/hooks/openscreen-sfx-click-002.mp3is excluded by!**/*.mp3public/audio/library/hooks/openscreen-sfx-clock-001.mp3is excluded by!**/*.mp3public/audio/library/hooks/openscreen-sfx-fade-out-005.wavis excluded by!**/*.wavpublic/audio/library/hooks/openscreen-sfx-glitch-001.mp3is excluded by!**/*.mp3public/audio/library/hooks/openscreen-sfx-glitch-002.mp3is excluded by!**/*.mp3public/audio/library/hooks/openscreen-sfx-misc-002.mp3is excluded by!**/*.mp3public/audio/library/hooks/openscreen-sfx-misc-014.wavis excluded by!**/*.wavpublic/audio/library/hooks/openscreen-sfx-money-002.mp3is excluded by!**/*.mp3public/audio/library/hooks/openscreen-sfx-money-003.mp3is excluded by!**/*.mp3public/audio/library/hooks/openscreen-sfx-notification-001.mp3is excluded by!**/*.mp3public/audio/library/hooks/openscreen-sfx-notification-003.mp3is excluded by!**/*.mp3public/audio/library/hooks/openscreen-sfx-notification-004.mp3is excluded by!**/*.mp3public/audio/library/hooks/openscreen-sfx-reveal-013.mp3is excluded by!**/*.mp3public/audio/library/hooks/openscreen-sfx-typing-001.mp3is excluded by!**/*.mp3public/audio/library/hooks/openscreen-sfx-typing-007.mp3is excluded by!**/*.mp3public/audio/library/hooks/openscreen-sfx-whoosh-001.wavis excluded by!**/*.wavpublic/audio/library/hooks/openscreen-sfx-whoosh-004.mp3is excluded by!**/*.mp3public/audio/library/hooks/openscreen-sfx-whoosh-005.mp3is excluded by!**/*.mp3public/audio/library/hooks/openscreen-sfx-whoosh-006.mp3is excluded by!**/*.mp3public/audio/library/music/openscreen-ambient-001.mp3is excluded by!**/*.mp3public/audio/library/music/openscreen-ambient-002.mp3is excluded by!**/*.mp3public/audio/library/music/openscreen-corporate-001.mp3is excluded by!**/*.mp3public/audio/library/music/openscreen-corporate-002.mp3is excluded by!**/*.mp3public/audio/library/music/openscreen-lofi-001.mp3is excluded by!**/*.mp3public/audio/library/music/openscreen-lofi-002.mp3is excluded by!**/*.mp3public/audio/library/music/openscreen-track-001.mp3is excluded by!**/*.mp3public/audio/library/music/openscreen-upbeat-001.mp3is excluded by!**/*.mp3
📒 Files selected for processing (25)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/preload.tsscripts/prepare_audio_library.pysrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/projectPersistence.test.tssrc/components/video-editor/projectPersistence.tssrc/components/video-editor/timeline/Item.tsxsrc/components/video-editor/timeline/ItemGlass.module.csssrc/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/types.tssrc/hooks/useEditorHistory.tssrc/i18n/locales/en/settings.jsonsrc/i18n/locales/en/timeline.jsonsrc/i18n/locales/es/settings.jsonsrc/i18n/locales/fr/settings.jsonsrc/i18n/locales/ko-KR/settings.jsonsrc/i18n/locales/tr/settings.jsonsrc/i18n/locales/zh-CN/settings.jsonsrc/lib/audioLibrary.jsonsrc/lib/audioLibrary.tssrc/lib/exporter/audioEncoder.tssrc/lib/exporter/videoExporter.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/video-editor/timeline/TimelineEditor.tsx (1)
822-827:⚠️ Potential issue | 🟡 MinorHook-row hint is still hardcoded.
Line 826 is still bypassing
t(...), so this row stays English-only while neighboring rows are localized.nit: cleaner i18n wiring
- <Row - id={HOOK_ROW_ID} - isEmpty={hookItems.length === 0} - hint="Use hook sounds from Audio settings" - > + <Row + id={HOOK_ROW_ID} + isEmpty={hookItems.length === 0} + hint={t("hints.pressHook")} + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/timeline/TimelineEditor.tsx` around lines 822 - 827, The Hook row currently passes a hardcoded English hint string to the Row component (see Row with id=HOOK_ROW_ID and hint="Use hook sounds from Audio settings"), so localizations are skipped; replace the literal with a translated string using the project's i18n function (e.g., call t('timeline.hookHint') or the existing translation hook used in this file) and supply that result to the hint prop (keep the check using hookItems.length and rowVisibility.hook unchanged).
🧹 Nitpick comments (1)
src/components/video-editor/timeline/TimelineEditor.tsx (1)
950-955: Harden localStorage hydration for row visibility.Current spread trusts JSON shape directly. If persisted data is malformed (or manually edited), checkbox state can get kinda cursed. Worth sanitizing each key to booleans before
setRowVisibility.safe parse/sanitize pattern
- const parsed = JSON.parse(raw) as Partial<TimelineRowVisibility>; - setRowVisibility({ - ...DEFAULT_ROW_VISIBILITY, - ...parsed, - music: true, - }); + const parsed = JSON.parse(raw) as Partial<Record<TimelineRowKey, unknown>>; + setRowVisibility({ + zoom: parsed.zoom === true ? true : DEFAULT_ROW_VISIBILITY.zoom, + trim: parsed.trim === true ? true : DEFAULT_ROW_VISIBILITY.trim, + annotation: parsed.annotation === true ? true : DEFAULT_ROW_VISIBILITY.annotation, + blur: parsed.blur === true ? true : DEFAULT_ROW_VISIBILITY.blur, + speed: parsed.speed === true ? true : DEFAULT_ROW_VISIBILITY.speed, + hook: parsed.hook === true ? true : DEFAULT_ROW_VISIBILITY.hook, + music: true, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/timeline/TimelineEditor.tsx` around lines 950 - 955, The localStorage hydration in TimelineEditor trusts the parsed object shape (parsed) and can set invalid checkbox states; update the hydration before calling setRowVisibility to (1) guard JSON.parse with try/catch and ignore/clear malformed raw, (2) build a sanitized object by iterating the keys of DEFAULT_ROW_VISIBILITY (or TimelineRowVisibility keys) and coercing each value to a boolean (use parsed[key] === true), falling back to DEFAULT_ROW_VISIBILITY[key] when missing/invalid, and (3) still enforce music: true; then call setRowVisibility with the merged sanitized object instead of directly spreading parsed.
🤖 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/components/video-editor/timeline/TimelineEditor.tsx`:
- Around line 969-977: Replace the hardcoded English labels in the
TimelineEditor with i18n keys: use the component's translation helper (e.g., t
or useTranslation) to change the array rowVisibilityOptions to return { key:
"zoom", label: t("timeline.row.zoom") } etc. Do the same for the other hardcoded
UI strings referenced in this component (the fallback hook label and the
menu/track labels flagged elsewhere in the TimelineEditor) — add new translation
keys for Zoom/Trim/Annotation/Blur/Speed/Hook/Music in your locale files and
call t(...) where those raw strings are currently used so all locales can
translate them.
---
Duplicate comments:
In `@src/components/video-editor/timeline/TimelineEditor.tsx`:
- Around line 822-827: The Hook row currently passes a hardcoded English hint
string to the Row component (see Row with id=HOOK_ROW_ID and hint="Use hook
sounds from Audio settings"), so localizations are skipped; replace the literal
with a translated string using the project's i18n function (e.g., call
t('timeline.hookHint') or the existing translation hook used in this file) and
supply that result to the hint prop (keep the check using hookItems.length and
rowVisibility.hook unchanged).
---
Nitpick comments:
In `@src/components/video-editor/timeline/TimelineEditor.tsx`:
- Around line 950-955: The localStorage hydration in TimelineEditor trusts the
parsed object shape (parsed) and can set invalid checkbox states; update the
hydration before calling setRowVisibility to (1) guard JSON.parse with try/catch
and ignore/clear malformed raw, (2) build a sanitized object by iterating the
keys of DEFAULT_ROW_VISIBILITY (or TimelineRowVisibility keys) and coercing each
value to a boolean (use parsed[key] === true), falling back to
DEFAULT_ROW_VISIBILITY[key] when missing/invalid, and (3) still enforce music:
true; then call setRowVisibility with the merged sanitized object instead of
directly spreading parsed.
🪄 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: 413ccc24-f1f9-429f-80fc-55054d1260fc
📒 Files selected for processing (1)
src/components/video-editor/timeline/TimelineEditor.tsx
Reason:\n- background music drifted during trim jumps because only source media sought\n- hook audio remained audible at 0 volume due to a non-zero gain clamp\n- browser exporter tests failed with ERR_INVALID_URL for fixture paths in this runtime\n\nWhat changed:\n- keep background media in timeline sync when source skips trims\n- allow true zero gain for hook export mix\n- preserve bundled /audio URLs in editor track resolution\n- normalize relative/root URLs before fetch in streaming decoder\n- make browser-only exporter tests environment-safe under Node runtimes
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/lib/exporter/audioEncoder.ts (1)
481-490:⚠️ Potential issue | 🟠 MajorDropped hook triggers when multiple events are crossed in one frame
lowkey risky: on Line 487,
events.some(...)fires at most once per tick per hook type. If two events land in the same frame window (easy with dense edits or higher playbackRate), one gets dropped.💡 Minimal fix
- const events = hookEventTimes[hook] ?? []; - if (events.some((timeMs) => crossed(timeMs))) { - playHookSound(hook); - } + const events = hookEventTimes[hook] ?? []; + const crossedCount = events.reduce( + (count, timeMs) => count + (crossed(timeMs) ? 1 : 0), + 0, + ); + for (let i = 0; i < crossedCount; i += 1) { + playHookSound(hook); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/audioEncoder.ts` around lines 481 - 490, The current logic in the hasAnyHook/hookEventTimes block uses events.some((timeMs) => crossed(timeMs)) which only triggers playHookSound(hook) once per hook type per frame, dropping additional events that also crossed; change the logic to iterate all events for each hook (using forEach or a filter+forEach) and call playHookSound(hook) for every timeMs where crossed(timeMs) is true so multiple events in the same frame are all fired; look for HOOK_FREQUENCIES, AudioHookType, hookEventTimes, crossed, and playHookSound to locate and update the loop accordingly.
🧹 Nitpick comments (2)
src/lib/exporter/audioEncoder.ts (1)
35-38: Doc comment is stale relative to runtime mode selectionnit: cleaner if the
process()docblock is updated — Line 35-38 still says timeline render is speed-only, but Line 66 also routes background/hook cases through that path now.Also applies to: 65-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/audioEncoder.ts` around lines 35 - 38, Update the stale docblock above the process() function to reflect current runtime routing: instead of saying the timeline render pipeline is used only for speed regions, state that audio export uses the fast WebCodecs trim-only pipeline when no special processing is needed, and uses the pitch-preserving rendered timeline pipeline when speed regions are present or when background/hook cases require rendered timeline processing; mention both conditions explicitly and reference the process() function name so readers know which function's behavior is described.src/lib/exporter/videoExporter.browser.test.ts (1)
9-36: Refactor suggestion: extract shared electronAPI fixture mock helper.These setup/teardown blocks are basically copy-pasted between GIF and Video browser tests. kinda cursed to maintain long-term; one helper would keep behavior aligned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/videoExporter.browser.test.ts` around lines 9 - 36, Extract the repeated electronAPI mock into a shared test helper: create a helper function (e.g., createElectronAPIMock or withMockedElectronAPI) that accepts parameters like readBinaryFile implementation or fixture path (sampleVideoPath) and returns a teardown restore function or the originalElectronAPI; replace the current beforeAll/afterAll pair that mutates windowWithElectron.electronAPI and originalElectronAPI in videoExporter.browser.test.ts (and the GIF test) with a call to that helper inside beforeAll to install the mock and call the returned restore in afterAll (keep the same readBinaryFile behavior that returns { success, data, path, message } and still uses readFile). Ensure the helper references windowWithElectron and the readBinaryFile signature so both tests can reuse it.
🤖 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/audioEncoder.ts`:
- Around line 464-466: The current crossed(timeMs) predicate (used with
previousTimeMs and sourceMedia.currentTime) only checks numeric crossing and can
fire for times that are inside trimmed/ skipped timeline ranges because
trim/skipping logic runs later; modify crossed (or wrap its usage) to first
consult the trim/skip test so it returns false for any timeMs that falls within
a trimmed-out region (e.g., incorporate the existing trim-check predicate used
around lines ~500 or call isTrimmed(timeMs) inside crossed), and apply the same
change to the other similar checks referenced (the blocks around 493-499 and
500-518) so no hook/clip start is emitted for times inside trimmed ranges.
Ensure you update references to crossed wherever it’s used so the early
trim-skip guard prevents firing before the jump.
In `@src/lib/exporter/gifExporter.browser.test.ts`:
- Around line 1-2: The tests import Node-only modules (node:fs/promises,
node:path) which break Vitest browser mode; replace their use in
src/lib/exporter/gifExporter.browser.test.ts and
src/lib/exporter/videoExporter.browser.test.ts by loading fixture assets via the
browser-friendly URL/fetch pattern: construct the fixture URL with new
URL('./fixtures/yourfile.bin', import.meta.url) or use a hardcoded dev-fixture
URL, then fetch() that URL and call response.arrayBuffer()/blob() instead of
readFile, and remove any path.join/path.resolve usage; update any helper
utilities in these tests that call readFile or path to accept ArrayBuffer/Blob
or convert fetched data as needed. Ensure tests no longer import node:fs or
node:path.
---
Duplicate comments:
In `@src/lib/exporter/audioEncoder.ts`:
- Around line 481-490: The current logic in the hasAnyHook/hookEventTimes block
uses events.some((timeMs) => crossed(timeMs)) which only triggers
playHookSound(hook) once per hook type per frame, dropping additional events
that also crossed; change the logic to iterate all events for each hook (using
forEach or a filter+forEach) and call playHookSound(hook) for every timeMs where
crossed(timeMs) is true so multiple events in the same frame are all fired; look
for HOOK_FREQUENCIES, AudioHookType, hookEventTimes, crossed, and playHookSound
to locate and update the loop accordingly.
---
Nitpick comments:
In `@src/lib/exporter/audioEncoder.ts`:
- Around line 35-38: Update the stale docblock above the process() function to
reflect current runtime routing: instead of saying the timeline render pipeline
is used only for speed regions, state that audio export uses the fast WebCodecs
trim-only pipeline when no special processing is needed, and uses the
pitch-preserving rendered timeline pipeline when speed regions are present or
when background/hook cases require rendered timeline processing; mention both
conditions explicitly and reference the process() function name so readers know
which function's behavior is described.
In `@src/lib/exporter/videoExporter.browser.test.ts`:
- Around line 9-36: Extract the repeated electronAPI mock into a shared test
helper: create a helper function (e.g., createElectronAPIMock or
withMockedElectronAPI) that accepts parameters like readBinaryFile
implementation or fixture path (sampleVideoPath) and returns a teardown restore
function or the originalElectronAPI; replace the current beforeAll/afterAll pair
that mutates windowWithElectron.electronAPI and originalElectronAPI in
videoExporter.browser.test.ts (and the GIF test) with a call to that helper
inside beforeAll to install the mock and call the returned restore in afterAll
(keep the same readBinaryFile behavior that returns { success, data, path,
message } and still uses readFile). Ensure the helper references
windowWithElectron and the readBinaryFile signature so both tests can reuse it.
🪄 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: b079d26b-86a0-4faa-b20c-e46358819edb
📒 Files selected for processing (5)
src/components/video-editor/VideoEditor.tsxsrc/lib/exporter/audioEncoder.tssrc/lib/exporter/gifExporter.browser.test.tssrc/lib/exporter/streamingDecoder.tssrc/lib/exporter/videoExporter.browser.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/video-editor/VideoEditor.tsx
Collaborator
Author
|
@siddharthvaddem can you review this PR? from my side all looks good just make sure merge my PR first :) don't wanna deal with more conflicts |
Owner
Sure thing. I'll make sure to prioritise getting this in since it's a larger change. But please give me some time to get back on this. |
theabhipareek
suggested changes
Apr 16, 2026
There was a problem hiding this comment.
Review Summary: Audio Hooks & Music Timeline Support
Thanks for this comprehensive audio feature! The UX improvements are solid, but there are 2 critical bugs + linting issues that need fixing before merge.
🔴 Critical Blockers
1. Missing webcamSizePreset in Project Snapshot
- File:
src/components/video-editor/VideoEditor.tsx(lines 346-403) - Issue: The
currentProjectSnapshotmemo excludeswebcamSizePreset, so unsaved-change detection breaks for webcam size edits—projects with non-default sizes show as dirty after reload. - Fix: Add
webcamSizePresetto thecreateProjectSnapshot()object alongside other fields likewallpaper,backgroundMusicPath, etc.
2. Multiple Hook Events Dropped When Firing Simultaneously
- File:
src/lib/exporter/audioEncoder.ts(lines 481-490) - Issue: Using
events.some()only firesplayHookSound()once per frame. If 2+ hook events land in the same frame (common with dense edits), the extras are silently dropped. - Current:
if (events.some((timeMs) => crossed(timeMs))) { playHookSound(hook); } - Fix: Iterate all events:
events.forEach((timeMs) => { if (crossed(timeMs)) playHookSound(hook); })
🟡 Linting + i18n Issues
3. Biome Import Formatting Failure
- File:
src/components/video-editor/SettingsPanel.tsx(lines 1-18) - Fix:
npx biome check --write src/components/video-editor/SettingsPanel.tsx
4. Hardcoded English Strings (Not Localized)
Missing t(...) / mainT(...) calls:
electron/ipc/handlers.ts(line 768): Dialog title "Select audio" + filter namesSettingsPanel.tsx(lines 726-729, 741-744): Toast error messages ("Unable to preview this track")TimelineEditor.tsx(line 826): Hook row hint "Use hook sounds from Audio settings"
All should use i18n keys for consistency with the rest of the UI.
🟢 Minor Improvements (Optional)
- Line 709-746 (SettingsPanel): Add
mountedRefguard to prevent setState on unmounted component during async preview loading - Line 950-955 (TimelineEditor): Sanitize localStorage hydration to coerce each boolean key instead of trusting JSON shape
- Line 1374 (SettingsPanel): Extract magic number
80→const MAX_VISIBLE_TRACKS = 80 - Line 35-38 (audioEncoder.ts): Update stale docblock—speed regions aren't the only case anymore (background/hook also use timeline render)
- projectPersistence.test.ts (lines 34-55): Add test fixture with overlapping
hookRegions+ non-default volumes
✅ Strengths
- Clear feature scope, good UX motivation
- Solid audio library (30+ assets)
- Proper Electron IPC security validation
- i18n structure in place (just needs completeness)
- Fix
webcamSizePresetserialization - Fix hook event loop (iterate all crossings)
- Run Biome on SettingsPanel
- Replace hardcoded strings with i18n
- Consider: unmount guard, storage sanitization, test coverage
7 tasks
Owner
siddharthvaddem
left a comment
siddharthvaddem
left a comment
There was a problem hiding this comment.
can you take another look - the audio stuff seems to work fine, but I want to make sure nothing else is affected.
|
ahh shit, I was tweaking audio hook sync with zoom timing and kinda ended up committing everything in the same branch, my bad |
|
@n0taaryash I have tested your feature. I want to report you that the sample audio files are not included in the build artifact. This results in a "unable to preview this track". Consider adding them to the electron builder file. P.s. I'm still testing, I have noticed that the trim functionality also cuts the music, resulting in the music not being continuous. P.s. It would be nice to have some options on the audio tracks. Like fade in and fade out P.s. When exporting the file to video, the audio is not included |
Collaborator
Author
|
hey @LucaFontanot thanks for the reviews, ill fix these right away |
…ettings UX - add top-right export action with dedicated ExportSettingsPopup - move support actions (GitHub, bug report, Discord) into header controls - add searchable audio libraries with Fuse.js and smooth list scrolling - extract audio preview/hook logic into useAudioPreview hook - add background music fade-in/fade-out support and persist/export fields - include packaged public audio assets in electron-builder
theabhipareek
approved these changes
Apr 17, 2026
Owner
|
@imAaryash fix the final merge conflicts and should be good to merge. I'll cut release here, want to do extensive testing before making a release |
Owner
|
@imAaryash gentle bump |
Collaborator
Author
yoo, i thought we decided to push this feature in the next release, so I kept it idle. I'll fix the conflicts |
|
Is this pull request still active? Is help needed? |
Owner
|
@LucaFontanot it is still active - the author is currently occupied and will get to it when he can. |
Collaborator
Author
|
hey @LucaFontanot im just stuck with my exams, this pr is still active :) |
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
5 participants
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 improves the video editor audio workflow and timeline usability by adding:
Motivation
As the editor gained more timeline lanes and audio options, the workflow became harder to navigate:
This PR solves those pain points by making audio selection faster, timeline editing more precise, and lane management more user-controlled.
Type of Change
Related Issue(s)
#397
Screenshots / Video
Testing
Reviewers can verify with these steps:
Checklist
Summary by CodeRabbit
New Features
UI Improvements
Localization
Export