[HDX-5192] UI for creating and editing inline chart alerts - #3069
Conversation
🦋 Changeset detectedLatest commit: 6c2e010 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.
|
wrn14897
added
the
ai-generated
Greptile SummaryThis PR adds UI support for creating, opening, previewing, and editing alerts whose chart configuration is persisted directly on the alert.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/app/src/DBChartPage.tsx | Adds inline-alert creation and alert-ID-based chart seeding to the chart explorer. |
| packages/app/src/components/alerts/AlertDetailChart.tsx | Adds shared chart-config assembly and inline-alert detail previews, with an explanatory fallback for single-value raw-SQL alerts. |
| packages/app/src/components/alerts/EditInlineAlertModal.tsx | Adds full-chart editing for persisted inline alerts, including lazy detail loading and replacement updates. |
| packages/app/src/hooks/useAlertSeededChartConfig.ts | Resolves an inline alert ID through the detail API and seeds its persisted chart configuration into the explorer. |
| packages/app/src/utils/alerts.ts | Adds inline-alert URLs, source metadata, form-channel conversion, and API payload construction. |
| packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx | Generalizes alert controls and validation so charts outside dashboards can create and update inline alerts. |
Sequence Diagram
sequenceDiagram
actor User
participant Explorer as Chart explorer
participant API as Alerts API
participant Store as Alert store
User->>Explorer: Build chart and configure alert
Explorer->>API: POST inline alert with chartConfig
API->>Store: Persist alert and chartConfig
Store-->>API: Created alert
API-->>Explorer: Alert ID
User->>Explorer: "Open /chart?alertId=<id>"
Explorer->>API: GET alert detail
API->>Store: Load persisted alert
Store-->>API: Alert with chartConfig
API-->>Explorer: Persisted chart configuration
Explorer-->>User: Render editable chart
Reviews (8): Last reviewed commit: "Merge branch 'main' into warren/HDX-5192..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!isTimeSeriesDisplayType(savedConfig.displayType)) { | ||
| return undefined; |
There was a problem hiding this comment.
When an inline alert uses a raw-SQL Number chart, this branch rejects the persisted config because isTimeSeriesDisplayType only accepts Line and Stacked bar, causing the detail page to show the unsupported-preview fallback instead of its threshold chart.
Knowledge Base Used:
| if (errors.length > 0) { | ||
| notifications.show({ | ||
| id: 'chart-error', | ||
| title: 'Invalid Chart', |
There was a problem hiding this comment.
The new Invalid Chart notification violates the required sentence-case convention for user-facing text, making this error inconsistent with the rest of the UI.
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!
E2E Test Results✅ All tests passed • 357 passed • 1 skipped • 1161s
Tests ran across 4 shards in parallel. |
🔴 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
|
|
Checked this against the evaluator — the guard is correct, so I've kept the behavior and made it explain itself instead. A raw SQL Number alert is a valid alert, but it has no threshold-over-time chart to draw. Its SQL deliberately carries no interval parameter to bucket by ( // packages/api/src/tasks/checkAlerts/index.ts
// Raw SQL charts with Number display type don't use interval parameters, so they cannot be treated as timeseries.
// Number-type Builder Charts are rendered as time-series, to maintain legacy behavior for existing alerts.
if (isRawSqlChartConfig(chartConfig) && chartConfig.displayType === DisplayType.Number) {
return { type: 'single_value', valueColumnNames };
}So there is no series for the client to render either — charting it would require re-bucketing SQL the user controls. (Builder Number configs are converted to a line chart, which is the branch just below; only raw SQL is single-value.) This guard is also pre-existing rather than new here — it was in What was fair in the report is the UX: the fallback said only "can't be previewed here", which reads like a defect. |
Deep Review✅ No critical issues found. No P0/P1: the new inline-alert flows type-check cleanly, the client payload matches the server's 🟡 P2 -- recommended
🔵 P3 nitpicks (6)
Reviewers (11): correctness, testing, maintainability, project-standards, agent-native, learnings-researcher, kieran-typescript, julik-frontend-races, adversarial, api-contract, previous-comments. Testing gaps:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| return { | ||
| ...alert.chartConfig, | ||
| alert: { |
There was a problem hiding this comment.
🔵 minor — inlineAlertToChartConfig duplicates alertToFormValues's alert-field mapping verbatim
The 11-field alert object here (id, interval, threshold, thresholdMax, thresholdType, scheduleOffsetMinutes, the string/Date scheduleStartAt coercion, the toAlertChannels(...).map(webhookId ?? '') block with its comment, name, message, note, numConsecutiveWindows ?? undefined) is a character-for-character copy of the base object in alertToFormValues (packages/app/src/components/alerts/EditAlertModal.tsx:89-115) — two sources of truth for the same alert→editor mapping, which the repo's REQUIRED DRY rule asks you to grep for first. Export the shared base builder (e.g. alertToEditableFields in @/utils/alerts) and have both modals call it, so a future change to channel coercion or a new alert field only has to be made once.
| }; | ||
| } | ||
|
|
||
| if (alert.source === AlertSource.INLINE && alert.chartConfig) { |
There was a problem hiding this comment.
🔵 minor — The defensive INLINE branch does not fire in the only case it exists for
AlertRowMenu is the sole caller of EditAlertModal and already routes AlertSource.INLINE to EditInlineAlertModal (AlertRowMenu.tsx:202), so this branch is unreachable today. Worse, it is guarded on alert.chartConfig, which GET /alerts deliberately omits for list rows (packages/api/src/routers/api/alerts.ts:88) — so a list-shaped inline alert reaching here would skip the branch and still fall through to the saved-search return, rewriting source to saved_search with an empty savedSearchId: precisely the corruption the branch is meant to prevent. Guard on alert.source === AlertSource.INLINE alone and refuse the save when chartConfig is missing, or delete the branch.
| if (alert.source === AlertSource.SAVED_SEARCH && alert.savedSearch) { | ||
| return `/search/${alert.savedSearchId}`; | ||
| } | ||
| if (alert.source === AlertSource.INLINE && alert.chartConfig) { |
There was a problem hiding this comment.
🔵 minor — Inline alerts have no link to their chart on the alerts list in the default configuration
getAlertSourceUrl needs alert.chartConfig, which the list endpoint never returns, so it returns '' for every inline row. With IS_ALERT_DETAILS_ENABLED off — the production default (packages/app/src/config.ts:85-88) — AlertDetails then renders the name as plain text (AlertDetails.tsx:179-181) and AlertRowMenu receives alertUrl={undefined}, so there is no "Open chart" item either; the only way to see the query is Edit alert. The PR summary states these rows "link back to the explorer seeded with their config", which only holds behind the details flag. Either include chartConfig on the list response for inline alerts, or derive the row's href from /alerts/:id-independent state so the chart stays reachable with the details page off.
PR Review4 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 3 minor 4 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. |
github-actions
Bot
removed
the
review/tier-3
|
Good catch — this one was a real gap, and it's now fixed in this branch rather than deferred: The link was derived from Rather than reopen the list-response contract, inline alerts are now linked by id: Two behaviors worth calling out, both covered by tests:
|
github-actions
Bot
added
the
review/tier-4
| notifications.show({ | ||
| color: 'red', | ||
| title: 'Error creating alert', | ||
| message: |
There was a problem hiding this comment.
🟠 major — Create-alert failures discard the server's reason and show a generic HTTP message
error.message on a ky HTTPError is the boilerplate "Request failed with status code 400 Bad Request" — the API's actual reason is in the JSON body ({ message }, see packages/api/src/middleware/error.ts:110). Inline-alert creation has many specific 400s the user must act on (validateAlertInput in packages/api/src/controllers/alerts.ts:116-198: "Raw SQL alert query is invalid: …", "Source does not belong to the specified connection", "Source not found", "At least one notification channel is required"), and none of them reach the toast. Follow the pattern already used elsewhere in this package — if (error instanceof HTTPError) { const body = await error.response.json(); … } as in packages/app/src/components/TeamSettings/WebhookForm.tsx:158 and packages/app/src/AuthPage.tsx:95 — and fall back to the generic string only for non-HTTP errors.
| } | ||
| return { | ||
| ...alert.chartConfig, | ||
| alert: { |
There was a problem hiding this comment.
🔵 minor — inlineAlertToChartConfig re-implements EditAlertModal.alertToFormValues's base block verbatim
The alert: { … } object (lines 35-61) is a field-for-field copy of the base object in packages/app/src/components/alerts/EditAlertModal.tsx:89-115 — same scheduleStartAt string/Date ternary, same toAlertChannels(...).map(c => ({ ...c, webhookId: c.webhookId ?? '' })), same comment text — and previousScheduleFields (lines 141-153) is a second copy of EditAlertModal.tsx:257-272. The repo conventions mark DRY REQUIRED and prescribe factoring shared logic into a sibling module; @/utils/alerts already hosts toAlertChannels and normalizeNoOpAlertScheduleFields. Extract alertToBaseFormValues(alert) and a toScheduleStartAtString(value) helper there and call them from both modals, so an added alert field can't be round-tripped by one editor and dropped by the other.
| data-testid="inline-alert-editor-form" | ||
| chartConfig={chartConfig} | ||
| dateRange={dateRange} | ||
| onSave={onSave} |
There was a problem hiding this comment.
🔵 minor — The two inline-alert save surfaces use different props and duplicate the "alert must exist" check
The explorer persists an inline alert through onSaveAlert, which routes to handleSaveAlert in EditTimeChartForm.tsx:565-593 and rejects a config whose alert was dropped; this modal persists the same concept through the generic onSave/handleSave, which has no such check, so the identical guard is re-implemented here as the buildInlineAlertPayload(config) == null branch (lines 127-136) with different copy. Drive both from onSaveAlert (with saveAlertLabel="Save alert") so the inline-alert validation and the save button live in one place, and onSave keeps meaning "save the chart".
|
Correct, and it bit both save paths — fixed in The explorer showed ky's status line; the edit modal was arguably worse, showing a fixed "Something went wrong. Please contact HyperDX team." for what is usually a specific, fixable 400. These matter more here than in most places because the API validates more than the editor can — raw SQL templates, source/connection ownership, formula references — so its reason is often the only thing that says what to fix. Added
Anything unrecognised falls back to the caller's copy rather than surfacing ky's status line, which tells the user nothing actionable. One deliberate deviation from the Covered by 5 helper tests (both body shapes, multi-issue join, non-JSON body, non-HTTP error, thrown non-error) plus one on |
| }; | ||
| } | ||
|
|
||
| if (alert.source === AlertSource.INLINE && alert.chartConfig) { |
There was a problem hiding this comment.
🔵 minor — Inline guard in alertToFormValues misses the only case it defends against
The branch requires alert.chartConfig, but the case it is documented to protect — an inline alert reaching this modal from a list row, where GET /alerts omits chartConfig — has no config and still falls through to the saved-search branch, rewriting source to SAVED_SEARCH with an empty savedSearchId on save. Either key the branch on alert.source === AlertSource.INLINE alone (spreading chartConfig only when present is not enough — the PUT would still be rejected/wrong, so bail out instead), or drop the branch, since AlertRowMenu is the sole caller and already routes inline alerts elsewhere.
| // no saved search or dashboard tile behind it. The alert is dropped from the | ||
| // explorer's config afterwards: it now lives on the alert document, and | ||
| // leaving it in the URL would offer to create a second copy. | ||
| const onSaveAlert = useCallback( |
There was a problem hiding this comment.
🔵 minor — The explorer's create-alert flow has no test
There is no test file for DBChartPage, so onSaveAlert is exercised nowhere: not the payload it POSTs, not the success notification's /alerts/{id} link, and in particular not setChartConfig({ ...config, alert: undefined }) — the step whose whole purpose is to stop the same URL config offering to create a duplicate alert. buildInlineAlertPayload is covered in utils/tests/alerts.test.ts, but the wiring around it is not. Add a test that mounts the page with a stubbed useCreateAlert, saves an alert, and asserts the config written back has no alert.
ecb40d0 to
db5a132
Compare
|
Rebased onto
One incidental dedup fell out of it: both edit modals normalized the response's loose channel shape onto the form's strict one with the same cast and the same comment. That is now
|
| }; | ||
| } | ||
|
|
||
| if (alert.source === AlertSource.INLINE && alert.chartConfig) { |
There was a problem hiding this comment.
🔵 minor — The defensive INLINE branch in alertToFormValues cannot fire, and its guard excludes the only shape that would reach it
AlertRowMenu is the sole caller and routes inline alerts to EditInlineAlertModal, so this branch (and the isTileAlert→isChartAlert rename) is dead today. Worse, GET /alerts omits chartConfig (packages/api/src/routers/api/alerts.ts:96-97), so a list-shaped inline alert that ever did reach here fails && alert.chartConfig and falls through to the SAVED_SEARCH branch — the rewrite the comment says it prevents. Either drop the branch, or key it on alert.source === AlertSource.INLINE alone and refuse to submit when chartConfig is missing.
| )} | ||
| {/* Only once an alert exists on the chart: with none there is nothing | ||
| to save, and the button would read as a second way to add one. */} | ||
| {onSaveAlert != null && handleSaveAlert != null && hasAlert && ( |
There was a problem hiding this comment.
🔵 minor — Switching configType to PromQL leaves a stale alert in form state, so "Create alert" stays visible but can only fail
The alert-clearing effect returns early unless displayType changed (EditTimeChartForm.tsx:283-292), so switching the Builder/SQL/PromQL segmented control to PromQL keeps alert in form state while PromqlChartEditor renders no alert panel. hasAlert is still true, so the button shows; clicking it hits convertFormStateToSavedChartConfig's PromQL branch, which drops alert (ChartEditor/utils.ts:203), and the user gets "This chart has no alert to save." with nothing to act on. Clear alert when configType becomes promql (and gate the button on a config type that supports alerts) — the comment at utils/alerts.ts:358 assumes this already holds.
| return `/search/${alert.savedSearchId}`; | ||
| } | ||
| if (alert.source === AlertSource.INLINE) { | ||
| return `/chart?alertId=${alert._id}`; |
There was a problem hiding this comment.
🔵 minor — The inline alert's explorer link carries no time range, so long-interval alerts open on a window too short to plot
/chart?alertId=<id> sets no from/to, so the explorer falls back to "Past 1h" while the seeded config keeps the alert's granularity — a 6h/12h/1d-interval alert opens showing one bucket or none. The server-built notification link already extends the window around the evaluation (packages/api/src/tasks/checkAlerts/providers/default.ts:512-536), and useNewTimeQuery reads from/to from the URL (packages/app/src/timeQuery.ts:442-453). Append a range derived from the interval — intervalToDateRange(alert.interval) in this same file already computes one.
| try { | ||
| const { data: created } = await createAlert.mutateAsync(payload); | ||
| queryClient.invalidateQueries({ queryKey: api.getAlertsQueryKey() }); | ||
| setChartConfig({ ...config, alert: undefined }); |
There was a problem hiding this comment.
🔵 minor — Edits made while the create-alert request is in flight are discarded when it resolves
setChartConfig({ ...config, alert: undefined }) re-writes the URL config from the pre-request snapshot. Because EditTimeChartForm passes chartConfig through RHF's values prop, that write resets the form: type in the SQL editor or add a series while the POST is outstanding and the change vanishes when the response lands. Strip the alert off whatever config is current at that moment instead — nuqs' setter accepts an updater, e.g. setChartConfig(prev => ({ ...prev, alert: undefined })).
|
nit: on the chart explorer, there are no tags to inherit so this input should say 0 instead of Inherited by default 
And the name input should probably be required since there is nothing to inherit: 
|
pulpdrew
left a comment
pulpdrew
left a comment
There was a problem hiding this comment.
LGTM other than the comment about the placeholders for alert name and tags.
I also think this is something that could use a few E2E tests to prevent regressions in the future.
HDX-5090's backend added `AlertSource.INLINE` — alerts that persist their own
chartConfig and evaluate with no saved search or dashboard tile behind them.
Nothing could create or edit one. This adds the UI.
Chart explorer: the alert affordances are gated on a new `enableAlerts` prop
rather than on being inside a dashboard, and a "Create alert" action POSTs
`{ source: 'inline', chartConfig, ...alertFields }`. The alert editor grows a
name field outside a dashboard — a tile alert is named by its tile, an inline
alert has nothing else to name it, and the name doubles as the notification
title (falling back to the chart's name).
Alerts list: inline alerts show their name with a chart icon and link back to
the explorer seeded with their config, matching the link the notification
sends. The row name renders as plain text when nothing resolves, rather than
as a link to an empty href — which also covers a tile alert whose dashboard
was deleted.
Alert details: the chart renders from the persisted config through a builder
extracted from the tile variant, so the two previews cannot drift. Editing
opens the full chart editor seeded from the alert, so the query and the alert's
fields are changed in one place. The list response omits chartConfig, so an
alert opened from a row fetches its detail response first.
EditAlertModal keeps handling saved-search and tile alerts; an inline alert
routed there would previously have fallen through and been rewritten as a
saved-search alert.
…iews Lowering `--max-warnings` to 563 to match a local count broke CI, which counts one more (type-aware rules resolve differently against the built common-utils). The cap is unrelated to this branch; put it back at 564. A raw SQL Number alert has no threshold-over-time chart to draw: its SQL carries no interval parameter to bucket by, which is why the check-alerts task evaluates it as a single value per window (`getResponseMetadata`). The preview correctly declines to chart one — pre-existing behavior for tile alerts, now shared with inline alerts — but said only "can't be previewed here", which reads as a defect. Name the reason instead, and pin it with a test.
`getAlertSourceUrl` built an inline alert's explorer link from its chartConfig, which `GET /alerts` omits by design — so with the alert detail page disabled, a list row had no link to the query at all (the row menu suppresses its own source link when details are off, leaving Edit alert as the only way in). Link by id instead: `/chart?alertId=<id>`, which the explorer resolves itself. Independent of what the list response carries, and it opens the query as it stands now rather than a snapshot taken when the link was built. The notification link keeps inlining the config — it is built server-side, with no session to fetch with — and lands on the same chart. The explorer holds its form back until the seed resolves: it auto-runs once on mount, so seeding after that would leave a result for the query it no longer shows. Clearing the param is what releases it, and also what keeps a reload from re-seeding over the user's edits.
ky's HTTPError carries only the status line ("Request failed with status
code 400 Bad Request"); the reason is in the response body. Both inline
alert save paths showed the former — the explorer showed ky's message,
the edit modal a fixed "contact the team" line — so none of the API's
specific 400s reached the user.
They are the ones that matter here: the API validates more than the
editor can (raw SQL templates, source/connection ownership, formula
references), so its reason is usually the only thing that says what to
fix. Add getApiErrorMessage, which reads both body shapes the API
produces — a route's `{ message }` and zod-express-middleware's
`[{ errors: { issues } }]`, reporting every issue rather than the first
so one save doesn't take several round trips to fix — and falls back to
the caller's copy for anything else.
Detects the response structurally rather than with `instanceof
HTTPError`: that identity holds only while there is exactly one copy of
ky in the module graph, and it is unavailable under the ESM mock the
app's tests run against.
Rebasing onto #3063 landed alert-level `displayName` and `tags`, which supersede how this branch labelled inline alerts. Adopt them rather than keeping a parallel scheme: - Drop the hand-rolled "Alert name" field added to TileAlertEditor. That editor now renders the shared AlertDisplayFields (name + tags) for every alert, so the inline create flow gets the same UX as the others and writes the field the alerts page actually reads. - Stop forcing `name` in buildInlineAlertPayload. `name` is the notification title template, not the label; the server already derives an inline alert's displayName from its chart config, so sending one would freeze a copy that stops tracking the chart's name. - Round-trip displayName/tags through the inline edit modal, and mirror the server's inline derivation in getDerivedAlertDisplayName so the name field's placeholder previews it. - Drop the obsolete getAlertDisplayName branch; AlertDetails reads alert.displayName now. Also extracts toFormAlertChannels: both edit modals normalized the response's loose channel shape onto the form's strict one with the same cast and the same comment. One copy, one cast — which is what takes the as-any ratchet down two.
#3093 added a `mockUseSource` handle read back through `requireMock`, which is already typed as the mocked shape. Point `mockUseSourceData` at it instead of casting a `{ data }` stub to a full `UseQueryResult`, which drops the eslint-disable that cast needed.
|
Rebased onto Checked the one place the two features could actually collide: #3093 disables its "Apply filters" toggle when an alert is configured, and its tests render a config that has one. The Also folded in
|
db5a132 to
7f946ba
Compare
| name: alert.name ?? null, | ||
| // The alerts page reads displayName, so an edit has to round-trip it | ||
| // (and the tags) rather than dropping back to a derived name. | ||
| displayName: alert.displayName ?? null, |
There was a problem hiding this comment.
🟠 major — Editing an inline alert freezes its server-derived name onto the alert document
GET /alerts/:id returns a resolved displayName — packages/api/src/routers/api/alerts.ts:49 spreads resolveAlertDisplayFields, which is stored ?? derived ?? 'Alert' (packages/api/src/utils/alerts.ts:163), and AlertsPageItem.displayName is a required z.string(), so it is never null. Seeding the editor from it and round-tripping it through buildInlineAlertPayload therefore persists the derived value: create an inline alert on a chart named "Error rate" without naming the alert, open Edit alert, change only the threshold, save — the alert now stores displayName: "Error rate" and stops tracking the chart's name (with a blank chart name it stores the literal "Alert"). This is exactly what the doc comment on buildInlineAlertPayload (packages/app/src/utils/alerts.ts:347) says must not happen. Seed the field from the stored value — either expose it separately on the detail response, or null it out here when it equals getDerivedAlertDisplayName(alert) (or the 'Alert' fallback) — and pass the derived name to AlertDisplayFields as the placeholder instead. EditAlertModal.alertToFormValues:114 has the same latent problem for tile alerts, so the fix belongs in both.
There was a problem hiding this comment.
Good catch on the round-trip, but I don't think it changes anything stored, and I'd rather not fix it here.
The server already saves the derived name when an alert is created without one (makeAlert, controllers/alerts.ts:264), and #3063 describes that as intended. So an inline alert on "Error rate" already has displayName: "Error rate" in Mongo before anyone opens Edit. Saving a threshold change writes the same value back — nothing is frozen that wasn't already.
Prefilling the resolved name is also how #3063 built the other editors: EditAlertModal seeds displayName: alert.displayName, and the dashboard tile editor gets the resolved value via withResolvedDisplayFields. The inline editor just matches them. Making it blank-with-placeholder instead would need the API to expose stored vs. derived separately, and would make only the inline editor behave differently. If we want that model, it should be a change across all three sources — happy to open a ticket.
Where you're right: the doc comment on buildInlineAlertPayload claims a "stops tracking the chart's name" hazard that doesn't exist. I'll rewrite it in the follow-up commit.
| }; | ||
| } | ||
|
|
||
| if (alert.source === AlertSource.INLINE && alert.chartConfig) { |
There was a problem hiding this comment.
🔵 minor — Unreachable inline branch in alertToFormValues
AlertRowMenu (packages/app/src/components/alerts/AlertRowMenu.tsx:200) is the only caller of EditAlertModal and routes every AlertSource.INLINE alert to EditInlineAlertModal, so this branch can never run; it also only guards the case where chartConfig is present, which is precisely the case that can't reach here (a list row has no chartConfig and would still fall through to the saved-search branch). Drop it rather than maintaining a second alert→form-values mapping that has to stay in sync with inlineAlertToChartConfig.
An inline alert has no dashboard tile or saved search behind it, so the shared display fields were describing a parent that isn't there: the tags button read "Inherited" and the name field offered to derive one. - Tags show the count (0 when unset) rather than "Inherited". - The name is required, marked with an asterisk and placeheld "Alert name" instead of "Defaults to the saved search or dashboard tile name". The server still derives a name from the chart config for API and MCP callers that omit one; only the editor insists. Both are driven off `dashboardId == null` in TileAlertEditor, which is already what separates an inline alert from a tile one, and the requirement is enforced in validateChartForm alongside the other alert checks so it covers the explorer and the edit modal in one place. Also rewrites the buildInlineAlertPayload comment, which claimed sending a displayName would stop it tracking the chart's name. It wouldn't -- the server persists the derived value on create either way.
|
Both done in Tags now show the count, so an unset list on the chart explorer reads Both are driven off Note the server still derives a name from the chart config for API and MCP callers that omit one ( |
| const errors = validateChartForm(form, tableSource, setError); | ||
| const errors = validateChartForm(form, tableSource, setError, { | ||
| // An inline alert has no tile to inherit a name from. | ||
| requireAlertDisplayName: alertsEnabled && dashboardId == null, |
There was a problem hiding this comment.
🟠 major — Requiring an alert name inside validateAndNormalize blocks Run/auto-run in the chart explorer
validateAndNormalize is shared by the save paths and by onSubmit — the Run button (ChartActionBar.tsx:203), the granularity-change effect (line 668), the display-type-change effect (line 719) and the autoRun mount submit (line 601). So once requireAlertDisplayName is true, every query run fails while the alert has no name. On /chart this is the normal path: DEFAULT_CHART_CONFIG has name: '' (ChartUtils.tsx:79), so Add Alert (ChartEditorControls.tsx:501) seeds no displayName, and the next Run pops "Invalid Chart — Alert name is required" and leaves the chart stale (the auto-submits suppress the notification, so those just silently stop re-running). Move the requireAlertDisplayName check out of validateAndNormalize into handleSaveAlert/handleSave only, so it gates saving without gating running the query.
| onTimeRangeSelect={onTimeRangeSelect} | ||
| submitRef={submitRef} | ||
| autoRun | ||
| enableAlerts |
There was a problem hiding this comment.
🔵 minor — Explorer still offers "Save to dashboard" while the chart carries an alert, duplicating it onto a tile
DBChartPage leaves showSaveToDashboard unset, so ChartActionBar falls back to !dashboardId and keeps the menu. SaveToDashboardModal writes config: chartConfig verbatim (SaveToDashboardModal.tsx:87) and the dashboard controller creates a tile alert from tile.config.alert (packages/api/src/controllers/dashboard.ts:24). Sequence: add an alert, name it, Run (which writes the alert into the URL config via setChartConfig), then Save to dashboard and Create alert — two alert documents now fire on the same query. This is exactly the hazard EditInlineAlertModal guards with showSaveToDashboard={false}. Either pass showSaveToDashboard={!chartConfig.alert} here, or strip alert from the config handed to SaveToDashboardModal.
| <Text size="xxs" opacity={0.5} mb={4}> | ||
| Name | ||
| {displayNameRequired && ( | ||
| <Text span c="red" ms={2} aria-hidden="true"> |
There was a problem hiding this comment.
🔵 minor — Required-name marker uses a raw palette color and is hidden from assistive tech
<Text span c="red"> breaks the REQUIRED semantic-variant rule in the repo conventions ("don't reach for raw palette colors for semantic status text" — use <Text span variant="danger">, which is token-driven and theme-aware). Separately, the asterisk is aria-hidden and the TextInput below carries no required/aria-required, so a screen-reader user gets no indication the field is mandatory until the submit-time error fires; add required (or aria-required) to the TextInput in the displayNameRequired case.
| * Falls back for anything unrecognised, so a caller always has something to | ||
| * show. Async because reading the body is. | ||
| */ | ||
| export async function getApiErrorMessage( |
There was a problem hiding this comment.
🔵 minor — getApiErrorMessage duplicates error-body extraction that already exists inline in several call sites
The { message } branch is the same operation already hand-rolled at packages/app/src/components/TeamSettings/WebhooksSection.tsx:54 ((await e.response.json())?.message), TeamSettings/WebhookForm.tsx:191 and :274, TeamSettings/TeamMembersSection.tsx:76 and :133, and ConnectionForm.tsx:107. The new helper is the better version (it also handles the zod-express-middleware array shape), but leaving the six copies in place means the app now has seven implementations of "read the API's reason out of a ky error". Migrate those call sites to getApiErrorMessage so there's one.
2 participants
Summary
HDX-5090's backend (#3010) added
AlertSource.INLINE— alerts that persist their ownchartConfigand evaluate with no saved search or dashboard tile behind them. The internal/alertsAPI accepts and returns them, but no UI could create or edit one. This is that UI.The motivating case is Epidemic Sound migrating 1000+ Grafana rules: today every alert needs a saved search (logs) or a dashboard tile (metrics) created first, which does not scale.
Chart explorer (
/chart)enableAlertsprop rather than on being inside a dashboard (previouslydashboardId != null), and a Create alert action in the action bar POSTs{ source: 'inline', chartConfig, ...alertFields }. Works for builder configs (log/trace/metric) and raw SQL, on Line / Stacked bar / Number — the display types the evaluator supports.AlertDisplayFields(name + tags) that feat: Persist alert-level displayName and tags #3063 added to the alert editor, so an inline alert is labelled the same way as any other. Left blank, the server derives the name from the chart config — it already handlesAlertSource.INLINEinderiveAlertDisplayFields.Alerts list
/chart?alertId=<id>, which the explorer resolves into the persisted query. By id rather than by an inlined config becauseGET /alertsomitschartConfig— a config-carrying link would leave every list row without one whenever the alert detail page is disabled. It also opens the query as it stands now rather than a snapshot. (The notification link still inlines the config: it is built server-side, with no session to fetch with, and lands on the same chart.)href. That also covers a pre-existing case: a tile alert whose dashboard was deleted, with the details page disabled.Alert detail page
chartConfig. The tile variant's config assembly is extracted into a sharedbuildAlertChartConfig, so the two previews cannot drift.EditInlineAlertModal), so the query and the alert's threshold/channels/schedule are changed in one place, then split apart again on save. The remove-alert control is hidden there, and "Save to dashboard" is suppressed — saving that chart as a tile would copy its alert onto the tile, leaving two alerts on one query.GET /alertsdeliberately omitschartConfig, so an alert opened from a list row fetches its detail response before seeding the editor.EditAlertModalkeeps handling saved-search and tile alerts. An inline alert routed there would previously have fallen through to the saved-search branch and been rewritten as a saved-search alert with no saved search; it now round-trips its config instead.Not in scope (matching the backend PR): External API v2, MCP write support, and Terraform for inline alerts.
Screenshots or video
How to test on Vercel preview
N/A — the preview runs in
LOCAL_MODE, which has no API server behind it, and alert creation is gated off there (!IS_LOCAL_MODE). Exercise this against a full-stack dev server:/chart, pick a source, and build a Time series or Number chart./alerts— the alert is listed with a chart icon, and its name links back to the explorer with the same query. Works withNEXT_PUBLIC_ENABLE_ALERT_DETAILSboth on and off; with it on, the detail page header and the row menu's Open chart use the same link.NEXT_PUBLIC_ENABLE_ALERT_DETAILS) — the query chart renders with the threshold line. Edit alert opens the chart editor; change the threshold and the query, save, and confirm both persist.References
Testing done:
make ci-lint,make ci-unit— pass (3637 app tests). New unit tests cover the inline-alert helpers, the explorer's alert gating / name field / save payload, the?alertId=seeding hook, the detail-chart config builder, and the inline edit modal's seeding, PUT payload, and lazy fetch.Note: the ticket calls the source
AlertSource.CHART; the merged backend named itAlertSource.INLINE, which is what this follows.