mirror of
https://github.com/cpvalente/ontime.git
synced 2026-08-17 13:23:35 +00:00
refactor(automation): trim the branch to reduce risk
Adoption is low, and the app must not be put at risk fixing that. A measured look at the diff showed the risk was concentrated in exactly two places: the timer's hot path and a new file-writing route. Everything that actually makes automations discoverable — recipes, a panel that explains itself, one-step creation, Test buttons that report — is client-only settings-panel code with zero server churn. This cuts the two pieces that weren't, and simplifies two more that only existed to serve them. Removed entirely: templates. Newest, riskiest, and the least connected to the adoption problem — it helps someone who already uses automations share them, not someone who has never tried the feature. It was also the source of two of the bugs found in review. Removed: the websocket broadcast behind "last fired". reportFired ran inside triggerAutomations, called from onClock every second and from onUpdate. A new protocol message pushed to every connected client, including stage displays, up to once a second per automation, is real new traffic on the busiest code path in the app for a feature still trying to prove it's worth using. The log line stays: it's additive, throttled, nowhere near the hot path once written, and answers the same question — did this run — without a new message. Trimmed: the demo goes from two automations tied together by necessity — a danger-time message and a second automation whose only job was undoing the first on finish — to one, attached by an event-level trigger instead of a global one. Same two discovery wins, an automation visibly fires on Play and per-event triggers are found by opening an event, with no message mutation and nothing to keep in sync. That pairing was also the first thing review found broken. Trimmed: the delete dialog no longer deletes blocking triggers and restores them if the automation still won't delete. That rollback was the other multi-request destructive sequence review found a bug in. It now confirms, names what's blocking, and says to remove global triggers from the Global Triggers list first — a single always-safe request the user already has. What stays: one-step creation (lifecycles picked on the automation form), the recipe library, and the panel legibility work — none of it touches the server or the runtime. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LpbLJVVT26tzWkduck1M9H
This commit is contained in:
Claude
@@ -1,6 +1,5 @@
|
||||
import { PlayableEvent, TimerLifeCycle } from 'ontime-types';
|
||||
|
||||
import { socket } from '../../../adapters/WebsocketAdapter.js';
|
||||
import { logger } from '../../../classes/Logger.js';
|
||||
import { makeRuntimeStoreData } from '../../../stores/__mocks__/runtimeStore.mocks.js';
|
||||
import { RuntimeState } from '../../../stores/runtimeState.js';
|
||||
@@ -653,12 +652,10 @@ describe('testConditions()', () => {
|
||||
*/
|
||||
describe('automation reporting', () => {
|
||||
let logSpy = vi.spyOn(logger, 'info');
|
||||
let socketSpy = vi.spyOn(socket, 'sendAsJson');
|
||||
|
||||
beforeEach(async () => {
|
||||
vi.spyOn(oscClient, 'emitOSC').mockImplementation(() => {});
|
||||
logSpy = vi.spyOn(logger, 'info').mockImplementation(() => {});
|
||||
socketSpy = vi.spyOn(socket, 'sendAsJson').mockImplementation(() => {});
|
||||
|
||||
await deleteAllTriggers();
|
||||
resetAutomationLogState();
|
||||
@@ -753,18 +750,4 @@ describe('automation reporting', () => {
|
||||
triggerAutomations(TimerLifeCycle.onDanger);
|
||||
expect(logSpy).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it('reports a fire to the clients at most once a second, including on continuous lifecycles', async () => {
|
||||
vi.useFakeTimers();
|
||||
await bind('reporting-clock', TimerLifeCycle.onClock);
|
||||
socketSpy.mockClear();
|
||||
|
||||
triggerAutomations(TimerLifeCycle.onClock);
|
||||
triggerAutomations(TimerLifeCycle.onClock);
|
||||
expect(socketSpy).toHaveBeenCalledTimes(1);
|
||||
|
||||
vi.advanceTimersByTime(1001);
|
||||
triggerAutomations(TimerLifeCycle.onClock);
|
||||
expect(socketSpy).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -4,7 +4,6 @@ import {
|
||||
type AutomationOutput,
|
||||
type FilterRule,
|
||||
LogOrigin,
|
||||
MessageTag,
|
||||
RuntimeStore,
|
||||
TimerLifeCycle,
|
||||
isHTTPOutput,
|
||||
@@ -13,7 +12,6 @@ import {
|
||||
} from 'ontime-types';
|
||||
import { getPropertyFromPath } from 'ontime-utils';
|
||||
|
||||
import { socket } from '../../adapters/WebsocketAdapter.js';
|
||||
import { logger } from '../../classes/Logger.js';
|
||||
import { isOntimeCloud } from '../../setup/environment.js';
|
||||
import { eventStore } from '../../stores/EventStore.js';
|
||||
@@ -37,8 +35,6 @@ const reportThrottleMs = 1000;
|
||||
const suppressionNotices = new Set<string>();
|
||||
/** last time we logged a given automation + cycle pair */
|
||||
const lastLoggedAt = new Map<string, number>();
|
||||
/** last time we told the clients about a given automation */
|
||||
const lastReportedAt = new Map<string, number>();
|
||||
|
||||
/**
|
||||
* Clears the reporting state.
|
||||
@@ -48,7 +44,6 @@ const lastReportedAt = new Map<string, number>();
|
||||
export function resetAutomationLogState() {
|
||||
suppressionNotices.clear();
|
||||
lastLoggedAt.clear();
|
||||
lastReportedAt.clear();
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -115,19 +110,11 @@ function fireForCycle(cycle: TimerLifeCycle) {
|
||||
}
|
||||
|
||||
/**
|
||||
* Makes a successful automation visible, which it previously was not:
|
||||
* the log answers what happened, the socket message answers whether an automation is alive
|
||||
* Makes a successful automation fire visible in the log, which it previously was not
|
||||
*/
|
||||
function reportFired(automationId: string, automation: Automation, cycle: TimerLifeCycle) {
|
||||
const now = Date.now();
|
||||
|
||||
// the panel shows a last fired time, so continuous lifecycles still report, but at most once a second
|
||||
const lastReported = lastReportedAt.get(automationId);
|
||||
if (lastReported === undefined || now - lastReported >= reportThrottleMs) {
|
||||
lastReportedAt.set(automationId, now);
|
||||
socket.sendAsJson(MessageTag.AutomationFired, { automationId, cycle });
|
||||
}
|
||||
|
||||
if (continuousCycles.includes(cycle)) {
|
||||
// one notice per load is enough to explain why the log goes quiet from here
|
||||
if (!suppressionNotices.has(automationId)) {
|
||||
|
||||
@@ -231,30 +231,6 @@ export async function loadDemo(_req: Request, res: Response<MessageResponse | Er
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Creates a template: a new project file containing only the selected sections of an existing one.
|
||||
* The result is not loaded, so making a template does not disturb the running show.
|
||||
*/
|
||||
export async function partialDuplicateProjectFile(req: Request, res: Response<{ filename: string } | ErrorResponse>) {
|
||||
const { filename } = req.params;
|
||||
const { newFilename, sections } = req.body;
|
||||
|
||||
try {
|
||||
// the created name can differ from what was asked for, generateUniqueFileName resolves collisions
|
||||
const created = await projectService.createProjectFromSections(filename, newFilename, sections);
|
||||
|
||||
res.status(201).send({ filename: created });
|
||||
} catch (error) {
|
||||
const message = getErrorMessage(error);
|
||||
if (message.startsWith('Project file')) {
|
||||
res.status(403).send({ message });
|
||||
return;
|
||||
}
|
||||
|
||||
res.status(500).send({ message });
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Duplicates a project file.
|
||||
* Receives the original project filename (`filename`) from the request parameters
|
||||
|
||||
@@ -8,7 +8,6 @@ import {
|
||||
listProjects,
|
||||
loadDemo,
|
||||
loadProject,
|
||||
partialDuplicateProjectFile,
|
||||
patchPartialProjectFile,
|
||||
postProjectFile,
|
||||
projectDownload,
|
||||
@@ -23,7 +22,6 @@ import {
|
||||
validateNewProject,
|
||||
validatePatchProject,
|
||||
validateQuickProject,
|
||||
validateSectionsBody,
|
||||
} from './db.validation.js';
|
||||
|
||||
export const router: Router = express.Router();
|
||||
@@ -41,12 +39,5 @@ router.get('/all', listProjects);
|
||||
router.post('/load', validateFilenameBody, loadProject);
|
||||
router.post('/demo', loadDemo);
|
||||
router.post('/:filename/duplicate', validateFilenameParam, validateNewFilenameBody, duplicateProjectFile);
|
||||
router.post(
|
||||
'/:filename/partial-duplicate',
|
||||
validateFilenameParam,
|
||||
validateNewFilenameBody,
|
||||
validateSectionsBody,
|
||||
partialDuplicateProjectFile,
|
||||
);
|
||||
router.put('/:filename/rename', validateFilenameParam, validateNewFilenameBody, renameProjectFile);
|
||||
router.delete('/:filename', validateFilenameParam, deleteProjectFile);
|
||||
|
||||
@@ -1,5 +1,4 @@
|
||||
import { body, param } from 'express-validator';
|
||||
import { isTemplateSection, templateSections } from 'ontime-types';
|
||||
import sanitize from 'sanitize-filename';
|
||||
|
||||
import { ensureJsonExtension } from '../../utils/fileManagement.js';
|
||||
@@ -68,21 +67,6 @@ export const validateNewFilenameBody = [
|
||||
requestValidationFunction,
|
||||
];
|
||||
|
||||
/**
|
||||
* @description Validates a request to clone selected sections of a project into a template.
|
||||
*/
|
||||
export const validateSectionsBody = [
|
||||
body('sections')
|
||||
.isArray({ min: 1 })
|
||||
.withMessage(`Select at least one of: ${templateSections.join(', ')}`)
|
||||
.custom((sections: unknown[]) =>
|
||||
sections.every((section) => typeof section === 'string' && isTemplateSection(section)),
|
||||
)
|
||||
.withMessage(`Sections must be any of: ${templateSections.join(', ')}`),
|
||||
|
||||
requestValidationFunction,
|
||||
];
|
||||
|
||||
/**
|
||||
* @description Validates request with filename in the body.
|
||||
*/
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { DatabaseModel, OntimeView, TimerLifeCycle } from 'ontime-types';
|
||||
import { DatabaseModel, OntimeView } from 'ontime-types';
|
||||
|
||||
import { backstageRundown, broadcastRundown, stageRundown } from './demoRundowns.js';
|
||||
|
||||
@@ -77,12 +77,14 @@ export const demoDb: DatabaseModel = {
|
||||
},
|
||||
},
|
||||
/**
|
||||
* The demo ships with working automations so the engine is visible the first time
|
||||
* The demo ships with a working automation so the engine is visible the first time
|
||||
* someone presses Play, rather than hidden behind an empty settings panel.
|
||||
*
|
||||
* Everything that actually fires is an Ontime action: the demo must not put traffic
|
||||
* on whatever network it happens to be opened on. The OSC entry is there to be read
|
||||
* and edited, and is deliberately left without a trigger.
|
||||
* It fires an Ontime action: the demo must not put traffic on whatever network it
|
||||
* happens to be opened on. It is attached via an event-level trigger rather than a
|
||||
* global one, so per-event triggers are also discoverable by browsing the rundown
|
||||
* instead of reading docs. The OSC entry is there to be read and edited, and is
|
||||
* deliberately left without a trigger of its own.
|
||||
*
|
||||
* The ids are hand written and must match the map keys. Ids are only generated for
|
||||
* automations created through the DAO, so literals are safe here.
|
||||
@@ -92,20 +94,7 @@ export const demoDb: DatabaseModel = {
|
||||
// never open a listening socket without the user asking for it
|
||||
enabledOscIn: false,
|
||||
oscPortIn: 8888,
|
||||
triggers: [
|
||||
{
|
||||
id: 'demo-trigger-aux',
|
||||
title: 'Demo: aux timer on start',
|
||||
trigger: TimerLifeCycle.onStart,
|
||||
automationId: 'demo-aux-timer',
|
||||
},
|
||||
{
|
||||
id: 'demo-trigger-clear',
|
||||
title: 'Demo: clear the wrap up warning',
|
||||
trigger: TimerLifeCycle.onFinish,
|
||||
automationId: 'demo-clear-message',
|
||||
},
|
||||
],
|
||||
triggers: [],
|
||||
automations: {
|
||||
'demo-aux-timer': {
|
||||
id: 'demo-aux-timer',
|
||||
@@ -117,23 +106,6 @@ export const demoDb: DatabaseModel = {
|
||||
{ type: 'ontime', action: 'aux1-start' },
|
||||
],
|
||||
},
|
||||
'demo-danger-message': {
|
||||
id: 'demo-danger-message',
|
||||
title: 'Demo: warn the stage at danger',
|
||||
filterRule: 'all',
|
||||
filters: [],
|
||||
// self labelled, so nobody mistakes it for something Ontime does on its own
|
||||
outputs: [{ type: 'ontime', action: 'message-set', text: 'Demo automation: please wrap up', visible: true }],
|
||||
},
|
||||
'demo-clear-message': {
|
||||
id: 'demo-clear-message',
|
||||
title: 'Demo: clear the wrap up warning',
|
||||
filterRule: 'all',
|
||||
filters: [],
|
||||
// the pair to the warning above. Without it the message would stay on the stage
|
||||
// timer for the rest of the session, blanking the countdown on every later event
|
||||
outputs: [{ type: 'ontime', action: 'message-set', text: '', visible: false }],
|
||||
},
|
||||
'demo-osc-example': {
|
||||
id: 'demo-osc-example',
|
||||
title: 'Demo: OSC to a lighting console (example, not wired up)',
|
||||
|
||||
@@ -125,9 +125,9 @@ export const stageRundown: Rundown = {
|
||||
triggers: [
|
||||
{
|
||||
id: 'demo-event-trigger',
|
||||
title: 'Wrap up warning',
|
||||
trigger: TimerLifeCycle.onDanger,
|
||||
automationId: 'demo-danger-message',
|
||||
title: 'Aux timer with the event',
|
||||
trigger: TimerLifeCycle.onStart,
|
||||
automationId: 'demo-aux-timer',
|
||||
},
|
||||
],
|
||||
},
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
import { copyFile, writeFile } from 'fs/promises';
|
||||
import { copyFile } from 'fs/promises';
|
||||
import { join } from 'path';
|
||||
|
||||
import { DatabaseModel, LogOrigin, ProjectFileListResponse, TemplateSection } from 'ontime-types';
|
||||
import { DatabaseModel, LogOrigin, ProjectFileListResponse } from 'ontime-types';
|
||||
import { getErrorMessage, getFirstRundown } from 'ontime-utils';
|
||||
|
||||
import { parseCustomFields } from '../../api-data/custom-fields/customFields.parser.js';
|
||||
@@ -9,7 +9,7 @@ import { parseDatabaseModel } from '../../api-data/db/db.parser.js';
|
||||
import { getCurrentRundown } from '../../api-data/rundown/rundown.dao.js';
|
||||
import { parseRundowns } from '../../api-data/rundown/rundown.parser.js';
|
||||
import { initRundown } from '../../api-data/rundown/rundown.service.js';
|
||||
import { flushPendingWrites, getDataProvider, initPersistence } from '../../classes/data-provider/DataProvider.js';
|
||||
import { getDataProvider, initPersistence } from '../../classes/data-provider/DataProvider.js';
|
||||
import { safeMerge } from '../../classes/data-provider/DataProvider.utils.js';
|
||||
import { logger } from '../../classes/Logger.js';
|
||||
import { makeNewProject } from '../../models/dataModel.js';
|
||||
@@ -318,60 +318,6 @@ export async function createProjectWithPatch(fileName: string, initialData: Part
|
||||
return createProject(fileName, sanitisedData);
|
||||
}
|
||||
|
||||
/**
|
||||
* Creates a new project file containing only the given sections of an existing one.
|
||||
* This is how a user makes a template: a small project holding, say, only automations
|
||||
* or only custom fields, which can then be shared and applied with a partial load.
|
||||
*
|
||||
* Unlike createProjectWithPatch, this does NOT load the result. Saving a template must
|
||||
* not pull the operator out of the show they are running.
|
||||
*
|
||||
* @throws if the source does not exist or cannot be parsed
|
||||
*/
|
||||
export async function createProjectFromSections(
|
||||
sourceFilename: string,
|
||||
newFilename: string,
|
||||
sections: TemplateSection[],
|
||||
): Promise<string> {
|
||||
const projectFilePath = doesProjectExist(sourceFilename);
|
||||
if (projectFilePath === null) {
|
||||
throw new Error('Project file not found');
|
||||
}
|
||||
|
||||
if (sections.length === 0) {
|
||||
throw new Error('At least one section must be selected');
|
||||
}
|
||||
|
||||
// writes are debounced, and the natural flow here is "make some automations, then save them
|
||||
// as a template". Without this the template silently misses anything from the last few seconds
|
||||
await flushPendingWrites();
|
||||
|
||||
const fileData = await parseJsonFile(projectFilePath);
|
||||
const { data } = parseDatabaseModel(fileData);
|
||||
|
||||
const patch: Partial<DatabaseModel> = {};
|
||||
for (const section of sections) {
|
||||
// a rundown without its custom fields is not usable on the other side
|
||||
if (section === 'rundowns') {
|
||||
patch.customFields = data.customFields;
|
||||
}
|
||||
Object.assign(patch, { [section]: data[section] });
|
||||
}
|
||||
|
||||
const template = safeMerge(makeNewProject(), patch);
|
||||
|
||||
// makeNewProject seeds an empty rundown and safeMerge merges rundowns by key, so without
|
||||
// this the template would ship a phantom "Default" rundown alongside the real ones
|
||||
if (patch.rundowns !== undefined) {
|
||||
template.rundowns = patch.rundowns;
|
||||
}
|
||||
|
||||
const fileNameWithExtension = generateUniqueFileName(publicDir.projectsDir, ensureJsonExtension(newFilename));
|
||||
await writeFile(getPathToProject(fileNameWithExtension), JSON.stringify(template, null, 2), 'utf-8');
|
||||
|
||||
return fileNameWithExtension;
|
||||
}
|
||||
|
||||
/**
|
||||
* Deletes a project file
|
||||
*/
|
||||
|
||||
@@ -1,17 +1,8 @@
|
||||
import { writeFile } from 'fs/promises';
|
||||
|
||||
import { OntimeView, TimerLifeCycle } from 'ontime-types';
|
||||
import { Mock } from 'vitest';
|
||||
|
||||
import { makeNewProject } from '../../../models/dataModel.js';
|
||||
import { isLastLoadedProject } from '../../app-state-service/AppStateService.js';
|
||||
import {
|
||||
createProjectFromSections,
|
||||
deleteProjectFile,
|
||||
duplicateProjectFile,
|
||||
renameProjectFile,
|
||||
} from '../ProjectService.js';
|
||||
import { doesProjectExist, parseJsonFile } from '../projectServiceUtils.js';
|
||||
import { deleteProjectFile, duplicateProjectFile, renameProjectFile } from '../ProjectService.js';
|
||||
import { doesProjectExist } from '../projectServiceUtils.js';
|
||||
|
||||
// stop the database loading from initiating
|
||||
vi.mock('../../../setup/loadDb.js', () => {
|
||||
@@ -26,13 +17,7 @@ vi.mock('../../app-state-service/AppStateService.js', () => ({
|
||||
|
||||
vi.mock('../projectServiceUtils.js', () => ({
|
||||
doesProjectExist: vi.fn(),
|
||||
getPathToProject: vi.fn().mockImplementation((name: string) => `/projects/${name}`),
|
||||
parseJsonFile: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock('fs/promises', async (importOriginal) => ({
|
||||
...(await importOriginal<typeof import('fs/promises')>()),
|
||||
writeFile: vi.fn(),
|
||||
getPathToProject: vi.fn(),
|
||||
}));
|
||||
|
||||
/**
|
||||
@@ -80,54 +65,3 @@ describe('renameProjectFile', () => {
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('createProjectFromSections', () => {
|
||||
it('throws an error if origin project does not exist', async () => {
|
||||
(doesProjectExist as Mock).mockReturnValue(null);
|
||||
await expect(createProjectFromSections('does not exist', 'template', ['automation'])).rejects.toThrow(
|
||||
'Project file not found',
|
||||
);
|
||||
});
|
||||
|
||||
it('throws an error if nothing was selected', async () => {
|
||||
(doesProjectExist as Mock).mockReturnValue('/projects/source.json');
|
||||
await expect(createProjectFromSections('source.json', 'template', [])).rejects.toThrow(
|
||||
'At least one section must be selected',
|
||||
);
|
||||
});
|
||||
|
||||
it('writes a project carrying only the selected sections', async () => {
|
||||
(doesProjectExist as Mock).mockReturnValue('/projects/source.json');
|
||||
(parseJsonFile as Mock).mockResolvedValue({
|
||||
...makeNewProject(),
|
||||
urlPresets: [{ target: OntimeView.Timer, enabled: true, alias: 'from-source', search: '', displayInNav: false }],
|
||||
automation: {
|
||||
enabledAutomations: true,
|
||||
enabledOscIn: false,
|
||||
oscPortIn: 8888,
|
||||
triggers: [{ id: 't1', title: 'on start', trigger: TimerLifeCycle.onStart, automationId: 'a1' }],
|
||||
automations: {
|
||||
a1: {
|
||||
id: 'a1',
|
||||
title: 'from source',
|
||||
filterRule: 'all',
|
||||
filters: [],
|
||||
outputs: [{ type: 'http', url: 'http://127.0.0.1/go' }],
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
await createProjectFromSections('source.json', 'template.json', ['automation']);
|
||||
|
||||
expect(writeFile).toHaveBeenCalledOnce();
|
||||
const written = JSON.parse((writeFile as Mock).mock.calls[0][1] as string);
|
||||
|
||||
// the automations came across, triggers included
|
||||
expect(written.automation.automations.a1.title).toBe('from source');
|
||||
expect(written.automation.triggers).toHaveLength(1);
|
||||
|
||||
// and the url preset, which parses cleanly but was not selected, did not
|
||||
expect(written.urlPresets).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user