Skip to content

fix(alerts): show every notification channel on the summary line - #2991

Open
jordan-simonovski wants to merge 1 commit into
mainfrom
claude/alert-properties-all-channels
Open

fix(alerts): show every notification channel on the summary line#2991
jordan-simonovski wants to merge 1 commit into
mainfrom
claude/alert-properties-all-channels

Conversation

@jordan-simonovski

Copy link
Copy Markdown
Contributor

Why

AlertPropertiesSummary reads alert.channel — the legacy single-value mirror of channels[0] — so an alert configured with several notification targets renders as though it has one, with nothing to indicate the others exist. Multi-channel alerts have been supported since #2845#2848; this surface never caught up.

Dispatch is correct, so this is a reporting gap rather than a delivery one. It does point the wrong way, though: someone checking which targets an alert notifies is shown one, and would reasonably conclude a channel hadn't saved.

Both surfaces that share the component are affected — the alerts page rows and the alert detail page.

What changed

Render channels when present, falling back to [alert.channel] otherwise, so rows written before multi-channel (and an alert with a null-typed channel) render exactly as they did.

A single channel still names its webhook — a count there would be a regression for the overwhelmingly common case. Several channels render an icon each plus a count, bounded by the ten the API allows.

Testing

Two tests on AlertDetailProperties: a two-channel alert reports 2 channels, and a single-channel alert still names its webhook. Verified the first fails on main; the second passes either way by design, as parity cover.

make ci-lint (0 errors) and make ci-unit (3115 app tests) in an upstream checkout.

Note on scope

I kept this to a count rather than listing every channel's name. Resolving N webhook names means the caller fetching and threading N lookups, and an alerts-page row has no room for ten names. Happy to go further if you'd rather the detail page enumerate them.

Found while merging this stack into the ClickHouse EE fork, where the same components had drifted out of step with the multi-channel model.

AlertPropertiesSummary read `alert.channel` — the legacy single-value mirror
of channels[0] — so an alert configured with several notification targets
rendered as though it had one, with nothing to say the others existed.

Dispatch was always correct, so this is a reporting gap rather than a delivery
one. It points the wrong way, though: someone checking which targets an alert
notifies is shown one, and would reasonably conclude a channel never saved.

A single channel still names its webhook, so the common case is unchanged.
Several channels render an icon each plus a count, which stays bounded at the
ten channels the API allows. Both surfaces that share the component — the
alerts page rows and the alert detail page — are fixed together.
@changeset-bot

changeset-bot Bot commented Aug 25, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 573149a

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

This PR includes changesets to release 3 packages
Name Type
@hyperdx/app Patch
@hyperdx/api 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 25, 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 25, 2026 1:00am
hyperdx-storybook Ready Ready Preview Aug 25, 2026 1:00am

Request Review

@github-actions github-actions Bot added the review/tier-2 Low risk — AI review + quick human skim label Aug 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔵 Tier 2 — Low Risk

Small, isolated change with no API route or data model modifications.

Why this tier:

  • Standard feature/fix — introduces new logic or modifies core functionality

Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns.
SLA: Resolve within 4 business hours.

Stats
  • Production files changed: 1
  • Production lines changed: 23 (+ 38 in test files, excluded from tier calculation)
  • Branch: claude/alert-properties-all-channels
  • Author: jordan-simonovski

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

@greptile-apps

greptile-apps Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR updates alert summaries to use the canonical channels collection while preserving legacy single-channel behavior.

  • Multi-channel alerts now display an icon for each target and the total channel count.
  • Single-channel and legacy alerts continue to display the resolved webhook name or generic fallback.
  • Component tests cover both multi-channel counting and single-channel naming.

Confidence Score: 5/5

The PR appears safe to merge, with the new multi-channel summary behavior preserving existing single-channel and legacy rendering.

The component consumes the canonical channel array when available, safely falls back to the required legacy channel object, and retains the resolved webhook name for single-channel alerts; no actionable failure remains.

Important Files Changed

Filename Overview
packages/app/src/components/alerts/AlertPropertiesSummary.tsx Uses canonical channel data for summary rendering while retaining a safe legacy fallback and existing single-channel labels.
packages/app/src/components/alerts/tests/AlertDetailProperties.test.tsx Adds focused regression coverage for multi-channel counts and preservation of single-channel webhook names.
.changeset/alert-properties-all-channels.md Accurately documents the user-visible alert-summary correction as an application patch.

Reviews (1): Last reviewed commit: "fix(alerts): show every notification cha..." | Re-trigger Greptile

@github-actions

Copy link
Copy Markdown
Contributor

Deep Review

Scope: 1 component + 1 test + 1 changeset (AlertPropertiesSummary renders all notification channels instead of only alert.channel).
Intent: Close a reporting gap where a multi-channel alert displayed as if it had one channel; render an icon per channel plus a count, falling back to the legacy channel mirror for older rows.

The change is small, correct on the happy path, and backward-compatible: alert.channel is a required object per the page-item schema, so the [alert.channel] fallback never dereferences null, and getWebhookChannelIcon already handles a null type. No correctness, security, or contract regressions were found.

🟡 P2 — recommended

  • packages/app/src/components/alerts/__tests__/AlertDetailProperties.test.tsx:63 — the two-channel test asserts only the derived 2 channels text, so a regression that dropped the channels.map and rendered a single icon would still pass.
    • Fix: Assert that one channel icon renders per entry (e.g. count the rendered icon nodes or add a stable test id to the icon wrapper) in the multi-channel case.
    • testing, maintainability, project-standards, kieran-typescript
🔵 P3 nitpicks (1)
  • packages/app/src/components/alerts/AlertPropertiesSummary.tsx:80 — the React key `${channel.type}-${channel.webhookId ?? index}` can collide (e.g. a channel with webhookId "1" at index 0 and a channel with no webhookId at index 1 both key to webhook-1), yielding a dev-console key warning; render impact is nil since same-type icons are identical.
    • Fix: Include the index unconditionally in the key, e.g. `${channel.type}-${channel.webhookId ?? 'none'}-${index}`.

Reviewers (7): correctness, testing, maintainability, project-standards, kieran-typescript, agent-native, learnings-researcher.

Testing gaps: The explicit empty-array case (channels: []) is not tested, though it is logically identical to the absent-channels fallback, which is already covered by the existing "renders all persisted metadata fields" test (line 134). The alerts-page-row path (no webhookName prop) exercises the same branch logic but has no dedicated test.

Note: prior PR comments were bot-only (changeset-bot, vercel), so the previous-comments reviewer was not run — there was no human feedback to verify.

@github-actions

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 306 passed • 1 skipped • 1214s

Status Count
✅ Passed 306
❌ Failed 0
⚠️ Flaky 2
⏭️ 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-2 Low risk — AI review + quick human skim

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant