From 99a274770566291f2779dcd4b50e42d9180622ac Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 15:28:49 +0000 Subject: [PATCH] fix(automation): make a failed save retryable without duplicating triggers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found that syncTriggers diffed additions against the snapshot taken when the form opened, so a save that failed part way through would, on the next attempt, create a second trigger for every cycle it had already created. The same held in reverse: deletes were replayed too, and the server rejects a delete for a trigger that is already gone, so the retry died on its first request. The snapshot now advances as each request succeeds. It is still seeded at mount rather than from the live prop — settings are polled, and a save must not remove a trigger the user could not see — but once a trigger is created or deleted it becomes part of what this form knows the server holds. A retry is then left with only the outstanding work, and unticking a lifecycle whose trigger was created by the failed attempt now removes it rather than orphaning it. Reproducing that turned up a second problem in the same path. A failed save reports itself through setError('root'), which react-hook-form counts against isValid, which disabled Save. The retry the error message asks for was unreachable until the user edited some unrelated field to force revalidation — and editing a field was also what made the duplicate reachable. A root error on its own no longer blocks submitting, and a new attempt clears the previous one rather than leaving it under a successful save. Verified by intercepting the second trigger request: the failed save leaves one trigger, the retry adds only the missing one and keeps the first trigger's id. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AfDKsy6PE3Rbyt32Fg4YKf --- .../automations-panel/AutomationForm.tsx | 50 ++++++++++++------- .../src/api-data/automation/automation.dao.ts | 2 +- 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/apps/client/src/features/app-settings/panel/automations-panel/AutomationForm.tsx b/apps/client/src/features/app-settings/panel/automations-panel/AutomationForm.tsx index d0dafbffe..73bb22ebd 100644 --- a/apps/client/src/features/app-settings/panel/automations-panel/AutomationForm.tsx +++ b/apps/client/src/features/app-settings/panel/automations-panel/AutomationForm.tsx @@ -65,27 +65,27 @@ export default function AutomationForm({ automation, triggers, onClose }: Automa const fieldList = useMemo(() => makeFieldList(data), [data]); /** - * Triggers are a separate entity, so they live outside the form state. + * The triggers the server holds for this automation, as far as this form knows. * - * We snapshot the automation's triggers when the form opens and reconcile against that - * snapshot, never against the live prop: settings are polled, so a trigger created - * elsewhere while this form is open must not be deleted by a save that never saw it. + * Seeded from a snapshot taken when the form opens, never from the live prop: settings are + * polled, so a trigger created elsewhere while this form is open must not be deleted by a + * save that never saw it. It then advances as each request succeeds, so a save that fails + * half way leaves only the outstanding work for the retry. */ - const [initialTriggers] = useState(() => + const [syncedTriggers, setSyncedTriggers] = useState(() => isAutomation(automation) ? triggers.filter((trigger) => trigger.automationId === automation.id) : [], ); - const initialCycles = useMemo( - () => Array.from(new Set(initialTriggers.map((trigger) => trigger.trigger))), - [initialTriggers], + const syncedCycles = useMemo( + () => Array.from(new Set(syncedTriggers.map((trigger) => trigger.trigger))), + [syncedTriggers], ); - const [selectedCycles, setSelectedCycles] = useState(initialCycles); + const [selectedCycles, setSelectedCycles] = useState(syncedCycles); /** set once a create succeeds, so a retry after a failed trigger sync edits instead of creating a duplicate */ const [createdId, setCreatedId] = useState(null); + // both are deduped, so equal lengths and one being a subset makes them the same selection const cyclesAreDirty = - selectedCycles.length !== initialCycles.length || - selectedCycles.some((cycle) => !initialCycles.includes(cycle)) || - initialCycles.some((cycle) => !selectedCycles.includes(cycle)); + selectedCycles.length !== syncedCycles.length || selectedCycles.some((cycle) => !syncedCycles.includes(cycle)); const toggleCycle = (cycle: TimerLifeCycle) => { setSelectedCycles((prev) => (prev.includes(cycle) ? prev.filter((c) => c !== cycle) : [...prev, cycle])); @@ -95,7 +95,7 @@ export default function AutomationForm({ automation, triggers, onClose }: Automa * A lifecycle can carry several differently named triggers, which the chips collapse into one. * Unchecking it removes all of them, so say which ones rather than deleting them quietly. */ - const triggersToRemove = initialTriggers.filter((trigger) => !selectedCycles.includes(trigger.trigger)); + const triggersToRemove = syncedTriggers.filter((trigger) => !selectedCycles.includes(trigger.trigger)); /** * Test results are keyed by the field array id rather than the index: @@ -105,6 +105,7 @@ export default function AutomationForm({ automation, triggers, onClose }: Automa const feedbackTimers = useRef>>({}); const { + clearErrors, control, handleSubmit, getValues, @@ -219,21 +220,28 @@ export default function AutomationForm({ automation, triggers, onClose }: Automa * Reconciles the lifecycle selection against the global triggers. * Runs after the automation itself is saved: a new automation has no id until then. * - * Both sides are diffed against the mount-time snapshot, so this only ever removes - * triggers the user could actually see when they made the change. + * Every request advances the synced snapshot as it succeeds, so pressing save again after + * a failure half way through retries only what is left. Without that a retry would re-add + * a trigger it already created, and re-delete one it already deleted, which the server + * rejects outright. */ const syncTriggers = async (automationId: string, title: string) => { for (const trigger of triggersToRemove) { await deleteTrigger(trigger.id); + setSyncedTriggers((prev) => prev.filter((synced) => synced.id !== trigger.id)); } - const toAdd = selectedCycles.filter((cycle) => !initialCycles.includes(cycle)); + const toAdd = selectedCycles.filter((cycle) => !syncedCycles.includes(cycle)); for (const cycle of toAdd) { - await addTrigger({ title: makeTriggerTitle(title, cycle), trigger: cycle, automationId }); + const created = await addTrigger({ title: makeTriggerTitle(title, cycle), trigger: cycle, automationId }); + setSyncedTriggers((prev) => [...prev, created]); } }; const onSubmit = async (values: AutomationDTO) => { + // a stale failure from the previous attempt would otherwise sit under a successful retry + clearErrors('root'); + // saving happens in two requests, so a retry after a partial failure must edit rather than create again const existingId = isAutomation(automation) ? automation.id : createdId; let automationId: string; @@ -280,7 +288,13 @@ export default function AutomationForm({ automation, triggers, onClose }: Automa return `${fieldLabel} ${operatorLabel} ${value ? `“${value}”` : 'nothing'}`; }; - const canSubmit = !isSubmitting && (isDirty || cyclesAreDirty) && isValid; + /** + * A failed save reports itself as a root error, which react-hook-form counts against + * isValid. Left alone that disables the very retry the message is asking the user to make, + * so a root error on its own does not block submitting again. + */ + const invalidFields = Object.keys(errors).filter((field) => field !== 'root'); + const canSubmit = !isSubmitting && (isDirty || cyclesAreDirty) && (isValid || invalidFields.length === 0); const hasContinuousCycle = selectedCycles.some((cycle) => continuousCycles.includes(cycle)); return ( diff --git a/apps/server/src/api-data/automation/automation.dao.ts b/apps/server/src/api-data/automation/automation.dao.ts index bb849b73a..d2e39500d 100644 --- a/apps/server/src/api-data/automation/automation.dao.ts +++ b/apps/server/src/api-data/automation/automation.dao.ts @@ -145,7 +145,7 @@ export async function deleteAutomation(projectRundowns: ProjectRundowns, automat return; } - // prevent deleting a automation that is in use in events, the user has to unlink it there + // prevent deleting an automation that is in use in events, the user has to unlink it there const isInUse = isAutomationUsed(projectRundowns, automationId); if (isInUse) { throw new Error(`Unable to delete automation used in rundown: ${isInUse[0]}, in event with ID: ${isInUse[1]}`);