Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/slack-block-limit.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@hyperdx/api": patch
---

fix(api): prevent Slack notification failures when alert message exceeds 3000 chars
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
import {
formatSlackSectionText,
getFallbackHdxLink,
handleSendSlackWebhook,
} from '@/tasks/checkAlerts/transports/slack';
import type { Message, WebhookChannel } from '@/tasks/checkAlerts/transports/types';
import * as slack from '@/utils/slack';

jest.mock('@/utils/slack');
jest.mock('@/utils/validators');

describe('Slack transport', () => {
describe('getFallbackHdxLink', () => {
it('returns origin + /alerts for valid URLs', () => {
expect(
getFallbackHdxLink('https://app.hyperdx.io/chart?config=%7B%22very%22%3A%22long%22%7D'),
).toBe('https://app.hyperdx.io/alerts');
});

it('returns original input if not a valid URL', () => {
expect(getFallbackHdxLink('not-a-url')).toBe('not-a-url');
});
});

describe('formatSlackSectionText', () => {
it('keeps standard link and body when under character limit', () => {
const link = 'https://app.hyperdx.io/alerts/123';
const title = 'High Error Rate';
const body = 'Error rate exceeded 5%';
expect(formatSlackSectionText(link, title, body)).toBe(
`*<${link} | ${title}>*\n${body}`,
);
});

it('falls back to /alerts when large query string exceeds limit', () => {
const longQuery = 'x'.repeat(3100);
const longLink = `https://app.hyperdx.io/chart?config=${longQuery}`;
const title = 'High Latency';
const body = 'p99 latency > 500ms';
const formatted = formatSlackSectionText(longLink, title, body, 3000);

expect(formatted).toBe(`*<https://app.hyperdx.io/alerts | ${title}>*\n${body}`);
expect(formatted.length).toBeLessThanOrEqual(3000);
});

it('truncates body when text still exceeds limit after fallback link', () => {
const longQuery = 'x'.repeat(2900);
const longLink = `https://app.hyperdx.io/chart?config=${longQuery}`;
const title = 'High Latency';
const longBody = 'b'.repeat(3500);
const formatted = formatSlackSectionText(longLink, title, longBody, 3000);

expect(formatted.startsWith(`*<https://app.hyperdx.io/alerts | ${title}>*\n`)).toBe(true);
expect(formatted.endsWith('...')).toBe(true);
expect(formatted.length).toBeLessThanOrEqual(3000);
});
});

describe('handleSendSlackWebhook', () => {
it('passes formatted section block text to postMessageToWebhook', async () => {
const mockPostMessage = jest.spyOn(slack, 'postMessageToWebhook').mockResolvedValue({} as any);

const channel: WebhookChannel = {
type: 'webhook',
channel: {
_id: 'wh-1' as any,

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.

🔵 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

name: 'Slack Alerts',

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.

🔵 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

service: 'slack' as any,
url: 'https://hooks.slack.com/services/T00/B00/X00',
} as any,
Comment on lines +65 to +70

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.

P2 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!

Fix in Claude Code Fix in Conductor Fix in Cursor Fix in Codex

};

const message: Message = {
hdxLink: 'https://app.hyperdx.io/chart?config=test',
title: 'CPU Alert',
body: 'CPU > 90%',
state: 'ALERT' as any,
startTime: 0,
endTime: 1000,
eventId: 'evt-1',
};

await handleSendSlackWebhook(channel, message);

expect(mockPostMessage).toHaveBeenCalledTimes(1);
expect(mockPostMessage).toHaveBeenCalledWith(
'https://hooks.slack.com/services/T00/B00/X00',
{
text: 'CPU Alert',
blocks: [
{

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.

🔵 minor — Webhook test never exercises the over-limit path the PR fixes

handleSendSlackWebhook is only tested with a short hdxLink, so it would still pass if formatSlackSectionText were removed from the payload. Add a case with a >3000-char chart link and assert the posted block text is the fallback/truncated value and is ≤3000 chars. Add a fallback test where the link has a path prefix (e.g. https://host/prefix/chart?...). While there, replace the as any casts with properly typed fixtures, per the repo conventions.

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: 'section',
text: {
type: 'mrkdwn',
text: '*<https://app.hyperdx.io/chart?config=test | CPU Alert>*\nCPU > 90%',
},
},
],
},
);
});
});
});
49 changes: 48 additions & 1 deletion packages/api/src/tasks/checkAlerts/transports/slack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,49 @@ import type {
import * as slack from '@/utils/slack';
import { validateWebhookUrl } from '@/utils/validators';

const MAX_SLACK_SECTION_LENGTH = 3000;

export const getFallbackHdxLink = (hdxLink: string): string => {

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.

🔵 minor — Transport guesses app routes by string-parsing hdxLink; the fallback belongs in the provider and should target the specific alert

The Slack transport has no knowledge of frontend routes or the alert id, so all it can do is link to the generic list. Have AlertProvider (providers/default.ts) build a short link alongside hdxLink, e.g. ${FRONTEND_URL}/alerts/${alert.id} (that detail route is used in packages/app/src/components/alerts/AlertDetails.tsx:150). Pass it on Message so the recipient lands on the alert that fired, and other length-limited transports can reuse it.

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

try {
const url = new URL(hdxLink);
return `${url.origin}/alerts`;

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.

🟠 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

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 is a valid concern. @Tyagiquamar Could you address it?

Comment on lines +20 to +21

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.

P2 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.

Fix in Claude Code Fix in Conductor Fix in Cursor Fix in Codex

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.

🟠 major — Fallback link drops the deployment path prefix and points to a page that doesn't exist

The links come from ${config.FRONTEND_URL}/chart (packages/api/src/tasks/checkAlerts/providers/default.ts:523). The app also supports serving under a path prefix (NEXT_PUBLIC_HYPERDX_BASE_PATH, packages/app/src/config.ts:62-66). url.origin throws that prefix away, so a FRONTEND_URL like https://host/clickstack falls back to https://host/alerts, which is a 404. Build the fallback from config.FRONTEND_URL instead of parsing it back out of the link.

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

} catch {
return hdxLink;
}
};

export const formatSlackSectionText = (
hdxLink: string,
title: string,
body: string,
maxLength = MAX_SLACK_SECTION_LENGTH,
): string => {
let link = hdxLink;
let text = `*<${link} | ${title}>*\n${body}`;
if (text.length <= maxLength) {
return text;
}

// If text exceeds limit, try falling back to the shorter /alerts link
const fallbackLink = getFallbackHdxLink(hdxLink);

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.

🟠 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

if (fallbackLink !== link) {
text = `*<${fallbackLink} | ${title}>*\n${body}`;
if (text.length <= maxLength) {
return text;
}
link = fallbackLink;
}

// If still exceeding limit, truncate body with an ellipsis
const header = `*<${link} | ${title}>*\n`;
const availableBodyLen = maxLength - header.length - 3;
if (availableBodyLen > 0) {
return `${header}${body.slice(0, availableBodyLen)}...`;

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.

🔵 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

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.

🔵 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

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.

🔵 minor — Re-implements the existing truncateString helper

truncateString in packages/api/src/utils/common.ts:38 already appends '...' after slicing, and template.ts uses it. Reuse it (e.g. header + truncateString(body, maxLength - header.length - 3)) rather than adding a second inline version. The repo conventions mark this DRY check as REQUIRED.

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 `*<${link} | ${title}>*`.slice(0, maxLength);

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.

🔵 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

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.

P2 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.

Fix in Claude Code Fix in Conductor Fix in Cursor Fix in Codex

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.

🔵 minor — Last-resort slice cuts through the mrkdwn link markup

When the title alone pushes the header past the limit, *<link | title>*.slice(0, maxLength) drops the closing >*. Slack then renders the raw <https://... | ... text instead of a link. Truncate title so the *<link | ...>* wrapper stays intact.

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

};

export const handleSendSlackWebhook = async (
channel: WebhookChannel,
message: Message,
Expand All @@ -29,7 +72,11 @@ export const handleSendSlackWebhook = async (
type: 'section',
text: {
type: 'mrkdwn',
text: `*<${message.hdxLink} | ${message.title}>*\n${message.body}`,
text: formatSlackSectionText(
message.hdxLink,
message.title,
message.body,
),
Comment on lines +75 to +79

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.

P2 Required changeset is missing

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!

Fix in Claude Code Fix in Conductor Fix in Cursor Fix in Codex

},
},
],
Expand Down
Loading