feat: Rework alerts into customisable notifications#521
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a multi-channel notification system: persistent NotificationChannel entities (email, Telegram, Discord, Slack, webhook, web push), channel CRUD/verification API, per-alert channel selection and templating, dispatchers for each channel type, template rendering, UI for channel management and template editing, and DB migrations/backfill. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as TaskManager
participant DB as Database
participant Template as TemplateRenderer
participant Dispatcher as ChannelDispatcher
participant ChannelEmail as EmailChannelService
participant ChannelWebhook as WebhookChannelService
participant ChannelWebpush as WebpushChannelService
participant External as External Services
Scheduler->>DB: load alerts with channels
Scheduler->>Template: render(template, context)
Template-->>Scheduler: renderedMessage
Scheduler->>Dispatcher: dispatch(channels, renderedMessage)
par Email Delivery
Dispatcher->>ChannelEmail: send(channel, message)
ChannelEmail->>External: SMTP/sendMail
External-->>ChannelEmail: response
and Webhook Delivery
Dispatcher->>ChannelWebhook: send(channel, message)
ChannelWebhook->>External: POST JSON (+signature)
External-->>ChannelWebhook: response
and Webpush Delivery
Dispatcher->>ChannelWebpush: send(channel, message)
ChannelWebpush->>External: webpush.sendNotification
External-->>ChannelWebpush: response / 410 -> disable channel
end
Dispatcher-->>Scheduler: allSettled results
Scheduler->>DB: update lastTriggered
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
README.md (1)
89-89:⚠️ Potential issue | 🟡 MinorComparison-table entry is now stale relative to line 57.
Line 57 lists Email, Slack, Telegram, Discord, webhook and web push as supported delivery methods, but the Cloud column on Line 89 still advertises only "Yes (Slack/Telegram/Discord)". Worth aligning so users don't see contradictory info.
📝 Suggested update
-| **Alerts & notifications** | ✅ Yes (Slack/Telegram/Discord) | ⚠️ Not included | +| **Alerts & notifications** | ✅ Yes (Email, Slack, Telegram, Discord, webhook, browser web push) with per-alert custom message templates | ⚠️ Not included |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 89, Update the stale comparison table cell for "Alerts & notifications" so the Cloud column matches the supported delivery methods listed elsewhere (Email, Slack, Telegram, Discord, webhook, web push); locate the table row containing the "Alerts & notifications" label and change the Cloud entry from "Yes (Slack/Telegram/Discord)" to a matching phrase (e.g., "Yes (Email, Slack, Telegram, Discord, webhook, web push)") or alternatively reduce the supported list earlier to match—ensure both the row for "Alerts & notifications" and the earlier supported-delivery list are consistent.backend/apps/cloud/src/task-manager/task-manager.service.ts (1)
1830-1852:⚠️ Potential issue | 🟠 Major
lastTriggeredis bumped before the ERRORS path is sure it can dispatch — a transient detail-query failure silently swallows the alert window.The flow is:
- Line 1816–1828: fetch
errorDetails(this is wrapped in try/catch and on failure leaveserrorDetails = nullwhile only logging).- Line 1831: update
alert.lastTriggered = new Date().- Line 1847: if it's an ERRORS alert and
errorDetailsis null, return early.Because the ERRORS alert path uses
lastTriggeredas the lower bound forlastEventSubtractSeconds(Line 1682) and bypasses the 24h cooldown (Lines 1659–1666), the next cron tick will compute its window starting from "now", so any errors that occurred in the dropped tick are never re-evaluated and the user gets no notification at all. Reorder solastTriggeredis only persisted once dispatch is committed (after theerrorDetailsguard), or skip the update on the early-return branch.🛠️ Suggested fix
- // `@ts-expect-error` - await this.alertService.update(alert.id, { - lastTriggered: new Date(), - }) - const effectiveQueryTimeString = ... const clientUrl = this.configService.get('CLIENT_URL') const dashboardUrl = `${clientUrl}/projects/${project.id}` if (alert.queryMetric === QueryMetric.ERRORS && !errorDetails) { this.logger.warn( `[CRON WORKER](checkMetricAlerts) Error details not found for alert ${alert.id}`, ) return } + // `@ts-expect-error` + await this.alertService.update(alert.id, { + lastTriggered: new Date(), + }) + let context: AlertContext🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/task-manager/task-manager.service.ts` around lines 1830 - 1852, The alert `lastTriggered` is being persisted too early — update via alertService.update({ lastTriggered }) should only occur after the ERRORS branch is confirmed to have valid errorDetails (or after dispatch is committed); move the call to this.alertService.update(...) out of the pre-check section (or conditionally skip it) so it runs after the errorDetails guard (i.e., only after the block that verifies alert.queryMetric === QueryMetric.ERRORS && errorDetails) and after successful dispatch logic in checkMetricAlerts, ensuring errorDetails (the variable fetched earlier) is non-null before persisting lastTriggered.web/app/pages/Project/Settings/Alerts/ProjectAlertsView.tsx (1)
371-381:⚠️ Potential issue | 🟠 Major
isIntegrationLinkedstill inspects legacy per-user integration fields.This PR moves alert delivery to the new notification-channels system (each alert keeps its own
channels[]), and the backend cron explicitly skips alerts whosechannelsarray is empty:// task-manager.service.ts (~Line 1576 / 1657) if (!alert.channels || alert.channels.length === 0) returnBut this callout's gate still reads the deprecated
user.telegramChatId / slackWebhookUrl / discordWebhookUrlfields. Two user-visible problems result:
- A user who has configured the new project/personal channels but has none of the legacy fields will be incorrectly warned that "no notification channel is set" and pushed to fix something that's already configured.
- A user with a legacy
slackWebhookUrlbut whose alerts have zero attached channels gets no warning, even though their alerts will never fire.Drive the warning off the alerts' own
channels(e.g. surface a per-AlertRowbadge whenalert.channels.length === 0, and/or hide the global callout once any alert has channels) so it actually reflects deliverability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/pages/Project/Settings/Alerts/ProjectAlertsView.tsx` around lines 371 - 381, The computed isIntegrationLinked currently inspects legacy per-user fields (user.telegramChatId, user.slackWebhookUrl, user.discordWebhookUrl); change it to reflect the new notification-channels model by checking the alerts data instead: compute a flag like hasAnyAlertWithChannels = alerts.some(a => Array.isArray(a.channels) && a.channels.length > 0) and use that to hide the global "no notification channel" callout (replace references to isIntegrationLinked in ProjectAlertsView). Also update the per-row UI (AlertRow) to render a clear badge/indicator when an individual alert has no channels (a.channels === undefined || a.channels.length === 0) so users see which alerts are non-deliverable.
🟡 Minor comments (16)
web/app/pages/Project/Settings/SettingsSidebar.tsx-105-107 (1)
105-107:⚠️ Potential issue | 🟡 MinorPotential SSR hydration mismatch on the initial expanded state.
getGroupExpandedStatereturnstrueon the server (nowindow), but on the client theuseStateinitializer runs synchronously during the first render and may read a storedfalsefromlocalStorage. If any user has previously collapsed a group, the server-rendered HTML and the first client render will differ, triggering a hydration warning and a visible flicker.Safer pattern: initialise to a deterministic default (e.g.
true, ortabs.some(... activeTab)), then sync fromlocalStoragein auseEffect.🛠️ Proposed change
- const [isExpanded, setIsExpanded] = useState(() => - getGroupExpandedState(group.id), - ) + const [isExpanded, setIsExpanded] = useState(true) + + useEffect(() => { + setIsExpanded(getGroupExpandedState(group.id)) + // Only run on mount per group + }, [group.id])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/pages/Project/Settings/SettingsSidebar.tsx` around lines 105 - 107, The initial state for isExpanded currently calls getGroupExpandedState during render which can differ between server and client and cause hydration mismatches; change the initializer to a deterministic default (e.g. true or other sane default) and then, inside a useEffect, read the stored value via getGroupExpandedState(group.id) and call setIsExpanded(...) to sync the client state from localStorage; update the component to reference isExpanded and setIsExpanded and keep getGroupExpandedState only inside the effect to avoid reading window/localStorage during SSR.web/app/pages/UserSettings/UserSettings.tsx-1596-1605 (1)
1596-1605:⚠️ Potential issue | 🟡 MinorStale wrapper id
integrationsafter the rename.The section now renders
NotificationChannelsand uses thenotificationChannels.*i18n keys, but the wrapper still hasid='integrations'. If this is preserved for backwards-compatible deep links (e.g.?tab=communications#integrations), consider keeping it but adding an alias id; otherwise rename it to match the new naming for consistency with the rest of the project settings refactor.♻️ Suggested rename (drop only if backwards-compat anchors are required)
- <div id='integrations'> + <div id='notification-channels'> <NotificationChannels scope='user' /> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/pages/UserSettings/UserSettings.tsx` around lines 1596 - 1605, The wrapper div still uses the stale id 'integrations' while the section now renders NotificationChannels and uses notificationChannels.* i18n keys; either add a secondary alias id (e.g., keep id='integrations' and also set id='notification-channels' or id='notificationChannels') to preserve backwards-compatible anchors, or rename the wrapper to match the new naming (e.g., change id='integrations' to id='notification-channels' or id='notificationChannels') so the SettingsSection/NotificationChannels usage stays consistent; update any links or tests that rely on the old id if you choose to rename.web/app/routes/projects.$id.tsx-889-894 (1)
889-894:⚠️ Potential issue | 🟡 MinorUsers cannot clear all alert channels via the update action.
When the frontend form has an empty
channelIdsarray, it renders zero hiddenchannelIdsinputs (lines 504–506 of ProjectAlertsSettings.tsx). The backend checkformData.has('channelIds')(line 889) then returnsfalse, sobody.channelIdsis never set. The server keeps the existing channels, and the user cannot empty the list.This differs from
messageTemplateandemailSubjectTemplate, which always render a hidden input (with empty string as fallback), allowing those fields to be cleared.Suggested fix: Always send a sentinel value or marker when the field is being updated. For example, have the frontend send an explicit
channelIdsProvided=trueflag when updating, or simpler: always append at least one entry (possibly empty) when the list changes, and let the backend filter empties as it already does:- if (formData.has('channelIds')) { + if (formData.getAll('channelIds').length > 0 || formData.has('channelIds')) { body.channelIds = formData .getAll('channelIds') .map((v) => v.toString()) .filter(Boolean) }Or: have the frontend always send at least one
channelIdsentry (even if empty) to maintain thehas()invariant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/routes/projects`.$id.tsx around lines 889 - 894, The update action currently skips setting body.channelIds when formData.has('channelIds') is false, preventing clearing the list; change the backend to detect an explicit update intent (e.g., check for a sentinel form field like channelIdsProvided or examine formData.getAll('channelIds') regardless of has()) and always set body.channelIds when that sentinel is present (or when getAll returns any items), mapping to strings and filtering empties as before; also ensure the frontend sends that sentinel (channelIdsProvided=true) or always includes at least one channelIds entry (possibly empty) from ProjectAlertsSettings so the backend can distinguish “no change” from “clear list.”backend/apps/cloud/src/notification-channel/dispatchers/telegram-channel.service.ts-26-30 (1)
26-30:⚠️ Potential issue | 🟡 MinorMarkdown V1 + raw user-rendered body will sometimes 400 from Telegram.
The body is produced by
templateRenderer.render()over anAlertContextwhose fields (e.g.project_name,event_name,error_message) are user-controlled. Telegram's legacyMarkdownmode rejects messages with unmatched_,*,`, or[, which are common in real project/event names. When that happens,addMessagethrows → logged and swallowed, silently dropping the alert for Telegram users.Options:
- Use
parse_mode: 'MarkdownV2'and escape special characters in the body before sending.- Drop
parse_modeentirely and send plain text (the rendered template already contains formatting hints that won't break delivery).- Strip
*_[frommessage.body` like the webpush dispatcher does.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/notification-channel/dispatchers/telegram-channel.service.ts` around lines 26 - 30, The Telegram sender in telegram-channel.service.ts calls this.telegramService.addMessage(cfg.chatId, message.body, { parse_mode: 'Markdown', ... }) using unescaped user-controlled fields from templateRenderer.render, which can cause Telegram Markdown V1 parsing errors and silently drop alerts; fix by either switching to plain text (remove parse_mode), or switch to 'MarkdownV2' and escape Telegram special characters in the rendered body (or mirror the webpush dispatcher behavior and strip/escape the characters `* _ ` [ ]` from message.body) before calling addMessage; update the send path in the TelegramChannelService (the method that calls this.telegramService.addMessage) to sanitize/escape or remove parse_mode accordingly so addMessage never receives raw, unescaped user input that can trigger a 400.docs/content/docs/analytics-dashboard/alerts.mdx-60-60 (1)
60-60:⚠️ Potential issue | 🟡 MinorTypo in image filename.
alert-conect-a-channel.pngis missing annin "connect". Unless the asset is actually committed under that misspelling, the image will 404. Please rename the asset (or the reference) consistently.-<img alt="Connect a channel" src="/docs/img/analytics-dashboard/alert-conect-a-channel.png" /> +<img alt="Connect a channel" src="/docs/img/analytics-dashboard/alert-connect-a-channel.png" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/analytics-dashboard/alerts.mdx` at line 60, The image reference in alerts.mdx points to a misspelled asset src "alert-conect-a-channel.png" (missing 'n'); locate the <img> tag in docs/content/docs/analytics-dashboard/alerts.mdx and either rename the actual image file to "alert-connect-a-channel.png" or update the src to match the committed asset name so the filename is consistent and the image stops 404-ing.backend/apps/cloud/src/notification-channel/dispatchers/email-channel.service.ts-53-54 (1)
53-54:⚠️ Potential issue | 🟡 MinorSingle-asterisk emphasis is rendered as
<strong>instead of<em>.CommonMark (and the docs at
docs/content/docs/analytics-dashboard/alerts.mdx) treat*text*as italics — but here both**…**and*…*are mapped to<strong>, so emails will never render italic emphasis and any single-asterisk text will appear identical to bold.🛠️ Proposed fix
out = out.replace(/\*\*(.+?)\*\*/g, '<strong>$1</strong>') - out = out.replace(/\*(.+?)\*/g, '<strong>$1</strong>') + out = out.replace(/\*(.+?)\*/g, '<em>$1</em>')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/notification-channel/dispatchers/email-channel.service.ts` around lines 53 - 54, The second replacement mapping single asterisks is wrong: change the call to out.replace(/\*(.+?)\*/g, '<strong>$1</strong>') so it produces italics instead of bold; update it to out.replace(/\*(?!\*)(.+?)\*(?!\*)/g, '<em>$1</em>') (keep the existing bold replacement for /\*\*(.+?)\*\*/g first) so single-asterisk emphasis renders as <em> and does not collide with **bold**.backend/apps/cloud/src/notification-channel/dispatchers/email-channel.service.ts-49-58 (1)
49-58:⚠️ Potential issue | 🟡 MinorMarkdown link converter does not validate URL scheme.
simpleMarkdownToHtmlblindly inlines whatever sits between[…](…)into anhref, so a template like[click]({{some_var}})resolving tojavascript:alert(1)(ordata:) would produce a clickable XSS link in the resulting email. Templates are admin-authored, but variable values can be user-controlled (e.g.event_name,error_message). Consider rejecting or stripping non-http(s)/mailto:schemes when building the anchor.🛡️ Suggested hardening
- out = out.replace( - /\[([^\]]+)\]\(([^)]+)\)/g, - '<a href="$2" style="color: `#2563eb`;">$1</a>', - ) + out = out.replace(/\[([^\]]+)\]\(([^)]+)\)/g, (_, label, url) => { + const safe = /^(https?:|mailto:|\/)/i.test(url) ? url : '#' + return `<a href="${safe}" style="color: `#2563eb`;">${label}</a>` + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/notification-channel/dispatchers/email-channel.service.ts` around lines 49 - 58, The markdown-to-HTML link replacement in simpleMarkdownToHtml currently inlines the raw URL from the [text](url) capture into href, which allows javascript:, data:, or other dangerous schemes; modify the link handling in simpleMarkdownToHtml (the regex replace that produces '<a href="$2"...') to parse/validate the captured URL (e.g., with URL or a simple scheme whitelist) and only emit an <a> if the scheme is http, https, or mailto; for other/invalid schemes, either render the link text without an anchor or sanitize/encode the URL (e.g., prefix with "https://" or strip the scheme) so no executable schemes are injected. Ensure the change targets the link replacement logic and leaves other replacements (bold, italic, code, newline) unchanged.backend/apps/cloud/src/notification-channel/entity/notification-channel.entity.ts-38-41 (1)
38-41:⚠️ Potential issue | 🟡 MinorConstraint name should not include backticks.
The first argument to
@Checkis the constraint name; passing'`channel_scope_check`'includes literal backticks in the constraint identifier. TypeORM quotes identifiers automatically, so the name should be a plain string:'channel_scope_check'. Backticks belong inside the SQL expression for column references (like`userId`), not in the constraint name itself.🛠️ Proposed fix
`@Check`( - '`channel_scope_check`', + 'channel_scope_check', '((`userId` IS NOT NULL) + (`organisationId` IS NOT NULL) + (`projectId` IS NOT NULL)) = 1', )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/notification-channel/entity/notification-channel.entity.ts` around lines 38 - 41, The `@Check` decorator on the NotificationChannel entity uses a constraint name that incorrectly includes backticks; change the first argument from '`channel_scope_check`' to a plain string 'channel_scope_check' (leave the SQL expression string exactly as-is so column references keep their backticks), i.e., update the `@Check`(...) call to use the identifier without backticks to avoid creating a constraint name with literal backticks.backend/apps/cloud/src/notification-channel/notification-channel.controller.ts-290-299 (1)
290-299:⚠️ Potential issue | 🟡 MinorWebpush endpoint always gets
…appended even when shorter than 64 chars.
cfg.endpoint.slice(0, 64) + '…'unconditionally appends the ellipsis, producing misleading UI likehttps://short.example/abc…for endpoints already shorter than 64 chars.🐛 Proposed fix
- return { - endpoint: cfg.endpoint.slice(0, 64) + '…', - userAgent: cfg.userAgent, - } + return { + endpoint: + cfg.endpoint.length > 64 + ? cfg.endpoint.slice(0, 64) + '…' + : cfg.endpoint, + userAgent: cfg.userAgent, + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/notification-channel/notification-channel.controller.ts` around lines 290 - 299, The Webpush endpoint handling in the notification-channel.controller (the block checking channel.type === NotificationChannelType.WEBPUSH) always appends an ellipsis; change it to only truncate and append '…' when cfg.endpoint.length > 64 — otherwise return the full cfg.endpoint; keep returning userAgent as-is. Locate the endpoint logic in the WEBPUSH branch and replace the unconditional slice+append with a conditional that checks length before truncating so short endpoints are not mutated.backend/apps/cloud/src/notification-channel/dto/notification-channel.dto.ts-45-47 (1)
45-47:⚠️ Potential issue | 🟡 MinorAdd
@IsBoolean()touserScoped.With only
@IsOptional(), any value (string, number, object) passes validation. The service downstream likely does a truthy check, but this bypasses the validation pipeline and weakens API contract hygiene.🛡️ Proposed fix
-import { +import { + IsBoolean, IsEnum, IsNotEmpty, IsOptional, IsString, Length, IsObject, IsUUID, Matches, } from 'class-validator' @@ `@ApiProperty`({ required: false, default: false }) `@IsOptional`() + `@IsBoolean`() userScoped?: boolean🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/notification-channel/dto/notification-channel.dto.ts` around lines 45 - 47, The userScoped property on the DTO only has `@IsOptional`(), so non-boolean values pass validation; add the `@IsBoolean`() decorator to the userScoped field in notification-channel.dto.ts (on the userScoped property) and ensure IsBoolean is imported from class-validator (or added to the existing import alongside IsOptional) so the validation pipeline enforces boolean values.backend/apps/cloud/src/notification-channel/notification-channel.service.ts-500-510 (1)
500-510:⚠️ Potential issue | 🟡 MinorFull-table scan on Telegram unlink.
This loads every Telegram channel in the system into memory, then filters in JS by
config.chatId. On a Telegram/stop, that scales linearly with all linked Telegram users. Prefer a JSON-path WHERE so the database does the filtering.♻️ Suggested approach (MySQL JSON_EXTRACT)
async deleteTelegramChannelsByChatId(chatId: string) { await this.channelRepository .createQueryBuilder() .delete() .where('type = :type', { type: NotificationChannelType.TELEGRAM }) .andWhere(`JSON_UNQUOTE(JSON_EXTRACT(config, '$.chatId')) = :chatId`, { chatId }) .execute() }Adjust the JSON expression to your DB dialect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/notification-channel/notification-channel.service.ts` around lines 500 - 510, The current deleteTelegramChannelsByChatId function does a full-table load via channelRepository.find and filters in JS, causing scalability issues; change it to perform a DB-side delete using channelRepository.createQueryBuilder() (or equivalent ORM query) to delete rows where type = NotificationChannelType.TELEGRAM and the JSON path for config.chatId equals the provided chatId (use your DB's JSON_EXTRACT/JSON_UNQUOTE or dialect-appropriate JSON operator), so the filtering happens in SQL and no Telegram channels are loaded into memory.web/app/routes/notification-channel.tsx-134-162 (1)
134-162:⚠️ Potential issue | 🟡 MinorUpdate path silently swallows malformed
configJSON.
parseConfigreturns{}onJSON.parsefailure, soformData.has('config')plus malformed JSON results inbody.config = {}and a server-side reset of the channel's stored config (the backend'snormaliseConfigwill then throw or accept an empty/invalid config depending on type). Consider returning a 400 to the caller when the config string is present but unparseable, to avoid accidental data loss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/routes/notification-channel.tsx` around lines 134 - 162, The update-channel handler currently uses parseConfig which masks JSON parse errors by returning {} and can silently overwrite stored config; change the branch that handles formData.has('config') to first read the raw config string, attempt JSON.parse in a try/catch (or call parseConfig but propagate parse errors), and if parsing fails return data<NotificationChannelActionData> with intent and an error message and status 400 instead of proceeding to call serverFetch; only set body.config when parsing succeeds, then call serverFetch as before (symbols to edit: the 'update-channel' case, parseConfig usage, and the early return using data(..., { status: 400 })).backend/apps/cloud/src/notification-channel/notification-channel.service.ts-322-335 (1)
322-335:⚠️ Potential issue | 🟡 Minor
isVerifiedis asymmetric for WEBPUSH between create and update.In
create(Line 322-328) only SLACK and DISCORD are marked verified by default; WEBPUSH falls through and staysfalse. Inupdate(Line 431-436) WEBPUSH is added to the auto-verified set. Since thewebpush-subscriberoute flow lands increate, freshly subscribed webpush channels will appear asPendingin the UI, but editing them later will flip them toVerifiedwithout any verification step. Pick one rule and apply it consistently (most likely: webpush is implicitly verified on creation since the browser proved possession of the subscription).♻️ Proposed fix
if ( dto.type === NotificationChannelType.SLACK || - dto.type === NotificationChannelType.DISCORD + dto.type === NotificationChannelType.DISCORD || + dto.type === NotificationChannelType.WEBPUSH ) { partial.isVerified = true }Also applies to: 431-443
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/notification-channel/notification-channel.service.ts` around lines 322 - 335, The create flow currently marks only SLACK and DISCORD as implicitly verified, but update also treats WEBPUSH as auto-verified, causing inconsistent state; update the create logic inside the create method to include NotificationChannelType.WEBPUSH in the same branch that sets partial.isVerified = true (the block that checks dto.type for SLACK/DISCORD), so that partial.isVerified is set for WEBPUSH on creation just like in the update method; ensure you reference NotificationChannelType.WEBPUSH, partial.isVerified, and the create/update handlers to keep both flows consistent.backend/apps/cloud/src/notification-channel/notification-channel.service.ts-462-466 (1)
462-466:⚠️ Potential issue | 🟡 MinorReject empty/falsy verification tokens defensively.
If a controller path ever feeds an empty string into this lookup, TypeORM will happily issue
WHERE verificationToken = '', which can match channels whose token defaulted to empty string in the DB (or, depending on column nullability, surface unintended rows). Guard the input here so the helper is safe regardless of caller hygiene.🛡️ Proposed fix
async findByVerificationToken(token: string) { + if (!token) return null return this.channelRepository.findOne({ where: { verificationToken: token }, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/notification-channel/notification-channel.service.ts` around lines 462 - 466, The findByVerificationToken helper should defensively reject empty/falsy tokens before calling channelRepository.findOne to avoid accidental matches; update the async function findByVerificationToken(token: string) to first validate token (e.g., if token is falsy or token.trim() === '' ) and return null (or throw a controlled error) immediately, otherwise proceed to call channelRepository.findOne({ where: { verificationToken: token } }); this keeps the lookup safe regardless of caller input.web/app/components/NotificationChannels/NotificationChannels.tsx-235-243 (1)
235-243:⚠️ Potential issue | 🟡 Minor
onUpdateskips the empty-name guard.
onCreaterejects an empty name (Line 219-222) butonUpdatedoes not, so a user can save an all-whitespace name and rely on backend rejection. Mirror the create-side validation for consistency.♻️ Proposed fix
const onUpdate = () => { if (!editing) return + if (!form.name.trim()) { + toast.error(t('notificationChannels.nameRequired')) + return + } const formData = new FormData()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/components/NotificationChannels/NotificationChannels.tsx` around lines 235 - 243, The onUpdate handler currently allows saving a channel with an all-whitespace name; add the same empty-name guard used in onCreate: before building FormData in onUpdate, trim form.name and if it's empty return early (or abort the update) to prevent submitting invalid names, then proceed to set 'intent', 'channelId', 'name' (use the trimmed value), 'config' (via buildConfig(form)) and call submitMutate; reference the onUpdate function, the editing variable, form.name, buildConfig, and submitMutate when applying the change.backend/apps/cloud/src/notification-channel/notification-channel.service.ts-384-390 (1)
384-390:⚠️ Potential issue | 🟡 MinorConsider adding
discordapp.comto the allowlist for backward compatibility.Legacy Discord webhook URLs created before the domain migration (e.g.,
https://discordapp.com/api/webhooks/...) remain functional. If you want to support existing integrations that reference discordapp.com, add'discordapp.com'to the suffix list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/notification-channel/notification-channel.service.ts` around lines 384 - 390, The Discord webhook validation only allows 'discord.com' but should also permit legacy 'discordapp.com'; update the NotificationChannelType.DISCORD branch to pass both domains to isValidHttpsUrl (e.g., add 'discordapp.com' alongside 'discord.com') when validating the url derived from raw.url so legacy webhook URLs remain accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1de3c6f-8e12-49f7-b6af-89a6725203cb
⛔ Files ignored due to path filters (5)
backend/package-lock.jsonis excluded by!**/package-lock.jsondocs/public/img/analytics-dashboard/alert-conect-a-channel.pngis excluded by!**/*.pngdocs/public/img/analytics-dashboard/alert-template-editor.pngis excluded by!**/*.pngdocs/public/img/analytics-dashboard/alerts-list.pngis excluded by!**/*.pngdocs/public/img/analytics-dashboard/create-an-alert.pngis excluded by!**/*.png
📒 Files selected for processing (57)
README.mdbackend/.env.examplebackend/apps/cloud/src/ai/ai.controller.tsbackend/apps/cloud/src/alert/alert.controller.tsbackend/apps/cloud/src/alert/alert.module.tsbackend/apps/cloud/src/alert/alert.service.tsbackend/apps/cloud/src/alert/dto/alert.dto.tsbackend/apps/cloud/src/alert/entity/alert.entity.tsbackend/apps/cloud/src/analytics/utils/clickIdSources.tsbackend/apps/cloud/src/app.module.tsbackend/apps/cloud/src/mailer/mailer.service.tsbackend/apps/cloud/src/notification-channel/alert-context.tsbackend/apps/cloud/src/notification-channel/dispatchers/channel-dispatcher.service.tsbackend/apps/cloud/src/notification-channel/dispatchers/discord-channel.service.tsbackend/apps/cloud/src/notification-channel/dispatchers/email-channel.service.tsbackend/apps/cloud/src/notification-channel/dispatchers/slack-channel.service.tsbackend/apps/cloud/src/notification-channel/dispatchers/telegram-channel.service.tsbackend/apps/cloud/src/notification-channel/dispatchers/types.tsbackend/apps/cloud/src/notification-channel/dispatchers/webhook-channel.service.tsbackend/apps/cloud/src/notification-channel/dispatchers/webpush-channel.service.tsbackend/apps/cloud/src/notification-channel/dto/notification-channel.dto.tsbackend/apps/cloud/src/notification-channel/entity/notification-channel.entity.tsbackend/apps/cloud/src/notification-channel/notification-channel.controller.tsbackend/apps/cloud/src/notification-channel/notification-channel.module.tsbackend/apps/cloud/src/notification-channel/notification-channel.service.tsbackend/apps/cloud/src/notification-channel/template-renderer.service.tsbackend/apps/cloud/src/task-manager/task-manager.module.tsbackend/apps/cloud/src/task-manager/task-manager.service.tsbackend/migrations/mysql/2026_04_23_notification_channels.sqlbackend/migrations/mysql/_pending_drop_legacy_user_notification_columns.sqlbackend/package.jsondocs/components/channel-icons.tsxdocs/content/docs/accountsettings/teams-and-integrations.mdxdocs/content/docs/analytics-dashboard/alerts.mdxdocs/content/docs/sitesettings/get-traffic-alerts.mdxdocs/content/docs/sitesettings/meta.jsondocs/lib/source.tsdocs/mdx-components.tsxweb/app/components/NotificationChannels/EnableWebPushButton.tsxweb/app/components/NotificationChannels/NotificationChannels.tsxweb/app/components/NotificationChannels/utils.tsxweb/app/lib/models/Alerts.tsweb/app/lib/models/NotificationChannel.tsweb/app/pages/Organisations/Settings/index.tsxweb/app/pages/Project/Settings/Alerts/AlertTemplateEditor.tsxweb/app/pages/Project/Settings/Alerts/ProjectAlertsSettings.tsxweb/app/pages/Project/Settings/Alerts/ProjectAlertsView.tsxweb/app/pages/Project/Settings/Alerts/RichTemplateInput.tsxweb/app/pages/Project/Settings/ProjectSettings.tsxweb/app/pages/Project/Settings/SettingsSidebar.tsxweb/app/pages/UserSettings/UserSettings.tsxweb/app/pages/UserSettings/components/Integrations.tsxweb/app/routes/notification-channel.tsxweb/app/routes/projects.$id.tsxweb/app/ui/Tooltip.tsxweb/public/locales/en.jsonweb/public/sw.js
💤 Files with no reviewable changes (4)
- docs/content/docs/sitesettings/meta.json
- docs/lib/source.ts
- docs/content/docs/sitesettings/get-traffic-alerts.mdx
- web/app/pages/UserSettings/components/Integrations.tsx
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
web/public/sw.js (1)
37-55: Use the normalisedtargetUrl.hrefforopenWindowtoo.
openWindowstill receives the rawurl. When thenew URL(url, self.location.origin)constructor throws and the catch falls backtargetUrlto the origin's/, this branch will still try to open the malformed value. It's also inconsistent with the matching path that usestargetUrl.href.♻️ Proposed change
- if (self.clients.openWindow) return self.clients.openWindow(url) + if (self.clients.openWindow) + return self.clients.openWindow(targetUrl.href) return null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/public/sw.js` around lines 37 - 55, The service worker currently falls back to a normalized targetUrl but still calls self.clients.openWindow(url) with the raw/un-normalized url; update the event.waitUntil callback so any call to openWindow uses the normalized targetUrl.href instead of url (keep the existing targetUrl creation and the client.url === targetUrl.href check, then replace self.clients.openWindow(url) with self.clients.openWindow(targetUrl.href) to ensure consistency and avoid trying to open a malformed URL).backend/apps/cloud/src/main.ts (1)
19-19: Consider enablingwhitelistto strip unknown request properties.
transform: trueis correctly applied to support the@Type()decorators (WebpushSubscribeDTO) and@Transformdecorators (alert.dto.ts). Since this validation is now global, addingwhitelist: trueensures unknown properties in request bodies are stripped, preventing unintended fields from reaching the database. This is a straightforward security hardening with no risk—handlers use only their declared DTO properties.♻️ Proposed refinement
- app.useGlobalPipes(new ValidationPipe({ transform: true })) + app.useGlobalPipes( + new ValidationPipe({ transform: true, whitelist: true }), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/main.ts` at line 19, The global ValidationPipe is missing whitelist: true, so unknown request properties are not being stripped; update the app.useGlobalPipes call that creates the ValidationPipe (where transform: true is set) to also enable whitelist: true so incoming requests are sanitized and only DTO-declared properties (e.g., WebpushSubscribeDTO, alert DTOs using `@Type/`@Transform) are kept.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/cloud/src/alert/alert.controller.ts`:
- Around line 221-226: The controller currently hardcodes the default email
subject to 'Alert: {{alert.name}}', which uses incorrect dotted variable syntax
and the wrong prefix/suffix; instead import and use the exported
DEFAULT_EMAIL_SUBJECT_TEMPLATE from template-renderer.service.ts and assign that
to alertDTO.emailSubjectTemplate when an EMAIL channel is present and no subject
was provided (the relevant symbols are validatedChannels,
NotificationChannelType.EMAIL, alertDTO.emailSubjectTemplate, and
DEFAULT_EMAIL_SUBJECT_TEMPLATE); ensure you remove the hardcoded string so the
controller and TemplateRendererService remain synchronized with the flat
underscore variables like alert_name.
In
`@backend/apps/cloud/src/notification-channel/notification-channel.controller.ts`:
- Around line 245-262: sendEmailVerification injects unescaped cfg.address into
the HTML body (see sendEmailVerification and
NotificationChannelService.isValidEmailAddress), enabling HTML injection; fix by
HTML-escaping cfg.address before interpolating it into the html string (or move
this email into the existing templating pipeline such as a Pug/Handlebars
LetterTemplate which auto-escapes), i.e., sanitize/escape the address (reject or
encode characters like <, >, ", ') and then use the escaped value in the
template and in mailerService.sendRawEmail to ensure no raw user-controlled
markup is sent.
- Around line 248-250: The verification URL construction in
notification-channel.controller (const clientUrl and url =
`${clientUrl}/api/notification-channel/verify/${channel.verificationToken}`) is
inconsistent with the unsubscribe URL built in email-channel.service (which uses
`${clientUrl}/notification-channel/unsubscribe/${token}`) and both controller
handlers are public (`@Get`('/verify/:token') and `@Get`('/unsubscribe/:token'));
fix by aligning the public path prefix for both endpoints: either remove the
`/api` prefix from the verify URL or add `/api` to the unsubscribe URL so both
use the same public path pattern, or centralize the path construction into a
shared helper/constant (e.g., NOTIFICATION_CHANNEL_PATH) and replace the inline
string in both places to ensure consistency across
notification-channel.controller and email-channel.service.
In `@backend/apps/cloud/src/task-manager/task-manager.service.ts`:
- Around line 1842-1847: When an alert's condition is met in checkMetricAlerts
but errorDetails is missing, ensure the alert doesn't keep refiring: update the
alert's lastTriggered (or call the existing persistence/update path used
elsewhere) before returning and/or dispatch the handler with a generic error
context (e.g., empty error_name/error_message fields) so a notification is sent;
locate the conditional that checks queryMetric === QueryMetric.ERRORS and the
variables conditionMet, errorDetails and lastTriggered and either set
lastTriggered (persisting the change) or build a fallback context and call the
same dispatch/handler used for successful details.
- Around line 1907-1913: Move the update of alert.lastTriggered to before
rendering/dispatch to match checkOnlineUsersAlerts and prevent duplicate sends:
call alertService.update(alert.id, { lastTriggered: new Date() }) immediately
when handling the alert (before invoking renderAlertMessage and
channelDispatcher.dispatch), then proceed to call renderAlertMessage(alert,
context, hasEmail) and channelDispatcher.dispatch(alert.channels, message); keep
existing error handling around dispatch but ensure failures in render/dispatch
do not revert the prior lastTriggered update so the alert won't be retriggered
within the cron window. Reference: renderAlertMessage,
channelDispatcher.dispatch, alertService.update, checkOnlineUsersAlerts.
In `@web/app/components/NotificationChannels/NotificationChannels.tsx`:
- Around line 235-248: The onUpdate handler currently always attaches config
(via buildConfig(form)) which causes webpush channels to be overwritten with an
empty config; modify onUpdate so it only sets formData.set('config', ...) when
the editing channel is NOT of type 'webpush' (e.g., check editing.type !==
'webpush' before adding config and calling JSON.stringify(buildConfig(form))).
Keep the name update path intact for webpush, and also consider gating the edit
UI (the pencil/open-edit flow) so the modal or submit handler (submitMutate)
cannot be triggered for channels where channel.type === 'webpush' to prevent
Enter-key submits from firing.
---
Nitpick comments:
In `@backend/apps/cloud/src/main.ts`:
- Line 19: The global ValidationPipe is missing whitelist: true, so unknown
request properties are not being stripped; update the app.useGlobalPipes call
that creates the ValidationPipe (where transform: true is set) to also enable
whitelist: true so incoming requests are sanitized and only DTO-declared
properties (e.g., WebpushSubscribeDTO, alert DTOs using `@Type/`@Transform) are
kept.
In `@web/public/sw.js`:
- Around line 37-55: The service worker currently falls back to a normalized
targetUrl but still calls self.clients.openWindow(url) with the
raw/un-normalized url; update the event.waitUntil callback so any call to
openWindow uses the normalized targetUrl.href instead of url (keep the existing
targetUrl creation and the client.url === targetUrl.href check, then replace
self.clients.openWindow(url) with self.clients.openWindow(targetUrl.href) to
ensure consistency and avoid trying to open a malformed URL).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21e5570a-ecd8-4640-937f-fbbc4bb6b8f5
⛔ Files ignored due to path filters (2)
backend/package-lock.jsonis excluded by!**/package-lock.jsondocs/public/img/analytics-dashboard/alert-connect-a-channel.pngis excluded by!**/*.png
📒 Files selected for processing (31)
README.mdbackend/apps/cloud/src/alert/alert.controller.tsbackend/apps/cloud/src/alert/dto/alert.dto.tsbackend/apps/cloud/src/mailer/mailer.service.tsbackend/apps/cloud/src/main.tsbackend/apps/cloud/src/notification-channel/dispatchers/email-channel.service.tsbackend/apps/cloud/src/notification-channel/dispatchers/telegram-channel.service.tsbackend/apps/cloud/src/notification-channel/dispatchers/webhook-channel.service.tsbackend/apps/cloud/src/notification-channel/dispatchers/webpush-channel.service.tsbackend/apps/cloud/src/notification-channel/dto/notification-channel.dto.tsbackend/apps/cloud/src/notification-channel/entity/notification-channel.entity.tsbackend/apps/cloud/src/notification-channel/notification-channel.controller.tsbackend/apps/cloud/src/notification-channel/notification-channel.service.tsbackend/apps/cloud/src/project/project.service.tsbackend/apps/cloud/src/task-manager/task-manager.service.tsbackend/migrations/mysql/2026_04_26_notification_channel_disabled_reason.sqlbackend/package.jsondocs/content/docs/analytics-dashboard/alerts.mdxweb/app/components/NotificationChannels/NotificationChannels.tsxweb/app/lib/models/NotificationChannel.tsweb/app/pages/Project/Settings/Alerts/AlertTemplateEditor.tsxweb/app/pages/Project/Settings/Alerts/ProjectAlertsSettings.tsxweb/app/pages/Project/Settings/Alerts/ProjectAlertsView.tsxweb/app/pages/Project/Settings/Alerts/RichTemplateInput.tsxweb/app/pages/Project/Settings/SettingsSidebar.tsxweb/app/pages/Project/tabs/AskAI/AskAIView.tsxweb/app/pages/UserSettings/UserSettings.tsxweb/app/routes/notification-channel.tsxweb/app/routes/projects.$id.tsxweb/public/locales/en.jsonweb/public/sw.js
✅ Files skipped from review due to trivial changes (3)
- backend/migrations/mysql/2026_04_26_notification_channel_disabled_reason.sql
- web/app/lib/models/NotificationChannel.ts
- backend/apps/cloud/src/notification-channel/dispatchers/webpush-channel.service.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- backend/package.json
- backend/apps/cloud/src/mailer/mailer.service.ts
- README.md
- web/app/pages/UserSettings/UserSettings.tsx
- backend/apps/cloud/src/notification-channel/dispatchers/webhook-channel.service.ts
- web/public/locales/en.json
- backend/apps/cloud/src/notification-channel/dispatchers/email-channel.service.ts
- backend/apps/cloud/src/notification-channel/dispatchers/telegram-channel.service.ts
- web/app/pages/Project/Settings/SettingsSidebar.tsx
- web/app/routes/notification-channel.tsx
- web/app/pages/Project/Settings/Alerts/ProjectAlertsSettings.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
web/public/sw.js (2)
11-32: Consider adding atagto deduplicate notifications.Without
options.tag, repeated pushes for the same alert/dashboard will stack as separate OS notifications instead of replacing the previous one. If the payload carries a stable identifier (e.g., alert id), thread it through and settag(optionally withrenotify: true) so re-fires for the same alert update in place rather than piling up.♻️ Example
const options = { body, icon: '/favicon.ico', badge: '/favicon.ico', data: { url }, + tag: data.tag || data.alert_id || undefined, timestamp: Date.now(), }Requires the backend
webpush-channel.service.tspayload to include a stabletag/alert_id.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/public/sw.js` around lines 11 - 32, The push handler (self.addEventListener('push')) currently builds an options object without a tag, causing duplicate OS notifications; update the options object in the push event to set options.tag to a stable identifier from the payload (e.g., data.tag || data.alert_id || data.id || data.url) and optionally set options.renotify = true if you want updates to re-open the notification, then keep using event.waitUntil(self.registration.showNotification(title, options)); ensure the backend includes a stable tag/alert id in the push payload so this field is available.
1-58: Addpushsubscriptionchangehandler to re-subscribe when push credentials are invalidated.Browsers fire
pushsubscriptionchangewhen a push subscription is invalidated or rotated (e.g., key rotation, GCM/FCM refresh). Without a handler, the client silently stops receiving notifications until the user re-visits a page that re-subscribes. The backend supports re-subscription viaPOST /notification-channelwith the same endpoint and structure thatEnableWebPushButton.tsxuses. Consider adding:self.addEventListener('pushsubscriptionchange', (event) => { event.waitUntil( self.registration.pushManager .subscribe(event.oldSubscription?.options ?? { userVisibleOnly: true }) .then((subscription) => { // POST subscription to backend (same structure as EnableWebPushButton.tsx) // Note: requires auth headers (Bearer token) in the fetch request }), ) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/public/sw.js` around lines 1 - 58, Add a pushsubscriptionchange handler in the service worker to re-subscribe when credentials are rotated: implement self.addEventListener('pushsubscriptionchange', ...) that calls event.waitUntil and uses self.registration.pushManager.subscribe(event.oldSubscription?.options ?? { userVisibleOnly: true }) to obtain a new subscription, then POST the subscription JSON to the backend endpoint used by EnableWebPushButton.tsx (POST /notification-channel) including the same auth (Bearer) header; on failure handle/log errors and ensure the promise returned to event.waitUntil resolves/rejects appropriately.backend/apps/cloud/src/notification-channel/dispatchers/email-channel.service.ts (1)
106-121: Optional: prefercrypto.timingSafeEqualover a hand-rolled comparison loop.The manual XOR loop is correct, but Node's
crypto.timingSafeEqualis the conventional primitive for this — it asserts equal lengths via aRangeErrorafter Buffer construction (you already early-return on length mismatch) and avoids the small risk that a future refactor regresses the constant-time property by introducing an earlybreakor short-circuit.♻️ Suggested refactor
-import { createHmac } from 'crypto' +import { createHmac, timingSafeEqual } from 'crypto' @@ verifyUnsubscribeToken(token: string): string | null { const dot = token.indexOf('.') if (dot === -1) return null const channelId = token.slice(0, dot) const sig = token.slice(dot + 1) const secret = this.getUnsubscribeSecret() - const expected = createHmac('sha256', secret) - .update(channelId) - .digest('hex') - if (sig.length !== expected.length) return null - let mismatch = 0 - for (let i = 0; i < sig.length; i++) { - mismatch |= sig.charCodeAt(i) ^ expected.charCodeAt(i) - } - return mismatch === 0 ? channelId : null + const expected = createHmac('sha256', secret).update(channelId).digest() + let sigBuf: Buffer + try { + sigBuf = Buffer.from(sig, 'hex') + } catch { + return null + } + if (sigBuf.length !== expected.length) return null + return timingSafeEqual(sigBuf, expected) ? channelId : null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/notification-channel/dispatchers/email-channel.service.ts` around lines 106 - 121, Replace the hand-rolled constant-time comparison in verifyUnsubscribeToken with Node's crypto.timingSafeEqual: after computing expected, convert sig and expected to Buffers (or Uint8Array), keep the existing early-return length check, then call timingSafeEqual on the two buffers and return channelId only if it returns true; remove the XOR loop and related mismatch logic and reference crypto.timingSafeEqual instead of manual comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/cloud/src/task-manager/task-manager.service.ts`:
- Around line 1599-1604: The code reads this.configService.get('CLIENT_URL')
into clientUrl without a fallback, causing dashboard_url/errors_url to be
"undefined/..." in misconfigured envs; update the clientUrl retrieval (where
clientUrl is used to build AlertContext.dashboard_url and errors_url) to default
to 'https://swetrix.com' when the env var is unset (e.g., replace the direct get
with a fallback expression) and apply the same change to the other call site
that builds those URLs.
In `@web/app/components/NotificationChannels/NotificationChannels.tsx`:
- Around line 82-93: Editing a Slack/Discord channel currently sends the masked
URL back to the server because buildConfig unconditionally returns { url:
form.url.trim() }; change buildConfig (the slack/discord branch) to omit the url
key when form.url is the backend-redacted placeholder (e.g., contains the
ellipsis character '…' or '...' or matches the redactConfig pattern used for
SLACK/DISCORD), mirroring the webhook-secret fix: only attach url to the
returned config when form.url.trim() is non-empty and does not look redacted;
references: buildConfig, onEdit, form.url, redactConfig.
---
Nitpick comments:
In
`@backend/apps/cloud/src/notification-channel/dispatchers/email-channel.service.ts`:
- Around line 106-121: Replace the hand-rolled constant-time comparison in
verifyUnsubscribeToken with Node's crypto.timingSafeEqual: after computing
expected, convert sig and expected to Buffers (or Uint8Array), keep the existing
early-return length check, then call timingSafeEqual on the two buffers and
return channelId only if it returns true; remove the XOR loop and related
mismatch logic and reference crypto.timingSafeEqual instead of manual
comparison.
In `@web/public/sw.js`:
- Around line 11-32: The push handler (self.addEventListener('push')) currently
builds an options object without a tag, causing duplicate OS notifications;
update the options object in the push event to set options.tag to a stable
identifier from the payload (e.g., data.tag || data.alert_id || data.id ||
data.url) and optionally set options.renotify = true if you want updates to
re-open the notification, then keep using
event.waitUntil(self.registration.showNotification(title, options)); ensure the
backend includes a stable tag/alert id in the push payload so this field is
available.
- Around line 1-58: Add a pushsubscriptionchange handler in the service worker
to re-subscribe when credentials are rotated: implement
self.addEventListener('pushsubscriptionchange', ...) that calls event.waitUntil
and uses self.registration.pushManager.subscribe(event.oldSubscription?.options
?? { userVisibleOnly: true }) to obtain a new subscription, then POST the
subscription JSON to the backend endpoint used by EnableWebPushButton.tsx (POST
/notification-channel) including the same auth (Bearer) header; on failure
handle/log errors and ensure the promise returned to event.waitUntil
resolves/rejects appropriately.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc5070da-aa55-4117-ba62-b7a3c1c5c585
📒 Files selected for processing (8)
backend/apps/cloud/src/alert/alert.controller.tsbackend/apps/cloud/src/main.tsbackend/apps/cloud/src/notification-channel/dispatchers/email-channel.service.tsbackend/apps/cloud/src/notification-channel/notification-channel.controller.tsbackend/apps/cloud/src/notification-channel/notification-channel.paths.tsbackend/apps/cloud/src/task-manager/task-manager.service.tsweb/app/components/NotificationChannels/NotificationChannels.tsxweb/public/sw.js
✅ Files skipped from review due to trivial changes (1)
- backend/apps/cloud/src/notification-channel/notification-channel.paths.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/cloud/src/main.ts
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/cloud/src/integrations/telegram/telegram.service.ts`:
- Around line 127-162: Replace the duplicate persistence logic in
TelegramService by delegating to NotificationChannelService: remove the private
methods upsertTelegramChannel and deleteTelegramChannelsByChatId and the
`@InjectRepository`(NotificationChannel) wiring, inject NotificationChannelService
and call its public upsertTelegramChannel(...) and
deleteTelegramChannelsByChatId(...) instead; if injecting causes a circular
module dependency, move the Telegram-specific link/unlink logic into
NotificationChannelService (expose the two methods there) and have
TelegramService call those public methods so only one implementation remains.
- Around line 153-162: deleteTelegramChannelsByChatId currently deletes all
NotificationChannel rows of type TELEGRAM for a chatId regardless of owner;
change the method signature (deleteTelegramChannelsByChatId) to accept the
resolved userId (from confirmUnlinkAccount / getUserByTelegramId) and update the
channelRepository query to add an AND filter for userId = :userId so the DELETE
is scoped to the unlinking user's NotificationChannel rows only; ensure callers
(e.g., confirmUnlinkAccount) pass the userId through when invoking this method
and bind the userId param in the query.
In `@backend/apps/cloud/src/notification-channel/notification-channel.service.ts`:
- Around line 286-329: enforceScopeLimit currently does a non-transactional
channelRepository.count followed by channelRepository.save which allows a TOCTOU
race; fix by wrapping the count+save in a database transaction and taking a
pessimistic lock on the scope parent row (user/organisation/project) before
re-checking the count and saving the new channel, or alternatively enforce the
limit at the DB level with a constraint/trigger. Concretely, in the create flow
call a transactional helper (TypeORM QueryRunner or
repository.manager.transaction) that: 1) loads the parent scope row with a
SELECT ... FOR UPDATE / lock (e.g., repository.manager.findOne or findOneBy with
lock: { mode: 'pessimistic_write' } on the user/organisation/project entity
depending on scope), 2) runs channelRepository.count({ where }) inside the same
transaction, 3) throws the same ForbiddenException if count >=
MAX_CHANNELS_PER_SCOPE, and 4) saves via channelRepository.save within the
transaction so count+save are atomic; as an alternative implement a DB-side
enforcement (unique/check constraint or trigger) to guarantee the max even under
concurrency.
- Around line 467-499: These two methods (upsertTelegramChannel and
deleteTelegramChannelsByChatId) are duplicates — remove them from
NotificationChannelService and centralize ownership in the TelegramService
implementation; specifically, delete upsertTelegramChannel and
deleteTelegramChannelsByChatId from this file and ensure the canonical methods
live in TelegramService (the canonical upsert should also set disabledReason =
null on existing rows like the other copy does), and make
deleteTelegramChannelsByChatId accept/handle an optional userId parameter (or
otherwise scope deletion by user when callers have userId) to avoid deleting
rows across users; update any callers to call TelegramService's methods.
- Around line 374-385: The WEBPUSH branch (NotificationChannelType.WEBPUSH)
currently accepts any non-empty endpoint string which can lead to SSRF; update
the validation to require a valid HTTPS URL (reuse isValidHttpsUrl(...) or
WebhookChannelService.validateUrl(...) to leverage existing SSRF protections)
and additionally enforce an allowlist of known push service hostnames (e.g.
fcm.googleapis.com, updates.push.services.mozilla.com, *.notify.windows.com,
web.push.apple.com); if the endpoint fails HTTPS validation or is not on the
allowlist, throw BadRequestException('Invalid web push subscription') and
preserve the existing keys/userAgent handling.
- Around line 90-96: getProjectChannelsForUserOwnedProjects currently calls
projectService.getProjectIdsByAdminId(userId) which only returns direct owner
projects, causing shared admins to not see channels they can manage; update
getProjectChannelsForUserOwnedProjects (and any callers like getVisibleChannels)
to use the broader projectService.getProjectIdsViewableByUser(userId) (or the
equivalent viewable/project IDs method that includes confirmed shared admins and
org members) so visible channels align with
allowedToManage/ensureCallerCanManage authorization checks; ensure the
repository query still filters by project IDs (In(pids)) and preserve the
empty-array short-circuit when no IDs are returned.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f82afa6-2617-4614-b452-c05201a1da6d
📒 Files selected for processing (7)
backend/apps/cloud/src/integrations/telegram/telegram.module.tsbackend/apps/cloud/src/integrations/telegram/telegram.service.tsbackend/apps/cloud/src/notification-channel/dispatchers/telegram-channel.service.tsbackend/apps/cloud/src/notification-channel/dispatchers/types.tsbackend/apps/cloud/src/notification-channel/notification-channel.service.tsbackend/apps/cloud/src/task-manager/task-manager.module.tsbackend/apps/cloud/src/task-manager/task-manager.service.ts
✅ Files skipped from review due to trivial changes (1)
- backend/apps/cloud/src/notification-channel/dispatchers/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/cloud/src/task-manager/task-manager.module.ts
- backend/apps/cloud/src/notification-channel/dispatchers/telegram-channel.service.ts
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/apps/cloud/src/task-manager/task-manager.service.ts (1)
1827-1926:⚠️ Potential issue | 🟡 MinorMove
lastTriggeredupdate ahead of the detail query, likecheckOnlineUsersAlertsdoes.Today the order is: count query → detail query (errors only) → context build →
alertService.update(... lastTriggered)→ dispatch. If two cron ticks overlap (slow ClickHouse on the detail query, restart, etc.), a non-ERRORS metric whose cooldown gate at line 1670-1684 still hasn't been refreshed could re-enter and re-dispatch. The fix mirrorscheckOnlineUsersAlerts(line 1607): updatelastTriggeredimmediately afterconditionMetis true, then run detail/render/dispatch. ERRORS andalertOnEveryCustomEventpaths bypass the cooldown anyway and aren't affected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/task-manager/task-manager.service.ts` around lines 1827 - 1926, The cron alert handler checkMetricAlerts currently updates lastTriggered only after fetching error details and building the context, which allows a race where non-ERRORS alerts can retrigger if the detail query is slow; move the update call to invoke alertService.update(alert.id, { lastTriggered: new Date() }) immediately after the code path where conditionMet is true (the same place checkOnlineUsersAlerts updates lastTriggered) and before making the ClickHouse detailQuery / building context and rendering/dispatching; ensure you keep ERRORS and alert.alertOnEveryCustomEvent behavior unchanged (they bypass cooldown) while performing the early update for the other metric paths.backend/apps/cloud/src/integrations/telegram/telegram.service.ts (1)
84-128:⚠️ Potential issue | 🟡 MinorLink/unlink across two services is non-atomic — partial failures leave inconsistent state.
Both flows commit their User mutation and the NotificationChannel mutation as separate writes:
confirmLinkAccountflipsisTelegramChatIdConfirmed = truefirst, then upserts the channel. If the upsert throws, the user appears linked but has no channel and won't receive any new-style alerts.confirmUnlinkAccountdeletes channels first, then clearstelegramChatId. If the clear fails, the user keeps a stale chat id while the channel records are gone.Wrapping both in a single TypeORM transaction (or shifting both writes into
NotificationChannelService.upsertTelegramChannel/deleteTelegramChannelsByChatIdand using one transaction there) eliminates the divergent states. Low severity in practice, but cheap to make robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/integrations/telegram/telegram.service.ts` around lines 84 - 128, The confirmLinkAccount and confirmUnlinkAccount flows perform user and notification-channel writes separately, causing possible inconsistent state on partial failure; wrap both operations in a single TypeORM transaction (or move both writes into NotificationChannelService methods and have that service start the transaction) so that confirmLinkAccount executes userService.updateUserTelegramId(..., true) and getNotificationChannelService().upsertTelegramChannel(...) inside one transaction, and confirmUnlinkAccount executes getNotificationChannelService().deleteTelegramChannelsByChatId(...) and userService.updateUserTelegramId(..., null) inside one transaction, ensuring either both commits or a rollback on error.
🧹 Nitpick comments (6)
web/app/components/NotificationChannels/NotificationChannels.tsx (3)
460-474: Optional: gate the edit pencil forwebpushchannels.For webpush, the edit modal renders only a name input and the "Use button" hint (Lines 629–633); all other inputs are hidden. The functional wipe is now prevented in
onUpdate, but the UX is still a near-empty modal that can only rename. Either hide the pencil for webpush, or rename the modal title/empty state for that case.♻️ Suggested gating
- <Tooltip - text={t('common.edit')} - ariaLabel={t('common.edit')} - asChild - tooltipNode={ - <button - type='button' - onClick={() => onEdit(channel)} - disabled={isMutating} - ... - > - <PencilSimpleIcon className='size-4' aria-hidden /> - </button> - } - /> + {channel.type !== 'webpush' ? ( + <Tooltip + text={t('common.edit')} + ariaLabel={t('common.edit')} + asChild + tooltipNode={ + <button + type='button' + onClick={() => onEdit(channel)} + disabled={isMutating} + ... + > + <PencilSimpleIcon className='size-4' aria-hidden /> + </button> + } + /> + ) : null}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/components/NotificationChannels/NotificationChannels.tsx` around lines 460 - 474, The edit pencil should be hidden for channels of type 'webpush' to avoid opening an almost-empty rename-only modal: update the conditional around the Tooltip/PencilSimpleIcon render in the NotificationChannels component so it checks channel.type !== 'webpush' before rendering the Tooltip with onEdit(channel); reference the existing Tooltip, PencilSimpleIcon and onEdit(channel) symbols and gate that rendering path accordingly (alternatively, you can branch inside the click handler to open a different modal/title for webpush, but prefer hiding the pencil by checking channel.type).
284-295: Tighten thecfgcast to a typed shape.
(channel.config || {}) as Record<string, any>discards the discriminated channel-config type and lets typos slip through (e.g.,cfg.adress). Since the keys are well-known per channel type, narrow them in one place.♻️ Suggested typing
- const onEdit = (channel: NotificationChannel) => { - const cfg = (channel.config || {}) as Record<string, any> + const onEdit = (channel: NotificationChannel) => { + const cfg = (channel.config || {}) as Partial<{ + address: string + chatId: string + url: string + }> setEditing(channel) setForm({ name: channel.name, type: channel.type, email: cfg.address || '', chatId: cfg.chatId || '', url: cfg.url || '', secret: '', }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/components/NotificationChannels/NotificationChannels.tsx` around lines 284 - 295, The onEdit handler currently casts channel.config to a loose Record<string, any> which loses the discriminated types and allows typos; instead narrow the config by the channel.type before reading fields: inspect channel.type (or switch on it) and cast channel.config to the appropriate shaped interface (e.g., EmailConfig, ChatConfig, WebhookConfig) so you read cfg.address, cfg.chatId, cfg.url with proper types, then call setEditing and setForm as before; update onEdit to reference channel.config, channel.type, onEdit, setForm, and setEditing when applying the narrowed config.
147-163: Minor: stabilisetriggerListand drop the eslint-disable.
triggerListis already wrapped inuseCallback, butlistFetcheris omitted from its deps (Line 158) and from the consumer effect (Line 163). IncludingtriggerListin the effect deps would be both correct and idiomatic, since its identity already tracksscope/organisationId/projectId. This removes both// eslint-disable-next-line react-hooks/exhaustive-depsdirectives and avoids stale-closure surprises ifuseFetcher's identity ever changes between renders.♻️ Suggested cleanup
const triggerList = useCallback(() => { const formData = new FormData() formData.set('intent', 'list-channels') formData.set('scope', scope) if (projectId) formData.set('projectId', projectId) if (organisationId) formData.set('organisationId', organisationId) listFetcher.submit(formData, { method: 'POST', action: '/notification-channel', }) - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [scope, organisationId, projectId]) + }, [scope, organisationId, projectId, listFetcher]) useEffect(() => { triggerList() - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [scope, organisationId, projectId]) + }, [triggerList])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/components/NotificationChannels/NotificationChannels.tsx` around lines 147 - 163, The triggerList callback currently omits listFetcher from its dependency array and the consuming useEffect disables linting; update the useCallback for triggerList to include listFetcher in its dependency list so its identity is stable with respect to the fetcher, then change the useEffect to depend on triggerList (instead of repeating scope/organisationId/projectId) and remove both eslint-disable-next-line react-hooks/exhaustive-deps comments; this ensures triggerList, listFetcher and the useEffect stay in sync and prevents stale-closure issues.backend/apps/cloud/src/task-manager/task-manager.service.ts (1)
345-377: Telegram-context escape allow-list is hard-coded — make sure new URL fields stay in sync.
TELEGRAM_MARKDOWN_URL_KEYS = new Set(['dashboard_url', 'errors_url'])matches the URL-bearing fields inAlertContexttoday. If anyone adds another URL variable toalert-context.ts(e.g.,subscribers_url,goal_url), it'll silently get markdown-escaped here and Telegram users will see backslashes in their links. A type-level mapping (e.g., aURL_KEYSconstant exported fromalert-context.tsconsumed both here and by the renderer) would prevent that drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/task-manager/task-manager.service.ts` around lines 345 - 377, The hard-coded TELEGRAM_MARKDOWN_URL_KEYS set can drift from AlertContext fields; export a single source-of-truth URL_KEYS (or equivalent typed constant) from alert-context.ts that lists the URL-bearing keys, then import and use that constant inside renderAlertMessage instead of the local TELEGRAM_MARKDOWN_URL_KEYS; update the code that builds telegramContext to consult this shared URL_KEYS and ensure its type aligns with AlertContext so future additions (e.g., subscribers_url, goal_url) remain in sync.backend/apps/cloud/src/integrations/telegram/telegram.service.ts (1)
131-136:@Optional()injection that throws on use — confirm it's reachable only when wired.The module-level
forwardRef(() => NotificationChannelModule)import intelegram.module.tsshould always provide this dependency in production; the@Optional()and runtime throw exist only because the alternative is an unhandledundefined. If a non-cloud build (e.g., self-hosted) ever loadsTelegramModulewithoutNotificationChannelModule,confirmLinkAccount/confirmUnlinkAccountwill surface an opaque "NotificationChannelService is not available" error to the bot. If that scenario is real, prefer guarding both methods early with a check + user-facing message; if it isn't, drop@Optional()and let DI fail loudly at boot instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/integrations/telegram/telegram.service.ts` around lines 131 - 136, The current getNotificationChannelService() throws a runtime "NotificationChannelService is not available" error when the `@Optional-injected` NotificationChannelService is absent; decide whether this dependency is always provided by keeping NotificationChannelModule (forwardRef in telegram.module.ts) and removing `@Optional` so DI fails at boot, or handle missing service gracefully by keeping `@Optional` and updating confirmLinkAccount and confirmUnlinkAccount to check for notificationChannelService via getNotificationChannelService() guard (or a safe isAvailable() helper) and reply to the user with a clear message instead of throwing. Locate getNotificationChannelService(), confirmLinkAccount, confirmUnlinkAccount, and the `@Optional` injection in telegram.service.ts and apply the chosen fix consistently.backend/apps/cloud/src/notification-channel/notification-channel.service.ts (1)
346-391: Pessimistic-write transaction closes the count/save TOCTOU.Locking the scope parent (User/Organisation/Project) before counting + saving is the right shape and addresses the prior race comment. One thing to keep an eye on operationally: this serializes channel creation per scope behind a row-level write lock on the parent entity, so any unrelated long-running transaction that holds a read/write lock on that User/Organisation/Project row will now block channel creation. Not a bug — just worth knowing if you start seeing lock-wait timeouts on
notification_channelcreates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/notification-channel/notification-channel.service.ts` around lines 346 - 391, The current locking in lockScopeParent invoked by saveWithinScopeLimit correctly prevents the TOCTOU by acquiring a pessimistic_write on the parent row; keep the logic but add a short developer note and operational mitigations: add an inline comment in lockScopeParent/saveWithinScopeLimit describing the serialization tradeoff and potential for lock-wait timeouts, and implement a small operational change — expose a configurable lock-wait/transaction timeout (read from config) and emit or log a metric/event when acquiring the lock or when a lock wait occurs so operators can monitor contention; do not change the existing lock semantics that prevent the race, only add the comment, config-driven timeout handling, and metric/log emission around the lock acquisition to make the behavioral tradeoff observable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/apps/cloud/src/notification-channel/dispatchers/email-channel.service.ts`:
- Around line 57-71: The current simpleMarkdownToHtml function applies link
replacement first and then runs bold/italic/code regexes over the whole output,
which lets those later regexes corrupt characters inside generated <a href="…">
attributes; fix by making the link pass emit placeholders for each anchor
(capture both href and anchor HTML using isAllowedLinkHref) and store the full
anchor string in a map, then run the bold/italic/code/linebreak replacements
only on the remaining text, and finally re-insert the stored anchor HTML
placeholders back into the output; reference simpleMarkdownToHtml, escapeHtml
and isAllowedLinkHref to locate where to implement the placeholder
map-and-reinsert approach.
- Around line 106-119: Wrap calls to
getUnsubscribeSecret()/verifyUnsubscribeToken in the unsubscribe controller
endpoint with a try-catch: catch any thrown Error and rethrow a Nest
HttpException with HttpStatus.INTERNAL_SERVER_ERROR (no stack) so the response
is consistently formatted; reference the verifyUnsubscribeToken and
getUnsubscribeSecret calls to locate where to add the try-catch. Also add a
bootstrap-time validation (e.g., in NotificationChannelModule.onModuleInit or
main.ts startup) that checks process.env.JWT_SECRET ||
process.env.JWT_ACCESS_TOKEN_SECRET is present and throws a clear error at
startup if missing so misconfiguration fails fast.
In `@backend/apps/cloud/src/notification-channel/notification-channel.module.ts`:
- Around line 25-58: The Telegram integration currently causes silent failures:
update TelegramChannelService.send() to log an explicit warning/error when
telegramService is null (include channel id/type and intended recipient info) so
operators see delivery was skipped, and add validation in normaliseConfig() to
reject or mark TELEGRAM channel configs when ENABLE_TELEGRAM_INTEGRATION !==
'true' (throw or return a validation error) to prevent creating TELEGRAM
channels while the TelegramModule/telegramService is disabled; reference
TelegramChannelService.send(), telegramService, and normaliseConfig() when
making these changes.
In `@backend/apps/cloud/src/notification-channel/notification-channel.service.ts`:
- Around line 458-500: The update method currently re-normalises and always
resets verification for EMAIL and WEBHOOK when dto.config is present; change
this so after computing config = normaliseConfig(channel.type, config) you
compare the new normalized config against the existing channel.config
(deep/byte-equal comparison) and only assign channel.config, reset
channel.isVerified, set verificationToken, and clear disabledReason when the
normalized config actually differs; use the existing function names update,
normaliseConfig, and fields channel.config, channel.isVerified,
channel.verificationToken to locate the logic and short-circuit the no-op config
save to avoid spurious re-verification emails.
---
Outside diff comments:
In `@backend/apps/cloud/src/integrations/telegram/telegram.service.ts`:
- Around line 84-128: The confirmLinkAccount and confirmUnlinkAccount flows
perform user and notification-channel writes separately, causing possible
inconsistent state on partial failure; wrap both operations in a single TypeORM
transaction (or move both writes into NotificationChannelService methods and
have that service start the transaction) so that confirmLinkAccount executes
userService.updateUserTelegramId(..., true) and
getNotificationChannelService().upsertTelegramChannel(...) inside one
transaction, and confirmUnlinkAccount executes
getNotificationChannelService().deleteTelegramChannelsByChatId(...) and
userService.updateUserTelegramId(..., null) inside one transaction, ensuring
either both commits or a rollback on error.
In `@backend/apps/cloud/src/task-manager/task-manager.service.ts`:
- Around line 1827-1926: The cron alert handler checkMetricAlerts currently
updates lastTriggered only after fetching error details and building the
context, which allows a race where non-ERRORS alerts can retrigger if the detail
query is slow; move the update call to invoke alertService.update(alert.id, {
lastTriggered: new Date() }) immediately after the code path where conditionMet
is true (the same place checkOnlineUsersAlerts updates lastTriggered) and before
making the ClickHouse detailQuery / building context and rendering/dispatching;
ensure you keep ERRORS and alert.alertOnEveryCustomEvent behavior unchanged
(they bypass cooldown) while performing the early update for the other metric
paths.
---
Nitpick comments:
In `@backend/apps/cloud/src/integrations/telegram/telegram.service.ts`:
- Around line 131-136: The current getNotificationChannelService() throws a
runtime "NotificationChannelService is not available" error when the
`@Optional-injected` NotificationChannelService is absent; decide whether this
dependency is always provided by keeping NotificationChannelModule (forwardRef
in telegram.module.ts) and removing `@Optional` so DI fails at boot, or handle
missing service gracefully by keeping `@Optional` and updating confirmLinkAccount
and confirmUnlinkAccount to check for notificationChannelService via
getNotificationChannelService() guard (or a safe isAvailable() helper) and reply
to the user with a clear message instead of throwing. Locate
getNotificationChannelService(), confirmLinkAccount, confirmUnlinkAccount, and
the `@Optional` injection in telegram.service.ts and apply the chosen fix
consistently.
In `@backend/apps/cloud/src/notification-channel/notification-channel.service.ts`:
- Around line 346-391: The current locking in lockScopeParent invoked by
saveWithinScopeLimit correctly prevents the TOCTOU by acquiring a
pessimistic_write on the parent row; keep the logic but add a short developer
note and operational mitigations: add an inline comment in
lockScopeParent/saveWithinScopeLimit describing the serialization tradeoff and
potential for lock-wait timeouts, and implement a small operational change —
expose a configurable lock-wait/transaction timeout (read from config) and emit
or log a metric/event when acquiring the lock or when a lock wait occurs so
operators can monitor contention; do not change the existing lock semantics that
prevent the race, only add the comment, config-driven timeout handling, and
metric/log emission around the lock acquisition to make the behavioral tradeoff
observable.
In `@backend/apps/cloud/src/task-manager/task-manager.service.ts`:
- Around line 345-377: The hard-coded TELEGRAM_MARKDOWN_URL_KEYS set can drift
from AlertContext fields; export a single source-of-truth URL_KEYS (or
equivalent typed constant) from alert-context.ts that lists the URL-bearing
keys, then import and use that constant inside renderAlertMessage instead of the
local TELEGRAM_MARKDOWN_URL_KEYS; update the code that builds telegramContext to
consult this shared URL_KEYS and ensure its type aligns with AlertContext so
future additions (e.g., subscribers_url, goal_url) remain in sync.
In `@web/app/components/NotificationChannels/NotificationChannels.tsx`:
- Around line 460-474: The edit pencil should be hidden for channels of type
'webpush' to avoid opening an almost-empty rename-only modal: update the
conditional around the Tooltip/PencilSimpleIcon render in the
NotificationChannels component so it checks channel.type !== 'webpush' before
rendering the Tooltip with onEdit(channel); reference the existing Tooltip,
PencilSimpleIcon and onEdit(channel) symbols and gate that rendering path
accordingly (alternatively, you can branch inside the click handler to open a
different modal/title for webpush, but prefer hiding the pencil by checking
channel.type).
- Around line 284-295: The onEdit handler currently casts channel.config to a
loose Record<string, any> which loses the discriminated types and allows typos;
instead narrow the config by the channel.type before reading fields: inspect
channel.type (or switch on it) and cast channel.config to the appropriate shaped
interface (e.g., EmailConfig, ChatConfig, WebhookConfig) so you read
cfg.address, cfg.chatId, cfg.url with proper types, then call setEditing and
setForm as before; update onEdit to reference channel.config, channel.type,
onEdit, setForm, and setEditing when applying the narrowed config.
- Around line 147-163: The triggerList callback currently omits listFetcher from
its dependency array and the consuming useEffect disables linting; update the
useCallback for triggerList to include listFetcher in its dependency list so its
identity is stable with respect to the fetcher, then change the useEffect to
depend on triggerList (instead of repeating scope/organisationId/projectId) and
remove both eslint-disable-next-line react-hooks/exhaustive-deps comments; this
ensures triggerList, listFetcher and the useEffect stay in sync and prevents
stale-closure issues.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a94150e4-775e-46a1-b2a9-b6350dfcbef6
📒 Files selected for processing (9)
backend/apps/cloud/src/integrations/telegram/telegram.module.tsbackend/apps/cloud/src/integrations/telegram/telegram.service.tsbackend/apps/cloud/src/notification-channel/dispatchers/email-channel.service.tsbackend/apps/cloud/src/notification-channel/dispatchers/webpush-channel.service.tsbackend/apps/cloud/src/notification-channel/notification-channel.module.tsbackend/apps/cloud/src/notification-channel/notification-channel.service.tsbackend/apps/cloud/src/task-manager/task-manager.service.tsweb/app/components/NotificationChannels/NotificationChannels.tsxweb/public/sw.js
🚧 Files skipped from review as they are similar to previous changes (1)
- web/public/sw.js
Comment on lines
+25
to
+58
| const telegramImports = | ||
| process.env.ENABLE_TELEGRAM_INTEGRATION === 'true' | ||
| ? [forwardRef(() => TelegramModule)] | ||
| : [] | ||
|
|
||
| @Module({ | ||
| imports: [ | ||
| TypeOrmModule.forFeature([NotificationChannel]), | ||
| forwardRef(() => ProjectModule), | ||
| OrganisationModule, | ||
| MailerModule, | ||
| AppLoggerModule, | ||
| DiscordModule, | ||
| SlackModule, | ||
| ...telegramImports, | ||
| ], | ||
| providers: [ | ||
| NotificationChannelService, | ||
| TemplateRendererService, | ||
| ChannelDispatcherService, | ||
| EmailChannelService, | ||
| TelegramChannelService, | ||
| DiscordChannelService, | ||
| SlackChannelService, | ||
| WebhookChannelService, | ||
| WebpushChannelService, | ||
| ], | ||
| controllers: [NotificationChannelController], | ||
| exports: [ | ||
| NotificationChannelService, | ||
| TemplateRendererService, | ||
| ChannelDispatcherService, | ||
| ], | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect TelegramChannelService.send() for the unconfigured-bot path.
fd -e ts telegram-channel.service
rg -nP --type=ts -B2 -A30 'class TelegramChannelService\b'Repository: Swetrix/swetrix
Length of output: 3812
🏁 Script executed:
# Check if ChannelDispatcherService logs dispatcher invocations
rg -nP --type=ts -B3 -A15 'class ChannelDispatcherService\b'Repository: Swetrix/swetrix
Length of output: 2503
🏁 Script executed:
# Search for normaliseConfig to see if it validates TELEGRAM channels
fd -e ts -e js normalise-config normaliser config || rg -nP --type=ts 'normaliseConfig|normalise.*onfig' --max-count=20Repository: Swetrix/swetrix
Length of output: 617
🏁 Script executed:
# Check how TelegramChannelService is provided/injected
rg -nP --type=ts 'TelegramService|TelegramModule' backend/apps/cloud/src/notification-channel/ -B2 -A2Repository: Swetrix/swetrix
Length of output: 2349
🏁 Script executed:
# Get the dispatch method from ChannelDispatcherService
rg -nP --type=ts -A20 'dispatch\s*\(' backend/apps/cloud/src/notification-channel/dispatchers/channel-dispatcher.service.tsRepository: Swetrix/swetrix
Length of output: 857
🏁 Script executed:
# Get the full normaliseConfig implementation
sed -n '393,450p' backend/apps/cloud/src/notification-channel/notification-channel.service.tsRepository: Swetrix/swetrix
Length of output: 2157
Address silent delivery failures for TELEGRAM channels when integration is disabled.
When ENABLE_TELEGRAM_INTEGRATION !== 'true', TelegramChannelService.send() silently returns on line 24 without logging. Combined with the fact that normaliseConfig() does not prevent creation of TELEGRAM channels when the integration is disabled, this creates a silent delivery failure: alerts attached to TELEGRAM notification channels will fail to dispatch with zero operator visibility.
Either log the silent return in TelegramChannelService.send() (when telegramService is null), or validate in normaliseConfig() that the TELEGRAM integration is enabled before allowing a TELEGRAM channel to be created—preferably both.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/apps/cloud/src/notification-channel/notification-channel.module.ts`
around lines 25 - 58, The Telegram integration currently causes silent failures:
update TelegramChannelService.send() to log an explicit warning/error when
telegramService is null (include channel id/type and intended recipient info) so
operators see delivery was skipped, and add validation in normaliseConfig() to
reject or mark TELEGRAM channel configs when ENABLE_TELEGRAM_INTEGRATION !==
'true' (throw or return a validation error) to prevent creating TELEGRAM
channels while the TelegramModule/telegramService is disabled; reference
TelegramChannelService.send(), telegramService, and normaliseConfig() when
making these changes.
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
2 participants
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.
Changes
If applicable, please describe what changes were made in this pull request.
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
New Features
Documentation