fix(alert): stop false unsaved-changes prompt when closing a deployment - #3570
fix(alert): stop false unsaved-changes prompt when closing a deployment#3570baktun14 wants to merge 7 commits into
Conversation
The alerts form derived its dirty state from a baseline recomputed live from deployment.escrowBalance, so closing a deployment (escrow drops) flipped it to "changed" with no user edits, badging the Alerts tab and triggering the navigation guard popup. Use react-hook-form formState.isDirty against the mount-time baseline instead, and suppress reporting for closed (disabled) deployments. Removes the now-redundant racy useWhen band-aid.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change updates deployment alert dirty-state tracking, selective submission, conditional validation, save-button loading state, and related tests. It also removes inactive deployment tab reset behavior. ChangesDeployment alert form state
Deployment detail tab state
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3570 +/- ##
==========================================
- Coverage 75.89% 75.53% -0.37%
==========================================
Files 1161 1044 -117
Lines 30309 27245 -3064
Branches 7537 6896 -641
==========================================
- Hits 23003 20579 -2424
+ Misses 6449 5869 -580
+ Partials 857 797 -60
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — approving despite the two flagged issues, since both are pre-existing (not introduced by this diff) and low-impact rather than blockers.
Extended reasoning...
Overview
This PR fixes a real bug: the Alerts tab badged as "unsaved changes" and blocked navigation with a confirmation dialog purely because closing a deployment drops escrowBalance, shifting the hand-rolled providedValues baseline used for the dirty check. The fix swaps that hand-rolled isEqual(providedValues, values) comparison for react-hook-form's own formState.isDirty, which is computed against the frozen mount-time defaultValues and is immune to later baseline drift. It also removes a now-redundant, previously racy useWhen band-aid in DeploymentDetail.tsx. Three files touched, all scoped to this one feature (deployment alerts tab).
Security risks
None. No auth, crypto, or permission logic involved — this is client-side form dirty-state tracking for a notification-alert config UI.
Level of scrutiny
Low-to-medium is appropriate: this is a client-only UX correctness fix (badge + navigation guard state), not a data-mutation or security-sensitive path. The PR includes two new regression tests that were confirmed to fail against pre-fix code, plus a Save-enablement test, giving good confidence the intended behavior is preserved and the regression is closed.
Other factors
Both bugs flagged by the bug-hunting pass are pre-existing, not introduced by this diff: the Save button's loading binding to the GET query (not the mutation) is untouched by this PR's changes (only the sibling disabled prop changed), and the submit() diff-against-providedValues issue mirrors the exact same drift class the PR fixes for the badge check, but in code this PR only renamed values to form.getValues() without changing the comparison semantics — so the same drift-diff behavior existed before. Neither is a regression caused by this change, and their blast radius is narrow (redundant network calls in one case, an inert disabled-alert persisted in an edge case in the other). Given the fix is well-targeted, tested with two regression tests that fail on old code, and the flagged issues are pre-existing/minor, I'm comfortable approving.
submit() decided which sections to persist by comparing form values against providedValues, which recomputes from escrowBalance. After escrow drift, editing only deploymentClosed could persist an untouched deploymentBalance section, or trip the threshold max validation on a field the user never edited. Diff on react-hook-form dirtyFields instead, the same frozen baseline isDirty uses.
The Save button's loading was bound to the alerts GET query, never the upsert mutation, so it showed no spinner and stayed clickable during a save, allowing duplicate concurrent submits. Expose the mutation's isPending as isSaving from the container and bind the button's loading and disabled to it.
There was a problem hiding this comment.
LGTM — the two issues I flagged on earlier commits are now fixed (dirtyFields-based payload diffing in 94456e4, mutation-pending-gated Save button in d87afc8), and this run found no new issues.
What was reviewed: the isDirty/dirtyFields-based change-tracking and selective payload submission in DeploymentAlerts.tsx, the isSaving plumbing through DeploymentAlertsContainer.tsx, and the removed useWhen band-aid in DeploymentDetail.tsx. The regression tests (escrow-drift-on-close, dirty-field-only payload, save-button disable/enable) exercise the scenarios described in the PR.
Extended reasoning...
Overview
Small, well-scoped fix in apps/deploy-web alert components: replaces a hand-rolled dirty-check (isEqual against a baseline that drifts with escrowBalance) with react-hook-form's own formState.isDirty/dirtyFields, removes a redundant useWhen band-aid, and threads the upsert mutation's pending state into the Save button. Four files touched, two of them tests.
Security risks
None — pure client-side form-state/UI logic, no auth, crypto, or data-access changes.
Level of scrutiny
Low-to-moderate. This is a UI correctness fix for a real bug (false "unsaved changes" prompt), not a critical path, with good regression test coverage added for the exact scenarios described (escrow drift on close, selective section persistence, save-button enablement).
Other factors
Two real issues I found on earlier revisions of this PR (stale-baseline payload diffing, and Save button not gating on the in-flight mutation) have both been fixed in commits 94456e4 and d87afc8, each with a dedicated regression test. A third, pre-existing issue I flagged (threshold validation bound to a live, drifting maxBalanceThreshold while the form value stays pinned) is explicitly out of scope for this PR and unrelated to the fix at hand. This run's bug hunt found nothing new.
The threshold schema max tracks the live balance, but the form value was pinned at mount, so if escrow dropped below the mount-time threshold an unrelated deploymentClosed save failed validation on a field the user never touched. Resync the untouched threshold to its computed default whenever that default changes.
…dited Replaces the resync effect from the previous commit, which only covered an unsaved threshold. A saved balance threshold legitimately exceeds the current balance once escrow drops below it (that is when the alert fires), yet the whole-form validation blocked saving an untouched deploymentClosed edit. Gate the threshold max() on the balance section being dirty so an unrelated save is never blocked, while still validating the max while the user edits the threshold.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/deploy-web/src/components/deployments/DeploymentAlerts/DeploymentAlerts.tsx (1)
140-143: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPrevent edits from being lost during an in-flight save.
submitawaitsupsertand then resets the whole form.isSavingdisables only the Save button, while both alert editors remain enabled. An edit made during the request is overwritten byform.reset(...).Pass
isSavingto both childdisabledprops, or preserve fields changed after submission before resetting.Proposed fix
- <d.DeploymentBalanceAlert disabled={isLoading || disabled} /> - {isDeploymentClosedEnabled && <d.DeploymentCloseAlert disabled={isLoading || disabled} />} + <d.DeploymentBalanceAlert disabled={isLoading || disabled || isSaving} /> + {isDeploymentClosedEnabled && <d.DeploymentCloseAlert disabled={isLoading || disabled || isSaving} />}Add a regression test that edits another alert while
upsertis unresolved and verifies that the edit remains after the request completes.Also applies to: 151-152
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/deploy-web/src/components/deployments/DeploymentAlerts/DeploymentAlerts.tsx` around lines 140 - 143, Prevent in-flight saves from overwriting edits by passing isSaving to the disabled props of both alert editor children surrounding submit, keeping them non-editable until upsert resolves; retain the existing form.reset(assignDefaults(nextValues.alerts)) behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/deploy-web/src/components/deployments/DeploymentAlerts/DeploymentAlerts.tsx`:
- Around line 77-83: Update strictSchema in the useMemo block to extend
schema.shape.deploymentBalance rather than replacing it with a new z.object.
Preserve the existing threshold validation while retaining validation for
notificationChannelId and enabled before submit reads form values.
---
Outside diff comments:
In
`@apps/deploy-web/src/components/deployments/DeploymentAlerts/DeploymentAlerts.tsx`:
- Around line 140-143: Prevent in-flight saves from overwriting edits by passing
isSaving to the disabled props of both alert editor children surrounding submit,
keeping them non-editable until upsert resolves; retain the existing
form.reset(assignDefaults(nextValues.alerts)) behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6025ee25-420d-4368-bf04-37d1e1beaa93
📒 Files selected for processing (2)
apps/deploy-web/src/components/deployments/DeploymentAlerts/DeploymentAlerts.spec.tsxapps/deploy-web/src/components/deployments/DeploymentAlerts/DeploymentAlerts.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/deploy-web/src/components/deployments/DeploymentAlerts/DeploymentAlerts.spec.tsx
…schema
strictSchema replaced the whole deploymentBalance shape with { threshold }, dropping
the notificationChannelId (required) and enabled checks, so an enabled balance alert
with no channel could be submitted. Extend the base shape instead, overriding only
threshold, so those validations are preserved.
…section The conditional max() keyed off dirtyFields.deploymentBalance, which react-hook-form marks truthy when any field in the group changes, so toggling only enabled or the channel re-validated an untouched stale threshold and blocked the save. Key it on the threshold field dirty state instead.
There was a problem hiding this comment.
No bugs found in this run. Given the extensive iteration history on this PR (five rounds of subtle dirty-state/validation bugs found and fixed across the review thread), I'd still like a human to take a final pass before merge rather than relying solely on automated review.
What was reviewed: the isDirty/dirtyFields-based change detection replacing the old isEqual comparison, the section-vs-field-level threshold max() gating (now keyed on dirtyFields.deploymentBalance?.threshold per the latest commit), the Save button's isSaving/isPending wiring to prevent duplicate submits, and the new regression tests covering escrow drift, saved-threshold drift, and no-channel validation. The latest commit's fix for the field-granularity issue I raised (section-level dirty check misfiring on unrelated field edits) looks correctly scoped.
Extended reasoning...
This run's bug hunting pass found no new issues. The PR has already been through five rounds of review back-and-forth (my own prior comments plus CodeRabbit) surfacing and fixing genuinely subtle bugs: stale escrow-derived baselines leaking into the save payload, a Save button not reflecting mutation pending state, threshold validation blocking unrelated field saves after escrow drift, a shallow schema.extend() dropping nested validation, and finally a dirty-check granularity bug (section-level vs field-level). Each was fixed with a targeted commit and a regression test, and the final commit (d961223) addresses the last reported issue by keying the conditional max() validation off dirtyFields.deploymentBalance?.threshold specifically rather than the whole deploymentBalance group.
Security risks
None. This is client-side form/validation logic for an alerts configuration UI; no auth, crypto, or data-exposure surface is touched.
Level of scrutiny
This is exactly the kind of change that warrants a careful look despite passing this run clean: it is deceptively simple-looking (dirty-state tracking) but has repeatedly hidden edge cases around React Query refetches, react-hook-form dirty-field semantics, and Zod schema composition. The fix history itself is evidence that a quick skim is insufficient for this code path — five substantive bugs were found here, not zero.
Other factors
Tests were added alongside each fix (7 new test cases total), and CodeRabbit independently confirmed one of the fixes. The code now in the diff matches the latest commit history and appears internally consistent, but the density of prior findings on this exact file is reason enough to keep a human in the loop for the final merge decision.
1 participant
Why
Closing a deployment showed a red "modified" dot on the Alerts tab and popped a "You have unsaved alert configuration changes that will be lost" dialog on navigation, even when the user never touched any alert setting.
Root cause: the alerts form computed its dirty state by comparing the live form values against a baseline (
providedValues) that is recomputed fromdeployment.escrowBalance(it seeds the default balance threshold viamaxBalanceThreshold). Closing a deployment refetches the deployment and drops the escrow balance, so the baseline shifted while the form values stayed pinned at mount, flipping the comparison to "changed" with zero user interaction. The earlieruseWhen(state !== "active", ...)band-aid only partially masked this and lost a race.What
DeploymentAlerts.tsx: use react-hook-form's ownformState.isDirty(compared against the mount / last-saved baseline) instead of the hand-rolled!isEqual(providedValues, values), and reporthasChanges: !disabled && isDirtyso a closed deployment never badges the tab or arms the navigation guard. The Save button now keys offisDirty.DeploymentDetail.tsx: remove the now-redundant racyuseWhenband-aid (and its unused import).An active deployment with genuine unsaved edits still badges the tab and still prompts on navigation, so the feature is preserved.
Verification:
test:unit4/4 green,lint --quietclean,tsc --noEmitshows 0 new errors vs theorigin/mainbaseline (180 == 180).Summary by CodeRabbit