Uh oh!
There was an error while loading. Please reload this page.
feat(app): list all webhook template variables; fix sourceQuery and thresholdMax - #3071
Conversation
The form advertised seven variables while the Generic and incident.io transports render nineteen, so the ten enriched variables added in #3057 were undiscoverable from the UI. List every variable from buildWebhookTemplateVariables with a one-line description, in a two-column grid, and note the JSON-escaping and empty-string-fallback behaviour.
🦋 Changeset detectedLatest commit: 334e939 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🔴 Tier 4 — CriticalTouches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI. Why this tier:
Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
Greptile SummaryThe PR expands webhook template-variable guidance and fixes range-bound and source-query values in generated webhook messages.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/api/src/tasks/checkAlerts/template.ts | Resolves source queries by alert source and includes the upper threshold only for range comparators. |
| packages/api/src/routers/api/webhooks.ts | Populates test webhook messages with representative values for every supported template variable. |
| packages/app/src/components/TeamSettings/WebhookForm.tsx | Adds a responsive, described list of Generic webhook template variables. |
| packages/app/src/components/TeamSettings/tests/WebhookForm.test.tsx | Covers visibility of the template-variable help box for Generic and Slack webhook selections. |
Reviews (5): Last reviewed commit: "Merge branch 'main' into warren/expose-a..." | Re-trigger Greptile
| await user.click(screen.getByRole('radio', { name: 'Generic' })); | ||
| const variables = screen.getByTestId('webhook-template-variables'); | ||
| for (const { name } of getWebhookTemplateVariables('HyperDX')) { |
There was a problem hiding this comment.
The assertion derives its expected names from the same getWebhookTemplateVariables helper that the form renders, so removing or mistyping an entry changes both sides and leaves the test green while the UI drifts from the API-supported variable set. Use an independent expected list to enforce the synchronization contract documented in WebhookForm.tsx.
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 Review✅ No critical issues found. The change is additive and back-compatible: 🟡 P2 -- recommended
🔵 P3 nitpicks (3)
Reviewers (7): correctness, adversarial, testing, maintainability, kieran-typescript, api-contract, previous-comments. Testing gaps:
|
| ))} | ||
| </span> | ||
| </SimpleGrid> | ||
| <Text size="xs" c="dimmed" mt="xs"> |
There was a problem hiding this comment.
🟠 major — "Test Webhook" sends a payload in which all 12 newly-advertised variables are empty
The test message built in packages/api/src/routers/api/webhooks.ts:463-471 only sets hdxLink, title, body, startTime, endTime, state, eventId — every enriched field (status, alertId, alertType, comparator, threshold, value, groupKey, sourceQuery, teamId, note, and both *TimeISO) is undefined. So a user who writes a body from the list this PR adds and immediately clicks Test Webhook gets blanks for most of it, and for the numeric slots the copy just told them to leave unquoted ("value": {{value}}, as in docs/alert-webhook-template-variables.md:44) Handlebars renders nothing, producing {"value": } — malformed JSON that the receiver rejects, surfacing as "Failed to send test webhook. Please check your webhook configuration." for a body that would work in production. Populate the enriched fields with sample values in the test message (e.g. status: 'firing', alertType: 'search', comparator: '>=', threshold: 1, value: 2, alertId: 'test-alert-id', teamId, ISO times) so Test Webhook exercises the same variable set the form documents.
| await user.click(screen.getByRole('radio', { name: 'Generic' })); | ||
| const variables = screen.getByTestId('webhook-template-variables'); | ||
| for (const { name } of getWebhookTemplateVariables('HyperDX')) { |
There was a problem hiding this comment.
🔵 minor — The new test is a tautology — it asserts the rendered list against the same function that renders it
for (const { name } of getWebhookTemplateVariables('HyperDX')) compares the DOM to the very constant that produced the DOM, so it passes for any list, including one that has drifted from buildWebhookTemplateVariables (packages/api/src/tasks/checkAlerts/transports/generic.ts:84) — which is the only failure mode the new comment at WebhookForm.tsx:47 warns about. Assert against a literal expected array of the 19 names (so adding/removing a variable forces a deliberate test edit), and add a test pinning that array to the API key set once the list is shared (see the common-utils finding); the descriptions are currently unasserted too.
| // Mirrors buildWebhookTemplateVariables in | ||
| // packages/api/src/tasks/checkAlerts/transports/generic.ts — keep in sync when | ||
| // variables are added or removed there. | ||
| export const getWebhookTemplateVariables = ( |
There was a problem hiding this comment.
🔵 minor — Third hand-maintained copy of the variable set; a shared list in common-utils would make it compiler-enforced
The set now lives in three places that must be edited together: buildWebhookTemplateVariables (packages/api/src/tasks/checkAlerts/transports/generic.ts:84), docs/alert-webhook-template-variables.md, and this constant. The PR description's rationale ("the app package can't import from packages/api, so a type can't enforce the pairing") holds only for a direct app→api import — both packages already import @hyperdx/common-utils (this file imports dist/types, generic.ts imports dist/types too, and WebhookSchema lives there). Put the canonical name list in common-utils and type the API builder's return as satisfies Record<WebhookTemplateVariable, string | number>, so adding a variable in one place fails to compile in the other; that also keeps the 45 lines of static data out of a component file the repo conventions already want under 300 lines (it is now 504).
| </SimpleGrid> | ||
| <Text size="xs" c="dimmed" mt="xs"> | ||
| Strings are JSON-escaped, so they are safe inside quotes. Numbers | ||
| are emitted raw for unquoted slots. A variable the alert |
There was a problem hiding this comment.
🔵 minor — The advice "numbers are emitted raw for unquoted slots" is flagged as an error by the editor directly above it
The body field is linted with jsonLinterWithEmptyCheck (line 92), plain jsonParseLinter. A template following the new guidance — {"value": {{value}}, "threshold": {{threshold}}} — is not valid JSON, so CodeMirror marks it with a red error squiggle immediately above the help box that recommends it. Either lint the template with {{…}} slots substituted for placeholder literals before parsing, or drop the unquoted-number sentence and show "{{value}}" as the supported form.
| defaultOptions: { queries: { retry: false } }, | ||
| }); | ||
| return render( | ||
| <QueryClientProvider client={queryClient}> |
There was a problem hiding this comment.
🔵 minor — Hand-rolled Mantine wrapper duplicates the global renderWithMantine, and the QueryClientProvider is dead
renderWithMantine is defined in packages/app/src/setupTests.tsx:36 and used by 74 other test files; use it instead of a bespoke MantineProvider wrapper. The QueryClient/QueryClientProvider can also go — @/api is fully mocked and WebhookForm touches react-query only through useSaveWebhook/useUpdateWebhook/useTestWebhook.
PR Review6 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 5 minor 6 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. |
E2E Test Results✅ All tests passed • 342 passed • 1 skipped • 1341s
Tests ran across 4 shards in parallel. |
…oldMax Two gaps in the enriched webhook template variables from #3057. sourceQuery read only savedSearch.where, so an inline-query or dashboard-tile alert rendered it empty — the query for those lives on the alert's or the tile's chart config. It now resolves from whichever config backs the alert, reading the builder `where` or the raw `sqlTemplate`. A between/outside condition sent only its lower bound, so a receiver could not reconstruct the range that fired. Adds {{thresholdMax}}, left undefined (and so rendered empty) for every other comparator.
| ))} | ||
| </span> | ||
| </SimpleGrid> | ||
| <Text size="xs" c="dimmed" mt="xs"> |
There was a problem hiding this comment.
🔵 minor — The form recommends unquoted numeric slots, which the body editor's own JSON linter marks as a syntax error
The new footer says "Numbers are emitted raw for unquoted slots", but the body editor above it runs jsonParseLinter() on the raw Handlebars source (jsonLinterWithEmptyCheck, WebhookForm.tsx:96-100), so the recommended {"value": {{value}}} is flagged as invalid JSON in the editor. Either strip/replace {{…}} expressions with equal-length filler before handing the text to jsonParseLinter (keeps diagnostic offsets valid), or drop the raw-number guidance from the form. Also carry over the caveat this PR adds to docs/alert-webhook-template-variables.md:34 — that an absent optional number breaks the JSON, hence {{#if thresholdMax}}…{{/if}} — since the form, not the doc, is where bodies get written.
| // Mirrors buildWebhookTemplateVariables in | ||
| // packages/api/src/tasks/checkAlerts/transports/generic.ts — keep in sync when | ||
| // variables are added or removed there. | ||
| export const getWebhookTemplateVariables = ( |
There was a problem hiding this comment.
🔵 minor — Second source of truth for the variable set, and the new test can't detect drift
getWebhookTemplateVariables restates the 20 keys of buildWebhookTemplateVariables (api/src/tasks/checkAlerts/transports/generic.ts:84) with only a "keep in sync" comment holding them together, and the new WebhookForm.test.tsx iterates that very array to assert the DOM — so it pins the component to itself and would still pass if the API added or dropped a variable. Put the canonical name list in packages/common-utils/src/types.ts next to WebhookService (both packages already import from there), have the form render it, and add satisfies Record<WebhookTemplateVariable, unknown> to the object literal in buildWebhookTemplateVariables so a variable added on either side is a compile error rather than silent drift.
The new inline/tile cases reached for `as any` and matching eslint disables, which pushed packages/api past the escape-hatch ratchet baseline. Build the chart configs as typed AlertChartConfig literals and let makeTileView take the tile, so no cast is needed at all.
| const DEFAULT_GENERIC_WEBHOOK_BODY_TEMPLATE = | ||
| DEFAULT_GENERIC_WEBHOOK_BODY.join(' | '); | ||
| // Mirrors buildWebhookTemplateVariables in |
There was a problem hiding this comment.
🔵 minor — The variable list is a second source of truth for buildWebhookTemplateVariables, and the new test cannot detect drift
getWebhookTemplateVariables restates the 20 keys of buildWebhookTemplateVariables (packages/api/src/tasks/checkAlerts/transports/generic.ts:84) with only a comment holding them together, and WebhookForm.test.tsx asserts the rendered DOM against that same list, so dropping or renaming an entry — or adding a variable on the API side — still passes. Move the canonical name list into packages/common-utils (both packages already import @hyperdx/common-utils/dist/types) and have buildWebhookTemplateVariables key off it, or at minimum assert the form list against a literal expected array in the test so drift fails.
| an empty `{{groupKey}}`, and a non-range alert has an empty `{{thresholdMax}}`. | ||
| An empty variable in an unquoted numeric slot produces invalid JSON, so guard | ||
| the optional numbers: `{{#if thresholdMax}}"max": {{thresholdMax}},{{/if}}`. |
There was a problem hiding this comment.
🔵 minor — The documented {{#if}} guard silently drops a value of 0
Handlebars' if treats 0 as falsy unless includeZero=true, so {{#if thresholdMax}}"max": {{thresholdMax}},{{/if}} omits the bound for a valid between -10 and 0 alert, and the same pattern applied to {{value}} drops the very common value: 0 case. Document the guard as {{#if thresholdMax includeZero=true}}...{{/if}}.
| </span> | ||
| </SimpleGrid> | ||
| <Text size="xs" c="dimmed" mt="xs"> | ||
| Strings are JSON-escaped, so they are safe inside quotes. Numbers |
There was a problem hiding this comment.
🔵 minor — New help text recommends unquoted numeric slots that the body editor's own JSON linter flags as errors
The body CodeMirror runs linter(jsonLinterWithEmptyCheck()) (line 437), which parses the raw text, so a body following the new advice — {"value": {{value}}} — is underlined as invalid JSON even though it renders to valid JSON at delivery time. Make jsonLinterWithEmptyCheck substitute {{...}} expressions with a placeholder literal (e.g. 0 outside quotes) before delegating to jsonParseLinter, so template bodies lint on their rendered shape.
| if (chartConfig == null) { | ||
| return ''; | ||
| } | ||
| // Narrowed on `configType` rather than through isRawSqlSavedChartConfig / |
There was a problem hiding this comment.
🔵 minor — Re-implements the existing chart-config type guards inline, on a justification the codebase contradicts
The comment says isRawSqlSavedChartConfig / isPromqlSavedChartConfig (packages/common-utils/src/guards.ts:58) can't be used because they predicate on SavedChartConfig, but packages/api/src/controllers/alerts.ts:122 already calls isRawSqlSavedChartConfig on an AlertChartConfig — the without-alert variants are assignable, since alert is optional on the saved variants. Replace the hand-rolled 'configType' in chartConfig narrowing with if (isRawSqlSavedChartConfig(cfg)) return cfg.sqlTemplate; / if (isPromqlSavedChartConfig(cfg)) return ''; / return cfg.where ?? ''; per the repo's DRY rule, or correct the comment if the compiler actually rejects it.
The webhook form lists all 20 variables directly above the Test Webhook
button, but the test payload carried only the original 7. Enriched string
variables rendered empty, and because threshold, thresholdMax and value
are emitted unquoted, a body like {"value": {{value}}} was sent as
{"value": } — invalid JSON, so the receiver rejected a template that
works on a real firing.
Populate every field with a sample, using a range comparator so
thresholdMax is exercised too, and type the literal as Message so a new
field has to be given one. State moves to ALERT to agree with
status: 'firing'.| state: AlertState.INSUFFICIENT_DATA, | ||
| startTime: now - ms('5m'), | ||
| endTime: now, | ||
| state: AlertState.ALERT, |
There was a problem hiding this comment.
🟠 major — Test Webhook now sends a firing payload, so testing an incident.io webhook opens a real alert
state went from AlertState.INSUFFICIENT_DATA to AlertState.ALERT. The incident.io body the form generates (WebhookForm.tsx:186) is "status": "{{#if (eq state \"ALERT\")}}firing{{else}}resolved{{/if}}", so a test send now posts status: "firing" with deduplication_key: "test-event-id" — incident.io creates an alert that can escalate/page and stays open until someone resolves it by hand (and every subsequent test dedupes onto the same one). Previously it rendered resolved, which was a no-op. Keep all the newly populated sample fields but pick a non-firing pairing — state: AlertState.OK with status: 'resolved' — so a test send still exercises every variable without paging on-call.
| // Mirrors buildWebhookTemplateVariables in | ||
| // packages/api/src/tasks/checkAlerts/transports/generic.ts — keep in sync when | ||
| // variables are added or removed there. | ||
| export const getWebhookTemplateVariables = ( |
There was a problem hiding this comment.
🔵 minor — Variable list is a second source of truth kept in sync by a comment, and the new test can't detect drift
getWebhookTemplateVariables restates the key set of buildWebhookTemplateVariables (packages/api/src/tasks/checkAlerts/transports/generic.ts:84) with only a "keep in sync" comment; WebhookForm.test.tsx:46 then iterates that same exported array, so it re-asserts the list against itself and would still pass after the next drift — which is exactly the 7-vs-19 drift this PR exists to fix. The app can share with the API via common-utils (both already import @hyperdx/common-utils/dist/types, which is where the webhook schemas live): put the variable name/description table there and have the API builder key off it (satisfies Record<WebhookTemplateVariable, unknown>) so adding a variable on one side fails to compile on the other.
| name: '{{status}}', | ||
| description: 'firing, resolved, no_data, pending or error', | ||
| }, | ||
| { name: '{{eventId}}', description: 'Unique id for this firing' }, |
There was a problem hiding this comment.
🔵 minor — {{eventId}} is described as "Unique id for this firing" but it is constant across firings
eventId is objectHash({ alertId, channel, isGrouped, groupId }) (packages/api/src/tasks/checkAlerts/template.ts:561) — no time component, so every firing of the same alert/channel/group produces the identical value. That's why generic.ts:203 hashes eventIdtogether withstartTime/endTime/state to build the Idempotency-Key. A template author following this copy would collapse every firing into one receiver-side event. Describe it as "stable dedup key for this alert/channel/group" (and fix the same wording in docs/alert-webhook-template-variables.md:13).
| ))} | ||
| </span> | ||
| </SimpleGrid> | ||
| <Text size="xs" c="dimmed" mt="xs"> |
There was a problem hiding this comment.
🔵 minor — Form footer omits the empty-raw-number caveat, and the test sample always populates thresholdMax so the trap passes the test
The footer says numbers are emitted raw and a missing variable "renders as an empty string", but doesn't say that combination yields invalid JSON — the exact warning added to docs/alert-webhook-template-variables.md:34. It bites hardest for {{thresholdMax}}: the test message hardcodes comparator: 'between' with thresholdMax: 10, so {"max": {{thresholdMax}}} passes Test Webhook and then sends {"max": } on every non-range alert. Add the docs' one-liner to the footer, e.g. "An optional number left unset renders as nothing — guard it: {{#if thresholdMax}}\"max\": {{thresholdMax}},{{/if}}."
| an empty `{{groupKey}}`, and a non-range alert has an empty `{{thresholdMax}}`. | ||
| An empty variable in an unquoted numeric slot produces invalid JSON, so guard | ||
| the optional numbers: `{{#if thresholdMax}}"max": {{thresholdMax}},{{/if}}`. |
There was a problem hiding this comment.
🔵 minor — Recommended {{#if thresholdMax}} guard silently drops a thresholdMax of 0
Handlebars #if is falsy on 0, so an alert configured as between -10 and 0 renders no "max" key at all — the receiver silently loses the bound the guard was meant to preserve. Either note the caveat or show a presence check that survives zero, e.g. {{#unless (eq thresholdMax undefined)}}"max": {{thresholdMax}},{{/unless}} using the already-registered eq helper (packages/api/src/tasks/checkAlerts/transports/generic.ts:113).
| placeholder: jest.fn(), | ||
| })); | ||
| function renderForm() { |
There was a problem hiding this comment.
🔵 minor — Test hand-rolls a Mantine wrapper and wraps in a QueryClientProvider that nothing uses
packages/app/src/setupTests.tsx:36 already exposes a global renderWithMantine (used by SearchWhereInput.test.tsx, TimelineMinimap.test.tsx, …), and it also mounts <Notifications />, which this form needs if a test ever exercises the submit/test paths. The QueryClientProvider is dead scaffolding here — @/api is mocked wholesale at the top of the file, so no react-query hook ever runs. Replace renderForm with renderWithMantine(<WebhookForm onClose={jest.fn()} onSuccess={jest.fn()} />) and drop the @tanstack/react-query import.
Uh oh!
There was an error while loading. Please reload this page.
The webhook form advertised seven template variables while the Generic and incident.io transports render nineteen. The ten enriched variables added in #3057 —
{{alertId}},{{status}},{{comparator}},{{value}}and friends — were undiscoverable unless you readdocs/alert-webhook-template-variables.md.Writing that list out surfaced two variables that don't hold up their end, both flagged by Greptile on #3057. They're fixed here.
UI
The help box under Webhook Body now lists every variable in
buildWebhookTemplateVariables, each with a one-line description lifted from the docs, in a two-column grid that collapses to one column on narrow viewports. A footer states the two rules a template author needs: strings are JSON-escaped and safe inside quotes, numbers are emitted raw, and a variable the alert doesn't carry renders empty.{{link}}'s description interpolates the brand display name rather than hardcoding "HyperDX".DEFAULT_GENERIC_WEBHOOK_BODYis untouched — it still drives the default body and the editor placeholder, and must keep matchingDEFAULT_GENERIC_WEBHOOK_BODY_TEMPLATEintransports/generic.ts. The new list is a separate display-only constant. The app package can't import frompackages/api, so a type can't enforce the pairing; a comment points at the API function as the source of truth.{{sourceQuery}}was empty for chart-backed alertsIt read only
savedSearch?.where. An inline-query alert keeps its query on the alert's ownchartConfig, and a tile alert on the tile's config, so both rendered an empty string — the variable was advertised but never arrived.getAlertSourceQuerynow resolves the query from whichever config backs the alert, reading the builderwhereor the rawsqlTemplate.Greptile flagged only the inline case. Tile alerts have the identical shape, so leaving them empty would be an arbitrary gap; the docs sentence that described tile alerts as intentionally empty is updated.
Narrowing is on
configTyperather than theisRawSqlSavedChartConfig/isPromqlSavedChartConfigguards: those predicate onSavedChartConfig, and an inline alert'sAlertChartConfigunion is built from the without-alert variants, so the guards can't subtract a member from it. There's a comment saying so, to stop a future reader "simplifying" it back.{{thresholdMax}}— the missing half of a range conditionA
between/outsidealert sent onlyalert.threshold, so a receiver saw a lower bound with no way to reconstruct the condition. New{{thresholdMax}}carries the upper bound, leftundefined(and so rendered empty) for every other comparator rather than leaking a bound that isn't part of the condition.Like the other raw numbers it's emitted unquoted, so an empty value in an unquoted slot produces invalid JSON. That trade-off already existed for the other optional numbers; the docs now show the
{{#if}}guard.Testing
WebhookForm.test.tsxasserts every variable renders for a Generic webhook and that the box is absent for Slack — verified against the rendered DOM, not just the markup.Five integration tests cover the API fixes: both bounds on a range condition, no upper bound on a non-range one, and
sourceQueryfor tile, inline builder, and inline raw SQL alerts. I checked they're not vacuous by reverting the fix — four of the five fail without it. (omits the upper boundpasses either way, since it asserts absence.)Screenshots