Skip to content

Commit aa8d3bb

Browse files
committed
fix(timeline): size pills and clips in px, not in fractions of the timeline
Reported on a 30-minute recording: agent-placed pills render far wider than the effect they stand for, and the mismatch changes with the zoom. Three screen-space rules were written as a fraction of `total`, which is a DURATION in disguise — it scales with the recording, so what looked right on a one-minute clip was nonsense on a podcast: - `Math.max(1.5, pctOf(dur))` floored every pill at 1.5% of the timeline: 27 s on a 30-minute project, 58 s on the 65-minute one this was measured against, at every zoom level. Touching pills merged into one block and no pill could be read as a duration. - `total * 0.012` as the drag snap radius was a 21-second magnet, so an edge jumped to a clip boundary it was nowhere near — the more so the longer the recording, and the worse the further you zoomed in to place it precisely. - `.tlClips`'s flex `gap: 6px` was a fixed pixel amount inserted into a proportional layout: each junction pushed what followed 6px right while every clip shrank to pay for it, so a clip's left edge missed its own start time (+2px on clip 2, +6px on clip 3 of a three-clip timeline) while the pills and ruler above it sat at the true position. Constant in px means it was worth 5 s and 15 s of a 30-minute recording zoomed out, and a fraction of a second zoomed in — that changing ratio is what reads as "the pills move when I zoom". Everything on the canvas is now positioned by `pctOf` (clips included, now absolutely positioned instead of flex), and everything that must be a fixed SCREEN size goes through `pxPerSec`: `pillAffordance`, `PILL_SNAP_PX`, `CLIP_GUTTER_PX`. The only floor left on a width is 1px, in CSS. Handles follow from the width rather than fighting it. Above 18px (two 6px handles + a grabbable body) they sit inside the pill as before; below it they mount outside, with the gap on each side belonging to the pill's own hit strip — so a 1px pill still offers ~9px to move and 6px per side to resize, at every zoom. Nothing becomes unreachable at any size. The chrome re-flows mid-drag without disturbing the gesture: deltas come from the pointer and the listeners live on `window`. The flat 0.2 s minimum region is gone too (it refused the last fifth of a second however far you zoomed in); the floor is now the storage grid, 1 ms, since how SHORT a region may be is a data question and how PRECISELY you can aim at one is the zoom's business. Measured in the app on the reported project (65 min, 15 agent-placed trims): zoomed out each trim was drawn at 22.4px (58.5 s) and now sits at its true 0.45–6.94 s; zoomed 30x the old floor would have been 672px. Clip 2 now starts at exactly 50% of the canvas instead of +3px. Every test in V4Timeline.geometry.test.tsx was ablated — reverting each constant turns the matching one red.
1 parent ada1a0d commit aa8d3bb

4 files changed

Lines changed: 395 additions & 27 deletions

File tree

src/components/ai-edition/v4/EditorShellV4.module.css

