From 9c3d5e2b61ef48c169435b70648f7241aaed4198 Mon Sep 17 00:00:00 2001 From: Brad Harris Date: Thu, 13 Aug 2026 20:22:52 -0600 Subject: [PATCH 1/2] Cover the Brain Explorer panes and fix two routing bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the last two zero-coverage surfaces from the #925 brains split, and fixes two real bugs the coverage work surfaced in brains-detail-pane.tsx. A malformed base64url repo root recovered by calling navigate() during render. React Router v7 refuses imperative navigation during render — it warns and no-ops — so the undecodable root stayed in the address bar and came straight back on reload or from a shared link. It is now a declarative . The collection path segment was decoded a second time on the way to the collection view, but react-router has already decoded it. A collection named "50%off" threw URIError and took the pane down; one named "%41" silently selected a collection that does not exist. Both names are reachable — the sidebar links to whatever collection an agent created. brains-detail-pane.test.tsx (20 tests) mounts the pane on the same three routes the real router declares and pins the repo-root decode and its malformed fallback, the collection round trip, the pill counts and active marker, the search wiring, and the destructive "Clear project" flow including its wire contract, summed toast, failure path and pending gating. 16/17 mutants killed; the survivor is the collections loading guard, which is unobservable because the query's data already defaults to []. brains-pane.test.tsx (14 tests) pins the encode half of the same URL contract plus the sidebar's list states, per-type count gating, keyboard activation and selection highlight. 18/18 mutants killed. Co-Authored-By: Claude Opus 5 (1M context) --- .../app/brains-detail-pane.test.tsx | 449 ++++++++++++++++++ .../src/components/app/brains-detail-pane.tsx | 14 +- .../src/components/app/brains-pane.test.tsx | 280 +++++++++++ 3 files changed, 738 insertions(+), 5 deletions(-) create mode 100644 apps/web/src/components/app/brains-detail-pane.test.tsx create mode 100644 apps/web/src/components/app/brains-pane.test.tsx diff --git a/apps/web/src/components/app/brains-detail-pane.test.tsx b/apps/web/src/components/app/brains-detail-pane.test.tsx new file mode 100644 index 00000000..0395f51d --- /dev/null +++ b/apps/web/src/components/app/brains-detail-pane.test.tsx @@ -0,0 +1,449 @@ +// @vitest-environment jsdom +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { + cleanup, + fireEvent, + render, + screen, + waitFor, +} from "@testing-library/react"; +import { + MemoryRouter, + Route, + Routes, + useLocation, + type Location, +} from "react-router-dom"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import type { BrainCollectionSummary } from "@/hooks/use-brain"; +import { encodeRepoRoot } from "@/lib/brain-encoding"; + +import { BrainsDetailPane } from "./brains-detail-pane"; + +// The pane is mounted on the same three routes the real router declares, so +// useParams supplies the params exactly as production does. What is under test +// is everything the pane owns between the URL and its children: decoding the +// base64url repo root (and recovering when it is malformed), decoding and +// re-encoding collection names through the path segment, and the destructive +// "Clear project" flow. The real use-brain queries and mutations run; only the +// HTTP seam, the toaster, and the already-covered collection view are stubbed. +vi.mock("@/lib/api", () => ({ api: vi.fn() })); +vi.mock("sonner", () => ({ toast: { success: vi.fn(), error: vi.fn() } })); + +// BrainCollectionView has its own 590-line suite. Here it stands in as a probe +// for the three props the pane is responsible for wiring, plus the cleared +// callback — a real mount would fire its own queries and drown those out. +vi.mock("@/components/app/brains-collection-view", () => ({ + BrainCollectionView: ({ + repoRoot, + collection, + search, + onCollectionCleared, + }: { + repoRoot: string; + collection: string | null; + search: string; + onCollectionCleared: () => void; + }) => ( +
+ {repoRoot} + {collection ?? ""} + {search} + +
+ ), +})); + +const { api } = await import("@/lib/api"); +const { toast } = await import("sonner"); +const apiMock = vi.mocked(api); +const toastSuccess = vi.mocked(toast.success); +const toastError = vi.mocked(toast.error); + +const REPO = "/Users/brad/dev/apps/dispatch"; +const ENCODED = encodeRepoRoot(REPO); + +function summary( + collection: string, + objectCount = 0, + listCount = 0, + eventCount = 0 +): BrainCollectionSummary { + return { collection, objectCount, listCount, eventCount }; +} + +function LocationProbe(): JSX.Element { + const location: Location = useLocation(); + return {location.pathname}; +} + +function locationPath(): string { + return screen.getByTestId("location").textContent ?? ""; +} + +function mountPane({ + path = `/automations/brains/${ENCODED}`, + collections = [] as BrainCollectionSummary[] | null, + onDeleteProject, +}: { + path?: string; + /** null keeps the collections query pending, standing for "still loading". */ + collections?: BrainCollectionSummary[] | null; + onDeleteProject?: () => Promise; +} = {}) { + apiMock.mockImplementation( + async (requestPath: string, init?: RequestInit) => { + if (init?.method === "DELETE") { + if (!onDeleteProject) + return { objects: 0, lists: 0, events: 0 } as never; + return (await onDeleteProject()) as never; + } + if (requestPath.split("?")[0] === "/api/v1/brain/collections") { + if (collections === null) return new Promise(() => {}) as never; + return collections as never; + } + throw new Error(`unexpected request: ${requestPath}`); + } + ); + + const client = new QueryClient({ + defaultOptions: { + queries: { retry: false, gcTime: 0 }, + mutations: { retry: false }, + }, + }); + + render( + + + + } /> + } + /> + } + /> + + + + + ); +} + +function deleteCalls(): string[] { + return apiMock.mock.calls + .filter( + ([, init]) => (init as RequestInit | undefined)?.method === "DELETE" + ) + .map(([path]) => path as string); +} + +function pillName(name: string): RegExp { + // A pill's accessible name is the label immediately followed by its count + // badge with no separator ("config10"), so the count is matched explicitly + // rather than left to a word boundary that never lands between two word + // characters. Fully anchored so one label cannot match a longer one. + return new RegExp(`^${name.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}\\d*$`); +} + +/** Pills for real collections only appear once the collections query lands. */ +function findPill(name: string): Promise { + return screen.findByRole("button", { name: pillName(name) }); +} + +function pill(name: string): HTMLElement { + return screen.getByRole("button", { name: pillName(name) }); +} + +function openProjectDelete() { + fireEvent.click(screen.getByRole("button", { name: /Clear project/ })); +} + +function confirmButton(): HTMLElement { + return screen.getByRole("button", { name: /Delete permanently|Deleting/ }); +} + +beforeEach(() => { + apiMock.mockReset(); + toastSuccess.mockReset(); + toastError.mockReset(); +}); + +afterEach(cleanup); + +describe("repo root routing", () => { + it("shows the explorer placeholder when no project is in the URL", () => { + mountPane({ path: "/automations/brains" }); + + expect(screen.getByText("Brain Explorer")).toBeTruthy(); + expect(screen.queryByRole("button", { name: /Clear project/ })).toBeNull(); + }); + + it("decodes the base64url repo root into the project header", () => { + mountPane(); + + // Both the basename and the full path come out of the decode — a decoder + // that dropped the base64url substitutions would render mojibake here. + expect(screen.getByRole("heading", { name: "dispatch" })).toBeTruthy(); + expect(screen.getByTitle(REPO).textContent).toBe(REPO); + expect(screen.getByTestId("view-repo-root").textContent).toBe(REPO); + }); + + it("replaces a malformed repo root in the URL with the overview route", async () => { + // "!!!" is not valid base64, so decodeRepoRoot throws. Recovering by + // rendering the overview is not enough — the undecodable root has to leave + // the address bar too, or a reload or a shared link lands right back on it. + mountPane({ path: "/automations/brains/!!!" }); + + await waitFor(() => expect(locationPath()).toBe("/automations/brains")); + expect(await screen.findByText("Brain Explorer")).toBeTruthy(); + }); + + it("hands the view the real collection name from an escaped path segment", () => { + // Collection names may contain slashes, which only survive the path as + // %2F — the view must receive the real name, not the escaped one. + mountPane({ + path: `/automations/brains/${ENCODED}/${encodeURIComponent("ops/deploy")}`, + collections: [summary("ops/deploy", 1, 0, 0)], + }); + + expect(screen.getByTestId("view-collection").textContent).toBe( + "ops/deploy" + ); + }); + + it("leaves a percent sign in the collection name alone", () => { + // react-router already decodes path params. Decoding a second time throws + // URIError on "50%off" — a collection an agent can create and the sidebar + // will happily link to — taking the whole pane down with it. + mountPane({ + path: `/automations/brains/${ENCODED}/${encodeURIComponent("50%off")}`, + collections: [summary("50%off", 1, 0, 0)], + }); + + expect(screen.getByTestId("view-collection").textContent).toBe("50%off"); + }); + + it("does not treat an escape sequence in a collection name as an escape", () => { + // "%41" is a legal collection name, not an encoded "A". A second decode + // would select a collection that does not exist and render it empty. + mountPane({ + path: `/automations/brains/${ENCODED}/${encodeURIComponent("%41")}`, + collections: [summary("%41", 1, 0, 0)], + }); + + expect(screen.getByTestId("view-collection").textContent).toBe("%41"); + }); +}); + +describe("collection pills", () => { + it("renders one pill per collection with its summed entry count", async () => { + mountPane({ + collections: [summary("config", 2, 3, 5), summary("notes", 1, 0, 0)], + }); + + // The badge is the total across all three entry types, not just objects. + expect((await findPill("config")).textContent).toContain("10"); + expect(pill("notes").textContent).toContain("1"); + }); + + it("marks All active while no collection is selected", async () => { + mountPane({ collections: [summary("config", 1, 0, 0)] }); + + const config = await findPill("config"); + expect(pill("All").className).toContain("bg-primary/15"); + expect(config.className).not.toContain("bg-primary/15"); + }); + + it("moves the active marker to the collection named in the URL", async () => { + mountPane({ + path: `/automations/brains/${ENCODED}/config`, + collections: [summary("config", 1, 0, 0)], + }); + + const config = await findPill("config"); + expect(config.className).toContain("bg-primary/15"); + expect(pill("All").className).not.toContain("bg-primary/15"); + }); + + it("renders no collection pills while the collections query is loading", async () => { + mountPane({ collections: null }); + + expect(pill("All")).toBeTruthy(); + // Only the two chrome buttons — the All pill and Clear project — exist + // until the query resolves. + await waitFor(() => + expect( + screen + .getAllByRole("button") + .filter((b) => (b.textContent ?? "").includes("config")) + ).toHaveLength(0) + ); + }); + + it("re-encodes the collection name when navigating to its pill", async () => { + mountPane({ collections: [summary("ops/deploy", 1, 0, 0)] }); + + fireEvent.click(await findPill("ops/deploy")); + + // A raw slash here would push a route the router does not declare. + await waitFor(() => + expect(locationPath()).toBe(`/automations/brains/${ENCODED}/ops%2Fdeploy`) + ); + }); + + it("returns to the project root from the All pill", async () => { + mountPane({ + path: `/automations/brains/${ENCODED}/config`, + collections: [summary("config", 1, 0, 0)], + }); + await findPill("config"); + + fireEvent.click(pill("All")); + + await waitFor(() => + expect(locationPath()).toBe(`/automations/brains/${ENCODED}`) + ); + }); + + it("returns to the project root when the view reports its collection cleared", async () => { + mountPane({ + path: `/automations/brains/${ENCODED}/config`, + collections: [summary("config", 1, 0, 0)], + }); + + fireEvent.click(screen.getByRole("button", { name: "simulate cleared" })); + + await waitFor(() => + expect(locationPath()).toBe(`/automations/brains/${ENCODED}`) + ); + }); +}); + +describe("search filter", () => { + it("passes the typed filter down to the collection view", () => { + mountPane(); + + fireEvent.change(screen.getByPlaceholderText("Filter..."), { + target: { value: "queue" }, + }); + + expect(screen.getByTestId("view-search").textContent).toBe("queue"); + }); +}); + +describe("clear project", () => { + it("keeps the confirmation closed until the action is pressed", () => { + mountPane(); + + expect(screen.queryByText("Clear this project?")).toBeNull(); + + openProjectDelete(); + + expect(screen.getByText("Clear this project?")).toBeTruthy(); + }); + + it("deletes nothing when the confirmation is cancelled", async () => { + mountPane(); + + openProjectDelete(); + fireEvent.click(screen.getByRole("button", { name: "Cancel" })); + + await waitFor(() => + expect(screen.queryByText("Clear this project?")).toBeNull() + ); + expect(deleteCalls()).toHaveLength(0); + }); + + it("scopes the delete to this project and reports the total removed", async () => { + mountPane({ + onDeleteProject: async () => ({ objects: 4, lists: 2, events: 7 }), + }); + + openProjectDelete(); + fireEvent.click(confirmButton()); + + await waitFor(() => expect(deleteCalls()).toHaveLength(1)); + // repoRoot must be query-encoded; an unencoded path would truncate at the + // first slash and clear the wrong project. + expect(deleteCalls()[0]).toBe( + `/api/v1/brain/projects?repoRoot=${encodeURIComponent(REPO)}` + ); + // The toast quotes the sum of all three entry types, not one of them. + await waitFor(() => + expect(toastSuccess).toHaveBeenCalledWith( + "Deleted 13 entries from this project." + ) + ); + }); + + it("closes the dialog and leaves the deleted project on success", async () => { + mountPane({ + onDeleteProject: async () => ({ objects: 1, lists: 0, events: 0 }), + }); + + openProjectDelete(); + fireEvent.click(confirmButton()); + + await waitFor(() => expect(locationPath()).toBe("/automations/brains")); + expect(screen.queryByText("Clear this project?")).toBeNull(); + }); + + it("keeps the dialog open on the project view when the delete fails", async () => { + mountPane({ + onDeleteProject: async () => { + throw new Error("boom"); + }, + }); + + openProjectDelete(); + fireEvent.click(confirmButton()); + + await waitFor(() => + expect(toastError).toHaveBeenCalledWith( + "Could not delete project brain data." + ) + ); + // Navigating away on a failed delete would hide a project that still has + // all of its data. + expect(locationPath()).toBe(`/automations/brains/${ENCODED}`); + expect(screen.getByText("Clear this project?")).toBeTruthy(); + expect(toastSuccess).not.toHaveBeenCalled(); + }); + + it("disables both dialog buttons while the delete is in flight", async () => { + let release: (value: unknown) => void = () => {}; + mountPane({ + onDeleteProject: () => + new Promise((resolve) => { + release = resolve; + }), + }); + + openProjectDelete(); + fireEvent.click(confirmButton()); + + // A second confirm click during the request would fire a second DELETE. + await waitFor(() => + expect(screen.getByRole("button", { name: "Deleting..." })).toBeTruthy() + ); + expect( + (screen.getByRole("button", { name: "Deleting..." }) as HTMLButtonElement) + .disabled + ).toBe(true); + expect( + (screen.getByRole("button", { name: "Cancel" }) as HTMLButtonElement) + .disabled + ).toBe(true); + + release({ objects: 1, lists: 0, events: 0 }); + await waitFor(() => expect(locationPath()).toBe("/automations/brains")); + expect(deleteCalls()).toHaveLength(1); + }); +}); diff --git a/apps/web/src/components/app/brains-detail-pane.tsx b/apps/web/src/components/app/brains-detail-pane.tsx index c58a13f3..d0d69362 100644 --- a/apps/web/src/components/app/brains-detail-pane.tsx +++ b/apps/web/src/components/app/brains-detail-pane.tsx @@ -1,5 +1,5 @@ import { useState } from "react"; -import { useNavigate, useParams } from "react-router-dom"; +import { Navigate, useNavigate, useParams } from "react-router-dom"; import { Brain, Search, Trash2 } from "lucide-react"; import { toast } from "sonner"; @@ -26,7 +26,6 @@ import { import { cn } from "@/lib/utils"; export function BrainsDetailPane(): JSX.Element { - const navigate = useNavigate(); const { encodedRepoRoot, collection } = useParams<{ encodedRepoRoot?: string; collection?: string; @@ -40,15 +39,20 @@ export function BrainsDetailPane(): JSX.Element { try { repoRoot = decodeRepoRoot(encodedRepoRoot); } catch { - navigate("/automations/brains", { replace: true }); - return ; + // Has to be declarative: react-router refuses an imperative navigate() + // during render, so calling it here would only warn and leave the + // undecodable repo root sitting in the address bar. + return ; } return ( ); } diff --git a/apps/web/src/components/app/brains-pane.test.tsx b/apps/web/src/components/app/brains-pane.test.tsx new file mode 100644 index 00000000..8537d14e --- /dev/null +++ b/apps/web/src/components/app/brains-pane.test.tsx @@ -0,0 +1,280 @@ +// @vitest-environment jsdom +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { + cleanup, + fireEvent, + render, + screen, + waitFor, +} from "@testing-library/react"; +import { + MemoryRouter, + Route, + Routes, + useLocation, + type Location, +} from "react-router-dom"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import type { BrainProject } from "@/hooks/use-brain"; +import { encodeRepoRoot } from "@/lib/brain-encoding"; + +import { BrainsListContent } from "./brains-pane"; + +// The sidebar owns the encode half of the Brain Explorer's URL contract: it +// turns a repo root into the base64url segment the detail pane decodes, and +// decides which row is selected by comparing that encoding against the param +// already in the URL. It is mounted on the real routes so the comparison runs +// against genuine params rather than a hand-built string. +vi.mock("@/lib/api", () => ({ api: vi.fn() })); + +const { api } = await import("@/lib/api"); +const apiMock = vi.mocked(api); + +const REPO = "/Users/brad/dev/apps/dispatch"; +const OTHER = "/srv/other-repo"; + +function project(overrides: Partial = {}): BrainProject { + return { + repoRoot: REPO, + objectCount: 0, + listCount: 0, + eventCount: 0, + ...overrides, + }; +} + +function LocationProbe(): JSX.Element { + const location: Location = useLocation(); + return {location.pathname}; +} + +function locationPath(): string { + return screen.getByTestId("location").textContent ?? ""; +} + +function mountSidebar({ + path = "/automations/brains", + projects = [] as BrainProject[] | null, + onItemSelect, +}: { + path?: string; + /** null keeps the projects query pending, standing for "still loading". */ + projects?: BrainProject[] | null; + onItemSelect?: () => void; +} = {}) { + apiMock.mockImplementation(async (requestPath: string) => { + if (requestPath === "/api/v1/brain/projects") { + if (projects === null) return new Promise(() => {}) as never; + return projects as never; + } + throw new Error(`unexpected request: ${requestPath}`); + }); + + const client = new QueryClient({ + defaultOptions: { queries: { retry: false, gcTime: 0 } }, + }); + + const element = ; + render( + + + + + + + + + + ); +} + +function row(name: string): Promise { + return screen.findByRole("button", { name: new RegExp(name) }); +} + +beforeEach(() => { + apiMock.mockReset(); +}); + +afterEach(cleanup); + +describe("list states", () => { + it("shows placeholders while the projects query is pending", async () => { + mountSidebar({ projects: null }); + + // Distinguishable from the empty state: neither the empty copy nor any row + // may appear before the answer is known. + await waitFor(() => + expect(document.querySelectorAll(".animate-pulse")).toHaveLength(3) + ); + expect(screen.queryByText("No brain data yet.")).toBeNull(); + }); + + it("explains the empty state once the query resolves with nothing", async () => { + mountSidebar({ projects: [] }); + + expect(await screen.findByText("No brain data yet.")).toBeTruthy(); + expect(document.querySelectorAll(".animate-pulse")).toHaveLength(0); + }); + + it("shows the basename, the shortened path, and the full path on hover", async () => { + mountSidebar({ projects: [project()] }); + + await row("dispatch"); + // Exact text, not a substring of the row: the full repo root also ends in + // "dispatch", so a row that skipped repoBasename would still contain it. + expect(screen.getByText("dispatch", { selector: "div" })).toBeTruthy(); + // shortPath keeps only the last three segments; the untruncated root stays + // reachable through the title attribute. + expect(screen.getByText(".../dev/apps/dispatch")).toBeTruthy(); + expect(screen.getByTitle(REPO)).toBeTruthy(); + }); +}); + +describe("entry counts", () => { + it("renders a count per entry type that has entries", async () => { + mountSidebar({ + projects: [project({ objectCount: 4, listCount: 9, eventCount: 2 })], + }); + + const item = await row("dispatch"); + expect(item.textContent).toContain("4"); + expect(item.textContent).toContain("9"); + expect(item.textContent).toContain("2"); + }); + + it("omits an entry type with a zero count instead of showing a 0", async () => { + // Each type is gated independently, so one fixture carrying a single + // non-zero count leaves the other two guards free to fall away unnoticed — + // their mutant renders the same row. Every type therefore gets a row where + // it is the only non-zero count, and a row where it is zero. + mountSidebar({ + projects: [ + project({ repoRoot: "/a/objects-only", objectCount: 7 }), + project({ repoRoot: "/a/lists-only", listCount: 7 }), + project({ repoRoot: "/a/events-only", eventCount: 7 }), + ], + }); + + for (const name of ["objects-only", "lists-only", "events-only"]) { + const item = await row(name); + expect(item.textContent).toContain("7"); + expect(item.textContent).not.toContain("0"); + } + }); +}); + +describe("navigation", () => { + it("routes a project row to its base64url-encoded repo root", async () => { + mountSidebar({ projects: [project()] }); + + fireEvent.click(await row("dispatch")); + + // This is the exact segment the detail pane decodes back into a repo root. + await waitFor(() => + expect(locationPath()).toBe(`/automations/brains/${encodeRepoRoot(REPO)}`) + ); + }); + + it("returns to the overview from the Overview row", async () => { + mountSidebar({ + path: `/automations/brains/${encodeRepoRoot(REPO)}`, + projects: [project()], + }); + + fireEvent.click(await screen.findByRole("button", { name: /Overview/ })); + + await waitFor(() => expect(locationPath()).toBe("/automations/brains")); + }); + + it("closes the mobile sidebar after either kind of selection", async () => { + const onItemSelect = vi.fn(); + mountSidebar({ projects: [project()], onItemSelect }); + + fireEvent.click(await row("dispatch")); + expect(onItemSelect).toHaveBeenCalledTimes(1); + + fireEvent.click(screen.getByRole("button", { name: /Overview/ })); + expect(onItemSelect).toHaveBeenCalledTimes(2); + }); + + it("navigates without a select handler wired up", async () => { + mountSidebar({ projects: [project()] }); + + fireEvent.click(await row("dispatch")); + + await waitFor(() => + expect(locationPath()).toBe(`/automations/brains/${encodeRepoRoot(REPO)}`) + ); + }); +}); + +describe("keyboard activation", () => { + it("opens a project with Enter and with Space", async () => { + const onItemSelect = vi.fn(); + mountSidebar({ projects: [project()], onItemSelect }); + + const item = await row("dispatch"); + fireEvent.keyDown(item, { key: "Enter" }); + await waitFor(() => + expect(locationPath()).toBe(`/automations/brains/${encodeRepoRoot(REPO)}`) + ); + + fireEvent.keyDown(item, { key: " " }); + expect(onItemSelect).toHaveBeenCalledTimes(2); + }); + + it("swallows the Space keypress so the sidebar does not scroll", async () => { + mountSidebar({ projects: [project()] }); + + const item = await row("dispatch"); + const event = new KeyboardEvent("keydown", { + key: " ", + bubbles: true, + cancelable: true, + }); + item.dispatchEvent(event); + + expect(event.defaultPrevented).toBe(true); + }); + + it("ignores keys that are not Enter or Space", async () => { + const onItemSelect = vi.fn(); + mountSidebar({ projects: [project()], onItemSelect }); + + fireEvent.keyDown(await row("dispatch"), { key: "a" }); + + expect(onItemSelect).not.toHaveBeenCalled(); + expect(locationPath()).toBe("/automations/brains"); + }); +}); + +describe("selection highlight", () => { + it("marks only the project whose encoding matches the URL", async () => { + mountSidebar({ + path: `/automations/brains/${encodeRepoRoot(REPO)}`, + projects: [project(), project({ repoRoot: OTHER })], + }); + + expect((await row("dispatch")).className).toContain("border-r-primary"); + expect((await row("other-repo")).className).not.toContain( + "border-r-primary" + ); + // Overview has to give up its highlight too, or two rows read as current. + expect( + screen.getByRole("button", { name: /Overview/ }).className + ).not.toContain("border-r-primary"); + }); + + it("marks Overview instead when no project is in the URL", async () => { + mountSidebar({ projects: [project()] }); + + const overview = await screen.findByRole("button", { name: /Overview/ }); + expect(overview.className).toContain("border-r-primary"); + expect((await row("dispatch")).className).not.toContain("border-r-primary"); + }); +}); From 442d4a0fce87ac7f3ddb7cec754029b87d9bb320 Mon Sep 17 00:00:00 2001 From: Brad Harris Date: Thu, 13 Aug 2026 20:38:40 -0600 Subject: [PATCH 2/2] Address review: assert the replace semantics, drop two hollow tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first battery never mutated the { replace: true } flags, so nothing guarded the half of the malformed-root fix that matters most: pushing instead of replacing leaves the undecodable URL one Back press away, and going Back redirects forward again — a trap. A navigation-type probe now pins REPLACE on all three redirects and PUSH on ordinary pill navigation. Deleted the loading-pills test. It could not fail: useBrainCollections already defaults data to [], so both collectionsLoading guards are unobservable, and the assertion filtered for a collection no fixture declared. The dead guards are recorded for a future run rather than covered by a test that certifies nothing. Renamed the post-delete test to the navigation it actually protects — leaving the project unmounts the dialog either way, so that test cannot be what proves the dialog closes. Also: dropped a sidebar test that duplicated the routing case, split the Enter/Space test so neither reuses a node captured before its own route change, tightened the pill count assertions to exact text, relaxed the skeleton count to presence, and stubbed Toaster alongside toast. Batteries after the changes: 20/21 on brains-detail-pane.tsx (the survivor is the proven-unobservable loading guard) and 19/19 on brains-pane.tsx. Co-Authored-By: Claude Opus 5 (1M context) --- .../app/brains-detail-pane.test.tsx | 65 ++++++++++++------- .../src/components/app/brains-pane.test.tsx | 29 +++++---- 2 files changed, 59 insertions(+), 35 deletions(-) diff --git a/apps/web/src/components/app/brains-detail-pane.test.tsx b/apps/web/src/components/app/brains-detail-pane.test.tsx index 0395f51d..8accbeb3 100644 --- a/apps/web/src/components/app/brains-detail-pane.test.tsx +++ b/apps/web/src/components/app/brains-detail-pane.test.tsx @@ -12,7 +12,9 @@ import { Route, Routes, useLocation, + useNavigationType, type Location, + type NavigationType, } from "react-router-dom"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; @@ -29,7 +31,12 @@ import { BrainsDetailPane } from "./brains-detail-pane"; // "Clear project" flow. The real use-brain queries and mutations run; only the // HTTP seam, the toaster, and the already-covered collection view are stubbed. vi.mock("@/lib/api", () => ({ api: vi.fn() })); -vi.mock("sonner", () => ({ toast: { success: vi.fn(), error: vi.fn() } })); +// Toaster is stubbed alongside toast so that a component pulled into the tree +// later fails on its own terms rather than as "Element type is invalid". +vi.mock("sonner", () => ({ + toast: { success: vi.fn(), error: vi.fn() }, + Toaster: () => null, +})); // BrainCollectionView has its own 590-line suite. Here it stands in as a probe // for the three props the pane is responsible for wiring, plus the cleared @@ -77,13 +84,24 @@ function summary( function LocationProbe(): JSX.Element { const location: Location = useLocation(); - return {location.pathname}; + const navigationType: NavigationType = useNavigationType(); + return ( + <> + {location.pathname} + {navigationType} + + ); } function locationPath(): string { return screen.getByTestId("location").textContent ?? ""; } +/** "PUSH" leaves the previous entry in history; "REPLACE" discards it. */ +function navigationType(): string { + return screen.getByTestId("navigation-type").textContent ?? ""; +} + function mountPane({ path = `/automations/brains/${ENCODED}`, collections = [] as BrainCollectionSummary[] | null, @@ -203,6 +221,10 @@ describe("repo root routing", () => { await waitFor(() => expect(locationPath()).toBe("/automations/brains")); expect(await screen.findByText("Brain Explorer")).toBeTruthy(); + // Replaced, not pushed: a pushed redirect leaves the undecodable root one + // Back press away, and going Back immediately redirects forward again — + // the user is trapped on the button. + expect(navigationType()).toBe("REPLACE"); }); it("hands the view the real collection name from an escaped path segment", () => { @@ -248,9 +270,10 @@ describe("collection pills", () => { collections: [summary("config", 2, 3, 5), summary("notes", 1, 0, 0)], }); - // The badge is the total across all three entry types, not just objects. - expect((await findPill("config")).textContent).toContain("10"); - expect(pill("notes").textContent).toContain("1"); + // The badge is the total across all three entry types, not just objects, + // and is matched whole so a stray digit cannot stand in for the sum. + expect((await findPill("config")).textContent).toBe("config10"); + expect(pill("notes").textContent).toBe("notes1"); }); it("marks All active while no collection is selected", async () => { @@ -272,21 +295,6 @@ describe("collection pills", () => { expect(pill("All").className).not.toContain("bg-primary/15"); }); - it("renders no collection pills while the collections query is loading", async () => { - mountPane({ collections: null }); - - expect(pill("All")).toBeTruthy(); - // Only the two chrome buttons — the All pill and Clear project — exist - // until the query resolves. - await waitFor(() => - expect( - screen - .getAllByRole("button") - .filter((b) => (b.textContent ?? "").includes("config")) - ).toHaveLength(0) - ); - }); - it("re-encodes the collection name when navigating to its pill", async () => { mountPane({ collections: [summary("ops/deploy", 1, 0, 0)] }); @@ -310,9 +318,12 @@ describe("collection pills", () => { await waitFor(() => expect(locationPath()).toBe(`/automations/brains/${ENCODED}`) ); + // Choosing a different collection is ordinary navigation, so it stays on + // the history stack — unlike the redirects, which replace. + expect(navigationType()).toBe("PUSH"); }); - it("returns to the project root when the view reports its collection cleared", async () => { + it("replaces history when the view reports its collection cleared", async () => { mountPane({ path: `/automations/brains/${ENCODED}/config`, collections: [summary("config", 1, 0, 0)], @@ -323,6 +334,9 @@ describe("collection pills", () => { await waitFor(() => expect(locationPath()).toBe(`/automations/brains/${ENCODED}`) ); + // The collection no longer exists, so its URL must not stay reachable + // through the Back button. + expect(navigationType()).toBe("REPLACE"); }); }); @@ -383,7 +397,11 @@ describe("clear project", () => { ); }); - it("closes the dialog and leaves the deleted project on success", async () => { + it("leaves the deleted project for the overview without keeping it in history", async () => { + // Only the navigation is asserted here. The dialog does close, but this + // test cannot be the thing that proves it: leaving the project unmounts + // the dialog either way, so a missing setProjectDeleteOpen(false) is + // invisible from the outside. mountPane({ onDeleteProject: async () => ({ objects: 1, lists: 0, events: 0 }), }); @@ -392,7 +410,8 @@ describe("clear project", () => { fireEvent.click(confirmButton()); await waitFor(() => expect(locationPath()).toBe("/automations/brains")); - expect(screen.queryByText("Clear this project?")).toBeNull(); + // Back must not return to a project whose data is gone. + expect(navigationType()).toBe("REPLACE"); }); it("keeps the dialog open on the project view when the delete fails", async () => { diff --git a/apps/web/src/components/app/brains-pane.test.tsx b/apps/web/src/components/app/brains-pane.test.tsx index 8537d14e..65e0161e 100644 --- a/apps/web/src/components/app/brains-pane.test.tsx +++ b/apps/web/src/components/app/brains-pane.test.tsx @@ -109,7 +109,9 @@ describe("list states", () => { // Distinguishable from the empty state: neither the empty copy nor any row // may appear before the answer is known. await waitFor(() => - expect(document.querySelectorAll(".animate-pulse")).toHaveLength(3) + expect( + document.querySelectorAll(".animate-pulse").length + ).toBeGreaterThan(0) ); expect(screen.queryByText("No brain data yet.")).toBeNull(); }); @@ -201,31 +203,34 @@ describe("navigation", () => { fireEvent.click(screen.getByRole("button", { name: /Overview/ })); expect(onItemSelect).toHaveBeenCalledTimes(2); }); +}); - it("navigates without a select handler wired up", async () => { - mountSidebar({ projects: [project()] }); +describe("keyboard activation", () => { + it("opens a project with Enter", async () => { + const onItemSelect = vi.fn(); + mountSidebar({ projects: [project()], onItemSelect }); - fireEvent.click(await row("dispatch")); + fireEvent.keyDown(await row("dispatch"), { key: "Enter" }); await waitFor(() => expect(locationPath()).toBe(`/automations/brains/${encodeRepoRoot(REPO)}`) ); + expect(onItemSelect).toHaveBeenCalledTimes(1); }); -}); -describe("keyboard activation", () => { - it("opens a project with Enter and with Space", async () => { + it("opens a project with Space", async () => { + // Kept separate from the Enter case rather than firing both at one + // captured node: the first key navigates, and re-using a node captured + // before that route change would only work by accident of reconciliation. const onItemSelect = vi.fn(); mountSidebar({ projects: [project()], onItemSelect }); - const item = await row("dispatch"); - fireEvent.keyDown(item, { key: "Enter" }); + fireEvent.keyDown(await row("dispatch"), { key: " " }); + await waitFor(() => expect(locationPath()).toBe(`/automations/brains/${encodeRepoRoot(REPO)}`) ); - - fireEvent.keyDown(item, { key: " " }); - expect(onItemSelect).toHaveBeenCalledTimes(2); + expect(onItemSelect).toHaveBeenCalledTimes(1); }); it("swallows the Space keypress so the sidebar does not scroll", async () => {