diff --git a/apps/server/src/auth/RpcAuthorization.ts b/apps/server/src/auth/RpcAuthorization.ts index 6b35f0d54e18..57b18b11f596 100644 --- a/apps/server/src/auth/RpcAuthorization.ts +++ b/apps/server/src/auth/RpcAuthorization.ts @@ -58,6 +58,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, @@ -66,6 +67,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/pullRequest/GitHubPullRequestCli.test.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts index 33d0d120ccce..910eafe50316 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts @@ -2603,4 +2603,149 @@ layer("GitHubPullRequestCli.layer", (it) => { ]); }), ); + + 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( + // @effect-diagnostics-next-line preferSchemaOverJson:off + 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 + // @effect-diagnostics-next-line preferSchemaOverJson:off + .mockReturnValueOnce( + Effect.succeed( + 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 6272737d4a82..c9134bbacc2a 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, @@ -30,10 +31,12 @@ import { ADD_REACTION_GRAPHQL_MUTATION, buildReviewSubmissionJson, buildReviewerRequestJson, + buildSetFilesViewedGraphQlMutation, decodeActorAvatarsJson, decodePullRequestActivityJson, decodePullRequestDetailJson, decodePullRequestFilesJson, + decodePullRequestFilesViewedJson, decodePullRequestListJson, decodePullRequestNodeIdJson, decodePullRequestSearchJson, @@ -53,6 +56,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, @@ -263,6 +267,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 @@ -308,6 +318,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, { @@ -415,6 +431,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; @@ -912,14 +952,25 @@ export const make = Effect.gen(function* () { readonly host: string; readonly query: string; readonly variables: Readonly>; + /** What this write is expected to spend, for a batch that carries more than one mutation. */ + readonly estimatedCost?: number | undefined; }) => - github - .execute({ - cwd: input.cwd, - args: ["api", "graphql", "--hostname", input.host, "--input", "-"], - stdin: encodeGraphQlRequestJson({ query: input.query, variables: input.variables }), - }) - .pipe(Effect.asVoid); + graphQlBudget + // A write is counted against the hourly budget but never held back by it, so the reserve + // that pauses reads is measured against what has really been spent rather than against + // reads alone. It cannot fail here: the budget only refuses reads. + .query(input.host, input.query, { estimatedCost: input.estimatedCost ?? 1 }) + .pipe( + Effect.orElseSucceed(() => input.query), + Effect.flatMap((query) => + github.execute({ + cwd: input.cwd, + args: ["api", "graphql", "--hostname", input.host, "--input", "-"], + stdin: encodeGraphQlRequestJson({ query, variables: input.variables }), + }), + ), + Effect.asVoid, + ); /** A GraphQL read whose answer is decoded, reporting a failure against the read that made it. */ const graphqlRead = (input: { @@ -1764,6 +1815,59 @@ 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 }, + estimatedCost: input.files.length, + }), + ), + ); + }, + setReviewThreadResolution: (input) => graphql({ cwd: input.cwd, diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts index cc097c30c2ed..ae057251fca9 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts @@ -33,6 +33,7 @@ const CAPABILITIES: PullRequestCapabilities = { updateMethods: ["merge", "rebase"], search: true, reactions: true, + viewedFiles: true, review: { inlineComment: true, reply: true, @@ -399,6 +400,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/PullRequestProvider.ts b/apps/server/src/pullRequest/PullRequestProvider.ts index 644f3552cbc5..1ecba8c04224 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, @@ -201,6 +202,12 @@ 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; +} + export interface ProviderRepositoryRef { readonly cwd: string; /** Provider-native repository identity, e.g. `owner/repo` or `group/subgroup/project`. */ @@ -355,6 +362,29 @@ export interface PullRequestProviderApi { }, ) => Effect.Effect; + /** + * Which files the reader has already cleared. Only called when `capabilities.viewedFiles` is + * true, 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 true. + * + * 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; + 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 84bd57dfa27b..987dba0d1bde 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -3385,3 +3385,84 @@ 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: true, + 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); + }), +); + +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 fc76a6501931..41b2a5bd61c5 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -22,6 +22,7 @@ import { type PullRequestDiffFileContentsResult, type PullRequestDiffStat, type PullRequestDiffInput, + type PullRequestFilesViewedResult, type PullRequestDiffResult, type PullRequestInvalidateInput, type PullRequestListEntry, @@ -37,6 +38,7 @@ import { type PullRequestReviewVerdict, type PullRequestReviewerCandidateList, type PullRequestReviewerRequestInput, + type PullRequestSetFilesViewedInput, type PullRequestSubmitReviewInput, type PullRequestThreadReplyInput, type PullRequestThreadResolutionInput, @@ -102,6 +104,12 @@ 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 a cache's last success may still be served while a fresh read runs behind it. * Bounded by how the page actually revalidates: clients re-read on mount and once a minute @@ -119,6 +127,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; export type PullRequestError = PullRequestUnavailableError | PullRequestOperationError; @@ -144,6 +153,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; @@ -450,6 +465,12 @@ 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) }), runAction: interactive("runAction", api.runAction), ...(api.updateChangeRequest === undefined ? {} @@ -1297,6 +1318,51 @@ export const make = Effect.gen(function* () { }), ); + const filesViewedUncached = (input: PullRequestRef) => + requireProject(input).pipe( + Effect.flatMap((project) => { + const read = project.api.getFilesViewed; + return project.api.capabilities.viewedFiles === true && read + ? read({ + cwd: project.project.workspaceRoot, + repository: project.repository, + host: project.host, + number: input.number, + }).pipe(Effect.mapError(toPullRequestError("filesViewed"))) + : 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; + return project.api.capabilities.viewedFiles === true && write + ? write({ + cwd: project.project.workspaceRoot, + repository: project.repository, + host: project.host, + number: input.number, + files: input.files, + }).pipe(Effect.mapError(toPullRequestError("setFilesViewed"))) + : 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: PullRequestService["Service"]["runAction"] = (input) => requireProject(input).pipe( Effect.flatMap((project): Effect.Effect => { @@ -1872,14 +1938,20 @@ export const make = Effect.gen(function* () { const REF_EPOCH_CAPACITY = 2_048; const refScope = (ref: PullRequestRef) => `${ref.projectId} ${ref.repository} ${ref.number}`; const refEpoch = (ref: PullRequestRef) => refEpochs.get(refScope(ref)) ?? 0; - 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 = ( @@ -2060,6 +2132,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]>]; @@ -2130,6 +2230,8 @@ export const make = Effect.gen(function* () { threadComments, diff, diffFileContents, + filesViewed, + setFilesViewed, runAction: invalidatedByMutation(runAction), update: invalidatedByMutation(update), comment: invalidatedByMutation(comment), diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts index f372ac3000a0..f20f20d5a265 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, @@ -1361,3 +1363,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 6ec17ea111b3..773b3aa6700b 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, PullRequestOmittedFileStat, @@ -2239,3 +2240,122 @@ export function decodePullRequestFilesJson( omittedFileStats, }); } + +/** + * Which files of a pull request the signed-in account has cleared. + * + * GraphQL only — 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 — `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/sourceControl/githubGraphQlBudget.test.ts b/apps/server/src/sourceControl/githubGraphQlBudget.test.ts index a166bf0dbbaf..b85371c810e2 100644 --- a/apps/server/src/sourceControl/githubGraphQlBudget.test.ts +++ b/apps/server/src/sourceControl/githubGraphQlBudget.test.ts @@ -186,4 +186,31 @@ describe("GitHub GraphQL budget", () => { expect(yield* budget.query("github.com", mutation)).toBe(mutation); }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), ); + + it.effect("charges a write for the batch it carries, since it cannot report its own cost", () => + Effect.gen(function* () { + yield* TestClock.setTime(BEFORE_RESET); + const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; + // Twenty points above the reserve, which is exactly what the mutation below spends. + yield* budget.observe("github.com", rateLimit(520)); + + yield* budget.query("github.com", "mutation { f0: markFileAsViewed { id } }", { + estimatedCost: 20, + }); + + const error = yield* Effect.flip(budget.query("github.com", "query { viewer { login } }")); + expect(error).toMatchObject({ _tag: "SourceControlRateLimitPausedError" }); + }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), + ); + + it.effect("lets a write through even with nothing left, rather than holding a press back", () => + Effect.gen(function* () { + yield* TestClock.setTime(BEFORE_RESET); + const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; + yield* budget.observe("github.com", rateLimit(0)); + + const mutation = "mutation { f0: markFileAsViewed { id } }"; + expect(yield* budget.query("github.com", mutation, { estimatedCost: 40 })).toBe(mutation); + }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), + ); }); diff --git a/apps/server/src/sourceControl/githubGraphQlBudget.ts b/apps/server/src/sourceControl/githubGraphQlBudget.ts index 9c43de8e0586..8745d691bc84 100644 --- a/apps/server/src/sourceControl/githubGraphQlBudget.ts +++ b/apps/server/src/sourceControl/githubGraphQlBudget.ts @@ -23,7 +23,14 @@ export class GitHubGraphQlBudget extends Context.Service< readonly query: ( host: string, document: string, - options?: { readonly allowReserve: boolean }, + options?: { + readonly allowReserve?: boolean | undefined; + /** + * What a write is expected to spend, for the debit above. Ignored for a read, which + * reports its own cost. Defaults to one point, which is a mutation's floor. + */ + readonly estimatedCost?: number | undefined; + }, ) => Effect.Effect; readonly observe: (host: string, raw: string) => Effect.Effect; } @@ -82,8 +89,27 @@ export const make = Effect.gen(function* () { const query: GitHubGraphQlBudget["Service"]["query"] = Effect.fn("GitHubGraphQlBudget.query")( function* (host, document, options) { - if (!isReadOperation(document)) return document; const now = yield* Clock.currentTimeMillis; + // A write spends the same hourly points a read does, and `rateLimit` is a field of Query + // alone — so a mutation cannot report its own cost and is debited from the held snapshot + // instead. Never paused, only counted: a mutation is somebody pressing something, and + // holding it back to protect a read nobody has asked for yet is the wrong trade. The + // estimate only has to last until the next read, whose answer replaces the snapshot with + // the host's own number. + if (!isReadOperation(document)) { + yield* Ref.update(snapshots, (current) => { + const key = hostKey(host); + const snapshot = current.get(key); + if (snapshot === undefined || snapshot.resetAtMs <= now) return current; + const next = new Map(current); + next.set(key, { + ...snapshot, + remaining: Math.max(0, snapshot.remaining - Math.max(1, options?.estimatedCost ?? 1)), + }); + return next; + }); + return document; + } const retryAt = yield* Ref.modify(snapshots, (current) => { const key = hostKey(host); const snapshot = current.get(key); diff --git a/apps/server/src/ws.ts b/apps/server/src/ws.ts index 3f5d11c980be..74d664afa582 100644 --- a/apps/server/src/ws.ts +++ b/apps/server/src/ws.ts @@ -1378,6 +1378,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/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index b0e00d57cc61..fa9e5ed97026 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -58,6 +58,7 @@ import { DiffWorkerPoolProvider } from "../DiffWorkerPoolProvider"; import { DiffCommentAnnotation } from "../diffs/DiffCommentAnnotation"; import { StyledDiffCodeView } from "../diffs/StyledDiffCodeView"; import { Button } from "../ui/button"; +import { Checkbox } from "../ui/checkbox"; import { Collapsible, CollapsiblePanel, CollapsibleTrigger } from "../ui/collapsible"; import { DropdownMenu, @@ -73,9 +74,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, @@ -396,6 +399,17 @@ 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 itself stays the host's: it is kept + // against the change request, so clearing a file here clears it everywhere. + const filesViewed = usePullRequestFilesViewed({ + environmentId, + reference, + enabled: detail.capabilities.viewedFiles === true, + paths: filePaths, + }); + const { setViewed } = filesViewed; 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. @@ -587,6 +601,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], + ); + const toggleAllFiles = () => { // Held as an override of the default rather than as the file keys on screen: a diff that is // still paging would otherwise bring its next slice in folded, moments after the reader @@ -722,19 +749,51 @@ 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 = ( ); + if (!filesViewed.enabled) return stat; + const viewed = filesViewed.isViewed(path); + const stale = filesViewed.isStale(path); + return ( + + {stat} + {/* The header itself folds the file, so the tick has to keep its press to itself. */} + + + ); }, - [omittedFileStats], + [filesViewed, omittedFileStats, setFileViewed], ); const diffViewOptions = useMemo( @@ -1058,6 +1117,11 @@ export function PullRequestCodeTab({ {files.length} {files.length === 1 ? "file" : "files"} {nextCursor === null ? "" : "+"} + {filesViewed.enabled && files.length > 0 ? ( + + {filesViewed.viewedCount} / {files.length} viewed + + ) : null} {withheldContent ? ( }> diff --git a/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts index b39cfd9ff1b5..5a5ae8149097 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,31 @@ 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 folded, so one the reader had opened is the case that has somewhere to go. + const opened = new Set(["a.ts"]); + expect([...toggleFileDiffFoldForViewed("a.ts", true, null, opened)]).toEqual([]); + }); + + it("brings a file back when the tick is taken off", () => { + expect([...toggleFileDiffFoldForViewed("a.ts", false, null, new Set())]).toEqual(["a.ts"]); + }); + + it("leaves the fold alone when it already says what the tick does", () => { + const folded = new Set(); + 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", true, 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 b3c19c4fe9c2..8a6061c4e5c6 100644 --- a/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts @@ -42,3 +42,24 @@ export function isFileDiffCollapsed( const foldedByDefault = foldOverride !== "expanded"; 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..90c3d71f9b04 --- /dev/null +++ b/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.test.ts @@ -0,0 +1,100 @@ +import { describe, expect, it } from "vite-plus/test"; + +import { + countViewedFiles, + isFileViewed, + isStaleViewedState, + 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 }, + ]); + }); +}); 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..2bc011297a53 --- /dev/null +++ b/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts @@ -0,0 +1,82 @@ +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 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..32d6934a57ec --- /dev/null +++ b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts @@ -0,0 +1,147 @@ +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, + 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(); +const NO_PATHS: ReadonlySet = new Set(); + +export interface PullRequestFilesViewedView { + /** Whether the host tracks this at all, which is what hides the whole control. */ + readonly enabled: boolean; + readonly isViewed: (path: string) => boolean; + /** The host says 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; +} + +/** + * Which files this reader has already cleared, as the host records it. + * + * The state lives on the host rather than here so a review carried on from another machine, or + * from the host's own web UI, picks up where it was left. Presses show immediately and are held + * over the host'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 [overlay, setOverlay] = useState(NO_OVERLAY); + const setFilesViewed = useAtomCommand(pullRequestEnvironment.setFilesViewed); + + // Presses waiting for the next flush, and the ones a request is already carrying. Both are + // refs rather than state: nothing on screen reads them, and the flush must see the latest. + const queued = useRef>(new Map()); + const inFlight = useRef>(NO_PATHS); + const flushTimer = useRef | null>(null); + + const referenceKey = `${reference.projectId} ${reference.repository} ${reference.number}`; + // Everything held here is about one change request, so switching away drops it rather than + // letting a press meant for one land on another. + useEffect(() => { + queued.current = new Map(); + inFlight.current = NO_PATHS; + setOverlay(NO_OVERLAY); + }, [referenceKey]); + + useEffect(() => { + setOverlay((current) => + settleFileViewedOverlay( + current, + states, + new Set([...queued.current.keys(), ...inFlight.current]), + ), + ); + }, [states]); + + const flush = useCallback(() => { + flushTimer.current = null; + const batch = toFileViewedBatch(queued.current); + if (batch.length === 0) return; + queued.current = new Map(); + const sent = new Set(batch.map((file) => file.path)); + inFlight.current = sent; + void setFilesViewed({ environmentId, input: { ...reference, files: batch } }).then((result) => { + inFlight.current = NO_PATHS; + if (result._tag === "Failure") { + // The host never heard these, so the ticks go back to whatever it last said. + setOverlay((current) => { + const next = new Map(current); + for (const path of sent) next.delete(path); + return next; + }); + 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; + + // A tab closed mid-gather still records what was pressed. + useEffect( + () => () => { + if (flushTimer.current === null) return; + clearTimeout(flushTimer.current); + flushRef.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], + ); + + return { enabled, isViewed, isStale, setViewed, viewedCount }; +} diff --git a/docs/fork/0013-keep-your-place-in-a-review.md b/docs/fork/0013-keep-your-place-in-a-review.md new file mode 100644 index 000000000000..24c375a61701 --- /dev/null +++ b/docs/fork/0013-keep-your-place-in-a-review.md @@ -0,0 +1,48 @@ +# 0013: Keep your place in a long review + +- PR: [TrogonStack/t3code#23](https://github.com/TrogonStack/t3code/pull/23) +- Status: active + +## What you can do now + +- Tick a file off as you finish reading it. The file collapses out of the way, + and the toolbar keeps a running count of how much of the change is behind + you, so a two hundred file review stops being a wall and becomes a list you + work down. +- Come back to a review on another machine, or in the browser, and find the + same files already ticked. The marks are kept with the pull request itself + rather than in this app, so they are the same marks GitHub shows you. +- Untick a file to open it back up when a second read is warranted. +- See a file you already cleared come back marked as changed once somebody + pushes to it, so a late commit cannot slip past a review that was finished + before it landed. +- Read a change one commit at a time and still tick files off, since the mark + belongs to the pull request rather than to the commit you happen to be + looking at. + +## Why + +Reviewing in this app was fine for a small change and unusable for a large +one. Nothing remembered where you were, so a review spread over an afternoon, +or picked up on a different device, started again from the top every time. The +practical result was that big reviews went to the browser and only small ones +stayed here, which undermines the reason to read code in this app at all. + +Keeping the marks on the host rather than locally is the part that matters. +A checkbox that only this app remembers is worse than no checkbox: it looks +like the one GitHub shows, disagrees with it, and leaves you unsure which of +the two knows what you have actually read. Deferring to the host means there +is exactly one answer, and switching between this app and the browser +mid-review costs nothing. + +## Upstream considerations + +Worth submitting. It is a plain product gap rather than anything specific to +how we work, and the shape it takes here is the one upstream would want: the +capability is declared per provider, so GitHub offers it and the hosts that +have no equivalent hide it rather than showing a control that cannot work. + +The rebase burden is moderate. It touches the pull request code tab, the +provider port, and the wire contracts, all of which upstream changes often, so +a sync is likely to want a hand in those files. The pieces that carry the +reasoning are in files of their own, which keeps the conflicts to the wiring. diff --git a/docs/fork/README.md b/docs/fork/README.md index e65ccd798b48..ad82f8987e74 100644 --- a/docs/fork/README.md +++ b/docs/fork/README.md @@ -43,3 +43,5 @@ Each entry uses these sections: active, [#20](https://github.com/TrogonStack/t3code/pull/20) - **0012** [The timeline scrolls only as far as its content](./0012-timeline-scrolls-only-as-far-as-its-content.md) active, [#21](https://github.com/TrogonStack/t3code/pull/21) +- **0013** [Keep your place in a long review](./0013-keep-your-place-in-a-review.md) + active, [#23](https://github.com/TrogonStack/t3code/pull/23) diff --git a/docs/user/source-control.md b/docs/user/source-control.md index c64a63f7bc49..2f639e737f98 100644 --- a/docs/user/source-control.md +++ b/docs/user/source-control.md @@ -53,6 +53,21 @@ T3 Code works with the platforms your team already uses: - Works on GitHub, GitLab, and Bitbucket. Azure DevOps takes a new title and description; its comments stay read-only here, as they already were +**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 +- GitHub only. GitLab, Bitbucket, and Azure DevOps do not keep this, so the checkbox is not shown + there +- 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 d4830fa197d4..33d9b528a699 100644 --- a/packages/client-runtime/src/state/pullRequests.ts +++ b/packages/client-runtime/src/state/pullRequests.ts @@ -106,6 +106,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.ts b/packages/contracts/src/pullRequest.ts index a49868937844..86a8927d4461 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -384,6 +384,16 @@ export const PullRequestCapabilities = Schema.Struct({ * what every server before this field was. */ reactions: Schema.optional(Schema.Boolean), + /** + * A file can be marked as read by the person reading it, and the mark taken back. 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. + * + * True on GitHub alone so far. The others expose no equivalent, and a checkbox whose mark is + * forgotten the moment the tab closes is worse than no checkbox — it looks like the one beside + * it and keeps none of its promises. + */ + viewedFiles: Schema.optional(Schema.Boolean), review: PullRequestReviewCapabilities, reviewers: PullRequestReviewerCapabilities, /** @@ -800,6 +810,59 @@ export const PullRequestDiffFileContentsResult = Schema.Struct({ }); export type PullRequestDiffFileContentsResult = typeof PullRequestDiffFileContentsResult.Type; +/** + * 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` — 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: TrimmedNonEmptyString, + 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: TrimmedNonEmptyString, + 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 51c65f50e1a2..af1b4ba2a1ac 100644 --- a/packages/contracts/src/rpc.ts +++ b/packages/contracts/src/rpc.ts @@ -75,6 +75,7 @@ import { PullRequestDetail, PullRequestDiffFileContentsInput, PullRequestDiffFileContentsResult, + PullRequestFilesViewedResult, PullRequestInvalidateInput, PullRequestListInput, PullRequestListResult, @@ -85,6 +86,7 @@ import { PullRequestRef, PullRequestReviewerCandidateList, PullRequestReviewerRequestInput, + PullRequestSetFilesViewedInput, PullRequestSubmitReviewInput, PullRequestThreadCommentsInput, PullRequestThreadCommentsResult, @@ -285,6 +287,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", @@ -517,6 +521,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, @@ -1012,6 +1033,8 @@ export const WsRpcGroup = RpcGroup.make( WsPullRequestsActivityRpc, WsPullRequestsThreadCommentsRpc, WsPullRequestsDiffFileContentsRpc, + WsPullRequestsFilesViewedRpc, + WsPullRequestsSetFilesViewedRpc, WsPullRequestsRunActionRpc, WsPullRequestsUpdateRpc, WsPullRequestsCommentRpc,