fix(timeline): preserve partially anchored regions - #307
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
ChangesClip removal behavior
Estimated code review effort: 3 (Moderate) | ~15–30 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 |
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
Reviewed at the PR head (732d2b75). Verdict: ship with nits.
The one-line change is the right fix and it is in the right place. I verified removeClip at src/lib/ai-edition/document/timeline.ts:939 is the only site with that shape, and — importantly — that the trim filter three lines above at :935 must not get the same treatment: a trim's complete v5 anchor is clipId plus its own startSec/endSec (trim-mapping.ts:46, schema/index.ts:225-234), so bare clipId is correct there. Good instinct not to touch it.
I also confirmed the defect is unreachable from any producer on current main — every modifier-creation path (anchorRegionsWithDerivedMs, replacePillSpan, upgradeV4DocumentToV5, useTimeline.addAnnotation/suggestZooms) writes all three anchor fields or none. So this is defensive coverage for hypothetical on-disk data, which matches how #249 described it.
Verification: 851/851 tests pass in src/lib/ai-edition + electron/ai-edition + src/components/ai-edition; both typecheck configs, biome, docs and i18n checks clean. Reverting timeline.ts to main makes timeline.test.ts > preserves a bare clipId… fail as expected — that test genuinely pins the fix.
1. Two divergent copies of the complete-anchor predicate
isAnchored (document/timeline.ts:30-39) carries the comment "One definition, so 'is this anchored?' can never be asked two different ways" — but hasCompleteClipAnchor (timeline/timelineMap.ts:418-428) is a second copy, and they are not equivalent:
isAnchored:!!region.clipId && region.sourceStartSec !== undefinedhasCompleteClipAnchor:typeof clipId === "string" && typeof sourceStartSec === "number"
A region with sourceStartSec: null — reachable for any document not freshly re-parsed by zod, and both the store and the agent tools mutate in memory — is anchored to removeClip/rederiveRegionMs and unanchored to the export path. rederiveAnchoredRegion then runs Math.max(null, clip.sourceStartSec) and silently rewrites the region to the clip's start, while the exporter keeps using its raw ms. Preview and export disagree.
The PR description says this "use[s] the shared complete-anchor predicate" — there isn't one yet. Worth exporting hasCompleteClipAnchor from timelineMap.ts (it already owns ClipAnchored/RegionClipAnchor) and deleting isAnchored, so the comment becomes true.
2. The branch #249 actually described is untested
The new test only covers newClips.length > 0. The last-clip branch changes behaviour here too and nothing pins it — I checked with a throwaway probe: single clip clip_a, one zoom {id:"partial", clipId:"clip_a", startMs:500, endMs:1500} with no source range. On main the zoom is deleted; at PR head it survives. Neither test exercises that path, so the newClips.length === 0 ? … : … ternary can be refactored back to the old semantics with a green suite.
In the existing "drops every modifier anchored to the last remaining clip" test (timeline.test.ts:1394), adding makeZoom({ id: "partial_zoom", clipId: "clip_a", sourceStartSec: undefined, sourceEndSec: undefined }) alongside legacy_zoom and asserting it survives would close this.
3. document-service.test.ts passes on main unmodified
The 85-line test is 79% of the PR's test lines and pins nothing about this fix — zoom_b_2 is fully anchored and the removed clips are a_1/a_2, so both the old and new filter keep it (verified by running the file against main's timeline.ts). It is legitimate as the characterization coverage #249 part 2 asked for, but the PR body lists it under this fix's regression coverage, which will mislead whoever next breaks the predicate. Either say so in a comment above it, or add a second zoom with clipId: "a_2" and no source range and assert it survives removeAsset — that would route the actual fix through removeAsset.
4. The asymmetry needs the comment #249 asked for
A reader sees !(isAnchored(region) && region.clipId === clipId) on :939 and (t) => t.clipId !== clipId on :935 with nothing saying the difference is deliberate, so the obvious "cleanup" reintroduces #249. #249 explicitly asked for this ("Either mirror isAnchored in the filter, or state in the comment…") and the code half shipped without the prose half. Every other non-obvious decision in this file carries a why-comment. Something like:
A region carrying only a
clipIdand no source range is NOT anchored (isAnchored) — it is still placed by its RAW ms, so a clip deletion does not own it. Trims are the opposite:clipIdalone IS their complete anchor (trimAppliesToClip).
5. Trade-off worth stating explicitly (no code change)
A preserved partial-anchor region becomes unreachable but immortal: with clips clip_1 [0–10s] and clip_2 [10–20s] and a zoom carrying bare clipId:"clip_2" at raw 12000–14000ms, deleting clip_2 leaves the zoom at 12–14s on a 10s timeline — off the ruler, no clickable pill, dropped by projectRegionsToSource, and re-emitted by both rederiveRegionMs and reconcileRegionsAfterReplace. If the user then hits Restore full timeline, anchorRegionsWithDerivedMs re-anchors it from those stale raw ms onto whatever footage now sits at 12–14s — the "slid region" class the docblock at timeline.ts:696-717 exists to prevent.
This is the same treatment fully-unanchored legacy regions already get, so it is consistent and defensible. Flagging it so it is a decision rather than an accident.
Nits
document/timeline.ts:938-943— with the predicate tightened, the pre-filter is redundant on thenewClips.length > 0path:rederiveRegionMs(:305-306) already drops every anchored region whoseclipIdis absent fromnewClips, a strict superset. Costs a second walk of all four region collections plus a second document spread per clip delete. Only worth churning if you are editing these lines for the comment above anyway.document-service.test.ts:302-352— four hand-rolled 11-line clip literals differing only inid/assetId/four numbers, for a 6-line assertion. A localconst clip = (id, assetId, src, tl) => ({…})would cut ~40 lines.- Pre-existing, surfacing because the changed line lives inside its callback:
mapAllRegionCollections(:243-272) has noArray.isArrayguard on thelegacyEditorenvelopes, unlikeupgradeV4DocumentToV5(schema/index.ts:577-582). SincelegacyEditorSchemaisz.object({}).passthrough(), a project file withlegacyEditor: { speedRegions: "oops" }loads fine and then throwsregions.filter is not a functionon the first clip delete/move/duplicate.
Nothing blocking. Happy to merge once the comment in §4 lands — the rest can be follow-ups.
|
Filed the |
2 participants
Summary
clipIdRelated issue
Fixes #249
Type of change
Release impact
Desktop impact
Screenshots / video
Not applicable; this changes timeline document behavior and regression coverage.
Testing
npx vitest --run src/lib/ai-edition/document/timeline.test.ts electron/ai-edition/document-service.test.ts(92 passed)npm run test(1,679 passed, 1 skipped)npx tsc --noEmitnpx tsc -p tsconfig.test.json --noEmitnpx biome check src/lib/ai-edition/document/timeline.ts src/lib/ai-edition/document/timeline.test.ts electron/ai-edition/document-service.test.tsnpm run docs:checknpm run i18n:checknpm run build-viteAuthored with Codex assistance and manually verified against the issue's edge cases.
Summary by CodeRabbit
Bug Fixes
Tests