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.
This commit is contained in:
Claude
2026-08-24 18:51:39 +00:00
parent 45487985be
commit 10dffb5d72
2 changed files with 174 additions and 47 deletions
@@ -8,13 +8,21 @@ import {
TimerType, TimerType,
Trigger, Trigger,
} from 'ontime-types'; } 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 { assertType } from 'vitest';
import { makeOntimeEvent, makeOntimeGroup, makeOntimeMilestone, makeRundown } from '../__mocks__/rundown.mocks.js'; import { makeOntimeEvent, makeOntimeGroup, makeOntimeMilestone, makeRundown } from '../__mocks__/rundown.mocks.js';
import { parseRundown } from '../rundown.parser.js'; import { parseRundown } from '../rundown.parser.js';
import { import {
calculateDayOffset, calculateDayOffset,
cloneEntryData,
deleteById, deleteById,
doesInvalidateMetadata, doesInvalidateMetadata,
getIntegerAndFraction, getIntegerAndFraction,
@@ -715,3 +723,83 @@ describe('eventDurationMatchGroupTarget()', () => {
expect(result).toStrictEqual(null); 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',
);
});
});
@@ -499,67 +499,106 @@ export function cloneSimpleRundownEntry(entry: OntimeEntry, newId: EntryId): Ont
throw new Error(`Unsupported entry type for cloning: ${entry}`); 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 * Fast, shape-aware clones of a rundown entry, one per entry type.
* fields a shallow spread aliases instead of copying, and so exactly the fields *
* `cloneEntryData` has to give a fresh copy to. * Every field is listed explicitly rather than spread - the same pattern as the
* Branded primitives (eg. `Day`) resolve as primitives, which is what we want - they are * `create*Patch` functions above - so adding a field to an entry type fails to compile here
* plain numbers at runtime. * 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<T> = { [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 */ function cloneGroupData(entry: OntimeGroup): OntimeGroup {
type EntryOfType<K extends SupportedEntry> = Extract<OntimeEntry, { type: K }>; 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'],
};
}
/** function cloneMilestoneData(entry: OntimeMilestone): OntimeMilestone {
* The reference-typed fields that `cloneEntryData` below gives a fresh copy to, per entry type. return {
* This is the one place to update when an entry gains or loses such a field - the shape is id: entry.id,
* checked against the real types by `UnclonedReferenceFields`. type: entry.type,
*/ cue: entry.cue,
type ClonedReferenceFields = { title: entry.title,
[SupportedEntry.Event]: 'custom' | 'triggers'; note: entry.note,
[SupportedEntry.Group]: 'custom' | 'entries'; colour: entry.colour,
[SupportedEntry.Milestone]: 'custom'; parent: entry.parent,
[SupportedEntry.Delay]: never; revision: entry.revision,
}; custom: { ...entry.custom },
};
}
/** function cloneDelayData(entry: OntimeDelay): OntimeDelay {
* Every reference-typed field `cloneEntryData` would alias instead of copy. Stays `never` return {
* while the clone is complete, so `AssertEntryClonesEveryReference` fails the build when: id: entry.id,
* - an entry type gains a reference-typed field -> the field name resolves here, and is type: entry.type,
* named in the error, until it is copied in the switch and listed above duration: entry.duration,
* - `SupportedEntry` gains a member -> indexing `ClonedReferenceFields` fails here parent: entry.parent,
* A new primitive field needs no action: the spread already copies it by value. };
*/ }
type UnclonedReferenceFields = {
[K in SupportedEntry]: Exclude<ReferenceKeys<EntryOfType<K>>, ClonedReferenceFields[K]>;
}[SupportedEntry];
type AssertNever<T extends never> = T;
/**
* Compile-time guard only, with no runtime meaning - see `UnclonedReferenceFields`.
* Exported because `noUnusedLocals` rejects an unreferenced local type.
*/
export type AssertEntryClonesEveryReference = AssertNever<UnclonedReferenceFields>;
/** /**
* Fast, shape-aware clone of a single entry, preserving its identity (id, revision, etc). * Fast, shape-aware clone of a single entry, preserving its identity (id, revision, etc).
* Drop-in replacement for `structuredClone(entry)`: the generic structured-clone * Drop-in replacement for `structuredClone(entry)`.
* algorithm does far more work than the plain object spreads an entry this shallow needs.
*/ */
export function cloneEntryData<T extends OntimeEntry>(entry: T): T { export function cloneEntryData<T extends OntimeEntry>(entry: T): T {
switch (entry.type) { switch (entry.type) {
case SupportedEntry.Event: case SupportedEntry.Event:
return { ...entry, custom: { ...entry.custom }, triggers: entry.triggers?.slice() ?? [] } as T; return cloneEventData(entry) as T;
case SupportedEntry.Group: case SupportedEntry.Group:
return { ...entry, custom: { ...entry.custom }, entries: entry.entries?.slice() ?? [] } as T; return cloneGroupData(entry) as T;
case SupportedEntry.Milestone: case SupportedEntry.Milestone:
return { ...entry, custom: { ...entry.custom } } as T; return cloneMilestoneData(entry) as T;
case SupportedEntry.Delay: case SupportedEntry.Delay:
return { ...entry } as T; return cloneDelayData(entry) as T;
default: { default: {
// exhaustiveness guard: a new member of `SupportedEntry` is named in the error here // exhaustiveness guard: a new member of `SupportedEntry` is named in the error here
const unhandled: never = entry; const unhandled: never = entry;