Skip to content

feat(alerts): model and schema groundwork for multiple notification channels - #2845

Open
jordan-simonovski wants to merge 4 commits into
jordansimonovski/alerts-notifications-modulefrom
jordansimonovski/alerts-multi-channel-model
Open

feat(alerts): model and schema groundwork for multiple notification channels#2845
jordan-simonovski wants to merge 4 commits into
jordansimonovski/alerts-notifications-modulefrom
jordansimonovski/alerts-multi-channel-model

Conversation

@jordan-simonovski

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

Copy link
Copy Markdown
Contributor

Adds the shared Zod schemas and the Mongoose field that let an alert hold several notification channels instead of one. Nothing reads the new field yet.

What changed

zAlertChannels (1 to MAX_ALERT_CHANNELS, currently 10) and a shared cross-field rule that both API layers chain into their alert input schemas. channel becomes optional on AlertBaseObjectSchema, with the new rule enforcing that an alert still has a target.

The Mongoose model gains a canonical channels array, and getAlertChannels resolves an alert's targets regardless of when the document was written.

Key decisions

channels is canonical, channel is kept and mirrored. Storing both means a task runner from before this change still notifies the first target during a rolling upgrade, and a downgrade doesn't strand alerts. No data migration: getAlertChannels handles both vintages, and default: undefined stops Mongoose materialising an empty array on old documents.

Both fields may be sent together when they agree. The rule started as a strict exactly-one-of, but responses carry both fields, so any read-modify-write client would echo both back and get a 400 — the API's own response would not have been a valid request body. Sending both is now accepted when channel matches channels[0], and a genuine disagreement is still rejected rather than resolved by a silent precedence rule.

Only the channel rule is applied to tile alerts. Tile alerts embedded in a saved chart config validate through SavedChartConfigSchema rather than the API's alertSchema, so the rule had to be attached there too. It deliberately does not pull in the schedule and threshold refinements, which have never run on that path — adding them could reject dashboards that parse today.

Impact

Single-channel payloads remain valid unchanged. channel going optional required one defensive fix in the app (alert.channel?.type), where the helper already handled a missing value.

Implementation detail

getAlertChannels prefers a non-empty channels, falls back to channel when its type is non-null, and otherwise returns empty.

The tile-alert rule is pinned by a test that fails without it: SavedChartConfigSchema rejects a tile alert carrying neither field, and accepts both the plural and legacy-singular forms.

Verification: 1693 common-utils unit tests, 669 API unit tests, 265 dashboard integration tests.

…hannels

zAlertChannels (1..10 entries), MAX_ALERT_CHANNELS, and a shared
cross-field rule (exactly one of channel/channels, no duplicates) that
both API layers will chain into their alert input schemas. channel
becomes optional on AlertBaseObjectSchema; single-channel payloads
remain valid unchanged. The Mongoose model gains a canonical channels
array and getAlertChannels resolves either document vintage.
Alert responses carry both the canonical channels array and the legacy
channel mirror, so a read-modify-write client echoes both back on PUT.
A strict exactly-one rule would 400 every GET-then-PUT caller. Accept
both when channel equals channels[0], and reject only a genuine
disagreement so no client has to guess which field won.
Tile alerts embedded in a saved chart config validate through
SavedChartConfigSchema, not the API's alertSchema, so making `channel`
optional let a tile alert be saved through the dashboards endpoint with
no notification target at all -- it would fire and notify nobody. Apply
the channel rule to those unions too.

Only the channel rule is applied, not the schedule/threshold refinements,
so existing saved dashboards that never passed those keep parsing.
@changeset-bot

changeset-bot Bot commented Aug 9, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: bffb953

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@hyperdx/common-utils Minor
@hyperdx/api Patch
@hyperdx/app Patch
@hyperdx/otel-collector Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another 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 10, 2026 1:35am
hyperdx-storybook Ready Ready Preview Aug 10, 2026 1:35am

Request Review

@greptile-apps

