feat: represent a/v tracks on the timeline separately#17
Conversation
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughVideo imports create linked stereo audio companions, channel-aware waveforms, and persisted AV relationships. Timeline edits preserve linked timing. Runtime and offline audio isolate channels, while per-track RMS/LUFS/peak metering is added to the monitor UI. ChangesAV-linked editing and audio monitoring
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MediaAsset
participant addClipFromMedia
participant Timeline
participant AudioGraph
participant MeterWorklet
participant VUMeter
MediaAsset->>addClipFromMedia: provide video media
addClipFromMedia->>Timeline: create linked video and stereo audio clips
Timeline->>AudioGraph: route clips by audioChannel
AudioGraph->>MeterWorklet: provide per-track audio
MeterWorklet->>VUMeter: report RMS, peak, and LUFS values
Possibly related PRs
🚥 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: 1
🤖 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 704-724: Update syncLinkedTiming to accept the trim offset, rebase
L.keyframes with the existing shiftKF(...) helper before saving the linked clip,
and preserve the current start, in, duration, and speed synchronization
behavior. Update each caller to pass the offset produced by the trim operation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| function linkedClip(c) { | ||
| return c?.linkedId ? getClip(c.linkedId) : null; | ||
| } | ||
| /* Expand a clip list so each AV-linked partner is included once. */ | ||
| function withLinked(clips) { | ||
| const out = new Map(); | ||
| for (const c of clips) { | ||
| out.set(c.id, c); | ||
| const L = linkedClip(c); | ||
| if (L) out.set(L.id, L); | ||
| } | ||
| return [...out.values()]; | ||
| } | ||
| function syncLinkedTiming(c) { | ||
| const L = linkedClip(c); | ||
| if (!L) return; | ||
| L.start = c.start; | ||
| L.in = c.in; | ||
| L.duration = c.duration; | ||
| if (c.props?.speed != null) L.props.speed = c.props.speed; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section of app.js
nl -ba app.js | sed -n '660,760p'
# Find keyframe helpers and all uses of shiftKF/keyframes that might affect linked clips
rg -n "function shiftKF|shiftKF\(|keyframes" app.js
# Inspect the trim paths mentioned in the comment
nl -ba app.js | sed -n '880,960p'
nl -ba app.js | sed -n '1120,1195p'Repository: ronak-create/FableCut
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
line_range() {
local start="$1" end="$2" file="$3"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e { printf "%d:%s\n", NR, $0 }' "$file"
}
echo "== syncLinkedTiming area =="
line_range 690 740 app.js
echo
echo "== shiftKF/keyframes references =="
rg -n "function shiftKF|shiftKF\(|keyframes" app.js
echo
echo "== trim call sites =="
line_range 900 940 app.js
echo
line_range 1145 1185 app.jsRepository: ronak-create/FableCut
Length of output: 7672
🏁 Script executed:
#!/bin/bash
set -euo pipefail
line_range() {
local start="$1" end="$2" file="$3"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e { printf "%d:%s\n", NR, $0 }' "$file"
}
echo "== shiftKF implementation =="
line_range 1800 1845 app.js
echo
echo "== linkedId references =="
rg -n "linkedId|linkedClip\(|withLinked\(" app.js
echo
echo "== clip duplication / linking flows =="
line_range 1060 1135 app.jsRepository: ronak-create/FableCut
Length of output: 6666
🏁 Script executed:
#!/bin/bash
set -euo pipefail
line_range() {
local start="$1" end="$2" file="$3"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e { printf "%d:%s\n", NR, $0 }' "$file"
}
echo "== link/duplicate/remove logic =="
line_range 740 905 app.js
echo
echo "== keyframe mutation helpers =="
rg -n "keyframes\\s*=|delete .*keyframes|Object\\.values\\(c\\.keyframes\\)|shiftKF\\(" app.jsRepository: ronak-create/FableCut
Length of output: 7664
🏁 Script executed:
#!/bin/bash
set -euo pipefail
line_range() {
local start="$1" end="$2" file="$3"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e { printf "%d:%s\n", NR, $0 }' "$file"
}
echo "== keyframe UI / handlers =="
line_range 1580 1645 app.js
echo
echo "== keyframe rendering / usage =="
line_range 1720 1755 app.jsRepository: ronak-create/FableCut
Length of output: 4623
syncLinkedTiming should rebase the linked clip’s keyframes too.
When trim changes start/duration, the partner keeps stale keyframes, so any existing curves drift or get clipped at the wrong times. Pass the trim offset through and apply the same shiftKF(...) rebasing to L.keyframes before saving.
🤖 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 704 - 724, Update syncLinkedTiming to accept the trim
offset, rebase L.keyframes with the existing shiftKF(...) helper before saving
the linked clip, and preserve the current start, in, duration, and speed
synchronization behavior. Update each caller to pass the offset produced by the
trim operation.
|
If we have the video clip deconstructed into video/audio tracks like on the screenshot below, audio meters (also visible below) are actually useful and logical. On the timeline, audio tracks are linked to a video clip, so editing process is unchanged. Audio meter has RMS, LUFS and peak implemented already. 
If this makes sense, I will create a separate pull request with all the features implemented. |
ronak-create
left a comment
ronak-create
left a comment
There was a problem hiding this comment.
Good direction for the proposal — linked A/V is the right model, and the pairwise re-linking of split halves is done carefully. One data-loss bug and a few design points to settle:
Blocking:
linkedIdis dropped on save.projectJSON()serializes clips through a field whitelist ({ id, mediaId, kind, track, start, in, duration, name, props, keyframes, transitionIn, transitionOut }) andlinkedIdisn't in it — so every AV link silently vanishes on page refresh or any external reload, leaving orphaned muted videos + unlinked audio clips. Needs: addlinkedIdto the whitelist, and document it in CLAUDE.md's schema section (it's served to agents viafablecut_docs; agents writing project.json directly should be able to create linked pairs too).
Design questions (worth deciding before this leaves proposal stage):
- CLAUDE.md's semantics say "a video clip's own audio plays with it" and "fade also fades audio". With the video muted and audio split out, a
transitionOut: fadeon the video clip no longer fades its sound — transitions aren't synced between partners. Either sync transitions too, or document the new contract. - Linked audio always lands on A1, which the docs establish as the music-bed lane — importing a video on top of an existing music bed overlaps it. Consider the first free A track.
- Delete removes both partners with no unlink affordance — there's no way to keep picture-only (short of muting) or audio-only. Alt+drag/alt+delete to break the link would round this out.
- Behavior change beyond linking: group drags can now move the grabbed clip to a different track (previously track changes were locked to single-clip drags). Intentional?
Nits: groupOrig now snapshots in/duration/keyframes but only .start is ever read — leftover? Inspector timing edits don't call syncLinkedTiming, so numeric edits desync partners. Also heads-up: #21 refactors splitAtPlayhead into splitClipAt, so these two will conflict — coordinate merge order.
Verified node --check passes on the branch head; currently mergeable against main.
b05efed to
d69db92
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
style.css (1)
1085-1101: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMove truncation off the flex container.
.clip-labelis a flex container, but the label text is injected as a raw text node, so thewhite-space/overflow/text-overflowrules on this element won’t produce an ellipsis reliably. Wrap the text in an inner element and apply truncation there, or removedisplay: flexfrom.clip-label.🤖 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 `@style.css` around lines 1085 - 1101, Update the `.clip .clip-label` truncation structure so the raw label text is wrapped in an inner element that receives `white-space`, `overflow`, and `text-overflow: ellipsis`; preserve the current flex positioning and alignment on `.clip-label`, and ensure the inner element can shrink within the flex container.
🤖 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 1461-1465: Update the clip-label construction in the body-building
flow to prevent HTML injection from user-controlled c.name and c.props.text
values. Escape every dynamic label value before interpolating it into the
innerHTML string, including the text caption’s first line and names for each
clip kind, while preserving the existing prefixes and label formatting.
In `@CLAUDE.md`:
- Around line 317-320: Update the “Rendering order” documentation in CLAUDE.md
so A1–A4 are described as containing both standalone audio clips and separately
visible, linked audio companions from imported video; retain the existing
V-track and video-clip audio guidance.
In `@style.css`:
- Around line 599-606: Update the .vu-scale height and its explanatory comment
to match the actual .vu-segs layout: use the .vu-seg height of 12px, the gap of
2px, and the verified METER_SEGS count of 16, producing a 222px total height so
the tick marks align with the segments.
---
Outside diff comments:
In `@style.css`:
- Around line 1085-1101: Update the `.clip .clip-label` truncation structure so
the raw label text is wrapped in an inner element that receives `white-space`,
`overflow`, and `text-overflow: ellipsis`; preserve the current flex positioning
and alignment on `.clip-label`, and ensure the inner element can shrink within
the flex container.
🪄 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
Run ID: d3316c08-9948-4a5e-81a6-0457c4529e0f
📒 Files selected for processing (6)
CLAUDE.mdREADME.mdapp.jsindex.htmlmeter-worklet.jsstyle.css
|
I hope all the previous issues have been addressed. There are few things, however:
As ffmpeg availability is checked anyway, this leads to having some options enabled conditionally. Do you think this would be acceptable as the app grows? |
# Conflicts: # app.js
# Conflicts: # app.js
Unlinking audio from video is not supported (yet)
…ers, LUFS, RMS and peak. A video clip on a timeline shows audio tracks as (linked) audio tracks. This way, for a 2 channel audio track, one of the channels can be muted, for example. Also, audio meter measures each channel separately. The default number of audio tracks is now 4, 3 video tracks.
f251711 to
ed05f69
Compare
2 participants
What does this PR do?
This is just a proposal how video clips with audio tracks could be represented on the timeline. Audio tracks are separated yet linked to the video clip.
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