refactor(teleprompter): simplify the scroll engine and drop settings that overlap

Three simplifications from a review of where the complexity actually sat.

The animation frame loop now runs for the lifetime of the view instead of being
woken around each action. Waking a sleeping loop meant that every path which
could move the script had to remember to do it, across eight call sites, and
forgetting simply lost the action with no error. An idle frame costs one pass
of arithmetic and the browser suspends the loop entirely while the tab is
hidden. This removes ensureLoop, the frame handle and the work test.

Running always did break something, which is worth recording: any scroll the
loop had not made itself was reverted on the next frame, so a scrollbar drag or
find in page would snap back. The loop now compares the scroller against where
it left it and adopts the DOM position when they differ. That covers every
cause rather than only the gestures which had a handler, so the adopt flag and
its wiring are gone too, and there is an end to end test for it.

Dropped hidePast in favour of dimPast. They read as alternatives but only one
is safe during a read: removing past events shortens the document underneath
the reader, while dimming is only paint. Also dropped the shade reading line
variant, which drew the same gradient over the same box as dimPast, so turning
both on stacked two of them.

The reading line position was carried as two CSS variables, a percentage and a
unitless number. That split caused the padding bug earlier. One unitless
variable now serves both, scaled by 1% for the overlays and 1dvh for the
padding.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Cb8RVPNQ2ETPJxdy4b8CHf

apps/client/src/views/teleprompter/Teleprompter.scss

@@ -69,8 +69,8 @@
    * percentage padding resolves against the containing block's *width*, which
    * would put the first line nowhere near the reading line.
    */
-  padding-top: calc(var(--tp-reading-line-offset, 40) * 1dvh);
-  padding-bottom: calc(100dvh - var(--tp-reading-line-offset, 40) * 1dvh);
+  padding-top: calc(var(--tp-reading-line, 40) * 1dvh);
+  padding-bottom: calc(100dvh - var(--tp-reading-line, 40) * 1dvh);
 }
 
 .teleprompter__block {
@@ -113,7 +113,7 @@
 .teleprompter__dim {
   position: absolute;
   inset: 0 0 auto 0;
-  height: var(--tp-reading-line, 40%);
+  height: calc(var(--tp-reading-line, 40) * 1%);
   pointer-events: none;
   background: linear-gradient(to bottom, rgba(0, 0, 0, 0.8), rgba(0, 0, 0, 0));
 }
@@ -122,18 +122,12 @@
   position: absolute;
   left: 0;
   right: 0;
-  top: var(--tp-reading-line, 40%);
+  top: calc(var(--tp-reading-line, 40) * 1%);
   pointer-events: none;
 
   &--line {
     border-top: 2px solid rgba($accent-color, 0.6);
   }
-
-  &--shade {
-    top: 0;
-    height: var(--tp-reading-line, 40%);
-    background: linear-gradient(to bottom, rgba(0, 0, 0, 0.75), rgba(0, 0, 0, 0));
-  }
 }
 
 .teleprompter__arrow {

apps/client/src/views/teleprompter/Teleprompter.tsx

@@ -66,7 +66,6 @@ function Teleprompter({ rundown, rundownMetadata, customFields }: TeleprompterDa
     scriptSource: options.scriptSource,
     heading: options.heading,
     hideEmpty: options.hideEmpty,
-    hidePast: options.hidePast,
     showGroups: options.showGroups,
   });
 
