[codex] Fix changed clip ID merge contract - #26
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 948bb018f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let TrackChild::Clip(anchor) = child | ||
| && let Some(anchor_id) = &anchor.clip_id | ||
| && !source_changed.contains(anchor_id) | ||
| && let Some(index) = target_clip_index(target_track, anchor_id) | ||
| { | ||
| return Some(index + 1); | ||
| } |
There was a problem hiding this comment.
When multiple changed clips appear consecutively at the end of a source track, this insertion logic can reverse their order in the merged result. In source_insertion_index, each trailing clip anchors to the same unchanged predecessor (index + 1), so later clips are inserted before earlier ones (e.g., source x,a,b with a,b changed merges as x,b,a). This produces an incorrect timeline ordering even for non-overlapping merges.
Useful? React with 👍 / 👎.
| if let TrackChild::Clip(clip) = child | ||
| && let Some(id) = &clip.clip_id | ||
| { |
There was a problem hiding this comment.
Move detection misses cases where an identified clip changes position relative to clips without IDs, because clip_index is only tracked for TrackChild::Clip values that already have clip_id. If a no-ID clip is inserted/moved before an identified clip, that identified clip keeps the same computed index and is treated as unchanged, so source-side reorder edits can be silently dropped during changed-clip-ID merges.
Useful? React with 👍 / 👎.
| TrackChild::Clip(clip) => clip | ||
| .clip_id | ||
| .as_ref() | ||
| .is_none_or(|id| !source_changed.contains(id)), | ||
| _ => true, |
There was a problem hiding this comment.
This retain pass removes changed clips but always keeps non-clip children, so source-side clip deletions can leave dangling transitions/gaps from the target timeline. For example, deleting a changed clip that had an adjacent transition preserves that transition even though one side of it is gone, producing an invalid or semantically corrupted track ordering after an otherwise “clean” merge.
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes the issue #4 merge contract gaps found in review:
merge_changed_clip_idstargetsHEADRoot Cause
The changed-clip merge path keyed changes only by clip contents, so source moves were invisible. Its overlay also replaced or appended source clips but never removed clips deleted by the source branch. Finally, successful
HEADtarget merges created a commit without advancing the current branch ref.Validation
cargo fmt --checkcargo test -p vedit-corecargo testCloses #4.