From 0fcd144ee3d7d434a8de5e1b8539e72c37e28431 Mon Sep 17 00:00:00 2001 From: thomasalvaedison7777-lgtm Date: Fri, 17 Jul 2026 06:45:43 -0700 Subject: [PATCH] feat(miner-ui): enhance portfolio queue with pagination and chart visualization (#6831) - Introduced pagination for the per-repo and queue-actions tables, ensuring they only paginate when exceeding 20 rows. - Added a horizontal bar chart to visualize queue status counts, improving the UI's data representation. - Updated tests to cover new pagination behavior and chart rendering, ensuring accurate functionality and user experience. Closes #6831 --- .../src/portfolio-queue-actions.test.tsx | 91 +++++- .../src/portfolio-queue.test.tsx | 76 ++++- .../src/routes/portfolio.tsx | 300 ++++++++++++++---- 3 files changed, 405 insertions(+), 62 deletions(-) diff --git a/apps/loopover-miner-ui/src/portfolio-queue-actions.test.tsx b/apps/loopover-miner-ui/src/portfolio-queue-actions.test.tsx index a1487a1cfc..1234e09bf8 100644 --- a/apps/loopover-miner-ui/src/portfolio-queue-actions.test.tsx +++ b/apps/loopover-miner-ui/src/portfolio-queue-actions.test.tsx @@ -41,8 +41,18 @@ const doneItem: PortfolioQueueActionItem = { status: "done", }; +function manyActionItems(count: number): PortfolioQueueActionItem[] { + return Array.from({ length: count }, (_, index) => ({ + apiBaseUrl: "https://api.github.com", + repoFullName: `acme/repo-${String(index).padStart(2, "0")}`, + identifier: `issue:${index}`, + // Alternating status so the default sort (in_progress first) still leaves enough rows for page 2. + status: index % 2 === 0 ? ("in_progress" as const) : ("done" as const), + })); +} + describe("PortfolioQueueActionsSection (#4857)", () => { - it("renders a content-shaped skeleton before the first result arrives", () => { + it("renders a content-shaped loading skeleton (role=status), not the old flat loading text (#6511, #6831)", () => { // #6511: StateBoundary renders the skeleton INSTEAD of a loading title, so the old // "Loading actionable queue items…" text is intentionally gone; assert the placeholder instead. render( @@ -54,7 +64,9 @@ describe("PortfolioQueueActionsSection (#4857)", () => { onRequeue={() => undefined} />, ); + expect(screen.getByRole("status", { name: /loading actionable queue items/i })).toBeTruthy(); expect(screen.getByTestId("queue-actions-skeleton")).toBeTruthy(); + expect(screen.queryByText("Loading actionable queue items…")).toBeNull(); // Shaped like the real content, not one generic bar: the real table is not rendered yet. expect(screen.queryByRole("table")).toBeNull(); }); @@ -108,6 +120,65 @@ describe("PortfolioQueueActionsSection (#4857)", () => { expect(onRequeue).toHaveBeenCalledWith(doneItem); }); + it("does not paginate the queue-actions table at or below 20 rows (#6831)", () => { + const items = manyActionItems(20); + render( + undefined} + onRequeue={() => undefined} + />, + ); + expect(screen.queryByRole("navigation", { name: /pagination/i })).toBeNull(); + expect(screen.getByText("acme/repo-00")).toBeTruthy(); + expect(screen.getByText("acme/repo-19")).toBeTruthy(); + }); + + it("paginates the queue-actions table client-side above 20 rows (#6831)", () => { + const items = manyActionItems(45); + render( + undefined} + onRequeue={() => undefined} + />, + ); + expect(screen.getByRole("navigation", { name: /pagination/i })).toBeTruthy(); + // Sorted in_progress first (even indices by name): page 1 ends at repo-38; repo-40 is the 21st. + expect(screen.getByText("acme/repo-00")).toBeTruthy(); + expect(screen.queryByText("acme/repo-40")).toBeNull(); + fireEvent.click(screen.getByRole("link", { name: "2" })); + expect(screen.getByText("acme/repo-40")).toBeTruthy(); + expect(screen.queryByText("acme/repo-00")).toBeNull(); + fireEvent.click(screen.getByRole("link", { name: /go to previous page/i })); + expect(screen.getByText("acme/repo-00")).toBeTruthy(); + fireEvent.click(screen.getByRole("link", { name: /go to next page/i })); + expect(screen.getByText("acme/repo-40")).toBeTruthy(); + }); + + it("breaks same-status/same-repo ties by identifier ascending (#6831)", () => { + const sameRepoDone: PortfolioQueueActionItem[] = [ + { ...doneItem, identifier: "issue:20" }, + { ...doneItem, identifier: "issue:7" }, + ]; + render( + undefined} + onRequeue={() => undefined} + />, + ); + const rows = screen.getAllByRole("row"); + expect(rows[1]?.textContent).toContain("issue:20"); + expect(rows[2]?.textContent).toContain("issue:7"); + }); + it("disables action buttons while an action is pending", () => { render( { await waitFor(() => expect(releaseItem).toHaveBeenCalledWith(inProgressItem)); }); + it("wires requeue through the injected action for done rows (#6831)", async () => { + const requeueItem = vi.fn(async () => ({ + ok: true as const, + entry: { repoFullName: "acme/widgets", identifier: "issue:7", status: "queued" }, + })); + render( + ({ ok: true as const, items: [doneItem] })} + requeueItem={requeueItem} + pollIntervalMs={60_000} + />, + ); + await waitFor(() => expect(screen.getByRole("button", { name: "Requeue" })).toBeTruthy()); + fireEvent.click(screen.getByRole("button", { name: "Requeue" })); + await waitFor(() => expect(requeueItem).toHaveBeenCalledWith(doneItem)); + }); + it("REGRESSION (#6090): a failing release action renders the error and does not re-fetch items as if it succeeded", async () => { const loadPortfolioQueueItems = vi.fn(async () => ({ ok: true as const, items: [inProgressItem] })); const releaseItem = vi.fn(async (): Promise => ({ diff --git a/apps/loopover-miner-ui/src/portfolio-queue.test.tsx b/apps/loopover-miner-ui/src/portfolio-queue.test.tsx index fb38a87ff0..d9bab4fdd6 100644 --- a/apps/loopover-miner-ui/src/portfolio-queue.test.tsx +++ b/apps/loopover-miner-ui/src/portfolio-queue.test.tsx @@ -1,4 +1,4 @@ -import { render, screen, waitFor } from "@testing-library/react"; +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; import { afterEach, describe, expect, it, vi } from "vitest"; import { @@ -6,6 +6,7 @@ import { PORTFOLIO_QUEUE_API_PATH, type PortfolioQueueResult, type PortfolioQueueSummary, + type PortfolioRepoSummary, } from "./lib/portfolio-queue"; import { PortfolioPage, PortfolioQueueView } from "./routes/portfolio"; import { handlePortfolioQueueRequest, type PortfolioQueueApiDeps } from "../vite-portfolio-queue-api"; @@ -70,6 +71,15 @@ describe("emptyPortfolioQueueSummary (#4306)", () => { }); }); +function manyRepos(count: number): PortfolioRepoSummary[] { + // Descending totals so the default sort (total desc) puts repo-00 first — mirrors ledgers' manyEventTypes. + return Array.from({ length: count }, (_, index) => ({ + repoFullName: `acme/repo-${String(index).padStart(2, "0")}`, + byStatus: { queued: count - index, in_progress: 0, done: 0 }, + total: count - index, + })); +} + describe("PortfolioQueueView (#4306, per-repo detail added by #4846)", () => { it("renders one card per status with the aggregated global counts", () => { render(); @@ -88,6 +98,65 @@ describe("PortfolioQueueView (#4306, per-repo detail added by #4846)", () => { expect(screen.getAllByRole("row")).toHaveLength(3); }); + it("renders a queue-by-status chart via the ui-kit ChartContainer (#6831)", () => { + render(); + expect(screen.getByLabelText("Queue by status chart")).toBeTruthy(); + }); + + it("does not paginate the per-repo table at or below 20 rows (#6831)", () => { + const repos = manyRepos(20); + const summary: PortfolioQueueSummary = { + total: 20, + byStatus: { queued: 20, in_progress: 0, done: 0 }, + repos, + oldestQueuedAgeMs: null, + }; + render(); + expect(screen.queryByRole("navigation", { name: /pagination/i })).toBeNull(); + expect(screen.getByText("acme/repo-00")).toBeTruthy(); + expect(screen.getByText("acme/repo-19")).toBeTruthy(); + }); + + it("paginates the per-repo table client-side above 20 rows, sorted by total desc (#6831)", () => { + const repos = manyRepos(45); + const summary: PortfolioQueueSummary = { + total: 45, + byStatus: { queued: 45, in_progress: 0, done: 0 }, + repos, + oldestQueuedAgeMs: null, + }; + render(); + expect(screen.getByRole("navigation", { name: /pagination/i })).toBeTruthy(); + // Highest total first: repo-00 has total 45, repo-20 has total 25 — only the first page is visible. + expect(screen.getByText("acme/repo-00")).toBeTruthy(); + expect(screen.queryByText("acme/repo-20")).toBeNull(); + fireEvent.click(screen.getByRole("link", { name: "2" })); + expect(screen.getByText("acme/repo-20")).toBeTruthy(); + expect(screen.queryByText("acme/repo-00")).toBeNull(); + // Previous / Next buttons also advance the page (covers both onClick arms). + fireEvent.click(screen.getByRole("link", { name: /go to previous page/i })); + expect(screen.getByText("acme/repo-00")).toBeTruthy(); + fireEvent.click(screen.getByRole("link", { name: /go to next page/i })); + expect(screen.getByText("acme/repo-20")).toBeTruthy(); + }); + + it("breaks equal-total ties by repo name ascending (#6831)", () => { + const summary: PortfolioQueueSummary = { + total: 4, + byStatus: { queued: 4, in_progress: 0, done: 0 }, + repos: [ + { repoFullName: "acme/zeta", byStatus: { queued: 2, in_progress: 0, done: 0 }, total: 2 }, + { repoFullName: "acme/alpha", byStatus: { queued: 2, in_progress: 0, done: 0 }, total: 2 }, + ], + oldestQueuedAgeMs: null, + }; + render(); + const rows = screen.getAllByRole("row"); + // header + alpha then zeta + expect(rows[1]?.textContent).toContain("acme/alpha"); + expect(rows[2]?.textContent).toContain("acme/zeta"); + }); + it("renders the fresh-install empty state without erroring", () => { render(); // #6511: asserted as the exact sentence, not a loose regex -- the whole original string is the EmptyState @@ -105,11 +174,14 @@ describe("PortfolioQueueView (#4306, per-repo detail added by #4846)", () => { expect(screen.getByRole("alert").textContent).toContain("connection refused"); }); - it("renders a content-shaped skeleton before the first result arrives", () => { + it("renders a content-shaped loading skeleton (role=status), not the old flat loading text (#6511, #6831)", () => { // #6511: StateBoundary renders the skeleton INSTEAD of a loading title, so the old // "Loading local portfolio queue…" text is intentionally gone; assert the placeholder instead. + // #6831: skeleton also announces via role=status (matching ledgers/run-history) and includes a chart-shaped bar. render(); + expect(screen.getByRole("status", { name: /loading local portfolio queue/i })).toBeTruthy(); expect(screen.getByTestId("portfolio-queue-skeleton")).toBeTruthy(); + expect(screen.queryByText("Loading local portfolio queue…")).toBeNull(); // Shaped like the real content, not one generic bar: the real table is not rendered yet. expect(screen.queryByRole("table")).toBeNull(); }); diff --git a/apps/loopover-miner-ui/src/routes/portfolio.tsx b/apps/loopover-miner-ui/src/routes/portfolio.tsx index 738471b9c4..2199c91f82 100644 --- a/apps/loopover-miner-ui/src/routes/portfolio.tsx +++ b/apps/loopover-miner-ui/src/routes/portfolio.tsx @@ -1,8 +1,18 @@ import { createFileRoute } from "@tanstack/react-router"; import { useCallback, useEffect, useState } from "react"; +import { Bar, BarChart, Cell, XAxis, YAxis } from "recharts"; import { Button } from "@loopover/ui-kit/components/button"; import { Card, CardContent, CardHeader } from "@loopover/ui-kit/components/card"; +import { ChartContainer, ChartTooltip, ChartTooltipContent, type ChartConfig } from "@loopover/ui-kit/components/chart"; +import { + Pagination, + PaginationContent, + PaginationItem, + PaginationLink, + PaginationNext, + PaginationPrevious, +} from "@loopover/ui-kit/components/pagination"; import { Skeleton } from "@loopover/ui-kit/components/skeleton"; import { StateBoundary } from "@loopover/ui-kit/components/state-views"; import { Table, TableBody, TableCell, TableHead, TableHeader, TableRow } from "@loopover/ui-kit/components/table"; @@ -16,7 +26,14 @@ import { type PortfolioQueueItemsResult, } from "../lib/portfolio-queue-actions"; import { DEFAULT_POLL_INTERVAL_MS, usePolledFetch } from "../lib/use-polled-fetch"; -import { fetchPortfolioQueue, type PortfolioQueueResult, type QueueStatus } from "../lib/portfolio-queue"; +import { + fetchPortfolioQueue, + QUEUE_STATUSES, + type PortfolioQueueResult, + type PortfolioRepoSummary, + type QueueStatus, + type QueueStatusCounts, +} from "../lib/portfolio-queue"; export const Route = createFileRoute("/portfolio")({ component: PortfolioPage, @@ -24,6 +41,17 @@ export const Route = createFileRoute("/portfolio")({ // Portfolio/queue summary cards + per-repo table (#4306, reunified with the CLI's own richer `queue dashboard` // by #4846), plus release/requeue controls (#4857) backed by the same store methods the CLI uses. +// +// #6511: the hand-rolled loading/error/empty `

` branches are replaced by the shared @loopover/ui-kit +// `StateBoundary`, with content-shaped `Skeleton` placeholders so the layout doesn't jump when the poll lands. +// The summary and the queue-actions section each keep their OWN independent boundary — a failure in one must +// not blank the other. +// +// #6831: status cards gain a ui-kit `ChartContainer` bar chart (so the bare numbers aren't the only signal), +// and both the per-repo table + the queue-actions table paginate client-side via the kit's `Pagination` once +// they exceed PAGE_SIZE rows — matching the ledgers restyle (#6832) and run-history (#6510). Purely +// presentational: `lib/portfolio-queue.ts` / `lib/portfolio-queue-actions.ts`, the poll/fetch loops, and the +// release/requeue Button wiring stay untouched. const STATUS_LABELS: Record = { queued: "Queued", @@ -37,13 +65,210 @@ const STATUS_TONE: Record = { done: "text-success", }; -/** Placeholder shaped like the real summary -- three status cards over the repo table -- so the layout doesn't - * jump when the 10s poll lands. A single generic bar would just move the jump later. */ +/** Rows per page once a repos/actions table grows past this; below it the full table renders unpaginated. */ +const PAGE_SIZE = 20; + +const QUEUE_CHART_CONFIG = { + count: { label: "Queue items" }, + queued: { label: "Queued", color: "var(--muted-foreground)" }, + in_progress: { label: "In progress", color: "var(--warning)" }, + done: { label: "Done", color: "var(--success)" }, +} satisfies ChartConfig; + +function TablePagination({ + page, + pageCount, + onPageChange, +}: { + page: number; + pageCount: number; + onPageChange: (next: number) => void; +}) { + return ( + + + + { + event.preventDefault(); + onPageChange(Math.max(0, page - 1)); + }} + /> + + {Array.from({ length: pageCount }).map((_, index) => ( + + { + event.preventDefault(); + onPageChange(index); + }} + > + {index + 1} + + + ))} + + = pageCount - 1} + onClick={(event) => { + event.preventDefault(); + onPageChange(Math.min(pageCount - 1, page + 1)); + }} + /> + + + + ); +} + +/** Horizontal bar chart of queue status counts — the chart.tsx adoption for the status cards section (#6831). + * Cards still show the exact numbers; the chart is the glanceable breakdown the bare `

`s alone weren't. */ +function QueueStatusChart({ byStatus }: { byStatus: QueueStatusCounts }) { + const data = QUEUE_STATUSES.map((status) => ({ + status, + label: STATUS_LABELS[status], + count: byStatus[status], + })); + return ( + + + + + } /> + + {data.map((entry) => ( + + ))} + + + + ); +} + +function ReposTable({ repos }: { repos: PortfolioRepoSummary[] }) { + const [page, setPage] = useState(0); + // Sorted by total desc (then name) so the busiest repos surface first — same "sort then page" shape as the + // ledgers CountTable (#6832), without inventing interactive column headers. + const sorted = [...repos].sort((a, b) => b.total - a.total || a.repoFullName.localeCompare(b.repoFullName)); + const pageCount = Math.max(1, Math.ceil(sorted.length / PAGE_SIZE)); + const isPaginated = sorted.length > PAGE_SIZE; + const safePage = Math.min(page, pageCount - 1); + const visible = isPaginated ? sorted.slice(safePage * PAGE_SIZE, safePage * PAGE_SIZE + PAGE_SIZE) : sorted; + return ( +
+ + + + Repository + Queued + In progress + Done + Total + + + + {visible.map((repo) => ( + + {repo.repoFullName} + {repo.byStatus.queued} + {repo.byStatus.in_progress} + {repo.byStatus.done} + {repo.total} + + ))} + +
+ {isPaginated && } +
+ ); +} + +function QueueActionsTable({ + items, + pending, + onRelease, + onRequeue, +}: { + items: PortfolioQueueActionItem[]; + pending: boolean; + onRelease: (item: PortfolioQueueActionItem) => void; + onRequeue: (item: PortfolioQueueActionItem) => void; +}) { + const [page, setPage] = useState(0); + // in_progress before done, then repo/identifier — actionable release rows float to the top of page 1. + const sorted = [...items].sort((a, b) => { + const statusOrder = (status: PortfolioQueueActionItem["status"]) => (status === "in_progress" ? 0 : 1); + return ( + statusOrder(a.status) - statusOrder(b.status) || + a.repoFullName.localeCompare(b.repoFullName) || + a.identifier.localeCompare(b.identifier) + ); + }); + const pageCount = Math.max(1, Math.ceil(sorted.length / PAGE_SIZE)); + const isPaginated = sorted.length > PAGE_SIZE; + const safePage = Math.min(page, pageCount - 1); + const visible = isPaginated ? sorted.slice(safePage * PAGE_SIZE, safePage * PAGE_SIZE + PAGE_SIZE) : sorted; + return ( +
+ + + + Repository + Identifier + Status + Action + + + + {visible.map((item) => ( + + {item.repoFullName} + {item.identifier} + {STATUS_LABELS[item.status]} + + {item.status === "in_progress" ? ( + + ) : ( + + )} + + + ))} + +
+ {isPaginated && } +
+ ); +} + +/** Placeholder shaped like the real summary -- three status cards, the status chart, and the repo table -- so + * the layout doesn't jump when the 10s poll lands. A single generic bar would just move the jump later. */ function PortfolioQueueSkeleton() { return ( -
+
- {(Object.keys(STATUS_LABELS) as QueueStatus[]).map((status) => ( + {QUEUE_STATUSES.map((status) => ( @@ -52,6 +277,7 @@ function PortfolioQueueSkeleton() { ))}
+
{[0, 1, 2].map((row) => ( @@ -83,7 +309,7 @@ export function PortfolioQueueView({ result }: { result: PortfolioQueueResult | {summary === null ? null : (
- {(Object.keys(STATUS_LABELS) as QueueStatus[]).map((status) => ( + {QUEUE_STATUSES.map((status) => (
@@ -96,28 +322,8 @@ export function PortfolioQueueView({ result }: { result: PortfolioQueueResult | ))}
- - - - Repository - Queued - In progress - Done - Total - - - - {summary.repos.map((repo) => ( - - {repo.repoFullName} - {repo.byStatus.queued} - {repo.byStatus.in_progress} - {repo.byStatus.done} - {repo.total} - - ))} - -
+ +
)} @@ -127,7 +333,12 @@ export function PortfolioQueueView({ result }: { result: PortfolioQueueResult | /** Placeholder shaped like the queue-actions table's rows, for the same reason as the summary's. */ function QueueActionsSkeleton() { return ( -
+
{[0, 1, 2].map((row) => ( ))} @@ -172,36 +383,7 @@ export function PortfolioQueueActionsSection({ emptyDescription={null} > {result === null || !result.ok || result.items.length === 0 ? null : ( - - - - Repository - Identifier - Status - Action - - - - {result.items.map((item) => ( - - {item.repoFullName} - {item.identifier} - {STATUS_LABELS[item.status]} - - {item.status === "in_progress" ? ( - - ) : ( - - )} - - - ))} - -
+ )}