diff --git a/apps/server/src/auth/RpcAuthorization.ts b/apps/server/src/auth/RpcAuthorization.ts index 9629ef1c8b2..96f5a131b72 100644 --- a/apps/server/src/auth/RpcAuthorization.ts +++ b/apps/server/src/auth/RpcAuthorization.ts @@ -54,6 +54,7 @@ export const RPC_REQUIRED_SCOPES = { [WS_METHODS.pullRequestsListStats]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsDetail]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsDiff]: AuthOrchestrationReadScope, + [WS_METHODS.pullRequestsDiffFileContents]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsRunAction]: AuthOrchestrationOperateScope, [WS_METHODS.pullRequestsComment]: AuthOrchestrationOperateScope, [WS_METHODS.pullRequestsSubmitReview]: AuthOrchestrationOperateScope, diff --git a/apps/server/src/cloud/bootService.test.ts b/apps/server/src/cloud/bootService.test.ts index a9ae3f49f70..029987fc312 100644 --- a/apps/server/src/cloud/bootService.test.ts +++ b/apps/server/src/cloud/bootService.test.ts @@ -69,6 +69,8 @@ const makeHarness = Effect.fn("test.make_boot_service_harness")(function* ( timedOut: false, stdoutTruncated: false, stderrTruncated: false, + stdoutInvalidUtf8: false, + stderrInvalidUtf8: false, }; }), }); diff --git a/apps/server/src/cloud/pinnedRuntime.test.ts b/apps/server/src/cloud/pinnedRuntime.test.ts index e6ec99e7e8c..f34f0f5cf4d 100644 --- a/apps/server/src/cloud/pinnedRuntime.test.ts +++ b/apps/server/src/cloud/pinnedRuntime.test.ts @@ -31,6 +31,8 @@ const successfulRunner = (fs: FileSystem.FileSystem, path: Path.Path) => timedOut: false, stdoutTruncated: false, stderrTruncated: false, + stdoutInvalidUtf8: false, + stderrInvalidUtf8: false, }; }), }); diff --git a/apps/server/src/cloud/selfUpdate.test.ts b/apps/server/src/cloud/selfUpdate.test.ts index 276ee037773..71880ec4ecf 100644 --- a/apps/server/src/cloud/selfUpdate.test.ts +++ b/apps/server/src/cloud/selfUpdate.test.ts @@ -45,6 +45,8 @@ const makeHarness = Effect.fn("test.make_self_update_harness")(function* ( timedOut: false, stdoutTruncated: false, stderrTruncated: false, + stdoutInvalidUtf8: false, + stderrInvalidUtf8: false, }; } order.push("preflight"); @@ -64,6 +66,8 @@ const makeHarness = Effect.fn("test.make_self_update_harness")(function* ( timedOut: false, stdoutTruncated: false, stderrTruncated: false, + stdoutInvalidUtf8: false, + stderrInvalidUtf8: false, }; }), }); diff --git a/apps/server/src/environment/ServerEnvironmentLabel.test.ts b/apps/server/src/environment/ServerEnvironmentLabel.test.ts index b5bb8a8ff1c..6fc06b889db 100644 --- a/apps/server/src/environment/ServerEnvironmentLabel.test.ts +++ b/apps/server/src/environment/ServerEnvironmentLabel.test.ts @@ -81,6 +81,8 @@ describe("resolveServerEnvironmentLabel", () => { timedOut: false, stdoutTruncated: false, stderrTruncated: false, + stdoutInvalidUtf8: false, + stderrInvalidUtf8: false, }), ); @@ -120,6 +122,8 @@ describe("resolveServerEnvironmentLabel", () => { timedOut: false, stdoutTruncated: false, stderrTruncated: false, + stdoutInvalidUtf8: false, + stderrInvalidUtf8: false, }), ); @@ -223,6 +227,8 @@ describe("resolveServerEnvironmentLabel", () => { timedOut: false, stdoutTruncated: false, stderrTruncated: false, + stdoutInvalidUtf8: false, + stderrInvalidUtf8: false, }), ); @@ -264,6 +270,8 @@ describe("resolveServerEnvironmentLabel", () => { timedOut: false, stdoutTruncated: false, stderrTruncated: false, + stdoutInvalidUtf8: false, + stderrInvalidUtf8: false, }), ); diff --git a/apps/server/src/processRunner.ts b/apps/server/src/processRunner.ts index c1ee2b2cb0c..16b5625d469 100644 --- a/apps/server/src/processRunner.ts +++ b/apps/server/src/processRunner.ts @@ -13,6 +13,7 @@ import { HostProcessPlatform } from "@t3tools/shared/hostProcess"; import { resolveSpawnCommand } from "@t3tools/shared/shell"; import { collectUint8StreamText, + decodeUtf8, type CollectedUint8StreamText, } from "./stream/collectUint8StreamText.ts"; @@ -41,6 +42,8 @@ export interface ProcessRunOutput { readonly timedOut: boolean; readonly stdoutTruncated: boolean; readonly stderrTruncated: boolean; + readonly stdoutInvalidUtf8: boolean; + readonly stderrInvalidUtf8: boolean; } const ProcessInvocationFields = { @@ -238,7 +241,7 @@ const collectText = Effect.fn("processRunner.collectText")(function* (input: { ), Effect.map( (state): CollectedUint8StreamText => ({ - text: Buffer.concat(state.chunks, state.bytes).toString("utf8"), + ...decodeUtf8(Buffer.concat(state.chunks, state.bytes)), bytes: state.bytes, truncated: false, }), @@ -268,6 +271,8 @@ function finalizeRunProcess( timedOut: true, stdoutTruncated: false, stderrTruncated: false, + stdoutInvalidUtf8: false, + stderrInvalidUtf8: false, } satisfies ProcessRunOutput); } return Effect.fail( @@ -394,6 +399,8 @@ const runProcessCore = Effect.fn("processRunner.runProcessCore")(function* ( timedOut: false, stdoutTruncated: stdout.truncated, stderrTruncated: stderr.truncated, + stdoutInvalidUtf8: stdout.invalidUtf8, + stderrInvalidUtf8: stderr.invalidUtf8, } satisfies ProcessRunOutput; }); diff --git a/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.test.ts b/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.test.ts index 267917b5102..b52b6d497d6 100644 --- a/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.test.ts @@ -29,19 +29,24 @@ function output(stdout: string) { }; } +function pullRequestRows( + count: number, + firstNumber: number, +): ReadonlyArray> { + return Array.from({ length: count }, (_, index) => ({ + pullRequestId: firstNumber + index, + title: `Pull request ${firstNumber + index}`, + status: "active", + sourceRefName: "refs/heads/feat/page", + targetRefName: "refs/heads/main", + creationDate: "2026-07-01T00:00:00Z", + repository: { name: "web", project: { name: "platform" } }, + url: `https://dev.azure.com/acme/_apis/git/repositories/web/pullRequests/${firstNumber + index}`, + })); +} + function pullRequests(count: number, firstNumber: number): string { - return JSON.stringify( - Array.from({ length: count }, (_, index) => ({ - pullRequestId: firstNumber + index, - title: `Pull request ${firstNumber + index}`, - status: "active", - sourceRefName: "refs/heads/feat/page", - targetRefName: "refs/heads/main", - creationDate: "2026-07-01T00:00:00Z", - repository: { name: "web", project: { name: "platform" } }, - url: `https://dev.azure.com/acme/_apis/git/repositories/web/pullRequests/${firstNumber + index}`, - })), - ); + return JSON.stringify(pullRequestRows(count, firstNumber)); } /** The arguments of the nth az invocation. */ @@ -172,6 +177,44 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { assert.strictEqual(batch.items.length, 10); assert.isTrue(batch.truncated); + assert.strictEqual(batch.cursorAdvance, 10); + }), + ); + + it.effect("advances by malformed raw rows and keeps reading until the page is full", () => + Effect.gen(function* () { + mockedExecute + .mockReturnValueOnce( + Effect.succeed( + output( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify([ + { pullRequestId: "malformed" }, + pullRequestRows(1, 1)[0], + { pullRequestId: "also malformed" }, + ]), + ), + ), + ) + .mockReturnValueOnce(Effect.succeed(output(pullRequests(2, 2)))); + const cli = yield* AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli; + + const batch = yield* cli.listPullRequests({ + cwd: "/w", + repository: "web", + state: "open", + involvement: "all", + viewer: "bilal@acme.dev", + limit: 2, + }); + + expect(batch.items.map((item) => item.number)).toEqual([1, 2]); + assert.isTrue(batch.truncated); + // Three raw rows from the first request and one from the second produced this page. + assert.strictEqual(batch.cursorAdvance, 4); + const secondArgs = argsOfCall(1); + assert.strictEqual(secondArgs[secondArgs.indexOf("--skip") + 1], "3"); + assert.strictEqual(secondArgs[secondArgs.indexOf("--top") + 1], "2"); }), ); diff --git a/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.ts b/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.ts index ab6e28de97c..43f929163db 100644 --- a/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.ts +++ b/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.ts @@ -132,7 +132,12 @@ export class AzureDevOpsPullRequestCli extends Context.Service< */ readonly cursor?: ProviderListCursor | undefined; }) => Effect.Effect< - { readonly items: ReadonlyArray; readonly truncated: boolean }, + { + readonly items: ReadonlyArray; + readonly truncated: boolean; + /** Raw Azure rows consumed to produce this page, including malformed rows. */ + readonly cursorAdvance: number; + }, AzureDevOpsPullRequestCliError >; @@ -242,6 +247,98 @@ export const make = Effect.gen(function* () { args: [...input.args, "--only-show-errors", "--output", "json"], }); + /** + * Azure pages by raw offset. Keep reading when malformed rows leave the decoded page short, and + * retain the raw count so the next public cursor skips every row this walk consumed. + */ + const listPullRequestPage = (input: { + readonly cwd: string; + readonly repository: string; + readonly state: PullRequestListState; + readonly involvement: PullRequestInvolvement; + readonly viewer: string; + readonly limit: number; + readonly skip: number; + readonly cursorAdvance: number; + readonly items: ReadonlyArray; + }): Effect.Effect< + { + readonly items: ReadonlyArray; + readonly truncated: boolean; + readonly cursorAdvance: number; + }, + AzureDevOpsPullRequestCliError + > => { + const remaining = input.limit - input.items.length; + const top = remaining + 1; + return executeJson({ + cwd: input.cwd, + args: [ + "repos", + "pr", + "list", + ...detectArgs, + "--repository", + input.repository, + ...statusArgs(input.state), + ...involvementArgs(input), + // A web link per row, which is the only url that needs no assembling. + "--include-links", + ...(input.skip === 0 ? [] : ["--skip", String(input.skip)]), + "--top", + String(top), + ], + }).pipe( + Effect.flatMap((result) => { + const raw = result.stdout.trim(); + if (raw.length === 0) { + return Effect.succeed({ + items: input.items, + truncated: false, + cursorAdvance: input.cursorAdvance, + }); + } + const decoded = decodePullRequestListJson(raw); + if (!Result.isSuccess(decoded)) { + return Effect.fail( + new AzureDevOpsPullRequestReadError({ + command: "az", + cwd: input.cwd, + operation: "listPullRequests", + cause: decoded.failure, + }), + ); + } + + const lastItemIndex = decoded.success.rawIndexes[remaining - 1]; + if (lastItemIndex !== undefined) { + const consumed = lastItemIndex + 1; + return Effect.succeed({ + items: [...input.items, ...decoded.success.items.slice(0, remaining)], + // A full raw response may have more rows even when malformed entries used the probe. + truncated: consumed < decoded.success.rawCount || decoded.success.rawCount === top, + cursorAdvance: input.cursorAdvance + consumed, + }); + } + + const items = [...input.items, ...decoded.success.items]; + if (decoded.success.rawCount < top) { + return Effect.succeed({ + items, + truncated: false, + cursorAdvance: input.cursorAdvance + decoded.success.rawCount, + }); + } + return listPullRequestPage({ + ...input, + skip: input.skip + decoded.success.rawCount, + cursorAdvance: input.cursorAdvance + decoded.success.rawCount, + items, + }); + }), + ); + }; + return AzureDevOpsPullRequestCli.of({ getViewer: (input) => executeJson({ cwd: input.cwd, args: ["account", "show", "--query", "user"] }).pipe( @@ -266,51 +363,21 @@ export const make = Effect.gen(function* () { ), listPullRequests: (input) => - executeJson({ + listPullRequestPage({ cwd: input.cwd, - args: [ - "repos", - "pr", - "list", - ...detectArgs, - "--repository", - input.repository, - ...statusArgs(input.state), - ...involvementArgs(input), - // A web link per row, which is the only url that needs no assembling. - "--include-links", - // Azure counts rather than filters, so a slice carries on by stepping over what has - // already been handed over. That is an offset into a list that can shift underneath - // it: a pull request opened between two slices moves everything down one, and the row - // on the seam is the one that pays for it. - ...(input.cursor === undefined ? [] : ["--skip", String(input.cursor.delivered)]), - "--top", - // One row over the page reveals that the repository has more than the page shows. - String(input.limit + 1), - ], - }).pipe( - Effect.flatMap((result) => { - const raw = result.stdout.trim(); - if (raw.length === 0) { - return Effect.succeed({ items: [], truncated: false }); - } - const decoded = decodePullRequestListJson(raw); - return Result.isSuccess(decoded) - ? Effect.succeed({ - items: decoded.success.items.slice(0, input.limit), - // Counted before decoding, so a skipped malformed row cannot end paging early. - truncated: decoded.success.rawCount > input.limit, - }) - : Effect.fail( - new AzureDevOpsPullRequestReadError({ - command: "az", - cwd: input.cwd, - operation: "listPullRequests", - cause: decoded.failure, - }), - ); - }), - ), + repository: input.repository, + state: input.state, + involvement: input.involvement, + viewer: input.viewer, + limit: input.limit, + // Azure counts rather than filters, so a slice carries on by stepping over every raw row + // the prior slice consumed. That is an offset into a list that can shift underneath it: + // a pull request opened between two slices moves everything down one, and the row on the + // seam is the one that pays for it. + skip: input.cursor?.delivered ?? 0, + cursorAdvance: 0, + items: [], + }), getPullRequest: (input) => executeJson({ diff --git a/apps/server/src/pullRequest/AzureDevOpsPullRequestProvider.ts b/apps/server/src/pullRequest/AzureDevOpsPullRequestProvider.ts index 61802819072..dd3c8a4aa0f 100644 --- a/apps/server/src/pullRequest/AzureDevOpsPullRequestProvider.ts +++ b/apps/server/src/pullRequest/AzureDevOpsPullRequestProvider.ts @@ -131,6 +131,7 @@ export const make = Effect.gen(function* () { Effect.map((batch) => ({ items: batch.items.map(toChangeRequest), truncated: batch.truncated, + cursorAdvance: batch.cursorAdvance, // Azure answers in one order whether or not it is being carried on from, so a slice // can always be stepped past — by counting, which is all Azure offers. continues: true, diff --git a/apps/server/src/pullRequest/BitbucketPullRequestApi.test.ts b/apps/server/src/pullRequest/BitbucketPullRequestApi.test.ts index 4bcc68bf9e8..1248b396956 100644 --- a/apps/server/src/pullRequest/BitbucketPullRequestApi.test.ts +++ b/apps/server/src/pullRequest/BitbucketPullRequestApi.test.ts @@ -40,6 +40,10 @@ function page(count: number, firstNumber: number, next?: string): string { }); } +function valuePage(values: ReadonlyArray, next?: string): string { + return JSON.stringify({ values, ...(next === undefined ? {} : { next }) }); +} + /** Who opened the pull request, and two accounts that could review it. */ const bilal = { uuid: "{bilal}", nickname: "bilal" }; const octocat = { uuid: "{octocat}", nickname: "octocat" }; @@ -358,6 +362,93 @@ layer("BitbucketPullRequestApi.layer", (it) => { }), ); + it.effect("aggregates every diffstat page", () => + Effect.gen(function* () { + const next = "https://api.bitbucket.org/2.0/diffstat?page=2"; + mockedRequest + .mockReturnValueOnce( + Effect.succeed( + response( + valuePage( + [ + { lines_added: 9, lines_removed: 2 }, + { lines_added: 3, lines_removed: 1 }, + ], + next, + ), + ), + ), + ) + .mockReturnValueOnce( + Effect.succeed(response(valuePage([{ lines_added: 4, lines_removed: 7 }]))), + ); + const api = yield* BitbucketPullRequestApi.BitbucketPullRequestApi; + + const stat = yield* api.getDiffStat({ repository: "acme/web", number: 7 }); + + expect(stat).toEqual({ additions: 16, deletions: 10, changedFiles: 3 }); + expect(callAt(1).url).toBe(next); + }), + ); + + it.effect("returns the complete commit timeline oldest first across pages", () => + Effect.gen(function* () { + const next = "https://api.bitbucket.org/2.0/commits?page=2"; + mockedRequest + .mockReturnValueOnce( + Effect.succeed( + response( + valuePage( + [ + { hash: "ddd", message: "fourth", date: "2026-07-04T00:00:00Z" }, + { hash: "ccc", message: "third", date: "2026-07-03T00:00:00Z" }, + ], + next, + ), + ), + ), + ) + .mockReturnValueOnce( + Effect.succeed( + response( + valuePage([ + { hash: "bbb", message: "second", date: "2026-07-02T00:00:00Z" }, + { hash: "aaa", message: "first", date: "2026-07-01T00:00:00Z" }, + ]), + ), + ), + ); + const api = yield* BitbucketPullRequestApi.BitbucketPullRequestApi; + + const commits = yield* api.listCommits({ repository: "acme/web", number: 7 }); + + expect(commits.map((commit) => commit.oid)).toEqual(["aaa", "bbb", "ccc", "ddd"]); + expect(callAt(1).url).toBe(next); + }), + ); + + it.effect("returns build statuses from every page", () => + Effect.gen(function* () { + const next = "https://api.bitbucket.org/2.0/statuses?page=2"; + mockedRequest + .mockReturnValueOnce( + Effect.succeed(response(valuePage([{ name: "Build", state: "SUCCESSFUL" }], next))), + ) + .mockReturnValueOnce( + Effect.succeed(response(valuePage([{ name: "Lint", state: "FAILED" }]))), + ); + const api = yield* BitbucketPullRequestApi.BitbucketPullRequestApi; + + const checks = yield* api.listChecks({ repository: "acme/web", number: 7 }); + + expect(checks.map((check) => [check.name, check.status])).toEqual([ + ["Build", "success"], + ["Lint", "failure"], + ]); + expect(callAt(1).url).toBe(next); + }), + ); + it.effect("reads an empty conflict list as mergeable", () => Effect.gen(function* () { mockedRequest.mockReturnValueOnce(Effect.succeed(response(page(0, 1)))); diff --git a/apps/server/src/pullRequest/BitbucketPullRequestApi.ts b/apps/server/src/pullRequest/BitbucketPullRequestApi.ts index 511c66df691..7c0a7d11744 100644 --- a/apps/server/src/pullRequest/BitbucketPullRequestApi.ts +++ b/apps/server/src/pullRequest/BitbucketPullRequestApi.ts @@ -114,7 +114,7 @@ export type BitbucketPullRequestApiError = const MAX_PAGE_SIZE = 50; /** Pages to walk before a listing is reported as truncated. */ const MAX_LIST_PAGES = 10; -/** Commits and checks are read one page deep; the conversation is walked to its end. */ +/** The page size for pull request conversations, commits, and checks. */ const CONVERSATION_PAGE_SIZE = 50; /** * Pages of the conversation to follow before it is reported as truncated. Bitbucket serves @@ -446,6 +446,46 @@ export const make = Effect.gen(function* () { }), ); + /** Walks a Bitbucket cursor to its end and combines every decoded item. */ + const itemPages = (input: { + readonly operation: string; + readonly url: string; + readonly decode: ( + body: string, + ) => Result.Result<{ readonly items: ReadonlyArray; readonly next: string | null }, unknown>; + readonly items: ReadonlyArray; + /** Commit pages are individually oldest-first, so older pages are prepended. */ + readonly prepend: boolean; + }): Effect.Effect, BitbucketPullRequestApiError> => + readPage({ operation: input.operation, url: input.url, decode: input.decode }).pipe( + Effect.flatMap((page) => { + const items = input.prepend + ? [...page.items, ...input.items] + : [...input.items, ...page.items]; + return page.next === null + ? Effect.succeed(items) + : itemPages({ ...input, url: page.next, items }); + }), + ); + + /** Diffstat has one aggregate per page, so its totals are folded while following `next`. */ + const diffStatPages = (input: { + readonly url: string; + readonly totals: BitbucketDiffStat; + }): Effect.Effect => + readPage({ operation: "getDiffStat", url: input.url, decode: decodeDiffstatJson }).pipe( + Effect.flatMap((page) => { + const totals = { + additions: input.totals.additions + page.additions, + deletions: input.totals.deletions + page.deletions, + changedFiles: input.totals.changedFiles + page.changedFiles, + }; + return page.next === null + ? Effect.succeed(totals) + : diffStatPages({ url: page.next, totals }); + }), + ); + return BitbucketPullRequestApi.of({ getViewer: () => bitbucket.request({ method: "GET", url: "/user" }).pipe( @@ -533,10 +573,9 @@ export const make = Effect.gen(function* () { getDiffStat: (input) => withRepository(input.repository, (path) => - readPage({ - operation: "getDiffStat", + diffStatPages({ url: `${path}/pullrequests/${input.number}/diffstat?pagelen=${MAX_PAGE_SIZE}`, - decode: decodeDiffstatJson, + totals: { additions: 0, deletions: 0, changedFiles: 0 }, }), ), @@ -561,19 +600,23 @@ export const make = Effect.gen(function* () { listCommits: (input) => withRepository(input.repository, (path) => - readPage({ + itemPages({ operation: "listCommits", url: `${path}/pullrequests/${input.number}/commits?pagelen=${CONVERSATION_PAGE_SIZE}`, decode: decodeCommitsJson, + items: [], + prepend: true, }), ), listChecks: (input) => withRepository(input.repository, (path) => - readPage({ + itemPages({ operation: "listChecks", url: `${path}/pullrequests/${input.number}/statuses?pagelen=${CONVERSATION_PAGE_SIZE}`, decode: decodeStatusesJson, + items: [], + prepend: false, }), ), diff --git a/apps/server/src/pullRequest/BitbucketPullRequestProvider.test.ts b/apps/server/src/pullRequest/BitbucketPullRequestProvider.test.ts index 94aba9fa407..7e57d6c771e 100644 --- a/apps/server/src/pullRequest/BitbucketPullRequestProvider.test.ts +++ b/apps/server/src/pullRequest/BitbucketPullRequestProvider.test.ts @@ -1,6 +1,24 @@ import { describe, expect, it } from "vite-plus/test"; -import { bitbucketViewerPermissions } from "./BitbucketPullRequestProvider.ts"; +import * as BitbucketApi from "../sourceControl/BitbucketApi.ts"; +import { + bitbucketErrorReason, + bitbucketViewerPermissions, +} from "./BitbucketPullRequestProvider.ts"; + +describe("bitbucketErrorReason", () => { + it("treats only an HTTP 401 as unusable credentials", () => { + const responseError = (status: number) => + new BitbucketApi.BitbucketResponseError({ + operation: "request", + status, + responseBodyLength: 0, + }); + + expect(bitbucketErrorReason(responseError(401))).toBe("unauthenticated"); + expect(bitbucketErrorReason(responseError(403))).toBe("failed"); + }); +}); describe("bitbucketViewerPermissions", () => { it("offers both actions to credentials with write access", () => { diff --git a/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts b/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts index 4a5ee4ee586..6839e0c3b97 100644 --- a/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts +++ b/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts @@ -54,12 +54,12 @@ export function bitbucketViewerPermissions(input: { } /** The failures that mean the credentials are the problem, rather than one request. */ -function reasonFor( +export function bitbucketErrorReason( error: BitbucketPullRequestApi.BitbucketPullRequestApiError, ): PullRequestProviderError["reason"] { // Bitbucket is read over HTTP with credentials from the environment, so there is no tool to be // missing: unusable always means the credentials are absent or refused. - if (error._tag === "BitbucketResponseError" && (error.status === 401 || error.status === 403)) { + if (error._tag === "BitbucketResponseError" && error.status === 401) { return "unauthenticated"; } return "failed"; @@ -95,7 +95,7 @@ export const make = Effect.gen(function* () { new PullRequestProviderError({ provider: "bitbucket", operation, - reason: reasonFor(error), + reason: bitbucketErrorReason(error), // Every Bitbucket failure states its own fact; this names the operation around it, so // the two do not stack into "failed in x: failed in y: ...". detail: error.detail, diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts index 84f0d99e1b4..07d79feaca1 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts @@ -18,17 +18,22 @@ const layer = it.layer( ), ); -function output(stdout: string, stdoutTruncated = false) { +function output(stdout: string, stdoutTruncated = false, stdoutInvalidUtf8 = false) { return { exitCode: ChildProcessSpawner.ExitCode(0), stdout, stderr: "", stdoutTruncated, stderrTruncated: false, + stdoutInvalidUtf8, }; } -function pullRequests(count: number, firstNumber: number): string { +function pullRequests( + count: number, + firstNumber: number, + overrides: (number: number) => Readonly> = () => ({}), +): string { return JSON.stringify( Array.from({ length: count }, (_, index) => ({ number: firstNumber + index, @@ -38,6 +43,7 @@ function pullRequests(count: number, firstNumber: number): string { baseRefName: "main", createdAt: "2026-07-01T00:00:00Z", updatedAt: "2026-07-02T00:00:00Z", + ...overrides(firstNumber + index), })), ); } @@ -665,7 +671,9 @@ layer("GitHubPullRequestCli.layer", (it) => { Effect.gen(function* () { // GitHub answers for a repository outside its search index with no rows and no error. mockedExecute.mockReturnValueOnce(Effect.succeed(output("[]"))); - mockedExecute.mockReturnValueOnce(Effect.succeed(output(pullRequests(3, 1)))); + mockedExecute.mockReturnValueOnce( + Effect.succeed(output(pullRequests(3, 1, () => ({ state: "CLOSED" })))), + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; const batch = yield* cli.listPullRequests({ @@ -679,14 +687,116 @@ layer("GitHubPullRequestCli.layer", (it) => { }); assert.strictEqual(batch.items.length, 3); - // Nothing of the search survives the fallback, not even the tab's own qualifier: this read - // happens precisely because search answered nothing here, and `is:unmerged` is a search - // too. Those rows arrive in gh's own order, so nothing can carry on from them. + // The fallback itself uses no search, then narrows the decoded rows locally. They still + // arrive in gh's own order, so nothing can carry on from them. + expect(searchOfCall(1)).toBeUndefined(); + assert.isFalse(batch.continues); + }), + ); + + it.effect("keeps state and involvement filters on the search-free fallback", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce(Effect.succeed(output("[]"))); + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output( + pullRequests(4, 1, (number) => ({ + state: number === 4 ? "OPEN" : "CLOSED", + ...(number === 3 ? { mergedAt: "2026-07-03T00:00:00Z" } : {}), + reviewRequests: + number === 2 ? [{ slug: "platform", name: "Platform" }] : [{ login: "bilal" }], + })), + ), + ), + ); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + const batch = yield* cli.listPullRequests({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + state: "closed", + involvement: "reviewing", + viewer: "bilal", + limit: 10, + }); + + // Individual requests for this viewer and team requests survive. The fallback cannot + // resolve team membership, so dropping team-routed reviews would hide legitimate work. + expect(batch.items.map((item) => item.number)).toEqual([1, 2]); expect(searchOfCall(1)).toBeUndefined(); assert.isFalse(batch.continues); }), ); + it.effect("grows the search-free fallback until it fills the filtered page", () => + Effect.gen(function* () { + const unrelated = () => ({ reviewRequests: [{ login: "somebody-else" }] }); + mockedExecute.mockReturnValueOnce(Effect.succeed(output("[]"))); + mockedExecute.mockReturnValueOnce(Effect.succeed(output(pullRequests(3, 1, unrelated)))); + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output( + pullRequests(4, 1, (number) => + number === 4 ? { reviewRequests: [{ login: "bilal" }] } : unrelated(), + ), + ), + ), + ); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + const batch = yield* cli.listPullRequests({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + state: "open", + involvement: "reviewing", + viewer: "bilal", + limit: 2, + }); + + expect(batch.items.map((item) => item.number)).toEqual([4]); + const firstFallbackArgs = callAt(1).args; + const secondFallbackArgs = callAt(2).args; + expect(firstFallbackArgs[firstFallbackArgs.indexOf("--limit") + 1]).toBe("3"); + expect(secondFallbackArgs[secondFallbackArgs.indexOf("--limit") + 1]).toBe("6"); + assert.isFalse(batch.truncated); + }), + ); + + it.effect("bounds a sparse search-free fallback and reports the unread tail", () => + Effect.gen(function* () { + mockedExecute.mockImplementation((_input) => { + if (mockedExecute.mock.calls.length === 1) return Effect.succeed(output("[]")); + const args = callAt(mockedExecute.mock.calls.length - 1).args; + const limit = Number(args[args.indexOf("--limit") + 1]); + return Effect.succeed( + output( + pullRequests(limit, 1, () => ({ + reviewRequests: [{ login: "somebody-else" }], + })), + ), + ); + }); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + const batch = yield* cli.listPullRequests({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + state: "open", + involvement: "reviewing", + viewer: "bilal", + limit: 2, + }); + + const finalArgs = callAt(mockedExecute.mock.calls.length - 1).args; + expect(finalArgs[finalArgs.indexOf("--limit") + 1]).toBe("1000"); + assert.strictEqual(batch.items.length, 0); + assert.isTrue(batch.truncated); + }), + ); + it.effect("takes an empty slice for a repository that has run out, not one to read again", () => Effect.gen(function* () { mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); @@ -1009,6 +1119,129 @@ layer("GitHubPullRequestCli.layer", (it) => { }), ); + it.effect("expands a new file from a root commit without requiring a parent", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce(Effect.succeed(output("\ta1b2c3d\n"))); + mockedExecute.mockReturnValueOnce(Effect.succeed(output("root contents\n"))); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + const contents = yield* cli.getPullRequestDiffFileContents({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + commit: "a1b2c3d", + changeType: "new", + oldPath: "src/root.ts", + newPath: "src/root.ts", + }); + + expect(contents).toEqual({ oldContents: "", newContents: "root contents\n" }); + assert.strictEqual(mockedExecute.mock.calls.length, 2); + expect(callAt(1).args.join(" ")).toContain("contents/src/root.ts?ref=a1b2c3d"); + }), + ); + + it.effect("reports unusable diff revisions as a structured error", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce(Effect.succeed(output("not-a-sha\tstill-not-a-sha\n"))); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + const error = yield* Effect.flip( + cli.getPullRequestDiffFileContents({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + commit: "a1b2c3d", + changeType: "change", + oldPath: "src/a.ts", + newPath: "src/a.ts", + }), + ); + + assert.strictEqual(error._tag, "GitHubDiffRevisionsUnavailableError"); + if (error._tag === "GitHubDiffRevisionsUnavailableError") { + assert.strictEqual(error.number, 7); + assert.strictEqual(error.commit, "a1b2c3d"); + } + }), + ); + + it.effect("reports an oversized diff file with its path and reason", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce(Effect.succeed(output("a1b2c3d\tb1c2d3e\n"))); + mockedExecute.mockReturnValueOnce(Effect.succeed(output("partial", true))); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + const error = yield* Effect.flip( + cli.getPullRequestDiffFileContents({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + changeType: "deleted", + oldPath: "src/large.ts", + newPath: "src/large.ts", + }), + ); + + assert.strictEqual(error._tag, "GitHubDiffFileContentsUnavailableError"); + if (error._tag === "GitHubDiffFileContentsUnavailableError") { + assert.strictEqual(error.path, "src/large.ts"); + assert.strictEqual(error.reason, "oversized"); + } + }), + ); + + it.effect("reports undecodable diff file contents as binary", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce(Effect.succeed(output("a1b2c3d\tb1c2d3e\n"))); + mockedExecute.mockReturnValueOnce( + Effect.succeed(output("binary\uFFFDcontents", false, true)), + ); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + const error = yield* Effect.flip( + cli.getPullRequestDiffFileContents({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + changeType: "deleted", + oldPath: "assets/logo.png", + newPath: "assets/logo.png", + }), + ); + + assert.strictEqual(error._tag, "GitHubDiffFileContentsUnavailableError"); + if (error._tag === "GitHubDiffFileContentsUnavailableError") { + assert.strictEqual(error.path, "assets/logo.png"); + assert.strictEqual(error.reason, "binary"); + } + }), + ); + + it.effect("returns valid text containing a literal replacement character", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce(Effect.succeed(output("a1b2c3d\tb1c2d3e\n"))); + mockedExecute.mockReturnValueOnce(Effect.succeed(output("before\uFFFDafter"))); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + const contents = yield* cli.getPullRequestDiffFileContents({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + changeType: "deleted", + oldPath: "docs/encoding.md", + newPath: "docs/encoding.md", + }); + + assert.strictEqual(contents.oldContents, "before\uFFFDafter"); + }), + ); + it.effect("ends the diff on a page with no files rather than asking for it again", () => Effect.gen(function* () { mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.ts index 682e243b66c..fa0558c12d2 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.ts @@ -131,6 +131,48 @@ export class GitHubDiffCommitError extends Schema.TaggedErrorClass()( + "GitHubDiffRevisionsUnavailableError", + { + command: Schema.Literal("gh"), + cwd: Schema.String, + number: Schema.Int, + commit: Schema.optional(Schema.String), + }, +) { + get detail(): string { + return this.commit === undefined + ? `Pull request #${this.number} reported no usable base and head revisions.` + : `Commit ${this.commit} reported no usable revisions for this file.`; + } + + override get message(): string { + return `GitHub CLI failed in getPullRequestDiffFileContents: ${this.detail}`; + } +} + +/** A blob exists, but expanding it would be unsafe or would not produce text. */ +export class GitHubDiffFileContentsUnavailableError extends Schema.TaggedErrorClass()( + "GitHubDiffFileContentsUnavailableError", + { + command: Schema.Literal("gh"), + cwd: Schema.String, + path: Schema.String, + reason: Schema.Literals(["oversized", "binary"]), + }, +) { + get detail(): string { + return this.reason === "oversized" + ? `The diff file '${this.path}' exceeds the 1 MB expansion limit.` + : `The diff file '${this.path}' is binary.`; + } + + override get message(): string { + return `GitHub CLI failed in getPullRequestDiffFileContents: ${this.detail}`; + } +} + /** * Not a decode failure: a repository was named that cannot go into a search or into a GraphQL * document as itself. Every qualifier and every alias below is composed from `owner/name`, so a @@ -159,12 +201,19 @@ export type GitHubPullRequestCliError = | GitHubPullRequestReadError | GitHubDiffCursorError | GitHubDiffCommitError + | GitHubDiffRevisionsUnavailableError + | GitHubDiffFileContentsUnavailableError | GitHubRepositorySelectorError | GitHubViewerLoginUnavailableError; /** A large pull request can produce a multi-megabyte patch; past this it is truncated. */ const DIFF_MAX_OUTPUT_BYTES = 8 * 1024 * 1024; const DIFF_TIMEOUT_MS = 60_000; +/** Pierre expansion is for source files, not blobs large enough to stall a review surface. */ +const DIFF_FILE_MAX_OUTPUT_BYTES = 1024 * 1024; + +/** A search-free fallback may scan older rows for local filters, but never the whole repository. */ +const PULL_REQUEST_FALLBACK_MAX_ROWS = 1_000; /** What the files API serves at most in one response, which is what one slice is made of. */ const DIFF_FILES_PAGE_SIZE = 100; @@ -287,6 +336,20 @@ export class GitHubPullRequestCli extends Context.Service< readonly commit?: string | undefined; }) => Effect.Effect; + readonly getPullRequestDiffFileContents: (input: { + readonly cwd: string; + readonly repository: string; + readonly host: string; + readonly number: number; + readonly commit?: string | undefined; + readonly changeType: "change" | "rename-pure" | "rename-changed" | "new" | "deleted"; + readonly oldPath: string; + readonly newPath: string; + }) => Effect.Effect< + { readonly oldContents: string; readonly newContents: string }, + GitHubPullRequestCliError + >; + readonly listReviewThreadComments: (input: { readonly cwd: string; readonly repository: string; @@ -446,8 +509,8 @@ function involvementArgs(input: { // takes one `--search`, so the reader's text joins the qualifiers rather than replacing them. const query = input.query?.trim() ?? ""; // The fallback read exists because this repository's search index answered nothing, so it goes - // nowhere near search: no order, no cursor, and no qualifiers either, since `review-requested:` - // and `is:unmerged` are searches too and would come back just as empty. + // nowhere near search: no order, cursor or qualifiers. Its decoded rows are narrowed by state + // and involvement below, since widening either would put unrelated pull requests on the page. const searchTerms = !input.sorted ? [] : [ @@ -470,6 +533,26 @@ function involvementArgs(input: { ]; } +/** The search-free fallback is wider than the request, so narrow its decoded rows locally. */ +function matchesUnsortedListing( + item: GitHubPullRequestListItem, + input: { + readonly state: PullRequestListState; + readonly involvement: PullRequestInvolvement; + readonly viewer: string; + }, +): boolean { + const matchesState = input.state === "all" || item.state === input.state; + const viewer = input.viewer.toLowerCase(); + const matchesInvolvement = + input.involvement === "all" || + (input.involvement === "authored" + ? item.author?.login.toLowerCase() === viewer + : item.hasTeamReviewRequest || + item.reviewRequestLogins.some((login) => login.toLowerCase() === viewer)); + return matchesState && matchesInvolvement; +} + /** What a repository selector may hold before it goes into a search as itself. */ const SEARCH_REPOSITORY = /^[A-Za-z0-9._-]+\/[A-Za-z0-9._-]+$/; @@ -708,6 +791,96 @@ export const make = Effect.gen(function* () { ); }; + const getPullRequestDiffFileContents: GitHubPullRequestCli["Service"]["getPullRequestDiffFileContents"] = + (input) => + Effect.gen(function* () { + if (input.commit !== undefined && !isCommitSha(input.commit)) { + return yield* new GitHubDiffCommitError({ command: "gh", cwd: input.cwd }); + } + const { owner, name } = parseRepositorySelector(input.repository); + const refsResult = yield* github.execute({ + cwd: input.cwd, + args: [ + "api", + "--hostname", + input.host, + input.commit === undefined + ? `repos/${owner}/${name}/pulls/${input.number}` + : `repos/${owner}/${name}/commits/${input.commit}`, + "--jq", + input.commit === undefined + ? "[.base.sha, .head.sha] | @tsv" + : "[.parents[0].sha, .sha] | @tsv", + ], + maxOutputBytes: 1024, + timeoutMs: DIFF_TIMEOUT_MS, + }); + // Keep a leading tab: a root commit has no parent, and jq represents that absent old + // revision as the empty field before the tab. Every file in it is new, so that is a + // usable answer whenever the caller does not need the old side. + const [baseRef, headRef, ...extraRefs] = refsResult.stdout.trimEnd().split("\t"); + const rootCommitNewFile = + input.commit !== undefined && input.changeType === "new" && baseRef === ""; + if ( + refsResult.stdoutTruncated || + !headRef || + extraRefs.length > 0 || + (!rootCommitNewFile && (baseRef === undefined || !isCommitSha(baseRef))) || + !isCommitSha(headRef) + ) { + return yield* new GitHubDiffRevisionsUnavailableError({ + command: "gh", + cwd: input.cwd, + number: input.number, + ...(input.commit === undefined ? {} : { commit: input.commit }), + }); + } + + const readFile = (revision: string, filePath: string) => + github + .execute({ + cwd: input.cwd, + args: [ + "api", + "--hostname", + input.host, + "--header", + "Accept: application/vnd.github.raw+json", + `repos/${owner}/${name}/contents/${filePath + .split("/") + .map(encodeURIComponent) + .join("/")}?ref=${encodeURIComponent(revision)}`, + ], + maxOutputBytes: DIFF_FILE_MAX_OUTPUT_BYTES, + timeoutMs: DIFF_TIMEOUT_MS, + }) + .pipe( + Effect.flatMap((result) => + result.stdoutTruncated || + result.stdout.includes("\0") || + result.stdoutInvalidUtf8 === true + ? Effect.fail( + new GitHubDiffFileContentsUnavailableError({ + command: "gh", + cwd: input.cwd, + path: filePath, + reason: result.stdoutTruncated ? "oversized" : "binary", + }), + ) + : Effect.succeed(result.stdout), + ), + ); + + const [oldContents, newContents] = yield* Effect.all( + [ + input.changeType === "new" ? Effect.succeed("") : readFile(baseRef, input.oldPath), + input.changeType === "deleted" ? Effect.succeed("") : readFile(headRef, input.newPath), + ], + { concurrency: 2 }, + ); + return { oldContents, newContents }; + }); + return GitHubPullRequestCli.of({ getViewerLogin: (input) => github.execute({ cwd: input.cwd, args: ["api", "user", "--jq", ".login"] }).pipe( @@ -720,8 +893,10 @@ export const make = Effect.gen(function* () { ), listPullRequests: (input) => { + const fallbackMaxRows = Math.max(input.limit + 1, PULL_REQUEST_FALLBACK_MAX_ROWS); const read = ( continues: boolean, + requestedRows = input.limit + 1, ): Effect.Effect => github .execute({ @@ -735,7 +910,7 @@ export const make = Effect.gen(function* () { input.state, "--limit", // One extra row reveals that the repository has more than the page shows. - String(input.limit + 1), + String(requestedRows), "--json", PULL_REQUEST_LIST_JSON_FIELDS, ], @@ -747,22 +922,37 @@ export const make = Effect.gen(function* () { return Effect.succeed({ items: [], truncated: false, continues }); } const decoded = decodePullRequestListJson(raw); - return Result.isSuccess(decoded) - ? Effect.succeed({ - items: decoded.success.items.slice(0, input.limit), - // One row over the page size is the probe for a next page, and it is - // counted before decoding: a skipped malformed row must not end paging. - truncated: decoded.success.rawCount > input.limit, - continues, - }) - : Effect.fail( - new GitHubPullRequestReadError({ - command: "gh", - cwd: input.cwd, - operation: "listPullRequests", - cause: decoded.failure, - }), - ); + if (Result.isSuccess(decoded)) { + const items = continues + ? decoded.success.items + : decoded.success.items.filter((item) => matchesUnsortedListing(item, input)); + if ( + !continues && + items.length < input.limit && + decoded.success.rawCount >= requestedRows && + requestedRows < fallbackMaxRows + ) { + const nextRows = Math.min(requestedRows * 2, fallbackMaxRows); + if (nextRows > requestedRows) return read(false, nextRows); + } + return Effect.succeed({ + items: items.slice(0, input.limit), + // One row over the page size is the probe for a next page, and it is + // counted before decoding: a skipped malformed row must not end paging. + truncated: continues + ? decoded.success.rawCount > input.limit + : items.length > input.limit || decoded.success.rawCount >= requestedRows, + continues, + }); + } + return Effect.fail( + new GitHubPullRequestReadError({ + command: "gh", + cwd: input.cwd, + operation: "listPullRequests", + cause: decoded.failure, + }), + ); }), ); // GitHub does not index every repository for search, and one it will not search answers @@ -947,6 +1137,8 @@ export const make = Effect.gen(function* () { ); }, + getPullRequestDiffFileContents, + listReviewThreadComments: (input) => Effect.gen(function* () { const { owner, name } = parseRepositorySelector(input.repository); @@ -988,7 +1180,7 @@ export const make = Effect.gen(function* () { const entries: GitHubReviewThreadEntry[] = []; const avatarsByLogin = new Map(); let reviewers: ReadonlyArray = []; - let viewer: GitHubReviewThreadPage["viewer"] = { canUpdate: true, didAuthor: true }; + let viewer: GitHubReviewThreadPage["viewer"] = { canUpdate: true, didAuthor: false }; let cursor: string | null = null; let page = 0; do { diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts index d54553774f2..0abd8ef56b7 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts @@ -1,6 +1,9 @@ -import { describe, expect, it } from "vite-plus/test"; +import { describe, expect, it } from "@effect/vitest"; +import * as Effect from "effect/Effect"; +import * as Layer from "effect/Layer"; -import { gitHubViewerPermissions, loginAvatarUrl } from "./GitHubPullRequestProvider.ts"; +import * as GitHubPullRequestCli from "./GitHubPullRequestCli.ts"; +import { gitHubViewerPermissions, loginAvatarUrl, make } from "./GitHubPullRequestProvider.ts"; describe("gitHubViewerPermissions", () => { it("offers everything to a viewer who can write to the repository", () => { @@ -39,6 +42,72 @@ describe("gitHubViewerPermissions", () => { requestReviewers: false, }); }); + + it.effect("does not assume authorship when the review-thread read fails", () => + Effect.gen(function* () { + const provider = yield* make; + const detail = yield* provider.getChangeRequest({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + }); + + expect(detail.viewerPermissions).toEqual({ + actions: ["ready", "draft", "close", "reopen"], + comment: true, + resolve: false, + verdicts: ["comment", "approve", "request-changes"], + requestReviewers: false, + }); + }).pipe( + Effect.provide( + Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + getPullRequestDetail: () => + Effect.succeed({ + authorId: null, + number: 7, + title: "Pull request 7", + url: "https://github.com/acme/web/pull/7", + author: null, + headBranch: "feat/page", + baseBranch: "main", + state: "open", + isDraft: false, + mergeability: "mergeable", + additions: 1, + deletions: 1, + createdAt: "2026-07-01T00:00:00Z", + updatedAt: "2026-07-02T00:00:00Z", + reviewRequestLogins: [], + hasTeamReviewRequest: false, + labels: [], + body: "", + changedFiles: 1, + mergedAt: null, + closedAt: null, + checks: [], + comments: [], + commits: [], + }), + getRepositoryAccess: () => + Effect.succeed({ + canWrite: false, + mergeCapabilities: { merge: true, squash: true, rebase: true }, + }), + listReviewThreadComments: () => + Effect.fail( + new GitHubPullRequestCli.GitHubPullRequestReadError({ + command: "gh", + cwd: "/w", + operation: "listReviewThreadComments", + cause: new Error("transient GraphQL failure"), + }), + ), + }), + ), + ), + ); }); describe("loginAvatarUrl", () => { diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts index 7202f162c4e..3851a3952fb 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts @@ -214,7 +214,7 @@ export const make = Effect.gen(function* () { avatarsByLogin: new Map(), // A read that never happened says nothing about the reader, and an unknown // permission is granted: the controls stay live and GitHub explains any refusal. - viewer: { canUpdate: true, didAuthor: true }, + viewer: { canUpdate: true, didAuthor: false }, })), ), ], @@ -263,6 +263,9 @@ export const make = Effect.gen(function* () { getDiff: (input) => cli.getPullRequestDiff(input).pipe(Effect.mapError(fail("getDiff"))), + getDiffFileContents: (input) => + cli.getPullRequestDiffFileContents(input).pipe(Effect.mapError(fail("getDiffFileContents"))), + listReviewerCandidates: (input) => cli.listReviewerCandidates(input).pipe(Effect.mapError(fail("listReviewerCandidates"))), diff --git a/apps/server/src/pullRequest/GitLabPullRequestCli.test.ts b/apps/server/src/pullRequest/GitLabPullRequestCli.test.ts index 583d5e28693..9bfad2648c1 100644 --- a/apps/server/src/pullRequest/GitLabPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/GitLabPullRequestCli.test.ts @@ -18,13 +18,14 @@ const layer = it.layer( ), ); -function output(stdout: string) { +function output(stdout: string, stdoutTruncated = false, stdoutInvalidUtf8 = false) { return { exitCode: ChildProcessSpawner.ExitCode(0), stdout, stderr: "", - stdoutTruncated: false, + stdoutTruncated, stderrTruncated: false, + stdoutInvalidUtf8, }; } @@ -117,6 +118,7 @@ layer("GitLabPullRequestCli.layer", (it) => { assert.strictEqual(batch.items.length, 3); assert.isFalse(batch.truncated); + assert.strictEqual(batch.cursorAdvance, 3); const path = argsOfCall(0)[1] ?? ""; expect(path).toContain("projects/acme%2Fweb/merge_requests"); expect(path).toContain("per_page=11"); @@ -170,7 +172,7 @@ layer("GitLabPullRequestCli.layer", (it) => { }), ); - it.effect("carries on from the instant the last slice ended on", () => + it.effect("carries on from the number of rows already delivered", () => Effect.gen(function* () { mockedExecute.mockReturnValueOnce(Effect.succeed(output(mergeRequests(3, 1)))); const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; @@ -185,11 +187,65 @@ layer("GitLabPullRequestCli.layer", (it) => { cursor: { updatedBefore: "2026-07-02T00:00:00Z", delivered: 10 }, }); - // GitLab reads `updated_before` inclusively, so the rows already sent at that instant come - // back for the caller to drop rather than the ones beside them being skipped. + // GitLab's timestamp filter has no tie-breaker, so an offset is what advances through a + // boundary shared by more rows than one page can hold. const path = argsOfCall(0)[1] ?? ""; - expect(path).toContain("updated_before=2026-07-02T00%3A00%3A00Z"); + expect(path).not.toContain("updated_before="); expect(path).toContain("order_by=updated_at"); + expect(path).toContain("per_page=11"); + expect(path).toContain("page=1"); + }), + ); + + it.effect("advances beyond several pages sharing the cursor timestamp", () => + Effect.gen(function* () { + mockedExecute + .mockReturnValueOnce(Effect.succeed(output(mergeRequests(11, 144)))) + .mockReturnValueOnce(Effect.succeed(output(mergeRequests(11, 155)))); + const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + + const batch = yield* cli.listMergeRequests({ + cwd: "/w", + repository: "acme/web", + state: "open", + involvement: "all", + viewer: "bilal", + limit: 10, + cursor: { updatedBefore: "2026-07-02T00:00:00Z", delivered: 150 }, + }); + + expect(argsOfCall(0)[1]).toContain("per_page=11"); + expect(argsOfCall(0)[1]).toContain("page=14"); + expect(argsOfCall(1)[1]).toContain("page=15"); + expect(batch.items.map((item) => item.number)).toEqual([ + 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, + ]); + assert.isTrue(batch.truncated); + }), + ); + + it.effect("advances the cursor through malformed raw rows", () => + Effect.gen(function* () { + // @effect-diagnostics-next-line preferSchemaOverJson:off + const rows = JSON.parse(mergeRequests(2, 1)) as ReadonlyArray; + mockedExecute.mockReturnValueOnce( + // @effect-diagnostics-next-line preferSchemaOverJson:off + Effect.succeed(output(JSON.stringify([{ iid: "malformed" }, ...rows]))), + ); + const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + + const batch = yield* cli.listMergeRequests({ + cwd: "/w", + repository: "acme/web", + state: "open", + involvement: "all", + viewer: "bilal", + limit: 2, + }); + + expect(batch.items.map((item) => item.number)).toEqual([1, 2]); + assert.strictEqual(batch.cursorAdvance, 3); + assert.isTrue(batch.truncated); }), ); @@ -505,6 +561,167 @@ layer("GitLabPullRequestCli.layer", (it) => { }), ); + it.effect("reports a commit with no parent as a structured error", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce( + // @effect-diagnostics-next-line preferSchemaOverJson:off + Effect.succeed(output(JSON.stringify({ id: "a1b2c3d", parent_ids: [] }))), + ); + const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + + const error = yield* Effect.flip( + cli.getMergeRequestDiffFileContents({ + cwd: "/w", + repository: "acme/web", + number: 7, + commit: "a1b2c3d", + changeType: "change", + oldPath: "src/a.ts", + newPath: "src/a.ts", + }), + ); + + assert.strictEqual(error._tag, "GitLabDiffCommitParentUnavailableError"); + if (error._tag === "GitLabDiffCommitParentUnavailableError") { + assert.strictEqual(error.commit, "a1b2c3d"); + } + }), + ); + + it.effect("expands a new file from a root commit without requiring a parent", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce( + // @effect-diagnostics-next-line preferSchemaOverJson:off + Effect.succeed(output(JSON.stringify({ id: "a1b2c3d", parent_ids: [] }))), + ); + mockedExecute.mockReturnValueOnce(Effect.succeed(output("first contents\n"))); + const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + + const contents = yield* cli.getMergeRequestDiffFileContents({ + cwd: "/w", + repository: "acme/web", + number: 7, + commit: "a1b2c3d", + changeType: "new", + oldPath: "src/first.ts", + newPath: "src/first.ts", + }); + + expect(contents).toEqual({ oldContents: "", newContents: "first contents\n" }); + expect(argsOfCall(1)[1]).toContain("raw?ref=a1b2c3d"); + }), + ); + + it.effect("reports an oversized diff file with its path and reason", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ + diff_refs: { + base_sha: "a1b2c3d", + head_sha: "b1c2d3e", + start_sha: "a1b2c3d", + }, + }), + ), + ), + ); + mockedExecute.mockReturnValueOnce(Effect.succeed(output("partial", true))); + const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + + const error = yield* Effect.flip( + cli.getMergeRequestDiffFileContents({ + cwd: "/w", + repository: "acme/web", + number: 7, + changeType: "deleted", + oldPath: "src/large.ts", + newPath: "src/large.ts", + }), + ); + + assert.strictEqual(error._tag, "GitLabDiffFileContentsUnavailableError"); + if (error._tag === "GitLabDiffFileContentsUnavailableError") { + assert.strictEqual(error.path, "src/large.ts"); + assert.strictEqual(error.reason, "oversized"); + } + }), + ); + + it.effect("reports undecodable diff file contents as binary", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ + diff_refs: { + base_sha: "a1b2c3d", + head_sha: "b1c2d3e", + start_sha: "a1b2c3d", + }, + }), + ), + ), + ); + mockedExecute.mockReturnValueOnce( + Effect.succeed(output("binary\uFFFDcontents", false, true)), + ); + const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + + const error = yield* Effect.flip( + cli.getMergeRequestDiffFileContents({ + cwd: "/w", + repository: "acme/web", + number: 7, + changeType: "deleted", + oldPath: "assets/logo.png", + newPath: "assets/logo.png", + }), + ); + + assert.strictEqual(error._tag, "GitLabDiffFileContentsUnavailableError"); + if (error._tag === "GitLabDiffFileContentsUnavailableError") { + assert.strictEqual(error.path, "assets/logo.png"); + assert.strictEqual(error.reason, "binary"); + } + }), + ); + + it.effect("returns valid text containing a literal replacement character", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ + diff_refs: { + base_sha: "a1b2c3d", + head_sha: "b1c2d3e", + start_sha: "a1b2c3d", + }, + }), + ), + ), + ); + mockedExecute.mockReturnValueOnce(Effect.succeed(output("before\uFFFDafter"))); + const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + + const contents = yield* cli.getMergeRequestDiffFileContents({ + cwd: "/w", + repository: "acme/web", + number: 7, + changeType: "deleted", + oldPath: "docs/encoding.md", + newPath: "docs/encoding.md", + }); + + assert.strictEqual(contents.oldContents, "before\uFFFDafter"); + }), + ); + it.effect("ends the diff on a page with no files rather than asking for it again", () => Effect.gen(function* () { mockedExecute.mockReturnValueOnce(Effect.succeed(output("[]"))); diff --git a/apps/server/src/pullRequest/GitLabPullRequestCli.ts b/apps/server/src/pullRequest/GitLabPullRequestCli.ts index a7c968c80cb..2335d155edf 100644 --- a/apps/server/src/pullRequest/GitLabPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitLabPullRequestCli.ts @@ -19,6 +19,7 @@ import type { import * as GitLabCli from "../sourceControl/GitLabCli.ts"; import { + decodeCommitDiffRefsJson, decodeCommitsJson, decodeDiffRefsJson, decodeDiscussionsJson, @@ -128,11 +129,52 @@ export class GitLabDiffCommitError extends Schema.TaggedErrorClass()( + "GitLabDiffCommitParentUnavailableError", + { + command: Schema.Literal("glab"), + cwd: Schema.String, + commit: Schema.String, + }, +) { + get detail(): string { + return `Commit ${this.commit} reported no parent revision.`; + } + + override get message(): string { + return `GitLab CLI failed in getMergeRequestDiffFileContents: ${this.detail}`; + } +} + +/** A blob exists, but expanding it would be unsafe or would not produce text. */ +export class GitLabDiffFileContentsUnavailableError extends Schema.TaggedErrorClass()( + "GitLabDiffFileContentsUnavailableError", + { + command: Schema.Literal("glab"), + cwd: Schema.String, + path: Schema.String, + reason: Schema.Literals(["oversized", "binary"]), + }, +) { + get detail(): string { + return this.reason === "oversized" + ? `The diff file '${this.path}' exceeds the 1 MB expansion limit.` + : `The diff file '${this.path}' is binary.`; + } + + override get message(): string { + return `GitLab CLI failed in getMergeRequestDiffFileContents: ${this.detail}`; + } +} + export type GitLabPullRequestCliError = | GitLabCli.GitLabCliError | GitLabMergeRequestReadError | GitLabDiffCursorError | GitLabDiffCommitError + | GitLabDiffCommitParentUnavailableError + | GitLabDiffFileContentsUnavailableError | GitLabDiffRefsUnavailableError | GitLabViewerUnavailableError; @@ -148,10 +190,13 @@ const COMMIT_PAGE_SIZE = 100; const CONVERSATION_PAGES = 10; const DIFF_MAX_OUTPUT_BYTES = 8 * 1024 * 1024; const DIFF_TIMEOUT_MS = 60_000; +const DIFF_FILE_MAX_OUTPUT_BYTES = 1024 * 1024; export interface GitLabMergeRequestListBatch { readonly items: ReadonlyArray; readonly truncated: boolean; + /** Raw GitLab rows consumed to produce this page, including malformed rows. */ + readonly cursorAdvance: number; } export interface GitLabMergeRequestDiffSlice { @@ -178,7 +223,7 @@ export class GitLabPullRequestCli extends Context.Service< readonly limit: number; /** Free text for GitLab's own `search`, which matches title and description. */ readonly query?: string | undefined; - /** Where to carry on from, as GitLab's own `updated_before`. */ + /** Where to carry on from in GitLab's stable update-ordered row set. */ readonly cursor?: ProviderListCursor | undefined; }) => Effect.Effect; @@ -213,6 +258,19 @@ export class GitLabPullRequestCli extends Context.Service< readonly commit?: string | undefined; }) => Effect.Effect; + readonly getMergeRequestDiffFileContents: (input: { + readonly cwd: string; + readonly repository: string; + readonly number: number; + readonly commit?: string | undefined; + readonly changeType: "change" | "rename-pure" | "rename-changed" | "new" | "deleted"; + readonly oldPath: string; + readonly newPath: string; + }) => Effect.Effect< + { readonly oldContents: string; readonly newContents: string }, + GitLabPullRequestCliError + >; + readonly getProjectMergeCapabilities: (input: { readonly cwd: string; readonly repository: string; @@ -412,11 +470,19 @@ export const make = Effect.gen(function* () { readonly cursor?: ProviderListCursor | undefined; readonly page: number; readonly collected: ReadonlyArray; + readonly cursorAdvance: number; }): Effect.Effect => { - // Fixed across the walk: GitLab pages by offset, so a page size that changed between - // requests would skip or repeat rows. One row over the limit probes for a next page. + // A continuation uses GitLab's offset pagination. Its timestamp filter is inclusive and has + // no tie-breaker, so a page where many rows share the boundary would otherwise return the + // same prefix forever. `delivered` is the stable offset the service has already handed over. + const delivered = input.cursor?.delivered ?? 0; const perPage = Math.min(input.limit + 1, MAX_PAGE_SIZE); - const lastPage = Math.ceil((input.limit + 1) / perPage); + const firstPage = Math.floor(delivered / perPage) + 1; + const skipOnFirstPage = input.page === firstPage ? delivered % perPage : 0; + // A page made entirely of malformed rows has no item from which the service can build a + // continuation. Bound the walk to the raw span this request asked for rather than recursing + // forever on a host that keeps returning full unusable pages. + const lastPage = Math.floor((delivered + input.limit) / perPage) + 1; return api({ cwd: input.cwd, path: `projects/${projectPath(input.repository)}/merge_requests?${query([ @@ -427,12 +493,6 @@ export const make = Effect.gen(function* () { // matches title and description, and travels URL-encoded like every other value here, // so no text in it can become a parameter of its own. ...searchParams(input.query), - // The instant the last slice ended on, which GitLab reads inclusively — so the rows - // already sent at it come back and the caller drops them, rather than the ones beside - // them being lost to a strictly-older read. - ...(input.cursor === undefined - ? [] - : [["updated_before", input.cursor.updatedBefore] as const]), ["order_by", "updated_at"], ["sort", "desc"], ["per_page", String(perPage)], @@ -442,7 +502,11 @@ export const make = Effect.gen(function* () { Effect.flatMap((result) => { const raw = result.stdout.trim(); if (raw.length === 0) { - return Effect.succeed({ items: input.collected, truncated: false }); + return Effect.succeed({ + items: input.collected, + truncated: false, + cursorAdvance: input.cursorAdvance, + }); } const decoded = decodeMergeRequestListJson(raw); if (!Result.isSuccess(decoded)) { @@ -455,18 +519,50 @@ export const make = Effect.gen(function* () { }), ); } - const collected = [...input.collected, ...decoded.success.items]; + const pageItems: GitLabMergeRequestListItem[] = []; + const pageRawIndexes: number[] = []; + for (const [index, item] of decoded.success.items.entries()) { + const rawIndex = decoded.success.rawIndexes[index]!; + if (rawIndex < skipOnFirstPage) continue; + pageItems.push(item); + pageRawIndexes.push(rawIndex); + } + const remaining = input.limit - input.collected.length; + const lastItemRawIndex = pageRawIndexes[remaining - 1]; + if (lastItemRawIndex !== undefined) { + const consumed = lastItemRawIndex + 1 - skipOnFirstPage; + return Effect.succeed({ + items: [...input.collected, ...pageItems.slice(0, remaining)], + truncated: + lastItemRawIndex + 1 < decoded.success.rawCount || + decoded.success.rawCount === perPage, + cursorAdvance: input.cursorAdvance + consumed, + }); + } + const collected = [...input.collected, ...pageItems]; + const consumed = Math.max(0, decoded.success.rawCount - skipOnFirstPage); // Counted before decoding, so a skipped malformed row cannot end paging early. const exhausted = decoded.success.rawCount < perPage; - if (exhausted || collected.length > input.limit || input.page >= lastPage) { + if (exhausted) { return Effect.succeed({ - items: collected.slice(0, input.limit), - // Anything but a short final page means GitLab may still have rows, including the - // case where enough rows failed to decode to keep the collected count down. - truncated: !exhausted || collected.length > input.limit, + items: collected, + truncated: false, + cursorAdvance: input.cursorAdvance + consumed, }); } - return listPage({ ...input, page: input.page + 1, collected }); + if (input.page >= lastPage) { + return Effect.succeed({ + items: collected, + truncated: true, + cursorAdvance: input.cursorAdvance + consumed, + }); + } + return listPage({ + ...input, + page: input.page + 1, + collected, + cursorAdvance: input.cursorAdvance + consumed, + }); }), ); }; @@ -672,6 +768,46 @@ export const make = Effect.gen(function* () { }), ); + const getCommitDiffRefs = (input: { + readonly cwd: string; + readonly repository: string; + readonly commit: string; + readonly allowRoot: boolean; + }): Effect.Effect => + api({ + cwd: input.cwd, + path: `projects/${projectPath(input.repository)}/repository/commits/${input.commit}`, + }).pipe( + Effect.flatMap((result): Effect.Effect => { + const decoded = decodeCommitDiffRefsJson(result.stdout.trim()); + if (!Result.isSuccess(decoded)) { + return Effect.fail( + new GitLabMergeRequestReadError({ + command: "glab", + cwd: input.cwd, + operation: "getMergeRequestDiffFileContents", + cause: decoded.failure, + }), + ); + } + return decoded.success === null + ? input.allowRoot + ? Effect.succeed({ + baseSha: "", + headSha: input.commit, + startSha: "", + }) + : Effect.fail( + new GitLabDiffCommitParentUnavailableError({ + command: "glab", + cwd: input.cwd, + commit: input.commit, + }), + ) + : Effect.succeed(decoded.success); + }), + ); + /** * The merge request itself, which several calls need for different parts of it: the detail for * everything, and the reviewer paths for the ids GitLab writes a reviewer set with. @@ -747,7 +883,11 @@ export const make = Effect.gen(function* () { }), ), - listMergeRequests: (input) => listPage({ ...input, page: 1, collected: [] }), + listMergeRequests: (input) => { + const perPage = Math.min(input.limit + 1, MAX_PAGE_SIZE); + const page = Math.floor((input.cursor?.delivered ?? 0) / perPage) + 1; + return listPage({ ...input, page, collected: [], cursorAdvance: 0 }); + }, getMergeRequestDetail: mergeRequestDetail, @@ -794,6 +934,57 @@ export const make = Effect.gen(function* () { : diffPage({ ...target, page }); }, + getMergeRequestDiffFileContents: (input) => + Effect.gen(function* () { + if (input.commit !== undefined && !isCommitSha(input.commit)) { + return yield* Effect.fail(new GitLabDiffCommitError({ command: "glab", cwd: input.cwd })); + } + const refs = yield* input.commit === undefined + ? getDiffRefs(input) + : getCommitDiffRefs({ + cwd: input.cwd, + repository: input.repository, + commit: input.commit, + allowRoot: input.changeType === "new", + }); + + const readFile = (revision: string, filePath: string) => + api({ + cwd: input.cwd, + path: `projects/${projectPath(input.repository)}/repository/files/${encodeURIComponent( + filePath, + )}/raw?ref=${encodeURIComponent(revision)}`, + maxOutputBytes: DIFF_FILE_MAX_OUTPUT_BYTES, + timeoutMs: DIFF_TIMEOUT_MS, + }).pipe( + Effect.flatMap((result) => + result.stdoutTruncated || + result.stdout.includes("\0") || + result.stdoutInvalidUtf8 === true + ? Effect.fail( + new GitLabDiffFileContentsUnavailableError({ + command: "glab", + cwd: input.cwd, + path: filePath, + reason: result.stdoutTruncated ? "oversized" : "binary", + }), + ) + : Effect.succeed(result.stdout), + ), + ); + + const [oldContents, newContents] = yield* Effect.all( + [ + input.changeType === "new" ? Effect.succeed("") : readFile(refs.baseSha, input.oldPath), + input.changeType === "deleted" + ? Effect.succeed("") + : readFile(refs.headSha, input.newPath), + ], + { concurrency: 2 }, + ); + return { oldContents, newContents }; + }), + getProjectMergeCapabilities: (input) => api({ cwd: input.cwd, diff --git a/apps/server/src/pullRequest/PullRequestProvider.ts b/apps/server/src/pullRequest/PullRequestProvider.ts index 127ca6b4e87..5e4fc82085e 100644 --- a/apps/server/src/pullRequest/PullRequestProvider.ts +++ b/apps/server/src/pullRequest/PullRequestProvider.ts @@ -70,6 +70,11 @@ export interface ProviderChangeRequestPage { readonly items: ReadonlyArray; /** True when the host has more rows than the page size asked for. */ readonly truncated: boolean; + /** + * Optional count-based cursor advance. Most hosts advance by the rows delivered after local + * de-duplication; an offset-paged host may need to count malformed raw rows it consumed too. + */ + readonly cursorAdvance?: number; /** * This page can be carried on from, so the service may hand the caller a cursor for it. False * where the host answered in an order a cursor means nothing in, which leaves a larger `limit` @@ -93,8 +98,9 @@ export interface ProviderListCursor { */ readonly updatedBefore: string; /** - * How many rows this repository has handed over so far, for a host that carries on by counting - * rather than by date. + * How many provider rows this repository has consumed so far, for a host that carries on by + * counting rather than by date. Usually this is the number handed over; malformed raw rows may + * count too when the provider reports a `cursorAdvance`. */ readonly delivered: number; } @@ -157,6 +163,11 @@ export interface ProviderDiffSlice { readonly nextCursor: string | null; } +export interface ProviderDiffFileContents { + readonly oldContents: string; + readonly newContents: string; +} + export interface ProviderRepositoryRef { readonly cwd: string; /** Provider-native repository identity, e.g. `owner/repo` or `group/subgroup/project`. */ @@ -276,6 +287,20 @@ export interface PullRequestProviderApi { }, ) => Effect.Effect; + /** + * Full files at the exact revisions the host used for its patch. Optional where the provider + * exposes no diff at all; the service refuses expansion there just as it refuses the patch. + */ + readonly getDiffFileContents?: ( + input: ProviderRepositoryRef & { + readonly number: number; + readonly commit?: string | undefined; + readonly changeType: "change" | "rename-pure" | "rename-changed" | "new" | "deleted"; + readonly oldPath: string; + readonly newPath: string; + }, + ) => Effect.Effect; + readonly runAction: ( input: ProviderRepositoryRef & { readonly number: number; diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 248b7e6e0a5..eb28d165010 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -296,6 +296,40 @@ it.effect("offers no continuation for a host that cannot be carried on from", () }), ); +it.effect("uses a provider's raw cursor advance when it consumed malformed rows", () => + Effect.gen(function* () { + const service = yield* makeService({ + projects: [ + project({ + id: "p1", + title: "web", + workspaceRoot: "/a", + repository: "acme/web", + provider: "azure-devops", + host: "dev.azure.com", + }), + ], + providers: [ + fakeProvider("azure-devops", { + listChangeRequests: () => + Effect.succeed({ + items: [changeRequest(7, "2026-07-02T00:00:00Z")], + truncated: true, + cursorAdvance: 4, + continues: true, + }), + }), + ], + }); + + const result = yield* service.list({ state: "open" }); + + assert.deepStrictEqual(result.nextCursors, { + "dev.azure.com acme/web": "2026-07-02T00:00:00Z|4|7", + }); + }), +); + it.effect("reads only the repositories it was asked to carry on with", () => Effect.gen(function* () { const listed: string[] = []; diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 612784dc2a5..bd1158d24c8 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -14,6 +14,8 @@ import { type PullRequestActionInput, type PullRequestCommentInput, type PullRequestDetail, + type PullRequestDiffFileContentsInput, + type PullRequestDiffFileContentsResult, type PullRequestDiffStat, type PullRequestDiffInput, type PullRequestDiffResult, @@ -104,6 +106,9 @@ export class PullRequestService extends Context.Service< readonly diff: ( input: PullRequestDiffInput, ) => Effect.Effect; + readonly diffFileContents: ( + input: PullRequestDiffFileContentsInput, + ) => Effect.Effect; readonly runAction: (input: PullRequestActionInput) => Effect.Effect; readonly comment: (input: PullRequestCommentInput) => Effect.Effect; readonly submitReview: ( @@ -247,6 +252,8 @@ function nextListCursor( fetched: ReadonlyArray, /** What is being sent on, which is what the count of delivered rows is about. */ delivered: ReadonlyArray, + /** A provider may consume malformed offset-paged rows that never appear in `delivered`. */ + cursorAdvance = delivered.length, ): string | null { // The host had nothing at all, so there is no row to carry on from — and repeating the cursor // that produced the empty slice would ask the same question forever. @@ -256,7 +263,7 @@ function nextListCursor( // repository's boring afternoon — and reading "nothing new" as "nothing left" would end the // walk on the instant it was stuck on, with everything older unreachable for good. const oldest = fetched.reduce((left, right) => (right.updatedAt < left.updatedAt ? right : left)); - return listCursorAt(previous, oldest.updatedAt, fetched, delivered.length); + return listCursorAt(previous, oldest.updatedAt, fetched, cursorAdvance); } /** @@ -663,7 +670,7 @@ export const make = Effect.gen(function* () { truncated: page.truncated, nextCursor: page.continues && page.truncated - ? nextListCursor(cursor, page.items, items) + ? nextListCursor(cursor, page.items, items, page.cursorAdvance) : null, }; }), @@ -895,6 +902,30 @@ export const make = Effect.gen(function* () { ), ); + const diffFileContents: PullRequestService["Service"]["diffFileContents"] = (input) => + requireProject(input).pipe( + Effect.flatMap((project) => { + const read = project.api.getDiffFileContents; + return project.api.capabilities.diff && read + ? read({ + cwd: project.project.workspaceRoot, + repository: project.repository, + host: project.host, + number: input.number, + ...(input.commit === undefined ? {} : { commit: input.commit }), + changeType: input.changeType, + oldPath: input.oldPath, + newPath: input.newPath, + }).pipe(Effect.mapError(toPullRequestError("diffFileContents"))) + : Effect.fail( + new PullRequestOperationError({ + operation: "diffFileContents", + detail: "This host cannot expand unchanged pull request lines.", + }), + ); + }), + ); + const runAction: PullRequestService["Service"]["runAction"] = (input) => requireProject(input).pipe( Effect.flatMap((project): Effect.Effect => { @@ -1453,6 +1484,7 @@ export const make = Effect.gen(function* () { listStats, detail, diff, + diffFileContents, runAction: invalidatedByMutation(runAction), comment: invalidatedByMutation(comment), submitReview: invalidatedByMutation(submitReview), diff --git a/apps/server/src/pullRequest/azureDevOpsPullRequestJson.test.ts b/apps/server/src/pullRequest/azureDevOpsPullRequestJson.test.ts index 34ea974f3af..3ac55cde1e8 100644 --- a/apps/server/src/pullRequest/azureDevOpsPullRequestJson.test.ts +++ b/apps/server/src/pullRequest/azureDevOpsPullRequestJson.test.ts @@ -122,6 +122,7 @@ describe("decodePullRequestListJson", () => { expect(batch.items).toHaveLength(1); expect(batch.rawCount).toBe(2); + expect(batch.rawIndexes).toEqual([1]); }); }); diff --git a/apps/server/src/pullRequest/azureDevOpsPullRequestJson.ts b/apps/server/src/pullRequest/azureDevOpsPullRequestJson.ts index 360b8003984..a51eef4f0ce 100644 --- a/apps/server/src/pullRequest/azureDevOpsPullRequestJson.ts +++ b/apps/server/src/pullRequest/azureDevOpsPullRequestJson.ts @@ -237,6 +237,8 @@ type DecodeFailure = Cause.Cause; export interface AzureDevOpsPullRequestBatch { readonly items: ReadonlyArray; + /** Zero-based positions of the decoded items in Azure's raw page. */ + readonly rawIndexes: ReadonlyArray; /** Rows Azure returned, counted before decoding, so a skipped row cannot hide a next page. */ readonly rawCount: number; } @@ -250,13 +252,17 @@ export function decodePullRequestListJson( return Result.fail(decoded.failure); } const items: AzureDevOpsPullRequest[] = []; - for (const entry of decoded.success) { + const rawIndexes: number[] = []; + for (const [rawIndex, entry] of decoded.success.entries()) { const item = decodePullRequestEntry(entry); if (Exit.isFailure(item)) continue; const pullRequest = toPullRequest(item.value); - if (pullRequest !== null) items.push(pullRequest); + if (pullRequest !== null) { + items.push(pullRequest); + rawIndexes.push(rawIndex); + } } - return Result.succeed({ items, rawCount: decoded.success.length }); + return Result.succeed({ items, rawIndexes, rawCount: decoded.success.length }); } /** Null carries "Azure answered, but with too little to use", which the caller reports. */ diff --git a/apps/server/src/pullRequest/bitbucketPullRequestJson.test.ts b/apps/server/src/pullRequest/bitbucketPullRequestJson.test.ts index 9ecc8fc0099..6e119d4d4ce 100644 --- a/apps/server/src/pullRequest/bitbucketPullRequestJson.test.ts +++ b/apps/server/src/pullRequest/bitbucketPullRequestJson.test.ts @@ -245,8 +245,9 @@ describe("decodeCommitsJson", () => { ), ); - expect(decoded.map((commit) => commit.oid)).toEqual(["aaa", "bbb"]); - expect(decoded[1]?.messageHeadline).toBe("second"); + expect(decoded.items.map((commit) => commit.oid)).toEqual(["aaa", "bbb"]); + expect(decoded.items[1]?.messageHeadline).toBe("second"); + expect(decoded.next).toBeNull(); }); }); @@ -266,14 +267,17 @@ describe("decodeStatusesJson", () => { ), ); - expect(decoded).toEqual([ - { - name: "Pipeline - custom: check-version-and-pr", - status: "success", - description: null, - url: "https://bitbucket.org/acme/web/pipelines/results/8126", - }, - ]); + expect(decoded).toEqual({ + items: [ + { + name: "Pipeline - custom: check-version-and-pr", + status: "success", + description: null, + url: "https://bitbucket.org/acme/web/pipelines/results/8126", + }, + ], + next: null, + }); }); it.each([ @@ -285,7 +289,7 @@ describe("decodeStatusesJson", () => { ])("reads the %s build state as %s", (state, expected) => { const decoded = expectSuccess(decodeStatusesJson(page([{ name: "Pipeline", state }]))); - expect(decoded[0]?.status).toBe(expected); + expect(decoded.items[0]?.status).toBe(expected); }); }); @@ -300,7 +304,7 @@ describe("decodeDiffstatJson", () => { ), ); - expect(decoded).toEqual({ additions: 41, deletions: 16, changedFiles: 2 }); + expect(decoded).toEqual({ additions: 41, deletions: 16, changedFiles: 2, next: null }); }); }); diff --git a/apps/server/src/pullRequest/bitbucketPullRequestJson.ts b/apps/server/src/pullRequest/bitbucketPullRequestJson.ts index e48b41aa2b5..c6c39ca4b81 100644 --- a/apps/server/src/pullRequest/bitbucketPullRequestJson.ts +++ b/apps/server/src/pullRequest/bitbucketPullRequestJson.ts @@ -502,7 +502,7 @@ export function decodeCommentsJson(raw: string): Result.Result, DecodeFailure> { +): Result.Result, DecodeFailure> { const decoded = decodePage(raw); if (!Result.isSuccess(decoded)) { return Result.fail(decoded.failure); @@ -521,12 +521,12 @@ export function decodeCommitsJson( }); } // Bitbucket lists a pull request's commits newest first; the timeline reads oldest first. - return Result.succeed(commits.toReversed()); + return Result.succeed({ items: commits.toReversed(), next: trimmed(decoded.success.next) }); } export function decodeStatusesJson( raw: string, -): Result.Result, DecodeFailure> { +): Result.Result, DecodeFailure> { const decoded = decodePage(raw); if (!Result.isSuccess(decoded)) { return Result.fail(decoded.failure); @@ -545,7 +545,7 @@ export function decodeStatusesJson( url: trimmed(status.url), }); } - return Result.succeed(checks); + return Result.succeed({ items: checks, next: trimmed(decoded.success.next) }); } export interface BitbucketDiffStat { @@ -554,8 +554,14 @@ export interface BitbucketDiffStat { readonly changedFiles: number; } +export interface BitbucketDiffStatPage extends BitbucketDiffStat { + readonly next: string | null; +} + /** One entry per changed file, each carrying that file's line counts. */ -export function decodeDiffstatJson(raw: string): Result.Result { +export function decodeDiffstatJson( + raw: string, +): Result.Result { const decoded = decodePage(raw); if (!Result.isSuccess(decoded)) { return Result.fail(decoded.failure); @@ -570,7 +576,12 @@ export function decodeDiffstatJson(raw: string): Result.Result; + /** At least one outstanding request targets a team rather than an individual login. */ + readonly hasTeamReviewRequest: boolean; readonly labels: ReadonlyArray; } @@ -645,9 +647,8 @@ function toLabels( } /** - * User review requests only. A team request carries a slug, and the viewer check compares - * these against a login, so keeping slugs here would let an unrelated team read as the - * viewer. Team-routed requests need GitHub's review-requested search to resolve. + * User review requests only. Team requests are tracked separately because a slug cannot be + * compared with the viewer's login. */ function toReviewRequestLogins( raw: ReadonlyArray> | undefined, @@ -658,6 +659,16 @@ function toReviewRequestLogins( }); } +function hasTeamReviewRequest( + raw: ReadonlyArray> | undefined, +): boolean { + return (raw ?? []).some( + (request) => + trimmed(request.login) === null && + (trimmed(request.slug) !== null || trimmed(request.name) !== null), + ); +} + function toCheckStatus(raw: Schema.Schema.Type): PullRequestCheckStatus { // Commit statuses report a single `state`; check runs report `status` plus a `conclusion` // that only exists once the run has completed. @@ -779,6 +790,7 @@ function toListItem(raw: Schema.Schema.Type): GitHubPu createdAt: raw.createdAt, updatedAt: raw.updatedAt, reviewRequestLogins: toReviewRequestLogins(raw.reviewRequests), + hasTeamReviewRequest: hasTeamReviewRequest(raw.reviewRequests), labels: toLabels(raw.labels), }; } diff --git a/apps/server/src/pullRequest/gitLabMergeRequestJson.test.ts b/apps/server/src/pullRequest/gitLabMergeRequestJson.test.ts index 7ab212fb7e7..22d0866787b 100644 --- a/apps/server/src/pullRequest/gitLabMergeRequestJson.test.ts +++ b/apps/server/src/pullRequest/gitLabMergeRequestJson.test.ts @@ -128,6 +128,7 @@ describe("decodeMergeRequestListJson", () => { ); expect(batch.items).toHaveLength(1); + expect(batch.rawIndexes).toEqual([1]); expect(batch.rawCount).toBe(2); }); }); diff --git a/apps/server/src/pullRequest/gitLabMergeRequestJson.ts b/apps/server/src/pullRequest/gitLabMergeRequestJson.ts index 34cd51f000f..90f35a6c54f 100644 --- a/apps/server/src/pullRequest/gitLabMergeRequestJson.ts +++ b/apps/server/src/pullRequest/gitLabMergeRequestJson.ts @@ -137,6 +137,7 @@ const RawCommitSchema = Schema.Struct({ title: Schema.optional(Schema.NullOr(Schema.String)), committed_date: Schema.optional(Schema.NullOr(Schema.String)), created_at: Schema.optional(Schema.NullOr(Schema.String)), + parent_ids: Schema.optional(Schema.Array(Schema.String)), }); const RawDiffSchema = Schema.Struct({ @@ -347,6 +348,7 @@ const decodeMergeRequest = decodeJsonResult(RawMergeRequestSchema); const decodeNoteEntry = Schema.decodeUnknownExit(RawNoteSchema); const decodeUserEntry = Schema.decodeUnknownExit(RawUserSchema); const decodeCommitEntry = Schema.decodeUnknownExit(RawCommitSchema); +const decodeCommit = decodeJsonResult(RawCommitSchema); const decodeDiffEntry = Schema.decodeUnknownExit(RawDiffSchema); const decodeDiscussionEntry = Schema.decodeUnknownExit(RawDiscussionSchema); const decodeDiffRefs = decodeJsonResult(RawDiffRefsSchema); @@ -363,6 +365,8 @@ export interface GitLabProjectUsers { export interface GitLabMergeRequestListBatch { readonly items: ReadonlyArray; + /** Zero-based positions of the decoded items in GitLab's raw page. */ + readonly rawIndexes: ReadonlyArray; /** Rows GitLab returned, counted before decoding, so a skipped row cannot hide a next page. */ readonly rawCount: number; } @@ -377,13 +381,15 @@ export function decodeMergeRequestListJson( return Result.fail(decoded.failure); } const items: GitLabMergeRequestListItem[] = []; - for (const entry of decoded.success) { + const rawIndexes: number[] = []; + for (const [rawIndex, entry] of decoded.success.entries()) { const item = decodeMergeRequestEntry(entry); if (Exit.isSuccess(item)) { items.push(toListItem(item.value)); + rawIndexes.push(rawIndex); } } - return Result.succeed({ items, rawCount: decoded.success.length }); + return Result.succeed({ items, rawIndexes, rawCount: decoded.success.length }); } export function decodeMergeRequestDetailJson( @@ -594,6 +600,19 @@ export function decodeCommitsJson( return Result.succeed(commits.toReversed()); } +/** The exact comparison GitLab uses for a commit-scoped diff. */ +export function decodeCommitDiffRefsJson( + raw: string, +): Result.Result { + const decoded = decodeCommit(raw); + if (!Result.isSuccess(decoded)) return Result.fail(decoded.failure); + const baseSha = trimmed(decoded.success.parent_ids?.[0]); + const headSha = trimmed(decoded.success.id); + return Result.succeed( + baseSha === null || headSha === null ? null : { baseSha, headSha, startSha: baseSha }, + ); +} + function diffHeaderPaths(raw: Schema.Schema.Type): { readonly from: string; readonly to: string; diff --git a/apps/server/src/stream/collectUint8StreamText.test.ts b/apps/server/src/stream/collectUint8StreamText.test.ts index d6715294cce..4a41cf11ec6 100644 --- a/apps/server/src/stream/collectUint8StreamText.test.ts +++ b/apps/server/src/stream/collectUint8StreamText.test.ts @@ -17,6 +17,7 @@ describe("collectUint8StreamText", () => { text: "hello world", bytes: 11, truncated: false, + invalidUtf8: false, }); }), ); @@ -33,7 +34,24 @@ describe("collectUint8StreamText", () => { text: "abcde[truncated]", bytes: 5, truncated: true, + invalidUtf8: false, }); }), ); + + it.effect("reports invalid UTF-8 separately from a literal replacement character", () => + Effect.gen(function* () { + const invalid = yield* collectUint8StreamText({ + stream: Stream.make(new Uint8Array([0x66, 0x80, 0x6f])), + }); + const literal = yield* collectUint8StreamText({ + stream: Stream.make(encoder.encode("before\uFFFDafter")), + }); + + assert.strictEqual(invalid.invalidUtf8, true); + assert.strictEqual(invalid.text, "f\uFFFDo"); + assert.strictEqual(literal.invalidUtf8, false); + assert.strictEqual(literal.text, "before\uFFFDafter"); + }), + ); }); diff --git a/apps/server/src/stream/collectUint8StreamText.ts b/apps/server/src/stream/collectUint8StreamText.ts index 7ac5530474e..71114e1de1b 100644 --- a/apps/server/src/stream/collectUint8StreamText.ts +++ b/apps/server/src/stream/collectUint8StreamText.ts @@ -1,12 +1,21 @@ import * as Effect from "effect/Effect"; import * as Stream from "effect/Stream"; +import * as NodeBuffer from "node:buffer"; export interface CollectedUint8StreamText { readonly text: string; readonly truncated: boolean; readonly bytes: number; + readonly invalidUtf8: boolean; } +export const decodeUtf8 = ( + bytes: Uint8Array, +): Pick => ({ + text: Buffer.from(bytes).toString("utf8"), + invalidUtf8: !NodeBuffer.isUtf8(bytes), +}); + interface CollectState { chunks: Uint8Array[]; readonly bytes: number; @@ -59,11 +68,15 @@ export const collectUint8StreamText = (input: { }, ), Effect.map((state): CollectedUint8StreamText => { - const text = Buffer.concat(state.chunks, state.bytes).toString("utf8"); + const decoded = decodeUtf8(Buffer.concat(state.chunks, state.bytes)); return { - text: state.truncated && truncatedMarker.length > 0 ? `${text}${truncatedMarker}` : text, + text: + state.truncated && truncatedMarker.length > 0 + ? `${decoded.text}${truncatedMarker}` + : decoded.text, bytes: state.bytes, truncated: state.truncated, + invalidUtf8: decoded.invalidUtf8, }; }), ); diff --git a/apps/server/src/vcs/VcsProcess.test.ts b/apps/server/src/vcs/VcsProcess.test.ts index 675d20cb82c..d202fa48ca4 100644 --- a/apps/server/src/vcs/VcsProcess.test.ts +++ b/apps/server/src/vcs/VcsProcess.test.ts @@ -192,6 +192,8 @@ describe("VcsProcess.run", () => { timedOut: false, stdoutTruncated: false, stderrTruncated: false, + stdoutInvalidUtf8: false, + stderrInvalidUtf8: false, }), ); diff --git a/apps/server/src/vcs/VcsProcess.ts b/apps/server/src/vcs/VcsProcess.ts index 52db6f9b1fb..299990e56ea 100644 --- a/apps/server/src/vcs/VcsProcess.ts +++ b/apps/server/src/vcs/VcsProcess.ts @@ -37,6 +37,9 @@ export interface VcsProcessOutput { readonly stderr: string; readonly stdoutTruncated: boolean; readonly stderrTruncated: boolean; + /** Present on real process output; optional so narrow test doubles remain lightweight. */ + readonly stdoutInvalidUtf8?: boolean; + readonly stderrInvalidUtf8?: boolean; } export class VcsProcess extends Context.Service< @@ -163,6 +166,8 @@ export const make = Effect.gen(function* () { stderr: result.stderr, stdoutTruncated: result.stdoutTruncated, stderrTruncated: result.stderrTruncated, + stdoutInvalidUtf8: result.stdoutInvalidUtf8 ?? false, + stderrInvalidUtf8: result.stderrInvalidUtf8 ?? false, } satisfies VcsProcessOutput; }); diff --git a/apps/server/src/ws.ts b/apps/server/src/ws.ts index 6a299aa49b7..3b796f594c3 100644 --- a/apps/server/src/ws.ts +++ b/apps/server/src/ws.ts @@ -1593,6 +1593,12 @@ const makeWsRpcLayer = ( observeRpcEffect(WS_METHODS.pullRequestsDiff, pullRequests.diff(input), { "rpc.aggregate": "pull-requests", }), + [WS_METHODS.pullRequestsDiffFileContents]: (input) => + observeRpcEffect( + WS_METHODS.pullRequestsDiffFileContents, + pullRequests.diffFileContents(input), + { "rpc.aggregate": "pull-requests" }, + ), [WS_METHODS.pullRequestsRunAction]: (input) => observeRpcEffect(WS_METHODS.pullRequestsRunAction, pullRequests.runAction(input), { "rpc.aggregate": "pull-requests", diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index b388a7e505f..98f2ceafa06 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -119,6 +119,7 @@ import { buildTemporaryWorktreeBranchName } from "@t3tools/shared/git"; import { useMediaQuery } from "../hooks/useMediaQuery"; import { RIGHT_PANEL_INLINE_LAYOUT_MEDIA_QUERY } from "../rightPanelLayout"; import { + pullRequestSurfaceId, selectActiveRightPanel, selectActiveRightPanelSurface, selectThreadRightPanelState, @@ -140,7 +141,7 @@ import { usePreviewMiniPlayerStore, } from "../previewMiniPlayerStore"; import { PullRequestDetailPanel } from "./pullRequest/PullRequestDetailPanel"; -import { RightPanelTabs } from "./RightPanelTabs"; +import { RightPanelTabs, type PullRequestTabStatus } from "./RightPanelTabs"; import { DiffWorkerPoolProvider } from "./DiffWorkerPoolProvider"; import { BranchToolbar } from "./BranchToolbar"; import { resolveShortcutCommand, shortcutLabelForCommand } from "../keybindings"; @@ -1522,6 +1523,17 @@ function ChatViewContent(props: ChatViewProps) { const activeRightPanelSurface = useRightPanelStore((state) => selectActiveRightPanelSurface(state.byThreadKey, activeThreadRef), ); + const [pullRequestTabStatuses, setPullRequestTabStatuses] = useState< + Record + >({}); + const handlePullRequestTabStatusChange = useCallback((status: PullRequestTabStatus) => { + const id = pullRequestSurfaceId(status); + setPullRequestTabStatuses((current) => + current[id]?.state === status.state && current[id]?.isDraft === status.isDraft + ? current + : { ...current, [id]: status }, + ); + }, []); const activeFileSurface = activeRightPanelSurface?.kind === "file" ? activeRightPanelSurface : null; const activePreviewState = useThreadPreviewState(activeThreadRef); @@ -5794,6 +5806,7 @@ function ChatViewContent(props: ChatViewProps) { ? "thread" : "page" } + onStateChange={handlePullRequestTabStatusChange} /> ) : activeRightPanelSurface?.kind === "plan" ? ( {rightPanelContent} @@ -6270,9 +6285,11 @@ function ChatViewContent(props: ChatViewProps) { onAddFiles={addFilesSurface} onAddPullRequest={addPullRequestSurface} browserAvailable={isPreviewSupportedInRuntime()} + terminalAvailable={activeProject !== null} diffAvailable={isServerThread && isGitRepo} filesAvailable={activeProject !== null} pullRequestAvailable={pullRequestSurfaceAvailable} + pullRequestStatuses={pullRequestTabStatuses} > {rightPanelContent} diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index a10cdafd783..1855990aa1a 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -74,6 +74,7 @@ import { serverEnvironment } from "../state/server"; import { reviewEnvironment } from "../state/review"; import { vcsEnvironment } from "../state/vcs"; import { buildBaseRefChoices, filterBaseRefChoices } from "../lib/baseRefChoices"; +import { createGitDiffFileContentsLoader } from "../lib/diffFileContents"; type DiffRenderMode = "stacked" | "split"; type DiffThemeType = "light" | "dark"; @@ -86,262 +87,6 @@ interface CollapsedDiffFilesState { const EMPTY_COLLAPSED_DIFF_FILE_KEYS: ReadonlySet = new Set(); -const DIFF_PANEL_UNSAFE_CSS = ` -[data-diffs-header], -[data-diff], -[data-file], -[data-error-wrapper], -[data-virtualizer-buffer] { - --diffs-header-font-family: var(--font-sans) !important; - --diffs-font-family: var(--font-mono) !important; - --diffs-bg: var(--background) !important; - --diffs-light-bg: var(--background) !important; - --diffs-dark-bg: var(--background) !important; - --diffs-token-light-bg: transparent; - --diffs-token-dark-bg: transparent; - - --diffs-bg-context-override: color-mix(in srgb, var(--background) 97%, var(--foreground)); - --diffs-bg-hover-override: color-mix(in srgb, var(--background) 94%, var(--foreground)); - --diffs-bg-separator-override: color-mix(in srgb, var(--background) 95%, var(--foreground)); - --diffs-bg-buffer-override: color-mix(in srgb, var(--background) 90%, var(--foreground)); - - --diffs-bg-addition-override: light-dark( - color-mix(in srgb, var(--background) 50%, var(--success)), - color-mix(in srgb, var(--background) 70%, var(--success)) - ); - --diffs-bg-addition-number-override: light-dark( - color-mix(in srgb, var(--background) 35%, var(--success)), - color-mix(in srgb, var(--background) 60%, var(--success)) - ); - --diffs-bg-addition-hover-override: color-mix(in srgb, var(--background) 85%, var(--success)); - --diffs-bg-addition-emphasis-override: color-mix(in srgb, var(--background) 80%, var(--success)); - - --diffs-bg-deletion-override: light-dark( - color-mix(in srgb, var(--background) 50%, var(--destructive)), - color-mix(in srgb, var(--background) 70%, var(--destructive)) - ); - --diffs-bg-deletion-number-override: light-dark( - color-mix(in srgb, var(--background) 35%, var(--destructive)), - color-mix(in srgb, var(--background) 60%, var(--destructive)) - ); - --diffs-bg-deletion-hover-override: color-mix(in srgb, var(--background) 85%, var(--destructive)); - --diffs-bg-deletion-emphasis-override: color-mix( - in srgb, - var(--background) 80%, - var(--destructive) - ); - - background-color: var(--diffs-bg) !important; -} - -:is( - [data-line], - [data-line-annotation], - [data-merge-conflict], - [data-merge-conflict-actions], - [data-no-newline] -)[data-selected-line] { - --diffs-line-bg: light-dark( - color-mix( - in lab, - var(--background) 88%, - color-mix(in srgb, var(--background) 50%, var(--diffs-modified-base)) - ), - color-mix( - in lab, - var(--background) 80%, - color-mix(in srgb, var(--background) 70%, var(--diffs-modified-base)) - ) - ) !important; -} - -:is([data-gutter-buffer], [data-column-number])[data-selected-line] { - --diffs-line-bg: light-dark( - color-mix( - in lab, - var(--background) 91%, - color-mix(in srgb, var(--background) 35%, var(--diffs-modified-base)) - ), - color-mix( - in lab, - var(--background) 85%, - color-mix(in srgb, var(--background) 60%, var(--diffs-modified-base)) - ) - ) !important; -} - -[data-indicators="bars"] - :is([data-column-number], [data-gutter-buffer="annotation"])[data-selected-line] { - position: relative; -} - -[data-indicators="bars"] - :is([data-column-number], [data-gutter-buffer="annotation"])[data-selected-line]::before { - position: absolute !important; - inset-block: 0 !important; - inset-inline-start: 0 !important; - display: block !important; - width: 4px !important; - min-width: 4px !important; - max-width: 4px !important; - height: auto !important; - padding: 0 !important; - content: "" !important; - background-color: var(--diffs-modified-base) !important; - background-image: none !important; -} - -[data-file-info] { - background-color: var(--background) !important; - border-block-color: transparent !important; - color: var(--foreground) !important; -} - -[data-diffs-header] { - position: sticky !important; - top: 0; - z-index: 4; - background-color: var(--background) !important; - border-bottom-color: transparent !important; - align-items: center !important; - font-family: var(--font-sans) !important; - font-size: 12px !important; - line-height: 1 !important; - min-height: 32px !important; - padding-block: 6px !important; - padding-inline: 8px 12px !important; -} - -[data-diffs-header]:hover { - background-color: color-mix(in srgb, var(--background) 97%, var(--foreground)) !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]) { - height: 24px !important; - margin-block: 0 !important; - background-color: var(--background) !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]) - [data-separator-wrapper] { - padding-inline: 8px 12px !important; - background-color: transparent !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]) - [data-separator-content] { - gap: 8px; - padding-inline: 0 !important; - background-color: transparent !important; - color: color-mix(in srgb, var(--foreground) 52%, var(--background)) !important; - font-family: var(--font-sans) !important; - font-size: 11px !important; - text-decoration: none !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]) - [data-unmodified-lines] { - display: flex !important; - min-width: 0; - flex: 1 1 auto; - align-items: center; - gap: 8px; - cursor: pointer; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]) - [data-unmodified-lines]::before, -:is([data-separator="line-info"], [data-separator="line-info-basic"]) - [data-unmodified-lines]::after { - width: auto; - height: 1px; - flex: 1 1 auto; - content: ""; - background-color: color-mix(in srgb, var(--background) 92%, var(--foreground)); -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"])[data-expand-index] - [data-separator-wrapper] { - grid-template-columns: 0 minmax(0, 1fr) !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"])[data-expand-index] - [data-separator-content] { - grid-column: 2 !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]) - [data-expand-button] { - display: none !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( - [data-expand-button] - ) - [data-separator-content] { - cursor: pointer; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( - [data-expand-button] - ):hover - [data-separator-content] { - color: color-mix(in srgb, var(--foreground) 76%, var(--background)) !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( - [data-expand-button] - ):hover - [data-unmodified-lines]::before, -:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( - [data-expand-button] - ):hover - [data-unmodified-lines]::after { - background-color: color-mix(in srgb, var(--background) 84%, var(--foreground)); -} - -[data-diffs-header] [data-header-content] { - align-items: center !important; - line-height: 1 !important; -} - -[data-diffs-header] [data-metadata] { - align-items: center !important; - line-height: 1 !important; - font-variant-numeric: tabular-nums; -} - -[data-diffs-header] [data-additions-count], -[data-diffs-header] [data-deletions-count] { - font-family: var(--font-mono) !important; - font-size: 11px !important; - font-variant-numeric: tabular-nums; - line-height: 1 !important; -} - -[data-diffs-header] [data-change-icon], -[data-diffs-header] [data-rename-icon] { - display: block; - flex-shrink: 0; -} - -[data-title] { - cursor: pointer; - transition: - color 120ms ease, - text-decoration-color 120ms ease; - text-decoration: underline; - text-decoration-color: transparent; - text-underline-offset: 2px; - font-family: var(--font-sans) !important; -} - -[data-title]:hover { - color: color-mix(in srgb, var(--foreground) 84%, var(--primary)) !important; - text-decoration-color: currentColor; -} -`; - interface DiffPanelProps { mode?: DiffPanelMode; composerDraftTarget: ScopedThreadRef | DraftId; @@ -569,45 +314,14 @@ export default function DiffPanel({ return undefined; } - const source = selectedGitSource; - return async (fileDiff) => { - const newPath = resolveFileDiffPath(fileDiff); - const oldPath = fileDiff.prevName - ? resolveFileDiffPath({ ...fileDiff, name: fileDiff.prevName }) - : newPath; - const result = await getDiffFileContents({ - environmentId: activeThread.environmentId, - input: { - cwd: preview.cwd, - sourceKind: source.kind, - changeType: fileDiff.type, - baseRef: source.baseRef, - headRef: source.headRef, - oldPath, - newPath, - }, - }); - if (result._tag !== "Success") { - throw squashAtomCommandFailure(result); - } - - const newFile = { - name: newPath, - contents: result.value.newContents, - cacheKey: `${source.diffHash}:new:${newPath}`, - }; - if (fileDiff.type === "rename-pure") { - return { oldFile: null, newFile }; - } - return { - oldFile: { - name: oldPath, - contents: result.value.oldContents, - cacheKey: `${source.diffHash}:old:${oldPath}`, - }, - newFile, - }; - }; + return createGitDiffFileContentsLoader(getDiffFileContents, { + environmentId: activeThread.environmentId, + cwd: preview.cwd, + sourceKind: selectedGitSource.kind, + baseRef: selectedGitSource.baseRef, + headRef: selectedGitSource.headRef, + cacheKey: selectedGitSource.diffHash, + }); }, [ activeThread, branchDiffPreview.data, @@ -1160,7 +874,7 @@ export default function DiffPanel({ key={collapseScopeKey ?? reviewSectionId} viewerRef={codeViewRef} codeViewKey={codeViewMountKey} - className="diff-render-surface h-full min-h-0 overflow-auto" + className="h-full min-h-0 overflow-auto" files={codeViewFiles} sectionId={reviewSectionId} sectionTitle={reviewSectionTitle} @@ -1204,16 +918,8 @@ export default function DiffPanel({ overflow: wordWrap ? "wrap" : "scroll", theme: resolveDiffThemeName(resolvedTheme), themeType: resolvedTheme as DiffThemeType, - unsafeCSS: DIFF_PANEL_UNSAFE_CSS, stickyHeaders: true, ...(loadDiffFiles ? { loadDiffFiles } : {}), - itemMetrics: { - diffHeaderHeight: 32, - hunkSeparatorHeight: 24, - paddingTop: 0, - paddingBottom: 0, - }, - layout: { paddingTop: 0, paddingBottom: 0, gap: 0 }, }} /> diff --git a/apps/web/src/components/RightPanelTabs.tsx b/apps/web/src/components/RightPanelTabs.tsx index 772a877877d..5933f3ffa57 100644 --- a/apps/web/src/components/RightPanelTabs.tsx +++ b/apps/web/src/components/RightPanelTabs.tsx @@ -1,4 +1,4 @@ -import type { ContextMenuItem, PreviewSessionSnapshot } from "@t3tools/contracts"; +import type { ContextMenuItem, PreviewSessionSnapshot, PullRequestState } from "@t3tools/contracts"; import { getTerminalLabel } from "@t3tools/shared/terminalLabels"; import { ClipboardList, @@ -55,14 +55,25 @@ interface RightPanelTabsProps { onAddFiles: () => void; onAddPullRequest: () => void; browserAvailable: boolean; + terminalAvailable: boolean; diffAvailable: boolean; filesAvailable: boolean; pullRequestAvailable: boolean; + pullRequestStatuses?: Readonly>; children: ReactNode; } +export interface PullRequestTabStatus { + projectId: string; + repository: string; + number: number; + state: PullRequestState; + isDraft: boolean; +} + const SURFACE_DISABLED_REASONS = { browser: "Browser previews are only available in the T3 Code desktop app.", + terminal: "Terminal surfaces are only available from a project thread.", files: "Files are only available when a project is open.", diff: "Diff is only available for server threads in Git repositories.", pullRequest: "This thread's branch has no pull request yet.", @@ -105,6 +116,7 @@ function RightPanelEmptyState(props: { onAddFiles: () => void; onAddPullRequest: () => void; browserAvailable: boolean; + terminalAvailable: boolean; diffAvailable: boolean; filesAvailable: boolean; pullRequestAvailable: boolean; @@ -116,16 +128,16 @@ function RightPanelEmptyState(props: { icon: Globe2, available: props.browserAvailable, disabledReason: SURFACE_DISABLED_REASONS.browser, - wide: false, + centered: false, onClick: props.onAddBrowser, }, { label: "Terminal", description: "Start a shell in this workspace.", icon: TerminalSquare, - available: true, - disabledReason: null, - wide: false, + available: props.terminalAvailable, + disabledReason: SURFACE_DISABLED_REASONS.terminal, + centered: false, onClick: props.onAddTerminal, }, { @@ -134,7 +146,7 @@ function RightPanelEmptyState(props: { icon: Files, available: props.filesAvailable, disabledReason: SURFACE_DISABLED_REASONS.files, - wide: false, + centered: false, onClick: props.onAddFiles, }, { @@ -143,7 +155,7 @@ function RightPanelEmptyState(props: { icon: FileDiff, available: props.diffAvailable, disabledReason: SURFACE_DISABLED_REASONS.diff, - wide: false, + centered: false, onClick: props.onAddDiff, }, { @@ -152,9 +164,7 @@ function RightPanelEmptyState(props: { icon: GitPullRequest, available: props.pullRequestAvailable, disabledReason: SURFACE_DISABLED_REASONS.pullRequest, - // Last and across both columns, because it is the one surface that is about the thread's - // work as a whole rather than a tool to look at it with. - wide: true, + centered: true, onClick: props.onAddPullRequest, }, ] as const; @@ -171,7 +181,9 @@ function RightPanelEmptyState(props: {
{actions.map((action) => { const Icon = action.icon; - const spanClass = action.wide ? "col-span-2" : undefined; + const placementClass = action.centered + ? "col-span-2 w-[calc(50%-0.25rem)] justify-self-center" + : undefined; const content = ( <> @@ -189,7 +201,7 @@ function RightPanelEmptyState(props: { onClick={action.onClick} className={cn( "flex min-h-28 w-full flex-col items-start rounded-lg border border-border/80 bg-card p-4 text-left transition hover:border-border hover:bg-accent/60 dark:border-transparent dark:shadow-none dark:inset-ring-1 dark:inset-ring-white/5", - spanClass, + placementClass, )} > {content} @@ -201,7 +213,7 @@ function RightPanelEmptyState(props: { type="button" className={cn( "flex min-h-28 w-full cursor-not-allowed flex-col items-start rounded-lg border border-border/80 bg-card p-4 text-left opacity-40 dark:border-transparent dark:shadow-none dark:inset-ring-1 dark:inset-ring-white/5", - spanClass, + placementClass, )} aria-disabled="true" > @@ -276,10 +288,12 @@ function SurfaceIcon({ surface, sessions, theme, + pullRequestStatuses, }: { surface: RightPanelSurface; sessions: Readonly>; theme: "light" | "dark"; + pullRequestStatuses: Readonly> | undefined; }) { switch (surface.kind) { case "preview": { @@ -304,8 +318,20 @@ function SurfaceIcon({ return ; case "plan": return ; - case "pull-request": - return ; + case "pull-request": { + const status = pullRequestStatuses?.[surface.id] ?? null; + const toneClassName = + status?.state === "merged" + ? "text-violet-600 dark:text-violet-300/90" + : status?.state === "closed" + ? "text-red-600 dark:text-red-300/90" + : status?.isDraft + ? "text-zinc-500 dark:text-zinc-400/80" + : status?.state === "open" + ? "text-emerald-600 dark:text-emerald-300/90" + : "text-muted-foreground"; + return ; + } } } @@ -399,7 +425,7 @@ export function RightPanelTabs(props: RightPanelTabsProps) { className={cn( "workspace-topbar gap-1 pl-2", props.mode !== "inline" && "[--workspace-topbar-height:--spacing(11)]", - props.mode === "inline" ? "pr-28" : "pr-3", + props.mode === "inline" && !props.layoutControls ? "pr-28" : "pr-3", ownsDesktopTitleBar && "wco:pr-[calc(var(--workspace-native-controls-inset)+6rem)]", props.mode === "inline" && props.maximized && COLLAPSED_SIDEBAR_TITLEBAR_INSET_CLASS, )} @@ -442,6 +468,7 @@ export function RightPanelTabs(props: RightPanelTabsProps) { surface={surface} sessions={props.previewSessions} theme={resolvedTheme} + pullRequestStatuses={props.pullRequestStatuses} /> {pending ? ( Browser - + Terminal @@ -530,6 +561,7 @@ export function RightPanelTabs(props: RightPanelTabsProps) { onAddFiles={props.onAddFiles} onAddPullRequest={props.onAddPullRequest} browserAvailable={props.browserAvailable} + terminalAvailable={props.terminalAvailable} diffAvailable={props.diffAvailable} filesAvailable={props.filesAvailable} pullRequestAvailable={props.pullRequestAvailable} diff --git a/apps/web/src/components/chat/PanelLayoutControls.tsx b/apps/web/src/components/chat/PanelLayoutControls.tsx index deb7b3ee9af..8897d7bac40 100644 --- a/apps/web/src/components/chat/PanelLayoutControls.tsx +++ b/apps/web/src/components/chat/PanelLayoutControls.tsx @@ -5,6 +5,7 @@ import { Toggle } from "../ui/toggle"; import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; interface PanelLayoutControlsProps { + showTerminalControl?: boolean; terminalAvailable: boolean; terminalOpen: boolean; terminalShortcutLabel: string | null; @@ -16,6 +17,7 @@ interface PanelLayoutControlsProps { } export const PanelLayoutControls = memo(function PanelLayoutControls({ + showTerminalControl = true, terminalAvailable, terminalOpen, terminalShortcutLabel, @@ -30,28 +32,30 @@ export const PanelLayoutControls = memo(function PanelLayoutControls({ className="flex h-full shrink-0 items-center gap-1 [-webkit-app-region:no-drag]" data-panel-layout-controls > - - - - - } - /> - - {terminalAvailable - ? `Toggle terminal drawer${terminalShortcutLabel ? ` (${terminalShortcutLabel})` : ""}` - : "Terminal drawer is unavailable"} - - + {showTerminalControl ? ( + + + + + } + /> + + {terminalAvailable + ? `Toggle terminal drawer${terminalShortcutLabel ? ` (${terminalShortcutLabel})` : ""}` + : "Terminal drawer is unavailable"} + + + ) : null} ({ }), })); -vi.mock("../files/LocalCommentAnnotation", () => ({ - LocalCommentAnnotation: () => null, +vi.mock("./DiffCommentAnnotation", () => ({ + DiffCommentAnnotation: () => null, })); vi.mock("../files/fileCommentAnnotations", () => ({ diff --git a/apps/web/src/components/diffs/AnnotatableCodeView.tsx b/apps/web/src/components/diffs/AnnotatableCodeView.tsx index 0decaa3acf5..f0d989a7d5b 100644 --- a/apps/web/src/components/diffs/AnnotatableCodeView.tsx +++ b/apps/web/src/components/diffs/AnnotatableCodeView.tsx @@ -6,7 +6,7 @@ import type { FileDiffMetadata, SelectedLineRange, } from "@pierre/diffs"; -import { CodeView, type CodeViewHandle, type CodeViewProps } from "@pierre/diffs/react"; +import type { CodeViewHandle } from "@pierre/diffs/react"; import type { ScopedThreadRef } from "@t3tools/contracts"; import { useCallback, useMemo, useState, type ReactNode, type Ref } from "react"; @@ -18,8 +18,9 @@ import { type ReviewCommentContext, } from "~/reviewCommentContext"; -import { LocalCommentAnnotation } from "../files/LocalCommentAnnotation"; import { nextFileCommentId } from "../files/fileCommentAnnotations"; +import { DiffCommentAnnotation } from "./DiffCommentAnnotation"; +import { StyledDiffCodeView, type StyledDiffCodeViewOptions } from "./StyledDiffCodeView"; interface DiffCommentAnnotationEntry { id: string; @@ -81,7 +82,7 @@ interface AnnotatableCodeViewProps { sectionId: string; sectionTitle: string; composerDraftTarget: ScopedThreadRef | DraftId; - options: NonNullable["options"]>; + options: StyledDiffCodeViewOptions; viewerRef?: Ref; className?: string; renderHeaderPrefix: ( @@ -237,9 +238,9 @@ export function AnnotatableCodeView({ const hasOpenComment = draft !== null; return ( - + key={codeViewKey} - {...(viewerRef ? { ref: viewerRef } : {})} + {...(viewerRef ? { viewerRef } : {})} {...(className ? { className } : {})} items={items} selectedLines={selectedLines} @@ -262,7 +263,7 @@ export function AnnotatableCodeView({ className={hasDraft ? "py-1" : "divide-y divide-border/30 border-y border-border/30"} > {annotation.metadata.entries.map((entry) => ( - { - it("renders the draft composer directly in the selected diff", () => { +describe("DiffCommentAnnotation", () => { + it("renders the shared draft composer directly in the selected diff", () => { const markup = renderToStaticMarkup( - , + , ); expect(markup).toContain("font-sans"); @@ -31,9 +31,35 @@ describe("LocalCommentAnnotation", () => { expect(markup).toContain("cursor-text"); }); + it("lets a pull-request diff configure actions without replacing the composer", () => { + const markup = renderToStaticMarkup( + , + allowEmpty: true, + onAction: vi.fn(), + }} + />, + ); + + expect(markup).toContain("Add a comment…"); + expect(markup).toContain(">Ask"); + expect(markup).toContain(">Add to review"); + expect(markup.match(/]*disabled[^>]*>Add to review<\/button>/)).not.toBeNull(); + const askButton = markup.match(/]*>.*?Ask<\/button>/)?.[0]; + expect(askButton).toBeDefined(); + expect(askButton).not.toContain(' disabled=""'); + }); + it("renders a saved comment without a nested card or redundant range label", () => { const markup = renderToStaticMarkup( - { it("renders draft text owned by the annotation wrapper", () => { const markup = renderToStaticMarkup( - void; +} + +interface DiffCommentAnnotationProps { kind: "draft" | "comment"; rangeLabel: string; text: string; onTextChange?: (text: string) => void; onCancel: () => void; onComment: (text: string) => void; - onDelete: () => void; + onDelete?: () => void; + placeholder?: string; + submitLabel?: string; + pending?: boolean; + secondaryAction?: DiffCommentSecondaryAction; } -export function LocalCommentAnnotation({ +/** The shared inline comment treatment for file previews, thread diffs, and pull-request diffs. */ +export function DiffCommentAnnotation({ kind, rangeLabel, text, @@ -22,36 +36,43 @@ export function LocalCommentAnnotation({ onCancel, onComment, onDelete, -}: LocalCommentAnnotationProps) { + placeholder = "Add a comment…", + submitLabel = "Comment", + pending = false, + secondaryAction, +}: DiffCommentAnnotationProps) { const [localDraftText, setLocalDraftText] = useState(""); const displayedText = kind === "draft" && !onTextChange ? localDraftText : text; + const trimmedText = displayedText.trim(); if (kind === "comment") { return (
event.stopPropagation()} >
); } return (
event.stopPropagation()} @@ -62,7 +83,7 @@ export function LocalCommentAnnotation({ className="relative inline-flex w-full rounded-md border border-border/50 bg-background/20 font-sans text-foreground transition-colors focus-within:border-border/70 [&_[data-slot=textarea]]:min-h-12 [&_[data-slot=textarea]]:cursor-text [&_[data-slot=textarea]]:px-2.5 [&_[data-slot=textarea]]:py-1.5 [&_[data-slot=textarea]]:font-sans [&_[data-slot=textarea]]:text-xs [&_[data-slot=textarea]]:leading-5 max-sm:[&_[data-slot=textarea]]:min-h-12" size="sm" value={displayedText} - placeholder="Add a comment…" + placeholder={placeholder} aria-label={`Comment on lines ${rangeLabel}`} onChange={(event) => (onTextChange ?? setLocalDraftText)(event.target.value)} onFocus={(event) => { @@ -74,9 +95,9 @@ export function LocalCommentAnnotation({ event.preventDefault(); onCancel(); } - if ((event.metaKey || event.ctrlKey) && event.key === "Enter" && displayedText.trim()) { + if (isCommentSubmitShortcut(event, trimmedText, pending)) { event.preventDefault(); - onComment(displayedText.trim()); + onComment(trimmedText); } }} /> @@ -90,12 +111,19 @@ export function LocalCommentAnnotation({ > Cancel - + ) : null} +
diff --git a/apps/web/src/components/diffs/StyledDiffCodeView.test.tsx b/apps/web/src/components/diffs/StyledDiffCodeView.test.tsx new file mode 100644 index 00000000000..1baf3fcba98 --- /dev/null +++ b/apps/web/src/components/diffs/StyledDiffCodeView.test.tsx @@ -0,0 +1,58 @@ +import { renderToStaticMarkup } from "react-dom/server"; +import { beforeEach, describe, expect, it, vi } from "vite-plus/test"; + +const testState = vi.hoisted(() => ({ + codeViewClassName: null as string | null, + codeViewOptions: null as Record | null, +})); + +vi.mock("@pierre/diffs/react", () => ({ + CodeView: (props: { className: string; options: Record }) => { + testState.codeViewClassName = props.className; + testState.codeViewOptions = props.options; + return null; + }, +})); + +import { StyledDiffCodeView } from "./StyledDiffCodeView"; + +describe("StyledDiffCodeView", () => { + beforeEach(() => { + testState.codeViewClassName = null; + testState.codeViewOptions = null; + }); + + it("always pairs the shared diff styling with its virtualized geometry", () => { + const loadDiffFiles = vi.fn(async () => ({ + oldFile: { name: "before.ts", contents: "before\n" }, + newFile: { name: "after.ts", contents: "after\n" }, + })); + renderToStaticMarkup( + , + ); + + expect(testState.codeViewClassName).toBe("diff-render-surface min-h-0"); + expect(testState.codeViewOptions).toMatchObject({ + theme: "pierre-dark", + stickyHeaders: true, + loadDiffFiles, + itemMetrics: { + diffHeaderHeight: 32, + hunkSeparatorHeight: 24, + paddingTop: 0, + paddingBottom: 0, + }, + layout: { paddingTop: 0, paddingBottom: 0, gap: 0 }, + }); + expect(testState.codeViewOptions?.unsafeCSS).toEqual( + expect.stringContaining("[data-unmodified-lines]::before"), + ); + expect(testState.codeViewOptions?.unsafeCSS).toEqual( + expect.stringContaining(")[data-expand-index]\n [data-unmodified-lines]"), + ); + }); +}); diff --git a/apps/web/src/components/diffs/StyledDiffCodeView.tsx b/apps/web/src/components/diffs/StyledDiffCodeView.tsx new file mode 100644 index 00000000000..5714eb244f2 --- /dev/null +++ b/apps/web/src/components/diffs/StyledDiffCodeView.tsx @@ -0,0 +1,310 @@ +/* oxlint-disable eslint/no-restricted-imports -- This is the single styled adapter around Pierre's raw viewer. */ +import { + CodeView, + type CodeViewHandle, + type CodeViewProps, + type ControlledCodeViewProps, + type UncontrolledCodeViewProps, +} from "@pierre/diffs/react"; +/* oxlint-enable eslint/no-restricted-imports */ +import type { Ref } from "react"; + +const DIFF_VIEW_UNSAFE_CSS = ` +[data-diffs-header], +[data-diff], +[data-file], +[data-error-wrapper], +[data-virtualizer-buffer] { + --diffs-header-font-family: var(--font-sans) !important; + --diffs-font-family: var(--font-mono) !important; + --diffs-bg: var(--background) !important; + --diffs-light-bg: var(--background) !important; + --diffs-dark-bg: var(--background) !important; + --diffs-token-light-bg: transparent; + --diffs-token-dark-bg: transparent; + + --diffs-bg-context-override: color-mix(in srgb, var(--background) 97%, var(--foreground)); + --diffs-bg-hover-override: color-mix(in srgb, var(--background) 94%, var(--foreground)); + --diffs-bg-separator-override: color-mix(in srgb, var(--background) 95%, var(--foreground)); + --diffs-bg-buffer-override: color-mix(in srgb, var(--background) 90%, var(--foreground)); + + --diffs-bg-addition-override: light-dark( + color-mix(in srgb, var(--background) 50%, var(--success)), + color-mix(in srgb, var(--background) 70%, var(--success)) + ); + --diffs-bg-addition-number-override: light-dark( + color-mix(in srgb, var(--background) 35%, var(--success)), + color-mix(in srgb, var(--background) 60%, var(--success)) + ); + --diffs-bg-addition-hover-override: color-mix(in srgb, var(--background) 85%, var(--success)); + --diffs-bg-addition-emphasis-override: color-mix(in srgb, var(--background) 80%, var(--success)); + + --diffs-bg-deletion-override: light-dark( + color-mix(in srgb, var(--background) 50%, var(--destructive)), + color-mix(in srgb, var(--background) 70%, var(--destructive)) + ); + --diffs-bg-deletion-number-override: light-dark( + color-mix(in srgb, var(--background) 35%, var(--destructive)), + color-mix(in srgb, var(--background) 60%, var(--destructive)) + ); + --diffs-bg-deletion-hover-override: color-mix(in srgb, var(--background) 85%, var(--destructive)); + --diffs-bg-deletion-emphasis-override: color-mix( + in srgb, + var(--background) 80%, + var(--destructive) + ); + + background-color: var(--diffs-bg) !important; +} + +:is( + [data-line], + [data-line-annotation], + [data-merge-conflict], + [data-merge-conflict-actions], + [data-no-newline] +)[data-selected-line] { + --diffs-line-bg: light-dark( + color-mix( + in lab, + var(--background) 88%, + color-mix(in srgb, var(--background) 50%, var(--diffs-modified-base)) + ), + color-mix( + in lab, + var(--background) 80%, + color-mix(in srgb, var(--background) 70%, var(--diffs-modified-base)) + ) + ) !important; +} + +:is([data-gutter-buffer], [data-column-number])[data-selected-line] { + --diffs-line-bg: light-dark( + color-mix( + in lab, + var(--background) 91%, + color-mix(in srgb, var(--background) 35%, var(--diffs-modified-base)) + ), + color-mix( + in lab, + var(--background) 85%, + color-mix(in srgb, var(--background) 60%, var(--diffs-modified-base)) + ) + ) !important; +} + +[data-indicators="bars"] + :is([data-column-number], [data-gutter-buffer="annotation"])[data-selected-line] { + position: relative; +} + +[data-indicators="bars"] + :is([data-column-number], [data-gutter-buffer="annotation"])[data-selected-line]::before { + position: absolute !important; + inset-block: 0 !important; + inset-inline-start: 0 !important; + display: block !important; + width: 4px !important; + min-width: 4px !important; + max-width: 4px !important; + height: auto !important; + padding: 0 !important; + content: "" !important; + background-color: var(--diffs-modified-base) !important; + background-image: none !important; +} + +[data-file-info] { + background-color: var(--background) !important; + border-block-color: transparent !important; + color: var(--foreground) !important; +} + +[data-diffs-header] { + position: sticky !important; + top: 0; + z-index: 4; + background-color: var(--background) !important; + border-bottom-color: transparent !important; + align-items: center !important; + font-family: var(--font-sans) !important; + font-size: 12px !important; + line-height: 1 !important; + min-height: 32px !important; + padding-block: 6px !important; + padding-inline: 8px 12px !important; +} + +[data-diffs-header]:hover { + background-color: color-mix(in srgb, var(--background) 97%, var(--foreground)) !important; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"]) { + height: 24px !important; + margin-block: 0 !important; + background-color: var(--background) !important; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"]) + [data-separator-wrapper] { + padding-inline: 8px 12px !important; + background-color: transparent !important; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"]) + [data-separator-content] { + gap: 8px; + padding-inline: 0 !important; + background-color: transparent !important; + color: color-mix(in srgb, var(--foreground) 52%, var(--background)) !important; + font-family: var(--font-sans) !important; + font-size: 11px !important; + text-decoration: none !important; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"]) + [data-unmodified-lines] { + display: flex !important; + min-width: 0; + flex: 1 1 auto; + align-items: center; + gap: 8px; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"])[data-expand-index] + [data-unmodified-lines] { + cursor: pointer; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"]) + [data-unmodified-lines]::before, +:is([data-separator="line-info"], [data-separator="line-info-basic"]) + [data-unmodified-lines]::after { + width: auto; + height: 1px; + flex: 1 1 auto; + content: ""; + background-color: color-mix(in srgb, var(--background) 92%, var(--foreground)); +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"])[data-expand-index] + [data-separator-wrapper] { + grid-template-columns: 0 minmax(0, 1fr) !important; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"])[data-expand-index] + [data-separator-content] { + grid-column: 2 !important; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"]) + [data-expand-button] { + display: none !important; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( + [data-expand-button] + ) + [data-separator-content] { + cursor: pointer; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( + [data-expand-button] + ):hover + [data-separator-content] { + color: color-mix(in srgb, var(--foreground) 76%, var(--background)) !important; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( + [data-expand-button] + ):hover + [data-unmodified-lines]::before, +:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( + [data-expand-button] + ):hover + [data-unmodified-lines]::after { + background-color: color-mix(in srgb, var(--background) 84%, var(--foreground)); +} + +[data-diffs-header] [data-header-content] { + align-items: center !important; + line-height: 1 !important; +} + +[data-diffs-header] [data-metadata] { + align-items: center !important; + line-height: 1 !important; + font-variant-numeric: tabular-nums; +} + +[data-diffs-header] [data-additions-count], +[data-diffs-header] [data-deletions-count] { + font-family: var(--font-mono) !important; + font-size: 11px !important; + font-variant-numeric: tabular-nums; + line-height: 1 !important; +} + +[data-diffs-header] [data-change-icon], +[data-diffs-header] [data-rename-icon] { + display: block; + flex-shrink: 0; +} + +[data-title] { + cursor: pointer; + transition: + color 120ms ease, + text-decoration-color 120ms ease; + text-decoration: underline; + text-decoration-color: transparent; + text-underline-offset: 2px; + font-family: var(--font-sans) !important; +} + +[data-title]:hover { + color: color-mix(in srgb, var(--foreground) 84%, var(--primary)) !important; + text-decoration-color: currentColor; +} +`; + +export type StyledDiffCodeViewOptions = Omit< + NonNullable["options"]>, + "unsafeCSS" | "itemMetrics" | "layout" +>; + +type StyledDiffCodeViewProps = ( + | Omit, "options"> + | Omit, "options"> +) & { + readonly options?: StyledDiffCodeViewOptions; + readonly viewerRef?: Ref>; +}; + +/** The shared web CodeView surface: app styling and virtualized geometry stay paired here. */ +export function StyledDiffCodeView({ + options, + viewerRef, + className, + ...props +}: StyledDiffCodeViewProps) { + return ( + + {...props} + {...(viewerRef ? { ref: viewerRef } : {})} + className={className ? `diff-render-surface ${className}` : "diff-render-surface"} + options={{ + ...options, + unsafeCSS: DIFF_VIEW_UNSAFE_CSS, + itemMetrics: { + diffHeaderHeight: 32, + hunkSeparatorHeight: 24, + paddingTop: 0, + paddingBottom: 0, + }, + layout: { paddingTop: 0, paddingBottom: 0, gap: 0 }, + }} + /> + ); +} diff --git a/apps/web/src/components/diffs/commentSubmitShortcut.test.ts b/apps/web/src/components/diffs/commentSubmitShortcut.test.ts new file mode 100644 index 00000000000..434228db57e --- /dev/null +++ b/apps/web/src/components/diffs/commentSubmitShortcut.test.ts @@ -0,0 +1,26 @@ +import { describe, expect, it } from "vite-plus/test"; + +import { isCommentSubmitShortcut } from "./commentSubmitShortcut"; + +describe("isCommentSubmitShortcut", () => { + it("accepts Command or Ctrl+Enter only while an eligible comment is idle", () => { + expect( + isCommentSubmitShortcut({ key: "Enter", metaKey: true, ctrlKey: false }, "Looks good", false), + ).toBe(true); + expect( + isCommentSubmitShortcut({ key: "Enter", metaKey: false, ctrlKey: true }, "Looks good", false), + ).toBe(true); + expect( + isCommentSubmitShortcut({ key: "Enter", metaKey: true, ctrlKey: false }, "Looks good", true), + ).toBe(false); + }); + + it("rejects empty comments and unrelated key presses", () => { + expect( + isCommentSubmitShortcut({ key: "Enter", metaKey: true, ctrlKey: false }, " ", false), + ).toBe(false); + expect( + isCommentSubmitShortcut({ key: "K", metaKey: true, ctrlKey: false }, "Looks good", false), + ).toBe(false); + }); +}); diff --git a/apps/web/src/components/diffs/commentSubmitShortcut.ts b/apps/web/src/components/diffs/commentSubmitShortcut.ts new file mode 100644 index 00000000000..ee5626aeb97 --- /dev/null +++ b/apps/web/src/components/diffs/commentSubmitShortcut.ts @@ -0,0 +1,16 @@ +interface CommentSubmitShortcutEvent { + readonly key: string; + readonly metaKey: boolean; + readonly ctrlKey: boolean; +} + +/** Shared guard for inline comment composers that submit on Command/Ctrl+Enter. */ +export function isCommentSubmitShortcut( + event: CommentSubmitShortcutEvent, + value: string, + pending: boolean, +): boolean { + return ( + !pending && (event.metaKey || event.ctrlKey) && event.key === "Enter" && value.trim().length > 0 + ); +} diff --git a/apps/web/src/components/files/FilePreviewPanel.tsx b/apps/web/src/components/files/FilePreviewPanel.tsx index a736cf96cd3..67ba43d9c2a 100644 --- a/apps/web/src/components/files/FilePreviewPanel.tsx +++ b/apps/web/src/components/files/FilePreviewPanel.tsx @@ -52,7 +52,7 @@ import { } from "./fileCommentAnnotations"; import { installFileEditorDismissal } from "./fileEditorDismissal"; import { resolveCenteredFileLineScrollTop } from "./fileLineReveal"; -import { LocalCommentAnnotation } from "./LocalCommentAnnotation"; +import { DiffCommentAnnotation } from "../diffs/DiffCommentAnnotation"; import { projectFileCacheKey, projectFileEditorCacheKey } from "./fileContentRevision"; import { fileBreadcrumbs } from "./filePath"; import { isMarkdownPreviewFile, setMarkdownTaskChecked } from "./filePreviewMode"; @@ -668,7 +668,7 @@ function EditableFileSurface({ renderAnnotation={(annotation) => (
{annotation.metadata.entries.map((entry) => ( - + createPullRequestDiffFileContentsLoader(getDiffFileContents, { + environmentId, + reference, + commit, + cacheKey: `pull-request:${referenceKey}:${detail.updatedAt}:${commit ?? "all"}`, + }), + [commit, detail.updatedAt, environmentId, getDiffFileContents, reference, referenceKey], + ); // What is offered is the intersection of two different questions: what this host can do at // all, and what this account may do on this repository. Either one saying no means a control @@ -315,7 +326,9 @@ export function PullRequestCodeTab({ const cacheKey = `pull-request:${scopeKey}:${resolvedTheme}:${slice.cursor ?? "first"}:${fnv1a32(slice.patch)}`; const cached = parseCache.current.get(cacheKey); if (cached) return cached; - const parsed = getRenderablePatch(slice.patch, cacheKey); + const parsed = getRenderablePatch(slice.patch, cacheKey, { + compactPartialHunkOffsets: true, + }); if (parsed) parseCache.current.set(cacheKey, parsed); return parsed; }), @@ -940,8 +953,8 @@ export function PullRequestCodeTab({ {/* The viewer virtualizes against the element it is told is scrolling and places its rows absolutely, so it has to own that element — the thread diff panel hands it the same one. Scrolling from a parent instead leaves it painting over its neighbours. */} - - className="diff-render-surface min-h-0 flex-1 overflow-auto" + + className="min-h-0 flex-1 overflow-auto" items={items} selectedLines={selectedLines} onSelectedLinesChange={setSelectedLines} @@ -952,8 +965,7 @@ export function PullRequestCodeTab({ theme: resolveDiffThemeName(resolvedTheme), themeType: resolvedTheme, stickyHeaders: true, - itemMetrics: { diffHeaderHeight: 33 }, - layout: { paddingTop: 0, paddingBottom: 8, gap: 8 }, + loadDiffFiles, enableGutterUtility: canCommentOnLines && draft === null, enableLineSelection: canCommentOnLines && draft === null, // Two gestures reach the same place: dragging the line numbers selects a range, @@ -1024,17 +1036,26 @@ export function PullRequestCodeTab({ /> ))} {annotation.metadata.draft && draft ? ( - askAboutSelection(draft, question) } + ? { + secondaryAction: { + label: "Ask", + icon: , + allowEmpty: true, + onAction: (question: string) => askAboutSelection(draft, question), + }, + } : {})} onCancel={() => { setDraft(null); setSelectedLines(null); }} - onSubmit={(body) => { + onComment={(body) => { addComment(reviewKey, { id: nextPendingReviewCommentId(), path: draft.path, diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index 43b3d690c5d..e2ec9ca7a23 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -5,11 +5,14 @@ import type { PullRequestAction, PullRequestMergeMethod, PullRequestRef, + PullRequestState, } from "@t3tools/contracts"; import { + ArrowLeftIcon, + ArrowUpRightIcon, BookOpenIcon, ChevronDownIcon, - ExternalLinkIcon, + FilesIcon, FolderGit2Icon, GitBranchIcon, GitMergeIcon, @@ -22,12 +25,13 @@ import { MoreHorizontalIcon, PanelRightIcon, RefreshCwIcon, + TriangleAlertIcon, } from "lucide-react"; import { lazy, Suspense, useEffect, useRef, useState } from "react"; import { type DraftId, useComposerDraftStore } from "~/composerDraftStore"; import { useNewThreadHandler } from "~/hooks/useHandleNewThread"; -import { writeTextToClipboard } from "~/hooks/useCopyToClipboard"; +import { useCopyToClipboard, writeTextToClipboard } from "~/hooks/useCopyToClipboard"; import { usePreparePullRequestThreadAction } from "~/lib/sourceControlActions"; import { cn } from "~/lib/utils"; import { readLocalApi } from "~/localApi"; @@ -36,6 +40,7 @@ import { useEnvironmentQuery } from "~/state/query"; import { useLiveRefresh } from "~/hooks/useLiveRefresh"; import { pullRequestEnvironment } from "~/state/pullRequests"; import { useAtomCommand } from "~/state/use-atom-command"; +import { formatRelativeTimeLabel } from "~/timestampFormat"; import { AlertDialog, @@ -75,7 +80,12 @@ import { readableFailure, type PullRequestFinding, } from "./pullRequestDetail.logic"; -import { PullRequestStateGlyph } from "./pullRequestPresentation"; +import { + PullRequestActorLabel, + PullRequestDiffStat, + PullRequestMetaLine, + resolvePullRequestState, +} from "./pullRequestPresentation"; type DetailTab = "summary" | "timeline" | "code"; @@ -137,6 +147,7 @@ export function PullRequestDetailPanel({ refreshToken: forcedRefreshToken = 0, onActed, onClose, + onStateChange, context = "page", }: { environmentId: EnvironmentId; @@ -152,8 +163,16 @@ export function PullRequestDetailPanel({ * Told rather than assumed: only the page knows whether it is showing one. */ onActed?: () => void; - /** Absent when something around the panel already owns closing it — a surface tab's own X. */ + /** Page-owned detail columns use this to clear the selected pull request. */ onClose?: () => void; + /** Keeps compact chrome, such as the right-panel tab, in step with refreshed host state. */ + onStateChange?: (status: { + projectId: string; + repository: string; + number: number; + state: PullRequestState; + isDraft: boolean; + }) => void; /** * Beside a thread, the checkout affordance disappears: the panel is showing that thread's * own pull request, so the branch is already under the reader's feet — and checking it out @@ -168,11 +187,25 @@ export function PullRequestDetailPanel({ // Which handoff is preparing, keyed so a per-finding button can say "Preparing..." on itself // alone. One at a time whatever the key: they all check the same pull request out. const [handoff, setHandoff] = useState(null); + const { copyToClipboard: copyBranchToClipboard, isCopied: isBranchCopied } = useCopyToClipboard({ + target: "branch name", + timeout: 1600, + }); const detailQuery = useEnvironmentQuery( pullRequestEnvironment.detail({ environmentId, input: reference }), ); const detail = detailQuery.data; + useEffect(() => { + if (!detail) return; + onStateChange?.({ + projectId: detail.projectId, + repository: detail.repository, + number: detail.number, + state: detail.state, + isDraft: detail.isDraft, + }); + }, [detail, onStateChange]); // A pull request changes while it is open in front of somebody — a push lands, a check // finishes, a review arrives — so the panel reads it again on the way back to the window and // while a reader sits on it. Keyed by the pull request rather than by the panel, because this @@ -561,41 +594,37 @@ export function PullRequestDetailPanel({ : allowedMergeMethods.length > 0 ? "merge" : null; + // The pull request number carries this state in the overview and the right-panel tab mirrors + // it. Conflicts keep their own row below: an open pull request remains green there. + const statePresentation = detail + ? resolvePullRequestState({ state: detail.state, isDraft: detail.isDraft }) + : null; return (
-
- {detail ? ( - - ) : null} - -
+
+
+ {detail && statePresentation ? ( + <> + + {detail.repository} + + + + ) : null} +
+
{detail ? ( <> @@ -605,14 +634,28 @@ export function PullRequestDetailPanel({ > - + void refreshFromHost()}> Refresh - void readLocalApi()?.shell.openExternal(detail.url)}> - - {OPEN_ON_HOST_LABELS[detail.provider] ?? "Open on host"} + + + + {handoff === "ask" ? "Opening..." : "Ask a question"} + + Opens a thread that knows which pull request you mean. + + + + + + + {handoff === "explain" ? "Opening..." : "Explain this PR"} + + A walk through the diff and what to read closely. + + {detail.state === "open" ? ( @@ -701,6 +744,15 @@ export function PullRequestDetailPanel({ ) : null} + {/* Checking a pull request out is the reason to open one here at all, so it is a button of its own rather than a side effect of asking an agent for something. It asks where, because the two answers are not interchangeable: one leaves your @@ -746,57 +798,10 @@ export function PullRequestDetailPanel({ ) : null} - {/* Beside checking out, because they are the two things somebody opening a pull - request wants: the code, or an answer about it. Asking takes no checkout — a - question is not a reason to move the working tree or to make a worktree nobody - asked for — which is what keeps it a separate press rather than a mode of the - one next to it. */} - - - {handoff === "ask" || handoff === "explain" ? ( - "Opening..." - ) : ( - <> - - Ask - - - )} - - } - /> - - - - - Ask a question - - Opens a thread that knows which pull request you mean. - - - - - - - Explain this PR - - A walk through the diff: what it is for, and what to read closely. - - - - - {primaryAction === "ready" ? ( - ) : primaryAction === "resolve" ? ( - ) : primaryAction === "merge" ? (
+ + {detail ? ( +
+

{detail.title}

+ + + updated {formatRelativeTimeLabel(detail.updatedAt)} + + +
+ + {detail.baseBranch} + + + + + + + {detail.changedFiles.toLocaleString()}{" "} + {detail.changedFiles === 1 ? "file" : "files"} + + + +
+
+ ) : null} + + {detail && conflicting ? ( +
+ + Merge conflicts + + with {detail.baseBranch} + + +
+ ) : null} + +
diff --git a/apps/web/src/components/pullRequest/PullRequestListFilters.test.tsx b/apps/web/src/components/pullRequest/PullRequestListFilters.test.tsx new file mode 100644 index 00000000000..37f635323b7 --- /dev/null +++ b/apps/web/src/components/pullRequest/PullRequestListFilters.test.tsx @@ -0,0 +1,130 @@ +import type { ProjectId, PullRequestProviderSummary } from "@t3tools/contracts"; +import { CircleIcon } from "lucide-react"; +import { Children, isValidElement, type ReactElement, type ReactNode } from "react"; +import { describe, expect, it, vi } from "vite-plus/test"; + +import { + PullRequestFilterPills, + PullRequestProjectFilter, + PullRequestProviderFilter, +} from "./PullRequestListFilters"; + +type Clickable = ReactElement<{ + readonly "aria-pressed"?: boolean; + readonly children?: ReactNode; + readonly onClick?: () => void; +}>; + +function buttonsIn(element: ReactElement<{ readonly children?: ReactNode }> | null): Clickable[] { + if (element === null) return []; + return Children.toArray(element.props.children).filter(isValidElement) as Clickable[]; +} + +function findValueChange( + node: ReactNode, +): + | ReactElement<{ readonly children?: ReactNode; readonly onValueChange: (value: string) => void }> + | undefined { + for (const child of Children.toArray(node)) { + if (!isValidElement(child)) continue; + const props = child.props as { + readonly children?: ReactNode; + readonly onValueChange?: (value: string) => void; + }; + if (props.onValueChange) { + return child as ReactElement<{ + readonly children?: ReactNode; + readonly onValueChange: (value: string) => void; + }>; + } + const nested = findValueChange(props.children); + if (nested) return nested; + } + return undefined; +} + +describe("pull request list filters", () => { + it("does not emit a change when the selected filter pill is pressed again", () => { + const onChange = vi.fn(); + const view = PullRequestFilterPills({ + value: "open", + label: "State", + onChange, + options: [ + { value: "open", label: "Open", Icon: CircleIcon }, + { value: "closed", label: "Closed", Icon: CircleIcon }, + ], + }); + const [selected, unselected] = buttonsIn(view); + + selected?.props.onClick?.(); + expect(onChange).not.toHaveBeenCalled(); + + unselected?.props.onClick?.(); + expect(onChange).toHaveBeenCalledOnce(); + expect(onChange).toHaveBeenCalledWith("closed"); + }); + + it("does not emit a change when the selected host or All is pressed again", () => { + const providers = [ + { + host: "github.com", + kind: "github", + searchesOnHost: true, + projectCount: 1, + configured: true, + detail: null, + }, + { + host: "gitlab.com", + kind: "gitlab", + searchesOnHost: true, + projectCount: 1, + configured: true, + detail: null, + }, + ] as ReadonlyArray; + const onChange = vi.fn(); + + const selectedHost = PullRequestProviderFilter({ + providers, + value: "github.com", + expectedHosts: [], + onChange, + }); + buttonsIn(selectedHost) + .find((button) => button.props["aria-pressed"]) + ?.props.onClick?.(); + expect(onChange).not.toHaveBeenCalled(); + + const allHosts = PullRequestProviderFilter({ + providers, + value: undefined, + expectedHosts: [], + onChange, + }); + buttonsIn(allHosts) + .find((button) => button.props["aria-pressed"]) + ?.props.onClick?.(); + expect(onChange).not.toHaveBeenCalled(); + }); + + it("does not emit a change when the selected project is chosen again", () => { + const projectId = "project-1" as ProjectId; + const onChange = vi.fn(); + const view = PullRequestProjectFilter({ + projects: [{ id: projectId, title: "T3 Code" }], + value: projectId, + unavailable: new Map(), + onChange, + }); + const radioGroup = findValueChange(view); + expect(radioGroup).toBeDefined(); + + radioGroup?.props.onValueChange(projectId); + expect(onChange).not.toHaveBeenCalled(); + + radioGroup?.props.onValueChange("all"); + expect(onChange).toHaveBeenCalledWith(undefined); + }); +}); diff --git a/apps/web/src/components/pullRequest/PullRequestListFilters.tsx b/apps/web/src/components/pullRequest/PullRequestListFilters.tsx index 5dfad5ae61b..fbae17d5cc2 100644 --- a/apps/web/src/components/pullRequest/PullRequestListFilters.tsx +++ b/apps/web/src/components/pullRequest/PullRequestListFilters.tsx @@ -53,7 +53,9 @@ export function PullRequestFilterPills({ key={option.value} type="button" aria-pressed={option.value === value} - onClick={() => onChange(option.value)} + onClick={() => { + if (option.value !== value) onChange(option.value); + }} className={filterOptionClass(option.value === value)} > @@ -141,7 +143,9 @@ export function PullRequestProviderFilter({