Skip to content

[HDX-3044] Add optional note field to alerts#2210

Merged
kodiakhq[bot] merged 24 commits into
mainfrom
dhable/HDX-3044-add-alert-note-field
May 13, 2026
Merged

[HDX-3044] Add optional note field to alerts#2210
kodiakhq[bot] merged 24 commits into
mainfrom
dhable/HDX-3044-add-alert-note-field

Conversation

@dhable
Copy link
Copy Markdown
Contributor

@dhable dhable commented May 6, 2026

Summary

Adds an optional freeform note/reason field to alerts, allowing on-call responders to document why an alert exists, threshold decision history, and links to runbooks. The note field supports markdown formatting.

Additionally, this PR brings the saved-search alert UX in line with dashboards by adding visual indicators for firing alerts:

  • Alert tabs in the saved-search alert modal show a red bell firing indicator alongside the webhook channel icon, using the shared AlertStatusIcon component (same pattern as dashboard tiles and app nav)
  • The Alerts button on the search page shows a red bell icon when at least one alert in the saved search is firing

Changes across the codebase:

  • Backend: Added note field to Mongoose model, Zod validation schemas (shared alertNoteSchema in common-utils + API), controller mapping, internal API response formatting, external API type + transformer, and OpenAPI docs
  • Frontend (note field): Extracted shared AlertNoteField component with empty→null onChange transform. Notes are displayed on the /alerts page in a collapsible section (hidden by default) with markdown rendering gated behind the expanded state. Custom <a> and <img> overrides for link security and referrer privacy.
  • Frontend (alert status indicators): Alert tabs in the saved-search modal show AlertStatusIcon alongside the webhook channel icon. The Alerts button on the search page shows AlertStatusIcon when alerts are configured.
  • E2E tests: Two Playwright tests covering note creation from both saved-search and dashboard tile flows, with markdown rendering and link attribute verification
  • Unit/integration tests: External API transformer tests for note null/undefined/string handling. Internal API round-trip test for note create, update, and clear.

Screenshots or video

Screen.Recording.2026-05-06.at.2.09.59.PM.mov

