diff --git a/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.ts b/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.ts index 43f929163db..19ab683c199 100644 --- a/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.ts +++ b/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.ts @@ -218,6 +218,9 @@ function actionArgs( return ["--draft", "true"]; case "close": return ["--status", "abandoned"]; + // Never reached: this host does not declare the action, so nothing offers it. + case "update-branch": + return []; case "reopen": return ["--status", "active"]; } diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts index bf7e8951afe..be2aee98ef9 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts @@ -5,6 +5,7 @@ import { ChildProcessSpawner } from "effect/unstable/process"; import * as GitHubCli from "../sourceControl/GitHubCli.ts"; import * as GitHubPullRequestCli from "./GitHubPullRequestCli.ts"; +import { BASE_COMPARISON_GRAPHQL_QUERY } from "./gitHubPullRequestJson.ts"; const mockedExecute = vi.fn(); @@ -551,6 +552,107 @@ layer("GitHubPullRequestCli.layer", (it) => { }), ); + it.effect("carries the further narrowings into the search as qualifiers", () => + Effect.gen(function* () { + mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + yield* cli.listPullRequests({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + state: "open", + involvement: "all", + viewer: "bilal", + limit: 10, + filters: { + draft: "hide", + review: "changes-requested", + checks: "failing", + labels: [["needs design"], ['quo"te']], + excludedLabels: ["wip"], + author: "octocat", + }, + }); + + // Quotes around anything a reader typed, and the one character that could end a quoted + // value early dropped rather than escaped. + expect(searchOfCall(0)).toBe( + 'label:"needs design" label:"quote" -label:"wip" author:"octocat" draft:false ' + + "review:changes_requested status:failure sort:updated-desc", + ); + }), + ); + + it.effect("sends one label qualifier per group, its names joined the way GitHub ors them", () => + Effect.gen(function* () { + mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + yield* cli.listPullRequests({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + state: "open", + involvement: "all", + viewer: "bilal", + limit: 10, + filters: { labels: [["size:S", "size:XS"], ["bug"]] }, + }); + + // One qualifier satisfied by either size, and a second one that must hold as well. + expect(searchOfCall(0)).toBe('label:"size:S","size:XS" label:"bug" sort:updated-desc'); + expect(callAt(0).args).toContain('label:"size:S","size:XS" label:"bug" sort:updated-desc'); + }), + ); + + it.effect("takes an empty filtered answer as an answer rather than falling back", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce(Effect.succeed(output("[]"))); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + const batch = yield* cli.listPullRequests({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + state: "open", + involvement: "all", + viewer: "bilal", + limit: 10, + filters: { draft: "hide", checks: "passing", excludedLabels: ["wip"], author: "OctoCat" }, + }); + + // The filters were qualifiers on that very search, so nothing matching them exists. The + // search-free fallback is for a repository the index does not cover, and it could not + // judge `checks` at all — no listed row says anything about them. + assert.strictEqual(mockedExecute.mock.calls.length, 1); + assert.deepStrictEqual(batch.items, []); + }), + ); + + it.effect("carries the further narrowings into a batched search", () => + Effect.gen(function* () { + mockedExecute.mockReturnValue(Effect.succeed(searchPage([]))); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + yield* cli.searchPullRequests({ + cwd: "/w", + host: "github.com", + repositories: ["acme/web"], + state: "open", + involvement: "all", + viewer: "bilal", + limit: 10, + filters: { draft: "only", review: "none", labels: [["bug"]] }, + }); + + assert.strictEqual( + searchQueryOfCall(0), + 'is:pr is:open label:"bug" draft:true review:none sort:updated-desc repo:acme/web', + ); + }), + ); + it.effect("quotes a search, so it cannot add a qualifier or a flag of its own", () => Effect.gen(function* () { mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); @@ -819,6 +921,40 @@ layer("GitHubPullRequestCli.layer", (it) => { }), ); + it.effect("updates a stale branch with a merge commit unless asked to rebase", () => + Effect.gen(function* () { + mockedExecute.mockReturnValue(Effect.succeed(output(""))); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + yield* cli.runPullRequestAction({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + action: "update-branch", + }); + // GitHub's own default, and `gh`'s: a merge commit unless the rebase flag says otherwise. + expect(callAt(0).args).toEqual(["pr", "update-branch", "7", "--repo", "github.com/acme/web"]); + + yield* cli.runPullRequestAction({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + action: "update-branch", + updateMethod: "rebase", + }); + expect(callAt(1).args).toEqual([ + "pr", + "update-branch", + "7", + "--repo", + "github.com/acme/web", + "--rebase", + ]); + }), + ); + it.effect("merges with the strategy it was asked for", () => Effect.gen(function* () { mockedExecute.mockReturnValue(Effect.succeed(output(""))); @@ -1472,7 +1608,7 @@ layer("GitHubPullRequestCli.layer", (it) => { expect(detail.body).toBe("Core body"); expect(activity.author?.login).toBe("octocat"); expect(callAt(0).args.at(-1)).toBe( - "number,title,url,author,headRefName,baseRefName,state,isDraft,mergeable,additions,deletions,createdAt,updatedAt,mergedAt,reviewRequests,labels,body,changedFiles,closedAt,statusCheckRollup", + "number,title,url,author,headRefName,baseRefName,state,isDraft,mergeable,reviewDecision,additions,deletions,createdAt,updatedAt,mergedAt,reviewRequests,labels,statusCheckRollup,body,changedFiles,closedAt,headRepositoryOwner", ); expect(callAt(1).args.at(-1)).toBe("author,comments,reviews,commits"); }), @@ -1634,6 +1770,58 @@ layer("GitHubPullRequestCli.layer", (it) => { }), ); + it.effect("sends the base comparison's variables as gh flags, not as bare words", () => + Effect.gen(function* () { + mockedExecute.mockReturnValue( + Effect.succeed( + output( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ + data: { + repository: { + pullRequest: { + viewerCanUpdateBranch: true, + baseRef: { compare: { behindBy: 4 } }, + }, + }, + }, + }), + ), + ), + ); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + const comparison = yield* cli.getPullRequestBaseComparison({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + headRef: "fork:feat/page", + }); + + // The tuples are flattened straight into argv, so a variable without its flag is a + // positional argument gh refuses outright. + const args = callAt(0).args; + expect(args).toEqual([ + "api", + "graphql", + "--hostname", + "github.com", + "-f", + "owner=acme", + "-f", + "name=web", + "-F", + "number=7", + "-f", + "headRef=fork:feat/page", + "-f", + `query=${BASE_COMPARISON_GRAPHQL_QUERY}`, + ]); + expect(comparison).toEqual({ behindBy: 4, viewerCanUpdate: true }); + }), + ); + it.effect("reads the viewer's role off the same call as the merge settings", () => Effect.gen(function* () { mockedExecute.mockReturnValue( diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.ts index 392fac8564f..e35f0dc7089 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.ts @@ -7,13 +7,16 @@ import type { PullRequestAction, PullRequestActor, PullRequestInvolvement, + PullRequestListFilters, PullRequestListState, PullRequestMergeMethod, + PullRequestOmittedFileStat, PullRequestReviewCommentDraft, PullRequestReviewVerdict, PullRequestReviewerCandidateList, PullRequestReviewerKind, PullRequestThreadComment, + PullRequestUpdateMethod, } from "@t3tools/contracts"; import * as GitHubCli from "../sourceControl/GitHubCli.ts"; @@ -30,6 +33,7 @@ import { decodePullRequestStatsJson, decodeRepositoryAccessJson, decodeReviewerCandidatesJson, + decodeReviewDismissalsJson, decodeReviewThreadCommentsJson, decodeReviewThreadsJson, buildPullRequestStatsGraphQlQuery, @@ -37,18 +41,22 @@ import { pullRequestSearchGraphQlQuery, PULL_REQUEST_SEARCH_MAX_ROWS, PULL_REQUEST_ACTIVITY_JSON_FIELDS, + BASE_COMPARISON_GRAPHQL_QUERY, + decodeBaseComparisonJson, PULL_REQUEST_DETAIL_JSON_FIELDS, PULL_REQUEST_LIST_JSON_FIELDS, REPOSITORY_ACCESS_JSON_FIELDS, RESOLVE_REVIEW_THREAD_GRAPHQL_MUTATION, REVIEWER_CANDIDATES_GRAPHQL_QUERY, REVIEW_THREAD_COMMENTS_GRAPHQL_QUERY, + REVIEW_DISMISSALS_GRAPHQL_QUERY, REVIEW_THREAD_REPLY_GRAPHQL_MUTATION, REVIEW_THREADS_GRAPHQL_QUERY, reviewThreadConversation, UNRESOLVE_REVIEW_THREAD_GRAPHQL_MUTATION, VIEWER_PERMISSIONS_GRAPHQL_QUERY, decodeViewerPermissionsJson, + type GitHubBaseComparison, type GitHubPullRequestDetail, type GitHubPullRequestActivity, type GitHubPullRequestListItem, @@ -270,6 +278,8 @@ export interface GitHubPullRequestDiffSlice { readonly truncated: boolean; /** Where the next slice starts, or null once the patch is whole. */ readonly nextCursor: string | null; + /** GitHub's own counts for the files whose hunks it withheld from this slice. */ + readonly omittedFileStats?: ReadonlyArray; } export class GitHubPullRequestCli extends Context.Service< @@ -291,6 +301,8 @@ export class GitHubPullRequestCli extends Context.Service< readonly query?: string | undefined; /** Where to carry on from, as a `updated:` qualifier on the same search. */ readonly cursor?: ProviderListCursor | undefined; + /** Further narrowings, as qualifiers on the search and as a local pass on the fallback. */ + readonly filters?: PullRequestListFilters | undefined; }) => Effect.Effect; /** @@ -309,6 +321,7 @@ export class GitHubPullRequestCli extends Context.Service< readonly limit: number; readonly query?: string | undefined; readonly cursor?: ProviderListCursor | undefined; + readonly filters?: PullRequestListFilters | undefined; }) => Effect.Effect; /** The line counts the search leaves out, for rows already on the page. */ @@ -328,6 +341,20 @@ export class GitHubPullRequestCli extends Context.Service< readonly number: number; }) => Effect.Effect; + /** + * How far the branch trails its base, and whether this viewer may update it. Its own read + * because the comparison needs the head ref the detail answers with — a fork's branch is not + * addressable in the base repository by name alone. + */ + readonly getPullRequestBaseComparison: (input: { + readonly cwd: string; + readonly repository: string; + readonly host: string; + readonly number: number; + /** Qualified `owner:branch`, which is the only form a fork's head resolves under. */ + readonly headRef: string; + }) => Effect.Effect; + readonly getPullRequestActivity: (input: { readonly cwd: string; readonly repository: string; @@ -418,6 +445,7 @@ export class GitHubPullRequestCli extends Context.Service< readonly number: number; readonly action: PullRequestAction; readonly mergeMethod?: PullRequestMergeMethod; + readonly updateMethod?: PullRequestUpdateMethod; }) => Effect.Effect; readonly commentOnPullRequest: (input: { @@ -501,6 +529,64 @@ function searchPhrase(query: string): string { return `"${query.replaceAll("\\", "\\\\").replaceAll('"', '\\"')}"`; } +/** GitHub's own spelling of a review state, which is not the contract's. */ +const REVIEW_QUALIFIERS = { + approved: "approved", + "changes-requested": "changes_requested", + "review-required": "required", + none: "none", +} as const; + +/** + * The extra narrowings as GitHub search qualifiers. Values a reader typed are quoted, and the + * one character that could end the quoted value early is dropped rather than escaped: no GitHub + * label or login holds a double quote, so there is nothing to preserve and everything to lose. + */ +function qualifierValue(value: string): string { + return `"${value.replaceAll('"', "").trim()}"`; +} + +function filterQualifiers(filters: PullRequestListFilters | undefined): ReadonlyArray { + if (filters === undefined) return []; + return [ + // One qualifier per group, its names joined by commas — GitHub's own OR. + ...(filters.labels ?? []).flatMap((group) => + group.length === 0 ? [] : [`label:${group.map(qualifierValue).join(",")}`], + ), + ...(filters.excludedLabels ?? []).map((label) => `-label:${qualifierValue(label)}`), + ...(filters.author === undefined ? [] : [`author:${qualifierValue(filters.author)}`]), + ...(filters.draft === undefined ? [] : [`draft:${filters.draft === "only"}`]), + ...(filters.review === undefined ? [] : [`review:${REVIEW_QUALIFIERS[filters.review]}`]), + ...(filters.checks === undefined + ? [] + : [`status:${filters.checks === "passing" ? "success" : "failure"}`]), + ]; +} + +/** + * The same narrowings over a row that has already arrived, for the search-free fallback. Checks + * are not here: no listed row carries its check state, so that one filter is the host's alone. + */ +function matchesFilters( + item: GitHubPullRequestListItem, + filters: PullRequestListFilters | undefined, +): boolean { + if (filters === undefined) return true; + const labels = item.labels.map((label) => label.name.trim().toLowerCase()); + const holds = (label: string) => labels.includes(label.trim().toLowerCase()); + return ( + (filters.draft === undefined || item.isDraft === (filters.draft === "only")) && + (filters.review === undefined || + (filters.review === "none" + ? item.reviewDecision === null + : item.reviewDecision === filters.review)) && + (filters.labels === undefined || filters.labels.every((group) => group.some(holds))) && + (filters.excludedLabels === undefined || !filters.excludedLabels.some(holds)) && + (filters.author === undefined || + item.author?.login.toLowerCase() === filters.author.trim().toLowerCase()) + ); +} + function involvementArgs(input: { readonly state: PullRequestListState; readonly involvement: PullRequestInvolvement; @@ -513,6 +599,7 @@ function involvementArgs(input: { * cannot use search at all and takes whatever order `gh pr list` answers in. */ readonly sorted: boolean; + readonly filters?: PullRequestListFilters | undefined; }): ReadonlyArray { // `--state closed` includes merged pull requests, so the Closed tab additionally excludes // them through search; `--author` and `review-requested:` are GitHub's own filters. `gh` @@ -531,6 +618,7 @@ function involvementArgs(input: { // sharing one instant are ordinary and the caller drops the ones it has already sent — // asking for strictly older would lose the rest of them instead. ...(input.cursor === undefined ? [] : [`updated:<=${input.cursor.updatedBefore}`]), + ...filterQualifiers(input.filters), // `gh pr list` answers newest-created first, which is not the order the page reads rows in // and not an order a continuation can carry on from: a change request opened last year and // touched this morning belongs at the top of the list and at the front of the first slice. @@ -550,6 +638,7 @@ function matchesUnsortedListing( readonly state: PullRequestListState; readonly involvement: PullRequestInvolvement; readonly viewer: string; + readonly filters?: PullRequestListFilters | undefined; }, ): boolean { const matchesState = input.state === "all" || item.state === input.state; @@ -560,7 +649,7 @@ function matchesUnsortedListing( ? item.author?.login.toLowerCase() === viewer : item.hasTeamReviewRequest || item.reviewRequestLogins.some((login) => login.toLowerCase() === viewer)); - return matchesState && matchesInvolvement; + return matchesState && matchesInvolvement && matchesFilters(item, input.filters); } /** What a repository selector may hold before it goes into a search as itself. */ @@ -587,6 +676,7 @@ function searchQuery(input: { readonly viewer: string; readonly query?: string | undefined; readonly cursor?: ProviderListCursor | undefined; + readonly filters?: PullRequestListFilters | undefined; }): string | null { if (input.repositories.length === 0) return null; const repositories = input.repositories.map((repository) => repository.trim()); @@ -603,6 +693,7 @@ function searchQuery(input: { ...(query.length === 0 ? [] : [searchPhrase(query)]), // Inclusive, and de-duplicated by the caller, for the reason the per-repository read gives. ...(input.cursor === undefined ? [] : [`updated:<=${input.cursor.updatedBefore}`]), + ...filterQualifiers(input.filters), // The order the page reads its rows in, and the only order a continuation can carry on from. "sort:updated-desc", ...repositories.map((repository) => `repo:${repository}`), @@ -621,10 +712,14 @@ function cursorVariable(cursor: string | null): readonly [string, string] { function actionArgs( action: PullRequestAction, mergeMethod: PullRequestMergeMethod | undefined, + updateMethod: PullRequestUpdateMethod | undefined, ): ReadonlyArray { switch (action) { case "merge": return ["merge", `--${mergeMethod ?? "merge"}`]; + // `gh` updates with a merge commit unless asked to rebase, which is GitHub's own default. + case "update-branch": + return ["update-branch", ...(updateMethod === "rebase" ? ["--rebase"] : [])]; case "ready": return ["ready"]; case "draft": @@ -796,6 +891,9 @@ export const make = Effect.gen(function* () { patch: decoded.success.patch, truncated: decoded.success.truncated, nextCursor: morePages ? String(input.page + 1) : null, + ...(decoded.success.omittedFileStats.length === 0 + ? {} + : { omittedFileStats: decoded.success.omittedFileStats }), }); }), ); @@ -977,7 +1075,12 @@ export const make = Effect.gen(function* () { // repository's whole list, which is every row the reader did not search for. The fallback // is for a repository the index does not cover, and a listing with no text to match is the // only place an empty answer can mean that. - const searched = (input.query?.trim().length ?? 0) > 0; + // Filters are qualifiers on the very same search, so a filtered read that comes back empty + // has also been answered: falling back would hand over rows the filters exclude, and the + // fallback cannot judge `checks` at all — no listed row carries its check state. + const searched = + (input.query?.trim().length ?? 0) > 0 || + (input.filters !== undefined && Object.keys(input.filters).length > 0); return read(true).pipe( Effect.flatMap((batch) => batch.items.length === 0 && input.cursor === undefined && !searched @@ -1085,6 +1188,23 @@ export const make = Effect.gen(function* () { }), ), + getPullRequestBaseComparison: (input) => { + const { owner, name } = parseRepositorySelector(input.repository); + return graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "getPullRequestBaseComparison", + variables: [ + ["-f", `owner=${owner}`], + ["-f", `name=${name}`], + ["-F", `number=${input.number}`], + ["-f", `headRef=${input.headRef}`], + ], + query: BASE_COMPARISON_GRAPHQL_QUERY, + decode: decodeBaseComparisonJson, + }); + }, + getPullRequestActivity: (input) => github .execute({ @@ -1218,6 +1338,8 @@ export const make = Effect.gen(function* () { let reviewers: ReadonlyArray = []; let commits: GitHubReviewThreadPage["commits"] = []; let viewer: GitHubReviewThreadPage["viewer"] = { canUpdate: true, didAuthor: false }; + const dismissalsByReviewId = new Map(); + let dismissalCursor: string | null = null; let cursor: string | null = null; let page = 0; do { @@ -1231,12 +1353,42 @@ export const make = Effect.gen(function* () { reviewers = read.reviewers; commits = read.commits; viewer = read.viewer; + for (const [id, message] of read.dismissalsByReviewId) + dismissalsByReviewId.set(id, message); + dismissalCursor = read.nextDismissalCursor; for (const [oid, stat] of read.commitStats) commitStats.set(oid, stat); } cursor = read.nextCursor; page += 1; } while (cursor !== null && page < REVIEW_THREAD_PAGES); + // Almost never entered: the embedded page already holds every dismissal a pull request + // ordinarily accrues. Followed so a review whose event fell past that page still finds + // its reason. + let dismissalPage = 0; + while (dismissalCursor !== null && dismissalPage < REVIEW_THREAD_PAGES) { + const read: { + readonly dismissalsByReviewId: ReadonlyMap; + readonly nextCursor: string | null; + } = yield* graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "listReviewThreadComments", + variables: [ + ["-f", `owner=${owner}`], + ["-f", `name=${name}`], + ["-F", `number=${input.number}`], + ["-f", `cursor=${dismissalCursor}`], + ], + query: REVIEW_DISMISSALS_GRAPHQL_QUERY, + decode: decodeReviewDismissalsJson, + }); + for (const [id, message] of read.dismissalsByReviewId) + dismissalsByReviewId.set(id, message); + dismissalCursor = read.nextCursor; + dismissalPage += 1; + } + // Only the threads GitHub said were unfinished cost a request; the rest arrived whole // with the page they were listed on. const finished = yield* Effect.forEach( @@ -1264,6 +1416,7 @@ export const make = Effect.gen(function* () { const reviewThreads = finished.map((entry) => entry.thread); return { comments: reviewThreadConversation(reviewThreads), + dismissalsByReviewId, reviewThreads, // GitHub's own count of each thread, so the number the page shows is the host's even // where a bound kept some of the words on GitHub. @@ -1396,7 +1549,11 @@ export const make = Effect.gen(function* () { }, runPullRequestAction: (input) => { - const [subcommand, ...flags] = actionArgs(input.action, input.mergeMethod); + const [subcommand, ...flags] = actionArgs( + input.action, + input.mergeMethod, + input.updateMethod, + ); return github .execute({ cwd: input.cwd, diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts index 453ac31dfdb..a1e24cf8718 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts @@ -71,17 +71,20 @@ describe("gitHubViewerPermissions", () => { title: "Pull request 7", url: "https://github.com/acme/web/pull/7", author: null, + headRepositoryOwner: null, headBranch: "feat/page", baseBranch: "main", state: "open", isDraft: false, mergeability: "mergeable", + reviewDecision: null, additions: 1, deletions: 1, createdAt: "2026-07-01T00:00:00Z", updatedAt: "2026-07-02T00:00:00Z", reviewRequestLogins: [], hasTeamReviewRequest: false, + checksState: null, labels: [], body: "", changedFiles: 1, @@ -104,6 +107,101 @@ describe("gitHubViewerPermissions", () => { ); }); +describe("getViewerPermissions", () => { + const openDetail = { + authorId: null, + number: 7, + title: "Pull request 7", + url: "https://github.com/acme/web/pull/7", + author: null, + headRepositoryOwner: "acme", + headBranch: "feat/page", + baseBranch: "main", + state: "open" as const, + isDraft: false, + mergeability: "mergeable" as const, + reviewDecision: null, + additions: 1, + deletions: 1, + createdAt: "2026-07-01T00:00:00Z", + updatedAt: "2026-07-02T00:00:00Z", + reviewRequestLogins: [], + hasTeamReviewRequest: false, + checksState: null, + labels: [], + body: "", + changedFiles: 1, + mergedAt: null, + closedAt: null, + checks: [], + comments: [], + commits: [], + }; + + const layerWithComparison = ( + comparison: Effect.Effect<{ + readonly behindBy: number | null; + readonly viewerCanUpdate: boolean; + }>, + ) => + Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + getPullRequestDetail: () => Effect.succeed(openDetail), + getPullRequestBaseComparison: () => comparison, + getViewerAccess: () => Effect.succeed({ canWrite: true, canUpdate: true, didAuthor: false }), + }); + + it.effect("offers update-branch when the comparison grants it", () => + Effect.gen(function* () { + const provider = yield* make; + const permissions = yield* provider.getViewerPermissions({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + }); + + expect(permissions.actions).toContain("update-branch"); + expect(permissions.updateMethods).toEqual(["merge", "rebase"]); + }).pipe( + Effect.provide(layerWithComparison(Effect.succeed({ behindBy: 3, viewerCanUpdate: true }))), + ), + ); + + it.effect("withholds update-branch when the comparison cannot be read", () => + Effect.gen(function* () { + const provider = yield* make; + const permissions = yield* provider.getViewerPermissions({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + }); + + expect(permissions.actions).not.toContain("update-branch"); + expect(permissions.updateMethods).toBeUndefined(); + // The rest of the answer survives a comparison nobody could make. + expect(permissions.actions).toContain("merge"); + }).pipe( + Effect.provide( + Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + getPullRequestDetail: () => Effect.succeed(openDetail), + getPullRequestBaseComparison: () => + Effect.fail( + new GitHubPullRequestCli.GitHubPullRequestReadError({ + command: "gh", + cwd: "/w", + operation: "getPullRequestBaseComparison", + cause: new Error("unreadable"), + }), + ), + getViewerAccess: () => + Effect.succeed({ canWrite: true, canUpdate: true, didAuthor: false }), + }), + ), + ), + ); +}); + describe("getChangeRequest commits", () => { const baseDetail = { authorId: null, @@ -122,6 +220,7 @@ describe("getChangeRequest commits", () => { updatedAt: "2026-07-02T00:00:00Z", reviewRequestLogins: [], hasTeamReviewRequest: false, + checksState: null, labels: [], body: "", changedFiles: 1, @@ -133,6 +232,7 @@ describe("getChangeRequest commits", () => { const baseThreadComments = { comments: [], + dismissalsByReviewId: new Map(), reviewThreads: [], commentCount: 0, truncated: false, @@ -200,6 +300,66 @@ describe("getChangeRequest commits", () => { ); }); +describe("getChangeRequestActivity dismissed reviews", () => { + const dismissedReview = (body: string) => ({ + id: "PRR_1", + kind: "review" as const, + author: null, + body, + createdAt: "2026-07-03T00:00:00Z", + url: null, + path: null, + reviewState: "DISMISSED", + }); + const threadComments: GitHubReviewThreadComments = { + comments: [], + dismissalsByReviewId: new Map([["PRR_1", "Dismissing prior approval to re-evaluate 9b66581"]]), + reviewThreads: [], + commentCount: 0, + truncated: false, + reviewers: [], + avatarsByLogin: new Map(), + commitStats: new Map(), + commits: [], + viewer: { canUpdate: true, didAuthor: false }, + }; + const layerFor = (body: string) => + Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + getPullRequestActivity: () => + Effect.succeed({ author: null, comments: [dismissedReview(body)], commits: [] }), + listReviewThreadComments: () => Effect.succeed(threadComments), + }); + const readActivity = Effect.gen(function* () { + const provider = yield* make; + return yield* provider.getChangeRequestActivity({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + }); + }); + + it.effect("fills a marker-only dismissed review with the timeline's reason", () => + // Macroscope's approvals carry only an HTML comment, which markdown renders as nothing — + // an empty-string check misses them and the card opens onto nothing. + readActivity.pipe( + Effect.map((activity) => { + expect(activity.comments[0]?.body).toBe("Dismissing prior approval to re-evaluate 9b66581"); + }), + Effect.provide(layerFor("")), + ), + ); + + it.effect("keeps the words of a dismissed review that has its own", () => + readActivity.pipe( + Effect.map((activity) => { + expect(activity.comments[0]?.body).toBe("These findings still stand."); + }), + Effect.provide(layerFor("These findings still stand.")), + ), + ); +}); + describe("loginAvatarUrl", () => { it("serves a user's picture from the host they belong to", () => { expect(loginAvatarUrl("octocat", "github.com")).toBe("https://github.com/octocat.png?size=80"); diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts index b77c20a541c..a758c4d4c15 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts @@ -17,8 +17,9 @@ import type { GitHubViewerAccess } from "./gitHubPullRequestJson.ts"; const CAPABILITIES: PullRequestCapabilities = { diff: true, comment: true, - actions: ["merge", "ready", "draft", "close", "reopen"], + actions: ["merge", "ready", "draft", "close", "reopen", "update-branch"], mergeMethods: ["merge", "squash", "rebase"], + updateMethods: ["merge", "rebase"], search: true, review: { inlineComment: true, @@ -51,6 +52,9 @@ export function gitHubViewerPermissions(access: GitHubViewerAccess): PullRequest actions: [ ...(access.canWrite ? (["merge"] as const) : []), ...(access.canUpdate ? (["ready", "draft", "close", "reopen"] as const) : []), + // Whether this viewer may update the branch is GitHub's own answer, read with the + // comparison; without it the action is offered to nobody rather than to everybody. + ...(access.canUpdateBranch === true ? (["update-branch"] as const) : []), ], comment: true, resolve: access.canWrite || access.didAuthor, @@ -59,6 +63,7 @@ export function gitHubViewerPermissions(access: GitHubViewerAccess): PullRequest // leaves them commenting, which is what an author has to say about their own change anyway. verdicts: access.didAuthor ? (["comment"] as const) : CAPABILITIES.review.verdicts, requestReviewers: access.canWrite, + ...(access.canUpdateBranch === true ? { updateMethods: CAPABILITIES.updateMethods } : {}), }; } @@ -98,6 +103,10 @@ export function loginAvatarUrl(login: string, host: string): string | null { return /^[a-z0-9][a-z0-9-]{0,38}$/iu.test(login) ? `https://${host}/${login}.png?size=80` : null; } +/** True where markdown would render nothing: whitespace, or only HTML comments. */ +const rendersEmpty = (body: string): boolean => + body.replace(//g, "").trim().length === 0; + export const make = Effect.gen(function* () { const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; @@ -129,6 +138,7 @@ export const make = Effect.gen(function* () { limit: input.limit, query: input.query, cursor: input.cursor, + filters: input.filters, }) .pipe( Effect.mapError(fail("listChangeRequests")), @@ -172,6 +182,7 @@ export const make = Effect.gen(function* () { limit: input.limit, query: input.query, cursor: input.cursor, + filters: input.filters, }) .pipe( Effect.mapError(fail("listChangeRequestsAcross")), @@ -196,7 +207,24 @@ export const make = Effect.gen(function* () { getChangeRequest: (input) => Effect.all( [ - cli.getPullRequestDetail(input), + cli.getPullRequestDetail(input).pipe( + Effect.flatMap((pullRequest) => + // Only an open pull request can be behind anything worth saying so about, and only + // one whose head repository is known can be compared at all. A comparison that + // fails is left unknown: the banner is an offer, never a blocker. + pullRequest.state !== "open" || pullRequest.headRepositoryOwner === null + ? Effect.succeed({ pullRequest, comparison: null }) + : cli + .getPullRequestBaseComparison({ + ...input, + headRef: `${pullRequest.headRepositoryOwner}:${pullRequest.headBranch}`, + }) + .pipe( + Effect.map((comparison) => ({ pullRequest, comparison })), + Effect.orElseSucceed(() => ({ pullRequest, comparison: null })), + ), + ), + ), cli.getRepositoryAccess({ cwd: input.cwd, repository: input.repository, @@ -210,15 +238,27 @@ export const make = Effect.gen(function* () { ).pipe( Effect.mapError(fail("getChangeRequest")), Effect.map( - ([pullRequest, repository, viewerAccess]): ProviderChangeRequestDetail => ({ - ...pullRequest, - reviewers: pullRequest.reviewRequestLogins.map((login) => ({ + ([detail, repository, viewerAccess]): ProviderChangeRequestDetail => ({ + ...detail.pullRequest, + reviewers: detail.pullRequest.reviewRequestLogins.map((login) => ({ login, name: null, avatarUrl: null, })), mergeCapabilities: repository.mergeCapabilities, - viewerPermissions: gitHubViewerPermissions(viewerAccess), + viewerPermissions: gitHubViewerPermissions({ + ...viewerAccess, + canUpdateBranch: detail.comparison?.viewerCanUpdate === true, + }), + baseComparison: + detail.comparison === null || detail.comparison.behindBy === null + ? "unknown" + : detail.comparison.behindBy > 0 + ? "behind" + : "up-to-date", + ...(detail.comparison?.behindBy == null + ? {} + : { behindBy: detail.comparison.behindBy }), }), ), ), @@ -232,6 +272,7 @@ export const make = Effect.gen(function* () { cli.listReviewThreadComments(input).pipe( Effect.orElseSucceed(() => ({ comments: [], + dismissalsByReviewId: new Map(), reviewThreads: [], commentCount: 0, truncated: true, @@ -266,6 +307,16 @@ export const make = Effect.gen(function* () { comments: [...pullRequest.comments, ...reviewThreads.comments] .map((comment) => ({ ...comment, + // GitHub keeps the dismissal reason on the timeline event, not on the review, + // so a dismissed review with nothing visible of its own reads its words from + // there. "Visible" and not "empty": bot reviews often carry only an HTML + // marker comment, which markdown renders as nothing. + body: + comment.kind === "review" && + comment.reviewState?.toUpperCase() === "DISMISSED" && + rendersEmpty(comment.body) + ? (reviewThreads.dismissalsByReviewId.get(comment.id) ?? comment.body) + : comment.body, author: withAvatar(comment.author, reviewThreads.avatarsByLogin, input.host), })) .toSorted((left, right) => left.createdAt.localeCompare(right.createdAt)), @@ -285,9 +336,34 @@ export const make = Effect.gen(function* () { ), getViewerPermissions: (input) => - cli - .getViewerAccess(input) - .pipe(Effect.mapError(fail("getViewerPermissions")), Effect.map(gitHubViewerPermissions)), + Effect.all( + [ + cli.getViewerAccess(input), + // Whether this viewer may update the branch is only on the comparison, and the + // comparison only resolves through the head ref the detail carries. A failure here + // withholds that one action rather than the whole answer, the way the detail path + // leaves the banner unknown. + cli.getPullRequestDetail(input).pipe( + Effect.flatMap((pullRequest) => + pullRequest.state !== "open" || pullRequest.headRepositoryOwner === null + ? Effect.succeed(false) + : cli + .getPullRequestBaseComparison({ + ...input, + headRef: `${pullRequest.headRepositoryOwner}:${pullRequest.headBranch}`, + }) + .pipe(Effect.map((comparison) => comparison.viewerCanUpdate === true)), + ), + Effect.orElseSucceed(() => false), + ), + ], + { concurrency: 2 }, + ).pipe( + Effect.mapError(fail("getViewerPermissions")), + Effect.map(([access, canUpdateBranch]) => + gitHubViewerPermissions({ ...access, canUpdateBranch }), + ), + ), getDiff: (input) => cli.getPullRequestDiff(input).pipe(Effect.mapError(fail("getDiff"))), @@ -318,6 +394,7 @@ export const make = Effect.gen(function* () { number: input.number, action: input.action, ...(input.mergeMethod === undefined ? {} : { mergeMethod: input.mergeMethod }), + ...(input.updateMethod === undefined ? {} : { updateMethod: input.updateMethod }), }) .pipe(Effect.mapError(fail("runAction"))), diff --git a/apps/server/src/pullRequest/GitLabPullRequestCli.ts b/apps/server/src/pullRequest/GitLabPullRequestCli.ts index 4cbb74d32a6..c73b619a733 100644 --- a/apps/server/src/pullRequest/GitLabPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitLabPullRequestCli.ts @@ -418,6 +418,9 @@ function actionArgs( return ["update", "--draft"]; case "close": return ["close"]; + // Never reached: this host does not declare the action, so nothing offers it. + case "update-branch": + return []; case "reopen": return ["reopen"]; } diff --git a/apps/server/src/pullRequest/PullRequestProvider.ts b/apps/server/src/pullRequest/PullRequestProvider.ts index 34ec28b4106..a94351c249e 100644 --- a/apps/server/src/pullRequest/PullRequestProvider.ts +++ b/apps/server/src/pullRequest/PullRequestProvider.ts @@ -3,22 +3,28 @@ import * as Schema from "effect/Schema"; import type { PullRequestAction, PullRequestActor, + PullRequestBaseComparison, PullRequestCapabilities, + PullRequestChecksState, PullRequestCheck, PullRequestComment, PullRequestCommit, PullRequestInvolvement, PullRequestLabel, + PullRequestListFilters, PullRequestListState, PullRequestMergeCapabilities, PullRequestMergeMethod, PullRequestMergeability, + PullRequestOmittedFileStat, PullRequestReviewCommentDraft, + PullRequestReviewDecision, PullRequestReviewThread, PullRequestReviewVerdict, PullRequestReviewerCandidateList, PullRequestReviewerKind, PullRequestState, + PullRequestUpdateMethod, PullRequestViewerPermissions, SourceControlProviderKind, } from "@t3tools/contracts"; @@ -64,6 +70,10 @@ export interface ProviderChangeRequest { /** Accounts with a review requested. Team-level requests are excluded by each provider. */ readonly reviewRequestLogins: ReadonlyArray; readonly labels: ReadonlyArray; + /** Absent from a host that does not summarise its reviews, which is every host but GitHub. */ + readonly reviewDecision?: PullRequestReviewDecision | null | undefined; + /** Absent from a host that reports no check rollup on its listings. */ + readonly checksState?: PullRequestChecksState | null | undefined; } export interface ProviderChangeRequestPage { @@ -140,6 +150,9 @@ export interface ProviderChangeRequestDetail extends ProviderChangeRequest { readonly checks: ReadonlyArray; readonly mergeCapabilities: PullRequestMergeCapabilities; readonly viewerPermissions: PullRequestViewerPermissions; + /** Absent from a host that cannot compare the branch with its base, which is most of them. */ + readonly baseComparison?: PullRequestBaseComparison; + readonly behindBy?: number; } /** The conversation-shaped half of a detail, loaded after the core can already render. */ @@ -165,6 +178,8 @@ export interface ProviderDiffSlice { /** Something in this slice could not be shown, as opposed to there being more slices. */ readonly truncated: boolean; readonly nextCursor: string | null; + /** The host's own counts for the files whose hunks it withheld from this slice. */ + readonly omittedFileStats?: ReadonlyArray; } export interface ProviderDiffFileContents { @@ -216,6 +231,12 @@ export interface PullRequestProviderApi { * asks for the first slice, which is every listing that has not been continued. */ readonly cursor?: ProviderListCursor | undefined; + /** + * Further narrowings, which a host applies as far as it can and ignores the rest of — + * an unnarrowed page is a wider answer rather than a wrong one, and the caller narrows + * what it gets for the fields a row carries. + */ + readonly filters?: PullRequestListFilters | undefined; }, ) => Effect.Effect; @@ -244,6 +265,7 @@ export interface PullRequestProviderApi { readonly limit: number; readonly query?: string | undefined; readonly cursor?: ProviderListCursor | undefined; + readonly filters?: PullRequestListFilters | undefined; }) => Effect.Effect; /** @@ -315,6 +337,8 @@ export interface PullRequestProviderApi { readonly number: number; readonly action: PullRequestAction; readonly mergeMethod?: PullRequestMergeMethod; + /** Only meaningful for `update-branch`; absent takes the host's own default. */ + readonly updateMethod?: PullRequestUpdateMethod; }, ) => Effect.Effect; diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index c46808aa2d8..1cc39ef3da4 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -325,8 +325,9 @@ it.effect("uses a provider's raw cursor advance when it consumed malformed rows" const result = yield* service.list({ state: "open" }); + // Keyed by the selector Azure is actually asked with, which is the repository's own name. assert.deepStrictEqual(result.nextCursors, { - "dev.azure.com acme/web": "2026-07-02T00:00:00Z|4|7", + "dev.azure.com web": "2026-07-02T00:00:00Z|4|7", }); }), ); @@ -2260,3 +2261,222 @@ it.effect( assert.strictEqual(activityCalls, 2); }), ); + +it("names an Azure DevOps repository by its own name, not its project path", () => { + // `az repos pr list --repository` takes a name and detects the organisation and project from + // the checkout; the recorded `org/project/_git/repo` path is refused, and the repository then + // reads as unavailable on the page. + const selector = PullRequestService.repositoryIdentityOf({ + repositoryIdentity: { + provider: "azure-devops", + displayName: "contoso/payments/_git/checkout", + owner: "contoso", + name: "checkout", + }, + } as never); + assert.strictEqual(selector, "checkout"); +}); + +it("falls back to the path's last segment where an Azure identity has no name", () => { + const selector = PullRequestService.repositoryIdentityOf({ + repositoryIdentity: { + provider: "azure-devops", + displayName: "contoso/payments/_git/checkout", + }, + } as never); + assert.strictEqual(selector, "checkout"); +}); + +it("keeps a GitLab identity's whole path, because a nested group is part of the name", () => { + const selector = PullRequestService.repositoryIdentityOf({ + repositoryIdentity: { + provider: "gitlab", + displayName: "group/subgroup/service", + owner: "group", + name: "service", + }, + } as never); + assert.strictEqual(selector, "group/subgroup/service"); +}); + +it.effect("narrows the rows of a host that ignored the filters it was handed", () => + Effect.gen(function* () { + const service = yield* makeService({ + projects: [ + project({ + id: "p1", + title: "web", + workspaceRoot: "/a", + repository: "acme/web", + provider: "gitlab", + }), + ], + providers: [ + // Only GitHub narrows a listing for itself; every other host answers unnarrowed, and + // sending it a draft filter it quietly ignores used to put drafts on a filtered page. + fakeProvider("gitlab", { + listChangeRequests: () => + Effect.succeed({ + items: [ + { ...changeRequest(1, "2026-07-02T00:00:00Z"), isDraft: true }, + changeRequest(2, "2026-07-01T00:00:00Z"), + ], + truncated: false, + continues: false, + }), + }), + ], + }); + + const result = yield* service.list({ state: "open", filters: { draft: "hide" } }); + + assert.deepStrictEqual( + result.entries.map((entry) => entry.number), + [2], + ); + }), +); + +it.effect("keeps a row of a host that ignored the filters if any name of a label group holds", () => + Effect.gen(function* () { + const sized = (number: number, updatedAt: string, ...names: ReadonlyArray) => ({ + ...changeRequest(number, updatedAt), + labels: names.map((name) => ({ name, color: null })), + }); + const service = yield* makeService({ + projects: [ + project({ + id: "p1", + title: "web", + workspaceRoot: "/a", + repository: "acme/web", + provider: "gitlab", + }), + ], + providers: [ + fakeProvider("gitlab", { + listChangeRequests: () => + Effect.succeed({ + items: [ + sized(1, "2026-07-04T00:00:00Z", "size:S", "bug"), + sized(2, "2026-07-03T00:00:00Z", "size:XS", "bug"), + sized(3, "2026-07-02T00:00:00Z", "size:L", "bug"), + sized(4, "2026-07-01T00:00:00Z", "size:S"), + ], + truncated: false, + continues: false, + }), + }), + ], + }); + + // Either size satisfies the first group; the second group is its own question, so the row + // carrying a size but no bug goes. + const result = yield* service.list({ + state: "open", + filters: { labels: [["size:S", "size:XS"], ["bug"]] }, + }); + + assert.deepStrictEqual( + result.entries.map((entry) => entry.number), + [1, 2], + ); + }), +); + +it.effect("refuses a way of updating a branch that the host or the viewer does not allow", () => + Effect.gen(function* () { + let taken: string | null = null; + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + capabilities: { + diff: true, + comment: true, + actions: ["merge", "close", "update-branch"], + mergeMethods: ["merge"], + // This host brings a stale branch up to date with a merge commit and nothing else. + updateMethods: ["merge"], + search: true, + review: FULL_REVIEW, + reviewers: FULL_REVIEWERS, + }, + getViewerPermissions: () => + Effect.succeed({ + actions: ["close", "update-branch"], + comment: true, + resolve: true, + verdicts: ["comment"], + requestReviewers: false, + updateMethods: ["merge"], + }), + runAction: (input) => { + taken = input.updateMethod ?? "default"; + return Effect.void; + }, + }), + ], + }); + const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + + // Asking for a rebase a host does not offer must fail rather than quietly merge instead. + const error = yield* Effect.flip( + service.runAction({ ...reference, action: "update-branch", updateMethod: "rebase" }), + ); + assert.strictEqual(error._tag, "PullRequestOperationError"); + assert.strictEqual(taken, null); + + yield* service.runAction({ ...reference, action: "update-branch", updateMethod: "merge" }); + assert.strictEqual(taken, "merge"); + }), +); + +it.effect("judges the review filter only on a host that summarises its reviews", () => + Effect.gen(function* () { + const service = yield* makeService({ + projects: [ + project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" }), + project({ + id: "p2", + title: "on gitlab", + workspaceRoot: "/b", + repository: "group/project", + provider: "gitlab", + }), + ], + providers: [ + // GitHub answers with the field on every row: null is "nobody has decided yet". + fakeProvider("github", { + listChangeRequests: () => + Effect.succeed({ + items: [ + { ...changeRequest(1, "2026-07-02T00:00:00Z"), reviewDecision: null }, + { + ...changeRequest(2, "2026-07-02T00:00:00Z"), + reviewDecision: "approved" as const, + }, + ], + truncated: false, + continues: true, + }), + }), + // GitLab never supplies the field, so its rows are not the filter's to judge. + fakeProvider("gitlab", { + listChangeRequests: () => + Effect.succeed({ + items: [changeRequest(3, "2026-07-02T00:00:00Z")], + truncated: false, + continues: true, + }), + }), + ], + }); + + const none = yield* service.list({ state: "open", filters: { review: "none" } }); + assert.deepStrictEqual(none.entries.map((entry) => entry.number).toSorted(), [1, 3]); + + const approved = yield* service.list({ state: "open", filters: { review: "approved" } }); + assert.deepStrictEqual(approved.entries.map((entry) => entry.number).toSorted(), [2, 3]); + }), +); diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 8652e4b9c9f..0c6fc6a23ca 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -23,6 +23,7 @@ import { type PullRequestDiffResult, type PullRequestInvalidateInput, type PullRequestListEntry, + type PullRequestListFilters, type PullRequestListInput, type PullRequestListProjectError, type PullRequestListResult, @@ -171,6 +172,8 @@ const ACTION_ACCESS_REFUSALS: Record = { "You need write access on this repository, or to have opened this change request, to return it to a draft.", close: "You need write access on this repository, or to have opened this change request, to close it.", + "update-branch": + "You need write access on this repository, or to have opened this change request, to update its branch.", reopen: "You need write access on this repository, or to have opened this change request, to reopen it.", }; @@ -346,13 +349,25 @@ function toPullRequestError( } /** - * The provider-native repository identity. `displayName` is the full path below the host, which - * is what nested GitLab groups and Azure project paths need; owner/name is the two-segment - * fallback for identities recorded before that field existed. + * The provider-native repository selector. `displayName` is the full path below the host, which + * is what nested GitLab groups need; owner/name is the two-segment fallback for identities + * recorded before that field existed. + * + * Azure DevOps is the exception: `az repos pr list --repository` takes a repository name, and + * takes the organisation and project from the checkout it detects — so the recorded + * `org/project/_git/repo` path is refused outright and the whole repository reads as + * unavailable. Its name is the last segment, which is what this hands over. + * + * One function because everything downstream is keyed by what it answers: the rows' own + * `repository`, the per-repository cursors, and the detail and diff reads a row leads to. */ -function repositoryIdentityOf(project: OrchestrationProjectShell): string | null { +export function repositoryIdentityOf(project: OrchestrationProjectShell): string | null { const identity = project.repositoryIdentity; if (!identity) return null; + if (identity.provider === "azure-devops") { + const segments = (identity.displayName ?? "").split("/").filter((part) => part !== "_git"); + return identity.name || segments.at(-1) || null; + } if (identity.displayName) return identity.displayName; return identity.owner && identity.name ? `${identity.owner}/${identity.name}` : null; } @@ -362,7 +377,7 @@ export const make = Effect.gen(function* () { const projections = yield* ProjectionSnapshotQuery.ProjectionSnapshotQuery; const listWorkspaceProjects = ( - filter: Pick, + filter: Pick, ): Effect.Effect => projections.getShellSnapshot().pipe( Effect.mapError( @@ -383,6 +398,7 @@ export const make = Effect.gen(function* () { const seen = new Set(); for (const project of snapshot.projects) { if (filter.projectId !== undefined && project.id !== filter.projectId) continue; + if (filter.projectIds !== undefined && !filter.projectIds.includes(project.id)) continue; const kind = project.repositoryIdentity?.provider as | SourceControlProviderKind | undefined; @@ -533,6 +549,41 @@ export const make = Effect.gen(function* () { { concurrency: REPOSITORY_CONCURRENCY }, ); + /** + * The narrowings a row can be judged by from its own fields, applied here rather than trusted + * to the host. Only GitHub is asked to narrow a listing for itself; every other provider + * answers unnarrowed, and without this pass a draft filter or a label filter would be sent, + * accepted and quietly ignored. Idempotent for the hosts that did narrow. + * + * `checks` is absent because no listed row carries its check state: that one filter is the + * host's alone, and a row nobody narrowed stays rather than being guessed at. + */ + const matchesRowFilters = ( + item: ProviderChangeRequest, + filters: PullRequestListFilters | undefined, + ): boolean => { + if (filters === undefined) return true; + const labels = item.labels.map((label) => label.name.trim().toLowerCase()); + const holds = (label: string) => labels.includes(label.trim().toLowerCase()); + return ( + (filters.draft === undefined || item.isDraft === (filters.draft === "only")) && + // Judged on the provider row rather than the entry, because the two absences mean + // different things and the entry keeps only one of them: `null` is a host that summarises + // its reviews saying there is no decision yet, which is what "none" asks for, while + // `undefined` is a host that does not summarise at all — an unjudgeable row, left alone + // the way an unreadable check state is. + (filters.review === undefined || + item.reviewDecision === undefined || + (filters.review === "none" + ? item.reviewDecision === null + : item.reviewDecision === filters.review)) && + (filters.labels === undefined || filters.labels.every((group) => group.some(holds))) && + (filters.excludedLabels === undefined || !filters.excludedLabels.some(holds)) && + (filters.author === undefined || + item.author?.login.toLowerCase() === filters.author.trim().toLowerCase()) + ); + }; + const toEntry = (input: { readonly project: SupportedProject; readonly item: ProviderChangeRequest; @@ -558,10 +609,16 @@ export const make = Effect.gen(function* () { deletions: input.item.deletions, createdAt: input.item.createdAt, updatedAt: input.item.updatedAt, + ...(input.item.checksState === undefined || input.item.checksState === null + ? {} + : { checksState: input.item.checksState }), viewerReviewRequested: input.item.author?.login.toLowerCase() !== viewer && input.item.reviewRequestLogins.some((login) => login.toLowerCase() === viewer), labels: input.item.labels, + ...(input.item.reviewDecision === undefined || input.item.reviewDecision === null + ? {} + : { reviewDecision: input.item.reviewDecision }), }; }; @@ -685,6 +742,7 @@ export const make = Effect.gen(function* () { // Each host matches this its own way, and one that cannot match text at all // answers unnarrowed rather than failing. query: input.query, + filters: input.filters, // Only the two fields a host can act on: which rows have already been sent at the // boundary instant is this service's business, not a provider's. ...(cursor === undefined @@ -708,7 +766,9 @@ export const make = Effect.gen(function* () { ); return { key, - entries: items.map((item) => toEntry({ project, item, viewer })), + entries: items + .filter((item) => matchesRowFilters(item, input.filters)) + .map((item) => toEntry({ project, item, viewer })), errors: [], truncated: page.truncated, nextCursor: @@ -766,6 +826,7 @@ export const make = Effect.gen(function* () { viewer, limit, query: input.query, + filters: input.filters, ...(cursor === undefined ? {} : { cursor: { updatedBefore: cursor.updatedBefore, delivered: cursor.delivered } }), @@ -811,7 +872,9 @@ export const make = Effect.gen(function* () { ); return Effect.succeed({ key: listCursorKey(project.host, project.repository), - entries: items.map((item) => toEntry({ project, item, viewer })), + entries: items + .filter((item) => matchesRowFilters(item, input.filters)) + .map((item) => toEntry({ project, item, viewer })), errors: [], truncated: page.truncated, nextCursor: @@ -911,6 +974,12 @@ export const make = Effect.gen(function* () { checks: changeRequest.checks, mergeCapabilities: changeRequest.mergeCapabilities, viewerPermissions: changeRequest.viewerPermissions, + ...(changeRequest.baseComparison === undefined + ? {} + : { baseComparison: changeRequest.baseComparison }), + ...(changeRequest.behindBy === undefined + ? {} + : { behindBy: changeRequest.behindBy }), }), ), ), @@ -1018,6 +1087,19 @@ export const make = Effect.gen(function* () { }), ); } + // The same for the way a stale branch is brought up to date: a host that only merges + // must not be asked to rebase and left to pick something else. + if ( + input.updateMethod !== undefined && + !(project.api.capabilities.updateMethods ?? []).includes(input.updateMethod) + ) { + return Effect.fail( + new PullRequestOperationError({ + operation: "runAction", + detail: `This host cannot update a branch by ${input.updateMethod}.`, + }), + ); + } // What the host can do and what this account may ask of it are two questions, and both // have to say yes. The second is asked last, because it costs a request and the checks // above do not. @@ -1031,6 +1113,17 @@ export const make = Effect.gen(function* () { }), ); } + if ( + input.updateMethod !== undefined && + !(viewer.updateMethods ?? []).includes(input.updateMethod) + ) { + return Effect.fail( + new PullRequestOperationError({ + operation: "runAction", + detail: ACTION_ACCESS_REFUSALS["update-branch"], + }), + ); + } return project.api .runAction({ cwd: project.project.workspaceRoot, @@ -1039,6 +1132,7 @@ export const make = Effect.gen(function* () { number: input.number, action: input.action, ...(input.mergeMethod === undefined ? {} : { mergeMethod: input.mergeMethod }), + ...(input.updateMethod === undefined ? {} : { updateMethod: input.updateMethod }), }) .pipe(Effect.mapError(toPullRequestError("runAction"))); }), @@ -1450,6 +1544,23 @@ export const make = Effect.gen(function* () { refEpochs.set(scope, ++epochCounter); }; + /** The positional filter slot of a cache key, back as the record `listUncached` takes. */ + const filtersOfKey = ( + slots: ReadonlyArray< + string | ReadonlyArray | ReadonlyArray> | null + >, + ): PullRequestListFilters => { + const [draft, review, checks, author, labels, excludedLabels] = slots; + return { + ...(typeof draft === "string" ? { draft: draft as "only" | "hide" } : {}), + ...(typeof review === "string" ? { review: review as PullRequestListFilters["review"] } : {}), + ...(typeof checks === "string" ? { checks: checks as PullRequestListFilters["checks"] } : {}), + ...(typeof author === "string" ? { author } : {}), + ...(Array.isArray(labels) ? { labels: labels as ReadonlyArray> } : {}), + ...(Array.isArray(excludedLabels) ? { excludedLabels } : {}), + }; + }; + // Keys serialize positionally and parse back in the lookup, so the cache is the only holder // of in-flight state: concurrent identical reads coalesce on the key into one host request. // The continuation cursors are part of the key, entries sorted so one continuation is one @@ -1458,21 +1569,22 @@ export const make = Effect.gen(function* () { (key: string) => { // The parse undoes this module's own serialization, so the shapes are known exactly; // the cast restores the branded field types JSON cannot carry. - const [, state, involvement, projectId, host, limit, query, cursorEntries] = JSON.parse( - key, - ) as [ - number, - string, - string | null, - string | null, - string | null, - number | null, - string | null, - ReadonlyArray<[string, string]> | null, - ]; + const [, state, involvement, filters, projectId, host, limit, query, cursorEntries] = + JSON.parse(key) as [ + number, + string, + string | null, + ReadonlyArray | null> | null, + string | null, + string | null, + number | null, + string | null, + ReadonlyArray<[string, string]> | null, + ]; return listUncached({ state, ...(involvement === null ? {} : { involvement }), + ...(filters === null ? {} : { filters: filtersOfKey(filters) }), ...(projectId === null ? {} : { projectId }), ...(host === null ? {} : { host }), ...(limit === null ? {} : { limit }), @@ -1494,6 +1606,17 @@ export const make = Effect.gen(function* () { listingsEpoch, input.state, input.involvement ?? null, + // Positional so two identical filter sets key alike however their record was assembled. + input.filters === undefined + ? null + : [ + input.filters.draft ?? null, + input.filters.review ?? null, + input.filters.checks ?? null, + input.filters.author ?? null, + input.filters.labels ?? null, + input.filters.excludedLabels ?? null, + ], input.projectId ?? null, input.host ?? null, input.limit ?? null, diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts index d3d9945da3a..baf77208576 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts @@ -4,10 +4,12 @@ import { describe, expect, it } from "vite-plus/test"; import { buildReviewSubmissionJson, buildReviewerRequestJson, + decodeBaseComparisonJson, decodePullRequestActivityJson, decodePullRequestDetailJson, decodePullRequestFilesJson, decodePullRequestListJson, + decodePullRequestSearchJson, decodeRepositoryAccessJson, decodeReviewerCandidatesJson, decodeReviewThreadCommentsJson, @@ -67,6 +69,65 @@ describe("pull request list decoding", () => { expect(entry?.reviewRequestLogins).toEqual(["octocat"]); }); + it("normalizes the review decision and reports nothing for one GitHub does not summarize", () => { + const batch = expectSuccess( + decodePullRequestListJson( + listJson([ + { reviewDecision: "APPROVED" }, + { reviewDecision: "CHANGES_REQUESTED" }, + { reviewDecision: "REVIEW_REQUIRED" }, + { reviewDecision: null }, + ]), + ), + ); + expect(batch.items.map((entry) => entry.reviewDecision)).toEqual([ + "approved", + "changes-requested", + "review-required", + null, + ]); + }); + + it("rolls the head commit's checks up to the one word a row has space for", () => { + const batch = expectSuccess( + decodePullRequestListJson( + listJson([ + // A failure outranks a run still going, and a completed run has to be read through its + // conclusion rather than its status. + { + statusCheckRollup: [ + { name: "lint", status: "COMPLETED", conclusion: "SUCCESS" }, + { name: "build", status: "IN_PROGRESS" }, + { name: "test", status: "COMPLETED", conclusion: "FAILURE" }, + ], + }, + { + statusCheckRollup: [ + { name: "lint", status: "COMPLETED", conclusion: "SUCCESS" }, + { name: "build", status: "QUEUED" }, + ], + }, + { statusCheckRollup: [{ name: "lint", status: "COMPLETED", conclusion: "SUCCESS" }] }, + // A commit status reports one `state` and no `status` at all. + { statusCheckRollup: [{ context: "ci/legacy", state: "ERROR" }] }, + // Neither a pass, a failure nor a wait is no verdict rather than a green tick. + { statusCheckRollup: [{ name: "lint", status: "COMPLETED", conclusion: "SKIPPED" }] }, + { statusCheckRollup: [] }, + {}, + ]), + ), + ); + expect(batch.items.map((entry) => entry.checksState)).toEqual([ + "failing", + "pending", + "passing", + "failing", + null, + null, + null, + ]); + }); + it("skips malformed entries but still counts them, so paging does not stop early", () => { const raw = `[${listJson([{}]).slice(1, -1)},{"number":"not-a-number"}]`; const batch = expectSuccess(decodePullRequestListJson(raw)); @@ -75,6 +136,49 @@ describe("pull request list decoding", () => { }); }); +describe("pull request search decoding", () => { + function searchJson(rollupStates: ReadonlyArray): string { + return JSON.stringify({ + data: { + search: { + pageInfo: { hasNextPage: false }, + nodes: rollupStates.map((state, index) => ({ + number: index + 1, + title: "Add the pull requests page", + url: "https://github.com/pingdotgg/t3code/pull/1", + headRefName: "feat/page", + baseRefName: "main", + createdAt: "2026-07-01T00:00:00Z", + updatedAt: "2026-07-02T00:00:00Z", + repository: { nameWithOwner: "pingdotgg/t3code" }, + commits: { + nodes: [{ commit: { statusCheckRollup: state === null ? null : { state } } }], + }, + })), + }, + }, + }); + } + + it("maps the rollup enum the search answers with onto the same three words", () => { + // The search asks GitHub for the verdict rather than the checks behind it, so this path sees + // one enum where the listing sees an array. + const batch = expectSuccess( + decodePullRequestSearchJson( + searchJson(["SUCCESS", "FAILURE", "ERROR", "PENDING", "EXPECTED", null]), + ), + ); + expect(batch.items.map((entry) => entry.checksState)).toEqual([ + "passing", + "failing", + "failing", + "pending", + "pending", + null, + ]); + }); +}); + describe("pull request detail decoding", () => { const detailJson = JSON.stringify({ number: 7, @@ -957,3 +1061,45 @@ describe("decodePullRequestFilesJson", () => { expect(result.truncated).toBe(false); }); }); + +describe("how far a branch trails its base", () => { + const comparison = (pullRequest: unknown) => + JSON.stringify({ data: { repository: { pullRequest } } }); + + it("reads the commit count and whether this viewer may move the branch", () => { + const decoded = expectSuccess( + decodeBaseComparisonJson( + comparison({ viewerCanUpdateBranch: true, baseRef: { compare: { behindBy: 12 } } }), + ), + ); + expect(decoded).toEqual({ behindBy: 12, viewerCanUpdate: true }); + }); + + it("reads a current branch as nothing to do", () => { + expect( + expectSuccess( + decodeBaseComparisonJson( + comparison({ viewerCanUpdateBranch: false, baseRef: { compare: { behindBy: 0 } } }), + ), + ), + ).toEqual({ behindBy: 0, viewerCanUpdate: false }); + }); + + it("answers unknown where the head could not be compared", () => { + // A pull request from a fork whose repository is gone, which GitHub answers with a null + // comparison beside a perfectly good pull request. + expect( + expectSuccess( + decodeBaseComparisonJson(comparison({ viewerCanUpdateBranch: true, baseRef: null })), + ).behindBy, + ).toBeNull(); + expect(expectSuccess(decodeBaseComparisonJson(comparison(null)))).toEqual({ + behindBy: null, + viewerCanUpdate: false, + }); + }); + + it("refuses a body that is not the answer to this question", () => { + expect(Result.isSuccess(decodeBaseComparisonJson("{"))).toBe(false); + }); +}); diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.ts index 8668d840ce1..a8c10d93291 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.ts @@ -6,12 +6,15 @@ import type { PullRequestActor, PullRequestCheck, PullRequestCheckStatus, + PullRequestChecksState, PullRequestComment, PullRequestCommit, PullRequestLabel, PullRequestMergeCapabilities, + PullRequestOmittedFileStat, PullRequestMergeability, PullRequestReviewCommentDraft, + PullRequestReviewDecision, PullRequestReviewThread, PullRequestReviewVerdict, PullRequestReviewerCandidate, @@ -51,6 +54,18 @@ const RawReviewRequestSchema = Schema.Struct({ name: Schema.optional(Schema.NullOr(Schema.String)), }); +const RawCheckSchema = Schema.Struct({ + __typename: Schema.optional(Schema.String), + name: Schema.optional(Schema.NullOr(Schema.String)), + context: Schema.optional(Schema.NullOr(Schema.String)), + status: Schema.optional(Schema.NullOr(Schema.String)), + conclusion: Schema.optional(Schema.NullOr(Schema.String)), + state: Schema.optional(Schema.NullOr(Schema.String)), + description: Schema.optional(Schema.NullOr(Schema.String)), + detailsUrl: Schema.optional(Schema.NullOr(Schema.String)), + targetUrl: Schema.optional(Schema.NullOr(Schema.String)), +}); + const RawListItemSchema = Schema.Struct({ number: Schema.Int, title: Schema.String, @@ -61,6 +76,7 @@ const RawListItemSchema = Schema.Struct({ state: Schema.optional(Schema.NullOr(Schema.String)), isDraft: Schema.optional(Schema.Boolean), mergeable: Schema.optional(Schema.NullOr(Schema.String)), + reviewDecision: Schema.optional(Schema.NullOr(Schema.String)), additions: Schema.optional(Schema.Int), deletions: Schema.optional(Schema.Int), createdAt: Schema.String, @@ -68,6 +84,14 @@ const RawListItemSchema = Schema.Struct({ mergedAt: Schema.optional(Schema.NullOr(Schema.String)), reviewRequests: Schema.optional(Schema.Array(RawReviewRequestSchema)), labels: Schema.optional(Schema.Array(RawLabelSchema)), + /** + * Every check of the head commit, which is the only rollup `gh pr list --json` can give: there + * is no field for the one-word verdict. Measured against `pingdotgg/t3code`, asking for it costs + * 0.6s -> 7.9s at a hundred rows and 0.9s -> 2.1s at thirty, for 425 KB of checks a listing + * reduces to one word. The listing pays it because the alternative is a request per row; the + * cross-repository search below asks GitHub for the verdict itself instead. + */ + statusCheckRollup: Schema.optional(Schema.NullOr(Schema.Array(RawCheckSchema))), }); /** @@ -85,6 +109,7 @@ const RawSearchItemSchema = Schema.Struct({ state: Schema.optional(Schema.NullOr(Schema.String)), isDraft: Schema.optional(Schema.Boolean), mergeable: Schema.optional(Schema.NullOr(Schema.String)), + reviewDecision: Schema.optional(Schema.NullOr(Schema.String)), createdAt: Schema.String, updatedAt: Schema.String, mergedAt: Schema.optional(Schema.NullOr(Schema.String)), @@ -113,6 +138,36 @@ const RawSearchItemSchema = Schema.Struct({ }), ), ), + /** + * GraphQL answers the rollup a listing actually wants — one enum for the head commit, rather + * than the whole check array `gh pr list --json` insists on. Measured at a hundred rows across + * this repository: 0.8s -> 3.0s and 15 KB, against 425 KB for the same verdict over `gh`. + */ + commits: Schema.optional( + Schema.NullOr( + Schema.Struct({ + nodes: Schema.optional( + Schema.NullOr( + Schema.Array( + Schema.NullOr( + Schema.Struct({ + commit: Schema.optional( + Schema.NullOr( + Schema.Struct({ + statusCheckRollup: Schema.optional( + Schema.NullOr(Schema.Struct({ state: Schema.String })), + ), + }), + ), + ), + }), + ), + ), + ), + ), + }), + ), + ), }); const RawSearchSchema = Schema.Struct({ @@ -149,18 +204,6 @@ const RawStatsSchema = Schema.Struct({ ), }); -const RawCheckSchema = Schema.Struct({ - __typename: Schema.optional(Schema.String), - name: Schema.optional(Schema.NullOr(Schema.String)), - context: Schema.optional(Schema.NullOr(Schema.String)), - status: Schema.optional(Schema.NullOr(Schema.String)), - conclusion: Schema.optional(Schema.NullOr(Schema.String)), - state: Schema.optional(Schema.NullOr(Schema.String)), - description: Schema.optional(Schema.NullOr(Schema.String)), - detailsUrl: Schema.optional(Schema.NullOr(Schema.String)), - targetUrl: Schema.optional(Schema.NullOr(Schema.String)), -}); - const RawCommentSchema = Schema.Struct({ id: Schema.String, author: Schema.optional(Schema.NullOr(RawActorSchema)), @@ -196,10 +239,11 @@ const RawCommitSchema = Schema.Struct({ const RawDetailSchema = Schema.Struct({ ...RawListItemSchema.fields, + /** Names the fork a pull request came from, which is what qualifies its head ref. */ + headRepositoryOwner: Schema.optional(Schema.NullOr(Schema.Struct({ login: Schema.String }))), body: Schema.optional(Schema.String), changedFiles: Schema.optional(Schema.Int), closedAt: Schema.optional(Schema.NullOr(Schema.String)), - statusCheckRollup: Schema.optional(Schema.NullOr(Schema.Array(RawCheckSchema))), }); const RawActivitySchema = Schema.Struct({ @@ -284,6 +328,23 @@ const RawReviewThreadsSchema = Schema.Struct({ }), ), ), + reviewDismissals: Schema.optional( + Schema.NullOr( + Schema.Struct({ + pageInfo: Schema.optional(RawPageInfoSchema), + nodes: Schema.Array( + Schema.Struct({ + dismissalMessage: Schema.optional(Schema.NullOr(Schema.String)), + review: Schema.optional( + Schema.NullOr( + Schema.Struct({ id: Schema.optional(Schema.NullOr(Schema.String)) }), + ), + ), + }), + ), + }), + ), + ), commits: Schema.optional( Schema.NullOr( Schema.Struct({ @@ -384,9 +445,9 @@ export function decodeActorAvatarsJson( } export const PULL_REQUEST_LIST_JSON_FIELDS = - "number,title,url,author,headRefName,baseRefName,state,isDraft,mergeable,additions,deletions,createdAt,updatedAt,mergedAt,reviewRequests,labels"; + "number,title,url,author,headRefName,baseRefName,state,isDraft,mergeable,reviewDecision,additions,deletions,createdAt,updatedAt,mergedAt,reviewRequests,labels,statusCheckRollup"; -export const PULL_REQUEST_DETAIL_JSON_FIELDS = `${PULL_REQUEST_LIST_JSON_FIELDS},body,changedFiles,closedAt,statusCheckRollup`; +export const PULL_REQUEST_DETAIL_JSON_FIELDS = `${PULL_REQUEST_LIST_JSON_FIELDS},body,changedFiles,closedAt,headRepositoryOwner`; export const PULL_REQUEST_ACTIVITY_JSON_FIELDS = "author,comments,reviews,commits"; /** GitHub's own ceiling on a connection page, which is what both thread reads ask for. */ @@ -429,12 +490,14 @@ export function pullRequestSearchGraphQlQuery(rows: number): string { state isDraft mergeable + reviewDecision createdAt updatedAt mergedAt repository { nameWithOwner } reviewRequests(first: 20) { nodes { requestedReviewer { ... on User { login } } } } labels(first: 20) { nodes { name color } } + commits(last: 1) { nodes { commit { statusCheckRollup { state } } } } } } } @@ -495,6 +558,10 @@ export const REVIEW_THREADS_GRAPHQL_QUERY = `query($owner: String!, $name: Strin latestReviews(first: 50) { nodes { author { login avatarUrl } } } + reviewDismissals: timelineItems(itemTypes: [REVIEW_DISMISSED_EVENT], first: ${GRAPHQL_PAGE_SIZE}) { + pageInfo { hasNextPage endCursor } + nodes { ... on ReviewDismissedEvent { dismissalMessage review { id } } } + } commits(last: ${GRAPHQL_PAGE_SIZE}) { nodes { commit { @@ -588,6 +655,21 @@ const REVIEW_EVENTS: Record; + /** Null where the head commit reported no checks, which is not the same as passing none. */ + readonly checksState: PullRequestChecksState | null; } export interface GitHubPullRequestDetail extends GitHubPullRequestListItem { + /** The owner of the head branch's repository; null where `gh` did not say. */ + readonly headRepositoryOwner: string | null; readonly body: string; readonly changedFiles: number; readonly mergedAt: string | null; @@ -731,6 +819,19 @@ function toMergeability(value: string | null | undefined): PullRequestMergeabili } } +function toReviewDecision(value: string | null | undefined): PullRequestReviewDecision | null { + switch (value?.trim().toUpperCase()) { + case "APPROVED": + return "approved"; + case "CHANGES_REQUESTED": + return "changes-requested"; + case "REVIEW_REQUIRED": + return "review-required"; + default: + return null; + } +} + function toLabels( raw: ReadonlyArray> | undefined, ): ReadonlyArray { @@ -792,6 +893,24 @@ function toCheckStatus(raw: Schema.Schema.Type): PullRequ } } +/** + * The one word a listing row has space for. A failure outranks anything still running, the way + * GitHub's own indicator reads: a run that has already gone red will not go green by finishing. + * + * Null rather than "passing" for a head commit with no checks at all, so a repository that runs + * none shows nothing instead of a green tick it never earned. Checks whose verdict is neither a + * pass, a failure nor a wait — skipped, cancelled, neutral — count towards neither. + */ +function rollupChecksState( + raw: ReadonlyArray> | null | undefined, +): PullRequestChecksState | null { + const statuses = (raw ?? []).map((check) => toCheckStatus(check)); + if (statuses.length === 0) return null; + if (statuses.includes("failure")) return "failing"; + if (statuses.includes("pending")) return "pending"; + return statuses.includes("success") ? "passing" : null; +} + function toChecks( raw: ReadonlyArray> | null | undefined, ): ReadonlyArray { @@ -894,6 +1013,7 @@ function toListItem(raw: Schema.Schema.Type): GitHubPu state: toState(raw), isDraft: raw.isDraft ?? false, mergeability: toMergeability(raw.mergeable), + reviewDecision: toReviewDecision(raw.reviewDecision), additions: raw.additions ?? 0, deletions: raw.deletions ?? 0, createdAt: raw.createdAt, @@ -901,12 +1021,14 @@ function toListItem(raw: Schema.Schema.Type): GitHubPu reviewRequestLogins: toReviewRequestLogins(raw.reviewRequests), hasTeamReviewRequest: hasTeamReviewRequest(raw.reviewRequests), labels: toLabels(raw.labels), + checksState: rollupChecksState(raw.statusCheckRollup), }; } function toDetail(raw: Schema.Schema.Type): GitHubPullRequestDetail { return { ...toListItem(raw), + headRepositoryOwner: trimmed(raw.headRepositoryOwner?.login), body: raw.body ?? "", changedFiles: raw.changedFiles ?? 0, mergedAt: trimmed(raw.mergedAt), @@ -1006,6 +1128,12 @@ export function decodePullRequestSearchJson( return login === null ? [] : [{ login }]; }), labels: (node.labels?.nodes ?? []).flatMap((label) => (label === null ? [] : [label])), + // The search asks for the verdict rather than the checks behind it, so it arrives as one + // enum. Dressed as a single check here so the rollup is read the same way on both paths. + statusCheckRollup: (node.commits?.nodes ?? []).flatMap((commitNode) => { + const state = trimmed(commitNode?.commit?.statusCheckRollup?.state); + return state === null ? [] : [{ state }]; + }), }), repository, }); @@ -1094,6 +1222,8 @@ export function decodePullRequestActivityJson( export interface GitHubReviewThreadComments { readonly comments: ReadonlyArray; + /** Dismissal reasons by the dismissed review's node id, read off the timeline. */ + readonly dismissalsByReviewId: ReadonlyMap; /** Whole conversations, kept anchored so the diff can pin them to their line. */ readonly reviewThreads: ReadonlyArray; /** The host's own count of the conversation, which a bounded read can fall short of. */ @@ -1148,6 +1278,10 @@ export interface GitHubReviewThreadPage { >; readonly commits: ReadonlyArray; readonly viewer: { readonly canUpdate: boolean; readonly didAuthor: boolean }; + /** Dismissal reasons by the dismissed review's node id, which the review itself never carries. */ + readonly dismissalsByReviewId: ReadonlyMap; + /** Where the rest of the dismissal events start, or null once this page carried them all. */ + readonly nextDismissalCursor: string | null; } /** @@ -1175,6 +1309,64 @@ export function reviewThreadConversation( } /** One page of review threads. Following the cursors it hands back is the caller's job. */ +function toDismissalEntries( + nodes: + | ReadonlyArray<{ + readonly dismissalMessage?: string | null | undefined; + readonly review?: { readonly id?: string | null | undefined } | null | undefined; + }> + | undefined, +): Map { + const entries = new Map(); + for (const node of nodes ?? []) { + const reviewId = trimmed(node.review?.id); + const message = trimmed(node.dismissalMessage); + if (reviewId !== null && message !== null) entries.set(reviewId, message); + } + return entries; +} + +const RawReviewDismissalsSchema = Schema.Struct({ + data: Schema.Struct({ + repository: Schema.Struct({ + pullRequest: Schema.Struct({ + timelineItems: Schema.Struct({ + pageInfo: Schema.optional(RawPageInfoSchema), + nodes: Schema.Array( + Schema.Struct({ + dismissalMessage: Schema.optional(Schema.NullOr(Schema.String)), + review: Schema.optional( + Schema.NullOr(Schema.Struct({ id: Schema.optional(Schema.NullOr(Schema.String)) })), + ), + }), + ), + }), + }), + }), + }), +}); + +const decodeReviewDismissals = decodeJsonResult(RawReviewDismissalsSchema); + +/** One further page of dismissal events, in the shape the thread read's own page carries. */ +export function decodeReviewDismissalsJson(raw: string): Result.Result< + { + readonly dismissalsByReviewId: ReadonlyMap; + readonly nextCursor: string | null; + }, + DecodeFailure +> { + const decoded = decodeReviewDismissals(raw); + if (!Result.isSuccess(decoded)) { + return Result.fail(decoded.failure); + } + const items = decoded.success.data.repository.pullRequest.timelineItems; + return Result.succeed({ + dismissalsByReviewId: toDismissalEntries(items.nodes), + nextCursor: nextCursorOf(items.pageInfo), + }); +} + export function decodeReviewThreadsJson( raw: string, ): Result.Result { @@ -1268,6 +1460,8 @@ export function decodeReviewThreadsJson( commitStats, commits, viewer: toPullRequestViewerFields(pullRequest), + dismissalsByReviewId: toDismissalEntries(pullRequest.reviewDismissals?.nodes), + nextDismissalCursor: nextCursorOf(pullRequest.reviewDismissals?.pageInfo), }); } @@ -1351,6 +1545,75 @@ export function decodeRepositoryAccessJson( * need `read:org`, which a repository-scoped token need not carry — and a query GitHub refuses * fails whole, taking the people down with the teams. */ +/** + * Where the branch stands against its base, and whether this viewer may move it. + * + * `mergeStateStatus` is not the answer: GitHub only reports BEHIND where the repository requires + * branches to be up to date before merging, so on every other repository a stale branch reads as + * CLEAN or BLOCKED like any other. The comparison counts the commits instead, which is the same + * number GitHub's own "out-of-date" banner shows. + * + * `headRef` is qualified `owner:branch` because a pull request from a fork has no branch of that + * name in the base repository, and an unqualified name is simply not found there. + */ +export const BASE_COMPARISON_GRAPHQL_QUERY = `query($owner: String!, $name: String!, $number: Int!, $headRef: String!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + viewerCanUpdateBranch + baseRef { + compare(headRef: $headRef) { + behindBy + } + } + } + } +}`; + +const RawBaseComparisonSchema = Schema.Struct({ + data: Schema.Struct({ + repository: Schema.NullOr( + Schema.Struct({ + pullRequest: Schema.NullOr( + Schema.Struct({ + viewerCanUpdateBranch: Schema.optional(Schema.NullOr(Schema.Boolean)), + /** Null where the head repository is gone, which is a comparison nobody can make. */ + baseRef: Schema.optional( + Schema.NullOr( + Schema.Struct({ + compare: Schema.optional( + Schema.NullOr(Schema.Struct({ behindBy: Schema.Number })), + ), + }), + ), + ), + }), + ), + }), + ), + }), +}); + +const decodeBaseComparison = decodeJsonResult(RawBaseComparisonSchema); + +export interface GitHubBaseComparison { + /** Null where the host could not compare, which the page reads as "unknown". */ + readonly behindBy: number | null; + readonly viewerCanUpdate: boolean; +} + +export function decodeBaseComparisonJson( + raw: string, +): Result.Result { + const decoded = decodeBaseComparison(raw); + if (!Result.isSuccess(decoded)) return Result.fail(decoded.failure); + const pullRequest = decoded.success.data.repository?.pullRequest; + const behindBy = pullRequest?.baseRef?.compare?.behindBy; + return Result.succeed({ + behindBy: typeof behindBy === "number" && behindBy >= 0 ? behindBy : null, + viewerCanUpdate: pullRequest?.viewerCanUpdateBranch === true, + }); +} + export const REVIEWER_CANDIDATES_GRAPHQL_QUERY = `query($owner: String!, $name: String!, $number: Int!) { repository(owner: $owner, name: $name) { assignableUsers(first: ${GRAPHQL_PAGE_SIZE}) { @@ -1492,6 +1755,12 @@ export interface GitHubViewerAccess { /** GitHub's own `viewerCanUpdate`, true for the author as well as for anyone with write. */ readonly canUpdate: boolean; readonly didAuthor: boolean; + /** + * GitHub's own `viewerCanUpdateBranch`, read with the base comparison rather than here: it is + * false for a branch that is already current, so it answers "may update, and there is + * something to update" at once. Absent where the comparison was not read. + */ + readonly canUpdateBranch?: boolean; } /** @@ -1539,6 +1808,8 @@ export interface GitHubPullRequestFilesPatch { readonly truncated: boolean; /** Files GitHub returned, counted before decoding, so the caller can page. */ readonly rawCount: number; + /** GitHub's own counts for the files whose hunks it withheld. */ + readonly omittedFileStats: ReadonlyArray; } /** @@ -1554,6 +1825,7 @@ export function decodePullRequestFilesJson( return Result.fail(decoded.failure); } const sections: string[] = []; + const omittedFileStats: PullRequestOmittedFileStat[] = []; let truncated = false; for (const entry of decoded.success) { const file = decodeFileEntry(entry); @@ -1565,7 +1837,12 @@ export function decodePullRequestFilesJson( // A file with no hunks is still a file that changed: a pure rename has none to give, and // a binary one has none that can be shown. Both are listed, and only the second is a hole // in the patch — leaving them out entirely would drop them from the change altogether. - if ((value.additions ?? 0) + (value.deletions ?? 0) > 0) truncated = true; + const additions = value.additions ?? 0; + const deletions = value.deletions ?? 0; + if (additions + deletions > 0) { + truncated = true; + omittedFileStats.push({ path: value.filename, additions, deletions }); + } } // A rename counts its hunks against the old path, which is the only place it is named. const oldPath = @@ -1586,5 +1863,6 @@ export function decodePullRequestFilesJson( patch: sections.join(""), truncated, rawCount: decoded.success.length, + omittedFileStats, }); } diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index 762dac559f2..7346e760388 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -144,6 +144,7 @@ import { selectThreadPreviewMiniPlayer, usePreviewMiniPlayerStore, } from "../previewMiniPlayerStore"; +import { isThreadOwnPullRequest } from "./pullRequest/pullRequestDetail.logic"; import { PullRequestDetailPanel } from "./pullRequest/PullRequestDetailPanel"; import { PullRequestDetailGhost } from "./pullRequest/PullRequestGhosts"; import { PullRequestsUnavailableState } from "./pullRequest/PullRequestsUnavailableState"; @@ -6050,11 +6051,23 @@ function ChatViewContent(props: ChatViewProps) { number: activeRightPanelSurface.number, }} context={ - activeThreadPr?.number === activeRightPanelSurface.number && - threadRepository === activeRightPanelSurface.repository + isThreadOwnPullRequest( + { + projectId: activeProject?.id ?? null, + repository: threadRepository, + number: activeThreadPr?.number ?? null, + }, + { + projectId: activeRightPanelSurface.projectId, + repository: activeRightPanelSurface.repository, + number: activeRightPanelSurface.number, + }, + ) ? "thread" : "page" } + chromeVariant="collapse" + composerDraftTarget={composerDraftTarget} onStateChange={handlePullRequestTabStatusChange} /> ) : activeRightPanelSurface?.kind === "agents" ? ( diff --git a/apps/web/src/components/diffs/StyledDiffCodeView.tsx b/apps/web/src/components/diffs/StyledDiffCodeView.tsx index f422c7aebbd..37ce2085b71 100644 --- a/apps/web/src/components/diffs/StyledDiffCodeView.tsx +++ b/apps/web/src/components/diffs/StyledDiffCodeView.tsx @@ -269,6 +269,11 @@ type StyledDiffCodeViewProps = ( ) & { readonly options?: StyledDiffCodeViewOptions; readonly viewerRef?: Ref>; + /** + * Appended to the shared stylesheet inside the viewer's shadow root, for a surface that has + * to restyle chrome the viewer owns — such as replacing its per-file line counts. + */ + readonly unsafeCSSExtra?: string; }; /** The shared web CodeView surface: app styling and virtualized geometry stay paired here. */ @@ -276,6 +281,7 @@ export function StyledDiffCodeView({ options, viewerRef, className, + unsafeCSSExtra, ...props }: StyledDiffCodeViewProps) { return ( @@ -291,7 +297,9 @@ export function StyledDiffCodeView({ } options={{ ...options, - unsafeCSS: DIFF_VIEW_UNSAFE_CSS, + unsafeCSS: unsafeCSSExtra + ? `${DIFF_VIEW_UNSAFE_CSS}\n${unsafeCSSExtra}` + : DIFF_VIEW_UNSAFE_CSS, itemMetrics: { diffHeaderHeight: 32, hunkSeparatorHeight: 24, diff --git a/apps/web/src/components/pullRequest/PullRequestChecksPopover.tsx b/apps/web/src/components/pullRequest/PullRequestChecksPopover.tsx new file mode 100644 index 00000000000..e281a324209 --- /dev/null +++ b/apps/web/src/components/pullRequest/PullRequestChecksPopover.tsx @@ -0,0 +1,132 @@ +import type { + EnvironmentId, + PullRequestCheck, + PullRequestChecksState, + PullRequestRef, +} from "@t3tools/contracts"; + +import { readLocalApi } from "~/localApi"; +import { cn } from "~/lib/utils"; +import { pullRequestEnvironment } from "~/state/pullRequests"; +import { useEnvironmentQuery } from "~/state/query"; + +import { Popover, PopoverPopup, PopoverTrigger } from "../ui/popover"; +import { + PullRequestCheckStatusIcon, + pullRequestCheckStatusLabel, + pullRequestChecksStatePresentation, + summarizePullRequestChecks, +} from "./pullRequestPresentation"; + +/** + * The checks behind the rollup, for a row that only carries the rollup. Mounted by the popup, so + * the read starts when somebody opens it rather than once per row of a listing — the detail read + * is a request per pull request, and a page of them at rest would be a hundred. + */ +function LazyChecksBody({ + environmentId, + reference, +}: { + environmentId: EnvironmentId; + reference: PullRequestRef; +}) { + const detailQuery = useEnvironmentQuery( + pullRequestEnvironment.detail({ environmentId, input: reference }), + ); + if (detailQuery.error !== null) { + return

{detailQuery.error}

; + } + if (detailQuery.data === null) { + return ( +

+ {detailQuery.isPending ? "Loading checks…" : "No checks reported"} +

+ ); + } + return ; +} + +function ChecksBody({ checks }: { checks: ReadonlyArray }) { + if (checks.length === 0) { + return

No checks reported

; + } + return ( +
    + {checks.map((check) => ( +
  • + + + {check.name} + + + {pullRequestCheckStatusLabel(check.status)} + + {check.url === null ? null : ( + + )} +
  • + ))} +
+ ); +} + +/** + * The checks indicator and what it opens, in both places a change request is shown: a listing + * row, which knows only the rollup, and the detail header, which is already holding every check. + * + * `checks` decides between the two. Given them, nothing is read; without them, the popup reads + * the detail itself, which is why the row must also say which environment it came from. + */ +export function PullRequestChecksPopover({ + checksState, + checks, + environmentId, + reference, + className, +}: { + checksState: PullRequestChecksState; + /** The checks already in hand, for the detail header. Absent on a listing row. */ + checks?: ReadonlyArray; + environmentId?: EnvironmentId; + reference?: PullRequestRef; + className?: string; +}) { + const presentation = pullRequestChecksStatePresentation(checksState); + // Counts beat the rollup's own wording where they are known, the way GitHub's own header reads. + const summary = checks === undefined ? null : summarizePullRequestChecks(checks); + return ( + + {/* A listing row is itself a button, so the trigger renders as a span: a nested button is + not valid inside one. The click is stopped here so opening the checks does not also + select the row it sits on. */} + + } + onClick={(event) => event.stopPropagation()} + > + + + +

{presentation.label}

+ {summary === null ? null :

{summary}

} + {checks !== undefined ? ( + + ) : environmentId !== undefined && reference !== undefined ? ( + + ) : null} +
+
+ ); +} diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 93418e7c301..80a3b820b9c 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -4,6 +4,7 @@ import type { EnvironmentId, PullRequestDetailView, PullRequestDiffSide, + PullRequestOmittedFileStat, PullRequestRef, PullRequestReviewThread, } from "@t3tools/contracts"; @@ -28,6 +29,7 @@ import { useClientSettings } from "~/hooks/useSettings"; import { useTheme } from "~/hooks/useTheme"; import { areAllDiffFilesCollapsed } from "~/lib/diffCollapse"; import { pullRequestFindingKey, type PullRequestFinding } from "./pullRequestDetail.logic"; +import { orderDiffFiles } from "./pullRequestFileOrder.logic"; import { buildFileDiffRenderKey, fnv1a32, @@ -95,8 +97,20 @@ interface DiffSlice { readonly patch: string; readonly truncated: boolean; readonly nextCursor: string | null; + readonly omittedFileStats: ReadonlyArray; } +/** + * The viewer's own per-file counts are hidden and drawn from this side of its shadow root + * instead: its counts are hunk sums, and a file whose hunks the host withheld would read as + * an empty change rather than as the counts the host did report. + */ +const REPLACE_FILE_COUNTS_CSS = ` +[data-diffs-header] [data-additions-count], +[data-diffs-header] [data-deletions-count] { + display: none !important; +}`; + /** Nothing loaded yet, as one identity, so the memos below do not see a new array every render. */ const NO_SLICES: ReadonlyArray = []; @@ -246,6 +260,7 @@ export function PullRequestCodeTab({ patch: data.patch, truncated: data.truncated, nextCursor: data.nextCursor, + omittedFileStats: data.omittedFileStats ?? [], }; const index = slices.findIndex((slice) => slice.cursor === cursor); if (index === -1) { @@ -256,7 +271,17 @@ export function PullRequestCodeTab({ existing !== undefined && existing.patch === next.patch && existing.truncated === next.truncated && - existing.nextCursor === next.nextCursor + existing.nextCursor === next.nextCursor && + existing.omittedFileStats.length === next.omittedFileStats.length && + existing.omittedFileStats.every((file, index) => { + const refreshed = next.omittedFileStats[index]; + return ( + refreshed !== undefined && + refreshed.path === file.path && + refreshed.additions === file.additions && + refreshed.deletions === file.deletions + ); + }) ) { return previous; } @@ -337,16 +362,12 @@ export function PullRequestCodeTab({ }), [loadedSlices, resolvedTheme, scopeKey], ); - // Sorted within a slice rather than across them: sorting the accumulated set would let a late + // Ordered within a slice rather than across them: ordering the accumulated set would let a late // slice push a file the reader is part way through further down the page. const files = useMemo( () => parsedSlices.flatMap((parsed) => - parsed?.kind === "files" - ? parsed.files.toSorted((left, right) => - resolveFileDiffPath(left).localeCompare(resolveFileDiffPath(right)), - ) - : [], + parsed?.kind === "files" ? orderDiffFiles(parsed.files) : [], ), [parsedSlices], ); @@ -471,6 +492,15 @@ export function PullRequestCodeTab({ ], ); const lineStat = useMemo(() => getDiffLineStat(files), [files]); + const omittedFileStats = useMemo( + () => + new Map( + loadedSlices.flatMap((slice) => + slice.omittedFileStats.map((file) => [file.path, file] as const), + ), + ), + [loadedSlices], + ); const fileKeys = useMemo(() => items.map((item) => item.id), [items]); const collapsedFileKeys = useMemo( () => new Set(items.filter((item) => item.collapsed === true).map((item) => item.id)), @@ -649,6 +679,30 @@ export function PullRequestCodeTab({ [toggleFile], ); + const renderHeaderMetadata = useCallback( + (item: CodeViewItem) => { + if (item.type !== "diff") return null; + let additions = 0; + let deletions = 0; + for (const hunk of item.fileDiff.hunks) { + additions += hunk.additionLines; + deletions += hunk.deletionLines; + } + if (additions === 0 && deletions === 0) { + const withheld = omittedFileStats.get(resolveFileDiffPath(item.fileDiff)); + if (withheld) ({ additions, deletions } = withheld); + } + return ( + + ); + }, + [omittedFileStats], + ); + const diffViewOptions = useMemo( () => ({ diffStyle: diffRenderMode === "split" ? ("split" as const) : ("unified" as const), @@ -742,7 +796,7 @@ export function PullRequestCodeTab({ const renderAnnotation = useCallback( (annotation: ReviewAnnotation) => ( -
+
{annotation.metadata.threads.map(renderThreadCard)} {annotation.metadata.pending.map((comment) => ( {reviewOverlay}
diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index 4ac18128010..b9d9f2925e4 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -1,11 +1,13 @@ -import { scopeProjectRef } from "@t3tools/client-runtime/environment"; +import { scopedThreadKey, scopeProjectRef } from "@t3tools/client-runtime/environment"; import { squashAtomCommandFailure } from "@t3tools/client-runtime/state/runtime"; import type { EnvironmentId, PullRequestAction, PullRequestMergeMethod, + PullRequestUpdateMethod, PullRequestRef, PullRequestState, + ScopedThreadRef, } from "@t3tools/contracts"; import { ArrowDownUpIcon, @@ -29,6 +31,7 @@ import { MoreHorizontalIcon, PanelRightIcon, RefreshCwIcon, + ServerIcon, TriangleAlertIcon, } from "lucide-react"; import { @@ -49,6 +52,8 @@ import { usePreparePullRequestThreadAction } from "~/lib/sourceControlActions"; import { cn } from "~/lib/utils"; import { readLocalApi } from "~/localApi"; import type { ReviewCommentContext } from "~/reviewCommentContext"; +import { useProjects } from "~/state/entities"; +import { useEnvironments } from "~/state/environments"; import { useEnvironmentQuery } from "~/state/query"; import { useLiveRefresh } from "~/hooks/useLiveRefresh"; import { pullRequestEnvironment } from "~/state/pullRequests"; @@ -94,12 +99,19 @@ import { handoffReviewComments, pullRequestFindingKey, readableFailure, + resolveBaseFreshness, type PullRequestFinding, } from "./pullRequestDetail.logic"; +import { + resolvePickableEnvironments, + type PickableEnvironment, +} from "./pullRequestProjectAssignment.logic"; +import { PullRequestChecksPopover } from "./PullRequestChecksPopover"; import { PullRequestActorLabel, PullRequestDiffStat, PullRequestMetaLine, + pullRequestChecksState, resolvePullRequestState, summarizePullRequestChecks, } from "./pullRequestPresentation"; @@ -112,6 +124,7 @@ const ACTION_SUCCESS_LABELS: Record = { draft: "Converted to draft", close: "Pull request closed", reopen: "Pull request reopened", + "update-branch": "Branch updated with the base branch", }; /** Said as the thing that did not happen, rather than as the operation that returned an error. */ @@ -121,6 +134,7 @@ const ACTION_FAILURE_LABELS: Record = { draft: "Could not convert this to a draft", close: "Could not close this pull request", reopen: "Could not reopen this pull request", + "update-branch": "Could not update this branch", }; /** What to try, for the times the host says only that it refused. */ @@ -132,6 +146,10 @@ const ACTION_FAILURE_HINTS: Record = { close: "The host refused it. Check that you have write access, or that you opened it.", reopen: "The host refused it. Check that you have write access, and that the branch still exists.", + // A rebase is the one that fails on its own merits: GitHub replays the commits and stops at + // the first that does not apply, which is a conflict the reader has to resolve themselves. + "update-branch": + "The host refused it. A rebase stops at the first commit that does not apply cleanly; updating with a merge commit may still work.", }; /** Named for the host rather than "externally": the point is where you will land. */ @@ -159,7 +177,53 @@ const PullRequestCodeTab = lazy(loadCodeTab); * is closed by the time the next one opens. It is how a prompt the reader has since edited is told * apart from the one they were handed: only the sentence still exactly as written may be replaced. */ -const lastHandoffPromptByDraft = new Map(); +const lastHandoffPromptByDraft = new Map(); + +const composerTargetKey = (target: ScopedThreadRef | DraftId): string => + typeof target === "string" ? target : scopedThreadKey(target); + +/** + * Which server the checkout and the hand-offs land on, where more than one of them holds this + * repository. The list picked one of them to show the pull request under, so that everything on + * it is read from somewhere; where the reader wants to work is a separate answer, and this is + * where they give it. + */ +function ActOnEnvironmentPicker({ + environments, + value, + onChange, + disabled, +}: { + environments: ReadonlyArray; + value: EnvironmentId; + onChange: (environmentId: EnvironmentId) => void; + disabled: boolean; +}) { + return ( + <> + + onChange(environmentId as EnvironmentId)} + > + {environments.map((environment) => ( + + {/* The radio item lays its children out as one block, so the icon and the label + need their own row to share a line. */} + + + {environment.label} + + + ))} + + + ); +} export function PullRequestDetailPanel({ environmentId, @@ -170,6 +234,7 @@ export function PullRequestDetailPanel({ onStateChange, context = "page", chromeVariant = "full", + composerDraftTarget, }: { environmentId: EnvironmentId; reference: PullRequestRef; @@ -206,6 +271,11 @@ export function PullRequestDetailPanel({ * top — the chrome spends its height on what is being read. */ chromeVariant?: "full" | "collapse"; + /** + * The open thread's composer. Beside the thread whose own pull request this is, hand-offs + * land here instead of opening a new thread — the branch is already under the reader's feet. + */ + composerDraftTarget?: ScopedThreadRef | DraftId; }) { const pullRequestKey = `${reference.projectId}:${reference.repository}#${reference.number}`; const [tab, setTab] = useState("summary"); @@ -357,17 +427,54 @@ export function PullRequestDetailPanel({ const runAction = useAtomCommand(pullRequestEnvironment.runAction, { reportFailure: false }); const [actionPending, setActionPending] = useState(false); const newThread = useNewThreadHandler(); + const { environments } = useEnvironments(); + const projects = useProjects(); + // Beside a thread there is nothing to pick: the hand-offs land in that thread's composer, and + // the thread is already on one server's copy of the branch. + const pickableEnvironments = useMemo( + () => + context === "page" + ? resolvePickableEnvironments( + { environmentId, projectId: reference.projectId }, + projects, + environments, + ) + : [], + [context, environmentId, environments, projects, reference.projectId], + ); + // Which server the reader chose, and only for the pull request they chose it on: this one panel + // shows a different pull request every time it is opened, and the choice does not follow. + const [actingScope, setActingScope] = useState<{ + readonly pullRequestKey: string; + readonly environmentId: EnvironmentId; + } | null>(null); + const chosenEnvironmentId = + actingScope?.pullRequestKey === pullRequestKey ? actingScope.environmentId : environmentId; + // Null wherever there is no choice on offer — one server, or a chosen one that has since gone — + // and then the panel's own server and its own checkout are the answer, as they always were. + const acting = + pickableEnvironments.find((entry) => entry.environmentId === chosenEnvironmentId) ?? null; + const actingEnvironmentId = acting?.environmentId ?? environmentId; const prepareThread = usePreparePullRequestThreadAction({ - environmentId, - cwd: detail?.workspaceRoot ?? null, + environmentId: actingEnvironmentId, + cwd: acting?.workspaceRoot ?? detail?.workspaceRoot ?? null, }); - const perform = async (action: PullRequestAction, method?: PullRequestMergeMethod) => { + const perform = async ( + action: PullRequestAction, + method?: PullRequestMergeMethod, + updateMethod?: PullRequestUpdateMethod, + ) => { if (actionPending) return; setActionPending(true); const result = await runAction({ environmentId, - input: { ...reference, action, ...(method ? { mergeMethod: method } : {}) }, + input: { + ...reference, + action, + ...(method ? { mergeMethod: method } : {}), + ...(updateMethod ? { updateMethod } : {}), + }, }); setActionPending(false); if (result._tag === "Failure") { @@ -393,6 +500,27 @@ export function PullRequestDetailPanel({ reviewComments?: ReadonlyArray; }; + // Beside the thread whose own pull request this is, a task belongs in that thread's composer: + // the branch is already checked out under it, so opening a second thread would only scatter + // the work. + const attachTarget = context === "thread" ? (composerDraftTarget ?? null) : null; + + const writeTaskToComposer = (target: ScopedThreadRef | DraftId, task: ThreadTask) => { + const store = useComposerDraftStore.getState(); + const draft = store.getComposerDraft(target); + const key = composerTargetKey(target); + const prompt = handoffPrompt( + { prompt: draft?.prompt ?? "", lastHandoffPrompt: lastHandoffPromptByDraft.get(key) }, + task.prompt, + ); + lastHandoffPromptByDraft.set(key, task.prompt); + store.setPrompt(target, prompt); + store.setReviewComments( + target, + handoffReviewComments(draft?.reviewComments ?? [], task.reviewComments ?? []), + ); + }; + /** * Opens a thread on this project and leaves the task in its composer for the reader to send. * @@ -412,37 +540,32 @@ export function PullRequestDetailPanel({ () => null, )); if (session === null) return null; - const store = useComposerDraftStore.getState(); if (task === null) return session; // The latest press is the ask: it takes over what an earlier hand-off left, prompt and chips // both, rather than stacking a second one under the first. What the reader typed themselves // survives — the composer they are handed is not always a fresh one, and a prompt they have // since edited is theirs rather than the hand-off's. - const draft = store.getComposerDraft(session.draftId); - const existingComments = draft?.reviewComments ?? []; - const prompt = handoffPrompt( - { - prompt: draft?.prompt ?? "", - lastHandoffPrompt: lastHandoffPromptByDraft.get(session.draftId), - }, - task.prompt, - ); - // Remember the hand-off's own contribution, not the merged prompt: only that sentence is - // this session's to take back next time, and the reader's text around it is not. - lastHandoffPromptByDraft.set(session.draftId, task.prompt); - store.setPrompt(session.draftId, prompt); - store.setReviewComments( - session.draftId, - handoffReviewComments(existingComments, task.reviewComments ?? []), - ); + writeTaskToComposer(session.draftId, task); return session; }; /** A question about the change, which needs a thread and nothing else. */ const startAsk = async (kind: string, task: ThreadTask) => { if (!detail || handoff !== null) return; + if (attachTarget !== null) { + writeTaskToComposer(attachTarget, task); + toastManager.add({ + type: "success", + title: "Added to the composer", + description: + task.prompt.length > 0 + ? "The question is in the composer — read it over, then send." + : "The pull request is in the composer — type your question, then send.", + }); + return; + } setHandoff(kind); - const projectRef = scopeProjectRef(environmentId, detail.projectId); + const projectRef = scopeProjectRef(actingEnvironmentId, acting?.projectId ?? detail.projectId); const opened = await openThreadWithTask(projectRef, task); setHandoff(null); if (opened === null) { @@ -477,6 +600,15 @@ export function PullRequestDetailPanel({ mode: "worktree" | "local" = "worktree", ) => { if (!detail || handoff !== null) return; + if (attachTarget !== null && task !== null) { + writeTaskToComposer(attachTarget, task); + toastManager.add({ + type: "success", + title: "Added to the composer", + description: "The task is in the composer — read it over, then send.", + }); + return; + } setHandoff(kind); // The menu closes on the press and takes its "Preparing..." label with it, so this is the // only thing answering for the checkout. It carries no timeout of its own: a loading toast @@ -485,7 +617,9 @@ export function PullRequestDetailPanel({ type: "loading", title: "Preparing the pull request checkout...", }); - const projectRef = scopeProjectRef(environmentId, detail.projectId); + // Wherever the reader chose to act: the thread, the checkout it is pointed at and the composer + // the task lands in are all one server's, and picking another one moves all three. + const projectRef = scopeProjectRef(actingEnvironmentId, acting?.projectId ?? detail.projectId); // The thread is opened before the checkout rather than after it, because the project's setup // script only runs for a checkout that knows which thread it is for — and a worktree with no // dependencies installed is not something anyone can test. @@ -688,6 +822,9 @@ export function PullRequestDetailPanel({ ? mergeMethod : (allowedMergeMethods[0] ?? "merge"); const conflicting = detail?.state === "open" && detail.mergeability === "conflicting"; + // Out of date with the base, and still cleanly mergeable — the one pairing an update button + // exists for. Null everywhere else, including hosts that cannot compare at all. + const freshness = detail === null ? null : resolveBaseFreshness(detail); // A host that cannot produce a patch has no Code tab to open. The tabs themselves stay hidden // until the detail arrives, so the loading ghost is the panel's only unfinished UI. const visibleTabs = TABS.filter( @@ -725,6 +862,7 @@ export function PullRequestDetailPanel({ ? resolvePullRequestState({ state: detail.state, isDraft: detail.isDraft }) : null; const checksSummary = detail ? summarizePullRequestChecks(detail.checks) : null; + const checksState = detail ? pullRequestChecksState(detail.checks) : null; return (
@@ -804,7 +942,10 @@ export function PullRequestDetailPanel({ Conflicts ) : checksSummary ? ( - + + {detail && checksState !== null ? ( + + ) : null} {checksSummary} ) : null} @@ -849,6 +990,14 @@ export function PullRequestDetailPanel({ {handoff === "findings" ? "Preparing..." : "Fix findings in a thread"} + {pickableEnvironments.length > 0 ? ( + setActingScope({ pullRequestKey, environmentId: next })} + disabled={handoff !== null} + /> + ) : null} {detail.state === "open" ? ( <> @@ -982,6 +1131,14 @@ export function PullRequestDetailPanel({ + {pickableEnvironments.length > 0 ? ( + setActingScope({ pullRequestKey, environmentId: next })} + disabled={handoff !== null} + /> + ) : null} ) : null} @@ -1015,7 +1172,16 @@ export function PullRequestDetailPanel({ {/* The condensed chrome's second row: the tabs that the closing fold takes with it, and compact copies of the branch pair and diff stat so they stay in sight while the full rows are folded away. Same zero-track mechanism as the fold, inverted. */} -
+
{visibleTabs.map((item) => ( @@ -1202,7 +1371,13 @@ export function PullRequestDetailPanel({ className="ml-auto inline-flex shrink-0 items-center gap-1.5 text-xs text-muted-foreground" aria-label={checksSummary ? `Checks: ${checksSummary}` : "Checks"} > - + {/* The rollup icon opens the checks behind the summary; with none reported + there is nothing to open, so the plain glyph stays. */} + {detail && checksState !== null ? ( + + ) : ( + + )} {checksSummary} ) : tab === "timeline" ? ( @@ -1272,6 +1447,61 @@ export function PullRequestDetailPanel({
+ {freshness ? ( +
+ + + This branch is out-of-date with {detail?.baseBranch ?? "the base branch"} + {freshness.behindBy === null + ? "" + : ` by ${freshness.behindBy.toLocaleString()} ${ + freshness.behindBy === 1 ? "commit" : "commits" + }`} + . + + Changes can be cleanly merged. + {freshness.methods.length > 0 ? ( + + + {/* The second way only where the host offers it and this reader may take it: + GitHub replays the commits for a rebase and refuses the ones it cannot. */} + {freshness.methods.length > 1 ? ( + + + + + + {freshness.methods.map((method) => ( + void perform("update-branch", undefined, method)} + > + + {method === "rebase" ? "Update with rebase" : "Update with merge commit"} + + ))} + + + ) : null} + + ) : null} +
+ ) : null} +
0) { - compensationRef.current = chromeDelta; next = false; } } else if (foldHeight > 0 && top > foldHeight + 32) { diff --git a/apps/web/src/components/pullRequest/PullRequestListFilters.test.tsx b/apps/web/src/components/pullRequest/PullRequestListFilters.test.tsx index 545e3066f81..4c5e0ca6744 100644 --- a/apps/web/src/components/pullRequest/PullRequestListFilters.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestListFilters.test.tsx @@ -1,4 +1,4 @@ -import type { ProjectId } from "@t3tools/contracts"; +import type { EnvironmentId, ProjectId } from "@t3tools/contracts"; import { CircleIcon } from "lucide-react"; import { Children, isValidElement, type ReactElement, type ReactNode } from "react"; import { describe, expect, it, vi } from "vite-plus/test"; @@ -53,10 +53,14 @@ function menu(overrides: Partial[0]>) involvement: "all", involvementOptions: [{ value: "all", label: "All", Icon: CircleIcon }], onInvolvement: () => undefined, + filters: {}, + onFilters: () => undefined, host: undefined, hostOptions: [], onHost: () => undefined, - environmentId: null, + server: undefined, + serverOptions: [], + onServer: () => undefined, projects: [], projectId: undefined, unavailable: new Map(), @@ -79,11 +83,43 @@ describe("pull request filters menu", () => { expect(onState).toHaveBeenCalledWith("closed"); }); + it("names the chosen narrowing and leaves the others alone", () => { + const onFilters = vi.fn(); + const group = findValueChange( + findLabeledGroup(menu({ filters: { review: "approved" }, onFilters }), "Draft"), + ); + expect(group).toBeDefined(); + + group?.props.onValueChange("hide"); + expect(onFilters).toHaveBeenCalledWith({ review: "approved", draft: "hide" }); + }); + + it("drops a narrowing chosen back to all rather than sending it as undefined", () => { + const onFilters = vi.fn(); + const group = findValueChange( + findLabeledGroup( + menu({ filters: { review: "none", checks: "failing" }, onFilters }), + "Review", + ), + ); + expect(group).toBeDefined(); + + group?.props.onValueChange("all"); + expect(onFilters).toHaveBeenCalledWith({ checks: "failing" }); + }); + it("does not emit a change when the selected project is chosen again", () => { const projectId = "project-1" as ProjectId; const onProject = vi.fn(); const view = menu({ - projects: [{ id: projectId, title: "T3 Code", workspaceRoot: "/work/t3code" }], + projects: [ + { + id: projectId, + environmentId: "env-1" as EnvironmentId, + title: "T3 Code", + workspaceRoot: "/work/t3code", + }, + ], projectId, onProject, }); diff --git a/apps/web/src/components/pullRequest/PullRequestListFilters.tsx b/apps/web/src/components/pullRequest/PullRequestListFilters.tsx index cb95a5be35b..d5a1fc38c9b 100644 --- a/apps/web/src/components/pullRequest/PullRequestListFilters.tsx +++ b/apps/web/src/components/pullRequest/PullRequestListFilters.tsx @@ -2,10 +2,23 @@ import type { EnvironmentId, ProjectId, PullRequestInvolvement, + PullRequestListFilters, PullRequestListState, SourceControlProviderKind, } from "@t3tools/contracts"; -import { FolderGit2Icon, LayersIcon, ListFilterIcon, LoaderIcon, SearchIcon } from "lucide-react"; +import { + CircleCheckIcon, + CircleDashedIcon, + CircleSlashIcon, + CircleXIcon, + EyeOffIcon, + FolderGit2Icon, + GitPullRequestDraftIcon, + LayersIcon, + ListFilterIcon, + LoaderIcon, + SearchIcon, +} from "lucide-react"; import type { ElementType } from "react"; import { cn } from "~/lib/utils"; @@ -82,7 +95,7 @@ export function PullRequestSearchInput({ type="text" value={value} onChange={(event) => onChange(event.currentTarget.value)} - placeholder="Search pull requests" + placeholder="Search pull requests, or label:bug" aria-label="Search pull requests" // Tracks the shared input's height at both widths, so it stays level with the icon // button beside it rather than towering over it on wide screens. @@ -101,6 +114,30 @@ export function PullRequestSearchInput({ const ALL_PROJECTS_VALUE = "all"; /** MenuRadioGroup wants a string, so "every host" wears the one value no host can be. */ const ALL_HOSTS_VALUE = ""; +/** The same trick for the servers, which are named by an id no empty string can collide with. */ +const ALL_SERVERS_VALUE = ""; +/** The unset value of each narrowing group, which no filter of theirs is named after. */ +const UNFILTERED_VALUE = "all"; + +const DRAFT_OPTIONS = [ + { value: UNFILTERED_VALUE, label: "All", Icon: LayersIcon }, + { value: "only", label: "Drafts only", Icon: GitPullRequestDraftIcon }, + { value: "hide", label: "Hide drafts", Icon: EyeOffIcon }, +] as const satisfies ReadonlyArray>; + +const REVIEW_OPTIONS = [ + { value: UNFILTERED_VALUE, label: "All", Icon: LayersIcon }, + { value: "approved", label: "Approved", Icon: CircleCheckIcon }, + { value: "changes-requested", label: "Changes requested", Icon: CircleXIcon }, + { value: "review-required", label: "Review required", Icon: CircleDashedIcon }, + { value: "none", label: "No reviews", Icon: CircleSlashIcon }, +] as const satisfies ReadonlyArray>; + +const CHECKS_OPTIONS = [ + { value: UNFILTERED_VALUE, label: "All", Icon: LayersIcon }, + { value: "passing", label: "Passing", Icon: CircleCheckIcon }, + { value: "failing", label: "Failing", Icon: CircleXIcon }, +] as const satisfies ReadonlyArray>; function PullRequestFilterRadioGroup({ label, @@ -147,10 +184,14 @@ export function PullRequestFiltersMenu({ involvement, involvementOptions, onInvolvement, + filters, + onFilters, host, hostOptions, onHost, - environmentId, + server, + serverOptions, + onServer, projects, projectId, unavailable, @@ -162,6 +203,9 @@ export function PullRequestFiltersMenu({ involvement: PullRequestInvolvement; involvementOptions: ReadonlyArray>; onInvolvement: (involvement: PullRequestInvolvement) => void; + /** The narrowings beyond state and involvement; an absent field is that group unfiltered. */ + filters: PullRequestListFilters; + onFilters: (filters: PullRequestListFilters) => void; host: string | undefined; /** * Includes the "all hosts" entry, whose value is the empty string. With fewer than two real @@ -169,10 +213,17 @@ export function PullRequestFiltersMenu({ */ hostOptions: ReadonlyArray>; onHost: (host: string | undefined) => void; - /** Where the projects' own favicons are read from; null before the environment is known. */ - environmentId: EnvironmentId | null; + server: EnvironmentId | undefined; + /** + * Includes the "all servers" entry, whose value is the empty string. With one server there is + * nothing to switch between, so the whole group stays out of the menu. + */ + serverOptions: ReadonlyArray>; + onServer: (server: EnvironmentId | undefined) => void; + /** The projects of every connected environment, each carrying the one its favicon is read from. */ projects: ReadonlyArray<{ readonly id: ProjectId; + readonly environmentId: EnvironmentId; readonly title: string; readonly workspaceRoot: string; }>; @@ -186,7 +237,22 @@ export function PullRequestFiltersMenu({ onProject: (projectId: ProjectId | undefined) => void; }) { const filtered = - state !== "open" || involvement !== "all" || host !== undefined || projectId !== undefined; + state !== "open" || + involvement !== "all" || + host !== undefined || + server !== undefined || + projectId !== undefined || + Object.keys(filters).length > 0; + /** + * Rebuilt rather than spread so an unfiltered group leaves the record instead of lingering in + * it as an explicit `undefined`, which the listing input does not accept. + */ + const withFilter = (key: keyof PullRequestListFilters, value: string): PullRequestListFilters => + Object.fromEntries( + Object.entries({ ...filters, [key]: value === UNFILTERED_VALUE ? undefined : value }).filter( + ([, held]) => held !== undefined, + ), + ) as PullRequestListFilters; return ( + + onFilters(withFilter("draft", next))} + /> + + onFilters(withFilter("review", next))} + /> + + onFilters(withFilter("checks", next))} + /> {hostOptions.length > 2 ? ( <> @@ -230,6 +317,19 @@ export function PullRequestFiltersMenu({ /> ) : null} + {serverOptions.length > 2 ? ( + <> + + + onServer(next === ALL_SERVERS_VALUE ? undefined : (next as EnvironmentId)) + } + /> + + ) : null} - {environmentId === null ? ( - - ) : ( - - )} + {project.title} {reason === undefined ? null : ( diff --git a/apps/web/src/components/pullRequest/PullRequestRow.tsx b/apps/web/src/components/pullRequest/PullRequestRow.tsx index e1ada6efcde..62d3d236058 100644 --- a/apps/web/src/components/pullRequest/PullRequestRow.tsx +++ b/apps/web/src/components/pullRequest/PullRequestRow.tsx @@ -1,5 +1,3 @@ -import type { PullRequestListEntry } from "@t3tools/contracts"; - import { memo } from "react"; import { cn } from "~/lib/utils"; @@ -7,6 +5,8 @@ import { getSourceControlPresentationForKind } from "~/sourceControlPresentation import { formatRelativeTimeLabel } from "~/timestampFormat"; import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; +import { PullRequestChecksPopover } from "./PullRequestChecksPopover"; +import type { EnvironmentPullRequestEntry } from "./pullRequestList.logic"; import { PullRequestActorLabel, PullRequestDiffStat, @@ -19,20 +19,23 @@ function PullRequestRowImpl({ selected, showProjectTitle, showProvider, + environmentLabel, matchedElsewhere, onSelect, }: { - entry: PullRequestListEntry; + entry: EnvironmentPullRequestEntry; selected: boolean; showProjectTitle: boolean; /** Only when the list spans more than one host, where the repository alone is ambiguous. */ showProvider: boolean; + /** Names the server this row was read from, where the list spans more than one. */ + environmentLabel?: string; /** * A search found this, but in something the row does not show — a description, a comment, a * commit message. Saying so is the difference between a result and an apparently random row. */ matchedElsewhere?: boolean; - onSelect: (entry: PullRequestListEntry) => void; + onSelect: (entry: EnvironmentPullRequestEntry) => void; }) { const { Icon, providerName } = getSourceControlPresentationForKind(entry.provider); return ( @@ -70,10 +73,38 @@ function PullRequestRowImpl({ #{entry.number} {showProjectTitle ? {entry.repository} : null} + {environmentLabel ? ( + {environmentLabel} + ) : null} {entry.headBranch} + {/* Only a verdict somebody has actually given: "review required" is the absence of + one, and saying so on every unreviewed row would say nothing. */} + {entry.reviewDecision === "approved" || entry.reviewDecision === "changes-requested" ? ( + + {entry.reviewDecision === "approved" ? "Approved" : "Changes requested"} + + ) : null} + {entry.checksState === undefined ? null : ( + + )} {matchedElsewhere ? ( matched in the description diff --git a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx index 345753fe880..33a09176913 100644 --- a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx @@ -1,10 +1,18 @@ -import type { EnvironmentId, PullRequestDetailView, PullRequestRef } from "@t3tools/contracts"; +import type { + EnvironmentId, + PullRequestActor, + PullRequestComment, + PullRequestDetailView, + PullRequestRef, +} from "@t3tools/contracts"; import { ArrowDownUpIcon, + ChevronDownIcon, ChevronRightIcon, HammerIcon, MessageSquareIcon, SendIcon, + TagIcon, UsersIcon, } from "lucide-react"; import { useState, type ReactNode } from "react"; @@ -21,9 +29,9 @@ import { Textarea } from "../ui/textarea"; import { toastManager } from "../ui/toast"; import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; import { + PullRequestActorAvatar, PullRequestActorLabel, PullRequestCheckStatusIcon, - PullRequestMetaLine, pullRequestCheckStatusLabel, } from "./pullRequestPresentation"; import { PullRequestReviewerPicker } from "./PullRequestReviewerPicker"; @@ -36,6 +44,83 @@ import { import { PullRequestMarkdown } from "./PullRequestMarkdown"; import { PullRequestConversationGhost } from "./PullRequestGhosts"; +/** A host colour only when it is one, so a malformed value falls back to the neutral dot. */ +function labelDotColor(color: string | null): string | null { + const hex = color?.trim().replace(/^#/, "") ?? ""; + return /^[0-9a-fA-F]{6}$/.test(hex) ? `#${hex}` : null; +} + +/** The avatar carries the attribution alone; who it is arrives on hover, like the reviewer row. */ +function CommentAuthor({ actor }: { actor: PullRequestActor | null }) { + const login = actor?.login ?? "ghost"; + return ( + + }> + + + + {actor?.name && actor.name !== login ? `${actor.name} (@${login})` : login} + + + ); +} + +/** "CHANGES_REQUESTED" reads as "Changes requested": one capital, the host's underscores gone. */ +function reviewStateLabel(state: string): string { + const words = state.toLowerCase().replace(/_/g, " "); + return words.charAt(0).toUpperCase() + words.slice(1); +} + +/** Finished work — a resolved conversation or a dismissed approval — opens collapsed. */ +function CollapsedComment({ + comment, + cwd, + label, +}: { + comment: PullRequestComment; + cwd: string; + label: string; +}) { + const [open, setOpen] = useState(false); + return ( + +
+ + + + {formatRelativeTimeLabel(comment.createdAt)} + {label} + + + + + {open ? ( +
+ {comment.path ? ( +

+ {comment.path} +

+ ) : null} + +
+ ) : null} +
+
+
+ ); +} + function MetaRow({ icon, label, @@ -261,6 +346,28 @@ export function PullRequestSummaryTab({ ) : null}
+ {detail.labels.length > 0 ? ( + } label="Labels"> + + {detail.labels.map((label) => { + const dot = labelDotColor(label.color); + return ( + + + {label.name} + + ); + })} + + + ) : null} } label="Comments"> {activityPending ? "Loading conversation…" @@ -385,14 +492,25 @@ export function PullRequestSummaryTab({ ) : null} {visibleComments.map((comment) => { const thread = threadByCommentId.get(comment.id); + const reviewState = comment.reviewState?.toLowerCase(); + if (thread?.isResolved || reviewState === "dismissed") { + return ( + + ); + } + // An approval is a verdict, not a finding: there is nothing in it to fix. const finding: PullRequestFinding | null = - comment.kind !== "review" && comment.kind !== "review-comment" + (comment.kind !== "review" && comment.kind !== "review-comment") || + reviewState === "approved" ? null : thread === undefined ? { kind: "comment", comment } - : thread.isResolved - ? null - : { kind: "thread", thread }; + : { kind: "thread", thread }; return (
- - + + {formatRelativeTimeLabel(comment.createdAt)} {comment.reviewState ? ( - {comment.reviewState.toLowerCase()} + {reviewStateLabel(comment.reviewState)} ) : null} - + {/* Review remarks only. A plain conversation comment is talk, not a finding, and offering to fix one would promise more than it says. */} {onFixFinding && finding ? ( diff --git a/apps/web/src/components/pullRequest/pullRequestChecks.test.tsx b/apps/web/src/components/pullRequest/pullRequestChecks.test.tsx new file mode 100644 index 00000000000..23d70ae7b7d --- /dev/null +++ b/apps/web/src/components/pullRequest/pullRequestChecks.test.tsx @@ -0,0 +1,85 @@ +import type { EnvironmentId, ProjectId, PullRequestCheck } from "@t3tools/contracts"; +import { Children, isValidElement, type ReactNode } from "react"; +import { describe, expect, it } from "vite-plus/test"; + +import { PullRequestChecksPopover } from "./PullRequestChecksPopover"; +import type { EnvironmentPullRequestEntry } from "./pullRequestList.logic"; +import { PullRequestRow } from "./PullRequestRow"; +import { pullRequestChecksState } from "./pullRequestPresentation"; + +function check(status: PullRequestCheck["status"]): PullRequestCheck { + return { name: `check-${status}`, status, description: null, url: null }; +} + +describe("pullRequestChecksState", () => { + it("lets a failure outrank a run still going, and reports nothing without checks", () => { + expect(pullRequestChecksState([check("success"), check("pending"), check("failure")])).toBe( + "failing", + ); + expect(pullRequestChecksState([check("success"), check("cancelled")])).toBe("failing"); + expect(pullRequestChecksState([check("success"), check("pending")])).toBe("pending"); + expect(pullRequestChecksState([check("success")])).toBe("passing"); + // Skipped and neutral are neither a pass nor a failure, so they are no verdict at all. + expect(pullRequestChecksState([check("skipped"), check("neutral")])).toBe(null); + expect(pullRequestChecksState([])).toBe(null); + }); +}); + +/** Every element of the tree the row returned, so a nested indicator can be looked for. */ +function flatten(node: ReactNode): ReadonlyArray> { + const found: unknown[] = []; + for (const child of Children.toArray(node)) { + if (!isValidElement(child)) continue; + found.push(child); + found.push(...flatten((child.props as { readonly children?: ReactNode }).children)); + } + return found as ReadonlyArray>; +} + +function entry(overrides: Partial): EnvironmentPullRequestEntry { + return { + environmentId: "env-1" as EnvironmentId, + projectId: "project-1" as ProjectId, + provider: "github", + repository: "pingdotgg/t3code", + number: 1, + title: "Add the pull requests page", + url: "https://github.com/pingdotgg/t3code/pull/1", + author: null, + headBranch: "feat/page", + baseBranch: "main", + state: "open", + isDraft: false, + mergeability: "mergeable", + additions: 1, + deletions: 0, + createdAt: "2026-07-01T00:00:00Z", + updatedAt: "2026-07-02T00:00:00Z", + viewerReviewRequested: false, + labels: [], + ...overrides, + } as EnvironmentPullRequestEntry; +} + +function row(overrides: Partial): ReactNode { + return PullRequestRow.type({ + entry: entry(overrides), + selected: false, + showProjectTitle: false, + showProvider: false, + onSelect: () => {}, + }); +} + +describe("PullRequestRow checks indicator", () => { + function indicators(node: ReactNode): number { + return flatten(node).filter( + (element) => (element as { type?: unknown }).type === PullRequestChecksPopover, + ).length; + } + + it("shows the indicator only for a row the host reported a rollup for", () => { + expect(indicators(row({ checksState: "failing" }))).toBe(1); + expect(indicators(row({}))).toBe(0); + }); +}); diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts index d9de190e7ac..1eed80b1946 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts @@ -15,9 +15,11 @@ import { groupPullRequestTimelineConversations, handoffPrompt, handoffReviewComments, + isThreadOwnPullRequest, orderPullRequestComments, pullRequestFindingKey, readableFailure, + resolveBaseFreshness, buildPullRequestTimeline, describePullRequestState, } from "./pullRequestDetail.logic"; @@ -787,3 +789,98 @@ describe("a second ask into the same composer", () => { ]); }); }); + +describe("how the branch stands against its base", () => { + const detail = (overrides: Record = {}) => + ({ + state: "open", + mergeability: "mergeable", + baseComparison: "behind", + behindBy: 12, + capabilities: { updateMethods: ["merge", "rebase"] }, + viewerPermissions: { updateMethods: ["merge", "rebase"] }, + ...overrides, + }) as Parameters[0]; + + it("offers both ways where the host and the reader both allow them", () => { + expect(resolveBaseFreshness(detail())).toEqual({ behindBy: 12, methods: ["merge", "rebase"] }); + }); + + it("says nothing about a branch that is already current", () => { + expect(resolveBaseFreshness(detail({ baseComparison: "up-to-date" }))).toBeNull(); + }); + + it("says nothing where the host could not compare, rather than claiming it is current", () => { + expect(resolveBaseFreshness(detail({ baseComparison: "unknown" }))).toBeNull(); + expect(resolveBaseFreshness(detail({ baseComparison: undefined }))).toBeNull(); + }); + + it("leaves a conflicting branch to the conflicts row", () => { + expect(resolveBaseFreshness(detail({ mergeability: "conflicting" }))).toBeNull(); + }); + + it("says nothing where the host has no merge verdict yet", () => { + expect(resolveBaseFreshness(detail({ mergeability: "unknown" }))).toBeNull(); + }); + + it("says nothing about a merged or closed pull request", () => { + expect(resolveBaseFreshness(detail({ state: "merged" }))).toBeNull(); + expect(resolveBaseFreshness(detail({ state: "closed" }))).toBeNull(); + }); + + it("narrows to what this reader may actually take", () => { + expect( + resolveBaseFreshness(detail({ viewerPermissions: { updateMethods: ["merge"] } }))?.methods, + ).toEqual(["merge"]); + }); + + it("still reports the news where the reader may take none of it", () => { + // Somebody reading another account's pull request is told why it is blocked without being + // offered a button the host would refuse. + expect(resolveBaseFreshness(detail({ viewerPermissions: {} }))).toEqual({ + behindBy: 12, + methods: [], + }); + }); + + it("reports a count only where the host counted", () => { + expect(resolveBaseFreshness(detail({ behindBy: undefined }))?.behindBy).toBeNull(); + }); +}); + +describe("whether the panel is showing the thread's own pull request", () => { + const surface = { projectId: "proj-a", repository: "acme/app", number: 7 }; + + it("matches on project, repository and number together", () => { + expect( + isThreadOwnPullRequest({ projectId: "proj-a", repository: "acme/app", number: 7 }, surface), + ).toBe(true); + }); + + it("rejects a second checkout of the same repository under another project", () => { + expect( + isThreadOwnPullRequest({ projectId: "proj-b", repository: "acme/app", number: 7 }, surface), + ).toBe(false); + }); + + it("rejects another repository or another number", () => { + expect( + isThreadOwnPullRequest({ projectId: "proj-a", repository: "acme/web", number: 7 }, surface), + ).toBe(false); + expect( + isThreadOwnPullRequest({ projectId: "proj-a", repository: "acme/app", number: 8 }, surface), + ).toBe(false); + }); + + it("rejects a thread with no project or no pull request of its own", () => { + expect( + isThreadOwnPullRequest({ projectId: null, repository: "acme/app", number: 7 }, surface), + ).toBe(false); + expect( + isThreadOwnPullRequest( + { projectId: "proj-a", repository: "acme/app", number: null }, + surface, + ), + ).toBe(false); + }); +}); diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts index e7d16fd3147..facea6a4d3b 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts @@ -1,14 +1,41 @@ import type { PullRequestActor, + PullRequestBaseComparison, PullRequestCheck, PullRequestComment, PullRequestDetailView, + PullRequestMergeability, PullRequestReviewThread, PullRequestState, + PullRequestUpdateMethod, } from "@t3tools/contracts"; import { inferReviewCommentFenceLanguage, type ReviewCommentContext } from "~/reviewCommentContext"; +/** + * Whether the pull request on a right-panel surface is the thread's own one. Repository and + * number are not enough: one environment can hold two checkouts of the same repository under + * different projects, and the other project's checkout is somebody else's branch. + */ +export function isThreadOwnPullRequest( + thread: { + readonly projectId: string | null; + readonly repository: string | null; + readonly number: number | null; + }, + surface: { + readonly projectId: string; + readonly repository: string; + readonly number: number; + }, +): boolean { + return ( + thread.projectId === surface.projectId && + thread.repository === surface.repository && + thread.number === surface.number + ); +} + /** Plain-language state, shown beside the author. Conflicts are a merge signal, not a state. */ export function describePullRequestState(state: PullRequestState, isDraft: boolean): string { if (state === "merged") return "Merged"; @@ -648,3 +675,40 @@ export function readableFailure(failure: unknown, hint: string): string { // that contradicts it is worse than no guess at all. return bounded; } + +/** + * Where the branch stands against its base, said the way GitHub says it: current, out of date but + * still cleanly mergeable, or conflicting. Only the middle one is an offer — the conflicts row + * already speaks for a branch that collides, and a current branch has nothing to report. + * + * Null where there is nothing to show, which is also every host that cannot compare or has not + * said yet: silence is not the same claim as "up to date", and a banner nobody can act on is + * noise. Only a host verdict of "mergeable" earns the clean-merge wording. + */ +export function resolveBaseFreshness(detail: { + readonly state: PullRequestState; + readonly mergeability: PullRequestMergeability; + readonly baseComparison?: PullRequestBaseComparison | undefined; + readonly behindBy?: number | undefined; + readonly capabilities: { + readonly updateMethods?: ReadonlyArray | undefined; + }; + readonly viewerPermissions: { + readonly updateMethods?: ReadonlyArray | undefined; + }; +}): { + readonly behindBy: number | null; + /** Empty where the branch is stale but this reader may not move it: news, not an offer. */ + readonly methods: ReadonlyArray; +} | null { + if (detail.state !== "open" || detail.baseComparison !== "behind") return null; + // A conflicting branch cannot be updated cleanly either, and the conflicts row is already + // saying the more useful half of that. An unknown verdict is not a clean merge in waiting. + if (detail.mergeability !== "mergeable") return null; + const offered = detail.capabilities.updateMethods ?? []; + const allowed = detail.viewerPermissions.updateMethods ?? []; + return { + behindBy: detail.behindBy ?? null, + methods: offered.filter((method) => allowed.includes(method)), + }; +} diff --git a/apps/web/src/components/pullRequest/pullRequestFileOrder.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestFileOrder.logic.test.ts new file mode 100644 index 00000000000..720a5669178 --- /dev/null +++ b/apps/web/src/components/pullRequest/pullRequestFileOrder.logic.test.ts @@ -0,0 +1,167 @@ +import type { FileDiffMetadata } from "@pierre/diffs"; +import { describe, expect, it } from "vite-plus/test"; + +import { diffFileTier, orderDiffFiles } from "./pullRequestFileOrder.logic"; + +/** Only the path and the patch's own lines matter here; the viewer fills the rest in. */ +function file(name: string, additionLines: ReadonlyArray = []): FileDiffMetadata { + return { name, hunks: [], additionLines, deletionLines: [] } as unknown as FileDiffMetadata; +} + +function order(files: ReadonlyArray): Array { + return orderDiffFiles(files).map((entry) => entry.name); +} + +describe("diffFileTier", () => { + it("puts lockfiles, snapshots and build output last", () => { + expect(diffFileTier("pnpm-lock.yaml")).toBe("generated"); + expect(diffFileTier("apps/web/package-lock.json")).toBe("generated"); + expect(diffFileTier("src/__snapshots__/app.ts")).toBe("generated"); + expect(diffFileTier("src/app.test.ts.snap")).toBe("generated"); + expect(diffFileTier("src/api.generated.ts")).toBe("generated"); + expect(diffFileTier("public/app.min.js")).toBe("generated"); + expect(diffFileTier("dist/app.js")).toBe("generated"); + expect(diffFileTier("packages/core/vendor/lib.js")).toBe("generated"); + }); + + it("recognises a test by its name or by the directory holding it", () => { + expect(diffFileTier("src/app.test.ts")).toBe("test"); + expect(diffFileTier("src/app.spec.tsx")).toBe("test"); + expect(diffFileTier("src/__tests__/app.ts")).toBe("test"); + expect(diffFileTier("test/app.ts")).toBe("test"); + expect(diffFileTier("tests/helpers/app.ts")).toBe("test"); + }); + + it("treats everything else as source, including files merely named like a directory", () => { + expect(diffFileTier("src/app.ts")).toBe("source"); + expect(diffFileTier("src/testing.ts")).toBe("source"); + expect(diffFileTier("src/dist.ts")).toBe("source"); + }); +}); + +describe("orderDiffFiles", () => { + it("answers an empty diff with an empty order", () => { + expect(order([])).toEqual([]); + }); + + it("puts a dependency before what imports it", () => { + expect(order([file("src/a.ts", ['import { b } from "./b";']), file("src/b.ts")])).toEqual([ + "src/b.ts", + "src/a.ts", + ]); + }); + + it("follows a chain the whole way down", () => { + expect( + order([ + file("src/a.ts", ['import { b } from "./b";']), + file("src/b.ts", ['import { c } from "./c";']), + file("src/c.ts"), + ]), + ).toEqual(["src/c.ts", "src/b.ts", "src/a.ts"]); + }); + + it("resolves a specifier that climbs out of the importer's directory", () => { + expect( + order([file("src/ui/a.ts", ['import { b } from "../lib/b";']), file("src/lib/b.ts")]), + ).toEqual(["src/lib/b.ts", "src/ui/a.ts"]); + }); + + it("resolves a directory specifier to that directory's index file", () => { + expect( + order([file("src/a.ts", ['import { b } from "./lib";']), file("src/lib/index.ts")]), + ).toEqual(["src/lib/index.ts", "src/a.ts"]); + }); + + it("falls back to the specifier's last segment when the path does not line up", () => { + // An aliased import (`~/lib/b`) resolves to nothing on disk here, but the diff has only one + // file that could be meant. + expect(order([file("src/a.ts", ['import { b } from "~/lib/b";']), file("lib/b.ts")])).toEqual([ + "lib/b.ts", + "src/a.ts", + ]); + }); + + it("ignores an ambiguous last segment rather than guessing", () => { + expect( + order([ + file("src/a.ts", ['import { b } from "~/somewhere/b";']), + file("one/b.ts"), + file("two/b.ts"), + ]), + ).toEqual(["one/b.ts", "src/a.ts", "two/b.ts"]); + }); + + it("resolves a specifier naming an extension, including the .js beside a .ts", () => { + expect(order([file("src/a.ts", ['import { b } from "./b.js";']), file("src/b.ts")])).toEqual([ + "src/b.ts", + "src/a.ts", + ]); + }); + + it("reads require and bare imports too", () => { + expect( + order([ + file("src/a.ts", ['const b = require("./b");', 'import "./c";']), + file("src/b.ts"), + file("src/c.ts"), + ]), + ).toEqual(["src/b.ts", "src/c.ts", "src/a.ts"]); + }); + + it("falls back to path order inside a cycle", () => { + expect( + order([ + file("src/b.ts", ['import { a } from "./a";']), + file("src/a.ts", ['import { b } from "./b";']), + ]), + ).toEqual(["src/a.ts", "src/b.ts"]); + }); + + it("clusters unrelated files by path", () => { + expect(order([file("src/ui/b.ts"), file("src/lib/z.ts"), file("src/lib/a.ts")])).toEqual([ + "src/lib/a.ts", + "src/lib/z.ts", + "src/ui/b.ts", + ]); + }); + + it("keeps the tiers apart and orders tests by the source they cover", () => { + expect( + order([ + file("pnpm-lock.yaml"), + file("src/a.test.ts"), + file("src/__tests__/b.ts"), + file("src/a.ts", ['import { b } from "./b";']), + file("src/b.ts"), + file("dist/a.js"), + ]), + ).toEqual([ + "src/b.ts", + "src/a.ts", + "src/__tests__/b.ts", + "src/a.test.ts", + "dist/a.js", + "pnpm-lock.yaml", + ]); + }); + + it("puts a test with no source in the diff after the ones that have theirs", () => { + expect(order([file("src/orphan.spec.ts"), file("src/a.test.ts"), file("src/a.ts")])).toEqual([ + "src/a.ts", + "src/a.test.ts", + "src/orphan.spec.ts", + ]); + }); + + it("orders the same diff the same way whatever order it arrives in", () => { + const files = [ + file("src/a.ts", ['import { b } from "./b";']), + file("src/b.ts"), + file("src/c.ts"), + file("src/a.test.ts"), + file("yarn.lock"), + ]; + expect(order(files)).toEqual(order(files.toReversed())); + }); +}); diff --git a/apps/web/src/components/pullRequest/pullRequestFileOrder.logic.ts b/apps/web/src/components/pullRequest/pullRequestFileOrder.logic.ts new file mode 100644 index 00000000000..b46ed88539c --- /dev/null +++ b/apps/web/src/components/pullRequest/pullRequestFileOrder.logic.ts @@ -0,0 +1,197 @@ +import type { FileDiffMetadata } from "@pierre/diffs"; + +import { resolveFileDiffPath } from "~/lib/diffRendering"; + +/** + * Which of the three reading passes a file belongs to. + * + * A reviewer reads the change itself first, then what proves it, and a lockfile or a build output + * only ever confirms what the source already said. Ordering by path alone buries the one file the + * change is really about under whichever directory happens to sort first. + */ +export type DiffFileTier = "source" | "test" | "generated"; + +const GENERATED_FILE_NAMES = new Set([ + "pnpm-lock.yaml", + "yarn.lock", + "package-lock.json", + "bun.lockb", + "Cargo.lock", + "go.sum", + "composer.lock", + "Gemfile.lock", +]); +const GENERATED_DIRECTORIES = new Set([ + "__snapshots__", + "__generated__", + "dist", + "build", + "vendor", +]); +const TEST_DIRECTORIES = new Set(["__tests__", "tests", "test"]); +const MODULE_EXTENSIONS = [".ts", ".tsx", ".js", ".jsx", ".mjs", ".cjs"]; + +export function diffFileTier(path: string): DiffFileTier { + const segments = path.split("/"); + const name = segments.at(-1) ?? ""; + if ( + GENERATED_FILE_NAMES.has(name) || + name.endsWith(".snap") || + name.endsWith(".min.js") || + name.endsWith(".min.css") || + /\.generated\./.test(name) || + segments.slice(0, -1).some((segment) => GENERATED_DIRECTORIES.has(segment)) + ) { + return "generated"; + } + if ( + /\.(?:test|spec)\./.test(name) || + segments.slice(0, -1).some((segment) => TEST_DIRECTORIES.has(segment)) + ) { + return "test"; + } + return "source"; +} + +function stripExtension(path: string): string { + const dot = path.lastIndexOf("."); + const slash = path.lastIndexOf("/"); + return dot > slash ? path.slice(0, dot) : path; +} + +function baseName(path: string): string { + return stripExtension(path.split("/").at(-1) ?? ""); +} + +/** Resolves `../x` against the importer's directory; no package resolution, only what git shows. */ +function resolveRelative(fromPath: string, specifier: string): string { + const segments = fromPath.split("/").slice(0, -1); + for (const part of specifier.split("/")) { + if (part === "." || part === "") continue; + if (part === "..") segments.pop(); + else segments.push(part); + } + return segments.join("/"); +} + +// `import x from "y"`, `import "y"`, `export … from "y"` and `require("y")` all reduce to a quoted +// specifier preceded by one of three words. +const IMPORT_SPECIFIER = /(?:\bfrom|\bimport|\brequire\s*\()\s*\(?\s*["']([^"']+)["']/g; + +/** Which changed files a file's patch lines import, by path. */ +function importedPaths( + path: string, + lines: ReadonlyArray, + byModulePath: ReadonlyMap, + byBaseName: ReadonlyMap, +): ReadonlySet { + const imported = new Set(); + for (const line of lines) { + IMPORT_SPECIFIER.lastIndex = 0; + let match = IMPORT_SPECIFIER.exec(line); + while (match !== null) { + const specifier = match[1] ?? ""; + const withExtension = specifier.startsWith(".") + ? resolveRelative(path, specifier) + : specifier; + // A specifier may name the extension the module map dropped, and it need not be the one on + // disk: TypeScript's own imports point at the `.js` beside a `.ts`. + const extension = MODULE_EXTENSIONS.find((candidate) => withExtension.endsWith(candidate)); + const resolved = + extension === undefined ? withExtension : withExtension.slice(0, -extension.length); + const target = + byModulePath.get(resolved) ?? + byModulePath.get(`${resolved}/index`) ?? + byBaseName.get(baseName(resolved)); + if (target !== undefined && target !== path) imported.add(target); + match = IMPORT_SPECIFIER.exec(line); + } + } + return imported; +} + +/** + * Source files with what they import ahead of what imports them. + * + * Kahn's, but every step takes the lowest remaining path rather than any ready node, so the same + * change always reads the same way and files of one directory stay together. A cycle leaves nothing + * ready: the lowest path still standing is taken anyway, which is the alphabetical order the tier + * would have had without a graph. + */ +function orderByImports( + paths: ReadonlyArray, + imports: ReadonlyMap>, +): Array { + const remaining = new Set(paths); + const ordered: Array = []; + const sorted = [...paths].sort((left, right) => left.localeCompare(right)); + while (remaining.size > 0) { + const candidates = sorted.filter((path) => remaining.has(path)); + const next = + candidates.find((path) => + [...(imports.get(path) ?? [])].every((dependency) => !remaining.has(dependency)), + ) ?? candidates[0]!; + remaining.delete(next); + ordered.push(next); + } + return ordered; +} + +/** The implementation a test names: `foo.test.ts` and `__tests__/foo.ts` both point at `foo`. */ +function testedBaseName(path: string): string { + return (path.split("/").at(-1) ?? "").replace(/\.(?:test|spec)\..*$/, "").replace(/\.[^.]+$/, ""); +} + +/** + * Diff files in reading order: source in dependency order, then the tests that cover it, then + * whatever a tool wrote. + */ +export function orderDiffFiles( + files: ReadonlyArray, +): ReadonlyArray { + const byPath = new Map(); + for (const file of files) byPath.set(resolveFileDiffPath(file), file); + const tiers = new Map(); + for (const path of byPath.keys()) tiers.set(path, diffFileTier(path)); + + const sourcePaths = [...byPath.keys()].filter((path) => tiers.get(path) === "source"); + const byModulePath = new Map(); + const ambiguousBaseNames = new Set(); + const byBaseName = new Map(); + for (const path of sourcePaths) { + byModulePath.set(stripExtension(path), path); + const base = baseName(path); + if (byBaseName.has(base)) ambiguousBaseNames.add(base); + byBaseName.set(base, path); + } + for (const base of ambiguousBaseNames) byBaseName.delete(base); + + const imports = new Map>(); + for (const path of sourcePaths) { + const file = byPath.get(path)!; + imports.set( + path, + importedPaths(path, [...file.additionLines, ...file.deletionLines], byModulePath, byBaseName), + ); + } + const orderedSource = orderByImports(sourcePaths, imports); + + const sourcePositions = new Map(); + orderedSource.forEach((path, index) => { + const base = baseName(path); + if (!sourcePositions.has(base)) sourcePositions.set(base, index); + }); + const orderedTests = [...byPath.keys()] + .filter((path) => tiers.get(path) === "test") + .sort((left, right) => { + const leftPosition = sourcePositions.get(testedBaseName(left)) ?? Number.MAX_SAFE_INTEGER; + const rightPosition = sourcePositions.get(testedBaseName(right)) ?? Number.MAX_SAFE_INTEGER; + return leftPosition - rightPosition || left.localeCompare(right); + }); + + const orderedGenerated = [...byPath.keys()] + .filter((path) => tiers.get(path) === "generated") + .sort((left, right) => left.localeCompare(right)); + + return [...orderedSource, ...orderedTests, ...orderedGenerated].map((path) => byPath.get(path)!); +} diff --git a/apps/web/src/components/pullRequest/pullRequestList.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestList.logic.test.ts index 11f5f86d420..4fe85851807 100644 --- a/apps/web/src/components/pullRequest/pullRequestList.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestList.logic.test.ts @@ -1,10 +1,16 @@ -import type { PullRequestListEntry } from "@t3tools/contracts"; +import type { EnvironmentId, PullRequestListEntry } from "@t3tools/contracts"; import { describe, expect, it } from "vite-plus/test"; import { filterPullRequestsByInvolvement, + findScopedProject, + mergePullRequestLists, + pullRequestEntryKey, + pullRequestEnvironmentSetKey, groupPullRequestsByInvolvement, + matchesPullRequestFilters, matchesPullRequestQuery, + parsePullRequestQuery, mergePullRequestDiffStats, narrowPullRequestsToFilters, partitionPullRequestsWithPriority, @@ -14,13 +20,17 @@ import { scorePullRequestMatch, withDiffStat, resolveProjectScope, + type EnvironmentPullRequestEntry, } from "./pullRequestList.logic"; const VIEWERS = { "github.com": "Bilal" } as const; const NO_VIEWERS = {} as const; -function entry(overrides: Partial & Pick) { +function entry( + overrides: Partial & Pick, +) { return { + environmentId: "env-1", provider: "github", host: "github.com", projectId: "project-1", @@ -41,7 +51,7 @@ function entry(overrides: Partial & Pick { @@ -208,6 +218,125 @@ describe("carrying rows already read into filters nothing has answered yet", () }); }); +describe("narrowing rows by the filters a host may not have applied", () => { + const rows = [ + entry({ number: 1 }), + entry({ number: 2, isDraft: true }), + entry({ number: 3, reviewDecision: "approved" }), + entry({ + number: 4, + labels: [{ name: "Needs design", color: null }], + author: { login: "Hubot", name: null, avatarUrl: null }, + reviewDecision: "changes-requested", + }), + ]; + const narrow = (filters: Parameters[1]) => + rows.filter((row) => matchesPullRequestFilters(row, filters)).map((row) => row.number); + + it("keeps or drops drafts as asked", () => { + expect(narrow({ draft: "only" })).toEqual([2]); + expect(narrow({ draft: "hide" })).toEqual([1, 3, 4]); + }); + + it("keeps the review state that was asked for, and no reviews means none at all", () => { + expect(narrow({ review: "approved" })).toEqual([3]); + expect(narrow({ review: "changes-requested" })).toEqual([4]); + expect(narrow({ review: "none" })).toEqual([1, 2]); + }); + + it("matches labels and authors however either was capitalized", () => { + expect(narrow({ labels: [["needs DESIGN"]] })).toEqual([4]); + expect(narrow({ excludedLabels: ["needs design"] })).toEqual([1, 2, 3]); + expect(narrow({ author: "hubot" })).toEqual([4]); + }); + + it("takes a group of labels as an either-or", () => { + const sized = [ + entry({ number: 10, labels: [{ name: "size:S", color: null }] }), + entry({ number: 11, labels: [{ name: "size:XS", color: null }] }), + entry({ number: 12, labels: [{ name: "size:L", color: null }] }), + ]; + const kept = (labels: ReadonlyArray>) => + sized.filter((row) => matchesPullRequestFilters(row, { labels })).map((row) => row.number); + + expect(kept([["size:S", "size:XS"]])).toEqual([10, 11]); + expect(kept([["size:XXL"]])).toEqual([]); + }); + + it("takes two groups as an and, each satisfied on its own", () => { + const row = entry({ + number: 20, + labels: [ + { name: "size:S", color: null }, + { name: "bug", color: null }, + ], + }); + expect(matchesPullRequestFilters(row, { labels: [["size:S", "size:XS"], ["bug"]] })).toBe(true); + // The second group holds nothing this row carries, so the first one satisfied is not enough. + expect(matchesPullRequestFilters(row, { labels: [["size:S"], ["wip"]] })).toBe(false); + }); + + it("holds on to every row for a filter no row carries the answer to", () => { + expect(narrow({ checks: "passing" })).toEqual([1, 2, 3, 4]); + }); +}); + +describe("reading qualifiers out of a typed query", () => { + it("keeps quoted values whole and separates negation", () => { + expect(parsePullRequestQuery('label:"needs design" -label:wip Fix the parser')).toEqual({ + text: "Fix the parser", + filters: { labels: [["needs design"]], excludedLabels: ["wip"] }, + }); + }); + + it("reads a comma-separated list as one group either name satisfies", () => { + expect(parsePullRequestQuery("label:size:S,size:XS").filters.labels).toEqual([ + ["size:S", "size:XS"], + ]); + }); + + it("keeps the spaces in a quoted name of a list, and drops an empty part", () => { + expect(parsePullRequestQuery('label:"needs design","wip"').filters.labels).toEqual([ + ["needs design", "wip"], + ]); + expect(parsePullRequestQuery("label:a,,b").filters.labels).toEqual([["a", "b"]]); + }); + + it("excludes every name of a negated list", () => { + expect(parsePullRequestQuery("-label:a,b").filters).toEqual({ excludedLabels: ["a", "b"] }); + }); + + it("makes each label qualifier its own group, so the groups are an AND", () => { + expect(parsePullRequestQuery("label:bug label:a,b").filters.labels).toEqual([ + ["bug"], + ["a", "b"], + ]); + }); + + it("reads the scalar qualifiers in GitHub's own spelling", () => { + expect( + parsePullRequestQuery("author:octocat DRAFT:false review:changes_requested status:failure") + .filters, + ).toEqual({ + author: "octocat", + draft: "hide", + review: "changes-requested", + checks: "failing", + }); + }); + + it("leaves an unknown value and a stray colon as text, and reads an unknown key as a label", () => { + const parsed = parsePullRequestQuery("milestone:v2 draft:maybe status: parser"); + expect(parsed.filters).toEqual({ labels: [["milestone:v2"]] }); + // A known key whose value it does not take is text, so "status:" itself stays findable. + expect(parsed.text).toBe("draft:maybe status: parser"); + }); + + it("answers an empty query with an empty everything", () => { + expect(parsePullRequestQuery(" ")).toEqual({ text: "", filters: {} }); + }); +}); + describe("resolveProjectScope", () => { const projects = [{ id: "p1" }, { id: "p2" }]; @@ -283,7 +412,7 @@ describe("ranking what a search found", () => { }); describe("line counts that arrive after the rows", () => { - const stats = new Map([["project-1 7", { additions: 42, deletions: 3 }]]); + const stats = new Map([["env-1 project-1 7", { additions: 42, deletions: 3 }]]); it("fills in a row whose host left the counts for later", () => { const row = entry({ number: 7, additions: 0, deletions: 0 }); @@ -304,33 +433,33 @@ describe("line counts that arrive after the rows", () => { describe("merging line counts across keyed stats queries", () => { it("keeps counts already held while a fresh batch says nothing about them", () => { const held = mergePullRequestDiffStats(new Map(), [ - { projectId: "project-1", number: 1, additions: 10, deletions: 2 }, - { projectId: "project-1", number: 2, additions: 5, deletions: 1 }, + { environmentId: "env-1", projectId: "project-1", number: 1, additions: 10, deletions: 2 }, + { environmentId: "env-1", projectId: "project-1", number: 2, additions: 5, deletions: 1 }, ]); // A third row appeared; its batch is still pending and contributes nothing yet. const merged = mergePullRequestDiffStats(held, []); - expect(merged.get("project-1 1")).toEqual({ additions: 10, deletions: 2 }); - expect(merged.get("project-1 2")).toEqual({ additions: 5, deletions: 1 }); + expect(merged.get("env-1 project-1 1")).toEqual({ additions: 10, deletions: 2 }); + expect(merged.get("env-1 project-1 2")).toEqual({ additions: 5, deletions: 1 }); }); it("replaces a count once its replacement arrives, keeping its neighbours", () => { const held = mergePullRequestDiffStats(new Map(), [ - { projectId: "project-1", number: 1, additions: 10, deletions: 2 }, + { environmentId: "env-1", projectId: "project-1", number: 1, additions: 10, deletions: 2 }, ]); const merged = mergePullRequestDiffStats(held, [ - { projectId: "project-1", number: 1, additions: 11, deletions: 2 }, - { projectId: "project-1", number: 3, additions: 7, deletions: 0 }, + { environmentId: "env-1", projectId: "project-1", number: 1, additions: 11, deletions: 2 }, + { environmentId: "env-1", projectId: "project-1", number: 3, additions: 7, deletions: 0 }, ]); - expect(merged.get("project-1 1")).toEqual({ additions: 11, deletions: 2 }); - expect(merged.get("project-1 3")).toEqual({ additions: 7, deletions: 0 }); + expect(merged.get("env-1 project-1 1")).toEqual({ additions: 11, deletions: 2 }); + expect(merged.get("env-1 project-1 3")).toEqual({ additions: 7, deletions: 0 }); }); it("does not mutate the map it was handed", () => { - const held = new Map([["project-1 1", { additions: 1, deletions: 1 }]]); + const held = new Map([["env-1 project-1 1", { additions: 1, deletions: 1 }]]); mergePullRequestDiffStats(held, [ - { projectId: "project-1", number: 1, additions: 2, deletions: 2 }, + { environmentId: "env-1", projectId: "project-1", number: 1, additions: 2, deletions: 2 }, ]); - expect(held.get("project-1 1")).toEqual({ additions: 1, deletions: 1 }); + expect(held.get("env-1 project-1 1")).toEqual({ additions: 1, deletions: 1 }); }); }); @@ -443,3 +572,254 @@ describe("the list snapshot across a reload", () => { expect(readPullRequestListSnapshot(undefined, "env-1")).toBeNull(); }); }); + +const ENV_1 = "env-1" as EnvironmentId; +const ENV_2 = "env-2" as EnvironmentId; + +describe("merging the environments' own listings", () => { + const answer = ( + overrides: Partial[0][number][1]> = {}, + ) => + ({ + viewers: { "github.com": "Bilal" }, + providers: [ + { + host: "github.com", + kind: "github", + searchesOnHost: true, + projectCount: 1, + configured: true, + detail: null, + }, + ], + entries: [], + errors: [], + truncated: false, + nextCursors: {}, + ...overrides, + }) as Parameters[0][number][1]; + + it("answers nothing until an environment has", () => { + expect(mergePullRequestLists([])).toBeNull(); + }); + + it("tags every row with the environment that read it, newest first", () => { + const merged = mergePullRequestLists([ + [ENV_1, answer({ entries: [entry({ number: 1, updatedAt: "2026-07-01T00:00:00Z" })] })], + [ENV_2, answer({ entries: [entry({ number: 2, updatedAt: "2026-08-01T00:00:00Z" })] })], + ]); + expect(merged?.entries.map((row) => [row.environmentId, row.number])).toEqual([ + [ENV_2, 2], + [ENV_1, 1], + ]); + }); + + it("tells two environments' copies of one pull request apart", () => { + const row = entry({ number: 4 }); + expect(pullRequestEntryKey({ ...row, environmentId: ENV_1 })).not.toBe( + pullRequestEntryKey({ ...row, environmentId: ENV_2 }), + ); + }); + + it("folds a host reached from two environments into one switcher row", () => { + const merged = mergePullRequestLists([ + [ENV_1, answer()], + [ + ENV_2, + answer({ + providers: [ + { + host: "github.com", + kind: "github", + searchesOnHost: false, + projectCount: 2, + configured: false, + detail: "Not signed in.", + }, + ], + }), + ], + ]); + // Readable because one environment could read it, but narrowed locally because the other + // answers unsearched. + expect(merged?.providers).toEqual([ + { + host: "github.com", + kind: "github", + searchesOnHost: false, + projectCount: 3, + configured: true, + detail: null, + }, + ]); + }); + + it("keeps each environment's continuation to itself", () => { + const merged = mergePullRequestLists([ + [ENV_1, answer({ nextCursors: { "github.com pingdotgg/t3code": "cursor-1" } })], + [ENV_2, answer()], + ]); + expect(merged?.nextCursors).toEqual({ + [ENV_1]: { "github.com pingdotgg/t3code": "cursor-1" }, + }); + }); + + it("reads the same key whichever order the environments connected in", () => { + expect(pullRequestEnvironmentSetKey(["env-2", "env-1"])).toBe( + pullRequestEnvironmentSetKey(["env-1", "env-2"]), + ); + }); +}); + +describe('who "I" am, per server', () => { + const answer = (viewers: Record, entries: ReadonlyArray) => + ({ + viewers, + providers: [], + entries, + errors: [], + truncated: false, + nextCursors: {}, + }) as Parameters[0][number][1]; + const byBilal = { login: "Bilal", name: null, avatarUrl: null }; + + it("does not let one server's account decide who authored another server's rows", () => { + // Both servers reach github.com, signed in as different people. Folded into one host-keyed + // record, whichever answered last spoke for both — and every row of the reader's own work + // was filed under Others. + const merged = mergePullRequestLists([ + [ENV_1, answer({ "github.com": "Bilal" }, [entry({ number: 1, author: byBilal })])], + [ENV_2, answer({ "github.com": "Octocat" }, [entry({ number: 2, author: byBilal })])], + ])!; + + const groups = groupPullRequestsByInvolvement(merged.entries, merged.viewers); + expect( + groups.find((group) => group.key === "authored")?.entries.map((row) => row.number), + ).toEqual([1]); + expect( + filterPullRequestsByInvolvement(merged.entries, merged.viewers, "authored").map( + (row) => row.number, + ), + ).toEqual([1]); + }); + + it("still reads a single server's host-keyed viewers, which is what a snapshot carries", () => { + expect( + filterPullRequestsByInvolvement( + [entry({ number: 1, author: byBilal })], + { "github.com": "Bilal" }, + "authored", + ).map((row) => row.number), + ).toEqual([1]); + }); + + it("names the servers with more rows and no cursor to reach them by", () => { + const merged = mergePullRequestLists([ + [ + ENV_1, + { ...answer({}, []), truncated: true, nextCursors: { "github.com acme/web": "cursor-1" } }, + ], + [ENV_2, { ...answer({}, []), truncated: true }], + ])!; + + expect(merged.truncatedEnvironments).toEqual([ENV_1, ENV_2]); + expect(Object.keys(merged.nextCursors)).toEqual([ENV_1]); + }); +}); + +describe("the project an id names", () => { + const projects = [ + { id: "project-1", environmentId: ENV_1 }, + { id: "project-1", environmentId: ENV_2 }, + { id: "project-2", environmentId: ENV_2 }, + ]; + + it("takes the server it was given", () => { + expect(findScopedProject(projects, ENV_2, "project-1")?.environmentId).toBe(ENV_2); + }); + + it("answers a bare id only where one server has it", () => { + expect(findScopedProject(projects, null, "project-2")?.environmentId).toBe(ENV_2); + // Two servers hold this id: narrowing to either would be a coin toss the reader cannot see. + expect(findScopedProject(projects, null, "project-1")).toBeUndefined(); + }); + + it("answers nothing for a server that does not have it", () => { + expect(findScopedProject(projects, ENV_1, "project-2")).toBeUndefined(); + }); +}); + +describe("colon-namespaced labels typed as a search", () => { + it("reads an unknown key as the label it almost always is", () => { + expect(parsePullRequestQuery("size:XXL").filters.labels).toEqual([["size:XXL"]]); + expect(parsePullRequestQuery("vouch:trusted").filters.labels).toEqual([["vouch:trusted"]]); + expect(parsePullRequestQuery("size:XXL").text).toBe(""); + }); + + it("reads the key as the namespace of every bare name in its list", () => { + expect(parsePullRequestQuery("size:S,XS").filters.labels).toEqual([["size:S", "size:XS"]]); + }); + + it("leaves a name that already carries its own namespace alone", () => { + // `size:S,size:XS` is the same pair written out; prefixing again would ask for `size:size:XS`. + expect(parsePullRequestQuery("size:S,size:XS").filters.labels).toEqual([["size:S", "size:XS"]]); + }); + + it("excludes one the same way", () => { + expect(parsePullRequestQuery("-size:XXL").filters.excludedLabels).toEqual(["size:XXL"]); + expect(parsePullRequestQuery("-size:S,XS").filters.excludedLabels).toEqual([ + "size:S", + "size:XS", + ]); + }); + + it("keeps a quoted token as text, which is the way back to a literal search", () => { + const parsed = parsePullRequestQuery('"size:XXL"'); + expect(parsed.filters.labels).toBeUndefined(); + expect(parsed.text).toBe('"size:XXL"'); + }); + + it("leaves a pasted link alone rather than naming a label after its scheme", () => { + const parsed = parsePullRequestQuery("https://github.com/pingdotgg/t3code/pull/1"); + expect(parsed.filters.labels).toBeUndefined(); + expect(parsed.text).toBe("https://github.com/pingdotgg/t3code/pull/1"); + }); + + it("mixes with the keys it does know, and with plain words", () => { + const parsed = parsePullRequestQuery("area:web draft:true wizard label:bug"); + expect(parsed.filters).toEqual({ labels: [["area:web"], ["bug"]], draft: "only" }); + expect(parsed.text).toBe("wizard"); + }); + + it("shares the ceiling with the labels typed as labels", () => { + const many = Array.from({ length: 12 }, (_, index) => `area:${index}`).join(" "); + expect(parsePullRequestQuery(many).filters.labels).toHaveLength(10); + }); +}); + +describe("the priority groups against a paginated feed", () => { + const authoredRow = (number: number, updatedAt: string) => + entry({ number, updatedAt, author: { login: "Bilal", name: null, avatarUrl: null } }); + + it("shows every authored row the host reported, not only the ones the feed page holds", () => { + // The feed is one page ordered by recency, so on a busy repository it can hold exactly one of + // somebody's own pull requests while the rest sit further down the host's list. The Authored + // group comes from its own server-filtered read for that reason: grouping the page instead + // leaves the older ones under Others, or off the page altogether. + const newest = authoredRow(6039, "2026-08-11T08:00:00Z"); + const older = [ + authoredRow(5499, "2026-08-10T17:00:00Z"), + authoredRow(5544, "2026-08-10T16:00:00Z"), + ]; + const feed = [newest, entry({ number: 6123, updatedAt: "2026-08-11T09:00:00Z" })]; + + const groups = partitionPullRequestsWithPriority(feed, [newest, ...older], []); + + expect( + groups.find((group) => group.key === "authored")?.entries.map((row) => row.number), + ).toEqual([6039, 5499, 5544]); + expect( + groups.find((group) => group.key === "others")?.entries.map((row) => row.number), + ).toEqual([6123]); + }); +}); diff --git a/apps/web/src/components/pullRequest/pullRequestList.logic.ts b/apps/web/src/components/pullRequest/pullRequestList.logic.ts index 05900444764..95693382ed7 100644 --- a/apps/web/src/components/pullRequest/pullRequestList.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestList.logic.ts @@ -1,19 +1,49 @@ import * as Schema from "effect/Schema"; -import { PullRequestListEntry, PullRequestListResult } from "@t3tools/contracts"; -import type { PullRequestInvolvement, PullRequestListState } from "@t3tools/contracts"; +import { EnvironmentId, PullRequestListEntry, PullRequestListResult } from "@t3tools/contracts"; +import type { + PullRequestDiffStat, + PullRequestInvolvement, + PullRequestListCursors, + PullRequestListFilters, + PullRequestListState, +} from "@t3tools/contracts"; + +/** + * A listed change request with the environment that read it. Nothing on a row says which machine + * it came from, and the page unions every connected one — so acting on a row, refreshing it, or + * opening its detail all need the tag the listing itself does not carry. + */ +export interface EnvironmentPullRequestEntry extends PullRequestListEntry { + readonly environmentId: EnvironmentId; +} + +export interface EnvironmentPullRequestStat extends PullRequestDiffStat { + readonly environmentId: EnvironmentId; +} export type PullRequestGroupKey = "reviewRequested" | "authored" | "others"; -export interface PullRequestGroup { +export interface PullRequestGroup { readonly key: PullRequestGroupKey; readonly label: string; - readonly entries: ReadonlyArray; + readonly entries: ReadonlyArray; } -/** The signed-in account per host, as the listing reports it. */ +/** + * The signed-in account per host. Keyed `" "` once a listing spans more than + * one environment: two machines can both reach github.com signed in as different people, and a + * single host-keyed record would let whichever answered last decide who "I" am — which is how + * every row of somebody's own work came to be filed under Others. + */ export type PullRequestViewers = PullRequestListResult["viewers"]; +/** A row plus the environment that read it, where the caller has one to give. */ +type ScopedEntry = PullRequestListEntry & { readonly environmentId?: string }; + +export const pullRequestViewerKey = (entry: ScopedEntry): string => + `${entry.environmentId ?? ""} ${entry.host}`; + const GROUP_LABELS: Record = { reviewRequested: "Review requested", authored: "Authored", @@ -30,11 +60,143 @@ function normalize(value: string | null | undefined): string | null { * GitHub, GitLab and a GitHub Enterprise install, and the account that owns one says nothing * about the others. */ -function isAuthoredByViewer(entry: PullRequestListEntry, viewers: PullRequestViewers): boolean { - const viewer = normalize(viewers[entry.host]); +function isAuthoredByViewer(entry: ScopedEntry, viewers: PullRequestViewers): boolean { + // The environment's own answer first; a plain host key is what a single-environment listing + // still writes, and what the snapshot from one carries. + const viewer = normalize(viewers[pullRequestViewerKey(entry)] ?? viewers[entry.host]); return viewer !== null && normalize(entry.author?.login) === viewer; } +/** What `review:` and `status:` take, in GitHub's spelling and in the contract's. */ +const REVIEW_VALUES: Record = { + approved: "approved", + changes_requested: "changes-requested", + "changes-requested": "changes-requested", + required: "review-required", + "review-required": "review-required", + none: "none", +}; +const CHECKS_VALUES: Record = { + success: "passing", + passing: "passing", + failure: "failing", + failing: "failing", +}; + +/** + * One token of a typed query: a run of non-space characters in which a quoted stretch counts as + * part of the token, so `label:"needs design"` stays whole. An unbalanced quote is dropped rather + * than swallowing the rest of the line. + */ +const QUERY_TOKEN = /(?:[^\s"]|"[^"]*")+/g; +/** The contract's own ceiling on a qualifier list, past which further ones are ignored. */ +const MAX_QUALIFIER_VALUES = 10; + +function qualifierValue(raw: string): string { + return raw.replaceAll('"', "").trim(); +} + +/** A qualifier's value as the list it may be: split on commas, each name unquoted on its own. */ +function splitQualifierList(raw: string): string[] { + return raw + .split(",") + .map((part) => qualifierValue(part)) + .filter((part) => part.length > 0); +} + +/** + * A typed query split into the qualifiers the hosts can act on and the text that is left. Written + * GitHub's way — `label:foo`, `-label:"needs design"`, `author:octocat`, `draft:true`, + * `review:approved`, `status:success` — because a project's labels are its own and a menu cannot + * list them. + * + * A key this does not know is read as a label of that whole name: repositories namespace their + * labels with a colon — `size:XXL`, `area:web`, `vouch:trusted` — and someone typing one means + * the label, not a description that happens to contain it. `-size:XXL` excludes the same way. + * + * Quoting is the way back to plain text: `"size:XXL"` is searched for as written, which is what + * makes a literal search for a colon still possible. A known key whose value it does not take is + * text too, so a search for "status:" itself is still findable. + */ +export function parsePullRequestQuery(raw: string): { + readonly text: string; + readonly filters: PullRequestListFilters; +} { + const text: string[] = []; + const labels: string[][] = []; + const excludedLabels: string[] = []; + let author: string | undefined; + let draft: PullRequestListFilters["draft"]; + let review: PullRequestListFilters["review"]; + let checks: PullRequestListFilters["checks"]; + for (const [token] of raw.matchAll(QUERY_TOKEN)) { + const qualifier = /^(-?)([A-Za-z]+):(.*)$/.exec(token); + const value = qualifier === null ? "" : qualifierValue(qualifier[3] ?? ""); + const negated = qualifier?.[1] === "-"; + switch (value.length === 0 ? "" : (qualifier?.[2]?.toLowerCase() ?? "")) { + case "label": { + // GitHub's own OR: `label:a,b` is one qualifier satisfied by either name. Negated, the + // comma excludes each — a row carrying any of them goes. + const names = splitQualifierList(qualifier?.[3] ?? ""); + if (negated) excludedLabels.push(...names); + else labels.push(names); + continue; + } + case "author": + if (negated) break; + author = value; + continue; + case "draft": + if (negated || (value.toLowerCase() !== "true" && value.toLowerCase() !== "false")) break; + draft = value.toLowerCase() === "true" ? "only" : "hide"; + continue; + case "review": { + const decision = negated ? undefined : REVIEW_VALUES[value.toLowerCase()]; + if (decision === undefined) break; + review = decision; + continue; + } + case "status": + case "checks": { + const state = negated ? undefined : CHECKS_VALUES[value.toLowerCase()]; + if (state === undefined) break; + checks = state; + continue; + } + case "": + break; + default: + // An unknown key, read as the namespaced label it almost always is. A pasted link is + // not one — `https://…` would otherwise become a label named after its own scheme. + if (!value.startsWith("/")) { + // The key names the namespace, so the bare parts of `size:S,XS` are both sizes. A part + // that already carries a colon names its whole label — `size:S,size:XS` is the same + // pair written out, and prefixing it again would ask for `size:size:XS`. + const names = splitQualifierList(qualifier?.[3] ?? "").map((name) => + name.includes(":") ? name : `${qualifier?.[2] ?? ""}:${name}`, + ); + if (negated) excludedLabels.push(...names); + else labels.push(names); + continue; + } + } + text.push(token); + } + return { + text: text.join(" "), + filters: { + ...(labels.length === 0 ? {} : { labels: labels.slice(0, MAX_QUALIFIER_VALUES) }), + ...(excludedLabels.length === 0 + ? {} + : { excludedLabels: excludedLabels.slice(0, MAX_QUALIFIER_VALUES) }), + ...(author === undefined ? {} : { author }), + ...(draft === undefined ? {} : { draft }), + ...(review === undefined ? {} : { review }), + ...(checks === undefined ? {} : { checks }), + }, + }; +} + /** Free-text filter over the fields a row actually shows, plus `#123` / `123`. */ export function matchesPullRequestQuery(entry: PullRequestListEntry, query: string): boolean { const normalizedQuery = query.trim().toLowerCase(); @@ -48,11 +210,11 @@ export function matchesPullRequestQuery(entry: PullRequestListEntry, query: stri * The server returns the involvement superset for a state, so switching between the Reviewing * and Authored tabs never waits on the network. */ -export function filterPullRequestsByInvolvement( - entries: ReadonlyArray, +export function filterPullRequestsByInvolvement( + entries: ReadonlyArray, viewers: PullRequestViewers, involvement: PullRequestInvolvement, -): ReadonlyArray { +): ReadonlyArray { if (involvement === "reviewing") { return entries.filter((entry) => entry.viewerReviewRequested); } @@ -75,14 +237,14 @@ export function filterPullRequestsByInvolvement( * it needs to know who is signed in on each host, and the search text because searching is the * hosts' own answer; both are narrowed where that knowledge already lives. */ -export function narrowPullRequestsToFilters( - entries: ReadonlyArray, +export function narrowPullRequestsToFilters( + entries: ReadonlyArray, filters: { readonly state: PullRequestListState; readonly projectId: string | undefined; readonly host: string | undefined; }, -): ReadonlyArray { +): ReadonlyArray { return entries.filter( (entry) => (filters.state === "all" || entry.state === filters.state) && @@ -91,15 +253,41 @@ export function narrowPullRequestsToFilters( ); } +/** + * The further narrowings over rows that have already arrived, for the hosts that could not apply + * them themselves and for the moment before an answer that did lands. + * + * `checks` is absent because no listed row carries its check state: that one filter is the + * host's alone, and a row a host did not narrow stays rather than being guessed at. + */ +export function matchesPullRequestFilters( + entry: PullRequestListEntry, + filters: PullRequestListFilters, +): boolean { + const labels = entry.labels.map((label) => label.name.trim().toLowerCase()); + const holds = (label: string) => labels.includes(label.trim().toLowerCase()); + return ( + (filters.draft === undefined || entry.isDraft === (filters.draft === "only")) && + (filters.review === undefined || + (filters.review === "none" + ? entry.reviewDecision === undefined + : entry.reviewDecision === filters.review)) && + (filters.labels === undefined || filters.labels.every((group) => group.some(holds))) && + (filters.excludedLabels === undefined || !filters.excludedLabels.some(holds)) && + (filters.author === undefined || + entry.author?.login.toLowerCase() === filters.author.trim().toLowerCase()) + ); +} + /** * Only relationships the list data actually carries: no "previously reviewed" bucket is * inferred, because the listing has no review history. */ -export function groupPullRequestsByInvolvement( - entries: ReadonlyArray, +export function groupPullRequestsByInvolvement( + entries: ReadonlyArray, viewers: PullRequestViewers, -): ReadonlyArray { - const buckets: Record = { +): ReadonlyArray> { + const buckets: Record = { reviewRequested: [], authored: [], others: [], @@ -118,9 +306,14 @@ export function groupPullRequestsByInvolvement( .map((key) => ({ key, label: GROUP_LABELS[key], entries: buckets[key] })); } -/** Repository plus number is unique on one host, so the host makes the key unique overall. */ -export function pullRequestEntryKey(entry: PullRequestListEntry): string { - return `${entry.host}:${entry.repository}#${entry.number}`; +/** + * Repository plus number is unique on one host, so the host makes the key unique overall — and + * the environment on top of that, because two connected machines can hold the same repository and + * would otherwise contribute two rows under one key. + */ +export function pullRequestEntryKey(entry: ScopedEntry): string { + const scope = entry.environmentId === undefined ? "" : `${entry.environmentId}:`; + return `${scope}${entry.host}:${entry.repository}#${entry.number}`; } /** @@ -131,11 +324,11 @@ export function pullRequestEntryKey(entry: PullRequestListEntry): string { * server-filtered reads, the feed fills "Others" in its own order, and a continuation can only * append — a row it carries that a partition already holds is dropped rather than moved. */ -export function partitionPullRequestsWithPriority( - entries: ReadonlyArray, - authored: ReadonlyArray, - reviewRequested: ReadonlyArray, -): ReadonlyArray { +export function partitionPullRequestsWithPriority( + entries: ReadonlyArray, + authored: ReadonlyArray, + reviewRequested: ReadonlyArray, +): ReadonlyArray> { const authoredByKey = new Map(authored.map((entry) => [pullRequestEntryKey(entry), entry])); // A row can be both authored and review-requested; authored wins, as the local grouping has it. const reviewByKey = new Map( @@ -144,7 +337,7 @@ export function partitionPullRequestsWithPriority( return authoredByKey.has(key) ? [] : [[key, entry] as const]; }), ); - const others: PullRequestListEntry[] = []; + const others: Entry[] = []; for (const entry of entries) { const key = pullRequestEntryKey(entry); // The feed's copy of a partitioned row is at least as fresh — it replaces in place. @@ -156,8 +349,7 @@ export function partitionPullRequestsWithPriority( others.push(entry); } } - const byRecency = (left: PullRequestListEntry, right: PullRequestListEntry) => - right.updatedAt.localeCompare(left.updatedAt); + const byRecency = (left: Entry, right: Entry) => right.updatedAt.localeCompare(left.updatedAt); return ( [ { key: "reviewRequested", entries: [...reviewByKey.values()].toSorted(byRecency) }, @@ -183,6 +375,7 @@ export type PullRequestDiffStats = ReadonlyMap< export function mergePullRequestDiffStats( previous: PullRequestDiffStats, stats: ReadonlyArray<{ + readonly environmentId: string; readonly projectId: string; readonly number: number; readonly additions: number; @@ -192,20 +385,109 @@ export function mergePullRequestDiffStats( if (stats.length === 0) return previous; const next = new Map(previous); for (const stat of stats) { - next.set(`${stat.projectId} ${stat.number}`, { - additions: stat.additions, - deletions: stat.deletions, - }); + next.set(diffStatKey(stat), { additions: stat.additions, deletions: stat.deletions }); } return next; } +/** A project id only names a project within its own environment, so the key carries both. */ +const diffStatKey = (row: { + readonly environmentId: string; + readonly projectId: string; + readonly number: number; +}) => `${row.environmentId} ${row.projectId} ${row.number}`; + +/** + * Every connected environment's listing, read as one list. + * + * `nextCursors` is keyed by environment rather than by repository: a cursor only means anything + * to the host that issued it, and two machines can hold the same repository. `viewers` is not — + * a host names one account, and the same host reached from two machines is the same account. + */ +export interface MergedPullRequestList { + /** Keyed `" "`, so one host's two accounts stay two accounts. */ + readonly viewers: PullRequestViewers; + readonly providers: PullRequestListResult["providers"]; + readonly entries: ReadonlyArray; + readonly errors: PullRequestListResult["errors"]; + readonly truncated: boolean; + readonly nextCursors: Readonly>; + /** + * The environments with rows still on their hosts. Those with a cursor are continued from it; + * the rest can only be reached by asking them for a longer page, and are named here so that + * asking still happens once every other environment has run out of cursors. + */ + readonly truncatedEnvironments: ReadonlyArray; +} + +/** + * The environments' answers folded into one. A host reached from more than one environment is one + * row in the switcher, readable if any environment could read it and searched on the host only if + * every one of them did — a host answering unnarrowed anywhere still needs the local pass. + */ +export function mergePullRequestLists( + answers: ReadonlyArray, +): MergedPullRequestList | null { + if (answers.length === 0) return null; + const viewers: Record = {}; + const truncatedEnvironments: string[] = []; + const providers = new Map(); + const entries: EnvironmentPullRequestEntry[] = []; + const errors: Array = []; + const nextCursors: Record = {}; + let truncated = false; + for (const [environmentId, answer] of answers) { + for (const [host, login] of Object.entries(answer.viewers)) { + viewers[`${environmentId} ${host}`] = login; + } + for (const provider of answer.providers) { + const held = providers.get(provider.host); + providers.set( + provider.host, + held === undefined + ? provider + : { + ...(held.configured ? held : provider), + projectCount: held.projectCount + provider.projectCount, + searchesOnHost: held.searchesOnHost && provider.searchesOnHost, + configured: held.configured || provider.configured, + }, + ); + } + entries.push(...answer.entries.map((entry) => ({ ...entry, environmentId }))); + errors.push(...answer.errors); + truncated ||= answer.truncated; + if (answer.truncated) truncatedEnvironments.push(environmentId); + if (Object.keys(answer.nextCursors).length > 0) { + nextCursors[environmentId] = answer.nextCursors; + } + } + return { + viewers, + providers: [...providers.values()], + entries: entries.toSorted((left, right) => right.updatedAt.localeCompare(left.updatedAt)), + errors, + truncated, + nextCursors, + truncatedEnvironments, + }; +} + /** One page is what the list itself starts with, and all a cold start needs to look warm. */ const SNAPSHOT_MAX_ENTRIES = 99; type SnapshotStorage = Pick; -const snapshotStorageKey = (environmentId: string) => `t3.pullRequests.list:${environmentId}`; +/** + * Keyed by the whole set of environments the list was read from: connecting or dropping one + * changes which rows belong on the page, and a snapshot taken from a different set would + * hydrate rows no longer being read. + */ +export const pullRequestEnvironmentSetKey = (environmentIds: ReadonlyArray): string => + [...environmentIds].sort((left, right) => left.localeCompare(right)).join(","); + +const snapshotStorageKey = (environmentSetKey: string) => + `t3.pullRequests.list:${environmentSetKey}`; /** * The priority groups' own server-filtered answers, carried with the feed. An authored pull @@ -213,13 +495,13 @@ const snapshotStorageKey = (environmentId: string) => `t3.pullRequests.list:${en * cold-starts into an Authored group missing exactly the rows that made it worth having. */ export interface PullRequestPartitionsSnapshot { - readonly authored: ReadonlyArray; - readonly reviewing: ReadonlyArray; + readonly authored: ReadonlyArray; + readonly reviewing: ReadonlyArray; } export interface PullRequestListSnapshot { readonly scope: string; - readonly data: PullRequestListResult; + readonly data: MergedPullRequestList; readonly partitions?: PullRequestPartitionsSnapshot | undefined; } @@ -229,15 +511,26 @@ export interface PullRequestListSnapshot { * otherwise crash the list on every reload until the key is cleared. A snapshot from before a * schema change is rejected the same way, which is exactly the cold start it would have broken. */ +const EnvironmentPullRequestEntrySchema = Schema.Struct({ + ...PullRequestListEntry.fields, + environmentId: EnvironmentId, +}); + const decodeSnapshot = Schema.decodeUnknownOption( Schema.Struct({ scope: Schema.String, - data: PullRequestListResult, + data: Schema.Struct({ + ...PullRequestListResult.fields, + entries: Schema.Array(EnvironmentPullRequestEntrySchema), + // Per environment here, unlike the wire shape, which is per repository within one. + nextCursors: Schema.Record(Schema.String, PullRequestListResult.fields.nextCursors), + truncatedEnvironments: Schema.Array(Schema.String), + }), // Optional so a snapshot written before the partitions existed still hydrates the feed. partitions: Schema.optional( Schema.Struct({ - authored: Schema.Array(PullRequestListEntry), - reviewing: Schema.Array(PullRequestListEntry), + authored: Schema.Array(EnvironmentPullRequestEntrySchema), + reviewing: Schema.Array(EnvironmentPullRequestEntrySchema), }), ), }), @@ -252,10 +545,10 @@ const decodeSnapshot = Schema.decodeUnknownOption( */ export function readPullRequestListSnapshot( storage: SnapshotStorage | undefined, - environmentId: string, + environmentSetKey: string, ): PullRequestListSnapshot | null { try { - const raw = storage?.getItem(snapshotStorageKey(environmentId)); + const raw = storage?.getItem(snapshotStorageKey(environmentSetKey)); if (!raw) return null; const decoded = decodeSnapshot(JSON.parse(raw)); return decoded._tag === "Some" ? decoded.value : null; @@ -266,12 +559,12 @@ export function readPullRequestListSnapshot( export function writePullRequestListSnapshot( storage: SnapshotStorage | undefined, - environmentId: string, + environmentSetKey: string, snapshot: PullRequestListSnapshot, ): void { try { storage?.setItem( - snapshotStorageKey(environmentId), + snapshotStorageKey(environmentSetKey), JSON.stringify({ scope: snapshot.scope, data: { @@ -281,6 +574,8 @@ export function writePullRequestListSnapshot( // position in a listing the host has long since forgotten. errors: [], nextCursors: {}, + // Where a listing stopped is as stale as the cursor that named it. + truncatedEnvironments: [], }, ...(snapshot.partitions === undefined ? {} @@ -316,6 +611,27 @@ export function resolveProjectScope( return projects.some((project) => project.id === projectId) ? projectId : undefined; } +/** + * The project an id names, on the server that owns it. A project id is only unique within its own + * environment, so an id alone can name two projects on two connected machines: with a server in + * hand the answer is exact, and without one it is only given where a single environment has that + * id — narrowing to the wrong machine reads an empty list nobody asked for. + */ +export function findScopedProject< + Project extends { readonly id: string; readonly environmentId: string }, +>( + projects: ReadonlyArray, + environmentId: string | null | undefined, + projectId: string | undefined, +): Project | undefined { + if (projectId === undefined) return undefined; + const matches = projects.filter((project) => project.id === projectId); + if (environmentId === null || environmentId === undefined) { + return matches.length === 1 ? matches[0] : undefined; + } + return matches.find((project) => project.environmentId === environmentId); +} + /** * How well a row answers the text that was searched for, as a number to order by. * @@ -353,10 +669,10 @@ export function scorePullRequestMatch(entry: PullRequestListEntry, query: string * Search results in the order they answer the question, most convincing first, and by recency * among equals. Only for a search: without one, a listing is a timeline and recency is the order. */ -export function rankPullRequestMatches( - entries: ReadonlyArray, +export function rankPullRequestMatches( + entries: ReadonlyArray, query: string, -): ReadonlyArray { +): ReadonlyArray { if (query.trim().length === 0) return entries; return entries.toSorted((left, right) => { const byScore = scorePullRequestMatch(right, query) - scorePullRequestMatch(left, query); @@ -369,11 +685,13 @@ export function rankPullRequestMatches( * listing that carried them is not second-guessed — and only where they have arrived, since a row * draws perfectly well without them in the meantime. */ -export function withDiffStat( - entry: PullRequestListEntry, +export function withDiffStat< + Entry extends PullRequestListEntry & { readonly environmentId: string }, +>( + entry: Entry, statsByRow: ReadonlyMap, -): PullRequestListEntry { +): Entry { if (entry.additions !== 0 || entry.deletions !== 0) return entry; - const stat = statsByRow.get(`${entry.projectId} ${entry.number}`); + const stat = statsByRow.get(diffStatKey(entry)); return stat === undefined ? entry : { ...entry, ...stat }; } diff --git a/apps/web/src/components/pullRequest/pullRequestPresentation.tsx b/apps/web/src/components/pullRequest/pullRequestPresentation.tsx index 28e952015ad..3704002924b 100644 --- a/apps/web/src/components/pullRequest/pullRequestPresentation.tsx +++ b/apps/web/src/components/pullRequest/pullRequestPresentation.tsx @@ -2,12 +2,14 @@ import type { PullRequestActor, PullRequestCheck, PullRequestCheckStatus, + PullRequestChecksState, PullRequestMergeability, PullRequestState, } from "@t3tools/contracts"; import { CircleCheckIcon, CircleDashedIcon, + CircleDotIcon, CircleXIcon, GitMergeIcon, GitPullRequestClosedIcon, @@ -144,6 +146,51 @@ export function PullRequestCheckStatusIcon({ status }: { status: PullRequestChec ); } +/** + * The rollup a listing row carries, which is one word rather than the checks behind it. The + * headline is GitHub's own wording, so a reader who knows that page reads this one the same way. + */ +const CHECKS_STATE_PRESENTATION = { + passing: { + label: "All checks have passed", + Icon: CircleCheckIcon, + toneClassName: "text-emerald-600 dark:text-emerald-300/90", + }, + failing: { + label: "Some checks were not successful", + Icon: CircleXIcon, + toneClassName: "text-destructive", + }, + pending: { + label: "Some checks haven't completed yet", + Icon: CircleDotIcon, + toneClassName: "text-amber-600 dark:text-amber-400/90", + }, +} as const satisfies Record< + PullRequestChecksState, + { label: string; Icon: typeof CircleCheckIcon; toneClassName: string } +>; + +export function pullRequestChecksStatePresentation(state: PullRequestChecksState) { + return CHECKS_STATE_PRESENTATION[state]; +} + +/** + * The same rollup the server sends with a listing row, worked out here from the checks a detail + * already holds — so the header shows the icon without a second field travelling with it. + * + * Null for a change request with no checks: nothing to show beats a tick nobody earned. + */ +export function pullRequestChecksState( + checks: ReadonlyArray, +): PullRequestChecksState | null { + if (checks.length === 0) return null; + const statuses = checks.map((check) => check.status); + if (statuses.includes("failure") || statuses.includes("cancelled")) return "failing"; + if (statuses.includes("pending")) return "pending"; + return statuses.includes("success") ? "passing" : null; +} + export function PullRequestActorAvatar({ actor, className, diff --git a/apps/web/src/components/pullRequest/pullRequestProjectAssignment.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestProjectAssignment.logic.test.ts new file mode 100644 index 00000000000..3ee59a5b046 --- /dev/null +++ b/apps/web/src/components/pullRequest/pullRequestProjectAssignment.logic.test.ts @@ -0,0 +1,247 @@ +import type { EnvironmentId, ProjectId } from "@t3tools/contracts"; +import { describe, expect, it } from "vite-plus/test"; + +import { + assignProjectsToEnvironments, + resolvePickableEnvironments, + type AssignableProject, +} from "./pullRequestProjectAssignment.logic"; + +const project = ( + id: string, + environmentId: string, + canonicalKey?: string, +): AssignableProject & { workspaceRoot: string } => ({ + id: id as ProjectId, + environmentId: environmentId as EnvironmentId, + repositoryIdentity: canonicalKey === undefined ? null : { canonicalKey }, + workspaceRoot: `/srv/${environmentId}/${id}`, +}); + +const envs = (...ids: ReadonlyArray) => ids as ReadonlyArray; + +const plain = (assignment: Map) => + Object.fromEntries([...assignment].map(([id, projectIds]) => [id, projectIds])); + +describe("one server per repository", () => { + it("lets the first server list a repository both hold", () => { + const assignment = assignProjectsToEnvironments( + [ + project("a1", "env-1", "github.com/acme/app"), + project("b1", "env-1", "github.com/acme/tools"), + project("a2", "env-2", "github.com/acme/app"), + project("c2", "env-2", "github.com/acme/site"), + ], + envs("env-1", "env-2"), + "env-1" as EnvironmentId, + ); + expect(plain(assignment)).toEqual({ "env-1": ["a1", "b1"], "env-2": ["c2"] }); + }); + + it("prefers the named server over the first one", () => { + const assignment = assignProjectsToEnvironments( + [ + project("a1", "env-1", "github.com/acme/app"), + project("a2", "env-2", "github.com/acme/app"), + ], + envs("env-1", "env-2"), + "env-2" as EnvironmentId, + ); + expect(plain(assignment)).toEqual({ "env-2": ["a2"] }); + }); + + it("drops a server with nothing of its own", () => { + const assignment = assignProjectsToEnvironments( + [ + project("a1", "env-1", "github.com/acme/app"), + project("a2", "env-2", "github.com/acme/app"), + ], + envs("env-1", "env-2"), + "env-1" as EnvironmentId, + ); + expect(assignment.has("env-2" as EnvironmentId)).toBe(false); + }); + + it("keeps every copy of a project that has no identity to compare", () => { + const assignment = assignProjectsToEnvironments( + [project("p1", "env-1"), project("p2", "env-2")], + envs("env-1", "env-2"), + "env-1" as EnvironmentId, + ); + expect(plain(assignment)).toEqual({ "env-1": ["p1"], "env-2": ["p2"] }); + }); + + it("keeps a repository listed by the one server that holds it", () => { + const assignment = assignProjectsToEnvironments( + [ + project("a1", "env-1", "github.com/acme/app"), + project("b2", "env-2", "gitlab.com/acme/app"), + ], + envs("env-1", "env-2"), + "env-1" as EnvironmentId, + ); + expect(plain(assignment)).toEqual({ "env-1": ["a1"], "env-2": ["b2"] }); + }); + + it("keeps a server's own worktrees of the repository it lists", () => { + const assignment = assignProjectsToEnvironments( + [ + project("a1", "env-1", "github.com/acme/app"), + project("a1-wt", "env-1", "GitHub.com/acme/app"), + project("a2", "env-2", "github.com/acme/app"), + ], + envs("env-1", "env-2"), + "env-1" as EnvironmentId, + ); + expect(plain(assignment)).toEqual({ "env-1": ["a1", "a1-wt"] }); + }); + + it("ignores a project on a server that is not being read", () => { + const assignment = assignProjectsToEnvironments( + [ + project("a1", "env-1", "github.com/acme/app"), + project("a2", "env-2", "github.com/acme/app"), + ], + envs("env-2"), + "env-2" as EnvironmentId, + ); + expect(plain(assignment)).toEqual({ "env-2": ["a2"] }); + }); + + it("answers the same whatever order the projects arrive in", () => { + const projects = [ + project("a2", "env-2", "github.com/acme/app"), + project("a1", "env-1", "github.com/acme/app"), + ]; + const forward = assignProjectsToEnvironments(projects, envs("env-1", "env-2"), null); + const backward = assignProjectsToEnvironments( + projects.toReversed(), + envs("env-1", "env-2"), + null, + ); + expect(plain(forward)).toEqual({ "env-1": ["a1"] }); + expect(plain(backward)).toEqual(plain(forward)); + }); +}); + +const connected = (...ids: ReadonlyArray) => + ids.map((id) => ({ environmentId: id as EnvironmentId, label: `Server ${id}` })); + +const on = (environmentId: string, projectId: string) => ({ + environmentId: environmentId as EnvironmentId, + projectId: projectId as ProjectId, +}); + +describe("where a pull request can be acted on", () => { + it("offers every server holding the repository, the panel's own first", () => { + const pickable = resolvePickableEnvironments( + on("env-2", "a2"), + [ + project("a1", "env-1", "github.com/acme/app"), + project("a2", "env-2", "github.com/acme/app"), + project("c3", "env-3", "github.com/acme/site"), + ], + connected("env-1", "env-2", "env-3"), + ); + expect(pickable).toEqual([ + { + environmentId: "env-2", + projectId: "a2", + workspaceRoot: "/srv/env-2/a2", + label: "Server env-2", + }, + { + environmentId: "env-1", + projectId: "a1", + workspaceRoot: "/srv/env-1/a1", + label: "Server env-1", + }, + ]); + }); + + it("matches copies however the remote is cased", () => { + const pickable = resolvePickableEnvironments( + on("env-1", "a1"), + [ + project("a1", "env-1", "github.com/acme/app"), + project("a2", "env-2", "GitHub.com/ACME/app"), + ], + connected("env-1", "env-2"), + ); + expect(pickable.map((entry) => entry.environmentId)).toEqual(["env-1", "env-2"]); + }); + + it("offers nothing where one server holds the repository", () => { + expect( + resolvePickableEnvironments( + on("env-1", "a1"), + [ + project("a1", "env-1", "github.com/acme/app"), + project("c2", "env-2", "github.com/acme/site"), + ], + connected("env-1", "env-2"), + ), + ).toEqual([]); + }); + + it("offers nothing for a project with no identity to compare", () => { + expect( + resolvePickableEnvironments( + on("env-1", "p1"), + [project("p1", "env-1"), project("p2", "env-2")], + connected("env-1", "env-2"), + ), + ).toEqual([]); + }); + + it("offers nothing while the projects are still arriving", () => { + expect(resolvePickableEnvironments(on("env-1", "a1"), [], connected("env-1", "env-2"))).toEqual( + [], + ); + }); + + it("leaves out a server that is not connected", () => { + expect( + resolvePickableEnvironments( + on("env-1", "a1"), + [ + project("a1", "env-1", "github.com/acme/app"), + project("a2", "env-2", "github.com/acme/app"), + ], + connected("env-1"), + ), + ).toEqual([]); + }); + + it("names one copy per server, so two worktrees are one choice", () => { + const pickable = resolvePickableEnvironments( + on("env-1", "a1"), + [ + project("a1", "env-1", "github.com/acme/app"), + project("a1-wt", "env-1", "github.com/acme/app"), + project("a2", "env-2", "github.com/acme/app"), + project("a2-wt", "env-2", "github.com/acme/app"), + ], + connected("env-1", "env-2"), + ); + expect(pickable.map((entry) => entry.projectId)).toEqual(["a1", "a2"]); + }); + + it("keeps the panel's own worktree rather than the server's first copy", () => { + const pickable = resolvePickableEnvironments( + on("env-1", "a1-wt"), + [ + project("a1", "env-1", "github.com/acme/app"), + project("a1-wt", "env-1", "github.com/acme/app"), + project("a2", "env-2", "github.com/acme/app"), + ], + connected("env-1", "env-2"), + ); + expect(pickable[0]).toEqual({ + environmentId: "env-1", + projectId: "a1-wt", + workspaceRoot: "/srv/env-1/a1-wt", + label: "Server env-1", + }); + }); +}); diff --git a/apps/web/src/components/pullRequest/pullRequestProjectAssignment.logic.ts b/apps/web/src/components/pullRequest/pullRequestProjectAssignment.logic.ts new file mode 100644 index 00000000000..c546f56fee9 --- /dev/null +++ b/apps/web/src/components/pullRequest/pullRequestProjectAssignment.logic.ts @@ -0,0 +1,127 @@ +import type { EnvironmentId, ProjectId } from "@t3tools/contracts"; + +/** The little of a project this needs: who holds it, and which repository it is a copy of. */ +export interface AssignableProject { + readonly id: ProjectId; + readonly environmentId: EnvironmentId; + readonly repositoryIdentity?: { readonly canonicalKey?: string | undefined } | null | undefined; +} + +/** + * The remote's normalized URL is what says "same repository" across machines — it comes from the + * remote, not from a local path. Empty where the project has no identity to compare with. + */ +function repositoryKey(project: AssignableProject): string | undefined { + return project.repositoryIdentity?.canonicalKey?.toLowerCase(); +} + +/** + * Two servers can hold the same repository, and both would list the same pull requests, so one + * copy is picked by its repository key. + * + * A project with no identity is never de-duplicated: nothing proves it is a copy of anything, and + * dropping it would lose its rows outright. + */ +export function assignProjectsToEnvironments( + projects: ReadonlyArray, + environmentIds: ReadonlyArray, + preferredEnvironmentId?: EnvironmentId | null, +): Map { + const rank = new Map(environmentIds.map((id, index) => [id, index] as const)); + // Which server lists each repository: the preferred one where it has it, else the first. + const owner = new Map(); + for (const project of projects) { + const key = repositoryKey(project); + if (!key) continue; + const environmentRank = rank.get(project.environmentId); + if (environmentRank === undefined) continue; + const current = owner.get(key); + if (current === undefined) { + owner.set(key, project.environmentId); + continue; + } + if (current === preferredEnvironmentId) continue; + if ( + project.environmentId === preferredEnvironmentId || + environmentRank < (rank.get(current) ?? Number.MAX_SAFE_INTEGER) + ) { + owner.set(key, project.environmentId); + } + } + const assignment = new Map(); + for (const project of projects) { + if (!rank.has(project.environmentId)) continue; + const key = repositoryKey(project); + if (key && owner.get(key) !== project.environmentId) continue; + const listed = assignment.get(project.environmentId); + if (listed === undefined) assignment.set(project.environmentId, [project.id]); + else listed.push(project.id); + } + return assignment; +} + +/** A copy of the repository the reader could act on, named by the server holding it. */ +export interface PickableEnvironment { + readonly environmentId: EnvironmentId; + readonly projectId: ProjectId; + readonly workspaceRoot: string; + readonly label: string; +} + +/** + * Which servers a pull request could be acted on, given the one it is listed under. + * + * Listing picks a single server per repository so each pull request appears once; acting is a + * different question. Checking the branch out, or handing it to a thread, happens wherever the + * reader means to work — and the copy the listing happened not to pick is just as real. + * + * Empty where there is no choice to offer: one server, no repository identity to match copies by, + * or projects not read yet. The caller renders nothing then, so a lone server keeps the surface it + * has always had. + */ +export function resolvePickableEnvironments( + current: { readonly environmentId: EnvironmentId; readonly projectId: ProjectId }, + projects: ReadonlyArray, + environments: ReadonlyArray<{ readonly environmentId: EnvironmentId; readonly label: string }>, +): ReadonlyArray { + const own = projects.find( + (project) => + project.environmentId === current.environmentId && project.id === current.projectId, + ); + const key = own === undefined ? undefined : repositoryKey(own); + const ownLabel = environments.find( + (environment) => environment.environmentId === current.environmentId, + )?.label; + if (own === undefined || !key || ownLabel === undefined) return []; + const others = environments.flatMap((environment) => { + if (environment.environmentId === current.environmentId) return []; + // One entry per server, whichever copy comes first: a server holding two worktrees of the + // repository is still one place to act, and what is being picked here is the server. + const copy = projects.find( + (project) => + project.environmentId === environment.environmentId && repositoryKey(project) === key, + ); + return copy === undefined + ? [] + : [ + { + environmentId: environment.environmentId, + projectId: copy.id, + workspaceRoot: copy.workspaceRoot, + label: environment.label, + }, + ]; + }); + if (others.length === 0) return []; + // The panel's own server first: it is what everything else on the panel is showing, so it is + // also what acting means until the reader says otherwise. + return [ + { + environmentId: current.environmentId, + projectId: own.id, + workspaceRoot: own.workspaceRoot, + label: ownLabel, + }, + ...others, + ]; +} diff --git a/apps/web/src/components/sidebar/SidebarChrome.tsx b/apps/web/src/components/sidebar/SidebarChrome.tsx index 8ce42cf45df..637155b69b2 100644 --- a/apps/web/src/components/sidebar/SidebarChrome.tsx +++ b/apps/web/src/components/sidebar/SidebarChrome.tsx @@ -4,7 +4,7 @@ import { Link, useNavigate } from "@tanstack/react-router"; import { useEnvironmentIdentificationMode } from "../../hooks/useSettings"; import { cn } from "../../lib/utils"; -import { usePrimaryEnvironment } from "../../state/environments"; +import { useEnvironments } from "../../state/environments"; import { resolveEnvironmentIdentificationPillLabel, resolveSidebarStageBackdropVariant, @@ -112,9 +112,12 @@ function T3Wordmark() { export const SidebarChromeFooter = memo(function SidebarChromeFooter() { const navigate = useNavigate(); const { isMobile, setOpenMobile } = useSidebar(); - const primaryEnvironment = usePrimaryEnvironment(); - const pullRequestsSupported = - primaryEnvironment?.serverConfig?.environment.capabilities.pullRequests === true; + const { environments } = useEnvironments(); + // The page reads every connected server, so one of them offering pull requests is enough for + // the link to lead somewhere. + const pullRequestsSupported = environments.some( + (environment) => environment.serverConfig?.environment.capabilities.pullRequests === true, + ); const closeMobileSidebar = useCallback(() => { if (isMobile) { setOpenMobile(false); diff --git a/apps/web/src/lib/openPullRequestLink.ts b/apps/web/src/lib/openPullRequestLink.ts index 951c02e4b4d..b70345e7122 100644 --- a/apps/web/src/lib/openPullRequestLink.ts +++ b/apps/web/src/lib/openPullRequestLink.ts @@ -1,4 +1,4 @@ -import type { LocalApi, ScopedThreadRef } from "@t3tools/contracts"; +import type { EnvironmentId, LocalApi, ScopedThreadRef } from "@t3tools/contracts"; import { useNavigate } from "@tanstack/react-router"; import * as Schema from "effect/Schema"; import { type MouseEvent, useCallback } from "react"; @@ -187,20 +187,28 @@ export function useOpenChangeRequestLink( return useCallback( (event, targetUrl, targetThreadRef) => { const resolvedThreadRef = targetThreadRef ?? threadRef; - const environmentId = resolvedThreadRef?.environmentId ?? primaryEnvironmentId; - if ( - environmentId === null || - serverConfigs.get(environmentId)?.environment.capabilities.pullRequests !== true - ) { - return false; - } + const parsed = parseChangeRequestUrl(targetUrl); + if (parsed === null) return false; + const reads = (environmentId: string) => + serverConfigs.get(environmentId as EnvironmentId)?.environment.capabilities.pullRequests === + true; // Beside a thread the panel reads on that thread's environment, so a project from another // one could not be read there whatever its remote says: two environments can hold the same // repository, and handing the panel the wrong one's id opens a surface that never loads. - const projects = allProjects.filter((project) => project.environmentId === environmentId); - const parsed = parseChangeRequestUrl(targetUrl); - const project = parsed === null ? undefined : findProjectForChangeRequest(projects, parsed); - if (parsed === null || project === undefined) return false; + // + // The page has no such tie — it lists every server at once — so the link is resolved + // against all of them, the primary first where two hold the same repository. + const projects = resolvedThreadRef + ? allProjects.filter((project) => project.environmentId === resolvedThreadRef.environmentId) + : allProjects + .filter((project) => reads(project.environmentId)) + .toSorted( + (left, right) => + Number(right.environmentId === primaryEnvironmentId) - + Number(left.environmentId === primaryEnvironmentId), + ); + const project = findProjectForChangeRequest(projects, parsed); + if (project === undefined || !reads(project.environmentId)) return false; event.preventDefault(); event.stopPropagation(); if (resolvedThreadRef) { @@ -223,6 +231,8 @@ export function useOpenChangeRequestLink( repository: parsed.repository, number: parsed.number, selectedProjectId: project.id, + // Named so the page opens the right one of two servers holding this project. + selectedEnvironmentId: project.environmentId, }, }); return true; diff --git a/apps/web/src/rightPanelStore.test.ts b/apps/web/src/rightPanelStore.test.ts index 039f8ef7230..6a5730c5483 100644 --- a/apps/web/src/rightPanelStore.test.ts +++ b/apps/web/src/rightPanelStore.test.ts @@ -419,6 +419,25 @@ describe("rightPanelStore", () => { expect(state.activeSurfaceId).toBe(pullRequestSurfaceId(first)); }); + it("keeps one pull request read from two servers as two tabs", () => { + const local = { + environmentId: "local", + projectId: "project-a", + repository: "pingdotgg/t3code", + number: 4909, + }; + const remote = { ...local, environmentId: "remote" }; + + useRightPanelStore.getState().openPullRequest(refA, local); + useRightPanelStore.getState().openPullRequest(refA, remote); + + const state = selectThreadRightPanelState(useRightPanelStore.getState().byThreadKey, refA); + expect(state.surfaces.map((surface) => surface.id)).toEqual([ + pullRequestSurfaceId(local), + pullRequestSurfaceId(remote), + ]); + }); + it("tracks one surface per terminal session", () => { useRightPanelStore.getState().openTerminal(refA, "term-1"); useRightPanelStore.getState().openTerminal(refA, "term-2"); diff --git a/apps/web/src/rightPanelStore.ts b/apps/web/src/rightPanelStore.ts index 5adee07a185..d5df23fb7c4 100644 --- a/apps/web/src/rightPanelStore.ts +++ b/apps/web/src/rightPanelStore.ts @@ -52,6 +52,12 @@ export type RightPanelSurface = */ id: `pull-request:${string}`; kind: "pull-request"; + /** + * Which server the change request was read from. The list spans every connected one, so + * two of them can hold the same project id; a panel beside a thread leaves this out and + * takes the environment from its own ref. + */ + environmentId?: string; projectId: string; repository: string; number: number; @@ -86,7 +92,7 @@ interface RightPanelStoreState { openFile: (ref: ScopedThreadRef, relativePath: string, line?: number) => void; openPullRequest: ( ref: ScopedThreadRef, - target: { projectId: string; repository: string; number: number }, + target: { environmentId?: string; projectId: string; repository: string; number: number }, ) => void; openTerminal: (ref: ScopedThreadRef, terminalId: string) => void; splitTerminal: ( @@ -161,14 +167,20 @@ const terminalSurface = (terminalId: string): RightPanelSurface => ({ export type PullRequestSurface = Extract; export function pullRequestSurfaceId(target: { + environmentId?: string; projectId: string; repository: string; number: number; }): PullRequestSurface["id"] { - return `pull-request:${encodeURIComponent(target.projectId)}:${encodeURIComponent(target.repository)}:${target.number}`; + // The environment leads the id where there is one, so the same change request read from two + // servers is two tabs rather than one tab that changes its mind about which server it is on. + const scope = + target.environmentId === undefined ? "" : `${encodeURIComponent(target.environmentId)}:`; + return `pull-request:${scope}${encodeURIComponent(target.projectId)}:${encodeURIComponent(target.repository)}:${target.number}`; } export function pullRequestSurface(target: { + environmentId?: string; projectId: string; repository: string; number: number; @@ -176,6 +188,7 @@ export function pullRequestSurface(target: { return { id: pullRequestSurfaceId(target), kind: "pull-request", + ...(target.environmentId === undefined ? {} : { environmentId: target.environmentId }), projectId: target.projectId, repository: target.repository, number: target.number, @@ -260,7 +273,14 @@ export function migratePersistedRightPanelState(persistedState: unknown): { ) { return []; } - return [pullRequestSurface(surface)]; + const { environmentId, ...rest } = surface; + // Anything else stored under that name is not an environment. + return [ + pullRequestSurface({ + ...rest, + ...(typeof environmentId === "string" ? { environmentId } : {}), + }), + ]; } if (surface.kind !== "terminal") return [surface]; if ( diff --git a/apps/web/src/routes/_chat.pull-requests.tsx b/apps/web/src/routes/_chat.pull-requests.tsx index ecdc183b85b..6bb2197affa 100644 --- a/apps/web/src/routes/_chat.pull-requests.tsx +++ b/apps/web/src/routes/_chat.pull-requests.tsx @@ -4,7 +4,9 @@ import type { EnvironmentId, ProjectId, PullRequestInvolvement, - PullRequestListEntry, + PullRequestListCursors, + PullRequestListFilters, + PullRequestListInput, PullRequestListResult, PullRequestListState, SourceControlProviderKind, @@ -13,6 +15,8 @@ import { createFileRoute, useNavigate } from "@tanstack/react-router"; import { ChevronDownIcon, EyeIcon, + MonitorIcon, + ServerIcon, GitMergeIcon, GitPullRequestClosedIcon, GitPullRequestIcon, @@ -26,21 +30,28 @@ import { useCallback, useEffect, useMemo, useRef, useState, type ReactNode } fro import { filterPullRequestsByInvolvement, + findScopedProject, groupPullRequestsByInvolvement, + matchesPullRequestFilters, matchesPullRequestQuery, + parsePullRequestQuery, narrowPullRequestsToFilters, mergePullRequestDiffStats, partitionPullRequestsWithPriority, pullRequestEntryKey, rankPullRequestMatches, + pullRequestEnvironmentSetKey, readPullRequestListSnapshot, resolveProjectScope, withDiffStat, writePullRequestListSnapshot, scorePullRequestMatch, + type EnvironmentPullRequestEntry, + type MergedPullRequestList, type PullRequestDiffStats, type PullRequestPartitionsSnapshot, } from "../components/pullRequest/pullRequestList.logic"; +import { assignProjectsToEnvironments } from "../components/pullRequest/pullRequestProjectAssignment.logic"; import { PullRequestDetailPanel } from "../components/pullRequest/PullRequestDetailPanel"; import { PullRequestFiltersMenu, @@ -74,9 +85,13 @@ import { } from "../rightPanelStore"; import { useDebouncedValue } from "../state/queries"; import { useAllEnvironmentShellsBootstrapped, useProjects } from "../state/entities"; -import { usePrimaryEnvironment } from "../state/environments"; -import { pullRequestEnvironment } from "../state/pullRequests"; -import { useEnvironmentQuery } from "../state/query"; +import { useEnvironments } from "../state/environments"; +import { + pullRequestEnvironment, + usePullRequestList, + usePullRequestListStats, + type EnvironmentQueryTarget, +} from "../state/pullRequests"; import { useAtomCommand } from "../state/use-atom-command"; import { cn } from "~/lib/utils"; import { getSourceControlPresentationForKind } from "~/sourceControlPresentation"; @@ -85,6 +100,11 @@ import { COLLAPSED_SIDEBAR_TITLEBAR_INSET_CLASS } from "~/workspaceTitlebar"; export interface PullRequestsSearch { readonly involvement: PullRequestInvolvement; readonly state: PullRequestListState; + /** + * Narrows the list to one server. Absent means every connected one, which is the default the + * page has now — so a link written before servers could be chosen still opens the whole list. + */ + readonly environmentId?: EnvironmentId; /** Scopes the list. Separate from the selection so one cannot silently change the other. */ readonly projectId?: ProjectId; /** @@ -95,7 +115,20 @@ export interface PullRequestsSearch { readonly repository?: string; readonly number?: number; readonly selectedProjectId?: ProjectId; + /** + * Which server the selected pull request was read from. A project id only names a project on + * its own server, so this is what tells two servers holding one project apart. Optional: a + * link without it still opens, resolved by project id alone where that is unambiguous. + */ + readonly selectedEnvironmentId?: EnvironmentId; readonly q?: string; + /** + * The narrowings beyond state and involvement, each absent when that group is unfiltered. Flat + * in the URL because a link is read and edited by hand; folded into one record for the listing. + */ + readonly draft?: "only" | "hide"; + readonly review?: NonNullable; + readonly checks?: NonNullable; } // The state filters wear the same glyphs the rows do, so the two read as one vocabulary. @@ -128,6 +161,8 @@ const MAX_PAGE_SIZE = 500; const EMPTY_VIEWERS: PullRequestListResult["viewers"] = {}; /** The list owns one environment-scoped right panel rather than borrowing a real thread's. */ const PULL_REQUESTS_PANEL_ID = ThreadId.make("pull-requests-panel"); +/** Stable so a read that is not wanted right now does not re-key on every render. */ +const NO_LIST_TARGETS: ReadonlyArray> = []; const EMPTY_PREVIEW_SESSIONS = {}; const EMPTY_TERMINAL_LABELS = new Map(); const EMPTY_PENDING_SURFACES = new Set(); @@ -147,11 +182,25 @@ export const Route = createFileRoute("/_chat/pull-requests")({ ...(typeof raw.projectId === "string" && raw.projectId ? { projectId: raw.projectId as ProjectId } : {}), + ...(typeof raw.environmentId === "string" && raw.environmentId + ? { environmentId: raw.environmentId as EnvironmentId } + : {}), ...(typeof raw.host === "string" && raw.host ? { host: raw.host.slice(0, 200) } : {}), ...(typeof raw.selectedProjectId === "string" && raw.selectedProjectId ? { selectedProjectId: raw.selectedProjectId as ProjectId } : {}), + ...(typeof raw.selectedEnvironmentId === "string" && raw.selectedEnvironmentId + ? { selectedEnvironmentId: raw.selectedEnvironmentId as EnvironmentId } + : {}), ...(typeof raw.q === "string" && raw.q ? { q: raw.q.slice(0, 200) } : {}), + ...(raw.draft === "only" || raw.draft === "hide" ? { draft: raw.draft } : {}), + ...(raw.review === "approved" || + raw.review === "changes-requested" || + raw.review === "review-required" || + raw.review === "none" + ? { review: raw.review } + : {}), + ...(raw.checks === "passing" || raw.checks === "failing" ? { checks: raw.checks } : {}), }), component: PullRequestsRouteView, }); @@ -159,44 +208,143 @@ export const Route = createFileRoute("/_chat/pull-requests")({ function PullRequestsRouteView() { const search = Route.useSearch(); const navigate = useNavigate({ from: Route.fullPath }); - const primaryEnvironment = usePrimaryEnvironment(); - const environmentId = primaryEnvironment?.environmentId ?? null; - const capabilityKnown = primaryEnvironment !== null && primaryEnvironment.serverConfig !== null; - const pullRequestsSupported = - primaryEnvironment?.serverConfig?.environment.capabilities.pullRequests === true; - // The primary environment may still be connecting, or may predate this feature. In either - // case every query remains idle until the server has explicitly advertised these APIs. - const pullRequestEnvironmentId = pullRequestsSupported ? environmentId : null; + const { environments } = useEnvironments(); + // Every connected environment that has said it can list pull requests. Sorted, so the query + // keys, the scope key and the stored snapshot all read the same whichever order the + // connections happened to come up in. + const capableEnvironments = useMemo( + () => + environments + .filter( + (environment) => environment.serverConfig?.environment.capabilities.pullRequests === true, + ) + .toSorted((left, right) => left.environmentId.localeCompare(right.environmentId)), + [environments], + ); + // The server the URL asks for, kept only while it is one the page could read: a link naming a + // server this workspace no longer has falls back to all of them rather than to nothing. + const scopedEnvironmentId = + capableEnvironments.find((environment) => environment.environmentId === search.environmentId) + ?.environmentId ?? null; + const environmentIds = useMemo( + () => + capableEnvironments + .filter( + (environment) => + scopedEnvironmentId === null || environment.environmentId === scopedEnvironmentId, + ) + .map((environment) => environment.environmentId), + [capableEnvironments, scopedEnvironmentId], + ); + const environmentKey = useMemo( + () => pullRequestEnvironmentSetKey(environmentIds), + [environmentIds], + ); + // An environment may still be connecting, or may predate this feature. Until at least one has + // reported, an empty set means "not known yet" rather than "no environment can", and the page + // waits rather than telling a reader to upgrade a server that has not spoken. + const capabilityKnown = environments.some((environment) => environment.serverConfig !== null); + const pullRequestsSupported = environmentIds.length > 0; const allProjects = useProjects(); // Whether the workspace has said what it holds yet. Until it has, an empty project list is // "not loaded" rather than "none", and telling a reader to add a project they already have is // the one wrong answer the empty state can give. const projectsKnown = useAllEnvironmentShellsBootstrapped(); - // The page reads one environment, so a project from another one could neither be listed - // nor acted on: scoping here keeps the filter and the selection honest. + // Only the projects the page can actually read: one on an environment that cannot list pull + // requests could neither be listed nor acted on. const projects = useMemo( - () => allProjects.filter((project) => project.environmentId === environmentId), - [allProjects, environmentId], + () => allProjects.filter((project) => environmentIds.includes(project.environmentId)), + [allProjects, environmentIds], ); - const scopedProjects = useMemo( + const environmentLabels = useMemo( () => - projects - .map((project) => ({ - id: project.id, - title: project.title, - workspaceRoot: project.workspaceRoot, - })) - .toSorted((left, right) => left.title.localeCompare(right.title)), - [projects], + new Map( + environments.map((environment) => [environment.environmentId, environment.label] as const), + ), + [environments], ); - // The scope the URL asks for, once the environment has had its say about whether it exists. + const scopedProjects = useMemo(() => { + // Two machines can hold the same repository, so a title the workspace carries twice is told + // apart by the environment it lives on rather than left as two identical rows. + const titleCounts = new Map(); + for (const project of projects) { + titleCounts.set(project.title, (titleCounts.get(project.title) ?? 0) + 1); + } + return projects + .map((project) => ({ + id: project.id, + environmentId: project.environmentId, + title: + (titleCounts.get(project.title) ?? 0) > 1 + ? `${project.title} · ${environmentLabels.get(project.environmentId) ?? project.environmentId}` + : project.title, + workspaceRoot: project.workspaceRoot, + })) + .toSorted((left, right) => left.title.localeCompare(right.title)); + }, [environmentLabels, projects]); + // The scope the URL asks for, once the environments have had their say about whether it exists. const scopedProjectId = useMemo( () => resolveProjectScope(search.projectId, projects, projectsKnown), [projects, projectsKnown, search.projectId], ); + const scopedProject = useMemo( + () => findScopedProject(projects, scopedEnvironmentId, scopedProjectId), + [projects, scopedEnvironmentId, scopedProjectId], + ); + + // A link from a thread or the sidebar only knows the repository, so the owning project is + // resolved here; an explicit `projectId` in the URL still wins. + const projectIdForRepository = useMemo(() => { + const repository = search.repository?.toLowerCase(); + if (repository === undefined) return undefined; + const identity = projects.find( + (project) => + project.repositoryIdentity?.owner && + project.repositoryIdentity.name && + `${project.repositoryIdentity.owner}/${project.repositoryIdentity.name}`.toLowerCase() === + repository && + // The same `owner/name` can exist on two hosts. Without this the first match wins, and + // a link that named its host opens the pull request from the other one. + (search.host === undefined || + pullRequestHostOf( + project.repositoryIdentity, + project.repositoryIdentity.provider as SourceControlProviderKind, + ) === search.host.toLowerCase()), + ); + return identity?.id; + }, [projects, search.host, search.repository]); + + // The selection is resolved the same way the scope is: an id no connected environment has can + // never be read here, and one that arrived before the projects did is not yet wrong. + const linkedProjectId = useMemo( + () => resolveProjectScope(search.selectedProjectId, projects, projectsKnown), + [projects, projectsKnown, search.selectedProjectId], + ); + // The scope filter stands in as a last resort: a link can carry `projectId` with a repository + // whose identity the inference above cannot match, and refusing to open it because of the + // weaker signal would ignore the stronger one the URL spelled out. + const selectedProjectId = linkedProjectId ?? projectIdForRepository ?? scopedProjectId; + // Which server the selection belongs to. Named by the URL where a link had one to give; + // otherwise the project id decides, and only where exactly one server has that project. + const selectedProject = useMemo( + () => + findScopedProject( + projects, + search.selectedEnvironmentId ?? scopedEnvironmentId, + selectedProjectId, + ), + [projects, scopedEnvironmentId, search.selectedEnvironmentId, selectedProjectId], + ); + // One panel for the page rather than one per server: the surfaces carry the server they were + // read from, so tabs from two of them sit side by side instead of replacing each other. Its + // ref is fixed to the first environment so that the tab strip survives changing the scope. + const panelHomeEnvironmentId = capableEnvironments[0]?.environmentId ?? null; const rightPanelRef = useMemo( - () => (environmentId === null ? null : scopeThreadRef(environmentId, PULL_REQUESTS_PANEL_ID)), - [environmentId], + () => + panelHomeEnvironmentId === null + ? null + : scopeThreadRef(panelHomeEnvironmentId, PULL_REQUESTS_PANEL_ID), + [panelHomeEnvironmentId], ); const rightPanelState = useRightPanelStore((state) => selectThreadRightPanelState(state.byThreadKey, rightPanelRef), @@ -207,6 +355,12 @@ function PullRequestsRouteView() { const selectedPullRequestSurface = selectedRightPanelSurface?.kind === "pull-request" ? selectedRightPanelSurface : null; const activePullRequestSurface = rightPanelState.isOpen ? selectedPullRequestSurface : null; + // The open tab names its own server; a link that arrived before any tab was opened names it + // through the project it selected. + const panelEnvironmentId = + (activePullRequestSurface?.environmentId as EnvironmentId | undefined) ?? + selectedProject?.environmentId ?? + null; const [pullRequestTabStatuses, setPullRequestTabStatuses] = useState< Record >({}); @@ -234,9 +388,16 @@ function PullRequestsRouteView() { ...(next.repository ? { repository: next.repository } : {}), ...(next.number ? { number: next.number } : {}), ...(next.projectId ? { projectId: next.projectId } : {}), + ...(next.environmentId ? { environmentId: next.environmentId } : {}), ...(next.host ? { host: next.host } : {}), ...(next.selectedProjectId ? { selectedProjectId: next.selectedProjectId } : {}), + ...(next.selectedEnvironmentId + ? { selectedEnvironmentId: next.selectedEnvironmentId } + : {}), ...(next.q ? { q: next.q } : {}), + ...(next.draft ? { draft: next.draft } : {}), + ...(next.review ? { review: next.review } : {}), + ...(next.checks ? { checks: next.checks } : {}), }; }, replace: true, @@ -250,6 +411,7 @@ function PullRequestsRouteView() { repository: undefined, number: undefined, selectedProjectId: undefined, + selectedEnvironmentId: undefined, }; const updateListScope = (patch: { [Key in keyof PullRequestsSearch]?: PullRequestsSearch[Key] | undefined; @@ -267,47 +429,165 @@ function PullRequestsRouteView() { const typedQuery = (search.q ?? "").trim(); const sentQuery = useDebouncedValue(typedQuery, SEARCH_DEBOUNCE_MS); const querySettled = typedQuery === sentQuery; + // What was typed, split into the qualifiers the hosts can act on and the words that are left. + // The URL keeps the line as it was written; only the request sees the two halves. + const typedParsed = useMemo(() => parsePullRequestQuery(typedQuery), [typedQuery]); + const sentParsed = useMemo(() => parsePullRequestQuery(sentQuery), [sentQuery]); + // One record for the hosts, rebuilt only when the URL's own fields change so the listing + // inputs stay identical between renders. + const menuFilters = useMemo( + (): PullRequestListFilters => ({ + ...(search.draft ? { draft: search.draft } : {}), + ...(search.review ? { review: search.review } : {}), + ...(search.checks ? { checks: search.checks } : {}), + }), + [search.checks, search.draft, search.review], + ); + const menuFiltered = Object.keys(menuFilters).length > 0; + // A typed qualifier wins over the menu's own answer for the same thing, since it is the more + // recent word on it; labels only ever come from the query, so there is nothing to overrule. + const filters = useMemo( + (): PullRequestListFilters => ({ ...menuFilters, ...sentParsed.filters }), + [menuFilters, sentParsed.filters], + ); + const hasFilters = Object.keys(filters).length > 0; + /** The same narrowings the moment they are typed, for the rows already on screen. */ + const localFilters = useMemo( + (): PullRequestListFilters => ({ ...menuFilters, ...typedParsed.filters }), + [menuFilters, typedParsed.filters], + ); + const hasLocalFilters = Object.keys(localFilters).length > 0; + // Scoping to a project scopes to the server that owns it, saving every other server a read + // that could only answer with nothing. Where an id is ambiguous — two servers holding the + // same project id, with no server named — every server is asked for it, and the ones without + // it answer empty: two honest copies beat a coin toss between them. + const queryEnvironmentIds = useMemo( + () => + scopedProject === undefined + ? environmentIds + : environmentIds.filter((environmentId) => environmentId === scopedProject.environmentId), + [environmentIds, scopedProject], + ); + /** + * Which projects each server is asked about. Two servers holding the same repository would both + * list the same pull requests, so each repository is listed by one of them — the first, which is + * where the page's actions land — and the others are asked only for what is theirs alone. A + * server left with nothing of its own is not read at all. + * + * Left alone while the projects are still arriving, and while the scope is a single project: + * that path deliberately asks both servers holding an ambiguous id. + */ + const environmentQueries = useMemo((): ReadonlyArray<{ + readonly environmentId: EnvironmentId; + readonly projectIds?: ReadonlyArray; + }> => { + const plain = queryEnvironmentIds.map((environmentId) => ({ environmentId })); + if (!projectsKnown || scopedProjectId !== undefined) return plain; + const assignment = assignProjectsToEnvironments( + projects, + queryEnvironmentIds, + queryEnvironmentIds[0], + ); + const totals = new Map(); + for (const project of projects) { + totals.set(project.environmentId, (totals.get(project.environmentId) ?? 0) + 1); + } + return queryEnvironmentIds.flatMap((environmentId) => { + const projectIds = assignment.get(environmentId); + if (projectIds === undefined) return []; + // It lists everything it holds anyway, so the filter is left off and a one-server workspace + // asks exactly the question it asked before. + if (projectIds.length === (totals.get(environmentId) ?? 0)) return [{ environmentId }]; + return [{ environmentId, projectIds }]; + }); + }, [projects, projectsKnown, queryEnvironmentIds, scopedProjectId]); + // Part of the scope, since a different split is a different question and its answers must not + // be filed under the same page state. + const assignmentKey = useMemo( + () => + environmentQueries + .map(({ environmentId, projectIds }) => `${environmentId}#${projectIds?.join("+") ?? "*"}`) + .join("|"), + [environmentQueries], + ); // Page size is view state, not a URL concern: a shared link should open the first page. - const scopeKey = `${environmentId ?? ""}:${search.state}:${search.involvement}:${scopedProjectId ?? ""}:${search.host ?? ""}`; + const scopeKey = `${environmentKey}:${assignmentKey}:${search.state}:${search.involvement}:${scopedProjectId ?? ""}:${search.host ?? ""}:${search.draft ?? ""}:${search.review ?? ""}:${search.checks ?? ""}`; const filterKey = `${scopeKey}:${sentQuery}`; - // Where the next slice carries on from, per repository, as the server handed it back. Sending - // it is what makes a second page cost a second page rather than the whole list again — and a - // repository it does not name has run out and is not read a second time. + // Where the next slice carries on from, per repository within each environment, as that + // environment handed it back. Sending it is what makes a second page cost a second page rather + // than the whole list again — and a repository it does not name has run out and is not read a + // second time. An environment absent from it has nothing more to give at all. const [page, setPage] = useState<{ key: string; size: number; - cursors: Record | null; - }>({ key: filterKey, size: PAGE_SIZE, cursors: null }); + cursors: Readonly> | null; + /** + * The environments read again from the top at the larger page rather than continued. An + * environment can say it has more without saying where to carry on from — its provider has + * no cursor to give — and a continuation naming only the others would strand its rows on + * the host forever. + */ + regrown: ReadonlyArray; + }>({ key: filterKey, size: PAGE_SIZE, cursors: null, regrown: [] }); const pageSize = page.key === filterKey ? page.size : PAGE_SIZE; const sentCursors = page.key === filterKey ? page.cursors : null; + const sentRegrown = page.key === filterKey ? page.regrown : []; // Typing a search, or clearing one, starts the list again at its first page. Without this the // paging state from before the search is still filed under these filters and comes back with // it, so clearing would return to the slice that had been scrolled to rather than to the list. useEffect(() => { - setPage({ key: filterKey, size: PAGE_SIZE, cursors: null }); + setPage({ key: filterKey, size: PAGE_SIZE, cursors: null, regrown: [] }); }, [filterKey]); - const listQuery = useEnvironmentQuery( - pullRequestEnvironmentId === null - ? null - : pullRequestEnvironment.list({ - environmentId: pullRequestEnvironmentId, - input: { - state: search.state, - // The hosts narrow by involvement themselves — GitHub by author and review request, - // and so on — so asking them is the difference between a page of results and a page - // of everything with the answer somewhere further down it. - involvement: search.involvement, - limit: pageSize, - ...(scopedProjectId ? { projectId: scopedProjectId } : {}), - ...(search.host ? { host: search.host } : {}), - ...(sentQuery ? { query: sentQuery } : {}), - ...(sentCursors ? { cursors: sentCursors } : {}), + /** The listing input each environment is asked for, which differs only in its continuation. */ + const listTargets = useMemo( + () => + environmentQueries.flatMap(({ environmentId, projectIds }) => { + const cursors = sentCursors?.[environmentId]; + // A continuation asks the environments that said where to carry on from, plus the ones + // that have more to give but no cursor to give it from — those are read again at the + // larger page. The rest have run out, and re-reading them would answer with the page + // that is already on screen. + if (sentCursors !== null && cursors === undefined && !sentRegrown.includes(environmentId)) { + return []; + } + return [ + { + environmentId, + input: { + state: search.state, + // The hosts narrow by involvement themselves — GitHub by author and review + // request, and so on — so asking them is the difference between a page of results + // and a page of everything with the answer somewhere further down it. + involvement: search.involvement, + limit: pageSize, + ...(scopedProjectId ? { projectId: scopedProjectId } : {}), + ...(projectIds ? { projectIds } : {}), + ...(search.host ? { host: search.host } : {}), + ...(hasFilters ? { filters } : {}), + ...(sentParsed.text ? { query: sentParsed.text } : {}), + ...(cursors === undefined ? {} : { cursors }), + } satisfies PullRequestListInput, }, - }), + ]; + }), + [ + filters, + hasFilters, + pageSize, + environmentQueries, + scopedProjectId, + search.host, + search.involvement, + search.state, + sentCursors, + sentRegrown, + sentParsed.text, + ], ); + const listQuery = usePullRequestList(listTargets); /** * The same filters with nothing typed, read whether or not anything is. It is the same atom the @@ -319,20 +599,31 @@ function PullRequestsRouteView() { * say which question it belongs to: mid-switch the text has already changed and the data has * not, and a search's answer would file itself under the workspace. */ - const baselineQuery = useEnvironmentQuery( - pullRequestEnvironmentId === null - ? null - : pullRequestEnvironment.list({ - environmentId: pullRequestEnvironmentId, - input: { - state: search.state, - involvement: search.involvement, - limit: PAGE_SIZE, - ...(scopedProjectId ? { projectId: scopedProjectId } : {}), - ...(search.host ? { host: search.host } : {}), - }, - }), + const baselineTargets = useMemo( + () => + environmentQueries.map(({ environmentId, projectIds }) => ({ + environmentId, + input: { + state: search.state, + involvement: search.involvement, + limit: PAGE_SIZE, + ...(scopedProjectId ? { projectId: scopedProjectId } : {}), + ...(projectIds ? { projectIds } : {}), + ...(search.host ? { host: search.host } : {}), + ...(menuFiltered ? { filters: menuFilters } : {}), + } satisfies PullRequestListInput, + })), + [ + menuFiltered, + menuFilters, + environmentQueries, + scopedProjectId, + search.host, + search.involvement, + search.state, + ], ); + const baselineQuery = usePullRequestList(baselineTargets); // The priority groups' own reads. The feed below is paginated by recency, so an older authored // or review-requested row can be missing from its first page; partitioned from these // server-filtered reads instead, the priority view is complete up front and a continuation can @@ -340,34 +631,35 @@ 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; - const authoredQuery = useEnvironmentQuery( - pullRequestEnvironmentId === null || !partitionsWanted - ? null - : pullRequestEnvironment.list({ - environmentId: pullRequestEnvironmentId, - input: { - state: search.state, - involvement: "authored", - limit: PAGE_SIZE, - ...(scopedProjectId ? { projectId: scopedProjectId } : {}), - ...(search.host ? { host: search.host } : {}), - }, - }), - ); - const reviewingQuery = useEnvironmentQuery( - pullRequestEnvironmentId === null || !partitionsWanted - ? null - : pullRequestEnvironment.list({ - environmentId: pullRequestEnvironmentId, - input: { - state: search.state, - involvement: "reviewing", - limit: PAGE_SIZE, - ...(scopedProjectId ? { projectId: scopedProjectId } : {}), - ...(search.host ? { host: search.host } : {}), - }, - }), - ); + // 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(() => { + if (!partitionsWanted) 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, + 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 // empty states retry plainly, because a failure is never cached. const invalidate = useAtomCommand(pullRequestEnvironment.invalidate, { reportFailure: false }); @@ -382,9 +674,11 @@ function PullRequestsRouteView() { const refreshFromHost = async () => { setInvalidating(true); try { - if (pullRequestEnvironmentId !== null) { - await invalidate({ environmentId: pullRequestEnvironmentId, input: {} }); - } + // Every environment the page is reading, since what the reader pressed refresh for is the + // list in front of them rather than whichever machine happens to be first. + await Promise.all( + queryEnvironmentIds.map((environmentId) => invalidate({ environmentId, input: {} })), + ); } finally { setInvalidating(false); } @@ -402,38 +696,38 @@ function PullRequestsRouteView() { // out: a longer page reads as growth, and a search shows the rows it already has, narrowed // here, until the hosts answer for themselves. const [loaded, setLoaded] = useState<{ - environmentId: EnvironmentId | null; + environmentKey: string; scope: string; query: string; - data: PullRequestListResult; + data: MergedPullRequestList; /** The priority groups' own answers, carried so a cold start has whole groups too. */ partitions?: PullRequestPartitionsSnapshot; } | null>(null); // A reload recreates the registry the queries live in, so with nothing held the page would // cold-start into skeletons even though almost every row is unchanged. The last answer for - // this environment is kept across reloads and hydrated here as the carried rows: they render - // at once — narrowed to the current filters like any carried answer — and the live read + // this set of environments is kept across reloads and hydrated here as the carried rows: they + // render at once — narrowed to the current filters like any carried answer — and the live read // reconciles them in place by key rather than replacing them with ghosts. useEffect(() => { - if (environmentId === null) return; + if (environmentKey.length === 0) return; setLoaded((current) => { - // Another environment's rows could not even be narrowed — nothing on a row says which - // environment read it — so its snapshot beats holding them. - if (current !== null && current.environmentId === environmentId) return current; + // Rows read from a different set of environments cannot even be narrowed — one of them may + // no longer be connected at all — so that set's own snapshot beats holding them. + if (current !== null && current.environmentKey === environmentKey) return current; const snapshot = readPullRequestListSnapshot( typeof window === "undefined" ? undefined : window.localStorage, - environmentId, + environmentKey, ); if (snapshot === null) return null; return { - environmentId, + environmentKey, scope: snapshot.scope, query: "", data: snapshot.data, ...(snapshot.partitions === undefined ? {} : { partitions: snapshot.partitions }), }; }); - }, [environmentId]); + }, [environmentKey]); useEffect(() => { // Only once this query has settled. While a search is being swapped in or out the text has // already changed and the data has not, so recording them together would file the previous @@ -450,22 +744,22 @@ function PullRequestsRouteView() { partitionsWanted && authoredQuery.data !== null && reviewingQuery.data !== null ? { authored: authoredQuery.data.entries, reviewing: reviewingQuery.data.entries } : current !== null && - current.environmentId === environmentId && + current.environmentKey === environmentKey && current.scope === scopeKey ? current.partitions : undefined; // A search's answer is the search's, not the workspace's, so only unsearched lists // persist. Written here where the held partitions are in reach, so a feed settling // ahead of them cannot overwrite a stored snapshot that already had both groups. - if (environmentId !== null && sentQuery.length === 0) { + if (environmentKey.length > 0 && sentQuery.length === 0) { writePullRequestListSnapshot( typeof window === "undefined" ? undefined : window.localStorage, - environmentId, + environmentKey, { scope: scopeKey, data, ...(partitions === undefined ? {} : { partitions }) }, ); } return { - environmentId, + environmentKey, scope: scopeKey, query: sentQuery, data, @@ -473,7 +767,7 @@ function PullRequestsRouteView() { }; }); }, [ - environmentId, + environmentKey, scopeKey, sentQuery, listQuery.data, @@ -486,10 +780,10 @@ function PullRequestsRouteView() { // perfectly good rows for the last one. Rather than blank out for the round trip, those rows // are narrowed to the new filters and stay until the answer lands — a subset of it, never a // row it excludes. Narrowed to nothing there is nothing to carry, and the skeletons below are - // right after all. Another environment's rows are dropped rather than narrowed: nothing on a - // row says which environment read it. + // right after all. Rows read from a different set of environments are dropped rather than + // narrowed: one of those environments may no longer be connected. const narrowed = useMemo(() => { - if (loaded === null || loaded.environmentId !== environmentId || loaded.scope === scopeKey) { + if (loaded === null || loaded.environmentKey !== environmentKey || loaded.scope === scopeKey) { return null; } const entries = narrowPullRequestsToFilters(loaded.data.entries, { @@ -498,7 +792,7 @@ function PullRequestsRouteView() { host: search.host, }); return entries.length === 0 ? null : { ...loaded.data, entries }; - }, [environmentId, loaded, scopeKey, scopedProjectId, search.host, search.state]); + }, [environmentKey, loaded, scopeKey, scopedProjectId, search.host, search.state]); // With nothing typed and nothing to carry on from, the answer is taken from the read that is // keyed to exactly that question. Otherwise a search's answer lingers for a render after the // text has gone — the data cannot say which question it belongs to, but the read it came from @@ -530,13 +824,16 @@ function PullRequestsRouteView() { // the order again. const [ordered, setOrdered] = useState<{ key: string; - entries: ReadonlyArray; + entries: ReadonlyArray; } | null>(null); useEffect(() => { if (!answered) return; setOrdered((previous) => { if (previous === null || previous.key !== filterKey) { - return { key: filterKey, entries: rankPullRequestMatches(answered.entries, sentQuery) }; + return { + key: filterKey, + entries: rankPullRequestMatches(answered.entries, sentParsed.text), + }; } if (sentCursors !== null) { // A continuation is a slice, not the list: it carries only what comes after the rows @@ -547,7 +844,7 @@ function PullRequestsRouteView() { const arrived = answered.entries.filter((entry) => !held.has(pullRequestEntryKey(entry))); const appended = rankPullRequestMatches( arrived.toSorted((left, right) => right.updatedAt.localeCompare(left.updatedAt)), - sentQuery, + sentParsed.text, ); return { key: filterKey, entries: [...previous.entries, ...appended] }; } @@ -556,9 +853,9 @@ function PullRequestsRouteView() { // since the last read at the bottom of the page — below rows a week older — where "the // latest" is exactly what a refresh was for. The host answers in the order the page // reads, so its order stands; a row that moved was updated, and moving is the news. - return { key: filterKey, entries: rankPullRequestMatches(answered.entries, sentQuery) }; + return { key: filterKey, entries: rankPullRequestMatches(answered.entries, sentParsed.text) }; }); - }, [answered, filterKey, sentCursors, sentQuery]); + }, [answered, filterKey, sentCursors, sentParsed.text]); // Carrying on where the last answer stopped, and only raising the page size for the hosts that // could not say where that was. @@ -566,16 +863,27 @@ function PullRequestsRouteView() { // continuation is a boundary in one listing, and carrying one across a search would ask the // new question to start where the old one stopped — skipping its newest matches entirely. const nextCursors = answered?.nextCursors ?? {}; + // The environments with more rows and no cursor to reach them by. They come along with any + // continuation, read again at a page one step larger, which is the only way they grow. + const regrown = (answered?.truncatedEnvironments ?? []).filter( + (environmentId) => nextCursors[environmentId] === undefined, + ); const canContinue = !showingCarried && Object.keys(nextCursors).length > 0; const loadMore = () => { if (canContinue) { - setPage({ key: filterKey, size: pageSize, cursors: nextCursors }); + setPage({ + key: filterKey, + size: regrown.length === 0 ? pageSize : Math.min(pageSize + PAGE_SIZE, MAX_PAGE_SIZE), + cursors: nextCursors, + regrown, + }); return; } setPage({ key: filterKey, size: Math.min(pageSize + PAGE_SIZE, MAX_PAGE_SIZE), cursors: null, + regrown: [], }); }; @@ -591,6 +899,7 @@ function PullRequestsRouteView() { const loadedCount = ordered?.key === filterKey ? ordered.entries.length : pageSize; setPage({ key: filterKey, + regrown: [], size: Math.min( Math.max(pageSize, Math.ceil(loadedCount / PAGE_SIZE) * PAGE_SIZE), MAX_PAGE_SIZE, @@ -611,7 +920,7 @@ function PullRequestsRouteView() { authoredQuery.refresh(); reviewingQuery.refresh(); }, - { enabled: pullRequestEnvironmentId !== null }, + { enabled: pullRequestsSupported }, ); const viewers = baselineQuery.data?.viewers ?? listData?.viewers ?? EMPTY_VIEWERS; @@ -636,22 +945,37 @@ function PullRequestsRouteView() { // The local pass stands in for the answer that has not arrived yet, and for the hosts that // answered without searching at all: Azure DevOps has no text filter, so its rows arrive // whole and would otherwise sit under a search that never touched them. - if (typedQuery.length === 0) return involvementEntries; + // The rows the hosts could not narrow themselves, for the fields a row actually carries: + // a host with no filter of its own answers unnarrowed, and so does one whose answer for the + // new filters has not arrived yet. Checks are absent here, because no row carries them. + const narrowedEntries = hasLocalFilters + ? involvementEntries.filter((entry) => matchesPullRequestFilters(entry, localFilters)) + : involvementEntries; + // Newest update first once nothing is typed, so a list merged from several hosts reads in + // one order rather than in each host's. With a search on, the relevance ranking above is + // the order, and re-sorting by date here would undo it. + if (typedParsed.text.length === 0) { + return narrowedEntries.toSorted((left, right) => + right.updatedAt.localeCompare(left.updatedAt), + ); + } const answeredLocally = querySettled && !showingCarried; - return involvementEntries.filter( + return narrowedEntries.filter( (entry) => (answeredLocally && searchingHosts.has(entry.host)) || - matchesPullRequestQuery(entry, typedQuery), + matchesPullRequestQuery(entry, typedParsed.text), ); }, [ filterKey, + hasLocalFilters, + localFilters, listData, ordered, querySettled, search.involvement, searchingHosts, showingCarried, - typedQuery, + typedParsed.text, viewers, ]); @@ -717,21 +1041,32 @@ function PullRequestsRouteView() { // authored row older than it. Once the live reads land they take over; with neither, // the local grouping is still better than nothing. const held = - loaded !== null && loaded.environmentId === environmentId && loaded.scope === scopeKey + loaded !== null && loaded.environmentKey === environmentKey && loaded.scope === scopeKey ? loaded.partitions : undefined; - const authored = partitionsWanted ? (authoredQuery.data?.entries ?? held?.authored) : undefined; - const reviewing = partitionsWanted - ? (reviewingQuery.data?.entries ?? held?.reviewing) - : undefined; + // The priority reads answer the same question the feed does, so they take the same local + // narrowing: a host that cannot filter for itself would otherwise put rows into Authored + // that the filters above just took out of the feed. + const narrow = (rows: ReadonlyArray | undefined) => + rows === undefined || !hasLocalFilters + ? rows + : rows.filter((entry) => matchesPullRequestFilters(entry, localFilters)); + const authored = narrow( + partitionsWanted ? (authoredQuery.data?.entries ?? held?.authored) : undefined, + ); + const reviewing = narrow( + partitionsWanted ? (reviewingQuery.data?.entries ?? held?.reviewing) : undefined, + ); if (authored === undefined || reviewing === undefined) { return groupPullRequestsByInvolvement(entries, viewers); } return partitionPullRequestsWithPriority(entries, authored, reviewing); }, [ + hasLocalFilters, + localFilters, authoredQuery.data?.entries, entries, - environmentId, + environmentKey, loaded, partitionsWanted, reviewingQuery.data?.entries, @@ -741,75 +1076,51 @@ function PullRequestsRouteView() { ]); // Keyed by every row being shown — the partitions can hold rows the feed has not paged to — - // so scrolling further asks only about what is new. - const statsInput = useMemo( - () => ({ - refs: groups - .flatMap((group) => group.entries) - .map((entry) => ({ + // so scrolling further asks only about what is new. One read per environment, each asking only + // about its own rows: a reference names a project, and a project belongs to one machine. + const statsTargets = useMemo(() => { + const refsByEnvironment = new Map< + EnvironmentId, + Array<{ projectId: ProjectId; repository: string; number: number }> + >(); + for (const group of groups) { + for (const entry of group.entries) { + const refs = refsByEnvironment.get(entry.environmentId) ?? []; + refs.push({ projectId: entry.projectId, repository: entry.repository, number: entry.number, - })), - }), - [groups], - ); - const statsQuery = useEnvironmentQuery( - pullRequestEnvironmentId === null || statsInput.refs.length === 0 - ? null - : pullRequestEnvironment.listStats({ - environmentId: pullRequestEnvironmentId, - input: statsInput, - }), - ); + }); + refsByEnvironment.set(entry.environmentId, refs); + } + } + return [...refsByEnvironment].map(([environmentId, refs]) => ({ + environmentId, + input: { refs }, + })); + }, [groups]); + const statsQuery = usePullRequestListStats(statsTargets); // Adding or removing one row keys a fresh stats query with nothing in it yet, so the counts // are merged into what is already held rather than rebuilt: every count on screen stays until // its replacement arrives. const [statsByRow, setStatsByRow] = useState(() => new Map()); useEffect(() => { - const stats = statsQuery.data?.stats; - if (stats === undefined) return; + const stats = statsQuery.stats; + if (stats === null) return; setStatsByRow((previous) => mergePullRequestDiffStats(previous, stats)); - }, [statsQuery.data]); + }, [statsQuery.stats]); - // A link from a thread or the sidebar only knows the repository, so the owning project is - // resolved here; an explicit `projectId` in the URL still wins. - const projectIdForRepository = useMemo(() => { - const repository = search.repository?.toLowerCase(); - if (repository === undefined) return undefined; - const identity = projects.find( - (project) => - project.repositoryIdentity?.owner && - project.repositoryIdentity.name && - `${project.repositoryIdentity.owner}/${project.repositoryIdentity.name}`.toLowerCase() === - repository && - // The same `owner/name` can exist on two hosts. Without this the first match wins, and - // a link that named its host opens the pull request from the other one. - (search.host === undefined || - pullRequestHostOf( - project.repositoryIdentity, - project.repositoryIdentity.provider as SourceControlProviderKind, - ) === search.host.toLowerCase()), - ); - return identity?.id; - }, [projects, search.host, search.repository]); - - // The selection is resolved the same way the scope is: an id from another environment can - // never be read here, and one that arrived before the projects did is not yet wrong. - const linkedProjectId = useMemo( - () => resolveProjectScope(search.selectedProjectId, projects, projectsKnown), - [projects, projectsKnown, search.selectedProjectId], - ); - // The scope filter stands in as a last resort: a link can carry `projectId` with a repository - // whose identity the inference above cannot match, and refusing to open it because of the - // weaker signal would ignore the stronger one the URL spelled out. - const selectedProjectId = linkedProjectId ?? projectIdForRepository ?? scopedProjectId; const linkedSelection = useMemo( () => - search.repository && search.number && selectedProjectId - ? { repository: search.repository, number: search.number, projectId: selectedProjectId } + search.repository && search.number && selectedProject + ? { + environmentId: selectedProject.environmentId, + repository: search.repository, + number: search.number, + projectId: selectedProject.id, + } : null, - [search.number, search.repository, selectedProjectId], + [search.number, search.repository, selectedProject], ); useEffect(() => { if (!pullRequestsSupported || rightPanelRef === null || linkedSelection === null) return; @@ -819,6 +1130,7 @@ function PullRequestsRouteView() { const selected = rightPanelState.isOpen && activePullRequestSurface !== null ? { + environmentId: activePullRequestSurface.environmentId, repository: activePullRequestSurface.repository, number: activePullRequestSurface.number, projectId: activePullRequestSurface.projectId as ProjectId, @@ -833,6 +1145,9 @@ function PullRequestsRouteView() { repository: surface.repository, number: surface.number, selectedProjectId: surface.projectId as ProjectId, + ...(surface.environmentId === undefined + ? {} + : { selectedEnvironmentId: surface.environmentId as EnvironmentId }), }, ); @@ -884,13 +1199,15 @@ function PullRequestsRouteView() { // Stable so the memoized rows can skip re-rendering when the list around them changes. const selectEntry = useCallback( - (entry: PullRequestListEntry) => { + (entry: EnvironmentPullRequestEntry) => { + // The surface carries the row's own server, which is what its detail reads and acts on. if (rightPanelRef === null) return; useRightPanelStore.getState().openPullRequest(rightPanelRef, entry); updateSearch({ repository: entry.repository, number: entry.number, selectedProjectId: entry.projectId, + selectedEnvironmentId: entry.environmentId, }); }, [rightPanelRef, updateSearch], @@ -936,7 +1253,7 @@ function PullRequestsRouteView() { ) : !pullRequestsSupported ? ( ) : firstLoad ? ( @@ -980,14 +1297,20 @@ function PullRequestsRouteView() { entry={withDiffStat(entry, statsByRow)} showProjectTitle showProvider={showProvider} + {...(capableEnvironments.length > 1 && + environmentLabels.get(entry.environmentId) !== undefined + ? { environmentLabel: environmentLabels.get(entry.environmentId)! } + : {})} // Ten is the floor the ranking gives a row whose own fields say nothing // about the search: the host matched something this row cannot show. matchedElsewhere={ - typedQuery.length > 0 && - scorePullRequestMatch(entry, typedQuery) <= MATCHED_ELSEWHERE_SCORE + typedParsed.text.length > 0 && + scorePullRequestMatch(entry, typedParsed.text) <= MATCHED_ELSEWHERE_SCORE } selected={ - selected?.repository === entry.repository && selected.number === entry.number + selected?.environmentId === entry.environmentId && + selected.repository === entry.repository && + selected.number === entry.number } onSelect={selectEntry} /> @@ -1037,6 +1360,16 @@ function PullRequestsRouteView() { }; }), ]; + // The same shape the host pills take, so the two groups read as one control. A local + // connection wears the screen it is on; every other server wears a server. + const serverMenuOptions: ReadonlyArray> = [ + { value: "", label: "All servers", Icon: LayersIcon }, + ...capableEnvironments.map((environment) => ({ + value: environment.environmentId, + label: environment.label, + Icon: environment.displayUrl === null ? MonitorIcon : ServerIcon, + })), + ]; const filtersMenu = ( updateListScope({ involvement })} + filters={menuFilters} + onFilters={(next) => + updateListScope({ draft: next.draft, review: next.review, checks: next.checks }) + } host={search.host} hostOptions={hostMenuOptions} onHost={(host) => updateListScope({ host })} - environmentId={environmentId} + server={scopedEnvironmentId ?? undefined} + serverOptions={serverMenuOptions} + // Narrowing to one server drops a project scope belonging to another, which would + // otherwise narrow the list to nothing with no visible filter to explain it. + onServer={(server) => updateListScope({ environmentId: server, projectId: undefined })} projects={scopedProjects} projectId={scopedProjectId} unavailable={unavailableProjects} @@ -1114,7 +1455,7 @@ function PullRequestsRouteView() { {pullRequestsSupported && rightPanelState.isOpen ? openPanelControls : null} - {rightPanelState.isOpen && activePullRequestSurface && pullRequestEnvironmentId !== null ? ( + {rightPanelState.isOpen && activePullRequestSurface && panelEnvironmentId !== null ? ( { + readonly environmentId: EnvironmentId; + readonly input: Input; +} + +interface MergedEnvironmentQueryView { + /** One entry per environment that has answered, in the order the targets were given. */ + readonly values: ReadonlyArray; + /** The first environment that failed. Others may still have answered — this is not fatal. */ + readonly error: string | null; + readonly isPending: boolean; +} + +/** + * The same per-environment query read across several environments at once. React cannot subscribe + * to a list of atoms whose length changes, so the fan-out happens inside one derived atom keyed by + * the targets — the same shape the cross-environment thread search uses. + * + * An environment that fails contributes nothing rather than blanking the page: the pull request + * list is a union, and one unreachable machine should not hide the others' rows. + */ +function createMergedEnvironmentQuery( + label: string, + atomFor: ( + target: EnvironmentQueryTarget, + ) => Atom.Atom>, +) { + const family = Atom.family((key: string) => + Atom.make((get): MergedEnvironmentQueryView => { + const targets = JSON.parse(key) as ReadonlyArray>; + const values: Array = []; + let error: string | null = null; + let isPending = false; + for (const target of targets) { + const result = get(atomFor(target)); + isPending ||= result.waiting; + if (result._tag === "Failure" && error === null) { + error = formatEnvironmentQueryError(result.cause); + } + const value = Option.getOrNull(AsyncResult.value(result)); + if (value !== null) values.push([target.environmentId, value]); + } + return { values, error, isPending }; + }).pipe(Atom.withLabel(`${label}:${key}`)), + ); + const empty = Atom.make>({ + values: [], + error: null, + isPending: false, + }).pipe(Atom.withLabel(`${label}:empty`)); + return function useMergedQuery(targets: ReadonlyArray>) { + const key = JSON.stringify(targets); + const view = useAtomValue(targets.length === 0 ? empty : family(key)); + const refresh = useCallback(() => { + for (const target of JSON.parse(key) as ReadonlyArray>) { + appAtomRegistry.refresh(atomFor(target)); + } + }, [key]); + return { ...view, refresh }; + }; +} + +const usePullRequestListsQuery = createMergedEnvironmentQuery( + "web-pull-requests:list", + pullRequestEnvironment.list, +); + +const usePullRequestStatsQuery = createMergedEnvironmentQuery( + "web-pull-requests:list-stats", + pullRequestEnvironment.listStats, +); + +export interface MergedPullRequestListView { + readonly data: MergedPullRequestList | null; + readonly error: string | null; + readonly isPending: boolean; + readonly refresh: () => void; +} + +/** One listing per environment, merged into the single list the page renders. */ +export function usePullRequestList( + targets: ReadonlyArray>, +): MergedPullRequestListView { + const query = usePullRequestListsQuery(targets); + const data = useMemo(() => mergePullRequestLists(query.values), [query.values]); + return { data, error: query.error, isPending: query.isPending, refresh: query.refresh }; +} + +/** The line counts for the rows on screen, asked of each environment for its own rows. */ +export function usePullRequestListStats( + targets: ReadonlyArray>, +): { + readonly stats: ReadonlyArray | null; + readonly refresh: () => void; +} { + const query = usePullRequestStatsQuery(targets); + const stats = useMemo( + () => + query.values.length === 0 + ? null + : query.values.flatMap(([environmentId, result]) => + result.stats.map((stat) => ({ ...stat, environmentId })), + ), + [query.values], + ); + return { stats, refresh: query.refresh }; +} diff --git a/apps/web/src/state/query.ts b/apps/web/src/state/query.ts index 2610f1724a0..9bc16b01fe3 100644 --- a/apps/web/src/state/query.ts +++ b/apps/web/src/state/query.ts @@ -14,7 +14,7 @@ export interface EnvironmentQueryView { readonly refresh: () => void; } -function formatError(cause: Cause.Cause): string { +export function formatEnvironmentQueryError(cause: Cause.Cause): string { const error = Cause.squash(cause); return error instanceof Error && error.message.trim().length > 0 ? error.message @@ -29,7 +29,7 @@ export function useEnvironmentQuery( const refresh = useAtomRefresh(selectedAtom); return { data: Option.getOrNull(AsyncResult.value(result)), - error: result._tag === "Failure" ? formatError(result.cause) : null, + error: result._tag === "Failure" ? formatEnvironmentQueryError(result.cause) : null, isPending: atom !== null && result.waiting, refresh, }; diff --git a/packages/contracts/src/environmentHttp.test.ts b/packages/contracts/src/environmentHttp.test.ts new file mode 100644 index 00000000000..4cd39074f3e --- /dev/null +++ b/packages/contracts/src/environmentHttp.test.ts @@ -0,0 +1,62 @@ +import { describe, expect, it } from "vite-plus/test"; + +import { + EnvironmentAuthInvalidError, + EnvironmentInternalError, + EnvironmentOperationForbiddenError, + EnvironmentRequestInvalidError, + EnvironmentResourceNotFoundError, + EnvironmentScopeRequiredError, +} from "./environmentHttp.ts"; + +const traceId = "trace-1"; + +describe("environment HTTP errors", () => { + // A client squashes the cause and shows `message`; an empty one becomes a generic + // "The environment request failed." that names nothing the reader can act on. + it("each carries a message that names its reason", () => { + const errors = [ + new EnvironmentRequestInvalidError({ + code: "invalid_request", + reason: "invalid_command", + traceId, + }), + new EnvironmentAuthInvalidError({ + code: "auth_invalid", + reason: "missing_credential", + traceId, + }), + new EnvironmentScopeRequiredError({ + code: "insufficient_scope", + requiredScope: "orchestration:read", + traceId, + }), + new EnvironmentOperationForbiddenError({ + code: "operation_forbidden", + reason: "current_session_revoke_not_allowed", + traceId, + }), + new EnvironmentResourceNotFoundError({ + code: "not_found", + reason: "thread_not_found", + traceId, + }), + new EnvironmentInternalError({ + code: "internal_error", + reason: "orchestration_snapshot_failed", + traceId, + }), + ] as const; + const details = [ + "invalid_command", + "missing_credential", + "orchestration:read", + "current_session_revoke_not_allowed", + "thread_not_found", + "orchestration_snapshot_failed", + ]; + errors.forEach((error, index) => { + expect(error.message).toContain(details[index]); + }); + }); +}); diff --git a/packages/contracts/src/environmentHttp.ts b/packages/contracts/src/environmentHttp.ts index d209588b609..e7494862251 100644 --- a/packages/contracts/src/environmentHttp.ts +++ b/packages/contracts/src/environmentHttp.ts @@ -106,6 +106,10 @@ export class EnvironmentRequestInvalidError extends Schema.TaggedErrorClass()( @@ -120,6 +124,10 @@ export class EnvironmentAuthInvalidError extends Schema.TaggedErrorClass()( @@ -134,6 +142,10 @@ export class EnvironmentScopeRequiredError extends Schema.TaggedErrorClass()( @@ -148,6 +160,10 @@ export class EnvironmentOperationForbiddenError extends Schema.TaggedErrorClass< [HttpServerRespondable.symbol]() { return HttpServerResponse.schemaJson(EnvironmentOperationForbiddenError)(this, { status: 403 }); } + + override get message(): string { + return `The environment refused this operation (${this.reason}).`; + } } export class EnvironmentInternalError extends Schema.TaggedErrorClass()( @@ -162,6 +178,10 @@ export class EnvironmentInternalError extends Schema.TaggedErrorClass { ).toEqual(["user", "team"]); }); }); + +describe("updating a branch that has fallen behind its base", () => { + const decodeAction = Schema.decodeUnknownSync(PullRequestActionInput); + const ref = { projectId: "project-1", repository: "acme/web", number: 7 }; + + it("carries the way the branch should be brought up to date", () => { + expect(decodeAction({ ...ref, action: "update-branch", updateMethod: "rebase" })).toMatchObject( + { + action: "update-branch", + updateMethod: "rebase", + }, + ); + }); + + it("takes the action without a method, which is the host's own default", () => { + expect(decodeAction({ ...ref, action: "update-branch" }).updateMethod).toBeUndefined(); + }); + + it("refuses a way no host offers", () => { + expect(() => + decodeAction({ ...ref, action: "update-branch", updateMethod: "squash" }), + ).toThrow(); + }); +}); diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index 707ec540d15..1e9888f67ff 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -25,15 +25,87 @@ export type PullRequestState = typeof PullRequestState.Type; export const PullRequestListState = Schema.Literals(["all", "open", "closed", "merged"]); export type PullRequestListState = typeof PullRequestListState.Type; +/** Where a review stands overall, as a host that summarises its reviews reports it. */ +export const PullRequestReviewDecision = Schema.Literals([ + "approved", + "changes-requested", + "review-required", +]); +export type PullRequestReviewDecision = typeof PullRequestReviewDecision.Type; + +/** One qualifier's value, bounded because it is written into a host's own search query. */ +const PullRequestQualifierValue = TrimmedNonEmptyString.check(Schema.isMaxLength(200)); +const PullRequestQualifierValues = Schema.Array(PullRequestQualifierValue).check( + Schema.isMaxLength(10), +); + +/** + * Narrowings beyond state and involvement, each absent by default — an absent field filters + * nothing, which is what every listing did before there were any. Optional as a whole so a page + * and a server of different ages still speak to each other. + * + * `checks` is host-side only: no row carries its own check state, so a host that cannot match it + * answers unnarrowed rather than the page pretending to know. + */ +export const PullRequestListFilters = Schema.Struct({ + draft: Schema.optional(Schema.Literals(["only", "hide"])), + review: Schema.optional( + Schema.Literals(["approved", "changes-requested", "review-required", "none"]), + ), + checks: Schema.optional(Schema.Literals(["passing", "failing"])), + /** + * Labels as GitHub's own search reads them: each group is one `label:` qualifier, a row must + * satisfy every group, and a group holding several names is satisfied by any one of them — + * `label:size:S,size:XS` finds either size. Typed rather than picked, since a project's labels + * are its own and no menu can list them. Bounded because each becomes a host search qualifier. + */ + labels: Schema.optional(Schema.Array(PullRequestQualifierValues).check(Schema.isMaxLength(10))), + excludedLabels: Schema.optional(PullRequestQualifierValues), + /** One login, as `author:` names it. */ + author: Schema.optional(PullRequestQualifierValue), +}); +export type PullRequestListFilters = typeof PullRequestListFilters.Type; + +/** The one-glyph summary of a change request's checks, as its list row wears it. */ +export const PullRequestChecksState = Schema.Literals(["passing", "failing", "pending"]); +export type PullRequestChecksState = typeof PullRequestChecksState.Type; + export const PullRequestMergeability = Schema.Literals(["mergeable", "conflicting", "unknown"]); export type PullRequestMergeability = typeof PullRequestMergeability.Type; export const PullRequestMergeMethod = Schema.Literals(["merge", "squash", "rebase"]); export type PullRequestMergeMethod = typeof PullRequestMergeMethod.Type; -export const PullRequestAction = Schema.Literals(["merge", "ready", "draft", "close", "reopen"]); +export const PullRequestAction = Schema.Literals([ + "merge", + "ready", + "draft", + "close", + "reopen", + /** Bring the base branch's commits into this one, which is what unblocks a stale branch. */ + "update-branch", +]); export type PullRequestAction = typeof PullRequestAction.Type; +/** + * How a stale branch catches up with its base: a merge commit, or a rebase onto it. The two are + * the host's own choices, not this page's — GitHub offers both and refuses a rebase it cannot + * replay, so what is offered comes from the host and what is allowed comes from the viewer. + */ +export const PullRequestUpdateMethod = Schema.Literals(["merge", "rebase"]); +export type PullRequestUpdateMethod = typeof PullRequestUpdateMethod.Type; + +/** + * Where the branch stands against the base it would merge into. Separate from `mergeability`, + * which answers a different question: a branch can be behind and still merge cleanly, and that + * pairing — out of date, no conflicts — is the one an update button exists for. + * + * "unknown" where the host was not asked or could not say, which is every host but GitHub and + * every pull request whose head repository could not be compared. + */ +export const PullRequestBaseComparison = Schema.Literals(["up-to-date", "behind", "unknown"]); +export type PullRequestBaseComparison = typeof PullRequestBaseComparison.Type; + export const PullRequestActor = Schema.Struct({ login: TrimmedNonEmptyString, name: Schema.NullOr(Schema.String), @@ -230,6 +302,11 @@ export const PullRequestCapabilities = Schema.Struct({ actions: Schema.Array(PullRequestAction), /** Merge strategies the provider itself offers, before repository settings narrow them. */ mergeMethods: Schema.Array(PullRequestMergeMethod), + /** + * How this host can bring a stale branch up to date. Absent where it cannot at all, which is + * every host that has not said otherwise — so a provider that says nothing offers nothing. + */ + updateMethods: Schema.optional(Schema.Array(PullRequestUpdateMethod)), /** * The host can narrow a listing by free text. False means it answers unnarrowed and whoever * asked has to do the narrowing — which is a different promise, so the page is told rather @@ -262,6 +339,11 @@ export const PullRequestViewerPermissions = Schema.Struct({ verdicts: Schema.Array(PullRequestReviewVerdict), /** This viewer may ask somebody for a review, and take the request back again. */ requestReviewers: Schema.Boolean, + /** + * The ways this viewer may bring the branch up to date, narrowed from what the host offers. + * Absent or empty means they may not, which is also what a host with no such action says. + */ + updateMethods: Schema.optional(Schema.Array(PullRequestUpdateMethod)), }); export type PullRequestViewerPermissions = typeof PullRequestViewerPermissions.Type; @@ -303,6 +385,10 @@ export const PullRequestListEntry = Schema.Struct({ updatedAt: IsoDateTime, viewerReviewRequested: Schema.Boolean, labels: Schema.Array(PullRequestLabel), + /** Absent where the host does not summarise its reviews, which is every host but GitHub. */ + reviewDecision: Schema.optional(PullRequestReviewDecision), + /** Absent where the host reports no check rollup, or the change request has no checks. */ + checksState: Schema.optional(PullRequestChecksState), }); export type PullRequestListEntry = typeof PullRequestListEntry.Type; @@ -324,7 +410,14 @@ export type PullRequestListCursors = typeof PullRequestListCursors.Type; export const PullRequestListInput = Schema.Struct({ state: PullRequestListState, involvement: Schema.optional(PullRequestInvolvement), + filters: Schema.optional(PullRequestListFilters), projectId: Schema.optional(ProjectId), + /** + * Only these projects, for a client that assigns each shared repository to one of its + * connections and asks the others to stay quiet about it. Absent means every project, which + * is what every listing asked for before there were several connections to spread across. + */ + projectIds: Schema.optional(Schema.Array(ProjectId).check(Schema.isMaxLength(100))), /** * Narrows the listing to one host, named as the host itself rather than as its provider kind: * github.com and a GitHub Enterprise install are two accounts, and a kind cannot tell them @@ -499,6 +592,14 @@ export const PullRequestDetail = Schema.Struct({ labels: Schema.Array(PullRequestLabel), checks: Schema.Array(PullRequestCheck), mergeCapabilities: PullRequestMergeCapabilities, + /** + * Where the branch stands against its base. Optional so a host that cannot compare says + * nothing rather than claiming the branch is current — the page shows a banner only where the + * answer is "behind", and silence is not that answer. + */ + baseComparison: Schema.optional(PullRequestBaseComparison), + /** How many commits the base is ahead by, where the host counted them. */ + behindBy: Schema.optional(NonNegativeInt), }); export type PullRequestDetail = typeof PullRequestDetail.Type; @@ -562,6 +663,14 @@ export const PullRequestDiffInput = Schema.Struct({ }); export type PullRequestDiffInput = typeof PullRequestDiffInput.Type; +/** Real line counts for a file whose hunks the host withheld from the patch. */ +export const PullRequestOmittedFileStat = Schema.Struct({ + path: TrimmedNonEmptyString, + additions: Schema.Number, + deletions: Schema.Number, +}); +export type PullRequestOmittedFileStat = typeof PullRequestOmittedFileStat.Type; + export const PullRequestDiffResult = Schema.Struct({ patch: Schema.String, /** @@ -571,6 +680,11 @@ export const PullRequestDiffResult = Schema.Struct({ truncated: Schema.Boolean, /** Where the next slice starts, or null once the diff is whole. */ nextCursor: Schema.NullOr(TrimmedNonEmptyString), + /** + * The host's own counts for the files whose hunks it withheld, so a file the patch cannot + * show still reports what changed instead of a zero the diff never had. + */ + omittedFileStats: Schema.optional(Schema.Array(PullRequestOmittedFileStat)), }); export type PullRequestDiffResult = typeof PullRequestDiffResult.Type; @@ -595,6 +709,8 @@ export const PullRequestActionInput = Schema.Struct({ ...PullRequestRef.fields, action: PullRequestAction, mergeMethod: Schema.optional(PullRequestMergeMethod), + /** Only read for `update-branch`, where absent means the host's own default. */ + updateMethod: Schema.optional(PullRequestUpdateMethod), }); export type PullRequestActionInput = typeof PullRequestActionInput.Type;