Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions apps/web/src/components/pullRequest/pullRequestList.logic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { describe, expect, it } from "vite-plus/test";
import {
filterPullRequestsByInvolvement,
findScopedProject,
buildPullRequestPartitionTargets,
mergePullRequestLists,
pullRequestEntryKey,
pullRequestEnvironmentSetKey,
Expand Down Expand Up @@ -1496,3 +1497,88 @@ describe("the priority groups against a paginated feed", () => {
).toEqual([6123]);
});
});

describe("the priority groups' deferred reads", () => {
const base = {
wanted: true,
armed: true,
hasBaselineRows: true,
environments: [{ environmentId: ENV_1 }],
state: "open" as const,
limit: 99,
};

it("stays unread until armed, so deleting the guard fails without it", () => {
expect(buildPullRequestPartitionTargets({ ...base, armed: false })).toEqual({
authored: [],
reviewing: [],
});
});

it("stays unread without baseline rows or when unwanted", () => {
expect(buildPullRequestPartitionTargets({ ...base, hasBaselineRows: false })).toEqual({
authored: [],
reviewing: [],
});
expect(buildPullRequestPartitionTargets({ ...base, wanted: false })).toEqual({
authored: [],
reviewing: [],
});
});

it("builds one server-filtered read per involvement once armed", () => {
const targets = buildPullRequestPartitionTargets(base);
expect(targets.authored).toHaveLength(1);
expect(targets.reviewing).toHaveLength(1);
expect(targets.authored[0]).toMatchObject({
environmentId: ENV_1,
input: { state: "open", involvement: "authored", limit: 99 },
});
expect(targets.reviewing[0]).toMatchObject({
environmentId: ENV_1,
input: { state: "open", involvement: "reviewing", limit: 99 },
});
});

it("fans scoped project, host, and filters out to every environment in feed field order", () => {
const filters = { draft: "hide" } as const;
const targets = buildPullRequestPartitionTargets({
...base,
environments: [{ environmentId: ENV_1 }, { environmentId: ENV_2 }],
scopedProjectId: "project-1" as ProjectId,
host: "github.com",
filters,
});
expect(targets.authored.map((target) => target.environmentId)).toEqual([ENV_1, ENV_2]);
expect(targets.reviewing.map((target) => target.environmentId)).toEqual([ENV_1, ENV_2]);
for (const [involvement, group] of [
["authored", targets.authored],
["reviewing", targets.reviewing],
] as const) {
for (const target of group) {
expect(target.input).toEqual({
state: "open",
involvement,
limit: 99,
projectId: "project-1",
host: "github.com",
filters,
});
expect(Object.keys(target.input)).toEqual([
"state",
"involvement",
"limit",
"projectId",
"host",
"filters",
]);
expect(target.input.projectIds).toBeUndefined();
}
}
});

// No column render/targets assertion: arming lives in the route's unexported column wiring
// (pointer/focus + idle callback into query atoms), which needs the full environment/query
// harness to mount; asserting the JSX prop alone would not prove a fetch, so the gating
// above carries the guarantee.
});
68 changes: 68 additions & 0 deletions apps/web/src/components/pullRequest/pullRequestList.logic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,74 @@ export function partitionPullRequestsWithPriority<Entry extends PullRequestListE
.map((group) => ({ ...group, label: GROUP_LABELS[group.key] }));
}

/**
* One environment the priority groups can be asked of, with the project split the feed uses.
* Kept structural (rather than reusing the state's query-target type) so the page's gating
* stays a pure function of its inputs and unit-testable without the query layer.
*/
export interface PullRequestPartitionEnvironment {
readonly environmentId: EnvironmentId;
readonly projectIds?: ReadonlyArray<ProjectId>;
}

export interface PullRequestPartitionListTarget {
readonly environmentId: EnvironmentId;
readonly input: {
readonly state: PullRequestListState;
readonly involvement: PullRequestInvolvement;
readonly limit: number;
readonly projectId?: ProjectId;
readonly projectIds?: ReadonlyArray<ProjectId>;
readonly host?: string;
readonly filters?: PullRequestListFilters;
};
}

