feat: hydrate persisted PR details on task disclosure (#3055)

This commit is contained in:
Carlos Florêncio
2026-08-26 15:54:59 +00:00
committed by GitHub
parent 49dd944bd3
commit a6896bca9a
68 changed files with 1764 additions and 130 deletions
@@ -22,6 +22,7 @@ function renderWithStore(ui: ReactNode) {
function makeTaskPR(overrides: Partial<TaskPR> = {}): TaskPR {
return {
id: "pr-1",
workspace_id: "workspace-1",
task_id: "task-1",
owner: "o",
repo: "r",
@@ -103,6 +103,7 @@ function makeOptions(overrides: Partial<TaskCIAutomationOptions> = {}): TaskCIAu
function makePR(overrides: Partial<TaskPR> = {}): TaskPR {
return {
id: "id",
workspace_id: "workspace-1",
task_id: "task-1",
owner: "o",
repo: "r",
@@ -41,6 +41,7 @@ const UPDATING_LABEL = "Updating…";
function makePR(): TaskPR {
return {
id: "id",
workspace_id: "workspace-1",
task_id: "task-1",
owner: "acme",
repo: "app",
@@ -5,6 +5,7 @@ import type { CheckRun, TaskPR } from "@/lib/types/github";
function makePR(overrides: Partial<TaskPR> = {}): TaskPR {
return {
id: "id",
workspace_id: "workspace-1",
task_id: "task",
owner: "o",
repo: "r",
@@ -99,6 +99,7 @@ function PRCIPopoverHeader({
<ChangeRequestPopoverHeader
number={pr.pr_number}
title={title}
author={pr.author_login}
url={pr.pr_url}
onOpenReview={onOpenDetailPanel}
openDetailsLabel={t("github:openDetails", { title: `#${pr.pr_number} ${title}` })}
@@ -50,6 +50,7 @@ afterEach(() => {
function makeTaskPR(overrides: Partial<TaskPR> = {}): TaskPR {
return {
id: "id",
workspace_id: "workspace-1",
task_id: "task",
owner: "o",
repo: "r",
@@ -24,6 +24,7 @@ const reviewMocks = vi.hoisted(() => ({ requestReviewers: vi.fn(), submitReview:
const RE_REQUEST_BUTTON = "change-request-review-action-rerequest-review-octocat";
const PENDING_REVIEWER = "change-request-pending-reviewer-octocat";
const DISMISSED_REVIEWED_AT = "2026-01-01T00:00:00Z";
const WORKSPACE_ID = "workspace-1";
vi.mock("@/hooks/domains/github/use-pr-feedback", () => ({
usePRFeedback: () => ({
@@ -72,6 +73,7 @@ afterEach(async () => {
function makeTaskPR(overrides: Partial<TaskPR> = {}): TaskPR {
return {
id: "id",
workspace_id: WORKSPACE_ID,
task_id: "task",
owner: "o",
repo: "r",
@@ -141,7 +143,7 @@ function ReviewRequestHarness({
onReady,
requestedReviewers = [],
reviews = [],
workspaceId = "workspace-1",
workspaceId = WORKSPACE_ID,
}: {
onReady: (reviewRequest: ReturnType<typeof usePRScopedReviewRequest>) => void;
requestedReviewers?: { login: string; type: "user" }[];
@@ -171,7 +173,7 @@ function dismissedReview(id = 1): PRReview {
}
const TEST_INITIAL_STATE = {
workspaces: { activeId: "workspace-1" },
workspaces: { activeId: WORKSPACE_ID },
} as unknown as Partial<AppState>;
function prDetailTree(taskPR = makeTaskPR()) {
@@ -305,7 +307,7 @@ describe("PRDetailContent deferred re-request", () => {
"r",
1,
["octocat"],
"workspace-1",
WORKSPACE_ID,
);
expect(screen.queryByTestId(RE_REQUEST_BUTTON)).toBeNull();
});
@@ -172,7 +172,10 @@ describe("PRDetailPanelComponent — task switch on the singleton legacy panel",
await act(async () => {
getStoreApi().setState((prev) => ({
...prev,
taskPRs: { byTaskId: { ...prev.taskPRs.byTaskId, "task-2": [pr2] } },
taskPRs: {
...prev.taskPRs,
byTaskId: { ...prev.taskPRs.byTaskId, "task-2": [pr2] },
},
tasks: { ...prev.tasks, activeTaskId: "task-2", activeSessionId: "session-2" },
}));
});
@@ -13,6 +13,7 @@ const QUEUE_STATUS_TEST_ID = "pr-merge-queue-status";
function makePR(overrides: Partial<TaskPR> = {}): TaskPR {
return {
id: "id",
workspace_id: "workspace-1",
task_id: "task-1",
owner: "acme",
repo: "demo",
@@ -15,6 +15,7 @@ const CONFLICT_BANNER_TEST_ID = "pr-conflict-banner";
function makePR(overrides: Partial<TaskPR> = {}): TaskPR {
return {
id: "id",
workspace_id: "workspace-1",
task_id: "task-1",
owner: "o",
repo: "r",
@@ -64,6 +64,7 @@ function renderWithStore(initialState: Partial<AppState>, ui: ReactNode) {
function makePR(overrides: Partial<TaskPR> = {}): TaskPR {
return {
id: "pr-id",
workspace_id: "workspace-1",
task_id: "task-1",
owner: "acme",
repo: "demo",
@@ -3,6 +3,7 @@ import type { TaskCIAutomationOptions, TaskPR } from "@/lib/types/github";
export function makeTestPR(overrides: Partial<TaskPR> = {}): TaskPR {
return {
id: "pr-id",
workspace_id: "workspace-1",
task_id: "task-1",
owner: "acme",
repo: "demo",
@@ -259,6 +259,7 @@ describe("PRStatusChip mobile branch", () => {
expect(document.querySelector("[data-testid='pr-topbar-popover-inner']")).not.toBeNull();
expect(document.querySelector("[data-testid='pr-status-chip-drawer-close']")).not.toBeNull();
expect(screen.getByTestId("pr-popover-title").textContent).toBe("#42 Test PR");
expect(screen.getByTestId("pr-popover-author").textContent).toBe("by alice");
expect(drawer?.textContent).not.toContain("Open PR details");
});
@@ -76,6 +76,7 @@ function renderWithStore(initialState: Partial<AppState>, ui: ReactNode) {
function makePR(overrides: Partial<TaskPR> = {}): TaskPR {
return {
id: "pr-id",
workspace_id: "workspace-1",
task_id: "task-1",
owner: "acme",
repo: "demo",
@@ -6,6 +6,7 @@ import { derivePRTaskStatusSummary } from "./pr-task-status-summary";
function draftPR(): TaskPR {
return {
id: "id",
workspace_id: "workspace-1",
task_id: "task",
owner: "o",
repo: "r",
@@ -1,12 +1,21 @@
import { afterEach, describe, expect, it } from "vitest";
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import type { ReactNode } from "react";
import { cleanup, render } from "@testing-library/react";
import { act, cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react";
import { TooltipProvider } from "@kandev/ui/tooltip";
import { StateProvider } from "@/components/state-provider";
import { TaskContributionIcons } from "@/components/task/task-contribution-icons";
import { PRTaskIcon } from "./pr-task-icon";
import type { AppState } from "@/lib/state/store";
import type { TaskPR } from "@/lib/types/github";
const listTaskPRsMock = vi.hoisted(() => vi.fn());
const TASK_ID = "task-1";
const WORKSPACE_ID = "workspace-1";
vi.mock("@/lib/api/domains/github-api", () => ({
listTaskPRs: listTaskPRsMock,
}));
function renderWithStore(initialState: Partial<AppState> | undefined, ui: ReactNode) {
return render(
<StateProvider initialState={initialState}>
@@ -18,7 +27,8 @@ function renderWithStore(initialState: Partial<AppState> | undefined, ui: ReactN
function makePR(overrides: Partial<TaskPR> = {}): TaskPR {
return {
id: "id",
task_id: "task-1",
workspace_id: WORKSPACE_ID,
task_id: TASK_ID,
owner: "o",
repo: "r",
pr_number: 1,
@@ -48,18 +58,22 @@ function makePR(overrides: Partial<TaskPR> = {}): TaskPR {
};
}
beforeEach(() => {
listTaskPRsMock.mockReset().mockReturnValue(new Promise(() => {}));
});
afterEach(() => cleanup());
describe("PRTaskIcon corrupted store entry", () => {
// Regression: an upstream payload (partial hydration, WS reorder, etc.) once
// landed in taskPRs.byTaskId["task-1"] as a non-array truthy value. The
// landed in taskPRs.byTaskId[TASK_ID] as a non-array truthy value. The
// length-based guards then fell through into MultiPRIcon, where for-of
// threw `prs is not iterable`. PRTaskIcon must bail rather than crash.
it("renders nothing when byTaskId[taskId] is a non-array object", () => {
const { container } = renderWithStore(
// eslint-disable-next-line @typescript-eslint/no-explicit-any
{ taskPRs: { byTaskId: { "task-1": {} as any } } } as Partial<AppState>,
<PRTaskIcon taskId="task-1" />,
{ taskPRs: { byTaskId: { [TASK_ID]: {} as any } } } as Partial<AppState>,
<PRTaskIcon taskId={TASK_ID} />,
);
expect(container.firstChild).toBeNull();
});
@@ -71,10 +85,10 @@ describe("PRTaskIcon corrupted store entry", () => {
it("renders an icon when byTaskId[taskId] is a valid array of one PR", () => {
const { container } = renderWithStore(
{ taskPRs: { byTaskId: { "task-1": [makePR()] } } },
<PRTaskIcon taskId="task-1" />,
{ taskPRs: { byTaskId: { [TASK_ID]: [makePR()] } } },
<PRTaskIcon taskId={TASK_ID} />,
);
expect(container.querySelector('[data-testid="pr-task-icon-task-1"]')).not.toBeNull();
expect(container.querySelector(`[data-testid="pr-task-icon-${TASK_ID}"]`)).not.toBeNull();
});
it("renders the multi-PR icon when byTaskId[taskId] has multiple PRs", () => {
@@ -82,17 +96,84 @@ describe("PRTaskIcon corrupted store entry", () => {
{
taskPRs: {
byTaskId: {
"task-1": [
[TASK_ID]: [
makePR({ id: "a", repository_id: "repo-a", pr_number: 1 }),
makePR({ id: "b", repository_id: "repo-b", pr_number: 2 }),
],
},
},
},
<PRTaskIcon taskId="task-1" />,
<PRTaskIcon taskId={TASK_ID} />,
);
const icon = container.querySelector('[data-testid="pr-task-icon-task-1"]');
const icon = container.querySelector(`[data-testid="pr-task-icon-${TASK_ID}"]`);
expect(icon).not.toBeNull();
expect(icon?.getAttribute("data-pr-count")).toBe("2");
expect(icon?.getAttribute("data-pr-ready-to-merge")).toBe("false");
});
it("opens a loading disclosure for a compact PR projection", () => {
renderWithStore(
{ workspaces: { items: [], activeId: WORKSPACE_ID } },
<TaskContributionIcons
taskId={TASK_ID}
prInfo={{ number: 7, state: "open", aggregateState: "pending" }}
/>,
);
const icon = screen.getByTestId(`pr-task-icon-${TASK_ID}`);
expect(icon.getAttribute("role")).toBe("img");
fireEvent.pointerEnter(icon, { pointerType: "mouse" });
expect(screen.getAllByTestId("pr-task-tooltip-loading").length).toBeGreaterThan(0);
});
it("opens a loading disclosure when a compact PR projection receives keyboard focus", () => {
renderWithStore(
{ workspaces: { items: [], activeId: WORKSPACE_ID } },
<TaskContributionIcons
taskId={TASK_ID}
prInfo={{ number: 7, state: "open", aggregateState: "pending" }}
/>,
);
const icon = screen.getByTestId(`pr-task-icon-${TASK_ID}`);
const matches = vi.spyOn(icon, "matches").mockReturnValue(true);
fireEvent.focus(icon);
matches.mockRestore();
expect(screen.getAllByTestId("pr-task-tooltip-loading").length).toBeGreaterThan(0);
});
it("keeps keyboard focus and the open tooltip when hydration completes", async () => {
let resolveResponse!: (value: { task_prs: Record<string, TaskPR[]> }) => void;
const response = new Promise<{ task_prs: Record<string, TaskPR[]> }>((resolve) => {
resolveResponse = resolve;
});
listTaskPRsMock.mockReturnValue(response);
renderWithStore(
{ workspaces: { items: [], activeId: WORKSPACE_ID } },
<TaskContributionIcons
taskId={TASK_ID}
prInfo={{ number: 7, state: "open", aggregateState: "pending" }}
/>,
);
const icon = screen.getByTestId(`pr-task-icon-${TASK_ID}`);
const matches = vi.spyOn(icon, "matches").mockReturnValue(true);
icon.focus();
matches.mockRestore();
await waitFor(() =>
expect(screen.getAllByTestId("pr-task-tooltip-loading").length).toBeGreaterThan(0),
);
await act(async () => {
resolveResponse({ task_prs: { [TASK_ID]: [makePR()] } });
await response;
});
await waitFor(() =>
expect(screen.getAllByTestId("pr-task-status-summary").length).toBeGreaterThan(0),
);
expect(document.activeElement).toBe(screen.getByTestId(`pr-task-icon-${TASK_ID}`));
});
});
@@ -13,8 +13,8 @@ import {
} from "./pr-task-icon";
import type { TaskPR } from "@/lib/types/github";
const SKY_400 = "text-sky-400";
const RED_500 = "text-red-500";
const SKY_400 = "text-sky-400",
RED_500 = "text-red-500";
const YELLOW_500 = "text-yellow-500";
const EMERALD_400 = "text-emerald-400";
const GREEN_500 = "text-green-500";
@@ -51,7 +51,7 @@ function makePR(overrides: Partial<TaskPR> = {}): TaskPR {
closed_at: null,
last_synced_at: null,
updated_at: "",
...overrides,
...{ workspace_id: "workspace-1", ...overrides },
};
}
+122 -51
View File
@@ -11,6 +11,11 @@ import {
CHANGE_REQUEST_STATUS_RANK,
getChangeRequestAggregateStatusColor,
} from "@/components/integrations/change-request-task-status-color";
import {
getTaskPRsForCurrentWorkspace,
useTaskPRTooltipHydration,
type TaskPRTooltipHydrationStatus,
} from "@/hooks/domains/github/use-task-pr-tooltip-hydration";
import type { TaskPR } from "@/lib/types/github";
import { derivePRTaskStatusSummary, PRTaskStatusSummary } from "./pr-task-status-summary";
@@ -233,40 +238,114 @@ export function pickDefaultPR(prs: TaskPR[]): TaskPR | null {
return best;
}
export function PRTaskIcon({ taskId }: { taskId: string }) {
const prs = useAppStore((state) => state.taskPRs.byTaskId[taskId] ?? null);
export type TaskPRInfo = {
number: number;
state: string;
aggregateState?: string;
};
export function PRTaskIcon({ taskId, prInfo }: { taskId: string; prInfo?: TaskPRInfo }) {
const prs = useAppStore((state) => getTaskPRsForCurrentWorkspace(state, taskId));
const hydration = useTaskPRTooltipHydration(taskId);
const fullPRs = Array.isArray(prs) && prs.length > 0 ? prs : [];
// Defensive: an upstream payload may briefly seed byTaskId[taskId] with a
// non-array value (e.g. an empty object from a partial hydration). Bail
// instead of falling through into MultiPRIcon, where for-of throws.
if (!Array.isArray(prs) || prs.length === 0) return null;
if (prs.length === 1) return <SinglePRIcon taskId={taskId} pr={prs[0]} />;
return <MultiPRIcon taskId={taskId} prs={prs} />;
// instead of falling through into a full-data summary, where for-of throws.
if (fullPRs.length === 0 && !prInfo) return null;
return <PRTaskIconView taskId={taskId} prInfo={prInfo} prs={fullPRs} hydration={hydration} />;
}
function SinglePRIcon({ taskId, pr }: { taskId: string; pr: TaskPR }) {
type TaskPRIconPresentation = {
hasFullData: boolean;
singlePR: TaskPR | null;
readyToMerge: boolean;
allReadyToMerge: boolean;
summaries: ReturnType<typeof derivePRTaskStatusSummary>[];
iconColor: string;
displayState: string | undefined;
displayCount: number;
};
function getTaskPRIconPresentation(prs: TaskPR[], prInfo?: TaskPRInfo): TaskPRIconPresentation {
const hasFullData = prs.length > 0;
const singlePR = prs.length === 1 ? prs[0] : null;
const readyToMerge = singlePR ? isPRReadyToMerge(singlePR) : false;
return {
hasFullData,
singlePR,
readyToMerge,
allReadyToMerge: areAllOpenPRsReadyToMerge(prs),
summaries: prs.map((pr) => derivePRTaskStatusSummary(pr, isPRReadyToMerge(pr))),
iconColor: getTaskPRIconColor(prs, prInfo),
displayState: singlePR?.state ?? (hasFullData ? undefined : prInfo?.state),
displayCount: hasFullData ? prs.length : 1,
};
}
function PRTaskIconView({
taskId,
prInfo,
prs,
hydration,
}: {
taskId: string;
prInfo?: TaskPRInfo;
prs: TaskPR[];
hydration: ReturnType<typeof useTaskPRTooltipHydration>;
}) {
const { t } = useTranslation();
const tooltip = useChangeRequestTaskTooltipState();
const readyToMerge = isPRReadyToMerge(pr);
const summary = derivePRTaskStatusSummary(pr, readyToMerge);
const hasFullData = prs.length > 0;
const tooltip = useChangeRequestTaskTooltipState(
!hasFullData && prInfo
? () => {
void hydration.hydrate();
}
: undefined,
);
const {
singlePR,
readyToMerge,
allReadyToMerge,
summaries,
iconColor,
displayState,
displayCount,
} = getTaskPRIconPresentation(prs, prInfo);
const ariaLabel =
prs.length > 1
? t("github:pullRequestStatuses", { count: prs.length })
: t("github:pullRequestStatus", { number: singlePR?.pr_number ?? prInfo?.number });
return (
<Tooltip open={tooltip.open}>
<TooltipTrigger asChild>
<span
data-testid={`pr-task-icon-${taskId}`}
data-pr-state={pr.state}
data-pr-count="1"
data-pr-ready-to-merge={readyToMerge ? "true" : "false"}
data-pr-state={displayState}
data-pr-count={displayCount}
data-pr-ready-to-merge={
hasFullData ? String(prs.length === 1 ? readyToMerge : allReadyToMerge) : undefined
}
role="img"
tabIndex={0}
aria-label={t("github:pullRequestStatus", { number: pr.pr_number })}
aria-label={ariaLabel}
onPointerEnter={tooltip.onPointerEnter}
onPointerLeave={tooltip.onPointerLeave}
onFocus={tooltip.onFocus}
onBlur={tooltip.onBlur}
className={cn("inline-flex items-center shrink-0", getPRStatusColor(pr))}
className={cn(
"inline-flex items-center shrink-0",
prs.length > 1 && "gap-0.5",
iconColor,
)}
>
<IconGitPullRequest aria-hidden="true" className="h-3.5 w-3.5" />
{prs.length > 1 ? (
<span className="text-[9px] font-semibold leading-none tabular-nums">{prs.length}</span>
) : null}
</span>
</TooltipTrigger>
<TooltipContent
@@ -274,45 +353,37 @@ function SinglePRIcon({ taskId, pr }: { taskId: string; pr: TaskPR }) {
onEscapeKeyDown={tooltip.onEscapeKeyDown}
className="w-80 max-w-[calc(100vw-1rem)] p-3"
>
<PRTaskStatusSummary summaries={[summary]} />
{hasFullData ? (
<PRTaskStatusSummary summaries={summaries} />
) : (
<CompactPRTooltipContent status={hydration.status} />
)}
</TooltipContent>
</Tooltip>
);
}
function MultiPRIcon({ taskId, prs }: { taskId: string; prs: TaskPR[] }) {
const { t } = useTranslation();
const tooltip = useChangeRequestTaskTooltipState();
const aggregateColor = aggregatePRStatusColor(prs);
const allReady = areAllOpenPRsReadyToMerge(prs);
const summaries = prs.map((pr) => derivePRTaskStatusSummary(pr, isPRReadyToMerge(pr)));
return (
<Tooltip open={tooltip.open}>
<TooltipTrigger asChild>
<span
data-testid={`pr-task-icon-${taskId}`}
data-pr-count={prs.length}
data-pr-ready-to-merge={allReady ? "true" : "false"}
role="img"
tabIndex={0}
aria-label={t("github:pullRequestStatuses", { count: prs.length })}
onPointerEnter={tooltip.onPointerEnter}
onPointerLeave={tooltip.onPointerLeave}
onFocus={tooltip.onFocus}
onBlur={tooltip.onBlur}
className={cn("inline-flex items-center gap-0.5 shrink-0", aggregateColor)}
>
<IconGitPullRequest aria-hidden="true" className="h-3.5 w-3.5" />
<span className="text-[9px] font-semibold leading-none tabular-nums">{prs.length}</span>
</span>
</TooltipTrigger>
<TooltipContent
sideOffset={6}
onEscapeKeyDown={tooltip.onEscapeKeyDown}
className="w-80 max-w-[calc(100vw-1rem)] p-3"
>
<PRTaskStatusSummary summaries={summaries} />
</TooltipContent>
</Tooltip>
);
function getTaskPRIconColor(prs: TaskPR[], prInfo?: TaskPRInfo): string {
if (prs.length === 1) return getPRStatusColor(prs[0]);
if (prs.length > 1) return aggregatePRStatusColor(prs);
return getPRAggregateStatusColor(prInfo?.aggregateState ?? prInfo?.state);
}
function CompactPRTooltipContent({ status }: { status: TaskPRTooltipHydrationStatus }) {
const { t } = useTranslation();
if (status === "loading" || status === "idle") {
return (
<span data-testid="pr-task-tooltip-loading" className="text-sm text-muted-foreground">
{t("github:taskPrDetailsLoading")}
</span>
);
}
if (status === "unavailable") {
return (
<span data-testid="pr-task-tooltip-unavailable" className="text-sm text-muted-foreground">
{t("github:taskPrDetailsUnavailable")}
</span>
);
}
return null;
}
@@ -5,6 +5,7 @@ import { derivePRTaskStatusSummary } from "./pr-task-status-summary";
function makePR(overrides: Partial<TaskPR> = {}): TaskPR {
return {
id: "pr-id",
workspace_id: "workspace-1",
task_id: "task-id",
owner: "kdlbs",
repo: "kandev",
@@ -48,6 +49,7 @@ describe("structured PR task status summary", () => {
expect(summary).toEqual({
number: 2966,
title: "Make pull request status easier to scan",
author: "octocat",
rows: [
{ kind: "review", status: "approved", tone: "success" },
{ kind: "ci", status: "passed", tone: "success" },
@@ -133,6 +135,12 @@ describe("structured PR task status summary", () => {
expect(summary.rows).toEqual([{ kind: "merge", status: "draft", tone: "muted" }]);
});
it("omits an empty author identity", () => {
const summary = derivePRTaskStatusSummary(makePR({ author_login: " " }), false);
expect(summary).not.toHaveProperty("author");
});
});
describe("queued PR task status summaries", () => {
@@ -100,7 +100,13 @@ export function derivePRTaskStatusSummary(
deriveMergeRow(pr, readyToMerge),
].filter((row): row is PRTaskSummaryRow => row !== null);
return { number: pr.pr_number, title: pr.pr_title, rows };
const author = pr.author_login.trim();
return {
number: pr.pr_number,
title: pr.pr_title,
...(author ? { author } : {}),
rows,
};
}
export function PRTaskStatusSummary({ summaries }: { summaries: PRTaskStatusSummaryData[] }) {