greptile-apps Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds shared multi-channel alert schemas and persists a canonical channels array while retaining a singular compatibility mirror.

  • Accepts one to ten unique notification channels and validates singular/plural field consistency.
  • Persists plural-only tile-alert targets through dashboard synchronization.
  • Preserves compatibility with older alert readers through channel = channels[0].
  • Adds schema, model, and dashboard integration coverage.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the previously reported plural-only persistence issue is addressed by normalizing the input and writing both the canonical channel list and singular compatibility mirror.

Important Files Changed

Filename Overview
packages/common-utils/src/types.ts Adds bounded plural-channel schemas and shared validation across API and embedded tile-alert contracts.
packages/api/src/controllers/alerts.ts Normalizes singular and plural inputs, validates every webhook, and persists both canonical channels and the compatibility mirror.
packages/api/src/models/alert.ts Adds plural-channel persistence and a compatibility resolver for old and new alert documents.
packages/api/src/routers/api/tests/dashboard.int.test.ts Verifies that plural-only tile alerts persist both the full channel list and legacy first-channel mirror.
packages/api/src/utils/zod.ts Applies the shared channel-selection rule to the internal alert input schema.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Input[Alert input: channel and/or channels] --> Validate[Shared cross-field validation]
  Validate --> Resolve[getAlertChannels]
  Resolve --> Persist[(Alert document)]
  Persist --> Canonical[channels: all targets]
  Persist --> Mirror[channel: first target]
  Mirror --> Legacy[Legacy task readers]
Loading

Reviews (2): Last reviewed commit: "fix(alerts): make the write path underst..." | Re-trigger Greptile

Comment thread packages/common-utils/src/types.ts
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Deep Review

No critical issues found. No P0/P1: the write path is internally consistent (makeAlert always co-writes channel + channels, preserving the rolling-upgrade mirror), webhook team-scoping is preserved under the countDocuments refactor, and the schema changes are additive. The most-cited concern — a GET-then-edit channel/channels mismatch producing a permanent 400 in the app — was investigated and does not reproduce: unregistered form fields like channels are dropped from the react-hook-form submit payload (the same reason id and source are re-added manually), so the app never echoes a stale channels back. The findings below are recommendations and nits.

🟡 P2 -- recommended

  • packages/api/src/tasks/checkAlerts/template.ts:240 -- The API now accepts, validates, and persists up to MAX_ALERT_CHANNELS channels, but the dispatch path reads only the singular alert.channel mirror, so any client using the new plural field has channels [1..N] silently never notified.
    • Fix: Reject channels.length > 1 at the API boundary until dispatch iterates getAlertChannels(alert), or wire the firing path to fan out over every channel.
    • data-migrations, reliability, api-contract, correctness, kieran-typescript, adversarial
  • packages/api/src/controllers/alerts.ts:114 -- The rewritten webhook-ownership check (per-id findOne → deduped countDocuments) has no direct test for its new branches: multiple channels, cross-team webhook rejection, partial match (found !== uniqueIds.length), and duplicate webhookIds.
    • Fix: Add integration tests covering multi-webhook success, cross-team and non-existent webhook rejection, and duplicate webhookIds.
    • testing, correctness, security, reliability, adversarial
  • .changeset/alert-multi-channel-schemas.md:2 -- The changeset bumps only @hyperdx/common-utils, omitting @hyperdx/api despite behavior changes there (new 400, channel made optional, channels accepted), which AGENTS.md:206 requires for that package.
    • Fix: Add an @hyperdx/api entry to the changeset.
    • project-standards
  • packages/api/src/routers/external-api/v2/alerts.ts:250 -- The external v2 OpenAPI JSDoc/openapi.json still marks channel required and omits channels, diverging from the widened alertSchema used as the request body at lines 545 and 640 (openapi.json contains no channels).
    • Fix: Regenerate the spec (yarn docgen / yarn lint:openapi) and update the JSDoc to make channel optional and document channels.
    • project-standards, api-contract
  • packages/api/src/routers/api/alerts.ts:51 -- formatAlertResponse and translateAlertDocumentToExternalAlert never populate channels even though AlertsPageItemSchema and the external schema now declare it, so persisted multi-channel data cannot round-trip on any read endpoint.
    • Fix: Serialize channels in both response builders, or drop the schema field until read support lands.
    • api-contract, data-migrations, agent-native
