fix(timeline): close the same ambiguity in playback and auto-zoom - #221
Merged
EtienneLescot merged 2 commits intoAug 1, 2026
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes make playback resolution clip-aware and add regression coverage for overlapping clips, boundaries, trims, and source-time fallback. Automatic zoom generation now processes telemetry per applicable timeline clip and projects suggestions into raw timeline coordinates. ChangesClip-aware playback disambiguation
Clip-aware automatic zoom
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VideoPlayback
participant VirtualPreview
participant PlaybackResolver
participant RawClip
VideoPlayback->>VirtualPreview: report current source time
VirtualPreview->>PlaybackResolver: locateKeptSegment with activeClipId
PlaybackResolver->>RawClip: resolve active clip source window
PlaybackResolver-->>VirtualPreview: return kept segment
VirtualPreview->>VideoPlayback: seek next segment and continue playback
sequenceDiagram
participant V4Timeline
participant CursorTelemetry
participant buildAutoZoomSuggestionsForClips
participant ZoomRegions
V4Timeline->>CursorTelemetry: fetch telemetry per source
V4Timeline->>buildAutoZoomSuggestionsForClips: pass clips and telemetry
buildAutoZoomSuggestionsForClips->>ZoomRegions: rebase existing regions per clip
buildAutoZoomSuggestionsForClips-->>V4Timeline: return raw-timeline suggestions
V4Timeline->>ZoomRegions: submit accumulated suggestions
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 (1)
src/components/ai-edition/v4/V4Timeline.tsx (1)
899-925: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFetch telemetry for each source in parallel.
The
forloop awaitsnativeBridgeClient.cursor.getTelemetryone source at a time. Each iteration blocks on an IPC round trip before starting the next. With multiple recordings on the timeline, this adds up the latency of every fetch instead of overlapping them.
existingRegionsis read once before the loop and does not depend on iteration order, so the fetches can run concurrently without changing behavior.⚡ Proposed fix to parallelize telemetry fetches
const existingRegions = tl.zoomRegions.map((z) => ({ startMs: z.startMs, endMs: z.endMs })); - const suggestions: AutoZoomSuggestion[] = []; - for (const source of sources) { - const telemetry = - (await nativeBridgeClient.cursor.getTelemetry(fromFileUrl(source.src))) ?? []; - suggestions.push( - ...buildAutoZoomSuggestionsForClips({ - cursorTelemetry: telemetry, - assetId: source.id, - clips, - existingRegions, - defaultDurationMs: 2000, - }), - ); - } + const perSourceSuggestions = await Promise.all( + sources.map(async (source) => { + const telemetry = + (await nativeBridgeClient.cursor.getTelemetry(fromFileUrl(source.src))) ?? []; + return buildAutoZoomSuggestionsForClips({ + cursorTelemetry: telemetry, + assetId: source.id, + clips, + existingRegions, + defaultDurationMs: 2000, + }); + }), + ); + const suggestions: AutoZoomSuggestion[] = perSourceSuggestions.flat();🤖 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 `@src/components/ai-edition/v4/V4Timeline.tsx` around lines 899 - 925, Update runAutoZooms to fetch telemetry for all sources concurrently instead of awaiting each nativeBridgeClient.cursor.getTelemetry call sequentially. Use Promise.all while preserving the existing source-to-asset association and existingRegions handling, then build AutoZoomSuggestion results from the completed telemetry responses without changing current 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 `@src/components/ai-edition/VirtualPreview.playback.test.tsx`:
- Around line 185-187: Update the AxcutTrimRange fixture in VirtualPreview
playback tests to include the required reason and origin fields, using values
consistent with the AxcutTrimRange type and existing test conventions so tsc
--noEmit passes.
In `@src/lib/ai-edition/timeline/virtual-preview.test.ts`:
- Around line 291-293: Update the AxcutTrimRange fixture passed to
resolvePlaybackSegments in virtual-preview.test.ts by adding the required reason
and origin fields, matching the established fixture shape used in
VirtualPreview.playback.test.tsx while preserving the existing trim values.
- Around line 451-453: Update the AxcutTrimRange fixture passed to
resolvePlaybackSegments in the playbackClips test setup to include the required
reason and origin fields, matching the complete fixture shape used elsewhere.
Keep the existing id, assetId, clipId, and trim range values unchanged.
In `@src/lib/ai-edition/timeline/virtual-preview.ts`:
- Around line 143-160: Update isWithinClipBounds to default an absent
clip.sourceEndSec to clip.sourceStartSec, matching resolvePlaybackSegments.
Preserve the existing inclusive/exclusive epsilon handling while ensuring
unprobed clips with nonzero sourceStartSec can match their source content.
---
Nitpick comments:
In `@src/components/ai-edition/v4/V4Timeline.tsx`:
- Around line 899-925: Update runAutoZooms to fetch telemetry for all sources
concurrently instead of awaiting each nativeBridgeClient.cursor.getTelemetry
call sequentially. Use Promise.all while preserving the existing source-to-asset
association and existingRegions handling, then build AutoZoomSuggestion results
from the completed telemetry responses without changing current 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: ca52d276-811c-4aff-afa3-d6f88b0e405f
📒 Files selected for processing (8)
src/components/ai-edition/VirtualPreview.playback.test.tsxsrc/components/ai-edition/VirtualPreview.tsxsrc/components/ai-edition/v4/V4Timeline.tsxsrc/lib/ai-edition/timeline/virtual-preview.test.tssrc/lib/ai-edition/timeline/virtual-preview.tssrc/lib/ai-edition/timeline/zoom-suggestions.test.tssrc/lib/ai-edition/timeline/zoom-suggestions.tstechnical-documentation/architecture/timeline-model.md
EtienneLescot
changed the title
EtienneLescot
force-pushed
the
claude/shared-recording-clips-bugs-a449bf
branch
from
August 1, 2026 11:43
f3f7071 to
5a9334f
Compare
v7 gave trims a `clipId`, but three READERS still answered "which clip is this?" from `(assetId, sourceTime)` — or from POSITION IN THE ARRAY. Source time is per asset: the moment two clips draw on the same media, it names two places at once. One cause, the two reported symptoms, and two adjacent defects found while verifying. --- Playback stops on reaching the second clip `isWithinClipBounds` gave a clip an EXCLUSIVE closing edge unless it was the LAST ELEMENT OF THE ARRAY, and the `preferredClipId` guard shared that edge. In its final ~50 ms — exactly when the rAF loop decides what comes next — a clip disowned its own last frames and the ambiguous scan handed them to its twin, placing the playhead near the END of that twin. `reachedClipEnd` then fired on a clip nothing follows: pause, playhead parked at the end of the timeline. Because the exception keyed off array position, the bug DEPENDED ON CLIP ORDER, which is what the reporter's experiments isolated: `A1 -> A2 -> C3` did not bug (the last clip belongs to another asset, the filter excludes it, the scan returns null, and the timeline-order fallback did the right thing by accident); `A1 -> C3 -> A2`, `C3 -> A1 -> A2` and any pair of twins did. All six layouts are in the test. An INCLUSIVE bound when the clip is named — identity beats proximity, the exclusive bound exists only to break ties in the scan and a named clip has no tie to break — plus a two-pass scan: strict containment first, closing edge only if nobody claims the instant. Same answer on a single-asset timeline, reached without consulting the order. --- "Automatic zooms" only decorates the first clip Cursor telemetry is recorded against the ORIGINAL file: `timeMs` is the asset's SOURCE time (the axis `cursor-track.ts` maps through `locateSourcePosition`). Zooms are authored in RAW timeline ms. The two axes coincide for one layout only: a single clip, at 0, covering the whole recording. Anywhere else, suggestions landed where `[0, asset duration]` falls on the ruler — the first clip. Telemetry was also read only for `videoSources[0]`, so a second recording was never consulted. `buildAutoZoomSuggestionsForClips` does the projection, per clip and by a plain shift (a raw clip is identity between its source and raw-virtual time). A dwell replayed by two clips therefore yields one zoom on EACH. Each clip sees only the samples in its own source window: a dwell a cut splits is no longer one dwell, which is right — the cursor did not sit still across the cut on the timeline being watched. --- Two adjacent defects, found while verifying A cut was never skipped during playback if a twin kept the stretch: "am I inside a cut?" scanned every segment by asset. `locateKeptSegment` narrows the question to the segments of the clip being played — a source time none of them covers is inside THAT clip's cut, however many other clips keep it. `findNextKeptSegment`'s source-clock fallback compared source positions across a whole asset. "Later in source time" only means something within one clip: with a late slice laid down BEFORE a trimmed early slice, entering that cut answered the first clip — raw start 0 — so playback jumped to the top of the timeline, fell into the same cut, and looped. Pre-existing, but the fix above makes that path far more reachable: leaving it would have traded one bug for a worse one. --- What is NOT touched `totalVirtualDuration` (`clips.at(-1)`) and `locateVirtualPosition`'s `index === clips.length - 1` still depend on array order. `resequenceClips` maintains that order, so neither is reachable today, and `locateVirtualPosition` has five callers: that is a separate change, not something to smuggle into a bugfix. Tests: every fix was verified FAILING without it. The integration test reproduces the symptom end to end on the pre-fix code (`pause()` at 9.96 s). No existing test covered the closing edge of a clip with a twin over the same media.
The three new AxcutTrimRange fixtures omitted `reason` / `origin`. Invisible to `tsc --noEmit`, which excludes `**/*.test.ts(x)` — `tsconfig.test.json` exists precisely for what the main config leaves out, and it is the gate CI runs. Caught by review, not by me: I checked the wrong one of the two typecheck jobs. Two review points taken while here: - `isWithinClipBounds` defaulted a missing `sourceEndSec` to 0 (as it always had), where `resolvePlaybackSegments` reads the same absent field as a zero-width window at the in-point. `locateKeptSegment` now feeds this function that very output, so the two sit in series and must not read one field two ways. No behaviour changes: a clip only awaits probing with `sourceStartSec === 0`, where both defaults coincide. A divergence removed, not a bug. - The per-asset telemetry fetches ran one IPC round trip after another. Nothing in the loop depends on visit order (`existingRegions` is read up front), so they now run concurrently; `Promise.all` preserves order, so the output is identical.
EtienneLescot
force-pushed
the
claude/shared-recording-clips-bugs-a449bf
branch
from
August 1, 2026 12:49
5a9334f to
dca3aa5
Compare
This was referenced Aug 4, 2026
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
1 participant
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.
v7 gave trims a
clipId, but three readers still answered "which clip is this?" from(assetId, sourceTime)— or from array position. Source time is per asset, so the moment two clips draw on the same media it names two places at once. One cause, both reported symptoms, and two adjacent defects found while verifying.Playback stops on reaching the second clip
isWithinClipBoundsgave a clip an exclusive closing edge (sourceEndSec - ε) unless it was the last element of the clips array, and thepreferredClipIdguard shared that edge. In a clip's final ~50 ms — exactly when the rAF loop decides what to play next — the clip disowned its own last frames, and the ambiguous scan handed them to its twin, reporting the playhead near the end of that twin.reachedClipEndthen fired on a clip nothing follows: pause, playhead parked at the end of the timeline.Because the exception keyed off array position, the bug depended on clip order, which is what the reporter's experiments isolated:
A1 → A2 → C3null, and the timeline-order fallback did the right thing by accidentA1 → C3 → A2C3 → A1 → A2A1 → A2/A2 → A1Fix: an inclusive closing edge when the clip is named — identity beats proximity, the exclusive bound exists only to break ties in the scan, and a named clip has no tie to break — plus a two-pass scan (strict containment first, closing edge only if nobody claims the instant). Same answer on a plain single-asset timeline, reached without consulting the order.
"Automatic zooms" only decorates the first clip
Cursor telemetry is recorded against the original file:
timeMsis the asset's source time (the axiscursor-track.tsmaps throughlocateSourcePosition). Zoom regions are authored in raw timeline ms. The two axes coincide for exactly one layout: a single clip, at 0, covering the whole recording. Anywhere else, suggestions landed wherever[0, assetDuration]falls on the ruler — the first clip. Telemetry was also only read forvideoSources[0], so a second recording was never consulted at all.buildAutoZoomSuggestionsForClipsdoes the projection, per clip and by a plain shift (a raw clip is identity between its source and raw-virtual time). A dwell replayed by two clips therefore yields one zoom on each.Two adjacent defects, found while verifying
locateKeptSegmentnarrows "am I inside a cut?" to the segments of the clip being played.findNextKeptSegment's source-clock fallback compared source positions across a whole asset. "Later in source time" only means something within one clip: with a slice from late in a recording laid down before a trimmed slice from early in it, playing into that cut answered the first clip — raw start 0 — so playback jumped to the top of the timeline, fell into the same cut, and looped. Pre-existing, but the fix above makes that path far more reachable: leaving it would have traded one bug for a worse one.Deliberately not touched
totalVirtualDuration(clips.at(-1)) andlocateVirtualPosition'sindex === clips.length - 1still depend on array order.resequenceClipsmaintains that order, so neither is reachable today, andlocateVirtualPositionhas five callers: that is a separate change, not something to smuggle into a bugfix.Tests
Every fix was verified failing without it. The integration test (
VirtualPreview.playback.test.tsx, rAF loop driven by hand) reproduces the symptom end to end on the pre-fix code —pause()at 9.96 s. No existing test covered the closing edge of a clip with a twin over the same media.npm run test: 1419 tests / 117 files ✅ ·tsc --noEmit✅ ·biome check✅ ·check-docs✅ ·i18n:check✅technical-documentation/architecture/timeline-model.mdgains the playback and telemetry sections, plus the invariant: clip order is never an input.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Documentation