diff --git a/apps/server/package.json b/apps/server/package.json index ca74368348ea..e37fa97ccb20 100644 --- a/apps/server/package.json +++ b/apps/server/package.json @@ -29,6 +29,7 @@ "@ff-labs/fff-node": "0.9.4", "@opencode-ai/sdk": "^1.3.15", "@pierre/diffs": "catalog:", + "diff": "8.0.3", "effect": "catalog:", "msgpackr-extract": "3.0.4", "node-pty": "^1.1.0", diff --git a/apps/server/src/auth/RpcAuthorization.ts b/apps/server/src/auth/RpcAuthorization.ts index 65f590d1a838..ac2e9ce6f2e1 100644 --- a/apps/server/src/auth/RpcAuthorization.ts +++ b/apps/server/src/auth/RpcAuthorization.ts @@ -70,6 +70,7 @@ export const RPC_REQUIRED_SCOPES = { [WS_METHODS.pullRequestsActivity]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsThreadComments]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsDiffFileContents]: AuthOrchestrationReadScope, + [WS_METHODS.pullRequestsFilesViewed]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsRunAction]: AuthOrchestrationOperateScope, [WS_METHODS.pullRequestsUpdate]: AuthOrchestrationOperateScope, [WS_METHODS.pullRequestsComment]: AuthOrchestrationOperateScope, @@ -78,6 +79,7 @@ export const RPC_REQUIRED_SCOPES = { [WS_METHODS.pullRequestsReplyToThread]: AuthOrchestrationOperateScope, [WS_METHODS.pullRequestsSetThreadResolution]: AuthOrchestrationOperateScope, [WS_METHODS.pullRequestsSetReaction]: AuthOrchestrationOperateScope, + [WS_METHODS.pullRequestsSetFilesViewed]: AuthOrchestrationOperateScope, // Read scope like the reads it un-caches: refreshing is part of reading, and a read-only // client pressing refresh must not be told it may not look again. [WS_METHODS.pullRequestsInvalidate]: AuthOrchestrationReadScope, diff --git a/apps/server/src/persistence/Errors.ts b/apps/server/src/persistence/Errors.ts index 03edaec77d63..1772d0a23a6c 100644 --- a/apps/server/src/persistence/Errors.ts +++ b/apps/server/src/persistence/Errors.ts @@ -134,5 +134,6 @@ export type OrchestrationCommandReceiptRepositoryError = export type ProviderSessionRuntimeRepositoryError = PersistenceSqlError | PersistenceDecodeError; export type AuthPairingLinkRepositoryError = PersistenceSqlError | PersistenceDecodeError; export type AuthSessionRepositoryError = PersistenceSqlError | PersistenceDecodeError; +export type PullRequestFilesViewedRepositoryError = PersistenceSqlError | PersistenceDecodeError; export type ProjectionRepositoryError = PersistenceSqlError | PersistenceDecodeError; diff --git a/apps/server/src/persistence/Migrations.ts b/apps/server/src/persistence/Migrations.ts index 92dc18291057..a42ee68f38a2 100644 --- a/apps/server/src/persistence/Migrations.ts +++ b/apps/server/src/persistence/Migrations.ts @@ -59,6 +59,7 @@ import Migration0044 from "./Migrations/044_ClearAutomaticProjectModelDefaults.t import Migration0045 from "./Migrations/045_ProjectionProjectsAutoPull.ts"; import Migration0046 from "./Migrations/046_RepairAutomaticSettlementTimestamps.ts"; import Migration0047 from "./Migrations/047_ProjectionProjectIcon.ts"; +import Migration0048 from "./Migrations/048_PullRequestFilesViewed.ts"; /** * Migration loader with all migrations defined inline. @@ -118,6 +119,7 @@ export const migrationEntries = [ [45, "ProjectionProjectsAutoPull", Migration0045], [46, "RepairAutomaticSettlementTimestamps", Migration0046], [47, "ProjectionProjectIcon", Migration0047], + [48, "PullRequestFilesViewed", Migration0048], ] as const; export const migrationManifest = migrationEntries.map(([id, name]) => [id, name] as const); diff --git a/apps/server/src/persistence/Migrations/048_PullRequestFilesViewed.ts b/apps/server/src/persistence/Migrations/048_PullRequestFilesViewed.ts new file mode 100644 index 000000000000..675df1918a24 --- /dev/null +++ b/apps/server/src/persistence/Migrations/048_PullRequestFilesViewed.ts @@ -0,0 +1,27 @@ +import * as Effect from "effect/Effect"; +import * as SqlClient from "effect/unstable/sql/SqlClient"; + +export default Effect.gen(function* () { + const sql = yield* SqlClient.SqlClient; + + // One row per file a reader has cleared on a host that keeps no record of its own. `revision` + // is what the file was when it was cleared, so a push that changes it is reported as changed + // rather than silently left ticked. It is nullable because a host asked mid-press does not + // always answer: null is no baseline to compare against, which is not the same as the empty + // string, which is the host saying the head has nothing of the file. Unticking deletes the row: + // absent is the resting state, and a table of "not viewed" rows would grow with every diff + // anybody scrolled past. + yield* sql` + CREATE TABLE IF NOT EXISTS pull_request_files_viewed ( + provider TEXT NOT NULL, + host TEXT NOT NULL, + repository TEXT NOT NULL, + number INTEGER NOT NULL, + viewer TEXT NOT NULL, + path TEXT NOT NULL, + revision TEXT, + viewed_at TEXT NOT NULL, + PRIMARY KEY (provider, host, repository, number, viewer, path) + ) WITHOUT ROWID + `; +}); diff --git a/apps/server/src/persistence/PullRequestFilesViewed.ts b/apps/server/src/persistence/PullRequestFilesViewed.ts new file mode 100644 index 000000000000..f9f663ed4aa1 --- /dev/null +++ b/apps/server/src/persistence/PullRequestFilesViewed.ts @@ -0,0 +1,167 @@ +import * as Context from "effect/Context"; +import * as Effect from "effect/Effect"; +import * as Layer from "effect/Layer"; +import * as Schema from "effect/Schema"; +import * as SqlClient from "effect/unstable/sql/SqlClient"; +import * as SqlSchema from "effect/unstable/sql/SqlSchema"; + +import { SourceControlProviderKind } from "@t3tools/contracts"; + +import { + PersistenceDecodeError, + PersistenceSqlError, + type PullRequestFilesViewedRepositoryError, +} from "./Errors.ts"; + +/** + * Which change request, on which host, for which reader. + * + * The host is part of it because a repository path is not unique across installs: the same + * `group/project` exists on gitlab.com and on a self-managed instance, and a mark made against one + * must not turn up on the other. The reader is part of it for the same reason the host's own + * record is per-account: signing in as somebody else must not inherit their ticks. A host that + * will not say who the reader is leaves it empty, which is one reader rather than none. + */ +export const PullRequestFilesViewedScope = Schema.Struct({ + provider: SourceControlProviderKind, + host: Schema.String, + repository: Schema.String, + number: Schema.Int, + viewer: Schema.String, +}); +export type PullRequestFilesViewedScope = typeof PullRequestFilesViewedScope.Type; + +/** A file this reader cleared, and what it was when they cleared it. */ +export const PullRequestFileViewedMark = Schema.Struct({ + path: Schema.String, + /** + * The host's own name for that version of the file, opaque here. + * + * Empty where the host said it had none to give, which is its own answer rather than a missing + * one: a file with no version at the head is one the change request deletes, and it stays + * deleted. Null where the host could not say at all, which is no baseline rather than an empty + * one: stamping such a mark with the empty revision would report the file as changed the moment + * anything did answer, so a mark with no baseline stays cleared until a press replaces it. + */ + revision: Schema.NullOr(Schema.String), +}); +export type PullRequestFileViewedMark = typeof PullRequestFileViewedMark.Type; + +export interface SetPullRequestFilesViewedInput extends PullRequestFilesViewedScope { + readonly files: ReadonlyArray; + /** When the presses landed, as an ISO instant. */ + readonly viewedAt: string; +} + +/** + * The marks this environment keeps for hosts that keep none of their own. + * + * Only cleared files are rows. Unticking deletes rather than writing a "not viewed" row, so the + * table holds what a reader has done and not what they have merely scrolled past. + */ +export class PullRequestFilesViewedRepository extends Context.Service< + PullRequestFilesViewedRepository, + { + readonly list: ( + input: PullRequestFilesViewedScope, + ) => Effect.Effect< + ReadonlyArray, + PullRequestFilesViewedRepositoryError + >; + readonly set: ( + input: SetPullRequestFilesViewedInput, + ) => Effect.Effect; + } +>()("t3/persistence/PullRequestFilesViewed/PullRequestFilesViewedRepository") {} + +function toSqlOrDecodeError(sqlOperation: string, decodeOperation: string) { + return (cause: unknown): PullRequestFilesViewedRepositoryError => + Schema.isSchemaError(cause) + ? PersistenceDecodeError.fromSchemaError(decodeOperation, cause) + : new PersistenceSqlError({ operation: sqlOperation, cause }); +} + +export const make = Effect.gen(function* () { + const sql = yield* SqlClient.SqlClient; + + const listRows = SqlSchema.findAll({ + Request: PullRequestFilesViewedScope, + Result: PullRequestFileViewedMark, + execute: ({ provider, host, repository, number, viewer }) => + sql` + SELECT + path AS "path", + revision AS "revision" + FROM pull_request_files_viewed + WHERE provider = ${provider} + AND host = ${host} + AND repository = ${repository} + AND number = ${number} + AND viewer = ${viewer} + `, + }); + + return PullRequestFilesViewedRepository.of({ + list: (input) => + listRows(input).pipe( + Effect.mapError(toSqlOrDecodeError("listPullRequestFilesViewed", "PullRequestFileViewed")), + ), + + // One statement per file rather than one for the batch: the batch is what a reader ticked in + // the last few hundred milliseconds, so it is a handful of rows on a local database, and a + // mixed batch of clears and un-clears has no single statement anyway. + set: (input) => + // One transaction for the batch. A press is a handful of files, and a failure part way + // through would otherwise leave some of them cleared and the rest not, which the reader + // sees on the next read as marks they never made. + sql + .withTransaction( + Effect.forEach( + input.files, + (file) => + file.viewed + ? sql` + INSERT INTO pull_request_files_viewed ( + provider, + host, + repository, + number, + viewer, + path, + revision, + viewed_at + ) + VALUES ( + ${input.provider}, + ${input.host}, + ${input.repository}, + ${input.number}, + ${input.viewer}, + ${file.path}, + ${file.revision}, + ${input.viewedAt} + ) + ON CONFLICT (provider, host, repository, number, viewer, path) + DO UPDATE SET revision = excluded.revision, viewed_at = excluded.viewed_at + ` + : sql` + DELETE FROM pull_request_files_viewed + WHERE provider = ${input.provider} + AND host = ${input.host} + AND repository = ${input.repository} + AND number = ${input.number} + AND viewer = ${input.viewer} + AND path = ${file.path} + `, + { discard: true }, + ), + ) + .pipe( + Effect.mapError( + (cause) => new PersistenceSqlError({ operation: "setPullRequestFilesViewed", cause }), + ), + ), + }); +}); + +export const layer = Layer.effect(PullRequestFilesViewedRepository, make); diff --git a/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.test.ts b/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.test.ts index d893924b3f2a..6e378c43b1b6 100644 --- a/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.test.ts @@ -29,6 +29,45 @@ function output(stdout: string) { }; } +/** What VcsProcess allows a read that asked for no ceiling of its own. */ +const VCS_DEFAULT_MAX_OUTPUT_BYTES = 1_000_000; + +/** + * The runner as it really behaves: it cuts stdout at the ceiling its caller asked for, and cuts + * it at the process default when the caller asked for none. A read whose response is larger than + * its ceiling gets JSON that stops mid-string, which is the whole cost of an unset ceiling. + */ +function outputWithin(maxOutputBytes: number | undefined, response: string) { + const ceiling = maxOutputBytes ?? VCS_DEFAULT_MAX_OUTPUT_BYTES; + return ceiling >= Buffer.byteLength(response) + ? output(response) + : { ...output(response.slice(0, ceiling)), stdoutTruncated: true }; +} + +/** A fixture's own shape, spelled the way `az` would answer with it. */ +const json = (value: Record) => JSON.stringify(value); + +const pullRequestRow = { + pullRequestId: 42, + title: "Add the page", + status: "active", + sourceRefName: "refs/heads/feat/page", + targetRefName: "refs/heads/main", + creationDate: "2026-07-01T00:00:00Z", + url: "https://dev.azure.com/acme/_apis/git/repositories/web/pullRequests/42", + repository: { name: "web", project: { name: "platform" } }, +}; + +const oneIteration = { + value: [ + { + id: 1, + sourceRefCommit: { commitId: "a".repeat(40) }, + commonRefCommit: { commitId: "b".repeat(40) }, + }, + ], +}; + function pullRequestRows( count: number, firstNumber: number, @@ -56,6 +95,92 @@ function argsOfCall(index: number): ReadonlyArray { return call[0].args; } +/** The output ceiling the nth az invocation asked for, if it asked for one at all. */ +function maxOutputBytesOfCall(index: number) { + const call = mockedExecute.mock.calls[index]; + assert.isDefined(call); + return call[0].maxOutputBytes; +} + +/** A page of change entries the size Azure really answers with, url and object ids and all. */ +function changeEntries(count: number): ReadonlyArray> { + const commit = "c".repeat(40); + return Array.from({ length: count }, (_, index) => { + const path = `/apps/server/src/generated/module-${index}/persisted-projection-${index}.ts`; + return { + changeType: "edit", + item: { + path, + objectId: "a".repeat(40), + originalObjectId: "b".repeat(40), + commitId: commit, + gitObjectType: "blob", + url: `https://dev.azure.com/acme/platform/_apis/git/repositories/6f9c9b7f-0000-0000-0000-000000000000/items${path}?versionType=Commit&version=${commit}`, + }, + }; + }); +} + +/** An Azure identity, which rides along with every comment and every push Azure answers with. */ +function identity(name: string) { + const id = "6f9c9b7f-0000-0000-0000-000000000000"; + return { + displayName: name, + id, + uniqueName: `${name.toLowerCase().replace(/ /g, ".")}@acme.com`, + descriptor: `aad.${"z".repeat(52)}`, + imageUrl: `https://dev.azure.com/acme/_api/_common/identityImage?id=${id}`, + url: `https://spsprodweu1.vssps.visualstudio.com/A${id}/_apis/Identities/${id}`, + _links: { + avatar: { + href: `https://dev.azure.com/acme/_apis/GraphProfile/MemberAvatars/aad.${"z".repeat(52)}`, + }, + }, + }; +} + +/** A review's threads the shape Azure answers with, system threads and identities and all. */ +function threadRows(count: number): ReadonlyArray> { + return Array.from({ length: count }, (_, index) => ({ + id: index + 1, + publishedDate: "2026-07-02T00:00:00Z", + lastUpdatedDate: "2026-07-02T00:00:00Z", + status: "active", + threadContext: { filePath: `/apps/server/src/generated/module-${index}.ts` }, + identities: { 1: identity("Reviewer One") }, + isDeleted: false, + comments: [ + { + id: 1, + parentCommentId: 0, + author: identity("Reviewer One"), + content: `Comment ${index}: ${"this needs another look. ".repeat(20)}`, + publishedDate: "2026-07-02T00:00:00Z", + lastUpdatedDate: "2026-07-02T00:00:00Z", + commentType: "text", + usersLiked: [], + }, + ], + })); +} + +/** A review's iterations the shape Azure answers with, one per push. */ +function iterationRows(count: number): ReadonlyArray> { + return Array.from({ length: count }, (_, index) => ({ + id: index + 1, + description: `Pushed ${index} commits`, + author: identity("Author One"), + createdDate: "2026-07-02T00:00:00Z", + updatedDate: "2026-07-02T00:00:00Z", + sourceRefCommit: { commitId: `${index}`.padStart(40, "a") }, + targetRefCommit: { commitId: "b".repeat(40) }, + commonRefCommit: { commitId: "c".repeat(40) }, + hasMultipleCommits: true, + reason: "push", + push: { pushId: index + 1, date: "2026-07-02T00:00:00Z", pushedBy: identity("Author One") }, + })); +} + afterEach(() => { mockedExecute.mockReset(); }); @@ -107,20 +232,9 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { const response = JSON.stringify(rows); expect(Buffer.byteLength(response)).toBeGreaterThan(1_000_000); - mockedExecute.mockImplementationOnce((input) => { - const maxOutputBytes = - "maxOutputBytes" in input && typeof input.maxOutputBytes === "number" - ? input.maxOutputBytes - : 1_000_000; - return Effect.succeed( - maxOutputBytes >= Buffer.byteLength(response) - ? output(response) - : { - ...output(response.slice(0, maxOutputBytes)), - stdoutTruncated: true, - }, - ); - }); + mockedExecute.mockImplementationOnce((input) => + Effect.succeed(outputWithin(input.maxOutputBytes, response)), + ); const cli = yield* AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli; const batch = yield* cli.listPullRequests({ @@ -516,6 +630,590 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { }), ); + it.effect("names the head's blob as what a cleared file was cleared at", () => + Effect.gen(function* () { + mockedExecute + .mockReturnValueOnce( + Effect.succeed( + output( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ + pullRequestId: 42, + title: "Add the page", + status: "active", + sourceRefName: "refs/heads/feat/page", + targetRefName: "refs/heads/main", + creationDate: "2026-07-01T00:00:00Z", + url: "https://dev.azure.com/acme/_apis/git/repositories/web/pullRequests/42", + repository: { name: "web", project: { name: "platform" } }, + }), + ), + ), + ) + .mockReturnValueOnce( + Effect.succeed( + output( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ + value: [ + { + id: 1, + sourceRefCommit: { commitId: "a".repeat(40) }, + commonRefCommit: { commitId: "b".repeat(40) }, + }, + { + id: 2, + sourceRefCommit: { commitId: "c".repeat(40) }, + commonRefCommit: { commitId: "b".repeat(40) }, + }, + ], + }), + ), + ), + ) + .mockReturnValueOnce( + Effect.succeed( + output( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ + changeEntries: [ + { changeType: "edit", item: { path: "/README.md", objectId: "8f80" } }, + { changeType: "add", item: { path: "/DEMO.md", objectId: "0ca4" } }, + ], + }), + ), + ), + ); + const provider = yield* AzureDevOpsPullRequestProvider.make; + + // Kept here rather than on Azure: its own record of what a reader has read is behind an + // undocumented endpoint, so the marks belong to this environment and need a revision of + // their own to tell a re-push from a file still as it was read. + assert.strictEqual(provider.capabilities.viewedFiles, "environment"); + assert.isDefined(provider.getFileRevisions); + const answer = yield* provider.getFileRevisions({ + cwd: "/w", + repository: "web", + host: "dev.azure.com", + number: 42, + paths: ["README.md"], + }); + + // The latest push, since an iteration's changes are reported against the merge base rather + // than against the push before it. + expect(argsOfCall(2)).toContain("iterationId=2"); + // Only what was asked for. DEMO.md changed too, and nobody has marked it. + expect([...answer.revisions]).toEqual([["README.md", "8f80"]]); + }), + ); + + it.effect("reads where a pull request lives once, however often it is asked about", () => + Effect.gen(function* () { + // A pull request cannot move repositories, and the marks would otherwise pay for a whole + // pull request read every time they checked whether a file had been pushed to. + const pullRequest = Effect.succeed( + output( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ + pullRequestId: 42, + title: "Add the page", + status: "active", + sourceRefName: "refs/heads/feat/page", + targetRefName: "refs/heads/main", + creationDate: "2026-07-01T00:00:00Z", + url: "https://dev.azure.com/acme/_apis/git/repositories/web/pullRequests/42", + repository: { name: "web", project: { name: "platform" } }, + }), + ), + ); + const iterations = () => + Effect.succeed( + output( + JSON.stringify({ + value: [ + { + id: 1, + sourceRefCommit: { commitId: "a".repeat(40) }, + commonRefCommit: { commitId: "b".repeat(40) }, + }, + ], + }), + ), + ); + const changes = () => + Effect.succeed( + output( + JSON.stringify({ + changeEntries: [ + { changeType: "edit", item: { path: "/README.md", objectId: "8f80" } }, + ], + }), + ), + ); + mockedExecute + .mockReturnValueOnce(pullRequest) + .mockReturnValueOnce(iterations()) + .mockReturnValueOnce(changes()) + .mockReturnValueOnce(iterations()) + .mockReturnValueOnce(changes()); + const provider = yield* AzureDevOpsPullRequestProvider.make; + const read = provider.getFileRevisions; + assert.isDefined(read); + const ask = () => + read({ + cwd: "/w", + repository: "web", + host: "dev.azure.com", + number: 42, + paths: ["README.md"], + }); + + yield* ask(); + const again = yield* ask(); + + assert.strictEqual(mockedExecute.mock.calls.length, 5); + // The second read goes straight to the pushes, and still answers with the head's blob. + expect(argsOfCall(3)).toContain("pullRequestIterations"); + expect([...again.revisions]).toEqual([["README.md", "8f80"]]); + }), + ); + + it.effect( + "answers for a marked file the pull request no longer changes as the empty version", + () => + Effect.gen(function* () { + // Which is what was stored for it when it was ticked with nothing on the head, so a file + // the pull request deletes is cleared once and stays cleared. + mockedExecute + .mockReturnValueOnce( + Effect.succeed( + output( + json({ + pullRequestId: 42, + title: "Add the page", + status: "active", + sourceRefName: "refs/heads/feat/page", + targetRefName: "refs/heads/main", + creationDate: "2026-07-01T00:00:00Z", + url: "https://dev.azure.com/acme/_apis/git/repositories/web/pullRequests/42", + repository: { name: "web", project: { name: "platform" } }, + }), + ), + ), + ) + .mockReturnValueOnce( + Effect.succeed( + output( + json({ + value: [ + { + id: 1, + sourceRefCommit: { commitId: "a".repeat(40) }, + commonRefCommit: { commitId: "b".repeat(40) }, + }, + ], + }), + ), + ), + ) + .mockReturnValueOnce(Effect.succeed(output('{"changeEntries":[]}'))); + const provider = yield* AzureDevOpsPullRequestProvider.make; + assert.isDefined(provider.getFileRevisions); + + const answer = yield* provider.getFileRevisions({ + cwd: "/w", + repository: "web", + host: "dev.azure.com", + number: 42, + paths: ["GONE.md"], + }); + + expect([...answer.revisions]).toEqual([["GONE.md", ""]]); + }), + ); + + it.effect("says nothing about the files past the end of a change it gave up following", () => + Effect.gen(function* () { + // Every page is an `az` process of its own, so a change past the ceiling stops being + // followed. A path nobody looked at must not be answered for as deleted. + const entries = (from: number, count: number) => + Array.from({ length: count }, (_, index) => ({ + changeType: "edit", + item: { path: `/src/f${from + index}.ts`, objectId: `blob-${from + index}` }, + })); + mockedExecute + .mockReturnValueOnce( + Effect.succeed( + output( + json({ + pullRequestId: 42, + title: "Add the page", + status: "active", + sourceRefName: "refs/heads/feat/page", + targetRefName: "refs/heads/main", + creationDate: "2026-07-01T00:00:00Z", + url: "https://dev.azure.com/acme/_apis/git/repositories/web/pullRequests/42", + repository: { name: "web", project: { name: "platform" } }, + }), + ), + ), + ) + .mockReturnValueOnce( + Effect.succeed( + output( + json({ + value: [ + { + id: 1, + sourceRefCommit: { commitId: "a".repeat(40) }, + commonRefCommit: { commitId: "b".repeat(40) }, + }, + ], + }), + ), + ), + ) + .mockReturnValueOnce( + Effect.succeed(output(json({ changeEntries: entries(0, 5_000), nextSkip: 5_000 }))), + ) + .mockReturnValueOnce( + Effect.succeed(output(json({ changeEntries: entries(5_000, 5_000), nextSkip: 10_000 }))), + ); + const provider = yield* AzureDevOpsPullRequestProvider.make; + assert.isDefined(provider.getFileRevisions); + + const answer = yield* provider.getFileRevisions({ + cwd: "/w", + repository: "web", + host: "dev.azure.com", + number: 42, + paths: ["src/f1.ts", "src/f5001.ts", "src/past-the-cut.ts"], + }); + + // The second page picks up where the first said it ended. + expect(argsOfCall(3)).toContain("$skip=5000"); + expect([...answer.revisions]).toEqual([ + ["src/f1.ts", "blob-1"], + ["src/f5001.ts", "blob-5001"], + ]); + }), + ); + + it.effect("stops following pages by what Azure counts, not by what survives decoding", () => + Effect.gen(function* () { + mockedExecute + .mockReturnValueOnce( + Effect.succeed( + output( + json({ + pullRequestId: 42, + title: "Add the page", + status: "active", + sourceRefName: "refs/heads/feat/page", + targetRefName: "refs/heads/main", + creationDate: "2026-07-01T00:00:00Z", + url: "https://dev.azure.com/acme/_apis/git/repositories/web/pullRequests/42", + repository: { name: "web", project: { name: "platform" } }, + }), + ), + ), + ) + .mockReturnValueOnce( + Effect.succeed( + output( + json({ + value: [ + { + id: 1, + sourceRefCommit: { commitId: "a".repeat(40) }, + commonRefCommit: { commitId: "b".repeat(40) }, + }, + ], + }), + ), + ), + ) + // Nothing a review can show, so every page decodes to nothing at all and a ceiling counted + // in files would never be reached however long the walk went on. + .mockImplementation((command) => { + const skip = command.args.find((arg) => arg.startsWith("$skip=")); + const from = Number(skip?.slice("$skip=".length) ?? 0); + return Effect.succeed( + output( + json({ + changeEntries: [{ changeType: "add", item: { path: "/src", isFolder: true } }], + nextSkip: from + 2_000, + }), + ), + ); + }); + const provider = yield* AzureDevOpsPullRequestProvider.make; + assert.isDefined(provider.getFileRevisions); + + const answer = yield* provider.getFileRevisions({ + cwd: "/w", + repository: "web", + host: "dev.azure.com", + number: 42, + paths: ["src/page.ts"], + }); + + // The pull request, its pushes, and five pages: the walk gives up on Azure's own offset + // rather than spending an `az` process a page for as long as Azure keeps paging. + assert.strictEqual(mockedExecute.mock.calls.length, 7); + // And it read part of a change, so it says nothing about the file it never saw. + assert.strictEqual(answer.revisions.size, 0); + }), + ); + + it.effect("leaves one file the host would not hand over listed without its hunks", () => + Effect.gen(function* () { + mockedExecute + .mockReturnValueOnce(Effect.succeed(output(json(pullRequestRow)))) + .mockReturnValueOnce(Effect.succeed(output(json(oneIteration)))) + .mockReturnValueOnce( + Effect.succeed( + output( + json({ + changeEntries: [ + { changeType: "add", item: { path: "/huge.bin", objectId: "8f80" } }, + { changeType: "add", item: { path: "/DEMO.md", objectId: "0ca4" } }, + ], + }), + ), + ), + ) + .mockReturnValueOnce( + Effect.fail( + new AzureDevOpsCli.AzureDevOpsCommandFailedError({ + operation: "execute", + command: "az", + cwd: "/w", + argumentCount: 1, + cause: "the blob is past what the route will carry", + }), + ), + ) + .mockReturnValueOnce(Effect.succeed(output(json({ content: "hello\n" })))); + const provider = yield* AzureDevOpsPullRequestProvider.make; + + const slice = yield* provider.getDiff({ + cwd: "/w", + repository: "web", + host: "dev.azure.com", + number: 42, + }); + + assert.isTrue(slice.truncated); + expect(slice.patch).toContain("diff --git a/huge.bin b/huge.bin"); + // And the file behind it still renders, which is the point of giving up on one file. + expect(slice.patch).toContain("+hello"); + }), + ); + + it.effect("fails the whole read when it is the connection that would not answer", () => + Effect.gen(function* () { + // A rate limit is not this file's problem, and answering with a change full of files listed + // without their hunks would read as a change nobody can see rather than as a host to wait + // for. + mockedExecute + .mockReturnValueOnce(Effect.succeed(output(json(pullRequestRow)))) + .mockReturnValueOnce(Effect.succeed(output(json(oneIteration)))) + .mockReturnValueOnce( + Effect.succeed( + output( + json({ + changeEntries: [ + { changeType: "add", item: { path: "/DEMO.md", objectId: "0ca4" } }, + ], + }), + ), + ), + ) + .mockReturnValueOnce( + Effect.fail( + new AzureDevOpsCli.AzureDevOpsCliRateLimitError({ + operation: "execute", + command: "az", + cwd: "/w", + argumentCount: 1, + cause: "429", + }), + ), + ); + const provider = yield* AzureDevOpsPullRequestProvider.make; + + const error = yield* Effect.flip( + provider.getDiff({ cwd: "/w", repository: "web", host: "dev.azure.com", number: 42 }), + ); + + assert.strictEqual(error.reason, "rate-limited"); + }), + ); + + it.effect("takes Azure's own word on a file it will not spell out", () => + Effect.gen(function* () { + mockedExecute + .mockReturnValueOnce( + Effect.succeed( + output( + json({ + pullRequestId: 42, + title: "Add the page", + status: "active", + sourceRefName: "refs/heads/feat/page", + targetRefName: "refs/heads/main", + creationDate: "2026-07-01T00:00:00Z", + url: "https://dev.azure.com/acme/_apis/git/repositories/web/pullRequests/42", + repository: { name: "web", project: { name: "platform" } }, + }), + ), + ), + ) + .mockReturnValueOnce( + Effect.succeed( + output( + json({ + value: [ + { + id: 1, + sourceRefCommit: { commitId: "a".repeat(40) }, + commonRefCommit: { commitId: "b".repeat(40) }, + }, + ], + }), + ), + ), + ) + .mockReturnValueOnce( + Effect.succeed( + output( + json({ + changeEntries: [ + { changeType: "add", item: { path: "/logo.png", objectId: "8f80" } }, + ], + }), + ), + ), + ) + .mockReturnValueOnce( + Effect.succeed( + output(json({ content: "iVBORw0KGgo=", contentMetadata: { isBinary: true } })), + ), + ); + const provider = yield* AzureDevOpsPullRequestProvider.make; + + const slice = yield* provider.getDiff({ + cwd: "/w", + repository: "web", + host: "dev.azure.com", + number: 42, + }); + + // Azure leaves the metadata out unless it is asked for, and without it every file reads as + // text however it was stored. + expect(argsOfCall(3)).toContain("includeContentMetadata=true"); + expect(slice.patch).toContain("Binary files a/logo.png and b/logo.png differ"); + assert.isTrue(slice.truncated); + }), + ); + + it.effect("stops following pages when one of them does not move the cursor on", () => + Effect.gen(function* () { + mockedExecute + .mockReturnValueOnce( + Effect.succeed( + output( + json({ + pullRequestId: 42, + title: "Add the page", + status: "active", + sourceRefName: "refs/heads/feat/page", + targetRefName: "refs/heads/main", + creationDate: "2026-07-01T00:00:00Z", + url: "https://dev.azure.com/acme/_apis/git/repositories/web/pullRequests/42", + repository: { name: "web", project: { name: "platform" } }, + }), + ), + ), + ) + .mockReturnValueOnce( + Effect.succeed( + output( + json({ + value: [ + { + id: 1, + sourceRefCommit: { commitId: "a".repeat(40) }, + commonRefCommit: { commitId: "b".repeat(40) }, + }, + ], + }), + ), + ), + ) + .mockReturnValueOnce( + Effect.succeed( + output( + json({ + changeEntries: [{ changeType: "edit", item: { path: "/a.ts", objectId: "8f80" } }], + nextSkip: 2_000, + }), + ), + ), + ) + .mockReturnValueOnce( + Effect.succeed( + output( + json({ + changeEntries: [{ changeType: "edit", item: { path: "/b.ts", objectId: "0ca4" } }], + nextSkip: 2_000, + }), + ), + ), + ); + const provider = yield* AzureDevOpsPullRequestProvider.make; + assert.isDefined(provider.getFileRevisions); + + const answer = yield* provider.getFileRevisions({ + cwd: "/w", + repository: "web", + host: "dev.azure.com", + number: 42, + paths: ["a.ts", "b.ts", "unlisted.ts"], + }); + + // Four reads and no more: a page pointing at where it already is would be read forever. + assert.strictEqual(mockedExecute.mock.calls.length, 4); + // And what was read is not the whole change, so the file nobody listed is left unanswered + // rather than reported as gone from the change request. + expect([...answer.revisions]).toEqual([ + ["a.ts", "8f80"], + ["b.ts", "0ca4"], + ]); + }), + ); + + it.effect("asks Azure nothing when no file has been ticked off", () => + Effect.gen(function* () { + mockedExecute.mockReturnValue(Effect.succeed(output('{"changeEntries":[]}'))); + const provider = yield* AzureDevOpsPullRequestProvider.make; + assert.isDefined(provider.getFileRevisions); + + const answer = yield* provider.getFileRevisions({ + cwd: "/w", + repository: "web", + host: "dev.azure.com", + number: 42, + paths: [], + }); + + expect(answer.revisions.size).toBe(0); + // Nothing was marked, so Azure was not asked at all. + assert.strictEqual(mockedExecute.mock.calls.length, 0); + }), + ); + it.effect("reads the conversation through the REST API, pinned to a version", () => Effect.gen(function* () { mockedExecute.mockReturnValueOnce( @@ -539,14 +1237,134 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { const comments = yield* cli.listThreads({ cwd: "/w", - threadsUrl: "https://dev.azure.com/acme/platform/_apis/git/r/web/pullRequests/42/threads", + location: { project: "platform", repository: "web" }, + number: 42, }); assert.strictEqual(comments.length, 1); - expect(argsOfCall(0)).toContain("rest"); - expect(argsOfCall(0)).toContain( - "https://dev.azure.com/acme/platform/_apis/git/r/web/pullRequests/42/threads?api-version=7.1", + // `az devops invoke` rather than `az rest`: it signs in the way the azure-devops extension + // does, and `az rest` mints its own token against whichever tenant `az` defaults to. + expect(argsOfCall(0)).toContain("invoke"); + expect(argsOfCall(0)).toContain("pullRequestThreads"); + expect(argsOfCall(0)).toContain("project=platform"); + expect(argsOfCall(0)).toContain("repositoryId=web"); + expect(argsOfCall(0)).toContain("pullRequestId=42"); + // A review's threads grow with how long it ran, and this route does not page, so the read + // asks for more than the process default rather than taking whatever it is given. + expect(maxOutputBytesOfCall(0)).toBeGreaterThan(VCS_DEFAULT_MAX_OUTPUT_BYTES); + }), + ); + + it.effect("reads a long review's threads, which are past the default output limit", () => + Effect.gen(function* () { + const response = json({ value: threadRows(800) }); + // Azure opens a thread per vote and per ref update beside the ones people wrote, and every + // comment carries a full identity, so a review argued over for weeks outgrows the default. + expect(Buffer.byteLength(response)).toBeGreaterThan(VCS_DEFAULT_MAX_OUTPUT_BYTES); + mockedExecute.mockImplementationOnce((input) => + Effect.succeed(outputWithin(input.maxOutputBytes, response)), ); + const cli = yield* AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli; + + const comments = yield* cli.listThreads({ + cwd: "/w", + location: { project: "platform", repository: "web" }, + number: 42, + }); + + assert.strictEqual(comments.length, 800); + }), + ); + + it.effect("reads a long review's iterations, which are past the default output limit", () => + Effect.gen(function* () { + const response = json({ value: iterationRows(1_200) }); + // This route does not page, so the whole history arrives at once. Cut at the default it is + // JSON stopping mid-string, and every diff and file revision read on this host fails with + // it, since each of them starts by asking which iteration is the latest. + expect(Buffer.byteLength(response)).toBeGreaterThan(VCS_DEFAULT_MAX_OUTPUT_BYTES); + mockedExecute.mockImplementationOnce((input) => + Effect.succeed(outputWithin(input.maxOutputBytes, response)), + ); + const cli = yield* AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli; + + const iterations = yield* cli.listIterations({ + cwd: "/w", + location: { project: "platform", repository: "web" }, + number: 42, + }); + + assert.strictEqual(iterations.length, 1_200); + assert.strictEqual(iterations.at(-1)?.id, 1_200); + }), + ); + + it.effect("reads a full page of change entries, which is past the default output limit", () => + Effect.gen(function* () { + const response = json({ changeEntries: changeEntries(2_000) }); + // Azure's own maximum for this route, and every entry carries a path, a url and three + // object ids, so an ordinary page of a large change already outgrows the default. + expect(Buffer.byteLength(response)).toBeGreaterThan(VCS_DEFAULT_MAX_OUTPUT_BYTES); + mockedExecute.mockImplementationOnce((input) => + Effect.succeed(outputWithin(input.maxOutputBytes, response)), + ); + const cli = yield* AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli; + + const page = yield* cli.listIterationChanges({ + cwd: "/w", + location: { project: "platform", repository: "web" }, + number: 42, + iterationId: 1, + }); + + // Cut at the default this would arrive as JSON stopping mid-string, and a perfectly + // ordinary page would be reported as a host answering with nonsense. + assert.strictEqual(page.changes.length, 2_000); + assert.isFalse(page.truncated); + }), + ); + + it.effect("reads a file whose JSON envelope is past the default output limit", () => + Effect.gen(function* () { + // Under a megabyte as bytes on the host, so this is a file the other hosts hand over. + const file = "const value = 1;\n".repeat(57_000); + const response = json({ content: file }); + expect(Buffer.byteLength(file)).toBeLessThan(VCS_DEFAULT_MAX_OUTPUT_BYTES); + // And past it once Azure wraps it, because there is no route here that serves the bytes. + expect(Buffer.byteLength(response)).toBeGreaterThan(VCS_DEFAULT_MAX_OUTPUT_BYTES); + mockedExecute.mockImplementationOnce((input) => + Effect.succeed(outputWithin(input.maxOutputBytes, response)), + ); + const cli = yield* AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli; + + const item = yield* cli.readItemContent({ + cwd: "/w", + location: { project: "platform", repository: "web" }, + path: "src/generated/schema.ts", + commit: "a".repeat(40), + }); + + assert.strictEqual(item.contents, file); + assert.isFalse(item.isBinary); + }), + ); + + it.effect("asks for a file by Azure's own spelling of its path", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce(Effect.succeed(output(json({ content: "const a = 1;" })))); + const cli = yield* AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli; + + // The path carried around here has had Azure's leading slash taken off so it matches the + // patch and the viewed mark. The items route is documented in Azure's spelling, so it goes + // back on the way out rather than being sent as the shorter name. + yield* cli.readItemContent({ + cwd: "/w", + location: { project: "platform", repository: "web" }, + path: "src/app.ts", + commit: "a".repeat(40), + }); + + expect(argsOfCall(0)).toContain("path=/src/app.ts"); }), ); diff --git a/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.ts b/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.ts index fe87692e1cc3..f90fa5f3683c 100644 --- a/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.ts +++ b/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.ts @@ -13,11 +13,18 @@ import type { import * as AzureDevOpsCli from "../sourceControl/AzureDevOpsCli.ts"; import { + decodeItemContentJson, + decodeIterationChangesJson, + decodeIterationsJson, decodePullRequestJson, decodePullRequestListJson, decodeThreadsJson, decodeViewerJson, + type AzureDevOpsChangeEntry, + type AzureDevOpsItemContent, + type AzureDevOpsIteration, type AzureDevOpsPullRequest, + type AzureDevOpsRepositoryLocation, } from "./azureDevOpsPullRequestJson.ts"; import type { ProviderListCursor } from "./PullRequestProvider.ts"; @@ -112,6 +119,50 @@ export type AzureDevOpsPullRequestCliError = /** The version every REST call below is pinned to, so a new default cannot reshape a response. */ const REST_API_VERSION = "7.1"; const PULL_REQUEST_LIST_MAX_OUTPUT_BYTES = 16 * 1024 * 1024; +/** + * A full page of change entries is two thousand files, each carrying its path, its url and + * several object ids, which is past the megabyte a read is given by default. Output cut at that + * ceiling arrives here as JSON that will not parse, so a change large enough to be paged would + * report itself as a host returning nonsense rather than as the ordinary page it is. + */ +const CHANGE_ENTRIES_MAX_OUTPUT_BYTES = 8 * 1024 * 1024; +/** + * Four times the megabyte of file the other hosts hand over, because Azure has no route that + * serves the bytes themselves: the file arrives inside a JSON envelope, escaped if it is text and + * base64 if it is not, and both are larger than the file they carry. + */ +const ITEM_CONTENT_MAX_OUTPUT_BYTES = 4 * 1024 * 1024; +/** + * What a review's own history is given. Neither of these routes pages, so each answers with the + * whole of it at once and grows with how long the review ran rather than with how large the change + * is. Threads are the nearer ceiling of the two: Azure opens one per vote and per ref update + * alongside the ones people wrote, and every comment carries a full identity beside its text, so + * the answer is far larger than the handful of fields read back out of it. Cut at the default, + * both arrive as JSON that stops mid-string, and a long review would report its host as answering + * with nonsense. + */ +const REVIEW_HISTORY_MAX_OUTPUT_BYTES = 8 * 1024 * 1024; + +/** Azure's own ceiling for one page of an iteration's changes. */ +const CHANGE_ENTRIES_PER_PAGE = 2000; + +/** + * Where following the pages stops, counted in the entries Azure was asked to skip rather than in + * the files that survived decoding. Every page is an `az` process of its own, and a change this + * long is past what any reader will get through, so the read gives up rather than spending a + * minute of spawns on it. Saying so is the point: the diff reports itself as incomplete instead + * of presenting five pages as the whole change. + * + * Azure's own count is what bounds this, because a page can be entirely folders and other entries + * a review has nothing to show for. Bounding on what was kept would follow such a change forever. + */ +const MAX_CHANGE_ENTRIES = 10_000; + +/** What an iteration changed, and whether following its pages reached the end of it. */ +export interface AzureDevOpsIterationChanges { + readonly changes: ReadonlyArray; + readonly truncated: boolean; +} export class AzureDevOpsPullRequestCli extends Context.Service< AzureDevOpsPullRequestCli, @@ -150,9 +201,42 @@ export class AzureDevOpsPullRequestCli extends Context.Service< /** Threads are not reachable through `az repos pr`, so they come from the REST API. */ readonly listThreads: (input: { readonly cwd: string; - readonly threadsUrl: string; + readonly location: AzureDevOpsRepositoryLocation; + readonly number: number; }) => Effect.Effect, AzureDevOpsPullRequestCliError>; + /** + * The pushes a pull request has had, oldest first. Azure hangs the changed files off an + * iteration rather than off the pull request, so reading a diff starts here. + */ + readonly listIterations: (input: { + readonly cwd: string; + readonly location: AzureDevOpsRepositoryLocation; + readonly number: number; + }) => Effect.Effect, AzureDevOpsPullRequestCliError>; + + /** + * What one iteration changed, against the merge base rather than against the previous push, + * which is the whole of the pull request rather than the latest slice of it. + */ + readonly listIterationChanges: (input: { + readonly cwd: string; + readonly location: AzureDevOpsRepositoryLocation; + readonly number: number; + readonly iterationId: number; + }) => Effect.Effect; + + /** + * One file's text at one commit. Azure has no diff route that carries content, so both sides + * of every changed file are read this way and the patch is made from them here. + */ + readonly readItemContent: (input: { + readonly cwd: string; + readonly location: AzureDevOpsRepositoryLocation; + readonly path: string; + readonly commit: string; + }) => Effect.Effect; + readonly runPullRequestAction: (input: { readonly cwd: string; readonly number: number; @@ -280,6 +364,77 @@ export const make = Effect.gen(function* () { ...(input.maxOutputBytes === undefined ? {} : { maxOutputBytes: input.maxOutputBytes }), }); + /** + * A REST route reached through `az devops invoke`, which addresses it by area, resource and + * route parameters rather than by URL. It is used in place of `az rest` because it signs in the + * way the azure-devops extension does, and `az rest` mints its own token against the tenant `az` + * defaults to. For an organisation in any other tenant that token is rejected and Azure answers + * with a sign-in page, which arrives here as unreadable output rather than as a failure. + */ + const invoke = (input: { + readonly cwd: string; + readonly operation: string; + readonly resource: string; + readonly routeParameters: ReadonlyArray; + readonly queryParameters?: ReadonlyArray; + readonly maxOutputBytes?: number; + readonly decode: (raw: string) => Result.Result; + }): Effect.Effect => + executeJson({ + cwd: input.cwd, + ...(input.maxOutputBytes === undefined ? {} : { maxOutputBytes: input.maxOutputBytes }), + args: [ + "devops", + "invoke", + ...detectArgs, + "--area", + "git", + "--resource", + input.resource, + "--api-version", + REST_API_VERSION, + "--route-parameters", + ...input.routeParameters, + ...(input.queryParameters === undefined + ? [] + : ["--query-parameters", ...input.queryParameters]), + ], + }).pipe( + Effect.flatMap((result) => { + const decoded = input.decode(result.stdout.trim()); + return Result.isSuccess(decoded) + ? Effect.succeed(decoded.success) + : Effect.fail( + new AzureDevOpsPullRequestReadError({ + command: "az", + cwd: input.cwd, + operation: input.operation, + cause: decoded.failure, + }), + ); + }), + ); + + /** + * Azure names its own items with a leading slash, which the repository paths carried around + * here have had taken off so they match the patch and the viewed mark. Put it back on the way + * out, because the items route is documented in Azure's own spelling. + */ + const toItemPath = (path: string) => (path.startsWith("/") ? path : `/${path}`); + + const repositoryRoute = (location: AzureDevOpsRepositoryLocation): ReadonlyArray => [ + `project=${location.project}`, + `repositoryId=${location.repository}`, + ]; + + const pullRequestRoute = (input: { + readonly location: AzureDevOpsRepositoryLocation; + readonly number: number; + }): ReadonlyArray => [ + ...repositoryRoute(input.location), + `pullRequestId=${input.number}`, + ]; + /** * 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. @@ -447,30 +602,80 @@ export const make = Effect.gen(function* () { ), listThreads: (input) => - executeJson({ + invoke({ cwd: input.cwd, - args: [ - "rest", - "--method", - "get", - "--url", - `${input.threadsUrl}?api-version=${REST_API_VERSION}`, + operation: "listThreads", + resource: "pullRequestThreads", + routeParameters: pullRequestRoute(input), + maxOutputBytes: REVIEW_HISTORY_MAX_OUTPUT_BYTES, + decode: decodeThreadsJson, + }), + + listIterations: (input) => + invoke({ + cwd: input.cwd, + operation: "listIterations", + resource: "pullRequestIterations", + routeParameters: pullRequestRoute(input), + maxOutputBytes: REVIEW_HISTORY_MAX_OUTPUT_BYTES, + decode: decodeIterationsJson, + }), + + listIterationChanges: (input) => { + const page = (skip: number) => + invoke({ + cwd: input.cwd, + operation: "listIterationChanges", + resource: "pullRequestIterationChanges", + routeParameters: [...pullRequestRoute(input), `iterationId=${input.iterationId}`], + // Azure pages this route at 1000 entries by default; this is its own maximum per page, + // and it names where the next page starts rather than answering with the whole change. + queryParameters: [`$top=${CHANGE_ENTRIES_PER_PAGE}`, `$skip=${skip}`], + maxOutputBytes: CHANGE_ENTRIES_MAX_OUTPUT_BYTES, + decode: decodeIterationChangesJson, + }); + const from = ( + skip: number, + collected: ReadonlyArray, + ): Effect.Effect => + page(skip).pipe( + Effect.flatMap((answer) => { + const changes = [...collected, ...answer.changes]; + // The last page names no page after it, and only that is the end of the change. + if (answer.nextSkip === null) return Effect.succeed({ changes, truncated: false }); + // A page pointing at where the read already is would be followed forever, and one + // past the ceiling is a change nobody will read to the end of. Both stop the read + // and both say so, rather than presenting part of a change as the whole of it. + return answer.nextSkip <= skip || answer.nextSkip >= MAX_CHANGE_ENTRIES + ? Effect.succeed({ changes, truncated: true }) + : from(answer.nextSkip, changes); + }), + ); + return from(0, []); + }, + + readItemContent: (input) => + invoke({ + cwd: input.cwd, + operation: "readItemContent", + resource: "items", + routeParameters: repositoryRoute(input.location), + queryParameters: [ + `path=${toItemPath(input.path)}`, + "versionDescriptor.versionType=commit", + `versionDescriptor.version=${input.commit}`, + "includeContent=true", + // Azure leaves `contentMetadata` out unless this is asked for, and with it goes its own + // word on whether the file is binary — which is the only reliable one, since a binary + // file arrives encoded rather than as the bytes it is on the host. + "includeContentMetadata=true", + // Without this Azure answers with the file's own bytes rather than with a JSON + // envelope, and `az devops invoke` refuses anything it cannot parse as JSON. + "$format=json", ], - }).pipe( - Effect.flatMap((result) => { - const decoded = decodeThreadsJson(result.stdout.trim()); - return Result.isSuccess(decoded) - ? Effect.succeed(decoded.success) - : Effect.fail( - new AzureDevOpsPullRequestReadError({ - command: "az", - cwd: input.cwd, - operation: "listThreads", - cause: decoded.failure, - }), - ); - }), - ), + maxOutputBytes: ITEM_CONTENT_MAX_OUTPUT_BYTES, + decode: decodeItemContentJson, + }), setPullRequestReviewers: (input) => input.reviewers.some((reviewer) => !isReviewerName(reviewer)) diff --git a/apps/server/src/pullRequest/AzureDevOpsPullRequestProvider.ts b/apps/server/src/pullRequest/AzureDevOpsPullRequestProvider.ts index 8461e57d5685..89a5ddc55d98 100644 --- a/apps/server/src/pullRequest/AzureDevOpsPullRequestProvider.ts +++ b/apps/server/src/pullRequest/AzureDevOpsPullRequestProvider.ts @@ -2,20 +2,38 @@ import * as Effect from "effect/Effect"; import type { PullRequestCapabilities, PullRequestViewerPermissions } from "@t3tools/contracts"; import * as AzureDevOpsPullRequestCli from "./AzureDevOpsPullRequestCli.ts"; +import { + azureDevOpsFilePatch, + azureDevOpsUnreadableFilePatch, + formatAzureDevOpsDiffCursor, + parseAzureDevOpsDiffCursor, + MAX_DIFF_SLICE_BYTES, + byteLength, + MAX_FILE_DIFF_MILLIS, + type AzureDevOpsFileTexts, +} from "./azureDevOpsDiff.ts"; import { PullRequestProviderError, type PullRequestProviderFailure, type ProviderChangeRequest, type ProviderChangeRequestActivity, type ProviderChangeRequestDetail, + type ProviderDiffSlice, type PullRequestProviderApi, } from "./PullRequestProvider.ts"; -import type { AzureDevOpsPullRequest } from "./azureDevOpsPullRequestJson.ts"; +import type { AzureDevOpsIterationChanges } from "./AzureDevOpsPullRequestCli.ts"; +import type { + AzureDevOpsChangeEntry, + AzureDevOpsItemContent, + AzureDevOpsIteration, + AzureDevOpsPullRequest, + AzureDevOpsRepositoryLocation, +} from "./azureDevOpsPullRequestJson.ts"; const CAPABILITIES: PullRequestCapabilities = { - // `az repos pr` has no diff command, and the REST route reports changed files without their - // contents, so there is no patch to show. The Code tab is hidden rather than empty. - diff: false, + // Azure serves no patch of its own, so the one the Code tab reads is built here out of the + // files an iteration changed and both sides of each of them. + diff: true, // Reading a conversation is a plain REST read, but posting one is not something this can // claim without having run it, so the composer stays hidden. comment: false, @@ -33,7 +51,8 @@ const CAPABILITIES: PullRequestCapabilities = { // `az repos pr list` filters by status, creator, reviewer and branch, and by no text at all. search: false, reactions: false, - // With no patch to show there are no lines to write against, so nothing here is offered. + // The patch has lines to write against, but writing a remark at all is what Azure is not + // offered for here, so nothing in a review is either. review: { inlineComment: false, reply: false, resolve: false, verdicts: [] }, // `az repos pr reviewer add` and `remove` name identities, and nothing anywhere in `az repos` // lists the ones this repository could name — that lives behind the identity and graph APIs, a @@ -44,6 +63,11 @@ const CAPABILITIES: PullRequestCapabilities = { // Rewriting a remark is false for the same reason posting one is: this cannot put a remark on // Azure DevOps at all, so there is nothing here it could rewrite either. edit: { changeRequest: true, comment: false }, + // Azure does keep a viewed record of its own, but only behind the undocumented contribution + // endpoint its web UI talks to, keyed on an iteration so a push would drop every mark anyway. + // So they are kept here instead, and the client says whose they are rather than implying the + // Azure DevOps page will show them. + viewedFiles: "environment", }; /** @@ -85,8 +109,8 @@ function toChangeRequest(pullRequest: AzureDevOpsPullRequest): ProviderChangeReq state: pullRequest.state, isDraft: pullRequest.isDraft, mergeability: pullRequest.mergeability, - // Azure reports no line counts on a pull request, and with no patch to read there is - // nothing to count them from either. + // Azure counts a pull request's files but never its lines, and counting them here would mean + // reading every file on both sides of every row of a listing. additions: 0, deletions: 0, createdAt: pullRequest.createdAt, @@ -121,6 +145,119 @@ export const make = Effect.gen(function* () { }), ); + /** A pull request Azure could not place has no diff to read, which reads as an empty one. */ + const EMPTY_DIFF_SLICE: ProviderDiffSlice = { patch: "", truncated: false, nextCursor: null }; + + /** + * Where a pull request's repository lives, which is the route every other read of it needs and + * the one thing only the pull request itself states. A pull request cannot move between + * repositories, so it is remembered rather than re-read: the marks alone would otherwise pay for + * a whole pull request read every time they checked whether a file had been pushed to. + * + * Bounded and oldest-first, since a long-lived server sees far more pull requests than a reader + * ever has open. + */ + const LOCATION_CACHE_CAPACITY = 128; + const locations = new Map(); + + const locationOf = (input: { readonly cwd: string; readonly number: number }) => { + const key = `${input.cwd} ${input.number}`; + const held = locations.get(key); + if (held !== undefined) return Effect.succeed(held); + return cli.getPullRequest({ cwd: input.cwd, number: input.number }).pipe( + Effect.map((pullRequest) => { + const location = pullRequest.location; + if (location === null) return null; + if (locations.size >= LOCATION_CACHE_CAPACITY) { + const oldest = locations.keys().next().value; + if (oldest !== undefined) locations.delete(oldest); + } + locations.set(key, location); + return location; + }), + ); + }; + + /** + * Everything a diff read needs before it can ask for a file: where the repository lives, and + * which pushes the pull request has had. A client names neither, and the iterations are read + * afresh every time because the newest one is what a push adds. + */ + const diffScope = (input: { readonly cwd: string; readonly number: number }) => + Effect.gen(function* () { + const location = yield* locationOf(input); + if (location === null) return null; + const iterations = yield* cli.listIterations({ + cwd: input.cwd, + location, + number: input.number, + }); + return { location, iterations }; + }); + + const EMPTY_ITEM: AzureDevOpsItemContent = { contents: "", isBinary: false }; + + /** + * Both sides of one changed file. Only the sides a change actually has are asked for: Azure + * answers for a file that is not at a commit with a failure rather than with nothing. + */ + const readTexts = (input: { + readonly cwd: string; + readonly location: AzureDevOpsRepositoryLocation; + readonly iteration: AzureDevOpsIteration; + readonly change: Pick; + }) => + Effect.gen(function* () { + const oldItem = + input.change.changeKind === "new" + ? EMPTY_ITEM + : yield* cli.readItemContent({ + cwd: input.cwd, + location: input.location, + path: input.change.oldPath, + commit: input.iteration.mergeBaseCommit, + }); + const newItem = + input.change.changeKind === "deleted" + ? EMPTY_ITEM + : yield* cli.readItemContent({ + cwd: input.cwd, + location: input.location, + path: input.change.path, + commit: input.iteration.headCommit, + }); + const texts: AzureDevOpsFileTexts = { + oldContents: oldItem.contents, + newContents: newItem.contents, + // Azure hands a file it calls binary over in an encoding of its own, so its own word on + // that is taken rather than looked for in bytes it may never have sent verbatim. + binary: oldItem.isBinary || newItem.isBinary, + }; + return texts; + }); + + /** + * What the whole pull request changed, taken from its latest push. An iteration's changes are + * reported against the merge base rather than against the push before it, so the newest one is + * the whole of the change rather than the last slice of it. + */ + const listLatestChanges = (input: { + readonly cwd: string; + readonly location: AzureDevOpsRepositoryLocation; + readonly number: number; + readonly iterations: ReadonlyArray; + }) => { + const latest = input.iterations.at(-1); + return latest === undefined + ? Effect.succeed({ changes: [], truncated: false } as AzureDevOpsIterationChanges) + : cli.listIterationChanges({ + cwd: input.cwd, + location: input.location, + number: input.number, + iterationId: latest.id, + }); + }; + const provider: PullRequestProviderApi = { kind: "azure-devops", capabilities: CAPABILITIES, @@ -155,12 +292,30 @@ export const make = Effect.gen(function* () { ), getChangeRequest: (input) => - cli.getPullRequest({ cwd: input.cwd, number: input.number }).pipe( - Effect.mapError(fail("getChangeRequest")), - Effect.map((pullRequest): ProviderChangeRequestDetail => ({ + Effect.gen(function* () { + const pullRequest = yield* cli.getPullRequest({ cwd: input.cwd, number: input.number }); + const location = pullRequest.location; + // The file count is two reads past the pull request itself, and it is the only thing + // riding on them, so a failure leaves it unknown rather than losing the whole detail. + const changedFiles = + location === null + ? 0 + : yield* cli.listIterations({ cwd: input.cwd, location, number: input.number }).pipe( + Effect.flatMap((iterations) => + listLatestChanges({ + cwd: input.cwd, + location, + number: input.number, + iterations, + }), + ), + Effect.map((listed) => listed.changes.length), + Effect.orElseSucceed(() => 0), + ); + const detail: ProviderChangeRequestDetail = { ...toChangeRequest(pullRequest), body: pullRequest.body, - changedFiles: 0, + changedFiles, mergedAt: pullRequest.state === "merged" ? pullRequest.closedAt : null, closedAt: pullRequest.state === "closed" ? pullRequest.closedAt : null, reviewers: pullRequest.reviewers, @@ -171,19 +326,26 @@ export const make = Effect.gen(function* () { ...(pullRequest.autoMergeMethod === undefined ? {} : { autoMergeMethod: pullRequest.autoMergeMethod }), - })), - ), + }; + return detail; + }).pipe(Effect.mapError(fail("getChangeRequest"))), getChangeRequestActivity: (input) => cli.getPullRequest({ cwd: input.cwd, number: input.number }).pipe( Effect.mapError(fail("getChangeRequestActivity")), Effect.flatMap((pullRequest) => - (pullRequest.threadsUrl === null + (pullRequest.location === null ? Effect.succeed({ comments: [], truncated: true }) - : cli.listThreads({ cwd: input.cwd, threadsUrl: pullRequest.threadsUrl }).pipe( - Effect.map((comments) => ({ comments, truncated: false })), - Effect.orElseSucceed(() => ({ comments: [], truncated: true })), - ) + : cli + .listThreads({ + cwd: input.cwd, + location: pullRequest.location, + number: input.number, + }) + .pipe( + Effect.map((comments) => ({ comments, truncated: false })), + Effect.orElseSucceed(() => ({ comments: [], truncated: true })), + ) ).pipe( Effect.map((conversation): ProviderChangeRequestActivity => ({ comments: conversation.comments, @@ -200,16 +362,136 @@ export const make = Effect.gen(function* () { // reach, so the answer is the same constant the detail carries. getViewerPermissions: () => Effect.succeed(AZURE_DEVOPS_VIEWER_PERMISSIONS), - // Never called: `capabilities.diff` is false, and the service refuses a diff without it. - getDiff: () => - Effect.fail( - new PullRequestProviderError({ - provider: "azure-devops", - operation: "getDiff", - reason: "failed", - detail: "Azure DevOps cannot produce a patch for a pull request.", - }), - ), + // `input.commit` is deliberately dropped: Azure states no commit list on a pull request, so + // the Code tab has nothing to scope itself to and always asks for the whole change. + getDiff: (input) => + Effect.gen(function* () { + const scope = yield* diffScope(input); + if (scope === null) return EMPTY_DIFF_SLICE; + const cursor = parseAzureDevOpsDiffCursor(input.cursor); + // Reading on stays with the push the first slice was taken against. A push landing + // mid-read would otherwise renumber the files and hand the reader one twice, or none. + const iteration = + cursor === null + ? scope.iterations.at(-1) + : scope.iterations.find((candidate) => candidate.id === cursor.iterationId); + if (iteration === undefined) return EMPTY_DIFF_SLICE; + const listed = yield* cli.listIterationChanges({ + cwd: input.cwd, + location: scope.location, + number: input.number, + iterationId: iteration.id, + }); + const changes = listed.changes; + + const sections: string[] = []; + let truncated = listed.truncated; + let bytes = 0; + let index = cursor?.fileIndex ?? 0; + while (index < changes.length) { + const change = changes.at(index); + if (change === undefined) break; + // One file per pair of reads, and a pair Azure refuses is one file rather than the + // whole slice: an oversize blob or a path `az` will not carry through leaves that file + // listed without its hunks, and everything around it still renders. + const texts = yield* readTexts({ + cwd: input.cwd, + location: scope.location, + iteration, + change, + }).pipe( + // Only what is this one file's problem. A signed-out CLI, a rate limit or no `az` at + // all is the read failing rather than the file, and belongs to the caller, which + // pauses the host rather than showing every file in the change as unreadable. + Effect.catchTags({ + AzureDevOpsPullRequestNotFoundError: () => Effect.succeed(null), + AzureDevOpsCommandFailedError: () => Effect.succeed(null), + AzureDevOpsPullRequestReadError: () => Effect.succeed(null), + }), + ); + const file = + texts === null + ? azureDevOpsUnreadableFilePatch(change) + : azureDevOpsFilePatch({ change, texts, timeoutMillis: MAX_FILE_DIFF_MILLIS }); + sections.push(file.section); + bytes += byteLength(file.section); + truncated = truncated || file.truncated; + index += 1; + // A file whose diff was given up on spent the whole of what one file is allowed and has + // a header to show for it, so the byte budget would let a change full of them spend that + // over and over in the one request. The slice ends there instead, and reading on picks + // up at the file behind it. + if (bytes >= MAX_DIFF_SLICE_BYTES || file.abandoned) break; + } + + const slice: ProviderDiffSlice = { + patch: sections.join(""), + truncated, + nextCursor: + index >= changes.length + ? null + : formatAzureDevOpsDiffCursor({ iterationId: iteration.id, fileIndex: index }), + }; + return slice; + }).pipe(Effect.mapError(fail("getDiff"))), + + // The patch is built from whole files, so opening the lines around a hunk is the same two + // reads over again rather than a wider request. + // + // Read against the latest iteration, which is the one the patch was taken against unless a + // push landed in between. Nothing in the request says which push the reader is looking at, so + // there is no older iteration to go back to: expansion is stale after a mid-review push on + // every host here, and the diff it belongs to is stale with it. + getDiffFileContents: (input) => + Effect.gen(function* () { + const scope = yield* diffScope(input); + const iteration = scope?.iterations.at(-1); + if (scope === null || iteration === undefined) return { oldContents: "", newContents: "" }; + return yield* readTexts({ + cwd: input.cwd, + location: scope.location, + iteration, + change: { + changeKind: input.changeType, + path: input.newPath, + oldPath: input.oldPath, + }, + }); + }).pipe(Effect.mapError(fail("getDiffFileContents"))), + + /** + * What the head has of each marked file, which is the blob Azure already names on the change + * it reports. One read covers every path: the latest iteration lists the whole change, so + * asking per file would be the same answer fetched over and over. + * + * A path the change does not carry is at the empty revision, which is what a file the pull + * request deletes is at and leaves it cleared once and cleared for good. When the change was + * too long to follow to its end, those paths are left out instead: they were not looked at, + * and reporting them as deleted would clear a file nobody has read. + */ + getFileRevisions: (input) => + Effect.gen(function* () { + const revisions = new Map(); + if (input.paths.length === 0) return { revisions }; + const scope = yield* diffScope(input); + if (scope === null) return { revisions }; + const listed = yield* listLatestChanges({ + ...scope, + cwd: input.cwd, + number: input.number, + }); + const marked = new Set(input.paths); + for (const change of listed.changes) { + if (!marked.has(change.path) || change.objectId === null) continue; + revisions.set(change.path, change.objectId); + } + if (!listed.truncated) { + for (const path of input.paths) { + if (!revisions.has(path)) revisions.set(path, ""); + } + } + return { revisions }; + }).pipe(Effect.mapError(fail("getFileRevisions"))), runAction: (input) => cli diff --git a/apps/server/src/pullRequest/BitbucketPullRequestApi.test.ts b/apps/server/src/pullRequest/BitbucketPullRequestApi.test.ts index 8945ecc5e1e2..52ee64f4c6a9 100644 --- a/apps/server/src/pullRequest/BitbucketPullRequestApi.test.ts +++ b/apps/server/src/pullRequest/BitbucketPullRequestApi.test.ts @@ -362,6 +362,90 @@ layer("BitbucketPullRequestApi.layer", (it) => { }), ); + it.effect("reads file versions out of the patch, for the paths it was asked about", () => + Effect.gen(function* () { + mockedRequest.mockReturnValueOnce( + Effect.succeed( + response( + [ + "diff --git a/a.ts b/a.ts", + "index 1111111..2222222 100644", + "--- a/a.ts", + "+++ b/a.ts", + "@@ -1 +1 @@", + "-a", + "+b", + "diff --git a/b.ts b/b.ts", + "index 3333333..4444444 100644", + "--- a/b.ts", + "+++ b/b.ts", + "@@ -1 +1 @@", + "-c", + "+d", + "", + ].join("\n"), + ), + ), + ); + const api = yield* BitbucketPullRequestApi.BitbucketPullRequestApi; + + const revisions = yield* api.getFileRevisions({ + repository: "acme/web", + number: 71, + paths: ["a.ts", "missing.ts"], + }); + + // `b.ts` is in the patch and was not asked about, so it does not belong in the answer. + // `missing.ts` was asked about and the whole patch was read without finding it, which is + // what a file this pull request deletes looks like, so it is answered as the empty version. + assert.deepStrictEqual( + [...revisions], + [ + ["a.ts", "2222222"], + ["missing.ts", ""], + ], + ); + expect(callAt(0)).toMatchObject({ url: "/repositories/acme/web/pullrequests/71/diff" }); + }), + ); + + it.effect("says nothing about the files past the end of a patch it could not read whole", () => + Effect.gen(function* () { + // Bitbucket's patch is read up to a byte ceiling, and a file past the cut was not looked at. + // Answering for it as deleted would clear a mark on it once and for good. + mockedRequest.mockReturnValueOnce( + Effect.succeed({ + body: "diff --git a/a.ts b/a.ts\nindex 1111111..2222222 100644\n@@ -1 +1 @@\n", + truncated: true, + }), + ); + const api = yield* BitbucketPullRequestApi.BitbucketPullRequestApi; + + const revisions = yield* api.getFileRevisions({ + repository: "acme/web", + number: 72, + paths: ["a.ts", "past-the-cut.ts"], + }); + + assert.deepStrictEqual([...revisions], [["a.ts", "2222222"]]); + }), + ); + + it.effect("asks Bitbucket nothing when no file has been ticked off", () => + Effect.gen(function* () { + const api = yield* BitbucketPullRequestApi.BitbucketPullRequestApi; + + const revisions = yield* api.getFileRevisions({ + repository: "acme/web", + number: 73, + paths: [], + }); + + assert.strictEqual(revisions.size, 0); + assert.strictEqual(mockedRequest.mock.calls.length, 0); + }), + ); + it.effect("aggregates every diffstat page", () => Effect.gen(function* () { const next = "https://api.bitbucket.org/2.0/diffstat?page=2"; diff --git a/apps/server/src/pullRequest/BitbucketPullRequestApi.ts b/apps/server/src/pullRequest/BitbucketPullRequestApi.ts index 5b3149b0d75c..bd653bf58bb2 100644 --- a/apps/server/src/pullRequest/BitbucketPullRequestApi.ts +++ b/apps/server/src/pullRequest/BitbucketPullRequestApi.ts @@ -19,6 +19,7 @@ import type { } from "@t3tools/contracts"; import * as BitbucketApi from "../sourceControl/BitbucketApi.ts"; +import { parseDiffFileRevisions } from "./bitbucketDiffRevisions.ts"; import { buildReviewThreads, decodeCommentsJson, @@ -135,7 +136,6 @@ const CONVERSATION_PAGE_SIZE = 50; const CONVERSATION_PAGES = 10; /** The same ceiling the gh and glab diff reads use. */ const DIFF_MAX_BYTES = 8 * 1024 * 1024; - export interface BitbucketPullRequestBatch { readonly items: ReadonlyArray; readonly truncated: boolean; @@ -182,6 +182,22 @@ export class BitbucketPullRequestApi extends Context.Service< readonly number: number; }) => Effect.Effect; + /** + * What the pull request's head has of each of these paths, as opaque ids. + * + * Read off the pull request's own patch, the only place Bitbucket states a file's version. A + * path the patch does not carry is answered as the empty revision, and left out altogether + * when the patch was cut short at the byte ceiling and so cannot be spoken for. + * + * Held by the caller rather than here: the marks and the badge they feed share one window, + * and a second one underneath it would keep answering after a refresh had asked it not to. + */ + readonly getFileRevisions: (input: { + readonly repository: string; + readonly number: number; + readonly paths: ReadonlyArray; + }) => Effect.Effect, BitbucketPullRequestApiError>; + readonly getMergeability: (input: { readonly repository: string; readonly number: number; @@ -524,6 +540,34 @@ export const make = Effect.gen(function* () { }), ); + const pullRequestDiff = (input: { + readonly repository: string; + readonly number: number; + readonly commit?: string | undefined; + }): Effect.Effect< + { readonly patch: string; readonly truncated: boolean }, + BitbucketPullRequestApiError + > => + input.commit !== undefined && !isCommitSha(input.commit) + ? Effect.fail(new BitbucketDiffCommitError()) + : withRepository(input.repository, (path) => + // Already a unified patch, so it needs no decoding at all — only a bound, which a + // diff of any size would otherwise ignore. A commit's own patch sits beside the pull + // request's at `/diff/{sha}` and reads the same way. + bitbucket + .request({ + method: "GET", + url: + input.commit === undefined + ? `${path}/pullrequests/${input.number}/diff` + : `${path}/diff/${input.commit}`, + maxBytes: DIFF_MAX_BYTES, + }) + .pipe( + Effect.map((response) => ({ patch: response.body, truncated: response.truncated })), + ), + ); + return BitbucketPullRequestApi.of({ getViewer: () => bitbucket.request({ method: "GET", url: "/user" }).pipe( @@ -595,25 +639,28 @@ export const make = Effect.gen(function* () { }), ).pipe(Effect.catchIf(isRepositoryPermissionRemovedError, () => Effect.succeed(true))), - getPullRequestDiff: (input) => - input.commit !== undefined && !isCommitSha(input.commit) - ? Effect.fail(new BitbucketDiffCommitError()) - : withRepository(input.repository, (path) => - // Already a unified patch, so it needs no decoding at all — only a bound, which a - // diff of any size would otherwise ignore. A commit's own patch sits beside the pull - // request's at `/diff/{sha}` and reads the same way. - bitbucket - .request({ - method: "GET", - url: - input.commit === undefined - ? `${path}/pullrequests/${input.number}/diff` - : `${path}/diff/${input.commit}`, - maxBytes: DIFF_MAX_BYTES, - }) - .pipe( - Effect.map((response) => ({ patch: response.body, truncated: response.truncated })), - ), + getPullRequestDiff: pullRequestDiff, + + getFileRevisions: (input) => + input.paths.length === 0 + ? Effect.succeed(new Map()) + : pullRequestDiff({ repository: input.repository, number: input.number }).pipe( + Effect.map((diff) => { + const all = parseDiffFileRevisions(diff.patch); + // Narrowed to what was asked for rather than handed back whole: the caller compares + // the paths it named, and a patch of a thousand files has no business in its answer. + // + // A patch cut short at the byte ceiling says nothing about the files past the cut, + // so those paths are left out rather than reported as removed: the caller reads an + // absent path as one it could not learn about, and a mark on it is left alone. + const asked = new Map(); + for (const path of input.paths) { + const revision = all.get(path); + if (revision !== undefined) asked.set(path, revision); + else if (!diff.truncated) asked.set(path, ""); + } + return asked; + }), ), getDiffStat: (input) => diff --git a/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts b/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts index 47d41eeee6d9..2b51a29b3a12 100644 --- a/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts +++ b/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts @@ -31,6 +31,11 @@ const CAPABILITIES: PullRequestCapabilities = { }, reviewers: { request: true, listCandidates: true }, edit: { changeRequest: true, comment: true }, + // Bitbucket Cloud states nothing about what a reviewer has already read: no endpoint carries a + // viewed file, and the per-pull-request properties it does offer are one value shared by + // everyone rather than one per reader. So the marks are kept here, and the client says whose + // they are rather than implying bitbucket.org will show them. + viewedFiles: "environment", }; /** @@ -234,6 +239,18 @@ export const make = Effect.gen(function* () { Effect.map((diff) => ({ ...diff, nextCursor: null })), ), + getFileRevisions: (input) => + api + .getFileRevisions({ + repository: input.repository, + number: input.number, + paths: input.paths, + }) + .pipe( + Effect.mapError(fail("getFileRevisions")), + Effect.map((revisions) => ({ revisions })), + ), + // Users only: Bitbucket requests a review of an account, and has no group that stands in for // one on a pull request. listReviewerCandidates: (input) => diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts index 1e6ca0ed43a6..500bdeadaaa5 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts @@ -3091,4 +3091,148 @@ layer("GitHubPullRequestCli.layer", (it) => { expect(callAt(1).args).toContain("repos/acme/web/issues/7/labels/area%2Fweb"); }), ); + + it.effect("reads every page of viewed files, and says so when there are too many", () => + Effect.gen(function* () { + const page = (index: number, hasNextPage: boolean) => + Effect.succeed( + output( + JSON.stringify({ + data: { + repository: { + pullRequest: { + files: { + pageInfo: { hasNextPage, endCursor: `cursor-${index}` }, + nodes: [ + { path: `src/file${index}.ts`, viewerViewedState: "VIEWED" }, + { path: `src/other${index}.ts`, viewerViewedState: "UNVIEWED" }, + ], + }, + }, + }, + }, + }), + ), + ); + mockedExecute + .mockReturnValueOnce(page(0, true)) + .mockReturnValueOnce(page(1, true)) + .mockReturnValueOnce(page(2, false)); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + const viewed = yield* cli.getPullRequestFilesViewed({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + }); + + assert.strictEqual(mockedExecute.mock.calls.length, 3); + // The first page asks from the start; each one after it carries the cursor before it. + assert.isFalse(callAt(0).args.some((arg) => arg.startsWith("after="))); + expect(callAt(1).args).toContain("after=cursor-0"); + expect(callAt(2).args).toContain("after=cursor-1"); + assert.isFalse(viewed.truncated); + expect(viewed.files.map((file) => [file.path, file.state])).toEqual([ + ["src/file0.ts", "viewed"], + ["src/other0.ts", "unviewed"], + ["src/file1.ts", "viewed"], + ["src/other1.ts", "unviewed"], + ["src/file2.ts", "viewed"], + ["src/other2.ts", "unviewed"], + ]); + }), + ); + + it.effect("stops paging viewed files rather than following a change without end", () => + Effect.gen(function* () { + mockedExecute.mockReturnValue( + Effect.succeed( + output( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ + data: { + repository: { + pullRequest: { + files: { + pageInfo: { hasNextPage: true, endCursor: "cursor" }, + nodes: [{ path: "src/file.ts", viewerViewedState: "VIEWED" }], + }, + }, + }, + }, + }), + ), + ), + ); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + const viewed = yield* cli.getPullRequestFilesViewed({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + }); + + assert.strictEqual(mockedExecute.mock.calls.length, 5); + assert.isTrue(viewed.truncated); + assert.strictEqual(viewed.files.length, 5); + }), + ); + + it.effect("clears and restores a burst of files in one request", () => + Effect.gen(function* () { + mockedExecute + .mockReturnValueOnce( + Effect.succeed( + // @effect-diagnostics-next-line preferSchemaOverJson:off + output(JSON.stringify({ data: { repository: { pullRequest: { id: "PR_1" } } } })), + ), + ) + .mockReturnValueOnce(Effect.succeed(output("{}"))); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + yield* cli.setPullRequestFilesViewed({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + files: [ + { path: "src/a.ts", viewed: true }, + { path: "src/b.ts", viewed: false }, + ], + }); + + // One request to learn the pull request's node id, one for every press together. + assert.strictEqual(mockedExecute.mock.calls.length, 2); + // @effect-diagnostics-next-line preferSchemaOverJson:off + const sent = JSON.parse(callAt(1).stdin ?? "") as { + query: string; + variables: Record; + }; + expect(sent.query).toContain("f0: markFileAsViewed"); + expect(sent.query).toContain("f1: unmarkFileAsViewed"); + expect(sent.variables).toEqual({ + pullRequestId: "PR_1", + path0: "src/a.ts", + path1: "src/b.ts", + }); + }), + ); + + it.effect("asks the host nothing when nothing was pressed", () => + Effect.gen(function* () { + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + yield* cli.setPullRequestFilesViewed({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + files: [], + }); + + assert.strictEqual(mockedExecute.mock.calls.length, 0); + }), + ); }); diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.ts index 3dc41896037b..6ee620ee291e 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.ts @@ -7,6 +7,7 @@ import { resolvePullRequestAuthorFilter, type PullRequestAction, type PullRequestActor, + type PullRequestFileViewed, type PullRequestInvolvement, type PullRequestListFilters, type PullRequestListState, @@ -31,10 +32,12 @@ import { ADD_REACTION_GRAPHQL_MUTATION, buildReviewSubmissionJson, buildReviewerRequestJson, + buildSetFilesViewedGraphQlMutation, decodeActorAvatarsJson, decodePullRequestActivityJson, decodePullRequestDetailJson, decodePullRequestFilesJson, + decodePullRequestFilesViewedJson, decodePullRequestHeadsJson, decodePullRequestListJson, decodePullRequestNodeIdJson, @@ -58,6 +61,7 @@ import { decodeBaseComparisonJson, PULL_REQUEST_DETAIL_JSON_FIELDS, PULL_REQUEST_LIST_JSON_FIELDS, + PULL_REQUEST_FILES_VIEWED_GRAPHQL_QUERY, PULL_REQUEST_NODE_ID_GRAPHQL_QUERY, REACTION_SUBJECT_PULL_REQUEST_GRAPHQL_QUERY, REMOVE_REACTION_GRAPHQL_MUTATION, @@ -358,6 +362,12 @@ 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; +/** + * How many hundred-file pages of viewed state one read will walk. A point of the hourly GraphQL + * budget per page, against a change request nobody reviews in one sitting past the first few + * hundred files: beyond this the read stops and says it was cut short. + */ +const FILES_VIEWED_MAX_PAGES = 5; /** * Pages of review threads to follow before the conversation is reported as truncated. GitHub @@ -403,6 +413,12 @@ export interface GitHubPullRequestDiffSlice { readonly omittedFileStats?: ReadonlyArray; } +export interface GitHubPullRequestFilesViewed { + readonly files: ReadonlyArray; + /** GitHub had more files than the page budget below would read. */ + readonly truncated: boolean; +} + export class GitHubPullRequestCli extends Context.Service< GitHubPullRequestCli, { @@ -539,6 +555,30 @@ export class GitHubPullRequestCli extends Context.Service< GitHubPullRequestCliError >; + /** + * Which files of the pull request the signed-in account has cleared, and which of those have + * been pushed to since. Read apart from the patch because GitHub only reports it over GraphQL, + * and because the two answers go stale at completely different rates. + */ + readonly getPullRequestFilesViewed: (input: { + readonly cwd: string; + readonly repository: string; + readonly host: string; + readonly number: number; + }) => Effect.Effect; + + /** + * Clears files, or puts them back, as one request. GitHub takes a single path per mutation, + * so a burst is batched with aliases into one document rather than one subprocess per press. + */ + readonly setPullRequestFilesViewed: (input: { + readonly cwd: string; + readonly repository: string; + readonly host: string; + readonly number: number; + readonly files: ReadonlyArray<{ readonly path: string; readonly viewed: boolean }>; + }) => Effect.Effect; + readonly listReviewThreadComments: (input: { readonly cwd: string; readonly repository: string; @@ -2216,6 +2256,58 @@ export const make = Effect.gen(function* () { variables: { threadId: input.threadId, body: input.body }, }), + getPullRequestFilesViewed: (input) => { + const { owner, name } = parseRepositorySelector(input.repository); + const read = ( + after: string | null, + collected: ReadonlyArray, + pagesLeft: number, + ): Effect.Effect => + graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "getPullRequestFilesViewed", + variables: [ + ["-f", `owner=${owner}`], + ["-f", `name=${name}`], + ["-F", `number=${input.number}`], + ...(after === null + ? [] + : ([["-f", `after=${after}`]] as ReadonlyArray)), + ], + query: PULL_REQUEST_FILES_VIEWED_GRAPHQL_QUERY, + decode: decodePullRequestFilesViewedJson, + }).pipe( + Effect.flatMap((page) => { + const files = [...collected, ...page.files]; + if (page.nextCursor === null) { + return Effect.succeed({ files, truncated: false }); + } + // A change nobody could read in one sitting is not worth a point of budget a page: + // the boxes on screen still work, and the count says it is partial rather than lying. + return pagesLeft <= 1 + ? Effect.succeed({ files, truncated: true }) + : read(page.nextCursor, files, pagesLeft - 1); + }), + ); + return read(null, [], FILES_VIEWED_MAX_PAGES); + }, + + setPullRequestFilesViewed: (input) => { + const mutation = buildSetFilesViewedGraphQlMutation(input.files); + if (mutation === null) return Effect.void; + return pullRequestNodeId({ ...input, operation: "setPullRequestFilesViewed" }).pipe( + Effect.flatMap((pullRequestId) => + graphql({ + cwd: input.cwd, + host: input.host, + query: mutation.query, + variables: { pullRequestId, ...mutation.variables }, + }), + ), + ); + }, + setReviewThreadResolution: (input) => graphql({ cwd: input.cwd, diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts index e75ef3547c04..78ea5ef3732a 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts @@ -39,6 +39,7 @@ const CAPABILITIES: PullRequestCapabilities = { updateMethods: ["merge", "rebase"], search: true, reactions: true, + viewedFiles: "host", review: { inlineComment: true, reply: true, @@ -506,6 +507,12 @@ export const make = Effect.gen(function* () { getDiffFileContents: (input) => cli.getPullRequestDiffFileContents(input).pipe(Effect.mapError(fail("getDiffFileContents"))), + getFilesViewed: (input) => + cli.getPullRequestFilesViewed(input).pipe(Effect.mapError(fail("getFilesViewed"))), + + setFilesViewed: (input) => + cli.setPullRequestFilesViewed(input).pipe(Effect.mapError(fail("setFilesViewed"))), + 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 014d91a02740..daa85ee9eb8f 100644 --- a/apps/server/src/pullRequest/GitLabPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/GitLabPullRequestCli.test.ts @@ -1402,4 +1402,171 @@ layer("GitLabPullRequestCli.layer", (it) => { expect(callAt(0).stdin).toBe('{"body":"true"}'); }), ); + it.effect("reads blob ids for the marked paths at the merge request's head", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ + iid: 7, + title: "t", + web_url: "https://gitlab.com/acme/web/-/merge_requests/7", + source_branch: "feat", + target_branch: "main", + created_at: "2026-07-01T00:00:00Z", + updated_at: "2026-07-01T00:00:00Z", + diff_refs: { base_sha: "base", head_sha: "head", start_sha: "start" }, + }), + ), + ), + ); + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ + data: { + project: { repository: { blobs: { nodes: [{ path: "src/a.ts", oid: "aaa" }] } } }, + }, + }), + ), + ), + ); + const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + + const revisions = yield* cli.getFileRevisions({ + cwd: "/w", + repository: "acme/web", + number: 7, + paths: ["src/a.ts", "src/gone.ts"], + }); + + // Every path was looked for at the head, so one the head does not have is one the merge + // request removed: said as the empty version, which a mark on it was stamped with too. + expect([...revisions]).toEqual([ + ["src/a.ts", "aaa"], + ["src/gone.ts", ""], + ]); + // The head the reader is looking at, not whatever the source branch has moved on to. + // @effect-diagnostics-next-line preferSchemaOverJson:off + const body: unknown = JSON.parse(callAt(1).stdin ?? "{}"); + expect(body).toMatchObject({ + variables: { fullPath: "acme/web", ref: "head", paths: ["src/a.ts", "src/gone.ts"] }, + }); + }), + ); + + it.effect("leaves the paths out when GitLab did not answer the blobs query", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ + iid: 7, + title: "t", + web_url: "https://gitlab.com/acme/web/-/merge_requests/7", + source_branch: "feat", + target_branch: "main", + created_at: "2026-07-01T00:00:00Z", + updated_at: "2026-07-01T00:00:00Z", + diff_refs: { base_sha: "base", head_sha: "head", start_sha: "start" }, + }), + ), + ), + ); + // What GitLab says of a project the token cannot see. It is not the head having none of + // these files, and reading it that way would report every file the reader has cleared as + // changed on nothing worse than a permission. + mockedExecute.mockReturnValueOnce( + Effect.succeed( + // @effect-diagnostics-next-line preferSchemaOverJson:off + output(JSON.stringify({ data: { project: null } })), + ), + ); + const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + + const revisions = yield* cli.getFileRevisions({ + cwd: "/w", + repository: "acme/web", + number: 7, + paths: ["src/a.ts", "src/b.ts"], + }); + + expect([...revisions]).toEqual([]); + }), + ); + + it.effect("asks GitLab nothing when no file is marked", () => + Effect.gen(function* () { + const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + + const revisions = yield* cli.getFileRevisions({ + cwd: "/w", + repository: "acme/web", + number: 7, + paths: [], + }); + + expect([...revisions]).toEqual([]); + expect(mockedExecute).not.toHaveBeenCalled(); + }), + ); + + it.effect("splits the paths across requests, because GitLab charges the query by how many", () => + Effect.gen(function* () { + const paths = Array.from({ length: 150 }, (_, index) => `src/${index}.ts`); + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ + iid: 7, + title: "t", + web_url: "https://gitlab.com/acme/web/-/merge_requests/7", + source_branch: "feat", + target_branch: "main", + created_at: "2026-07-01T00:00:00Z", + updated_at: "2026-07-01T00:00:00Z", + diff_refs: { base_sha: "base", head_sha: "head", start_sha: "start" }, + }), + ), + ), + ); + mockedExecute.mockImplementation((request) => { + const body = JSON.parse(request.stdin ?? "{}") as { + readonly variables: { readonly paths: ReadonlyArray }; + }; + return Effect.succeed( + output( + JSON.stringify({ + data: { + project: { + repository: { + blobs: { + nodes: body.variables.paths.map((path) => ({ path, oid: `oid-${path}` })), + }, + }, + }, + }, + }), + ), + ); + }); + const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + + const revisions = yield* cli.getFileRevisions({ + cwd: "/w", + repository: "acme/web", + number: 7, + paths, + }); + + assert.strictEqual(revisions.size, 150); + assert.strictEqual(revisions.get("src/149.ts"), "oid-src/149.ts"); + // The diff refs, then two batches: a hundred paths and the fifty left over. + assert.strictEqual(mockedExecute.mock.calls.length, 3); + }), + ); }); diff --git a/apps/server/src/pullRequest/GitLabPullRequestCli.ts b/apps/server/src/pullRequest/GitLabPullRequestCli.ts index f291cbd89c7f..a7f6ff64e9f1 100644 --- a/apps/server/src/pullRequest/GitLabPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitLabPullRequestCli.ts @@ -35,8 +35,10 @@ import { decodeOwnAwardIdJson, decodeProjectMergeCapabilitiesJson, decodeProjectUsersJson, + decodeRepositoryBlobsJson, decodeViewerJson, gitLabAwardName, + REPOSITORY_BLOBS_GRAPHQL_QUERY, type GitLabDiffRefs, type GitLabMergeRequestDetail, type GitLabMergeRequestListItem, @@ -283,6 +285,20 @@ export class GitLabPullRequestCli extends Context.Service< readonly repository: string; }) => Effect.Effect; + /** + * What the merge request's head has of each of these paths, as blob ids. + * + * The head sha comes from the merge request's own diff refs, so the answer is the version a + * reader is looking at rather than whatever the source branch has moved on to. A path the + * head does not have is left out. + */ + readonly getFileRevisions: (input: { + readonly cwd: string; + readonly repository: string; + readonly number: number; + readonly paths: ReadonlyArray; + }) => Effect.Effect, GitLabPullRequestCliError>; + /** * Who this merge request may be sent to, and who it has already been sent to. Two reads at * once, because GitLab keeps the people with access on the project and the reviewers on the @@ -1030,6 +1046,88 @@ export const make = Effect.gen(function* () { }), ); + /** + * GitLab charges the blobs query by how many paths it is handed, and its connection hands back + * one page. A hundred at a time keeps each request inside both. + */ + const BLOB_PATHS_PER_REQUEST = 100; + + const blobsAt = (input: { + readonly cwd: string; + readonly repository: string; + readonly ref: string; + readonly paths: ReadonlyArray; + }): Effect.Effect | null, GitLabPullRequestCliError> => + api({ + cwd: input.cwd, + path: "graphql", + method: "POST", + stdin: JSON.stringify({ + query: REPOSITORY_BLOBS_GRAPHQL_QUERY, + variables: { fullPath: input.repository, ref: input.ref, paths: input.paths }, + }), + }).pipe( + Effect.flatMap( + (result): Effect.Effect | null, GitLabPullRequestCliError> => { + const decoded = decodeRepositoryBlobsJson(result.stdout.trim()); + return Result.isSuccess(decoded) + ? Effect.succeed(decoded.success) + : Effect.fail( + new GitLabMergeRequestReadError({ + command: "glab", + cwd: input.cwd, + operation: "getFileRevisions", + cause: decoded.failure, + }), + ); + }, + ), + ); + + const fileRevisions = (input: { + readonly cwd: string; + readonly repository: string; + readonly number: number; + readonly paths: ReadonlyArray; + }): Effect.Effect, GitLabPullRequestCliError> => + input.paths.length === 0 + ? Effect.succeed(new Map()) + : getDiffRefs(input).pipe( + Effect.flatMap((refs) => { + const batches: Array> = []; + for (let at = 0; at < input.paths.length; at += BLOB_PATHS_PER_REQUEST) { + batches.push(input.paths.slice(at, at + BLOB_PATHS_PER_REQUEST)); + } + return Effect.forEach( + batches, + (paths) => + blobsAt({ ...input, ref: refs.headSha, paths }).pipe( + Effect.map((page) => ({ paths, page })), + ), + { concurrency: 2 }, + ).pipe( + Effect.map((pages) => { + const revisions = new Map(); + for (const { paths, page } of pages) { + // A batch GitLab did not answer says nothing about its paths, so they are left + // out and the caller reads them as versions it could not learn, which leaves + // the marks on them alone. Filling them in as removed would report every file + // a reader has cleared as changed over a project the token cannot see. + if (page === null) continue; + for (const [path, oid] of page) revisions.set(path, oid); + // Within a batch that was answered, every path was looked for at the head, so + // one that is not there is one the merge request removed. Said as the empty + // revision, which is an answer the caller can compare against and keep. + for (const path of paths) { + if (!revisions.has(path)) revisions.set(path, ""); + } + } + return revisions as ReadonlyMap; + }), + ); + }), + ); + const viewerUsername = (input: { readonly cwd: string }) => api({ cwd: input.cwd, path: "user" }).pipe( Effect.flatMap((result): Effect.Effect => { @@ -1065,6 +1163,8 @@ export const make = Effect.gen(function* () { listReactions: (input) => awardsPage({ ...input, cursor: null, page: 1, collected: null }), + getFileRevisions: fileRevisions, + setReaction: (input) => Effect.gen(function* () { const subject = awardSubjectPath(input); diff --git a/apps/server/src/pullRequest/GitLabPullRequestProvider.ts b/apps/server/src/pullRequest/GitLabPullRequestProvider.ts index 46fce2884279..c1482d601bda 100644 --- a/apps/server/src/pullRequest/GitLabPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitLabPullRequestProvider.ts @@ -35,6 +35,11 @@ const CAPABILITIES: PullRequestCapabilities = { updateMethods: ["rebase"], search: true, reactions: true, + // GitLab keeps a reader's viewed files in one browser's local storage, where nothing outside + // that browser can read or write them. So the marks made here are this environment's own: they + // follow the reader between the clients connected to it, but they are not the ones gitlab.com + // shows, and the surface says so rather than implying a review can be carried on from there. + viewedFiles: "environment", review: { inlineComment: true, reply: true, @@ -218,6 +223,15 @@ export const make = Effect.gen(function* () { getDiff: (input) => cli.getMergeRequestDiff(input).pipe(Effect.mapError(fail("getDiff"))), + // What each marked file is at the head, which is what tells a mark that still stands from one + // the branch has moved past. GitLab's own local-storage marks are keyed on the blob id too, + // so this stales at the same moment its web UI would. + getFileRevisions: (input) => + cli.getFileRevisions(input).pipe( + Effect.mapError(fail("getFileRevisions")), + Effect.map((revisions) => ({ revisions })), + ), + // Users only: GitLab requests a review of a person, and the groups that can stand in for one // appear in approval rules rather than in a merge request's reviewers. listReviewerCandidates: (input) => diff --git a/apps/server/src/pullRequest/PullRequestProvider.ts b/apps/server/src/pullRequest/PullRequestProvider.ts index 1459d7cec921..1af8f94b1488 100644 --- a/apps/server/src/pullRequest/PullRequestProvider.ts +++ b/apps/server/src/pullRequest/PullRequestProvider.ts @@ -8,6 +8,7 @@ import type { PullRequestChecksState, PullRequestCheck, PullRequestComment, + PullRequestFileViewed, PullRequestCommit, PullRequestInvolvement, PullRequestLabel, @@ -218,6 +219,27 @@ export interface ProviderDiffFileContents { readonly newContents: string; } +export interface ProviderFilesViewed { + readonly files: ReadonlyArray; + /** The host has more files than were read, so the ones missing here are not "unviewed". */ + readonly truncated: boolean; +} + +/** + * What version each of the asked-for files is at, on the change request's head. + * + * Opaque strings: the caller only ever compares one against another, and every host names a + * version its own way. The empty string is an answer rather than a gap — it is what a file the + * change request deletes is at, and a mark taken against it stays cleared. + * + * A path is absent only when the read could not say: a host that answered for part of the change + * must leave the rest out rather than report it as deleted, or a file past the cut would be + * cleared once and cleared for good. + */ +export interface ProviderFileRevisions { + readonly revisions: ReadonlyMap; +} + export interface ProviderRepositoryRef { readonly cwd: string; /** Provider-native repository identity, e.g. `owner/repo` or `group/subgroup/project`. */ @@ -380,6 +402,46 @@ export interface PullRequestProviderApi { }, ) => Effect.Effect; + /** + * Which files the reader has already cleared. Only called when the host keeps that record + * itself — `capabilities.viewedFiles` of `"host"` — and read apart from the patch: a host that + * reports this at all reports it on a clock of its own, moving with every press rather than + * with every push. + */ + readonly getFilesViewed?: ( + input: ProviderRepositoryRef & { readonly number: number }, + ) => Effect.Effect; + + /** + * Clears files, or puts them back. Only called when `capabilities.viewedFiles` is `"host"`. + * + * Takes several at once because that is how they are pressed. A provider whose host has no + * bulk form still owes one round trip for the batch rather than one per file, since the point + * of gathering them here is that the host is asked once. + */ + readonly setFilesViewed?: ( + input: ProviderRepositoryRef & { + readonly number: number; + readonly files: ReadonlyArray<{ readonly path: string; readonly viewed: boolean }>; + }, + ) => Effect.Effect; + + /** + * What version the head has of each of these files. Required of a host whose + * `capabilities.viewedFiles` is `"environment"`, and unused by one that keeps the marks itself. + * + * The marks live here, but what counts as the same file does not: only the host can say whether + * what a reader cleared last week is still what is in front of them. Asked for the marked paths + * alone, so the cost follows how much of the change request has been read rather than how large + * it is. + */ + readonly getFileRevisions?: ( + input: ProviderRepositoryRef & { + readonly number: number; + readonly paths: ReadonlyArray; + }, + ) => 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 0dc7a928c264..38325e34bef6 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -15,6 +15,8 @@ import type { } from "@t3tools/contracts"; import * as ProjectionSnapshotQuery from "../orchestration/Services/ProjectionSnapshotQuery.ts"; +import { SqlitePersistenceMemory } from "../persistence/Layers/Sqlite.ts"; +import * as PullRequestFilesViewed from "../persistence/PullRequestFilesViewed.ts"; import * as SourceControlProviderRegistry from "../sourceControl/SourceControlProviderRegistry.ts"; import * as SourceControlRateLimit from "../sourceControl/SourceControlRateLimit.ts"; import { @@ -179,8 +181,10 @@ function makeService(input: { readonly providers: ReadonlyArray; readonly resolveHandle?: SourceControlProviderRegistry.SourceControlProviderRegistry["Service"]["resolveHandle"]; }) { - return PullRequestService.make.pipe( - Effect.provide( + // Built into the test's own scope rather than provided call by call: the marks store owns a + // database, and `Effect.provide` would close it the moment the service was handed back. + return Effect.flatMap( + Layer.build( Layer.mergeAll( Layer.succeed(PullRequestProviderRegistry, fromProviders(input.providers)), Layer.mock(SourceControlProviderRegistry.SourceControlProviderRegistry)({ @@ -197,8 +201,12 @@ function makeService(input: { }), }), SourceControlRateLimit.layer, + // The real store over a database of its own, so the environment-kept marks are exercised + // through the SQL that holds them rather than through a stand-in that agrees with itself. + PullRequestFilesViewed.layer.pipe(Layer.provide(SqlitePersistenceMemory)), ), ), + (context) => Effect.provideContext(PullRequestService.make, context), ); } @@ -3987,3 +3995,665 @@ it.effect("names the signed-in account in the detail, and says nothing where the assert.strictEqual(unnamed.viewer, undefined); }), ); + +it.effect("keeps the diff cached across a file being ticked off", () => + Effect.gen(function* () { + let diffReads = 0; + let viewedReads = 0; + const service = yield* makeService({ + projects: [ + project({ id: "p1", title: "t3code", workspaceRoot: "/a", repository: "pingdotgg/t3code" }), + ], + providers: [ + fakeProvider("github", { + capabilities: { + diff: true, + comment: true, + actions: ["merge"], + mergeMethods: ["merge"], + search: true, + reactions: true, + viewedFiles: "host", + review: FULL_REVIEW, + reviewers: FULL_REVIEWERS, + }, + getDiff: () => { + diffReads += 1; + return Effect.succeed({ patch: "@@", truncated: false, nextCursor: null }); + }, + getFilesViewed: () => { + viewedReads += 1; + return Effect.succeed({ + files: [{ path: "src/a.ts", state: "viewed" as const }], + truncated: false, + }); + }, + setFilesViewed: () => Effect.void, + }), + ], + }); + const reference = { projectId: "p1" as ProjectId, repository: "pingdotgg/t3code", number: 1 }; + + yield* service.diff(reference); + yield* service.filesViewed(reference); + yield* service.setFilesViewed({ ...reference, files: [{ path: "src/a.ts", viewed: false }] }); + yield* service.diff(reference); + yield* service.filesViewed(reference); + + // The press forgets only the reader's own ticks: a diff of any size survives it. + assert.strictEqual(diffReads, 1); + assert.strictEqual(viewedReads, 2); + }), +); + +const environmentViewedProvider = ( + revisions: Map, + asked: Array>, + unreadable: ReadonlySet = new Set(), +) => + fakeProvider("gitlab", { + capabilities: { + diff: true, + comment: true, + actions: ["merge"], + mergeMethods: ["merge"], + search: true, + reactions: true, + viewedFiles: "environment", + review: FULL_REVIEW, + reviewers: FULL_REVIEWERS, + }, + getFilesViewed: () => Effect.die("the host keeps no marks of its own"), + setFilesViewed: () => Effect.die("the host keeps no marks of its own"), + getFileRevisions: (input) => { + asked.push(input.paths); + return Effect.succeed({ + // A path the host looked at and did not find is at the empty version, which is what a + // file the change request deletes is at. One it could not look at is left out entirely. + revisions: new Map( + input.paths.flatMap((path) => + unreadable.has(path) ? [] : [[path, revisions.get(path) ?? ""] as const], + ), + ), + }); + }, + }); + +const environmentViewedService = ( + revisions: Map, + asked: Array>, + unreadable: ReadonlySet = new Set(), +) => + makeService({ + projects: [ + project({ + id: "p1", + title: "on gitlab", + workspaceRoot: "/a", + repository: "group/project", + provider: "gitlab", + }), + ], + providers: [environmentViewedProvider(revisions, asked, unreadable)], + }); + +const GITLAB_REFERENCE = { + projectId: "p1" as ProjectId, + repository: "group/project", + number: 1, +}; + +it.effect("keeps viewed files itself for a host that keeps none of its own", () => + Effect.gen(function* () { + const asked: Array> = []; + const service = yield* environmentViewedService( + new Map([ + ["src/a.ts", "blob-a"], + ["src/b.ts", "blob-b"], + ]), + asked, + ); + + // Nothing marked is nothing to ask the host about. + const empty = yield* service.filesViewed(GITLAB_REFERENCE); + assert.deepStrictEqual(empty, { files: [], truncated: false }); + assert.deepStrictEqual(asked, []); + + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [ + { path: "src/a.ts", viewed: true }, + { path: "src/b.ts", viewed: true }, + ], + }); + const marked = yield* service.filesViewed(GITLAB_REFERENCE); + + assert.deepStrictEqual( + [...marked.files].toSorted((left, right) => left.path.localeCompare(right.path)), + [ + { path: "src/a.ts", state: "viewed" }, + { path: "src/b.ts", state: "viewed" }, + ], + ); + assert.strictEqual(marked.truncated, false); + // The marked paths alone, so the cost follows how much has been read rather than PR size, + // and the read after the press is answered from what the press already heard. + assert.deepStrictEqual( + asked.map((paths) => [...paths].toSorted()), + [["src/a.ts", "src/b.ts"]], + ); + }), +); + +it.effect("reads the marks without asking the host what the head has every time", () => + Effect.gen(function* () { + const asked: Array> = []; + const service = yield* environmentViewedService(new Map([["src/a.ts", "blob-a"]]), asked); + + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/a.ts", viewed: true }], + }); + // Past the marks' own cache, so this read reaches the point where the host would be asked. + yield* TestClock.adjust("20 seconds"); + const marked = yield* service.filesViewed(GITLAB_REFERENCE); + + assert.deepStrictEqual(marked.files, [{ path: "src/a.ts", state: "viewed" }]); + assert.deepStrictEqual(asked, [["src/a.ts"]]); + }), +); + +it.effect("answers the marks from what it last heard while it asks the host again", () => + Effect.gen(function* () { + const asked: Array> = []; + const revisions = new Map([["src/a.ts", "blob-a"]]); + const service = yield* environmentViewedService(revisions, asked); + + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/a.ts", viewed: true }], + }); + revisions.set("src/a.ts", "blob-a-again"); + yield* TestClock.adjust("90 seconds"); + const held = yield* service.filesViewed(GITLAB_REFERENCE); + + // The push is not in this answer, because waiting for the host is the thing being avoided. + assert.deepStrictEqual(held.files, [{ path: "src/a.ts", state: "viewed" }]); + assert.strictEqual(asked.length, 2); + + yield* TestClock.adjust("20 seconds"); + const caught = yield* service.filesViewed(GITLAB_REFERENCE); + + assert.deepStrictEqual(caught.files, [{ path: "src/a.ts", state: "dismissed" }]); + // The refresh behind the previous answer is the one that heard about the push. + assert.strictEqual(asked.length, 2); + }), +); + +it.effect("asks the host about a file it has not been asked about before", () => + Effect.gen(function* () { + const asked: Array> = []; + const service = yield* environmentViewedService( + new Map([ + ["src/a.ts", "blob-a"], + ["src/b.ts", "blob-b"], + ]), + asked, + ); + + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/a.ts", viewed: true }], + }); + yield* TestClock.adjust("20 seconds"); + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/b.ts", viewed: true }], + }); + yield* TestClock.adjust("20 seconds"); + const marked = yield* service.filesViewed(GITLAB_REFERENCE); + + assert.deepStrictEqual( + [...marked.files].toSorted((left, right) => left.path.localeCompare(right.path)), + [ + { path: "src/a.ts", state: "viewed" }, + { path: "src/b.ts", state: "viewed" }, + ], + ); + // The second press paid for its own file; the read that follows was already covered. + assert.deepStrictEqual(asked, [["src/a.ts"], ["src/b.ts"]]); + }), +); + +it.effect("does not let a press about one file keep another file's version alive", () => + Effect.gen(function* () { + const asked: Array> = []; + const revisions = new Map([ + ["src/a.ts", "blob-a"], + ["src/b.ts", "blob-b"], + ]); + const service = yield* environmentViewedService(revisions, asked); + + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/a.ts", viewed: true }], + }); + yield* TestClock.adjust("40 seconds"); + // This press asks about its own file and carries the other one forward untouched. Counting + // the whole scope as heard from would put the first file's version back inside the window it + // had almost aged out of, and a reader working down a long diff renews it press after press. + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/b.ts", viewed: true }], + }); + assert.deepStrictEqual(asked, [["src/a.ts"], ["src/b.ts"]]); + + revisions.set("src/a.ts", "blob-a-again"); + yield* TestClock.adjust("30 seconds"); + yield* service.filesViewed(GITLAB_REFERENCE); + assert.strictEqual(asked.length, 3); + + yield* TestClock.adjust("20 seconds"); + const caught = yield* service.filesViewed(GITLAB_REFERENCE); + + assert.deepStrictEqual( + [...caught.files].toSorted((left, right) => left.path.localeCompare(right.path)), + [ + { path: "src/a.ts", state: "dismissed" }, + { path: "src/b.ts", state: "viewed" }, + ], + ); + }), +); + +it.effect("reports a file pushed to since it was cleared as changed", () => + Effect.gen(function* () { + const revisions = new Map([ + ["src/a.ts", "blob-a"], + ["src/b.ts", "blob-b"], + ]); + const service = yield* environmentViewedService(revisions, []); + + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [ + { path: "src/a.ts", viewed: true }, + { path: "src/b.ts", viewed: true }, + ], + }); + revisions.set("src/a.ts", "blob-a-again"); + // A push is not something the marks can hear about, so the reader asks to be re-answered. + yield* service.invalidate({ reference: GITLAB_REFERENCE }); + const marked = yield* service.filesViewed(GITLAB_REFERENCE); + + assert.deepStrictEqual( + [...marked.files].toSorted((left, right) => left.path.localeCompare(right.path)), + [ + { path: "src/a.ts", state: "dismissed" }, + { path: "src/b.ts", state: "viewed" }, + ], + ); + }), +); + +it.effect("clears a mark again when the file is put back", () => + Effect.gen(function* () { + const asked: Array> = []; + const service = yield* environmentViewedService(new Map([["src/a.ts", "blob-a"]]), asked); + + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/a.ts", viewed: true }], + }); + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/a.ts", viewed: false }], + }); + const marked = yield* service.filesViewed(GITLAB_REFERENCE); + + assert.deepStrictEqual(marked.files, []); + // Unticking asks the host nothing: the row is going away whatever the head has. + assert.deepStrictEqual(asked, [["src/a.ts"]]); + }), +); + +it.effect("keeps a deleted file cleared, which the head has no version of at all", () => + Effect.gen(function* () { + const service = yield* environmentViewedService(new Map(), []); + + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/gone.ts", viewed: true }], + }); + yield* service.invalidate({ reference: GITLAB_REFERENCE }); + const marked = yield* service.filesViewed(GITLAB_REFERENCE); + + assert.deepStrictEqual(marked.files, [{ path: "src/gone.ts", state: "viewed" }]); + }), +); + +it.effect("leaves a mark alone when the host could not say what the head has of it", () => + Effect.gen(function* () { + // A host answers for as much of a long change as it can read in one go. Reading the rest as + // deleted would clear every file past the cut over a version nobody ever looked at. + const revisions = new Map([ + ["src/a.ts", "blob-a"], + ["src/past-the-cut.ts", "blob-b"], + ]); + const service = yield* environmentViewedService( + revisions, + [], + new Set(["src/past-the-cut.ts"]), + ); + + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [ + { path: "src/a.ts", viewed: true }, + { path: "src/past-the-cut.ts", viewed: true }, + ], + }); + revisions.set("src/a.ts", "blob-a-again"); + yield* service.invalidate({ reference: GITLAB_REFERENCE }); + const marked = yield* service.filesViewed(GITLAB_REFERENCE); + + assert.deepStrictEqual( + [...marked.files].toSorted((left, right) => left.path.localeCompare(right.path)), + [ + { path: "src/a.ts", state: "dismissed" }, + { path: "src/past-the-cut.ts", state: "viewed" }, + ], + ); + }), +); + +it.effect("keeps a file cleared that the press could not learn a version for", () => + Effect.gen(function* () { + // The press is the only moment a mark is given something to be measured against, and a host + // reading as much of a long change as it can manage does not always reach the file being + // ticked. Storing the empty version there reads as the head having nothing of the file, so the + // first read that does reach it reports the reader's own press back to them as work to do. + const revisions = new Map([["src/past-the-cut.ts", "blob-b"]]); + const unreadable = new Set(["src/past-the-cut.ts"]); + const service = yield* environmentViewedService(revisions, [], unreadable); + + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/past-the-cut.ts", viewed: true }], + }); + unreadable.delete("src/past-the-cut.ts"); + yield* service.invalidate({ reference: GITLAB_REFERENCE }); + + assert.deepStrictEqual((yield* service.filesViewed(GITLAB_REFERENCE)).files, [ + { path: "src/past-the-cut.ts", state: "viewed" }, + ]); + }), +); + +it.effect("keeps the version it last heard when a later read of the head stops short", () => + Effect.gen(function* () { + const asked: Array> = []; + const revisions = new Map([["src/a.ts", "blob-a"]]); + const unreadable = new Set(); + const service = yield* environmentViewedService(revisions, asked, unreadable); + + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/a.ts", viewed: true }], + }); + revisions.set("src/a.ts", "blob-a-again"); + yield* service.invalidate({ reference: GITLAB_REFERENCE }); + assert.deepStrictEqual((yield* service.filesViewed(GITLAB_REFERENCE)).files, [ + { path: "src/a.ts", state: "dismissed" }, + ]); + + // The read behind the next answer has to stop before this file. Forgetting the version it was + // last seen at would put the badge the reader has already been shown back to cleared, over an + // answer that said nothing about the file either way. + unreadable.add("src/a.ts"); + yield* TestClock.adjust("90 seconds"); + yield* service.filesViewed(GITLAB_REFERENCE); + yield* TestClock.adjust("20 seconds"); + + assert.deepStrictEqual((yield* service.filesViewed(GITLAB_REFERENCE)).files, [ + { path: "src/a.ts", state: "dismissed" }, + ]); + }), +); + +it.effect("forgets what the head had of a marked file once a mutation moves the head", () => + Effect.gen(function* () { + const revisions = new Map([["src/a.ts", "blob-a"]]); + const service = yield* environmentViewedService(revisions, []); + + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/a.ts", viewed: true }], + }); + // Merging moves the head under the mark, and nobody asks for the refresh: the mutation is + // the thing that knows, so it drops what it was holding rather than waiting to be told. + revisions.set("src/a.ts", "blob-a-again"); + yield* service.runAction({ ...GITLAB_REFERENCE, action: "merge" }); + + assert.deepStrictEqual((yield* service.filesViewed(GITLAB_REFERENCE)).files, [ + { path: "src/a.ts", state: "dismissed" }, + ]); + }), +); + +it.effect("still reports its own marks when the host will not say what the head has", () => + Effect.gen(function* () { + let answering = true; + const service = yield* makeService({ + projects: [ + project({ + id: "p1", + title: "on gitlab", + workspaceRoot: "/a", + repository: "group/project", + provider: "gitlab", + }), + ], + providers: [ + fakeProvider("gitlab", { + capabilities: { + diff: true, + comment: true, + actions: ["merge"], + mergeMethods: ["merge"], + search: true, + reactions: true, + viewedFiles: "environment", + review: FULL_REVIEW, + reviewers: FULL_REVIEWERS, + }, + getFilesViewed: () => Effect.die("the host keeps no marks of its own"), + setFilesViewed: () => Effect.die("the host keeps no marks of its own"), + getFileRevisions: (input) => + answering + ? Effect.succeed({ + revisions: new Map(input.paths.map((path) => [path, "blob-a"] as const)), + }) + : Effect.fail(requestFailed), + }), + ], + }); + + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/a.ts", viewed: true }], + }); + answering = false; + yield* service.invalidate({ reference: GITLAB_REFERENCE }); + + // The rows are this environment's own. A rate limit or a signed-out CLI costs them the + // staleness they would have carried, not the reader's whole record of what they have read. + assert.deepStrictEqual((yield* service.filesViewed(GITLAB_REFERENCE)).files, [ + { path: "src/a.ts", state: "viewed" }, + ]); + }), +); + +it.effect("finishes two presses on one file in the order they were made", () => + Effect.gen(function* () { + // A tick asks the host what it has of the file before it stores anything, and an untick asks + // nothing at all, so the second press would otherwise land first and be overwritten by the + // first one finishing behind it. + const held = yield* Deferred.make(); + const service = yield* makeService({ + projects: [ + project({ + id: "p1", + title: "on gitlab", + workspaceRoot: "/a", + repository: "group/project", + provider: "gitlab", + }), + ], + providers: [ + fakeProvider("gitlab", { + capabilities: { + diff: true, + comment: true, + actions: ["merge"], + mergeMethods: ["merge"], + search: true, + reactions: true, + viewedFiles: "environment", + review: FULL_REVIEW, + reviewers: FULL_REVIEWERS, + }, + getFilesViewed: () => Effect.die("the host keeps no marks of its own"), + setFilesViewed: () => Effect.die("the host keeps no marks of its own"), + getFileRevisions: (input) => + Deferred.await(held).pipe( + Effect.as({ revisions: new Map(input.paths.map((path) => [path, "blob-a"])) }), + ), + }), + ], + }); + + const tick = service + .setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/a.ts", viewed: true }], + }) + .pipe(Effect.runFork); + // Far enough for the tick to be waiting on the host rather than still on its way there. + yield* TestClock.adjust("1 second"); + const untick = service + .setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/a.ts", viewed: false }], + }) + .pipe(Effect.runFork); + yield* TestClock.adjust("1 second"); + yield* Deferred.succeed(held, undefined); + yield* Fiber.join(tick); + yield* Fiber.join(untick); + + // The untick came second and stands: the file is open again. + const marked = yield* service.filesViewed(GITLAB_REFERENCE); + assert.deepStrictEqual(marked.files, []); + }), +); + +it.effect("keeps the marks of two Azure repositories of the same name apart", () => + Effect.gen(function* () { + // Azure addresses a repository by its bare name, which is unique inside one of its projects + // and not across an organisation. Two `web` repositories would otherwise share one row. + const service = yield* makeService({ + projects: [ + project({ + id: "p1", + title: "platform web", + workspaceRoot: "/a", + repository: "acme/platform/_git/web", + provider: "azure-devops", + host: "dev.azure.com", + }), + project({ + id: "p2", + title: "other web", + workspaceRoot: "/b", + repository: "acme/other/_git/web", + provider: "azure-devops", + host: "dev.azure.com", + }), + ], + providers: [ + fakeProvider("azure-devops", { + capabilities: { + diff: true, + comment: true, + actions: ["merge"], + mergeMethods: ["merge"], + search: true, + reactions: true, + viewedFiles: "environment", + review: FULL_REVIEW, + reviewers: FULL_REVIEWERS, + }, + getFilesViewed: () => Effect.die("the host keeps no marks of this environment's own"), + setFilesViewed: () => Effect.die("the host keeps no marks of this environment's own"), + getFileRevisions: (input) => + Effect.succeed({ revisions: new Map(input.paths.map((path) => [path, "blob-a"])) }), + }), + ], + }); + const platform = { projectId: "p1" as ProjectId, repository: "web", number: 1 }; + const other = { projectId: "p2" as ProjectId, repository: "web", number: 1 }; + + yield* service.setFilesViewed({ ...platform, files: [{ path: "src/a.ts", viewed: true }] }); + + assert.deepStrictEqual((yield* service.filesViewed(platform)).files, [ + { path: "src/a.ts", state: "viewed" }, + ]); + assert.deepStrictEqual((yield* service.filesViewed(other)).files, []); + }), +); + +it.effect("keeps environment marks apart from another change request's", () => + Effect.gen(function* () { + const service = yield* environmentViewedService(new Map([["src/a.ts", "blob-a"]]), []); + + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/a.ts", viewed: true }], + }); + const other = yield* service.filesViewed({ ...GITLAB_REFERENCE, number: 2 }); + + assert.deepStrictEqual(other.files, []); + }), +); + +it.effect("refuses to track viewed files on a host that does not", () => + Effect.gen(function* () { + const service = yield* makeService({ + projects: [ + project({ + id: "p1", + title: "on gitlab", + workspaceRoot: "/a", + repository: "group/project", + provider: "gitlab", + }), + ], + providers: [ + fakeProvider("gitlab", { + getFilesViewed: () => Effect.die("must not be called"), + setFilesViewed: () => Effect.die("must not be called"), + }), + ], + }); + const reference = { projectId: "p1" as ProjectId, repository: "group/project", number: 1 }; + + const read = yield* Effect.flip(service.filesViewed(reference)); + const write = yield* Effect.flip( + service.setFilesViewed({ ...reference, files: [{ path: "a.ts", viewed: true }] }), + ); + + assert.strictEqual(read._tag, "PullRequestOperationError"); + assert.strictEqual(write._tag, "PullRequestOperationError"); + }), +); diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index b27fff3534a7..47df32cf1725 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -9,12 +9,14 @@ import * as Layer from "effect/Layer"; import * as PubSub from "effect/PubSub"; import * as Schema from "effect/Schema"; import type * as Scope from "effect/Scope"; +import * as Semaphore from "effect/Semaphore"; import * as Stream from "effect/Stream"; import { PullRequestOperationError, PullRequestUnavailableError, pullRequestHostOf, pullRequestProviderRequirement, + pullRequestRepositoryOf, resolvePullRequestAuthorFilter, type OrchestrationProjectShell, type PullRequestAction, @@ -27,6 +29,7 @@ import { type PullRequestDiffFileContentsResult, type PullRequestDiffStat, type PullRequestDiffInput, + type PullRequestFilesViewedResult, type PullRequestDiffResult, type PullRequestInvalidateInput, type PullRequestListEntry, @@ -44,6 +47,7 @@ import { type PullRequestReviewerRequestInput, type PullRequestLabelCandidateList, type PullRequestLabelChangeInput, + type PullRequestSetFilesViewedInput, type PullRequestSubmitReviewInput, type PullRequestSummary, type PullRequestThreadReplyInput, @@ -57,6 +61,7 @@ import { import { detectSourceControlProviderFromRemoteUrl } from "@t3tools/shared/sourceControl"; import * as ProjectionSnapshotQuery from "../orchestration/Services/ProjectionSnapshotQuery.ts"; +import * as PullRequestFilesViewed from "../persistence/PullRequestFilesViewed.ts"; import * as SourceControlProviderRegistry from "../sourceControl/SourceControlProviderRegistry.ts"; import * as SourceControlRateLimit from "../sourceControl/SourceControlRateLimit.ts"; import { @@ -115,6 +120,21 @@ const DIFF_CACHE_TTL = Duration.seconds(60); const COMMIT_DIFF_CACHE_TTL = Duration.minutes(10); /** Sized like the client's own stale time; a row's counts move only when somebody pushes. */ const LIST_STATS_CACHE_TTL = Duration.seconds(60); +/** + * Short, and with no stale window behind it: this is the reader's own bookkeeping, and the + * press that changes it is the same press the page is already showing optimistically. Held at + * all only so opening a change request on two devices costs one read. + */ +const FILES_VIEWED_CACHE_TTL = Duration.seconds(15); +/** + * How long the head's blob for a file is believed without asking the host again, and how long a + * held answer still stands while the next one is fetched. The marks themselves are this + * environment's own rows and cost nothing to read; this is the host call behind the **Changed** + * badge alone, so a held answer costs a badge that is a minute behind rather than a stale tick. + */ +const FILE_REVISIONS_CACHE_TTL = Duration.seconds(60); +const FILE_REVISIONS_STALE_WINDOW = Duration.minutes(10); +const FILE_REVISIONS_CACHE_CAPACITY = 64; /** A diff can stay interactive while its next cached value is fetched off the critical path. */ const DIFF_STALE_WINDOW = Duration.minutes(10); /** How long one host's signed-in login is believed without asking its CLI again. */ @@ -126,6 +146,7 @@ const LIST_CACHE_CAPACITY = 64; const LIST_STATS_CACHE_CAPACITY = 32; const DETAIL_CACHE_CAPACITY = 128; const DIFF_CACHE_CAPACITY = 128; +const FILES_VIEWED_CACHE_CAPACITY = 128; const VIEWER_CACHE_CAPACITY = 32; export type PullRequestError = PullRequestUnavailableError | PullRequestOperationError; @@ -161,6 +182,12 @@ export class PullRequestService extends Context.Service< readonly diffFileContents: ( input: PullRequestDiffFileContentsInput, ) => Effect.Effect; + readonly filesViewed: ( + input: PullRequestRef, + ) => Effect.Effect; + readonly setFilesViewed: ( + input: PullRequestSetFilesViewedInput, + ) => Effect.Effect; readonly runAction: (input: PullRequestActionInput) => Effect.Effect; readonly update: (input: PullRequestUpdateInput) => Effect.Effect; readonly comment: (input: PullRequestCommentInput) => Effect.Effect; @@ -482,6 +509,15 @@ function withRateLimitBackoff( ...(api.getDiffFileContents === undefined ? {} : { getDiffFileContents: wrap("getDiffFileContents", api.getDiffFileContents) }), + ...(api.getFilesViewed === undefined + ? {} + : { getFilesViewed: wrap("getFilesViewed", api.getFilesViewed) }), + ...(api.setFilesViewed === undefined + ? {} + : { setFilesViewed: interactive("setFilesViewed", api.setFilesViewed) }), + ...(api.getFileRevisions === undefined + ? {} + : { getFileRevisions: wrap("getFileRevisions", api.getFileRevisions) }), runAction: interactive("runAction", api.runAction), ...(api.updateChangeRequest === undefined ? {} @@ -506,27 +542,13 @@ function withRateLimitBackoff( } /** - * The provider-native repository selector. `displayName` is the full path below the host, which - * is what nested GitLab groups need; owner/name is the two-segment fallback for identities - * recorded before that field existed. - * - * Azure DevOps is the exception: `az repos pr list --repository` takes a repository name, and - * takes the organisation and project from the checkout it detects — so the recorded - * `org/project/_git/repo` path is refused outright and the whole repository reads as - * unavailable. Its name is the last segment, which is what this hands over. - * - * One function because everything downstream is keyed by what it answers: the rows' own - * `repository`, the per-repository cursors, and the detail and diff reads a row leads to. + * The provider-native repository selector for a project, which everything downstream is keyed by: + * the rows' own `repository`, the per-repository cursors, and the detail and diff reads a row + * leads to. The rule itself is shared with the clients that build a ref, since a ref spelled any + * other way is refused before it reaches a provider. */ export function repositoryIdentityOf(project: OrchestrationProjectShell): string | null { - const identity = project.repositoryIdentity; - if (!identity) return null; - if (identity.provider === "azure-devops") { - const segments = (identity.displayName ?? "").split("/").filter((part) => part !== "_git"); - return identity.name || segments.at(-1) || null; - } - if (identity.displayName) return identity.displayName; - return identity.owner && identity.name ? `${identity.owner}/${identity.name}` : null; + return pullRequestRepositoryOf(project.repositoryIdentity); } export const make = Effect.gen(function* () { @@ -535,6 +557,7 @@ export const make = Effect.gen(function* () { const projections = yield* ProjectionSnapshotQuery.ProjectionSnapshotQuery; const sourceControlProviders = yield* SourceControlProviderRegistry.SourceControlProviderRegistry; const rateLimits = yield* SourceControlRateLimit.SourceControlRateLimit; + const filesViewedStore = yield* PullRequestFilesViewed.PullRequestFilesViewedRepository; const refineUnknownProjectKinds = ( projects: ReadonlyArray, @@ -1424,6 +1447,382 @@ export const make = Effect.gen(function* () { }), ); + const context = yield* Effect.context(); + /** Runs a refresh as its own fiber, for the reads that answer from a held value first. */ + const runFork = Effect.runForkWith(context); + + /** + * Which repository a row belongs to, spelled widely enough that two of them are two rows. + * + * A provider's own selector is what the reads are addressed by, and Azure's is the bare + * repository name: unique inside one project and not across an organisation, so `api` in two + * projects would otherwise share one row and show each other's ticks. The remote already + * carries the whole path, so the marks are keyed by that instead. + */ + const filesViewedRepositoryOf = (project: SupportedProject) => { + if (project.api.kind !== "azure-devops") return project.repository; + const path = project.project.repositoryIdentity?.displayName?.trim(); + return path === undefined || path.length === 0 ? project.repository : path; + }; + + /** + * Which change request's marks, and whose. The host is part of it because the same + * `owner/repo` exists on more than one install, and the reader is part of it for the reason + * a host's own record is per-account. A host that will not say who is reading leaves it + * empty, which is one reader rather than none. + */ + const filesViewedScope = (project: SupportedProject, number: number, viewer: string | null) => ({ + provider: project.api.kind, + host: project.host, + repository: filesViewedRepositoryOf(project), + number, + viewer: viewer ?? "", + }); + + const toFilesViewedStoreError = (operation: string) => (cause: unknown) => + new PullRequestOperationError({ + operation, + detail: "This environment could not reach its record of which files you have seen.", + cause, + }); + + /** + * What the head has of the files a reader has marked, held between reads. A host says the empty + * revision for a file the change request deletes, and leaves out a path it could not look at, so + * the entry remembers what it has been asked as well as what it heard: a path asked for and + * missing from an answer keeps whatever version was last given for it. + */ + interface HeldFileRevisions { + readonly at: number; + readonly asked: ReadonlySet; + readonly revisions: ReadonlyMap; + } + const heldFileRevisions = new Map(); + const refreshingFileRevisions = new Set(); + /** + * Moved every time a held answer is dropped. A refresh already in flight when that happens + * still answers its own caller, and its answer is simply not kept: it was taken against a head + * the reader has since asked to stop believing, and keeping it would put the dropped entry + * straight back. One counter for every scope rather than one each, so an unrelated refresh + * costs an in-flight read its place in the cache and nothing else. + */ + let fileRevisionsGeneration = 0; + /** + * Normalised, because a reference reaches here spelled however the client spelled it while the + * project carries the remote's own spelling, and a refresh that missed by a capital would leave + * the held answer standing. + */ + const fileRevisionsScope = (projectId: string, repository: string, number: number) => + `${projectId} ${repository.trim().toLowerCase()} ${number}`; + + const recordFileRevisions = ( + scope: string, + paths: ReadonlyArray, + answer: ReadonlyMap, + generation: number, + ) => + Effect.map(Clock.currentTimeMillis, (at) => { + // Answered from, never stored: the caller asked for this and it is as fresh as anything + // could be, but the scope it belongs to has been dropped since the read began. + if (generation !== fileRevisionsGeneration) return answer; + const held = heldFileRevisions.get(scope); + // Past the stale window the old entry is not worth merging into: it would carry paths + // nobody has asked about since, at revisions the head has long moved off. + const carried = + held !== undefined && at - held.at <= Duration.toMillis(FILE_REVISIONS_STALE_WINDOW) + ? held + : null; + const revisions = new Map(carried?.revisions ?? []); + const asked = new Set(carried?.asked ?? []); + for (const path of paths) { + asked.add(path); + const revision = answer.get(path); + // Left out of the answer is the host not saying, not the head having nothing: deleting + // the version it last gave would turn a file already reported as changed back into a + // cleared one on the next answer that had to stop short. + if (revision !== undefined) revisions.set(path, revision); + } + heldFileRevisions.delete(scope); + if (heldFileRevisions.size >= FILE_REVISIONS_CACHE_CAPACITY) { + const oldest = heldFileRevisions.keys().next().value; + if (oldest !== undefined) heldFileRevisions.delete(oldest); + } + // The entry is only as fresh as the oldest revision in it. Stamping it with now because + // this read answered would let a reader ticking one new file after another keep carrying the + // first file's revision past the point it would have been read again, since every press + // renews the whole scope while asking about one path. + const stamped = [...revisions.keys()].every((path) => answer.has(path)) + ? at + : (carried?.at ?? at); + heldFileRevisions.set(scope, { at: stamped, asked, revisions }); + return revisions; + }); + + /** A held entry that covers every path asked for and is still worth answering from. */ + const heldFileRevisionsFor = (scope: string, paths: ReadonlyArray, now: number) => { + const held = heldFileRevisions.get(scope); + if (held === undefined) return null; + if (now - held.at > Duration.toMillis(FILE_REVISIONS_STALE_WINDOW)) return null; + return paths.every((path) => held.asked.has(path)) ? held : null; + }; + + const forgetFileRevisions = (scope: string) => { + heldFileRevisions.delete(scope); + fileRevisionsGeneration += 1; + }; + + const forgetEveryFileRevision = () => { + heldFileRevisions.clear(); + fileRevisionsGeneration += 1; + }; + + /** + * What the head has of these files, or null where the host cannot say. Null is not an error: + * without it the marks simply stop reporting staleness, which is worse than the host's own + * record but better than refusing to remember anything. + * + * `held` answers from a value past its lifetime and fetches the next one off the critical path, + * because a badge a moment behind beats a page of ticks that will not paint until a host answers. + * `fresh` is for the press itself, which stamps what it stores and would otherwise write a + * revision the head had already moved off. + */ + const fileRevisionsOf = ( + project: SupportedProject, + number: number, + paths: ReadonlyArray, + operation: string, + freshness: "held" | "fresh" = "held", + ): Effect.Effect | null, PullRequestError> => { + const read = project.api.getFileRevisions; + if (read === undefined) return Effect.succeed(null); + const scope = fileRevisionsScope(project.project.id, project.repository, number); + // Suspended, so a held answer costs the host nothing: a provider is free to do its work as + // the request is built rather than as the effect is run. + const fetch = Effect.suspend(() => { + const generation = fileRevisionsGeneration; + return read({ + cwd: project.project.workspaceRoot, + repository: project.repository, + host: project.host, + number, + paths, + }).pipe( + Effect.mapError(toPullRequestError(operation)), + Effect.flatMap((answer) => recordFileRevisions(scope, paths, answer.revisions, generation)), + ); + }); + return Effect.flatMap(Clock.currentTimeMillis, (now) => { + const held = heldFileRevisionsFor(scope, paths, now); + if (held === null) return fetch; + if (now - held.at <= Duration.toMillis(FILE_REVISIONS_CACHE_TTL)) + return Effect.succeed(held.revisions); + if (freshness === "fresh") return fetch; + if (refreshingFileRevisions.has(scope)) return Effect.succeed(held.revisions); + // Its own fiber rather than a child: the caller has been answered and is gone before this + // lands. One at a time per change request, so a page of files costs one host read. + return Effect.sync(() => { + refreshingFileRevisions.add(scope); + runFork( + Effect.ignore(fetch).pipe( + Effect.ensuring(Effect.sync(() => refreshingFileRevisions.delete(scope))), + ), + ); + }).pipe(Effect.as(held.revisions)); + }); + }; + + /** + * The marks this environment keeps for a host that keeps none of its own. + * + * A file the head still has at the revision it was cleared at is cleared; one the head has + * moved on from is reported as changed, which is what GitHub says of a file pushed to since it + * was ticked. Revisions are asked for the marked paths alone, so the cost follows how much of + * the change request has been read rather than how large it is, and a reader who has marked + * nothing costs no host call at all. + */ + const environmentFilesViewed = ( + project: SupportedProject, + number: number, + ): Effect.Effect => + Effect.gen(function* () { + const viewer = yield* viewerOf(project); + const marks = yield* filesViewedStore + .list(filesViewedScope(project, number, viewer)) + .pipe(Effect.mapError(toFilesViewedStoreError("filesViewed"))); + if (marks.length === 0) return { files: [], truncated: false }; + // These rows are this environment's own. A rate limit or a signed-out CLI costs the marks + // their staleness, which is the thing `fileRevisionsOf` already answers null for, and must + // not cost the reader every tick they have made. The press itself still fails loudly on a + // host that errors, since a mark stamped with a revision nobody read is wrong rather than + // merely less informed; a host that answers without the path is stored with no baseline. + const revisions = yield* fileRevisionsOf( + project, + number, + marks.map((mark) => mark.path), + "filesViewed", + ).pipe( + Effect.catch((error) => + Effect.logWarning("reporting viewed files without what the head has of them", { + operation: "filesViewed", + reason: error._tag, + }).pipe(Effect.as(null)), + ), + ); + return { + files: marks.map((mark) => { + // A path the host had no answer for is one it could not look at, not one it looked at + // and found nothing: a read that saw part of a large change must not report the rest + // as changed against a revision nobody read. A file the change request deletes is + // answered as the empty revision, which is what its mark was stamped with, so it is + // cleared once and stays cleared. + // A mark stamped with no baseline has nothing to compare against, so it holds until + // the reader presses it again. That is the press the host would not answer for, and + // reporting it as changed against a revision it was never measured at would move the + // file the reader just cleared back into the pile. + if (mark.revision === null) return { path: mark.path, state: "viewed" as const }; + const revision = revisions?.get(mark.path); + return { + path: mark.path, + state: + revision === undefined || revision === mark.revision + ? ("viewed" as const) + : ("dismissed" as const), + }; + }), + // Every mark is a row this environment holds, so there is no page to run out of. + truncated: false, + }; + }); + + /** + * One environment-backed write at a time per change request. A tick asks the host what it has + * of the file before it stores anything and an untick asks nothing at all, so two presses in + * quick succession would otherwise finish in the other order and leave the tick's row standing + * over the untick that came after it. + */ + const filesViewedGates = new Map< + string, + { readonly gate: Semaphore.Semaphore; pending: number } + >(); + + const inFilesViewedOrder = ( + project: SupportedProject, + number: number, + write: Effect.Effect, + ) => + // Suspended rather than generated, so finding the gate, putting it in and taking a place in + // its queue are one step. `Semaphore.make` is an effect, and yielding for it between the + // lookup and the insert lets two presses each find nothing, each make a gate of their own, + // and neither wait on the other, which is the ordering this exists for. + Effect.suspend(() => { + const key = `${project.project.id} ${filesViewedRepositoryOf(project).trim().toLowerCase()} ${number}`; + const held = filesViewedGates.get(key); + const entry = held ?? { gate: Semaphore.makeUnsafe(1), pending: 0 }; + if (held === undefined) filesViewedGates.set(key, entry); + entry.pending += 1; + // Dropped once nobody is queued behind it, so a long-lived server does not keep a gate per + // change request anyone has ever ticked a file in. + return entry.gate + .withPermits(1)(write) + .pipe( + Effect.ensuring( + Effect.sync(() => { + entry.pending -= 1; + if (entry.pending === 0) filesViewedGates.delete(key); + }), + ), + ); + }); + + const environmentSetFilesViewed = ( + project: SupportedProject, + input: PullRequestSetFilesViewedInput, + ): Effect.Effect => + Effect.gen(function* () { + const viewer = yield* viewerOf(project); + // Only the files being cleared need a revision. An unticked one is about to lose its row, + // and what the head has of it changes nothing about deleting it. + const cleared = input.files.filter((file) => file.viewed).map((file) => file.path); + const revisions = + cleared.length === 0 + ? null + : yield* fileRevisionsOf(project, input.number, cleared, "setFilesViewed", "fresh"); + const viewedAt = DateTime.formatIso(yield* DateTime.now); + yield* filesViewedStore + .set({ + ...filesViewedScope(project, input.number, viewer), + // A path left out of the answer is the host declining to say, not the head having + // nothing of the file: the empty revision is an answer, and a mark stamped with it is + // reported as changed as soon as the file turns out to have a version after all. Such a + // mark is stored with no baseline instead, and a host too far behind to answer for a + // large change stays tickable rather than clearing files that come straight back. + files: input.files.map((file) => ({ + path: file.path, + revision: revisions?.get(file.path) ?? null, + viewed: file.viewed, + })), + viewedAt, + }) + .pipe(Effect.mapError(toFilesViewedStoreError("setFilesViewed"))); + }); + + const filesViewedUncached = (input: PullRequestRef) => + requireProject(input).pipe( + Effect.flatMap((project): Effect.Effect => { + const read = project.api.getFilesViewed; + if (project.api.capabilities.viewedFiles === "host" && read) { + return read({ + cwd: project.project.workspaceRoot, + repository: project.repository, + host: project.host, + number: input.number, + }).pipe(Effect.mapError(toPullRequestError("filesViewed"))); + } + if (project.api.capabilities.viewedFiles === "environment") { + return environmentFilesViewed(project, input.number); + } + return Effect.fail( + new PullRequestOperationError({ + operation: "filesViewed", + detail: "This host does not track which files a reader has seen.", + }), + ); + }), + ); + + const setFilesViewed: PullRequestService["Service"]["setFilesViewed"] = (input) => + requireProject(input).pipe( + Effect.flatMap((project): Effect.Effect => { + const write = project.api.setFilesViewed; + if (project.api.capabilities.viewedFiles === "host" && write) { + return write({ + cwd: project.project.workspaceRoot, + repository: project.repository, + host: project.host, + number: input.number, + files: input.files, + }).pipe(Effect.mapError(toPullRequestError("setFilesViewed"))); + } + if (project.api.capabilities.viewedFiles === "environment") { + return inFilesViewedOrder( + project, + input.number, + environmentSetFilesViewed(project, input), + ); + } + return Effect.fail( + new PullRequestOperationError({ + operation: "setFilesViewed", + detail: "This host does not track which files a reader has seen.", + }), + ); + }), + // Deliberately not `invalidatedByMutation`: ticking a file off says nothing about the + // change request, and dropping a 300-file diff on every checkbox is the whole cost of + // the feature. Only this reader's own bookkeeping is forgotten. + Effect.tap(() => Effect.sync(() => bumpFilesViewedEpoch(input))), + ); + const runAction = (input: PullRequestActionInput): Effect.Effect => requireProject(input).pipe( Effect.flatMap((project): Effect.Effect => { @@ -2023,9 +2422,6 @@ export const make = Effect.gen(function* () { return { stats: stats.flat() }; }); - const context = yield* Effect.context(); - const runFork = Effect.runForkWith(context); - /** * The diff is not live-polled and is expensive enough to keep its stale-while-revalidate path. * Explicit refreshes and mutations still strand held values through the reference epoch. @@ -2129,14 +2525,20 @@ export const make = Effect.gen(function* () { const refEpoch = (ref: PullRequestRef) => refEpochs.get(refScope(ref)) ?? 0; const refCacheKey = (ref: PullRequestRef) => JSON.stringify([refEpoch(ref), ref.projectId, ref.repository, ref.number]); - const bumpRefEpoch = (ref: PullRequestRef) => { + const bumpEpoch = (epochs: Map, ref: PullRequestRef) => { const scope = refScope(ref); - if (!refEpochs.has(scope) && refEpochs.size >= REF_EPOCH_CAPACITY) { - const oldest = refEpochs.keys().next().value; - if (oldest !== undefined) refEpochs.delete(oldest); + if (!epochs.has(scope) && epochs.size >= REF_EPOCH_CAPACITY) { + const oldest = epochs.keys().next().value; + if (oldest !== undefined) epochs.delete(oldest); } - refEpochs.set(scope, ++epochCounter); + epochs.set(scope, ++epochCounter); }; + const bumpRefEpoch = (ref: PullRequestRef) => bumpEpoch(refEpochs, ref); + // Its own scope, so a press forgets the reader's ticks and nothing else. The read's key + // carries both epochs, which is what makes an ordinary refresh re-ask for these too. + const filesViewedEpochs = new Map(); + const filesViewedEpoch = (ref: PullRequestRef) => filesViewedEpochs.get(refScope(ref)) ?? 0; + const bumpFilesViewedEpoch = (ref: PullRequestRef) => bumpEpoch(filesViewedEpochs, ref); /** The positional filter slot of a cache key, back as the record `listUncached` takes. */ const filtersOfKey = ( @@ -2356,6 +2758,34 @@ export const make = Effect.gen(function* () { return staleDiff(key, Cache.get(diffCache, key)); }; + const filesViewedCache = yield* Cache.makeWith( + (key: string) => { + const [, , projectId, repository, number] = JSON.parse(key) as [ + number, + number, + string, + string, + number, + ]; + return filesViewedUncached({ projectId, repository, number } as PullRequestRef); + }, + { + capacity: FILES_VIEWED_CACHE_CAPACITY, + timeToLive: (exit) => (Exit.isSuccess(exit) ? FILES_VIEWED_CACHE_TTL : Duration.zero), + }, + ); + const filesViewed: PullRequestService["Service"]["filesViewed"] = (input) => + Cache.get( + filesViewedCache, + JSON.stringify([ + refEpoch(input), + filesViewedEpoch(input), + input.projectId, + input.repository, + input.number, + ]), + ); + const listStatsCache = yield* Cache.makeWith( (key: string) => { const [, refs] = JSON.parse(key) as [number, ReadonlyArray<[string, string, number]>]; @@ -2388,11 +2818,20 @@ export const make = Effect.gen(function* () { const invalidate: PullRequestService["Service"]["invalidate"] = (input) => { const reference = input.reference; if (reference !== undefined) { - return Effect.sync(() => bumpRefEpoch(reference)); + return Effect.sync(() => { + bumpRefEpoch(reference); + // Not keyed by epoch, so this one is dropped by hand rather than stranded. + forgetFileRevisions( + fileRevisionsScope(reference.projectId, reference.repository, reference.number), + ); + }); } + // A whole-workspace refresh is the reader asking to be re-answered from the hosts, + // and that includes who the hosts say they are. return Effect.sync(() => { listingsEpoch = ++epochCounter; viewersByHost.clear(); + forgetEveryFileRevision(); }).pipe(Effect.andThen(Cache.invalidateAll(viewerFlights))); }; @@ -2409,6 +2848,12 @@ export const make = Effect.gen(function* () { Effect.sync(() => { bumpRefEpoch(input); listingsEpoch = ++epochCounter; + // Not keyed by epoch, so this one is dropped by hand. Merging or bringing a stale + // branch up to date moves the head, and a mark compared against what the head had + // before it moved reports a file as cleared that has been pushed to since. + forgetFileRevisions( + fileRevisionsScope(input.projectId, input.repository, input.number), + ); }), ), ); @@ -2418,6 +2863,10 @@ export const make = Effect.gen(function* () { const repository = yield* runAction(input); bumpRefEpoch({ ...input, repository }); listingsEpoch = ++epochCounter; + // Not keyed by epoch, so this one is dropped by hand. Merging or bringing a stale branch up + // to date moves the head, and a mark compared against what the head had before it moved + // reports a file as cleared that has been pushed to since. + forgetFileRevisions(fileRevisionsScope(input.projectId, repository, input.number)); if (input.action === "merge") { yield* PubSub.publish(mergedPullRequests, { projectId: input.projectId, @@ -2440,6 +2889,8 @@ export const make = Effect.gen(function* () { threadComments, diff, diffFileContents, + filesViewed, + setFilesViewed, runAction: runActionAndInvalidate, update: invalidatedByMutation(update), comment: invalidatedByMutation(comment), diff --git a/apps/server/src/pullRequest/azureDevOpsDiff.test.ts b/apps/server/src/pullRequest/azureDevOpsDiff.test.ts new file mode 100644 index 000000000000..3d6ab8d39830 --- /dev/null +++ b/apps/server/src/pullRequest/azureDevOpsDiff.test.ts @@ -0,0 +1,232 @@ +import { describe, expect, it } from "vite-plus/test"; + +import { + azureDevOpsFilePatch, + azureDevOpsUnreadableFilePatch, + formatAzureDevOpsDiffCursor, + parseAzureDevOpsDiffCursor, +} from "./azureDevOpsDiff.ts"; +import type { AzureDevOpsChangeEntry } from "./azureDevOpsPullRequestJson.ts"; + +function change(overrides: Partial = {}): AzureDevOpsChangeEntry { + return { + path: "README.md", + oldPath: "README.md", + changeKind: "change", + objectId: "8f80", + originalObjectId: "0ca4", + ...overrides, + }; +} + +function texts(oldContents: string, newContents: string, binary = false) { + return { oldContents, newContents, binary }; +} + +describe("azureDevOpsFilePatch", () => { + it("writes a changed file as the unified patch every diff viewer already reads", () => { + const patch = azureDevOpsFilePatch({ + change: change(), + texts: texts("one\ntwo\nthree\n", "one\ntwo again\nthree\n"), + }); + + expect(patch.truncated).toBe(false); + expect(patch.section).toBe( + [ + "diff --git a/README.md b/README.md", + "--- a/README.md", + "+++ b/README.md", + "@@ -1,3 +1,3 @@", + " one", + "-two", + "+two again", + " three", + "", + ].join("\n"), + ); + }); + + it("names the side a new file does not have as /dev/null", () => { + const patch = azureDevOpsFilePatch({ + change: change({ path: "DEMO.md", oldPath: "DEMO.md", changeKind: "new" }), + texts: texts("", "hello\n"), + }); + + expect(patch.section).toContain("new file mode 100644"); + expect(patch.section).toContain("--- /dev/null"); + expect(patch.section).toContain("+++ b/DEMO.md"); + // Git points the range a new file does not have at line zero, not at line one. + expect(patch.section).toContain("@@ -0,0 +1 @@"); + expect(patch.section).toContain("+hello"); + }); + + it("names the side a deleted file no longer has as /dev/null", () => { + const patch = azureDevOpsFilePatch({ + change: change({ path: "OLD.md", oldPath: "OLD.md", changeKind: "deleted" }), + texts: texts("gone\n", ""), + }); + + expect(patch.section).toContain("deleted file mode 100644"); + expect(patch.section).toContain("--- a/OLD.md"); + expect(patch.section).toContain("+++ /dev/null"); + expect(patch.section).toContain("@@ -1 +0,0 @@"); + expect(patch.section).toContain("-gone"); + }); + + it("keeps the carriage returns of a file with Windows line endings", () => { + // They are part of the line rather than around it, so a patch that dropped them would ask + // the reader to look at a change that is not the one on the host. + const patch = azureDevOpsFilePatch({ + change: change(), + texts: texts("one\r\ntwo\r\n", "one\r\ntwo again\r\n"), + }); + + expect(patch.section).toContain("-two\r"); + expect(patch.section).toContain("+two again\r"); + }); + + it("keeps a file that only moved, which has no hunks to give", () => { + const patch = azureDevOpsFilePatch({ + change: change({ path: "docs/new.md", oldPath: "docs/old.md", changeKind: "rename-pure" }), + texts: texts("same\n", "same\n"), + }); + + expect(patch.truncated).toBe(false); + expect(patch.section).toBe( + [ + "diff --git a/docs/old.md b/docs/new.md", + "rename from docs/old.md", + "rename to docs/new.md", + "--- a/docs/old.md", + "+++ b/docs/new.md", + "", + ].join("\n"), + ); + }); + + it("reports a binary file as changed rather than spelling it out", () => { + const patch = azureDevOpsFilePatch({ + change: change({ path: "logo.png", oldPath: "logo.png" }), + texts: texts("PNG\u0000old", "PNG\u0000new"), + }); + + expect(patch.truncated).toBe(true); + expect(patch.section).toContain("Binary files a/logo.png and b/logo.png differ"); + }); + + it("shows an overlong file as changed without its hunks", () => { + const patch = azureDevOpsFilePatch({ + change: change({ path: "bundle.js", oldPath: "bundle.js" }), + texts: texts("a\n".repeat(400_000), "b\n".repeat(400_000)), + }); + + expect(patch.truncated).toBe(true); + expect(patch.section).toBe( + ["diff --git a/bundle.js b/bundle.js", "--- a/bundle.js", "+++ b/bundle.js", ""].join("\n"), + ); + }); + + it("takes the host's word that a file is binary, whatever its bytes look like", () => { + // Azure hands such a file over base64-encoded, so nothing in the text it sent gives it away. + const patch = azureDevOpsFilePatch({ + change: change({ path: "logo.png", oldPath: "logo.png" }), + texts: texts("b2xk", "bmV3", true), + }); + + expect(patch.truncated).toBe(true); + expect(patch.section).toContain("Binary files a/logo.png and b/logo.png differ"); + }); + + it("counts an overlong file in bytes rather than in characters", () => { + // Three bytes each, so a ceiling counted in code units would let three times the size through. + const patch = azureDevOpsFilePatch({ + change: change({ path: "notes.md", oldPath: "notes.md" }), + texts: texts("\u4e00".repeat(200_000), "\u4e8c".repeat(200_000)), + }); + + expect(patch.truncated).toBe(true); + expect(patch.section).toBe( + ["diff --git a/notes.md b/notes.md", "--- a/notes.md", "+++ b/notes.md", ""].join("\n"), + ); + }); + + it("gives up on a file whose two sides are too far apart to diff in the time allowed", () => { + // The line diff costs the product of the two sides, so a pair under the size ceiling that + // shares nothing still runs long. Left to itself it would hold the server for as long as it + // took; here it is given a millisecond so the giving up is the thing being read. + const oldContents = Array.from({ length: 3_000 }, (_, line) => `old ${line}`).join("\n"); + const newContents = Array.from({ length: 3_000 }, (_, line) => `new ${line}`).join("\n"); + const patch = azureDevOpsFilePatch({ + change: change({ path: "generated.ts", oldPath: "generated.ts" }), + texts: texts(oldContents, newContents), + timeoutMillis: 1, + }); + + expect(patch.truncated).toBe(true); + // And it says so, because the reader of a run of files is meant to stop rather than spend + // that time again on each of the ones behind it. + expect(patch.abandoned).toBe(true); + expect(patch.section).toBe( + [ + "diff --git a/generated.ts b/generated.ts", + "--- a/generated.ts", + "+++ b/generated.ts", + "", + ].join("\n"), + ); + }); + + it("keeps a file it did diff out of the giving up", () => { + const patch = azureDevOpsFilePatch({ + change: change(), + texts: texts("one\ntwo\n", "one\ntwo again\n"), + }); + + expect(patch.abandoned).toBe(false); + }); + + it("marks a file that does not end in a newline, as git does", () => { + const patch = azureDevOpsFilePatch({ + change: change(), + texts: texts("one\n", "two"), + }); + + expect(patch.section).toContain("\\ No newline at end of file"); + }); +}); + +describe("azureDevOpsUnreadableFilePatch", () => { + it("keeps a file the host would not hand over, listed without its hunks", () => { + const patch = azureDevOpsUnreadableFilePatch(change({ path: "huge.bin", oldPath: "huge.bin" })); + + expect(patch.truncated).toBe(true); + expect(patch.section).toBe( + ["diff --git a/huge.bin b/huge.bin", "--- a/huge.bin", "+++ b/huge.bin", ""].join("\n"), + ); + }); +}); + +describe("a diff cursor", () => { + it("carries the push it was taken against back to the next slice", () => { + const cursor = formatAzureDevOpsDiffCursor({ iterationId: 3, fileIndex: 12 }); + + expect(parseAzureDevOpsDiffCursor(cursor)).toEqual({ iterationId: 3, fileIndex: 12 }); + }); + + it("reads anything it did not write as no position at all", () => { + // Which starts the read from the top rather than failing it: a cursor is the client's to + // hand back, and nothing downstream is worth refusing a whole diff over. + for (const raw of [undefined, null, "", "abc", "1", "0:4", "1:-2", "1:2:3"]) { + expect(parseAzureDevOpsDiffCursor(raw)).toBeNull(); + } + }); + + it("refuses a half it did not write rather than reading it as the first file", () => { + // `Number` is wider than the cursor: an empty, padded or hex half would otherwise pass as a + // position, and the read would resume against an iteration the client never saw instead of + // starting again from the latest one. + for (const raw of ["1:", ":4", "1: ", " 1:4", "1:0x2", "0x1:2", "1e2:0", "1:4.0"]) { + expect(parseAzureDevOpsDiffCursor(raw)).toBeNull(); + } + }); +}); diff --git a/apps/server/src/pullRequest/azureDevOpsDiff.ts b/apps/server/src/pullRequest/azureDevOpsDiff.ts new file mode 100644 index 000000000000..f8a451ae483f --- /dev/null +++ b/apps/server/src/pullRequest/azureDevOpsDiff.ts @@ -0,0 +1,195 @@ +import { structuredPatch } from "diff"; + +import type { AzureDevOpsChangeEntry } from "./azureDevOpsPullRequestJson.ts"; + +/** + * How far a diff read got, and which push it was reading. Azure hangs a pull request's changed + * files off an iteration, so the iteration travels with the position: a push landing mid-read + * would otherwise renumber the list under the cursor and hand the reader a file twice or not at + * all. + */ +export interface AzureDevOpsDiffCursor { + readonly iterationId: number; + readonly fileIndex: number; +} + +const CURSOR_SEPARATOR = ":"; + +/** + * Both halves are plain decimal, because `Number` is wider than what was written: it reads an + * empty or padded half as zero and `0x3` as three, so a cursor this did not write would resume + * from a position nothing ever handed out. + */ +const CURSOR_COMPONENT = /^\d+$/; + +export function formatAzureDevOpsDiffCursor(cursor: AzureDevOpsDiffCursor): string { + return `${cursor.iterationId}${CURSOR_SEPARATOR}${cursor.fileIndex}`; +} + +/** Null for anything this did not write, which starts the read from the top rather than failing. */ +export function parseAzureDevOpsDiffCursor( + raw: string | null | undefined, +): AzureDevOpsDiffCursor | null { + if (raw === null || raw === undefined) return null; + const [iteration, file, ...rest] = raw.split(CURSOR_SEPARATOR); + if (rest.length > 0) return null; + if (iteration === undefined || file === undefined) return null; + if (!CURSOR_COMPONENT.test(iteration) || !CURSOR_COMPONENT.test(file)) return null; + const iterationId = Number(iteration); + const fileIndex = Number(file); + if (!Number.isSafeInteger(iterationId) || iterationId <= 0) return null; + if (!Number.isSafeInteger(fileIndex) || fileIndex < 0) return null; + return { iterationId, fileIndex }; +} + +/** The two texts of one changed file, empty on whichever side the change does not have. */ +export interface AzureDevOpsFileTexts { + readonly oldContents: string; + readonly newContents: string; + /** + * The host's own word on whether this is a file it will not spell out. Azure hands such a file + * over base64-encoded, so its bytes are not in the text to be looked for. + */ + readonly binary: boolean; +} + +export interface AzureDevOpsFilePatch { + readonly section: string; + /** The file changed but its hunks are not in the section, so the patch has a hole in it. */ + readonly truncated: boolean; + /** + * The diff was given up on partway rather than declined on sight, so this file spent the whole + * of what one file is allowed and produced a header for it. The caller reading a run of files + * is meant to stop here rather than pay that again for each of the ones behind it. + */ + readonly abandoned: boolean; +} + +/** + * Beyond this a file is shown as changed without its hunks. Azure hands back whole files rather + * than a patch, so a generated bundle or a checked-in dump is paid for twice over before anything + * can be diffed, and nobody reads the result either way. + */ +const MAX_FILE_BYTES = 512 * 1024; + +/** Git's own default, and what the hunks from this repo's other hosts are already cut to. */ +const PATCH_CONTEXT_LINES = 3; + +/** + * How long one file may be diffed for. The line diff costs the product of the two sides, so a pair + * of files under the size ceiling that share almost nothing can still hold the whole server for a + * long time. Past this the file is listed without its hunks, which is what the size ceiling already + * does and what the reader is already shown a sign of. + */ +export const MAX_FILE_DIFF_MILLIS = 2_000; + +/** + * How much patch one slice carries before the rest is left for the next one. Every file costs a + * request per side, so the read stops on what it has produced rather than on a file count: a + * hundred one-line changes are cheaper to finish than three long ones. + */ +export const MAX_DIFF_SLICE_BYTES = 256 * 1024; + +/** A NUL byte is git's own test for it, and it survives Azure's JSON envelope intact. */ +function isBinary(contents: string): boolean { + return contents.includes("\u0000"); +} + +/** + * What a file costs on the wire, which is its bytes rather than its code units: a ceiling counted + * in characters lets a file of three-byte glyphs through at three times the size meant to be let + * through. + */ +export const byteLength = (contents: string) => Buffer.byteLength(contents, "utf8"); + +/** + * Git points an empty range at the line before it, which is line zero for a file that is wholly + * new or wholly gone, and writes a single line as its number alone. + */ +function hunkRange(start: number, lines: number): string { + if (lines === 0) return `${start - 1},0`; + return lines === 1 ? String(start) : `${start},${lines}`; +} + +/** + * The `diff --git` preamble a viewer reads a file's identity and fate from. Azure reports no file + * mode, so the ordinary one stands in, exactly as it does for the GitHub files API here. + */ +function patchHeader(change: AzureDevOpsChangeEntry): string { + const lines = [`diff --git a/${change.oldPath} b/${change.path}`]; + if (change.changeKind === "new") lines.push("new file mode 100644"); + if (change.changeKind === "deleted") lines.push("deleted file mode 100644"); + if (change.changeKind === "rename-pure" || change.changeKind === "rename-changed") { + lines.push(`rename from ${change.oldPath}`, `rename to ${change.path}`); + } + lines.push( + `--- ${change.changeKind === "new" ? "/dev/null" : `a/${change.oldPath}`}`, + `+++ ${change.changeKind === "deleted" ? "/dev/null" : `b/${change.path}`}`, + ); + return lines.join("\n"); +} + +/** + * One file's section of a unified patch, built here because Azure has no route that carries one: + * its diff routes name the files that changed and their blob ids, and the contents are a separate + * read per side. + */ +export function azureDevOpsFilePatch(input: { + readonly change: AzureDevOpsChangeEntry; + readonly texts: AzureDevOpsFileTexts; + /** How long this one file may be diffed for, at most what any file is allowed. */ + readonly timeoutMillis?: number; +}): AzureDevOpsFilePatch { + const header = patchHeader(input.change); + const { oldContents, newContents } = input.texts; + + if (input.texts.binary || isBinary(oldContents) || isBinary(newContents)) { + // Git's own wording for a file it will not spell out, which every diff viewer already reads. + const binary = `Binary files a/${input.change.oldPath} and b/${input.change.path} differ`; + return { section: `${header}\n${binary}\n`, truncated: true, abandoned: false }; + } + if (byteLength(oldContents) > MAX_FILE_BYTES || byteLength(newContents) > MAX_FILE_BYTES) { + return { section: `${header}\n`, truncated: true, abandoned: false }; + } + + const patch = structuredPatch( + `a/${input.change.oldPath}`, + `b/${input.change.path}`, + oldContents, + newContents, + undefined, + undefined, + { + context: PATCH_CONTEXT_LINES, + timeout: Math.min(input.timeoutMillis ?? MAX_FILE_DIFF_MILLIS, MAX_FILE_DIFF_MILLIS), + }, + ); + // The bound is reported by giving nothing back, and a file whose diff was given up on is a file + // listed without its hunks rather than a file dropped from the change. + if (patch === undefined) return { section: `${header}\n`, truncated: true, abandoned: true }; + + const hunks = patch.hunks.map((hunk) => + [ + `@@ -${hunkRange(hunk.oldStart, hunk.oldLines)} +${hunkRange(hunk.newStart, hunk.newLines)} @@`, + ...hunk.lines, + ].join("\n"), + ); + // A pure rename has no hunks to give. It is still listed, because dropping it would take the + // file out of the change altogether. + return { + section: hunks.length === 0 ? `${header}\n` : `${header}\n${hunks.join("\n")}\n`, + truncated: false, + abandoned: false, + }; +} + +/** + * A file listed without its hunks, for when the host would not hand one of its two sides over. + * The change still belongs in the patch: leaving it out would take the file out of the review + * altogether, and the reader would have no sign anything was missing. + */ +export function azureDevOpsUnreadableFilePatch( + change: AzureDevOpsChangeEntry, +): AzureDevOpsFilePatch { + return { section: `${patchHeader(change)}\n`, truncated: true, abandoned: false }; +} diff --git a/apps/server/src/pullRequest/azureDevOpsPullRequestJson.test.ts b/apps/server/src/pullRequest/azureDevOpsPullRequestJson.test.ts index 6f55eb937bc9..9b707662cb27 100644 --- a/apps/server/src/pullRequest/azureDevOpsPullRequestJson.test.ts +++ b/apps/server/src/pullRequest/azureDevOpsPullRequestJson.test.ts @@ -2,6 +2,9 @@ import * as Result from "effect/Result"; import { describe, expect, it } from "vite-plus/test"; import { + decodeItemContentJson, + decodeIterationChangesJson, + decodeIterationsJson, decodePullRequestJson, decodePullRequestListJson, decodeThreadsJson, @@ -187,17 +190,15 @@ describe("decodePullRequestJson", () => { expect(unspecified?.autoMergeMethod).toBeUndefined(); }); - it("works out where the conversation lives from what Azure returned", () => { + it("works out where the repository lives from what Azure returned", () => { const detail = expectSuccess(decodePullRequestJson(asJson(pullRequest()))); - expect(detail?.threadsUrl).toBe( - "https://dev.azure.com/acme/platform/_apis/git/repositories/web/pullRequests/42/threads", - ); + expect(detail?.location).toEqual({ project: "platform", repository: "web" }); }); - it("reports no conversation url when Azure said too little to build one", () => { - // A web link places the pull request, but without the REST url and repository there is - // nothing to hang a threads collection off. + it("reports no repository location when Azure said too little to name one", () => { + // A web link places the pull request, but with no repository named there is nothing to + // address the routes that read its files and its conversation. const detail = expectSuccess( decodePullRequestJson( asJson( @@ -212,7 +213,7 @@ describe("decodePullRequestJson", () => { ), ); - expect(detail?.threadsUrl).toBeNull(); + expect(detail?.location).toBeNull(); }); it("returns nothing when Azure gave no way to place the pull request at all", () => { @@ -341,3 +342,210 @@ describe("decodeThreadsJson", () => { expect(comments).toEqual([]); }); }); + +describe("decodeIterationsJson", () => { + const iteration = (id: number, head: string, base: string) => ({ + id, + sourceRefCommit: { commitId: head }, + commonRefCommit: { commitId: base }, + targetRefCommit: { commitId: base }, + }); + + it("reads every push in order, oldest first", () => { + const iterations = expectSuccess( + decodeIterationsJson( + asJson({ value: [iteration(2, "bbb", "base"), iteration(1, "aaa", "base")] }), + ), + ); + + expect(iterations.map((entry) => entry.id)).toEqual([1, 2]); + expect(iterations.at(-1)).toEqual({ id: 2, headCommit: "bbb", mergeBaseCommit: "base" }); + }); + + it("skips a push Azure could not place both ends of", () => { + // A patch is taken over a range, and an iteration missing either end names no range at all. + const iterations = expectSuccess( + decodeIterationsJson( + asJson({ + value: [{ id: 1, sourceRefCommit: { commitId: "aaa" } }, iteration(2, "bbb", "base")], + }), + ), + ); + + expect(iterations.map((entry) => entry.id)).toEqual([2]); + }); +}); + +describe("decodeIterationChangesJson", () => { + it("names each changed file without the slash Azure leads its paths with", () => { + const page = expectSuccess( + decodeIterationChangesJson( + asJson({ + changeEntries: [ + { changeType: "add", item: { path: "/DEMO.md", objectId: "ec00" } }, + { + changeType: "edit", + item: { path: "/README.md", objectId: "8f80", originalObjectId: "0ca4" }, + }, + { changeType: "delete", item: { path: "/OLD.md", originalObjectId: "1111" } }, + ], + }), + ), + ); + + expect(page.changes.map((change) => [change.path, change.changeKind])).toEqual([ + ["DEMO.md", "new"], + ["README.md", "change"], + ["OLD.md", "deleted"], + ]); + }); + + it("keeps a space at the end of a file's name, which belongs to the name", () => { + // Git will carry a name that ends in a space, and the patch and the viewed mark are both + // keyed by it. Tidying it here files the change under a name nothing else uses. + const page = expectSuccess( + decodeIterationChangesJson( + asJson({ + changeEntries: [ + { changeType: "edit", item: { path: "/docs/readme.md ", objectId: "ec00" } }, + { + changeType: "rename", + sourceServerItem: "/docs/old.md ", + item: { path: "/docs/moved.md", objectId: "aaaa", originalObjectId: "aaaa" }, + }, + ], + }), + ), + ); + + expect(page.changes.map((change) => [change.path, change.oldPath])).toEqual([ + ["docs/readme.md ", "docs/readme.md "], + ["docs/moved.md", "docs/old.md "], + ]); + }); + + it("reads a rename as one file that moved, and says whether it also changed", () => { + const page = expectSuccess( + decodeIterationChangesJson( + asJson({ + changeEntries: [ + { + changeType: "rename", + sourceServerItem: "/docs/old.md", + item: { path: "/docs/new.md", objectId: "aaaa", originalObjectId: "aaaa" }, + }, + { + changeType: "edit, rename", + sourceServerItem: "/src/old.ts", + item: { path: "/src/new.ts", objectId: "bbbb", originalObjectId: "cccc" }, + }, + ], + }), + ), + ); + + expect(page.changes).toEqual([ + { + path: "docs/new.md", + oldPath: "docs/old.md", + changeKind: "rename-pure", + objectId: "aaaa", + originalObjectId: "aaaa", + }, + { + path: "src/new.ts", + oldPath: "src/old.ts", + changeKind: "rename-changed", + objectId: "bbbb", + originalObjectId: "cccc", + }, + ]); + }); + + it("drops the folders Azure lists alongside the files that changed", () => { + // A review shows files, and a folder has no content on either side to show for one. + const page = expectSuccess( + decodeIterationChangesJson( + asJson({ + changeEntries: [ + { changeType: "add", item: { path: "/docs", isFolder: true, gitObjectType: "tree" } }, + { changeType: "add", item: { path: "/docs/page.md", objectId: "dddd" } }, + ], + }), + ), + ); + + expect(page.changes.map((change) => change.path)).toEqual(["docs/page.md"]); + }); + + it("carries where the next page of a long change starts", () => { + const page = expectSuccess( + decodeIterationChangesJson( + asJson({ + changeEntries: [{ changeType: "add", item: { path: "/DEMO.md", objectId: "ec00" } }], + nextSkip: 2000, + }), + ), + ); + + expect(page.nextSkip).toBe(2000); + }); + + it("reads the last page, which names no page after it, as the end of the change", () => { + const page = expectSuccess( + decodeIterationChangesJson( + asJson({ + changeEntries: [{ changeType: "add", item: { path: "/DEMO.md", objectId: "ec00" } }], + }), + ), + ); + + expect(page.nextSkip).toBeNull(); + }); + + it("reads where a rename came from out of either of the two places Azure names it", () => { + // The iteration-changes route answers with `originalPath`; the commit routes answer with + // `sourceServerItem`, and both are the same fact under two names. + const page = expectSuccess( + decodeIterationChangesJson( + asJson({ + changeEntries: [ + { + changeType: "rename", + originalPath: "/docs/old.md", + item: { path: "/docs/new.md", objectId: "aaaa", originalObjectId: "aaaa" }, + }, + ], + }), + ), + ); + + expect(page.changes.at(0)?.oldPath).toBe("docs/old.md"); + }); +}); + +describe("decodeItemContentJson", () => { + it("reads the file's text out of the envelope Azure wraps it in", () => { + expect( + expectSuccess(decodeItemContentJson(asJson({ path: "/a.md", content: "one\ntwo" }))), + ).toEqual({ contents: "one\ntwo", isBinary: false }); + }); + + it("reads an empty file as empty rather than as a failure to look", () => { + expect(expectSuccess(decodeItemContentJson(asJson({ path: "/a.md" })))).toEqual({ + contents: "", + isBinary: false, + }); + }); + + it("keeps Azure's own word that a file is binary", () => { + // Which it answers base64-encoded, so nothing in the text it sent would give it away. + expect( + expectSuccess( + decodeItemContentJson( + asJson({ path: "/logo.png", content: "b2xk", contentMetadata: { isBinary: true } }), + ), + ), + ).toEqual({ contents: "b2xk", isBinary: true }); + }); +}); diff --git a/apps/server/src/pullRequest/azureDevOpsPullRequestJson.ts b/apps/server/src/pullRequest/azureDevOpsPullRequestJson.ts index 55d9b544ab3d..87c222e94edb 100644 --- a/apps/server/src/pullRequest/azureDevOpsPullRequestJson.ts +++ b/apps/server/src/pullRequest/azureDevOpsPullRequestJson.ts @@ -12,10 +12,7 @@ import type { import { TrimmedNonEmptyString } from "@t3tools/contracts"; import { decodeJsonResult } from "@t3tools/shared/schemaJson"; -import { - azureDevOpsOrganizationBaseFromRestApiUrl, - azureDevOpsPullRequestWebUrl, -} from "../sourceControl/azureDevOpsPullRequests.ts"; +import { azureDevOpsPullRequestWebUrl } from "../sourceControl/azureDevOpsPullRequests.ts"; /** * Azure's enums are decoded as plain strings and normalized here, in the same tolerant style as @@ -117,6 +114,16 @@ const RawViewerSchema = Schema.Struct({ ), }); +/** + * Where a repository lives, in the terms Azure's REST routes address it by. They take the project + * and the repository as separate route parameters rather than as one path, so the pair travels + * together rather than as a URL that would have to be taken apart again to use. + */ +export interface AzureDevOpsRepositoryLocation { + readonly project: string; + readonly repository: string; +} + export interface AzureDevOpsPullRequest { readonly number: number; readonly title: string; @@ -137,8 +144,8 @@ export interface AzureDevOpsPullRequest { readonly body: string; readonly reviewRequestLogins: ReadonlyArray; readonly reviewers: ReadonlyArray; - /** Where this pull request's threads live, when Azure said enough to work it out. */ - readonly threadsUrl: string | null; + /** Where this pull request lives, when Azure said enough to work it out. */ + readonly location: AzureDevOpsRepositoryLocation | null; /** Whether Azure is set to complete this on its own once its policies pass. */ readonly autoMergeEnabled: boolean; /** The completion strategy Azure stored with auto-complete, where it reported one. */ @@ -188,15 +195,16 @@ function toMergeability(value: string | null | undefined): PullRequestMergeabili } /** - * The REST collection a pull request's threads hang from. Built from what Azure returned rather - * than from the local remote, whose shape differs between the modern, legacy and SSH forms. + * Where a pull request's own repository sits. Taken from what Azure returned rather than from the + * local remote, whose shape differs between the modern, legacy and SSH forms. */ -function toThreadsUrl(raw: Schema.Schema.Type): string | null { - const base = azureDevOpsOrganizationBaseFromRestApiUrl(raw.url); +function toLocation( + raw: Schema.Schema.Type, +): AzureDevOpsRepositoryLocation | null { const project = trimmed(raw.repository?.project?.name); const repository = trimmed(raw.repository?.name); - if (base === null || project === null || repository === null) return null; - return `${base}/${encodeURIComponent(project)}/_apis/git/repositories/${encodeURIComponent(repository)}/pullRequests/${raw.pullRequestId}/threads`; + if (project === null || repository === null) return null; + return { project, repository }; } function toAutoMergeMethod( @@ -259,7 +267,7 @@ function toPullRequest( body: raw.description ?? "", reviewRequestLogins: reviewers.map((reviewer) => reviewer.login), reviewers, - threadsUrl: toThreadsUrl(raw), + location: toLocation(raw), autoMergeEnabled: (raw.autoCompleteSetBy ?? null) !== null, ...(autoMergeMethod === undefined ? {} : { autoMergeMethod }), }; @@ -370,3 +378,203 @@ export function decodeThreadsJson( comments.toSorted((left, right) => left.createdAt.localeCompare(right.createdAt)), ); } + +/** + * One push's worth of a pull request. Azure records every push as an iteration and keys the whole + * review off them: the changed files, and the marks a reader leaves on those files, both hang + * from an iteration rather than from the pull request. + */ +const RawIterationSchema = Schema.Struct({ + id: Schema.Int, + sourceRefCommit: Schema.optional( + Schema.NullOr(Schema.Struct({ commitId: Schema.optional(Schema.NullOr(Schema.String)) })), + ), + commonRefCommit: Schema.optional( + Schema.NullOr(Schema.Struct({ commitId: Schema.optional(Schema.NullOr(Schema.String)) })), + ), +}); + +const RawIterationPageSchema = Schema.Struct({ value: Schema.Array(Schema.Unknown) }); + +const RawChangeEntrySchema = Schema.Struct({ + changeType: Schema.optional(Schema.NullOr(Schema.String)), + sourceServerItem: Schema.optional(Schema.NullOr(Schema.String)), + /** Where a renamed file came from. Azure states it here on an iteration's changes. */ + originalPath: Schema.optional(Schema.NullOr(Schema.String)), + item: Schema.optional( + Schema.NullOr( + Schema.Struct({ + path: Schema.optional(Schema.NullOr(Schema.String)), + objectId: Schema.optional(Schema.NullOr(Schema.String)), + originalObjectId: Schema.optional(Schema.NullOr(Schema.String)), + /** Azure marks a directory this way; a review has nothing to show for one. */ + isFolder: Schema.optional(Schema.NullOr(Schema.Boolean)), + gitObjectType: Schema.optional(Schema.NullOr(Schema.String)), + }), + ), + ), +}); + +const RawChangePageSchema = Schema.Struct({ + changeEntries: Schema.Array(Schema.Unknown), + /** Where the page after this one starts. Azure leaves it out on the last page. */ + nextSkip: Schema.optional(Schema.NullOr(Schema.Number)), +}); + +const RawItemContentSchema = Schema.Struct({ + content: Schema.optional(Schema.NullOr(Schema.String)), + /** What Azure makes of the file it is handing over, which is where it says it is not text. */ + contentMetadata: Schema.optional( + Schema.NullOr(Schema.Struct({ isBinary: Schema.optional(Schema.NullOr(Schema.Boolean)) })), + ), +}); + +/** The head and the merge base of one iteration, which is the range its patch is taken over. */ +export interface AzureDevOpsIteration { + readonly id: number; + readonly headCommit: string; + readonly mergeBaseCommit: string; +} + +/** + * What one file did across an iteration. `oldPath` differs from `path` only for a rename, which + * Azure reports by naming the file's previous home rather than as a delete and an add. + */ +export interface AzureDevOpsChangeEntry { + readonly path: string; + readonly oldPath: string; + readonly changeKind: "new" | "deleted" | "change" | "rename-pure" | "rename-changed"; + readonly objectId: string | null; + readonly originalObjectId: string | null; +} + +/** + * One page of what an iteration changed, and where the next one starts. Azure pages this route + * rather than answering with the whole change, so a review large enough to be paged is followed + * to its end instead of being cut off at the first page's worth. + */ +export interface AzureDevOpsChangePage { + readonly changes: ReadonlyArray; + readonly nextSkip: number | null; +} + +/** One file's text at one commit, and whether Azure says the text is text at all. */ +export interface AzureDevOpsItemContent { + readonly contents: string; + readonly isBinary: boolean; +} + +const decodeIterationPage = decodeJsonResult(RawIterationPageSchema); +const decodeIterationEntry = Schema.decodeUnknownExit(RawIterationSchema); +const decodeChangePage = decodeJsonResult(RawChangePageSchema); +const decodeChangeEntry = Schema.decodeUnknownExit(RawChangeEntrySchema); +const decodeItemContent = decodeJsonResult(RawItemContentSchema); + +/** + * Azure leads a path with a slash, which is its own spelling rather than part of the name. Every + * other host, and every patch, names the same file without it. + * + * Not trimmed, unlike everything else read out of this payload: a leading or trailing space is a + * legal part of a file's name, and a path trimmed here no longer matches the one the patch and the + * viewed mark are keyed by, so the file is filed under a name nothing else uses. + */ +function toRepositoryPath(value: string | null | undefined): string | null { + if (value === undefined || value === null) return null; + const path = value.replace(/^\/+/, ""); + return path.length === 0 ? null : path; +} + +/** + * Azure names a change with one word or two, and a rename arrives either alone or alongside the + * edit that came with it. Anything it has added since reads as a plain change, which shows the + * file rather than dropping it from the review. + */ +function toChangeKind( + raw: string | null | undefined, + renamed: boolean, +): AzureDevOpsChangeEntry["changeKind"] { + const parts = new Set( + (raw ?? "") + .toLowerCase() + .split(",") + .map((part) => part.trim()) + .filter((part) => part.length > 0), + ); + if (parts.has("delete")) return "deleted"; + if (renamed || parts.has("rename")) return parts.has("edit") ? "rename-changed" : "rename-pure"; + if (parts.has("add")) return "new"; + return "change"; +} + +export function decodeIterationsJson( + raw: string, +): Result.Result, DecodeFailure> { + const decoded = decodeIterationPage(raw); + if (!Result.isSuccess(decoded)) return Result.fail(decoded.failure); + const iterations: AzureDevOpsIteration[] = []; + for (const entry of decoded.success.value) { + const decodedIteration = decodeIterationEntry(entry); + if (Exit.isFailure(decodedIteration)) continue; + const iteration = decodedIteration.value; + const headCommit = trimmed(iteration.sourceRefCommit?.commitId); + const mergeBaseCommit = trimmed(iteration.commonRefCommit?.commitId); + // An iteration Azure cannot place both ends of names no range, and a patch needs both. + if (headCommit === null || mergeBaseCommit === null) continue; + iterations.push({ id: iteration.id, headCommit, mergeBaseCommit }); + } + return Result.succeed(iterations.toSorted((left, right) => left.id - right.id)); +} + +export function decodeIterationChangesJson( + raw: string, +): Result.Result { + const decoded = decodeChangePage(raw); + if (!Result.isSuccess(decoded)) return Result.fail(decoded.failure); + const changes: AzureDevOpsChangeEntry[] = []; + for (const entry of decoded.success.changeEntries) { + const decodedChange = decodeChangeEntry(entry); + if (Exit.isFailure(decodedChange)) continue; + const change = decodedChange.value; + const path = toRepositoryPath(change.item?.path); + if (path === null) continue; + // Azure lists the folders a change touched alongside the files themselves. A review shows + // files, and a folder has no content to show for either side of one. + if (change.item?.isFolder === true) continue; + if ((change.item?.gitObjectType ?? "blob").toLowerCase() !== "blob") continue; + // Azure names where a renamed file came from in either of two places depending on the route + // and the version, so both are read and the current path stands in when neither is there. + const oldPath = + toRepositoryPath(change.sourceServerItem) ?? toRepositoryPath(change.originalPath) ?? path; + changes.push({ + path, + oldPath, + changeKind: toChangeKind(change.changeType, oldPath !== path), + objectId: trimmed(change.item?.objectId), + originalObjectId: trimmed(change.item?.originalObjectId), + }); + } + const nextSkip = decoded.success.nextSkip ?? null; + return Result.succeed({ + changes, + nextSkip: nextSkip !== null && Number.isSafeInteger(nextSkip) && nextSkip > 0 ? nextSkip : null, + }); +} + +/** + * Azure answers an absent file with an empty body rather than an error, which reads as empty. + * + * Whether the bytes are text is Azure's to say and not this decoder's to guess: a file it calls + * binary is reported as such however innocent its first bytes look, since Azure hands the body + * over in an encoding of its own choosing rather than verbatim. + */ +export function decodeItemContentJson( + raw: string, +): Result.Result { + const decoded = decodeItemContent(raw); + return Result.isSuccess(decoded) + ? Result.succeed({ + contents: decoded.success.content ?? "", + isBinary: decoded.success.contentMetadata?.isBinary === true, + }) + : Result.fail(decoded.failure); +} diff --git a/apps/server/src/pullRequest/bitbucketDiffRevisions.test.ts b/apps/server/src/pullRequest/bitbucketDiffRevisions.test.ts new file mode 100644 index 000000000000..f4401eb7a036 --- /dev/null +++ b/apps/server/src/pullRequest/bitbucketDiffRevisions.test.ts @@ -0,0 +1,216 @@ +import { assert, describe, it } from "@effect/vitest"; + +import { parseDiffFileRevisions } from "./bitbucketDiffRevisions.ts"; + +function patchOf(...lines: ReadonlyArray): string { + return `${lines.join("\n")}\n`; +} + +describe("parseDiffFileRevisions", () => { + it("reads the head id of a changed file off its index line", () => { + const revisions = parseDiffFileRevisions( + patchOf( + "diff --git a/src/a.ts b/src/a.ts", + "index 7f2aa0ab6..b4a2a7c9a 100644", + "--- a/src/a.ts", + "+++ b/src/a.ts", + "@@ -1 +1 @@", + "-a", + "+b", + ), + ); + + assert.deepStrictEqual([...revisions], [["src/a.ts", "b4a2a7c9a"]]); + }); + + it("names a deletion by the path it had, which is the one still on screen", () => { + const revisions = parseDiffFileRevisions( + patchOf( + "diff --git a/gone.ts b/gone.ts", + "deleted file mode 100644", + "index 1111111..0000000", + "--- a/gone.ts", + "+++ /dev/null", + "@@ -1 +0,0 @@", + "-x", + ), + ); + + assert.deepStrictEqual([...revisions], [["gone.ts", "0000000"]]); + }); + + it("names a rename by where it moved to", () => { + const revisions = parseDiffFileRevisions( + patchOf( + "diff --git a/old.ts b/new.ts", + "similarity index 90%", + "rename from old.ts", + "rename to new.ts", + "index 2222222..3333333 100644", + "--- a/old.ts", + "+++ b/new.ts", + "@@ -1 +1 @@", + "-a", + "+b", + ), + ); + + assert.deepStrictEqual([...revisions], [["new.ts", "3333333"]]); + }); + + it("names a rename that changed nothing, which states no paths of its own", () => { + const revisions = parseDiffFileRevisions( + patchOf( + "diff --git a/old.ts b/new.ts", + "similarity index 100%", + "rename from old.ts", + "rename to new.ts", + "index 2222222..2222222 100644", + ), + ); + + assert.deepStrictEqual([...revisions], [["new.ts", "2222222"]]); + }); + + it("leaves out a file Bitbucket excluded, which it gives no index line for", () => { + const revisions = parseDiffFileRevisions( + patchOf( + "diff --git a/package.json b/package.json", + "index 7f2aa0ab6..b4a2a7c9a 100644", + "--- a/package.json", + "+++ b/package.json", + "@@ -1 +1 @@", + '- "x": "1"', + '+ "x": "2"', + "diff --git a/yarn.lock b/yarn.lock", + 'File excluded by pattern "yarn.lock"', + ), + ); + + assert.deepStrictEqual([...revisions], [["package.json", "b4a2a7c9a"]]); + }); + + it("stops reading headers at the first hunk, so content cannot pose as one", () => { + const revisions = parseDiffFileRevisions( + patchOf( + "diff --git a/notes.md b/notes.md", + "index aaaaaaa..bbbbbbb 100644", + "--- a/notes.md", + "+++ b/notes.md", + "@@ -1,2 +1,2 @@", + "--- a/decoy.ts", + "+++ b/decoy.ts", + "+index ccccccc..ddddddd 100644", + ), + ); + + assert.deepStrictEqual([...revisions], [["notes.md", "bbbbbbb"]]); + }); + + it("splits a header whose paths contain the separator, by the sides agreeing", () => { + const revisions = parseDiffFileRevisions( + patchOf( + "diff --git a/one b/two.ts b/one b/two.ts", + "index eeeeeee..fffffff 100644", + "@@ -1 +1 @@", + ), + ); + + assert.deepStrictEqual([...revisions], [["one b/two.ts", "fffffff"]]); + }); + + it("leaves out an added file that Bitbucket sent no index line for", () => { + const revisions = parseDiffFileRevisions( + patchOf("diff --git a/added.ts b/added.ts", "new file mode 100644", "--- /dev/null"), + ); + + assert.strictEqual(revisions.size, 0); + }); + + it("reads nothing out of an empty patch", () => { + assert.strictEqual(parseDiffFileRevisions("").size, 0); + }); + + it("reads a name git had to quote, here one holding a tab", () => { + const revisions = parseDiffFileRevisions( + patchOf( + 'diff --git "a/we\\tird.ts" "b/we\\tird.ts"', + "index 4444444..5555555 100644", + '--- "a/we\\tird.ts"', + '+++ "b/we\\tird.ts"', + "@@ -1 +1 @@", + "-a", + "+b", + ), + ); + + assert.deepStrictEqual([...revisions], [["we\tird.ts", "5555555"]]); + }); + + it("rejoins the octal bytes git writes for a name outside ASCII", () => { + const revisions = parseDiffFileRevisions( + patchOf( + 'diff --git "a/caf\\303\\251/r\\303\\251sum\\303\\251.ts" "b/caf\\303\\251/r\\303\\251sum\\303\\251.ts"', + "index 6666666..7777777 100644", + "@@ -1 +1 @@", + ), + ); + + assert.deepStrictEqual([...revisions], [["café/résumé.ts", "7777777"]]); + }); + + it("splits a rename header where git quoted only the side that needed it", () => { + const revisions = parseDiffFileRevisions( + patchOf( + 'diff --git a/old.ts "b/new\\tname.ts"', + "similarity index 90%", + "rename from old.ts", + 'rename to "new\\tname.ts"', + "index 8888888..9999999 100644", + "--- a/old.ts", + '+++ "b/new\\tname.ts"', + "@@ -1 +1 @@", + "-a", + "+b", + ), + ); + + assert.deepStrictEqual([...revisions], [["new\tname.ts", "9999999"]]); + }); + + it("names a quoted rename that changed nothing, which states no paths of its own", () => { + const revisions = parseDiffFileRevisions( + patchOf( + 'diff --git "a/old\\tname.ts" "b/new\\tname.ts"', + "similarity index 100%", + 'rename from "old\\tname.ts"', + 'rename to "new\\tname.ts"', + "index abcabca..abcabca 100644", + ), + ); + + assert.deepStrictEqual([...revisions], [["new\tname.ts", "abcabca"]]); + }); + + it("keeps a character from outside the basic plane that git left unescaped", () => { + // `core.quotePath` off leaves the name's own bytes in place, and git still quotes the header + // for the tab. Encoding what it left one unit at a time would split the pair into two halves. + const revisions = parseDiffFileRevisions( + patchOf( + 'diff --git "a/we\\tird-\u{1f680}.ts" "b/we\\tird-\u{1f680}.ts"', + "index aaaaaaa..bbbbbbb 100644", + "@@ -1 +1 @@", + ), + ); + + assert.deepStrictEqual([...revisions], [["we\tird-\u{1f680}.ts", "bbbbbbb"]]); + }); + + it("splits an unquoted header whose names hold a space, by the sides agreeing", () => { + const revisions = parseDiffFileRevisions( + patchOf("diff --git a/one two b/one two", "index ddddddd..eeeeeee 100644", "@@ -1 +1 @@"), + ); + + assert.deepStrictEqual([...revisions], [["one two", "eeeeeee"]]); + }); +}); diff --git a/apps/server/src/pullRequest/bitbucketDiffRevisions.ts b/apps/server/src/pullRequest/bitbucketDiffRevisions.ts new file mode 100644 index 000000000000..d02b1d310fee --- /dev/null +++ b/apps/server/src/pullRequest/bitbucketDiffRevisions.ts @@ -0,0 +1,202 @@ +const ENTRY = "diff --git "; +const QUOTE = '"'; + +const NAMED_ESCAPES: Record = { + '"': 0x22, + "\\": 0x5c, + a: 0x07, + b: 0x08, + f: 0x0c, + n: 0x0a, + r: 0x0d, + t: 0x09, + v: 0x0b, +}; + +const utf8 = new TextEncoder(); +const fromUtf8 = new TextDecoder(); + +interface Entry { + oldPath: string | null; + newPath: string | null; + deleted: boolean; + revision: string | null; + /** Past the first hunk header every line is content, and content can start like a header. */ + inBody: boolean; +} + +/** + * The name inside git's quoted form, which git reaches for when a name holds a tab, a newline, a + * quote, a backslash, or, under `core.quotePath`, any byte outside ASCII. + * + * The escapes are per byte, so a name in any other alphabet arrives as a run of octal and only + * reads back as itself once those bytes are rejoined and decoded together. A name git had no + * reason to quote is already the name. + */ +function unquotePath(token: string): string { + if (token.length < 2 || !token.startsWith(QUOTE) || !token.endsWith(QUOTE)) return token; + const body = token.slice(1, -1); + const bytes: Array = []; + // Anything git left as itself is encoded a run at a time rather than a unit at a time, so a + // character written outside the basic plane keeps its pair together and comes back as itself + // instead of as two halves neither of which is a character. + let literal = ""; + const flush = () => { + if (literal.length === 0) return; + bytes.push(...utf8.encode(literal)); + literal = ""; + }; + let at = 0; + while (at < body.length) { + const char = body.charAt(at); + if (char !== "\\") { + literal += char; + at += 1; + continue; + } + const escaped = body.charAt(at + 1); + if (escaped === "") { + flush(); + bytes.push(0x5c); + break; + } + const named = NAMED_ESCAPES[escaped]; + if (named !== undefined) { + flush(); + bytes.push(named); + at += 2; + continue; + } + const octal = body.slice(at + 1, at + 4); + if (/^[0-7]{3}$/.test(octal)) { + flush(); + bytes.push(Number.parseInt(octal, 8)); + at += 4; + continue; + } + literal += escaped; + at += 2; + } + flush(); + return fromUtf8.decode(new Uint8Array(bytes)); +} + +/** Where a quoted name closes, given git escapes every quote the name itself holds. */ +function quotedEnd(rest: string): number { + for (let at = 1; at < rest.length; at += 1) { + const char = rest.charAt(at); + if (char === "\\") { + at += 1; + continue; + } + if (char === QUOTE) return at; + } + return -1; +} + +/** `a/x` and `b/x` on a `---` or `+++` line; `/dev/null` is the side that has no file. */ +function sidePath(rest: string, prefix: string): string | null { + if (rest === "/dev/null") return null; + const path = unquotePath(rest); + return path.startsWith(prefix) ? path.slice(prefix.length) : path; +} + +function headerSide(token: string, prefix: string): string | null { + const path = unquotePath(token); + return path.startsWith(prefix) ? path.slice(prefix.length) : null; +} + +/** + * The two names on a `diff --git` line, which git writes with no delimiter between them. + * + * `a/one two b/one two` splits in more than one place, so the split that leaves both sides equal + * wins. A rename is the only entry whose sides differ, and a rename states its names on lines of + * its own. Anything still ambiguous is left unnamed rather than guessed at. + * + * A quoted name ends at its own closing quote, so a header carrying one splits there and needs + * none of that guessing. Git quotes only the side that needs it, so one side can be quoted alone. + */ +function headerPaths(rest: string): readonly [string | null, string | null] { + if (rest.startsWith(QUOTE)) { + const end = quotedEnd(rest); + if (end === -1 || rest.charAt(end + 1) !== " ") return [null, null]; + return [headerSide(rest.slice(0, end + 1), "a/"), headerSide(rest.slice(end + 2), "b/")]; + } + if (rest.endsWith(QUOTE)) { + const opens = rest.indexOf(QUOTE); + if (opens < 1 || rest.charAt(opens - 1) !== " ") return [null, null]; + return [headerSide(rest.slice(0, opens - 1), "a/"), headerSide(rest.slice(opens), "b/")]; + } + if (!rest.startsWith("a/")) return [null, null]; + const splits: Array = []; + for (let at = rest.indexOf(" b/"); at !== -1; at = rest.indexOf(" b/", at + 1)) splits.push(at); + const chosen = + splits.find((at) => rest.slice(2, at) === rest.slice(at + 3)) ?? + (splits.length === 1 ? splits[0] : undefined); + return chosen === undefined ? [null, null] : [rest.slice(2, chosen), rest.slice(chosen + 3)]; +} + +/** The right-hand id of `index .. `. */ +function headRevision(rest: string): string | null { + const gap = rest.indexOf(".."); + if (gap === -1) return null; + const after = rest.slice(gap + 2); + const end = after.indexOf(" "); + const head = end === -1 ? after : after.slice(0, end); + return head.length === 0 ? null : head; +} + +/** + * What the head has of each file in a unified patch, as the blob ids git writes into it. + * + * Bitbucket states a file's version nowhere else: its diffstat entries carry a commit and a path + * and no blob id, and no endpoint answers what a file is now. Git's own `index ..` + * line is in the patch the diff already reads, so the versions cost no call of their own. + * + * Keyed the way the client names files: the head's name for it, except for a deletion, where the + * head has no name and the one it had is what is on screen. An entry the patch gives no `index` + * line for, one Bitbucket excluded by pattern most often, is left out. Left out reads the same + * way when a file is ticked and when the tick is read back, so the mark still holds. + */ +export function parseDiffFileRevisions(patch: string): ReadonlyMap { + const revisions = new Map(); + let entry: Entry | null = null; + + const close = () => { + if (entry === null) return; + const path = entry.deleted ? entry.oldPath : (entry.newPath ?? entry.oldPath); + if (path !== null && path.length > 0 && entry.revision !== null) { + revisions.set(path, entry.revision); + } + entry = null; + }; + + for (const line of patch.split("\n")) { + if (line.startsWith(ENTRY)) { + close(); + const [oldPath, newPath] = headerPaths(line.slice(ENTRY.length)); + entry = { oldPath, newPath, deleted: false, revision: null, inBody: false }; + continue; + } + if (entry === null || entry.inBody) continue; + if (line.startsWith("@@")) { + entry.inBody = true; + } else if (line.startsWith("index ")) { + entry.revision = headRevision(line.slice("index ".length)); + } else if (line.startsWith("deleted file mode")) { + entry.deleted = true; + } else if (line.startsWith("rename from ")) { + entry.oldPath = unquotePath(line.slice("rename from ".length)); + } else if (line.startsWith("rename to ")) { + entry.newPath = unquotePath(line.slice("rename to ".length)); + } else if (line.startsWith("--- ")) { + entry.oldPath = sidePath(line.slice(4), "a/"); + } else if (line.startsWith("+++ ")) { + const side = sidePath(line.slice(4), "b/"); + entry.newPath = side; + if (side === null) entry.deleted = true; + } + } + close(); + return revisions; +} diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts index f6f5957f5875..e1044e1f1b71 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts @@ -4,10 +4,12 @@ import { describe, expect, it } from "vite-plus/test"; import { buildReviewSubmissionJson, buildReviewerRequestJson, + buildSetFilesViewedGraphQlMutation, decodeBaseComparisonJson, decodePullRequestActivityJson, decodePullRequestDetailJson, decodePullRequestFilesJson, + decodePullRequestFilesViewedJson, decodePullRequestListJson, decodePullRequestNodeIdJson, decodePullRequestSearchJson, @@ -1500,3 +1502,97 @@ describe("how far a branch trails its base", () => { expect(Result.isSuccess(decodeBaseComparisonJson("{"))).toBe(false); }); }); + +describe("decodePullRequestFilesViewedJson", () => { + const page = ( + nodes: ReadonlyArray, + pageInfo: { hasNextPage: boolean; endCursor: string | null }, + ) => + JSON.stringify({ + data: { repository: { pullRequest: { files: { pageInfo, nodes } } } }, + }); + + it("reads each file's state and where the next page carries on", () => { + const decoded = decodePullRequestFilesViewedJson( + page( + [ + { path: "src/a.ts", viewerViewedState: "VIEWED" }, + { path: "src/b.ts", viewerViewedState: "UNVIEWED" }, + { path: "src/c.ts", viewerViewedState: "DISMISSED" }, + ], + { hasNextPage: true, endCursor: "cursor-2" }, + ), + ); + expect(Result.isSuccess(decoded)).toBe(true); + if (!Result.isSuccess(decoded)) return; + expect(decoded.success).toEqual({ + files: [ + { path: "src/a.ts", state: "viewed" }, + { path: "src/b.ts", state: "unviewed" }, + { path: "src/c.ts", state: "dismissed" }, + ], + nextCursor: "cursor-2", + }); + }); + + it("treats a state it has never heard of as unread rather than failing the page", () => { + const decoded = decodePullRequestFilesViewedJson( + page([{ path: "src/a.ts", viewerViewedState: "SOMETHING_NEW" }], { + hasNextPage: false, + endCursor: null, + }), + ); + expect(Result.isSuccess(decoded)).toBe(true); + if (!Result.isSuccess(decoded)) return; + expect(decoded.success).toEqual({ + files: [{ path: "src/a.ts", state: "unviewed" }], + nextCursor: null, + }); + }); + + it("answers empty for a pull request the host has nothing to say about", () => { + const decoded = decodePullRequestFilesViewedJson( + JSON.stringify({ data: { repository: { pullRequest: null } } }), + ); + expect(Result.isSuccess(decoded)).toBe(true); + if (!Result.isSuccess(decoded)) return; + expect(decoded.success).toEqual({ files: [], nextCursor: null }); + }); +}); + +describe("buildSetFilesViewedGraphQlMutation", () => { + it("asks for nothing when nothing was pressed", () => { + expect(buildSetFilesViewedGraphQlMutation([])).toBeNull(); + }); + + it("clears and restores in one document, each file under its own alias", () => { + const mutation = buildSetFilesViewedGraphQlMutation([ + { path: "src/a.ts", viewed: true }, + { path: "src/b.ts", viewed: false }, + ]); + expect(mutation).not.toBeNull(); + if (mutation === null) return; + expect(mutation.query).toContain( + "mutation($pullRequestId: ID!, $path0: String!, $path1: String!)", + ); + expect(mutation.query).toContain( + "f0: markFileAsViewed(input: { pullRequestId: $pullRequestId, path: $path0 })", + ); + expect(mutation.query).toContain( + "f1: unmarkFileAsViewed(input: { pullRequestId: $pullRequestId, path: $path1 })", + ); + expect(mutation.variables).toEqual({ path0: "src/a.ts", path1: "src/b.ts" }); + }); + + it("keeps a path out of the document, so one cannot be read as part of it", () => { + const mutation = buildSetFilesViewedGraphQlMutation([ + { path: '") { __typename } evil: markFileAsViewed(input: { path: "x', viewed: true }, + ]); + expect(mutation).not.toBeNull(); + if (mutation === null) return; + expect(mutation.query).not.toContain("evil"); + expect(mutation.variables.path0).toBe( + '") { __typename } evil: markFileAsViewed(input: { path: "x', + ); + }); +}); diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.ts index 7887a81617d6..6b36ff4958e0 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.ts @@ -9,6 +9,7 @@ import type { PullRequestChecksState, PullRequestComment, PullRequestCommit, + PullRequestFileViewedState, PullRequestLabel, PullRequestMergeCapabilities, PullRequestMergeMethod, @@ -2445,3 +2446,122 @@ export function decodePullRequestFilesJson( omittedFileStats, }); } + +/** + * Which files of a pull request the signed-in account has cleared. + * + * GraphQL only, since the REST files endpoint the patch is read from carries no viewed state at + * all, so this is a second read rather than a wider version of the first. One page of a hundred + * files costs a single point of the hourly budget, which is why it can ride the diff's own + * refresh without being noticed. + */ +export const PULL_REQUEST_FILES_VIEWED_GRAPHQL_QUERY = `query($owner: String!, $name: String!, $number: Int!, $after: String) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + files(first: 100, after: $after) { + pageInfo { hasNextPage endCursor } + nodes { path viewerViewedState } + } + } + } +}`; + +const RawPullRequestFilesViewedSchema = Schema.Struct({ + data: Schema.Struct({ + repository: Schema.NullOr( + Schema.Struct({ + pullRequest: Schema.NullOr( + Schema.Struct({ + files: Schema.Struct({ + pageInfo: Schema.Struct({ + hasNextPage: Schema.Boolean, + endCursor: Schema.NullOr(Schema.String), + }), + nodes: Schema.NullOr( + Schema.Array( + Schema.NullOr( + Schema.Struct({ + path: Schema.String, + // Decoded as a plain string and narrowed below: a GitHub release that adds + // a fourth state must not fail the whole page. + viewerViewedState: Schema.String, + }), + ), + ), + ), + }), + }), + ), + }), + ), + }), +}); + +const decodePullRequestFilesViewed = decodeJsonResult(RawPullRequestFilesViewedSchema); + +export interface GitHubPullRequestFilesViewedPage { + readonly files: ReadonlyArray<{ + readonly path: string; + readonly state: PullRequestFileViewedState; + }>; + /** Where the next page carries on, or null once the host has no more to give. */ + readonly nextCursor: string | null; +} + +/** Anything this host does not name is treated as unread, which is the state that asks for least. */ +function toFileViewedState(raw: string): PullRequestFileViewedState { + switch (raw.trim().toUpperCase()) { + case "VIEWED": + return "viewed"; + case "DISMISSED": + return "dismissed"; + default: + return "unviewed"; + } +} + +export function decodePullRequestFilesViewedJson( + raw: string, +): Result.Result { + const decoded = decodePullRequestFilesViewed(raw); + if (!Result.isSuccess(decoded)) return Result.fail(decoded.failure); + const files = decoded.success.data.repository?.pullRequest?.files; + if (files === undefined) return Result.succeed({ files: [], nextCursor: null }); + return Result.succeed({ + files: (files.nodes ?? []).flatMap((node) => + node === null || node.path.length === 0 + ? [] + : [{ path: node.path, state: toFileViewedState(node.viewerViewedState) }], + ), + nextCursor: files.pageInfo.hasNextPage ? files.pageInfo.endCursor : null, + }); +} + +/** + * One document that clears and restores as many files as the reader ticked, rather than one + * request each. + * + * GitHub has no bulk form of either mutation, and `markFileAsViewed` and `unmarkFileAsViewed` + * take a single path, so the batching is done with aliases. Top-level mutation fields run in the order + * they are written, so the last word about a path is the one that sticks, and the whole burst + * costs one HTTP round trip and one subprocess instead of one of each per press. + * + * Paths travel as variables rather than inside the document: they are the host's own strings, but + * a path is data and a document is not, and building one out of the other is how injection starts. + */ +export function buildSetFilesViewedGraphQlMutation( + files: ReadonlyArray<{ readonly path: string; readonly viewed: boolean }>, +): { readonly query: string; readonly variables: Readonly> } | null { + if (files.length === 0) return null; + const parameters = files.map((_, index) => `$path${index}: String!`).join(", "); + const fields = files + .map( + (file, index) => + ` f${index}: ${file.viewed ? "markFileAsViewed" : "unmarkFileAsViewed"}(input: { pullRequestId: $pullRequestId, path: $path${index} }) { clientMutationId }`, + ) + .join("\n"); + return { + query: `mutation($pullRequestId: ID!, ${parameters}) {\n${fields}\n}`, + variables: Object.fromEntries(files.map((file, index) => [`path${index}`, file.path])), + }; +} diff --git a/apps/server/src/pullRequest/gitLabMergeRequestJson.test.ts b/apps/server/src/pullRequest/gitLabMergeRequestJson.test.ts index 4438aa0c521a..5bb5264e7306 100644 --- a/apps/server/src/pullRequest/gitLabMergeRequestJson.test.ts +++ b/apps/server/src/pullRequest/gitLabMergeRequestJson.test.ts @@ -3,6 +3,7 @@ import { describe, expect, it } from "vite-plus/test"; import { decodeAwardEmojiJson, + decodeRepositoryBlobsJson, decodeCommitsJson, decodeMergeRequestDetailJson, decodeMergeRequestDiffsJson, @@ -630,3 +631,122 @@ describe("gitLabAwardName", () => { expect(gitLabAwardName("hooray")).toBe("tada"); }); }); + +describe("decodeRepositoryBlobsJson", () => { + /** Null is the query going unanswered, which these cases are not about. */ + function expectBlobs(result: Result.Result | null, unknown>) { + const blobs = expectSuccess(result); + expect(blobs).not.toBe(null); + if (blobs === null) throw new Error("expected an answered blobs query"); + return blobs; + } + + it("reads a blob id per path", () => { + const blobs = expectBlobs( + decodeRepositoryBlobsJson( + JSON.stringify({ + data: { + project: { + repository: { + blobs: { + nodes: [ + { path: "src/a.ts", oid: "aaa111" }, + { path: "src/b.ts", oid: "bbb222" }, + ], + }, + }, + }, + }, + }), + ), + ); + + expect([...blobs]).toEqual([ + ["src/a.ts", "aaa111"], + ["src/b.ts", "bbb222"], + ]); + }); + + it("leaves out a node missing either half, which names no version", () => { + const blobs = expectBlobs( + decodeRepositoryBlobsJson( + JSON.stringify({ + data: { + project: { + repository: { + blobs: { + nodes: [ + { path: "src/a.ts", oid: null }, + { path: null, oid: "bbb222" }, + null, + { path: "src/c.ts", oid: "ccc333" }, + ], + }, + }, + }, + }, + }), + ), + ); + + expect([...blobs]).toEqual([["src/c.ts", "ccc333"]]); + }); + + it("keys a blob by the path the host spelled, spaces and all", () => { + // A leading or trailing space is a legal part of a file's name. Trimmed here, the id lands + // under a key the asked-for path is not spelled with, and the caller fills that path in as + // the empty revision: a mark on the file then never compares against the real head blob. + const blobs = expectBlobs( + decodeRepositoryBlobsJson( + JSON.stringify({ + data: { + project: { + repository: { + blobs: { + nodes: [ + { path: " leading.ts", oid: "aaa111" }, + { path: "trailing.ts ", oid: "bbb222" }, + { path: " ", oid: "ccc333" }, + { path: "", oid: "ddd444" }, + ], + }, + }, + }, + }, + }), + ), + ); + + expect([...blobs]).toEqual([ + [" leading.ts", "aaa111"], + ["trailing.ts ", "bbb222"], + // A name that is only spaces is one Git carries too, so it is a path like any other. + [" ", "ccc333"], + ]); + }); + + it("tells a project the reader cannot see from a revision with none of the files", () => { + // Null is the query going unanswered. Read as an empty answer it would say the head has none + // of the asked-for files, which reports every file a reader has cleared as changed. + expect( + expectSuccess(decodeRepositoryBlobsJson(JSON.stringify({ data: { project: null } }))), + ).toBe(null); + expect( + expectSuccess( + decodeRepositoryBlobsJson(JSON.stringify({ data: { project: { repository: null } } })), + ), + ).toBe(null); + expect( + expectSuccess( + decodeRepositoryBlobsJson( + JSON.stringify({ data: { project: { repository: { blobs: { nodes: [] } } } } }), + ), + ), + ).toEqual(new Map()); + }); + + it("fails on output that is not the query's shape", () => { + expect(Result.isSuccess(decodeRepositoryBlobsJson("not json"))).toBe(false); + expect(Result.isSuccess(decodeRepositoryBlobsJson(JSON.stringify({ errors: [] })))).toBe(false); + }); +}); diff --git a/apps/server/src/pullRequest/gitLabMergeRequestJson.ts b/apps/server/src/pullRequest/gitLabMergeRequestJson.ts index 4b3c797136f8..4c35d4539ca6 100644 --- a/apps/server/src/pullRequest/gitLabMergeRequestJson.ts +++ b/apps/server/src/pullRequest/gitLabMergeRequestJson.ts @@ -947,3 +947,90 @@ export function decodeOwnAwardIdJson( } return Result.succeed(null); } + +/** + * What the given paths are at one revision, as blob ids. + * + * Asked for by path rather than by walking the tree: the caller already knows which files it + * cares about, and GitLab charges this query by how many paths it is given. A path the revision + * does not have comes back missing rather than as an error, which is the answer for a file the + * merge request deletes. + */ +export const REPOSITORY_BLOBS_GRAPHQL_QUERY = `query($fullPath: ID!, $ref: String!, $paths: [String!]!) { + project(fullPath: $fullPath) { + repository { + blobs(ref: $ref, paths: $paths) { + nodes { path oid } + } + } + } +}`; + +const RawRepositoryBlobsSchema = Schema.Struct({ + data: Schema.Struct({ + project: Schema.NullOr( + Schema.Struct({ + repository: Schema.optional( + Schema.NullOr( + Schema.Struct({ + blobs: Schema.optional( + Schema.NullOr( + Schema.Struct({ + nodes: Schema.optional( + Schema.NullOr( + Schema.Array( + Schema.NullOr( + Schema.Struct({ + path: Schema.optional(Schema.NullOr(Schema.String)), + oid: Schema.optional(Schema.NullOr(Schema.String)), + }), + ), + ), + ), + ), + }), + ), + ), + }), + ), + ), + }), + ), + }), +}); + +const decodeRepositoryBlobs = decodeJsonResult(RawRepositoryBlobsSchema); + +/** + * Blob ids by path, or null where GitLab did not answer the query at all. + * + * A project the token cannot see comes back as `project: null`, and a repository can come back + * without a blobs connection, neither of which says anything about the paths that were asked for. + * That is worth telling apart from a connection that answered: read as "the revision has none of + * these files", an unanswered query reports every file a reader has cleared as changed. + * + * Within an answer, a node missing either half is left out, because half of one names no version, + * and the caller reads an absent path as one the revision does not carry. + */ +export function decodeRepositoryBlobsJson( + raw: string, +): Result.Result | null, DecodeFailure> { + const decoded = decodeRepositoryBlobs(raw); + if (!Result.isSuccess(decoded)) { + return Result.fail(decoded.failure); + } + const nodes = decoded.success.data.project?.repository?.blobs?.nodes; + if (nodes === undefined || nodes === null) return Result.succeed(null); + const blobs = new Map(); + for (const node of nodes) { + // Not trimmed, unlike everything else read out of this payload: a leading or trailing space + // is a legal part of a file's name, so a path trimmed here is filed under a key neither the + // asked-for path nor the viewed mark is spelled with, and the caller reads the file it was + // asked about as one this revision does not carry. + const path = node?.path; + const oid = trimmed(node?.oid); + if (path === undefined || path === null || path.length === 0 || oid === null) continue; + blobs.set(path, oid); + } + return Result.succeed(blobs); +} diff --git a/apps/server/src/server.test.ts b/apps/server/src/server.test.ts index d972e2e00803..3187552ea050 100644 --- a/apps/server/src/server.test.ts +++ b/apps/server/src/server.test.ts @@ -718,7 +718,11 @@ const buildAppUnderTest = (options?: { ); const servedRoutesLayer = HttpRouter.serve( - makeRoutesLayer.pipe(Layer.provide(serviceLauncherClientLayer)), + // Viewed-file marks for a host that keeps none of its own are rows, so the routes want a + // database. Its own, in memory: nothing here shares a table with the auth store. + makeRoutesLayer.pipe( + Layer.provide(Layer.mergeAll(serviceLauncherClientLayer, SqlitePersistenceMemory)), + ), { disableListenLog: true, disableLogger: true, diff --git a/apps/server/src/server.ts b/apps/server/src/server.ts index 3a93adc6d761..c0bb936e4001 100644 --- a/apps/server/src/server.ts +++ b/apps/server/src/server.ts @@ -28,6 +28,7 @@ import { pullRequestHttpApiLayer } from "./pullRequest/http.ts"; import * as PullRequestProviderRegistry from "./pullRequest/PullRequestProviderRegistry.ts"; import * as PullRequestService from "./pullRequest/PullRequestService.ts"; import { layerConfig as SqlitePersistenceLayerLive } from "./persistence/Layers/Sqlite.ts"; +import * as PullRequestFilesViewed from "./persistence/PullRequestFilesViewed.ts"; import * as ServerLifecycleEvents from "./serverLifecycleEvents.ts"; import * as AnalyticsService from "./telemetry/AnalyticsService.ts"; import { ProviderSessionDirectoryLive } from "./provider/Layers/ProviderSessionDirectory.ts"; @@ -307,6 +308,8 @@ const SourceControlProviderRegistryLayerLive = SourceControlProviderRegistry.lay const PullRequestServiceLive = PullRequestService.layer.pipe( Layer.provide(PullRequestProviderRegistry.layer), + // Where the viewed-file marks live for a host that keeps none of its own. + Layer.provide(PullRequestFilesViewed.layer), Layer.provide(SourceControlProviderRegistryLayerLive), Layer.provide(SourceControlRateLimit.layer), ); diff --git a/apps/server/src/ws.ts b/apps/server/src/ws.ts index 839937cf2ea7..4623cce043a9 100644 --- a/apps/server/src/ws.ts +++ b/apps/server/src/ws.ts @@ -2006,6 +2006,16 @@ const makeWsRpcLayer = ( pullRequests.diffFileContents(input), { "rpc.aggregate": "pull-requests" }, ), + [WS_METHODS.pullRequestsFilesViewed]: (input) => + observeRpcEffect(WS_METHODS.pullRequestsFilesViewed, pullRequests.filesViewed(input), { + "rpc.aggregate": "pull-requests", + }), + [WS_METHODS.pullRequestsSetFilesViewed]: (input) => + observeRpcEffect( + WS_METHODS.pullRequestsSetFilesViewed, + pullRequests.setFilesViewed(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/ChatMarkdown.tsx b/apps/web/src/components/ChatMarkdown.tsx index 75127ea124e8..72ec750af12d 100644 --- a/apps/web/src/components/ChatMarkdown.tsx +++ b/apps/web/src/components/ChatMarkdown.tsx @@ -28,6 +28,7 @@ import type { ServerProviderSkill, ThreadLinkedPullRequest, } from "@t3tools/contracts"; +import { pullRequestRepositoryOf } from "@t3tools/contracts"; import { isAtomCommandInterrupted, squashAtomCommandFailure, @@ -2143,7 +2144,7 @@ function ChatMarkdown({ if (project === undefined) return null; return { projectId: project.id, - repository: project.repositoryIdentity?.displayName ?? parsed.repository, + repository: pullRequestRepositoryOf(project.repositoryIdentity) ?? parsed.repository, number: parsed.number, url: href, }; diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index 0d33aec7bc20..5ad6076f5049 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -21,6 +21,7 @@ import { PROVIDER_SEND_TURN_MAX_ATTACHMENTS, ProviderInteractionMode, ProviderDriverKind, + pullRequestRepositoryOf, resolveEnvironmentMachineKind, RuntimeMode, TerminalOpenInput, @@ -3673,7 +3674,7 @@ function ChatViewContent(props: ChatViewProps) { const persistedLinkedThreadPullRequest = isServerThread ? (activeThreadShell?.linkedPullRequest ?? activeThread?.linkedPullRequest ?? null) : (activeThread?.linkedPullRequest ?? null); - const activeProjectRepository = activeProject?.repositoryIdentity?.displayName ?? null; + const activeProjectRepository = pullRequestRepositoryOf(activeProject?.repositoryIdentity); const persistedLinkedThreadPullRequestStatus = useLinkedThreadPullRequest( activeThreadRef?.environmentId ?? null, persistedLinkedThreadPullRequest, diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index cc9bf0f61887..65e78d1e44ae 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -17,6 +17,7 @@ import { ChevronsUpDownIcon, Columns2Icon, FolderTreeIcon, + InfoIcon, MessageSquareIcon, MessageSquareOffIcon, Rows3Icon, @@ -45,6 +46,7 @@ import { resolveFileDiffPreviousPath, type RenderablePatch, } from "~/lib/diffRendering"; +import { APP_BASE_NAME } from "~/branding"; import { PREFERRED_HIGHLIGHTER } from "~/lib/syntaxHighlighting"; import { cn } from "~/lib/utils"; import { createPullRequestDiffFileContentsLoader } from "~/lib/diffFileContents"; @@ -64,6 +66,7 @@ import { DiffFileTree } from "../diffs/DiffFileTree"; import { diffFileTreeEntries } from "../diffs/diffFileTree.logic"; import { StyledDiffCodeView } from "../diffs/StyledDiffCodeView"; import { Button } from "../ui/button"; +import { Checkbox } from "../ui/checkbox"; import { Collapsible, CollapsiblePanel, CollapsibleTrigger } from "../ui/collapsible"; import { DropdownMenu, @@ -79,9 +82,11 @@ import { PullRequestReviewBar } from "./PullRequestReviewBar"; import { isFileDiffCollapsed, isLineInFileDiff, + toggleFileDiffFoldForViewed, type DiffFoldOverride, } from "./pullRequestDiff.logic"; import { PullRequestDiffStat, PullRequestMetaLine } from "./pullRequestPresentation"; +import { usePullRequestFilesViewed } from "./usePullRequestFilesViewed"; import { nextPendingReviewCommentId, pullRequestReviewKey, @@ -332,13 +337,6 @@ export function PullRequestCodeTab({ input: { ...reference, ...(commit === null ? {} : { commit }) }, }), ); - const appliedRefreshToken = useRef(refreshToken); - useEffect(() => { - if (appliedRefreshToken.current === refreshToken) return; - appliedRefreshToken.current = refreshToken; - setSliceState({ key: scopeKey, cursor: null, slices: NO_SLICES }); - refreshFirstDiffPage(); - }, [refreshToken, scopeKey, refreshFirstDiffPage]); const reviewKey = referenceKey; const pendingComments = usePendingReviewComments(reference); const addComment = usePullRequestReviewStore((store) => store.addComment); @@ -411,6 +409,29 @@ export function PullRequestCodeTab({ ), [parsedSlices], ); + const filePaths = useMemo(() => files.map((file) => resolveFileDiffPath(file)), [files]); + // Offered under a commit scope as well as from the whole change, because reading a change one + // commit at a time is what the scope is for. The tick is kept against the change request rather + // than the scope it was made in, so clearing a file here clears it everywhere. + const viewedFilesStore = detail.capabilities.viewedFiles; + const filesViewed = usePullRequestFilesViewed({ + environmentId, + reference, + enabled: viewedFilesStore !== undefined, + paths: filePaths, + }); + const { setViewed, refresh: refreshFilesViewed } = filesViewed; + // The button goes around the host's cache, so everything the tab reads from it starts over: + // the diff from its first page, and with it the ticks, which a push since the last read can + // have marked as standing against an older version of the file. + const appliedRefreshToken = useRef(refreshToken); + useEffect(() => { + if (appliedRefreshToken.current === refreshToken) return; + appliedRefreshToken.current = refreshToken; + setSliceState({ key: scopeKey, cursor: null, slices: NO_SLICES }); + refreshFirstDiffPage(); + refreshFilesViewed(); + }, [refreshToken, scopeKey, refreshFirstDiffPage, refreshFilesViewed]); const nextCursor = loadedSlices.at(-1)?.nextCursor ?? null; // What a slice withheld: the host declining to inline part of it, or a patch the viewer could // not structure and so dropped. Neither says anything about there being more to fetch. @@ -486,6 +507,12 @@ export function PullRequestCodeTab({ } const collapsed = isFileDiffCollapsed(fileKey, foldOverride, toggledFiles); + // The header carries the reader's own tick, and the viewer redraws a file only when its + // version moves. Ticking a file that is already folded changes no fold, so without this + // the box on screen would keep saying the opposite of what the count says. + const viewedMark = filesViewed.enabled + ? `e${filesViewed.isViewed(path) ? "v" : ""}${filesViewed.isStale(path) ? "s" : ""}` + : ""; const annotations: ReviewAnnotation[] = [...groups.values()].map((group) => ({ side: toViewerSide(group.side), @@ -501,7 +528,7 @@ export function PullRequestCodeTab({ // The viewer re-renders an item only when its version changes, so everything the // annotations show has to be part of it. version: fnv1a32( - `${collapsed ? "1" : "0"}:${annotations + `${collapsed ? "1" : "0"}:${viewedMark}:${annotations .map( ({ side, lineNumber, metadata }) => `${side}:${lineNumber}:${metadata.draft ? "d" : ""}:${metadata.pending @@ -535,6 +562,7 @@ export function PullRequestCodeTab({ detail.reviewThreads, draft, files, + filesViewed, foldOverride, pendingComments, placedThreadIds, @@ -603,6 +631,19 @@ export function PullRequestCodeTab({ [], ); + // The tick and the fold are one gesture: clearing a file puts it away, un-clearing brings it + // back. Folding is still held as the reader's difference from the toolbar's default rather + // than derived from what has been ticked, so folding everything ticks nothing off. + const setFileViewed = useCallback( + (fileKey: string, path: string, viewed: boolean) => { + setViewed(path, viewed); + setToggledFiles((current) => + toggleFileDiffFoldForViewed(fileKey, viewed, foldOverride, current), + ); + }, + [foldOverride, setViewed], + ); + // Held as state so the scroll runs after a folded file has been drawn open; scrolling in the // same tick would land on the folded header's position. const [treeReveal, setTreeReveal] = useState<{ fileKey: string; id: number } | null>(null); @@ -746,6 +787,15 @@ export function PullRequestCodeTab({ [toggleFile], ); + // Read through refs rather than closed over. The viewer memoizes each visible file's header + // portal on the callback below, so a fresh identity on every tick, and on every refresh of the + // host's answer, would rebuild every header on screen. Each item's version carries the same + // marks, which is what redraws the one file whose tick moved. + const filesViewedRef = useRef(filesViewed); + filesViewedRef.current = filesViewed; + const setFileViewedRef = useRef(setFileViewed); + setFileViewedRef.current = setFileViewed; + const renderHeaderMetadata = useCallback( (item: CodeViewItem) => { if (item.type !== "diff") return null; @@ -755,17 +805,55 @@ export function PullRequestCodeTab({ additions += hunk.additionLines; deletions += hunk.deletionLines; } + const path = resolveFileDiffPath(item.fileDiff); if (additions === 0 && deletions === 0) { - const withheld = omittedFileStats.get(resolveFileDiffPath(item.fileDiff)); + const withheld = omittedFileStats.get(path); if (withheld) ({ additions, deletions } = withheld); } - return ( + const stat = ( ); + const viewedFiles = filesViewedRef.current; + if (!viewedFiles.enabled) return stat; + const viewed = viewedFiles.isViewed(path); + const stale = viewedFiles.isStale(path); + return ( + + {stat} + {/* The header itself folds the file, so the tick has to keep its press to itself. The + attribute is what the header's capture listener looks for: pressing the word next to + the box is pressing the box, and the fold that follows is the tick's to make. */} + + + ); }, [omittedFileStats], ); @@ -1084,6 +1172,49 @@ export function PullRequestCodeTab({ {files.length} {files.length === 1 ? "file" : "files"} {nextCursor === null ? "" : "+"} + {filesViewed.enabled && files.length > 0 ? ( + + {/* Named on a host that keeps no record of its own, so the reader is told whose + ticks these are without having to find the icon beside them. The count holds its + width and the wording gives way, so this segment cannot push the controls on the + right off the strip in the narrow right panel. */} + + {filesViewed.viewedCount} / {files.length} + + + {viewedFilesStore === "environment" ? `viewed in ${APP_BASE_NAME}` : "viewed"} + + {viewedFilesStore === "environment" ? ( + + }> + + + + This host keeps no shared record of which files you have read, so these ticks + are kept by this environment. They follow you between the apps connected to it, + but the host's own web UI will not show them. + + + ) : null} + {filesViewed.truncated ? ( + + }> + + + + This change has more files than the host will report ticks for in one read, so + the count is short and some boxes below start empty. + + + ) : null} + + ) : null} {withheldContent ? ( }> @@ -1374,6 +1505,10 @@ export function PullRequestCodeTab({ if (node instanceof HTMLButtonElement || node instanceof HTMLAnchorElement) { return; } + // A label answers for the control it names, and this listener runs before + // that control hears anything, so stopping the press here is the only way to + // keep the header from folding a file the tick is about to fold the other way. + if (node.hasAttribute("data-viewed-toggle")) return; if (node.hasAttribute("data-diffs-header")) { const filePath = node.querySelector("[data-title]")?.textContent?.trim(); if (filePath === undefined || filePath === "") return; diff --git a/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts index 203be64eebec..181ebe6cfec1 100644 --- a/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts @@ -1,7 +1,11 @@ import type { FileDiffMetadata } from "@pierre/diffs"; import { describe, expect, it } from "vite-plus/test"; -import { isFileDiffCollapsed, isLineInFileDiff } from "./pullRequestDiff.logic"; +import { + isFileDiffCollapsed, + isLineInFileDiff, + toggleFileDiffFoldForViewed, +} from "./pullRequestDiff.logic"; /** Only the hunk ranges matter here; the viewer fills the rest in when it renders. */ function fileWithHunks( @@ -79,3 +83,30 @@ describe("isFileDiffCollapsed", () => { expect(isFileDiffCollapsed("a.ts", "folded", new Set(["a.ts"]))).toBe(false); }); }); + +describe("toggleFileDiffFoldForViewed", () => { + it("puts a file away when it is ticked off", () => { + // Files start expanded, so ticking one off is the case that has somewhere to go. + expect([...toggleFileDiffFoldForViewed("a.ts", true, null, new Set())]).toEqual(["a.ts"]); + }); + + it("brings a file back when the tick is taken off", () => { + expect([...toggleFileDiffFoldForViewed("a.ts", false, null, new Set(["a.ts"]))]).toEqual([]); + }); + + it("leaves the fold alone when it already says what the tick does", () => { + const folded = new Set(["a.ts"]); + expect(toggleFileDiffFoldForViewed("a.ts", true, null, folded)).toBe(folded); + }); + + it("moves against whatever the toolbar last asked for", () => { + // Everything is open, so ticking a file off has to fold that one against the default. + expect([...toggleFileDiffFoldForViewed("a.ts", true, "expanded", new Set())]).toEqual(["a.ts"]); + expect(toggleFileDiffFoldForViewed("a.ts", false, "expanded", new Set()).size).toBe(0); + }); + + it("touches only the file that was ticked", () => { + const toggled = new Set(["a.ts", "b.ts"]); + expect([...toggleFileDiffFoldForViewed("a.ts", false, null, toggled)]).toEqual(["b.ts"]); + }); +}); diff --git a/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts b/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts index a286a4552cb6..417f91f334b0 100644 --- a/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts @@ -41,3 +41,24 @@ export function isFileDiffCollapsed( const foldedByDefault = foldOverride === "folded"; return toggledFileKeys.has(fileKey) ? !foldedByDefault : foldedByDefault; } + +/** + * The reader's fold choices after a file was ticked off, or put back. + * + * Clearing a file puts it away and un-clearing brings it back, so the tick moves the fold as if + * the reader had pressed the chevron themselves, which keeps folding a difference from what the + * toolbar last asked, and so keeps "collapse all" from ticking anything off. + */ +export function toggleFileDiffFoldForViewed( + fileKey: string, + viewed: boolean, + foldOverride: DiffFoldOverride, + toggledFileKeys: ReadonlySet, +): ReadonlySet { + if (isFileDiffCollapsed(fileKey, foldOverride, toggledFileKeys) === viewed) + return toggledFileKeys; + const next = new Set(toggledFileKeys); + if (next.has(fileKey)) next.delete(fileKey); + else next.add(fileKey); + return next; +} diff --git a/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.test.ts new file mode 100644 index 000000000000..88d3f5708c09 --- /dev/null +++ b/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.test.ts @@ -0,0 +1,140 @@ +import { describe, expect, it } from "vite-plus/test"; + +import { + countViewedFiles, + isFileViewed, + isStaleViewedState, + revertFileViewedOverlay, + settleFileViewedOverlay, + toFileViewedBatch, + toFileViewedStates, + type FileViewedOverlay, +} from "./pullRequestFilesViewed.logic"; + +const NO_OVERLAY: FileViewedOverlay = new Map(); +const NOTHING_PENDING: ReadonlySet = new Set(); + +const states = toFileViewedStates({ + files: [ + { path: "a.ts", state: "viewed" }, + { path: "b.ts", state: "unviewed" }, + { path: "c.ts", state: "dismissed" }, + ], + truncated: false, +}); + +describe("isFileViewed", () => { + it("follows the host for a file the reader has not pressed", () => { + expect(isFileViewed("a.ts", states, NO_OVERLAY)).toBe(true); + expect(isFileViewed("b.ts", states, NO_OVERLAY)).toBe(false); + }); + + it("reads a file pushed to since it was cleared as unread", () => { + expect(isFileViewed("c.ts", states, NO_OVERLAY)).toBe(false); + expect(isStaleViewedState(states?.get("c.ts"))).toBe(true); + expect(isStaleViewedState(states?.get("a.ts"))).toBe(false); + }); + + it("shows the press ahead of the host's answer", () => { + expect(isFileViewed("b.ts", states, new Map([["b.ts", true]]))).toBe(true); + expect(isFileViewed("a.ts", states, new Map([["a.ts", false]]))).toBe(false); + }); + + it("answers a file the host has said nothing about, before its answer arrives", () => { + expect(isFileViewed("z.ts", null, NO_OVERLAY)).toBe(false); + expect(isFileViewed("z.ts", null, new Map([["z.ts", true]]))).toBe(true); + }); +}); + +describe("countViewedFiles", () => { + it("counts only the files on screen, presses included", () => { + expect(countViewedFiles(["a.ts", "b.ts", "c.ts"], states, NO_OVERLAY)).toBe(1); + expect(countViewedFiles(["a.ts", "b.ts", "c.ts"], states, new Map([["b.ts", true]]))).toBe(2); + // A file the host knows about but the diff has not paged in yet is not counted. + expect(countViewedFiles(["b.ts"], states, NO_OVERLAY)).toBe(0); + }); +}); + +describe("settleFileViewedOverlay", () => { + it("drops a press the host has caught up on", () => { + const settled = settleFileViewedOverlay(new Map([["a.ts", true]]), states, NOTHING_PENDING); + expect(settled.size).toBe(0); + }); + + it("keeps a press the host still disagrees with", () => { + const overlay = new Map([["b.ts", true]]); + expect(settleFileViewedOverlay(overlay, states, NOTHING_PENDING)).toBe(overlay); + }); + + it("keeps a press the host cannot have heard yet", () => { + // An answer already on its way when the file was un-ticked would otherwise put the tick back. + const overlay = new Map([["a.ts", false]]); + const settled = settleFileViewedOverlay(overlay, states, new Set(["a.ts"])); + expect(settled.get("a.ts")).toBe(false); + }); + + it("settles a file pushed to since it was cleared against un-ticking it", () => { + const settled = settleFileViewedOverlay(new Map([["c.ts", false]]), states, NOTHING_PENDING); + expect(settled.size).toBe(0); + }); + + it("holds everything until the host has answered at all", () => { + const overlay = new Map([["a.ts", true]]); + expect(settleFileViewedOverlay(overlay, null, NOTHING_PENDING)).toBe(overlay); + }); +}); + +describe("toFileViewedBatch", () => { + it("carries both directions in one batch", () => { + expect( + toFileViewedBatch( + new Map([ + ["a.ts", false], + ["b.ts", true], + ]), + ), + ).toEqual([ + { path: "a.ts", viewed: false }, + { path: "b.ts", viewed: true }, + ]); + }); +}); + +describe("revertFileViewedOverlay", () => { + const batch = [ + { path: "a.ts", viewed: true }, + { path: "b.ts", viewed: false }, + ]; + const both = new Set(["a.ts", "b.ts"]); + + it("puts the checkbox back to the host's answer for everything the request answers for", () => { + const overlay = new Map([ + ["a.ts", true], + ["b.ts", false], + ]); + expect(revertFileViewedOverlay(overlay, batch, both).size).toBe(0); + }); + + it("leaves a press the reader made after the request went out", () => { + // The second press is waiting on a flush of its own, so the first one failing says nothing + // about it. + const overlay = new Map([ + ["a.ts", false], + ["b.ts", false], + ]); + const reverted = revertFileViewedOverlay(overlay, batch, new Set(["b.ts"])); + expect([...reverted]).toEqual([["a.ts", false]]); + }); + + it("leaves a path a later request took over, even pressed the same way", () => { + // Both requests carry `a.ts` as viewed, so the value cannot tell them apart. The later one + // owns the path now and is the one that answers for it. + const overlay = new Map([["a.ts", true]]); + expect(revertFileViewedOverlay(overlay, batch, new Set(["b.ts"]))).toBe(overlay); + }); + + it("leaves a path the request never carried", () => { + const overlay = new Map([["c.ts", true]]); + expect(revertFileViewedOverlay(overlay, batch, both)).toBe(overlay); + }); +}); diff --git a/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts b/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts new file mode 100644 index 000000000000..14d3d462ec7b --- /dev/null +++ b/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts @@ -0,0 +1,104 @@ +import type { PullRequestFileViewedState, PullRequestFilesViewedResult } from "@t3tools/contracts"; + +/** What the host last said about each file, by path. Absent means the host said nothing. */ +export type FileViewedStates = ReadonlyMap; + +/** Presses the host has not confirmed yet, by path. */ +export type FileViewedOverlay = ReadonlyMap; + +export function toFileViewedStates( + result: PullRequestFilesViewedResult | null, +): FileViewedStates | null { + if (result === null) return null; + return new Map(result.files.map((file) => [file.path, file.state])); +} + +/** + * Whether a file counts as seen. + * + * `dismissed` is the host saying it has been pushed to since the reader cleared it, which reads + * as unseen: the point of the tick is that the code behind it has been looked at, and it is not + * the same code any more. + */ +export function isViewedState(state: PullRequestFileViewedState | undefined): boolean { + return state === "viewed"; +} + +/** Whether the file was cleared and has since moved, which the header says out loud. */ +export function isStaleViewedState(state: PullRequestFileViewedState | undefined): boolean { + return state === "dismissed"; +} + +/** The press the reader made if it has not landed, and the host's answer otherwise. */ +export function isFileViewed( + path: string, + states: FileViewedStates | null, + overlay: FileViewedOverlay, +): boolean { + const pressed = overlay.get(path); + return pressed ?? isViewedState(states?.get(path)); +} + +export function countViewedFiles( + paths: ReadonlyArray, + states: FileViewedStates | null, + overlay: FileViewedOverlay, +): number { + return paths.reduce( + (total, path) => (isFileViewed(path, states, overlay) ? total + 1 : total), + 0, + ); +} + +/** + * The overlay with everything the host has caught up on removed. + * + * A press is held locally until the host's own answer agrees with it, rather than cleared when + * the request succeeds: the read that follows a write is a separate round trip, and dropping the + * press in between would flash the checkbox back for as long as that took. + * + * `unsettled` are the paths whose press the host cannot have heard yet, which an answer that was + * already on its way when they were pressed must not be allowed to overrule. + */ +export function settleFileViewedOverlay( + overlay: FileViewedOverlay, + states: FileViewedStates | null, + unsettled: ReadonlySet, +): FileViewedOverlay { + if (states === null || overlay.size === 0) return overlay; + const next = new Map(overlay); + for (const [path, pressed] of overlay) { + if (unsettled.has(path)) continue; + if (isViewedState(states.get(path)) === pressed) next.delete(path); + } + return next.size === overlay.size ? overlay : next; +} + +/** + * The overlay with a failed request's presses taken back. + * + * `owned` are the paths that request still answers for, which is what keeps a failure from + * reaching past its own presses: a path pressed again since belongs to a later request or to the + * next flush, and putting that box back to the host's answer would take a press out from under + * the reader's hand. Even among those, a press is only taken back where the checkbox still shows + * it. + */ +export function revertFileViewedOverlay( + overlay: FileViewedOverlay, + batch: ReadonlyArray<{ readonly path: string; readonly viewed: boolean }>, + owned: ReadonlySet, +): FileViewedOverlay { + const next = new Map(overlay); + for (const { path, viewed } of batch) { + if (!owned.has(path)) continue; + if (next.get(path) === viewed) next.delete(path); + } + return next.size === overlay.size ? overlay : next; +} + +/** The presses in an overlay as the batch the host is told about. */ +export function toFileViewedBatch( + overlay: FileViewedOverlay, +): ReadonlyArray<{ readonly path: string; readonly viewed: boolean }> { + return [...overlay].map(([path, viewed]) => ({ path, viewed })); +} diff --git a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts new file mode 100644 index 000000000000..f3c638ee5dba --- /dev/null +++ b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts @@ -0,0 +1,199 @@ +import { isAtomCommandInterrupted } from "@t3tools/client-runtime/state/runtime"; +import type { EnvironmentId, PullRequestRef } from "@t3tools/contracts"; +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; + +import { pullRequestEnvironment } from "~/state/pullRequests"; +import { useEnvironmentQuery } from "~/state/query"; +import { useAtomCommand } from "~/state/use-atom-command"; + +import { toastManager } from "../ui/toast"; +import { + countViewedFiles, + isFileViewed, + isStaleViewedState, + revertFileViewedOverlay, + settleFileViewedOverlay, + toFileViewedBatch, + toFileViewedStates, + type FileViewedOverlay, +} from "./pullRequestFilesViewed.logic"; + +/** + * How long presses gather before the host is told. Long enough that ticking down a file list + * costs one request rather than one per file, short enough that a reader who ticks one file and + * closes the tab has already been recorded. + */ +const FLUSH_DELAY_MS = 400; + +const NO_OVERLAY: FileViewedOverlay = new Map(); + +export interface PullRequestFilesViewedView { + /** Whether anything remembers this at all, which is what hides the whole control. */ + readonly enabled: boolean; + readonly isViewed: (path: string) => boolean; + /** This file has been pushed to since it was cleared. */ + readonly isStale: (path: string) => boolean; + readonly setViewed: (path: string, viewed: boolean) => void; + /** How many of the files on screen are ticked off. */ + readonly viewedCount: number; + /** The host had more files than the read covered, so the count above may be short. */ + readonly truncated: boolean; + /** + * Re-ask the host. The page's refresh button goes around the host's cache, and the ticks and + * the marks beside them are part of what the reader asked to be shown again. + */ + readonly refresh: () => void; +} + +/** + * Which files this reader has already cleared. + * + * The marks live on the server rather than in this tab, so a review carried on from another + * machine picks up where it was left. Where the host keeps a record of its own, those are the + * marks, and its web UI shows the same ones; where it does not, the environment keeps them and + * says so. Presses show immediately and are held over the server's answer until it agrees with + * them, so the checkbox never waits on a round trip. + */ +export function usePullRequestFilesViewed(options: { + readonly environmentId: EnvironmentId; + readonly reference: PullRequestRef; + readonly enabled: boolean; + /** The paths on screen, which is what the counter counts. */ + readonly paths: ReadonlyArray; +}): PullRequestFilesViewedView { + const { environmentId, reference, enabled, paths } = options; + const query = useEnvironmentQuery( + enabled ? pullRequestEnvironment.filesViewed({ environmentId, input: reference }) : null, + ); + const refresh = query.refresh; + const states = useMemo(() => toFileViewedStates(query.data), [query.data]); + const truncated = query.data?.truncated === true; + const [overlay, setOverlay] = useState(NO_OVERLAY); + const setFilesViewed = useAtomCommand(pullRequestEnvironment.setFilesViewed, { + reportFailure: false, + }); + + // Presses waiting for the next flush, and, for every path a request is already carrying, which + // request that is. Requests overlap and run in the order they were made, so a path pressed + // again while an earlier one is still out belongs to the later request from that moment on, and + // the earlier one stops answering for it. Both are refs rather than state: nothing on screen + // reads them, and the flush must see the latest. + const queued = useRef>(new Map()); + const sentBy = useRef>(new Map()); + const requests = useRef(0); + const flushTimer = useRef | null>(null); + + // Everything held here belongs to one change request on one environment. The environment is + // part of that: two of them can hand out the same project id, and a press made against one + // must never be answered for by the other. + const scopeKey = `${environmentId} ${reference.projectId} ${reference.repository} ${reference.number}`; + const scope = useRef(scopeKey); + + useEffect(() => { + setOverlay((current) => + settleFileViewedOverlay( + current, + states, + new Set([...queued.current.keys(), ...sentBy.current.keys()]), + ), + ); + }, [states]); + + const flush = useCallback(() => { + flushTimer.current = null; + const batch = toFileViewedBatch(queued.current); + if (batch.length === 0) return; + queued.current = new Map(); + const sentFrom = scope.current; + const request = ++requests.current; + for (const file of batch) sentBy.current.set(file.path, request); + void setFilesViewed({ environmentId, input: { ...reference, files: batch } }).then((result) => { + const mine = batch + .map((file) => file.path) + .filter((path) => sentBy.current.get(path) === request); + for (const path of mine) sentBy.current.delete(path); + // The reader has moved to another change request, or another environment, and what is on + // screen now has nothing to do with this answer. + if (scope.current !== sentFrom) return; + if (result._tag === "Failure") { + // The host never heard these, so the ticks go back to whatever it last said. Only the + // paths this request still answers for: one pressed again since is waiting on a request + // of its own, or on the next flush, and that press is the one on screen. + const owned = new Set(mine.filter((path) => !queued.current.has(path))); + setOverlay((current) => revertFileViewedOverlay(current, batch, owned)); + // Two silences here. Nothing was still this request's to answer for, so nothing on + // screen went back and a later press is the one that gets to speak for these paths. Or + // the connection went away mid-flight, which the reader is already being told about and + // which the host never refused. + if (owned.size > 0 && !isAtomCommandInterrupted(result)) { + toastManager.add({ type: "error", title: "Could not update viewed files" }); + } + return; + } + refresh(); + }); + }, [environmentId, reference, refresh, setFilesViewed]); + + // Read through a ref rather than closed over: `setViewed` is handed to every file header the + // viewer draws, and a new identity per render would rebuild all of them. + const flushRef = useRef(flush); + flushRef.current = flush; + + // Leaving a change request, the environment it lives on, or the page itself records what was + // pressed and then drops the rest. The flush kept here is the one bound to the scope being + // left, which is what sends those last presses where they were meant to go. + useEffect(() => { + const flushScope = flushRef.current; + scope.current = scopeKey; + return () => { + if (flushTimer.current !== null) { + clearTimeout(flushTimer.current); + flushScope(); + } + queued.current = new Map(); + sentBy.current = new Map(); + setOverlay(NO_OVERLAY); + }; + }, [scopeKey]); + + // Held through a ref for the same reason `setViewed` is: it goes into the view object below, + // which every file header keys off, so it has to keep one identity for the tab's life. + const refreshRef = useRef(refresh); + refreshRef.current = refresh; + const refreshFromHost = useCallback(() => refreshRef.current(), []); + + const setViewed = useCallback((path: string, viewed: boolean) => { + setOverlay((current) => new Map(current).set(path, viewed)); + queued.current.set(path, viewed); + if (flushTimer.current !== null) clearTimeout(flushTimer.current); + flushTimer.current = setTimeout(() => flushRef.current(), FLUSH_DELAY_MS); + }, []); + + const isViewed = useCallback( + (path: string) => isFileViewed(path, states, overlay), + [overlay, states], + ); + const isStale = useCallback( + (path: string) => !overlay.has(path) && isStaleViewedState(states?.get(path)), + [overlay, states], + ); + const viewedCount = useMemo( + () => countViewedFiles(paths, states, overlay), + [overlay, paths, states], + ); + + // One identity per change of what it says: the viewer keys every file it draws off this, and a + // fresh object each render would redraw the whole diff. + return useMemo( + () => ({ + enabled, + isViewed, + isStale, + setViewed, + viewedCount, + truncated, + refresh: refreshFromHost, + }), + [enabled, isStale, isViewed, refreshFromHost, setViewed, truncated, viewedCount], + ); +} diff --git a/apps/web/src/lib/openPullRequestLink.test.ts b/apps/web/src/lib/openPullRequestLink.test.ts index bd8cfe3d72d0..e938b5c958b1 100644 --- a/apps/web/src/lib/openPullRequestLink.test.ts +++ b/apps/web/src/lib/openPullRequestLink.test.ts @@ -343,6 +343,28 @@ describe("findProjectForChangeRequest", () => { ).toBeUndefined(); }); + it("matches an Azure repository cloned over SSH, whose remote shares no part with its URL", () => { + // Azure alone addresses one repository under two names: `ssh.dev.azure.com` and `v3/...` over + // SSH against `dev.azure.com` and `.../_git/...` everywhere a person sees it. The identity is + // recorded in the spelling a link arrives in, so both halves of this comparison line up. + const projects = [ + project({ + canonicalKey: "dev.azure.com/t3tools/platform/_git/t3code", + provider: "azure-devops", + displayName: "t3tools/platform/_git/t3code", + owner: "t3tools", + name: "t3code", + }), + ]; + expect( + findProjectForChangeRequest(projects, { + host: "dev.azure.com", + repository: "t3tools/platform/_git/t3code", + number: 1, + }), + ).toBe(projects[0]); + }); + it("claims nothing for a lookalike host, which is what keeps a link a link", () => { const projects = [ project({ diff --git a/apps/web/src/lib/openPullRequestLink.ts b/apps/web/src/lib/openPullRequestLink.ts index 785305fcd282..5e5c1c87cb05 100644 --- a/apps/web/src/lib/openPullRequestLink.ts +++ b/apps/web/src/lib/openPullRequestLink.ts @@ -9,7 +9,11 @@ import { useNavigate } from "@tanstack/react-router"; import * as Schema from "effect/Schema"; import { type MouseEvent, useCallback } from "react"; -import { pullRequestHostOf, type SourceControlProviderKind } from "@t3tools/contracts"; +import { + pullRequestHostOf, + pullRequestRepositoryOf, + type SourceControlProviderKind, +} from "@t3tools/contracts"; import { useOpenLink } from "../browser/useOpenLink"; import { stackedThreadToast, toastManager } from "../components/ui/toast"; @@ -296,12 +300,15 @@ export function useOpenChangeRequestLink( if (project === undefined || !reads(project.environmentId)) return false; event.preventDefault(); event.stopPropagation(); + // The selector the server derives from the same identity, not the one read out of the URL: + // a ref spelled any other way is refused before it reaches a provider, and matching a link + // only ever compares lower case. The page reads it back the same way the panel does, so + // both surfaces are handed the same spelling. + const repository = pullRequestRepositoryOf(project.repositoryIdentity) ?? parsed.repository; if (resolvedThreadRef) { useRightPanelStore.getState().openPullRequest(resolvedThreadRef, { projectId: project.id, - // The identity's own spelling, not the one read out of the URL: the panel asks the - // provider for this repository, while matching a link only ever compares lower case. - repository: project.repositoryIdentity?.displayName ?? parsed.repository, + repository, number: parsed.number, }); return true; @@ -313,7 +320,7 @@ export function useOpenChangeRequestLink( // Every state, so the pull request being opened is also in the list behind it whether // it is open, merged or closed. state: "all", - repository: parsed.repository, + repository, number: parsed.number, selectedProjectId: project.id, // Named so the page opens the right one of two servers holding this project. diff --git a/docs/user/source-control.md b/docs/user/source-control.md index 937cf91c9037..d464fce557e4 100644 --- a/docs/user/source-control.md +++ b/docs/user/source-control.md @@ -9,7 +9,7 @@ T3 Code works with the platforms your team already uses: - **GitHub** – Pull requests, repository creation, and clone integration - **GitLab** – Merge requests, repository publishing, and hosted clones - **Bitbucket** – Pull request workflows (via API token authentication) -- **Azure DevOps** – Pull request support for Microsoft-hosted repositories +- **Azure DevOps** – Pull request support for Microsoft-hosted repositories, including the file-by-file diff ## What You Can Do @@ -79,6 +79,27 @@ T3 Code works with the platforms your team already uses: - On GitHub, put a label on a pull request or take one off from the **Labels** row of the review. Changing labels needs triage access or better on the repository +**Keep your place in a long review** + +- Tick a file off in the **Code** tab once you have read it. The file collapses, and the toolbar + keeps a running count of how many files you have cleared +- Untick it to open the file back up +- Your ticks are stored with the pull request itself, so a review you start on one machine picks up + where you left it on the next, and in your browser too +- If a file is pushed to after you cleared it, it comes back marked **Changed** so you know to look + again +- On GitHub, the ticks are the ones GitHub keeps, so a review carries on between T3 Code and + github.com in either direction +- On GitLab, Bitbucket, and Azure DevOps, they are kept by the T3 Code server you are connected + to, because none of the three offers a record T3 Code can read: GitLab remembers it in one + browser's own storage, Bitbucket not at all, and Azure DevOps only inside its own web app. They + still follow you between the apps connected to that server, but the host's own site will not + show them. The count reads **viewed in T3 Code** so you can tell at a glance, and an info icon + beside it explains why +- Scope the **Code** tab to a single commit and the checkboxes stay, so you can read a change one + commit at a time. A tick belongs to the pull request, not to the commit, so a file you clear + there is cleared everywhere + ### Know Your Setup at a Glance The **Source Control settings** page shows you exactly what's connected: diff --git a/packages/client-runtime/src/state/pullRequests.ts b/packages/client-runtime/src/state/pullRequests.ts index 28c85ffe5c1e..e25036b3b101 100644 --- a/packages/client-runtime/src/state/pullRequests.ts +++ b/packages/client-runtime/src/state/pullRequests.ts @@ -147,6 +147,31 @@ export function createPullRequestEnvironmentAtoms( ]), }, }), + /** + * Which files this reader has already cleared, apart from the diff: the answer moves with + * every checkbox rather than with every push, and a patch of a few hundred files must not + * be re-fetched to learn that one box was ticked. + */ + filesViewed: createEnvironmentRpcQueryAtomFamily(runtime, { + label: "environment-data:pull-requests:files-viewed", + tag: WS_METHODS.pullRequestsFilesViewed, + staleTimeMs: 15_000, + }), + /** + * One request per batch of presses, and one in flight per change request: the host applies + * these in order, and a reader ticking down a file list faster than the round trip would + * otherwise race their own presses. + */ + setFilesViewed: createEnvironmentRpcCommand(runtime, { + label: "environment-data:pull-requests:set-files-viewed", + tag: WS_METHODS.pullRequestsSetFilesViewed, + scheduler: commandScheduler, + concurrency: { + mode: "serial", + key: ({ environmentId, input }) => + JSON.stringify([environmentId, input.projectId, input.repository, input.number]), + }, + }), runAction: createEnvironmentRpcCommand(runtime, { label: "environment-data:pull-requests:run-action", tag: WS_METHODS.pullRequestsRunAction, diff --git a/packages/contracts/src/pullRequest.test.ts b/packages/contracts/src/pullRequest.test.ts index 599006994d00..8e4cb00d8346 100644 --- a/packages/contracts/src/pullRequest.test.ts +++ b/packages/contracts/src/pullRequest.test.ts @@ -4,9 +4,12 @@ import { describe, expect, it } from "vite-plus/test"; import { PullRequestActionInput, PullRequestCapabilities, + PullRequestFilesViewedResult, PullRequestListInput, PullRequestListResult, PullRequestReviewerRequestInput, + PullRequestSetFilesViewedInput, + pullRequestRepositoryOf, resolvePullRequestAuthorFilter, } from "./pullRequest.ts"; @@ -14,6 +17,8 @@ const decodeListResult = Schema.decodeUnknownSync(PullRequestListResult); const decodeListInput = Schema.decodeUnknownSync(PullRequestListInput); const decodeReviewerRequest = Schema.decodeUnknownSync(PullRequestReviewerRequestInput); const decodeAction = Schema.decodeUnknownSync(PullRequestActionInput); +const decodeSetFilesViewed = Schema.decodeUnknownSync(PullRequestSetFilesViewedInput); +const decodeFilesViewed = Schema.decodeUnknownSync(PullRequestFilesViewedResult); const LIST_RESULT: PullRequestListResult = { viewers: { "github.com": "bilal", "gitlab.com": "bilal.hassan" }, @@ -256,3 +261,91 @@ describe("naming the reader as the author to narrow by", () => { expect(resolvePullRequestAuthorFilter("me", " ")).toBe("me"); }); }); + +describe("the repository a ref names", () => { + const identity = (fields: Record) => + ({ canonicalKey: "example.test/repo", locator: {}, ...fields }) as never; + + it("names an Azure DevOps repository by itself, not by the project path around it", () => { + // `az repos pr list --repository` takes a name and detects the organisation and project from + // the checkout; handed the recorded path it refuses, and the repository reads as unavailable. + expect( + pullRequestRepositoryOf( + identity({ + provider: "azure-devops", + displayName: "contoso/payments/_git/checkout", + owner: "contoso", + name: "checkout", + }), + ), + ).toBe("checkout"); + }); + + it("falls back to the path's last segment where an Azure identity has no name", () => { + expect( + pullRequestRepositoryOf( + identity({ provider: "azure-devops", displayName: "contoso/payments/_git/checkout" }), + ), + ).toBe("checkout"); + }); + + it("keeps a GitLab identity's whole path, because a nested group is part of the name", () => { + expect( + pullRequestRepositoryOf( + identity({ + provider: "gitlab", + displayName: "group/subgroup/service", + owner: "group", + name: "service", + }), + ), + ).toBe("group/subgroup/service"); + }); + + it("puts owner and name back together for an identity recorded before displayName", () => { + expect( + pullRequestRepositoryOf(identity({ provider: "github", owner: "t3tools", name: "t3code" })), + ).toBe("t3tools/t3code"); + }); + + it("names nothing for a project with no remote to name it by", () => { + expect(pullRequestRepositoryOf(null)).toBeNull(); + expect(pullRequestRepositoryOf(identity({ provider: "github" }))).toBeNull(); + }); +}); + +describe("naming the file a tick belongs to", () => { + // A space on either end of a name is part of the name as far as git is concerned. The patch on + // screen and the environment's record of what was cleared are both keyed by it, so a path + // tidied in transit ticks a file that does not exist and leaves the one on screen unticked. + it("keeps the spaces around a path being ticked", () => { + expect( + decodeSetFilesViewed({ + projectId: "p1", + repository: "group/project", + number: 7, + files: [{ path: "docs/readme.md ", viewed: true }], + }).files, + ).toEqual([{ path: "docs/readme.md ", viewed: true }]); + }); + + it("keeps the spaces around a path being reported back", () => { + expect( + decodeFilesViewed({ + files: [{ path: " leading.md", state: "viewed" }], + truncated: false, + }).files, + ).toEqual([{ path: " leading.md", state: "viewed" }]); + }); + + it("still refuses a path that is nothing at all", () => { + expect(() => + decodeSetFilesViewed({ + projectId: "p1", + repository: "group/project", + number: 7, + files: [{ path: "", viewed: true }], + }), + ).toThrow(); + }); +}); diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index 04c9d53cf777..89e19fb30c2b 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -9,6 +9,7 @@ import { ProjectId, TrimmedNonEmptyString, } from "./baseSchemas.ts"; +import { type RepositoryIdentity } from "./environment.ts"; import { SourceControlProviderKind } from "./sourceControl.ts"; export const PullRequestInvolvement = Schema.Literals(["all", "reviewing", "authored"]); @@ -368,6 +369,19 @@ export const PullRequestReviewerCapabilities = Schema.Struct({ }); export type PullRequestReviewerCapabilities = typeof PullRequestReviewerCapabilities.Type; +/** + * Who remembers which files a reader has cleared. + * + * `host` is the host's own record, so the marks are the ones its web UI shows and a review can be + * carried on from either side. `environment` is this server's record, for a host that keeps no + * shared one: GitLab holds its viewed files in one browser's local storage, where nothing outside + * that browser can read or write them, so marks made here are this environment's own. They still + * follow the reader between the clients connected to it, which is more than the host manages, but + * they are not the host's and the surface says so. + */ +export const PullRequestViewedFilesStore = Schema.Literals(["host", "environment"]); +export type PullRequestViewedFilesStore = typeof PullRequestViewedFilesStore.Type; + /** * What a provider can actually do, so a surface can hide what is missing rather than offer an * action that would fail. Every provider fills this in for itself; nothing is assumed. @@ -404,6 +418,18 @@ export const PullRequestCapabilities = Schema.Struct({ * what every server before this field was. */ reactions: Schema.optional(Schema.Boolean), + /** + * Where the reader's own marks are kept, or absent where they are kept nowhere and the + * checkbox is not offered at all. Optional for the same reason as `reactions`: a server that + * says nothing about it has none, which is what every server before this field was. + * + * Two answers rather than a flag, because the surface has to say which one it is. A mark the + * host keeps is the same mark its own web UI shows; a mark this environment keeps is not, and + * a reader who ticks twenty files here and then opens the host would find none of them ticked. + * A checkbox that looks the same either way and quietly means different things is the failure + * this whole feature exists to avoid. + */ + viewedFiles: Schema.optional(PullRequestViewedFilesStore), review: PullRequestReviewCapabilities, reviewers: PullRequestReviewerCapabilities, /** @@ -622,6 +648,32 @@ export const PullRequestRef = Schema.Struct({ }); export type PullRequestRef = typeof PullRequestRef.Type; +/** + * The `repository` a {@link PullRequestRef} carries, read off the project's recorded identity. + * + * `displayName` is the full path below the host, which is what nested GitLab groups need; + * owner/name is the two-segment fallback for identities recorded before that field existed. + * + * Azure DevOps is the exception: `az repos pr list --repository` takes a repository name, and + * takes the organisation and project from the checkout it detects, so the recorded + * `org/project/_git/repo` path is refused outright and the whole repository reads as + * unavailable. Its name is the last segment, which is what this hands over. + * + * Shared rather than server-only because the server checks a ref's `repository` against the one + * it derives here, so a client that spells it any other way is turned away at the door. + */ +export function pullRequestRepositoryOf( + identity: RepositoryIdentity | null | undefined, +): string | null { + if (!identity) return null; + if (identity.provider === "azure-devops") { + const segments = (identity.displayName ?? "").split("/").filter((part) => part !== "_git"); + return identity.name || segments.at(-1) || null; + } + if (identity.displayName) return identity.displayName; + return identity.owner && identity.name ? `${identity.owner}/${identity.name}` : null; +} + /** * The small live shape a linked thread needs. Keeping it separate from detail means a sidebar * status check never loads permissions, repository settings, checks, or base comparison data. @@ -855,6 +907,64 @@ export const PullRequestDiffFileContentsResult = Schema.Struct({ }); export type PullRequestDiffFileContentsResult = typeof PullRequestDiffFileContentsResult.Type; +// Not trimmed: a leading or trailing space is a legal part of a file's name, and both the patch +// and the environment's own record of what a reader cleared are keyed by the name the host gave. +// Trimming it here files the mark under a name nothing else uses, so the tick never comes back. +const FilePath = Schema.String.check(Schema.isNonEmpty()); + +/** + * Where one file of a change request stands with the person reading it. + * + * `dismissed` is the state that earns this its own read: the file was cleared, and has since been + * pushed to. It is not `viewed`, since the reader has not seen what is there now, and it is not + * `unviewed` either, because saying so would lose the one thing worth telling them, which is that + * this file and not the other forty is the one that moved. + */ +export const PullRequestFileViewedState = Schema.Literals(["unviewed", "viewed", "dismissed"]); +export type PullRequestFileViewedState = typeof PullRequestFileViewedState.Type; + +export const PullRequestFileViewed = Schema.Struct({ + path: FilePath, + state: PullRequestFileViewedState, +}); +export type PullRequestFileViewed = typeof PullRequestFileViewed.Type; + +/** + * Which files of a change request the reader has cleared, read apart from the diff itself. + * + * Its own read rather than a field on the patch, for the same reason the listing's line counts + * are their own: the two move on entirely different clocks. A patch changes when somebody pushes, + * and is cached by the minute; this changes on every press of the checkbox. Carrying it on the + * diff would mean either forgetting a three-hundred-file patch each time a box is ticked, or + * showing a reader their own last press as stale. + */ +export const PullRequestFilesViewedResult = Schema.Struct({ + /** Only the files the host reported a state for. A file missing from this list is unviewed. */ + files: Schema.Array(PullRequestFileViewed), + /** + * The host had more files than were read. The checkbox still works on everything on screen; + * the count beside it is the one thing that cannot be trusted to be whole, and says so. + */ + truncated: Schema.Boolean, +}); +export type PullRequestFilesViewedResult = typeof PullRequestFilesViewedResult.Type; + +/** + * Files to clear, or to put back. Several at once because a reader working down a diff ticks + * boxes far faster than a host answers: the surface gathers a burst into one request rather than + * opening a subprocess per press. + */ +export const PullRequestSetFilesViewedInput = Schema.Struct({ + ...PullRequestRef.fields, + files: Schema.Array( + Schema.Struct({ + path: FilePath, + viewed: Schema.Boolean, + }), + ), +}); +export type PullRequestSetFilesViewedInput = typeof PullRequestSetFilesViewedInput.Type; + export const PullRequestActionInput = Schema.Struct({ ...PullRequestRef.fields, action: PullRequestAction, diff --git a/packages/contracts/src/rpc.ts b/packages/contracts/src/rpc.ts index 9b2953a6009d..8afb985826d7 100644 --- a/packages/contracts/src/rpc.ts +++ b/packages/contracts/src/rpc.ts @@ -98,6 +98,7 @@ import { PullRequestDetail, PullRequestDiffFileContentsInput, PullRequestDiffFileContentsResult, + PullRequestFilesViewedResult, PullRequestInvalidateInput, PullRequestListInput, PullRequestListResult, @@ -111,6 +112,7 @@ import { PullRequestReviewerRequestInput, PullRequestLabelCandidateList, PullRequestLabelChangeInput, + PullRequestSetFilesViewedInput, PullRequestSubmitReviewInput, PullRequestThreadCommentsInput, PullRequestThreadCommentsResult, @@ -329,6 +331,8 @@ export const WS_METHODS = { pullRequestsActivity: "pullRequests.activity", pullRequestsThreadComments: "pullRequests.threadComments", pullRequestsDiffFileContents: "pullRequests.diffFileContents", + pullRequestsFilesViewed: "pullRequests.filesViewed", + pullRequestsSetFilesViewed: "pullRequests.setFilesViewed", pullRequestsRunAction: "pullRequests.runAction", pullRequestsUpdate: "pullRequests.update", pullRequestsComment: "pullRequests.comment", @@ -646,6 +650,23 @@ export const WsPullRequestsDiffFileContentsRpc = Rpc.make(WS_METHODS.pullRequest error: PullRequestRpcError, }); +/** + * Which files the reader has already cleared. Its own call rather than a field on the diff: the + * patch is cached by the minute and this moves on every press of a checkbox, so sharing a read + * would make one of the two wrong. + */ +export const WsPullRequestsFilesViewedRpc = Rpc.make(WS_METHODS.pullRequestsFilesViewed, { + payload: PullRequestRef, + success: PullRequestFilesViewedResult, + error: PullRequestRpcError, +}); + +export const WsPullRequestsSetFilesViewedRpc = Rpc.make(WS_METHODS.pullRequestsSetFilesViewed, { + payload: PullRequestSetFilesViewedInput, + success: Schema.Void, + error: PullRequestRpcError, +}); + export const WsPullRequestsRunActionRpc = Rpc.make(WS_METHODS.pullRequestsRunAction, { payload: PullRequestActionInput, success: Schema.Void, @@ -1192,6 +1213,8 @@ export const WsRpcGroup = RpcGroup.make( WsPullRequestsActivityRpc, WsPullRequestsThreadCommentsRpc, WsPullRequestsDiffFileContentsRpc, + WsPullRequestsFilesViewedRpc, + WsPullRequestsSetFilesViewedRpc, WsPullRequestsRunActionRpc, WsPullRequestsUpdateRpc, WsPullRequestsCommentRpc, diff --git a/packages/shared/src/git.test.ts b/packages/shared/src/git.test.ts index 8dea20f0b423..c3f1c024fe6f 100644 --- a/packages/shared/src/git.test.ts +++ b/packages/shared/src/git.test.ts @@ -49,6 +49,38 @@ describe("normalizeGitRemoteUrl", () => { "bitbucket.org/workspace/repo", ); }); + + it("gives an Azure DevOps repository the same key over SSH as over HTTPS", () => { + expect(normalizeGitRemoteUrl("git@ssh.dev.azure.com:v3/T3Tools/Platform/T3Code")).toBe( + "dev.azure.com/t3tools/platform/_git/t3code", + ); + expect(normalizeGitRemoteUrl("ssh://git@ssh.dev.azure.com:22/v3/T3Tools/Platform/T3Code")).toBe( + "dev.azure.com/t3tools/platform/_git/t3code", + ); + expect( + normalizeGitRemoteUrl("https://T3Tools@dev.azure.com/T3Tools/Platform/_git/T3Code"), + ).toBe("dev.azure.com/t3tools/platform/_git/t3code"); + }); + + it("puts the organization back in the host on the name dev.azure.com replaced", () => { + expect( + normalizeGitRemoteUrl("T3Tools@vs-ssh.visualstudio.com:v3/T3Tools/Platform/T3Code"), + ).toBe("t3tools.visualstudio.com/platform/_git/t3code"); + expect(normalizeGitRemoteUrl("https://T3Tools.visualstudio.com/Platform/_git/T3Code")).toBe( + "t3tools.visualstudio.com/platform/_git/t3code", + ); + }); + + it("leaves an Azure SSH host it cannot read as the path it was given", () => { + // Not `v3`, and not four segments: rewriting either would invent a repository that the web + // spelling has no name for, so the remote stands as it arrived. + expect(normalizeGitRemoteUrl("git@ssh.dev.azure.com:v4/T3Tools/Platform/T3Code")).toBe( + "ssh.dev.azure.com/v4/t3tools/platform/t3code", + ); + expect(normalizeGitRemoteUrl("git@ssh.dev.azure.com:v3/T3Tools/T3Code")).toBe( + "ssh.dev.azure.com/v3/t3tools/t3code", + ); + }); }); describe("parseGitHubRepositoryNameWithOwnerFromRemoteUrl", () => { diff --git a/packages/shared/src/git.ts b/packages/shared/src/git.ts index 7c088970d583..4b5107cf51bc 100644 --- a/packages/shared/src/git.ts +++ b/packages/shared/src/git.ts @@ -108,6 +108,26 @@ export function isTemporaryWorktreeBranch(refName: string): boolean { return TEMP_WORKTREE_BRANCH_PATTERN.test(refName.trim().toLowerCase()); } +/** + * The web spelling of an Azure DevOps repository reached over SSH, or null for anything else. + * + * Azure alone addresses one repository under two names that share no part: `ssh.dev.azure.com` and + * `v3/{org}/{project}/{repo}` over SSH, against `dev.azure.com` and `{org}/{project}/_git/{repo}` + * everywhere a person sees it. A project cloned over SSH would otherwise be a different repository + * to every comparison made against a pull request URL, which arrives in the web spelling. So the + * web spelling is the one both are keyed by. + */ +function azureDevOpsRepositoryKey(host: string, segments: ReadonlyArray): string | null { + if (host !== "ssh.dev.azure.com" && host !== "vs-ssh.visualstudio.com") return null; + const [marker, organization, project, repository] = segments; + if (segments.length !== 4 || marker !== "v3") return null; + if (!organization || !project || !repository) return null; + // The organization leads the host on the name dev.azure.com replaced, and the path below it. + return host === "ssh.dev.azure.com" + ? `dev.azure.com/${organization}/${project}/_git/${repository}` + : `${organization}.visualstudio.com/${project}/_git/${repository}`; +} + /** * Normalize a git remote URL into a stable comparison key. */ @@ -121,12 +141,12 @@ export function normalizeGitRemoteUrl(value: string): string { if (/^(?:ssh|https?|git):\/\//i.test(normalized)) { try { const url = new URL(normalized); - const repositoryPath = url.pathname - .split("/") - .filter((segment) => segment.length > 0) - .join("/"); - if (url.hostname && repositoryPath.includes("/")) { - return `${url.hostname}/${repositoryPath}`; + const repositorySegments = url.pathname.split("/").filter((segment) => segment.length > 0); + if (url.hostname && repositorySegments.length > 1) { + return ( + azureDevOpsRepositoryKey(url.hostname, repositorySegments) ?? + `${url.hostname}/${repositorySegments.join("/")}` + ); } } catch { return normalized; @@ -136,8 +156,10 @@ export function normalizeGitRemoteUrl(value: string): string { const scpStyleHostAndPath = /^[a-zA-Z0-9._-]+@([^:/\s]+):([^/\s]+(?:\/[^/\s]+)+)$/i.exec( normalized, ); - if (scpStyleHostAndPath?.[1] && scpStyleHostAndPath[2]) { - return `${scpStyleHostAndPath[1]}/${scpStyleHostAndPath[2]}`; + const scpHost = scpStyleHostAndPath?.[1]; + const scpPath = scpStyleHostAndPath?.[2]; + if (scpHost && scpPath) { + return azureDevOpsRepositoryKey(scpHost, scpPath.split("/")) ?? `${scpHost}/${scpPath}`; } return normalized; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 76cf816327be..97b24064f208 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -493,6 +493,9 @@ importers: '@pierre/diffs': specifier: 'catalog:' version: 1.3.0-beta.10(patch_hash=7ef7cb0cbabb17c15cdb137554068b36f15f6f5265e73fb51452aa3380db91aa)(@shikijs/themes@4.3.0)(react-dom@19.2.6(react@19.2.6))(react@19.2.6) + diff: + specifier: 8.0.3 + version: 8.0.3 effect: specifier: 4.0.0-beta.103 version: 4.0.0-beta.103(patch_hash=af36b7948b6f9c56623074662b51dade5699880c1a7c71245de73e13c3185fb6)