/**
* Which server-filtered reads the priority groups need. Unarmed (before engagement or idle),
* unwanted (a search re-ranks the whole list), or with no baseline rows, both stay unread and
* the groups fall back to local grouping. Built in the feed input's field order so the atoms
* stay keyed alike and the Authored/Reviewing tabs answer from this same read.
*
* The main list goes first: besides putting the visible rows on screen sooner, it proves which
* repositories the host search indexes, so an empty partition does not trigger the expensive
* per-repository fallback. With no baseline rows at all both partitions are already empty.
*/
export function buildPullRequestPartitionTargets(args: {
readonly wanted: boolean;
readonly armed: boolean;
readonly hasBaselineRows: boolean;
readonly environments: ReadonlyArray<PullRequestPartitionEnvironment>;
readonly state: PullRequestListState;
readonly limit: number;
readonly scopedProjectId?: ProjectId;
readonly host?: string;
readonly filters?: PullRequestListFilters;
}): {
readonly authored: ReadonlyArray<PullRequestPartitionListTarget>;
readonly reviewing: ReadonlyArray<PullRequestPartitionListTarget>;
} {
if (!args.wanted || !args.armed || !args.hasBaselineRows) {
return { authored: [], reviewing: [] };
}
const targetsFor = (
involvement: PullRequestInvolvement,
): ReadonlyArray<PullRequestPartitionListTarget> =>
args.environments.map(({ environmentId, projectIds }) => ({
environmentId,
input: {
state: args.state,
involvement,
limit: args.limit,
...(args.scopedProjectId ? { projectId: args.scopedProjectId } : {}),
...(projectIds ? { projectIds } : {}),
...(args.host ? { host: args.host } : {}),
...(args.filters ? { filters: args.filters } : {}),
},
}));
return { authored: targetsFor("authored"), reviewing: targetsFor("reviewing") };
}

export type PullRequestDiffStats = ReadonlyMap<
string,
{ readonly additions: number; readonly deletions: number }
Expand Down
74 changes: 74 additions & 0 deletions apps/web/src/routes/_chat.pull-requests.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import { act } from "react";
import { create, type ReactTestRenderer } from "react-test-renderer";
import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test";

import { PullRequestsColumn } from "./_chat.pull-requests";

let renderer: ReactTestRenderer | null = null;

beforeEach(() => {
// Same stub shape as useComposerMenuState.test.tsx: no DOM, just an event target.
// react-test-renderer needs no host nodes, but the column's effects touch
// window and IntersectionObserver, so those get minimal no-op stubs.
vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true);
vi.stubGlobal("window", { addEventListener() {}, removeEventListener() {} });
vi.stubGlobal(
"IntersectionObserver",
class {
observe() {}
unobserve() {}
disconnect() {}
},
);
});

afterEach(async () => {
await act(() => renderer?.unmount());
renderer = null;
vi.unstubAllGlobals();
});

