Skip to content

feat(app): edit multiple notification channels on alerts - #2848

Open
jordan-simonovski wants to merge 4 commits into
jordansimonovski/alerts-multi-channel-dispatchfrom
jordansimonovski/alerts-multi-channel-ui
Open

feat(app): edit multiple notification channels on alerts#2848
jordan-simonovski wants to merge 4 commits into
jordansimonovski/alerts-multi-channel-dispatchfrom
jordansimonovski/alerts-multi-channel-ui

Conversation

@jordan-simonovski

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

Copy link
Copy Markdown
Contributor

Lets the alert forms edit the full list of notification channels. Until now the API supported several channels but both forms still edited a single one, so saving from the UI would silently drop the rest.

CleanShot.2026-08-09.at.21.01.38.mp4

What changed

Saved search and dashboard tile alerts both edit the plural channels list through the shared AlertChannelForm. Channels can be added and removed inline up to the API's limit of 10, and each row shows the webhook's service icon so the destination is visible without opening webhook settings.

A webhook already used by the alert is disabled in the other pickers, since duplicates are rejected server-side.

Alerts saved before multi-channel support are normalised on load, and the legacy channel field is cleared on submit so a stale value can never conflict with an edited list.

Key decisions

The last channel row is not removable. An alert with no target would fire into the void, and the API rejects it anyway, so the remove control only appears once there are two or more rows.

A webhook created from the picker fills the first empty row. Otherwise the user would create a webhook and then have to find and select it manually.

AlertChannelForm takes the field path rather than a string prefix. Callers pass channels or alert.channels directly, which types correctly and removes two generic-boundary casts.

Impact

Closes the UI overwrite hazard documented as a known limitation until now: editing a multi-channel alert in the app now preserves its channels.

Implementation detail

The picker is a useFieldArray with a useWatch on the array — fields holds last-render values, so the live ones are needed for the duplicate check.

Component tests cover adding and removing rows, the last row staying non-removable, duplicate disabling across rows, and the cap disabling the add button. toAlertChannels has unit coverage for both document vintages.

Verification: 2745 app unit tests.

@changeset-bot

changeset-bot Bot commented Aug 9, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: c5d1dd7

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 Minor
@hyperdx/api Minor
@hyperdx/otel-collector Minor

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:48am
hyperdx-storybook Ready Ready Preview Aug 10, 2026 1:48am

Request Review

@greptile-apps

greptile-apps Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The alert forms now preserve and edit the complete notification-channel list for saved searches and dashboard tiles.

  • Normalizes legacy singular-channel alerts into the plural channel representation.
  • Adds inline channel creation, removal, duplicate prevention, and the ten-channel limit.
  • Clears the legacy singular field before submission to avoid conflicts with the edited list.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/app/src/components/Alerts.tsx Reworks the shared alert-channel editor around a watched field array with duplicate prevention, channel limits, and webhook service icons.
packages/app/src/DBSearchPageAlertModal.tsx Migrates saved-search alert validation, defaults, editing, and submission from the singular channel field to channels.
packages/app/src/components/ChartEditor/utils.ts Normalizes legacy tile-alert channel data when converting persisted chart configurations into form state.
packages/app/src/components/DBEditTimeChartForm/TileAlertEditor.tsx Updates the tile alert editor to bind its controls to the plural channel array.
packages/app/src/utils/alerts.ts Adds shared legacy-channel normalization and changes new tile alerts to use the plural channel representation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Legacy[Legacy singular channel] --> Normalize[toAlertChannels]
  Existing[Existing channels list] --> Normalize
  Normalize --> Form[AlertChannelForm field array]
  Form --> Edit[Add, remove, or select webhooks]
  Edit --> Submit[Submit plural channels]
  Submit --> Clear[Omit legacy channel field]
Loading

Reviews (3): Last reviewed commit: "test(app): cover the legacy channel-clea..." | Re-trigger Greptile

Comment thread packages/app/src/components/Alerts.tsx
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found.