Lines changed: 47 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1330,6 +1330,11 @@
13301330
position: absolute;
13311331
top: 1px;
13321332
height: 22px;
1333+
/* The ONLY floor on a pill's width, and it is in px so it is the same hairline
1334+
at every zoom: the box's width is the effect's duration, full stop. A `%`
1335+
minimum is a duration in disguise (the old 1.5% drew everything shorter than
1336+
27 s as 27 s on a 30-minute timeline). */
1337+
min-width: 1px;
13331338
display: inline-flex;
13341339
align-items: center;
13351340
gap: 5px;
@@ -1352,6 +1357,27 @@
13521357
.lanePillSel {
13531358
box-shadow: 0 0 0 3px var(--accent-ring);
13541359
}
1360+
/* Narrower than its own chrome (PILL_HANDLES_MIN_PX): the handles mount OUTSIDE
1361+
the box instead of inside it, so overflow must not clip them away — there is
1362+
no content to clip at this width anyway (see pillAffordance/roomForLabel).
1363+
::after widens the move target to reach the handles (PILL_MOVE_GAP_PX on each
1364+
side), which is what keeps a 1px pill grabbable without inflating the bar the
1365+
user is reading a duration off. */
1366+
.lanePillCompact {
1367+
overflow: visible;
1368+
padding: 0;
1369+
}
1370+
.lanePillCompact::after {
1371+
content: "";
1372+
position: absolute;
1373+
inset: 0 -4px;
1374+
}
1375+
/* An outside handle sits on bare lane background, where a transparent grab strip
1376+
is undiscoverable — on hover it shows itself as a bar flanking the pill. */
1377+
.lanePillCompact:hover .lanePillHandle {
1378+
background: color-mix(in oklch, currentColor 35%, transparent);
1379+
border-radius: 2px;
1380+
}
13551381
.lanePillLabel {
13561382
overflow: hidden;
13571383
text-overflow: ellipsis;
@@ -1378,8 +1404,14 @@
13781404
}
13791405
.tlClips {
13801406
position: relative;
1381-
display: flex;
1382-
gap: 6px;
1407+
/* NOT a flex row. Clips are absolutely positioned by percentage of the
1408+
timeline, like the pills, the ruler and the playhead above them.
1409+
A flex row's `gap` is a fixed pixel amount inserted into a proportional
1410+
layout: each junction pushed what followed 6px right while every clip
1411+
shrank proportionally to pay for the gaps, so a clip's left edge landed off
1412+
its true start time — measured at +2px and +6px for clips 2 and 3 of a
1413+
three-clip timeline, which is 5 s and 15 s of a 30-minute recording when
1414+
zoomed out, and a fraction of a second when zoomed in. See .tlClip. */
13831415
height: 66px;
13841416
width: 100%;
13851417
padding: 0;
@@ -1398,9 +1430,19 @@
13981430
pointer-events: none;
13991431
}
14001432
.tlClip {
1401-
position: relative;
1402-
flex: 1 0 0;
1403-
min-width: 0;
1433+
/* left/width come from V4Timeline, in percent of the timeline. The 6px gutter
1434+
that used to be a flex `gap` is now taken off each clip's own width, so it
1435+
separates the cards without ever moving the next one: a clip's LEFT edge is
1436+
its true start time at every zoom and every clip count.
1437+
ponytail: the right edge therefore reads 6px short. Constant, non-cumulative
1438+
and below the width of the border it sits next to; draw the separator inside
1439+
the box (inset shadow, square-butted cards) if that ever needs to be exact. */
1440+
position: absolute;
1441+
top: 0;
1442+
bottom: 0;
1443+
/* px, like .lanePill's: the gutter is subtracted from the clip's width, so a
1444+
clip narrower than 6px would otherwise compute to nothing. */
1445+
min-width: 1px;
14041446
border-radius: 11px;
14051447
border: 1.5px solid var(--border);
14061448
background: var(--surface-1);
Lines changed: 227 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,227 @@
1+
import "@testing-library/jest-dom";
2+
import { fireEvent, render, screen } from "@testing-library/react";
3+
import { beforeAll, describe, expect, it, vi } from "vitest";
4+
5+
// The regression under test is geometric, so the environment has to have a size:
6+
// jsdom reports 0 for every box, which would leave `pxPerSec` at 0 (the
7+
// "unmeasured" case) and hide exactly the thing being checked.
8+
const VIEWPORT_PX = 900;
9+
const TOTAL_SEC = 1800; // a 30-minute recording, as in the report
10+
11+
vi.mock("@/contexts/I18nContext", () => ({
12+
useScopedT: () => (key: string) => key,
13+
}));
14+
vi.mock("sonner", () => ({ toast: { error: vi.fn(), info: vi.fn(), success: vi.fn() } }));
15+
16+
import { V4Timeline } from "./V4Timeline";
17+
18+
beforeAll(() => {
19+
globalThis.ResizeObserver = class {
20+
// jsdom has none, and the width it would report is stubbed below anyway.
21+
observe() {
22+
/* noop */
23+
}
24+
unobserve() {
25+
/* noop */
26+
}
27+
disconnect() {
28+
/* noop */
29+
}
30+
} as unknown as typeof ResizeObserver;
31+
Object.defineProperty(HTMLElement.prototype, "clientWidth", {
32+
configurable: true,
33+
get: () => VIEWPORT_PX,
34+
});
35+
Object.defineProperty(HTMLElement.prototype, "getBoundingClientRect", {
36+
configurable: true,
37+
value: () => ({
38+
x: 0,
39+
y: 0,
40+
left: 0,
41+
top: 0,
42+
right: VIEWPORT_PX,
43+
bottom: 100,
44+
width: VIEWPORT_PX,
45+
height: 100,
46+
toJSON() {
47+
/* unused by the component */
48+
},
49+
}),
50+
});
51+
});
52+
53+
function clip(startSec: number, endSec: number) {
54+
return {
55+
id: `c@${startSec}`,
56+
assetId: "a1",
57+
timelineStartSec: startSec,
58+
timelineEndSec: endSec,
59+
sourceStartSec: 0,
60+
sourceEndSec: endSec - startSec,
61+
};
62+
}
63+
64+
/** By default one 30-minute clip carrying a single one-second annotation. */
65+
function renderTimeline(
66+
clips = [clip(0, TOTAL_SEC)],
67+
annotation = { id: "ann1", startMs: 10_000, endMs: 11_000 },
68+
) {
69+
const tl = {
70+
clips,
71+
assets: [{ id: "a1", label: "rec", durationSec: TOTAL_SEC }],
72+
annotationRegions: [annotation],
73+
speedRegions: [],
74+
cameraFullscreenRegions: [],
75+
zoomRegions: [],
76+
trimRanges: [],
77+
selection: null,
78+
multiSelection: [],
79+
clipSelection: null,
80+
clearSelection: vi.fn(),
81+
selectRegion: vi.fn(),
82+
selectClip: vi.fn(),
83+
updateAnnotationSpan: vi.fn(async () => {
84+
/* the drag only awaits it */
85+
}),
86+
};
87+
render(
88+
<V4Timeline
89+
// biome-ignore lint/suspicious/noExplicitAny: a partial timeline API is enough to draw pills
90+
tl={tl as any}
91+
setCurrentTime={vi.fn()}
92+
playing={false}
93+
onTogglePlay={vi.fn()}
94+
onPrevClip={vi.fn()}
95+
onNextClip={vi.fn()}
96+
onEditClip={vi.fn()}
97+
/>,
98+
);
99+
return {
100+
pill: screen.getByTitle("toolbar.newAnnotation"),
101+
clipEls: Array.from(document.querySelectorAll<HTMLElement>("[data-clip-id]")),
102+
tl,
103+
};
104+
}
105+
106+
/** Drag a handle by `dxPx`. The move/up listeners live on `window`, so the drag
107+
* is driven by pointer deltas alone — the handle may re-mount under it. */
108+
function dragHandle(handle: Element, dxPx: number) {
109+
fireEvent.pointerDown(handle, { clientX: 0 });
110+
window.dispatchEvent(new MouseEvent("pointermove", { clientX: dxPx }));
111+
window.dispatchEvent(new MouseEvent("pointerup", { clientX: dxPx }));
112+
}
113+
114+
/** Ctrl+wheel up = zoom in; the handler is a native listener, so dispatch real events. */
115+
function zoomIn(notches: number) {
116+
const canvas = document.querySelector("[class*=tlTracks]") as HTMLElement;
117+
for (let i = 0; i < notches; i++) {
118+
fireEvent.wheel(canvas, { ctrlKey: true, deltaY: -100, clientX: 0 });
119+
}
120+
}
121+
122+
describe("V4Timeline lane pills", () => {
123+
it("draws a pill exactly as wide as its region, at any zoom", () => {
124+
// 1 s of 1800 s. The old `Math.max(1.5, …)` floor drew this as 1.5% — 27
125+
// seconds of ruler for a one-second annotation — and did it at every zoom,
126+
// since the floor was a percentage of the timeline rather than of the screen.
127+
const { pill } = renderTimeline();
128+
const expected = (1 / TOTAL_SEC) * 100;
129+
expect(Number.parseFloat(pill.style.width)).toBeCloseTo(expected, 6);
130+
131+
// The canvas is what scales with zoom, so the pill's share of it must not
132+
// move at all — only the chrome inside it may react (below).
133+
zoomIn(40);
134+
expect(Number.parseFloat(pill.style.width)).toBeCloseTo(expected, 6);
135+
});
136+
137+
it("keeps both resize handles reachable when the pill is thinner than they are", () => {
138+
// 0.5 px wide at this zoom: the handles cannot sit inside the box without
139+
// swallowing it whole, so they mount outside it and the body stays a move
140+
// target. Resizing a hairline stays possible — it is the pointer precision
141+
// that is coarse there, not the affordance that is missing.
142+
const { pill } = renderTimeline();
143+
const [left, right] = Array.from(pill.querySelectorAll("span"));
144+
expect(left.style.left).toBe("-10px");
145+
expect(right.style.right).toBe("-10px");
146+
// Nothing legible fits, so no icon/label is rendered (the title attribute
147+
// still carries the value on hover).
148+
expect(pill.textContent).toBe("");
149+
150+
// Zoomed to the 50× ceiling the same second is 25 px wide and hosts its own
151+
// chrome again.
152+
zoomIn(40);
153+
expect(left.style.left).toBe("0px");
154+
expect(right.style.right).toBe("0px");
155+
});
156+
157+
it("grows and shrinks a hairline pill from its outside handles", () => {
158+
// Growing is unbounded by the pill's own size: 90 px right of a 900 px canvas
159+
// is a tenth of the 1800 s timeline, so the 10–11 s annotation ends at 191 s.
160+
// The chrome re-flows inside the box as it crosses PILL_HANDLES_MIN_PX
161+
// mid-drag, which the gesture never notices — the deltas come from the
162+
// pointer and the listeners live on `window`, not on the handle.
163+
const { pill, tl } = renderTimeline();
164+
const [left, right] = Array.from(pill.querySelectorAll("span"));
165+
dragHandle(right, 90);
166+
expect(tl.updateAnnotationSpan).toHaveBeenCalledWith("ann1", 10_000, 191_000);
167+
168+
// Shrinking stops at the storage grid (1 ms), not at the old flat 200 ms
169+
// floor that refused the last fifth of a second however far you zoomed in.
170+
dragHandle(left, 90_000);
171+
expect(tl.updateAnnotationSpan).toHaveBeenLastCalledWith("ann1", 10_999, 11_000);
172+
173+
// 18 s short of the timeline end: 9 px away on screen, so it stays where it
174+
// was dropped. The snap radius used to be 1.2% of the timeline — a 21-second
175+
// magnet here — which is what made a grown edge jump to a clip boundary it
176+
// was nowhere near, the more so the longer the recording.
177+
dragHandle(right, 885.5);
178+
expect(tl.updateAnnotationSpan).toHaveBeenLastCalledWith("ann1", 10_000, 1_782_000);
179+
});
180+
});
181+
182+
describe("V4Timeline clip row", () => {
183+
// Three clips = two junctions. As a flex row with `gap: 6px`, each junction
184+
// added 6px while every clip shrank proportionally to pay for it, so a clip's
185+
// left edge missed its true start: measured in a browser on this very fixture,
186+
// clip 2 by +2px and clip 3 by +6px, while the pills and ruler above them sat
187+
// at the true position. Being a fixed px error in a proportional layout, it was
188+
// worth 5 s and 15 s of timeline zoomed out but a fraction of a second zoomed
189+
// in — which is what reads as "the pills move when I zoom".
190+
const CLIPS = [clip(0, 600), clip(600, 900), clip(900, TOTAL_SEC)];
191+
const startsAt = (sec: number) => `${(sec / TOTAL_SEC) * 100}%`;
192+
193+
it("anchors every clip to its own start time, and keeps it there under zoom", () => {
194+
// The annotation starts exactly where the second clip does, so the pill and
195+
// the clip edge under it must resolve to the very same coordinate.
196+
const { clipEls, pill } = renderTimeline(CLIPS, {
197+
id: "ann1",
198+
startMs: 600_000,
199+
endMs: 601_000,
200+
});
201+
expect(clipEls.map((el) => el.style.left)).toEqual([startsAt(0), startsAt(600), startsAt(900)]);
202+
expect(pill.style.left).toBe(clipEls[1].style.left);
203+
204+
// Zoom scales the canvas these coordinates live in, so the coordinates
205+
// themselves must not move: same values, same agreement with the pill.
206+
zoomIn(40);
207+
expect(clipEls.map((el) => el.style.left)).toEqual([startsAt(0), startsAt(600), startsAt(900)]);
208+
expect(pill.style.left).toBe(clipEls[1].style.left);
209+
});
210+
211+
it("takes the card gutter out of each clip's own width", () => {
212+
// The 6px is what separates two cards. Taken off the clip's width it stays
213+
// local to that clip; inserted between them (a flex gap) it displaced every
214+
// clip that followed. The 1px floor keeps a clip shorter than the gutter
215+
// from collapsing to nothing on a long timeline.
216+
const { clipEls } = renderTimeline(CLIPS);
217+
const widths = clipEls.map((el) => el.style.width);
218+
// (jsdom re-serialises the percentage to 4 decimals, hence the numeric read)
219+
expect(widths.map((w) => w.endsWith("- 6px)"))).toEqual([true, true, true]);
220+
for (const [i, durSec] of [600, 300, 900].entries()) {
221+
expect(Number.parseFloat(widths[i].slice("calc(".length))).toBeCloseTo(
222+
(durSec / TOTAL_SEC) * 100,
223+
3,
224+
);
225+
}
226+
});
227+
});

0 commit comments

Comments
 (0)