Add/remove/solo tracks - #41
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR replaces fixed timeline tracks with persisted dynamic video/audio lanes, adds lane creation, removal, enable, and solo controls, and extends linked audio from stereo pairs to discrete multichannel stems across playback, metering, display, waveform rendering, and export. ChangesDynamic tracks and multichannel audio
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Media
participant Timeline
participant AudioGraph
participant Export
Media->>Timeline: add video and linked channel clips
Timeline->>AudioGraph: assign clips to dynamic audio buses
AudioGraph->>AudioGraph: route discrete channels
Export->>AudioGraph: mix routed channels offline
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
Accidentally, I include also another commit, this one is big. As AAC audio can contain more than one channel, all of them are linked and displayed on a timeline, not just a stereo pair. If there is not enough audio tracks on the timeline, they are added to incorporate all channels. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app.js (1)
2768-2799: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDelete buses for removed tracks in
syncAudioGraphTracks.Buses are created for new ids but never removed for deleted ones, so
audio.trackBusaccumulates stale, disconnected nodes. Because a re-added track reuses the same id (nextTrackIdreturnsmax+1), the stale bus is reused and only reconnected tomasterinside theif (audio.meter)branch — so if the meter worklet failed to install, a re-added track's bus would stay silent.♻️ Suggested cleanup
const ids = audioTrackIds(); for (const id of ids) { if (!audio.trackBus[id]) { const g = audio.ctx.createGain(); audio.trackBus[id] = g; g.connect(audio.master); } } + for (const id of Object.keys(audio.trackBus)) { + if (!ids.includes(id)) { + try { audio.trackBus[id].disconnect(); } catch { } + delete audio.trackBus[id]; + } + }🤖 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 `@app.js` around lines 2768 - 2799, Update syncAudioGraphTracks to remove entries from audio.trackBus whose IDs are absent from the current ids list, disconnecting each removed bus before deletion. Perform this cleanup independently of the audio.meter branch, and ensure buses for current IDs are connected to audio.master so re-added tracks remain audible even when meter installation failed.
🤖 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 `@app.js`:
- Around line 169-181: Update ensureTracksCoverClips so each newly discovered
track name is marked as present immediately after adding it, before processing
subsequent clips. Use the existing TRACK_IDS set (or an equivalent per-call seen
set) to prevent multiple makeTrack calls for the same missing lane, while
preserving the existing sorting and height application behavior.
- Around line 1671-1677: Update buildTrackDOM() so the track ID interpolation in
the generated innerHTML uses escapeHtml(t.id), matching the existing escaping
used for clip labels. Leave the surrounding track controls and other
interpolation unchanged.
---
Nitpick comments:
In `@app.js`:
- Around line 2768-2799: Update syncAudioGraphTracks to remove entries from
audio.trackBus whose IDs are absent from the current ids list, disconnecting
each removed bus before deletion. Perform this cleanup independently of the
audio.meter branch, and ensure buses for current IDs are connected to
audio.master so re-added tracks remain audible even when meter installation
failed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2afad12f-9da9-477a-a4d6-5ba8a93f5b5a
📒 Files selected for processing (5)
CLAUDE.mdREADME.mdapp.jsindex.htmlstyle.css
|
Apologies, not the whole commit was included. |
|
The feature has been updated. Now, dropping a video file with more audio channels than is available on the timeline, adds a necessary number of audio tracks. |
|
I am a little bit lost, not sure now if I mentioned anywhere the major upgrade how AAC audio tracks are handled. Let's say we have a multichannel audio - 3.0 layout. Before, an audio track was treated as a stereo pair, now all the channels are extracted into 3 mono audio tracks. |
|
The latest commit is rebased on main, and includes improved audio track handling. The main feature is still the possisbility to add/remove (empty) tracks and to solo a track with a single click. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
app.js (3)
1547-1573: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftTwo async paths build the same stem set.
finish()(viaattachLinkedAudioChannels) andreconcileAudioChannels(c)at line 1572 both probe the channel count, both grow A-tracks, and both create/remove linked stems for the samelinkGroup. They converge becausereconcileAudioChannelsre-readsproject.clipsafter its await, but the outcome (and whether the user sees one toast or two) depends on promise resolution order. Consider makingfinish()the single owner and callingreconcileAudioChannelsonly for the later replace/reload paths.🤖 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 `@app.js` around lines 1547 - 1573, Make the video initialization path use a single channel-reconciliation owner: retain finish() and its attachLinkedAudioChannels flow for the initial async probe, and remove the immediate reconcileAudioChannels(c) call from this kind === "video" block. Keep reconcileAudioChannels available for later replace/reload paths so initialization produces one consistent stem set and toast.
258-282: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrack removal isn't undoable.
removeTimelineTrackmutatesTRACKS/project.trackswithoutpushUndo(), andaddTimelineTracklikewise. Since removal is only allowed on empty lanes the blast radius is small, but Ctrl+Z after a mis-click silently does nothing (or restores clips onto a lane that no longer exists if undo state predates the add).🤖 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 `@app.js` around lines 258 - 282, Update removeTimelineTrack and addTimelineTrack to record their track-list mutations through pushUndo(), ensuring Ctrl+Z restores a removed track or reverses an added track along with the corresponding project.tracks state. Capture the undo state at the appropriate point before each mutation while preserving the existing empty-lane removal behavior.
228-246: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDead duplicate
ensureAudioTrackCount— remove the stale copy.Function declarations hoist, so the later implementation at lines 1465-1491 overrides this one. The live version also uses
MAX_TRACKS_PER_KIND, handles disabled-track bookkeeping, rebuilds clips, and leavesMAX_AUDIO_TRACKSonly in documentation/comment/text. Removing the lines 228-246 copy prevents the two versions drifting.🤖 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 `@app.js` around lines 228 - 246, Remove the earlier duplicate ensureAudioTrackCount function declaration near the top of app.js. Keep the later implementation as the sole live definition, including its MAX_TRACKS_PER_KIND limit, disabled-track bookkeeping, and clip rebuilding behavior.
🤖 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 `@app.js`:
- Around line 1493-1514: Update attachLinkedAudioChannels to release each
existing audio stem before removing it from project.clips: iterate the
filtered-out clips and call releaseClipEl(...) using the same cleanup pattern as
reconcileAudioChannels. Preserve the existing filtering and channel recreation
behavior while ensuring associated media elements and gain nodes are released.
- Around line 288-317: Update showTrackCtxMenu so the newly created Remove track
button receives focus after the menu is added to the document, allowing keyboard
users to activate or dismiss the destructive action. Keep the existing
disabled-state and click-handler behavior unchanged.
- Around line 162-170: Update applyTracksFromProject to deduplicate valid track
definitions by id before clearing and rebuilding TRACKS. Preserve the existing
kind filtering and first-definition behavior, then pass only one definition per
id to makeTrack so duplicate lanes are not persisted or rendered.
- Around line 1475-1491: Update the track-addition flow containing
sortTracksInPlace, serializeTracks, and syncAudioGraphTracks to call
scheduleSave() after the project state and UI have been updated. Ensure
reconcileAudioChannels persists newly created audio lanes and disabledTracks
even when it exits early because the stem set is already correct.
---
Nitpick comments:
In `@app.js`:
- Around line 1547-1573: Make the video initialization path use a single
channel-reconciliation owner: retain finish() and its attachLinkedAudioChannels
flow for the initial async probe, and remove the immediate
reconcileAudioChannels(c) call from this kind === "video" block. Keep
reconcileAudioChannels available for later replace/reload paths so
initialization produces one consistent stem set and toast.
- Around line 258-282: Update removeTimelineTrack and addTimelineTrack to record
their track-list mutations through pushUndo(), ensuring Ctrl+Z restores a
removed track or reverses an added track along with the corresponding
project.tracks state. Capture the undo state at the appropriate point before
each mutation while preserving the existing empty-lane removal behavior.
- Around line 228-246: Remove the earlier duplicate ensureAudioTrackCount
function declaration near the top of app.js. Keep the later implementation as
the sole live definition, including its MAX_TRACKS_PER_KIND limit,
disabled-track bookkeeping, and clip rebuilding behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5819a0db-34e6-4825-954f-4436c42337a9
📒 Files selected for processing (5)
CLAUDE.mdREADME.mdapp.jsindex.htmlstyle.css
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- index.html
- CLAUDE.md
- style.css
Assuming a stereo downmix, each audio clip has a new property 'Pan', this allows to build a proper audio in the final output
|
I think this PR should really be merged. What has been implemented:
|
1 participant
What does this PR do?
Add/delete/solo track feature has been added. +V adds a video track, +A add an audio track. Empty track can be removed via a context menu.
Each track has an additional 'solo' button, that toggles the active state of it, disabling other tracks.
Type of change
How was it verified?
node --check server.js && node --check app.js && node --check mcp-server.jspassesCLAUDE.md/README.mdif the schema, props, or API changedChecklist
Summary by CodeRabbit