diff --git a/apps/server/src/auth/RpcAuthorization.ts b/apps/server/src/auth/RpcAuthorization.ts index 28ceac4cec99..7522adec032a 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..e114abc180cb 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts @@ -2603,4 +2603,148 @@ 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( + 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 2084a50d0206..8f175d6d50d0 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; @@ -1763,6 +1803,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 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 d142e7368f6d..7903c3de6472 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -3422,3 +3422,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 3a0d1aac699d..0467fd0f9a00 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); /** 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. */ @@ -110,6 +118,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; @@ -135,6 +144,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; @@ -441,6 +456,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 ? {} @@ -1288,6 +1309,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 => { @@ -1855,14 +1921,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 = ( @@ -2027,6 +2099,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]>]; @@ -2093,6 +2193,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..c77514b5f6d6 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, 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/ws.ts b/apps/server/src/ws.ts index 226c82cdb1ac..9fdbef7b9d49 100644 --- a/apps/server/src/ws.ts +++ b/apps/server/src/ws.ts @@ -1822,6 +1822,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..23978183e825 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, @@ -317,13 +320,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); @@ -396,6 +392,28 @@ 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, 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. @@ -471,6 +489,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), @@ -486,7 +510,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 @@ -520,6 +544,7 @@ export function PullRequestCodeTab({ detail.reviewThreads, draft, files, + filesViewed, foldOverride, pendingComments, placedThreadIds, @@ -587,6 +612,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 @@ -713,6 +751,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; @@ -722,17 +769,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], ); @@ -1058,6 +1143,25 @@ export function PullRequestCodeTab({ {files.length} {files.length === 1 ? "file" : "files"} {nextCursor === null ? "" : "+"} + {filesViewed.enabled && files.length > 0 ? ( + + {filesViewed.viewedCount} / {files.length} viewed + {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 ? ( }> @@ -1327,6 +1431,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 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..75b680be1790 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..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..573b823b1170 --- /dev/null +++ b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts @@ -0,0 +1,197 @@ +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 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; + /** 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, 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 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/docs/user/source-control.md b/docs/user/source-control.md index 916536bbe736..1bf50e0cd1a1 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 ecb6a09a2ab5..23b159cb950c 100644 --- a/packages/client-runtime/src/state/pullRequests.ts +++ b/packages/client-runtime/src/state/pullRequests.ts @@ -137,6 +137,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..598c7caf18ad 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`, 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: 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 14363cfedff9..aecad268311e 100644 --- a/packages/contracts/src/rpc.ts +++ b/packages/contracts/src/rpc.ts @@ -88,6 +88,7 @@ import { PullRequestDetail, PullRequestDiffFileContentsInput, PullRequestDiffFileContentsResult, + PullRequestFilesViewedResult, PullRequestInvalidateInput, PullRequestListInput, PullRequestListResult, @@ -98,6 +99,7 @@ import { PullRequestRef, PullRequestReviewerCandidateList, PullRequestReviewerRequestInput, + PullRequestSetFilesViewedInput, PullRequestSubmitReviewInput, PullRequestThreadCommentsInput, PullRequestThreadCommentsResult, @@ -303,6 +305,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", @@ -535,6 +539,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, @@ -1047,6 +1068,8 @@ export const WsRpcGroup = RpcGroup.make( WsPullRequestsActivityRpc, WsPullRequestsThreadCommentsRpc, WsPullRequestsDiffFileContentsRpc, + WsPullRequestsFilesViewedRpc, + WsPullRequestsSetFilesViewedRpc, WsPullRequestsRunActionRpc, WsPullRequestsUpdateRpc, WsPullRequestsCommentRpc,