Repository navigation
fix(timeline): stop a region dragged into an identical one flush against it - #1144
Conversation
…nst it The repel clamp only treated a neighbour of a different identity as a wall, so an identical region dragged into another was stored over it. The pair then showed as one pill and an edit of either half reached both. Every other pill is now a wall; once flush, identical neighbours still merge on display and an edit splits them (#1017). Covers speed, zoom, annotation and Full Camera regions. Refs #1133.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 9 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to Dragging an identical region into another now stops flush against it instead of storing an overlap. Nothing in the supplied evidence points to a merge-blocking risk. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
The PR satisfies #1133. clampSpanAgainstNeighbours now treats every other pill as a wall. replacePillSpan passes all other pills to the clamp. The parameterized timelineMap.test.ts regression te… |
| Out of Scope Changes check | |
The changes stay within #1133. The audioTracks.ts update adapts an existing caller to the removed identity parameter and preserves its existing neighbour-only resize path. Timeline tests and archite… |
| Docstring Coverage | |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. (1 skipped: 1 … |
| Title check | |
The title clearly and concisely describes the primary fix: preventing an identical timeline region from overlapping its neighbor during dragging. |
| Description check | |
The description is complete and follows the repository template. It explains the behavior change, linked issue, change type, release impact, platform impact, testing, and manual verification. |
✨ Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
One gap: the agent's refuseIfBlocked (electron/ai-edition/agent-tools.ts, around line 143) still mirrors the old rule. Its wall filter keeps other.identity !== pill.identity, and its message says two regions "with different settings cannot overlap". It exists so a set* call with an exact span is refused rather than clamped somewhere the caller never asked for (its own comment: asked 7.6-9.5 s, landed 1.5-3.3 s). With this PR, setZoom / setSpeed / setAnnotation / setCameraFullscreen aimed into an identical neighbour passes refuseIfBlocked, then replacePillSpan clamps it, so the agent gets ok:true with a span it did not ask for. Suggested fix: drop the identity condition there, reword the message to "two s cannot overlap", update the doc comment above it ("other pills with other settings") and the comment in agent-tools.test.ts ("forbids two zooms of different identities from overlapping"), and extend "refuses a set that would run into another region" with a look-alike case, e.g. zoom depth 2 at 1-2 s and 12-14 s, setZoom on the first to 13-15 s expects ok:false; same for two speed 2 regions at 20-22 s and 24-26 s, setSpeed to 23-25 s. I checked that this test fails with the PR's timelineMap change alone and passes once the filter is dropped.
refuseIfBlocked still let a set* call through when the region in the way had the same settings. replacePillSpan now stops against it, so the agent got ok with a span it never asked for. The refusal now sees the same walls as the drag.
|
Addressed in 3666fd6:
|
|
@coderabbitai review |
|
1 participant
Summary
An identical region dragged into another now stops flush against it, like a region of different properties already did (#1008). It used to be stored over its neighbour: the pair showed as one pill, and an edit of "one half" changed both.
clampSpanAgainstNeighbourstreats every other pill as a wall, whatever its identity. Itsidentityparameter is gone.replacePillSpan. The audio lane's resize already passed only other tracks, so it is unchanged.refuseIfBlockedsees the same walls, so aset*call aimed into a look-alike is refused instead of landing short of where it was asked.Candidate for cherry-pick to release/v2.1.0.
Related issue
Fixes #1133
Type of change
Release impact
Desktop impact
Screenshots / video
Not applicable: no visual change beyond where the region lands.
Testing
timelineMap.test.ts: the report's layout (two identical regions 0.105 s apart, the second dragged 1.375 s left) for each of the four kinds. It lands flush, shows as one pill, andresolveRegionIdsreaches only the dragged half.agent-tools.test.ts:setZoomandsetSpeedaimed into a look-alike are refused.npx tsc --noEmit,npx tsc -p tsconfig.test.json --noEmit, lint, and thesrc/lib/ai-edition,src/components/ai-edition,electron/ai-editionsuites pass.🤖 Generated with Claude Code