From 8a31cc501885bac514c04936532160bcc1a99c93 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 19:04:32 +0000 Subject: [PATCH] fix(teleprompter): address review findings and the e2e ordering failure The e2e run failed because 214-rundown-switch-edit loads a fresh rundown, leaving the teleprompter spec with nothing to read. The spec now seeds its own script entry, idempotently, so it no longer depends on which rundown a previous spec happened to leave loaded. Review findings: - PresetView rendered outside ViewLoader, so a view reached through a preset never got the project's CSS override stylesheet. - The help overlay was a bare div. It is now a Dialog, so focus moves into it and back out, Escape closes it instead of rewinding the script, and the prompter keymap stands down while it is open. - Space is left unbound rather than made a no-op when a view claims it. useHotkeys calls preventDefault before reaching the handler, so an early return still swallowed the key and stopped Space activating a focused button. - The scroll observer attached on mount only, so a scroller which mounted later (the empty state resolving into a script) was never measured and could not play. Callback refs attach it whenever the elements appear. - jumpToEnd marked the end state before the document had been measured. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Cb8RVPNQ2ETPJxdy4b8CHf --- apps/client/src/AppRouter.tsx | 19 ++++-- .../navigation-menu/ViewNavigationMenu.tsx | 30 +++++++--- .../src/views/teleprompter/Teleprompter.scss | 21 +++++-- .../src/views/teleprompter/Teleprompter.tsx | 3 +- .../teleprompter/help-overlay/HelpOverlay.tsx | 58 ++++++++++++------- .../teleprompter/useTeleprompterControls.ts | 16 ++++- .../teleprompter/useTeleprompterScroll.ts | 29 +++++++++- e2e/tests/features/215-teleprompter.spec.ts | 35 +++++++++-- 8 files changed, 160 insertions(+), 51 deletions(-) diff --git a/apps/client/src/AppRouter.tsx b/apps/client/src/AppRouter.tsx index 308a52361..bc25e2649 100644 --- a/apps/client/src/AppRouter.tsx +++ b/apps/client/src/AppRouter.tsx @@ -225,12 +225,19 @@ function PresetView() { const Component = PresetViewMap[preset.target as OntimeViewPresettable]; return ( - - {Component ? : } + {/* + Presets render the same views as the direct routes and need the same + wrapper: ViewLoader is what injects the project's CSS override + stylesheet. Without it a preset silently ignores configured styling. + */} + + + {Component ? : } + ); } diff --git a/apps/client/src/common/components/navigation-menu/ViewNavigationMenu.tsx b/apps/client/src/common/components/navigation-menu/ViewNavigationMenu.tsx index cdc2c869f..5ea38fc8a 100644 --- a/apps/client/src/common/components/navigation-menu/ViewNavigationMenu.tsx +++ b/apps/client/src/common/components/navigation-menu/ViewNavigationMenu.tsx @@ -1,4 +1,4 @@ -import { useDisclosure, useHotkeys } from '@mantine/hooks'; +import { type HotkeyItem, useDisclosure, useHotkeys } from '@mantine/hooks'; import { memo } from 'react'; import { useSearchParams } from 'react-router'; @@ -24,15 +24,27 @@ function ViewNavigationMenu({ isNavigationLocked, suppressSettings, suppressSpac const [searchParams] = useSearchParams(); const hasSavedChanges = hasCustomParams(searchParams); + /** + * The Space binding is left out entirely rather than made a no-op, because + * useHotkeys calls preventDefault before it reaches the handler. A handler + * which returns early still swallows the key, which would stop Space + * activating whichever button the user has focused. + */ + const spaceHotkey: HotkeyItem[] = suppressSpaceHotkey + ? [] + : [ + [ + 'Space', + () => { + if (isNavigationLocked) return; + menuHandler.toggle(); + }, + { preventDefault: true }, + ], + ]; + useHotkeys([ - [ - 'Space', - () => { - if (isNavigationLocked || suppressSpaceHotkey) return; - menuHandler.toggle(); - }, - { preventDefault: true }, - ], + ...spaceHotkey, [ 'mod + ,', () => { diff --git a/apps/client/src/views/teleprompter/Teleprompter.scss b/apps/client/src/views/teleprompter/Teleprompter.scss index 39c3f8395..35c0fb8d5 100644 --- a/apps/client/src/views/teleprompter/Teleprompter.scss +++ b/apps/client/src/views/teleprompter/Teleprompter.scss @@ -205,19 +205,30 @@ color: $viewer-label-color; } +/** + * The help dialog is portalled to the body, so unlike the other overlays it sits + * outside the view root: fixed positioning is correct here, and it is not caught + * by the flip, which keeps it readable on a mirrored rig. Being outside also + * means it inherits nothing from the view and states its own colours. + */ .teleprompter__help { - position: absolute; + position: fixed; inset: 0; - display: grid; - place-items: center; background: rgba(0, 0, 0, 0.75); } .teleprompter__help-card { - max-height: 80%; + position: fixed; + top: 50%; + left: 50%; + transform: translate(-50%, -50%); + + max-height: 80dvh; overflow-y: auto; padding: clamp(16px, 2vw, 24px); - background: $viewer-card-bg-color; + + background: $viewer-background-color; + color: $viewer-color; border-radius: $element-border-radius; font-size: $base-font-size; } diff --git a/apps/client/src/views/teleprompter/Teleprompter.tsx b/apps/client/src/views/teleprompter/Teleprompter.tsx index 7e4d537d2..b1beed4c6 100644 --- a/apps/client/src/views/teleprompter/Teleprompter.tsx +++ b/apps/client/src/views/teleprompter/Teleprompter.tsx @@ -101,6 +101,7 @@ function Teleprompter({ rundown, rundownMetadata, customFields }: TeleprompterDa useTeleprompterControls({ controller, + isHelpOpen: showHelp, onFlip: handleFlip, onFontSize: handleFontSize, onResetFontSize: handleResetFontSize, @@ -168,7 +169,7 @@ function Teleprompter({ rundown, rundownMetadata, customFields }: TeleprompterDa )} - {showHelp && } + ); } diff --git a/apps/client/src/views/teleprompter/help-overlay/HelpOverlay.tsx b/apps/client/src/views/teleprompter/help-overlay/HelpOverlay.tsx index aa2b786d0..a2a6c03b2 100644 --- a/apps/client/src/views/teleprompter/help-overlay/HelpOverlay.tsx +++ b/apps/client/src/views/teleprompter/help-overlay/HelpOverlay.tsx @@ -1,6 +1,9 @@ +import { Dialog } from '@base-ui/react/dialog'; + import Button from '../../../common/components/buttons/Button'; interface HelpOverlayProps { + isOpen: boolean; onClose: () => void; } @@ -22,27 +25,42 @@ const shortcuts: { keys: string; action: string }[] = [ /** * Most prompter foot pedals and hand controllers are USB HID devices which send * these same keystrokes, so this list doubles as the hardware reference. + * + * Built on the shared Dialog rather than a bare overlay so it behaves as a + * modal: focus moves into it, stays inside it, and returns to where it came + * from. Escape closes the dialog instead of rewinding the script, and the + * prompter keymap stands down for as long as it is open. */ -export default function HelpOverlay({ onClose }: HelpOverlayProps) { +export default function HelpOverlay({ isOpen, onClose }: HelpOverlayProps) { return ( -
-
-
Controls
-
- {shortcuts.map(({ keys, action }) => ( -
-
{keys}
-
{action}
-
- ))} -
-
- Foot pedals and hand controllers which emit these keys work without any setup. -
- -
-
+ { + if (!open) { + onClose(); + } + }} + > + + + + Controls +
+ {shortcuts.map(({ keys, action }) => ( +
+
{keys}
+
{action}
+
+ ))} +
+
+ Foot pedals and hand controllers which emit these keys work without any setup. +
+ + + + ); } diff --git a/apps/client/src/views/teleprompter/useTeleprompterControls.ts b/apps/client/src/views/teleprompter/useTeleprompterControls.ts index a6a1f51d4..23b9ad0ac 100644 --- a/apps/client/src/views/teleprompter/useTeleprompterControls.ts +++ b/apps/client/src/views/teleprompter/useTeleprompterControls.ts @@ -6,6 +6,8 @@ import type { TeleprompterAction, TeleprompterController } from './teleprompter. interface UseTeleprompterControlsArgs { controller: TeleprompterController; + /** the help dialog is modal, so the keymap stands down while it is open */ + isHelpOpen: boolean; onFlip: (axis: 'h' | 'v') => void; onFontSize: (delta: number) => void; onResetFontSize: () => void; @@ -67,8 +69,18 @@ export function useTeleprompterControls(args: UseTeleprompterControlsArgs) { if (target && (ignoredTags.has(target.tagName) || target.isContentEditable)) { return; } - // the params editor is a form, it owns the keyboard while it is open - if (useViewParamsEditorStore.getState().isOpen) { + // the params editor and the help dialog are modal, they own the keyboard + if (useViewParamsEditorStore.getState().isOpen || argsRef.current.isHelpOpen) { + return; + } + + /** + * A focused button is activated by Space and Enter. Resolving those into + * prompter actions here, and calling preventDefault, would stop the button + * doing its own job: tabbing to the help control and pressing Space would + * start the script rather than open the help. + */ + if ((event.code === 'Space' || event.key === 'Enter') && target?.closest('button, a, [role="button"]')) { return; } diff --git a/apps/client/src/views/teleprompter/useTeleprompterScroll.ts b/apps/client/src/views/teleprompter/useTeleprompterScroll.ts index b8d5bf0ab..eff2a93c6 100644 --- a/apps/client/src/views/teleprompter/useTeleprompterScroll.ts +++ b/apps/client/src/views/teleprompter/useTeleprompterScroll.ts @@ -60,6 +60,23 @@ export function useTeleprompterScroll({ const contentRef = useRef(null); const blockRefs = useRef(new Map()); + /** + * The view renders an empty state instead of the scroller until a script is + * chosen, so these elements can arrive long after mount. Tracking that in + * state is what lets the measuring effect run when they do: keyed only on the + * refs it would have run once, against nothing, and playback would sit dead + * until the page was reloaded. + */ + const [isScrollerMounted, setIsScrollerMounted] = useState(false); + const attachScroller = useCallback((element: HTMLDivElement | null) => { + scrollerRef.current = element; + setIsScrollerMounted(Boolean(element && contentRef.current)); + }, []); + const attachContent = useCallback((element: HTMLDivElement | null) => { + contentRef.current = element; + setIsScrollerMounted(Boolean(element && scrollerRef.current)); + }, []); + // authoritative, sub-pixel scroll position const posRef = useRef(0); const lastTsRef = useRef(0); @@ -206,7 +223,7 @@ export function useTeleprompterScroll({ observer.observe(scroller); observer.observe(content); return () => observer.disconnect(); - }, [measure]); + }, [measure, isScrollerMounted]); // webfonts land after first paint and reflow the whole document useEffect(() => { @@ -319,14 +336,20 @@ export function useTeleprompterScroll({ }, jumpToEnd: () => { catchUpTargetRef.current = Math.max(maxScrollRef.current, 0); + // the eased branch never reports the end, only the playing one does + if (maxScrollRef.current > 0) { + runningRef.current = false; + setIsRunning(false); + setAtEnd(true); + } }, reengageFollow: () => setFollowLocked(false), }; }, []); return { - scrollerRef, - contentRef, + scrollerRef: attachScroller, + contentRef: attachContent, registerBlock, handleUserScroll, controller, diff --git a/e2e/tests/features/215-teleprompter.spec.ts b/e2e/tests/features/215-teleprompter.spec.ts index 71f851cc7..d6f68aa40 100644 --- a/e2e/tests/features/215-teleprompter.spec.ts +++ b/e2e/tests/features/215-teleprompter.spec.ts @@ -1,12 +1,15 @@ import { type Page, expect, test } from '@playwright/test'; /** - * The note field is used as the script source throughout: every event in the - * test fixture has one, which keeps these tests independent of how custom field - * keys happen to be spelled in the fixture. + * The note field is the script source throughout, so that these tests do not + * depend on how custom field keys happen to be spelled. */ const teleprompterUrl = '/teleprompter?script=note'; +const scriptMarker = 'E2E prompter script'; +/** long enough that the document scrolls well past a screen */ +const scriptText = `${scriptMarker}. `.repeat(40); + function scroller(page: Page) { return page.getByTestId('teleprompter-scroller'); } @@ -15,14 +18,36 @@ function scrollTop(page: Page) { return scroller(page).evaluate((element) => element.scrollTop); } +/** + * Puts a known script into whichever rundown happens to be loaded. + * + * These tests used to read the notes of the uploaded fixture, which made them + * depend on every spec that runs before them: 214 creates a fresh rundown and + * leaves it loaded, so by the time this file ran there were no notes anywhere + * and the view was showing its empty state. Seeding is idempotent, so the + * rundown gains one event no matter how many tests run. + */ +async function seedScript(page: Page) { + const rundown = await (await page.request.get('/data/rundowns/current')).json(); + const alreadySeeded = rundown.flatOrder.some((id: string) => rundown.entries[id]?.note?.startsWith(scriptMarker)); + if (alreadySeeded) return; + + await page.request.post(`/data/rundowns/${rundown.id}/entry`, { + data: { type: 'event', title: 'Teleprompter e2e', note: scriptText }, + }); +} + test.describe('teleprompter', () => { + test.beforeEach(async ({ page }) => { + await seedScript(page); + }); + test('shows the script from the selected source', async ({ page }) => { await page.goto(teleprompterUrl); await expect(page.getByTestId('teleprompter-view')).toBeVisible(); await expect(scroller(page)).toBeVisible(); - // the fixture uses cue style notes, the first event is Albania - await expect(page.getByText('SF1.01', { exact: true })).toBeVisible(); + await expect(page.getByText(scriptMarker).first()).toBeVisible(); }); test('asks for a script source when none is selected', async ({ page }) => {