feat: add disable/enable track buttons, remove extra track labels - #20
Conversation
Tracks can be disabled by buttons at the beginning of the track; they are dimmed and buttons state clearly shows the current status. 'video' and 'audio' lables removed as redundant, there are already A(i) and V(i) labels.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughDisabled-track state is persisted in ChangesDisabled Track Flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant TrackState
participant PlaybackRenderer
participant ExportSetup
participant FastExport
Editor->>TrackState: toggle track enabled state
TrackState->>PlaybackRenderer: apply enabled-track filtering
PlaybackRenderer->>PlaybackRenderer: skip disabled playback and rendering
TrackState->>ExportSetup: provide disabled track set
ExportSetup->>FastExport: start export with warning state
FastExport->>FastExport: omit disabled video, audio, and assets
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app.js (3)
2209-2220: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
.slice()call.Since
Array.prototype.filter()already returns a newly created shallow copy of the array, the subsequent.slice()before.reverse()is redundant and can be omitted.♻️ Proposed cleanup
function pickClipAt(pt, W, H) { const seq = []; - for (const tr of TRACKS.filter((tk) => tk.kind === "video" && isTrackEnabled(tk.id)).slice().reverse()) { + for (const tr of TRACKS.filter((tk) => tk.kind === "video" && isTrackEnabled(tk.id)).reverse()) { for (const c of project.clips.filter((c) => c.track === tr.id && activeAt(c, state.time)).sort((a, b) => a.start - b.start)) if (isVisualClip(c)) seq.push(c); }🤖 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 2209 - 2220, Remove the redundant slice() call from the TRACKS.filter(...).reverse() chain in pickClipAt, keeping the existing filtering and reverse iteration behavior unchanged.
161-177: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUpdate the preview immediately when toggling tracks while paused.
Currently, toggling a track updates the DOM and state, but the canvas won't reflect the change until the next timeline interaction or playback frame. Calling
seekMediaWhilePaused()anddrawFrame()when playback is paused ensures the UI is immediately responsive to the track state change.💡 Proposed fix to update the preview synchronously
function toggleTrackEnabled(id) { if (state.disabledTracks.has(id)) state.disabledTracks.delete(id); else state.disabledTracks.add(id); saveDisabledTracks(); const head = els.trackHeaders.querySelector(`.track-head[data-track="${id}"]`); const row = els.tracks.querySelector(`.track[data-track="${id}"]`); const on = isTrackEnabled(id); if (head) { head.classList.toggle("disabled", !on); const btn = head.querySelector(".track-toggle"); if (btn) { btn.setAttribute("aria-pressed", on ? "true" : "false"); btn.title = on ? "Disable track" : "Enable track"; } } if (row) row.classList.toggle("disabled", !on); + + if (!state.playing) { + seekMediaWhilePaused(); + drawFrame(); + } }🤖 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 161 - 177, Update toggleTrackEnabled to refresh the preview after changing the track state when playback is paused: call seekMediaWhilePaused() and drawFrame() in the paused path, while preserving the existing DOM updates and avoiding unnecessary refreshes during playback.
1699-1715: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDefer expensive property calculations for inactive or disabled clips.
evalProps(c, t)andmediaTimeAt(c, t)are computed for every clip on every frame, even when the clip is inactive or the track is disabled. Moving these inside theifblock prevents unnecessary processing (such as keyframe interpolation) and significantly optimizes the render loop.⚡ Proposed performance optimization
const enabled = isTrackEnabled(c.track); - const p = evalProps(c, t); - const sp = clamp(+p.speed || 1, 0.1, 8); - const mt = mediaTimeAt(c, t); - if (state.playing && enabled && activeAt(c, t)) { + if (state.playing && enabled && activeAt(c, t)) { + const p = evalProps(c, t); + const sp = clamp(+p.speed || 1, 0.1, 8); + const mt = mediaTimeAt(c, t); if (el.playbackRate !== sp) { try { el.playbackRate = sp; } catch {} } if (el.paused) el.play().catch(() => {}); if (Math.abs(el.currentTime - mt) > 0.25 * sp) { try { el.currentTime = mt; } catch {} } const vol = clamp(p.volume, 0, 4); const g = runtime.clipGain.get(c.id); if (g) g.gain.value = vol; else el.volume = clamp(vol, 0, 1); } else { if (!el.paused) el.pause(); const g = runtime.clipGain.get(c.id); if (g) g.gain.value = 0; }🤖 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 1699 - 1715, Move the evalProps(c, t) and mediaTimeAt(c, t) calls inside the state.playing && enabled && activeAt(c, t) branch, computing p before speed and volume use and mt only where synchronization is needed; keep inactive or disabled clips from performing these calculations while preserving existing playback 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.
Nitpick comments:
In `@app.js`:
- Around line 2209-2220: Remove the redundant slice() call from the
TRACKS.filter(...).reverse() chain in pickClipAt, keeping the existing filtering
and reverse iteration behavior unchanged.
- Around line 161-177: Update toggleTrackEnabled to refresh the preview after
changing the track state when playback is paused: call seekMediaWhilePaused()
and drawFrame() in the paused path, while preserving the existing DOM updates
and avoiding unnecessary refreshes during playback.
- Around line 1699-1715: Move the evalProps(c, t) and mediaTimeAt(c, t) calls
inside the state.playing && enabled && activeAt(c, t) branch, computing p before
speed and volume use and mt only where synchronization is needed; keep inactive
or disabled clips from performing these calculations while preserving existing
playback behavior.
ronak-create
left a comment
ronak-create
left a comment
There was a problem hiding this comment.
Reviewed — merging. Two follow-ups worth tracking:
-
Persistence home: disabled-track state lives in a single global
localStoragekey rather thanproject.json. Consequences: the same project.json exports differently depending on browser state; MCP/agent workflows can't see or set the flag; and the state isn't per-project (disabling V2 here disables it in every project opened in this browser). Since #21 persists its work-area markers in project.json, adisabledTracksarray there would fit the architecture better. Fine as a follow-up PR. -
Stale frame on re-enable while paused:
seekMediaWhilePaused()skips disabled tracks andtoggleTrackEnabled()doesn't force a reseek — re-enabling a video track after moving the playhead shows the old frame until the next seek. CallingseekMediaWhilePaused()at the end oftoggleTrackEnabled()would fix it.
Also: nice catch zeroing the gain node in syncMedia's else branch. Verified node --check passes on the branch head, and export parity holds by code reading (fast export reuses drawFrame, and renderAudioMix filters disabled tracks).
2 participants
What does this PR do?
Tracks can be disabled by buttons at the beginning of the track; they are dimmed and buttons state clearly shows the current status. 'video' and 'audio' labels removed as redundant, there are already A(i) and V(i) labels.
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
New Features
Style
Documentation