fix(automations): save-error banner cleared on failure (follow-up to #369)#371
Merged
Merged
Conversation
…ilure The error-surfacing fix from #369 was defeated by its own refresh call. saveField set the error in its catch, then unconditionally called loadDetail(), whose first synchronous statement is setError(null) (before its await). Both updates landed in the same microtask continuation, React batched them, null won, and the banner never rendered — so a rejected save (e.g. an out-of-range maxIterations) still looked like a silent no-op, the exact symptom #369 set out to fix. Refresh only on success. On failure the error persists and the closed inline editor re-renders the field at its unchanged saved value. No regression test: the bundle UIs have no component-test harness (@testing-library), so this React state-timing path can't be covered without standing up RTL. Verified manually against the batching trace.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #369. A QA reviewer correctly found that the "surface update errors" half of #369 doesn't actually work — verified against source.
The bug
AutomationDetailView.saveField:When
onUpdaterejects, the catch runssetError(message), thenloadDetail()runs synchronously to its firstawait— executingsetError(null). Both updates land in the same microtask continuation, React batches them,nullwins, and the banner ({error && …}) never renders. (Even without batching, it's two sequentialsetErrors andnullis last.) Net: a rejected save still looks like a silent no-op — the exact symptom #369 set out to fix.The fix
Refresh only on success:
On failure the error stays set; the field reverts because
setEditing(null)closes the inline editor and re-renders the unchangeddetailvalue.Notes
@testing-library); this React state-timing path can't be covered without standing up RTL. Verified manually against the batching trace;bun run verify:staticgreen and the automations bundle UI builds clean.