describe("PullRequestsColumn partitions intent", () => {
it("forwards pointer and focus warmers to onPartitionsIntent", async () => {
const onPartitionsIntent = vi.fn();
await act(() => {
renderer = create(
<PullRequestsColumn
refreshing={false}
onRefresh={() => {}}
searchValue=""
involvement="all"
state="open"
host={undefined}
hostMenuOptions={[]}
onInvolvement={() => {}}
onPartitionsIntent={onPartitionsIntent}
onState={() => {}}
onHost={() => {}}
searchInput={null}
sortMenu={null}
filtersMenu={null}
rightPanelControl={null}
titlebarControls={null}
rightPanelOpen={false}
listBody={null}
scrollRef={{ current: null }}
/>,
);
});

const outer = renderer!.root.findAllByType("div")[0]!;
expect(typeof outer.props.onPointerEnter).toBe("function");
expect(typeof outer.props.onFocusCapture).toBe("function");

await act(() => {
outer.props.onPointerEnter();
});
expect(onPartitionsIntent).toHaveBeenCalledTimes(1);

await act(() => {
outer.props.onFocusCapture();
});
expect(onPartitionsIntent).toHaveBeenCalledTimes(2);
});
});
96 changes: 59 additions & 37 deletions apps/web/src/routes/_chat.pull-requests.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import {
import {
filterPullRequestsByInvolvement,
findScopedProject,
buildPullRequestPartitionTargets,
collectPullRequestListFacets,
groupPullRequestsByInvolvement,
matchesPullRequestFilters,
Expand Down Expand Up @@ -760,43 +761,53 @@ function PullRequestsRouteView() {
// so no partitions are read for one. These are the same atoms the Authored and Reviewing tabs
// ask for, so switching to either is answered from cache.
const partitionsWanted = search.involvement === "all" && typedQuery.length === 0;
// The two reads below stay off the first-paint path. The feed renders from the single `all`
// read — grouped locally, or from the snapshot's held groups — and these follow once the
// reader engages (pointer or focus reaching the column, one gesture from the tabs) or the
// browser idles. A cold open therefore spends one host search to first paint instead of
// three, while a later tab switch still answers from a warmed cache.
// Cost window: a pre-arm tab switch pays one fetch per environment (the partitions were never
// warmed); post-arm the same switch is a cache hit on the atoms below, keyed like the tabs.
const [partitionsArmed, setPartitionsArmed] = useState(false);
const armPartitions = useCallback(() => setPartitionsArmed(true), []);
useEffect(() => {
if (!partitionsWanted || partitionsArmed || typeof window === "undefined") return;
if (typeof window.requestIdleCallback === "function") {
const id = window.requestIdleCallback(armPartitions, { timeout: 2000 });
return () => window.cancelIdleCallback(id);
}
const timer = window.setTimeout(armPartitions, 1500);
return () => window.clearTimeout(timer);
}, [armPartitions, partitionsWanted, partitionsArmed]);
// Built together so the two reads share one memo, and in the same field order the feed's own
// input uses: the atoms are keyed by their input, so the Authored tab then reads this answer.
const partitionTargets = useMemo(() => {
// The main list goes first. Besides putting the visible rows on screen sooner, it proves
// which repositories the host search indexes, so an empty partition does not trigger the
// expensive per-repository fallback. With no rows at all both partitions are already empty.
if (
!partitionsWanted ||
baselineQuery.data === null ||
baselineQuery.data.entries.length === 0
) {
return { authored: NO_LIST_TARGETS, reviewing: NO_LIST_TARGETS };
}
const targetsFor = (involvement: PullRequestInvolvement) =>
environmentQueries.map(({ environmentId, projectIds }) => ({
environmentId,
input: {
state: search.state,
involvement,
limit: PAGE_SIZE,
...(scopedProjectId ? { projectId: scopedProjectId } : {}),
...(projectIds ? { projectIds } : {}),
...(search.host ? { host: search.host } : {}),
...(menuFiltered ? { filters: menuFilters } : {}),
} satisfies PullRequestListInput,
}));
return { authored: targetsFor("authored"), reviewing: targetsFor("reviewing") };
}, [
menuFiltered,
menuFilters,
partitionsWanted,
baselineQuery.data,
environmentQueries,
scopedProjectId,
search.host,
search.state,
]);
// Gating lives in `buildPullRequestPartitionTargets` (unit-tested): unarmed, unwanted, or
// with no baseline rows, both stay unread and the groups fall back to local grouping.
const partitionTargets = useMemo(
() =>
buildPullRequestPartitionTargets({
wanted: partitionsWanted,
armed: partitionsArmed,
hasBaselineRows: baselineQuery.data !== null && baselineQuery.data.entries.length > 0,
environments: environmentQueries,
state: search.state,
limit: PAGE_SIZE,
...(scopedProjectId ? { scopedProjectId } : {}),
...(search.host ? { host: search.host } : {}),
...(menuFiltered ? { filters: menuFilters } : {}),
}),
[
menuFiltered,
menuFilters,
partitionsWanted,
partitionsArmed,
baselineQuery.data,
environmentQueries,
scopedProjectId,
search.host,
search.state,
],
);
const authoredQuery = usePullRequestList(partitionTargets.authored);
const reviewingQuery = usePullRequestList(partitionTargets.reviewing);
// The header's refresh punches through the server's cache before re-reading; the error and
Expand Down Expand Up @@ -1807,6 +1818,7 @@ function PullRequestsRouteView() {
host: search.host,
hostMenuOptions,
onInvolvement: (involvement: PullRequestInvolvement) => updateListScope({ involvement }),
onPartitionsIntent: armPartitions,
onState: (state: PullRequestListState) => updateListScope({ state }),
onHost: (host: string | undefined) => updateListScope({ host }),
searchInput,
Expand Down Expand Up @@ -2193,7 +2205,7 @@ function ExpandableSearch({
* window drag region throughout; its interactive children opt out through the `.drag-region`
* descendant rules.
*/
function PullRequestsColumn({
export function PullRequestsColumn({
refreshing,
onRefresh,
searchValue,
Expand All @@ -2202,6 +2214,7 @@ function PullRequestsColumn({
host,
hostMenuOptions,
onInvolvement,
onPartitionsIntent,
onState,
onHost,
searchInput,
Expand All @@ -2221,6 +2234,7 @@ function PullRequestsColumn({
host: string | undefined;
hostMenuOptions: ReadonlyArray<PullRequestFilterOption<string>>;
onInvolvement: (involvement: PullRequestInvolvement) => void;
onPartitionsIntent: () => void;
onState: (state: PullRequestListState) => void;
onHost: (host: string | undefined) => void;
searchInput: ReactNode;
Expand Down Expand Up @@ -2290,7 +2304,15 @@ function PullRequestsColumn({
return (
// Painted flat like the chat column: the inset underneath carries the chrome grain, and a
// content surface that lets it show reads as a different background than every thread.
<div className="@container/pr-list flex min-h-0 min-w-0 flex-1 flex-col bg-background">
// Reaching the column means its tabs and priority groups are one gesture away, so their
// deferred reads warm here rather than on first paint. The signal is deliberately broad
// (whole column): narrowing to the tab strip/scroll viewport is follow-up if profiling
// ever shows reason to narrow it.
<div
className="@container/pr-list flex min-h-0 min-w-0 flex-1 flex-col bg-background"
onPointerEnter={onPartitionsIntent}
onFocusCapture={onPartitionsIntent}
>
{/* A closed right panel leaves this column full-width, so the shared header
reserves native window controls and hosts the controls strip itself: on
desktop the header is a drag-region, and only a no-drag descendant wins
Expand Down
Loading