feat: add clip replacement/rename feature - #45
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe editor now supports in-place clip media replacement and multichannel video audio. Replacement preserves timeline editing properties and linked companions, while audio channels can create dynamic lanes, display channel-specific labels, and update linked clips when media changes. ChangesMedia and multichannel audio
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Inspector
participant MediaPicker
participant importFiles
participant replaceClipMedia
participant reconcileAudioChannels
Inspector->>MediaPicker: Open source picker
MediaPicker->>importFiles: Import replacement file
MediaPicker->>replaceClipMedia: Apply selected media
replaceClipMedia->>reconcileAudioChannels: Reconcile linked audio channels
reconcileAudioChannels->>Inspector: Refresh clip and channel state
🚥 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://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
app.js (2)
1274-1285: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: clamp position and support Esc.
Math.min(...)alone can place the menu at negative offsets on small viewports, and there's no keyboard dismissal/focus handling for the floating menu.♻️ Suggested tweak
- menu.style.left = Math.round(Math.min(r.left, innerWidth - 220)) + "px"; - menu.style.top = Math.round(Math.min(r.bottom + 4, innerHeight - 340)) + "px"; + menu.style.left = Math.round(Math.max(4, Math.min(r.left, innerWidth - 220))) + "px"; + menu.style.top = Math.round(Math.max(4, Math.min(r.bottom + 4, innerHeight - 340))) + "px";🤖 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 1274 - 1285, Update the floating media menu positioning near the anchor.getBoundingClientRect() logic to clamp both left and top coordinates to non-negative viewport offsets while retaining the existing maximum bounds. Add Escape-key dismissal and appropriate focus handling for the menu, reusing the existing close function and ensuring any added event listeners are removed when it closes.
4653-4654: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplitter reference is dropped here, contrary to the helper's contract.
connectChannelIsolated's doc comment sayssplittermust stay referenced. In the offline path the returned splitter is discarded; it survives only becausesrcholds the outgoing connection. Worth holding the reference (or relaxing the comment) so the two paths agree.🤖 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 4653 - 4654, Update the offline connection branch around connectChannelIsolated so the returned splitter remains strongly referenced for the required lifetime, matching the helper’s contract and the other path; do not discard the helper result before connecting its output to off.destination.
🤖 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 1179-1200: Update the media-replacement flow around the target
selection and reconcileAudioChannels so replacing a linked audio companion
operates on its linkGroup’s video member, keeping all linked clips bound to the
same mediaId and channel layout. Preserve the existing cascade, trimming, undo,
and cache-release behavior while preventing companion-only replacement.
- Around line 168-192: Update addLiveAudioTrackBuses and the initial install
flow in installMeterWorklet so IDs added while the meter is loading are included
when the meter is eventually built. Preserve pending audioTrackIds by merging
newly added lanes with the install snapshot after _loading completes, or defer
the lane update until installation finishes, ensuring new buses are routed
through the meter rather than remaining connected only to audio.master.
- Line 1232: Update the replacement toast in the import flow to choose the
article based on all vowel-starting media kinds, including image and svg, so it
displays “an image” or “an svg” instead of “a image” or “a svg”; preserve the
existing message and audio handling.
- Around line 1064-1085: When removing clips in the channel-reconciliation loop,
also prune each removed clip ID from state.selId and state.selIds so selection
cannot retain dangling references. Apply this alongside the project.clips
filtering in the existing releaseClipEl removal path, while preserving selection
state for clips that remain.
---
Nitpick comments:
In `@app.js`:
- Around line 1274-1285: Update the floating media menu positioning near the
anchor.getBoundingClientRect() logic to clamp both left and top coordinates to
non-negative viewport offsets while retaining the existing maximum bounds. Add
Escape-key dismissal and appropriate focus handling for the menu, reusing the
existing close function and ensuring any added event listeners are removed when
it closes.
- Around line 4653-4654: Update the offline connection branch around
connectChannelIsolated so the returned splitter remains strongly referenced for
the required lifetime, matching the helper’s contract and the other path; do not
discard the helper result before connecting its output to off.destination.
🪄 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: a910a2fc-8dd5-4f67-b14a-3f3c72147752
📒 Files selected for processing (3)
README.mdapp.jsstyle.css
Comment on lines
+168
to
+192
| function addLiveAudioTrackBuses(ids) { | ||
| const audio = runtime.audio; | ||
| if (!audio) return; | ||
| for (const id of ids) { | ||
| if (audio.trackBus[id]) continue; | ||
| audio.trackBus[id] = audio.ctx.createGain(); | ||
| audio.audioTrackIds.push(id); | ||
| } | ||
| if (!audio.meterReady) { | ||
| for (const id of ids) audio.trackBus[id]?.connect(audio.master); | ||
| return; | ||
| } | ||
| // Feed every bus (old + new) through master directly before tearing down | ||
| // the old meter — if the rebuild below fails, all tracks stay audible via | ||
| // this fallback instead of feeding an orphaned, disconnected meter node. | ||
| for (const id of Object.keys(audio.trackBus)) { | ||
| try { audio.trackBus[id].disconnect(); } catch {} | ||
| audio.trackBus[id].connect(audio.master); | ||
| } | ||
| try { audio.meter.port.onmessage = null; } catch {} | ||
| try { audio.meter.disconnect(); } catch {} | ||
| audio.meter = null; | ||
| audio.meterReady = false; | ||
| installMeterWorklet(audio).catch(() => {}); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern 'function installMeterWorklet($$$) { $$$ }' --lang javascript app.jsRepository: ronak-create/FableCut
Length of output: 2870
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== app.js relevant section =="
sed -n '136,196p' app.js
echo
echo "== installMeterWorklet and addLiveAudioTrackBuses call sites =="
rg -n "installMeterWorklet|addLiveAudioTrackBuses|meterReady|audioTrackIds" app.js
echo
echo "== structural verifier: early return ordering relative to async install =="
python3 - <<'PY'
from pathlib import Path
s = Path("app.js").read_text()
start = s.index("function addLiveAudioTrackBuses")
end = s.index("\nasync function installMeterWorklet")
fn = s[start:end]
checks = {
"has_meterReady_false_return": "if (!audio.meterReady) {",
"returns_before_install_call": "audio.meterReady = false;\n installMeterWorklet(audio).catch",
"install_start": "async function installMeterWorklet(audio)",
"snapshots_trackIds_before_async": "const trackIds = audio.audioTrackIds.slice();",
"early_loading_return": "if (audio.meterReady || !audio.ctx.audioWorklet || meterState._loading) return;",
}
for k, pat in checks.items():
print(f"{k}: {'FOUND' if pat in fn or pat in s else 'MISSING'}")
print("concurrent_race_window_found:", all(k in ("early_loading_return", "snapshots_trackIds_before_async") and checks[k] in s for k in ["has_meterReady_false_return", "returns_before_install_call"]))
PYRepository: ronak-create/FableCut
Length of output: 4063
Honor audioTrackIds when adding new live buses during initial install.
installMeterWorklet() exits while _loading is true and later builds the meter from an earlier audioTrackIds slice, while addLiveAudioTrackBuses() adds new lanes directly to master only when meterReady is false. Snapshot a merged track list in the initial install path on return from _loading, or queue the pending lane update until the current meter install completes.
🤖 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 168 - 192, Update addLiveAudioTrackBuses and the initial
install flow in installMeterWorklet so IDs added while the meter is loading are
included when the meter is eventually built. Preserve pending audioTrackIds by
merging newly added lanes with the install snapshot after _loading completes, or
defer the lane update until installation finishes, ensuring new buses are routed
through the meter rather than remaining connected only to audio.master.
Comment on lines
+1064
to
+1085
| const have = project.clips.filter((c) => c.linkGroup === lg && c.kind === "audio"); | ||
| let added = 0, removed = 0; | ||
| for (const c of have) { | ||
| if ((c.props?.audioChannel ?? 0) >= wantCh) { | ||
| releaseClipEl(c.id); | ||
| project.clips = project.clips.filter((x) => x !== c); | ||
| removed++; | ||
| } | ||
| } | ||
| for (let ch = 0; ch < wantCh; ch++) { | ||
| if (have.some((c) => c.props?.audioChannel === ch)) continue; | ||
| project.clips.push({ | ||
| id: "c_" + uid(), mediaId, kind: "audio", track: AUDIO_TRACK_IDS[ch], | ||
| start: live.start, in: live.in, duration: live.duration, name: live.name, | ||
| props: { ...DEFAULT_PROPS, audioChannel: ch }, | ||
| linkGroup: lg, | ||
| }); | ||
| added++; | ||
| } | ||
| if (!added && !removed) return; | ||
| state.dirtyTimeline = true; | ||
| scheduleSave(); renderInspector(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Removed channel clips can leave a stale selection.
project.clips is filtered without pruning state.selId / state.selIds, so if a removed per-channel clip was selected the inspector/selection state keeps a dangling id until the next selection change.
♻️ Proposed fix
if (!added && !removed) return;
+ if (removed) pruneSelection();
state.dirtyTimeline = true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const have = project.clips.filter((c) => c.linkGroup === lg && c.kind === "audio"); | |
| let added = 0, removed = 0; | |
| for (const c of have) { | |
| if ((c.props?.audioChannel ?? 0) >= wantCh) { | |
| releaseClipEl(c.id); | |
| project.clips = project.clips.filter((x) => x !== c); | |
| removed++; | |
| } | |
| } | |
| for (let ch = 0; ch < wantCh; ch++) { | |
| if (have.some((c) => c.props?.audioChannel === ch)) continue; | |
| project.clips.push({ | |
| id: "c_" + uid(), mediaId, kind: "audio", track: AUDIO_TRACK_IDS[ch], | |
| start: live.start, in: live.in, duration: live.duration, name: live.name, | |
| props: { ...DEFAULT_PROPS, audioChannel: ch }, | |
| linkGroup: lg, | |
| }); | |
| added++; | |
| } | |
| if (!added && !removed) return; | |
| state.dirtyTimeline = true; | |
| scheduleSave(); renderInspector(); | |
| const have = project.clips.filter((c) => c.linkGroup === lg && c.kind === "audio"); | |
| let added = 0, removed = 0; | |
| for (const c of have) { | |
| if ((c.props?.audioChannel ?? 0) >= wantCh) { | |
| releaseClipEl(c.id); | |
| project.clips = project.clips.filter((x) => x !== c); | |
| removed++; | |
| } | |
| } | |
| for (let ch = 0; ch < wantCh; ch++) { | |
| if (have.some((c) => c.props?.audioChannel === ch)) continue; | |
| project.clips.push({ | |
| id: "c_" + uid(), mediaId, kind: "audio", track: AUDIO_TRACK_IDS[ch], | |
| start: live.start, in: live.in, duration: live.duration, name: live.name, | |
| props: { ...DEFAULT_PROPS, audioChannel: ch }, | |
| linkGroup: lg, | |
| }); | |
| added++; | |
| } | |
| if (!added && !removed) return; | |
| if (removed) pruneSelection(); | |
| state.dirtyTimeline = true; | |
| scheduleSave(); renderInspector(); |
🤖 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 1064 - 1085, When removing clips in the
channel-reconciliation loop, also prune each removed clip ID from state.selId
and state.selIds so selection cannot retain dangling references. Apply this
alongside the project.clips filtering in the existing releaseClipEl removal
path, while preserving selection state for clips that remain.
Comment on lines
+1179
to
+1200
| if (!media || (clip.mediaId === media.id)) return; | ||
| pushUndo(); | ||
| const targets = (clip.kind === "video" && clip.linkGroup) | ||
| ? project.clips.filter((c) => c.linkGroup === clip.linkGroup) | ||
| : [clip]; | ||
| let trimmed = false; | ||
| for (const c of targets) { | ||
| c.mediaId = media.id; | ||
| // the clip's cached <video>/<audio> element (and its Web Audio graph node) | ||
| // is keyed by clip id and still points at the old src — drop it so | ||
| // getClipEl() rebuilds it against the new media on the next sync/frame. | ||
| releaseClipEl(c.id); | ||
| if (media.duration == null) continue; | ||
| const speed = c.props?.speed || 1; | ||
| // in + duration×speed ≤ media.duration (see CLAUDE.md props reference) | ||
| const maxIn = Math.max(0, media.duration - 0.001); | ||
| if (c.in > maxIn) { c.in = maxIn; trimmed = true; } | ||
| const maxDur = Math.max(0.05, (media.duration - c.in) / speed); | ||
| if (c.duration > maxDur) { c.duration = maxDur; trimmed = true; } | ||
| } | ||
| if (media.kind === "video" || media.kind === "audio") ensureWave(media); | ||
| if (clip.kind === "video") reconcileAudioChannels(clip); // add/drop A3+ channel clips once decoded |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Replacing a linked audio companion desyncs the group.
Only video members cascade to the linkGroup. Selecting an A-lane companion of a video and replacing its source rebinds just that clip, leaving the group with two different mediaIds while props.audioChannel still indexes the old layout; a later reconcileAudioChannels on the video member will then add/remove clips against the video's channel count only. Consider redirecting to the group's video member (or blocking replacement on linked companions).
🤖 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 1179 - 1200, Update the media-replacement flow around
the target selection and reconcileAudioChannels so replacing a linked audio
companion operates on its linkGroup’s video member, keeping all linked clips
bound to the same mediaId and channel layout. Preserve the existing cascade,
trimming, undo, and cache-release behavior while preventing companion-only
replacement.
| if (!c || !added.length) return; | ||
| const m = added.find((x) => x.kind === c.kind) || added[0]; | ||
| if (m.kind === c.kind) replaceClipMedia(c, m); | ||
| else toast(`Imported, but can't replace a ${c.kind} clip with ${m.kind === "audio" ? "an" : "a"} ${m.kind}`); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Wrong article for image/svg kinds.
m.kind === "audio" ? "an" : "a" yields "a image".
✏️ Proposed fix
- else toast(`Imported, but can't replace a ${c.kind} clip with ${m.kind === "audio" ? "an" : "a"} ${m.kind}`);
+ else toast(`Imported, but can't replace a ${c.kind} clip with ${/^[aeiou]/.test(m.kind) ? "an" : "a"} ${m.kind}`);🤖 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` at line 1232, Update the replacement toast in the import flow to
choose the article based on all vowel-starting media kinds, including image and
svg, so it displays “an image” or “an svg” instead of “a image” or “a svg”;
preserve the existing message and audio handling.
# Conflicts: # app.js
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
2 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.
What does this PR do?
Clips can be now replaced on a timeline keeping the settings/effects applied.
In the inspector, there is a field name and source. Name can be changed from the default one inherited from a clip's name.
But clicking on source displays a select with compatible media listed (so a video can be only replaced with another video). All the effects applied to a clip are not affected. This way it is easy to produce a different version of a final composition.
Also, audio channels linking system ware redone from scratch, it is now more flexible, and the number of audio tracks is capped to 16.
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