Refactor notification system with service approach #3231
Conversation
Co-authored-by: denguk <[email protected]>
|
Cursor Agent can help with this pull request. Just |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| } | ||
| } | ||
| return t.token != "" && t.chatID != "" | ||
| } |
There was a problem hiding this comment.
Bug: Notifier Enabled Methods Have Unpredictable Side Effects
Several notifier Enabled() methods (Telegram, Slack, Gotify, DingTalk) have side effects. Some mutate internal state from global configuration (Telegram, Gotify), while others rely on global config for their enabled status (Slack, DingTalk). This makes the Enabled() check non-deterministic and prone to race conditions, as fallback configuration is handled outside of initialization.
Additional Locations (1)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| if status.IsNotifiable() { | ||
| t.sendTelegramAlert() | ||
| t.sendSlackAlert() | ||
| t.sendRocketChatAlert() | ||
| t.sendMicrosoftTeamsAlert() | ||
| t.sendDingTalkAlert() | ||
| t.sendGotifyAlert() | ||
| // Prefer new notification service if configured; otherwise fallback to legacy methods | ||
| if t.notificationService != nil && t.notificationService.HasNotifiers() { | ||
| author, version := t.alertInfos() | ||
| alert := Alert{ | ||
| Name: t.Template.Name, | ||
| Author: author, | ||
| Color: t.alertColor("slack"), | ||
| Task: alertTask{ |
There was a problem hiding this comment.
[P1] Respect alert flags and success suppression
The new notification-service path sends an alert unconditionally whenever status.IsNotifiable() is true, but it no longer checks the project/template alert toggle or Template.SuppressSuccessAlerts like the legacy send*Alert functions do. As a result, enabling the unified notifications config will emit Telegram/Slack/etc. messages even when alerts are disabled for the project or when success notifications are explicitly suppressed. The branch should short-circuit when t.alert is false or the template suppresses success notifications before invoking SendAll.
Useful? React with 👍 / 👎.
Add a new service-based notification system with a common interface, using token/channel parameters and a new
notificationsconfig, while preserving old URL-based methods for backward compatibility.This change introduces a more secure and modular way to handle notifications by moving away from insecure URL parameters and providing a structured configuration, ensuring existing setups continue to function without modification.