From d2d5bba7038678eaa5fc6a885fa5ab05d0854725 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 20 Aug 2026 16:50:58 -0400 Subject: [PATCH 01/13] feat(web): pull request files can be marked as viewed A review spread over an afternoon, or picked up on a second machine, started again from the top every time, so large changes were read in the browser and only small ones stayed here. The marks are the host's rather than ours because a checkbox only this app remembers is worse than none: it looks like the one GitHub shows, disagrees with it, and leaves a reviewer unsure which of the two knows what they have actually read. Signed-off-by: Yordis Prieto --- apps/server/src/auth/RpcAuthorization.ts | 2 + .../pullRequest/GitHubPullRequestCli.test.ts | 144 +++++++++++++++++ .../src/pullRequest/GitHubPullRequestCli.ts | 118 +++++++++++++- .../pullRequest/GitHubPullRequestProvider.ts | 7 + .../src/pullRequest/PullRequestProvider.ts | 30 ++++ .../pullRequest/PullRequestService.test.ts | 81 ++++++++++ .../src/pullRequest/PullRequestService.ts | 112 ++++++++++++- .../pullRequest/gitHubPullRequestJson.test.ts | 96 ++++++++++++ .../src/pullRequest/gitHubPullRequestJson.ts | 120 ++++++++++++++ .../sourceControl/githubGraphQlBudget.test.ts | 27 ++++ .../src/sourceControl/githubGraphQlBudget.ts | 30 +++- apps/server/src/ws.ts | 10 ++ .../pullRequest/PullRequestCodeTab.tsx | 70 ++++++++- .../pullRequest/pullRequestDiff.logic.test.ts | 34 +++- .../pullRequest/pullRequestDiff.logic.ts | 21 +++ .../pullRequestFilesViewed.logic.test.ts | 100 ++++++++++++ .../pullRequestFilesViewed.logic.ts | 82 ++++++++++ .../pullRequest/usePullRequestFilesViewed.ts | 147 ++++++++++++++++++ docs/user/source-control.md | 15 ++ .../client-runtime/src/state/pullRequests.ts | 25 +++ packages/contracts/src/pullRequest.ts | 63 ++++++++ packages/contracts/src/rpc.ts | 23 +++ 22 files changed, 1339 insertions(+), 18 deletions(-) create mode 100644 apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.test.ts create mode 100644 apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts create mode 100644 apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts 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..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..73b9d29005dd 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: { @@ -1763,6 +1814,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 c5b7e50a8704..350b8bf6f7c9 100644 --- a/apps/server/src/ws.ts +++ b/apps/server/src/ws.ts @@ -1693,6 +1693,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/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 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, From cf5ac4c80da63b8f73630d80aefc0ecf7aac3430 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 20 Aug 2026 17:09:48 -0400 Subject: [PATCH 02/13] fix(server): an overpriced write guess no longer pauses reads until the window resets Signed-off-by: Yordis Prieto --- .../sourceControl/githubGraphQlBudget.test.ts | 36 +++++++++++++++++++ .../src/sourceControl/githubGraphQlBudget.ts | 24 ++++++++++--- 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/apps/server/src/sourceControl/githubGraphQlBudget.test.ts b/apps/server/src/sourceControl/githubGraphQlBudget.test.ts index b85371c810e2..da8377b9aeb8 100644 --- a/apps/server/src/sourceControl/githubGraphQlBudget.test.ts +++ b/apps/server/src/sourceControl/githubGraphQlBudget.test.ts @@ -203,6 +203,42 @@ describe("GitHub GraphQL budget", () => { }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), ); + it.effect("takes the host's own number over a write's guess, however high the guess was", () => + Effect.gen(function* () { + yield* TestClock.setTime(BEFORE_RESET); + const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; + yield* budget.observe("github.com", rateLimit(4_000)); + // A batch charged far more than it really spent would otherwise hold reads until the reset. + yield* budget.query("github.com", "mutation { f0: markFileAsViewed { id } }", { + estimatedCost: 3_900, + }); + yield* Effect.flip(budget.query("github.com", "query { viewer { login } }")); + + yield* budget.observe("github.com", rateLimit(3_990)); + + expect(yield* budget.query("github.com", "query { viewer { login } }")).toContain( + "rateLimit", + ); + }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), + ); + + it.effect("still ignores an out-of-order answer once the guess has been settled", () => + Effect.gen(function* () { + yield* TestClock.setTime(BEFORE_RESET); + const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; + yield* budget.observe("github.com", rateLimit(600)); + yield* budget.query("github.com", "mutation { f0: markFileAsViewed { id } }", { + estimatedCost: 50, + }); + // The host's own number settles the guess, and the answer behind it is stale again. + yield* budget.observe("github.com", rateLimit(513)); + yield* budget.observe("github.com", rateLimit(600)); + + 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); diff --git a/apps/server/src/sourceControl/githubGraphQlBudget.ts b/apps/server/src/sourceControl/githubGraphQlBudget.ts index 8745d691bc84..daa52bfb0d62 100644 --- a/apps/server/src/sourceControl/githubGraphQlBudget.ts +++ b/apps/server/src/sourceControl/githubGraphQlBudget.ts @@ -15,6 +15,12 @@ interface GraphQlBudgetSnapshot { readonly limit: number; readonly remaining: number; readonly resetAtMs: number; + /** + * Points taken off `remaining` for writes the host has not answered for yet. A mutation cannot + * ask what it cost, so this is a guess, and while a guess is standing the host's own number is + * allowed to raise `remaining` again instead of being read as an out-of-order answer. + */ + readonly estimatedSpend: number; } export class GitHubGraphQlBudget extends Context.Service< @@ -66,7 +72,9 @@ function snapshotFrom(raw: string): GraphQlBudgetSnapshot | null { return null; } const resetAtMs = Date.parse(resetAt); - return Number.isFinite(resetAtMs) ? { cost, limit, remaining, resetAtMs } : null; + return Number.isFinite(resetAtMs) + ? { cost, limit, remaining, resetAtMs, estimatedSpend: 0 } + : null; } catch { return null; } @@ -91,7 +99,7 @@ export const make = Effect.gen(function* () { function* (host, document, options) { 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 + // 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 @@ -101,10 +109,12 @@ export const make = Effect.gen(function* () { const key = hostKey(host); const snapshot = current.get(key); if (snapshot === undefined || snapshot.resetAtMs <= now) return current; + const spend = Math.max(1, options?.estimatedCost ?? 1); const next = new Map(current); next.set(key, { ...snapshot, - remaining: Math.max(0, snapshot.remaining - Math.max(1, options?.estimatedCost ?? 1)), + remaining: Math.max(0, snapshot.remaining - spend), + estimatedSpend: snapshot.estimatedSpend + spend, }); return next; }); @@ -148,10 +158,16 @@ export const make = Effect.gen(function* () { const previous = current.get(key); // Concurrent reads can finish out of order. Quota only falls within one reset window, and // an answer from an older window must not replace the current one. + // + // Unless a write's guess is standing: that number was never the host's, and an estimate + // pitched too high would otherwise pause every read until the window reset, with the one + // answer that could correct it thrown away for looking stale. if ( previous !== undefined && (snapshot.resetAtMs < previous.resetAtMs || - (snapshot.resetAtMs === previous.resetAtMs && snapshot.remaining >= previous.remaining)) + (snapshot.resetAtMs === previous.resetAtMs && + previous.estimatedSpend === 0 && + snapshot.remaining >= previous.remaining)) ) { return current; } From e62d906b920142210c31218c5d6b011c53ce8cf1 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 20 Aug 2026 17:09:49 -0400 Subject: [PATCH 03/13] fix(web): a failed press no longer takes back a tick the reader made since Signed-off-by: Yordis Prieto --- .../pullRequestFilesViewed.logic.test.ts | 32 ++++++++ .../pullRequestFilesViewed.logic.ts | 22 +++++- .../pullRequest/usePullRequestFilesViewed.ts | 77 +++++++++++-------- 3 files changed, 100 insertions(+), 31 deletions(-) diff --git a/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.test.ts index 90c3d71f9b04..78dd8d1298ee 100644 --- a/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.test.ts @@ -4,6 +4,7 @@ import { countViewedFiles, isFileViewed, isStaleViewedState, + revertFileViewedOverlay, settleFileViewedOverlay, toFileViewedBatch, toFileViewedStates, @@ -98,3 +99,34 @@ describe("toFileViewedBatch", () => { ]); }); }); + +describe("revertFileViewedOverlay", () => { + const batch = [ + { path: "a.ts", viewed: true }, + { path: "b.ts", viewed: false }, + ]; + + it("puts the checkbox back to the host's answer for everything the request carried", () => { + const overlay = new Map([ + ["a.ts", true], + ["b.ts", false], + ]); + expect(revertFileViewedOverlay(overlay, batch, new Set()).size).toBe(0); + }); + + it("leaves a press the reader made after the request went out", () => { + // The second press is queued behind a request 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(["a.ts"])); + expect([...reverted]).toEqual([["a.ts", false]]); + }); + + it("leaves a path the request never carried", () => { + const overlay = new Map([["c.ts", true]]); + expect(revertFileViewedOverlay(overlay, batch, new Set())).toBe(overlay); + }); +}); diff --git a/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts b/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts index 2bc011297a53..04c03ba424fa 100644 --- a/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts @@ -17,7 +17,7 @@ export function toFileViewedStates( * 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 + * 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 { @@ -74,6 +74,26 @@ export function settleFileViewedOverlay( return next.size === overlay.size ? overlay : next; } +/** + * The overlay with a failed request's presses taken back. + * + * Only the presses that request carried, and only where the checkbox still shows them: a path + * the reader has pressed again since is waiting on a request of its own, and putting that box + * back to the host's answer would take a press out from under the reader's hand. + */ +export function revertFileViewedOverlay( + overlay: FileViewedOverlay, + batch: ReadonlyArray<{ readonly path: string; readonly viewed: boolean }>, + superseded: ReadonlySet, +): FileViewedOverlay { + const next = new Map(overlay); + for (const { path, viewed } of batch) { + if (superseded.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, diff --git a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts index 32d6934a57ec..b84028ee7352 100644 --- a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts +++ b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts @@ -10,6 +10,7 @@ import { countViewedFiles, isFileViewed, isStaleViewedState, + revertFileViewedOverlay, settleFileViewedOverlay, toFileViewedBatch, toFileViewedStates, @@ -24,7 +25,6 @@ import { 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. */ @@ -35,6 +35,8 @@ export interface PullRequestFilesViewedView { 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; } /** @@ -57,30 +59,28 @@ export function usePullRequestFilesViewed(options: { ); 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); // 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 inFlight = useRef>(new Map()); 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]); + // 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(), ...inFlight.current]), + new Set([...queued.current.keys(), ...inFlight.current.keys()]), ), ); }, [states]); @@ -90,17 +90,22 @@ export function usePullRequestFilesViewed(options: { 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; + const sentFrom = scope.current; + for (const file of batch) inFlight.current.set(file.path, file.viewed); void setFilesViewed({ environmentId, input: { ...reference, files: batch } }).then((result) => { - inFlight.current = NO_PATHS; + // Only what this request carried, and only where a later press has not taken the path over. + for (const file of batch) { + if (inFlight.current.get(file.path) === file.viewed) inFlight.current.delete(file.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. - setOverlay((current) => { - const next = new Map(current); - for (const path of sent) next.delete(path); - return next; - }); + // The host never heard these, so the ticks go back to whatever it last said, except on + // a path pressed again since, where the newer press is still waiting on its own request. + setOverlay((current) => + revertFileViewedOverlay(current, batch, new Set(queued.current.keys())), + ); toastManager.add({ type: "error", title: "Could not update viewed files" }); return; } @@ -113,15 +118,22 @@ export function usePullRequestFilesViewed(options: { 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(); - }, - [], - ); + // 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(); + inFlight.current = new Map(); + setOverlay(NO_OVERLAY); + }; + }, [scopeKey]); const setViewed = useCallback((path: string, viewed: boolean) => { setOverlay((current) => new Map(current).set(path, viewed)); @@ -143,5 +155,10 @@ export function usePullRequestFilesViewed(options: { [overlay, paths, states], ); - return { enabled, isViewed, isStale, setViewed, viewedCount }; + // 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 }), + [enabled, isStale, isViewed, setViewed, truncated, viewedCount], + ); } From 19a679c710c9ee4638b00047eaef177d6897a12a Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 20 Aug 2026 17:09:49 -0400 Subject: [PATCH 04/13] fix(web): a viewed tick redraws its file, and a partial count says that it is partial Signed-off-by: Yordis Prieto --- .../pullRequest/PullRequestCodeTab.tsx | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index fa9e5ed97026..772333d94350 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -485,6 +485,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 + ? `${filesViewed.isViewed(path) ? "v" : ""}${filesViewed.isStale(path) ? "s" : ""}` + : ""; const annotations: ReviewAnnotation[] = [...groups.values()].map((group) => ({ side: toViewerSide(group.side), @@ -500,7 +506,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 @@ -534,6 +540,7 @@ export function PullRequestCodeTab({ detail.reviewThreads, draft, files, + filesViewed, foldOverride, pendingComments, placedThreadIds, @@ -774,7 +781,6 @@ export function PullRequestCodeTab({ > setFileViewed(item.id, path, next === true)} /> {stale ? ( @@ -1118,8 +1124,22 @@ export function PullRequestCodeTab({ {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 ? ( From 447fd191c007ec85345588b713522107ee5954db Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 20 Aug 2026 17:09:50 -0400 Subject: [PATCH 05/13] style: plainer punctuation in the viewed files comments Signed-off-by: Yordis Prieto --- apps/server/src/pullRequest/gitHubPullRequestJson.ts | 12 ++++++------ .../components/pullRequest/pullRequestDiff.logic.ts | 2 +- packages/contracts/src/pullRequest.ts | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.ts index 773b3aa6700b..c77514b5f6d6 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.ts @@ -2244,10 +2244,10 @@ export function decodePullRequestFilesJson( /** * 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. + * 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) { @@ -2335,8 +2335,8 @@ export function decodePullRequestFilesViewedJson( * 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 + * 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. * diff --git a/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts b/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts index 8a6061c4e5c6..75b680be1790 100644 --- a/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts @@ -47,7 +47,7 @@ export function isFileDiffCollapsed( * 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 + * 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( diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index 86a8927d4461..598c7caf18ad 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -390,7 +390,7 @@ export const PullRequestCapabilities = Schema.Struct({ * 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 + * 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), @@ -814,7 +814,7 @@ export type PullRequestDiffFileContentsResult = typeof PullRequestDiffFileConten * 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 + * 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. */ From d085d082b29b36578cd8d86b551041fff3a929da Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 20 Aug 2026 17:22:06 -0400 Subject: [PATCH 06/13] fix(web): pressing the word beside the box no longer folds the file the wrong way Signed-off-by: Yordis Prieto --- .../sourceControl/githubGraphQlBudget.test.ts | 63 ------------------- .../src/sourceControl/githubGraphQlBudget.ts | 50 ++------------- .../pullRequest/PullRequestCodeTab.tsx | 9 ++- 3 files changed, 12 insertions(+), 110 deletions(-) diff --git a/apps/server/src/sourceControl/githubGraphQlBudget.test.ts b/apps/server/src/sourceControl/githubGraphQlBudget.test.ts index da8377b9aeb8..a166bf0dbbaf 100644 --- a/apps/server/src/sourceControl/githubGraphQlBudget.test.ts +++ b/apps/server/src/sourceControl/githubGraphQlBudget.test.ts @@ -186,67 +186,4 @@ 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("takes the host's own number over a write's guess, however high the guess was", () => - Effect.gen(function* () { - yield* TestClock.setTime(BEFORE_RESET); - const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; - yield* budget.observe("github.com", rateLimit(4_000)); - // A batch charged far more than it really spent would otherwise hold reads until the reset. - yield* budget.query("github.com", "mutation { f0: markFileAsViewed { id } }", { - estimatedCost: 3_900, - }); - yield* Effect.flip(budget.query("github.com", "query { viewer { login } }")); - - yield* budget.observe("github.com", rateLimit(3_990)); - - expect(yield* budget.query("github.com", "query { viewer { login } }")).toContain( - "rateLimit", - ); - }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), - ); - - it.effect("still ignores an out-of-order answer once the guess has been settled", () => - Effect.gen(function* () { - yield* TestClock.setTime(BEFORE_RESET); - const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; - yield* budget.observe("github.com", rateLimit(600)); - yield* budget.query("github.com", "mutation { f0: markFileAsViewed { id } }", { - estimatedCost: 50, - }); - // The host's own number settles the guess, and the answer behind it is stale again. - yield* budget.observe("github.com", rateLimit(513)); - yield* budget.observe("github.com", rateLimit(600)); - - 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 daa52bfb0d62..9c43de8e0586 100644 --- a/apps/server/src/sourceControl/githubGraphQlBudget.ts +++ b/apps/server/src/sourceControl/githubGraphQlBudget.ts @@ -15,12 +15,6 @@ interface GraphQlBudgetSnapshot { readonly limit: number; readonly remaining: number; readonly resetAtMs: number; - /** - * Points taken off `remaining` for writes the host has not answered for yet. A mutation cannot - * ask what it cost, so this is a guess, and while a guess is standing the host's own number is - * allowed to raise `remaining` again instead of being read as an out-of-order answer. - */ - readonly estimatedSpend: number; } export class GitHubGraphQlBudget extends Context.Service< @@ -29,14 +23,7 @@ export class GitHubGraphQlBudget extends Context.Service< readonly query: ( host: string, document: string, - 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; - }, + options?: { readonly allowReserve: boolean }, ) => Effect.Effect; readonly observe: (host: string, raw: string) => Effect.Effect; } @@ -72,9 +59,7 @@ function snapshotFrom(raw: string): GraphQlBudgetSnapshot | null { return null; } const resetAtMs = Date.parse(resetAt); - return Number.isFinite(resetAtMs) - ? { cost, limit, remaining, resetAtMs, estimatedSpend: 0 } - : null; + return Number.isFinite(resetAtMs) ? { cost, limit, remaining, resetAtMs } : null; } catch { return null; } @@ -97,29 +82,8 @@ 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 spend = Math.max(1, options?.estimatedCost ?? 1); - const next = new Map(current); - next.set(key, { - ...snapshot, - remaining: Math.max(0, snapshot.remaining - spend), - estimatedSpend: snapshot.estimatedSpend + spend, - }); - return next; - }); - return document; - } const retryAt = yield* Ref.modify(snapshots, (current) => { const key = hostKey(host); const snapshot = current.get(key); @@ -158,16 +122,10 @@ export const make = Effect.gen(function* () { const previous = current.get(key); // Concurrent reads can finish out of order. Quota only falls within one reset window, and // an answer from an older window must not replace the current one. - // - // Unless a write's guess is standing: that number was never the host's, and an estimate - // pitched too high would otherwise pause every read until the window reset, with the one - // answer that could correct it thrown away for looking stale. if ( previous !== undefined && (snapshot.resetAtMs < previous.resetAtMs || - (snapshot.resetAtMs === previous.resetAtMs && - previous.estimatedSpend === 0 && - snapshot.remaining >= previous.remaining)) + (snapshot.resetAtMs === previous.resetAtMs && snapshot.remaining >= previous.remaining)) ) { return current; } diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 772333d94350..15f61bfdd401 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -774,8 +774,11 @@ export function PullRequestCodeTab({ return ( {stat} - {/* The header itself folds the file, so the tick has to keep its press to itself. */} + {/* 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. */} (input: { @@ -1861,7 +1850,6 @@ export const make = Effect.gen(function* () { host: input.host, query: mutation.query, variables: { pullRequestId, ...mutation.variables }, - estimatedCost: input.files.length, }), ), ); From dad689d0a63a9db6b54bc5e407227430266e0916 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 20 Aug 2026 17:22:10 -0400 Subject: [PATCH 08/13] fix(web): a failed request no longer answers for a press a later one carries Signed-off-by: Yordis Prieto --- .../pullRequestFilesViewed.logic.test.ts | 20 +++++++---- .../pullRequestFilesViewed.logic.ts | 12 ++++--- .../pullRequest/usePullRequestFilesViewed.ts | 35 +++++++++++-------- 3 files changed, 41 insertions(+), 26 deletions(-) diff --git a/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.test.ts index 78dd8d1298ee..88d3f5708c09 100644 --- a/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.test.ts @@ -105,28 +105,36 @@ describe("revertFileViewedOverlay", () => { { 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 carried", () => { + 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, new Set()).size).toBe(0); + expect(revertFileViewedOverlay(overlay, batch, both).size).toBe(0); }); it("leaves a press the reader made after the request went out", () => { - // The second press is queued behind a request of its own, so the first one failing says - // nothing about it. + // 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(["a.ts"])); + 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, new Set())).toBe(overlay); + 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 index 04c03ba424fa..14d3d462ec7b 100644 --- a/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts @@ -77,18 +77,20 @@ export function settleFileViewedOverlay( /** * The overlay with a failed request's presses taken back. * - * Only the presses that request carried, and only where the checkbox still shows them: a path - * the reader has pressed again since is waiting on a request of its own, and putting that box - * back to the host's answer would take a press out from under the reader's hand. + * `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 }>, - superseded: ReadonlySet, + owned: ReadonlySet, ): FileViewedOverlay { const next = new Map(overlay); for (const { path, viewed } of batch) { - if (superseded.has(path)) continue; + if (!owned.has(path)) continue; if (next.get(path) === viewed) next.delete(path); } return next.size === overlay.size ? overlay : next; diff --git a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts index b84028ee7352..fc1ecb671881 100644 --- a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts +++ b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts @@ -63,10 +63,14 @@ export function usePullRequestFilesViewed(options: { 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. + // 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 inFlight = 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 @@ -80,7 +84,7 @@ export function usePullRequestFilesViewed(options: { settleFileViewedOverlay( current, states, - new Set([...queued.current.keys(), ...inFlight.current.keys()]), + new Set([...queued.current.keys(), ...sentBy.current.keys()]), ), ); }, [states]); @@ -91,21 +95,22 @@ export function usePullRequestFilesViewed(options: { if (batch.length === 0) return; queued.current = new Map(); const sentFrom = scope.current; - for (const file of batch) inFlight.current.set(file.path, file.viewed); + const request = ++requests.current; + for (const file of batch) sentBy.current.set(file.path, request); void setFilesViewed({ environmentId, input: { ...reference, files: batch } }).then((result) => { - // Only what this request carried, and only where a later press has not taken the path over. - for (const file of batch) { - if (inFlight.current.get(file.path) === file.viewed) inFlight.current.delete(file.path); - } + 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, except on - // a path pressed again since, where the newer press is still waiting on its own request. - setOverlay((current) => - revertFileViewedOverlay(current, batch, new Set(queued.current.keys())), - ); + // 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)); toastManager.add({ type: "error", title: "Could not update viewed files" }); return; } @@ -130,7 +135,7 @@ export function usePullRequestFilesViewed(options: { flushScope(); } queued.current = new Map(); - inFlight.current = new Map(); + sentBy.current = new Map(); setOverlay(NO_OVERLAY); }; }, [scopeKey]); From 962864338f56630528864b03a374de807515b77d Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 20 Aug 2026 17:38:40 -0400 Subject: [PATCH 09/13] fix(web): a viewed tick no longer rebuilds every header on screen A press moved the whole viewed view, and every file header on screen was memoized on it, so one tick cost a rebuild of all of them. The same mark also has to say whether the control is offered at all, or a capability arriving after the first paint leaves the headers without a box. Signed-off-by: Yordis Prieto --- .../pullRequest/PullRequestCodeTab.tsx | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 15f61bfdd401..36326e6e2f78 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -489,7 +489,7 @@ export function PullRequestCodeTab({ // 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 - ? `${filesViewed.isViewed(path) ? "v" : ""}${filesViewed.isStale(path) ? "s" : ""}` + ? `e${filesViewed.isViewed(path) ? "v" : ""}${filesViewed.isStale(path) ? "s" : ""}` : ""; const annotations: ReviewAnnotation[] = [...groups.values()].map((group) => ({ @@ -747,6 +747,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; @@ -768,9 +777,10 @@ export function PullRequestCodeTab({ className="font-mono text-[11px]" /> ); - if (!filesViewed.enabled) return stat; - const viewed = filesViewed.isViewed(path); - const stale = filesViewed.isStale(path); + const viewedFiles = filesViewedRef.current; + if (!viewedFiles.enabled) return stat; + const viewed = viewedFiles.isViewed(path); + const stale = viewedFiles.isStale(path); return ( {stat} @@ -784,7 +794,7 @@ export function PullRequestCodeTab({ > setFileViewed(item.id, path, next === true)} + onCheckedChange={(next) => setFileViewedRef.current(item.id, path, next === true)} /> {stale ? ( @@ -802,7 +812,7 @@ export function PullRequestCodeTab({ ); }, - [filesViewed, omittedFileStats, setFileViewed], + [omittedFileStats], ); const diffViewOptions = useMemo( From 6b44e5156469157681c5c0b91b112f5ad12ed3f7 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 20 Aug 2026 17:38:47 -0400 Subject: [PATCH 10/13] fix(web): the viewed box says what it is for out loud It borrowed its name from the label beside it, and that label turns into "Changed" once the file has been pushed to, leaving a reader who cannot see it with no idea what the box does. Signed-off-by: Yordis Prieto --- apps/web/src/components/pullRequest/PullRequestCodeTab.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 36326e6e2f78..815f00a844fe 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -792,7 +792,10 @@ export function PullRequestCodeTab({ className="flex cursor-pointer select-none items-center gap-1.5 text-[11px] text-muted-foreground" onClick={(event) => event.stopPropagation()} > + {/* Named here rather than by the label, whose text turns into "Changed" once the + file has been pushed to. */} setFileViewedRef.current(item.id, path, next === true)} /> From aa7a828204f2703c189cde5c9c3eba0a2b925ab0 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 27 Aug 2026 20:02:24 -0400 Subject: [PATCH 11/13] fix(web): a refreshed review re-asks for the ticks The button exists for a reader who can see that what they are looking at is behind, so leaving one part of the page on the last read defeats the point of pressing it. A push since that read is exactly when the mark beside a ticked file stops being true. Signed-off-by: Yordis Prieto --- .../pullRequest/PullRequestCodeTab.tsx | 20 +++++++++------- .../pullRequest/usePullRequestFilesViewed.ts | 23 +++++++++++++++++-- 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 815f00a844fe..23978183e825 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -320,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); @@ -409,7 +402,18 @@ export function PullRequestCodeTab({ enabled: detail.capabilities.viewedFiles === true, paths: filePaths, }); - const { setViewed } = filesViewed; + 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. diff --git a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts index fc1ecb671881..06e7b9dfd2af 100644 --- a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts +++ b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts @@ -37,6 +37,11 @@ export interface PullRequestFilesViewedView { 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; } /** @@ -140,6 +145,12 @@ export function usePullRequestFilesViewed(options: { }; }, [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); @@ -163,7 +174,15 @@ export function usePullRequestFilesViewed(options: { // 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 }), - [enabled, isStale, isViewed, setViewed, truncated, viewedCount], + () => ({ + enabled, + isViewed, + isStale, + setViewed, + viewedCount, + truncated, + refresh: refreshFromHost, + }), + [enabled, isStale, isViewed, refreshFromHost, setViewed, truncated, viewedCount], ); } From 5f080475793566c93ba87312092e10ad7c996c5e Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 27 Aug 2026 20:21:29 -0400 Subject: [PATCH 12/13] fix(web): a superseded write no longer reports a failure An error the reader cannot act on, about a press they have already replaced, reads as their current tick having been lost when it has not. Signed-off-by: Yordis Prieto --- .../components/pullRequest/usePullRequestFilesViewed.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts index 06e7b9dfd2af..f8b727e9b66f 100644 --- a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts +++ b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts @@ -116,7 +116,12 @@ export function usePullRequestFilesViewed(options: { // 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)); - toastManager.add({ type: "error", title: "Could not update viewed files" }); + // Nothing here was still this request's to answer for, so nothing on screen went back. + // A later press carries every one of these paths now, and it is the one that gets to say + // whether the reader's tick reached the host. + if (owned.size > 0) { + toastManager.add({ type: "error", title: "Could not update viewed files" }); + } return; } refresh(); From 3c279bf305afc0a5cb83791d866c2a0ebb0314ba Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 27 Aug 2026 20:41:43 -0400 Subject: [PATCH 13/13] fix(web): a dropped connection no longer reports a rejected write Signed-off-by: Yordis Prieto --- .../pullRequest/usePullRequestFilesViewed.ts | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts index f8b727e9b66f..573b823b1170 100644 --- a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts +++ b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts @@ -1,3 +1,4 @@ +import { isAtomCommandInterrupted } from "@t3tools/client-runtime/state/runtime"; import type { EnvironmentId, PullRequestRef } from "@t3tools/contracts"; import { useCallback, useEffect, useMemo, useRef, useState } from "react"; @@ -66,7 +67,9 @@ export function usePullRequestFilesViewed(options: { 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); + 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 @@ -116,10 +119,11 @@ export function usePullRequestFilesViewed(options: { // 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)); - // Nothing here was still this request's to answer for, so nothing on screen went back. - // A later press carries every one of these paths now, and it is the one that gets to say - // whether the reader's tick reached the host. - if (owned.size > 0) { + // 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;