Skip to content

fix(notifications): don't auto-enable deployment balance alert - #3181

Merged
baktun14 merged 1 commit into
mainfrom
fix/notifications-disable-default-balance-alert
May 15, 2026
Merged

fix(notifications): don't auto-enable deployment balance alert#3181
baktun14 merged 1 commit into
mainfrom
fix/notifications-disable-default-balance-alert

Conversation

@baktun14

@baktun14 baktun14 commented May 14, 2026

Copy link
Copy Markdown
Contributor

Why

Auto top-up is the default for managed wallets, so the deployment balance alert is redundant for the majority of users. Worse: when a user closes a batch of deployments, escrow drops below the auto-created threshold and the balance alert fires once per deployment, spamming the user with emails (and costing us money).

There is no suppression path between auto top-up and balance alerts in apps/notifications today — they're fully independent.

What

  • Stop auto-creating the deploymentBalance alert when a deployment is created. The post-deploy job now only enables deploymentClosed (one-shot, not noisy).
  • Users who want a balance alert can still enable it manually from the deployment Alerts tab — the frontend form already supplies a sensible default threshold locally.
  • Existing deployments with the balance alert already enabled are intentionally left untouched. They can be turned off per-deployment in the UI.
  • Side benefit: removes a per-deployment chain RPC (getDeploymentEscrowBalance) that was only used to compute the auto-enabled threshold.

Files changed

  • apps/api/src/notifications/services/notification/notification.service.ts — drop balance alert from upsert; remove threshold calc; drop escrowBalance from input.
  • apps/api/src/app/services/enable-deployment-alert/enable-deployment-alert.handler.ts — drop DeploymentHttpService dependency and chain fetch.
  • Updated unit specs to match.
  • apps/deploy-web/tests/ui/managed-wallet-alerts.spec.ts — flip the default-state assertion (balance toggle starts off) and enable the toggle before updating the threshold.

The chore(repo): sync package-lock.json commit is preparatory: the lockfile on main was out of sync with package.json, causing the npm ci --dry-run pre-commit check to fail on any commit. Synced as a separate commit so it can be reviewed independently.

Summary by CodeRabbit

  • Bug Fixes

    • Default state of escrow balance alert in managed wallet settings changed to disabled.
  • Refactor

    • Simplified deployment alert flow to only auto-enable "deployment closed" alerts; removed balance-threshold calculations from automated alerts.
  • Tests

    • Updated unit and UI tests to reflect the new alert behavior and the adjusted default toggle state.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 14, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 85528fa7-7ec7-445f-a249-bd5754243736

📥 Commits

Reviewing files that changed from the base of the PR and between db585bc and 0296efc.

📒 Files selected for processing (5)
  • apps/api/src/app/services/enable-deployment-alert/enable-deployment-alert.handler.spec.ts
  • apps/api/src/app/services/enable-deployment-alert/enable-deployment-alert.handler.ts
  • apps/api/src/notifications/services/notification/notification.service.spec.ts
  • apps/api/src/notifications/services/notification/notification.service.ts
  • apps/deploy-web/tests/ui/managed-wallet-alerts.spec.ts
🚧 Files skipped from review as they are similar to previous changes (5)
  • apps/deploy-web/tests/ui/managed-wallet-alerts.spec.ts
  • apps/api/src/app/services/enable-deployment-alert/enable-deployment-alert.handler.spec.ts
  • apps/api/src/app/services/enable-deployment-alert/enable-deployment-alert.handler.ts
  • apps/api/src/notifications/services/notification/notification.service.spec.ts
  • apps/api/src/notifications/services/notification/notification.service.ts

📝 Walkthrough

Walkthrough

The PR removes deployment escrow balance handling from the auto-enable alert flow: the exported AutoEnableDeploymentAlertInput drops escrowBalance, EnableDeploymentAlertHandler no longer depends on DeploymentHttpService and directly invokes NotificationService.autoEnableDeploymentAlert, and NotificationService now only upserts deploymentClosed alerts. Tests and a UI spec are updated accordingly.

Changes

Remove escrow balance from deployment alerts

Layer / File(s) Summary
NotificationService contract and implementation
apps/api/src/notifications/services/notification/notification.service.ts
Removed escrowBalance from AutoEnableDeploymentAlertInput. Deleted balance-threshold constant and helper logic. autoEnableDeploymentAlert now calls upsertDeploymentClosedAlert and no longer creates/upserts a deploymentBalance alert.
NotificationService tests
apps/api/src/notifications/services/notification/notification.service.spec.ts
Updated calls to autoEnableDeploymentAlert to omit escrowBalance. Assertions now expect only deploymentClosed upsert; channel-creation and skip scenarios retained.
EnableDeploymentAlertHandler implementation
apps/api/src/app/services/enable-deployment-alert/enable-deployment-alert.handler.ts
Constructor signature simplified to remove DeploymentHttpService. handle no longer fetches deployment/escrow or retries; it directly invokes notificationService.autoEnableDeploymentAlert with userId, walletAddress, and dseq.
EnableDeploymentAlertHandler tests
apps/api/src/app/services/enable-deployment-alert/enable-deployment-alert.handler.spec.ts
Removed deployment-http mocking and "deployment not found" coverage. setup() simplified and spec asserts notificationService.autoEnableDeploymentAlert is called with payload identifiers only.
UI test: managed wallet alerts
apps/deploy-web/tests/ui/managed-wallet-alerts.spec.ts
Default state expectation for escrow balance alert toggle changed to unchecked; test now enables the toggle before performing threshold update/save assertions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/notifications-disable-default-balance-alert

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread package-lock.json
Auto top-up is the default for managed wallets, so balance alerts are
redundant for most users and produce email spam (and email cost) when
deployments are closed in batches. Stop auto-creating the balance alert
on deployment creation; users can still enable it manually from the
Alerts tab. The deployment-closed alert remains auto-enabled.

Side benefit: drops the per-deployment chain RPC call used only to
compute the balance-alert threshold.
@baktun14
baktun14 force-pushed the fix/notifications-disable-default-balance-alert branch from db585bc to 0296efc Compare May 15, 2026 00:05
@codecov

codecov Bot commented May 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.27%. Comparing base (a3b1b76) to head (0296efc).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3181      +/-   ##
==========================================
- Coverage   63.70%   62.27%   -1.44%     
==========================================
  Files        1089     1009      -80     
  Lines       26449    24555    -1894     
  Branches     6417     6037     -380     
==========================================
- Hits        16850    15292    -1558     
+ Misses       8403     8094     -309     
+ Partials     1196     1169      -27     
Flag Coverage Δ *Carryforward flag
api 84.29% <100.00%> (-0.01%) ⬇️
deploy-web 46.66% <ø> (-0.03%) ⬇️
log-collector ?
notifications 91.06% <ø> (ø) Carriedforward from a3b1b76
provider-console 81.48% <ø> (ø) Carriedforward from a3b1b76
provider-inventory ?
provider-proxy 86.08% <ø> (ø) Carriedforward from a3b1b76
tx-signer ?

*This pull request uses carry forward flags. Click here to find out more.

Files with missing lines Coverage Δ
...eployment-alert/enable-deployment-alert.handler.ts 100.00% <ø> (ø)
...ions/services/notification/notification.service.ts 100.00% <100.00%> (ø)

... and 84 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@baktun14
baktun14 added this pull request to the merge queue May 15, 2026
Merged via the queue into main with commit c73a6b0 May 15, 2026
56 checks passed
@baktun14
baktun14 deleted the fix/notifications-disable-default-balance-alert branch May 15, 2026 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants