authorgravatar for julius0216@outlook.comJulius Marminge <julius0216@outlook.com> 2026-10-07 17:26:25-07:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-10-07 17:26:25-07:00
loga21ac5fb153e3bf418ccd2a4abef2787c241813c
treed939c6da0f995dc3e06479dad8491799a4abb538
parent08b1222d0bae670433c275ec70165c7b5048726f
signature Signed by PGP key B5690EEEBB952194

refactor(server): GitHub GraphQL batches use variables and share one pager (#16960)

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

7 files changed, 565 insertions(+), 255 deletions(-)

apps/server/src/pullRequest/GitHubPullRequestApi.test.ts+87-20
...@@ -767,11 +767,15 @@ layer("GitHubPullRequestApi.layer", (it) => {...@@ -767,11 +767,15 @@ layer("GitHubPullRequestApi.layer", (it) => {
767 );767 );
768 assert.strictEqual(eight?.headBranch, "feat/8");768 assert.strictEqual(eight?.headBranch, "feat/8");
769 expect(mockedExecute).toHaveBeenCalledOnce();769 expect(mockedExecute).toHaveBeenCalledOnce();
770 const document = queryAt(0);770 expect(queryAt(0)).toContain(
771 expect(document).toContain(771 "s0: repository(owner: $s0_owner, name: $s0_name) { pullRequest(number: $s0_number)",
772 's0: repository(owner: "acme", name: "web") { pullRequest(number: 7)',
773 );772 );
774 expect(document).toContain("pullRequest(number: 8)");773 expect(varsAt(0)).toMatchObject({
774 s0_owner: "acme",
775 s0_name: "web",
776 s0_number: 7,
777 s1_number: 8,
778 });
775 }),779 }),
776 );780 );
777781
...@@ -815,10 +819,10 @@ layer("GitHubPullRequestApi.layer", (it) => {...@@ -815,10 +819,10 @@ layer("GitHubPullRequestApi.layer", (it) => {
815 // GitHub had no answer for #8, so its watch reads it in full.819 // GitHub had no answer for #8, so its watch reads it in full.
816 expect(eight).toBeNull();820 expect(eight).toBeNull();
817 expect(mockedExecute).toHaveBeenCalledOnce();821 expect(mockedExecute).toHaveBeenCalledOnce();
818 const call = callAt(0);822 expect(queryAt(0)).toContain(
819 expect(call.kind === "graphql" ? call.query : "").toContain(823 "w1: repository(owner: $w1_owner, name: $w1_name) { pullRequest(number: $w1_number)",
820 'w1: repository(owner: "acme", name: "web") { pullRequest(number: 8)',
821 );824 );
825 expect(varsAt(0)).toMatchObject({ w1_owner: "acme", w1_name: "web", w1_number: 8 });
822 }),826 }),
823 );827 );
824828
...@@ -1108,6 +1112,37 @@ layer("GitHubPullRequestApi.layer", (it) => {...@@ -1108,6 +1112,37 @@ layer("GitHubPullRequestApi.layer", (it) => {
1108 }),1112 }),
1109 );1113 );
11101114
1115 it.effect("reports truncation when GitHub repeats a cursor before the page is full", () =>
1116 Effect.gen(function* () {
1117 // Two pages of 100 that hand back the same cursor, for a page of 250.
1118 const repeating = output(
1119 encodeJson({
1120 data: {
1121 search: {
1122 pageInfo: { hasNextPage: true, endCursor: "same" },
1123 nodes: rows(100, 1),
1124 },
1125 },
1126 }),
1127 );
1128 mockedExecute.mockReturnValue(Effect.succeed(repeating));
1129 const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi;
1130
1131 const batch = yield* cli.listPullRequests({
1132 cwd: "/w",
1133 repository: "acme/web",
1134 host: "github.com",
1135 state: "open",
1136 involvement: "all",
1137 viewer: "bilal",
1138 limit: 250,
1139 });
1140
1141 assert.strictEqual(mockedExecute.mock.calls.length, 2);
1142 assert.isTrue(batch.truncated);
1143 }),
1144 );
1145
1111 it.effect("excludes merged pull requests from the Closed tab", () =>1146 it.effect("excludes merged pull requests from the Closed tab", () =>
1112 Effect.gen(function* () {1147 Effect.gen(function* () {
1113 mockedExecute.mockImplementation((call) =>1148 mockedExecute.mockImplementation((call) =>
...@@ -1408,11 +1443,12 @@ layer("GitHubPullRequestApi.layer", (it) => {...@@ -1408,11 +1443,12 @@ layer("GitHubPullRequestApi.layer", (it) => {
1408 expect(batch.continues).toBe(false);1443 expect(batch.continues).toBe(false);
1409 expect(mockedStackMemberships).toHaveBeenCalledTimes(1);1444 expect(mockedStackMemberships).toHaveBeenCalledTimes(1);
1410 const membership = mockedStackMemberships.mock.calls[0]?.[0];1445 const membership = mockedStackMemberships.mock.calls[0]?.[0];
1411 const query = membership?.kind === "graphql" ? membership.query : "";1446 const numbers = Object.entries(
1412 expect(query).toContain("pullRequest(number: 4)");1447 membership?.kind === "graphql" ? (membership.variables ?? {}) : {},
1413 expect(query).toContain("pullRequest(number: 5)");1448 )
1414 expect(query).not.toContain("pullRequest(number: 1)");1449 .filter(([name]) => name.endsWith("_number"))
1415 expect(query).not.toContain("pullRequest(number: 6)");1450 .map(([, number]) => number);
1451 expect(numbers).toEqual([4, 5]);
1416 }),1452 }),
1417 );1453 );
14181454
...@@ -1420,7 +1456,7 @@ layer("GitHubPullRequestApi.layer", (it) => {...@@ -1420,7 +1456,7 @@ layer("GitHubPullRequestApi.layer", (it) => {
1420 Effect.gen(function* () {1456 Effect.gen(function* () {
1421 mockedExecute.mockReturnValueOnce(Effect.succeed(output(pullRequests(27, 1))));1457 mockedExecute.mockReturnValueOnce(Effect.succeed(output(pullRequests(27, 1))));
1422 mockedStackMemberships.mockImplementation((input) =>1458 mockedStackMemberships.mockImplementation((input) =>
1423 input.kind === "graphql" && input.query.includes("pullRequest(number: 26)")1459 input.kind === "graphql" && Object.values(input.variables ?? {}).includes(26)
1424 ? Effect.fail(1460 ? Effect.fail(
1425 new GitHubApi.GitHubApiResponseError({1461 new GitHubApi.GitHubApiResponseError({
1426 host: "github.com",1462 host: "github.com",
...@@ -1513,9 +1549,8 @@ layer("GitHubPullRequestApi.layer", (it) => {...@@ -1513,9 +1549,8 @@ layer("GitHubPullRequestApi.layer", (it) => {
1513 { repository: "acme/web", number: 1, additions: 4, deletions: 1 },1549 { repository: "acme/web", number: 1, additions: 4, deletions: 1 },
1514 { repository: "acme/web", number: 26, additions: 4, deletions: 1 },1550 { repository: "acme/web", number: 26, additions: 4, deletions: 1 },
1515 ]);1551 ]);
1516 const document = queryAt(0);1552 expect(queryAt(0)).toContain("s0: repository(owner: $s0_owner, name: $s0_name)");
1517 expect(document).toContain('s0: repository(owner: "acme", name: "web")');1553 expect(varsAt(0)).toMatchObject({ s0_owner: "acme", s0_name: "web", s24_number: 25 });
1518 expect(document).toContain("pullRequest(number: 25)");
1519 }),1554 }),
1520 );1555 );
15211556
...@@ -2614,6 +2649,38 @@ layer("GitHubPullRequestApi.layer", (it) => {...@@ -2614,6 +2649,38 @@ layer("GitHubPullRequestApi.layer", (it) => {
2614 }),2649 }),
2615 );2650 );
26162651
2652 it.effect("refuses workflow approval when the head list stops before its end", () =>
2653 Effect.gen(function* () {
2654 // GitHub hands the same cursor back, so the second page is never read past. #7 looks
2655 // unique on what was read, and an unread page could still hold another head like it.
2656 const repeating = heads([7]);
2657 repeating.data.repository.pullRequests.pageInfo = {
2658 hasNextPage: true,
2659 endCursor: "same",
2660 } as never;
2661 workflowApprovalRoutes(() => crossRepositoryDetail(), repeating, workflowRuns([]));
2662 const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi;
2663
2664 const error = yield* Effect.flip(
2665 cli.listWorkflowRunsRequiringApproval({
2666 cwd: "/w",
2667 repository: "acme/web",
2668 host: "github.com",
2669 number: 7,
2670 headSha: "abc123",
2671 headBranch: "feat/page",
2672 headRepositoryOwner: "octocat",
2673 isCrossRepository: true,
2674 }),
2675 );
2676
2677 expect(error).toMatchObject({
2678 _tag: "GitHubWorkflowApprovalRefusedError",
2679 reason: "head-list-truncated",
2680 });
2681 }),
2682 );
2683
2617 it.effect("refuses workflow approval when GitHub omits the head repository", () =>2684 it.effect("refuses workflow approval when GitHub omits the head repository", () =>
2618 Effect.gen(function* () {2685 Effect.gen(function* () {
2619 mockedExecute.mockReturnValueOnce(2686 mockedExecute.mockReturnValueOnce(
...@@ -4386,9 +4453,9 @@ layer("GitHubPullRequestApi.layer", (it) => {...@@ -4386,9 +4453,9 @@ layer("GitHubPullRequestApi.layer", (it) => {
4386 number: 7,4453 number: 7,
4387 });4454 });
43884455
4389 assert.strictEqual(mockedExecute.mock.calls.length, 5);4456 // The same cursor twice is a page GitHub already gave, so reading stops there.
4457 assert.strictEqual(mockedExecute.mock.calls.length, 2);
4390 assert.isTrue(viewed.truncated);4458 assert.isTrue(viewed.truncated);
4391 assert.strictEqual(viewed.files.length, 5);
4392 }),4459 }),
4393 );4460 );
43944461
...@@ -4420,8 +4487,8 @@ layer("GitHubPullRequestApi.layer", (it) => {...@@ -4420,8 +4487,8 @@ layer("GitHubPullRequestApi.layer", (it) => {
4420 expect(queryAt(1)).toContain("f1: unmarkFileAsViewed");4487 expect(queryAt(1)).toContain("f1: unmarkFileAsViewed");
4421 expect(varsAt(1)).toEqual({4488 expect(varsAt(1)).toEqual({
4422 pullRequestId: "PR_1",4489 pullRequestId: "PR_1",
4423 path0: "src/a.ts",4490 f0_path: "src/a.ts",
4424 path1: "src/b.ts",4491 f1_path: "src/b.ts",
4425 });4492 });
4426 }),4493 }),
4427 );4494 );
apps/server/src/pullRequest/GitHubPullRequestApi.ts+138-130
...@@ -44,6 +44,7 @@ import {...@@ -44,6 +44,7 @@ import {
4444
45import { AllowGitHubReserve } from "../sourceControl/GitHubRepositoryApi.ts";45import { AllowGitHubReserve } from "../sourceControl/GitHubRepositoryApi.ts";
46import * as GitHubApi from "../sourceControl/GitHubApi.ts";46import * as GitHubApi from "../sourceControl/GitHubApi.ts";
47import { readGraphQlPages } from "../sourceControl/githubGraphQl.ts";
47import * as VcsProcess from "../vcs/VcsProcess.ts";48import * as VcsProcess from "../vcs/VcsProcess.ts";
48import * as SourceControlRateLimit from "../sourceControl/SourceControlRateLimit.ts";49import * as SourceControlRateLimit from "../sourceControl/SourceControlRateLimit.ts";
49import {50import {
...@@ -64,7 +65,6 @@ import {...@@ -64,7 +65,6 @@ import {
64 PULL_REQUEST_ACTIVITY_GRAPHQL_QUERY,65 PULL_REQUEST_ACTIVITY_GRAPHQL_QUERY,
65 PULL_REQUEST_HEADS_GRAPHQL_QUERY,66 PULL_REQUEST_HEADS_GRAPHQL_QUERY,
66 decodeWorkflowRunsJson,67 decodeWorkflowRunsJson,
67 type GitHubCheckContext,
68 pullRequestCoreGraphQlQuery,68 pullRequestCoreGraphQlQuery,
69 type GitHubPullRequestCore,69 type GitHubPullRequestCore,
70 type GitHubPullRequestSummary,70 type GitHubPullRequestSummary,
...@@ -1481,42 +1481,45 @@ export const make = Effect.gen(function* () {...@@ -1481,42 +1481,45 @@ export const make = Effect.gen(function* () {
1481 * its first hundred. Each page names the head it read, so a push mid-walk fails rather than1481 * its first hundred. Each page names the head it read, so a push mid-walk fails rather than
1482 * mixing two revisions' checks.1482 * mixing two revisions' checks.
1483 */1483 */
1484 type CheckContextsPage = Result.Result.Success<
1485 ReturnType<typeof decodePullRequestCheckContextsJson>
1486 >;
1484 const readAllCheckContexts = (1487 const readAllCheckContexts = (
1485 input: Parameters<GitHubPullRequestApi["Service"]["getPullRequestDetail"]>[0],1488 input: Parameters<GitHubPullRequestApi["Service"]["getPullRequestDetail"]>[0],
1486 allowReserve: boolean,1489 allowReserve: boolean,
1487 ) =>1490 ) =>
1488 Effect.gen(function* () {1491 Effect.gen(function* () {
1489 const { owner, name } = parseRepositorySelector(input.repository);1492 const { owner, name } = parseRepositorySelector(input.repository);
1490 const contexts: GitHubCheckContext[] = [];1493 const { pages, truncated } = yield* readGraphQlPages(
1491 let headSha: string | null = null;1494 (after, previous: ReadonlyArray<CheckContextsPage>) =>
1492 let after: string | null = null;1495 graphqlRead({
1493 for (let page = 0; page < CHECK_CONTEXT_PAGES; page++) {1496 cwd: input.cwd,
1494 const read: {1497 host: input.host,
1495 readonly headSha: string;1498 operation: "getPullRequestDetail",
1496 readonly contexts: ReadonlyArray<GitHubCheckContext>;1499 allowReserve,
1497 readonly nextCursor: string | null;1500 variables: { owner, name, number: input.number, after },
1498 } = yield* graphqlRead({1501 query: pullRequestCheckContextsGraphQlQuery(input.host),
1499 cwd: input.cwd,1502 decode: decodePullRequestCheckContextsJson,
1500 host: input.host,1503 }).pipe(
1501 operation: "getPullRequestDetail",1504 // Each page names the head it read, so a push mid-walk fails rather than mixing
1502 allowReserve,1505 // two revisions' checks.
1503 variables: { owner, name, number: input.number, after },1506 Effect.filterOrFail(
1504 query: pullRequestCheckContextsGraphQlQuery(input.host),1507 (page) => previous.length === 0 || page.headSha === previous[0]!.headSha,
1505 decode: decodePullRequestCheckContextsJson,1508 () =>
1506 });1509 readError(
1507 if (headSha !== null && read.headSha !== headSha) {1510 input.cwd,
1508 return yield* readError(1511 "getPullRequestDetail",
1509 input.cwd,1512 new Error("Pull request head changed while reading checks."),
1510 "getPullRequestDetail",1513 ),
1511 new Error("Pull request head changed while reading checks."),1514 ),
1512 );1515 ),
1513 }1516 { maxPages: CHECK_CONTEXT_PAGES, nextCursor: (page) => page.nextCursor },
1514 headSha = read.headSha;1517 );
1515 contexts.push(...read.contexts);1518 return {
1516 after = read.nextCursor;1519 headSha: pages[0]?.headSha ?? null,
1517 if (after === null) break;1520 contexts: pages.flatMap((page) => page.contexts),
1518 }1521 truncated,
1519 return { headSha, contexts, truncated: after !== null };1522 };
1520 });1523 });
15211524
1522 const getPullRequestDetail: GitHubPullRequestApi["Service"]["getPullRequestDetail"] = (input) => {1525 const getPullRequestDetail: GitHubPullRequestApi["Service"]["getPullRequestDetail"] = (input) => {
...@@ -1568,7 +1571,10 @@ export const make = Effect.gen(function* () {...@@ -1568,7 +1571,10 @@ export const make = Effect.gen(function* () {
1568 const workflowApprovalReadError = (cwd: string, cause: unknown) =>1571 const workflowApprovalReadError = (cwd: string, cause: unknown) =>
1569 readError(cwd, "listWorkflowRunsRequiringApproval", cause);1572 readError(cwd, "listWorkflowRunsRequiringApproval", cause);
15701573
1571 /** Every open pull request whose head branch carries this name, up to one past the limit. */1574 /**
1575 * Every open pull request whose head branch carries this name, up to one past the limit.
1576 * `truncated` says some were never read, which is never enough to call a head unique.
1577 */
1572 const listHeadsByBranch = (input: {1578 const listHeadsByBranch = (input: {
1573 readonly cwd: string;1579 readonly cwd: string;
1574 readonly repository: string;1580 readonly repository: string;
...@@ -1577,25 +1583,25 @@ export const make = Effect.gen(function* () {...@@ -1577,25 +1583,25 @@ export const make = Effect.gen(function* () {
1577 }) =>1583 }) =>
1578 Effect.gen(function* () {1584 Effect.gen(function* () {
1579 const { owner, name } = parseRepositorySelector(input.repository);1585 const { owner, name } = parseRepositorySelector(input.repository);
1580 const heads: GitHubPullRequestHead[] = [];1586 const countHeads = (pages: ReadonlyArray<{ readonly heads: ReadonlyArray<unknown> }>) =>
1581 let after: string | null = null;1587 pages.reduce((total, page) => total + page.heads.length, 0);
1582 do {1588 const { pages, truncated } = yield* readGraphQlPages(
1583 const page: {1589 (after) =>
1584 readonly heads: ReadonlyArray<GitHubPullRequestHead>;1590 graphqlRead({
1585 readonly nextCursor: string | null;1591 cwd: input.cwd,
1586 } = yield* graphqlRead({1592 host: input.host,
1587 cwd: input.cwd,1593 operation: "listWorkflowRunsRequiringApproval",
1588 host: input.host,1594 allowReserve: true,
1589 operation: "listWorkflowRunsRequiringApproval",1595 variables: { owner, name, head: input.headBranch, after },
1590 allowReserve: true,1596 query: PULL_REQUEST_HEADS_GRAPHQL_QUERY,
1591 variables: { owner, name, head: input.headBranch, after },1597 decode: decodePullRequestHeadsJson,
1592 query: PULL_REQUEST_HEADS_GRAPHQL_QUERY,1598 }),
1593 decode: decodePullRequestHeadsJson,1599 {
1594 });1600 nextCursor: (page) => page.nextCursor,
1595 heads.push(...page.heads);1601 until: (pages) => countHeads(pages) > workflowApprovalLimit,
1596 after = page.nextCursor;1602 },
1597 } while (after !== null && heads.length <= workflowApprovalLimit);1603 );
1598 return heads;1604 return { heads: pages.flatMap((page) => page.heads), truncated };
1599 });1605 });
16001606
1601 /**1607 /**
...@@ -1650,9 +1656,10 @@ export const make = Effect.gen(function* () {...@@ -1650,9 +1656,10 @@ export const make = Effect.gen(function* () {
1650 [1656 [
1651 listHeadsByBranch(input).pipe(1657 listHeadsByBranch(input).pipe(
1652 Effect.flatMap(1658 Effect.flatMap(
1653 (1659 ({
1654 heads,1660 heads,
1655 ): Effect.Effect<1661 truncated,
1662 }): Effect.Effect<
1656 GitHubPullRequestHead,1663 GitHubPullRequestHead,
1657 GitHubPullRequestReadError | GitHubWorkflowApprovalRefusedError1664 GitHubPullRequestReadError | GitHubWorkflowApprovalRefusedError
1658 > => {1665 > => {
...@@ -1663,7 +1670,7 @@ export const make = Effect.gen(function* () {...@@ -1663,7 +1670,7 @@ export const make = Effect.gen(function* () {
1663 pullRequest.headRepositoryOwner?.toLowerCase() ===1670 pullRequest.headRepositoryOwner?.toLowerCase() ===
1664 input.headRepositoryOwner.toLowerCase(),1671 input.headRepositoryOwner.toLowerCase(),
1665 );1672 );
1666 if (heads.length > workflowApprovalLimit) {1673 if (truncated || heads.length > workflowApprovalLimit) {
1667 return Effect.fail(1674 return Effect.fail(
1668 new GitHubWorkflowApprovalRefusedError({1675 new GitHubWorkflowApprovalRefusedError({
1669 cwd: input.cwd,1676 cwd: input.cwd,
...@@ -1755,18 +1762,18 @@ export const make = Effect.gen(function* () {...@@ -1755,18 +1762,18 @@ export const make = Effect.gen(function* () {
1755 );1762 );
1756 // Stack membership rides along where GitHub serves stacks, so the background sync can skip1763 // Stack membership rides along where GitHub serves stacks, so the background sync can skip
1757 // the REST stack read for pull requests that are in none.1764 // the REST stack read for pull requests that are in none.
1758 const query = buildPullRequestSummariesGraphQlQuery(1765 const document = buildPullRequestSummariesGraphQlQuery(
1759 batchable.map((entry) => entry.request),1766 batchable.map((entry) => entry.request),
1760 first.request.host === "github.com",1767 first.request.host === "github.com",
1761 );1768 );
1762 const batched =1769 const batched =
1763 query === null1770 document === null
1764 ? Effect.succeed(new Map<number, GitHubPullRequestSummary>())1771 ? Effect.succeed(new Map<number, GitHubPullRequestSummary>())
1765 : graphqlRead({1772 : graphqlRead({
1766 cwd: first.request.cwd,1773 cwd: first.request.cwd,
1767 host: first.request.host,1774 host: first.request.host,
1768 operation: "getPullRequestSummary",1775 operation: "getPullRequestSummary",
1769 query,1776 ...document,
1770 decode: decodePullRequestSummariesJson,1777 decode: decodePullRequestSummariesJson,
1771 });1778 });
1772 return batched.pipe(1779 return batched.pipe(
...@@ -1833,17 +1840,17 @@ export const make = Effect.gen(function* () {...@@ -1833,17 +1840,17 @@ export const make = Effect.gen(function* () {
1833 const batchable = entries.filter(1840 const batchable = entries.filter(
1834 (entry) => buildPullRequestWatchFingerprintsGraphQlQuery([entry.request]) !== null,1841 (entry) => buildPullRequestWatchFingerprintsGraphQlQuery([entry.request]) !== null,
1835 );1842 );
1836 const query = buildPullRequestWatchFingerprintsGraphQlQuery(1843 const document = buildPullRequestWatchFingerprintsGraphQlQuery(
1837 batchable.map((entry) => entry.request),1844 batchable.map((entry) => entry.request),
1838 );1845 );
1839 const read =1846 const read =
1840 query === null1847 document === null
1841 ? Effect.succeed(new Map<number, GitHubPullRequestWatchFingerprint>())1848 ? Effect.succeed(new Map<number, GitHubPullRequestWatchFingerprint>())
1842 : graphqlRead({1849 : graphqlRead({
1843 cwd: first.request.cwd,1850 cwd: first.request.cwd,
1844 host: first.request.host,1851 host: first.request.host,
1845 operation: "getPullRequestWatchFingerprint",1852 operation: "getPullRequestWatchFingerprint",
1846 query,1853 ...document,
1847 decode: decodePullRequestWatchFingerprintsJson,1854 decode: decodePullRequestWatchFingerprintsJson,
1848 });1855 });
1849 return read.pipe(1856 return read.pipe(
...@@ -1897,16 +1904,22 @@ export const make = Effect.gen(function* () {...@@ -1897,16 +1904,22 @@ export const make = Effect.gen(function* () {
1897 ) => Effect.Effect<GitHubPullRequestListPage, GitHubPullRequestApiError>,1904 ) => Effect.Effect<GitHubPullRequestListPage, GitHubPullRequestApiError>,
1898 ) =>1905 ) =>
1899 Effect.gen(function* () {1906 Effect.gen(function* () {
1900 const items: GitHubPullRequestListItem[] = [];1907 const countRows = (pages: ReadonlyArray<GitHubPullRequestListPage>) =>
1901 let rawCount = 0;1908 pages.reduce((total, read) => total + read.rawCount, 0);
1902 let after: string | null = null;1909 const { pages, truncated } = yield* readGraphQlPages(
1903 do {1910 (after, pages: ReadonlyArray<GitHubPullRequestListPage>) =>
1904 const read: GitHubPullRequestListPage = yield* page(after, rows - rawCount);1911 page(after, rows - countRows(pages)),
1905 items.push(...read.items);1912 {
1906 rawCount += read.rawCount;1913 nextCursor: (read) => read.endCursor,
1907 after = read.endCursor;1914 until: (pages) => countRows(pages) >= rows,
1908 } while (after !== null && rawCount < rows);1915 },
1909 return { items, rawCount };1916 );
1917 // `truncated` also covers a cursor GitHub repeated before `rows` arrived.
1918 return {
1919 items: pages.flatMap((read) => read.items),
1920 rawCount: countRows(pages),
1921 truncated,
1922 };
1910 });1923 });
1911 const read = (1924 const read = (
1912 continues: boolean,1925 continues: boolean,
...@@ -1932,7 +1945,7 @@ export const make = Effect.gen(function* () {...@@ -1932,7 +1945,7 @@ export const make = Effect.gen(function* () {
1932 decode: decodePullRequestListJson,1945 decode: decodePullRequestListJson,
1933 }),1946 }),
1934 ).pipe(1947 ).pipe(
1935 Effect.flatMap(({ items: rawItems, rawCount }) => {1948 Effect.flatMap(({ items: rawItems, rawCount, truncated: cutShort }) => {
1936 const items = continues1949 const items = continues
1937 ? rawItems1950 ? rawItems
1938 : rawItems.filter((item) => matchesUnsortedListing(item, input));1951 : rawItems.filter((item) => matchesUnsortedListing(item, input));
...@@ -1949,9 +1962,11 @@ export const make = Effect.gen(function* () {...@@ -1949,9 +1962,11 @@ export const make = Effect.gen(function* () {
1949 items: items.slice(0, input.limit),1962 items: items.slice(0, input.limit),
1950 // One row over the page size is the probe for a next page, and it is1963 // One row over the page size is the probe for a next page, and it is
1951 // counted before decoding: a skipped malformed row must not end paging.1964 // counted before decoding: a skipped malformed row must not end paging.
1952 truncated: continues1965 truncated:
1953 ? rawCount > input.limit1966 cutShort ||
1954 : items.length > input.limit || rawCount >= requestedRows,1967 (continues
1968 ? rawCount > input.limit
1969 : items.length > input.limit || rawCount >= requestedRows),
1955 continues,1970 continues,
1956 });1971 });
1957 }),1972 }),
...@@ -1989,16 +2004,16 @@ export const make = Effect.gen(function* () {...@@ -1989,16 +2004,16 @@ export const make = Effect.gen(function* () {
1989 return Effect.forEach(2004 return Effect.forEach(
1990 chunks,2005 chunks,
1991 (chunk) => {2006 (chunk) => {
1992 const query = buildPullRequestStackMembershipsGraphQlQuery(2007 const document = buildPullRequestStackMembershipsGraphQlQuery(
1993 input.repository,2008 input.repository,
1994 chunk.map((item) => item.number),2009 chunk.map((item) => item.number),
1995 );2010 );
1996 if (query === null) return Effect.succeed(chunk);2011 if (document === null) return Effect.succeed(chunk);
1997 return graphqlRead({2012 return graphqlRead({
1998 cwd: input.cwd,2013 cwd: input.cwd,
1999 host: input.host,2014 host: input.host,
2000 operation: "listPullRequestStackMemberships",2015 operation: "listPullRequestStackMemberships",
2001 query,2016 ...document,
2002 decode: decodePullRequestStackMembershipsJson,2017 decode: decodePullRequestStackMembershipsJson,
2003 }).pipe(2018 }).pipe(
2004 Effect.map((memberships) =>2019 Effect.map((memberships) =>
...@@ -2061,8 +2076,8 @@ export const make = Effect.gen(function* () {...@@ -2061,8 +2076,8 @@ export const make = Effect.gen(function* () {
2061 return Effect.forEach(2076 return Effect.forEach(
2062 chunks,2077 chunks,
2063 (chunk) => {2078 (chunk) => {
2064 const query = buildPullRequestStatsGraphQlQuery(chunk);2079 const document = buildPullRequestStatsGraphQlQuery(chunk);
2065 if (query === null) {2080 if (document === null) {
2066 return Effect.fail(2081 return Effect.fail(
2067 new GitHubRepositorySelectorError({2082 new GitHubRepositorySelectorError({
2068 cwd: input.cwd,2083 cwd: input.cwd,
...@@ -2074,7 +2089,7 @@ export const make = Effect.gen(function* () {...@@ -2074,7 +2089,7 @@ export const make = Effect.gen(function* () {
2074 cwd: input.cwd,2089 cwd: input.cwd,
2075 host: input.host,2090 host: input.host,
2076 operation: "listPullRequestStats",2091 operation: "listPullRequestStats",
2077 query,2092 ...document,
2078 decode: decodePullRequestStatsJson,2093 decode: decodePullRequestStatsJson,
2079 }).pipe(2094 }).pipe(
2080 Effect.map((stats) =>2095 Effect.map((stats) =>
...@@ -2328,23 +2343,26 @@ export const make = Effect.gen(function* () {...@@ -2328,23 +2343,26 @@ export const make = Effect.gen(function* () {
2328 // Almost never entered: the embedded page already holds every dismissal a pull request2343 // Almost never entered: the embedded page already holds every dismissal a pull request
2329 // ordinarily accrues. Followed so a review whose event fell past that page still finds2344 // ordinarily accrues. Followed so a review whose event fell past that page still finds
2330 // its reason.2345 // its reason.
2331 let dismissalPage = 0;2346 if (dismissalCursor !== null) {
2332 while (dismissalCursor !== null && dismissalPage < REVIEW_THREAD_PAGES) {2347 const { pages } = yield* readGraphQlPages(
2333 const read: {2348 (cursor) =>
2334 readonly dismissalsByReviewId: ReadonlyMap<string, string>;2349 graphqlRead({
2335 readonly nextCursor: string | null;2350 cwd: input.cwd,
2336 } = yield* graphqlRead({2351 host: input.host,
2337 cwd: input.cwd,2352 operation: "listReviewThreadComments",
2338 host: input.host,2353 variables: { owner, name, number: input.number, cursor },
2339 operation: "listReviewThreadComments",2354 query: REVIEW_DISMISSALS_GRAPHQL_QUERY,
2340 variables: { owner, name, number: input.number, cursor: dismissalCursor },2355 decode: decodeReviewDismissalsJson,
2341 query: REVIEW_DISMISSALS_GRAPHQL_QUERY,2356 }),
2342 decode: decodeReviewDismissalsJson,2357 {
2343 });2358 from: dismissalCursor,
2344 for (const [id, message] of read.dismissalsByReviewId)2359 maxPages: REVIEW_THREAD_PAGES,
2345 dismissalsByReviewId.set(id, message);2360 nextCursor: (read) => read.nextCursor,
2346 dismissalCursor = read.nextCursor;2361 },
2347 dismissalPage += 1;2362 );
2363 for (const read of pages)
2364 for (const [id, message] of read.dismissalsByReviewId)
2365 dismissalsByReviewId.set(id, message);
2348 }2366 }
23492367
2350 const reviewThreads = entries.map((entry) => ({2368 const reviewThreads = entries.map((entry) => ({
...@@ -2708,44 +2726,34 @@ export const make = Effect.gen(function* () {...@@ -2708,44 +2726,34 @@ export const make = Effect.gen(function* () {
27082726
2709 getPullRequestFilesViewed: (input) => {2727 getPullRequestFilesViewed: (input) => {
2710 const { owner, name } = parseRepositorySelector(input.repository);2728 const { owner, name } = parseRepositorySelector(input.repository);
2711 const read = (2729 return readGraphQlPages(
2712 after: string | null,2730 (after) =>
2713 collected: ReadonlyArray<PullRequestFileViewed>,2731 graphqlRead({
2714 pagesLeft: number,2732 cwd: input.cwd,
2715 ): Effect.Effect<GitHubPullRequestFilesViewed, GitHubPullRequestApiError> =>2733 host: input.host,
2716 graphqlRead({2734 operation: "getPullRequestFilesViewed",
2717 cwd: input.cwd,2735 variables: { owner, name, number: input.number, after },
2718 host: input.host,2736 query: PULL_REQUEST_FILES_VIEWED_GRAPHQL_QUERY,
2719 operation: "getPullRequestFilesViewed",2737 decode: decodePullRequestFilesViewedJson,
2720 variables: { owner, name, number: input.number, after },
2721 query: PULL_REQUEST_FILES_VIEWED_GRAPHQL_QUERY,
2722 decode: decodePullRequestFilesViewedJson,
2723 }).pipe(
2724 Effect.flatMap((page) => {
2725 const files = [...collected, ...page.files];
2726 if (page.nextCursor === null) {
2727 return Effect.succeed({ files, truncated: false });
2728 }
2729 return pagesLeft <= 1
2730 ? Effect.succeed({ files, truncated: true })
2731 : read(page.nextCursor, files, pagesLeft - 1);
2732 }),2738 }),
2733 );2739 { maxPages: FILES_VIEWED_MAX_PAGES, nextCursor: (page) => page.nextCursor },
2734 return read(null, [], FILES_VIEWED_MAX_PAGES);2740 ).pipe(
2741 Effect.map(({ pages, truncated }) => ({
2742 files: pages.flatMap((page) => page.files),
2743 truncated,
2744 })),
2745 );
2735 },2746 },
27362747
2737 setPullRequestFilesViewed: (input) => {2748 setPullRequestFilesViewed: (input) => {
2738 const mutation = buildSetFilesViewedGraphQlMutation(input.files);2749 if (input.files.length === 0) return Effect.void;
2739 if (mutation === null) return Effect.void;
2740 return pullRequestNodeId({ ...input, operation: "setPullRequestFilesViewed" }).pipe(2750 return pullRequestNodeId({ ...input, operation: "setPullRequestFilesViewed" }).pipe(
2741 Effect.flatMap((pullRequestId) =>2751 Effect.flatMap((pullRequestId) => {
2742 graphql({2752 const mutation = buildSetFilesViewedGraphQlMutation(pullRequestId, input.files);
2743 host: input.host,2753 return mutation === null
2744 operation: "setPullRequestFilesViewed",2754 ? Effect.void
2745 query: mutation.query,2755 : graphql({ host: input.host, operation: "setPullRequestFilesViewed", ...mutation });
2746 variables: { pullRequestId, ...mutation.variables },2756 }),
2747 }),
2748 ),
2749 );2757 );
2750 },2758 },
27512759
apps/server/src/pullRequest/gitHubPullRequestJson.test.ts+35-20
...@@ -1754,36 +1754,40 @@ describe("decodePullRequestFilesViewedJson", () => {...@@ -1754,36 +1754,40 @@ describe("decodePullRequestFilesViewedJson", () => {
17541754
1755describe("buildSetFilesViewedGraphQlMutation", () => {1755describe("buildSetFilesViewedGraphQlMutation", () => {
1756 it("asks for nothing when nothing was pressed", () => {1756 it("asks for nothing when nothing was pressed", () => {
1757 expect(buildSetFilesViewedGraphQlMutation([])).toBeNull();1757 expect(buildSetFilesViewedGraphQlMutation("PR_1", [])).toBeNull();
1758 });1758 });
17591759
1760 it("clears and restores in one document, each file under its own alias", () => {1760 it("clears and restores in one document, each file under its own alias", () => {
1761 const mutation = buildSetFilesViewedGraphQlMutation([1761 const mutation = buildSetFilesViewedGraphQlMutation("PR_1", [
1762 { path: "src/a.ts", viewed: true },1762 { path: "src/a.ts", viewed: true },
1763 { path: "src/b.ts", viewed: false },1763 { path: "src/b.ts", viewed: false },
1764 ]);1764 ]);
1765 expect(mutation).not.toBeNull();1765 expect(mutation).not.toBeNull();
1766 if (mutation === null) return;1766 if (mutation === null) return;
1767 expect(mutation.query).toContain(1767 expect(mutation.query).toContain(
1768 "mutation($pullRequestId: ID!, $path0: String!, $path1: String!)",1768 "mutation($pullRequestId: ID!, $f0_path: String!, $f1_path: String!)",
1769 );1769 );
1770 expect(mutation.query).toContain(1770 expect(mutation.query).toContain(
1771 "f0: markFileAsViewed(input: { pullRequestId: $pullRequestId, path: $path0 })",1771 "f0: markFileAsViewed(input: { pullRequestId: $pullRequestId, path: $f0_path })",
1772 );1772 );
1773 expect(mutation.query).toContain(1773 expect(mutation.query).toContain(
1774 "f1: unmarkFileAsViewed(input: { pullRequestId: $pullRequestId, path: $path1 })",1774 "f1: unmarkFileAsViewed(input: { pullRequestId: $pullRequestId, path: $f1_path })",
1775 );1775 );
1776 expect(mutation.variables).toEqual({ path0: "src/a.ts", path1: "src/b.ts" });1776 expect(mutation.variables).toEqual({
1777 pullRequestId: "PR_1",
1778 f0_path: "src/a.ts",
1779 f1_path: "src/b.ts",
1780 });
1777 });1781 });
17781782
1779 it("keeps a path out of the document, so one cannot be read as part of it", () => {1783 it("keeps a path out of the document, so one cannot be read as part of it", () => {
1780 const mutation = buildSetFilesViewedGraphQlMutation([1784 const mutation = buildSetFilesViewedGraphQlMutation("PR_1", [
1781 { path: '") { __typename } evil: markFileAsViewed(input: { path: "x', viewed: true },1785 { path: '") { __typename } evil: markFileAsViewed(input: { path: "x', viewed: true },
1782 ]);1786 ]);
1783 expect(mutation).not.toBeNull();1787 expect(mutation).not.toBeNull();
1784 if (mutation === null) return;1788 if (mutation === null) return;
1785 expect(mutation.query).not.toContain("evil");1789 expect(mutation.query).not.toContain("evil");
1786 expect(mutation.variables.path0).toBe(1790 expect(mutation.variables.f0_path).toBe(
1787 '") { __typename } evil: markFileAsViewed(input: { path: "x',1791 '") { __typename } evil: markFileAsViewed(input: { path: "x',
1788 );1792 );
1789 });1793 });
...@@ -1922,9 +1926,14 @@ describe("pull request stack membership batches", () => {...@@ -1922,9 +1926,14 @@ describe("pull request stack membership batches", () => {
1922 expect(buildPullRequestStackMembershipsGraphQlQuery("acme/web", [0])).toBeNull();1926 expect(buildPullRequestStackMembershipsGraphQlQuery("acme/web", [0])).toBeNull();
1923 expect(buildPullRequestStackMembershipsGraphQlQuery("acme/web", [1.5])).toBeNull();1927 expect(buildPullRequestStackMembershipsGraphQlQuery("acme/web", [1.5])).toBeNull();
1924 expect(buildPullRequestStackMembershipsGraphQlQuery("acme/web", [])).toBeNull();1928 expect(buildPullRequestStackMembershipsGraphQlQuery("acme/web", [])).toBeNull();
1925 expect(buildPullRequestStackMembershipsGraphQlQuery("acme/web", [7, 8])).toContain(1929 expect(buildPullRequestStackMembershipsGraphQlQuery("acme/web", [7, 8])?.variables).toEqual({
1926 "pullRequest(number: 8)",1930 s0_owner: "acme",
1927 );1931 s0_name: "web",
1932 s0_number: 7,
1933 s1_owner: "acme",
1934 s1_name: "web",
1935 s1_number: 8,
1936 });
1928 });1937 });
1929});1938});
19301939
...@@ -1982,10 +1991,10 @@ describe("batched pull request summaries", () => {...@@ -1982,10 +1991,10 @@ describe("batched pull request summaries", () => {
19821991
1983 it("reads stack membership only where the document asked for it", () => {1992 it("reads stack membership only where the document asked for it", () => {
1984 expect(1993 expect(
1985 buildPullRequestSummariesGraphQlQuery([{ repository: "acme/web", number: 7 }], true),1994 buildPullRequestSummariesGraphQlQuery([{ repository: "acme/web", number: 7 }], true)?.query,
1986 ).toContain("stack { number size baseRefName } stackEntry { position }");1995 ).toContain("stack { number size baseRefName } stackEntry { position }");
1987 expect(1996 expect(
1988 buildPullRequestSummariesGraphQlQuery([{ repository: "acme/web", number: 7 }]),1997 buildPullRequestSummariesGraphQlQuery([{ repository: "acme/web", number: 7 }])?.query,
1989 ).not.toContain("stack {");1998 ).not.toContain("stack {");
1990 const pullRequest = (number: number, stack: Record<string, unknown>) => ({1999 const pullRequest = (number: number, stack: Record<string, unknown>) => ({
1991 pullRequest: {2000 pullRequest: {
...@@ -2019,16 +2028,22 @@ describe("batched pull request summaries", () => {...@@ -2019,16 +2028,22 @@ describe("batched pull request summaries", () => {
20192028
2020describe("pull request watch fingerprints", () => {2029describe("pull request watch fingerprints", () => {
2021 it("asks for every pull request in one aliased read, and refuses an unsafe selector", () => {2030 it("asks for every pull request in one aliased read, and refuses an unsafe selector", () => {
2022 const query = buildPullRequestWatchFingerprintsGraphQlQuery([2031 const document = buildPullRequestWatchFingerprintsGraphQlQuery([
2023 { repository: "pingdotgg/t3code", number: 7 },2032 { repository: "pingdotgg/t3code", number: 7 },
2024 { repository: "pingdotgg/lakebed", number: 8 },2033 { repository: "pingdotgg/lakebed", number: 8 },
2025 ]);2034 ]);
2026 expect(query).toContain(2035 expect(document?.query).toContain(
2027 'w0: repository(owner: "pingdotgg", name: "t3code") { pullRequest(number: 7)',2036 "w0: repository(owner: $w0_owner, name: $w0_name) { pullRequest(number: $w0_number)",
2028 );2037 );
2029 expect(query).toContain(2038 expect(document?.query).toContain("w1: repository(owner: $w1_owner, name: $w1_name)");
2030 'w1: repository(owner: "pingdotgg", name: "lakebed") { pullRequest(number: 8)',2039 expect(document?.variables).toEqual({
2031 );2040 w0_owner: "pingdotgg",
2041 w0_name: "t3code",
2042 w0_number: 7,
2043 w1_owner: "pingdotgg",
2044 w1_name: "lakebed",
2045 w1_number: 8,
2046 });
2032 expect(2047 expect(
2033 buildPullRequestWatchFingerprintsGraphQlQuery([{ repository: 'evil") { x', number: 1 }]),2048 buildPullRequestWatchFingerprintsGraphQlQuery([{ repository: 'evil") { x', number: 1 }]),
2034 ).toBeNull();2049 ).toBeNull();
apps/server/src/pullRequest/gitHubPullRequestJson.ts+87-78
...@@ -35,6 +35,7 @@ import type {...@@ -35,6 +35,7 @@ import type {
35import { quoteGitPatchPath } from "@t3tools/shared/gitPatchPath";35import { quoteGitPatchPath } from "@t3tools/shared/gitPatchPath";
36import { decodeJsonResult } from "@t3tools/shared/schemaJson";36import { decodeJsonResult } from "@t3tools/shared/schemaJson";
3737
38import { aliasedGraphQlDocument, type GraphQlDocument } from "../sourceControl/githubGraphQl.ts";
38import { dedupeChecks } from "./pullRequestChecks.ts";39import { dedupeChecks } from "./pullRequestChecks.ts";
3940
40/**41/**
...@@ -1917,53 +1918,81 @@ function toStackMembership(...@@ -1917,53 +1918,81 @@ function toStackMembership(
1917 : undefined;1918 : undefined;
1918}1919}
19191920
1920/** What a repository selector may hold before it is written into a GraphQL document unquoted. */1921/** What GitHub allows in an owner or repository name. */
1921const REPOSITORY_PART = /^[A-Za-z0-9._-]+$/;1922const REPOSITORY_PART = /^[A-Za-z0-9._-]+$/;
19221923
1924/**
1925 * The owner and name of an `owner/name` selector, or null for one GitHub cannot name. A batch is
1926 * one document, and a single unanswerable alias fails all of it, so these are refused up front.
1927 */
1928function repositoryParts(
1929 repository: string,
1930): { readonly owner: string; readonly name: string } | null {
1931 const [owner, name, ...rest] = repository.trim().split("/");
1932 if (rest.length > 0 || owner === undefined || name === undefined) return null;
1933 return REPOSITORY_PART.test(owner) && REPOSITORY_PART.test(name) ? { owner, name } : null;
1934}
1935
1936const isPullRequestNumber = (number: number) => Number.isSafeInteger(number) && number > 0;
1937
1938/** One aliased `repository { pullRequest { selection } }` per change request, on one host. */
1939function aliasedPullRequestsDocument(
1940 name: string,
1941 alias: string,
1942 changeRequests: ReadonlyArray<{ readonly repository: string; readonly number: number }>,
1943 selection: string,
1944): GraphQlDocument | null {
1945 const addressed = changeRequests.map((changeRequest) => ({
1946 parts: repositoryParts(changeRequest.repository),
1947 number: changeRequest.number,
1948 }));
1949 if (addressed.some(({ parts, number }) => parts === null || !isPullRequestNumber(number))) {
1950 return null;
1951 }
1952 return aliasedGraphQlDocument({
1953 operation: "query",
1954 name,
1955 alias,
1956 items: addressed,
1957 variables: ({ parts, number }) => ({
1958 owner: ["String!", parts!.owner],
1959 name: ["String!", parts!.name],
1960 number: ["Int!", number],
1961 }),
1962 field: ({ owner, name, number }) =>
1963 `repository(owner: ${owner}, name: ${name}) { pullRequest(number: ${number}) { ${selection} } }`,
1964 });
1965}
1966
1923/**1967/**
1924 * The line counts for rows a listing already handed over, as one aliased lookup each.1968 * The line counts for rows a listing already handed over, as one aliased lookup each.
1925 *1969 *
1926 * Aliases rather than `nodes(ids:)` because the caller asks in the terms the page holds — a1970 * Aliases rather than `nodes(ids:)` because the caller asks in the terms the page holds — a
1927 * repository and a number — and never sees a node id. Owner, name and number are written into1971 * repository and a number — and never sees a node id. Null for a selector GitHub cannot name,
1928 * the document, so each is checked against what GitHub can actually name first: null for anything1972 * which the caller reports rather than sends, and for an empty request.
1929 * else, which the caller reports rather than sends.
1930 *
1931 * Null too for an empty request, since a GraphQL document with no selection is not a document.
1932 */1973 */
1933export function buildPullRequestStatsGraphQlQuery(1974export function buildPullRequestStatsGraphQlQuery(
1934 changeRequests: ReadonlyArray<{ readonly repository: string; readonly number: number }>,1975 changeRequests: ReadonlyArray<{ readonly repository: string; readonly number: number }>,
1935): string | null {1976): GraphQlDocument | null {
1936 if (changeRequests.length === 0) return null;1977 return aliasedPullRequestsDocument(
1937 const selections: string[] = [];1978 "PullRequestStats",
1938 for (const [index, changeRequest] of changeRequests.entries()) {1979 "s",
1939 const [owner, name, ...rest] = changeRequest.repository.trim().split("/");1980 changeRequests,
1940 if (rest.length > 0 || owner === undefined || name === undefined) return null;1981 "additions deletions",
1941 if (!REPOSITORY_PART.test(owner) || !REPOSITORY_PART.test(name)) return null;1982 );
1942 if (!Number.isSafeInteger(changeRequest.number) || changeRequest.number <= 0) return null;
1943 selections.push(
1944 ` s${index}: repository(owner: "${owner}", name: "${name}") { pullRequest(number: ${changeRequest.number}) { additions deletions } }`,
1945 );
1946 }
1947 return `query {\n${selections.join("\n")}\n}`;
1948}1983}
19491984
1950/** Stack membership for the visible rows of a per-repository listing. */1985/** Stack membership for the visible rows of a per-repository listing. */
1951export function buildPullRequestStackMembershipsGraphQlQuery(1986export function buildPullRequestStackMembershipsGraphQlQuery(
1952 repository: string,1987 repository: string,
1953 numbers: ReadonlyArray<number>,1988 numbers: ReadonlyArray<number>,
1954): string | null {1989): GraphQlDocument | null {
1955 if (numbers.length === 0) return null;1990 return aliasedPullRequestsDocument(
1956 const [owner, name, ...rest] = repository.trim().split("/");1991 "PullRequestStackMemberships",
1957 if (rest.length > 0 || owner === undefined || name === undefined) return null;1992 "s",
1958 if (!REPOSITORY_PART.test(owner) || !REPOSITORY_PART.test(name)) return null;1993 numbers.map((number) => ({ repository, number })),
1959 const selections: string[] = [];1994 STACK_MEMBERSHIP_SELECTION,
1960 for (const [index, number] of numbers.entries()) {1995 );
1961 if (!Number.isSafeInteger(number) || number <= 0) return null;
1962 selections.push(
1963 ` s${index}: repository(owner: "${owner}", name: "${name}") { pullRequest(number: ${number}) { stack { number size baseRefName } stackEntry { position } } }`,
1964 );
1965 }
1966 return `query PullRequestStackMemberships {\n${selections.join("\n")}\n}`;
1967}1996}
19681997
1969const decodeStackMemberships = decodeJsonResult(RawStackMembershipsSchema);1998const decodeStackMemberships = decodeJsonResult(RawStackMembershipsSchema);
...@@ -2024,26 +2053,17 @@ const PULL_REQUEST_SUMMARY_SELECTION =...@@ -2024,26 +2053,17 @@ const PULL_REQUEST_SUMMARY_SELECTION =
2024 "latestReviews(first: 20) { nodes { state author { login } } } " +2053 "latestReviews(first: 20) { nodes { state author { login } } } " +
2025 "commits(last: 1) { nodes { commit { statusCheckRollup { state } } } }";2054 "commits(last: 1) { nodes { commit { statusCheckRollup { state } } } }";
20262055
2027/**2056/** Summaries for pull requests anywhere on one host, one aliased lookup each; null when unsafe. */
2028 * Summaries for pull requests anywhere on one host, one aliased lookup each. Checked and written
2029 * into the document the way `buildPullRequestStatsGraphQlQuery` does, so null means "do not send".
2030 */
2031export function buildPullRequestSummariesGraphQlQuery(2057export function buildPullRequestSummariesGraphQlQuery(
2032 changeRequests: ReadonlyArray<{ readonly repository: string; readonly number: number }>,2058 changeRequests: ReadonlyArray<{ readonly repository: string; readonly number: number }>,
2033 includeStacks = false,2059 includeStacks = false,
2034): string | null {2060): GraphQlDocument | null {
2035 if (changeRequests.length === 0) return null;2061 return aliasedPullRequestsDocument(
2036 const selections: string[] = [];2062 "PullRequestSummaries",
2037 for (const [index, changeRequest] of changeRequests.entries()) {2063 "s",
2038 const [owner, name, ...rest] = changeRequest.repository.trim().split("/");2064 changeRequests,
2039 if (rest.length > 0 || owner === undefined || name === undefined) return null;2065 `${PULL_REQUEST_SUMMARY_SELECTION}${includeStacks ? ` ${STACK_MEMBERSHIP_SELECTION}` : ""}`,
2040 if (!REPOSITORY_PART.test(owner) || !REPOSITORY_PART.test(name)) return null;2066 );
2041 if (!Number.isSafeInteger(changeRequest.number) || changeRequest.number <= 0) return null;
2042 selections.push(
2043 ` s${index}: repository(owner: "${owner}", name: "${name}") { pullRequest(number: ${changeRequest.number}) { ${PULL_REQUEST_SUMMARY_SELECTION}${includeStacks ? ` ${STACK_MEMBERSHIP_SELECTION}` : ""} } }`,
2044 );
2045 }
2046 return `query PullRequestSummaries {\n${selections.join("\n")}\n}`;
2047}2067}
20482068
2049/** One pull request's summary, aliased the way the batch is so the same decoder reads it. */2069/** One pull request's summary, aliased the way the batch is so the same decoder reads it. */
...@@ -2170,19 +2190,13 @@ const PULL_REQUEST_WATCH_FINGERPRINT_SELECTION =...@@ -2170,19 +2190,13 @@ const PULL_REQUEST_WATCH_FINGERPRINT_SELECTION =
2170/** Watch fingerprints for pull requests on one host, one aliased lookup each; null when unsafe. */2190/** Watch fingerprints for pull requests on one host, one aliased lookup each; null when unsafe. */
2171export function buildPullRequestWatchFingerprintsGraphQlQuery(2191export function buildPullRequestWatchFingerprintsGraphQlQuery(
2172 changeRequests: ReadonlyArray<{ readonly repository: string; readonly number: number }>,2192 changeRequests: ReadonlyArray<{ readonly repository: string; readonly number: number }>,
2173): string | null {2193): GraphQlDocument | null {
2174 if (changeRequests.length === 0) return null;2194 return aliasedPullRequestsDocument(
2175 const selections: string[] = [];2195 "PullRequestWatchFingerprints",
2176 for (const [index, changeRequest] of changeRequests.entries()) {2196 "w",
2177 const [owner, name, ...rest] = changeRequest.repository.trim().split("/");2197 changeRequests,
2178 if (rest.length > 0 || owner === undefined || name === undefined) return null;2198 PULL_REQUEST_WATCH_FINGERPRINT_SELECTION,
2179 if (!REPOSITORY_PART.test(owner) || !REPOSITORY_PART.test(name)) return null;2199 );
2180 if (!Number.isSafeInteger(changeRequest.number) || changeRequest.number <= 0) return null;
2181 selections.push(
2182 ` w${index}: repository(owner: "${owner}", name: "${name}") { pullRequest(number: ${changeRequest.number}) { ${PULL_REQUEST_WATCH_FINGERPRINT_SELECTION} } }`,
2183 );
2184 }
2185 return `query PullRequestWatchFingerprints {\n${selections.join("\n")}\n}`;
2186}2200}
21872201
2188/**2202/**
...@@ -3357,25 +3371,20 @@ export function decodePullRequestFilesViewedJson(...@@ -3357,25 +3371,20 @@ export function decodePullRequestFilesViewedJson(
3357 * request each. GitHub has no bulk form of `markFileAsViewed`/`unmarkFileAsViewed`, which each3371 * request each. GitHub has no bulk form of `markFileAsViewed`/`unmarkFileAsViewed`, which each
3358 * take a single path, so the batching is done with aliases; top-level mutation fields run in3372 * take a single path, so the batching is done with aliases; top-level mutation fields run in
3359 * write order, so the last word about a path is the one that sticks.3373 * write order, so the last word about a path is the one that sticks.
3360 *
3361 * Paths travel as variables rather than interpolated into the document, since a path is data
3362 * and a document is not.
3363 */3374 */
3364export function buildSetFilesViewedGraphQlMutation(3375export function buildSetFilesViewedGraphQlMutation(
3376 pullRequestId: string,
3365 files: ReadonlyArray<{ readonly path: string; readonly viewed: boolean }>,3377 files: ReadonlyArray<{ readonly path: string; readonly viewed: boolean }>,
3366): { readonly query: string; readonly variables: Readonly<Record<string, string>> } | null {3378): GraphQlDocument | null {
3367 if (files.length === 0) return null;3379 return aliasedGraphQlDocument({
3368 const parameters = files.map((_, index) => `$path${index}: String!`).join(", ");3380 operation: "mutation",
3369 const fields = files3381 alias: "f",
3370 .map(3382 items: files,
3371 (file, index) =>3383 shared: { pullRequestId: ["ID!", pullRequestId] },
3372 ` f${index}: ${file.viewed ? "markFileAsViewed" : "unmarkFileAsViewed"}(input: { pullRequestId: $pullRequestId, path: $path${index} }) { clientMutationId }`,3384 variables: (file) => ({ path: ["String!", file.path] }),
3373 )3385 field: ({ path }, file) =>
3374 .join("\n");3386 `${file.viewed ? "markFileAsViewed" : "unmarkFileAsViewed"}(input: { pullRequestId: $pullRequestId, path: ${path} }) { clientMutationId }`,
3375 return {3387 });
3376 query: `mutation($pullRequestId: ID!, ${parameters}) {\n${fields}\n}`,
3377 variables: Object.fromEntries(files.map((file, index) => [`path${index}`, file.path])),
3378 };
3379}3388}
33803389
3381/** One pull request as the stacks API lists it: a number, a head, and whether it is done. */3390/** One pull request as the stacks API lists it: a number, a head, and whether it is done. */
apps/server/src/pullRequest/githubStackActions.ts+13-7
...@@ -9,6 +9,7 @@ import * as Result from "effect/Result";...@@ -9,6 +9,7 @@ import * as Result from "effect/Result";
9import * as Schema from "effect/Schema";9import * as Schema from "effect/Schema";
1010
11import * as GitHubApi from "../sourceControl/GitHubApi.ts";11import * as GitHubApi from "../sourceControl/GitHubApi.ts";
12import { aliasedGraphQlDocument } from "../sourceControl/githubGraphQl.ts";
12import { decodePullRequestStacksJson } from "./gitHubPullRequestJson.ts";13import { decodePullRequestStacksJson } from "./gitHubPullRequestJson.ts";
13import { cascadeRebaseStack } from "./githubStackRebase.ts";14import { cascadeRebaseStack } from "./githubStackRebase.ts";
1415
...@@ -192,13 +193,18 @@ export const runGitHubStackAction = Effect.fn("runGitHubStackAction")(function*...@@ -192,13 +193,18 @@ export const runGitHubStackAction = Effect.fn("runGitHubStackAction")(function*
192 host: input.host,193 host: input.host,
193 operation: "runGitHubStackAction",194 operation: "runGitHubStackAction",
194 allowReserve: true,195 allowReserve: true,
195 variables: { owner, name },196 // `open` is non-empty here, so there is always a document.
196 query: `query($owner:String!,$name:String!){repository(owner:$owner,name:$name){${open197 ...aliasedGraphQlDocument({
197 .map(198 operation: "query",
198 (layer) =>199 alias: "pr",
199 `pr${layer.number}:pullRequest(number:${layer.number}){headRepository{viewerPermission} maintainerCanModify}`,200 key: (layer) => layer.number,
200 )201 items: open,
201 .join(" ")}}}`,202 shared: { owner: ["String!", owner], name: ["String!", name] },
203 variables: (layer) => ({ number: ["Int!", layer.number] }),
204 field: ({ number }) =>
205 `pullRequest(number: ${number}) { headRepository { viewerPermission } maintainerCanModify }`,
206 within: (fields) => `repository(owner: $owner, name: $name) {\n${fields}\n}`,
207 })!,
202 });208 });
203 const access = yield* decodeBranchAccess(permissions).pipe(209 const access = yield* decodeBranchAccess(permissions).pipe(
204 Effect.mapError((cause) => new GitHubStackResponseInvalidError({ ...identity, cause })),210 Effect.mapError((cause) => new GitHubStackResponseInvalidError({ ...identity, cause })),
apps/server/src/sourceControl/githubGraphQl.test.ts created+105
...@@ -0,0 +1,105 @@
1import { assert, describe, expect, it } from "@effect/vitest";
2import * as Effect from "effect/Effect";
3
4import { aliasedGraphQlDocument, readGraphQlPages } from "./githubGraphQl.ts";
5
6describe("aliasedGraphQlDocument", () => {
7 it("keeps every value out of the document, each under its alias's own variable", () => {
8 const document = aliasedGraphQlDocument({
9 operation: "query",
10 name: "Lookups",
11 alias: "s",
12 items: ['acme") { x } #', "web"],
13 shared: { owner: ["String!", "acme"] },
14 variables: (repo) => ({ name: ["String!", repo] }),
15 field: ({ name }) => `repository(owner: $owner, name: ${name}) { id }`,
16 });
17 assert.isNotNull(document);
18 expect(document!.query).toBe(
19 "query Lookups($owner: String!, $s0_name: String!, $s1_name: String!) {\n" +
20 " s0: repository(owner: $owner, name: $s0_name) { id }\n" +
21 " s1: repository(owner: $owner, name: $s1_name) { id }\n}",
22 );
23 expect(document!.variables).toEqual({
24 owner: "acme",
25 s0_name: 'acme") { x } #',
26 s1_name: "web",
27 });
28 });
29
30 it("aliases by key and nests inside a shared parent when asked", () => {
31 const document = aliasedGraphQlDocument({
32 operation: "query",
33 alias: "pr",
34 key: (number) => number,
35 items: [2, 3],
36 variables: (number) => ({ number: ["Int!", number] }),
37 field: ({ number }) => `pullRequest(number: ${number}) { id }`,
38 within: (fields) => `repository(owner: "a", name: "b") {\n${fields}\n}`,
39 });
40 expect(document?.query).toContain(" pr2: pullRequest(number: $pr2_number) { id }");
41 expect(document?.query).toContain(" pr3: pullRequest(number: $pr3_number) { id }");
42 expect(document?.variables).toEqual({ pr2_number: 2, pr3_number: 3 });
43 });
44
45 it("asks for nothing when there is nothing to ask", () => {
46 expect(
47 aliasedGraphQlDocument({
48 operation: "mutation",
49 alias: "f",
50 items: [],
51 variables: () => ({}),
52 field: () => "x",
53 }),
54 ).toBeNull();
55 });
56});
57
58describe("readGraphQlPages", () => {
59 const pages = (cursors: ReadonlyArray<string | null>) => {
60 const asked: Array<string | null> = [];
61 const read = (after: string | null) =>
62 Effect.sync(() => {
63 asked.push(after);
64 return { next: cursors[asked.length - 1] ?? null };
65 });
66 return { asked, read };
67 };
68
69 it.effect("follows the cursor until GitHub has no more", () =>
70 Effect.gen(function* () {
71 const { asked, read } = pages(["a", "b", null]);
72 const result = yield* readGraphQlPages(read, { nextCursor: (page) => page.next });
73 expect(asked).toEqual([null, "a", "b"]);
74 expect(result.truncated).toBe(false);
75 }),
76 );
77
78 it.effect("stops as truncated at the page limit, at `until`, or on a repeated cursor", () =>
79 Effect.gen(function* () {
80 const limited = pages(["a", "b", "c"]);
81 const atLimit = yield* readGraphQlPages(limited.read, {
82 nextCursor: (page) => page.next,
83 maxPages: 2,
84 });
85 expect(limited.asked).toEqual([null, "a"]);
86 expect(atLimit.truncated).toBe(true);
87
88 const enough = pages(["a", "b", "c"]);
89 const satisfied = yield* readGraphQlPages(enough.read, {
90 nextCursor: (page) => page.next,
91 until: (read) => read.length === 1,
92 });
93 expect(enough.asked).toEqual([null]);
94 expect(satisfied.truncated).toBe(true);
95
96 const stuck = pages(["a", "a", "a"]);
97 const repeated = yield* readGraphQlPages(stuck.read, {
98 nextCursor: (page) => page.next,
99 from: "start",
100 });
101 expect(stuck.asked).toEqual(["start", "a"]);
102 expect(repeated.truncated).toBe(true);
103 }),
104 );
105});
apps/server/src/sourceControl/githubGraphQl.ts created+100
...@@ -0,0 +1,100 @@
1import * as Effect from "effect/Effect";
2
3/** GraphQL variables by name, each with the type it is declared as and the value it carries. */
4export type GraphQlVariables = Readonly<Record<string, readonly [type: string, value: unknown]>>;
5
6export interface GraphQlDocument {
7 readonly query: string;
8 readonly variables: Readonly<Record<string, unknown>>;
9}
10
11/**
12 * One document asking the same field for many items, each under its own alias: the prefix and
13 * the item's position (`s0`, `s1`, …), or its `key` when the answer is easier read by one. GitHub
14 * has no bulk form of most lookups, so aliases are how a batch travels in one request.
15 *
16 * Every value travels as a variable renamed per alias (`$s0_owner`), and `field` is handed those
17 * placeholders rather than the values: nothing an item holds is written into the document.
18 * `shared` variables are declared once and referenced by their own name; `within` nests the aliased
19 * fields inside one parent selection, such as a single repository.
20 *
21 * Null for no items, since a document with no selection is not a document.
22 */
23export function aliasedGraphQlDocument<Item, Variables extends GraphQlVariables>(input: {
24 readonly operation: "query" | "mutation";
25 /** The operation's name, which tells one batch from another in logs and recorded traffic. */
26 readonly name?: string;
27 readonly alias: string;
28 readonly key?: (item: Item, index: number) => number;
29 readonly items: ReadonlyArray<Item>;
30 readonly variables: (item: Item) => Variables;
31 readonly field: (
32 placeholders: { readonly [Name in keyof Variables]: string },
33 item: Item,
34 ) => string;
35 readonly shared?: GraphQlVariables;
36 readonly within?: (fields: string) => string;
37}): GraphQlDocument | null {
38 if (input.items.length === 0) return null;
39 const declarations: string[] = [];
40 const variables: Record<string, unknown> = {};
41 for (const [name, [type, value]] of Object.entries(input.shared ?? {})) {
42 declarations.push(`$${name}: ${type}`);
43 variables[name] = value;
44 }
45 const fields = input.items.map((item, index) => {
46 const alias = `${input.alias}${input.key?.(item, index) ?? index}`;
47 const placeholders: Record<string, string> = {};
48 for (const [name, [type, value]] of Object.entries(input.variables(item))) {
49 const variable = `${alias}_${name}`;
50 declarations.push(`$${variable}: ${type}`);
51 variables[variable] = value;
52 placeholders[name] = `$${variable}`;
53 }
54 return ` ${alias}: ${input.field(placeholders as { readonly [Name in keyof Variables]: string }, item)}`;
55 });
56 const selection = fields.join("\n");
57 const parameters = declarations.length === 0 ? "" : `(${declarations.join(", ")})`;
58 return {
59 query: `${input.operation}${input.name === undefined ? "" : ` ${input.name}`}${parameters} {\n${input.within?.(selection) ?? selection}\n}`,
60 variables,
61 };
62}
63
64/**
65 * The pages of a GraphQL connection, read one after another from `from` (the first page by
66 * default) until GitHub has no more, `until` is satisfied, or `maxPages` have been read. `read` is
67 * handed the pages so far, for reads that size or check the next page by what has already arrived.
68 *
69 * `truncated` says pages remained when reading stopped. A cursor GitHub hands back a second time
70 * would page forever, so it stops the read as truncated too.
71 */
72export const readGraphQlPages = <Page, E, R>(
73 read: (after: string | null, pages: ReadonlyArray<Page>) => Effect.Effect<Page, E, R>,
74 options: {
75 readonly nextCursor: (page: NoInfer<Page>) => string | null;
76 readonly from?: string | null;
77 readonly maxPages?: number;
78 readonly until?: (pages: ReadonlyArray<NoInfer<Page>>) => boolean;
79 },
80): Effect.Effect<{ readonly pages: ReadonlyArray<Page>; readonly truncated: boolean }, E, R> =>
81 Effect.gen(function* () {
82 const pages: Page[] = [];
83 const seen = new Set<string>();
84 let after = options.from ?? null;
85 while (true) {
86 const page: Page = yield* read(after, pages);
87 pages.push(page);
88 const next = options.nextCursor(page);
89 if (next === null) return { pages, truncated: false };
90 if (
91 seen.has(next) ||
92 pages.length >= (options.maxPages ?? Infinity) ||
93 options.until?.(pages) === true
94 ) {
95 return { pages, truncated: true };
96 }
97 seen.add(next);
98 after = next;
99 }
100 });