Repository navigation
Implementing AlertChannel interface and dispatcher core - #290
Conversation
|
@Olagoke22 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces an AlertChannel interface and configurable dispatcher routing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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)
tests/alerts/dispatcher.test.ts (1)
26-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the fixture aligned with the full default channel map.
The helper and per-test
channelsfixture still only modelwebhook,slack, andpagerduty, butDEFAULT_CHANNELSnow also shipsdiscordandtelegram. That leaves two production routes outside the typed test surface, so a broken mapping there would not fail this suite. Consider deriving these fixtures fromDEFAULT_CHANNELS(or wideningchannelType) so new defaults stay covered automatically.Also applies to: 77-85
🤖 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 `@tests/alerts/dispatcher.test.ts` around lines 26 - 27, The test fixture types in dispatcher.test.ts are out of sync with DEFAULT_CHANNELS because they only cover webhook, slack, and pagerduty, so discord and telegram are untested. Update the shared channel fixture and any helper types used by the dispatcher tests to derive from DEFAULT_CHANNELS or otherwise include all current channel types, and make sure the per-test channels setup still exercises the full default channel map through the relevant test helpers and fixtures.
🤖 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 `@src/alerts/dispatcher.ts`:
- Around line 66-76: The send and persistence steps in dispatcher logic are
currently wrapped in the same try/catch, so a failure in markAlertDelivered() is
treated like a channel.send() failure and causes an unnecessary retry. Update
the alert delivery flow in dispatcher.ts to separate the outbound send path from
the post-send database write, using distinct error handling around
channel.send() and markAlertDelivered(). Keep retry_count increments and
retry/error accounting only for send failures, and handle persistence errors
without re-queuing the alert as undelivered.
---
Outside diff comments:
In `@tests/alerts/dispatcher.test.ts`:
- Around line 26-27: The test fixture types in dispatcher.test.ts are out of
sync with DEFAULT_CHANNELS because they only cover webhook, slack, and
pagerduty, so discord and telegram are untested. Update the shared channel
fixture and any helper types used by the dispatcher tests to derive from
DEFAULT_CHANNELS or otherwise include all current channel types, and make sure
the per-test channels setup still exercises the full default channel map through
the relevant test helpers and fixtures.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 643a8120-b101-4c51-b3ea-2d5738a57ea6
📒 Files selected for processing (3)
src/alerts/dispatcher.tssrc/alerts/types.tstests/alerts/dispatcher.test.ts
📜 Review details
🔇 Additional comments (2)
src/alerts/types.ts (1)
56-58: LGTM!src/alerts/dispatcher.ts (1)
104-116: LGTM!
| await channel.send(alert.channelTarget, event, alert.webhookSecret); | ||
| markAlertDelivered(db, alert.alertFiredId); | ||
| result.delivered++; | ||
|
|
||
| logger.info( | ||
| `Alert delivered — id: ${alert.alertFiredId}, ` + | ||
| `channel: ${alert.channelType}, contract: ${alert.contractId}`, | ||
| `Alert delivered — id: ${alert.alertFiredId}, channel: ${alert.channelType}, contract: ${alert.contractId}`, | ||
| ); | ||
| } catch (err: unknown) { | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| result.failed++; | ||
| result.errors.push(message); | ||
|
|
||
| incrementRetryCount(db, alert.alertFiredId); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Separate send failures from post-send persistence failures.
markAlertDelivered() is inside the same try as channel.send(). If the outbound send succeeds and the DB write throws, Line 76 increments retry_count and the same alert will be retried on the next cycle, duplicating the notification. Handle the post-send persistence path separately instead of funneling it into the resend logic.
🤖 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 `@src/alerts/dispatcher.ts` around lines 66 - 76, The send and persistence
steps in dispatcher logic are currently wrapped in the same try/catch, so a
failure in markAlertDelivered() is treated like a channel.send() failure and
causes an unnecessary retry. Update the alert delivery flow in dispatcher.ts to
separate the outbound send path from the post-send database write, using
distinct error handling around channel.send() and markAlertDelivered(). Keep
retry_count increments and retry/error accounting only for send failures, and
handle persistence errors without re-queuing the alert as undelivered.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | - | Generic High Entropy Secret | 3dd17ce | tests/core/vault.test.ts | View secret |
| - | - | Generic High Entropy Secret | 3dd17ce | tests/core/vault.test.ts | View secret |
| - | - | Generic High Entropy Secret | 3dd17ce | tests/core/vault.test.ts | View secret |
| - | - | Generic High Entropy Secret | b59bef4 | tests/core/vault.test.ts | View secret |
| - | - | Generic High Entropy Secret | b59bef4 | tests/core/vault.test.ts | View secret |
| - | - | Generic High Entropy Secret | b59bef4 | tests/core/vault.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
2 participants
closes #109