Add/remove/solo tracks#41
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 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, 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. |
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