Skip to content

Commit 042ec2e

Browse files
fix(studio): restore overscan and publish the zoom viewport (#5151)
* fix(studio): restore the timeline overscan budget * test(studio): align window witnesses with the restored budget * fix(studio): publish the viewport when a zoom anchor lands * fix(studio): publish the viewport when a zoom-out ends at its laid-out scale An eased zoom-out that ends at the scale already laid out only moves the scroll position, so the timeline drew from the old position until the next scroll event. The zoom input now publishes that scroll through the same callback the zoom anchor uses. * fix(studio): a pan at the laid-out scale publishes its scroll before paint The same-scale ending of a zoom ran outside React, so its scroll publish rendered on the next task, after the browser painted the old window. It now flushes like the scale change beside it, and its test pans at the laid-out scale instead of changing it. * test(studio): a same-scale pan must render its scroll before paint Reads the render window outside act, so unwrapping the flush fails it, and records each published scroll at call time instead of reading the live element afterwards.
1 parent 4a335aa commit 042ec2e

14 files changed

Lines changed: 197 additions & 20 deletions

‎.github/workflows/ci.yml‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,10 +85,20 @@ jobs:
8585
- name: Reject accidental file deletions
8686
if: github.event_name == 'pull_request'
8787
run: node scripts/check-no-main-deletions.mjs --base origin/main
88+
# HEAD is the PR merge ref; its first parent is the current base tip, unlike the payload's
89+
# base.sha, which goes stale and would count the base's newer commits as the PR's changes.
90+
- name: Find the pull request's current base
91+
id: base
92+
if: github.event_name == 'pull_request'
93+
run: |
94+
git rev-parse --verify -q HEAD^2 > /dev/null || { echo "::error::HEAD is not the pull request merge commit"; exit 1; }
95+
sha="$(git rev-parse --verify HEAD^1)"
96+
echo "sha=$sha" >> "$GITHUB_OUTPUT"
8897
- uses: dorny/paths-filter@ceb8a2b8f2d89434be7ff52d3de7ec3738c5cc9d # v4
8998
id: filter
9099
with:
91100
token: ""
101+
base: ${{ steps.base.outputs.sha }}
92102
filters: |
93103
catalog_index:
94104
- "registry/**"

‎.github/workflows/player-perf.yml‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,20 @@ jobs:
3030
- uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4
3131
with:
3232
fetch-depth: 0
33+
# HEAD is the PR merge ref; its first parent is the current base tip, unlike the payload's
34+
# base.sha, which goes stale and would count the base's newer commits as the PR's changes.
35+
- name: Find the pull request's current base
36+
id: base
37+
if: github.event_name == 'pull_request'
38+
run: |
39+
git rev-parse --verify -q HEAD^2 > /dev/null || { echo "::error::HEAD is not the pull request merge commit"; exit 1; }
40+
sha="$(git rev-parse --verify HEAD^1)"
41+
echo "sha=$sha" >> "$GITHUB_OUTPUT"
3342
- uses: dorny/paths-filter@ceb8a2b8f2d89434be7ff52d3de7ec3738c5cc9d # v4
3443
id: filter
3544
with:
3645
token: ""
46+
base: ${{ steps.base.outputs.sha }}
3747
filters: |
3848
perf:
3949
- "packages/player/**"

‎.github/workflows/preview-regression.yml‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,20 @@ jobs:
2929
- uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4
3030
with:
3131
fetch-depth: 0
32+
# HEAD is the PR merge ref; its first parent is the current base tip, unlike the payload's
33+
# base.sha, which goes stale and would count the base's newer commits as the PR's changes.
34+
- name: Find the pull request's current base
35+
id: base
36+
if: github.event_name == 'pull_request'
37+
run: |
38+
git rev-parse --verify -q HEAD^2 > /dev/null || { echo "::error::HEAD is not the pull request merge commit"; exit 1; }
39+
sha="$(git rev-parse --verify HEAD^1)"
40+
echo "sha=$sha" >> "$GITHUB_OUTPUT"
3241
- uses: dorny/paths-filter@ceb8a2b8f2d89434be7ff52d3de7ec3738c5cc9d # v4
3342
id: filter
3443
with:
3544
token: ""
45+
base: ${{ steps.base.outputs.sha }}
3646
filters: |
3747
preview:
3848
- "packages/core/**"

‎.github/workflows/regression.yml‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,11 +50,21 @@ jobs:
5050
- uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4
5151
with:
5252
fetch-depth: 0
53+
# HEAD is the PR merge ref; its first parent is the current base tip, unlike the payload's
54+
# base.sha, which goes stale and would count the base's newer commits as the PR's changes.
55+
- name: Find the pull request's current base
56+
id: base
57+
if: github.event_name == 'pull_request'
58+
run: |
59+
git rev-parse --verify -q HEAD^2 > /dev/null || { echo "::error::HEAD is not the pull request merge commit"; exit 1; }
60+
sha="$(git rev-parse --verify HEAD^1)"
61+
echo "sha=$sha" >> "$GITHUB_OUTPUT"
5362
- uses: dorny/paths-filter@ceb8a2b8f2d89434be7ff52d3de7ec3738c5cc9d # v4
5463
id: filter
5564
if: github.event_name != 'schedule'
5665
with:
5766
token: ""
67+
base: ${{ steps.base.outputs.sha }}
5868
filters: |
5969
code:
6070
- "packages/core/**"

‎.github/workflows/windows-render.yml‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,10 +53,20 @@ jobs:
5353
- uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4
5454
with:
5555
fetch-depth: 0
56+
# HEAD is the PR merge ref; its first parent is the current base tip, unlike the payload's
57+
# base.sha, which goes stale and would count the base's newer commits as the PR's changes.
58+
- name: Find the pull request's current base
59+
id: base
60+
if: github.event_name == 'pull_request'
61+
run: |
62+
git rev-parse --verify -q HEAD^2 > /dev/null || { echo "::error::HEAD is not the pull request merge commit"; exit 1; }
63+
sha="$(git rev-parse --verify HEAD^1)"
64+
echo "sha=$sha" >> "$GITHUB_OUTPUT"
5665
- uses: dorny/paths-filter@ceb8a2b8f2d89434be7ff52d3de7ec3738c5cc9d # v4
5766
id: filter
5867
with:
5968
token: ""
69+
base: ${{ steps.base.outputs.sha }}
6070
# A file counts only if it matches every pattern. Player and Studio `src/` run in a browser
6171
# and no code reads a package README, so a diff confined to them skips Windows.
6272
predicate-quantifier: every

‎packages/studio/src/components/TimelineToolbar.test.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,7 @@ describe("TimelineToolbar Fit", () => {
271271
timelineFitPps: 10,
272272
timelinePps: 10,
273273
});
274-
return registerTimelineZoomViewport({ scroll, contentOrigin: 32 });
274+
return registerTimelineZoomViewport({ scroll, contentOrigin: 32, publishScroll: () => {} });
275275
}
276276

