Skip to content

Commit ffc9075

Browse files
kathiekiwiTrigger.dev RepoOps
authored andcommitted
fix(webapp): accept aggregate-alias expressions in query alerts and explain unwatchable queries
Query alerts now support values derived from aggregate aliases, and the Watch button explains why a query can't be watched instead of disappearing. Mono-RevId: 464572b7dd33a78b65ec109d2dad65d271ee29d1
1 parent e92bf68 commit ffc9075

11 files changed

Lines changed: 344 additions & 16 deletions

File tree

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
area: webapp
3+
type: improvement
4+
---
5+
6+
Query alerts now accept queries whose columns are computed from other aggregate columns, such as a percentage of two aggregates. When a query can't be watched, the Watch button stays visible but disabled and explains why.

‎apps/webapp/app/components/alerts/WatchQueryButton.dom.test.ts‎

Lines changed: 38 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -100,13 +100,45 @@ describe("WatchQueryButton", () => {
100100
expect(container.textContent).toBe("");
101101
});
102102

103-
it("renders nothing when the analysis is ok but has no modes", () => {
103+
it("disables the button and shows the first reason when the query can't be watched", () => {
104+
const reason = "Remove time buckets from GROUP BY.";
104105
render({
105106
alertsPath: "/alerts",
106107
period: "7d",
107-
watch: { query: "SELECT 1", analysis: analysis({}) },
108+
watch: {
109+
query: "SELECT 1",
110+
analysis: analysis({
111+
ok: false,
112+
unsupported: [
113+
{ mode: "threshold", code: "group_by_time", message: reason },
114+
{ mode: "absence", code: "group_by_time", message: reason },
115+
{ mode: "match", code: "aggregated", message: "Other reason." },
116+
],
117+
}),
118+
},
108119
});
109-
expect(container.textContent).toBe("");
120+
const button = container.querySelector<HTMLButtonElement>("[data-testid=watch-query] button")!;
121+
expect(button.textContent).toBe("Watch");
122+
expect(button.getAttribute("aria-disabled")).toBe("true");
123+
expect(button.tabIndex).toBe(0);
124+
expect(container.querySelector("a")).toBeNull();
125+
const describedBy = button.getAttribute("aria-describedby")!;
126+
expect(document.getElementById(describedBy)!.textContent).toBe(reason);
127+
expect(document.querySelector("[role=tooltip]")).toBeNull();
128+
act(() => button.focus());
129+
expect(document.querySelector("[role=tooltip]")!.textContent).toBe(reason);
130+
});
131+
132+
it("enables the button for a supported query", () => {
133+
render({
134+
alertsPath: "/alerts",
135+
period: "7d",
136+
watch: { query: "SELECT count() AS failures FROM runs", analysis: THRESHOLD },
137+
});
138+
expect(
139+
container.querySelector<HTMLAnchorElement>("[data-testid=watch-query] a")
140+
).not.toBeNull();
141+
expect(container.querySelector("button")).toBeNull();
110142
});
111143

112144
it("links to the alerts page with the query as base64url (catches dropped encoding)", () => {
@@ -168,11 +200,12 @@ describe("WatchQueryButton", () => {
168200
form.remove();
169201
});
170202

171-
it("renders nothing when the encoded query exceeds 8 KB even though it has fewer characters", () => {
203+
it("disables the button when the encoded query exceeds 8 KB even though it has fewer characters", () => {
172204
const query = `SELECT count() AS failures FROM runs -- ${"é".repeat(3500)}`;
173205
expect(query.length).toBeLessThan(8192);
174206
render({ alertsPath: "/alerts", period: "1h", watch: { query, analysis: THRESHOLD } });
175-
expect(container.textContent).toBe("");
207+
expect(container.querySelector("button")!.getAttribute("aria-disabled")).toBe("true");
208+
expect(container.querySelector("a")).toBeNull();
176209
});
177210

178211
it("renders nothing when the analysis failed", () => {

‎apps/webapp/app/components/alerts/WatchQueryButton.tsx‎

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
11
import { BellAlertIcon } from "@heroicons/react/20/solid";
2-
import { Button, LinkButton } from "~/components/primitives/Buttons";
2+
import { Button, ButtonContent, LinkButton } from "~/components/primitives/Buttons";
33
import {
44
watchBlockedReasons,
55
watchQueryChoice,
66
watchQueryHref,
77
} from "~/components/alerts/queryAlertLink";
8+
import { SimpleTooltip } from "~/components/primitives/Tooltip";
89
import { useDashboardAgent } from "~/components/dashboard-agent/dashboardAgentLauncher";
910
import type { QueryAlertAnalysis } from "~/v3/services/alerts/queryAlerts/analyzeQueryForAlert.server";
1011

@@ -21,8 +22,38 @@ export function WatchQueryButton({
2122
}) {
2223
const agent = useDashboardAgent();
2324
const analysis = watch?.analysis;
24-
if (!watch || !analysis?.ok || watchBlockedReasons(analysis, watch.query).length > 0) {
25-
return null;
25+
if (!watch || !analysis) return null;
26+
const [reason] = watchBlockedReasons(analysis, watch.query);
27+
if (reason) {
28+
return (
29+
<span data-testid="watch-query">
30+
<SimpleTooltip
31+
asChild
32+
tabbable
33+
button={
34+
<button
35+
type="button"
36+
aria-disabled="true"
37+
aria-describedby="watch-query-reason"
38+
className="group/button cursor-default opacity-60 outline-hidden focus-custom"
39+
onClick={(e) => e.preventDefault()}
40+
>
41+
<ButtonContent
42+
variant="secondary/small"
43+
LeadingIcon={BellAlertIcon}
44+
leadingIconClassName="text-alerts"
45+
>
46+
Watch
47+
</ButtonContent>
48+
</button>
49+
}
50+
content={reason}
51+
/>
52+
<span id="watch-query-reason" className="sr-only">
53+
{reason}
54+
</span>
55+
</span>
56+
);
2657
}
2758
return (
2859
<span data-testid="watch-query">

‎apps/webapp/app/components/alerts/queryAlertLink.ts‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
1+
import { getQuerySource } from "@internal/alert-contracts";
12
import type { QueryAlertAnalysis } from "~/v3/services/alerts/queryAlerts/analyzeQueryForAlert.server";
23
import { parsePeriodToMs } from "~/utils/periods";
34

45
export const MAX_WATCH_QUERY_LENGTH = 8192;
56
const MAX_DEFAULT_WINDOW_MINUTES = 24 * 60;
67
export const UNUSABLE_QUERY = "This query can't be used for an alert.";
78

9+
const MODE_MISMATCH_CODES = new Set(["aggregated", "rows_need_aggregate"]);
10+
811
export type QueryAlertMode = "threshold" | "absence" | "match";
912

1013
export type QueryAlertPrefill = {
@@ -43,7 +46,15 @@ export function watchBlockedReasons(analysis: QueryAlertAnalysis, query: string)
4346
if (encodeQueryParam(query).length > MAX_WATCH_QUERY_LENGTH)
4447
return ["Query is too long to watch"];
4548
if (analysis.ok && analysis.modes.length > 0) return [];
46-
const messages = [...analysis.errors, ...analysis.unsupported].map((issue) => issue.message);
49+
const matchable = analysis.sources.some((s) =>
50+
getQuerySource(s.sourceId)?.modes.includes("match")
51+
);
52+
const issues = [
53+
...analysis.errors,
54+
...analysis.unsupported.filter((issue) => matchable || issue.mode !== "match"),
55+
];
56+
const specific = issues.filter((issue) => !MODE_MISMATCH_CODES.has(issue.code));
57+
const messages = (specific.length ? specific : issues).map((issue) => issue.message);
4758
const unique = [...new Set(messages)].slice(0, 2);
4859
return unique.length ? unique : [UNUSABLE_QUERY];
4960
}

‎apps/webapp/app/components/primitives/Buttons.tsx‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -389,13 +389,32 @@ export function ButtonContent(props: ButtonContentPropsType) {
389389

390390
type ButtonPropsType = Pick<
391391
JSX.IntrinsicElements["button"],
392-
"type" | "disabled" | "onClick" | "name" | "value" | "form" | "autoFocus" | "aria-label"
392+
| "type"
393+
| "disabled"
394+
| "onClick"
395+
| "name"
396+
| "value"
397+
| "form"
398+
| "autoFocus"
399+
| "aria-label"
400+
| "aria-describedby"
393401
> &
394402
React.ComponentProps<typeof ButtonContent>;
395403

396404
/* oxlint-disable react/button-has-type -- Callers can select button, reset, or submit semantics. */
397405
export const Button = forwardRef<HTMLButtonElement, ButtonPropsType>(
398-
({ type, disabled, autoFocus, onClick, "aria-label": ariaLabel, ...props }, ref) => {
406+
(
407+
{
408+
type,
409+
disabled,
410+
autoFocus,
411+
onClick,
412+
"aria-label": ariaLabel,
413+
"aria-describedby": ariaDescribedBy,
414+
...props
415+
},
416+
ref
417+
) => {
399418
const innerRef = useRef<HTMLButtonElement>(null);
400419
useImperativeHandle(ref, () => innerRef.current as HTMLButtonElement);
401420

@@ -425,6 +444,7 @@ export const Button = forwardRef<HTMLButtonElement, ButtonPropsType>(
425444
form={props.form}
426445
autoFocus={autoFocus}
427446
aria-label={ariaLabel}
447+
aria-describedby={ariaDescribedBy}
428448
>
429449
<ButtonContent
430450
{...props}

‎apps/webapp/app/v3/services/alerts/queryAlerts/analyzeQueryForAlert.server.ts‎

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,10 @@ import {
1111
ExposedTSQLError,
1212
measureTSQLNesting,
1313
parseTSQLSelect,
14+
type Alias,
1415
type Call,
1516
type Expression,
17+
type Field,
1618
type OutputColumnMetadata,
1719
type SelectQuery,
1820
type TableSchema,
@@ -26,8 +28,10 @@ import {
2628
branchesOf,
2729
containsAggregate,
2830
containsWindowFunction,
31+
fieldsIn,
2932
fromEntries,
3033
hasFreeField,
34+
isStar,
3135
isField,
3236
itemName,
3337
lastName,
@@ -465,7 +469,30 @@ function analyzeShape(
465469

466470
const query = branches[0]!;
467471
const items = query.select;
468-
const aggregateItem = items.map((item) => containsAggregate(item) && !hasFreeField(item));
472+
const visible = visibleColumns(query, schemas, tables);
473+
const aliases = items.flatMap((item) =>
474+
item.expression_type === "alias" ? [(item as Alias).alias.toLowerCase()] : []
475+
);
476+
const ambiguous = new Set(aliases.filter((a, i) => aliases.indexOf(a) !== i));
477+
const aggregateAliases = new Set<string>();
478+
const isAggregateRef = (f: Field) => {
479+
const name = String(f.chain[0]);
480+
return f.chain.length === 1 && !visible.has(name) && aggregateAliases.has(name.toLowerCase());
481+
};
482+
const isAggregateItem = (item: Expression) =>
483+
(containsAggregate(item) || fieldsIn(item).some(isAggregateRef)) &&
484+
!hasFreeField(item, isAggregateRef);
485+
for (let grew = true; grew;) {
486+
grew = false;
487+
for (const item of items) {
488+
const alias = item.expression_type === "alias" ? (item as Alias).alias.toLowerCase() : "";
489+
if (alias && !ambiguous.has(alias) && !aggregateAliases.has(alias) && isAggregateItem(item)) {
490+
aggregateAliases.add(alias);
491+
grew = true;
492+
}
493+
}
494+
}
495+
const aggregateItem = items.map(isAggregateItem);
469496
const grouped = !!query.group_by?.length;
470497

471498
if (!grouped && aggregateItem.some((a) => !a)) {
@@ -529,6 +556,22 @@ function analyzeShape(
529556
};
530557
}
531558

559+
function visibleColumns(
560+
query: SelectQuery,
561+
schemas: Map<string, TableSchema>,
562+
tables: string[]
563+
): Set<string> {
564+
const columnsOf = (table: string) => Object.keys(schemas.get(table)?.columns ?? {});
565+
return new Set(
566+
fromEntries(query).flatMap((entry) => {
567+
if (entry.kind === "table") return columnsOf(entry.table);
568+
const select = branchesOf(entry.query)[0]?.select ?? [];
569+
if (select.some(isStar)) return tables.flatMap(columnsOf);
570+
return select.flatMap((item) => itemName(item) ?? []);
571+
})
572+
);
573+
}
574+
532575
function groupsByTime(
533576
query: SelectQuery,
534577
schemas: Map<string, TableSchema>,

0 commit comments

Comments
 (0)