Skip to content

Commit c90aaa4

Browse files
committed
fix(timeline): size a keyboard-created region by the zoom too
Reported on rc.8: adding a pill on a 30-minute recording still produced something invisible, hidden behind the playhead it was created at. The previous commit scaled only the toolbar buttons, because the zoom lives in V4Timeline's local `nav` state and the shortcuts are handled in NewEditorShell. That was the wrong line to draw: the empty lanes advertise the shortcut ("Press Z to add zoom") and the buttons are labelled "Add Zoom (Z)", so the keyboard is how most regions actually get created — and it kept the flat 2 s, which is 0.8px at 0.42 px/s. Measured in a browser on the reported case (30 min across a 761px panel): the button asked for 94.6 s, the shortcut for 2 s. The rule now lives in timeline/newRegionDuration, outside the component: the timeline publishes its scale (px per second) and both creation paths read it at the moment the user acts. Module state read imperatively, never subscribed — the value changes on every zoom notch and nothing renders it, so a subscription would re-render the whole editor shell for a number only a keypress reads. Same reasoning as playheadSec() in useTimeline. Reading it at CLICK time rather than at render time also drops a one-notch staleness: the old handler captured the duration computed by the render that preceded the effect publishing the new scale. Verified in a real browser, both paths: 761px / 1800 s → 94.61 s → a pill 40.0px wide, identical for the button and for what the shortcut passes. Unit-tested in newRegionDuration.test.ts, including the invariant that duration × scale is PILL_CREATE_PX at any zoom.
1 parent ef2adab commit c90aaa4

4 files changed

Lines changed: 135 additions & 36 deletions

File tree

