From bc8407c0e67db45b2fac70d4dc3f6bd1f109e6fd Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 17:21:24 +0000 Subject: [PATCH] refactor(rundown): address review on the rundown scope - extract getRundownCacheKey, the fallback to the bootstrap alias for an unresolved rundown id was spelled out in two readers and missing in the selection store, which could not resolve shift/ctrl ranges during startup - an unresolved target is no longer reported as the loaded rundown, it was briefly enabling run mode and playback following before a rundown existed - only the reader which bootstrapped drops the /current alias, a reader on another rundown was evicting a cache others still relied on - the selection store returns a new Set on toggle and unselect, components selecting the set by reference never saw the change - useSelectAndRevealEntry takes its rundown from the scope, callers were threading an id for no other reason Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011a5cbVjNC5XXF88b2PkUCa --- apps/client/src/common/api/constants.ts | 8 ++++++++ .../src/common/context/RundownScopeContext.tsx | 13 +++++++------ .../client/src/common/hooks-query/useRundown.ts | 13 ++++++++----- apps/client/src/common/hooks/useEntryAction.ts | 7 +++---- .../__tests__/eventSelectionStore.test.ts | 17 +++++++++++++++++ apps/client/src/common/stores/entryCopyStore.ts | 9 +++++---- .../src/common/stores/eventSelectionStore.ts | 16 +++++++++------- .../features/rundown/useSelectAndRevealEntry.ts | 4 +++- .../src/views/editor/finder/useFinder.tsx | 4 ++-- .../src/views/editor/title-list/TitleList.tsx | 12 ++---------- 10 files changed, 64 insertions(+), 39 deletions(-) diff --git a/apps/client/src/common/api/constants.ts b/apps/client/src/common/api/constants.ts index aa0551931..878408121 100644 --- a/apps/client/src/common/api/constants.ts +++ b/apps/client/src/common/api/constants.ts @@ -22,6 +22,14 @@ export const CLIENT_LIST = ['clientList']; export const REPORT = ['report']; export const TRANSLATION = ['translation']; +/** + * Cache key holding the data for a rundown. + * Before the loaded rundown is known there is no id to key by, + * and the data lives under the bootstrap alias. + */ +export const getRundownCacheKey = (rundownId: string) => + rundownId ? getRundownQueryKey(rundownId) : CURRENT_RUNDOWN_QUERY_KEY; + // API URLs export const apiEntryUrl = `${serverURL}/data`; diff --git a/apps/client/src/common/context/RundownScopeContext.tsx b/apps/client/src/common/context/RundownScopeContext.tsx index fa1d8bc6a..1578b21b0 100644 --- a/apps/client/src/common/context/RundownScopeContext.tsx +++ b/apps/client/src/common/context/RundownScopeContext.tsx @@ -1,7 +1,7 @@ -import { Rundown } from 'ontime-types'; +import { MaybeString, Rundown } from 'ontime-types'; import { PropsWithChildren, createContext, useContext, useEffect, useMemo, useRef } from 'react'; -import { getRundownQueryKey } from '../api/constants'; +import { getRundownCacheKey } from '../api/constants'; import { useProjectRundowns } from '../hooks-query/useProjectRundowns'; import { ontimeQueryClient } from '../queryClient'; import { createEventSelectionStore, type EventSelectionStoreApi } from '../stores/eventSelectionStore'; @@ -19,7 +19,7 @@ const RundownScopeContext = createContext(null); export interface RundownScopeProviderProps extends PropsWithChildren { /** rundown to operate on, null follows the loaded rundown */ - rundownId: string | null; + rundownId: MaybeString; } /** @@ -38,25 +38,26 @@ export function RundownScopeProvider({ children, rundownId }: RundownScopeProvid // the store reads the rundown lazily, the ref keeps it pointing at the current target const targetIdRef = useRef(targetId); - targetIdRef.current = targetId; const selectionStoreRef = useRef(null); if (selectionStoreRef.current === null) { selectionStoreRef.current = createEventSelectionStore(() => - ontimeQueryClient.getQueryData(getRundownQueryKey(targetIdRef.current)), + ontimeQueryClient.getQueryData(getRundownCacheKey(targetIdRef.current)), ); } const selectionStore = selectionStoreRef.current; // a selection refers to entries of a single rundown, it cannot survive a change of target useEffect(() => { + targetIdRef.current = targetId; selectionStore.getState().clearSelectedEvents(); }, [selectionStore, targetId]); const value = useMemo( (): RundownScopeValue => ({ rundownId: targetId, - isLoaded: targetId === loaded, + // an unresolved target is not the loaded rundown, it is not yet any rundown + isLoaded: Boolean(loaded) && targetId === loaded, selectionStore, }), [targetId, loaded, selectionStore], diff --git a/apps/client/src/common/hooks-query/useRundown.ts b/apps/client/src/common/hooks-query/useRundown.ts index 13f6495d2..c0d1f7551 100644 --- a/apps/client/src/common/hooks-query/useRundown.ts +++ b/apps/client/src/common/hooks-query/useRundown.ts @@ -1,9 +1,9 @@ import { useQuery, useQueryClient } from '@tanstack/react-query'; import { EntryId, OntimeEntry, Rundown } from 'ontime-types'; -import { useEffect, useMemo } from 'react'; +import { useEffect, useMemo, useRef } from 'react'; import { queryRefetchIntervalSlow } from '../../ontimeConfig'; -import { CURRENT_RUNDOWN_QUERY_KEY, getRundownQueryKey } from '../api/constants'; +import { CURRENT_RUNDOWN_QUERY_KEY, getRundownCacheKey, getRundownQueryKey } from '../api/constants'; import { fetchCurrentRundown, fetchRundown } from '../api/rundown'; import { useRundownScope } from '../context/RundownScopeContext'; import { useSelectedEventId } from '../hooks/useSocket'; @@ -32,7 +32,7 @@ export function useRundownById(rundownId: string | null | undefined) { const isBootstrap = id === ''; const { data, status, isError, refetch, isFetching } = useQuery({ - queryKey: isBootstrap ? CURRENT_RUNDOWN_QUERY_KEY : getRundownQueryKey(id), + queryKey: getRundownCacheKey(id), queryFn: ({ signal }) => (isBootstrap ? fetchCurrentRundown({ signal }) : fetchRundown(id, { signal })), placeholderData: (previousData, _previousQuery) => previousData, refetchInterval: queryRefetchIntervalSlow, @@ -44,9 +44,12 @@ export function useRundownById(rundownId: string | null | undefined) { queryClient.setQueryData(getRundownQueryKey(data.id), data); }, [data, isBootstrap, queryClient]); - // Once we have the ID, drop the temporary current cache + // Once we have the ID, drop the temporary current cache. + // Only the reader which bootstrapped may do so, others are still relying on it. + const didBootstrap = useRef(isBootstrap); useEffect(() => { - if (isBootstrap) return; + if (isBootstrap || !didBootstrap.current) return; + didBootstrap.current = false; queryClient.removeQueries({ queryKey: CURRENT_RUNDOWN_QUERY_KEY, exact: true }); }, [isBootstrap, queryClient]); diff --git a/apps/client/src/common/hooks/useEntryAction.ts b/apps/client/src/common/hooks/useEntryAction.ts index 53020c75d..de5a667f6 100644 --- a/apps/client/src/common/hooks/useEntryAction.ts +++ b/apps/client/src/common/hooks/useEntryAction.ts @@ -33,7 +33,7 @@ import { import { useCallback, useMemo } from 'react'; import { moveDown, moveUp, orderEntries } from '../../features/rundown/rundown.utils'; -import { CURRENT_RUNDOWN_QUERY_KEY, getRundownQueryKey } from '../api/constants'; +import { getRundownCacheKey } from '../api/constants'; import { ReorderEntry, deleteEntries, @@ -77,7 +77,7 @@ export const useEntryActions = () => useEntryActionsForRundown(useRundownScope() /** * Gather utilities for actions on entries in an explicitly selected rundown. */ -export const useScopedEntryActions = (rundownId: string | null) => useEntryActionsForRundown(rundownId ?? ''); +export const useScopedEntryActions = (rundownId: MaybeString) => useEntryActionsForRundown(rundownId ?? ''); function useEntryActionsForRundown(scopedRundownId: string) { const queryClient = useQueryClient(); @@ -92,9 +92,8 @@ function useEntryActionsForRundown(scopedRundownId: string) { inheritGroupColour, } = useEditorSettings(); - // an empty id means the loaded rundown has not resolved yet, the bootstrap alias holds the data const resolveCurrentRundownQueryKey = useCallback(() => { - return scopedRundownId ? getRundownQueryKey(scopedRundownId) : CURRENT_RUNDOWN_QUERY_KEY; + return getRundownCacheKey(scopedRundownId); }, [scopedRundownId]); /** diff --git a/apps/client/src/common/stores/__tests__/eventSelectionStore.test.ts b/apps/client/src/common/stores/__tests__/eventSelectionStore.test.ts index 5ff026683..1aa309fee 100644 --- a/apps/client/src/common/stores/__tests__/eventSelectionStore.test.ts +++ b/apps/client/src/common/stores/__tests__/eventSelectionStore.test.ts @@ -37,6 +37,23 @@ describe('createEventSelectionStore', () => { expect(store.getState().anchoredIndex).toBe(2); }); + it('replaces the selection set so subscribers on the set re-render', () => { + const store = createEventSelectionStore(() => makeRundown('rundown-a', ['a1', 'a2'])); + + store.getState().setSelectedEvents({ id: 'a1', index: 0, selectMode: 'click' }); + const afterClick = store.getState().selectedEvents; + + store.getState().setSelectedEvents({ id: 'a2', index: 1, selectMode: 'ctrl' }); + const afterAdd = store.getState().selectedEvents; + expect(afterAdd).not.toBe(afterClick); + expect(afterAdd).toEqual(new Set(['a1', 'a2'])); + + store.getState().unselect('a1'); + const afterUnselect = store.getState().selectedEvents; + expect(afterUnselect).not.toBe(afterAdd); + expect(afterUnselect).toEqual(new Set(['a2'])); + }); + it('does not select when the rundown is unavailable', () => { const store = createEventSelectionStore(() => undefined); diff --git a/apps/client/src/common/stores/entryCopyStore.ts b/apps/client/src/common/stores/entryCopyStore.ts index 33963265f..e35cd2ef9 100644 --- a/apps/client/src/common/stores/entryCopyStore.ts +++ b/apps/client/src/common/stores/entryCopyStore.ts @@ -1,11 +1,12 @@ +import { MaybeString } from 'ontime-types'; import { create } from 'zustand'; type EntryCopyStore = { - entryCopyId: string | null; + entryCopyId: MaybeString; /** rundown the copied entry belongs to, so a paste knows whether it crosses rundowns */ - entryCopyRundownId: string | null; + entryCopyRundownId: MaybeString; entryCopyMode: 'copy' | 'cut'; - setEntryCopyId: (eventId: string | null, rundownId: string | null, mode?: 'copy' | 'cut') => void; + setEntryCopyId: (eventId: MaybeString, rundownId: MaybeString, mode?: 'copy' | 'cut') => void; }; /** @@ -15,6 +16,6 @@ export const useEntryCopy = create()((set) => ({ entryCopyId: null, entryCopyRundownId: null, entryCopyMode: 'copy', - setEntryCopyId: (entryCopyId: string | null, entryCopyRundownId: string | null, mode: 'copy' | 'cut' = 'copy') => + setEntryCopyId: (entryCopyId: MaybeString, entryCopyRundownId: MaybeString, mode: 'copy' | 'cut' = 'copy') => set({ entryCopyId, entryCopyRundownId, entryCopyMode: mode }), })); diff --git a/apps/client/src/common/stores/eventSelectionStore.ts b/apps/client/src/common/stores/eventSelectionStore.ts index 5acb6a1ae..0db39c67e 100644 --- a/apps/client/src/common/stores/eventSelectionStore.ts +++ b/apps/client/src/common/stores/eventSelectionStore.ts @@ -63,7 +63,7 @@ export function createEventSelectionStore(getRundown: () => Rundown | undefined) // if it doesnt exist, simply add to the list and set an anchor if (!selectedEvents.has(id)) { return set({ - selectedEvents: selectedEvents.add(id), + selectedEvents: new Set(selectedEvents).add(id), anchoredIndex: index, cursor: id, entryMode: 'event', @@ -72,15 +72,16 @@ export function createEventSelectionStore(getRundown: () => Rundown | undefined) // if event is already selected, we remove it from selection // and set the anchor to the event after - selectedEvents.delete(id); + const withoutId = new Set(selectedEvents); + withoutId.delete(id); const nextIndex = rundownData.order.findIndex( - (eventId, i) => i > index && isOntimeEvent(rundownData.entries[eventId]) && selectedEvents.has(eventId), + (eventId, i) => i > index && isOntimeEvent(rundownData.entries[eventId]) && withoutId.has(eventId), ); // if we didnt find anything after, set the anchor to the last event return set({ - selectedEvents, + selectedEvents: withoutId, anchoredIndex: nextIndex < 0 ? rundownData.order.length - 1 : nextIndex, entryMode: 'event', }); @@ -125,10 +126,11 @@ export function createEventSelectionStore(getRundown: () => Rundown | undefined) }, unselect: (id: string) => { const { entryMode, selectedEvents } = get(); - selectedEvents.delete(id); + const remaining = new Set(selectedEvents); + remaining.delete(id); set({ - selectedEvents, - entryMode: selectedEvents.size === 0 ? null : entryMode, + selectedEvents: remaining, + entryMode: remaining.size === 0 ? null : entryMode, }); }, // Sets the scroll handler for programmatic scrolling to entries diff --git a/apps/client/src/features/rundown/useSelectAndRevealEntry.ts b/apps/client/src/features/rundown/useSelectAndRevealEntry.ts index 8db177c5c..fb1ac4188 100644 --- a/apps/client/src/features/rundown/useSelectAndRevealEntry.ts +++ b/apps/client/src/features/rundown/useSelectAndRevealEntry.ts @@ -1,6 +1,7 @@ import { EntryId, MaybeString } from 'ontime-types'; import { useCallback } from 'react'; +import { useRundownScope } from '../../common/context/RundownScopeContext'; import { useCollapsedGroups } from './useCollapsedGroups'; import { useEventSelection } from './useEventSelection'; @@ -10,7 +11,8 @@ type SelectAndRevealOptions = { parent?: MaybeString; }; -export function useSelectAndRevealEntry(rundownId: string) { +export function useSelectAndRevealEntry() { + const { rundownId } = useRundownScope(); const { expandGroup } = useCollapsedGroups(rundownId); const selectEntry = useEventSelection((state) => state.setSelectedEvents); const scrollToEntry = useEventSelection((state) => state.scrollToEntry); diff --git a/apps/client/src/views/editor/finder/useFinder.tsx b/apps/client/src/views/editor/finder/useFinder.tsx index e933834ce..53e6c4ba1 100644 --- a/apps/client/src/views/editor/finder/useFinder.tsx +++ b/apps/client/src/views/editor/finder/useFinder.tsx @@ -207,10 +207,10 @@ export function searchByText( * @param activeFilter - a field selected from the filter badges, if any */ export default function useFinder(searchValue: string, activeFilter: MaybeString) { - const { data, rundownId } = useFlatRundown(); + const { data } = useFlatRundown(); const { data: customFields } = useCustomFields(); - const selectAndRevealEntry = useSelectAndRevealEntry(rundownId); + const selectAndRevealEntry = useSelectAndRevealEntry(); /** The filters offered to the user: the fixed fields plus whatever the project defines */ const filters = useMemo(() => { diff --git a/apps/client/src/views/editor/title-list/TitleList.tsx b/apps/client/src/views/editor/title-list/TitleList.tsx index 1082a8677..cfe4ee231 100644 --- a/apps/client/src/views/editor/title-list/TitleList.tsx +++ b/apps/client/src/views/editor/title-list/TitleList.tsx @@ -46,7 +46,6 @@ export default function TitleList({ mode }: TitleListProps) { eventData={eventData} selectedEventId={selectedEventId} resolvedFollowEventId={resolvedFollowEventId} - rundownId={rundown.id} /> ); } @@ -56,21 +55,14 @@ interface TitleListContentProps { eventData: ExtendedEntry[]; selectedEventId: string | null; resolvedFollowEventId: string | null; - rundownId: string; } -function TitleListContent({ - mode, - eventData, - selectedEventId, - resolvedFollowEventId, - rundownId, -}: TitleListContentProps) { +function TitleListContent({ mode, eventData, selectedEventId, resolvedFollowEventId }: TitleListContentProps) { 'use memo'; const virtuosoRef = useRef(null); const scrollParentRef = useRef(null); - const selectAndRevealEntry = useSelectAndRevealEntry(rundownId); + const selectAndRevealEntry = useSelectAndRevealEntry(); // Calculate current event info const currentEventInfo = useMemo(() => {