From aeb8584045f1e432e6aa5b0af1e4836568e0b360 Mon Sep 17 00:00:00 2001 From: davethegut <87487318+davethegut@users.noreply.github.com> Date: Tue, 7 Jul 2026 11:50:35 -0400 Subject: [PATCH 1/2] Fix Alert Triage widget rendering blank on array-shaped alert fields Some ECS alert fields (host.name, user.name, process.name, source.ip, destination.ip) come back as arrays instead of scalars for certain correlated/network-derived alerts, even though SecurityAlert._source types them as scalars. The Alert Triage view's group/sort logic assumed a scalar and threw a TypeError on .localeCompare() when it got an array, and with no error boundary anywhere in the view tree, the exception silently unmounted the whole widget to a blank panel. Normalize array-shaped fields to scalars at the single chokepoint that feeds both the triage-alerts and poll-alerts tool paths (AlertsService.getAlerts), add defense-in-depth guards in useAlertSort's sort/group logic, and add a reusable ErrorBoundary around the view so any future unexpected shape shows a visible fallback instead of a silent blank screen. Fixes #47 Co-Authored-By: Claude Sonnet 5 --- src/elastic/service/alertsService.test.ts | 86 ++++++++++++++++++ src/elastic/service/alertsService.ts | 54 ++++++++++- src/elastic/service/entityDetailService.ts | 7 +- .../ErrorBoundary/ErrorBoundary.tsx | 55 +++++++++++ src/shared/components/index.ts | 3 + src/shared/field-utils.test.ts | 27 ++++++ src/shared/field-utils.ts | 25 +++++ .../alert-triage/hooks/useAlertSort.test.ts | 91 +++++++++++++++++++ src/views/alert-triage/hooks/useAlertSort.ts | 11 ++- src/views/alert-triage/mcp-app.tsx | 7 +- 10 files changed, 358 insertions(+), 8 deletions(-) create mode 100644 src/shared/components/ErrorBoundary/ErrorBoundary.tsx create mode 100644 src/shared/field-utils.test.ts create mode 100644 src/shared/field-utils.ts create mode 100644 src/views/alert-triage/hooks/useAlertSort.test.ts diff --git a/src/elastic/service/alertsService.test.ts b/src/elastic/service/alertsService.test.ts index 10e447b..a051a17 100644 --- a/src/elastic/service/alertsService.test.ts +++ b/src/elastic/service/alertsService.test.ts @@ -193,6 +193,92 @@ describe("AlertsService", () => { alerts: [], }); }); + + // Regression test for a real bug: some ECS alert fields (host.name, + // user.name/domain, process.name/executable, source.ip, destination.ip) + // come back from Elasticsearch as arrays instead of scalars for certain + // correlated/network-derived alerts, even though `SecurityAlert._source` + // types them as scalars. Downstream sort/group logic in the Alert Triage + // view (useAlertSort.ts) assumed a scalar and threw on `.localeCompare()` + // when it got an array, crashing the whole widget to a blank panel with + // no error boundary to catch it. getAlerts() must normalize these fields + // before returning so every caller sees the scalar shape the type promises. + it("normalizes array-shaped ECS entity/IP fields to scalars", async () => { + const alertsClient = createMockAlertsClient(); + vi.mocked(alertsClient.searchAlerts).mockResolvedValueOnce({ + hits: { + total: { value: 1 }, + hits: [ + { + _id: "a1", + _index: ".alerts-security.alerts-default", + _source: { + "@timestamp": "2024-01-01T00:00:00Z", + "kibana.alert.rule.name": "R", + "kibana.alert.rule.uuid": "r", + "kibana.alert.severity": "low", + "kibana.alert.risk_score": 21, + "kibana.alert.workflow_status": "open", + "kibana.alert.reason": "x", + host: { name: ["sa-da-vm-lls-01"] }, + user: { name: ["root"], domain: ["CORP"] }, + process: { + name: ["python3.9"], + executable: ["/usr/bin/python3.9"], + parent: { name: ["bash"] }, + }, + source: { ip: ["10.132.0.108"] }, + destination: { ip: ["35.195.130.253"] }, + } as unknown as SecurityAlert["_source"], + }, + ], + }, + }); + + const service = new AlertsService({ alertsClient }); + const out = await service.getAlerts(); + + expect(out.alerts).toHaveLength(1); + const src = out.alerts[0]._source; + expect(src.host?.name).toBe("sa-da-vm-lls-01"); + expect(src.user?.name).toBe("root"); + expect(src.user?.domain).toBe("CORP"); + expect(src.process?.name).toBe("python3.9"); + expect(src.process?.executable).toBe("/usr/bin/python3.9"); + expect(src.process?.parent?.name).toBe("bash"); + expect(src.source?.ip).toBe("10.132.0.108"); + expect(src.destination?.ip).toBe("35.195.130.253"); + }); + + it("leaves already-scalar ECS entity fields untouched", async () => { + const alertsClient = createMockAlertsClient(); + vi.mocked(alertsClient.searchAlerts).mockResolvedValueOnce({ + hits: { + total: { value: 1 }, + hits: [ + { + _id: "a2", + _index: ".alerts-security.alerts-default", + _source: { + "@timestamp": "2024-01-01T00:00:00Z", + "kibana.alert.rule.name": "R", + "kibana.alert.rule.uuid": "r", + "kibana.alert.severity": "high", + "kibana.alert.risk_score": 47, + "kibana.alert.workflow_status": "open", + "kibana.alert.reason": "x", + host: { name: "sa-da-ingest-01" }, + }, + }, + ], + }, + }); + + const service = new AlertsService({ alertsClient }); + const out = await service.getAlerts(); + + expect(out.alerts[0]._source.host?.name).toBe("sa-da-ingest-01"); + }); }); describe("getAlertContext", () => { diff --git a/src/elastic/service/alertsService.ts b/src/elastic/service/alertsService.ts index c907a85..49830c8 100644 --- a/src/elastic/service/alertsService.ts +++ b/src/elastic/service/alertsService.ts @@ -13,6 +13,7 @@ import type { SecurityAlert, } from "../../shared/types.js"; import type { AlertsClient } from "../client/alertsClient.js"; +import { toScalar } from "../../shared/field-utils.js"; /** Time window (ms) on either side of an alert used to gather host context. */ const CONTEXT_WINDOW_MS = 5 * 60 * 1000; @@ -116,7 +117,7 @@ export class AlertsService { name: b.key, count: b.doc_count, })), - alerts: response.hits.hits, + alerts: response.hits.hits.map(normalizeAlertEntityFields), }; } @@ -242,6 +243,57 @@ export class AlertsService { } } +/** + * Some ECS fields (`host.name`, `user.name`, `process.name`, `source.ip`, ...) + * come back as arrays instead of scalars for certain correlated/network- + * derived alerts, even though {@link SecurityAlert}'s `_source` types them as + * scalars — Elasticsearch does not enforce that shape at the document level. + * Downstream code (sorting/grouping in the Alert Triage view) assumes a + * scalar and throws (`TypeError: ... .localeCompare is not a function`) if it + * gets an array, so normalize once here at the boundary rather than + * defensively at every read site. + * + * The `Record` reads below reflect reality: this is raw JSON + * off the wire, not yet as trustworthy as the `SecurityAlert` type the rest of + * the codebase assumes. `host.ip` is intentionally left untouched — it's + * already typed and consumed as `string[]`. + */ +function normalizeAlertEntityFields(alert: SecurityAlert): SecurityAlert { + const src = alert._source as Record; + const scalar = (value: unknown): string | undefined => + toScalar(value as string | string[] | undefined); + + const host = src.host as Record | undefined; + const user = src.user as Record | undefined; + const proc = src.process as Record | undefined; + const parent = proc?.parent as Record | undefined; + const file = src.file as Record | undefined; + const source = src.source as Record | undefined; + const destination = src.destination as Record | undefined; + + return { + ...alert, + _source: { + ...alert._source, + host: host ? { ...host, name: scalar(host.name) } : host, + user: user ? { ...user, name: scalar(user.name), domain: scalar(user.domain) } : user, + process: proc + ? { + ...proc, + name: scalar(proc.name), + executable: scalar(proc.executable), + parent: parent + ? { ...parent, name: scalar(parent.name), executable: scalar(parent.executable) } + : parent, + } + : proc, + file: file ? { ...file, name: scalar(file.name), path: scalar(file.path) } : file, + source: source ? { ...source, ip: scalar(source.ip) } : source, + destination: destination ? { ...destination, ip: scalar(destination.ip) } : destination, + } as SecurityAlert["_source"], + }; +} + function buildHostEventQuery( hostName: string, timeRange: TimeRange diff --git a/src/elastic/service/entityDetailService.ts b/src/elastic/service/entityDetailService.ts index 10e92d4..f3945a4 100644 --- a/src/elastic/service/entityDetailService.ts +++ b/src/elastic/service/entityDetailService.ts @@ -7,6 +7,7 @@ import type { EntityDetailClient } from "../client/entityDetailClient.js"; import type { EntityDetail } from "../client/entityDetailClient.js"; +import { toScalar } from "../../shared/field-utils.js"; interface EntityDetailServiceOptions { readonly entityDetailClient: EntityDetailClient; @@ -184,11 +185,11 @@ export class EntityDetailService { if (firstProc) { const os = firstProc["host.os.name"] || firstProc["host.os.platform"]; if (os) fields.push({ label: "OS", value: String(os) }); - const ips = firstProc["host.ip"]; - if (ips) { + const ip = toScalar(firstProc["host.ip"] as string | string[] | undefined); + if (ip) { fields.push({ label: "IP", - value: String(Array.isArray(ips) ? ips[0] : ips), + value: String(ip), mono: true, }); } diff --git a/src/shared/components/ErrorBoundary/ErrorBoundary.tsx b/src/shared/components/ErrorBoundary/ErrorBoundary.tsx new file mode 100644 index 0000000..4251a56 --- /dev/null +++ b/src/shared/components/ErrorBoundary/ErrorBoundary.tsx @@ -0,0 +1,55 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import React from "react"; +import { EmptyState } from "../States/States"; + +export interface ErrorBoundaryProps { + /** Replaces the default fallback message. */ + fallback?: React.ReactNode; + children: React.ReactNode; +} + +interface ErrorBoundaryState { + error: Error | null; +} + +/** + * Catches render-time exceptions anywhere in `children` and shows a fallback + * instead of letting React unmount the whole tree to a blank screen. + * + * MCP app views render inside a host-provided webview with no dev console + * visible to the end user — an uncaught render error otherwise looks + * identical to "nothing loaded," with no indication anything went wrong. + * Wrap each view's root render in this boundary so an unexpected data shape + * degrades to a visible message instead of a silent blank panel. + */ +export class ErrorBoundary extends React.Component { + state: ErrorBoundaryState = { error: null }; + + static getDerivedStateFromError(error: Error): ErrorBoundaryState { + return { error }; + } + + componentDidCatch(error: Error, info: React.ErrorInfo): void { + console.error("Unhandled render error:", error, info.componentStack); + } + + render(): React.ReactNode { + if (this.state.error) { + return ( + this.props.fallback ?? ( + + Something went wrong rendering this view. Try reloading — if it keeps + happening, the underlying data may be in an unexpected shape. + + ) + ); + } + return this.props.children; + } +} diff --git a/src/shared/components/index.ts b/src/shared/components/index.ts index 610c28d..350c607 100644 --- a/src/shared/components/index.ts +++ b/src/shared/components/index.ts @@ -41,6 +41,9 @@ export type { KpiStripProps, KpiTileProps, KpiAccent } from "./KpiStrip/KpiStrip export { LoadingState, EmptyState } from "./States/States"; export type { LoadingStateProps, EmptyStateProps } from "./States/States"; +export { ErrorBoundary } from "./ErrorBoundary/ErrorBoundary"; +export type { ErrorBoundaryProps } from "./ErrorBoundary/ErrorBoundary"; + export { DetailPane } from "./DetailPane/DetailPane"; export type { DetailPaneProps } from "./DetailPane/DetailPane"; diff --git a/src/shared/field-utils.test.ts b/src/shared/field-utils.test.ts new file mode 100644 index 0000000..a73580a --- /dev/null +++ b/src/shared/field-utils.test.ts @@ -0,0 +1,27 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import { describe, expect, it } from "vitest"; +import { toScalar } from "./field-utils.js"; + +describe("toScalar", () => { + it("returns a scalar value unchanged", () => { + expect(toScalar("sa-da-vm-lls-01")).toBe("sa-da-vm-lls-01"); + }); + + it("returns the first element of a non-empty array", () => { + expect(toScalar(["sa-da-vm-lls-01", "sa-da-ingest-01"])).toBe("sa-da-vm-lls-01"); + }); + + it("returns undefined for an empty array", () => { + expect(toScalar([])).toBeUndefined(); + }); + + it("returns undefined for undefined", () => { + expect(toScalar(undefined)).toBeUndefined(); + }); +}); diff --git a/src/shared/field-utils.ts b/src/shared/field-utils.ts new file mode 100644 index 0000000..326c3a0 --- /dev/null +++ b/src/shared/field-utils.ts @@ -0,0 +1,25 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +/** + * Coerce an ECS field that may come back as either a scalar or an array (a + * multi-valued field, common on correlated/network-derived alerts) into a + * single scalar. + * + * Elasticsearch does not enforce single- vs. multi-valued fields at the + * document level, so the *same* mapped field can be a plain string on one + * document and a `string[]` on another. Code that expects a scalar (e.g. + * grouping/sorting by `host.name`) must normalize at the boundary rather than + * assume the shape declared in a TypeScript type actually holds at runtime. + * + * @returns the first element for a non-empty array, `undefined` for an empty + * array, or the value unchanged if it isn't an array. + */ +export function toScalar(value: T | T[] | undefined): T | undefined { + if (!Array.isArray(value)) return value; + return value.length > 0 ? value[0] : undefined; +} diff --git a/src/views/alert-triage/hooks/useAlertSort.test.ts b/src/views/alert-triage/hooks/useAlertSort.test.ts new file mode 100644 index 0000000..4e73d40 --- /dev/null +++ b/src/views/alert-triage/hooks/useAlertSort.test.ts @@ -0,0 +1,91 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import { describe, expect, it } from "vitest"; +import { groupAlerts, sortAlerts } from "./useAlertSort.js"; +import type { SecurityAlert } from "../../../shared/types.js"; + +function makeAlert( + id: string, + overrides: Partial = {} +): SecurityAlert { + return { + _id: id, + _index: ".alerts-security.alerts-default", + _source: { + "@timestamp": "2024-01-01T00:00:00Z", + "kibana.alert.rule.name": "R", + "kibana.alert.rule.uuid": "r", + "kibana.alert.severity": "low", + "kibana.alert.risk_score": 21, + "kibana.alert.workflow_status": "open", + "kibana.alert.reason": "x", + ...overrides, + }, + }; +} + +// `AlertsService.getAlerts()` normalizes ECS entity fields to scalars before +// this hook ever sees them, but these tests exercise the hook's own +// defense-in-depth guards directly — in case a stray array-shaped field ever +// reaches it via a different path (e.g. a future bootstrap/mock data source). +const arrayShapedHost = { name: ["sa-da-vm-lls-01"] } as unknown as { name: string }; + +describe("sortAlerts", () => { + it("sorts by host without throwing when a host.name is an array", () => { + const alerts = [ + makeAlert("a1", { host: { name: "zzz-host" } }), + makeAlert("a2", { host: arrayShapedHost }), + ]; + + expect(() => sortAlerts(alerts, "host")).not.toThrow(); + }); +}); + +describe("groupAlerts", () => { + // Regression test: previously, an array-shaped host.name passed the + // `!key || !name` truthiness guard (a non-empty array is truthy), then + // crashed the group sort at `a.name.localeCompare(b.name)` — arrays have no + // `.localeCompare`. This exact crash rendered the Alert Triage widget as a + // blank panel with no error toast, against real alert data. + it("groups by host without throwing when one alert's host.name is an array", () => { + const alerts = [ + makeAlert("a1", { host: { name: "sa-da-ingest-01" } }), + makeAlert("a2", { host: arrayShapedHost }), + ]; + + expect(() => groupAlerts(alerts, "host")).not.toThrow(); + }); + + it("skips an alert whose host.name is an array rather than grouping it incorrectly", () => { + const alerts = [ + makeAlert("a1", { host: { name: "sa-da-ingest-01" } }), + makeAlert("a2", { host: arrayShapedHost }), + ]; + + const groups = groupAlerts(alerts, "host")!; + expect(groups).toHaveLength(1); + expect(groups[0].name).toBe("sa-da-ingest-01"); + expect(groups[0].alerts).toHaveLength(1); + }); + + it("groups normally when all host names are scalar strings", () => { + const alerts = [ + makeAlert("a1", { host: { name: "host-b" } }), + makeAlert("a2", { host: { name: "host-a" } }), + makeAlert("a3", { host: { name: "host-a" } }), + ]; + + const groups = groupAlerts(alerts, "host")!; + expect(groups.map((g) => g.name).sort()).toEqual(["host-a", "host-b"]); + expect(groups.find((g) => g.name === "host-a")!.alerts).toHaveLength(2); + }); + + it("returns null for groupBy 'none'", () => { + expect(groupAlerts([makeAlert("a1")], "none")).toBeNull(); + }); +}); diff --git a/src/views/alert-triage/hooks/useAlertSort.ts b/src/views/alert-triage/hooks/useAlertSort.ts index b07e84c..a426fbf 100644 --- a/src/views/alert-triage/hooks/useAlertSort.ts +++ b/src/views/alert-triage/hooks/useAlertSort.ts @@ -45,7 +45,8 @@ export function sortAlerts(alerts: SecurityAlert[], sortBy: SortKey): SecurityAl arr.sort((a, b) => (a._source["kibana.alert.rule.name"] || "").localeCompare(b._source["kibana.alert.rule.name"] || "")); break; case "host": - arr.sort((a, b) => (a._source.host?.name || "").localeCompare(b._source.host?.name || "")); + arr.sort((a, b) => + String(a._source.host?.name ?? "").localeCompare(String(b._source.host?.name ?? ""))); break; } return arr; @@ -74,7 +75,11 @@ export function groupAlerts(sortedAlerts: SecurityAlert[], groupBy: GroupKey): A key = name; subtitle = src.process?.executable || (src.process?.parent?.name ? `Parent ${src.process.parent.name}` : undefined); } - if (!key || !name) continue; + // `key`/`name` are expected to be scalar strings (normalized upstream in + // AlertsService.getAlerts()), but guard against a stray array slipping + // through — a non-empty array is truthy and would otherwise pass this + // check, then throw at the `.localeCompare()` call below. + if (typeof key !== "string" || !key || typeof name !== "string" || !name) continue; let bucket = buckets.get(key); if (!bucket) { bucket = { key, name, subtitle, topSeverity: "low", alerts: [] }; @@ -90,7 +95,7 @@ export function groupAlerts(sortedAlerts: SecurityAlert[], groupBy: GroupKey): A if (d !== 0) return d; const c = b.alerts.length - a.alerts.length; if (c !== 0) return c; - return a.name.localeCompare(b.name); + return String(a.name ?? "").localeCompare(String(b.name ?? "")); }); } diff --git a/src/views/alert-triage/mcp-app.tsx b/src/views/alert-triage/mcp-app.tsx index 7251dbf..be21f81 100644 --- a/src/views/alert-triage/mcp-app.tsx +++ b/src/views/alert-triage/mcp-app.tsx @@ -7,6 +7,11 @@ import React from "react"; import { createRoot } from "react-dom/client"; +import { ErrorBoundary } from "../../shared/components"; import { App } from "./App"; -createRoot(document.getElementById("root")!).render(); +createRoot(document.getElementById("root")!).render( + + + +); From 76a6dc777c630f414f169db935b1c47ec972b51a Mon Sep 17 00:00:00 2001 From: davethegut <87487318+davethegut@users.noreply.github.com> Date: Tue, 7 Jul 2026 14:16:40 -0400 Subject: [PATCH 2/2] Fix Alert Triage filter staying applied after clearing the search text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The search input's onChange only updated local text state — it never cleared the actual backend filter (paramsRef.current.query) unless the user pressed Enter, pressed Escape, or clicked the filter chip's "x". Deleting the text by any other means (select-and-delete, backspacing without pressing Enter) left the widget filtered on the old value even though the input box and chip both appeared empty. Fix handleInputChange to clear the filter immediately once the input becomes empty, matching the existing Escape/chip-click behavior. Also add a request-token guard in loadAlertsImpl so a slower, older response can never overwrite a fresher one — a related latent race that could cause the same "stuck filtered" symptom intermittently even via the correct clearing paths. Verified against a live local environment: confirmed the widget now reverts to the unfiltered list immediately when the filter text is cleared directly. Co-Authored-By: Claude Sonnet 5 --- src/views/alert-triage/App.test.tsx | 195 ++++++++++++++++++++++++++++ src/views/alert-triage/App.tsx | 35 ++++- 2 files changed, 227 insertions(+), 3 deletions(-) create mode 100644 src/views/alert-triage/App.test.tsx diff --git a/src/views/alert-triage/App.test.tsx b/src/views/alert-triage/App.test.tsx new file mode 100644 index 0000000..96aa0ae --- /dev/null +++ b/src/views/alert-triage/App.test.tsx @@ -0,0 +1,195 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import { describe, it, expect, vi } from "vitest"; +import { render, fireEvent, waitFor, act } from "@testing-library/react"; +import type { App as McpApp } from "@modelcontextprotocol/ext-apps"; +import { McpAppContext, type McpAppContextValue } from "../../shared/hooks/McpAppContext.js"; +import { ToastProvider } from "../../shared/components/index.js"; +import { AppContent } from "./App.js"; +import type { AlertSummary, SecurityAlert } from "../../shared/types.js"; + +/** Mirrors the real `App`'s wrapping (`ToastProvider` + `McpAppProvider`), + * swapping the real provider/transport for a stubbed context. */ +function renderAppContent(ctx: McpAppContextValue) { + return render( + + + + + + ); +} + +interface FakeApp { + callServerTool: ReturnType; +} + +function makeContext(app: FakeApp): McpAppContextValue { + return { + app: app as unknown as McpApp, + getApp: () => app as unknown as McpApp, + connected: true, + bootstrapState: { status: "idle" }, + subscribeToToolResult: () => () => {}, + }; +} + +function makeAlert(id: string, host: string): SecurityAlert { + return { + _id: id, + _index: ".alerts-security.alerts-default", + _source: { + "@timestamp": "2024-01-01T00:00:00Z", + "kibana.alert.rule.name": "R", + "kibana.alert.rule.uuid": "r", + "kibana.alert.severity": "high", + "kibana.alert.risk_score": 73, + "kibana.alert.workflow_status": "open", + "kibana.alert.reason": "x", + host: { name: host }, + }, + }; +} + +function unfilteredSummary(): AlertSummary { + return { + total: 2, + bySeverity: { high: 2 }, + byRule: [{ name: "R", count: 2 }], + byHost: [ + { name: "host-a", count: 1 }, + { name: "host-b", count: 1 }, + ], + alerts: [makeAlert("a1", "host-a"), makeAlert("a2", "host-b")], + }; +} + +function filteredSummary(host: string): AlertSummary { + return { + total: 1, + bySeverity: { high: 1 }, + byRule: [{ name: "R", count: 1 }], + byHost: [{ name: host, count: 1 }], + alerts: [makeAlert("f1", host)], + }; +} + +function toolResult(summary: AlertSummary) { + return { content: [{ type: "text", text: JSON.stringify(summary) }] }; +} + +function pollAlertsCalls(callServerTool: ReturnType) { + return callServerTool.mock.calls.filter((call) => call[0]?.name === "poll-alerts"); +} + +describe("Alert Triage AppContent — filter clearing", () => { + it("clears the filter and reloads the unfiltered list when the input is emptied directly, not via Enter/Escape/the chip", async () => { + const callServerTool = vi.fn().mockImplementation(async ({ name, arguments: args }) => { + if (name !== "poll-alerts") return toolResult(unfilteredSummary()); + return toolResult(args?.query ? filteredSummary("host-x") : unfilteredSummary()); + }); + const ctx = makeContext({ callServerTool }); + + const { getByLabelText, queryByLabelText } = renderAppContent(ctx); + + // Initial unfiltered load on mount. + await waitFor(() => expect(pollAlertsCalls(callServerTool).length).toBeGreaterThan(0)); + + const input = getByLabelText("Filter") as HTMLInputElement; + + // Apply a filter the normal way: type + Enter. + fireEvent.change(input, { target: { value: "host-x" } }); + fireEvent.keyDown(input, { key: "Enter" }); + + await waitFor(() => expect(queryByLabelText("Clear filter")).toBeTruthy()); + const afterFilter = pollAlertsCalls(callServerTool); + expect(afterFilter[afterFilter.length - 1][0].arguments.query).toBe("host-x"); + + // Regression: clear by deleting the input's text directly — no Enter, + // no Escape, no click on the chip's "x". Before the fix, this only + // updated local text state and never re-queried, leaving the widget + // filtered on "host-x" even though the box (and, per the bug report, + // the chip) looked empty. + fireEvent.change(input, { target: { value: "" } }); + + await waitFor(() => { + const afterClear = pollAlertsCalls(callServerTool); + expect(afterClear[afterClear.length - 1][0].arguments.query).toBeUndefined(); + }); + + // The chip is driven by the same underlying filter state, so it should + // be gone too — proving the filter was actually cleared, not just the + // text box. + await waitFor(() => expect(queryByLabelText("Clear filter")).toBeNull()); + }); + + it("does not re-fire a reload when the input is already empty and no filter is active", async () => { + const callServerTool = vi.fn().mockResolvedValue(toolResult(unfilteredSummary())); + const ctx = makeContext({ callServerTool }); + + const { getByLabelText } = renderAppContent(ctx); + + await waitFor(() => expect(pollAlertsCalls(callServerTool).length).toBeGreaterThan(0)); + const countAfterMount = pollAlertsCalls(callServerTool).length; + + const input = getByLabelText("Filter") as HTMLInputElement; + // Typing and then deleting back to empty without ever having an active + // filter should not trigger an extra reload. + fireEvent.change(input, { target: { value: "h" } }); + fireEvent.change(input, { target: { value: "" } }); + + expect(pollAlertsCalls(callServerTool).length).toBe(countAfterMount); + }); +}); + +describe("Alert Triage AppContent — stale response guard", () => { + it("does not let an older, slower response overwrite a newer one that resolved first", async () => { + const deferred: Array<() => void> = []; + let pollCallIndex = 0; + + const callServerTool = vi.fn().mockImplementation(async ({ name, arguments: args }) => { + if (name !== "poll-alerts") return toolResult(unfilteredSummary()); + const idx = pollCallIndex++; + if (idx === 0) { + // Initial mount load — resolve immediately. + return toolResult(unfilteredSummary()); + } + return new Promise((resolve) => { + deferred[idx] = () => + resolve(toolResult(args?.query ? filteredSummary("host-old") : unfilteredSummary())); + }); + }); + const ctx = makeContext({ callServerTool }); + + const { getByLabelText, queryByText, getAllByText } = renderAppContent(ctx); + + await waitFor(() => expect(pollCallIndex).toBe(1)); + + const input = getByLabelText("Filter") as HTMLInputElement; + + // Request #1 (older): apply a filter — its response will be resolved LAST. + fireEvent.change(input, { target: { value: "host-old" } }); + fireEvent.keyDown(input, { key: "Enter" }); + await waitFor(() => expect(pollCallIndex).toBe(2)); + + // Request #2 (newer): clear it via the chip — its response will be + // resolved FIRST, simulating the older request's network round-trip + // taking longer than the newer one's. + fireEvent.click(getByLabelText("Clear filter")); + await waitFor(() => expect(pollCallIndex).toBe(3)); + + // Resolve out of order: newer (#2, unfiltered) first, older (#1, filtered) second. + await act(async () => deferred[2]()); + await act(async () => deferred[1]()); + + // The stale filtered response must have been discarded — the list + // should reflect the newer, unfiltered result, not "host-old". + expect(queryByText("host-old")).toBeNull(); + expect(getAllByText("host-a").length).toBeGreaterThan(0); + }); +}); diff --git a/src/views/alert-triage/App.tsx b/src/views/alert-triage/App.tsx index 1623419..bb63745 100644 --- a/src/views/alert-triage/App.tsx +++ b/src/views/alert-triage/App.tsx @@ -85,7 +85,11 @@ export function App() { ); } -function AppContent() { +// Exported (alongside the default `App`) so tests can render it directly +// against a stubbed `McpAppContext.Provider`, bypassing the real +// `McpAppProvider`/transport — mirrors the pattern used for the hook tests in +// src/shared/hooks/*.test.tsx. +export function AppContent() { const [summary, setSummary] = useState(null); const [selectedAlert, setSelectedAlert] = useState(null); const [alertContext, setAlertContext] = useState(null); @@ -103,19 +107,28 @@ function AppContent() { const [relatedOpen, setRelatedOpen] = useState(false); const paramsRef = useRef({ days: 7, limit: DEFAULT_LIMIT_NUM }); const listRef = useRef(null); + // Bumped at the start of every loadAlertsImpl call so a response that + // resolves after a newer request has already started can be told apart + // from the latest one — see requestId check below. + const requestIdRef = useRef(0); const loadAlertsImpl = useCallback(async (app: McpApp, overrideParams?: Partial) => { + const requestId = ++requestIdRef.current; setLoading(true); try { const args = { ...paramsRef.current, ...overrideParams }; if (overrideParams) paramsRef.current = { ...paramsRef.current, ...overrideParams }; const result = await app.callServerTool({ name: "poll-alerts", arguments: args }); + // A newer call started while this one was in flight (e.g. the 60s poll, + // or the user clearing/changing the filter) — its response should win, + // so discard this now-stale one rather than overwrite fresher data. + if (requestId !== requestIdRef.current) return; const text = extractCallResult(result); if (text) setSummary(JSON.parse(text)); } catch (e) { console.error("Load alerts failed:", e); } finally { - setLoading(false); + if (requestId === requestIdRef.current) setLoading(false); } }, []); @@ -432,6 +445,22 @@ function AppContent() { loadAlerts({ query: undefined }); }, [loadAlerts]); + /** + * `SearchInput`'s onChange fires on every keystroke but only updates local + * text state — it does not, by itself, clear the actual filter + * (`paramsRef.current.query`). Pressing Enter/Escape or clicking the chip's + * "x" all correctly clear it via `clearQuery`, but a user who selects the + * text and deletes it (or backspaces to empty) without pressing Enter would + * see an empty box and no chip while the list stayed filtered on the old + * value. Treat "input became empty" the same as clicking the chip's "x". + */ + const handleInputChange = useCallback((value: string) => { + setSearchInput(value); + if (value === "" && paramsRef.current.query) { + loadAlerts({ query: undefined }); + } + }, [loadAlerts]); + /** * Filter the alert list by a specific ECS field/value pair. * Called when the user clicks a dotted-underline fact value on a card or in the detail pane. @@ -647,7 +676,7 @@ function AppContent() { actions={