277277
it("moves the slider and its readout with a zoom while it is previewed", () => {

‎packages/studio/src/player/components/timelineLayout.test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,13 +27,13 @@ import { resolveInsertRow } from "./timelineCollision";
2727
import { getTimelineRenderTimeRange } from "./timelineViewportGeometry";
2828

2929
describe("horizontal timeline window", () => {
30-
it("adds the shared half-viewport overscan on each side and clamps to duration", () => {
30+
it("adds the shared quarter-viewport overscan on each side and clamps to duration", () => {
3131
expect(getTimelineRenderTimeRange({ scrollLeft: 300, clientWidth: 500 }, 100, 200, 20)).toEqual(
32-
{ start: 0, end: 8.5 },
32+
{ start: 0, end: 7.25 },
3333
);
3434
expect(
3535
getTimelineRenderTimeRange({ scrollLeft: 1_900, clientWidth: 500 }, 100, 200, 20),
36-
).toEqual({ start: 14.5, end: 20 });
36+
).toEqual({ start: 15.75, end: 20 });
3737
});
3838

3939
it("generates globally aligned ticks directly inside the bounded window", () => {

‎packages/studio/src/player/components/timelineZoomInput.test.ts‎

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ afterEach(() => {
4949
});
5050

5151
let unregisterViewport = () => {};
52+
const publishScroll = vi.fn();
5253

5354
/** A 1080px timeline viewport with 32px of track headers, holding one scaled row. */
5455
function viewport(scrollLeft = 0, scrollWidth = 20_000) {
@@ -60,7 +61,7 @@ function viewport(scrollLeft = 0, scrollWidth = 20_000) {
6061
});
6162
const row = scroll.appendChild(document.createElement("div"));
6263
row.setAttribute("data-timeline-zoom-scale", "");
63-
unregisterViewport = registerTimelineZoomViewport({ scroll, contentOrigin: 32 });
64+
unregisterViewport = registerTimelineZoomViewport({ scroll, contentOrigin: 32, publishScroll });
6465
return { scroll, row };
6566
}
6667

@@ -120,7 +121,7 @@ describe("requestTimelineZoom", () => {
120121
it("lays a zoom-out about the left edge out at once, before it shows unmounted time", () => {
121122
usePlayerStore.setState({ duration: 1000 });
122123
viewport();
123-
// Mounted to (1080 - 32 + 540) / 10 = 158.8 s; at 5 px/s the view reaches 209.6 s.
124+
// Mounted to (1080 - 32 + 270) / 10 = 131.8 s; at 5 px/s the view reaches 209.6 s.
124125
requestTimelineZoom(50, { time: 0, x: 32 });
125126
vi.advanceTimersToNextFrame();
126127
expect(usePlayerStore.getState().timelinePps).toBe(5);
@@ -134,14 +135,14 @@ describe("requestTimelineZoom", () => {
134135
timelinePps: 100,
135136
});
136137
viewport(5000);
137-
// Mounted 44.28..65.88 s; at 60 px/s about 55.24 s the view shows 46.5..63.97 s.
138-
requestTimelineZoom(600, { time: 55.24, x: 556 });
138+
// Mounted 46.98..63.18 s; at 70 px/s about 55.24 s the view shows 47.75..62.73 s.
139+
requestTimelineZoom(700, { time: 55.24, x: 556 });
139140
vi.advanceTimersToNextFrame();
140141
expect(usePlayerStore.getState().timelinePps).toBe(100);
141142
});
142143

143144
it("lays out a zoom-out before it shows past the window ruler ticks are drawn in", () => {
144-
// 50 s of clips in content 1996 s wide: ticks are drawn to 157 s, a view and a half in.
145+
// 50 s of clips in content 1996 s wide: ticks are drawn to 131.8 s, a view and a quarter in.
145146
usePlayerStore.setState({ duration: 50 });
146147
viewport();
147148
requestTimelineZoom(60, { time: 0, x: 32 });
@@ -230,7 +231,7 @@ describe("requestTimelineZoom", () => {
230231
const { scroll } = viewport();
231232
requestTimelineZoom(150);
232233
unregisterViewport();
233-
unregisterViewport = registerTimelineZoomViewport({ scroll, contentOrigin: 32 });
234+
unregisterViewport = registerTimelineZoomViewport({ scroll, contentOrigin: 32, publishScroll });
234235
await Promise.resolve();
235236
vi.advanceTimersByTime(200);
236237
expect(usePlayerStore.getState().timelinePps).toBe(15);
@@ -240,6 +241,7 @@ describe("requestTimelineZoom", () => {
240241
const unregisterOlder = registerTimelineZoomViewport({
241242
scroll: document.createElement("div"),
242243
contentOrigin: 32,
244+
publishScroll,
243245
});
244246
viewport();
245247
unregisterOlder();
@@ -258,6 +260,25 @@ describe("requestTimelineZoom", () => {
258260
});
259261

260262
describe("zoomTimelineToRange", () => {
263+
it("tells the timeline where a pan at the laid-out scale scrolled to", () => {
264+
usePlayerStore.setState({
265+
duration: 1000,
266+
zoomMode: "manual",
267+
manualZoomPercent: 1000,
268+
timelineFitPps: 10,
269+
timelinePps: 100,
270+
});
271+
const { scroll } = viewport(0);
272+
const published: number[] = [];
273+
publishScroll.mockImplementation((el: HTMLDivElement) => published.push(el.scrollLeft));
274+
void zoomTimelineToRange(60, 70);
275+
for (let i = 0; i < 40; i++) vi.advanceTimersToNextFrame();
276+
publishScroll.mockReset();
277+
expect(usePlayerStore.getState().timelinePps).toBe(100);
278+
expect(scroll.scrollLeft).toBeGreaterThan(5000);
279+
expect(published.at(-1)).toBe(scroll.scrollLeft);
280+
});
281+
261282
it("fills the width with the range and puts its start at the left margin", () => {
262283
viewport();
263284
void zoomTimelineToRange(40, 90, { smooth: false });

‎packages/studio/src/player/components/timelineZoomInput.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ export interface TimelineZoomAnchor {
2121
export interface TimelineZoomViewport {
2222
scroll: HTMLDivElement;
2323
contentOrigin: number;
24+
publishScroll: (scroll: HTMLDivElement) => void;
2425
}
2526

2627
/** How an eased zoom ended: it reached its range, or a person's zoom or the caller stopped it. */
@@ -209,7 +210,10 @@ function commitPreview() {
209210
);
210211
}
211212
preview = null;
212-
if (Math.abs(view.scroll.scrollLeft - left) >= 0.5) view.scroll.scrollLeft = left;
213+
if (Math.abs(view.scroll.scrollLeft - left) >= 0.5) {
214+
view.scroll.scrollLeft = left;
215+
flushSync(() => view.publishScroll(view.scroll));
216+
}
213217
clearScaled(view.scroll);
214218
emitPreview();
215219
}

‎packages/studio/src/player/components/useTimelinePlayhead.test.tsx‎

Lines changed: 90 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,23 @@
11
// @vitest-environment happy-dom
22

3-
import { act, useRef } from "react";
3+
import { act, useLayoutEffect, useRef } from "react";
44
import { createRoot, type Root } from "react-dom/client";
55
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
66
import { liveTime, usePlayerStore, type ZoomMode } from "../store/playerStore";
77
import { useTimelinePlayhead } from "./useTimelinePlayhead";
8+
import { useTimelineScrollViewport } from "./useTimelineScrollViewport";
9+
import { useTimelineClipRenderWindow } from "./useTimelineClipRenderWindow";
10+
import { requestTimelineZoom, settleTimelineZoom } from "./timelineZoomInput";
811

912
Object.assign(globalThis, { IS_REACT_ACT_ENVIRONMENT: true });
1013

1114
const ORIGIN = 32;
1215

13-
function scrollBox(scrollLeft: number) {
16+
function scrollBox(scrollLeft: number, clientWidth = 800) {
1417
const el = document.createElement("div");
1518
let left = scrollLeft;
1619
Object.defineProperties(el, {
17-
clientWidth: { value: 800 },
20+
clientWidth: { value: clientWidth },
1821
scrollWidth: { value: 20_000 },
1922
scrollLeft: { get: () => left, set: (v: number) => (left = v) },
2023
});
@@ -27,16 +30,24 @@ interface HarnessProps {
2730
scroll: HTMLDivElement;
2831
dragging?: boolean;
2932
zoomMode?: ZoomMode;
33+
syncScrollViewport?: (scroll: HTMLDivElement) => void;
3034
}
3135

32-
function Harness({ pps: fixedPps, scroll, dragging = false, zoomMode = "manual" }: HarnessProps) {
36+
function Harness({
37+
pps: fixedPps,
38+
scroll,
39+
dragging = false,
40+
zoomMode = "manual",
41+
syncScrollViewport = () => {},
42+
}: HarnessProps) {
3343
const storePps = usePlayerStore((s) => s.timelinePps);
3444
const pps = fixedPps ?? storePps;
3545
const scrollRef = useRef(scroll);
3646
const durationRef = useRef(60);
3747
useTimelinePlayhead({
3848
playheadRef: { current: document.createElement("div") },
3949
scrollRef,
50+
syncScrollViewport,
4051
ppsRef: { current: pps },
4152
durationRef,
4253
isDragging: { current: dragging },
@@ -238,6 +249,81 @@ describe("useTimelinePlayhead follow while paused", () => {
238249
});
239250
});
240251

252+
describe("useTimelinePlayhead committed viewport", () => {
253+
function mountViewport(scrollLeft: number) {
254+
vi.stubGlobal(
255+
"ResizeObserver",
256+
class {
257+
observe() {}
258+
disconnect() {}
259+
},
260+
);
261+
usePlayerStore.setState({
262+
zoomMode: "manual",
263+
manualZoomPercent: 1000,
264+
timelineFitPps: 10,
265+
timelinePps: 100,
266+
});
267+
const scroll = scrollBox(scrollLeft, 1080);
268+
const container = document.createElement("div");
269+
function Probe() {
270+
const pps = usePlayerStore((s) => s.timelinePps);
271+
const { viewport, setScrollRef, syncScrollViewport } = useTimelineScrollViewport(
272+
useRef(scroll),
273+
[],
274+
);
275+
useLayoutEffect(() => setScrollRef(scroll), [setScrollRef]);
276+
const { renderTimeRange } = useTimelineClipRenderWindow({
277+
tracks: [],
278+
viewport,
279+
pixelsPerSecond: pps,
280+
contentOrigin: ORIGIN,
281+
duration: 100,
282+
});
283+
return (
284+
<>
285+
<Harness scroll={scroll} syncScrollViewport={syncScrollViewport} />
286+
{renderTimeRange.start <= 59 && renderTimeRange.end >= 59 && <span data-clip="59" />}
287+
<output>{viewport.scrollLeft}</output>
288+
</>
289+
);
290+
}
291+
const root = createRoot(container);
292+
roots.push(root);
293+
act(() => root.render(<Probe />));
294+
return { scroll, container };
295+
}
296+
297+
afterEach(() => vi.unstubAllGlobals());
298+
299+
it("keeps the visible 59-second clip mounted when a pointer zoom commits", () => {
300+
const { scroll, container } = mountViewport(5000);
301+
expect(container.querySelector('[data-clip="59"]')).not.toBeNull();
302+
act(() => {
303+
requestTimelineZoom(1090, { time: 55.24, x: 556 });
304+
settleTimelineZoom();
305+
});
306+
expect(scroll.scrollLeft).toBeCloseTo(5497.16);
307+
expect(container.querySelector('[data-clip="59"]')).not.toBeNull();
308+
expect(Number(container.querySelector("output")?.textContent)).toBeCloseTo(5497.16);
309+
});
310+
311+
it("renders a pan at the laid-out scale before the browser paints", () => {
312+
const { scroll, container } = mountViewport(0);
313+
const actEnvironment = globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean };
314+
const wasActEnvironment = actEnvironment.IS_REACT_ACT_ENVIRONMENT;
315+
actEnvironment.IS_REACT_ACT_ENVIRONMENT = false;
316+
try {
317+
requestTimelineZoom(1000, { time: 60, x: ORIGIN });
318+
settleTimelineZoom();
319+
expect(scroll.scrollLeft).toBe(6000);
320+
expect(container.querySelector("output")?.textContent).toBe("6000");
321+
} finally {
322+
actEnvironment.IS_REACT_ACT_ENVIRONMENT = wasActEnvironment;
323+
}
324+
});
325+
});
326+
241327
describe("useTimelinePlayhead wheel zoom", () => {
242328
beforeEach(() => {
243329
vi.useFakeTimers({

0 commit comments

Comments
 (0)