The channel/channels round-trip is sound: the saved-search form clears the legacy channel on submit and the tile path clears it on load, the server treats channel: undefined as absent (so no stale-value conflict), duplicate webhooks are disabled in the pickers, and the non-removable last row keeps at least one target. No P0/P1 production-breaking or regression issues were introduced.

🟡 P2 — recommended

  • packages/app/src/__tests__/DBSearchPageAlertModal.test.tsx:172 — the only submit-path test covers a legacy single-channel alert being dropped to one channels entry; there is no integration test that adds a second channel through the modal and asserts both are submitted, which is the core new behavior.
    • Fix: add a saved-search modal test that adds a channel row, selects a second webhook, submits, and asserts the payload channels contains both entries with no channel field.
    • testing
  • packages/app/src/components/DBEditTimeChartForm/TileAlertEditor.tsx:250 — the tile alert submit path that must drop the legacy channel and send the edited channels list is only covered at the convertSavedChartConfigToFormState unit level, not through the tile editor submit, leaving the form-to-payload path for tile alerts unverified.
    • Fix: add a test exercising the tile alert editor submit and asserting the resulting config carries channels without a stale channel.
    • testing, api-contract
🔵 P3 nitpicks (2)
  • packages/app/src/components/ChartEditor/utils.ts:315 — the legacy channel field is cleared at load for tile alerts but at submit for saved-search alerts (DBSearchPageAlertModal.tsx), so the same invariant is enforced in two different lifecycle spots, which is easy to break when one form changes.
    • Fix: centralize the clear-legacy-channel step in one shared helper invoked consistently by both forms.
  • packages/app/src/components/Alerts.tsx:190 — in handleWebhookCreated, when all rows are filled and the channel cap is reached, a newly created webhook is silently neither placed nor appended, so the user creates a webhook that does not appear.
    • Fix: surface a message (or focus the cap notice) when a created webhook cannot be auto-placed because the channel limit is reached.

Reviewers (6): correctness, testing, maintainability, kieran-typescript, julik-frontend-races, api-contract.

Testing gaps:

  • No multi-channel submit coverage through either alert form (only isolated AlertChannelForm UI and legacy-single-channel submit are tested).
  • The alerts list (AlertsPage.tsx:195, outside this diff) still renders only alert.channel, so a multi-channel alert created via this new UI displays a single destination — worth a follow-up now that multi-channel is user-editable.

@jordan-simonovski
jordan-simonovski force-pushed the jordansimonovski/alerts-multi-channel-ui branch from d09ad2b to 06858e2 Compare August 10, 2026 01:32
@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:

  • Diff size: 299 production lines changed (Tier 2 max: < 250)

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

Stats
  • Production files changed: 5
  • Production lines changed: 299 (+ 216 in test files, excluded from tier calculation)
  • Branch: jordansimonovski/alerts-multi-channel-ui
  • 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

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 275 passed • 1 skipped • 915s

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

Tests ran across 4 shards in parallel.

View full report →

Saved search and dashboard tile alert forms now edit the plural channels
list instead of a single channel, so a multi-channel alert survives a
save from the UI. Channels can be added and removed inline up to the
API's limit of 10, and a webhook already used by the alert is disabled
in the other pickers because duplicates are rejected server-side.

Alerts saved before multi-channel support are normalised on load, and
the legacy channel field is cleared on submit so a stale value can never
conflict with an edited list.
Each option and the selected value carry the Slack / generic /
incident.io icon, so the destination is visible without opening the
webhook settings. AlertChannelForm now takes the channels field path
directly instead of a string prefix, which drops two generic-boundary
type assertions.
AlertChannelForm had no coverage: adding and removing rows, the last row
staying non-removable so an alert always has a target, a webhook already
chosen being disabled in the other pickers (duplicates are rejected
server-side), and the cap disabling the add button.
Both alert forms clear the legacy singular `channel` so a stale value
can't be submitted alongside an edited channels list — the API rejects
that combination. Neither path was tested.

Covers the tile form's load-time normalisation and the saved-search
form's submitted payload.
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