From 1e83f09401b134e523aedff8084b0295e3824ba1 Mon Sep 17 00:00:00 2001 From: badcuban <108198679+badcuban@users.noreply.github.com> Date: Thu, 3 Sep 2026 04:49:03 -0400 Subject: [PATCH 01/13] docs: pull requests step 6 polish spec --- docs/design/pull-requests.md | 122 +++++++++++++++++++++++++++++++++++ 1 file changed, 122 insertions(+) diff --git a/docs/design/pull-requests.md b/docs/design/pull-requests.md index e4928af8..dfba36b4 100644 --- a/docs/design/pull-requests.md +++ b/docs/design/pull-requests.md @@ -1053,3 +1053,125 @@ Server: the authored search is deduplicated against workspace rows; a failed sea entry, not the list; `canManage` for an author without push. Web: an authored row shows its repository and no thread affordances; the merge button is absent and Close present for an author without write access. + +# Step 6: the polish pass (avatars, pills, glyphs, tabs, header, menus) + +Will compared the page with t3code's (screenshots 2026-09-03) and wants the same level of finish: +GitHub avatars, label pills with a colour dot, a check glyph instead of words, a conflict triangle, +several pull requests open at once as tabs, a cleaner header, and Filters and Sort as menus. The +pills are a deliberate exception to the "no pills" rule in AGENTS.md, asked for by Will; keep them +small and quiet (hairline border, `bg-muted/40`, 10px type) so the page still reads flat. + +Facts about t3code's implementation are in the sol fact sheet +(`%LOCALAPPDATA%\Temp\threadlines-pr-review\sol-t3-facts.out`); the decisions below are ours. + +## 6a Contracts and server + +- `PullRequestActor` gains `avatarUrl: NullOr(String)`. Everything that carries an actor (list + rows, detail author, comments, review threads, timeline events) carries it. `PullRequestReviewer` + gains `avatarUrl: NullOr(String)` too. +- `PullRequestListEntry` gains `createdAt: String` (ISO) and `mergeability?: PullRequestMergeability` + (omitted where the host does not say). Needed for the Newest/Oldest sort and the conflict glyph. +- GitHub avatars, without a request per row: for a plain login (`/^[a-z0-9][a-z0-9-]{0,38}$/i`) + derive `https:///.png?size=80` (host = the remote's host, so Enterprise works). Logins + that fail that test (`dependabot[bot]`) are resolved in one batched GraphQL call per list read, + `nodes(ids: [...]) { ... on User { login avatarUrl } ... on Bot { login avatarUrl } }`, using the + `id` field `gh pr list --json author` already returns; keep the map alive with the list cache + entry. The authored search, detail, activity, and reviewer candidates add `avatarUrl` to their + GraphQL selections directly. `gh pr list` gains `createdAt` and `mergeable` in its field list; + the authored search selects `createdAt` and `mergeable`. +- GitLab: `author.avatar_url` from glab JSON. Bitbucket: `author.links.avatar.href`. Azure DevOps: + `createdBy.imageUrl` only if it is a plain URL that needs no auth header; otherwise null. Null + is fine: the web draws initials. +- Tests: decoders keep `avatarUrl` and `createdAt`; a bot login gets its URL from the batch and a + plain login gets the derived URL without a GraphQL call; `mergeability` rides on list rows. + +## 6b Web: the list + +- `PullRequestActorAvatar` in `pullRequestPresentation.tsx`: 16px round `` + with `bg-muted`, initials fallback (uppercase first letter, `text-[8px]`) when the URL is null + or fails to load (`onError`, like `MarkdownImage`). `PullRequestActorLabel` = avatar + login. +- Row meta becomes: `#number · repository (when shown) · [avatar] login · [pill][pill] +N · [check + glyph]`. Up to two label pills, then `+N`. Pill: `inline-flex max-w-40 items-center gap-1 + rounded-full border border-border/70 bg-muted/40 pl-1 pr-1.5 text-[10px] leading-3.5 + text-muted-foreground`, dot `size-2 rounded-full` coloured from the label's hex when valid + (`pullRequestLabelColor`), else `bg-muted-foreground`. Replaces today's dot-and-name text. +- Check glyph, `size-3.5`, in place of the words "Checks failing": passing `CircleCheckIcon` + emerald, failing `CircleXIcon` destructive, running `CircleDotIcon` amber, none = nothing. It is a + tooltip trigger ("All checks passed" / "Some checks failed" / "Checks running") with sr-only + text. Review words stay only for review states: "Approved", "Changes requested", + "Review required" (amber for the two that need work, emerald for Approved). +- Conflict glyph: when a row is open, not draft, and `mergeability === "conflicting"`, the PR glyph + at the left becomes `TriangleAlertIcon` in destructive with label "Conflicts with ". + Draft wins over conflict; merged and closed are unchanged. +- Toolbar: `[search] [Sort ▾] [Filters ▾ n] [refresh]`. Both menus are `Menu` from `ui/menu.tsx` + (Base UI, `MenuSub` for submenus), `align="end"`, `w-56`. +- Sort menu (radio): Merge readiness, Recently updated (default), Newest, Oldest, Largest, + Smallest. Merge readiness ranks open rows: approved with passing checks first, then review + required or no reviews, then checks running, then changes requested, then checks failing, then + conflicting, then drafts last; ties by `updatedAt` desc. Newest/Oldest use `createdAt`; + Largest/Smallest use `additions + deletions`. URL `sort` values: `readiness | updated | newest | + oldest | largest | smallest` (today's `created` and `size` map to `newest` and `largest`). +- Filters menu, one submenu per line with the current value right-aligned in muted text: + Involvement (All, Needs you, Yours, Others), separator, Author (searchable: an input at the top + of the submenu, then "Anyone" and the logins seen in the loaded rows of every state that has + been read, avatar + login, selected one first, max ten shown), Labels (searchable checklist of + the labels seen in loaded rows, with colour dots; "Any" clears), Draft (Any, Only drafts, No + drafts), Review (Any, Approved, Changes requested, Review required, No reviews), Checks (Any, + Passing, Failing, Running), separator, Project (All projects, then each project with pull + requests). State stays on the Open/Merged/Closed tab strip; it is not in the menu. +- Filters state stays in the URL (`involvement`, `author`, `labels`, `draft`, `review`, `checks`, + `project`). Drop `excludeLabels` from the UI and the URL. The active-filter chips row under the + toolbar stays as it is, gaining Involvement and Project chips. +- An Involvement narrowing hides the group headings (one group is left). +- `TextChoice` stays for the comment box verdict; the old Filters popover and `FilterText` go. + +## 6c Web: tabs and the header + +- Open pull requests are tabs. `pullRequestTabsStore.ts` (zustand, not persisted; tabs are for the + session): `tabs: PullRequestTab[]` (`{ id, environmentId, projectId, repository, number }`, + id = `env:project:repo:number`), `activeId`. `open(tab)` upserts and activates; `close(id)` + removes and activates the tab now at that index, else the last, else null. Row click = `open` + + route `pr=`. The route's `pr` param stays the source of truth for what is shown; the store + holds the set. On load with a `pr` param and no tabs, that one becomes the only tab. +- The detail column gets a tab strip at its top (page context only), in the visual language of + the thread right panel's strip (`chat/RightPanelTabStrip.tsx`: read it and reuse its classes; + do not fork its logic unless it cannot take generic tabs): each tab = state glyph + `#number`, + `role="tab"` with arrow-key roving, close `×` visible on the active tab and on hover, middle + click closes. No `+` button: the list beside the column is the way to add. Closing the last tab + clears the selection (the list takes the full width again). On phones the strip shows above the + back arrow row and the back arrow still returns to the list with the tabs kept. +- Header, both contexts, `px-4`: + 1. Row 1 (`h-7`): left `repository #number ↗` in mono `text-xs text-muted-foreground`, the + number a link to the host; right: `[Check out ▾]` (page context only; menu: "In a worktree", + "In this repository", each with a one-line description, wired to the existing checkout + dialog paths with `mode` preset), then the primary action for the state (`Merge ▾`, or + `Resolve conflicts` when conflicting, or `Update branch ▾` when behind and clean), `Close` / + `Reopen`, `⋯`, the refresh button, then the back arrow / close as today. Below `md` the + cluster wraps to a second row, as it does now. + 2. Title `text-base font-semibold leading-snug`, editable as today. + 3. `mt-2 text-xs text-muted-foreground`: `[avatar] login` (font-medium) `·` `updated 3h ago`; + right-aligned: `gh pr checkout 208` in mono as a copy button (tooltip "Copy"). + 4. `mt-3 font-mono text-xs`: `base ← head` (`ArrowLeftIcon` aria "receives changes from"), + the conflict triangle after `base` when conflicting (tooltip "Conflicts with base"), the + stacked marker as today, "behind by N" in muted after the head when behind; right: `N files` + with `FileDiffIcon` and `+adds −dels`. + 5. Tab strip Summary | Code | Timeline as today; right side on Summary: check glyph + summary + text from `detail.checks` ("All checks passed", "13 of 16 passing", "3 of 16 failing", + "9 of 11 running", "No checks reported"), a button that scrolls the Summary to Checks. + The old separate lines (labels row, base freshness line, conflict line, "Review in a thread") + fold into the above: labels move to the Summary meta rows, "Review in a thread" moves into the + Check out menu as the page's way to start a thread (keep the hand-off wiring), base freshness + into row 4 and the Update branch action. +- Summary tab opens with meta rows (`grid min-h-8 grid-cols-[6rem_minmax(0,1fr)] items-center + gap-2 py-1.5 text-xs`, icon + label at left): Reviewers (avatars + logins with state dot, the + request button at the end), Labels (pills, `text-xs` size, all of them; row hidden with none), + Comments ("2 comments", scrolls to the conversation). Then a collapsible Description section + (heading `text-sm font-medium` with a chevron, open by default, remembered in the session), + then Checks and the conversation as today. +- Update `PullRequestDetailSkeleton` to the new header shape (row 1 bar, title, author line, + branch line, tab strip). +- Tests: tabs store (open twice is one tab; close active picks the right neighbour, then the + last, then none); merge readiness order; the check summary text; the filters menu narrows the + list and writes the URL; a row with `mergeability: "conflicting"` draws the triangle; a bot + author with a null avatar draws initials. From 43c4e2e273d72bd2b322dfe32fda3bfd040060d5 Mon Sep 17 00:00:00 2001 From: badcuban <108198679+badcuban@users.noreply.github.com> Date: Thu, 3 Sep 2026 05:09:45 -0400 Subject: [PATCH 02/13] feat(server): pull request avatars and merge state on list rows Actors, reviewers, and reviewer candidates carry an avatar URL: derived from the host for plain GitHub logins, looked up in one batched GraphQL call for bots, read from the host payloads on GitLab, Bitbucket, and Azure DevOps. List rows carry the host's merge state so the list can mark conflicts. --- .../BitbucketPullRequestProvider.ts | 1 + .../GitHubPullRequestProvider.test.ts | 92 +++++++++++++++ .../pullRequest/GitHubPullRequestProvider.ts | 104 ++++++++++++++++- .../pullRequest/GitLabPullRequestProvider.ts | 10 +- .../src/pullRequest/PullRequestProvider.ts | 5 + .../pullRequest/PullRequestService.test.ts | 39 +++++++ .../src/pullRequest/PullRequestService.ts | 5 + .../src/pullRequest/azureDevOpsPullRequest.ts | 23 +++- .../pullRequest/bitbucketPullRequest.test.ts | 13 ++- .../src/pullRequest/bitbucketPullRequest.ts | 25 +++- apps/server/src/pullRequest/gitHubAvatar.ts | 110 ++++++++++++++++++ .../gitHubPullRequestDetail.test.ts | 4 +- .../pullRequest/gitHubPullRequestDetail.ts | 31 +++-- .../gitHubPullRequestGraphql.test.ts | 20 +++- .../pullRequest/gitHubPullRequestGraphql.ts | 40 +++++-- .../pullRequest/gitHubPullRequestList.test.ts | 25 ++++ .../src/pullRequest/gitHubPullRequestList.ts | 60 +++++++++- .../src/pullRequest/gitLabMergeRequest.ts | 21 +++- .../PullRequestDetailPanel.browser.tsx | 17 ++- .../PullRequestsView.browser.tsx | 10 +- .../pullRequestHandoffs.logic.test.ts | 4 +- .../pull-requests/pullRequests.logic.test.ts | 16 +-- .../pull-requests/pullRequests.logic.ts | 5 +- docs/design/pull-requests.md | 20 ++-- packages/contracts/src/pullRequest.ts | 14 ++- 25 files changed, 631 insertions(+), 83 deletions(-) create mode 100644 apps/server/src/pullRequest/gitHubAvatar.ts 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..7b9fc034 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; } diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index c23b20a9..9ba7b972 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: [], @@ -540,6 +542,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..063d95f4 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -387,6 +387,11 @@ function toEntry(input: { viewerReviewRequested: row.reviewRequestedLogins.some(matchesViewer), ...(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, }; 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..1aba48cb 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", }, diff --git a/apps/server/src/pullRequest/gitHubPullRequestGraphql.ts b/apps/server/src/pullRequest/gitHubPullRequestGraphql.ts index e4f58154..7dd5c98b 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} } } } } @@ -123,8 +124,9 @@ export const AUTHORED_PULL_REQUESTS_GRAPHQL_QUERY = `query($q: String!, $first: baseRefName additions deletions + mergeable reviewDecision - author { login } + author { login avatarUrl } repository { nameWithOwner } labels(first: ${AUTHORED_CONNECTION_PAGE_SIZE}) { nodes { name color } } reviewRequests(first: ${AUTHORED_CONNECTION_PAGE_SIZE}) { @@ -158,16 +160,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 +240,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 +266,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 +347,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 +398,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 +473,7 @@ export function decodeGitHubPullRequestConversationJson( annotationsByCommentId.set(id, { reactions: toReactions(node.reactionGroups), viewerIsAuthor: node.viewerDidAuthor === true, + author: normalizeActor(node.author), }); } @@ -523,6 +538,7 @@ 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( @@ -624,6 +640,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, @@ -720,6 +737,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 +805,7 @@ export function decodeGitHubReviewerCandidatesJson( kind, login: id, name: nonEmptyText(node.requestedReviewer?.name), + avatarUrl: nonEmptyText(node.requestedReviewer?.avatarUrl), requested: true, }); } @@ -805,6 +824,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/pull-requests/PullRequestDetailPanel.browser.tsx b/apps/web/src/components/pull-requests/PullRequestDetailPanel.browser.tsx index abd90edd..5a2c2dbb 100644 --- a/apps/web/src/components/pull-requests/PullRequestDetailPanel.browser.tsx +++ b/apps/web/src/components/pull-requests/PullRequestDetailPanel.browser.tsx @@ -89,7 +89,7 @@ const DETAIL: PullRequestDetail = { title: "Read a pull request in the app", body: "Adds the detail surface.", url: "https://github.com/threadlines/threadlines/pull/42", - author: { login: "ada", isBot: false }, + author: { login: "ada", isBot: false, avatarUrl: null }, state: "open", isDraft: false, mergeability: "mergeable", @@ -103,7 +103,7 @@ const DETAIL: PullRequestDetail = { mergedAt: null, closedAt: null, viewerIsAuthor: true, - reviewers: [{ id: "grace", kind: "user", login: "grace", state: "pending" }], + reviewers: [{ id: "grace", kind: "user", login: "grace", state: "pending", avatarUrl: null }], labels: [], checks: [ { name: "build", status: "success", description: "Passed in 2m", url: null }, @@ -132,7 +132,7 @@ const THREAD: PullRequestReviewThread = { comments: [ { id: "thread-comment-1", - author: { login: "grace", isBot: false }, + author: { login: "grace", isBot: false, avatarUrl: null }, body: "Name this something else.", createdAt: "2026-09-01T11:30:00.000Z", url: null, @@ -146,7 +146,7 @@ function makeComment(id: string, createdAt: string, body = `body ${id}`): PullRe return { id, kind: "issue-comment", - author: { login: "grace", isBot: false }, + author: { login: "grace", isBot: false, avatarUrl: null }, body, createdAt, url: null, @@ -199,7 +199,14 @@ async function renderPanel( updateComment: vi.fn(async () => undefined), reviewerCandidates: vi.fn(async () => ({ candidates: [ - { id: "grace", kind: "user" as const, login: "grace", name: "Grace H", requested: false }, + { + id: "grace", + kind: "user" as const, + login: "grace", + name: "Grace H", + avatarUrl: null, + requested: false, + }, ], })), requestReviewers, diff --git a/apps/web/src/components/pull-requests/PullRequestsView.browser.tsx b/apps/web/src/components/pull-requests/PullRequestsView.browser.tsx index 1ab8ae33..4b6101d4 100644 --- a/apps/web/src/components/pull-requests/PullRequestsView.browser.tsx +++ b/apps/web/src/components/pull-requests/PullRequestsView.browser.tsx @@ -76,7 +76,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 +122,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", @@ -399,7 +399,11 @@ describe("PullRequestsView", () => { 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: [], }); diff --git a/apps/web/src/components/pull-requests/pullRequestHandoffs.logic.test.ts b/apps/web/src/components/pull-requests/pullRequestHandoffs.logic.test.ts index ab1a50c8..ad281196 100644 --- a/apps/web/src/components/pull-requests/pullRequestHandoffs.logic.test.ts +++ b/apps/web/src/components/pull-requests/pullRequestHandoffs.logic.test.ts @@ -36,7 +36,7 @@ const CONTEXT = [ function threadComment(id: string, body: string): PullRequestReviewThread["comments"][number] { return { id, - author: { login: "grace", isBot: false }, + author: { login: "grace", isBot: false, avatarUrl: null }, body, createdAt: "2026-09-01T11:00:00.000Z", url: null, @@ -62,7 +62,7 @@ function comment(overrides: Partial = {}): PullRequestCommen return { id: "review-1", kind: "review", - author: { login: "grace", isBot: false }, + author: { login: "grace", isBot: false, avatarUrl: null }, body: "Please split this up.", createdAt: "2026-09-01T11:00:00.000Z", url: null, diff --git a/apps/web/src/components/pull-requests/pullRequests.logic.test.ts b/apps/web/src/components/pull-requests/pullRequests.logic.test.ts index 26b667c1..45d07d18 100644 --- a/apps/web/src/components/pull-requests/pullRequests.logic.test.ts +++ b/apps/web/src/components/pull-requests/pullRequests.logic.test.ts @@ -100,7 +100,7 @@ function entry(overrides: Partial = {}): PullRequestEntry { 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", @@ -424,7 +424,7 @@ describe("matchesPullRequestQuery", () => { const row = entry({ number: 412, title: "Add the pull requests page", - author: { login: "ada", isBot: false }, + author: { login: "ada", isBot: false, avatarUrl: null }, headBranch: "feature/pull-requests", repository: "threadlines/threadlines", labels: [{ name: "needs-design", color: "d73a4a" }], @@ -458,14 +458,14 @@ describe("narrowPullRequests", () => { const rows = [ entry({ number: 1, - author: { login: "ada", isBot: false }, + author: { login: "ada", isBot: false, avatarUrl: null }, labels: [{ name: "bug", color: null }], reviewDecision: "approved", checksState: "success", }), entry({ number: 2, - author: { login: "Grace", isBot: false }, + author: { login: "Grace", isBot: false, avatarUrl: null }, labels: [ { name: "bug", color: null }, { name: "wip", color: null }, @@ -731,7 +731,7 @@ describe("resolvePullRequestReviewPosition", () => { const TIMELINE_DETAIL = { createdAt: "2026-09-01T09:00:00.000Z", - author: { login: "ada", isBot: false }, + author: { login: "ada", isBot: false, avatarUrl: null }, mergedAt: null, closedAt: null, url: "https://github.com/threadlines/threadlines/pull/42", @@ -745,7 +745,7 @@ function timelineComment( return { id, kind: "issue-comment", - author: { login: "grace", isBot: false }, + author: { login: "grace", isBot: false, avatarUrl: null }, body: `body ${id}`, createdAt, url: null, @@ -805,7 +805,7 @@ describe("buildPullRequestTimeline", () => { comments: [ { id: "line-1", - author: { login: "grace", isBot: false }, + author: { login: "grace", isBot: false, avatarUrl: null }, body: "Name this something else.", createdAt: "2026-09-01T10:20:00.000Z", url: "https://github.com/threadlines/threadlines/pull/42#discussion_r1", @@ -814,7 +814,7 @@ describe("buildPullRequestTimeline", () => { }, { id: "line-2", - author: { login: "ada", isBot: false }, + author: { login: "ada", isBot: false, avatarUrl: null }, body: "Done.", createdAt: "2026-09-01T10:30:00.000Z", url: null, diff --git a/apps/web/src/components/pull-requests/pullRequests.logic.ts b/apps/web/src/components/pull-requests/pullRequests.logic.ts index 702b1c78..01943f91 100644 --- a/apps/web/src/components/pull-requests/pullRequests.logic.ts +++ b/apps/web/src/components/pull-requests/pullRequests.logic.ts @@ -1265,7 +1265,10 @@ export function buildPullRequestTimeline( id: commit.oid, at: commit.committedDate, kind: "commit", - actor: commit.authorLogin === null ? null : { login: commit.authorLogin, isBot: false }, + actor: + commit.authorLogin === null + ? null + : { login: commit.authorLogin, isBot: false, avatarUrl: null }, body: commit.messageHeadline.trim().length > 0 ? commit.messageHeadline : null, markdown: false, url: `${detail.url}/commits/${commit.oid}`, diff --git a/docs/design/pull-requests.md b/docs/design/pull-requests.md index dfba36b4..edb8bc11 100644 --- a/docs/design/pull-requests.md +++ b/docs/design/pull-requests.md @@ -1092,9 +1092,9 @@ Facts about t3code's implementation are in the sol fact sheet with `bg-muted`, initials fallback (uppercase first letter, `text-[8px]`) when the URL is null or fails to load (`onError`, like `MarkdownImage`). `PullRequestActorLabel` = avatar + login. - Row meta becomes: `#number · repository (when shown) · [avatar] login · [pill][pill] +N · [check - glyph]`. Up to two label pills, then `+N`. Pill: `inline-flex max-w-40 items-center gap-1 - rounded-full border border-border/70 bg-muted/40 pl-1 pr-1.5 text-[10px] leading-3.5 - text-muted-foreground`, dot `size-2 rounded-full` coloured from the label's hex when valid +glyph]`. Up to two label pills, then `+N`. Pill: `inline-flex max-w-40 items-center gap-1 +rounded-full border border-border/70 bg-muted/40 pl-1 pr-1.5 text-[10px] leading-3.5 +text-muted-foreground`, dot `size-2 rounded-full` coloured from the label's hex when valid (`pullRequestLabelColor`), else `bg-muted-foreground`. Replaces today's dot-and-name text. - Check glyph, `size-3.5`, in place of the words "Checks failing": passing `CircleCheckIcon` emerald, failing `CircleXIcon` destructive, running `CircleDotIcon` amber, none = nothing. It is a @@ -1111,7 +1111,7 @@ Facts about t3code's implementation are in the sol fact sheet required or no reviews, then checks running, then changes requested, then checks failing, then conflicting, then drafts last; ties by `updatedAt` desc. Newest/Oldest use `createdAt`; Largest/Smallest use `additions + deletions`. URL `sort` values: `readiness | updated | newest | - oldest | largest | smallest` (today's `created` and `size` map to `newest` and `largest`). +oldest | largest | smallest` (today's `created` and `size` map to `newest` and `largest`). - Filters menu, one submenu per line with the current value right-aligned in muted text: Involvement (All, Needs you, Yours, Others), separator, Author (searchable: an input at the top of the submenu, then "Anyone" and the logins seen in the loaded rows of every state that has @@ -1132,7 +1132,7 @@ Facts about t3code's implementation are in the sol fact sheet session): `tabs: PullRequestTab[]` (`{ id, environmentId, projectId, repository, number }`, id = `env:project:repo:number`), `activeId`. `open(tab)` upserts and activates; `close(id)` removes and activates the tab now at that index, else the last, else null. Row click = `open` - + route `pr=`. The route's `pr` param stays the source of truth for what is shown; the store + and a route change to `pr=`. The route's `pr` param stays the source of truth for what is shown; the store holds the set. On load with a `pr` param and no tabs, that one becomes the only tab. - The detail column gets a tab strip at its top (page context only), in the visual language of the thread right panel's strip (`chat/RightPanelTabStrip.tsx`: read it and reuse its classes; @@ -1159,12 +1159,12 @@ Facts about t3code's implementation are in the sol fact sheet 5. Tab strip Summary | Code | Timeline as today; right side on Summary: check glyph + summary text from `detail.checks` ("All checks passed", "13 of 16 passing", "3 of 16 failing", "9 of 11 running", "No checks reported"), a button that scrolls the Summary to Checks. - The old separate lines (labels row, base freshness line, conflict line, "Review in a thread") - fold into the above: labels move to the Summary meta rows, "Review in a thread" moves into the - Check out menu as the page's way to start a thread (keep the hand-off wiring), base freshness - into row 4 and the Update branch action. + The old separate lines (labels row, base freshness line, conflict line, "Review in a thread") + fold into the above: labels move to the Summary meta rows, "Review in a thread" moves into the + Check out menu as the page's way to start a thread (keep the hand-off wiring), base freshness + into row 4 and the Update branch action. - Summary tab opens with meta rows (`grid min-h-8 grid-cols-[6rem_minmax(0,1fr)] items-center - gap-2 py-1.5 text-xs`, icon + label at left): Reviewers (avatars + logins with state dot, the +gap-2 py-1.5 text-xs`, icon + label at left): Reviewers (avatars + logins with state dot, the request button at the end), Labels (pills, `text-xs` size, all of them; row hidden with none), Comments ("2 comments", scrolls to the conversation). Then a collapsible Description section (heading `text-sm font-medium` with a chevron, open by default, remembered in the session), diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index 33c17a04..d92e2af1 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -26,9 +26,14 @@ export type PullRequestReviewDecision = typeof PullRequestReviewDecision.Type; export const PullRequestChecksState = Schema.Literals(["pending", "success", "failure"]); export type PullRequestChecksState = typeof PullRequestChecksState.Type; +export const PullRequestMergeability = Schema.Literals(["mergeable", "conflicting", "unknown"]); +export type PullRequestMergeability = typeof PullRequestMergeability.Type; + export const PullRequestActor = Schema.Struct({ login: TrimmedNonEmptyString, isBot: Schema.Boolean, + /** The host's picture for this account; null where it names none. */ + avatarUrl: Schema.NullOr(Schema.String), }); export type PullRequestActor = typeof PullRequestActor.Type; @@ -80,6 +85,8 @@ export const PullRequestListEntry = Schema.Struct({ reviewDecision: Schema.optionalKey(PullRequestReviewDecision), /** Absent when there are no checks, or when checks were not requested. */ checksState: Schema.optionalKey(PullRequestChecksState), + /** Absent where the host does not say whether the branch still merges. */ + mergeability: Schema.optionalKey(PullRequestMergeability), labels: Schema.Array(PullRequestLabel), origin: PullRequestListEntryOrigin, }); @@ -150,9 +157,6 @@ export const PullRequestRef = Schema.Struct({ }); export type PullRequestRef = typeof PullRequestRef.Type; -export const PullRequestMergeability = Schema.Literals(["mergeable", "conflicting", "unknown"]); -export type PullRequestMergeability = typeof PullRequestMergeability.Type; - export const PullRequestCheckStatus = Schema.Literals(["pending", "success", "failure", "skipped"]); export type PullRequestCheckStatus = typeof PullRequestCheckStatus.Type; @@ -193,6 +197,8 @@ export const PullRequestReviewer = Schema.Struct({ kind: PullRequestReviewerKind, login: TrimmedNonEmptyString, state: PullRequestReviewerState, + /** The host's picture for this reviewer; null where it names none. */ + avatarUrl: Schema.NullOr(Schema.String), }); export type PullRequestReviewer = typeof PullRequestReviewer.Type; @@ -574,6 +580,8 @@ export const PullRequestReviewerCandidate = Schema.Struct({ kind: PullRequestReviewerKind, login: TrimmedNonEmptyString, name: Schema.NullOr(Schema.String), + /** The host's picture for this candidate; null where it names none. */ + avatarUrl: Schema.NullOr(Schema.String), requested: Schema.Boolean, }); export type PullRequestReviewerCandidate = typeof PullRequestReviewerCandidate.Type; From 92f5396f094e15885e4f331ec360f15f0a1e041a Mon Sep 17 00:00:00 2001 From: badcuban <108198679+badcuban@users.noreply.github.com> Date: Thu, 3 Sep 2026 05:33:49 -0400 Subject: [PATCH 03/13] feat(web): pull request list avatars, label pills, check glyphs, and menus Rows show the author's avatar, up to two label pills with the label's colour, a check glyph in place of the words, and a conflict triangle in place of the state glyph when the branch no longer merges. Sort and Filters are menus: Sort adds merge readiness, newest, oldest, largest and smallest; Filters gets submenus for involvement, author (searchable, with avatars), labels, draft, review, checks and project. Below tablet width the row keeps the number, author and glyph. --- .../pull-requests/PullRequestFilters.tsx | 528 +++++++++++++----- .../PullRequestsView.browser.tsx | 63 ++- .../pull-requests/PullRequestsView.tsx | 231 ++++++-- .../pull-requests/pullRequestPresentation.tsx | 134 ++++- .../pull-requests/pullRequests.logic.test.ts | 113 +++- .../pull-requests/pullRequests.logic.ts | 345 ++++++++++-- apps/web/src/lib/pullRequestsReactQuery.ts | 43 ++ 7 files changed, 1176 insertions(+), 281 deletions(-) diff --git a/apps/web/src/components/pull-requests/PullRequestFilters.tsx b/apps/web/src/components/pull-requests/PullRequestFilters.tsx index 57fd66f1..fc9d18b6 100644 --- a/apps/web/src/components/pull-requests/PullRequestFilters.tsx +++ b/apps/web/src/components/pull-requests/PullRequestFilters.tsx @@ -3,67 +3,137 @@ * left in. Everything here reads the rows the page already loaded, so a filter * never costs a read, and it all lives in the URL, so a link keeps it. */ -import { SlidersHorizontalIcon, XIcon } from "lucide-react"; -import { useState, type ReactNode } from "react"; +import { ArrowUpDownIcon, ChevronDownIcon, SlidersHorizontalIcon, XIcon } from "lucide-react"; +import { useMemo, useState, type ReactNode } from "react"; +import { useLoadedPullRequestEntries } from "../../lib/pullRequestsReactQuery"; import { cn } from "../../lib/utils"; import { Button } from "../ui/button"; import { Input } from "../ui/input"; -import { Popover, PopoverPopup, PopoverTrigger } from "../ui/popover"; -import { SECTION_LABEL_CLASS, TEXT_BUTTON_CLASS, TextChoice } from "./pullRequestPresentation"; import { + Menu, + MenuCheckboxItem, + MenuItem, + MenuPopup, + MenuRadioGroup, + MenuRadioItem, + MenuSeparator, + MenuSub, + MenuSubPopup, + MenuSubTrigger, + MenuTrigger, + MENU_PICK_ITEM_CLASS_NAME, + MENU_PICK_ITEM_SELECTED_CLASS_NAME, +} from "../ui/menu"; +import { PullRequestActorAvatar } from "./pullRequestPresentation"; +import { + hasPullRequestLabel, + PULL_REQUEST_INVOLVEMENT_WORDS, PULL_REQUEST_SORT_LABELS, + pullRequestAuthorFacets, pullRequestFilterChips, + pullRequestLabelColor, + pullRequestLabelFacets, + pullRequestProjectFacets, + togglePullRequestLabel, type PullRequestChecksFilter, type PullRequestDraftFilter, type PullRequestFilters, + type PullRequestInvolvementFilter, type PullRequestReviewFilter, type PullRequestSort, } from "./pullRequests.logic"; -const DRAFT_OPTIONS: readonly { value: PullRequestDraftFilter; label: string }[] = [ +/** A menu is not a place to scroll through a hundred logins. */ +const MAX_AUTHOR_CHOICES = 10; + +interface FilterOption { + readonly value: Value; + readonly label: string; +} + +const INVOLVEMENT_OPTIONS: readonly FilterOption[] = ( + ["all", "needs-you", "yours", "others"] as const +).map((value) => ({ value, label: PULL_REQUEST_INVOLVEMENT_WORDS[value] })); + +const DRAFT_OPTIONS: readonly FilterOption[] = [ { value: "any", label: "Any" }, { value: "only", label: "Only drafts" }, { value: "hide", label: "No drafts" }, ]; -const REVIEW_OPTIONS: readonly { value: PullRequestReviewFilter; label: string }[] = [ +const REVIEW_OPTIONS: readonly FilterOption[] = [ { value: "any", label: "Any" }, { value: "approved", label: "Approved" }, { value: "changes-requested", label: "Changes requested" }, { value: "review-required", label: "Review required" }, + { value: "none", label: "No reviews" }, ]; -const CHECKS_OPTIONS: readonly { value: PullRequestChecksFilter; label: string }[] = [ +const CHECKS_OPTIONS: readonly FilterOption[] = [ { value: "any", label: "Any" }, { value: "passing", label: "Passing" }, { value: "failing", label: "Failing" }, + { value: "running", label: "Running" }, ]; -const SORT_OPTIONS: readonly { value: PullRequestSort; label: string }[] = ( - ["updated", "created", "size"] as const +const SORT_OPTIONS: readonly FilterOption[] = ( + ["readiness", "updated", "newest", "oldest", "largest", "smallest"] as const ).map((value) => ({ value, label: PULL_REQUEST_SORT_LABELS[value] })); -/** The control beside the search, and everything it opens. */ +/** The order the list is read in, as one menu of plain choices. */ +export function PullRequestSortMenu({ + sort, + onSortChange, +}: { + readonly sort: PullRequestSort; + readonly onSortChange: (sort: PullRequestSort) => void; +}) { + return ( + + + } + > + + Sort + + + + onSortChange(value as PullRequestSort)} + > + {SORT_OPTIONS.map((option) => ( + + {option.label} + + ))} + + + + ); +} + +/** Every narrowing the list offers, one line each, the current value alongside. */ export function PullRequestFiltersButton({ filters, - sort, - viewer, onFiltersChange, - onSortChange, }: { readonly filters: PullRequestFilters; - readonly sort: PullRequestSort; - /** The signed-in login, offered as a quick pick for the author field. */ - readonly viewer: string | null; readonly onFiltersChange: (filters: PullRequestFilters) => void; - readonly onSortChange: (sort: PullRequestSort) => void; }) { const activeCount = pullRequestFilterChips(filters).length; return ( - - + ) : null} - - - - onFiltersChange({ ...filters, author })} - /> - + + + + + + + ); +} - - onFiltersChange({ ...filters, labels })} - /> - +/** + * The menu's lines. Its own component because the popup only mounts while it + * is open: the author, label and project choices are read from every listing + * the page holds, and there is no reason to gather them until they are asked + * for. + */ +function PullRequestFiltersMenuContent({ + filters, + onFiltersChange, +}: { + readonly filters: PullRequestFilters; + readonly onFiltersChange: (filters: PullRequestFilters) => void; +}) { + const entries = useLoadedPullRequestEntries(); + const authors = useMemo(() => pullRequestAuthorFacets(entries), [entries]); + const labels = useMemo(() => pullRequestLabelFacets(entries), [entries]); + const projects = useMemo(() => pullRequestProjectFacets(entries), [entries]); - - onFiltersChange({ ...filters, excludeLabels })} - /> - + return ( + <> + onFiltersChange({ ...filters, involvement })} + /> - - onFiltersChange({ ...filters, draft })} - /> - + - - onFiltersChange({ ...filters, review })} - /> - + onFiltersChange({ ...filters, author })} + /> + onFiltersChange({ ...filters, labels: nextLabels })} + /> + onFiltersChange({ ...filters, draft })} + /> + onFiltersChange({ ...filters, review })} + /> + onFiltersChange({ ...filters, checks })} + /> + + + + + onFiltersChange({ ...filters, project: "" })} + > + All projects + + {projects.map((project) => ( + onFiltersChange({ ...filters, project: project.key })} + > + {project.label} + + ))} + + + ); +} - - onFiltersChange({ ...filters, checks })} +/** One line of the menu: what it narrows, what it is narrowed to, and the choices. */ +function FilterSubmenu({ + label, + value, + children, +}: { + readonly label: string; + readonly value: string; + readonly children: ReactNode; +}) { + return ( + + + {label} + {value} + + {children} + + ); +} + +/** A submenu of one choice out of a few. */ +function FilterRadioSubmenu({ + label, + value, + options, + onChange, +}: { + readonly label: string; + readonly value: Value; + readonly options: readonly FilterOption[]; + readonly onChange: (value: Value) => void; +}) { + const current = options.find((option) => option.value === value); + return ( + + onChange(next as Value)}> + {options.map((option) => ( + + {option.label} + + ))} + + + ); +} + +/** The logins the loaded rows carry, the chosen one first, searchable. */ +function AuthorSubmenu({ + authors, + value, + onChange, +}: { + readonly authors: readonly { login: string; avatarUrl: string | null }[]; + readonly value: string; + readonly onChange: (author: string) => void; +}) { + const [search, setSearch] = useState(""); + const selected = value.trim().toLowerCase(); + const matching = authors + .filter((author) => author.login.toLowerCase().includes(search.trim().toLowerCase())) + .toSorted( + (left, right) => + Number(right.login.toLowerCase() === selected) - + Number(left.login.toLowerCase() === selected), + ) + .slice(0, MAX_AUTHOR_CHOICES); + + return ( + + + onChange("")} + > + Anyone + + {matching.map((author) => ( + onChange(author.login)} + > + - + {author.login} + + ))} + + ); +} + +/** The labels the loaded rows carry, as many as the user wants at once. */ +function LabelsSubmenu({ + labels, + value, + onChange, +}: { + readonly labels: readonly { name: string; color: string | null }[]; + readonly value: string; + readonly onChange: (labels: string) => void; +}) { + const [search, setSearch] = useState(""); + const matching = labels.filter((label) => + label.name.toLowerCase().includes(search.trim().toLowerCase()), + ); - - - - - + return ( + + + onChange("")} + > + Any + + {matching.map((label) => { + const dot = pullRequestLabelColor(label.color); + return ( + onChange(togglePullRequestLabel(value, label.name))} + > + + + {label.name} + + + ); + })} + ); } +/** The field at the top of a long submenu, filtering the lines below it. */ +function FilterSearchInput({ + label, + value, + onChange, +}: { + readonly label: string; + readonly value: string; + readonly onChange: (value: string) => void; +}) { + return ( +
+ onChange(event.target.value)} + // A menu treats letters as a jump to the item that starts with them, + // which would take the field's own typing away from it. Everything + // that steers the menu — arrows, Escape, Enter, Tab — still goes up. + onKeyDown={(event) => { + if (event.key.length === 1) { + event.stopPropagation(); + } + }} + /> +
+ ); +} + +/** What the Project line reads as, the key standing in until the rows name it. */ +function projectValueLabel( + projects: readonly { key: string; label: string }[], + value: string, +): string { + if (value.trim() === "") { + return "All projects"; + } + return projects.find((project) => project.key === value)?.label ?? value; +} + /** Every narrowing in force, each one a word and the way to lift it. */ export function PullRequestFilterChipsRow({ filters, + projectLabel, onFiltersChange, }: { readonly filters: PullRequestFilters; + /** What the chosen project is called, which only the loaded rows know. */ + readonly projectLabel?: string; readonly onFiltersChange: (filters: PullRequestFilters) => void; }) { - const chips = pullRequestFilterChips(filters); + const chips = pullRequestFilterChips(filters, projectLabel); if (chips.length === 0) { return null; } @@ -188,83 +486,3 @@ export function PullRequestFilterChipsRow({ ); } - -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/PullRequestsView.browser.tsx b/apps/web/src/components/pull-requests/PullRequestsView.browser.tsx index 4b6101d4..c8f9a826 100644 --- a/apps/web/src/components/pull-requests/PullRequestsView.browser.tsx +++ b/apps/web/src/components/pull-requests/PullRequestsView.browser.tsx @@ -37,6 +37,7 @@ import { PullRequestsView } from "./PullRequestsView"; import { DEFAULT_PULL_REQUEST_SORT, EMPTY_PULL_REQUEST_FILTERS, + pullRequestFiltersToSearch, type PullRequestFilters, type PullRequestSelection, type PullRequestSort, @@ -239,6 +240,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 +256,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); + }} /> ); } @@ -394,7 +404,7 @@ 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: [ @@ -410,10 +420,20 @@ describe("PullRequestsView", () => { 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"); + await vi.waitFor(() => { + expect(page.getByRole("menuitem", { name: "grace", exact: true }).elements()).toHaveLength(0); + }); + await userEvent.click(page.getByRole("menuitem", { 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); }); @@ -424,6 +444,39 @@ describe("PullRequestsView", () => { await rendered.cleanup(); }); + it("draws the conflict, 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", + 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 checks are a glyph, and it carries the words the row no longer spends + // its meta line on. + 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..fc70b49c 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"; @@ -44,8 +45,16 @@ 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 { + PullRequestFilterChipsRow, + PullRequestFiltersButton, + PullRequestSortMenu, +} from "./PullRequestFilters"; +import { + PullRequestActorAvatar, + PullRequestChecksGlyph, + pullRequestHostName, +} from "./pullRequestPresentation"; import { groupPullRequests, hasPullRequestProject, @@ -54,8 +63,11 @@ import { narrowPullRequests, projectRepository, pullRequestBadgeTone, + pullRequestConflictLabel, pullRequestEntryKey, pullRequestFilterChips, + pullRequestLabelColor, + pullRequestProjectFacets, requiresHostSignIn, resolveNeedsYouReason, resolvePullRequestListSpan, @@ -190,8 +202,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. @@ -469,9 +498,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} @@ -741,6 +770,32 @@ function PullRequestsNotice({ ); } +/** + * One label as the host paints it: a hairline pill with the label's own colour + * in the dot and nowhere else, so a row of them stays as quiet as the rest of + * the meta line. The pills are the page's one exception to the flat rule, and + * they earn it by naming what a colour alone cannot. + */ +function PullRequestLabelPill({ + name, + color, +}: { + readonly name: string; + readonly color: string | null; +}) { + const dot = pullRequestLabelColor(color); + return ( + + + {name} + + ); +} + function PullRequestRow({ entry, linkedThread, @@ -765,6 +820,9 @@ function PullRequestRow({ className: glyphClassName, label: glyphLabel, } = pullRequestBadgeTone(entry.state, entry.isDraft); + // 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); const reason = resolveNeedsYouReason(entry); const openOnHostLabel = `Open on ${pullRequestHostName(entry.provider)}`; // Nothing here is checked out, so there is no thread to open and no branch to @@ -775,26 +833,57 @@ 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 below md, separator and all, where the line has no room for it. */ + 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 + // The checks say for themselves that they failed, in a glyph at the end of + // the line; the rest of the reasons are review words with no glyph. + ...(reason && reason !== "Checks failing" ? [ { key: "reason", @@ -803,10 +892,39 @@ function PullRequestRow({ reason === "Approved" ? "text-emerald-600 dark:text-emerald-300/90" : "text-amber-600/90 dark:text-amber-400/80", - text: reason, + content: reason, }, ] : []), + ...(visibleLabels.length > 0 + ? [ + { + 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} + + ), + }, + ] + : []), + ...(entry.checksState === undefined + ? [] + : [ + { + key: "checks", + fit: "whole" as const, + content: , + }, + ]), ]; return ( @@ -833,10 +951,21 @@ function PullRequestRow({ />
{/* 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 +983,15 @@ function PullRequestRow({ className={cn( "flex items-center gap-1.5", item.fit === "whole" ? "shrink-0" : "min-w-0", + item.hideOnPhone && "max-md:hidden", )} > {index > 0 ? · : null} - - {item.text} + + {item.content} ))} - {visibleLabels.map((label) => ( - - - {label.name} - - ))} - {hiddenLabelCount > 0 ? +{hiddenLabelCount} : null} {linkedThread ? ( ) : 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 +542,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 +562,7 @@ function PullRequestDetailHeader({ isRefreshing, onRefresh, onClose, - onReviewInThread, + onCheckout, onOpenThread, handoffs, }: { @@ -514,83 +575,120 @@ 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. */} +
+ + {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 +699,185 @@ 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 ( + + + ); } @@ -773,7 +975,7 @@ function PullRequestTitle({ {/* A phone's column is narrow enough that one truncated line says almost nothing, so there it wraps to two before it gives up. */} {detail.title} @@ -829,16 +1031,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 +1063,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 +1110,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 +1181,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 +1290,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 +1318,10 @@ function PullRequestHeaderActions({ ) : null} - {handoffs && hasMenuWrites ? : null} + {(handoffs || showMergeInMenu) && + (showDraftToggle || showDisableAutoMerge || showEnableAutoMerge) ? ( + + ) : null} {showDraftToggle ? ( detail.isDraft ? ( run("ready")}>Mark as ready @@ -1109,7 +1349,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 +1378,69 @@ 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.`} + + + {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. */ diff --git a/apps/web/src/components/pull-requests/PullRequestSummaryTab.tsx b/apps/web/src/components/pull-requests/PullRequestSummaryTab.tsx index f2ac0ee2..1e875ada 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,18 +35,21 @@ 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, @@ -71,9 +84,61 @@ export function PullRequestSummaryTab({ readonly onFixCheck?: (check: PullRequestCheck) => void; }) { const comments = activity?.comments ?? null; + const conversation = useRef(null); + const scrollToConversation = useCallback(() => { + conversation.current?.scrollIntoView({ block: "start", behavior: "smooth" }); + }, []); 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" + > + + +
+ - +
+ -
-
-

- 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 +283,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 +395,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..21f0a9c3 --- /dev/null +++ b/apps/web/src/components/pull-requests/PullRequestTabStrip.tsx @@ -0,0 +1,169 @@ +/** + * 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 type { PullRequestState } from "@threadlines/contracts"; +import { XIcon } from "lucide-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"; + +/** A tab, plus what the list knows about the pull request it stands for. */ +export interface PullRequestTabView extends PullRequestTab { + readonly state: PullRequestState; + readonly isDraft: boolean; +} + +/** 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 PullRequestTabView[]; + readonly activeId: string | null; + /** The detail below, so each tab can name what it controls. */ + readonly panelId: string; + readonly onSelect: (tab: PullRequestTabView) => void; + readonly onClose: (tab: PullRequestTabView) => void; +}) { + 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={() => onClose(tab)} + /> + ))} +
+ +
+
+ ); +} + +function PullRequestTabStripItem({ + tab, + active, + panelId, + onSelect, + onClose, +}: { + readonly tab: PullRequestTabView; + readonly active: boolean; + readonly panelId: string; + readonly onSelect: () => void; + readonly onClose: () => void; +}) { + const tone = pullRequestBadgeTone(tab.state, tab.isDraft); + return ( + +
{ + if (event.button !== 1) return; + event.preventDefault(); + onClose(); + }} + 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 c8f9a826..ad3ccfc8 100644 --- a/apps/web/src/components/pull-requests/PullRequestsView.browser.tsx +++ b/apps/web/src/components/pull-requests/PullRequestsView.browser.tsx @@ -34,6 +34,7 @@ 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, @@ -308,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(); }); @@ -376,6 +380,36 @@ 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. + await userEvent.click(page.getByRole("button", { name: "Close pull request #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("steps back to the list from the detail on a phone", async () => { await page.viewport(390, 800); try { diff --git a/apps/web/src/components/pull-requests/PullRequestsView.tsx b/apps/web/src/components/pull-requests/PullRequestsView.tsx index fc70b49c..fe7ee2b1 100644 --- a/apps/web/src/components/pull-requests/PullRequestsView.tsx +++ b/apps/web/src/components/pull-requests/PullRequestsView.tsx @@ -45,14 +45,22 @@ import { PageTabButton, pageTabId } from "../ui/page-tabs"; import { Skeleton } from "../ui/skeleton"; import { TooltipWrapper } from "../ui/tooltip"; import { LazyPullRequestDetailPanel } from "./LazyPullRequestDetailPanel"; +import type { PullRequestCheckoutRequest } from "./PullRequestDetailPanel"; import { PullRequestFilterChipsRow, PullRequestFiltersButton, PullRequestSortMenu, } from "./PullRequestFilters"; +import { PullRequestTabStrip, type PullRequestTabView } from "./PullRequestTabStrip"; +import { + pullRequestTabId, + usePullRequestTabsStore, + type PullRequestTab, +} from "./pullRequestTabsStore"; import { PullRequestActorAvatar, PullRequestChecksGlyph, + PullRequestLabelPill, pullRequestHostName, } from "./pullRequestPresentation"; import { @@ -66,7 +74,6 @@ import { pullRequestConflictLabel, pullRequestEntryKey, pullRequestFilterChips, - pullRequestLabelColor, pullRequestProjectFacets, requiresHostSignIn, resolveNeedsYouReason, @@ -151,8 +158,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. @@ -185,6 +197,11 @@ 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 snapshot = usePullRequestLists({ state, @@ -256,7 +273,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, @@ -268,7 +285,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], @@ -288,19 +306,39 @@ export function PullRequestsView({ const userSelectedKey = pressedRow?.key ?? null; const rowToRefocus = useRef(null); - const handleSelect = useCallback( - (entry: PullRequestEntry) => { + // One pull request opened, from a row or from a tab. The repository rides + // along because the URL carries only the project, and the panel addresses a + // pull request by repository as well as by number. + const showPullRequest = useCallback( + (target: { + readonly environmentId: EnvironmentId; + readonly projectId: ProjectId; + readonly repository: string; + readonly number: number; + }) => { setPressedRow({ - key: `${entry.environmentId}:${entry.projectId}:${entry.number}`, - repository: entry.repository, + key: `${target.environmentId}:${target.projectId}:${target.number}`, + repository: target.repository, }); onSelectionChange({ + environmentId: target.environmentId, + projectId: target.projectId, + number: target.number, + }); + }, + [onSelectionChange], + ); + const handleSelect = useCallback( + (entry: PullRequestEntry) => { + openTab({ environmentId: entry.environmentId, projectId: entry.projectId, + repository: entry.repository, number: entry.number, }); + showPullRequest(entry); }, - [onSelectionChange], + [openTab, showPullRequest], ); const closeSelection = useCallback(() => { // Read while the row still wears the mark, since the mark goes with the @@ -363,6 +401,62 @@ 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, where the listing still + // carries that row; one it has dropped keeps the open glyph rather than + // vanishing from the strip. + const tabViews = useMemo( + () => + tabs.map((tab) => { + const entry = snapshot.entries.find( + (candidate) => + candidate.environmentId === tab.environmentId && + candidate.projectId === tab.projectId && + candidate.number === tab.number, + ); + return { ...tab, state: entry?.state ?? "open", isDraft: entry?.isDraft ?? false }; + }), + [snapshot.entries, 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) { + showPullRequest(next); + return; + } + closeSelection(); + }, + [activeTabId, closeSelection, closeTab, showPullRequest], + ); + // 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(() => { @@ -579,22 +673,37 @@ 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}
@@ -607,6 +716,7 @@ export function PullRequestsView({ threadId={dialogTarget.threadId} cwd={dialogTarget.cwd} initialReference={dialogTarget.url} + {...(dialogTarget.mode ? { defaultMode: dialogTarget.mode } : {})} onOpenChange={(open) => { if (!open) { setDialogTarget(null); @@ -770,32 +880,6 @@ function PullRequestsNotice({ ); } -/** - * One label as the host paints it: a hairline pill with the label's own colour - * in the dot and nowhere else, so a row of them stays as quiet as the rest of - * the meta line. The pills are the page's one exception to the flat rule, and - * they earn it by naming what a colour alone cannot. - */ -function PullRequestLabelPill({ - name, - color, -}: { - readonly name: string; - readonly color: string | null; -}) { - const dot = pullRequestLabelColor(color); - return ( - - - {name} - - ); -} - function PullRequestRow({ entry, linkedThread, diff --git a/apps/web/src/components/pull-requests/pullRequestPresentation.tsx b/apps/web/src/components/pull-requests/pullRequestPresentation.tsx index e0b4ce1d..c002548d 100644 --- a/apps/web/src/components/pull-requests/pullRequestPresentation.tsx +++ b/apps/web/src/components/pull-requests/pullRequestPresentation.tsx @@ -29,6 +29,7 @@ import { } from "lucide-react"; import { resolveChangeRequestPresentationForKind } from "../../sourceControlPresentation"; +import { pullRequestLabelColor } from "./pullRequests.logic"; export const SECTION_LABEL_CLASS = "mb-2 font-mono text-[10px] uppercase tracking-wider text-muted-foreground/55 select-none"; @@ -123,6 +124,40 @@ export function PullRequestActorLabel({ ); } +/** + * One label as the host paints it: a hairline pill with the label's own colour + * in the dot and nowhere else, so a row of them stays as quiet as the rest of + * the meta line. The pills are the page's one exception to the flat rule, and + * they earn it by naming what a colour alone cannot. + */ +export function PullRequestLabelPill({ + name, + color, + className, +}: { + readonly name: string; + readonly color: string | null; + /** The Summary's rows carry them at the surrounding `text-xs`. */ + readonly className?: string; +}) { + const dot = pullRequestLabelColor(color); + return ( + + + {name} + + ); +} + /** The check rollup as a glyph: a colour and a word, no room for a sentence. */ const CHECKS_STATE_PRESENTATION = { success: { @@ -147,6 +182,18 @@ const CHECKS_STATE_PRESENTATION = { { label: string; Icon: typeof CircleCheckIcon; className: string } >; +/** + * The glyph and words for a check rollup, for a surface that draws them itself + * rather than taking {@link PullRequestChecksGlyph} whole (the detail header, + * whose glyph sits inside a button of its own and must not carry a second + * tooltip). Null where the host reported no checks at all. + */ +export function pullRequestChecksTone( + state: PullRequestChecksState | "none" | undefined, +): (typeof CHECKS_STATE_PRESENTATION)[PullRequestChecksState] | null { + return state === undefined || state === "none" ? null : CHECKS_STATE_PRESENTATION[state]; +} + /** * Where a list row would otherwise spend its meta line on the words "Checks * failing". The word itself stays for anyone who cannot see the colour, and @@ -159,10 +206,10 @@ export function PullRequestChecksGlyph({ readonly state: PullRequestChecksState | undefined; readonly className?: string; }) { - if (state === undefined) { + const presentation = pullRequestChecksTone(state); + if (presentation === null) { return null; } - const presentation = CHECKS_STATE_PRESENTATION[state]; return ( void }) { return (
-
+
+ {/* Row 1: the repository line, with the way out where the header keeps it. */}
- {onClose ? : null} - - - {onClose ? : null} + + + {onClose ? : null} + {onClose ? : null} +
+ {/* Row 2: the title. */} + + {/* Row 3: the author and when it last moved. */}
- - +
+ {/* Row 4: base ← head, with the file count at the far end. */} +
+ + + + +
diff --git a/apps/web/src/components/pull-requests/pullRequestTabsStore.test.ts b/apps/web/src/components/pull-requests/pullRequestTabsStore.test.ts new file mode 100644 index 00000000..033a501a --- /dev/null +++ b/apps/web/src/components/pull-requests/pullRequestTabsStore.test.ts @@ -0,0 +1,67 @@ +import { EnvironmentId, ProjectId } from "@threadlines/contracts"; +import { beforeEach, describe, expect, it } from "vite-plus/test"; + +import { + resetPullRequestTabsForTests, + usePullRequestTabsStore, + type PullRequestTabTarget, +} from "./pullRequestTabsStore"; + +const ENVIRONMENT_ID = EnvironmentId.make("env-1"); +const PROJECT_ID = ProjectId.make("project-1"); + +function target(number: number): PullRequestTabTarget { + return { + environmentId: ENVIRONMENT_ID, + projectId: PROJECT_ID, + repository: "threadlines/threadlines", + number, + }; +} + +const store = () => usePullRequestTabsStore.getState(); +const numbers = () => store().tabs.map((tab) => tab.number); +const activeNumber = () => store().tabs.find((tab) => tab.id === store().activeId)?.number ?? null; + +describe("pullRequestTabsStore", () => { + beforeEach(() => { + resetPullRequestTabsForTests(); + }); + + it("opens a pull request once however often it is asked for", () => { + store().open(target(1)); + store().open(target(2)); + store().open(target(1)); + + expect(numbers()).toEqual([1, 2]); + // The second press moves to the tab rather than adding one beside it. + expect(activeNumber()).toBe(1); + }); + + it("closes the active tab onto its right neighbour, then the last, then nothing", () => { + store().open(target(1)); + store().open(target(2)); + store().open(target(3)); + store().open(target(2)); + + expect(store().close(store().activeId ?? "")?.number).toBe(3); + expect(numbers()).toEqual([1, 3]); + expect(activeNumber()).toBe(3); + + // Nothing to the right of the last tab, so the strip falls back to its end. + expect(store().close(store().activeId ?? "")?.number).toBe(1); + expect(activeNumber()).toBe(1); + + expect(store().close(store().activeId ?? "")).toBeNull(); + expect(numbers()).toEqual([]); + expect(store().activeId).toBeNull(); + }); + + it("leaves the active tab alone when a background one is closed", () => { + const first = store().open(target(1)); + store().open(target(2)); + + expect(store().close(first.id)?.number).toBe(2); + expect(numbers()).toEqual([2]); + }); +}); diff --git a/apps/web/src/components/pull-requests/pullRequestTabsStore.ts b/apps/web/src/components/pull-requests/pullRequestTabsStore.ts new file mode 100644 index 00000000..61cdd568 --- /dev/null +++ b/apps/web/src/components/pull-requests/pullRequestTabsStore.ts @@ -0,0 +1,82 @@ +/** + * The pull requests the page has open at once, as a strip of tabs. + * + * The route's `pr` param stays the source of truth for which one is on screen; + * this store only holds the set and the order they were opened in. It is not + * persisted: tabs are a working set for one sitting, and a reload that restored + * six of them would be restoring someone else's afternoon. + */ +import type { EnvironmentId, ProjectId } from "@threadlines/contracts"; +import { create } from "zustand"; + +export interface PullRequestTab { + /** `environment:project:repository:number`, from {@link pullRequestTabId}. */ + readonly id: string; + readonly environmentId: EnvironmentId; + readonly projectId: ProjectId; + readonly repository: string; + readonly number: number; +} + +export type PullRequestTabTarget = Omit; + +/** + * One pull request across environments and checkouts. The repository is in the + * key as well as the project, because one project can read more than one + * remote and the same number means a different pull request on each. + */ +export function pullRequestTabId(target: PullRequestTabTarget): string { + return `${target.environmentId}:${target.projectId}:${target.repository.toLowerCase()}:${target.number}`; +} + +interface PullRequestTabsState { + readonly tabs: readonly PullRequestTab[]; + readonly activeId: string | null; + /** Adds the tab if it is new, and makes it the active one either way. */ + readonly open: (target: PullRequestTabTarget) => PullRequestTab; + /** + * Drops one tab and answers with whichever is active afterwards: the tab now + * at the closed one's index (its right neighbour), else the last one left, + * else null. Closing a background tab leaves the active one alone. + */ + readonly close: (id: string) => PullRequestTab | null; +} + +export const usePullRequestTabsStore = create((set, get) => ({ + tabs: [], + activeId: null, + open: (target) => { + const id = pullRequestTabId(target); + const { tabs, activeId } = get(); + const existing = tabs.find((tab) => tab.id === id); + if (existing) { + if (activeId !== id) { + set({ activeId: id }); + } + return existing; + } + const tab: PullRequestTab = { id, ...target }; + set({ tabs: [...tabs, tab], activeId: id }); + return tab; + }, + close: (id) => { + const { tabs, activeId } = get(); + const index = tabs.findIndex((tab) => tab.id === id); + const active = tabs.find((tab) => tab.id === activeId) ?? null; + if (index < 0) { + return active; + } + const remaining = tabs.filter((tab) => tab.id !== id); + const next = + id === activeId + ? (remaining[index] ?? remaining[remaining.length - 1] ?? null) + : (remaining.find((tab) => tab.id === activeId) ?? null); + set({ tabs: remaining, activeId: next?.id ?? null }); + return next; + }, +})); + +/** Empties the strip, for a test that must not inherit the last one's tabs. */ +export function resetPullRequestTabsForTests(): void { + usePullRequestTabsStore.setState({ tabs: [], activeId: null }); +} diff --git a/apps/web/src/components/pull-requests/pullRequests.logic.test.ts b/apps/web/src/components/pull-requests/pullRequests.logic.test.ts index 44616aed..30591848 100644 --- a/apps/web/src/components/pull-requests/pullRequests.logic.test.ts +++ b/apps/web/src/components/pull-requests/pullRequests.logic.test.ts @@ -18,6 +18,7 @@ import { buildReviewCommentHandoff, countNeedsYou, formatPullRequestBaseFreshness, + formatPullRequestChecksHeadline, formatPullRequestChecksSummary, groupPullRequests, groupTimelineRows, @@ -719,6 +720,34 @@ describe("summarizePullRequestChecks", () => { expect(summary.state).toBe("none"); expect(formatPullRequestChecksSummary(summary)).toBe("No checks reported."); }); + + it("reads the rollup as one phrase for the header", () => { + const headline = (statuses: readonly ("pending" | "success" | "failure" | "skipped")[]) => + formatPullRequestChecksHeadline( + summarizePullRequestChecks( + statuses.map((status, index) => check(`check-${index}`, status)), + ), + ); + const times = (count: number, value: Value) => + Array.from({ length: count }, () => value); + + expect(headline([])).toBe("No checks reported"); + expect(headline(times(16, "success" as const))).toBe("All checks passed"); + // Skipped checks count towards the total but are never what it is about. + expect(headline([...times(13, "success" as const), ...times(3, "skipped" as const)])).toBe( + "13 of 16 passing", + ); + expect( + headline([ + ...times(3, "failure" as const), + ...times(4, "pending" as const), + ...times(9, "success" as const), + ]), + ).toBe("3 of 16 failing"); + expect(headline([...times(9, "pending" as const), ...times(2, "success" as const)])).toBe( + "9 of 11 running", + ); + }); }); describe("resolvePullRequestMergeBlock", () => { diff --git a/apps/web/src/components/pull-requests/pullRequests.logic.ts b/apps/web/src/components/pull-requests/pullRequests.logic.ts index 54f03358..9bdb663f 100644 --- a/apps/web/src/components/pull-requests/pullRequests.logic.ts +++ b/apps/web/src/components/pull-requests/pullRequests.logic.ts @@ -1185,6 +1185,27 @@ export function formatPullRequestChecksSummary(summary: PullRequestChecksSummary return parts.length === 0 ? "No checks reported." : parts.join(", "); } +/** + * The check rollup in one phrase, for the header's tab strip: what is wrong if + * anything is, and otherwise how far along the run is. Failures outrank running + * the way the glyph beside it does, and skipped checks count towards the total + * without ever being the thing the phrase is about. + */ +export function formatPullRequestChecksHeadline(summary: PullRequestChecksSummary): string { + if (summary.total === 0) { + return "No checks reported"; + } + if (summary.failing > 0) { + return `${summary.failing} of ${summary.total} failing`; + } + if (summary.pending > 0) { + return `${summary.pending} of ${summary.total} running`; + } + return summary.passing === summary.total + ? "All checks passed" + : `${summary.passing} of ${summary.total} passing`; +} + /** How each merge method reads in a menu item and in the confirm dialog. */ export const PULL_REQUEST_MERGE_METHOD_LABELS: Readonly> = { merge: "Create a merge commit", @@ -1644,6 +1665,22 @@ export function formatPullRequestBaseFreshness( return `Behind ${detail.baseBranch} by ${detail.behindBy} ${detail.behindBy === 1 ? "commit" : "commits"}`; } +/** + * The same fact as {@link formatPullRequestBaseFreshness}, short enough for the + * branch line to carry it after the head branch. The line already names the + * base, so this says only how far behind it the branch is. + */ +export function formatPullRequestBehindLabel( + detail: Pick, +): string | null { + if (detail.baseComparison !== "behind") { + return null; + } + return detail.behindBy === null || detail.behindBy <= 0 + ? "behind" + : `behind by ${detail.behindBy}`; +} + /** How each way of bringing a branch up to date reads in the update menu. */ export function pullRequestUpdateMethodLabel( method: PullRequestUpdateMethod, diff --git a/apps/web/src/components/ui/page-tabs.tsx b/apps/web/src/components/ui/page-tabs.tsx index deb714bb..bca5333a 100644 --- a/apps/web/src/components/ui/page-tabs.tsx +++ b/apps/web/src/components/ui/page-tabs.tsx @@ -62,8 +62,12 @@ export function PageTabButton({ ); } -/** Arrow keys walk the enclosing tablist and select the tab they land on. */ -function moveBetweenTabs(event: KeyboardEvent) { +/** + * Arrow keys walk the enclosing tablist and select the tab they land on. + * Exported for strips that draw their own tabs (the pull requests page's, which + * carries a close control per tab) but want the same keyboard behaviour. + */ +export function moveBetweenTabs(event: KeyboardEvent) { const tablist = event.currentTarget.closest('[role="tablist"]'); if (!tablist) return; const tabs = [...tablist.querySelectorAll('[role="tab"]')]; From 2078a4f7b4b3da193763fc54a1861ba41cc43c2a Mon Sep 17 00:00:00 2001 From: badcuban <108198679+badcuban@users.noreply.github.com> Date: Thu, 3 Sep 2026 06:03:17 -0400 Subject: [PATCH 05/13] fix(web): pull request rows drop pills and repo when the list is narrow The rule that trimmed rows on phones now reads the list's own width, so the column beside an open pull request gets the same treatment. --- .../src/components/pull-requests/PullRequestsView.tsx | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/apps/web/src/components/pull-requests/PullRequestsView.tsx b/apps/web/src/components/pull-requests/PullRequestsView.tsx index fe7ee2b1..f8154d39 100644 --- a/apps/web/src/components/pull-requests/PullRequestsView.tsx +++ b/apps/web/src/components/pull-requests/PullRequestsView.tsx @@ -580,7 +580,10 @@ export function PullRequestsView({ >
@@ -924,7 +927,8 @@ function PullRequestRow({ key: string; fit: "whole" | "truncate"; className?: string; - /** Dropped below md, separator and all, where the line has no room for it. */ + /** 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; }[] = [ @@ -1067,7 +1071,7 @@ function PullRequestRow({ className={cn( "flex items-center gap-1.5", item.fit === "whole" ? "shrink-0" : "min-w-0", - item.hideOnPhone && "max-md:hidden", + item.hideOnPhone && "@max-lg:hidden", )} > {index > 0 ? · : null} From 4f947570ef0d0289aab95ff97dae9d475727fd97 Mon Sep 17 00:00:00 2001 From: badcuban <108198679+badcuban@users.noreply.github.com> Date: Thu, 3 Sep 2026 06:36:40 -0400 Subject: [PATCH 06/13] fix(web): pull request review fixes for selection identity, tabs, menus, and phone rows The selection carries the repository so same-number pull requests on different repositories stay apart; tab selection keeps focus in the strip, tabs keep their last known state and scroll into view, close buttons leave the tab order when hidden; the Author and Project submenus are radio groups with their search field focused on open; the phone branch row wraps the files and diff stat onto their own line; row and tab names carry the state, conflict and repository for screen readers; copy buttons announce the copy. --- .../PullRequestDetailPanel.browser.tsx | 26 ++- .../pull-requests/PullRequestDetailPanel.tsx | 21 ++- .../pull-requests/PullRequestFilters.tsx | 96 +++++------ .../pull-requests/PullRequestSummaryTab.tsx | 28 +++- .../pull-requests/PullRequestTabStrip.tsx | 74 ++++++--- .../PullRequestsView.browser.tsx | 56 ++++++- .../pull-requests/PullRequestsView.tsx | 157 ++++++++++-------- .../pullRequestTabsStore.test.ts | 13 ++ .../pull-requests/pullRequestTabsStore.ts | 51 +++++- .../pull-requests/pullRequests.logic.test.ts | 83 ++++++++- .../pull-requests/pullRequests.logic.ts | 112 +++++++++++-- 11 files changed, 541 insertions(+), 176 deletions(-) diff --git a/apps/web/src/components/pull-requests/PullRequestDetailPanel.browser.tsx b/apps/web/src/components/pull-requests/PullRequestDetailPanel.browser.tsx index 2ffe5ee5..5557dc38 100644 --- a/apps/web/src/components/pull-requests/PullRequestDetailPanel.browser.tsx +++ b/apps/web/src/components/pull-requests/PullRequestDetailPanel.browser.tsx @@ -176,6 +176,8 @@ async function renderPanel( options: { readonly detail?: Partial; readonly activity?: Partial; + /** Makes the conversation read fail, which the meta rows answer for. */ + readonly activityFails?: boolean; readonly composerTarget?: ScopedThreadRef; } = {}, ) { @@ -187,7 +189,12 @@ async function renderPanel( __setEnvironmentApiOverrideForTests(ENVIRONMENT_ID, { pullRequests: { detail: vi.fn(async () => ({ ...DETAIL, ...options.detail })), - activity: vi.fn(async () => ({ ...ACTIVITY, ...options.activity })), + activity: vi.fn(async () => { + if (options.activityFails) { + throw new Error("the host said no"); + } + return { ...ACTIVITY, ...options.activity }; + }), diff: vi.fn(async () => ({ patch: PATCH, truncated: false })), comment, runAction, @@ -625,6 +632,11 @@ describe("PullRequestDetailPanel", () => { await vi.waitFor(() => { expect(writeText).toHaveBeenCalledWith("gh pr checkout 42"); }); + // "Copied" is the only sign it worked, so the button's name says it too + // rather than staying "Copy" for anyone who cannot see the word. + await expect + .element(page.getByRole("button", { name: "Copied gh pr checkout 42" })) + .toBeVisible(); } finally { if (previous) { Object.defineProperty(navigator, "clipboard", previous); @@ -633,6 +645,18 @@ describe("PullRequestDetailPanel", () => { } }); + it("says why the comment count is missing rather than offering a button with none", async () => { + const rendered = await renderPanel({ activityFails: true }); + + await expect + .element(page.getByTestId("pull-request-comment-count")) + .toHaveTextContent("Comments unavailable"); + // Nothing to scroll to, so nothing to press. + expect(page.getByRole("button", { name: "Comments unavailable" }).elements()).toHaveLength(0); + + await rendered.cleanup(); + }); + it("folds the description away and back", async () => { const rendered = await renderPanel(); diff --git a/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx b/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx index 254833a5..4e84aec0 100644 --- a/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx @@ -677,8 +677,10 @@ function PullRequestDetailHeader({
- {/* Row 4: the branches this joins, and how much it changes. */} -
+ {/* 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 ? ( @@ -714,7 +716,7 @@ function PullRequestDetailHeader({ ) : null} - + {pluralize(detail.changedFiles, "file")} @@ -871,7 +873,9 @@ function PullRequestCheckoutCommand({ + {/* 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"} + + ) : ( + + )}
diff --git a/apps/web/src/components/pull-requests/PullRequestTabStrip.tsx b/apps/web/src/components/pull-requests/PullRequestTabStrip.tsx index 21f0a9c3..28170662 100644 --- a/apps/web/src/components/pull-requests/PullRequestTabStrip.tsx +++ b/apps/web/src/components/pull-requests/PullRequestTabStrip.tsx @@ -8,8 +8,8 @@ * * There is no `+`: the list beside the column is how a pull request is opened. */ -import type { PullRequestState } from "@threadlines/contracts"; 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"; @@ -18,12 +18,6 @@ import { TooltipWrapper } from "../ui/tooltip"; import { pullRequestBadgeTone } from "./pullRequests.logic"; import type { PullRequestTab } from "./pullRequestTabsStore"; -/** A tab, plus what the list knows about the pull request it stands for. */ -export interface PullRequestTabView extends PullRequestTab { - readonly state: PullRequestState; - readonly isDraft: boolean; -} - /** 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"; @@ -39,13 +33,37 @@ export function PullRequestTabStrip({ onSelect, onClose, }: { - readonly tabs: readonly PullRequestTabView[]; + 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: PullRequestTabView) => void; - readonly onClose: (tab: PullRequestTabView) => void; + 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; } @@ -63,6 +81,7 @@ export function PullRequestTabStrip({ className={cn("min-w-0 flex-1 self-stretch", MINI_HORIZONTAL_SCROLLBAR_CLASS)} >
onSelect(tab)} - onClose={() => onClose(tab)} + onClose={(fromKeyboard) => { + focusActiveTab.current = fromKeyboard; + onClose(tab); + }} /> ))}
@@ -91,11 +113,12 @@ function PullRequestTabStripItem({ onSelect, onClose, }: { - readonly tab: PullRequestTabView; + readonly tab: PullRequestTab; readonly active: boolean; readonly panelId: string; readonly onSelect: () => void; - readonly onClose: () => 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 ( @@ -120,7 +143,7 @@ function PullRequestTabStripItem({ onAuxClick={(event) => { if (event.button !== 1) return; event.preventDefault(); - onClose(); + onClose(false); }} onMouseDown={(event) => { // Suppress the middle-click autoscroll cursor; the close itself @@ -142,24 +165,37 @@ function PullRequestTabStripItem({ - #{tab.number} - {tone.label} + {/* The strip has room for the number alone, but two repositories can + hold the same one, so the name a reader hears carries both. */} + + #{tab.number} + + {`${tab.repository} #${tab.number} ${tone.label}`} {/* 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 ad3ccfc8..281cea91 100644 --- a/apps/web/src/components/pull-requests/PullRequestsView.browser.tsx +++ b/apps/web/src/components/pull-requests/PullRequestsView.browser.tsx @@ -398,8 +398,9 @@ describe("PullRequestsView", () => { 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. - await userEvent.click(page.getByRole("button", { name: "Close pull request #2" })); + // 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); @@ -410,6 +411,49 @@ describe("PullRequestsView", () => { 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 { @@ -460,10 +504,14 @@ describe("PullRequestsView", () => { // 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("menuitem", { name: "grace", exact: true }).elements()).toHaveLength(0); + expect( + page.getByRole("menuitemradio", { name: "grace", exact: true }).elements(), + ).toHaveLength(0); }); - await userEvent.click(page.getByRole("menuitem", { name: "ada", exact: true })); + 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. diff --git a/apps/web/src/components/pull-requests/PullRequestsView.tsx b/apps/web/src/components/pull-requests/PullRequestsView.tsx index f8154d39..66438f29 100644 --- a/apps/web/src/components/pull-requests/PullRequestsView.tsx +++ b/apps/web/src/components/pull-requests/PullRequestsView.tsx @@ -23,6 +23,7 @@ import { openExternalUrl } from "../../lib/externalLinks"; import { PULL_REQUEST_PAGE_REFETCH_INTERVAL_MS, refreshPullRequestList, + useLoadedPullRequestEntries, usePullRequestLists, type PullRequestEnvironmentFailure, } from "../../lib/pullRequestsReactQuery"; @@ -51,23 +52,28 @@ import { PullRequestFiltersButton, PullRequestSortMenu, } from "./PullRequestFilters"; -import { PullRequestTabStrip, type PullRequestTabView } from "./PullRequestTabStrip"; +import { PullRequestTabStrip, pullRequestTabButtonId } from "./PullRequestTabStrip"; import { pullRequestTabId, usePullRequestTabsStore, type PullRequestTab, + type PullRequestTabStatus, + type PullRequestTabTarget, } from "./pullRequestTabsStore"; import { PullRequestActorAvatar, PullRequestChecksGlyph, PullRequestLabelPill, + pullRequestChecksTone, pullRequestHostName, } from "./pullRequestPresentation"; import { + formatPullRequestSelection, groupPullRequests, hasPullRequestProject, linkThreadsToPullRequests, matchesPullRequestQuery, + matchesPullRequestSelection, narrowPullRequests, projectRepository, pullRequestBadgeTone, @@ -202,6 +208,7 @@ export function PullRequestsView({ 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, @@ -294,40 +301,36 @@ 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 opened, from a row or from a tab. The repository rides - // along because the URL carries only the project, and the panel addresses a - // pull request by repository as well as by number. + // 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: { - readonly environmentId: EnvironmentId; - readonly projectId: ProjectId; - readonly repository: string; - readonly number: number; - }) => { - setPressedRow({ - key: `${target.environmentId}:${target.projectId}:${target.number}`, - repository: target.repository, - }); + (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) => { openTab({ @@ -335,7 +338,10 @@ export function PullRequestsView({ projectId: entry.projectId, repository: entry.repository, number: entry.number, + state: entry.state, + isDraft: entry.isDraft, }); + setPressedKey(formatPullRequestSelection(entry)); showPullRequest(entry); }, [openTab, showPullRequest], @@ -347,7 +353,7 @@ export function PullRequestsView({ rowToRefocus.current = document.querySelector( '[data-testid="pull-requests-row"][aria-current="true"]', ); - setPressedRow(null); + setPressedKey(null); onSelectionChange(null); }, [onSelectionChange]); @@ -365,20 +371,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( @@ -386,13 +387,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; @@ -414,21 +415,24 @@ export function PullRequestsView({ }); }, [openTab, selectedReference, selection]); - // Each tab wears the state of the row it stands for, where the listing still - // carries that row; one it has dropped keeps the open glyph rather than - // vanishing from the strip. - const tabViews = useMemo( - () => - tabs.map((tab) => { - const entry = snapshot.entries.find( - (candidate) => - candidate.environmentId === tab.environmentId && - candidate.projectId === tab.projectId && - candidate.number === tab.number, - ); - return { ...tab, state: entry?.state ?? "open", isDraft: entry?.isDraft ?? false }; - }), - [snapshot.entries, tabs], + // 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 @@ -449,12 +453,12 @@ export function PullRequestsView({ // its full width back. if (!wasShowing) return; if (next) { - showPullRequest(next); + handleSelectTab(next); return; } closeSelection(); }, - [activeTabId, closeSelection, closeTab, showPullRequest], + [activeTabId, closeSelection, closeTab, handleSelectTab], ); // Escape steps back to the list, but only when nothing else owns the key: a @@ -545,12 +549,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} @@ -621,8 +620,11 @@ export function PullRequestsView({ className="min-w-0 flex-1" size="sm" type="search" - aria-label="Search pull requests" - placeholder="Search title, #number, author, branch, label" + // The field is narrow on a phone, so the placeholder says + // what it is rather than everything it reads; the label + // spells the rest out for anyone who cannot see it. + aria-label="Search pull requests by title, number, author, branch or label" + placeholder="Search pull requests" spellCheck={false} value={query} onChange={(event) => setQuery(event.target.value)} @@ -687,17 +689,22 @@ export function PullRequestsView({ tabs={tabViews} activeId={activeTabId} panelId={DETAIL_PANEL_ID} - onSelect={showPullRequest} + onSelect={handleSelectTab} onClose={handleCloseTab} /> -
+
onSelect(entry)} />
diff --git a/apps/web/src/components/pull-requests/pullRequestTabsStore.test.ts b/apps/web/src/components/pull-requests/pullRequestTabsStore.test.ts index 033a501a..2f646f5a 100644 --- a/apps/web/src/components/pull-requests/pullRequestTabsStore.test.ts +++ b/apps/web/src/components/pull-requests/pullRequestTabsStore.test.ts @@ -57,6 +57,19 @@ describe("pullRequestTabsStore", () => { expect(store().activeId).toBeNull(); }); + it("keeps the last state a listing showed once the row leaves it", () => { + const tab = store().open(target(1)); + expect(tab.state).toBe("open"); + + store().markStatus(new Map([[tab.id, { state: "merged", isDraft: false }]])); + expect(store().tabs[0]?.state).toBe("merged"); + + // No listing on screen carries the row any more, so the glyph it was last + // seen with stands rather than falling back to open. + store().markStatus(new Map()); + expect(store().tabs[0]?.state).toBe("merged"); + }); + it("leaves the active tab alone when a background one is closed", () => { const first = store().open(target(1)); store().open(target(2)); diff --git a/apps/web/src/components/pull-requests/pullRequestTabsStore.ts b/apps/web/src/components/pull-requests/pullRequestTabsStore.ts index 61cdd568..9f9f1df7 100644 --- a/apps/web/src/components/pull-requests/pullRequestTabsStore.ts +++ b/apps/web/src/components/pull-requests/pullRequestTabsStore.ts @@ -6,10 +6,16 @@ * persisted: tabs are a working set for one sitting, and a reload that restored * six of them would be restoring someone else's afternoon. */ -import type { EnvironmentId, ProjectId } from "@threadlines/contracts"; +import type { EnvironmentId, ProjectId, PullRequestState } from "@threadlines/contracts"; import { create } from "zustand"; -export interface PullRequestTab { +/** What the strip draws a tab's glyph from, as the listings last said it. */ +export interface PullRequestTabStatus { + readonly state: PullRequestState; + readonly isDraft: boolean; +} + +export interface PullRequestTab extends PullRequestTabStatus { /** `environment:project:repository:number`, from {@link pullRequestTabId}. */ readonly id: string; readonly environmentId: EnvironmentId; @@ -18,7 +24,13 @@ export interface PullRequestTab { readonly number: number; } -export type PullRequestTabTarget = Omit; +/** + * A tab's identity, plus what was known about it when it was opened. The status + * is optional because the route can open a tab for a row no listing on screen + * carries; it then rests on open until a listing says otherwise. + */ +export type PullRequestTabTarget = Omit & + Partial; /** * One pull request across environments and checkouts. The repository is in the @@ -40,6 +52,12 @@ interface PullRequestTabsState { * else null. Closing a background tab leaves the active one alone. */ readonly close: (id: string) => PullRequestTab | null; + /** + * Records what the listings now say about the tabs they still carry, so a row + * that leaves a listing (merged, closed, filtered out of the read) keeps the + * glyph it was last seen with instead of falling back to open. + */ + readonly markStatus: (statusById: ReadonlyMap) => void; } export const usePullRequestTabsStore = create((set, get) => ({ @@ -55,7 +73,15 @@ export const usePullRequestTabsStore = create((set, get) = } return existing; } - const tab: PullRequestTab = { id, ...target }; + const tab: PullRequestTab = { + id, + environmentId: target.environmentId, + projectId: target.projectId, + repository: target.repository, + number: target.number, + state: target.state ?? "open", + isDraft: target.isDraft ?? false, + }; set({ tabs: [...tabs, tab], activeId: id }); return tab; }, @@ -74,6 +100,23 @@ export const usePullRequestTabsStore = create((set, get) = set({ tabs: remaining, activeId: next?.id ?? null }); return next; }, + markStatus: (statusById) => { + const { tabs } = get(); + let changed = false; + const next = tabs.map((tab) => { + const status = statusById.get(tab.id); + if (!status || (status.state === tab.state && status.isDraft === tab.isDraft)) { + return tab; + } + changed = true; + return { ...tab, state: status.state, isDraft: status.isDraft }; + }); + // Only when something moved: this runs off every listing read, and a fresh + // array each time would re-render the strip on every poll. + if (changed) { + set({ tabs: next }); + } + }, })); /** Empties the strip, for a test that must not inherit the last one's tabs. */ diff --git a/apps/web/src/components/pull-requests/pullRequests.logic.test.ts b/apps/web/src/components/pull-requests/pullRequests.logic.test.ts index 30591848..70e53ba4 100644 --- a/apps/web/src/components/pull-requests/pullRequests.logic.test.ts +++ b/apps/web/src/components/pull-requests/pullRequests.logic.test.ts @@ -18,16 +18,20 @@ import { buildReviewCommentHandoff, countNeedsYou, formatPullRequestBaseFreshness, + formatPullRequestSelection, formatPullRequestChecksHeadline, formatPullRequestChecksSummary, groupPullRequests, groupTimelineRows, linkThreadsToPullRequests, matchesPullRequestQuery, + matchesPullRequestSelection, narrowPullRequests, + parsePullRequestSelection, parsePullRequestsSearch, pullRequestEntryKey, pullRequestFilterChips, + pullRequestProjectFacets, pullRequestFiltersFromSearch, pullRequestFiltersToSearch, pullRequestLabelColor, @@ -162,7 +166,7 @@ describe("resolveNeedsYouReason", () => { checksState: "failure", }), ), - ).toBe("Review requested"); + ).toBe("Review required"); }); it("reports changes before failing checks for the author's own row", () => { @@ -535,6 +539,28 @@ describe("narrowPullRequests", () => { expect(involved({ project: `${ENVIRONMENT_ID}:${OTHER_PROJECT_ID}` })).toEqual([3]); }); + it("leaves a row authored outside the workspace out of every project", () => { + const authored = entry({ + number: 4, + origin: "authored", + repository: "someone/else", + projectId: OTHER_PROJECT_ID, + }); + const mixed = [entry({ number: 3, projectId: OTHER_PROJECT_ID }), authored]; + + // The project on an authored row is only the checkout whose host answered + // the search, so it neither offers that project nor hides behind it. + expect(pullRequestProjectFacets(mixed).map((facet) => facet.key)).toEqual([ + `${ENVIRONMENT_ID}:${OTHER_PROJECT_ID}`, + ]); + expect( + narrowPullRequests(mixed, { + ...EMPTY_PULL_REQUEST_FILTERS, + project: `${ENVIRONMENT_ID}:${OTHER_PROJECT_ID}`, + }).map((row) => row.number), + ).toEqual([3]); + }); + it("narrows on everything at once", () => { expect(numbersFor({ author: "ada", labels: "bug", checks: "passing" })).toEqual([1]); expect(numbersFor({ author: "ada", labels: "bug", checks: "failing" })).toEqual([]); @@ -670,6 +696,61 @@ describe("the filter chips and the route's params", () => { }); }); +describe("the pull request a link names", () => { + it("carries the repository through a round trip, separators and all", () => { + const selection = { + // Both the environment id and the repository hold the separator the + // param is spelled with, which is what the encoding is there for. + environmentId: EnvironmentId.make("environment:remote"), + projectId: PROJECT_ID, + repository: "group/sub:group/threadlines", + number: 214, + }; + + const written = formatPullRequestSelection(selection); + expect(written).toBe(`environment:remote:${PROJECT_ID}:214:group%2Fsub%3Agroup%2Fthreadlines`); + expect(parsePullRequestSelection(written)).toEqual(selection); + }); + + it("still reads a link written before the param carried a repository", () => { + expect(parsePullRequestSelection(`${ENVIRONMENT_ID}:${PROJECT_ID}:7`)).toEqual({ + environmentId: ENVIRONMENT_ID, + projectId: PROJECT_ID, + repository: null, + number: 7, + }); + // With no repository the row is matched on what the link does carry. + expect( + matchesPullRequestSelection( + { environmentId: ENVIRONMENT_ID, projectId: PROJECT_ID, repository: null, number: 7 }, + entry({ number: 7 }), + ), + ).toBe(true); + }); + + it("tells two pull requests with one number apart by their repository", () => { + const selection = { + environmentId: ENVIRONMENT_ID, + projectId: PROJECT_ID, + repository: "Threadlines/Threadlines", + number: 7, + }; + + // Repository names are case-insensitive on every host here. + expect(matchesPullRequestSelection(selection, entry({ number: 7 }))).toBe(true); + expect( + matchesPullRequestSelection(selection, entry({ number: 7, repository: "someone/else" })), + ).toBe(false); + }); + + it("drops a value that names no pull request", () => { + expect(parsePullRequestSelection("")).toBeNull(); + expect(parsePullRequestSelection(`${ENVIRONMENT_ID}:${PROJECT_ID}`)).toBeNull(); + expect(parsePullRequestSelection(`${ENVIRONMENT_ID}:${PROJECT_ID}:0`)).toBeNull(); + expect(parsePullRequestSelection(`${ENVIRONMENT_ID}:${PROJECT_ID}:none:repo`)).toBeNull(); + }); +}); + describe("pullRequestLabelColor", () => { it("takes a hex triplet with or without its hash and refuses anything else", () => { expect(pullRequestLabelColor("d73a4a")).toBe("#d73a4a"); diff --git a/apps/web/src/components/pull-requests/pullRequests.logic.ts b/apps/web/src/components/pull-requests/pullRequests.logic.ts index 9bdb663f..3d1ca9dc 100644 --- a/apps/web/src/components/pull-requests/pullRequests.logic.ts +++ b/apps/web/src/components/pull-requests/pullRequests.logic.ts @@ -51,7 +51,7 @@ export type PullRequestProjectFailure = PullRequestListProjectError & { /** The reasons a row is put in front of the user, in priority order. */ export type PullRequestNeedsYouReason = - | "Review requested" + | "Review required" | "Changes requested" | "Checks failing" | "Approved"; @@ -69,6 +69,12 @@ export interface PullRequestGroup { export interface PullRequestSelection { readonly environmentId: EnvironmentId; readonly projectId: ProjectId; + /** + * The repository the number belongs to: one project reads more than one + * remote, and #214 is a different pull request on each. Null only for a link + * written before the param carried it. + */ + readonly repository: string | null; readonly number: number; } @@ -118,7 +124,10 @@ export const DEFAULT_PULL_REQUEST_SORT: PullRequestSort = "updated"; export interface PullRequestsSearch { readonly state: PullRequestListState; - /** `::`, absent when the list is alone. */ + /** + * `:::`, absent when the list + * is alone; see {@link formatPullRequestSelection}. + */ readonly pr?: string; readonly author?: string; readonly labels?: string; @@ -221,28 +230,48 @@ export function pullRequestFiltersToSearch( }; } +/** A whole positive number and nothing else, which is what a segment must be. */ +const DIGITS = /^\d+$/; + +/** + * `environment:project:number:repository`, the repository last and + * percent-encoded so the `/` and `:` in a name cannot be read as separators. + * A link written before the param carried a repository ends in its number + * instead, which is how {@link parsePullRequestSelection} tells the two apart. + */ export function formatPullRequestSelection(selection: PullRequestSelection): string { - return `${selection.environmentId}:${selection.projectId}:${selection.number}`; + const base = `${selection.environmentId}:${selection.projectId}:${selection.number}`; + return selection.repository === null + ? base + : `${base}:${encodeURIComponent(selection.repository)}`; } /** * Read from the right, so an environment id carrying a colon of its own still - * parses. A value that does not resolve to a positive number is dropped rather - * than rendered as a broken selection. + * parses, and the three-part form old links carry is still read (with no + * repository, which the page then falls back to the project's remote for). A + * value that does not resolve to a positive number is dropped rather than + * rendered as a broken selection. */ export function parsePullRequestSelection(value: string): PullRequestSelection | null { - const lastSeparator = value.lastIndexOf(":"); - if (lastSeparator <= 0) { + const parts = value.split(":"); + const last = parts[parts.length - 1] ?? ""; + // The last segment is the number in the old form and the repository in the + // new one, and only one of the two is ever all digits. + const carriesRepository = !DIGITS.test(last); + const repository = carriesRepository ? decodeRepositorySegment(last) : null; + if (carriesRepository && repository === null) { return null; } - const projectSeparator = value.lastIndexOf(":", lastSeparator - 1); - if (projectSeparator <= 0) { + const numberIndex = parts.length - (carriesRepository ? 2 : 1); + if (numberIndex < 2) { return null; } - const environmentId = value.slice(0, projectSeparator); - const projectId = value.slice(projectSeparator + 1, lastSeparator); - const number = Number(value.slice(lastSeparator + 1)); - if (environmentId.length === 0 || projectId.length === 0) { + const numberPart = parts[numberIndex] ?? ""; + const projectId = parts[numberIndex - 1] ?? ""; + const environmentId = parts.slice(0, numberIndex - 1).join(":"); + const number = Number(numberPart); + if (environmentId.length === 0 || projectId.length === 0 || !DIGITS.test(numberPart)) { return null; } if (!Number.isSafeInteger(number) || number <= 0) { @@ -251,10 +280,47 @@ export function parsePullRequestSelection(value: string): PullRequestSelection | return { environmentId: EnvironmentId.make(environmentId), projectId: ProjectId.make(projectId), + repository, number, }; } +/** The repository the param spells, or null when it spells nothing readable. */ +function decodeRepositorySegment(value: string): string | null { + if (value.length === 0) { + return null; + } + try { + const decoded = decodeURIComponent(value); + return decoded.length === 0 ? null : decoded; + } catch { + return null; + } +} + +/** + * Whether a row is the one the route names. A link written before the param + * carried a repository names none, and then the project and the number are all + * there is to go on. + */ +export function matchesPullRequestSelection( + selection: PullRequestSelection, + entry: { + readonly environmentId: EnvironmentId; + readonly projectId: ProjectId; + readonly repository: string; + readonly number: number; + }, +): boolean { + return ( + selection.environmentId === entry.environmentId && + selection.projectId === entry.projectId && + selection.number === entry.number && + (selection.repository === null || + repositoryKey(selection.repository) === repositoryKey(entry.repository)) + ); +} + /** * Identifies one row across environments and repositories. The project is * deliberately left out: a checkout and its worktrees are separate projects on @@ -563,7 +629,7 @@ export function resolveNeedsYouReason(entry: PullRequestEntry): PullRequestNeeds return null; } if (entry.viewerReviewRequested) { - return "Review requested"; + return "Review required"; } if (!entry.viewerIsAuthor) { return null; @@ -705,7 +771,13 @@ export function narrowPullRequests( return false; } } - if (project.length > 0 && pullRequestProjectKey(entry) !== project) return false; + // An authored row belongs to no project here, so naming one hides it. + if ( + project.length > 0 && + (entry.origin === "authored" || pullRequestProjectKey(entry) !== project) + ) { + return false; + } if (filters.draft === "only" && !entry.isDraft) return false; if (filters.draft === "hide" && entry.isDraft) return false; if (filters.review === "none" && entry.reviewDecision !== undefined) return false; @@ -994,12 +1066,20 @@ export function pullRequestProjectKey(entry: { return `${entry.environmentId}:${entry.projectId}`; } -/** The projects the loaded rows came from, alphabetically. */ +/** + * The projects the loaded rows came from, alphabetically. A pull request the + * user authored somewhere outside the workspace is left out: the project on it + * is only the checkout whose host answered the search, so filing it under that + * project would put strangers' repositories inside it. + */ export function pullRequestProjectFacets( entries: readonly PullRequestEntry[], ): readonly PullRequestProjectFacet[] { const byKey = new Map(); for (const entry of entries) { + if (entry.origin === "authored") { + continue; + } const key = pullRequestProjectKey(entry); const seen = byKey.get(key); if (seen) { From 17ba6a90c114347a146b1768731b99d6463fc545 Mon Sep 17 00:00:00 2001 From: badcuban <108198679+badcuban@users.noreply.github.com> Date: Thu, 3 Sep 2026 11:55:06 -0400 Subject: [PATCH 07/13] feat(web): needs-you asks for write access, and review state is a glyph A pull request of the viewer's own counts as needing them only when they can push to its repository, so contributions to repositories they cannot merge stay under Yours. The list carries the viewer's write access per row: from the cached repository access read for workspace rows and from the search's viewerPermission for authored rows. Rows show the review state as a glyph beside the checks glyph instead of a coloured word. --- .../src/pullRequest/PullRequestProvider.ts | 7 + .../pullRequest/PullRequestService.test.ts | 66 +++++++ .../src/pullRequest/PullRequestService.ts | 176 ++++++++++++------ .../gitHubPullRequestGraphql.test.ts | 28 +++ .../pullRequest/gitHubPullRequestGraphql.ts | 42 ++++- .../PullRequestsView.browser.tsx | 8 +- .../pull-requests/PullRequestsView.tsx | 41 ++-- .../pull-requests/pullRequestPresentation.tsx | 81 ++++++++ .../pull-requests/pullRequests.logic.test.ts | 38 ++++ .../pull-requests/pullRequests.logic.ts | 9 +- packages/contracts/src/pullRequest.ts | 6 + 11 files changed, 420 insertions(+), 82 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestProvider.ts b/apps/server/src/pullRequest/PullRequestProvider.ts index 7b9fc034..f1f8bdb8 100644 --- a/apps/server/src/pullRequest/PullRequestProvider.ts +++ b/apps/server/src/pullRequest/PullRequestProvider.ts @@ -99,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 9ba7b972..2d535e81 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -328,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") { @@ -338,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" })]))); @@ -492,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([ diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 063d95f4..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,6 +393,7 @@ 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 @@ -397,6 +406,19 @@ function toEntry(input: { }; } +/** + * 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; @@ -607,6 +629,9 @@ export const make = Effect.fn("makePullRequestService")(function* () { viewer, origin: "authored", repository: row.repository, + ...(row.viewerCanWrite === undefined + ? {} + : { viewerCanWrite: row.viewerCanWrite }), }), ] : [], @@ -632,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 @@ -751,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/gitHubPullRequestGraphql.test.ts b/apps/server/src/pullRequest/gitHubPullRequestGraphql.test.ts index 1aba48cb..c0ea3c02 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestGraphql.test.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestGraphql.test.ts @@ -337,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 7dd5c98b..8eb08102 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestGraphql.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestGraphql.ts @@ -105,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) { @@ -127,7 +128,7 @@ export const AUTHORED_PULL_REQUESTS_GRAPHQL_QUERY = `query($q: String!, $first: mergeable reviewDecision author { login avatarUrl } - repository { nameWithOwner } + repository { nameWithOwner viewerPermission } labels(first: ${AUTHORED_CONNECTION_PAGE_SIZE}) { nodes { name color } } reviewRequests(first: ${AUTHORED_CONNECTION_PAGE_SIZE}) { nodes { requestedReviewer { ... on User { login } } } @@ -542,7 +543,12 @@ const RawAuthoredNodeSchema = Schema.Struct({ 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( @@ -615,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; + } } /** @@ -674,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); diff --git a/apps/web/src/components/pull-requests/PullRequestsView.browser.tsx b/apps/web/src/components/pull-requests/PullRequestsView.browser.tsx index 281cea91..1cc977f3 100644 --- a/apps/web/src/components/pull-requests/PullRequestsView.browser.tsx +++ b/apps/web/src/components/pull-requests/PullRequestsView.browser.tsx @@ -526,7 +526,7 @@ describe("PullRequestsView", () => { await rendered.cleanup(); }); - it("draws the conflict, the checks and an author the host gave no picture for", async () => { + 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: [ @@ -535,6 +535,7 @@ describe("PullRequestsView", () => { title: "Bump the runner", author: { login: "dependabot[bot]", isBot: true, avatarUrl: null }, mergeability: "conflicting", + reviewDecision: "approved", checksState: "failure", labels: [{ name: "dependencies", color: "0366d6" }], }), @@ -545,8 +546,9 @@ describe("PullRequestsView", () => { 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 checks are a glyph, and it carries the words the row no longer spends - // its meta line on. + // 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); diff --git a/apps/web/src/components/pull-requests/PullRequestsView.tsx b/apps/web/src/components/pull-requests/PullRequestsView.tsx index 66438f29..7540c747 100644 --- a/apps/web/src/components/pull-requests/PullRequestsView.tsx +++ b/apps/web/src/components/pull-requests/PullRequestsView.tsx @@ -64,8 +64,10 @@ import { PullRequestActorAvatar, PullRequestChecksGlyph, PullRequestLabelPill, + PullRequestReviewGlyph, pullRequestChecksTone, pullRequestHostName, + pullRequestReviewTone, } from "./pullRequestPresentation"; import { formatPullRequestSelection, @@ -82,7 +84,6 @@ import { pullRequestFilterChips, pullRequestProjectFacets, requiresHostSignIn, - resolveNeedsYouReason, resolvePullRequestListSpan, resolveSignInHost, type PullRequestEntry, @@ -931,7 +932,10 @@ function PullRequestRow({ ...(conflictLabel ? [lowerFirst(conflictLabel)] : []), ...(checksLabel ? [lowerFirst(checksLabel)] : []), ].join(", ")}: ${entry.title}`; - const reason = resolveNeedsYouReason(entry); + 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. @@ -990,21 +994,6 @@ function PullRequestRow({ }, ] : []), - // The checks say for themselves that they failed, in a glyph at the end of - // the line; the rest of the reasons are review words with no glyph. - ...(reason && reason !== "Checks failing" - ? [ - { - 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", - content: reason, - }, - ] - : []), ...(visibleLabels.length > 0 ? [ { @@ -1025,6 +1014,24 @@ function PullRequestRow({ }, ] : []), + // 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 ? [] : [ diff --git a/apps/web/src/components/pull-requests/pullRequestPresentation.tsx b/apps/web/src/components/pull-requests/pullRequestPresentation.tsx index c002548d..c5552aba 100644 --- a/apps/web/src/components/pull-requests/pullRequestPresentation.tsx +++ b/apps/web/src/components/pull-requests/pullRequestPresentation.tsx @@ -14,6 +14,7 @@ import { TooltipWrapper } from "../ui/tooltip"; import type { PullRequestActor, PullRequestChecksState, + PullRequestReviewDecision, PullRequestReviewerState, SourceControlProviderKind, } from "@threadlines/contracts"; @@ -25,6 +26,9 @@ import { CircleDotIcon, CircleXIcon, MinusIcon, + UserRoundCheckIcon, + UserRoundIcon, + UserRoundXIcon, XIcon, } from "lucide-react"; @@ -226,6 +230,83 @@ export function PullRequestChecksGlyph({ ); } +/** + * Where the reviewers stand, as a glyph: the same three words the Review filter + * uses, in the tones the checks glyph beside it already spends. + */ +const REVIEW_STATE_PRESENTATION = { + approved: { + label: "Approved", + Icon: UserRoundCheckIcon, + className: "text-emerald-600 dark:text-emerald-300/90", + }, + "changes-requested": { + label: "Changes requested", + Icon: UserRoundXIcon, + className: "text-amber-600/90 dark:text-amber-400/80", + }, + // Nobody has answered yet, which is a fact about the row rather than news: + // muted, the way the meta line around it is. + "review-required": { + label: "Review required", + Icon: UserRoundIcon, + className: "text-muted-foreground/70", + }, +} as const satisfies Record< + PullRequestReviewDecision, + { label: string; Icon: typeof UserRoundIcon; className: string } +>; + +/** + * How a row's reviews read, or null when there is nothing to say. A review the + * host is waiting on from the viewer is review required whatever else it + * reports, since that is the part the viewer can act on. + */ +export function pullRequestReviewTone(input: { + readonly decision: PullRequestReviewDecision | undefined; + readonly reviewRequested: boolean | undefined; +}): (typeof REVIEW_STATE_PRESENTATION)[PullRequestReviewDecision] | null { + if (input.reviewRequested === true) { + return REVIEW_STATE_PRESENTATION["review-required"]; + } + return input.decision === undefined ? null : REVIEW_STATE_PRESENTATION[input.decision]; +} + +/** + * Where a list row would otherwise spend its meta line on a coloured word. The + * word stays for anyone who cannot see the colour, and is a tooltip away for + * everyone else, exactly as {@link PullRequestChecksGlyph} does beside it. + */ +export function PullRequestReviewGlyph({ + decision, + reviewRequested, + className, +}: { + readonly decision: PullRequestReviewDecision | undefined; + /** Whether the host is waiting on the viewer's own review of this row. */ + readonly reviewRequested?: boolean; + readonly className?: string; +}) { + const presentation = pullRequestReviewTone({ decision, reviewRequested }); + if (presentation === null) { + return null; + } + return ( + + + + {presentation.label} + + + ); +} + /** The dot that separates two facts on a meta line. */ export function MetaSeparator() { return ( diff --git a/apps/web/src/components/pull-requests/pullRequests.logic.test.ts b/apps/web/src/components/pull-requests/pullRequests.logic.test.ts index 70e53ba4..90509e7e 100644 --- a/apps/web/src/components/pull-requests/pullRequests.logic.test.ts +++ b/apps/web/src/components/pull-requests/pullRequests.logic.test.ts @@ -192,6 +192,25 @@ describe("resolveNeedsYouReason", () => { ); }); + it("leaves the author's own row alone where they cannot push", () => { + // A contribution to someone else's repository: the approval is news, not + // something the author can act on, so it is not put in front of them. + expect( + resolveNeedsYouReason( + entry({ viewerIsAuthor: true, viewerCanWrite: false, reviewDecision: "approved" }), + ), + ).toBeNull(); + expect( + resolveNeedsYouReason( + entry({ viewerIsAuthor: true, viewerCanWrite: false, checksState: "failure" }), + ), + ).toBeNull(); + // Reviewing takes no rights over the repository at all. + expect( + resolveNeedsYouReason(entry({ viewerCanWrite: false, viewerReviewRequested: true })), + ).toBe("Review required"); + }); + it("says nothing about a merged or closed row", () => { expect( resolveNeedsYouReason(entry({ state: "merged", viewerReviewRequested: true })), @@ -268,6 +287,25 @@ describe("groupPullRequests", () => { expect(groups[0]?.label).toBeNull(); }); + it("files work on a repository the viewer cannot push to under Yours", () => { + const upstream = entry({ + number: 1, + viewerIsAuthor: true, + viewerCanWrite: false, + reviewDecision: "approved", + }); + const asked = entry({ number: 2, viewerCanWrite: false, viewerReviewRequested: true }); + + const groups = groupPullRequests({ entries: [upstream, asked], viewer: "ada", state: "open" }); + + expect(groups.map((group) => [group.label, group.entries.map((row) => row.number)])).toEqual([ + ["Needs you", [2]], + ["Yours", [1]], + ]); + // The sidebar count reads the same rows the page groups. + expect(countNeedsYou([upstream, asked])).toBe(1); + }); + it("counts only the rows that need the viewer", () => { expect( countNeedsYou([ diff --git a/apps/web/src/components/pull-requests/pullRequests.logic.ts b/apps/web/src/components/pull-requests/pullRequests.logic.ts index 3d1ca9dc..a3008fe0 100644 --- a/apps/web/src/components/pull-requests/pullRequests.logic.ts +++ b/apps/web/src/components/pull-requests/pullRequests.logic.ts @@ -623,6 +623,13 @@ function findThreadListEntry( /** * Why this row is waiting on the user, or null when it is only news. First * match wins so a row states one thing rather than a list of conditions. + * + * Reviewing needs nothing but an account, so a review request always counts. + * The author-side reasons are all things the user answers by merging or + * landing the work, so they count only where the user may push: a contribution + * to someone else's repository is news however it is going, and belongs under + * Yours rather than in front of them. A host that does not say whether they may + * push is taken at its word and left as it was. */ export function resolveNeedsYouReason(entry: PullRequestEntry): PullRequestNeedsYouReason | null { if (entry.state !== "open") { @@ -631,7 +638,7 @@ export function resolveNeedsYouReason(entry: PullRequestEntry): PullRequestNeeds if (entry.viewerReviewRequested) { return "Review required"; } - if (!entry.viewerIsAuthor) { + if (!entry.viewerIsAuthor || entry.viewerCanWrite === false) { return null; } if (entry.reviewDecision === "changes-requested") { diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index d92e2af1..7ba2f5ab 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -81,6 +81,12 @@ export const PullRequestListEntry = Schema.Struct({ updatedAt: IsoDateTime, viewerIsAuthor: Schema.Boolean, viewerReviewRequested: Schema.Boolean, + /** + * Push access on this row's repository: whether the viewer could merge it or + * update its branch at all. Absent where the host did not say, and a page + * reading it then keeps whatever it does for a host that never says. + */ + viewerCanWrite: Schema.optionalKey(Schema.Boolean), /** Absent when the host reports no decision. */ reviewDecision: Schema.optionalKey(PullRequestReviewDecision), /** Absent when there are no checks, or when checks were not requested. */ From 86c86d9abf8f0a83ca69ed1ef30ccb93362f1ea4 Mon Sep 17 00:00:00 2001 From: badcuban <108198679+badcuban@users.noreply.github.com> Date: Thu, 3 Sep 2026 11:56:26 -0400 Subject: [PATCH 08/13] docs: row review state is a glyph --- docs/design/pull-requests.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/design/pull-requests.md b/docs/design/pull-requests.md index edb8bc11..f33e2c66 100644 --- a/docs/design/pull-requests.md +++ b/docs/design/pull-requests.md @@ -1099,8 +1099,8 @@ text-muted-foreground`, dot `size-2 rounded-full` coloured from the label's hex - Check glyph, `size-3.5`, in place of the words "Checks failing": passing `CircleCheckIcon` emerald, failing `CircleXIcon` destructive, running `CircleDotIcon` amber, none = nothing. It is a tooltip trigger ("All checks passed" / "Some checks failed" / "Checks running") with sr-only - text. Review words stay only for review states: "Approved", "Changes requested", - "Review required" (amber for the two that need work, emerald for Approved). + text. The review state is a glyph too (`PullRequestReviewGlyph`: user-check emerald for Approved, + user-x amber for Changes requested, a muted user for Review required), just before the checks glyph. - Conflict glyph: when a row is open, not draft, and `mergeability === "conflicting"`, the PR glyph at the left becomes `TriangleAlertIcon` in destructive with label "Conflicts with ". Draft wins over conflict; merged and closed are unchanged. From fa4888685129c08cb35799de958eee26811e5193 Mon Sep 17 00:00:00 2001 From: badcuban <108198679+badcuban@users.noreply.github.com> Date: Thu, 3 Sep 2026 12:10:26 -0400 Subject: [PATCH 09/13] fix(web): checks summary scrolls only the pull request Summary Jumping to the checks used scrollIntoView, which also scrolled every scrolling ancestor: the whole shell moved up and left a blank band under it that could not be scrolled back. The jump now moves only the Summary's own scroll box, and the Comments row jump does the same. --- .../pull-requests/PullRequestDetailPanel.tsx | 7 ++++--- .../pull-requests/PullRequestSummaryTab.tsx | 3 ++- .../pull-requests/pullRequestPresentation.tsx | 16 ++++++++++++++++ 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx b/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx index 4e84aec0..5e6a873b 100644 --- a/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx @@ -89,6 +89,7 @@ import { TEXT_BUTTON_CLASS, pullRequestChecksTone, pullRequestHostName, + scrollPullRequestSummaryTo, changeRequestWord, } from "./pullRequestPresentation"; import { @@ -346,9 +347,9 @@ export function PullRequestDetailPanel({ [detail.data?.checks], ); const scrollToChecks = useCallback(() => { - panelRoot.current - ?.querySelector("[data-pull-request-checks]") - ?.scrollIntoView({ block: "start", behavior: "smooth" }); + scrollPullRequestSummaryTo( + panelRoot.current?.querySelector("[data-pull-request-checks]") ?? null, + ); }, []); // Below the two-column width the detail stands in for the list, so even a diff --git a/apps/web/src/components/pull-requests/PullRequestSummaryTab.tsx b/apps/web/src/components/pull-requests/PullRequestSummaryTab.tsx index 3798ec75..cfc28c08 100644 --- a/apps/web/src/components/pull-requests/PullRequestSummaryTab.tsx +++ b/apps/web/src/components/pull-requests/PullRequestSummaryTab.tsx @@ -54,6 +54,7 @@ import { SECTION_LABEL_CLASS, TEXT_BUTTON_CLASS, TextChoice, + scrollPullRequestSummaryTo, } from "./pullRequestPresentation"; import { formatPullRequestChecksSummary, summarizePullRequestChecks } from "./pullRequests.logic"; @@ -86,7 +87,7 @@ export function PullRequestSummaryTab({ const comments = activity?.comments ?? null; const conversation = useRef(null); const scrollToConversation = useCallback(() => { - conversation.current?.scrollIntoView({ block: "start", behavior: "smooth" }); + scrollPullRequestSummaryTo(conversation.current); }, []); return ( diff --git a/apps/web/src/components/pull-requests/pullRequestPresentation.tsx b/apps/web/src/components/pull-requests/pullRequestPresentation.tsx index c5552aba..930c26c3 100644 --- a/apps/web/src/components/pull-requests/pullRequestPresentation.tsx +++ b/apps/web/src/components/pull-requests/pullRequestPresentation.tsx @@ -307,6 +307,22 @@ export function PullRequestReviewGlyph({ ); } +/** + * Scrolls the Summary tab so `target` sits at its top. Only the Summary's own + * scroll box moves: `scrollIntoView` would also scroll every scrolling + * ancestor, which on the page shoved the whole shell up and left a blank band + * under it that nothing could scroll back from. + */ +export function scrollPullRequestSummaryTo(target: HTMLElement | null): void { + const scroller = target?.closest("[data-pull-request-summary-scroll]"); + if (!target || !scroller) { + return; + } + const top = + scroller.scrollTop + target.getBoundingClientRect().top - scroller.getBoundingClientRect().top; + scroller.scrollTo({ top, behavior: "smooth" }); +} + /** The dot that separates two facts on a meta line. */ export function MetaSeparator() { return ( From f66dff043251deb626f090d110b576764080da1e Mon Sep 17 00:00:00 2001 From: badcuban <108198679+badcuban@users.noreply.github.com> Date: Thu, 3 Sep 2026 12:24:58 -0400 Subject: [PATCH 10/13] fix(web): merge dialog checkbox row keeps the dialog's padding --- .../src/components/pull-requests/PullRequestDetailPanel.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx b/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx index 5e6a873b..744144c8 100644 --- a/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx @@ -1408,7 +1408,9 @@ function usePullRequestActions({ {confirming.action === "merge" ? ( -