fix(timeline): prefer exact segment clip matches - #241
Conversation
|
Warning Review limit reached
Next review available in: 37 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe raw clip lookup now prioritizes exact segment IDs, then matches derived segment IDs to the longest raw clip ID. Tests cover both cases and verify the resulting virtual start time. ChangesRaw clip resolution
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 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.
🧹 Nitpick comments (1)
src/lib/ai-edition/timeline/virtual-preview.ts (1)
62-64: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid sorting
rawClipson every lookup.
locateKeptSegmentcalls this helper once per playback segment. For each segment without an exact match, this code copies and sorts the fullrawClipsarray. Scan once while retaining the longest matching ID, or build a sorted index once outside this helper.Proposed O(n) lookup
rawClips.find((clip) => clip.id === segment.id) ?? - [...rawClips] - .sort((a, b) => b.id.length - a.id.length) - .find((clip) => segment.id.startsWith(`${clip.id}_seg`)) ?? + rawClips.reduce<AxcutClip | undefined>((longest, clip) => { + if (!segment.id.startsWith(`${clip.id}_seg`)) return longest; + return !longest || clip.id.length > longest.id.length ? clip : longest; + }, undefined) ??🤖 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/lib/ai-edition/timeline/virtual-preview.ts` around lines 62 - 64, Update locateKeptSegment to avoid copying and sorting rawClips for every segment lookup. Replace the per-call [...rawClips].sort(...).find(...) logic with a single scan that retains the longest clip ID matching segment.id.startsWith(`${clip.id}_seg`), preserving the current match preference without repeated sorting.
🤖 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 `@src/lib/ai-edition/timeline/virtual-preview.ts`:
- Around line 62-64: Update locateKeptSegment to avoid copying and sorting
rawClips for every segment lookup. Replace the per-call
[...rawClips].sort(...).find(...) logic with a single scan that retains the
longest clip ID matching segment.id.startsWith(`${clip.id}_seg`), preserving the
current match preference without repeated sorting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 76759673-7009-4438-bed8-252ff8746554
📒 Files selected for processing (2)
src/lib/ai-edition/timeline/virtual-preview.test.tssrc/lib/ai-edition/timeline/virtual-preview.ts
|
Addressed the CodeRabbit performance nit in commit feab5e9: longest generated-segment parent selection is now a single O(n) scan with no copied/sorted array, while preserving exact-ID priority and longest-prefix behavior. Verified with 27/27 focused tests, both TypeScript configs, Biome, and diff checks. |
|
Closing this one too, for a narrower reason than #240 — the code here is actually fine. The precedence flaw is real at the function level. I reproduced it: given a clip It just cannot happen. I enumerated every site that mints a clip id — five non-test sites — and they all produce That alone would not make me close it — defensive hardening of an internal helper is a reasonable thing to want. What tips it is the framing. The PR is titled If you want to reopen it as Six of the ten landed: #235, #236, #237, #238, #239, #242. Thanks for the batch — the hit rate on the ones that described a reachable bug was high. |
2 participants
Summary
_seg-like IDs remain unambiguousRelated issue
No linked issue; found while auditing segment-to-source identity handling.
Type of change
Release impact
Desktop impact
Screenshots / video
Not applicable; this fixes internal timeline identity resolution.
Testing
npm exec -- vitest run src/lib/ai-edition/timeline/virtual-preview.test.tsnpm exec -- biome check src/lib/ai-edition/timeline/virtual-preview.ts src/lib/ai-edition/timeline/virtual-preview.test.tsnpm run build-vitenpm run wb:typecheckSummary by CodeRabbit