How to test locally or on Vercel

  1. Create a saved search, open the alerts modal, fill in the note field with markdown (e.g. Threshold set to **1**. See [runbook](https://example.com).), and create the alert
  2. Navigate to /alerts — the alert row should show a "Note" toggle
  3. Click the toggle to expand — verify markdown renders correctly (bold, links open in new tab)
  4. Repeat from a dashboard tile alert editor
  5. Trigger an alert (or set state to ALERT in the DB) — verify the saved-search alert modal tab shows a red bell icon next to the webhook icon, and the Alerts button on the search page shows a red bell icon

References

@dhable dhable added the ai-generated AI-generated content; review carefully before merging. label May 6, 2026
@vercel
Copy link
Copy Markdown

vercel Bot commented May 6, 2026

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment May 13, 2026 7:42pm

Request Review

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 6, 2026

🦋 Changeset detected

Latest commit: a5df19a

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 Minor
@hyperdx/app 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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 6, 2026

E2E Test Results

All tests passed • 176 passed • 3 skipped • 1277s

Status Count
✅ Passed 176
❌ Failed 0
⚠️ Flaky 5
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

dhable added 4 commits May 6, 2026 14:06
Add a freeform note/reason field to alerts that supports markdown
formatting. This allows on-call responders to document alert reasoning,
threshold history, and links to runbooks.

- Add note field to Mongoose model, Zod schemas, and API responses
- Add note textarea to saved-search and tile alert editors
- Display notes on the alerts page in a collapsible section with
  markdown rendering
- Update OpenAPI docs for external API v2
- Add E2E tests for note creation and display
Display IconBellFilled in red on saved-search alert modal tabs when
the alert state is ALERT, matching the visual pattern used by
AlertStatusIcon on dashboard tiles.
…g alerts

Add IconBellFilled to the right of the Alerts button text on the
search page when at least one alert in the saved search is in the
ALERT state.
@dhable dhable force-pushed the dhable/HDX-3044-add-alert-note-field branch from 62946c0 to 1276b6b Compare May 6, 2026 19:06
…e target

- Omit note field from external API response when null/undefined,
  matching the conditional spread pattern used for scheduleOffsetMinutes
- Move data-testid from wrapper div to UnstyledButton so Playwright
  clicks the actual toggle handler
@dhable dhable marked this pull request as ready for review May 6, 2026 19:37
@github-actions github-actions Bot added the review/tier-4 Critical — deep review + domain expert sign-off label May 6, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 6, 2026

🔴 Tier 4 — Critical

Touches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD.

Why this tier:

  • Critical-path files (1):
    • packages/api/src/routers/external-api/v2/alerts.ts
  • Cross-layer change: touches frontend (packages/app) + backend (packages/api) + shared utils (packages/common-utils)

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: 14
  • Production lines changed: 162 (+ 273 in test files, excluded from tier calculation)
  • Branch: dhable/HDX-3044-add-alert-note-field
  • Author: dhable

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

Deep Review

Scope: PR #2210 — Adds optional note field to alerts plus saved-search alert firing indicators. 22 files changed (+449/-10) across backend (packages/api), shared types (packages/common-utils), frontend (packages/app), and e2e tests.

Reviewers (8): correctness, testing, maintainability, project-standards, agent-native, security, api-contract, kieran-typescript.

✅ No critical issues found. The change follows existing patterns (mirrors name/message lifecycle), routes the schema through the shared alertNoteSchema in common-utils, and lands solid coverage — internal round-trip (create/update/clear), external-transformer null/undefined/string, and two Playwright flows verifying rendered markdown plus link rel/target attributes. ReactMarkdown v10's default urlTransform already filters javascript: / data: URLs, and raw HTML is escaped because rehype-raw is not wired in — the custom a/img overrides correctly layer noopener noreferrer nofollow and referrer-policy: no-referrer on top.

🟡 P2 -- recommended

  • packages/api/src/utils/externalApi.ts:333 -- translateAlertDocumentToExternalAlert coerces note: alertObj.note ?? null but leaves the existing name/message fields raw on lines 331-332, so for pre-existing alerts the v2 API response is asymmetric (note: null, but name/message may be undefined and dropped from JSON). External clients reading the OpenAPI nullable: true contract will see inconsistent presence across the three text fields.
    • Fix: apply the same ?? null coercion to name and message, or drop the coercion on note and let the field be absent when undefined — pick one shape and apply it uniformly.
    • api-contract, maintainability
🔵 P3 nitpicks (4)
  • packages/common-utils/src/types.ts:554 -- alertNoteSchema = z.string().min(1).max(4096).nullish() accepts whitespace-only strings (e.g. " ") which pass min(1) but render as visually empty markdown and still trigger the collapsible "Note" section on /alerts.

    • Fix: add .trim() before .min(1) (or .refine(v => v == null || v.trim().length > 0)) so blank-but-non-empty notes are rejected at the schema boundary.
    • correctness, kieran-typescript
  • packages/app/src/components/AlertNoteField.tsx:24 -- the Textarea has no maxLength attribute, so users can type well past the server's 4096-char limit and only learn about the rejection after submit.

    • Fix: pass maxLength={4096} to the Textarea so the browser caps input at the same boundary the schema enforces.
    • kieran-typescript
  • packages/app/src/AlertsPage.tsx:101 -- {opened && <ReactMarkdown …>} is nested inside <Collapse expanded={opened}>, so the markdown subtree mounts/unmounts on every toggle and the collapse animation has no children to measure for its opening frame.

    • Fix: drop the opened && guard and let Collapse handle visibility, or hoist the markdown render outside and toggle only the wrapper — pick one and remove the redundant gate.
    • kieran-typescript
  • packages/app/src/components/AlertNoteField.tsx:6 -- labelMarginTop?: string = 'xs' is typed as a bare string while only Mantine size tokens ('xs' | 'sm' | …) actually work; callers can pass arbitrary strings without a type error.

    • Fix: narrow the prop type to Mantine's MantineSpacing (or a literal union of the accepted tokens) so misuse is caught at compile time.
    • kieran-typescript

Reviewers (8): correctness, testing, maintainability, project-standards, agent-native, security, api-contract, kieran-typescript.

Testing gaps:

  • No test that submitting a note longer than 4096 chars is rejected by the schema (boundary not exercised).
  • No test that whitespace-only notes are rejected (they currently pass min(1)).
  • No coverage for loading and saving a pre-PR alert (DB document missing the note key) through the edit modal without unintended field churn.

… tests

P1 fixes:
- Extract AlertNoteField component with onChange empty→null transform
- Seed note: null in form defaults to prevent uncontrolled→controlled
- Restore nullable: true + add minLength/maxLength to OpenAPI note spec

P2 fixes:
- Gate ReactMarkdown behind opened state to prevent image fetch on paint
- Add custom img component with referrerPolicy=no-referrer, loading=lazy
- Add max(4096) to AlertsPageItemSchema.note to mirror server cap
- Use alert._id as key in AlertCardList to preserve expand state on refetch

P3 fixes:
- Refactor makeAlert name/message/note to use ?? null

Testing:
- Add unit tests for note omit-when-null in external API transformer
- Add internal API round-trip test for note create, update, and clear
- Add toBeHidden assertion in tile E2E test before expanding note
- Strengthen link assertions to verify href, target, and rel attributes
- External API now returns note: null (matching name/message siblings)
  instead of omitting the field
- Extract alertNoteSchema in common-utils as single source of truth for
  note validation constraints (min(1), max(4096), nullish)
- Use shared schema in AlertBaseObjectSchema, AlertsPageItemSchema, and
  backend alertSchema
- Update unit and integration tests to expect note: null
@dhable
Copy link
Copy Markdown
Contributor Author

dhable commented May 7, 2026

Response to Deep Review P2 Items 7 and 8

Item 7 — Zod schema unit tests for note boundaries: Skipping. The alertNoteSchema uses standard Zod primitives (.min(1), .max(4096), .nullish()) whose behavior is well-tested by the Zod library itself. Adding boundary tests here would be testing framework internals rather than application logic. The schema is now defined once via the shared alertNoteSchema constant (extracted in this PR), so there is no drift risk between the three former callsites.

Item 8 — E2E test for edit/clear paths: Skipping. The create → update → clear round-trip is already covered by the integration test round-trips note through create, update, and clear in packages/api/src/routers/api/__tests__/alerts.test.ts. This test exercises the full controller + model + API response pipeline. An additional browser E2E test would re-verify the same server-side code paths with significantly more infrastructure cost and flake surface.

The AlertStatusIcon is shown alongside the webhook channel icon on
alert tabs, not replacing it. This matches the additive pattern used
on dashboard tiles and the app nav.
@hyperdxio hyperdxio deleted a comment from github-actions Bot May 7, 2026
@hyperdxio hyperdxio deleted a comment from github-actions Bot May 7, 2026
@teeohhem
Copy link
Copy Markdown
Contributor

teeohhem commented May 8, 2026

@dhable ignore the UI preview smoke failure for now. Working on it :)

@dhable
Copy link
Copy Markdown
Contributor Author

dhable commented May 8, 2026

Response to Deep Review P0/P1 — silent note wipe on update

makeAlert unconditional note: alert.note ?? null — Fixed. The note field now uses a conditional spread so omitting it from a PUT preserves the existing value:

...(alert.note !== undefined && { note: alert.note ?? null }),

The pre-existing name/message fields intentionally keep their unconditional pattern (name: alert.name ?? null) to avoid changing existing behavior — a comment in the code documents this decision. Changing those fields is out of scope for this PR.

Integration tests added for both the internal and external API confirming that a PUT without note preserves the stored value.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 8, 2026

PR Review

✅ No critical issues found.

A few small observations (not blocking):

  • ℹ️ alertNoteSchema = z.string().min(1).max(4096).nullish() accepts whitespace-only strings (" ") since .min(1) checks length, not content. The frontend e.target.value || null also passes whitespace through. → If you want notes to be "meaningful or null," consider .trim().min(1) in the schema and trimming in AlertNoteField's onChange. Otherwise, fine as-is.
  • ℹ️ Image rendering is allowed via markdown. referrerPolicy="no-referrer" mitigates leakage, but external images still fetch on render and could be used as tracking pixels for "alert note viewed" events. Acceptable given the gated-by-expand UX and documented runbook use case — worth being aware of.
  • ℹ️ AlertNoteField spreads {...field} before value / onChange — works correctly here, but consider destructuring field to avoid passing react-hook-form's ref/name props through Mantine Textarea unintentionally if internals change.

Nice touches:

  • Backend test covers the create → update → clear round-trip explicitly (the null-clear path is where these things usually break).
  • Shared alertNoteSchema in common-utils and reused by api/utils/zod.ts keeps validation consistent.
  • key={index}key={alert._id} is a correct fix piggybacked onto the change.
  • Link/image overrides correctly enforce target=_blank/rel=noopener noreferrer nofollow and referrerPolicy=no-referrer even though {...props} is spread first (because target/rel/referrerPolicy follow it).
  • react-markdown v10 doesn't render raw HTML by default and sanitizes URL schemes — XSS surface is appropriately constrained.

@dhable dhable requested review from a team and knudtty and removed request for a team May 8, 2026 18:24
Copy link
Copy Markdown
Contributor

@knudtty knudtty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking

* nullable: true
* minLength: 1
* maxLength: 4096
* example: "Threshold raised from 50 to 100 on 2026-01-15. See [runbook](https://wiki.example.com/runbook)."
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This link doesn't resolve to anything

@kodiakhq kodiakhq Bot merged commit d3a5a57 into main May 13, 2026
29 of 30 checks passed
@kodiakhq kodiakhq Bot deleted the dhable/HDX-3044-add-alert-note-field branch May 13, 2026 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-generated AI-generated content; review carefully before merging. automerge review/tier-4 Critical — deep review + domain expert sign-off

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants