From 10dffb5d72afd1cb3129525edb08272caa1b5a11 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 18:51:39 +0000 Subject: [PATCH] Address PR review: deep-clone triggers, stop normalising absent values Replaces the generic type machinery around cloneEntryData with a concrete clone per entry type, and fixes two review findings. Types: ReferenceKeys / EntryOfType / ClonedReferenceFields / UnclonedReferenceFields / AssertNever are gone. There are only four entry types, so each gets its own small function listing every field explicitly - the same pattern the create*Patch functions in this file already use. Adding a field to an entry type still fails to compile until it is handled, but now with a plain "property is missing" error rather than a constraint on never, and the clone reads as the logic it is instead of type gymnastics. The exhaustiveness guard in the switch is kept, so a new SupportedEntry member is still named in a compile error. Fixes, both from review: - triggers were copied with slice(), which shares every Trigger object with the source. A mutable transaction could edit a trigger of the cached rundown before commit. Each trigger is now copied too, making the structuredClone contract actually true - custom and entries were already deep since their values are primitives. - triggers and entries were normalised from undefined to [], so an entry missing either field never compared equal to its own snapshot. That would have the runtime re-broadcast and re-save the restore point on every tick. An absent value is now left absent. Tests: cloneEntryData is compared field-for-field against structuredClone for all four entry types built from the real factories, plus aliasing coverage for custom, triggers (array and elements), and group entries, the absent-value regression, and the unknown-type throw. Each was confirmed to fail when the corresponding fix is reverted. The remaining review comment, about the missing default branch, was already fixed in 4548798. --- .../rundown/__tests__/rundown.utils.test.ts | 90 +++++++++++- .../src/api-data/rundown/rundown.utils.ts | 131 ++++++++++++------ 2 files changed, 174 insertions(+), 47 deletions(-) diff --git a/apps/server/src/api-data/rundown/__tests__/rundown.utils.test.ts b/apps/server/src/api-data/rundown/__tests__/rundown.utils.test.ts index 0e5d9b349..3942935dd 100644 --- a/apps/server/src/api-data/rundown/__tests__/rundown.utils.test.ts +++ b/apps/server/src/api-data/rundown/__tests__/rundown.utils.test.ts @@ -8,13 +8,21 @@ import { TimerType, Trigger, } from 'ontime-types'; -import { MILLIS_PER_HOUR, MILLIS_PER_MINUTE, createEvent } from 'ontime-utils'; +import { + MILLIS_PER_HOUR, + MILLIS_PER_MINUTE, + createDelay, + createEvent, + createGroup, + createMilestone, +} from 'ontime-utils'; import { assertType } from 'vitest'; import { makeOntimeEvent, makeOntimeGroup, makeOntimeMilestone, makeRundown } from '../__mocks__/rundown.mocks.js'; import { parseRundown } from '../rundown.parser.js'; import { calculateDayOffset, + cloneEntryData, deleteById, doesInvalidateMetadata, getIntegerAndFraction, @@ -715,3 +723,83 @@ describe('eventDurationMatchGroupTarget()', () => { expect(result).toStrictEqual(null); }); }); + +describe('cloneEntryData()', () => { + const trigger: Trigger = { + id: 'trigger-1', + title: 'Go on air', + trigger: TimerLifeCycle.onStart, + automationId: 'automation-1', + }; + + const event = createEvent( + { title: 'An event', custom: { sponsor: 'a value' }, triggers: [trigger] }, + 'cue-1', + ) as OntimeEvent; + // the real factories, so these are complete entries exactly as the rundown holds them + const group = createGroup({ id: 'group-1', entries: ['a', 'b'], custom: { sponsor: 'a value' } }); + const milestone = createMilestone({ id: 'milestone-1', custom: { sponsor: 'a value' } }); + const delay = createDelay({ id: 'delay-1', duration: 10 }); + + /** + * The clone exists to replace structuredClone, so it has to agree with it exactly: + * this catches a field being dropped, added or normalised (eg. undefined -> []). + */ + it.each([ + ['event', event], + ['group', group], + ['milestone', milestone], + ['delay', delay], + ])('produces the same value as structuredClone for a %s', (_label, entry) => { + expect(cloneEntryData(entry)).toStrictEqual(structuredClone(entry)); + }); + + it('gives the event a custom object that is not shared with the source', () => { + const clone = cloneEntryData(event); + clone.custom.sponsor = 'changed'; + expect(event.custom.sponsor).toBe('a value'); + }); + + it('gives the event a triggers array, and each trigger, that is not shared with the source', () => { + const clone = cloneEntryData(event); + expect(clone.triggers).not.toBe(event.triggers); + expect(clone.triggers[0]).not.toBe(event.triggers[0]); + + clone.triggers[0].title = 'changed'; + clone.triggers.push({ ...trigger, id: 'trigger-2' }); + + expect(event.triggers[0].title).toBe('Go on air'); + expect(event.triggers).toHaveLength(1); + }); + + it('gives the group an entries array that is not shared with the source', () => { + const clone = cloneEntryData(group); + clone.entries.push('c'); + expect(group.entries).toStrictEqual(['a', 'b']); + }); + + it('gives the milestone a custom object that is not shared with the source', () => { + const clone = cloneEntryData(milestone); + clone.custom.sponsor = 'changed'; + expect(milestone.custom.sponsor).toBe('a value'); + }); + + /** + * Regression: normalising an absent value to [] makes deepEqual report a change on every + * comparison, which would have the runtime re-broadcast and re-save the restore point on + * every tick. See PR #2178. + */ + it('leaves an absent triggers or entries value absent rather than normalising it to []', () => { + const partialEvent = makeOntimeEvent({ id: 'partial-event' }); + const partialGroup = makeOntimeGroup({ id: 'partial-group', entries: undefined }); + + expect(cloneEntryData(partialEvent).triggers).toBeUndefined(); + expect(cloneEntryData(partialGroup).entries).toBeUndefined(); + }); + + it('throws on an entry type it does not know how to clone', () => { + expect(() => cloneEntryData({ id: 'x', type: 'unknown' } as unknown as OntimeEvent)).toThrow( + 'Unsupported entry type for cloning', + ); + }); +}); diff --git a/apps/server/src/api-data/rundown/rundown.utils.ts b/apps/server/src/api-data/rundown/rundown.utils.ts index dd7f4ffd1..68515305f 100644 --- a/apps/server/src/api-data/rundown/rundown.utils.ts +++ b/apps/server/src/api-data/rundown/rundown.utils.ts @@ -499,67 +499,106 @@ export function cloneSimpleRundownEntry(entry: OntimeEntry, newId: EntryId): Ont throw new Error(`Unsupported entry type for cloning: ${entry}`); } -type Primitive = string | number | boolean | bigint | symbol | null | undefined; - /** - * The keys of `T` holding a reference (object or array) rather than a primitive: exactly the - * fields a shallow spread aliases instead of copying, and so exactly the fields - * `cloneEntryData` has to give a fresh copy to. - * Branded primitives (eg. `Day`) resolve as primitives, which is what we want - they are - * plain numbers at runtime. + * Fast, shape-aware clones of a rundown entry, one per entry type. + * + * Every field is listed explicitly rather than spread - the same pattern as the + * `create*Patch` functions above - so adding a field to an entry type fails to compile here + * until it is handled, and whoever adds it has to decide whether it needs a copy of its own. + * The nested containers (`custom`, `triggers`, `entries`) get a fresh copy so the result can + * be mutated freely without touching the source, which is what makes these a drop-in + * replacement for `structuredClone` at a fraction of the cost. */ -type ReferenceKeys = { [K in keyof T]-?: T[K] extends Primitive ? never : K }[keyof T]; +function cloneEventData(entry: OntimeEvent): OntimeEvent { + return { + id: entry.id, + type: entry.type, + flag: entry.flag, + cue: entry.cue, + title: entry.title, + note: entry.note, + endAction: entry.endAction, + timerType: entry.timerType, + countToEnd: entry.countToEnd, + linkStart: entry.linkStart, + timeStrategy: entry.timeStrategy, + timeStart: entry.timeStart, + timeEnd: entry.timeEnd, + duration: entry.duration, + skip: entry.skip, + colour: entry.colour, + timeWarning: entry.timeWarning, + timeDanger: entry.timeDanger, + parent: entry.parent, + revision: entry.revision, + delay: entry.delay, + dayOffset: entry.dayOffset, + gap: entry.gap, + custom: { ...entry.custom }, + // the array and each trigger in it, otherwise a mutable transaction could edit a trigger + // of the cached rundown before commit. + // partially formed entries appear in fixtures: keep an absent value absent rather than + // normalising it to [], which would make deepEqual report a change on every comparison + triggers: entry.triggers?.map((trigger) => ({ ...trigger })) as OntimeEvent['triggers'], + }; +} -/** The member of the `OntimeEntry` union carrying a given `type` tag */ -type EntryOfType = Extract; +function cloneGroupData(entry: OntimeGroup): OntimeGroup { + return { + id: entry.id, + type: entry.type, + title: entry.title, + note: entry.note, + targetDuration: entry.targetDuration, + colour: entry.colour, + revision: entry.revision, + timeStart: entry.timeStart, + timeEnd: entry.timeEnd, + duration: entry.duration, + isFirstLinked: entry.isFirstLinked, + custom: { ...entry.custom }, + // ids are strings, so copying the array is enough + entries: entry.entries?.slice() as OntimeGroup['entries'], + }; +} -/** - * The reference-typed fields that `cloneEntryData` below gives a fresh copy to, per entry type. - * This is the one place to update when an entry gains or loses such a field - the shape is - * checked against the real types by `UnclonedReferenceFields`. - */ -type ClonedReferenceFields = { - [SupportedEntry.Event]: 'custom' | 'triggers'; - [SupportedEntry.Group]: 'custom' | 'entries'; - [SupportedEntry.Milestone]: 'custom'; - [SupportedEntry.Delay]: never; -}; +function cloneMilestoneData(entry: OntimeMilestone): OntimeMilestone { + return { + id: entry.id, + type: entry.type, + cue: entry.cue, + title: entry.title, + note: entry.note, + colour: entry.colour, + parent: entry.parent, + revision: entry.revision, + custom: { ...entry.custom }, + }; +} -/** - * Every reference-typed field `cloneEntryData` would alias instead of copy. Stays `never` - * while the clone is complete, so `AssertEntryClonesEveryReference` fails the build when: - * - an entry type gains a reference-typed field -> the field name resolves here, and is - * named in the error, until it is copied in the switch and listed above - * - `SupportedEntry` gains a member -> indexing `ClonedReferenceFields` fails here - * A new primitive field needs no action: the spread already copies it by value. - */ -type UnclonedReferenceFields = { - [K in SupportedEntry]: Exclude>, ClonedReferenceFields[K]>; -}[SupportedEntry]; - -type AssertNever = T; - -/** - * Compile-time guard only, with no runtime meaning - see `UnclonedReferenceFields`. - * Exported because `noUnusedLocals` rejects an unreferenced local type. - */ -export type AssertEntryClonesEveryReference = AssertNever; +function cloneDelayData(entry: OntimeDelay): OntimeDelay { + return { + id: entry.id, + type: entry.type, + duration: entry.duration, + parent: entry.parent, + }; +} /** * Fast, shape-aware clone of a single entry, preserving its identity (id, revision, etc). - * Drop-in replacement for `structuredClone(entry)`: the generic structured-clone - * algorithm does far more work than the plain object spreads an entry this shallow needs. + * Drop-in replacement for `structuredClone(entry)`. */ export function cloneEntryData(entry: T): T { switch (entry.type) { case SupportedEntry.Event: - return { ...entry, custom: { ...entry.custom }, triggers: entry.triggers?.slice() ?? [] } as T; + return cloneEventData(entry) as T; case SupportedEntry.Group: - return { ...entry, custom: { ...entry.custom }, entries: entry.entries?.slice() ?? [] } as T; + return cloneGroupData(entry) as T; case SupportedEntry.Milestone: - return { ...entry, custom: { ...entry.custom } } as T; + return cloneMilestoneData(entry) as T; case SupportedEntry.Delay: - return { ...entry } as T; + return cloneDelayData(entry) as T; default: { // exhaustiveness guard: a new member of `SupportedEntry` is named in the error here const unhandled: never = entry;