Skip to content

refactor(alerts): extract notification transport into notifications.ts - #2844

Open
jordan-simonovski wants to merge 1 commit into
mainfrom
jordansimonovski/alerts-notifications-module
Open

refactor(alerts): extract notification transport into notifications.ts#2844
jordan-simonovski wants to merge 1 commit into
mainfrom
jordansimonovski/alerts-notifications-module

Conversation

@jordan-simonovski

@jordan-simonovski jordan-simonovski commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Moves the webhook transport out of template.ts into a new tasks/checkAlerts/notifications.ts. Nothing changes at runtime; this is the groundwork for multi-channel alert dispatch, which needs the transport in a module of its own.

What changed

template.ts was doing two jobs: building the Handlebars message and performing the HTTP send. The send half — notifyChannel, handleSendSlackWebhook, handleSendGenericWebhook, sendGenericWebhook, and the delivery metrics — now lives in notifications.ts. createHandlebarsWithHelpers moved with it and is re-exported, since template.ts still uses it.

Callers were repointed: the alerts task, the webhooks router, and the spy sites in the webhook integration test.

Impact

None. The function bodies moved unchanged, and the handleSendGenericWebhook re-export from checkAlerts/index.ts is preserved because tests spy through it.

No changeset: no behaviour change and nothing user-facing.

Implementation detail

The move was verified mechanically rather than by eye — the extracted bodies were diffed against their originals on main and are identical apart from added export keywords.

notifications.ts imports nothing from template.ts or checkAlerts/index.ts, so the pre-existing template <-> index cycle is unchanged and no new one is introduced.

Verification: tsc --noEmit clean, 268 alert integration tests and 64 webhook integration tests pass with no expectation changes beyond import and spy paths.

template.ts mixed Handlebars templating with the HTTP transport for
Slack/generic/incident.io webhooks. Move the transport (notifyChannel,
handleSendSlackWebhook, handleSendGenericWebhook, sendGenericWebhook,
delivery metrics) into tasks/checkAlerts/notifications.ts unchanged, so
the upcoming multi-channel dispatch work lands in a focused module.
@changeset-bot

changeset-bot Bot commented Aug 9, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 1879a85

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel

vercel Bot commented Aug 9, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview Aug 9, 2026 10:57am
hyperdx-storybook Ready Ready Preview Aug 9, 2026 10:57am

Request Review

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

🔴 Tier 4 — Critical

Touches authentication, tenancy data models, the public API or shipped database config — or substantially changes background tasks, the OTel pipeline, image build, or release CI.

Why this tier:

  • Background tasks or delivery pipeline substantially modified — 534 lines (bar: 30):
    • packages/api/src/tasks/checkAlerts/index.ts
    • packages/api/src/tasks/checkAlerts/notifications.ts
    • packages/api/src/tasks/checkAlerts/template.ts

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats
  • Production files changed: 4
  • Production lines changed: 536 (+ 6 in test files, excluded from tier calculation)
  • Critical-path lines changed: 534
  • Branch: jordansimonovski/alerts-notifications-module
  • Author: jordan-simonovski

To override this classification, remove the review/tier-4 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@greptile-apps

greptile-apps Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR extracts alert webhook delivery from the template module into a dedicated notifications module while preserving the existing behavior and compatibility re-export.

  • Moves notification dispatch, webhook delivery, shared message types, Handlebars helper creation, and delivery metrics into notifications.ts.
  • Repoints the alert task, webhooks router, and webhook integration-test spies to the extracted module.
  • Keeps handleSendGenericWebhook exported through checkAlerts/index.ts for existing test compatibility.

Confidence Score: 5/5

The PR appears safe to merge because the extraction preserves the notification call paths, compatibility export, validation, retry, and delivery behavior.

The moved webhook implementation remains unchanged, all identified consumers resolve the handlers through valid paths, and the new module does not introduce a concrete initialization or runtime failure.

Important Files Changed

Filename Overview
packages/api/src/tasks/checkAlerts/notifications.ts Introduces the dedicated notification transport module using the webhook-delivery implementation extracted from template.ts.
packages/api/src/tasks/checkAlerts/template.ts Removes transport responsibilities and imports the extracted message, helper, and dispatch APIs without changing template behavior.
packages/api/src/tasks/checkAlerts/index.ts Imports the generic webhook handler from its new module while preserving the existing compatibility export.
packages/api/src/routers/api/webhooks.ts Repoints webhook test-delivery handlers to the new notifications module.
packages/api/src/routers/api/tests/webhooks.int.test.ts Repoints webhook-handler spies to the module that now owns those functions.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  AlertTask[Alert evaluation task] --> Template[template.ts: render alert message]
  Template --> Notifications[notifications.ts: notifyChannel]
  Notifications --> Slack[Slack webhook transport]
  Notifications --> Generic[Generic / Incident.io transport]
  WebhooksRouter[Webhooks API router] --> Notifications
Loading

Reviews (1): Last reviewed commit: "refactor(alerts): extract notification t..." | Re-trigger Greptile

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Deep Review

This PR moves the webhook notification transport out of packages/api/src/tasks/checkAlerts/template.ts into a new packages/api/src/tasks/checkAlerts/notifications.ts, repointing the alerts task, the webhooks router, and the webhook integration-test spies. Verification confirms the extracted function bodies are byte-identical to the originals apart from added export keywords, all consumers were repointed, the index.ts re-export of handleSendGenericWebhook is preserved, and no new import cycle is introduced.

✅ No critical issues found.

No P2 issues found. The delivery metrics, retry/redirect-SSRF handling, validateWebhookUrl guards, and JSON-escaping were all confirmed to move intact; the router HTTP contract is unchanged; and no stale @/tasks/checkAlerts/template imports of the moved symbols remain.

🔵 P3 nitpicks (1)
  • packages/api/src/tasks/checkAlerts/notifications.ts:52createHandlebarsWithHelpers is a template-rendering concern that now lives in the notifications module and is imported back into template.ts, a mild module-boundary inversion introduced by the split.
    • Fix: Consider leaving createHandlebarsWithHelpers in template.ts (or a shared helper module) and importing it into notifications.ts instead, so template concerns do not flow out of the transport module.

Reviewers (2 of 8 completed at synthesis time): project-standards, reliability. Findings above also reflect orchestrator-level verification of the diff (stale-import grep, re-export preservation, spy-path repointing, byte-identical body diff). The correctness, testing, maintainability, kieran-typescript, security, and api-contract reviewers were dispatched but had not returned when output was required.

Testing gaps: none — webhooks.int.test.ts spies were repointed to notifications, and checkAlerts.int.test.ts continues to spy through the preserved index.ts re-export, so interception coverage is unchanged.

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 276 passed • 1 skipped • 889s

Status Count
✅ Passed 276
❌ Failed 0
⚠️ Flaky 1
⏭️ Skipped 1

Tests ran across 4 shards in parallel.

View full report →

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-4 Critical — deep review + domain expert sign-off

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant