From fec1651e39601442fa1f6cfedca77cfb825460be Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 19:32:16 +0000 Subject: [PATCH] fix(timer): address review findings on the group timer - keep the pending and none phases when showing the group timer. These describe the playback state rather than a threshold, so collapsing them made roll standby render as a running countdown and silently dropped user styling keyed on the phase - only show the group indicator for timer types which render the running timer. It was labelling the wall clock, and the invisible display of the none timer type, as a group - accept useGroupTimer in the MCP group patch. It was documented in the group shape but dropped on write, which reported success without applying the change - narrow the timer preview tooltip, which claimed all views follow the group timer when only the timer views do Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Kniqs443KUNCRABwVJwT7K --- .../features/control/message/TimerPreview.tsx | 2 +- .../src/views/editor/pip-timer/PipTimer.tsx | 7 +- apps/client/src/views/timer/Timer.tsx | 9 +- .../views/timer/__tests__/timer.utils.test.ts | 110 ++++++++++++++++++ apps/client/src/views/timer/timer.utils.ts | 18 +-- apps/server/src/api-mcp/mcp.service.ts | 5 +- 6 files changed, 136 insertions(+), 15 deletions(-) create mode 100644 apps/client/src/views/timer/__tests__/timer.utils.test.ts diff --git a/apps/client/src/features/control/message/TimerPreview.tsx b/apps/client/src/features/control/message/TimerPreview.tsx index 8f35c22ca..3bc0af1dd 100644 --- a/apps/client/src/features/control/message/TimerPreview.tsx +++ b/apps/client/src/features/control/message/TimerPreview.tsx @@ -107,7 +107,7 @@ export default function TimerPreview() { } className={style.statusIcon} data-active={isGroupTimerActive} diff --git a/apps/client/src/views/editor/pip-timer/PipTimer.tsx b/apps/client/src/views/editor/pip-timer/PipTimer.tsx index 80f82ba37..76a46427c 100644 --- a/apps/client/src/views/editor/pip-timer/PipTimer.tsx +++ b/apps/client/src/views/editor/pip-timer/PipTimer.tsx @@ -10,7 +10,7 @@ import { getIsPlaying, getSecondaryDisplay, getShowMessage, - getShowProgressBar, + getShowsTimerValue, resolveTimerDisplay, } from '../../timer/timer.utils'; import { getTimerColour } from '../../utils/presentation.utils'; @@ -40,7 +40,8 @@ export function PipTimer({ viewSettings }: PipTimerProps) { const { showFinished, showWarning, showDanger } = timerDisplay; const isPlaying = getIsPlaying(time.playback); - const showProgressBar = getShowProgressBar(timerTypeNow); + const showsTimerValue = getShowsTimerValue(timerTypeNow); + const showProgressBar = showsTimerValue; // gather timer data const stageTimer = getTimerByType(false, timerTypeNow, clock, timerDisplay.source, timerTypeNow); @@ -87,7 +88,7 @@ export function PipTimer({ viewSettings }: PipTimerProps) { > {display} - {timerDisplay.isGroup &&
group
} + {timerDisplay.isGroup && showsTimerValue &&
group
}
{secondaryContent} diff --git a/apps/client/src/views/timer/Timer.tsx b/apps/client/src/views/timer/Timer.tsx index d75788e3a..9069bc6cb 100644 --- a/apps/client/src/views/timer/Timer.tsx +++ b/apps/client/src/views/timer/Timer.tsx @@ -25,7 +25,7 @@ import { getSecondaryDisplay, getShowClock, getShowMessage, - getShowProgressBar, + getShowsTimerValue, resolveTimerDisplay, } from './timer.utils'; import { TimerData, useTimerData } from './useTimerData'; @@ -92,7 +92,8 @@ function Timer({ customFields, projectData, isMirrored, settings, viewSettings, const { showEndMessage, showFinished, showWarning, showDanger } = timerDisplay; const isPlaying = getIsPlaying(time.playback); const showClock = !hideClock && getShowClock(viewTimerType); - const showProgressBar = !hideProgress && getShowProgressBar(viewTimerType); + const showsTimerValue = getShowsTimerValue(viewTimerType); + const showProgressBar = !hideProgress && showsTimerValue; // gather card data const { showNow, nowMain, nowSecondary, showNext, nextMain, nextSecondary } = getCardData( @@ -186,7 +187,9 @@ function Timer({ customFields, projectData, isMirrored, settings, viewSettings, {display}
)} - {timerDisplay.isGroup && !showEndMessage &&
group
} + {timerDisplay.isGroup && showsTimerValue && !showEndMessage && ( +
group
+ )}
{secondaryContent} diff --git a/apps/client/src/views/timer/__tests__/timer.utils.test.ts b/apps/client/src/views/timer/__tests__/timer.utils.test.ts new file mode 100644 index 000000000..cb1ca446d --- /dev/null +++ b/apps/client/src/views/timer/__tests__/timer.utils.test.ts @@ -0,0 +1,110 @@ +import { GroupTimerState, Playback, TimerPhase, TimerState, TimerType } from 'ontime-types'; + +import { getShowsTimerValue, resolveTimerDisplay } from '../timer.utils'; + +const makeTime = (patch: Partial = {}): TimerState => ({ + addedTime: 0, + current: 10000, + duration: 60000, + elapsed: 50000, + expectedFinish: null, + phase: TimerPhase.Default, + playback: Playback.Play, + secondaryTimer: null, + startedAt: null, + ...patch, +}); + +const makeGroupTimer = (patch: Partial = {}): GroupTimerState => ({ + current: 100000, + elapsed: 80000, + duration: 180000, + ...patch, +}); + +const resolve = (time: TimerState, groupTimer: GroupTimerState | null) => + resolveTimerDisplay({ + time, + groupTimer, + event: { timeWarning: 120000, timeDanger: 60000 }, + timerType: TimerType.CountDown, + countToEnd: false, + freezeOvertime: false, + freezeMessage: '', + hidePhase: false, + }); + +describe('resolveTimerDisplay()', () => { + it('uses the event timer when there is no group timer', () => { + const time = makeTime(); + const result = resolve(time, null); + + expect(result.isGroup).toBe(false); + expect(result.source).toBe(time); + expect(result.total).toBe(60000); + expect(result.warning).toBe(120000); + expect(result.danger).toBe(60000); + }); + + it('adds the time added to an event into the event total', () => { + expect(resolve(makeTime({ addedTime: 30000 }), null).total).toBe(90000); + }); + + it('uses the group timer when one is present', () => { + const groupTimer = makeGroupTimer(); + const result = resolve(makeTime(), groupTimer); + + expect(result.isGroup).toBe(true); + expect(result.source).toBe(groupTimer); + expect(result.total).toBe(180000); + }); + + it('drops the event thresholds while showing the group', () => { + const result = resolve(makeTime({ phase: TimerPhase.Danger }), makeGroupTimer()); + + expect(result.warning).toBeUndefined(); + expect(result.danger).toBeUndefined(); + }); + + it.each([TimerPhase.Warning, TimerPhase.Danger])( + 'reports the group as running while the event is in %s', + (phase) => { + const result = resolve(makeTime({ phase }), makeGroupTimer()); + + expect(result.phase).toBe(TimerPhase.Default); + expect(result.showWarning).toBe(false); + expect(result.showDanger).toBe(false); + expect(result.showFinished).toBe(false); + }, + ); + + it('reports overtime only once the group itself has run out of time', () => { + const overrunningEvent = makeTime({ current: -5000, phase: TimerPhase.Overtime }); + + // the event is over, but the group still has time left + const stillRunning = resolve(overrunningEvent, makeGroupTimer({ current: 60000 })); + expect(stillRunning.phase).toBe(TimerPhase.Default); + expect(stillRunning.showFinished).toBe(false); + + const overtime = resolve(overrunningEvent, makeGroupTimer({ current: -1000 })); + expect(overtime.phase).toBe(TimerPhase.Overtime); + expect(overtime.showFinished).toBe(true); + }); + + it.each([TimerPhase.Pending, TimerPhase.None])('keeps the %s playback phase on the group', (phase) => { + // these describe the playback state rather than a threshold, so they are true of the group too + expect(resolve(makeTime({ phase }), makeGroupTimer()).phase).toBe(phase); + }); +}); + +describe('getShowsTimerValue()', () => { + it('is true for timer types which render the running timer', () => { + expect(getShowsTimerValue(TimerType.CountDown)).toBe(true); + expect(getShowsTimerValue(TimerType.CountUp)).toBe(true); + }); + + it('is false for timer types which do not reflect what is loaded', () => { + expect(getShowsTimerValue(TimerType.Clock)).toBe(false); + expect(getShowsTimerValue(TimerType.None)).toBe(false); + }); +}); diff --git a/apps/client/src/views/timer/timer.utils.ts b/apps/client/src/views/timer/timer.utils.ts index 58b19d052..99a95139d 100644 --- a/apps/client/src/views/timer/timer.utils.ts +++ b/apps/client/src/views/timer/timer.utils.ts @@ -36,9 +36,11 @@ export function getTotalTime(duration: MaybeNumber, addedTime: MaybeNumber): num } /** - * Whether the progress bar should be shown for this timer type + * Whether this timer type renders a value derived from the running timer. + * Clock shows the time of day and none shows nothing, so neither reflects what is loaded, + * which also means neither can carry a progress bar or a group timer indicator */ -export function getShowProgressBar(timerType: TimerType) { +export function getShowsTimerValue(timerType: TimerType) { return timerType !== TimerType.None && timerType !== TimerType.Clock; } @@ -147,11 +149,13 @@ export function resolveTimerDisplay({ hidePhase, }: ResolveTimerDisplayOptions) { const isGroup = groupTimer !== null; - const phase = isGroup - ? groupTimer.current <= 0 - ? TimerPhase.Overtime - : TimerPhase.Default - : time.phase; + const phase = (() => { + if (!isGroup) return time.phase; + // pending and none describe the playback state rather than a threshold, + // they are just as true of the group as they are of the event + if (time.phase === TimerPhase.Pending || time.phase === TimerPhase.None) return time.phase; + return groupTimer.current <= 0 ? TimerPhase.Overtime : TimerPhase.Default; + })(); return { isGroup, diff --git a/apps/server/src/api-mcp/mcp.service.ts b/apps/server/src/api-mcp/mcp.service.ts index 4fbeafef4..3e1b0de37 100644 --- a/apps/server/src/api-mcp/mcp.service.ts +++ b/apps/server/src/api-mcp/mcp.service.ts @@ -54,7 +54,9 @@ export type EventFieldArgs = Partial< >; export type MilestoneFieldArgs = Partial>; export type DelayFieldArgs = Partial>; -export type GroupFieldArgs = Partial>; +export type GroupFieldArgs = Partial< + Pick +>; export type EntryFieldArgs = EventFieldArgs & MilestoneFieldArgs & DelayFieldArgs & GroupFieldArgs; export type TargetRundownArgs = { rundownId?: string }; @@ -170,6 +172,7 @@ function toGroupPatch(args: CreateEntryArgs, id: EntryId): PatchWithId 1 ? patch : null; }