mirror of
https://github.com/cpvalente/ontime.git
synced 2026-09-10 08:39:34 +00:00
fix(automation): make a failed save retryable without duplicating triggers
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AfDKsy6PE3Rbyt32Fg4YKf
This commit is contained in:
@@ -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<Trigger[]>(() =>
|
||||
const [syncedTriggers, setSyncedTriggers] = useState<Trigger[]>(() =>
|
||||
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<TimerLifeCycle[]>(initialCycles);
|
||||
const [selectedCycles, setSelectedCycles] = useState<TimerLifeCycle[]>(syncedCycles);
|
||||
/** set once a create succeeds, so a retry after a failed trigger sync edits instead of creating a duplicate */
|
||||
const [createdId, setCreatedId] = useState<string | null>(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<Record<string, ReturnType<typeof setTimeout>>>({});
|
||||
|
||||
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 (
|
||||
|
||||
Reference in New Issue
Block a user