src/components/ai-edition/NewEditorShell.tsx

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import {
2323
import { useUndoRedoShortcuts } from "@/lib/ai-edition/store/undo";
2424
import { useSequentialTimelineOps } from "@/lib/ai-edition/store/useSequentialTimelineOps";
2525
import { useTimeline } from "@/lib/ai-edition/store/useTimeline";
26+
import { newRegionDurationSec } from "@/lib/ai-edition/timeline/newRegionDuration";
2627
import { matchesShortcut } from "@/lib/shortcuts";
2728
import { nativeBridgeClient } from "@/native";
2829
import type { AiEditionProjectSummary } from "@/native/contracts";
@@ -911,29 +912,34 @@ export function NewEditorShell() {
911912
deleteSelection();
912913
return;
913914
}
915+
// Same size on screen as the toolbar buttons produce — these shortcuts are
916+
// what the empty lanes advertise ("Press Z to add zoom"), so they are the
917+
// way most regions get created. Left on the flat default they came out
918+
// under two pixels on a 30-minute recording, hidden behind the playhead
919+
// they were created at. See timeline/newRegionDuration.
914920
if (matchesShortcut(e, shortcuts.addZoom, isMac)) {
915921
e.preventDefault();
916-
void tl.addZoom();
922+
void tl.addZoom(newRegionDurationSec());
917923
return;
918924
}
919925
if (matchesShortcut(e, shortcuts.addTrim, isMac)) {
920926
e.preventDefault();
921-
void tl.addTrim();
927+
void tl.addTrim(newRegionDurationSec());
922928
return;
923929
}
924930
if (matchesShortcut(e, shortcuts.addAnnotation, isMac)) {
925931
e.preventDefault();
926-
void tl.addAnnotation();
932+
void tl.addAnnotation(newRegionDurationSec());
927933
return;
928934
}
929935
if (matchesShortcut(e, shortcuts.addSpeed, isMac)) {
930936
e.preventDefault();
931-
void tl.addSpeed();
937+
void tl.addSpeed(newRegionDurationSec());
932938
return;
933939
}
934940
if (matchesShortcut(e, shortcuts.addCameraFullscreen, isMac)) {
935941
e.preventDefault();
936-
void tl.addCameraFullscreen();
942+
void tl.addCameraFullscreen(newRegionDurationSec());
937943
return;
938944
}
939945

src/components/ai-edition/v4/V4Timeline.tsx

Lines changed: 22 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,10 @@ import { useChatPromptBus } from "@/lib/ai-edition/store/useChatPromptBus";
3939
import { useEditorSettings } from "@/lib/ai-edition/store/useEditorSettings";
4040
import type { useTimeline } from "@/lib/ai-edition/store/useTimeline";
4141
import { formatSec } from "@/lib/ai-edition/timeline/format";
42+
import {
43+
newRegionDurationSec,
44+
setTimelineScale,
45+
} from "@/lib/ai-edition/timeline/newRegionDuration";
4246
import { ventilateSpanAcrossClips } from "@/lib/ai-edition/timeline/region-ventilation";
4347
import { coalesceRegionsForRuler } from "@/lib/ai-edition/timeline/timelineMap";
4448
import {
@@ -125,25 +129,9 @@ const PILL_HANDLE_OUT_PX = PILL_HANDLE_PX + PILL_MOVE_GAP_PX;
125129
const PILL_CONTENT_MIN_PX = 34;
126130
/** Edge-snap radius while dragging a pill, in screen px. */
127131
const PILL_SNAP_PX = 8;
128-
/**
129-
* Width a pill created from the toolbar aims for on screen. Its DURATION is
130-
* whatever that width is worth at the current zoom: dezoomed you get a long
131-
* region, zoomed in a short one, and either way a pill you can see, read and
132-
* grab the moment it appears.
133-
*
134-
* A fixed 2 s did the opposite — on a 65-minute timeline zoomed out it is half a
135-
* pixel. It only ever looked usable because the old 1.5%-of-the-timeline minimum
136-
* width inflated it in the RENDERING, which is the lie this branch removed. Same
137-
* intent, honest implementation: the width now comes from the duration actually
138-
* stored, so what you see is what the effect covers.
139-
*
140-
* Sized to clear PILL_CONTENT_MIN_PX, so a new pill shows its icon and label
141-
* immediately with room left to grab either handle.
142-
*/
143-
const PILL_CREATE_PX = 40;
144-
/** Floor on that duration. Only bites past ~30x zoom, where 40px is worth a few
145-
* hundredths of a second and the region would be born unusable. */
146-
const PILL_CREATE_MIN_SEC = 0.25;
132+
// The size a newly created pill aims for (PILL_CREATE_PX) lives in
133+
// timeline/newRegionDuration, because the keyboard shortcuts create regions too
134+
// and they are handled in NewEditorShell, outside this component.
147135
/** Visual separation between two clip cards. Taken off each clip's own width
148136
* (see .tlClip) rather than inserted between them, so it cannot displace the
149137
* clips that follow — which is what a flex `gap` did, once per junction. */
@@ -470,13 +458,13 @@ export function V4Timeline({
470458
// `total`, which is a duration in disguise and so scales with the recording.
471459
const navSpan = Math.max(0.02, nav.end - nav.start);
472460
const pxPerSec = viewportWidthPx / navSpan / total;
473-
// Duration handed to the toolbar's create buttons, so the pill they produce is
474-
// PILL_CREATE_PX wide whatever the zoom. Only THIS path scales: the keyboard
475-
// shortcuts and the agent keep useTimeline's flat default, since neither knows
476-
// the zoom (`nav` is local state here). Before the panel is measured there is
477-
// no zoom to read, so the default stands.
478-
const createDurationSec =
479-
pxPerSec > 0 ? Math.max(PILL_CREATE_MIN_SEC, PILL_CREATE_PX / pxPerSec) : undefined;
461+
// Publish the scale so the keyboard shortcuts (NewEditorShell) size a new
462+
// region exactly like the buttons below do — `nav` never leaves this
463+
// component, so without this they fall back to a flat default and a pill
464+
// created with `Z` comes out invisible on a long recording.
465+
useEffect(() => {
466+
setTimelineScale(pxPerSec);
467+
}, [pxPerSec]);
480468

481469
// ── region lanes ────────────────────────────────────────────────
482470
// zoom/speed/annotation: one pill per row, never coalesced — each carries
@@ -1339,9 +1327,12 @@ export function V4Timeline({
13391327
title={tool.label}
13401328
aria-label={tool.label}
13411329
onClick={() => {
1342-
if (tool.id === "speed") void tl.addSpeed(createDurationSec);
1343-
if (tool.id === "comment") void tl.addAnnotation(createDurationSec);
1344-
if (tool.id === "cut") void tl.addTrim(createDurationSec);
1330+
// Read at CLICK time: a render-time value would be one zoom
1331+
// notch stale when the user zooms and immediately creates.
1332+
const dur = newRegionDurationSec();
1333+
if (tool.id === "speed") void tl.addSpeed(dur);
1334+
if (tool.id === "comment") void tl.addAnnotation(dur);
1335+
if (tool.id === "cut") void tl.addTrim(dur);
13451336
}}
13461337
>
13471338
{tool.icon}
@@ -1352,7 +1343,7 @@ export function V4Timeline({
13521343
className={styles.tlToolBtn}
13531344
title={t("buttons.addZoom")}
13541345
aria-label={t("buttons.addZoom")}
1355-
onClick={() => void tl.addZoom(createDurationSec)}
1346+
onClick={() => void tl.addZoom(newRegionDurationSec())}
13561347
>
13571348
<ZoomIn size={15} />
13581349
</button>
@@ -1375,7 +1366,7 @@ export function V4Timeline({
13751366
className={styles.tlToolBtn}
13761367
title={t("buttons.addCameraFullscreen")}
13771368
aria-label={t("buttons.addCameraFullscreen")}
1378-
onClick={() => void tl.addCameraFullscreen(createDurationSec)}
1369+
onClick={() => void tl.addCameraFullscreen(newRegionDurationSec())}
13791370
>
13801371
<Maximize2 size={15} />
13811372
</button>
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
import { beforeEach, describe, expect, it } from "vitest";
2+
import {
3+
newRegionDurationSec,
4+
PILL_CREATE_MIN_SEC,
5+
PILL_CREATE_PX,
6+
setTimelineScale,
7+
} from "./newRegionDuration";
8+
9+
describe("newRegionDurationSec", () => {
10+
beforeEach(() => setTimelineScale(0));
11+
12+
it("trades duration for a constant width", () => {
13+
// The reported case: 30 minutes across a ~760px panel is 0.42 px/s, where a
14+
// flat 2 s region is under a pixel — invisible behind the playhead it was
15+
// created at. At that scale 40px is worth a minute and a half.
16+
setTimelineScale(760 / 1800);
17+
expect(newRegionDurationSec()).toBeCloseTo(94.7, 1);
18+
19+
// Zoom in 50x and the same gesture creates a region 50x shorter — the pill
20+
// on screen is the same size either way, which is the whole point.
21+
setTimelineScale((760 / 1800) * 50);
22+
expect(newRegionDurationSec()).toBeCloseTo(1.89, 2);
23+
});
24+
25+
it("stays at the floor when the pixels are worth almost nothing", () => {
26+
// 40px of a 3-second clip zoomed to the ceiling: without the floor the
27+
// region would be born a few hundredths of a second long.
28+
setTimelineScale(10_000);
29+
expect(newRegionDurationSec()).toBe(PILL_CREATE_MIN_SEC);
30+
});
31+
32+
it("says nothing at all before the timeline has been measured", () => {
33+
// First paint, or no timeline mounted: callers fall back to their own
34+
// default rather than deriving a length from a width of zero.
35+
expect(newRegionDurationSec()).toBeUndefined();
36+
setTimelineScale(Number.NaN);
37+
expect(newRegionDurationSec()).toBeUndefined();
38+
setTimelineScale(Number.POSITIVE_INFINITY);
39+
expect(newRegionDurationSec()).toBeUndefined();
40+
});
41+
42+
it("keeps the width it promises", () => {
43+
for (const pxPerSec of [0.2, 1, 7.5, 120]) {
44+
setTimelineScale(pxPerSec);
45+
const width = (newRegionDurationSec() as number) * pxPerSec;
46+
// Above the floor the duration is exactly PILL_CREATE_PX worth of time.
47+
expect(width).toBeCloseTo(PILL_CREATE_PX, 6);
48+
}
49+
});
50+
});
Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
/**
2+
* How long a region should be when the USER creates one, so the pill they get is
3+
* always the same comfortable size on screen.
4+
*
5+
* A 30-minute recording has to fit the panel's width, so the default view is
6+
* heavily dezoomed — around 0.4 px per second. A fixed 2 s region is under a
7+
* pixel there: invisible, and hidden behind the very playhead it was created at.
8+
* What has to stay constant is the WIDTH; the duration is whatever that width is
9+
* worth at the current zoom, which is why it can be 95 s zoomed out and 2 s
10+
* zoomed in for the same gesture.
11+
*
12+
* (A flat 2 s only ever looked right because the old 1.5%-of-the-timeline
13+
* minimum pill width inflated it in the RENDERING — the lie removed in #233.)
14+
*
15+
* This lives outside the timeline component because BOTH ways of creating a
16+
* region must agree: the toolbar buttons in V4Timeline and the keyboard
17+
* shortcuts in NewEditorShell, which the empty lanes advertise ("Press Z to add
18+
* zoom") and which have no other access to the zoom — `nav` is local state
19+
* inside V4Timeline. Paths that are not a user placing a pill by hand (the
20+
* agent, auto-zooms) don't call this and keep useTimeline's flat default.
21+
*/
22+
23+
/** On-screen width a freshly created pill aims for. Clears PILL_CONTENT_MIN_PX,
24+
* so it shows its icon and label at once with room to grab either handle. */
25+
export const PILL_CREATE_PX = 40;
26+
/** Floor on the duration. Only bites past ~30x zoom, where 40px is worth a few
27+
* hundredths of a second and the region would be born unusable. */
28+
export const PILL_CREATE_MIN_SEC = 0.25;
29+
30+
/**
31+
* The timeline's current scale, in px per timeline-second.
32+
*
33+
* Module state, written by the one timeline that exists and read IMPERATIVELY at
34+
* the instant a region is created — deliberately not a store subscription. The
35+
* value changes on every zoom notch and nothing renders it, so subscribing would
36+
* re-render the whole editor shell for a number only a click ever reads. Same
37+
* reasoning as `playheadSec()` in useTimeline.
38+
*/
39+
let pxPerSec = 0;
40+
41+
export function setTimelineScale(value: number): void {
42+
pxPerSec = Number.isFinite(value) && value > 0 ? value : 0;
43+
}
44+
45+
/**
46+
* Duration to create a region with, or `undefined` while the timeline has not
47+
* been measured yet (first paint, or no timeline mounted) — callers then fall
48+
* back to their own default rather than inventing a length from a width of zero.
49+
*/
50+
export function newRegionDurationSec(): number | undefined {
51+
return pxPerSec > 0 ? Math.max(PILL_CREATE_MIN_SEC, PILL_CREATE_PX / pxPerSec) : undefined;
52+
}

0 commit comments

Comments
 (0)