diff --git a/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts b/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts index cca65155..41ca00bf 100644 --- a/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts +++ b/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts @@ -326,6 +326,7 @@ export const make = Effect.fn("makeBitbucketPullRequestProvider")(function* () { row.reviews.find( (review) => review.author?.login.toLowerCase() === reviewer.login.toLowerCase(), )?.reviewState ?? "pending", + avatarUrl: reviewer.avatarUrl, })), checks: checks.items, // Bitbucket compares no branch with its base. diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts index 3ec458ec..bf258c04 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts @@ -189,6 +189,98 @@ describe("GitHubPullRequestProvider.setReviewerRequest", () => { ); }); +describe("GitHubPullRequestProvider.listChangeRequests", () => { + const listRow = (input: { + readonly number: number; + readonly author: Record; + }) => ({ + number: input.number, + title: `Pull request ${input.number}`, + url: `https://github.example.com/octocat/example-app/pull/${input.number}`, + author: input.author, + headRefName: `feature/${input.number}`, + baseRefName: "main", + state: "OPEN", + mergedAt: null, + isDraft: false, + additions: 1, + deletions: 0, + createdAt: "2026-08-30T10:00:00Z", + updatedAt: "2026-08-31T10:00:00Z", + reviewDecision: "", + reviewRequests: [], + labels: [], + }); + + it.effect("derives a plain login's picture and looks up the rest in one request", () => + Effect.gen(function* () { + mockExecute.mockImplementation((input) => + Effect.succeed( + processOutput( + input.args[0] === "api" + ? JSON.stringify({ + data: { + nodes: [{ login: "dependabot[bot]", avatarUrl: "https://avatars.example/bot" }], + }, + }) + : JSON.stringify([ + listRow({ number: 1, author: { login: "octocat", is_bot: false, id: "U_1" } }), + listRow({ + number: 2, + author: { login: "dependabot[bot]", is_bot: true, id: "BOT_1" }, + }), + ]), + ), + ), + ); + const provider = yield* GitHubPullRequestProvider.make(); + + const rows = yield* provider.listChangeRequests({ + ...repository, + state: "open", + limit: 30, + }); + + assert.deepStrictEqual( + rows.map((row) => [row.number, row.author?.avatarUrl]), + [ + // Derived from the login on the row's own host, so Enterprise works. + [1, "https://github.example.com/octocat.png?size=80"], + [2, "https://avatars.example/bot"], + ], + ); + // One listing, then one lookup naming only the account it had to ask about. + const lookup = calls()[1]; + assert.deepStrictEqual(lookup?.args, ["api", "graphql", "--input", "-"]); + assert.deepStrictEqual( + (JSON.parse(lookup?.stdin ?? "{}") as { variables: Record }).variables, + { ids: ["BOT_1"] }, + ); + expect(calls()).toHaveLength(2); + }).pipe(Effect.provide(layer)), + ); + + it.effect("asks the host nothing extra when every login speaks for itself", () => + Effect.gen(function* () { + mockExecute.mockReturnValue( + Effect.succeed( + processOutput( + JSON.stringify([ + listRow({ number: 1, author: { login: "octocat", is_bot: false, id: "U_1" } }), + ]), + ), + ), + ); + const provider = yield* GitHubPullRequestProvider.make(); + + const rows = yield* provider.listChangeRequests({ ...repository, state: "open", limit: 30 }); + + assert.equal(rows[0]?.author?.avatarUrl, "https://github.example.com/octocat.png?size=80"); + expect(calls()).toHaveLength(1); + }).pipe(Effect.provide(layer)), + ); +}); + describe("GitHubPullRequestProvider.listAuthoredChangeRequests", () => { it.effect("searches the whole host for the viewer's own work, on stdin", () => Effect.gen(function* () { diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts index 497002ae..9d34a183 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts @@ -7,11 +7,13 @@ import type * as Schema from "effect/Schema"; import type { PullRequestAction, PullRequestActivity, + PullRequestActor, PullRequestBaseComparison, PullRequestCapabilities, PullRequestComment, PullRequestListState, PullRequestMergeMethod, + PullRequestReviewer, PullRequestUpdateMethod, } from "@threadlines/contracts"; import { formatSchemaError } from "@threadlines/shared/schemaJson"; @@ -21,6 +23,12 @@ import { findAuthenticatedGitHubAccount, parseGitHubAuthStatus, } from "../sourceControl/gitHubAuthStatus.ts"; +import { + AVATAR_NODES_GRAPHQL_QUERY, + decodeGitHubAvatarNodesJson, + derivedGitHubAvatarUrl, + gitHubHostFromUrl, +} from "./gitHubAvatar.ts"; import { decodeGitHubPullRequestActivityJson, decodeGitHubPullRequestDetailJson, @@ -43,6 +51,7 @@ import { encodeGraphQlRequestJson, gitHubAuthoredSearchQuery, gitHubReactionContent, + type GitHubGraphQlVariable, PULL_REQUEST_CONVERSATION_GRAPHQL_QUERY, PULL_REQUEST_NODE_ID_GRAPHQL_QUERY, REACTION_SUBJECT_SCOPE_GRAPHQL_QUERY, @@ -241,7 +250,7 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () { readonly operation: string; readonly cwd: string; readonly query: string; - readonly variables: Readonly>; + readonly variables: Readonly>; }) => run({ operation: input.operation, @@ -254,7 +263,7 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () { readonly operation: string; readonly cwd: string; readonly query: string; - readonly variables: Readonly>; + readonly variables: Readonly>; readonly decode: (raw: string) => Result.Result>; }) => graphql(input).pipe( @@ -266,6 +275,83 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () { }), ); + /** + * The pictures for a set of rows, in as few requests as possible. A plain + * login's picture is at a known URL on the host the row is on, so most rows + * cost nothing; the accounts left over (an app account such as + * `dependabot[bot]`) are looked up together in one request by the node id + * `gh pr list --json author` already reports. + * + * A lookup that fails costs the pictures, not the listing: the client draws + * initials wherever a picture is missing. + */ + const withAuthorAvatars = < + A extends { + readonly url: string; + readonly author: PullRequestActor | null; + readonly authorId: string | null; + }, + >(input: { + readonly operation: string; + readonly cwd: string; + readonly rows: ReadonlyArray; + }): Effect.Effect, PullRequestProviderError> => { + const derived = (row: A) => + row.author === null + ? null + : (row.author.avatarUrl ?? + derivedGitHubAvatarUrl({ + host: gitHubHostFromUrl(row.url), + login: row.author.login, + isBot: row.author.isBot, + })); + + const ids = new Set( + input.rows.flatMap((row) => + row.author !== null && row.authorId !== null && derived(row) === null ? [row.authorId] : [], + ), + ); + + const lookup = + ids.size === 0 + ? Effect.succeed>(new Map()) + : graphqlRead({ + operation: input.operation, + cwd: input.cwd, + query: AVATAR_NODES_GRAPHQL_QUERY, + variables: { ids: [...ids] }, + decode: decodeGitHubAvatarNodesJson, + }).pipe(Effect.catch(() => Effect.succeed>(new Map()))); + + return lookup.pipe( + Effect.map((byLogin) => + input.rows.map((row) => { + const author = row.author; + if (author === null) { + return row; + } + const avatarUrl = derived(row) ?? byLogin.get(author.login.toLowerCase()) ?? null; + return avatarUrl === author.avatarUrl + ? row + : { ...row, author: { ...author, avatarUrl } }; + }), + ), + ); + }; + + /** + * A reviewer's picture, derived from their login. GitHub reports no picture + * with a review request, and a team is not an account the host serves one + * for, so a team keeps none. + */ + const withReviewerAvatar = (host: string | null) => (reviewer: PullRequestReviewer) => + reviewer.avatarUrl !== null || reviewer.kind !== "user" + ? reviewer + : { + ...reviewer, + avatarUrl: derivedGitHubAvatarUrl({ host, login: reviewer.login, isBot: false }), + }; + /** * `gh` takes a body by path. In argv it would show up in process listings and * run into the command length limit. @@ -398,6 +484,7 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () { ? Effect.succeed(decoded.success) : Effect.fail(decodeError("list", "PR list", decoded.failure)); }), + Effect.flatMap((rows) => withAuthorAvatars({ operation: "list", cwd: input.cwd, rows })), ), listAuthoredChangeRequests: (input) => @@ -431,6 +518,17 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () { ? Effect.succeed(decoded.success) : Effect.fail(decodeError("detail", "pull request", decoded.failure)); }), + Effect.flatMap((row) => + withAuthorAvatars({ operation: "detail", cwd: input.cwd, rows: [row] }).pipe( + Effect.map((rows) => rows[0] ?? row), + Effect.map((withAvatar) => ({ + ...withAvatar, + reviewers: withAvatar.reviewers.map( + withReviewerAvatar(gitHubHostFromUrl(withAvatar.url)), + ), + })), + ), + ), Effect.flatMap((row) => graphqlRead({ operation: "detail", @@ -498,6 +596,8 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () { ...comment, reactions: annotation.reactions, viewerIsAuthor: annotation.viewerIsAuthor, + // The GraphQL read is the only one that names a picture. + author: annotation.author ?? comment.author, }; }), commits: activity.commits, diff --git a/apps/server/src/pullRequest/GitLabPullRequestProvider.ts b/apps/server/src/pullRequest/GitLabPullRequestProvider.ts index bcc04642..04fb54ec 100644 --- a/apps/server/src/pullRequest/GitLabPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitLabPullRequestProvider.ts @@ -375,13 +375,21 @@ export const make = Effect.fn("makeGitLabPullRequestProvider")(function* () { kind: "user", login: reviewer.login, state: approved.has(reviewer.login.toLowerCase()) ? "approved" : "pending", + avatarUrl: reviewer.avatarUrl, })); // Somebody may approve without having been asked, which GitLab // reports on the approvals endpoint alone. const listed = new Set(reviewers.map((reviewer) => reviewer.login.toLowerCase())); for (const login of approvals) { if (!listed.has(login.toLowerCase())) { - reviewers.push({ id: login, kind: "user", login, state: "approved" }); + // The approvals endpoint names no picture with the account. + reviewers.push({ + id: login, + kind: "user", + login, + state: "approved", + avatarUrl: null, + }); } } return { diff --git a/apps/server/src/pullRequest/PullRequestProvider.ts b/apps/server/src/pullRequest/PullRequestProvider.ts index ede806da..f1f8bdb8 100644 --- a/apps/server/src/pullRequest/PullRequestProvider.ts +++ b/apps/server/src/pullRequest/PullRequestProvider.ts @@ -83,6 +83,11 @@ export interface ProviderChangeRequest { readonly labels: ReadonlyArray; /** Absent from a host that does not summarise its reviews. */ readonly reviewDecision?: PullRequestReviewDecision; + /** + * Absent from a host a listing cannot ask about it without a request per row. + * `unknown` is the host saying it has not finished checking. + */ + readonly mergeability?: PullRequestMergeability; /** Absent where there are no checks, or where the listing did not ask for them. */ readonly checksState?: PullRequestChecksState; } @@ -94,6 +99,13 @@ export interface ProviderChangeRequest { export interface ProviderAuthoredChangeRequest extends ProviderChangeRequest { /** Host-native repository identity, the same spelling a listing takes. */ readonly repository: string; + /** + * Whether the viewer may push to that repository, where the search says so. + * A workspace row learns the same thing from {@link ProviderRepositoryAccess}; + * a search covers repositories nobody here has checked out, so it has to + * carry the answer itself. Absent where the host named no permission. + */ + readonly viewerCanWrite?: boolean; } export interface ProviderChangeRequestDetail extends ProviderChangeRequest { diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index c23b20a9..2d535e81 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -80,6 +80,7 @@ const pullRequestRow = (input: { readonly number: number; readonly author: string; readonly reviewRequests?: ReadonlyArray>; + readonly mergeable?: string; }) => ({ number: input.number, title: `Pull request ${input.number}`, @@ -94,6 +95,7 @@ const pullRequestRow = (input: { deletions: 0, createdAt: "2026-08-30T10:00:00Z", updatedAt: "2026-08-31T10:00:00Z", + mergeable: input.mergeable ?? "UNKNOWN", reviewDecision: "", reviewRequests: input.reviewRequests ?? [], labels: [], @@ -326,6 +328,8 @@ const listAnswers = (handlers: { args: ReadonlyArray, ) => Effect.Effect; readonly authored?: () => Effect.Effect; + /** The repository read the listing makes for the viewer's own rights. */ + readonly repository?: () => Effect.Effect; }) => { mockExecute.mockImplementation((input) => { if (input.args[0] === "auth") { @@ -336,10 +340,19 @@ const listAnswers = (handlers: { ? Effect.succeed(processOutput(authoredSearchJson())) : handlers.authored(); } + if (input.args[0] === "api" && handlers.repository !== undefined) { + return handlers.repository(); + } return handlers.list(input.args); }); }; +/** The `gh api repos/...` reads a listing made, in the order it made them. */ +const repositoryReadCalls = () => + mockExecute.mock.calls + .map(([input]) => input.args) + .filter((args) => args[0] === "api" && args[1] !== "graphql"); + /** The one row a listing test that is not about the rows themselves answers with. */ const oneOpenRow = () => Effect.succeed(processOutput(JSON.stringify([pullRequestRow({ number: 1, author: "hubot" })]))); @@ -490,6 +503,61 @@ describe("PullRequestService.list", () => { }).pipe(Effect.provide(layer)), ); + it.effect("says whether the viewer may push to the repository a row is on", () => + Effect.gen(function* () { + withProjects([ + project({ + id: "project-checkout", + title: "Example App", + provider: "github", + repository: "octocat/example-app", + }), + project({ + id: "project-worktree", + title: "Example App worktree", + provider: "github", + repository: "Octocat/Example-App", + }), + ]); + listAnswers({ + list: oneOpenRow, + repository: () => Effect.succeed(processOutput(repositoryJson({ push: false }))), + }); + + const service = yield* PullRequestService.PullRequestService; + const result = yield* service.list({ state: "open" }); + + assert.deepStrictEqual( + result.entries.map((entry) => [entry.number, entry.viewerCanWrite]), + [[1, false]], + ); + // One read per repository, however many checkouts point at it. + assert.deepStrictEqual(repositoryReadCalls(), [["api", "repos/octocat/example-app"]]); + }).pipe(Effect.provide(layer)), + ); + + it.effect("leaves the viewer's rights unsaid when the repository read fails", () => + Effect.gen(function* () { + onlyGitHubProject(); + listAnswers({ + list: oneOpenRow, + repository: () => + Effect.fail( + new GitHubCli.GitHubCliError({ operation: "execute", detail: "Not Found (HTTP 404)" }), + ), + }); + + const service = yield* PullRequestService.PullRequestService; + const result = yield* service.list({ state: "open" }); + + assert.equal(result.entries.length, 1); + assert.equal(result.entries[0]?.viewerCanWrite, undefined); + // The rows still stand: a repository we could not read is not a project + // the user has to do anything about. + assert.deepStrictEqual(result.errors, []); + }).pipe(Effect.provide(layer)), + ); + it.effect("marks the viewer's own pull requests and pending review requests", () => Effect.gen(function* () { withProjects([ @@ -540,6 +608,43 @@ describe("PullRequestService.list", () => { }).pipe(Effect.provide(layer)), ); + it.effect("carries the author's picture and a conflict onto the row", () => + Effect.gen(function* () { + withProjects([ + project({ + id: "project-app", + title: "Example App", + provider: "github", + repository: "octocat/example-app", + }), + ]); + listAnswers({ + list: () => + Effect.succeed( + processOutput( + JSON.stringify([ + pullRequestRow({ number: 1, author: "hubot", mergeable: "CONFLICTING" }), + // The host has not finished checking, which is not an answer. + pullRequestRow({ number: 2, author: "hubot" }), + ]), + ), + ), + }); + + const service = yield* PullRequestService.PullRequestService; + const result = yield* service.list({ state: "open" }); + + assert.deepStrictEqual( + result.entries.map((entry) => [entry.number, entry.mergeability]), + [ + [1, "conflicting"], + [2, undefined], + ], + ); + assert.equal(result.entries[0]?.author?.avatarUrl, "https://github.com/hubot.png?size=80"); + }).pipe(Effect.provide(layer)), + ); + it.effect("reports one failing project and still returns the others", () => Effect.gen(function* () { withProjects([ diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 193e06a1..8461c150 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -73,6 +73,12 @@ const PULL_REQUEST_CACHE_CAPACITY = 32; /** Repository settings and access change far more rarely than a pull request. */ const REPOSITORY_CACHE_TTL = Duration.minutes(10); const REPOSITORY_CACHE_CAPACITY = 16; +/** + * The hosts whose list rows say whether the viewer may push. Every host can be + * asked, but only GitHub's answer is one the page has been built against, and a + * row from a host we do not ask says nothing rather than guessing. + */ +const WRITE_ACCESS_HOSTS: ReadonlySet = new Set(["github"]); /** The one thing a caller can get wrong that is not the host's fault. */ const FOREIGN_PULL_REQUEST_DETAIL = "Pull request is not in this workspace."; @@ -362,6 +368,8 @@ function toEntry(input: { * repository it is on where a workspace row says which project it is in. */ readonly repository?: string; + /** Push access on the row's repository; omitted where the host did not say. */ + readonly viewerCanWrite?: boolean; }): PullRequestListEntry { const { project, row } = input; const matchesViewer = viewerMatcher(input.viewer); @@ -385,13 +393,32 @@ function toEntry(input: { updatedAt: row.updatedAt, viewerIsAuthor: row.author !== null && matchesViewer(row.author.login), viewerReviewRequested: row.reviewRequestedLogins.some(matchesViewer), + ...(input.viewerCanWrite === undefined ? {} : { viewerCanWrite: input.viewerCanWrite }), ...(row.reviewDecision === undefined ? {} : { reviewDecision: row.reviewDecision }), ...(row.checksState === undefined ? {} : { checksState: row.checksState }), + // A host that has not finished checking says "unknown", which the row + // carries as nothing at all rather than as an answer. + ...(row.mergeability === undefined || row.mergeability === "unknown" + ? {} + : { mergeability: row.mergeability }), labels: row.labels, origin: input.origin, }; } +/** + * A workspace row with the viewer's push access on it, where the access read + * answered for its repository. A row whose repository is missing from the map + * is left exactly as it was: the field is the host having said, not a default. + */ +function withWriteAccess( + entry: PullRequestListEntry, + writeAccess: ReadonlyMap, +): PullRequestListEntry { + const canWrite = writeAccess.get(repositoryScopeKey(entry.provider, entry.repository)); + return canWrite === undefined ? entry : { ...entry, viewerCanWrite: canWrite }; +} + function toDetail(input: { readonly target: PullRequestTarget; readonly row: ProviderChangeRequestDetail; @@ -602,6 +629,9 @@ export const make = Effect.fn("makePullRequestService")(function* () { viewer, origin: "authored", repository: row.repository, + ...(row.viewerCanWrite === undefined + ? {} + : { viewerCanWrite: row.viewerCanWrite }), }), ] : [], @@ -627,62 +657,6 @@ export const make = Effect.fn("makePullRequestService")(function* () { ); }); - const loadList = Effect.fn("PullRequestService.load")(function* (key: PullRequestListCacheKey) { - const projects = dedupeProjectsByRemote(yield* readProjects(key.projectId)); - const first = projects[0]; - if (first === undefined) { - return { viewer: null, entries: [], errors: [] } satisfies PullRequestListResult; - } - - // The viewer is read per project, because each host signs in on its own and - // a workspace can hold projects on several. The cache is keyed by host and - // checkout, so projects that share one share the read. - const viewer = yield* readViewer(first); - const results = yield* Effect.forEach( - projects, - (project) => - readViewer(project).pipe( - Effect.flatMap((projectViewer) => - readProject({ project, state: key.state, viewer: projectViewer }), - ), - ), - { concurrency: PROJECT_CONCURRENCY }, - ); - - const entries = results.flatMap((result) => result.entries); - const errors = results.flatMap((result) => (result.error === null ? [] : [result.error])); - if (!key.includeAuthored) { - return { viewer, entries, errors } satisfies PullRequestListResult; - } - - const covered = new Set( - projects.map((project) => repositoryScopeKey(project.provider, project.repository)), - ); - const seen = new Set( - entries.map((entry) => listRowKey(entry.provider, entry.repository, entry.number)), - ); - const authored = yield* Effect.forEach( - firstProjectPerProvider(projects), - (anchor) => readAuthored({ anchor, state: key.state, covered, seen }), - { concurrency: PROJECT_CONCURRENCY }, - ); - - return { - viewer, - entries: [...entries, ...authored.flatMap((result) => result.entries)], - errors: [ - ...errors, - ...authored.flatMap((result) => (result.error === null ? [] : [result.error])), - ], - } satisfies PullRequestListResult; - }); - - const listCache = yield* Cache.make({ - capacity: LIST_CACHE_CAPACITY, - timeToLive: LIST_CACHE_TTL, - lookup: (key: string) => loadList(parseListCacheKey(key)), - }); - /** * Where a per-pull-request call runs: the project has to be one this build can * read pull requests from, since its checkout is what the host's tool is run @@ -746,6 +720,101 @@ export const make = Effect.fn("makePullRequestService")(function* () { repositoryCacheKey({ projectId: target.project.projectId, repository: target.repository }), ); + /** + * Whether the viewer may push to each workspace repository, keyed by + * {@link repositoryScopeKey}. It is the same cached read the detail makes, so + * a page that has opened a pull request pays nothing for it here. + * + * A repository the read fails on is left out rather than guessed at, and the + * listing itself never fails over one: the rows still stand, saying nothing + * about the viewer's rights, which is what a host that cannot say leaves too. + */ + const readWorkspaceWriteAccess = (projects: ReadonlyArray) => + Effect.forEach( + projects.filter((project) => WRITE_ACCESS_HOSTS.has(project.provider)), + (project) => + Cache.get( + repositoryCache, + repositoryCacheKey({ + projectId: project.projectId, + repository: project.repository, + }), + ).pipe( + Effect.map((access): ReadonlyArray => [ + [repositoryScopeKey(project.provider, project.repository), access.canWrite], + ]), + Effect.catch(() => Effect.succeed>([])), + ), + { concurrency: PROJECT_CONCURRENCY }, + ).pipe(Effect.map((reads) => new Map(reads.flat()))); + + const loadList = Effect.fn("PullRequestService.load")(function* (key: PullRequestListCacheKey) { + const projects = dedupeProjectsByRemote(yield* readProjects(key.projectId)); + const first = projects[0]; + if (first === undefined) { + return { viewer: null, entries: [], errors: [] } satisfies PullRequestListResult; + } + + // The viewer is read per project, because each host signs in on its own and + // a workspace can hold projects on several. The cache is keyed by host and + // checkout, so projects that share one share the read. + const viewer = yield* readViewer(first); + // The rows and what the viewer may do with them are asked for at once: the + // access read is per repository, not per row, and waiting for the rows + // first would only make the listing slower. + const [results, writeAccess] = yield* Effect.all( + [ + Effect.forEach( + projects, + (project) => + readViewer(project).pipe( + Effect.flatMap((projectViewer) => + readProject({ project, state: key.state, viewer: projectViewer }), + ), + ), + { concurrency: PROJECT_CONCURRENCY }, + ), + readWorkspaceWriteAccess(projects), + ], + { concurrency: 2 }, + ); + + const entries = results.flatMap((result) => + result.entries.map((entry) => withWriteAccess(entry, writeAccess)), + ); + const errors = results.flatMap((result) => (result.error === null ? [] : [result.error])); + if (!key.includeAuthored) { + return { viewer, entries, errors } satisfies PullRequestListResult; + } + + const covered = new Set( + projects.map((project) => repositoryScopeKey(project.provider, project.repository)), + ); + const seen = new Set( + entries.map((entry) => listRowKey(entry.provider, entry.repository, entry.number)), + ); + const authored = yield* Effect.forEach( + firstProjectPerProvider(projects), + (anchor) => readAuthored({ anchor, state: key.state, covered, seen }), + { concurrency: PROJECT_CONCURRENCY }, + ); + + return { + viewer, + entries: [...entries, ...authored.flatMap((result) => result.entries)], + errors: [ + ...errors, + ...authored.flatMap((result) => (result.error === null ? [] : [result.error])), + ], + } satisfies PullRequestListResult; + }); + + const listCache = yield* Cache.make({ + capacity: LIST_CACHE_CAPACITY, + timeToLive: LIST_CACHE_TTL, + lookup: (key: string) => loadList(parseListCacheKey(key)), + }); + const loadDetail = Effect.fn("PullRequestService.loadDetail")(function* ( key: PullRequestCacheKey, ) { diff --git a/apps/server/src/pullRequest/azureDevOpsPullRequest.ts b/apps/server/src/pullRequest/azureDevOpsPullRequest.ts index 98a89972..507f853e 100644 --- a/apps/server/src/pullRequest/azureDevOpsPullRequest.ts +++ b/apps/server/src/pullRequest/azureDevOpsPullRequest.ts @@ -31,6 +31,8 @@ const AzureIdentitySchema = Schema.Struct({ uniqueName: Schema.optional(Schema.NullOr(Schema.String)), /** How `az repos pr reviewer` names one, when Azure carries it. */ id: Schema.optional(Schema.NullOr(Schema.String)), + /** Azure's picture for the identity, which is not always a plain URL. */ + imageUrl: Schema.optional(Schema.NullOr(Schema.String)), vote: Schema.optional(Schema.NullOr(Schema.Int)), }); @@ -151,13 +153,31 @@ function normalizeRefName(refName: string): string { return refName.trim().replace(/^refs\/heads\//, ""); } +/** + * The picture Azure named, if it is one a browser can fetch on its own. Azure + * writes a relative path or a `data:` blob as readily as a URL, and only an + * absolute http one is worth handing the client; the rest draw initials. + */ +function toAvatarUrl(value: string | null | undefined): string | null { + const raw = trimmed(value); + if (raw === null) { + return null; + } + try { + const parsed = new URL(raw); + return parsed.protocol === "https:" || parsed.protocol === "http:" ? raw : null; + } catch { + return null; + } +} + /** A login has to compare against `az account show`, which reports an email. */ function toActor( raw: Schema.Schema.Type | null | undefined, ): PullRequestActor | null { const login = trimmed(raw?.uniqueName) ?? trimmed(raw?.displayName); // Azure names no bot flag on the identities it hands back. - return login === null ? null : { login, isBot: false }; + return login === null ? null : { login, isBot: false, avatarUrl: toAvatarUrl(raw?.imageUrl) }; } function toState(raw: Schema.Schema.Type): PullRequestState { @@ -252,6 +272,7 @@ function toRow( kind: "user", login: actor.login, state: toReviewerState(reviewer.vote), + avatarUrl: actor.avatarUrl, }, ]; }, diff --git a/apps/server/src/pullRequest/bitbucketPullRequest.test.ts b/apps/server/src/pullRequest/bitbucketPullRequest.test.ts index 40d46904..dcba53fa 100644 --- a/apps/server/src/pullRequest/bitbucketPullRequest.test.ts +++ b/apps/server/src/pullRequest/bitbucketPullRequest.test.ts @@ -65,7 +65,14 @@ describe("decodeBitbucketPullRequestPageJson", () => { JSON.stringify({ values: [ pullRequest({ - reviewers: [{ uuid: "{abc}", nickname: "hubot" }, { nickname: "nouuid" }], + reviewers: [ + { + uuid: "{abc}", + nickname: "hubot", + links: { avatar: { href: "https://avatars.example/hubot" } }, + }, + { nickname: "nouuid" }, + ], participants: [ { user: { nickname: "hubot" }, @@ -80,7 +87,9 @@ describe("decodeBitbucketPullRequestPageJson", () => { ), ); - assert.deepStrictEqual(page.items[0]?.reviewers, [{ id: "{abc}", login: "hubot" }]); + assert.deepStrictEqual(page.items[0]?.reviewers, [ + { id: "{abc}", login: "hubot", avatarUrl: "https://avatars.example/hubot" }, + ]); assert.deepStrictEqual( page.items[0]?.reviews.map((review) => [review.author?.login, review.reviewState]), [["hubot", "changes-requested"]], diff --git a/apps/server/src/pullRequest/bitbucketPullRequest.ts b/apps/server/src/pullRequest/bitbucketPullRequest.ts index a1b4c1c7..f6ef25f6 100644 --- a/apps/server/src/pullRequest/bitbucketPullRequest.ts +++ b/apps/server/src/pullRequest/bitbucketPullRequest.ts @@ -39,6 +39,16 @@ const BitbucketUserSchema = Schema.Struct({ /** Absent on an app account, which is why `display_name` stands in for it. */ nickname: Schema.optional(Schema.NullOr(Schema.String)), display_name: Schema.optional(Schema.NullOr(Schema.String)), + /** Bitbucket hangs the account's picture off its links, like everything else. */ + links: Schema.optional( + Schema.NullOr( + Schema.Struct({ + avatar: Schema.optional( + Schema.NullOr(Schema.Struct({ href: Schema.optional(Schema.NullOr(Schema.String)) })), + ), + }), + ), + ), }); /** @@ -188,7 +198,11 @@ export interface BitbucketPullRequestRow { readonly body: string; readonly reviewRequestedLogins: ReadonlyArray; /** The reviewers as Bitbucket addresses them, which is what a write takes. */ - readonly reviewers: ReadonlyArray<{ readonly id: string; readonly login: string }>; + readonly reviewers: ReadonlyArray<{ + readonly id: string; + readonly login: string; + readonly avatarUrl: string | null; + }>; /** Approvals and change requests, which Bitbucket keeps on its participants. */ readonly reviews: ReadonlyArray; } @@ -222,7 +236,9 @@ function toActor( ): PullRequestActor | null { const login = trimmed(raw?.nickname) ?? trimmed(raw?.display_name); // Bitbucket names no bot flag on the accounts it hands back. - return login === null ? null : { login, isBot: false }; + return login === null + ? null + : { login, isBot: false, avatarUrl: trimmed(raw?.links?.avatar?.href) }; } function toState(raw: Schema.Schema.Type): PullRequestState { @@ -303,7 +319,9 @@ function toRow( const reviewers = (raw.reviewers ?? []).flatMap((reviewer) => { const actor = toActor(reviewer); const id = trimmed(reviewer.uuid); - return actor === null || id === null ? [] : [{ id, login: actor.login }]; + return actor === null || id === null + ? [] + : [{ id, login: actor.login, avatarUrl: actor.avatarUrl }]; }); return { number: raw.id, @@ -674,6 +692,7 @@ export function decodeBitbucketWorkspaceMembersJson( kind: "user", login: actor.login, name: trimmed(decoded.value.user?.display_name), + avatarUrl: actor.avatarUrl, requested: false, }); } diff --git a/apps/server/src/pullRequest/gitHubAvatar.ts b/apps/server/src/pullRequest/gitHubAvatar.ts new file mode 100644 index 00000000..75a5b1a2 --- /dev/null +++ b/apps/server/src/pullRequest/gitHubAvatar.ts @@ -0,0 +1,110 @@ +import type * as Cause from "effect/Cause"; +import * as Result from "effect/Result"; +import * as Schema from "effect/Schema"; + +import { decodeJsonResult } from "@threadlines/shared/schemaJson"; + +import { nonEmptyText } from "./gitHubPullRequestList.ts"; + +type DecodeFailure = Cause.Cause; + +/** + * A GitHub account name as the web host will serve a picture for: letters, + * digits and dashes, up to the 39 characters a login may have. An app account + * (`dependabot[bot]`) fails it, and so does anything else that would make + * `/.png` mean a different account than the one asked about. + */ +const PLAIN_LOGIN = /^[a-z0-9][a-z0-9-]{0,38}$/i; + +/** Big enough for a retina 16px avatar, small enough to stay cheap. */ +const AVATAR_SIZE = 80; + +/** + * The web host a pull request lives on, read from its own URL so a GitHub + * Enterprise install serves its own pictures. Null when the host gave a URL + * nothing can be read from. + */ +export function gitHubHostFromUrl(url: string | null | undefined): string | null { + const trimmed = url?.trim() ?? ""; + if (trimmed.length === 0) { + return null; + } + try { + const parsed = new URL(trimmed); + return parsed.protocol === "https:" || parsed.protocol === "http:" ? parsed.host : null; + } catch { + return null; + } +} + +/** + * The picture a plain login's account is served at, which costs no request at + * all. Null for an app account and for anything else that has to be looked up: + * a bot's login is not a name the web host resolves, and guessing would show + * somebody else's face. + */ +export function derivedGitHubAvatarUrl(input: { + readonly host: string | null; + readonly login: string; + readonly isBot: boolean; +}): string | null { + if (input.host === null || input.isBot || !PLAIN_LOGIN.test(input.login.trim())) { + return null; + } + return `https://${input.host}/${input.login.trim()}.png?size=${AVATAR_SIZE}`; +} + +/** + * The pictures of the accounts a listing could not derive one for, asked about + * together. `nodes(ids:)` takes every id in one request, and both kinds of + * account that can open a pull request answer with a login and a picture. + */ +export const AVATAR_NODES_GRAPHQL_QUERY = `query($ids: [ID!]!) { + nodes(ids: $ids) { + ... on User { login avatarUrl } + ... on Bot { login avatarUrl } + } +}`; + +const RawAvatarNodesSchema = Schema.Struct({ + data: Schema.Struct({ + nodes: Schema.optional( + Schema.NullOr( + Schema.Array( + Schema.NullOr( + Schema.Struct({ + login: Schema.optional(Schema.NullOr(Schema.String)), + avatarUrl: Schema.optional(Schema.NullOr(Schema.String)), + }), + ), + ), + ), + ), + }), +}); + +const decodeAvatarNodes = decodeJsonResult(RawAvatarNodesSchema); + +/** + * The looked-up pictures, keyed by lowercased login: an id is only ever asked + * about on behalf of the login that carried it, and a node the host would not + * name is simply left out. + */ +export function decodeGitHubAvatarNodesJson( + raw: string, +): Result.Result, DecodeFailure> { + const decoded = decodeAvatarNodes(raw); + if (!Result.isSuccess(decoded)) { + return Result.fail(decoded.failure); + } + + const byLogin = new Map(); + for (const node of decoded.success.data.nodes ?? []) { + const login = nonEmptyText(node?.login); + const avatarUrl = nonEmptyText(node?.avatarUrl); + if (login !== null && avatarUrl !== null) { + byLogin.set(login.toLowerCase(), avatarUrl); + } + } + return Result.succeed(byLogin); +} diff --git a/apps/server/src/pullRequest/gitHubPullRequestDetail.test.ts b/apps/server/src/pullRequest/gitHubPullRequestDetail.test.ts index 27015b5c..63e28544 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestDetail.test.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestDetail.test.ts @@ -79,8 +79,8 @@ describe("decodeGitHubPullRequestDetailJson", () => { }); assert.deepStrictEqual(detail.reviewers, [ - { id: "hubot", kind: "user", login: "hubot", state: "pending" }, - { id: "monalisa", kind: "user", login: "monalisa", state: "approved" }, + { id: "hubot", kind: "user", login: "hubot", state: "pending", avatarUrl: null }, + { id: "monalisa", kind: "user", login: "monalisa", state: "approved", avatarUrl: null }, ]); }); diff --git a/apps/server/src/pullRequest/gitHubPullRequestDetail.ts b/apps/server/src/pullRequest/gitHubPullRequestDetail.ts index 92e38d8e..81e120ef 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestDetail.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestDetail.ts @@ -24,6 +24,7 @@ import { normalizeActor, normalizeCheckStatus, normalizeGitHubPullRequestListRow, + normalizeMergeability, type GitHubPullRequestListRow, } from "./gitHubPullRequestList.ts"; @@ -36,7 +37,6 @@ export const GITHUB_PULL_REQUEST_DETAIL_FIELDS = [ GITHUB_PULL_REQUEST_LIST_CHECKS_FIELD, "body", "changedFiles", - "mergeable", "closedAt", "reviews", "autoMergeRequest", @@ -133,7 +133,6 @@ const GitHubPullRequestDetailRowSchema = Schema.Struct({ ...GitHubPullRequestListRowSchema.fields, body: Schema.optional(Schema.NullOr(Schema.String)), changedFiles: Schema.optional(Schema.NullOr(NonNegativeInt)), - mergeable: Schema.optional(Schema.NullOr(Schema.String)), closedAt: Schema.optional(Schema.NullOr(Schema.String)), reviews: Schema.optional(Schema.NullOr(Schema.Array(GitHubReviewSchema))), /** An object while auto-merge is armed, null once it is not, absent on an older CLI. */ @@ -158,17 +157,6 @@ const STANDALONE_REVIEW_STATES = new Set([ "dismissed", ]); -function normalizeMergeability(value: string | null | undefined): PullRequestMergeability { - switch (value?.trim().toUpperCase()) { - case "MERGEABLE": - return "mergeable"; - case "CONFLICTING": - return "conflicting"; - default: - return "unknown"; - } -} - function normalizeReviewState(value: string | null | undefined): PullRequestReviewState | null { switch (value?.trim().toUpperCase()) { case "APPROVED": @@ -201,8 +189,9 @@ function normalizeReviewers(input: { for (const login of input.reviewRequestedLogins) { const key = login.toLowerCase(); if (key !== excluded) { - // A GitHub user is addressed by their login; team requests never reach here. - requested.set(key, { id: login, kind: "user", login, state: "pending" }); + // A GitHub user is addressed by their login; team requests never reach + // here. `gh pr view --json` names no picture, so the provider fills it. + requested.set(key, { id: login, kind: "user", login, state: "pending", avatarUrl: null }); } } @@ -221,7 +210,13 @@ function normalizeReviewers(input: { if (state === "commented" && previous !== undefined && previous.state !== "commented") { continue; } - reviewed.set(key, { id: login, kind: "user", login, state }); + reviewed.set(key, { + id: login, + kind: "user", + login, + state, + avatarUrl: normalizeActor(review.author)?.avatarUrl ?? null, + }); } return [...requested.values(), ...reviewed.values()]; @@ -393,7 +388,9 @@ export function decodeGitHubPullRequestDetailJson( ...base, body: row.body ?? "", changedFiles: row.changedFiles ?? 0, - mergeability: normalizeMergeability(row.mergeable), + // A host that has not finished its check says nothing, which the detail + // renders as "unknown" rather than leaving the field out. + mergeability: normalizeMergeability(row.mergeable) ?? "unknown", mergedAt: nonEmptyText(row.mergedAt), closedAt: nonEmptyText(row.closedAt), reviewers: normalizeReviewers({ diff --git a/apps/server/src/pullRequest/gitHubPullRequestGraphql.test.ts b/apps/server/src/pullRequest/gitHubPullRequestGraphql.test.ts index 07b8ac53..c0ea3c02 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestGraphql.test.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestGraphql.test.ts @@ -45,7 +45,7 @@ describe("decodeGitHubPullRequestConversationJson", () => { nodes: [ { id: "PRRC_1", - author: { login: "hubot" }, + author: { login: "hubot", avatarUrl: "https://avatars.example/u/2" }, body: "This reads twice.", createdAt: "2026-08-30T10:00:00Z", url: "https://github.com/octocat/example-app/pull/12#discussion_r1", @@ -90,6 +90,7 @@ describe("decodeGitHubPullRequestConversationJson", () => { { id: "IC_1", viewerDidAuthor: true, + author: { login: "octocat", avatarUrl: "https://avatars.example/u/1" }, reactionGroups: [reactionGroup("EYES", 3)], }, ], @@ -121,14 +122,23 @@ describe("decodeGitHubPullRequestConversationJson", () => { assert.deepStrictEqual(conversation.reviewThreads[0]?.comments[0]?.reactions, [ { content: "thumbs-up", count: 1, viewerReacted: true }, ]); + assert.deepStrictEqual(conversation.reviewThreads[0]?.comments[0]?.author, { + login: "hubot", + isBot: false, + avatarUrl: "https://avatars.example/u/2", + }); assert.equal(conversation.reviewThreads[1]?.comments[0]?.viewerIsAuthor, true); + // The JSON read names no picture, so the conversation carries the author + // GraphQL answered with alongside the reactions. assert.deepStrictEqual(conversation.annotationsByCommentId.get("IC_1"), { reactions: [{ content: "eyes", count: 3, viewerReacted: false }], viewerIsAuthor: true, + author: { login: "octocat", isBot: false, avatarUrl: "https://avatars.example/u/1" }, }); assert.deepStrictEqual(conversation.annotationsByCommentId.get("PRR_1"), { reactions: [], viewerIsAuthor: false, + author: null, }); }); }); @@ -255,8 +265,9 @@ describe("decodeGitHubAuthoredPullRequestsJson", () => { baseRefName: "main", additions: 8, deletions: 2, + mergeable: "CONFLICTING", reviewDecision: "CHANGES_REQUESTED", - author: { login: "octocat" }, + author: { login: "octocat", avatarUrl: "https://avatars.example/u/1" }, repository: { nameWithOwner: "openai/codex" }, labels: { nodes: [{ name: "bug", color: "d73a4a" }] }, reviewRequests: { nodes: [{ requestedReviewer: { login: "hubot" } }] }, @@ -277,7 +288,9 @@ describe("decodeGitHubAuthoredPullRequestsJson", () => { number: 12, title: "Teach the runner to wait", url: "https://github.com/openai/codex/pull/12", - author: { login: "octocat", isBot: false }, + // The search selects the picture, so no row needs looking up. + author: { login: "octocat", isBot: false, avatarUrl: "https://avatars.example/u/1" }, + authorId: null, headBranch: "fix/waiting", baseBranch: "main", state: "open", @@ -289,6 +302,7 @@ describe("decodeGitHubAuthoredPullRequestsJson", () => { reviewRequestedLogins: ["hubot"], reviewDecision: "changes-requested", checksState: "failure", + mergeability: "conflicting", labels: [{ name: "bug", color: "d73a4a" }], repository: "openai/codex", }, @@ -323,6 +337,34 @@ describe("decodeGitHubAuthoredPullRequestsJson", () => { assert.equal(checksState(null), undefined); }); + it("reads the viewer's rights on the repository the row is on", () => { + const viewerCanWrite = (permission: string | null) => + decoded( + decodeGitHubAuthoredPullRequestsJson( + JSON.stringify({ + data: { + search: { + nodes: [ + node({ + repository: { nameWithOwner: "openai/codex", viewerPermission: permission }, + }), + ], + }, + }, + }), + ), + "authored search", + )[0]?.viewerCanWrite; + + assert.equal(viewerCanWrite("ADMIN"), true); + assert.equal(viewerCanWrite("MAINTAIN"), true); + assert.equal(viewerCanWrite("WRITE"), true); + assert.equal(viewerCanWrite("TRIAGE"), false); + assert.equal(viewerCanWrite("READ"), false); + // A search that names no permission is not an answer either way. + assert.equal(viewerCanWrite(null), undefined); + }); + it("drops a search hit that is not a pull request", () => { const rows = decoded( decodeGitHubAuthoredPullRequestsJson( diff --git a/apps/server/src/pullRequest/gitHubPullRequestGraphql.ts b/apps/server/src/pullRequest/gitHubPullRequestGraphql.ts index e4f58154..8eb08102 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestGraphql.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestGraphql.ts @@ -3,6 +3,7 @@ import * as Result from "effect/Result"; import * as Schema from "effect/Schema"; import type { + PullRequestActor, PullRequestListState, PullRequestReaction, PullRequestReactionContent, @@ -56,7 +57,7 @@ export const PULL_REQUEST_CONVERSATION_GRAPHQL_QUERY = `query($owner: String!, $ comments(first: ${GRAPHQL_PAGE_SIZE}) { nodes { id - author { login } + author { login avatarUrl } body createdAt url @@ -67,10 +68,10 @@ export const PULL_REQUEST_CONVERSATION_GRAPHQL_QUERY = `query($owner: String!, $ } } comments(first: ${GRAPHQL_PAGE_SIZE}) { - nodes { id viewerDidAuthor ${REACTION_GROUPS_FIELDS} } + nodes { id viewerDidAuthor author { login avatarUrl } ${REACTION_GROUPS_FIELDS} } } reviews(first: ${GRAPHQL_PAGE_SIZE}) { - nodes { id viewerDidAuthor ${REACTION_GROUPS_FIELDS} } + nodes { id viewerDidAuthor author { login avatarUrl } ${REACTION_GROUPS_FIELDS} } } } } @@ -104,8 +105,9 @@ const AUTHORED_CONNECTION_PAGE_SIZE = 20; * * `first` is a variable so the listing asks for as many rows as it would from a * repository. The fields are the ones a list row is built from, plus the - * repository each one is on and the check rollup of its last commit, since a - * search cannot be asked for `statusCheckRollup` the way `gh pr list` is. + * repository each one is on, what the viewer may do there, and the check rollup + * of its last commit, since a search cannot be asked for `statusCheckRollup` + * the way `gh pr list` is. */ export const AUTHORED_PULL_REQUESTS_GRAPHQL_QUERY = `query($q: String!, $first: Int!) { search(query: $q, type: ISSUE, first: $first) { @@ -123,9 +125,10 @@ export const AUTHORED_PULL_REQUESTS_GRAPHQL_QUERY = `query($q: String!, $first: baseRefName additions deletions + mergeable reviewDecision - author { login } - repository { nameWithOwner } + author { login avatarUrl } + repository { nameWithOwner viewerPermission } labels(first: ${AUTHORED_CONNECTION_PAGE_SIZE}) { nodes { name color } } reviewRequests(first: ${AUTHORED_CONNECTION_PAGE_SIZE}) { nodes { requestedReviewer { ... on User { login } } } @@ -158,16 +161,16 @@ export function gitHubAuthoredSearchQuery(input: { export const REVIEWER_CANDIDATES_GRAPHQL_QUERY = `query($owner: String!, $name: String!, $number: Int!) { repository(owner: $owner, name: $name) { assignableUsers(first: ${GRAPHQL_PAGE_SIZE}) { - nodes { login name } + nodes { login name avatarUrl } } pullRequest(number: $number) { author { login } reviewRequests(first: ${GRAPHQL_PAGE_SIZE}) { nodes { requestedReviewer { - ... on User { login name } - ... on Team { slug name } - ... on Bot { login } + ... on User { login name avatarUrl } + ... on Team { slug name avatarUrl } + ... on Bot { login avatarUrl } } } } @@ -238,11 +241,20 @@ export const UPDATE_REVIEW_COMMENT_GRAPHQL_MUTATION = `mutation($commentId: ID!, } }`; +/** A list of ids is a variable too: one read asks about a batch of accounts. */ +export type GitHubGraphQlVariable = string | number | boolean | null | ReadonlyArray; + const GraphQlRequestSchema = Schema.Struct({ query: Schema.String, variables: Schema.Record( Schema.String, - Schema.Union([Schema.String, Schema.Number, Schema.Boolean, Schema.Null]), + Schema.Union([ + Schema.String, + Schema.Number, + Schema.Boolean, + Schema.Null, + Schema.Array(Schema.String), + ]), ), }); @@ -255,7 +267,7 @@ const encodeGraphQlRequest = Schema.encodeSync(Schema.fromJsonString(GraphQlRequ */ export function encodeGraphQlRequestJson(input: { readonly query: string; - readonly variables: Readonly>; + readonly variables: Readonly>; }): string { return encodeGraphQlRequest({ query: input.query, variables: { ...input.variables } }); } @@ -336,6 +348,7 @@ const RawThreadCommentSchema = Schema.Struct({ const RawAnnotatedNodeSchema = Schema.Struct({ id: Schema.optional(Schema.NullOr(Schema.String)), viewerDidAuthor: Schema.optional(Schema.NullOr(Schema.Boolean)), + author: Schema.optional(Schema.NullOr(GitHubAuthorSchema)), reactionGroups: RawReactionGroupsSchema, }); @@ -386,6 +399,8 @@ const RawConversationSchema = Schema.Struct({ export interface GitHubCommentAnnotation { readonly reactions: ReadonlyArray; readonly viewerIsAuthor: boolean; + /** The author with their picture, which `gh pr view --json` never carries. */ + readonly author: PullRequestActor | null; } /** Everything one activity read learns from GraphQL, keyed the way it is used. */ @@ -459,6 +474,7 @@ export function decodeGitHubPullRequestConversationJson( annotationsByCommentId.set(id, { reactions: toReactions(node.reactionGroups), viewerIsAuthor: node.viewerDidAuthor === true, + author: normalizeActor(node.author), }); } @@ -523,10 +539,16 @@ const RawAuthoredNodeSchema = Schema.Struct({ baseRefName: Schema.optional(Schema.NullOr(Schema.String)), additions: Schema.optional(Schema.NullOr(Schema.Number)), deletions: Schema.optional(Schema.NullOr(Schema.Number)), + mergeable: Schema.optional(Schema.NullOr(Schema.String)), reviewDecision: Schema.optional(Schema.NullOr(Schema.String)), author: Schema.optional(Schema.NullOr(GitHubAuthorSchema)), repository: Schema.optional( - Schema.NullOr(Schema.Struct({ nameWithOwner: Schema.optional(Schema.NullOr(Schema.String)) })), + Schema.NullOr( + Schema.Struct({ + nameWithOwner: Schema.optional(Schema.NullOr(Schema.String)), + viewerPermission: Schema.optional(Schema.NullOr(Schema.String)), + }), + ), ), labels: Schema.optional( Schema.NullOr( @@ -599,6 +621,27 @@ const decodeAuthoredSearch = decodeJsonResult(RawAuthoredSearchSchema); /** One of the viewer's own pull requests, and the repository it is on. */ export interface GitHubAuthoredPullRequestRow extends GitHubPullRequestListRow { readonly repository: string; + /** Absent where the search named no permission on the repository. */ + readonly viewerCanWrite?: boolean; +} + +/** + * Whether a `RepositoryPermission` is push access, or null where GitHub named + * none. Admin and maintain both include write; triage and read do not, and + * anything unrecognised is treated as an answer we do not have. + */ +function toViewerCanWrite(permission: string | null | undefined): boolean | null { + switch (nonEmptyText(permission)?.toUpperCase() ?? null) { + case "ADMIN": + case "MAINTAIN": + case "WRITE": + return true; + case "READ": + case "TRIAGE": + return false; + default: + return null; + } } /** @@ -624,6 +667,7 @@ function toGitHubListRowShape(node: RawAuthoredNode): unknown { deletions: node.deletions, createdAt: node.createdAt, updatedAt: node.updatedAt, + mergeable: node.mergeable, reviewDecision: node.reviewDecision, reviewRequests: (node.reviewRequests?.nodes ?? []).map((request) => ({ login: request?.requestedReviewer?.login ?? null, @@ -657,7 +701,12 @@ export function decodeGitHubAuthoredPullRequestsJson( } const row = decodeGitHubPullRequestListRow(toGitHubListRowShape(node)); if (row !== null) { - rows.push({ ...row, repository }); + const viewerCanWrite = toViewerCanWrite(node.repository?.viewerPermission); + rows.push({ + ...row, + repository, + ...(viewerCanWrite === null ? {} : { viewerCanWrite }), + }); } } return Result.succeed(rows); @@ -720,6 +769,7 @@ const RawRequestedReviewerSchema = Schema.Struct({ login: Schema.optional(Schema.NullOr(Schema.String)), slug: Schema.optional(Schema.NullOr(Schema.String)), name: Schema.optional(Schema.NullOr(Schema.String)), + avatarUrl: Schema.optional(Schema.NullOr(Schema.String)), }); const RawReviewerCandidatesSchema = Schema.Struct({ @@ -787,6 +837,7 @@ export function decodeGitHubReviewerCandidatesJson( kind, login: id, name: nonEmptyText(node.requestedReviewer?.name), + avatarUrl: nonEmptyText(node.requestedReviewer?.avatarUrl), requested: true, }); } @@ -805,6 +856,7 @@ export function decodeGitHubReviewerCandidatesJson( kind: "user", login, name: nonEmptyText(node?.name), + avatarUrl: nonEmptyText(node?.avatarUrl), requested: false, }); } diff --git a/apps/server/src/pullRequest/gitHubPullRequestList.test.ts b/apps/server/src/pullRequest/gitHubPullRequestList.test.ts index f62a3dca..4c2518ea 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestList.test.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestList.test.ts @@ -93,6 +93,31 @@ describe("decodeGitHubPullRequestListJson", () => { ); }); + it("carries the merge state, the author's node id, and no picture of its own", () => { + const rows = decodeRows([ + { ...baseRow, number: 1, mergeable: "CONFLICTING" }, + { ...baseRow, number: 2, mergeable: "UNKNOWN" }, + { ...baseRow, number: 3, author: { login: "dependabot[bot]", is_bot: true, id: "BOT_1" } }, + { ...baseRow, number: 4 }, + ]); + + assert.deepStrictEqual( + rows.map((row) => [row.number, row.mergeability, row.authorId]), + [ + [1, "conflicting", null], + [2, "unknown", null], + [3, undefined, "BOT_1"], + [4, undefined, null], + ], + ); + // `gh pr list --json author` names no picture; the provider resolves it. + assert.deepStrictEqual(rows[0]?.author, { + login: "octocat", + isBot: false, + avatarUrl: null, + }); + }); + it("skips a malformed row and keeps the rest", () => { const rows = decodeRows([ { ...baseRow, number: 0 }, diff --git a/apps/server/src/pullRequest/gitHubPullRequestList.ts b/apps/server/src/pullRequest/gitHubPullRequestList.ts index deffa543..e1343919 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestList.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestList.ts @@ -6,8 +6,10 @@ import { NonNegativeInt, PositiveInt, TrimmedNonEmptyString, + type PullRequestActor, type PullRequestCheckStatus, type PullRequestChecksState, + type PullRequestMergeability, type PullRequestReviewDecision, type PullRequestState, } from "@threadlines/contracts"; @@ -33,6 +35,7 @@ export const GITHUB_PULL_REQUEST_LIST_FIELDS = [ "createdAt", "updatedAt", "mergedAt", + "mergeable", "reviewDecision", "reviewRequests", "labels", @@ -45,7 +48,13 @@ export interface GitHubPullRequestListRow { readonly number: number; readonly title: string; readonly url: string; - readonly author: { readonly login: string; readonly isBot: boolean } | null; + /** `avatarUrl` is null until the provider resolves it; `gh` reports none. */ + readonly author: PullRequestActor | null; + /** + * The author's node id, which is how an account whose picture cannot be + * derived from its login is looked up. Null where the host named none. + */ + readonly authorId: string | null; readonly headBranch: string; readonly baseBranch: string; readonly state: PullRequestState; @@ -58,6 +67,8 @@ export interface GitHubPullRequestListRow { readonly reviewRequestedLogins: ReadonlyArray; readonly reviewDecision?: PullRequestReviewDecision; readonly checksState?: PullRequestChecksState; + /** Absent where the host said nothing about whether the branch still merges. */ + readonly mergeability?: PullRequestMergeability; readonly labels: ReadonlyArray<{ readonly name: string; readonly color: string | null }>; } @@ -65,6 +76,10 @@ export const GitHubAuthorSchema = Schema.Struct({ login: Schema.String, is_bot: Schema.optional(Schema.NullOr(Schema.Boolean)), isBot: Schema.optional(Schema.NullOr(Schema.Boolean)), + /** The node id `gh` reports beside a login, and GraphQL looks an account up by. */ + id: Schema.optional(Schema.NullOr(Schema.String)), + /** GraphQL selects it; `gh pr list --json author` never carries one. */ + avatarUrl: Schema.optional(Schema.NullOr(Schema.String)), }); const GitHubLabelSchema = Schema.Struct({ @@ -107,6 +122,7 @@ export const GitHubPullRequestListRowSchema = Schema.Struct({ deletions: Schema.optional(Schema.NullOr(NonNegativeInt)), createdAt: TrimmedNonEmptyString, updatedAt: TrimmedNonEmptyString, + mergeable: Schema.optional(Schema.NullOr(Schema.String)), reviewDecision: Schema.optional(Schema.NullOr(Schema.String)), reviewRequests: Schema.optional(Schema.NullOr(Schema.Array(GitHubReviewRequestSchema))), labels: Schema.optional(Schema.NullOr(Schema.Array(GitHubLabelSchema))), @@ -130,12 +146,45 @@ export function nonEmptyText(value: string | null | undefined): string | null { return trimmed.length > 0 ? trimmed : null; } -/** The `PullRequestActor` behind a `gh` author object, or null when unnamed. */ +/** + * The `PullRequestActor` behind a `gh` author object, or null when unnamed. + * `avatarUrl` is whatever the payload carried: GraphQL selects one, while + * `gh pr view --json` reports none and leaves the picture to be resolved. + */ export function normalizeActor( raw: Schema.Schema.Type | null | undefined, -): { readonly login: string; readonly isBot: boolean } | null { +): PullRequestActor | null { const login = nonEmptyText(raw?.login); - return login === null ? null : { login, isBot: raw?.is_bot === true || raw?.isBot === true }; + return login === null + ? null + : { + login, + isBot: raw?.is_bot === true || raw?.isBot === true, + avatarUrl: nonEmptyText(raw?.avatarUrl), + }; +} + +/** The node id beside a `gh` author, which is what an avatar lookup addresses. */ +export function actorNodeId( + raw: Schema.Schema.Type | null | undefined, +): string | null { + return nonEmptyText(raw?.id); +} + +/** How GitHub says a branch stands against its base, in the contract's words. */ +export function normalizeMergeability( + value: string | null | undefined, +): PullRequestMergeability | undefined { + switch (value?.trim().toUpperCase()) { + case "MERGEABLE": + return "mergeable"; + case "CONFLICTING": + return "conflicting"; + case "UNKNOWN": + return "unknown"; + default: + return undefined; + } } function normalizeState(raw: { @@ -222,12 +271,14 @@ export function normalizeGitHubPullRequestListRow( ): GitHubPullRequestListRow { const reviewDecision = normalizeReviewDecision(raw.reviewDecision); const checksState = normalizeChecksState(raw.statusCheckRollup); + const mergeability = normalizeMergeability(raw.mergeable); return { number: raw.number, title: raw.title, url: raw.url, author: normalizeActor(raw.author), + authorId: actorNodeId(raw.author), headBranch: raw.headRefName, baseBranch: raw.baseRefName, state: normalizeState(raw), @@ -246,6 +297,7 @@ export function normalizeGitHubPullRequestListRow( }), ...(reviewDecision === undefined ? {} : { reviewDecision }), ...(checksState === undefined ? {} : { checksState }), + ...(mergeability === undefined ? {} : { mergeability }), labels: (raw.labels ?? []).flatMap((label) => { const name = nonEmptyText(label.name); return name === null ? [] : [{ name, color: nonEmptyText(label.color) }]; diff --git a/apps/server/src/pullRequest/gitLabMergeRequest.ts b/apps/server/src/pullRequest/gitLabMergeRequest.ts index 64c361ab..8a0fe5e3 100644 --- a/apps/server/src/pullRequest/gitLabMergeRequest.ts +++ b/apps/server/src/pullRequest/gitLabMergeRequest.ts @@ -42,6 +42,7 @@ const GitLabUserSchema = Schema.Struct({ id: Schema.optional(Schema.NullOr(Schema.Int)), username: Schema.optional(Schema.NullOr(Schema.String)), name: Schema.optional(Schema.NullOr(Schema.String)), + avatar_url: Schema.optional(Schema.NullOr(Schema.String)), }); const GitLabPipelineSchema = Schema.Struct({ @@ -206,6 +207,8 @@ export interface GitLabMergeRequestRow { readonly baseBranch: string; readonly state: PullRequestState; readonly isDraft: boolean; + /** `unknown` while GitLab is still checking, which every listing may see. */ + readonly mergeability: PullRequestMergeability; readonly createdAt: string; readonly updatedAt: string; readonly reviewRequestedLogins: ReadonlyArray; @@ -223,11 +226,14 @@ export interface GitLabDiffRefs { export interface GitLabMergeRequestDetailRow extends GitLabMergeRequestRow { readonly body: string; readonly changedFiles: number; - readonly mergeability: PullRequestMergeability; readonly mergedAt: string | null; readonly closedAt: string | null; /** Requested reviewers, keyed by the numeric id a reviewer write takes. */ - readonly reviewers: ReadonlyArray<{ readonly id: string; readonly login: string }>; + readonly reviewers: ReadonlyArray<{ + readonly id: string; + readonly login: string; + readonly avatarUrl: string | null; + }>; readonly checks: ReadonlyArray; /** Null where GitLab named neither auto-merge field, which is not "off". */ readonly autoMergeEnabled: boolean | null; @@ -247,7 +253,7 @@ function toActor( raw: Schema.Schema.Type | null | undefined, ): PullRequestActor | null { const login = trimmed(raw?.username); - return login === null ? null : { login, isBot: false }; + return login === null ? null : { login, isBot: false, avatarUrl: trimmed(raw?.avatar_url) }; } function toState(raw: Schema.Schema.Type): PullRequestState { @@ -366,6 +372,7 @@ function toRow(raw: Schema.Schema.Type): GitLab baseBranch: raw.target_branch, state: toState(raw), isDraft: raw.draft === true || raw.work_in_progress === true, + mergeability: toMergeability(raw), createdAt: raw.created_at, updatedAt: raw.updated_at, reviewRequestedLogins: (raw.reviewers ?? []).flatMap((reviewer) => { @@ -399,12 +406,13 @@ function toDetailRow( ...toRow(raw), body: raw.description ?? "", changedFiles: toChangedFiles(raw.changes_count), - mergeability: toMergeability(raw), mergedAt: trimmed(raw.merged_at), closedAt: trimmed(raw.closed_at), reviewers: (raw.reviewers ?? []).flatMap((reviewer) => { - const login = trimmed(reviewer.username); - return login === null || reviewer.id == null ? [] : [{ id: String(reviewer.id), login }]; + const actor = toActor(reviewer); + return actor === null || reviewer.id == null + ? [] + : [{ id: String(reviewer.id), login: actor.login, avatarUrl: actor.avatarUrl }]; }), checks: toChecks(raw), autoMergeEnabled: autoMerge, @@ -768,6 +776,7 @@ export function decodeGitLabProjectUsersJson( kind: "user", login, name: trimmed(decoded.value.name), + avatarUrl: trimmed(decoded.value.avatar_url), requested: false, }); } diff --git a/apps/web/src/components/PullRequestThreadDialog.tsx b/apps/web/src/components/PullRequestThreadDialog.tsx index ca50efe0..8fe7a346 100644 --- a/apps/web/src/components/PullRequestThreadDialog.tsx +++ b/apps/web/src/components/PullRequestThreadDialog.tsx @@ -30,6 +30,12 @@ interface PullRequestThreadDialogProps { threadId: ThreadId; cwd: string | null; initialReference: string | null; + /** + * The way in the caller asked for, which this opens on: its button takes the + * emphasis and Enter in the field runs it. Both ways stay offered, because + * this is where someone changes their mind about which one they wanted. + */ + defaultMode?: "local" | "worktree"; onOpenChange: (open: boolean) => void; onPrepared: (input: { branch: string; worktreePath: string | null }) => Promise | void; } @@ -40,6 +46,7 @@ export function PullRequestThreadDialog({ threadId, cwd, initialReference, + defaultMode, onOpenChange, onPrepared, }: PullRequestThreadDialogProps) { @@ -225,7 +232,7 @@ export function PullRequestThreadDialog({ } event.preventDefault(); if (!isResolving && !preparePullRequestThreadMutation.isPending) { - void handleConfirm("local"); + void handleConfirm(defaultMode ?? "local"); } }} /> @@ -270,7 +277,7 @@ export function PullRequestThreadDialog({ ) : null} + {/* The rollup the header used to spell out on a line of its own. It is + a way into the Summary's Checks section rather than a label, so it + is a button wherever that section is what the tab is showing. */} + {activeTab === "summary" ? ( + + ) : null} @@ -491,6 +543,16 @@ function DiffViewerWarmup({ return enabled ? {children} : children; } +/** + * Everything above the tabs, in five rows: where this pull request lives and + * what can be done to it, its title, who wrote it and when it last moved, the + * branches it joins and how big it is, and the tabs themselves (drawn by the + * panel, since only it knows which one is open). + * + * The same five rows serve the page and a thread's Pull request tab. Only the + * first row differs: the page can check the branch out and can close the panel, + * and a thread's own chrome already owns both. + */ function PullRequestDetailHeader({ environmentId, reference, @@ -501,7 +563,7 @@ function PullRequestDetailHeader({ isRefreshing, onRefresh, onClose, - onReviewInThread, + onCheckout, onOpenThread, handoffs, }: { @@ -514,83 +576,122 @@ function PullRequestDetailHeader({ readonly isRefreshing: boolean; readonly onRefresh: () => void; readonly onClose?: () => void; - readonly onReviewInThread?: () => void; + readonly onCheckout?: (request?: PullRequestCheckoutRequest) => void; readonly onOpenThread: () => void; readonly handoffs: PullRequestHandoffActions | null; }) { const tone = pullRequestBadgeTone(detail.state, detail.isDraft); - const meta: readonly { key: string; node: React.ReactNode }[] = [ - ...(detail.author ? [{ key: "author", node: {detail.author.login} }] : []), - { key: "updated", node: updated {formatRelativeTimeLabel(detail.updatedAt)} }, - { key: "files", node: {pluralize(detail.changedFiles, "file")} }, - ...(detail.additions > 0 || detail.deletions > 0 - ? [ - { - key: "stat", - node: , - }, - ] - : []), - ...(detail.autoMergeEnabled === true - ? [{ key: "auto-merge", node: Auto-merge on }] - : []), - // Open is the resting state and the glyph already says it; the other three - // are news, so they get a word. - ...(detail.state === "open" && !detail.isDraft - ? [] - : [{ key: "state", node: {tone.label} }]), - ]; - // Three labels is as many as the meta line can carry and still read as facts - // about the branch rather than a wall of tags. - const visibleLabels = detail.labels.slice(0, 3); - const hiddenLabels = detail.labels.slice(3); + const actions = usePullRequestActions({ environmentId, reference, detail, handoffs }); + // A branch that no longer merges, said where the branches are named rather + // than on a line of its own. + const conflictLabel = + detail.state === "open" && !detail.isDraft && detail.mergeability === "conflicting" + ? `Conflicts with ${detail.baseBranch}` + : null; + const behindLabel = formatPullRequestBehindLabel(detail); + const freshness = formatPullRequestBaseFreshness(detail); + // Open is the resting state and the glyph already says it; the other three + // are news, so they get a word. + const stateWord = detail.state === "open" && !detail.isDraft ? null : tone.label; + // The page's way to start work on the branch, and to get back to the thread + // already doing it. A thread's own tab is standing in that thread already. + const showCheckout = context === "page" && (onCheckout !== undefined || linkedThread !== null); return ( -
-
- {context === "page" && onClose ? : null} - - - {tone.label} - - +
+ {/* Row 1: where it lives, and what can be done to it. */} +
+
+ {/* On a phone the list is not on screen, so the way back sits where + a back arrow belongs: first, at the top left. */} + {context === "page" && onClose ? : null} + + + {tone.label} + + {detail.repository} + + {stateWord ? {stateWord} : null} + {detail.autoMergeEnabled === true ? ( + Auto-merge on + ) : null} +
+
+ {showCheckout ? ( + + ) : null} + {actions.controls} + + {/* Beside a thread the tab strip owns the dismissal; a second ✕ next + to it would be two controls for one action. Below the two-column + width the back arrow at the row's start stands in for it. */} + {context === "page" && onClose ? : null} +
+
+ + {/* Row 2: the title. */} +
- - {/* Beside a thread the tab strip owns the dismissal; a second ✕ next - to it would be two controls for one action. */} - {context === "page" && onClose ? : null}
-
- - - → + {/* Row 3: who wrote it, when it last moved, and the one command that + takes the branch on a machine this app is not running on. */} +
+ + {detail.author ? ( + <> + + + + ) : null} + updated {formatRelativeTimeLabel(detail.updatedAt)} - + +
+ + {/* Row 4: the branches this joins, and how much it changes. Below `md` + the counts drop to a line of their own: the branch names are what the + row is for, and sharing the line leaves them a letter each. */} +
+ + {conflictLabel ? ( + + + + {conflictLabel} + + + ) : null} {/* A base that is not the default branch means this sits on other work, which changes how its diff should be read. */} {detail.isStacked ? ( @@ -601,81 +702,187 @@ function PullRequestDetailHeader({ ) : null} - - {meta.map((item) => ( - - - {item.node} - - ))} - {visibleLabels.map((label) => ( - - - - - {label.name} - - - ))} - {/* A long label list would push the facts off the line, so the rest is - a count that names them on hover. */} - {hiddenLabels.length > 0 ? ( - - - label.name).join(", ")}> - +{hiddenLabels.length} + + + {/* Being behind the base is the reason a merge is refused or a check + is stale, so the branch line says so where it names the branch. */} + {behindLabel ? ( + + + {behindLabel} + + ) : null} + + + + + {pluralize(detail.changedFiles, "file")} - ) : null} + {detail.additions > 0 || detail.deletions > 0 ? ( + + ) : null} +
- {/* A branch the host cannot merge is work for the checkout, not for the - host's own buttons, so the way out is offered where the problem is - stated rather than only as a reason the Merge button is off. */} - {detail.mergeability === "conflicting" && handoffs?.resolveConflicts ? ( -
- Conflicts with {detail.baseBranch} - -
- ) : null} + {actions.notices} + {actions.dialog} +
+ ); +} - {context === "page" ? ( -
- {linkedThread ? ( - - ) : onReviewInThread ? ( - - ) : null} -
+ + + In a worktree + + Takes the branch into its own folder, leaving this one alone. + + + + onCheckout({ mode: "local" })} + > + + + In this repository + + Switches the branch you are working in, the way{" "} + {checkoutCommand(detail.provider, detail.number) ?? "a checkout"} does. + + + + + ) : null} + + + ); +} + +/** + * The rollup of the checks, beside the tabs: a glyph, a phrase, and a way down + * to the rows it counts. The glyph is drawn here rather than taken whole from + * the presentation module, because inside a button a second tooltip trigger + * would fight the button for the pointer. + */ +function PullRequestChecksRollup({ + summary, + onShowChecks, +}: { + readonly summary: PullRequestChecksSummary; + readonly onShowChecks: () => void; +}) { + const tone = pullRequestChecksTone(summary.state); + return ( + + ); +} - -
+/** + * The command that takes this branch on a machine Threadlines is not running + * on, as a button that copies it. Only where the host's own tool has one: `gh` + * is GitHub's, and spelling it for a host that does not answer to it would be + * a command that fails when it is pasted. + */ +function checkoutCommand(provider: SourceControlProviderKind, number: number): string | null { + if (provider === "github") return `gh pr checkout ${number}`; + if (provider === "gitlab") return `glab mr checkout ${number}`; + return null; +} + +function PullRequestCheckoutCommand({ + provider, + number, +}: { + readonly provider: SourceControlProviderKind; + readonly number: number; +}) { + const { copyToClipboard, isCopied } = useCopyToClipboard({ timeout: 1200 }); + const command = checkoutCommand(provider, number); + if (command === null) { + return null; + } + return ( + + + ); } @@ -770,10 +977,11 @@ function PullRequestTitle({ tabIndex={-1} className="group/title flex min-w-0 flex-1 items-center gap-1.5 rounded-sm focus-ring" > - {/* A phone's column is narrow enough that one truncated line says almost - nothing, so there it wraps to two before it gives up. */} + {/* A phone's column, and a tablet's, are narrow enough that one truncated + line says almost nothing, so there it wraps to two before it gives + up. */} {detail.title} @@ -829,16 +1037,29 @@ type PullRequestConfirmation = /** Remembered per computer: whoever deletes merged branches always does. */ const DELETE_BRANCH_STORAGE_PREFIX = "threadlines:pull-requests:delete-branch:v1"; +/** The three pieces the header hangs in three different places. */ +interface PullRequestActionsView { + /** The buttons, for the right of the header's first row. */ + readonly controls: ReactNode; + /** What a running or refused action has to say, under the header's rows. */ + readonly notices: ReactNode; + readonly dialog: ReactNode; +} + /** - * What the viewer may do to this pull request, under the meta line. + * What the viewer may do to this pull request, as the header's own controls. * * A write is offered only where the host says it takes that action and the * viewer's own permission covers it, so this hides what it knows cannot work * and never refuses on its own. The two irreversible ones ask first, by name * and by method; the rest simply run. The hand-offs are not writes to the host * at all, so a reader with no rights over this pull request still gets them. + * + * A hook rather than a component because its three parts land in three rows of + * the header, and the running action, the refusal and the confirmation are one + * piece of state between them. */ -function PullRequestHeaderActions({ +function usePullRequestActions({ environmentId, reference, detail, @@ -848,7 +1069,7 @@ function PullRequestHeaderActions({ readonly reference: PullRequestRef; readonly detail: PullRequestDetail; readonly handoffs: PullRequestHandoffActions | null; -}) { +}): PullRequestActionsView { const queryClient = useQueryClient(); const mutation = useMutation( pullRequestActionMutationOptions({ environmentId, reference, queryClient }), @@ -895,21 +1116,31 @@ function PullRequestHeaderActions({ canWrite && isOpen && detail.autoMergeEnabled === true && allows("disable-auto-merge"); const showEnableAutoMerge = canWrite && isOpen && detail.autoMergeEnabled !== true && allows("enable-auto-merge"); - const hasMenuWrites = showDraftToggle || showDisableAutoMerge || showEnableAutoMerge; - // The two menu halves: the hand-offs, and the writes that are not buttons. - const hasMenu = handoffs !== null || hasMenuWrites; - - const freshness = formatPullRequestBaseFreshness(detail); - const canUpdateBranch = canWrite && isOpen && allows("update-branch"); - const showFreshness = freshness !== null && canUpdateBranch; - if (!showMerge && !showClose && !showReopen && !hasMenu && !showFreshness) { - return null; - } const updateMethods = detail.capabilities.updateMethods; const mergeBlock = resolvePullRequestMergeBlock(detail); const mergeDisabled = isRunning || mergeBlock !== null; const defaultMergeMethod = resolveDefaultMergeMethod(detail.mergeMethods); + const canUpdateBranch = canWrite && isOpen && allows("update-branch"); + const isBehind = detail.baseComparison === "behind"; + + // One primary action for the state the branch is in: work it cannot merge + // through first, then bringing it up to date, then the merge itself. The + // displaced merge is not lost — it moves into the menu beside the rest. + const primary: "resolve-conflicts" | "update-branch" | "merge" | null = + isOpen && !detail.isDraft && detail.mergeability === "conflicting" && handoffs?.resolveConflicts + ? "resolve-conflicts" + : canUpdateBranch && isBehind && detail.mergeability !== "conflicting" + ? "update-branch" + : showMerge + ? "merge" + : null; + const showMergeInMenu = showMerge && primary !== "merge" && mergeBlock === null; + const hasMenuWrites = + showDraftToggle || showDisableAutoMerge || showEnableAutoMerge || showMergeInMenu; + // The two menu halves: the hand-offs, and the writes that are not buttons. + const hasMenu = handoffs !== null || hasMenuWrites; + const menuRunningWord = runningAction !== null && MENU_ACTIONS.has(runningAction) ? RUNNING_ACTION_WORDS[runningAction] @@ -956,66 +1187,65 @@ function PullRequestHeaderActions({ ); - return ( -
- {/* Being behind the base is the reason a merge is refused or a check is - stale, so it is said here rather than left for the host to explain. */} - {showFreshness ? ( -
- {freshness} - {updateMethods.length > 1 ? ( - - - } - > - {runningAction === "update-branch" - ? RUNNING_ACTION_WORDS["update-branch"] - : "Update branch"} - - - - {updateMethods.map((method) => ( - run("update-branch", { updateMethod: method })} - > - {pullRequestUpdateMethodLabel(method, detail.baseBranch)} - - ))} - - - ) : ( + const updateBranchControl = + updateMethods.length > 1 ? ( + + - run( - "update-branch", - updateMethods[0] ? { updateMethod: updateMethods[0] } : undefined, - ) - } - > - {runningAction === "update-branch" - ? RUNNING_ACTION_WORDS["update-branch"] - : "Update branch"} - - )} -
- ) : null} + /> + } + > + {runningAction === "update-branch" + ? RUNNING_ACTION_WORDS["update-branch"] + : "Update branch"} + + + + {updateMethods.map((method) => ( + run("update-branch", { updateMethod: method })}> + {pullRequestUpdateMethodLabel(method, detail.baseBranch)} + + ))} + + + ) : ( + + ); -
- {showMerge ? ( + const controls = + primary === null && !showClose && !showReopen && !hasMenu ? null : ( + <> + {primary === "resolve-conflicts" && handoffs?.resolveConflicts ? ( + // A branch the host cannot merge is work for an agent, not for the + // host's own buttons, so the way out stands where the merge would. + + ) : null} + {primary === "update-branch" ? updateBranchControl : null} + {primary === "merge" ? ( // A blocked merge stays on screen and says what is in the way: the // fix is on the host, and a vanished button explains nothing. mergeBlock === null ? ( @@ -1066,8 +1296,21 @@ function PullRequestHeaderActions({ + {/* Only where another action took the primary slot: the merge is + still available, it is simply no longer the thing to do. */} + {showMergeInMenu ? ( + + setConfirming({ action: "merge", mergeMethod: defaultMergeMethod }) + } + > + Merge + + ) : null} {handoffs ? ( <> + {showMergeInMenu ? : null} {handoffs.fixAll ? ( Fix all findings @@ -1081,7 +1324,10 @@ function PullRequestHeaderActions({ ) : null} - {handoffs && hasMenuWrites ? : null} + {(handoffs || showMergeInMenu) && + (showDraftToggle || showDisableAutoMerge || showEnableAutoMerge) ? ( + + ) : null} {showDraftToggle ? ( detail.isDraft ? ( run("ready")}>Mark as ready @@ -1109,7 +1355,11 @@ function PullRequestHeaderActions({ ) : null} -
+ + ); + + const notices = ( + <> {/* The menu is gone by the time the host answers, so its running action says so here instead of on the item that started it. */}

{/* A disabled button cannot be focused, so the reason it is disabled is written out as well as tucked in its tooltip. */} - {showMerge && mergeBlock !== null ? ( + {primary === "merge" && mergeBlock !== null ? (

{mergeBlock}

) : null} {mutation.isError ? ( @@ -1134,67 +1384,71 @@ function PullRequestHeaderActions({ : "The host refused that action."}

) : null} - - {confirming ? ( - { - if (!open) setConfirming(null); - }} - > - - - - {confirming.action === "merge" - ? `Merge #${detail.number} into ${detail.baseBranch}?` - : `Close #${detail.number} without merging?`} - - - {confirming.action === "merge" - ? `${PULL_REQUEST_MERGE_METHOD_LABELS[confirming.mergeMethod]}.` - : `The ${changeRequestWord(detail.provider)} stays on the host, and you can reopen it later.`} - - - {confirming.action === "merge" ? ( - - ) : null} - - {/* Cancel takes the focus: neither a merge nor a close can be - taken back, so a stray Enter must not run one. */} - } - > - Cancel - - - - - - ) : null} -
+ ); + + const dialog = confirming ? ( + { + if (!open) setConfirming(null); + }} + > + + + + {confirming.action === "merge" + ? `Merge #${detail.number} into ${detail.baseBranch}?` + : `Close #${detail.number} without merging?`} + + + {confirming.action === "merge" + ? `${PULL_REQUEST_MERGE_METHOD_LABELS[confirming.mergeMethod]}.` + : `The ${changeRequestWord(detail.provider)} stays on the host, and you can reopen it later.`} + + + {/* Between the header and the footer, which each carry their own + padding, so the row has to bring its own to line up with them. */} + {confirming.action === "merge" ? ( + + ) : null} + + {/* Cancel takes the focus: neither a merge nor a close can be + taken back, so a stray Enter must not run one. */} + } + > + Cancel + + + + + + ) : null; + + return { controls, notices, dialog }; } /** A branch name that copies itself, the way branch chips do elsewhere. */ @@ -1204,7 +1458,7 @@ function BranchCopyButton({ branch }: { readonly branch: string }) {
); } - -function FilterField({ - label, - children, -}: { - readonly label: string; - readonly children: ReactNode; -}) { - return ( -
-

{label}

- {children} -
- ); -} - -/** - * A typed field that answers to the keyboard before the route does. The route - * owns the value, but its round trip is a navigation, and a field waiting for - * one would drop characters; the field is read from the route when the popover - * opens and written to it from there on, which is the only direction it moves - * while it is on screen. - */ -function FilterText({ - label, - value, - placeholder, - quickPick, - onChange, -}: { - readonly label: string; - readonly value: string; - readonly placeholder: string; - /** A value worth one press, such as the signed-in login. */ - readonly quickPick?: string; - readonly onChange: (value: string) => void; -}) { - const [draft, setDraft] = useState(value); - const update = (next: string) => { - setDraft(next); - onChange(next); - }; - - return ( - <> - update(event.target.value)} - /> - {quickPick !== undefined && quickPick.toLowerCase() !== draft.trim().toLowerCase() ? ( - - ) : null} - - ); -} - -/** One choice out of a few, wrapping onto a second line where it must. */ -function FilterChoice({ - label, - value, - options, - onChange, -}: { - readonly label: string; - readonly value: Value; - readonly options: readonly { value: Value; label: string }[]; - readonly onChange: (value: Value) => void; -}) { - return ; -} diff --git a/apps/web/src/components/pull-requests/PullRequestSummaryTab.tsx b/apps/web/src/components/pull-requests/PullRequestSummaryTab.tsx index f2ac0ee2..cfc28c08 100644 --- a/apps/web/src/components/pull-requests/PullRequestSummaryTab.tsx +++ b/apps/web/src/components/pull-requests/PullRequestSummaryTab.tsx @@ -12,11 +12,21 @@ import type { PullRequestReaction, PullRequestRef, PullRequestReviewVerdict, + PullRequestReviewer, + PullRequestReviewerState, } from "@threadlines/contracts"; import { PULL_REQUEST_COMMENT_MAX_LENGTH } from "@threadlines/contracts"; import { useMutation, useQueryClient } from "@tanstack/react-query"; -import { ExternalLinkIcon, MessagesSquareIcon, PencilIcon, WandSparklesIcon } from "lucide-react"; -import { useCallback, useMemo, useState } from "react"; +import { + ChevronDownIcon, + ExternalLinkIcon, + MessagesSquareIcon, + PencilIcon, + TagIcon, + UsersIcon, + WandSparklesIcon, +} from "lucide-react"; +import { useCallback, useMemo, useRef, useState, type ReactNode } from "react"; import { pullRequestCommentMutationOptions, @@ -25,22 +35,26 @@ import { pullRequestUpdateMutationOptions, } from "../../lib/pullRequestsReactQuery"; import { openExternalUrl } from "../../lib/externalLinks"; -import { cn } from "../../lib/utils"; +import { cn, pluralize } from "../../lib/utils"; import { formatRelativeTimeLabel } from "../../timestampFormat"; import ChatMarkdown from "../ChatMarkdown"; import { Button } from "../ui/button"; import { Skeleton } from "../ui/skeleton"; import { Textarea } from "../ui/textarea"; +import { TooltipWrapper } from "../ui/tooltip"; import { PullRequestMarkdownEditor } from "./PullRequestMarkdownEditor"; import { PullRequestReactionBar } from "./PullRequestReactions"; import { PullRequestReviewerPicker } from "./PullRequestReviewerPicker"; import { CHECK_TONES, MetaSeparator, + PullRequestActorAvatar, + PullRequestLabelPill, REVIEW_STATE_WORDS, SECTION_LABEL_CLASS, TEXT_BUTTON_CLASS, TextChoice, + scrollPullRequestSummaryTo, } from "./pullRequestPresentation"; import { formatPullRequestChecksSummary, summarizePullRequestChecks } from "./pullRequests.logic"; @@ -71,9 +85,73 @@ export function PullRequestSummaryTab({ readonly onFixCheck?: (check: PullRequestCheck) => void; }) { const comments = activity?.comments ?? null; + const conversation = useRef(null); + const scrollToConversation = useCallback(() => { + scrollPullRequestSummaryTo(conversation.current); + }, []); return ( -
+
+ {/* What the host knows about this pull request, before what anyone said + about it: who is reading it, what it is filed under, how loud the + conversation is. */} +
+ } label="Reviewers"> + + {detail.reviewers.length === 0 ? ( + No reviewers + ) : ( + detail.reviewers.map((reviewer) => ( + + )) + )} + + {detail.capabilities.reviewers.request && detail.viewer.canWrite ? ( + + ) : null} + + {/* Nothing filed under nothing is not a row worth a line of its own. */} + {detail.labels.length > 0 ? ( + } label="Labels"> + + {detail.labels.map((label) => ( + + ))} + + + ) : null} + } + label="Comments" + > + {/* Nothing to scroll to until the conversation is read, so until then + the row says where it is up to rather than offering a button that + does nothing. */} + {comments === null ? ( + + {activityError ? "Comments unavailable" : "Loading comments"} + + ) : ( + + )} + +
+ - +
+ -
-
-

- Reviewers · {detail.reviewers.length} -

- {detail.capabilities.reviewers.request && detail.viewer.canWrite ? ( - - ) : null} -
- {detail.reviewers.length === 0 ? ( -

No reviewers.

- ) : ( -
- {detail.reviewers.map((reviewer) => ( -
- {reviewer.login} - {reviewer.kind === "team" ? ( - team - ) : null} - - - {REVIEW_STATE_WORDS[reviewer.state]} - -
- ))} -
- )} -
+
+

Comments · {comments?.length ?? 0}

+ {activityError ? ( +

+ The conversation could not be read. + +

+ ) : activityPending ? ( +
+ + +
+ ) : comments && comments.length > 0 ? ( +
+ {comments.map((comment) => ( + + ))} +
+ ) : ( +

No comments yet.

+ )} + +
+
+
+ ); +} -
-

Comments · {comments?.length ?? 0}

- {activityError ? ( -

- The conversation could not be read. - -

- ) : activityPending ? ( -
- - -
- ) : comments && comments.length > 0 ? ( -
- {comments.map((comment) => ( - - ))} -
- ) : ( -

No comments yet.

- )} - -
+/** + * One fact about the pull request as a labelled row: the icon and the word in + * a fixed first column, so Reviewers, Labels and Comments read as a list of + * facts rather than three sections that happen to sit together. + */ +function PullRequestMetaRow({ + icon, + label, + children, +}: { + readonly icon: ReactNode; + readonly label: string; + readonly children: ReactNode; +}) { + return ( +
+ + {icon} + {label} + + {children}
); } +/** The colour a reviewer's verdict wears, as the dot beside their name. */ +const REVIEWER_STATE_DOTS: Readonly> = { + approved: "bg-emerald-600 dark:bg-emerald-300/90", + "changes-requested": "bg-destructive", + commented: "bg-muted-foreground/60", + dismissed: "bg-muted-foreground/40", + pending: "bg-amber-600/90 dark:bg-amber-400/80", +}; + +/** A reviewer, their picture, and where they have got to, as one dot. */ +function PullRequestReviewerLabel({ + login, + kind, + state, + avatarUrl, +}: { + readonly login: string; + readonly kind: PullRequestReviewer["kind"]; + readonly state: PullRequestReviewerState; + readonly avatarUrl: string | null; +}) { + return ( + + + {login} + {kind === "team" ? team : null} + + + + {REVIEW_STATE_WORDS[state]} + + + + ); +} + +/** + * Whether each pull request's description is folded away, for as long as this + * page is loaded. Module level because the panel is unmounted and remounted by + * every tab press, and a section that reopened each time would be a section + * that cannot be closed. + */ +const descriptionOpenByPullRequest = new Map(); + /** * The description, and the pencil that rewrites it. Offered to whoever the host * lets rewrite it: its author, and anyone with push access on the repository. + * Open to begin with and foldable, because a long template pushes the checks + * and the conversation off the screen. */ function PullRequestDescription({ environmentId, @@ -174,15 +296,34 @@ function PullRequestDescription({ }) { const queryClient = useQueryClient(); const [editing, setEditing] = useState(false); + const sectionKey = `${environmentId}/${reference.projectId}/${reference.repository}#${reference.number}`; + const [open, setOpen] = useState(() => descriptionOpenByPullRequest.get(sectionKey) ?? true); const update = useMutation( pullRequestUpdateMutationOptions({ environmentId, reference, queryClient }), ); const canEdit = detail.capabilities.edit.pullRequest && detail.viewer.canManage; return ( -
+
-

Description

+

+ +

{canEdit && !editing ? (
- {editing ? ( - setEditing(false)} - onSave={(body) => update.mutate({ body }, { onSuccess: () => setEditing(false) })} - /> - ) : ( -
- 0 ? detail.body : "_No description._"} - cwd={detail.workspaceRoot} + {/* Folded away, the heading is the whole section. An edit in flight + keeps it open: nobody folds away the words they are writing. */} + {open || editing ? ( + <> + {editing ? ( + setEditing(false)} + onSave={(body) => update.mutate({ body }, { onSuccess: () => setEditing(false) })} + /> + ) : ( +
+ 0 ? detail.body : "_No description._"} + cwd={detail.workspaceRoot} + environmentId={environmentId} + html="github" + /> +
+ )} + {/* The editor above stays open on a refusal, so the words are still there + to try again with; this says why the host would not take them. */} + {update.isError ? ( +

+ {update.error instanceof Error && update.error.message.trim().length > 0 + ? update.error.message + : "That could not be saved."} +

+ ) : null} + -
- )} - {/* The editor above stays open on a refusal, so the words are still there - to try again with; this says why the host would not take them. */} - {update.isError ? ( -

- {update.error instanceof Error && update.error.message.trim().length > 0 - ? update.error.message - : "That could not be saved."} -

+ ) : null} -
); } @@ -261,7 +408,9 @@ function PullRequestChecksSection({ const rows = showAll ? checks : summary.attention; return ( -
+ // Named so the header's rollup, which counts the same checks, can bring + // the reader down to the rows behind its phrase. +

Checks · {summary.total}

{tone ? ( diff --git a/apps/web/src/components/pull-requests/PullRequestTabStrip.tsx b/apps/web/src/components/pull-requests/PullRequestTabStrip.tsx new file mode 100644 index 00000000..28170662 --- /dev/null +++ b/apps/web/src/components/pull-requests/PullRequestTabStrip.tsx @@ -0,0 +1,205 @@ +/** + * The pull requests open beside the list, in the internal browser's visual + * language: a short row of rounded-top tabs, the active one lifted onto the + * background, a close ✕ on it and on whichever the pointer is over. It is the + * thread right panel's strip narrowed to one kind of thing, so the classes are + * that strip's; its logic is not reusable here, since every tab there is one of + * a fixed set of surfaces and every tab here is a row the user pressed. + * + * There is no `+`: the list beside the column is how a pull request is opened. + */ +import { XIcon } from "lucide-react"; +import { useEffect, useRef } from "react"; + +import { cn } from "../../lib/utils"; +import { MINI_HORIZONTAL_SCROLLBAR_CLASS, ScrollArea } from "../ui/scroll-area"; +import { moveBetweenTabs } from "../ui/page-tabs"; +import { TooltipWrapper } from "../ui/tooltip"; +import { pullRequestBadgeTone } from "./pullRequests.logic"; +import type { PullRequestTab } from "./pullRequestTabsStore"; + +/** Hover on the tab, or keyboard focus anywhere in it, including the ✕ itself. */ +const REVEAL_ON_TAB_HOVER_OR_FOCUS = + "group-hover/pr-tab:opacity-100 group-has-[:focus-visible]/pr-tab:opacity-100"; + +export function pullRequestTabButtonId(id: string): string { + return `pull-request-tab-${id}`; +} + +export function PullRequestTabStrip({ + tabs, + activeId, + panelId, + onSelect, + onClose, +}: { + readonly tabs: readonly PullRequestTab[]; + readonly activeId: string | null; + /** The detail below, so each tab can name what it controls. */ + readonly panelId: string; + readonly onSelect: (tab: PullRequestTab) => void; + readonly onClose: (tab: PullRequestTab) => void; +}) { + const list = useRef(null); + // Set by a close the keyboard ran, so the cursor lands on whichever tab took + // the closed one's place rather than on the body. + const focusActiveTab = useRef(false); + + // A tab opened while the strip is already full can sit past its right edge, + // and the active tab is the one the detail below is showing. + useEffect(() => { + const active = activeId + ? list.current?.querySelector( + `[data-pull-request-tab="${activeId}"] [role="tab"]`, + ) + : null; + active?.scrollIntoView({ inline: "nearest", block: "nearest" }); + // Held until a tab is actually there to take it: the strip drops the closed + // tab before the route says which one stands in its place. + if (focusActiveTab.current && active) { + focusActiveTab.current = false; + active.focus(); + } + // Only the active tab matters here, and every way the strip gains or loses + // one ends with a different tab active. + }, [activeId]); + + if (tabs.length === 0) { + return null; + } + return ( +
+
+ {/* Tabs are drawn whole and scroll rather than shrink, exactly as the + thread panel's do: "#12…" names nothing. The bar is the hairline + overlay every strip in the app uses, so the row keeps its height. */} + +
+ {tabs.map((tab) => ( + onSelect(tab)} + onClose={(fromKeyboard) => { + focusActiveTab.current = fromKeyboard; + onClose(tab); + }} + /> + ))} +
+ +
+
+ ); +} + +function PullRequestTabStripItem({ + tab, + active, + panelId, + onSelect, + onClose, +}: { + readonly tab: PullRequestTab; + readonly active: boolean; + readonly panelId: string; + readonly onSelect: () => void; + /** True when the keyboard ran the close, which then has to hand focus on. */ + readonly onClose: (fromKeyboard: boolean) => void; +}) { + const tone = pullRequestBadgeTone(tab.state, tab.isDraft); + return ( + +
{ + if (event.button !== 1) return; + event.preventDefault(); + onClose(false); + }} + onMouseDown={(event) => { + // Suppress the middle-click autoscroll cursor; the close itself + // happens on auxclick, where the gesture is complete. + if (event.button === 1) event.preventDefault(); + }} + > + + {/* Overlaid on the tab's right edge rather than given a column of its + own: reserving width for a control that is invisible most of the + time is exactly what a strip of tabs cannot spare. */} + +
+ + ); +} diff --git a/apps/web/src/components/pull-requests/PullRequestsView.browser.tsx b/apps/web/src/components/pull-requests/PullRequestsView.browser.tsx index 1ab8ae33..1cc977f3 100644 --- a/apps/web/src/components/pull-requests/PullRequestsView.browser.tsx +++ b/apps/web/src/components/pull-requests/PullRequestsView.browser.tsx @@ -34,9 +34,11 @@ import { AppAtomRegistryProvider, resetAppAtomRegistryForTests } from "../../rpc import { useStore } from "../../store"; import { SidebarProvider } from "../ui/sidebar"; import { PullRequestsView } from "./PullRequestsView"; +import { resetPullRequestTabsForTests } from "./pullRequestTabsStore"; import { DEFAULT_PULL_REQUEST_SORT, EMPTY_PULL_REQUEST_FILTERS, + pullRequestFiltersToSearch, type PullRequestFilters, type PullRequestSelection, type PullRequestSort, @@ -76,7 +78,7 @@ function makeEntry(overrides: Partial = {}): PullRequestLi number: 1, title: "Add the pull requests page", url: "https://github.com/threadlines/threadlines/pull/1", - author: { login: "ada", isBot: false }, + author: { login: "ada", isBot: false, avatarUrl: null }, headBranch: "feature/pull-requests", baseBranch: "main", state: "open", @@ -122,7 +124,7 @@ function makeDetail(overrides: Partial = {}): PullRequestDeta title: "Add the pull requests page", body: "", url: "https://github.com/threadlines/threadlines/pull/1", - author: { login: "ada", isBot: false }, + author: { login: "ada", isBot: false, avatarUrl: null }, state: "open", isDraft: false, mergeability: "mergeable", @@ -239,6 +241,9 @@ function createTestRouter(children: ReactNode) { }); } +/** The params the route would have navigated with, as the page last wrote them. */ +let lastSearch: Record = {}; + /** Stands in for the route, which owns the selection, the filters and the sort. */ function SelectablePullRequestsView() { const [selection, setSelection] = useState(null); @@ -252,8 +257,14 @@ function SelectablePullRequestsView() { sort={sort} onStateChange={() => undefined} onSelectionChange={setSelection} - onFiltersChange={setFilters} - onSortChange={setSort} + onFiltersChange={(next) => { + lastSearch = pullRequestFiltersToSearch(next, sort); + setFilters(next); + }} + onSortChange={(next) => { + lastSearch = pullRequestFiltersToSearch(filters, next); + setSort(next); + }} /> ); } @@ -298,6 +309,9 @@ describe("PullRequestsView", () => { beforeEach(() => { resetAppAtomRegistryForTests(); resetSavedEnvironmentRuntimeStoreForTests(); + // The strip is a session, not a render: one test's open tabs would + // otherwise still be open in the next one. + resetPullRequestTabsForTests(); seedProject(); }); @@ -366,6 +380,80 @@ describe("PullRequestsView", () => { await rendered.cleanup(); }); + it("keeps both open pull requests as tabs and closes back onto the other", async () => { + const rendered = await renderPage({ + viewer: "ada", + entries: [ + makeEntry({ number: 1, title: "First" }), + makeEntry({ number: 2, title: "Second" }), + ], + errors: [], + }); + + await userEvent.click(page.getByRole("button", { name: "Open pull request #1: First" })); + await userEvent.click(page.getByRole("button", { name: "Open pull request #2: Second" })); + + const strip = page.getByTestId("pull-request-tab-strip"); + await expect.element(strip.getByRole("tab", { name: /#1/ })).toBeVisible(); + await expect.element(strip.getByRole("tab", { name: /#2/ })).toBeVisible(); + + // Closing the one on screen falls back to what is left rather than back to + // the bare list. Both the tab and its ✕ are named by repository as well as + // number, since two repositories can hold the same one. + await userEvent.click(page.getByRole("button", { name: "Close threadlines/threadlines #2" })); + + await vi.waitFor(() => { + expect(strip.getByRole("tab", { name: /#2/ }).elements()).toHaveLength(0); + }); + await expect.element(strip.getByRole("tab", { name: /#1/ })).toBeVisible(); + await expect.element(page.getByTestId("pull-requests-detail-column")).toBeVisible(); + + await rendered.cleanup(); + }); + + it("walks the tab strip with the arrow keys and hands focus on after a close", async () => { + const rendered = await renderPage({ + viewer: "ada", + entries: [ + makeEntry({ number: 1, title: "First" }), + makeEntry({ number: 2, title: "Second" }), + ], + errors: [], + }); + + await userEvent.click(page.getByRole("button", { name: "Open pull request #1: First" })); + await userEvent.click(page.getByRole("button", { name: "Open pull request #2: Second" })); + + const strip = page.getByTestId("pull-request-tab-strip"); + const second = strip.getByRole("tab", { name: /#2/ }); + await userEvent.click(second); + // The arrows only move the detail; handing the cursor to the new title + // would end the walk on its first step. + await userEvent.keyboard("{ArrowLeft}"); + // The title is what used to take the cursor, so the check waits until it is + // on screen before asking where the cursor is. + await expect + .element(page.getByRole("heading", { name: "Add the pull requests page" })) + .toBeVisible(); + const walked = document.activeElement; + expect(walked?.getAttribute("role")).toBe("tab"); + expect(walked?.textContent).toContain("#1"); + expect(walked?.getAttribute("aria-selected")).toBe("true"); + + // A ✕ pressed from the keyboard takes its own element away, so the tab that + // steps into its place takes the cursor. + const close = page.getByRole("button", { name: "Close threadlines/threadlines #1" }); + close.element().focus(); + await userEvent.keyboard("{Enter}"); + await vi.waitFor(() => { + const focused = document.activeElement; + expect(focused?.getAttribute("role")).toBe("tab"); + expect(focused?.textContent).toContain("#2"); + }); + + await rendered.cleanup(); + }); + it("steps back to the list from the detail on a phone", async () => { await page.viewport(390, 800); try { @@ -394,22 +482,40 @@ describe("PullRequestsView", () => { } }); - it("narrows the list from the filters and gives the rows back from the chip", async () => { + it("narrows the list from the filters menu and gives the rows back from the chip", async () => { const rendered = await renderPage({ viewer: "ada", entries: [ makeEntry({ number: 1, title: "Mine", viewerIsAuthor: true }), - makeEntry({ number: 2, title: "Someone else's", author: { login: "grace", isBot: false } }), + makeEntry({ + number: 2, + title: "Someone else's", + author: { login: "grace", isBot: false, avatarUrl: null }, + }), ], errors: [], }); await expect.element(page.getByText("Someone else's")).toBeVisible(); await userEvent.click(page.getByTestId("pull-requests-filters")); - await userEvent.click(page.getByRole("button", { name: "Use ada" })); - await userEvent.keyboard("{Escape}"); + // The authors are the ones the loaded rows carry, so ada is a line to pick + // rather than a login to spell. + await userEvent.click(page.getByRole("menuitem", { name: /^Author/ })); + // The field keeps its own typing: a menu would otherwise read the letters + // as a jump to the item that starts with them. + await userEvent.fill(page.getByRole("textbox", { name: "Search authors" }), "ad"); + // One author at a time, so the choices are radios and a reader hears which + // one is current. + await vi.waitFor(() => { + expect( + page.getByRole("menuitemradio", { name: "grace", exact: true }).elements(), + ).toHaveLength(0); + }); + await userEvent.click(page.getByRole("menuitemradio", { name: "ada", exact: true })); await expect.element(page.getByText("Author: ada")).toBeVisible(); + // What the route would put in the URL, which is how a link keeps the filter. + expect(lastSearch).toEqual({ author: "ada" }); await vi.waitFor(() => { expect(page.getByTestId("pull-requests-row").elements()).toHaveLength(1); }); @@ -420,6 +526,41 @@ describe("PullRequestsView", () => { await rendered.cleanup(); }); + it("draws the conflict, the reviews, the checks and an author the host gave no picture for", async () => { + const rendered = await renderPage({ + viewer: "ada", + entries: [ + makeEntry({ + number: 7, + title: "Bump the runner", + author: { login: "dependabot[bot]", isBot: true, avatarUrl: null }, + mergeability: "conflicting", + reviewDecision: "approved", + checksState: "failure", + labels: [{ name: "dependencies", color: "0366d6" }], + }), + ], + errors: [], + }); + + await expect.element(page.getByText("Bump the runner")).toBeVisible(); + // The triangle stands in for the open glyph, and says so in words. + expect(page.getByText("Conflicts with main").elements()).toHaveLength(1); + // The reviews and the checks are both glyphs, and each carries the words + // the row no longer spends its meta line on. + expect(page.getByText("Approved").elements()).toHaveLength(1); + expect(page.getByText("Some checks failed").elements()).toHaveLength(1); + // No picture to load, so the avatar is the login's first letter. + expect(document.querySelectorAll("#pull-requests-list img")).toHaveLength(0); + expect( + [...document.querySelectorAll("#pull-requests-list span")].filter( + (element) => element.textContent === "D", + ), + ).toHaveLength(1); + + await rendered.cleanup(); + }); + it("names the repository of a pull request from outside the workspace", async () => { const rendered = await renderPage({ viewer: "ada", diff --git a/apps/web/src/components/pull-requests/PullRequestsView.tsx b/apps/web/src/components/pull-requests/PullRequestsView.tsx index 4bc4d5f9..7540c747 100644 --- a/apps/web/src/components/pull-requests/PullRequestsView.tsx +++ b/apps/web/src/components/pull-requests/PullRequestsView.tsx @@ -13,8 +13,9 @@ import { GitBranchPlusIcon, MessagesSquareIcon, RefreshCwIcon, + TriangleAlertIcon, } from "lucide-react"; -import { useCallback, useEffect, useMemo, useRef, useState } from "react"; +import { useCallback, useEffect, useMemo, useRef, useState, type ReactNode } from "react"; import { useShallow } from "zustand/react/shallow"; import { useNewThreadHandler } from "../../hooks/useHandleNewThread"; @@ -22,6 +23,7 @@ import { openExternalUrl } from "../../lib/externalLinks"; import { PULL_REQUEST_PAGE_REFETCH_INTERVAL_MS, refreshPullRequestList, + useLoadedPullRequestEntries, usePullRequestLists, type PullRequestEnvironmentFailure, } from "../../lib/pullRequestsReactQuery"; @@ -44,20 +46,44 @@ import { PageTabButton, pageTabId } from "../ui/page-tabs"; import { Skeleton } from "../ui/skeleton"; import { TooltipWrapper } from "../ui/tooltip"; import { LazyPullRequestDetailPanel } from "./LazyPullRequestDetailPanel"; -import { PullRequestFilterChipsRow, PullRequestFiltersButton } from "./PullRequestFilters"; -import { pullRequestHostName } from "./pullRequestPresentation"; +import type { PullRequestCheckoutRequest } from "./PullRequestDetailPanel"; import { + PullRequestFilterChipsRow, + PullRequestFiltersButton, + PullRequestSortMenu, +} from "./PullRequestFilters"; +import { PullRequestTabStrip, pullRequestTabButtonId } from "./PullRequestTabStrip"; +import { + pullRequestTabId, + usePullRequestTabsStore, + type PullRequestTab, + type PullRequestTabStatus, + type PullRequestTabTarget, +} from "./pullRequestTabsStore"; +import { + PullRequestActorAvatar, + PullRequestChecksGlyph, + PullRequestLabelPill, + PullRequestReviewGlyph, + pullRequestChecksTone, + pullRequestHostName, + pullRequestReviewTone, +} from "./pullRequestPresentation"; +import { + formatPullRequestSelection, groupPullRequests, hasPullRequestProject, linkThreadsToPullRequests, matchesPullRequestQuery, + matchesPullRequestSelection, narrowPullRequests, projectRepository, pullRequestBadgeTone, + pullRequestConflictLabel, pullRequestEntryKey, pullRequestFilterChips, + pullRequestProjectFacets, requiresHostSignIn, - resolveNeedsYouReason, resolvePullRequestListSpan, resolveSignInHost, type PullRequestEntry, @@ -139,8 +165,13 @@ interface PullRequestThreadDialogTarget { readonly url: string; /** The hand-off the new draft opens with, when the checkout came from one. */ readonly initialPrompt: string | null; + /** The way in the header asked for, so the dialog opens on that button. */ + readonly mode: "local" | "worktree" | null; } +/** The detail beside the list, which the tab strip above it names. */ +const DETAIL_PANEL_ID = "pull-requests-detail"; + /** * The pull requests destination: every project in the workspace on a host we can read, in one * list, grouped by what the signed-in user still has to do about it. @@ -173,6 +204,12 @@ export function PullRequestsView({ const [query, setQuery] = useState(""); const [isRefreshing, setIsRefreshing] = useState(false); const [dialogTarget, setDialogTarget] = useState(null); + // The pull requests open at once. The route's `pr` says which of them is on + // screen; the store only holds the set and the order they were opened in. + const tabs = usePullRequestTabsStore((store) => store.tabs); + const openTab = usePullRequestTabsStore((store) => store.open); + const closeTab = usePullRequestTabsStore((store) => store.close); + const markTabStatus = usePullRequestTabsStore((store) => store.markStatus); const snapshot = usePullRequestLists({ state, @@ -190,8 +227,25 @@ export function PullRequestsView({ [filters, query, snapshot.entries], ); const groups = useMemo( - () => groupPullRequests({ entries: visibleEntries, viewer: snapshot.viewer, state, sort }), - [snapshot.viewer, sort, state, visibleEntries], + () => + groupPullRequests({ + entries: visibleEntries, + viewer: snapshot.viewer, + state, + sort, + involvement: filters.involvement, + }), + [filters.involvement, snapshot.viewer, sort, state, visibleEntries], + ); + // The chosen project is a key in the URL; only the rows know what it is called. + const projectLabel = useMemo( + () => + filters.project === "" + ? undefined + : pullRequestProjectFacets(snapshot.entries).find( + (project) => project.key === filters.project, + )?.label, + [filters.project, snapshot.entries], ); // Over every row, not just the visible ones: the selected pull request keeps // naming its thread while a search hides the row it came from. @@ -227,7 +281,7 @@ export function PullRequestsView({ ); const handleReviewInThread = useCallback( - (entry: PullRequestEntry, initialPrompt?: string) => { + (entry: PullRequestEntry, request?: PullRequestCheckoutRequest) => { const project = projects.find( (candidate) => candidate.environmentId === entry.environmentId && candidate.id === entry.projectId, @@ -239,7 +293,8 @@ export function PullRequestsView({ threadId: newThreadId(), cwd: project?.cwd ?? null, url: entry.url, - initialPrompt: initialPrompt ?? null, + initialPrompt: request?.initialPrompt ?? null, + mode: request?.mode ?? null, }); }, [projects], @@ -247,31 +302,50 @@ export function PullRequestsView({ // Which pull request the URL is on, and which row a press of the user's own // put it on. Only the second moves the cursor: a link opened straight onto a - // selection, or a step back through history, should leave focus alone. The - // pressed row also remembers its repository, which the URL does not carry. - const selectionKey = selection - ? `${selection.environmentId}:${selection.projectId}:${selection.number}` - : null; - const [pressedRow, setPressedRow] = useState<{ - readonly key: string; - readonly repository: string; - } | null>(null); - const userSelectedKey = pressedRow?.key ?? null; + // selection, a step back through history, or a move along the tab strip + // should leave focus where it is. + const selectionKey = selection ? formatPullRequestSelection(selection) : null; + const [pressedKey, setPressedKey] = useState(null); const rowToRefocus = useRef(null); + // One pull request shown. The repository rides along in the selection itself, + // because the panel addresses a pull request by repository as well as by + // number and two repositories can hold the same one. + const showPullRequest = useCallback( + (target: PullRequestTabTarget) => { + onSelectionChange({ + environmentId: target.environmentId, + projectId: target.projectId, + repository: target.repository, + number: target.number, + }); + }, + [onSelectionChange], + ); + // A tab moves the detail and nothing else: the arrow keys walk the strip, and + // handing the cursor to the new title would end that walk on its first step. + const handleSelectTab = useCallback( + (tab: PullRequestTab) => { + setPressedKey(null); + showPullRequest(tab); + }, + [showPullRequest], + ); + // A row press replaces the list with the detail, so the cursor follows it. const handleSelect = useCallback( (entry: PullRequestEntry) => { - setPressedRow({ - key: `${entry.environmentId}:${entry.projectId}:${entry.number}`, - repository: entry.repository, - }); - onSelectionChange({ + openTab({ environmentId: entry.environmentId, projectId: entry.projectId, + repository: entry.repository, number: entry.number, + state: entry.state, + isDraft: entry.isDraft, }); + setPressedKey(formatPullRequestSelection(entry)); + showPullRequest(entry); }, - [onSelectionChange], + [openTab, showPullRequest], ); const closeSelection = useCallback(() => { // Read while the row still wears the mark, since the mark goes with the @@ -280,7 +354,7 @@ export function PullRequestsView({ rowToRefocus.current = document.querySelector( '[data-testid="pull-requests-row"][aria-current="true"]', ); - setPressedRow(null); + setPressedKey(null); onSelectionChange(null); }, [onSelectionChange]); @@ -298,20 +372,15 @@ export function PullRequestsView({ const selectedEntry = useMemo( () => selection - ? (snapshot.entries.find( - (entry) => - entry.environmentId === selection.environmentId && - entry.projectId === selection.projectId && - entry.number === selection.number, - ) ?? null) + ? (snapshot.entries.find((entry) => matchesPullRequestSelection(selection, entry)) ?? null) : null, [selection, snapshot.entries], ); - // The panel addresses a pull request by repository as well as number, and the - // URL carries only the project. A selection the listing no longer carries - // falls back to the repository the row was pressed on, and only then to the - // project's own remote: a project reads more than one repository now, so - // guessing would open whatever else wears that number. + // The panel addresses a pull request by repository as well as number. The URL + // carries it, but a link written before it did carries only the project, and + // then the listing's own row answers for it, and only after that the + // project's remote: a project reads more than one repository now, so guessing + // would open whatever else wears that number. const selectedReference = useMemo(() => { if (!selection) return null; const project = projects.find( @@ -319,13 +388,13 @@ export function PullRequestsView({ candidate.environmentId === selection.environmentId && candidate.id === selection.projectId, ); const repository = + selection.repository ?? selectedEntry?.repository ?? - (pressedRow?.key === selectionKey ? pressedRow.repository : null) ?? (project ? projectRepository(project) : null); return repository ? { projectId: selection.projectId, repository, number: selection.number } : null; - }, [pressedRow, projects, selectedEntry, selection, selectionKey]); + }, [projects, selectedEntry, selection]); const selectedThread = selectedEntry ? (threadsByEntryKey.get(pullRequestEntryKey(selectedEntry))?.[0] ?? null) : null; @@ -334,6 +403,65 @@ export function PullRequestsView({ const checkoutableEntry = selectedEntry && selectedEntry.origin !== "authored" ? selectedEntry : null; + // The route is the source of truth for what is shown, so a pull request it + // names that the strip does not carry joins it: a link opened straight onto + // one, a step back through history, or the row press that just happened. + useEffect(() => { + if (!selection || !selectedReference) return; + openTab({ + environmentId: selection.environmentId, + projectId: selection.projectId, + repository: selectedReference.repository, + number: selection.number, + }); + }, [openTab, selectedReference, selection]); + + // Each tab wears the state of the row it stands for, read from every listing + // the page has loaded rather than only the one on screen: a merged pull + // request is not in the open list, and drawing it open would be a lie. A row + // no loaded listing carries keeps the state it was last seen with. + const loadedEntries = useLoadedPullRequestEntries(); + const tabStatuses = useMemo(() => { + const byId = new Map(); + for (const entry of [...loadedEntries, ...snapshot.entries]) { + byId.set(pullRequestTabId(entry), { state: entry.state, isDraft: entry.isDraft }); + } + return byId; + }, [loadedEntries, snapshot.entries]); + useEffect(() => { + markTabStatus(tabStatuses); + }, [markTabStatus, tabStatuses]); + const tabViews = useMemo( + () => tabs.map((tab) => ({ ...tab, ...tabStatuses.get(tab.id) })), + [tabStatuses, tabs], + ); + const activeTabId = + selection && selectedReference + ? pullRequestTabId({ + environmentId: selection.environmentId, + projectId: selection.projectId, + repository: selectedReference.repository, + number: selection.number, + }) + : null; + const handleCloseTab = useCallback( + (tab: PullRequestTab) => { + const wasShowing = tab.id === activeTabId; + const next = closeTab(tab.id); + // Closing a tab the detail was not showing changes only the strip. The + // strip and the detail otherwise move together: whatever is active after + // the close is what the route goes to, and an empty strip gives the list + // its full width back. + if (!wasShowing) return; + if (next) { + handleSelectTab(next); + return; + } + closeSelection(); + }, + [activeTabId, closeSelection, closeTab, handleSelectTab], + ); + // Escape steps back to the list, but only when nothing else owns the key: a // dialog on top of the page is closing itself first. useEffect(() => { @@ -422,12 +550,7 @@ export function PullRequestsView({ linkedThread={threadsByEntryKey.get(pullRequestEntryKey(entry))?.[0] ?? null} showRepository={span.multipleRepositories} showEnvironment={span.multipleEnvironments} - selected={ - selection !== null && - selection.environmentId === entry.environmentId && - selection.projectId === entry.projectId && - selection.number === entry.number - } + selected={selection !== null && matchesPullRequestSelection(selection, entry)} onSelect={handleSelect} onOpenThread={handleOpenThread} onReviewInThread={handleReviewInThread} @@ -457,7 +580,10 @@ export function PullRequestsView({ >
@@ -469,9 +595,8 @@ export function PullRequestsView({ {/* Nothing to filter, search or refresh when no server can answer at all. */} {snapshot.environments.length > 0 ? ( <> - {/* The same strip the settings pages use: text on a hairline, the - active tab underlined, the refresh control in the strip's - trailing slot. */} + {/* The same strip the settings pages use: text on a hairline, + the active tab underlined. */}
))}
+
+ {/* Search, then the two menus that stand for everything else the + list can be told, then the way to read it all again. */} +
+ setQuery(event.target.value)} + /> + +
-
- setQuery(event.target.value)} - /> - -
- + ) : null} @@ -550,22 +679,42 @@ export function PullRequestsView({
{selectedReference && selection ? ( -
- - handleReviewInThread(checkoutableEntry, initialPrompt), - } - : {})} +
+ {/* Several pull requests stay open at once, and the strip is where + the rest of them wait. It sits above the header, so on a phone + the back arrow still steps out to the list with the strip kept. */} + +
+ + handleReviewInThread(checkoutableEntry, request), + } + : {})} + /> +
) : null}
@@ -578,6 +727,7 @@ export function PullRequestsView({ threadId={dialogTarget.threadId} cwd={dialogTarget.cwd} initialReference={dialogTarget.url} + {...(dialogTarget.mode ? { defaultMode: dialogTarget.mode } : {})} onOpenChange={(open) => { if (!open) { setDialogTarget(null); @@ -741,6 +891,11 @@ function PullRequestsNotice({ ); } +/** A sentence's worth of words joined into the middle of a longer one. */ +function lowerFirst(value: string): string { + return value.charAt(0).toLowerCase() + value.slice(1); +} + function PullRequestRow({ entry, linkedThread, @@ -765,7 +920,22 @@ function PullRequestRow({ className: glyphClassName, label: glyphLabel, } = pullRequestBadgeTone(entry.state, entry.isDraft); - const reason = resolveNeedsYouReason(entry); + // A branch that no longer merges is the one thing about an open row worth + // more than its state, so it takes the glyph's place. + const conflictLabel = pullRequestConflictLabel(entry); + // Everything the row states in a glyph belongs in the name of the button that + // opens it, since a glyph in a sibling is not part of that name: the state + // word, then the conflict, then how the checks went. + const checksLabel = pullRequestChecksTone(entry.checksState)?.label ?? null; + const rowLabel = `${[ + `${glyphLabel} pull request #${entry.number}`, + ...(conflictLabel ? [lowerFirst(conflictLabel)] : []), + ...(checksLabel ? [lowerFirst(checksLabel)] : []), + ].join(", ")}: ${entry.title}`; + const reviewTone = pullRequestReviewTone({ + decision: entry.reviewDecision, + reviewRequested: entry.viewerReviewRequested, + }); const openOnHostLabel = `Open on ${pullRequestHostName(entry.provider)}`; // Nothing here is checked out, so there is no thread to open and no branch to // check out into one; the repository is the only thing that places the row. @@ -775,38 +945,102 @@ function PullRequestRow({ const hiddenLabelCount = entry.labels.length - visibleLabels.length; // A fixed, ordered set of optional slots, so the absent ones simply drop out // and the separators still land between what is left. A slot either keeps - // its width whole or truncates; the wrapper follows the same rule, or a - // whole-width slot overflows its shrunken wrapper and paints over the next - // slot's separator. + // its width whole or gives it up, and the one that gives it up truncates its + // own text; a whole-width slot in a shrinking wrapper would otherwise paint + // over the next slot's separator. const meta: readonly { key: string; fit: "whole" | "truncate"; className?: string; - text: string; + /** Dropped, separator and all, when the list is narrower than `lg` (32rem): a + * phone, or the column beside an open pull request. */ + hideOnPhone?: boolean; + content: ReactNode; }[] = [ - { key: "number", fit: "whole", className: "font-mono", text: `#${entry.number}` }, + { key: "number", fit: "whole", className: "font-mono", content: `#${entry.number}` }, ...(namesRepository - ? [{ key: "repository", fit: "truncate" as const, text: entry.repository }] + ? [ + { + key: "repository", + fit: "truncate" as const, + // A phone-width row has room for the number, the author and the + // glyph; the repository only stays where it is the row's one + // anchor (a pull request from outside the workspace). + hideOnPhone: !isAuthoredElsewhere, + content: {entry.repository}, + }, + ] : []), ...(entry.author - ? [{ key: "author", fit: "truncate" as const, text: entry.author.login }] + ? [ + { + key: "author", + fit: "truncate" as const, + content: ( + <> + + {entry.author.login} + + ), + }, + ] : []), ...(showEnvironment - ? [{ key: "environment", fit: "truncate" as const, text: entry.environmentLabel }] + ? [ + { + key: "environment", + fit: "truncate" as const, + content: {entry.environmentLabel}, + }, + ] : []), - ...(reason + ...(visibleLabels.length > 0 ? [ { - key: "reason", - fit: "whole" as const, - className: - reason === "Approved" - ? "text-emerald-600 dark:text-emerald-300/90" - : "text-amber-600/90 dark:text-amber-400/80", - text: reason, + key: "labels", + fit: "truncate" as const, + // Pills truncated to a letter each say nothing; below md they go. + hideOnPhone: true, + content: ( + <> + {visibleLabels.map((label) => ( + + ))} + {hiddenLabelCount > 0 ? ( + +{hiddenLabelCount} + ) : null} + + ), }, ] : []), + // Where the reviews stand and how the checks went, as two glyphs at the end + // of the line: the same pair on every row, in every group, so the eye finds + // them in the same place rather than reading a coloured word out of the + // meta. Both carry their words for anyone who cannot see the colour. + ...(reviewTone === null + ? [] + : [ + { + key: "review", + fit: "whole" as const, + content: ( + + ), + }, + ]), + ...(entry.checksState === undefined + ? [] + : [ + { + key: "checks", + fit: "whole" as const, + content: , + }, + ]), ]; return ( @@ -828,15 +1062,26 @@ function PullRequestRow({ className="absolute inset-0 z-0 w-full cursor-pointer rounded-md focus-ring" data-testid="pull-requests-row" aria-current={selected ? "true" : undefined} - aria-label={`${glyphLabel} pull request #${entry.number}: ${entry.title}`} + aria-label={rowLabel} onClick={() => onSelect(entry)} />
{/* The glyph is the only place the row states open/draft/merged/closed, - so the word rides along for anyone who cannot see the colour. */} - - - + so the word rides along in the row's own label for anyone who + cannot see the colour. A conflict has no word of its own there, so + it carries one itself. */} + {conflictLabel ? ( + + + + {conflictLabel} + + + ) : ( + + + + )} @@ -854,25 +1099,15 @@ function PullRequestRow({ className={cn( "flex items-center gap-1.5", item.fit === "whole" ? "shrink-0" : "min-w-0", + item.hideOnPhone && "@max-lg:hidden", )} > {index > 0 ? · : null} - - {item.text} + + {item.content} ))} - {visibleLabels.map((label) => ( - - - {label.name} - - ))} - {hiddenLabelCount > 0 ? +{hiddenLabelCount} : null} {linkedThread ? (