Repository navigation
fix(api): prevent Slack notification failures when alert message exceeds 3000 chars - #3473
Tyagiquamar wants to merge 1 commit into
Conversation
|
|
Someone is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
| export const getFallbackHdxLink = (hdxLink: string): string => { | ||
| try { | ||
| const url = new URL(hdxLink); | ||
| return `${url.origin}/alerts`; |
There was a problem hiding this comment.
🟠 major — Fallback link uses url.origin, which drops the base path in sub-path deployments, so the link 404s
Build the fallback from config.FRONTEND_URL (for example ${config.FRONTEND_URL}/alerts, the same way mcp/tools/alerts/getAlert.ts:81 and providers/default.ts:523 do), or add a buildAlertsLink() method to the provider. Today all alert links are built from FRONTEND_URL, and the app supports a base path (NEXT_PUBLIC_HYPERDX_BASE_PATH; ClickStack uses /clickstack in packages/app/next.config.mjs). With FRONTEND_URL=https://host/clickstack, this code turns https://host/clickstack/chart?... into https://host/alerts.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
There was a problem hiding this comment.
This is a valid concern. @Tyagiquamar Could you address it?
| } | ||
|
|
||
| // If text exceeds limit, try falling back to the shorter /alerts link | ||
| const fallbackLink = getFallbackHdxLink(hdxLink); |
There was a problem hiding this comment.
🟠 major — The deep link is swapped for generic /alerts even when the body is what's too long
Only fall back when the link itself is the problem: if header(hdxLink).length + 3 fits within maxLength, keep hdxLink and truncate the body; use /alerts only when the header alone is too big. Example: a saved-search alert has a ~120-char /search/:id?from=..&to=.. link and a ~3100-char body (sample results can be up to 2503 chars plus the template text). The code swaps in /alerts and then truncates the body anyway, so the user loses the link to the exact search for no reason.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| const header = `*<${link} | ${title}>*\n`; | ||
| const availableBodyLen = maxLength - header.length - 3; | ||
| if (availableBodyLen > 0) { | ||
| return `${header}${body.slice(0, availableBodyLen)}...`; |
There was a problem hiding this comment.
🔵 minor — Body truncation cuts off the closing ``` of the saved-search results block
Saved-search bodies end with a -fenced `__hdx_query_results__` block (`template.ts:928-930`). Cutting the body at a fixed index drops the closing fence, so Slack shows the raw fence and the code formatting breaks. Fix: if the cut lands inside an open fence, close it (e.g. append '\n' and reserve room for it), or truncate inside the fence. Add a test for this case.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| const header = `*<${link} | ${title}>*\n`; | ||
| const availableBodyLen = maxLength - header.length - 3; | ||
| if (availableBodyLen > 0) { | ||
| return `${header}${body.slice(0, availableBodyLen)}...`; |
There was a problem hiding this comment.
🔵 minor — Hand-written slice-plus-ellipsis duplicates the existing truncateString helper
Use truncateString(body, availableBodyLen) from packages/api/src/utils/common.ts:38, which template.ts already uses for alert bodies. This follows the REQUIRED DRY/'Before you add a helper' convention.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| return `${header}${body.slice(0, availableBodyLen)}...`; | ||
| } | ||
|
|
||
| return `*<${link} | ${title}>*`.slice(0, maxLength); |
There was a problem hiding this comment.
🔵 minor — Last-resort branch cuts through the mrkdwn link syntax
Slicing *<link | title>* at maxLength cuts through the <...> link markup, so Slack gets broken mrkdwn. Truncate title instead (keep the *<link | ...>* wrapper intact), or drop the link and send a plain truncated title.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| const channel: WebhookChannel = { | ||
| type: 'webhook', | ||
| channel: { | ||
| _id: 'wh-1' as any, |
There was a problem hiding this comment.
🔵 minor — The webhook integration test only covers the short-message path, which already worked
Add a handleSendSlackWebhook case with a >3000-char hdxLink (like the real inline-alert /chart?config= link) and assert the posted block text is ≤3000 chars and uses the fallback link. Also add a case for a FRONTEND_URL that has a path prefix. Without these, removing the formatSlackSectionText call from the send path would still pass.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| type: 'webhook', | ||
| channel: { | ||
| _id: 'wh-1' as any, | ||
| name: 'Slack Alerts', |
There was a problem hiding this comment.
🔵 minor — The test uses as any casts, against the TypeScript conventions
Build the fixtures with real types (WebhookService.Slack, AlertState.ALERT, a typed webhook object), and use mockResolvedValue(undefined) or a typed response instead of {} as any. The repo conventions say to avoid any and as casts.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
PR Review
7 finding(s): 🔴 0 critical · 🟠 2 major · 🔵 5 minor 7 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
[Medium impact] The change reaches Slack alert notifications sent through webhook channels. The typed test fixtures and required changeset must be added before merging.
|
| const url = new URL(hdxLink); | ||
| return `${url.origin}/alerts`; |
There was a problem hiding this comment.
Fallback links lose deployment paths
getFallbackHdxLink keeps only the origin. With FRONTEND_URL=https://host.example/hyperdx, an oversized chart link becomes https://host.example/alerts instead of https://host.example/hyperdx/alerts. The app supports this prefix through NEXT_PUBLIC_HYPERDX_BASE_PATH, so the shortened link points outside the app. Build the fallback from the configured frontend URL and add a prefixed-URL test.
| return `${header}${body.slice(0, availableBodyLen)}...`; | ||
| } | ||
|
|
||
| return `*<${link} | ${title}>*`.slice(0, maxLength); |
There was a problem hiding this comment.
Long titles break link formatting
When the title makes the header exceed 3000 characters, this branch cuts through the Slack link text and removes its closing >*. Titles can come from a rendered alert.name template. The message fits the length limit but contains an unfinished link. Shorten the title before building the header, keep the link markers intact, and test this branch.
| channel: { | ||
| _id: 'wh-1' as any, | ||
| name: 'Slack Alerts', | ||
| service: 'slack' as any, | ||
| url: 'https://hooks.slack.com/services/T00/B00/X00', | ||
| } as any, |
There was a problem hiding this comment.
Channel fixture bypasses type checks
Casting the entire channel fixture to any stops TypeScript from checking whether it matches WebhookChannel. The repository guide requires proper types instead of any; this requirement must be satisfied before merging. Build a complete typed fixture instead. The same pattern appears in the mocked response and message state.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| text: formatSlackSectionText( | ||
| message.hdxLink, | ||
| message.title, | ||
| message.body, | ||
| ), |
There was a problem hiding this comment.
This changes notification behavior in @hyperdx/api, but the PR adds no changeset. AGENTS.md requires a changeset for behavior changes and explicitly includes @hyperdx/api. Add a patch changeset before merging so the fix is included in the release notes and version plan.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Deep ReviewScope: 1 commit ( Intent: Long chart-explorer links (serialized ✅ No critical issues found. The length invariant holds on every return path — 🟡 P2 -- recommended
🔵 P3 nitpicks (5)
Reviewers (3): ce-correctness-reviewer, ce-testing-reviewer, ce-project-standards-reviewer. (Prior PR comments were bot-only — changeset-bot and vercel-bot — with no substantive feedback to verify.) Testing gaps: No coverage for the non-URL-link truncation branch, the header-exceeds-limit slice branch, or an end-to-end over-limit payload through No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
…eds 3000 chars Fixes hyperdxio#3452 Slack section blocks enforce a strict 3000-character limit on the text field. Inline alert links encode large chartConfig objects in query params, causing the message block text to exceed 3000 chars and fail delivery with WEBHOOK_ERROR. Format Slack section text using formatSlackSectionText, which falls back to the shorter /alerts link if the link causes an overflow, and truncates the body safely if necessary. Signed-off-by: Mohd Quamar Tyagi <347892697+Tyagiquamar@users.noreply.github.com>
c18a0e9 to
edd160a
Compare
Fixes #3452
Summary of changes
text.textfield.buildAlertMessageTemplateHdxLinkgenerates a chart explorer link containing the entire serializedchartConfigin query parameters. On alerts with multiple series or complex queries, this URL exceeds 2500+ characters, causing*<${message.hdxLink} | ${message.title}>*\n${message.body}to surpass Slack's 3000-character limit and fail delivery withWEBHOOK_ERROR.formatSlackSectionTextandgetFallbackHdxLinkinpackages/api/src/tasks/checkAlerts/transports/slack.ts:${origin}/alertslink....) so total block text never exceeds 3000 characters.packages/api/src/tasks/checkAlerts/transports/__tests__/slack.test.ts.Signed-off-by: Mohd Quamar Tyagi 347892697+Tyagiquamar@users.noreply.github.com