🔵 P3 nitpicks (5)
  • packages/app/src/components/DBEditTimeChartForm/utils.ts:63 -- The client zSavedChartConfig resolver omits validateAlertChannelSelection; with channel now optional it accepts a tile alert with no target that the server rejects with 400.
    • Fix: Add .superRefine(validateAlertChannelSelection) to the client resolver so validation matches the server.
  • packages/common-utils/src/types.ts:716 -- The alertChannelKey helper is defined but bypassed by an inline ${c.type}:${c.webhookId} duplicate in the same function, so the two key builders can drift.
    • Fix: Call alertChannelKey(c) in the duplicate-detection map.
  • packages/api/src/models/alert.ts:51 -- Channel-precedence logic is duplicated between getAlertChannels and validateAlertChannelSelection across packages with no compiler-enforced link, requiring lockstep updates.
    • Fix: Centralize the precedence rule in common-utils and have the model call it, or cross-reference the two in comments.
  • packages/common-utils/src/types.ts:685 -- validateAlertChannelSelection's parameter is typed looser (type?: unknown / bare string) than the zAlertChannel schema it validates, discarding literal-type safety and forcing the alert.channel! assertion.
    • Fix: Type the parameter from z.infer<typeof zAlertChannel> / zAlertChannels.
  • packages/api/src/mcp/tools/alerts/schemas.ts -- The MCP save_alert tool remains single-channel only, a forward-looking agent/human parity gap once multi-channel creation is exposed.
    • Fix: Add an optional channels input mirroring zAlertChannels and reuse the shared validation.

Reviewers (12): correctness, security, adversarial, testing, maintainability, api-contract, data-migrations, reliability, kieran-typescript, project-standards, agent-native, learnings-researcher.

Testing gaps:

  • Rewritten webhook validation (countDocuments) is untested for cross-team, partial-match, and duplicate-webhookId cases.
  • makeAlert channel/channels mirroring is untested on the update path (a single-channel update overwriting a previously multi-channel channels array truncates it silently).
  • No dispatch-path test asserts the current single-notify behavior for an alert stored with multiple channels.

Review found this PR was internally inconsistent: it made `channel`
optional and taught the tile schema to accept `channels`, but no writer
understood the new field. A channels-only tile alert saved through the
dashboards endpoint passed validation and then persisted with nothing to
notify, because makeAlert only ever wrote `channel`.

Move the write path in with the schema that enables it: alertSchema
accepts channel and/or channels, validateAlertInput checks every webhook
belongs to the team in one query, and makeAlert persists the canonical
channels array with `channel` mirrored to channels[0].

Tests drive the selection rule through a real AlertSchema parse rather
than only a hand-built RefinementCtx, and cover the dashboards path both
ways: a channels-only tile alert persists a resolvable target, and one
with no channel at all is rejected.
@github-actions github-actions Bot added the review/tier-3 Standard — full human review required label Aug 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

  • Cross-layer change: touches frontend (packages/app) + backend (packages/api) + shared utils (packages/common-utils)
  • Touches API routes or data models — hidden complexity risk

Additional context: touches background tasks or the delivery pipeline lightly (2 lines, under the 30-line bar for Tier 4)

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats
  • Production files changed: 6
  • Production lines changed: 197 (+ 227 in test files, excluded from tier calculation)
  • Branch: jordansimonovski/alerts-multi-channel-model
  • Author: jordan-simonovski

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

@github-actions

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 274 passed • 1 skipped • 875s

Status Count
✅ Passed 274
❌ Failed 0
⚠️ Flaky 3
⏭️ 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-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant