From 52f91357a280963b0402f185d12a84959285cc6c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 19:00:12 +0000 Subject: [PATCH] refactor(teleprompter): enforce each invariant in one place A maintenance pass over the view. Nothing changes on screen. The position was bounded twice on the playing path: advance() clamped its result and the loop clamped it again. The loop has to clamp regardless, for nudges and for a document which shrank, so advance() now reports reaching the end and leaves the bound to its only caller. maxScroll was floored at zero at each of four reads, though measure() is its only writer and already floors it. The invariant is stated once, where it is established. Arrival at an eased target was tested twice: easeCatchUp snaps to the target once it is close enough, and hasArrived then measured the same distance again. Equality with the target is the signal, so hasArrived is gone. Speed was clamped to its range by the option parser and then clamped to the identical range again by clampSpeed, on the same call. Only the keys and buttons, which add deltas, still need to clamp. The Enter guard in the key handler could never fire: the keymap does not bind Enter, so the event was already passing through untouched. The reason Enter stays unbound now lives in the keymap, with a test, rather than as a branch which reads as though something was being prevented. Also drops a redundant early return in ReadingLine, an export with no callers outside its module, and a JSON round trip in the param mirror which existed only to satisfy a dependency array. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Cb8RVPNQ2ETPJxdy4b8CHf --- .../src/views/teleprompter/Teleprompter.tsx | 24 ++++++++++++------- .../__tests__/teleprompter.keymap.test.ts | 6 +++++ .../__tests__/teleprompter.scroll.test.ts | 11 +++++---- .../teleprompter/reading-line/ReadingLine.tsx | 4 ---- .../views/teleprompter/teleprompter.keymap.ts | 5 ++++ .../teleprompter/teleprompter.options.ts | 4 ++-- .../views/teleprompter/teleprompter.scroll.ts | 23 +++++++++--------- .../views/teleprompter/useMirrorLiveParams.ts | 18 +++++++++++--- .../teleprompter/useTeleprompterControls.ts | 11 --------- .../teleprompter/useTeleprompterScroll.ts | 21 +++++++++------- 10 files changed, 73 insertions(+), 54 deletions(-) diff --git a/apps/client/src/views/teleprompter/Teleprompter.tsx b/apps/client/src/views/teleprompter/Teleprompter.tsx index 21550787e..77a65a51b 100644 --- a/apps/client/src/views/teleprompter/Teleprompter.tsx +++ b/apps/client/src/views/teleprompter/Teleprompter.tsx @@ -59,15 +59,13 @@ function Teleprompter({ rundown, rundownMetadata, customFields }: TeleprompterDa * frame of the previous orientation paint first, which on a prompter reads as * a flash. The speed option is kept live the same way, in useTeleprompterScroll. */ - const [live, setLive] = useState({ flipH: options.flipH, flipV: options.flipV, fontSize: options.fontSize }); - const [liveFromParams, setLiveFromParams] = useState(live); - if ( - liveFromParams.flipH !== options.flipH || - liveFromParams.flipV !== options.flipV || - liveFromParams.fontSize !== options.fontSize - ) { - const fromParams = { flipH: options.flipH, flipV: options.flipV, fontSize: options.fontSize }; - setLiveFromParams(fromParams); + const fromParams = { flipH: options.flipH, flipV: options.flipV, fontSize: options.fontSize }; + const paramsKey = `${fromParams.flipH}|${fromParams.flipV}|${fromParams.fontSize}`; + + const [live, setLive] = useState(fromParams); + const [seededFrom, setSeededFrom] = useState(paramsKey); + if (seededFrom !== paramsKey) { + setSeededFrom(paramsKey); setLive(fromParams); } @@ -106,6 +104,14 @@ function Teleprompter({ rundown, rundownMetadata, customFields }: TeleprompterDa const handleFontSize = (steps: number) => setLive((current) => ({ ...current, fontSize: stepFontSize(current.fontSize, steps) })); + + /** + * Back to the view's default rather than to the size this client happened to + * open at. Now that the keys write the size into the query there is no longer + * a separate configured value to return to, and remembering one privately + * would mean two people on the same link getting different results from the + * same key. + */ const handleResetFontSize = () => setLive((current) => ({ ...current, fontSize: defaults.fontSize })); const handleToggleHelp = () => setShowHelp((current) => !current); diff --git a/apps/client/src/views/teleprompter/__tests__/teleprompter.keymap.test.ts b/apps/client/src/views/teleprompter/__tests__/teleprompter.keymap.test.ts index 3ef856df4..626c075cd 100644 --- a/apps/client/src/views/teleprompter/__tests__/teleprompter.keymap.test.ts +++ b/apps/client/src/views/teleprompter/__tests__/teleprompter.keymap.test.ts @@ -95,4 +95,10 @@ describe('resolveTeleprompterAction()', () => { test('ignores keys it does not bind', () => { expect(resolveTeleprompterAction(makeEvent({ code: 'KeyQ', key: 'q' }))).toBeNull(); }); + + test('leaves Enter alone, so a focused transport button can still be pressed', () => { + // Space belongs to the prompter wherever focus is, which leaves Enter as the + // only way to work the overlay from the keyboard + expect(resolveTeleprompterAction(makeEvent({ code: 'Enter', key: 'Enter' }))).toBeNull(); + }); }); diff --git a/apps/client/src/views/teleprompter/__tests__/teleprompter.scroll.test.ts b/apps/client/src/views/teleprompter/__tests__/teleprompter.scroll.test.ts index 24cab6d1c..14b69619c 100644 --- a/apps/client/src/views/teleprompter/__tests__/teleprompter.scroll.test.ts +++ b/apps/client/src/views/teleprompter/__tests__/teleprompter.scroll.test.ts @@ -76,16 +76,17 @@ describe('advance()', () => { expect(position).toBeCloseTo((pxPerSecond * 100) / 60, 5); }); - test('clamps at the bounds of the document', () => { - expect(advance(0, -100, 1, 500).position).toBe(0); - expect(advance(490, 100, 1, 500).position).toBe(500); - }); - test('reports the end of the script once the bottom is reached', () => { expect(advance(499, 100, 1, 500).atEnd).toBe(true); expect(advance(100, 100, 1, 500).atEnd).toBe(false); }); + test('reports the end without bounding the position, which the caller owns', () => { + // the loop has to bound the position anyway, for nudges and for a document + // which shrank, so this does not do it a second time + expect(advance(490, 100, 1, 500)).toEqual({ position: 590, atEnd: true }); + }); + test('never reports the end for a document which does not overflow', () => { // it may simply not have been measured yet, and stopping playback on an // unmeasured document would look like the prompter refusing to run diff --git a/apps/client/src/views/teleprompter/reading-line/ReadingLine.tsx b/apps/client/src/views/teleprompter/reading-line/ReadingLine.tsx index e4ecb18f1..fade5de7c 100644 --- a/apps/client/src/views/teleprompter/reading-line/ReadingLine.tsx +++ b/apps/client/src/views/teleprompter/reading-line/ReadingLine.tsx @@ -14,10 +14,6 @@ interface ReadingLineProps { * pointing at a position between two of them. */ export default function ReadingLine({ showReadingLine, dimPast }: ReadingLineProps) { - if (!showReadingLine && !dimPast) { - return null; - } - return ( <> {dimPast &&
} diff --git a/apps/client/src/views/teleprompter/teleprompter.keymap.ts b/apps/client/src/views/teleprompter/teleprompter.keymap.ts index 33b6407f2..8c74352c0 100644 --- a/apps/client/src/views/teleprompter/teleprompter.keymap.ts +++ b/apps/client/src/views/teleprompter/teleprompter.keymap.ts @@ -23,6 +23,11 @@ export type TeleprompterKeyEvent = { * hardware protocol: foot pedals and hand controllers are USB HID devices which * emit these very keystrokes, so matching it is what makes them work here. * + * Enter is deliberately left unbound. Space is the run/stop pedal and has to + * reach the prompter wherever focus happens to be, so the transport overlay + * needs one key which still activates whichever control is focused, and Enter + * is it. Binding it here would take the overlay's keyboard operation away. + * * @returns the action to run, or null when the view should ignore the event */ export function resolveTeleprompterAction(event: TeleprompterKeyEvent): TeleprompterAction | null { diff --git a/apps/client/src/views/teleprompter/teleprompter.options.ts b/apps/client/src/views/teleprompter/teleprompter.options.ts index c03c69212..10ce3820a 100644 --- a/apps/client/src/views/teleprompter/teleprompter.options.ts +++ b/apps/client/src/views/teleprompter/teleprompter.options.ts @@ -7,7 +7,7 @@ import type { ViewOption } from '../../common/components/view-params-editor/view import { makeOptionsFromCustomFields } from '../../common/components/view-params-editor/viewParams.utils'; import { PresetContext } from '../../common/context/PresetContext'; import { isStringBoolean } from '../common/viewUtils'; -import { clampSpeed, DEFAULT_SPEED, MAX_FONT_SIZE, MAX_SPEED, MIN_FONT_SIZE, MIN_SPEED } from './teleprompter.scroll'; +import { DEFAULT_SPEED, MAX_FONT_SIZE, MAX_SPEED, MIN_FONT_SIZE, MIN_SPEED } from './teleprompter.scroll'; import type { HeadingSource, TeleprompterOptions } from './teleprompter.types'; /** @@ -230,7 +230,7 @@ export function getOptionsFromParams( hideEmpty: toBoolean(getValue('hideEmpty'), defaults.hideEmpty), showGroups: toBoolean(getValue('showGroups'), defaults.showGroups), - speed: clampSpeed(toNumber(getValue('speed'), bounds.speed, defaults.speed)), + speed: toNumber(getValue('speed'), bounds.speed, defaults.speed), followLoaded: toBoolean(getValue('followLoaded'), defaults.followLoaded), fontSize: toNumber(getValue('fontSize'), bounds.fontSize, defaults.fontSize), diff --git a/apps/client/src/views/teleprompter/teleprompter.scroll.ts b/apps/client/src/views/teleprompter/teleprompter.scroll.ts index aa22c8fa6..3d57250f0 100644 --- a/apps/client/src/views/teleprompter/teleprompter.scroll.ts +++ b/apps/client/src/views/teleprompter/teleprompter.scroll.ts @@ -35,13 +35,9 @@ export const MIN_FONT_SIZE = 12; export const MAX_FONT_SIZE = 400; const FONT_SIZE_STEP_RATIO = 1.1; -export function clampFontSize(value: number): number { - return clamp(value, MIN_FONT_SIZE, MAX_FONT_SIZE); -} - /** @param steps how many presses to apply, negative to shrink */ export function stepFontSize(current: number, steps: number): number { - return clampFontSize(Math.round(current * FONT_SIZE_STEP_RATIO ** steps)); + return clamp(Math.round(current * FONT_SIZE_STEP_RATIO ** steps), MIN_FONT_SIZE, MAX_FONT_SIZE); } /** @@ -85,11 +81,17 @@ export function frameDeltaSeconds(deltaMs: number): number { } /** - * Advances the scroll position at a constant rate. + * Advances the scroll position at a constant rate, and reports having reached + * the bottom. * * The position is kept as a float by the caller: at readable prompter speeds the * per frame delta is well under a pixel, so rounding on every frame would stall * the scroll entirely. We return the exact float and let the DOM round on write. + * + * Deliberately does not bound the result. The caller has to bound the position + * anyway, because a nudge or a shrinking document can put it out of range on + * frames this never runs on, and one clamp in one place is easier to trust than + * the same rule applied twice on one of the paths. */ export function advance( position: number, @@ -97,7 +99,7 @@ export function advance( deltaSeconds: number, maxScroll: number, ): { position: number; atEnd: boolean } { - const next = clamp(position + pxPerSecond * deltaSeconds, 0, Math.max(maxScroll, 0)); + const next = position + pxPerSecond * deltaSeconds; // a document which does not overflow is never "at the end": it may simply not // have been measured yet, and stopping playback on that would be wrong return { position: next, atEnd: maxScroll > 0 && next >= maxScroll }; @@ -108,13 +110,12 @@ export function advance( * * Framerate independent: the same wall clock duration produces the same curve * regardless of how many frames it was sampled over. + * + * Snaps to the target once it is close enough rather than approaching it + * forever, so the caller can treat equality with the target as arrival. */ export function easeCatchUp(current: number, target: number, deltaSeconds: number): number { if (deltaSeconds <= 0) return current; const next = target + (current - target) * Math.exp(-CATCH_UP_RATE * deltaSeconds); return Math.abs(next - target) < CATCH_UP_EPSILON ? target : next; } - -export function hasArrived(current: number, target: number): boolean { - return Math.abs(current - target) < CATCH_UP_EPSILON; -} diff --git a/apps/client/src/views/teleprompter/useMirrorLiveParams.ts b/apps/client/src/views/teleprompter/useMirrorLiveParams.ts index 168c27853..d04f9ca69 100644 --- a/apps/client/src/views/teleprompter/useMirrorLiveParams.ts +++ b/apps/client/src/views/teleprompter/useMirrorLiveParams.ts @@ -1,4 +1,4 @@ -import { use, useEffect } from 'react'; +import { use, useEffect, useRef } from 'react'; import { useSearchParams } from 'react-router'; import { PresetContext } from '../../common/context/PresetContext'; @@ -35,8 +35,20 @@ export function useMirrorLiveParams(live: Record) { const [, setSearchParams] = useSearchParams(); const isPreset = Boolean(use(PresetContext)); - // the effect depends on the values rather than the object, which is new every render + /** + * The caller passes a fresh object every render, so the effect keys off the + * values instead. Depending on the object itself would restart the timer on + * every unrelated re-render, and the view re-renders on socket traffic, so the + * write would keep being postponed and never land. + * + * The values are then read through a ref rather than closed over, which keeps + * the timeout reading the latest of them and keeps the dependency honest. + */ const serialised = JSON.stringify(live); + const liveRef = useRef(live); + useEffect(() => { + liveRef.current = live; + }); useEffect(() => { /** @@ -50,7 +62,7 @@ export function useMirrorLiveParams(live: Record) { setSearchParams( (current) => { const next = new URLSearchParams(current); - for (const [key, value] of Object.entries(JSON.parse(serialised) as Record)) { + for (const [key, value] of Object.entries(liveRef.current)) { if (value === null) { next.delete(key); } else { diff --git a/apps/client/src/views/teleprompter/useTeleprompterControls.ts b/apps/client/src/views/teleprompter/useTeleprompterControls.ts index 9fa602d7a..c6b94919a 100644 --- a/apps/client/src/views/teleprompter/useTeleprompterControls.ts +++ b/apps/client/src/views/teleprompter/useTeleprompterControls.ts @@ -74,17 +74,6 @@ export function useTeleprompterControls(args: UseTeleprompterControlsArgs) { return; } - /** - * Enter is left to a focused control so the overlay stays operable from - * the keyboard. Space is not: it is the play/pause pedal, and a prompter - * which stops rolling because the operator last touched a button would be - * broken in the one moment it matters. The overlay drops focus after a - * click so the two never compete for the same press. - */ - if (event.key === 'Enter' && target?.closest('button, a, [role="button"]')) { - return; - } - const action = resolveTeleprompterAction(event); if (!action) return; diff --git a/apps/client/src/views/teleprompter/useTeleprompterScroll.ts b/apps/client/src/views/teleprompter/useTeleprompterScroll.ts index eff2a93c6..ae1f493e3 100644 --- a/apps/client/src/views/teleprompter/useTeleprompterScroll.ts +++ b/apps/client/src/views/teleprompter/useTeleprompterScroll.ts @@ -7,7 +7,6 @@ import { clampSpeed, easeCatchUp, frameDeltaSeconds, - hasArrived, linesPerMinuteToPxPerSecond, } from './teleprompter.scroll'; import type { ScriptBlock, TeleprompterController } from './teleprompter.types'; @@ -83,6 +82,7 @@ export function useTeleprompterScroll({ const runningRef = useRef(false); const speedRef = useRef(initialSpeed); const lineHeightRef = useRef(0); + /** never negative: measure() is the only writer and floors it at zero */ const maxScrollRef = useRef(0); const catchUpTargetRef = useRef(null); const pendingDeltaRef = useRef(0); @@ -136,8 +136,9 @@ export function useTeleprompterScroll({ if (catchUpTargetRef.current !== null) { next = easeCatchUp(next, catchUpTargetRef.current, deltaSeconds); - if (hasArrived(next, catchUpTargetRef.current)) { - next = catchUpTargetRef.current; + // easeCatchUp snaps to the target once it is close enough, so landing on it + // exactly is the arrival signal and needs no second distance test + if (next === catchUpTargetRef.current) { catchUpTargetRef.current = null; } } else if (runningRef.current) { @@ -151,7 +152,8 @@ export function useTeleprompterScroll({ } } - const clamped = clamp(next, 0, Math.max(maxScrollRef.current, 0)); + // the one place the position is bounded, whichever branch produced it + const clamped = clamp(next, 0, maxScrollRef.current); // touch the DOM only when the position actually moved, so a paused prompter // costs arithmetic rather than a scroll write on every frame if (clamped !== posRef.current) { @@ -203,11 +205,12 @@ export function useTeleprompterScroll({ // the configured speed seeds the live one, and reclaims it whenever the option // changes. Adjusted during render rather than in an effect so the readout never - // commits the stale value first + // commits the stale value first. It arrives already bounded by the option parser, + // so only the keys and buttons, which add deltas, have to clamp const [speedFromOption, setSpeedFromOption] = useState(initialSpeed); if (speedFromOption !== initialSpeed) { setSpeedFromOption(initialSpeed); - setSpeed(clampSpeed(initialSpeed)); + setSpeed(initialSpeed); } // the observer covers every reflow that matters, script edits included, so @@ -269,7 +272,7 @@ export function useTeleprompterScroll({ const offset = (scroller.clientHeight * readingLinePos) / 100; const top = target.getBoundingClientRect().top - scroller.getBoundingClientRect().top + scroller.scrollTop - offset; - catchUpTargetRef.current = clamp(top, 0, Math.max(maxScrollRef.current, 0)); + catchUpTargetRef.current = clamp(top, 0, maxScrollRef.current); setAtEnd(false); }, [selectedEventId, followLoaded, followLocked, readingLinePos, hasSelectedBlock]); @@ -325,7 +328,7 @@ export function useTeleprompterScroll({ const scroller = scrollerRef.current; if (!scroller) return; const distance = scroller.clientHeight * PAGE_FRACTION * direction; - catchUpTargetRef.current = clamp(posRef.current + distance, 0, Math.max(maxScrollRef.current, 0)); + catchUpTargetRef.current = clamp(posRef.current + distance, 0, maxScrollRef.current); setAtEnd(false); }, changeSpeed: (delta: number) => setSpeed((current) => clampSpeed(current + delta)), @@ -335,7 +338,7 @@ export function useTeleprompterScroll({ setAtEnd(false); }, jumpToEnd: () => { - catchUpTargetRef.current = Math.max(maxScrollRef.current, 0); + catchUpTargetRef.current = maxScrollRef.current; // the eased branch never reports the end, only the playing one does if (maxScrollRef.current > 0) { runningRef.current = false;