From ef472b513a1e845aab4f84cb54b382752b7349b4 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 05:43:48 +0000 Subject: [PATCH] feat(report): tie events to their group, split running time from finishing time Three changes from reviewing the panel against a real run. Events are bracketed by the group they belong to. A coloured rail in the group's own colour runs down the heading and every event under it, closed by a foot where the group ends. This is the shape the rundown editor already uses for a group, so the report reads with the same grammar as the rundown it reports on, and an event can no longer be mistaken for belonging to the group above it. The show summary no longer reduces the run to one figure. Finishing time and running time answer different questions and can point opposite ways: a show which starts early and runs over still finishes early, and calling that a single "total plan deviation" reported it as having come in under when it had in fact overrun. The two are now separate: the headline is the show against its planned duration, which is what the team controls and what carries into the next run of the same rundown, and the finishing time sits beside it as its own row alongside the start it inherited. ShowOffsets gains plannedDuration, actualDuration and durationOffset, replacing duringShow, which was the same figure under a name that described neither. Nothing is forced into upper case any more. The summary labels are sentence case, and event rows carry their values in td rather than th: the panel's th styling is meant for column headings, so every event title in the report was being rendered small, bold and shouting. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019nr3FbLbM8gB8Jm771YgTV --- .../__tests__/reportSettings.utils.test.ts | 26 +---- .../composite/ReportShowSummary.tsx | 47 +++++----- .../composite/ReportSummaryCard.module.scss | 12 ++- .../composite/ReportSummaryCard.tsx | 4 +- .../composite/ReportTable.module.scss | 47 ++++++++-- .../feature-panel/composite/ReportTable.tsx | 60 +++++++----- .../feature-panel/reportSettings.utils.ts | 20 ---- .../types/src/definitions/core/Report.type.ts | 19 +++- .../src/report-utils/reportUtils.test.ts | 94 ++++++++++++++----- .../utils/src/report-utils/reportUtils.ts | 22 ++++- 10 files changed, 219 insertions(+), 132 deletions(-) diff --git a/apps/client/src/features/app-settings/panel/feature-panel/__tests__/reportSettings.utils.test.ts b/apps/client/src/features/app-settings/panel/feature-panel/__tests__/reportSettings.utils.test.ts index 895cec2a5..cafedff4f 100644 --- a/apps/client/src/features/app-settings/panel/feature-panel/__tests__/reportSettings.utils.test.ts +++ b/apps/client/src/features/app-settings/panel/feature-panel/__tests__/reportSettings.utils.test.ts @@ -8,7 +8,7 @@ import { TimerType, } from 'ontime-types'; -import { deviationBreakdown, formatOffset, getCombinedReport, makeReportCSV } from '../reportSettings.utils'; +import { formatOffset, getCombinedReport, makeReportCSV } from '../reportSettings.utils'; function makeEvent(patch: Partial): OntimeEvent { return { @@ -191,27 +191,3 @@ describe('makeReportCSV()', () => { expect(rows[1]).toContain('Act 1'); }); }); - -describe('deviationBreakdown()', () => { - it('separates a late start from time the show itself lost', () => { - // finished 4m12s behind, but only 2m12s of that was the show's doing - expect(deviationBreakdown(120000, 132000)).toBe('started +2m, +2m12s during the show'); - }); - - it('credits a show which made up a late start', () => { - expect(deviationBreakdown(300000, -120000)).toBe('started +5m, -2m during the show'); - }); - - it('reads a punctual start as such rather than as an offset of zero', () => { - expect(deviationBreakdown(0, 60000)).toBe('started on time, +1m during the show'); - }); - - it('says the show held schedule when it neither lost nor gained', () => { - expect(deviationBreakdown(120000, 0)).toBe('started +2m, held schedule from there'); - }); - - it('has nothing to say without both halves', () => { - expect(deviationBreakdown(null, 1000)).toBe(''); - expect(deviationBreakdown(1000, null)).toBe(''); - }); -}); diff --git a/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportShowSummary.tsx b/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportShowSummary.tsx index dd7f8b08d..9f9bc3f3c 100644 --- a/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportShowSummary.tsx +++ b/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportShowSummary.tsx @@ -2,10 +2,8 @@ import { MaybeNumber, RunSummary, ShowReport } from 'ontime-types'; import { getShowOffsets } from 'ontime-utils'; import { useMemo } from 'react'; -import Tooltip from '../../../../../common/components/tooltip/Tooltip'; import { enDash } from '../../../../../common/utils/styleUtils'; import { formatDuration, formatTime } from '../../../../../common/utils/time'; -import { deviationBreakdown } from '../reportSettings.utils'; import ReportSummaryCard, { FooterItem, Metric } from './ReportSummaryCard'; import style from './ReportShowSummary.module.scss'; @@ -18,20 +16,21 @@ interface ReportShowSummaryProps { } /** - * Leads the report with how far the show finished from where it was planned to. + * Leads the report with whether the show ran to the length it was planned for. * - * That single figure is what a producer is asked about afterwards, so it is - * the headline. It is then split into the part inherited from a late start and - * the part the show itself moved, which have different causes and different - * remedies. + * Running time is the headline rather than finishing time because it is the + * part the team controls and the part that carries into the next run of the + * same rundown. Finishing time is the other question a report is asked, and + * the two can point opposite ways, so it stays beside it as its own row + * rather than being folded into a single figure. */ export default function ReportShowSummary({ rundownTitle, show, summary, worstOverrunTitle }: ReportShowSummaryProps) { const offsets = useMemo(() => getShowOffsets(show), [show]); /** * A show that stopped early has no meaningful end: its last event is simply - * where it got to. Comparing that against the planned end would report a - * large recovery for a show that never finished. + * where it got to. Measuring that against the plan would report a show which + * never finished as having come in comfortably short. */ const didReachEnd = summary.eventsRun > 0 && summary.eventsRun === summary.eventsPlanned; const hasPlan = offsets.startOffset !== null; @@ -40,19 +39,12 @@ export default function ReportShowSummary({ rundownTitle, show, summary, worstOv
} - > - {deviationBreakdown(offsets.startOffset, offsets.duringShow)} - - ) : ( - 'The show did not reach the end of the rundown, so there is nothing to compare the finish against.' - ) + didReachEnd + ? undefined + : 'The show did not reach the end of the rundown, so there is nothing to measure it against.' } footer={ <> @@ -87,6 +79,15 @@ export default function ReportShowSummary({ rundownTitle, show, summary, worstOv offset={offsets.endOffset} /> )} + {didReachEnd && ( + // no offset: the headline is this row's offset, and repeating it here + // would read as a second, different figure + + )}
); @@ -95,3 +96,7 @@ export default function ReportShowSummary({ rundownTitle, show, summary, worstOv function formatMaybeTime(value: MaybeNumber): string { return value === null ? enDash : formatTime(value); } + +function formatMaybeDuration(value: MaybeNumber): string { + return value === null ? enDash : formatDuration(value, false); +} diff --git a/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportSummaryCard.module.scss b/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportSummaryCard.module.scss index 8e9ec4c10..c8c466f59 100644 --- a/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportSummaryCard.module.scss +++ b/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportSummaryCard.module.scss @@ -18,11 +18,17 @@ // at this weight the conclusion fits beside its label rather than under it, // keeping a group heading to two lines however many of them the table has .headline { - flex-direction: row; + flex-flow: row wrap; align-items: baseline; gap: 0.625rem; } + // the conclusion sits beside its label, but a reason why there is none + // needs the full line rather than pushing the workings off to the right + .note { + flex-basis: 100%; + } + .headlineValue { font-size: 1.25rem; } @@ -75,9 +81,7 @@ } .headlineLabel { - font-size: calc(1rem - 3px); - text-transform: uppercase; - letter-spacing: 0.04em; + font-size: calc(1rem - 2px); color: $gray-300; } diff --git a/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportSummaryCard.tsx b/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportSummaryCard.tsx index d81d4a6ab..70b83b1de 100644 --- a/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportSummaryCard.tsx +++ b/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportSummaryCard.tsx @@ -20,6 +20,7 @@ interface ReportSummaryCardProps { footer?: ReactNode; /** a subordinate tier, eg a group inside the show it belongs to */ compact?: boolean; + className?: string; } /** @@ -38,10 +39,11 @@ export default function ReportSummaryCard({ note, footer, compact, + className, children, }: PropsWithChildren) { return ( -
+
{title} {subtitle && {subtitle}} diff --git a/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportTable.module.scss b/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportTable.module.scss index 3abcfb00b..faf0d82b4 100644 --- a/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportTable.module.scss +++ b/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportTable.module.scss @@ -1,8 +1,15 @@ -th.over { +// the rail the whole group hangs off, falling back to a neutral edge when the +// user gave the group no colour of its own +$rail-width: 3px; +$rail-colour: var(--group-colour, #{$gray-500}); + +// event rows carry their values in td, not th: the panel's th styling is meant +// for column headings and renders whatever it holds small, bold and upper case +td.over { color: $playback-over; } -th.under { +td.under { color: $playback-under; } @@ -12,16 +19,38 @@ th.under { background-color: transparent; td { - padding: 1.25rem 0 0.5rem; + padding: 1.25rem 0 0; } } -.groupColour { - width: 0.25rem; - height: 1rem; - border-radius: 2px; - background-color: var(--group-colour, #{$gray-500}); - flex-shrink: 0; +// a continuous coloured edge down the heading and every event under it, closed +// by the cap below. Mirrors how the rundown editor brackets a group, so the +// report reads with the same grammar as the rundown it reports on +.grouped { + box-shadow: inset $rail-width 0 $rail-colour; +} + +td.grouped { + // clear the rail rather than sitting against it + padding-left: 0.75rem; +} + +.groupEndRow { + background-color: transparent; + + td { + padding: 0 0 1.25rem; + } +} + +.groupEnd { + display: block; + height: 0.5rem; + background-color: $rail-colour; + border-radius: 0 0 $rail-width $rail-width; + // the rail tapering off, not a second element: any wider and the foot reads + // as an underline belonging to the last event rather than to the group + width: $rail-width; } .eventCue, diff --git a/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportTable.tsx b/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportTable.tsx index a93fadce7..ba3321e7b 100644 --- a/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportTable.tsx +++ b/apps/client/src/features/app-settings/panel/feature-panel/composite/ReportTable.tsx @@ -42,8 +42,9 @@ export default function ReportTable({ rows, groups }: ReportTableProps) { {section.group && } {section.rows.map((entry) => ( - + ))} + {section.group && } ))} @@ -62,14 +63,10 @@ function GroupRow({ group }: { group: GroupReport }) { return ( - + - - {group.title || 'Untitled group'} - - } + className={style.grouped} + title={group.title || 'Untitled group'} subtitle={ group.actualStart !== null && group.actualEnd !== null ? `ran ${formatTime(group.actualStart)} to ${formatTime(group.actualEnd)}` @@ -78,13 +75,8 @@ function GroupRow({ group }: { group: GroupReport }) { compact headlineLabel={hasTarget ? 'Against target' : 'Against schedule'} headline={group.variance} - note={ - group.variance !== null - ? undefined - : isComplete - ? 'This group has not run.' - : 'Still to finish, so there is nothing to measure the group against yet.' - } + // terse, because the footer already carries how much of the group ran + note={group.variance !== null ? undefined : isComplete ? 'Did not run' : 'Still running'} footer={ <> @@ -118,23 +110,43 @@ function GroupRow({ group }: { group: GroupReport }) { ); } -function EventRow({ entry }: { entry: CombinedReport }) { +function EventRow({ entry, colour }: { entry: CombinedReport; colour?: string }) { const start = punctuality(entry.actualStart, entry.scheduledStart); const end = punctuality(entry.actualEnd, entry.scheduledEnd); + const grouped = colour !== undefined; return ( - - {entry.index} - {entry.cue} - {entry.title} - {formatTime(entry.scheduledStart)} - {formatTime(entry.actualStart)} - {formatTime(entry.scheduledEnd)} - {formatTime(entry.actualEnd)} + + {entry.index} + {entry.cue} + {entry.title} + {formatTime(entry.scheduledStart)} + {formatTime(entry.actualStart)} + {formatTime(entry.scheduledEnd)} + {formatTime(entry.actualEnd)} ); } +/** + * Closes the group off the way the rundown editor does, so the events between + * the heading and this cap read as belonging to it rather than following it. + */ +function GroupEndRow({ colour }: { colour: string }) { + return ( + + + + + + ); +} + +/** The group's own colour, or a neutral rail when the user set none */ +function groupColour(colour?: string): React.CSSProperties { + return { '--group-colour': colour || undefined } as React.CSSProperties; +} + /** Whether an actual time landed before (under) or after (over) its schedule */ function punctuality(actual: number | null, scheduled: number): 'under' | 'over' | null { if (actual === null) return null; diff --git a/apps/client/src/features/app-settings/panel/feature-panel/reportSettings.utils.ts b/apps/client/src/features/app-settings/panel/feature-panel/reportSettings.utils.ts index e23d8aa84..4fcbd281d 100644 --- a/apps/client/src/features/app-settings/panel/feature-panel/reportSettings.utils.ts +++ b/apps/client/src/features/app-settings/panel/feature-panel/reportSettings.utils.ts @@ -79,26 +79,6 @@ export function formatOffset(value: MaybeNumber): string { return `${value > 0 ? '+' : '-'}${formatDuration(Math.abs(value), false)}`; } -/** - * Reads a show's total plan deviation back as the two parts it is made of, so - * a show which finished late because it began late is not mistaken for one - * which overran. - */ -export function deviationBreakdown(startOffset: MaybeNumber, duringShow: MaybeNumber): string { - if (startOffset === null || duringShow === null) { - return ''; - } - - const fromStart = - Math.abs(startOffset) < MILLIS_PER_SECOND ? 'started on time' : `started ${formatOffset(startOffset)}`; - const inShow = - Math.abs(duringShow) < MILLIS_PER_SECOND - ? 'held schedule from there' - : `${formatOffset(duringShow)} during the show`; - - return `${fromStart}, ${inShow}`; -} - /** Whether an offset is behind, ahead, or neither, for colouring */ export function offsetTone(value: MaybeNumber): 'over' | 'under' | 'none' { if (value === null || Math.abs(value) < MILLIS_PER_SECOND) return 'none'; diff --git a/packages/types/src/definitions/core/Report.type.ts b/packages/types/src/definitions/core/Report.type.ts index 40b954242..877d15517 100644 --- a/packages/types/src/definitions/core/Report.type.ts +++ b/packages/types/src/definitions/core/Report.type.ts @@ -29,19 +29,28 @@ export type ShowReport = { }; /** - * How the show sat against its plan. + * How the show sat against its plan, as the two separate questions it answers. * * Follows Ontime's offset convention: positive means behind schedule. - * `duringShow` separates a late start from a badly run show, which have - * different causes and different remedies. + * + * `endOffset` asks whether the show came off air when it promised to, which is + * what an audience or a venue booking is measured against. `durationOffset` + * asks whether the show itself ran long, which is what the team controls and + * what carries over to the next run of the same rundown. They differ by + * exactly `startOffset`: a show can run over and still finish early if it + * started early, so reporting either one alone is misleading. */ export type ShowOffsets = { /** actual start against planned start */ startOffset: MaybeNumber; /** actual end against planned end */ endOffset: MaybeNumber; - /** time lost (positive) or recovered (negative) between start and end */ - duringShow: MaybeNumber; + /** how long the show was planned to take */ + plannedDuration: MaybeNumber; + /** how long it actually took */ + actualDuration: MaybeNumber; + /** actualDuration against plannedDuration, ie whether the show ran long */ + durationOffset: MaybeNumber; }; /** diff --git a/packages/utils/src/report-utils/reportUtils.test.ts b/packages/utils/src/report-utils/reportUtils.test.ts index 6358624d8..0005347a3 100644 --- a/packages/utils/src/report-utils/reportUtils.test.ts +++ b/packages/utils/src/report-utils/reportUtils.test.ts @@ -78,56 +78,106 @@ describe('getEventVariance()', () => { }); describe('getShowOffsets()', () => { + const HOUR = 60 * MIN; + it('reports a late start carried through to a late end', () => { // started 8 late, ended 8 late: the show itself ran clean const offsets = getShowOffsets({ - plannedStart: 19 * 60 * MIN, - plannedEnd: 21 * 60 * MIN, - actualStart: 19 * 60 * MIN + 8 * MIN, - actualEnd: 21 * 60 * MIN + 8 * MIN, + plannedStart: 19 * HOUR, + plannedEnd: 21 * HOUR, + actualStart: 19 * HOUR + 8 * MIN, + actualEnd: 21 * HOUR + 8 * MIN, }); - expect(offsets).toEqual({ startOffset: 8 * MIN, endOffset: 8 * MIN, duringShow: 0 }); + expect(offsets).toEqual({ + startOffset: 8 * MIN, + endOffset: 8 * MIN, + plannedDuration: 2 * HOUR, + actualDuration: 2 * HOUR, + durationOffset: 0, + }); }); - it('separates time lost during the show from a late start', () => { + it('separates how long the show ran from when it finished', () => { const offsets = getShowOffsets({ - plannedStart: 19 * 60 * MIN, - plannedEnd: 21 * 60 * MIN, - actualStart: 19 * 60 * MIN + 8 * MIN, - actualEnd: 21 * 60 * MIN + 12 * MIN, + plannedStart: 19 * HOUR, + plannedEnd: 21 * HOUR, + actualStart: 19 * HOUR + 8 * MIN, + actualEnd: 21 * HOUR + 12 * MIN, }); - expect(offsets).toMatchObject({ startOffset: 8 * MIN, endOffset: 12 * MIN, duringShow: 4 * MIN }); + expect(offsets).toMatchObject({ startOffset: 8 * MIN, endOffset: 12 * MIN, durationOffset: 4 * MIN }); }); - it('reports time recovered as a negative', () => { + it('reports a show which ran short as a negative', () => { const offsets = getShowOffsets({ - plannedStart: 19 * 60 * MIN, - plannedEnd: 21 * 60 * MIN, - actualStart: 19 * 60 * MIN + 10 * MIN, - actualEnd: 21 * 60 * MIN + 2 * MIN, + plannedStart: 19 * HOUR, + plannedEnd: 21 * HOUR, + actualStart: 19 * HOUR + 10 * MIN, + actualEnd: 21 * HOUR + 2 * MIN, }); - expect(offsets.duringShow).toBe(-8 * MIN); + expect(offsets.durationOffset).toBe(-8 * MIN); + }); + + it('can report a show which ran over and still finished early', () => { + // the case which makes reporting either figure alone misleading: + // started 10 early, ran 4 over, so came off 6 early + const offsets = getShowOffsets({ + plannedStart: 19 * HOUR, + plannedEnd: 21 * HOUR, + actualStart: 19 * HOUR - 10 * MIN, + actualEnd: 21 * HOUR - 6 * MIN, + }); + + expect(offsets.endOffset).toBe(-6 * MIN); + expect(offsets.durationOffset).toBe(4 * MIN); + }); + + it('keeps the two offsets exactly one start offset apart', () => { + const offsets = getShowOffsets({ + plannedStart: 19 * HOUR, + plannedEnd: 21 * HOUR, + actualStart: 19 * HOUR + 3 * MIN, + actualEnd: 21 * HOUR + 11 * MIN, + }); + + expect(offsets.endOffset! - offsets.startOffset!).toBe(offsets.durationOffset); }); it('does not read a show ending after midnight as a day early', () => { const offsets = getShowOffsets({ - plannedStart: 23 * 60 * MIN, + plannedStart: 23 * HOUR, plannedEnd: dayInMs - 10 * MIN, - actualStart: 23 * 60 * MIN, + actualStart: 23 * HOUR, actualEnd: 5 * MIN, // ran past midnight }); expect(offsets.endOffset).toBe(15 * MIN); + // 23:00 to 00:05 is an hour and five minutes, not a negative + expect(offsets.actualDuration).toBe(HOUR + 5 * MIN); + expect(offsets.durationOffset).toBe(15 * MIN); }); - it('has nothing to report without a plan', () => { - expect(getShowOffsets({ plannedStart: null, plannedEnd: null, actualStart: 1, actualEnd: 2 })).toEqual({ + it('still reports how long a show ran when it had no plan to run against', () => { + const offsets = getShowOffsets({ plannedStart: null, plannedEnd: null, actualStart: 0, actualEnd: 5 * MIN }); + + // how long it took is knowable, whether that was long or short is not + expect(offsets).toEqual({ startOffset: null, endOffset: null, - duringShow: null, + plannedDuration: null, + actualDuration: 5 * MIN, + durationOffset: null, + }); + }); + + it('has nothing to report for a show which has not run', () => { + expect(getShowOffsets({ plannedStart: 0, plannedEnd: MIN, actualStart: null, actualEnd: null })).toMatchObject({ + startOffset: null, + endOffset: null, + actualDuration: null, + durationOffset: null, }); }); }); diff --git a/packages/utils/src/report-utils/reportUtils.ts b/packages/utils/src/report-utils/reportUtils.ts index 6fadc666d..11496e2cc 100644 --- a/packages/utils/src/report-utils/reportUtils.ts +++ b/packages/utils/src/report-utils/reportUtils.ts @@ -65,18 +65,38 @@ export function getEventVariance(entry: OntimeEventReport | undefined): EventVar * A sum of event overruns cannot answer this: gaps absorb overrun, skipped * events give time back, and a late start moves the whole show without any * event running long. + * + * Finishing time and running time are reported separately because they answer + * different questions and can point opposite ways: a show which starts early + * and runs over still finishes early. */ export function getShowOffsets(show: ShowReport): ShowOffsets { const startOffset = offsetBetween(show.plannedStart, show.actualStart); const endOffset = offsetBetween(show.plannedEnd, show.actualEnd); + const plannedDuration = durationBetween(show.plannedStart, show.plannedEnd); + const actualDuration = durationBetween(show.actualStart, show.actualEnd); + return { startOffset, endOffset, - duringShow: startOffset === null || endOffset === null ? null : endOffset - startOffset, + plannedDuration, + actualDuration, + durationOffset: plannedDuration === null || actualDuration === null ? null : actualDuration - plannedDuration, }; } +/** + * Time from one point in the day to another, or null if either is missing. + * @private + */ +function durationBetween(from: number | null, to: number | null): number | null { + if (from === null || to === null) { + return null; + } + return elapsedBetween(from, to); +} + /** * Signed distance from a planned time to the time it happened. * Positive means late, matching Ontime's offset convention.