@@ -118,10 +117,10 @@ function Teleprompter({ rundown, rundownMetadata, customFields }: TeleprompterDa
     '--tp-font-size': `${options.fontSize * fontScale}px`,
     '--tp-line-height': options.lineHeight,
     '--tp-text-width': `${options.textWidth}%`,
-    // as a percentage for the overlays, which are positioned against the height,
-    // and unitless for the content padding, which is scaled by dvh instead
-    '--tp-reading-line': `${options.readingLinePos}%`,
-    '--tp-reading-line-offset': options.readingLinePos,
+    // unitless, so the stylesheet can scale it by 1% against the view height for
+    // the overlays and by 1dvh for the content padding. Percentage padding would
+    // resolve against width and put the first line nowhere near the reading line
+    '--tp-reading-line': options.readingLinePos,
     '--tp-align': options.align,
     '--tp-background': options.keyColour ?? '#000000',
     '--tp-color': options.textColour ?? '#ffffff',

apps/client/src/views/teleprompter/__tests__/teleprompter.options.test.ts

@@ -9,7 +9,6 @@ describe('getOptionsFromParams()', () => {
       scriptSource: null,
       heading: 'title',
       hideEmpty: true,
-      hidePast: false,
       showGroups: true,
       speed: DEFAULT_SPEED,
       autoplay: false,
@@ -45,10 +44,9 @@ describe('getOptionsFromParams()', () => {
   });
 
   test('booleans which default to false can be turned on', () => {
-    const options = getOptionsFromParams(new URLSearchParams('hidePast=true&autoplay=true&flipH=true&flipV=true'));
+    const options = getOptionsFromParams(new URLSearchParams('autoplay=true&flipH=true&flipV=true'));
 
     expect(options).toMatchObject({
-      hidePast: true,
       autoplay: true,
       flipH: true,
       flipV: true,

apps/client/src/views/teleprompter/__tests__/teleprompter.utils.test.ts

@@ -45,7 +45,6 @@ const defaultOptions = {
   scriptSource: 'custom-script',
   heading: 'title' as const,
   hideEmpty: true,
-  hidePast: false,
   showGroups: true,
 };
 
@@ -121,16 +120,6 @@ describe('buildScript()', () => {
     ).toEqual(['a', 'b']);
   });
 
-  test('hidePast drops events which already played', () => {
-    const rundown = makeRundown([makeEvent('a'), makeEvent('b')]);
-    const metadata = metadataFor(['a', 'b'], { a: { isPast: true } });
-
-    expect(
-      buildScript(rundown, metadata, customFields, { ...defaultOptions, hidePast: true }).map((b) => b.id),
-    ).toEqual(['b']);
-    expect(buildScript(rundown, metadata, customFields, defaultOptions).map((b) => b.id)).toEqual(['a', 'b']);
-  });
-
   test('marks the block belonging to the loaded event', () => {
     const rundown = makeRundown([makeEvent('a'), makeEvent('b')]);
     const metadata = metadataFor(['a', 'b'], { b: { isLoaded: true } });

apps/client/src/views/teleprompter/teleprompter.options.ts

@@ -20,7 +20,6 @@ const headingOptions = [
 const readingLineOptions = [
   { value: 'line', label: 'Line' },
   { value: 'arrows', label: 'Arrows' },
-  { value: 'shade', label: 'Shade' },
   { value: 'none', label: 'None' },
 ];
 
@@ -97,13 +96,6 @@ export const getTeleprompterOptions = (customFields: CustomFields): ViewOption[]
           type: 'boolean',
           defaultValue: true,
         },
-        {
-          id: 'hidePast',
-          title: 'Hide past events',
-          description: 'Prevents showing events which have already played',
-          type: 'boolean',
-          defaultValue: false,
-        },
         {
           id: 'showGroups',
           title: 'Show group names',
@@ -224,7 +216,7 @@ function toEnum<T extends string>(value: string | null, allowed: readonly T[], f
 }
 
 const headingSources: readonly HeadingSource[] = ['none', 'title', 'cue', 'both'];
-const readingLineVariants: readonly ReadingLineVariant[] = ['none', 'line', 'arrows', 'shade'];
+const readingLineVariants: readonly ReadingLineVariant[] = ['none', 'line', 'arrows'];
 
 /**
  * Utility extract the view options from URL Params
@@ -242,7 +234,6 @@ export function getOptionsFromParams(
     heading: toEnum(getValue('heading'), headingSources, 'title'),
 
     hideEmpty: getValue('hideEmpty') === null ? true : isStringBoolean(getValue('hideEmpty')),
-    hidePast: isStringBoolean(getValue('hidePast')),
     showGroups: getValue('showGroups') === null ? true : isStringBoolean(getValue('showGroups')),
 
     speed: clampSpeed(toNumberInRange(getValue('speed'), MIN_SPEED, MAX_SPEED, DEFAULT_SPEED)),

apps/client/src/views/teleprompter/teleprompter.types.ts

@@ -3,8 +3,12 @@ import type { MaybeString } from 'ontime-types';
 /** What the per-event heading shows above each script block */
 export type HeadingSource = 'none' | 'title' | 'cue' | 'both';
 
-/** Style of the eye-line indicator */
-export type ReadingLineVariant = 'none' | 'line' | 'arrows' | 'shade';
+/**
+ * Style of the eye-line indicator.
+ * Darkening what has already been read is the separate dimPast option, so there
+ * is deliberately no shade variant here: it would be the same element twice.
+ */
+export type ReadingLineVariant = 'none' | 'line' | 'arrows';
 
 /** A single readable segment of the prompter document */
 export type ScriptBlock = {
@@ -25,7 +29,6 @@ export type TeleprompterOptions = {
   scriptSource: string | null;
   heading: HeadingSource;
   hideEmpty: boolean;
-  hidePast: boolean;
   showGroups: boolean;
   /** lines per minute */
   speed: number;

apps/client/src/views/teleprompter/teleprompter.utils.ts

@@ -4,10 +4,7 @@ import type { RundownMetadataObject } from '../../common/utils/rundownMetadata';
 import { getPropertyValue } from '../common/viewUtils';
 import type { HeadingSource, ScriptBlock, TeleprompterOptions } from './teleprompter.types';
 
-type BuildScriptOptions = Pick<
-  TeleprompterOptions,
-  'scriptSource' | 'heading' | 'hideEmpty' | 'hidePast' | 'showGroups'
->;
+type BuildScriptOptions = Pick<TeleprompterOptions, 'scriptSource' | 'heading' | 'hideEmpty' | 'showGroups'>;
 
 /**
  * Resolves the heading shown above a script block.
@@ -49,7 +46,7 @@ export function buildScript(
   customFields: CustomFields,
   options: BuildScriptOptions,
 ): ScriptBlock[] {
-  const { scriptSource, heading, hideEmpty, hidePast, showGroups } = options;
+  const { scriptSource, heading, hideEmpty, showGroups } = options;
 
   if (!scriptSource || scriptSource === 'none' || !isReadableSource(scriptSource, customFields)) {
     return [];
@@ -65,10 +62,6 @@ export function buildScript(
     }
 
     const metadata = rundownMetadata[id];
-    if (hidePast && metadata?.isPast) {
-      continue;
-    }
-
     const text = getPropertyValue(entry, scriptSource, rundown.entries)?.trim() ?? '';
     if (hideEmpty && !text) {
       continue;

apps/client/src/views/teleprompter/useTeleprompterScroll.ts

@@ -18,6 +18,13 @@ const PAGE_FRACTION = 0.85;
 /** how long the user has to stop scrolling before we consider the gesture over */
 const FOLLOW_LOCK_THROTTLE = 1000;
 
+/**
+ * How far the scroller may sit from where we left it before we treat the move as
+ * somebody else's. Comfortably above the rounding the browser applies to the
+ * fractional value we write, and well below a line.
+ */
+const EXTERNAL_SCROLL_EPSILON = 2;
+
 interface UseTeleprompterScrollArgs {
   initialSpeed: number;
   autoplay: boolean;
@@ -58,7 +65,6 @@ export function useTeleprompterScroll({
   // authoritative, sub-pixel scroll position
   const posRef = useRef(0);
   const lastTsRef = useRef(0);
-  const frameRef = useRef<number | null>(null);
   const runningRef = useRef(false);
   const speedPxSecRef = useRef(0);
   const speedRef = useRef(initialSpeed);
@@ -66,7 +72,6 @@ export function useTeleprompterScroll({
   const maxScrollRef = useRef(0);
   const catchUpTargetRef = useRef<number | null>(null);
   const pendingDeltaRef = useRef(0);
-  const adoptScrollRef = useRef(false);
 
   const [isRunning, setIsRunning] = useState(false);
   const [speed, setSpeed] = useState(initialSpeed);
@@ -76,23 +81,31 @@ export function useTeleprompterScroll({
   /**
    * Advances the scroll by one frame.
    *
-   * Reads and writes only refs, so it never needs rebuilding: keeping it stable
-   * is what lets the loop reschedule itself without going through the render.
+   * Runs on every frame for the lifetime of the view rather than being started
+   * and stopped around each action. Waking a sleeping loop meant every path that
+   * could move the script had to remember to do so, and forgetting simply lost
+   * the action with no error. A frame that has nothing to do costs one pass of
+   * arithmetic, and the browser suspends the whole loop while the tab is hidden.
+   *
+   * Reads and writes only refs, so it never needs rebuilding.
    */
   const tick = useCallback((timestamp: number) => {
     const el = scrollerRef.current;
-    if (!el) {
-      frameRef.current = null;
-      return;
-    }
+    // no scroller while the view is showing an empty state
+    if (!el) return;
 
-    // the user scrolled by hand: adopt the browser's position as the truth.
-    // this is read here, rather than in the event handler, because the browser
-    // has by now applied the scroll
-    if (adoptScrollRef.current) {
+    /**
+     * The scroller is no longer where this loop left it, so something else moved
+     * it: a wheel, a touch drag, a scrollbar, find in page, or the browser
+     * clamping us because the document got shorter. Whatever the cause, the DOM
+     * is now the truth and an eased jump in flight is stale.
+     *
+     * Detecting divergence covers every one of those without each having to
+     * announce itself, which is why there is no adopt flag for handlers to set.
+     */
+    if (Math.abs(el.scrollTop - posRef.current) > EXTERNAL_SCROLL_EPSILON) {
       posRef.current = el.scrollTop;
       catchUpTargetRef.current = null;
-      adoptScrollRef.current = false;
     }
 
     const deltaSeconds = frameDeltaSeconds(timestamp - lastTsRef.current);
@@ -127,20 +140,15 @@ export function useTeleprompterScroll({
     // a fractional value is intentional, the browser rounds it for us while we
     // keep the remainder, which is what makes slow speeds move at all
     el.scrollTop = posRef.current;
-
-    const hasWork =
-      runningRef.current ||
-      catchUpTargetRef.current !== null ||
-      pendingDeltaRef.current !== 0 ||
-      adoptScrollRef.current;
-    frameRef.current = hasWork ? requestAnimationFrame(tick) : null;
   }, []);
 
-  /** wakes the animation frame loop if it is not already running */
-  const ensureLoop = useCallback(() => {
-    if (frameRef.current !== null) return;
+  useEffect(() => {
     lastTsRef.current = performance.now();
-    frameRef.current = requestAnimationFrame(tick);
+    let frame = requestAnimationFrame(function loop(timestamp) {
+      tick(timestamp);
+      frame = requestAnimationFrame(loop);
+    });
+    return () => cancelAnimationFrame(frame);
   }, [tick]);
 
   /** recomputes the scroll bounds and the line height the speed is derived from */
@@ -169,15 +177,6 @@ export function useTeleprompterScroll({
     speedPxSecRef.current = linesPerMinuteToPxPerSecond(speedRef.current, lineHeightRef.current);
   }, []);
 
-  useEffect(() => {
-    return () => {
-      if (frameRef.current !== null) {
-        cancelAnimationFrame(frameRef.current);
-        frameRef.current = null;
-      }
-    };
-  }, []);
-
   // keep the derived pixel speed in sync with the lines per minute the user sees
   useEffect(() => {
     speedRef.current = speed;
@@ -230,7 +229,6 @@ export function useTeleprompterScroll({
     if (!autoplay) return;
     runningRef.current = true;
     setIsRunning(true);
-    ensureLoop();
     // eslint-disable-next-line react-hooks/exhaustive-deps -- autoplay is a starting condition, not a live one
   }, []);
 
@@ -259,23 +257,23 @@ export function useTeleprompterScroll({
 
     catchUpTargetRef.current = clamp(top, 0, Math.max(maxScrollRef.current, 0));
     setAtEnd(false);
-    ensureLoop();
-  }, [selectedEventId, followLoaded, followLocked, readingLinePos, hasSelectedBlock, ensureLoop]);
+  }, [selectedEventId, followLoaded, followLocked, readingLinePos, hasSelectedBlock]);
 
   const lockFollow = useMemo(() => throttle(() => setFollowLocked(true), FOLLOW_LOCK_THROTTLE), []);
 
   /**
-   * Any gesture which could have moved the scroller.
-   * We do not react to the scroll event itself because it also fires for our own
-   * writes, and the two are indistinguishable.
+   * Releases the follow when the operator takes over by hand.
+   *
+   * Only deliberate gestures count, which is why this listens for wheel and
+   * touch rather than the scroll event: scroll also fires for the loop's own
+   * writes, and following the loaded event would switch itself off on its way
+   * there. Adopting the resulting position is the tick's job, not this one's.
    */
   const handleUserScroll = useCallback(() => {
-    adoptScrollRef.current = true;
-    ensureLoop();
     if (followLoaded) {
       lockFollow();
     }
-  }, [ensureLoop, followLoaded, lockFollow]);
+  }, [followLoaded, lockFollow]);
 
   // entry ids are not guaranteed to be valid CSS selectors, so the follow target
   // is looked up through this map rather than with querySelector
@@ -296,7 +294,6 @@ export function useTeleprompterScroll({
       runningRef.current = true;
       setIsRunning(true);
       setAtEnd(false);
-      ensureLoop();
     };
 
     const pause = () => {
@@ -309,7 +306,6 @@ export function useTeleprompterScroll({
       nudge: (lines: number) => {
         pendingDeltaRef.current += lines * lineHeightRef.current;
         setAtEnd(false);
-        ensureLoop();
       },
       page: (direction: 1 | -1) => {
         const scroller = scrollerRef.current;
@@ -317,22 +313,19 @@ export function useTeleprompterScroll({
         const distance = scroller.clientHeight * PAGE_FRACTION * direction;
         catchUpTargetRef.current = clamp(posRef.current + distance, 0, Math.max(maxScrollRef.current, 0));
         setAtEnd(false);
-        ensureLoop();
       },
       changeSpeed: (delta: number) => setSpeed((current) => clampSpeed(current + delta)),
       rewind: (alsoPause = false) => {
         catchUpTargetRef.current = 0;
         if (alsoPause) pause();
         setAtEnd(false);
-        ensureLoop();
       },
       jumpToEnd: () => {
         catchUpTargetRef.current = Math.max(maxScrollRef.current, 0);
-        ensureLoop();
       },
       reengageFollow: () => setFollowLocked(false),
     };
-  }, [ensureLoop]);
+  }, []);
 
   return {
     scrollerRef,