Skip to content

Conversation

@wrn14897
Copy link
Member

@wrn14897 wrn14897 commented Nov 10, 2025

Currently, the resolved alert will have the same title and body message as the alerting one, which is misleading

Ref: HDX-2786

Slack

ALERT

Screenshot 2025-11-09 at 10 02 52 PM

RESOLVED

Screenshot 2025-11-09 at 10 26 01 PM

incident.io

ALERT

Screenshot 2025-11-09 at 11 07 30 PM

RESOLVED

Screenshot 2025-11-09 at 11 08 56 PM

@changeset-bot
Copy link

changeset-bot bot commented Nov 10, 2025

🦋 Changeset detected

Latest commit: 3e6ae6b

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

This PR includes changesets to release 2 packages
Name Type
@hyperdx/api Minor
@hyperdx/app 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
Copy link

vercel bot commented Nov 10, 2025

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

Project Deployment Preview Comments Updated (UTC)
hyperdx-v2-oss-app Ready Ready Preview Comment Nov 10, 2025 6:28pm

@claude
Copy link

claude bot commented Nov 10, 2025

PR Review: Alert Template State Awareness

Overall assessment: Implementation is solid with good test coverage.

Critical Issues

None found.

Code Quality Observations

✅ Strengths:

  • Good separation of concerns with isAlertResolved() helper function (template.ts:86)
  • Comprehensive test coverage including edge cases for all alert states
  • Consistent emoji prefix logic (🚨 for alerts, ✅ for resolved)
  • Properly handles resolved alerts by skipping expensive ClickHouse queries (template.ts:543-545)

⚠️ Minor Observations:

1. Template body formatting inconsistency

  • Line 544: Resolved alert message has inconsistent formatting vs alert messages (lines 603, 617)
  • Resolved: Group: "xxx" - The alert... (note the dash)
  • Alert: Group: "xxx"\n... (newline, no dash)
  • Suggestion: Consider aligning format for consistency, though current approach is acceptable

2. Missing TILE source handling for resolved alerts

  • Resolved alerts only show simplified message regardless of source type (SAVED_SEARCH vs TILE)
  • This is likely intentional but worth confirming it matches product requirements
  • Test at line 663 only covers SAVED_SEARCH source

Test Coverage

✅ Excellent test coverage:

  • State-aware title generation (lines 233-280)
  • isAlertResolved() helper (lines 283-294)
  • Resolved alert rendering with simplified message (lines 663-718)
  • All existing tests updated to reflect emoji prefixes

Summary

No blocking issues. The implementation is production-ready. Minor formatting inconsistency noted but not critical.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 10, 2025

E2E Test Results

All tests passed • 39 passed • 3 skipped • 288s

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

View full report →

Copy link
Contributor

@pulpdrew pulpdrew left a comment

Choose a reason for hiding this comment

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

LGTM

@kodiakhq kodiakhq bot merged commit 840d730 into main Nov 10, 2025
9 checks passed
@kodiakhq kodiakhq bot deleted the warren/refine-resolved-alert-body-message branch November 10, 2025 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants