From c6f382a65b57270a8352ba39f0062f40577af106 Mon Sep 17 00:00:00 2001 From: Victor Vogel Date: Thu, 22 Oct 2026 16:44:08 -0500 Subject: [PATCH 1/9] test(server): pin a re-cleared file to the head's current revision Co-Authored-By: Claude Opus 5.5 --- .../pullRequest/PullRequestService.test.ts | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index a438ac780f43f..754fc89f10049 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -3438,6 +3438,32 @@ it.effect("reports a file pushed to since it was cleared as changed", () => }), ); +it.effect("clears a file against what the head has now, not what it had a moment ago", () => + Effect.gen(function* () { + const revisions = new Map([["src/a.ts", "blob-a"]]); + const service = yield* environmentViewedService(revisions, []); + + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/a.ts", viewed: true }], + }); + revisions.set("src/a.ts", "blob-a-again"); + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/a.ts", viewed: false }], + }); + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/a.ts", viewed: true }], + }); + yield* service.invalidate({ reference: GITLAB_REFERENCE }); + + assert.deepStrictEqual((yield* service.filesViewed(GITLAB_REFERENCE)).files, [ + { path: "src/a.ts", state: "viewed" }, + ]); + }), +); + it.effect("keeps a deleted file cleared, which the head has no version of at all", () => Effect.gen(function* () { const service = yield* environmentViewedService(new Map(), []); From 8ca2b7068092b93c766909974befa8a07960136b Mon Sep 17 00:00:00 2001 From: Victor Vogel Date: Thu, 22 Oct 2026 16:44:11 -0500 Subject: [PATCH 2/9] fix(server): refresh file revisions before recording viewed marks A press asks for fresh revisions so it never stores one the head has moved off, but a revision read in the last 60 seconds was returned before that request was honored. Co-Authored-By: Claude Opus 5.5 --- apps/server/src/pullRequest/pullRequestViewedFiles.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/apps/server/src/pullRequest/pullRequestViewedFiles.ts b/apps/server/src/pullRequest/pullRequestViewedFiles.ts index fc2913e3efd43..0ffc81e5e6ef2 100644 --- a/apps/server/src/pullRequest/pullRequestViewedFiles.ts +++ b/apps/server/src/pullRequest/pullRequestViewedFiles.ts @@ -142,10 +142,9 @@ const makeFileRevisions = (dependencies: FileRevisionsDependencies) => { return Effect.flatMap(Clock.currentTimeMillis, (now) => { const key = fileRevisionsKey(project, ref); const held = heldFileRevisionsFor(key, paths, now); - if (held === null) return fetch; + if (held === null || freshness === "fresh") return fetch; if (now - held.at <= Duration.toMillis(FILE_REVISIONS_CACHE_TTL)) return Effect.succeed(held.revisions); - if (freshness === "fresh") return fetch; if (refreshingFileRevisions.has(key)) return Effect.succeed(held.revisions); // Its own fiber rather than a child: the caller has been answered and is gone before this // lands. From 2d6909a663a3f036f34a8aa1a5d899be02af8311 Mon Sep 17 00:00:00 2001 From: Victor Vogel Date: Fri, 23 Oct 2026 04:06:28 -0500 Subject: [PATCH 3/9] fix: record the file version a viewed press was shown A press stored whatever the head had when the write arrived, so a push landing after the diff was drawn, or inside the 400 ms batch window, was stamped as seen. The web press now sends the blob id from the rendered diff's index line and the server stores that one. A press that names no version still records the head's version at the press. Co-Authored-By: Claude Opus 5.5 --- .../pullRequest/PullRequestService.test.ts | 77 +++++++++++++++++++ .../src/pullRequest/pullRequestViewedFiles.ts | 9 ++- .../pullRequest/PullRequestCodeTab.tsx | 23 +++++- .../pullRequestFilesViewed.logic.ts | 14 +++- .../pullRequest/usePullRequestFilesViewed.ts | 21 ++--- packages/contracts/src/pullRequest.ts | 5 ++ 6 files changed, 131 insertions(+), 18 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 754fc89f10049..355dd2f6ec850 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -45,6 +45,7 @@ import * as GitHubPullRequestApi from "@t3tools/source-control-github/server/Git import * as GitHubPullRequestProvider from "@t3tools/source-control-github/server/GitHubPullRequestProvider"; import * as GitHubQuota from "@t3tools/source-control-github/server/GitHubQuota"; import * as TestSourceControlHost from "@t3tools/source-control-testing/TestSourceControlHost"; +import { parseDiffFileRevisions } from "@t3tools/source-control-core/server/diffFileRevisions"; import { PullRequestProviderError, type ProviderChangeRequest, @@ -3464,6 +3465,82 @@ it.effect("clears a file against what the head has now, not what it had a moment }), ); +// The diff names each file's version on its `index` line, which is what a reader's press sends. +const environmentViewedDiffService = (revisions: Map) => + makeService({ + projects: [ + project({ + id: "p1", + title: "on gitlab", + workspaceRoot: "/a", + repository: "group/project", + provider: SourceControlProviderKind.make("gitlab"), + }), + ], + providers: [ + { + ...environmentViewedProvider(revisions, []), + getDiff: () => + Effect.sync(() => ({ + patch: [ + "diff --git a/src/a.ts b/src/a.ts", + `index 0000000..${revisions.get("src/a.ts")} 100644`, + "--- a/src/a.ts", + "+++ b/src/a.ts", + "@@ -0,0 +1 @@", + "+export const a = 1;", + "", + ].join("\n"), + truncated: false, + nextCursor: null, + })), + }, + ], + }); + +const shownRevision = (patch: string) => parseDiffFileRevisions(patch).get("src/a.ts") ?? ""; + +it.effect("records the version the reader was shown, not one pushed after the diff was drawn", () => + Effect.gen(function* () { + const revisions = new Map([["src/a.ts", "3464df0"]]); + const service = yield* environmentViewedDiffService(revisions); + + const shown = shownRevision((yield* service.diff(GITLAB_REFERENCE)).patch); + revisions.set("src/a.ts", "e9ed029"); + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/a.ts", viewed: true, revision: shown }], + }); + yield* service.invalidate({ reference: GITLAB_REFERENCE }); + + assert.deepStrictEqual((yield* service.filesViewed(GITLAB_REFERENCE)).files, [ + { path: "src/a.ts", state: "dismissed" }, + ]); + }), +); + +it.effect("records a pushed version as seen once the diff shows it", () => + Effect.gen(function* () { + const revisions = new Map([["src/a.ts", "3464df0"]]); + const service = yield* environmentViewedDiffService(revisions); + + yield* service.diff(GITLAB_REFERENCE); + revisions.set("src/a.ts", "e9ed029"); + yield* service.invalidate({ reference: GITLAB_REFERENCE }); + const shown = shownRevision((yield* service.diff(GITLAB_REFERENCE)).patch); + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/a.ts", viewed: true, revision: shown }], + }); + yield* service.invalidate({ reference: GITLAB_REFERENCE }); + + assert.strictEqual(shown, "e9ed029"); + assert.deepStrictEqual((yield* service.filesViewed(GITLAB_REFERENCE)).files, [ + { path: "src/a.ts", state: "viewed" }, + ]); + }), +); + it.effect("keeps a deleted file cleared, which the head has no version of at all", () => Effect.gen(function* () { const service = yield* environmentViewedService(new Map(), []); diff --git a/apps/server/src/pullRequest/pullRequestViewedFiles.ts b/apps/server/src/pullRequest/pullRequestViewedFiles.ts index 0ffc81e5e6ef2..f72fca60e45a2 100644 --- a/apps/server/src/pullRequest/pullRequestViewedFiles.ts +++ b/apps/server/src/pullRequest/pullRequestViewedFiles.ts @@ -277,7 +277,12 @@ export const make = (dependencies: Dependencies) => { input: PullRequestSetFilesViewedInput, ): Effect.fn.Return { const viewer = yield* requiredViewerOf(project, "setFilesViewed"); - const cleared = input.files.filter((file) => file.viewed).map((file) => file.path); + // A press that names the version it was shown records that version, so a push landing + // after the diff was drawn reads as changed rather than as seen. Only a press that names + // none asks the host what the head has. + const cleared = input.files + .filter((file) => file.viewed && file.revision === undefined) + .map((file) => file.path); const revisions = cleared.length === 0 ? null @@ -298,7 +303,7 @@ export const make = (dependencies: Dependencies) => { // out to have a version after all. files: input.files.map((file) => ({ path: file.path, - revision: revisions?.get(file.path) ?? null, + revision: file.revision ?? revisions?.get(file.path) ?? null, viewed: file.viewed, })), viewedAt, diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index ee152af458199..9974624d366a3 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -168,7 +168,12 @@ interface FileViewedToggleState { readonly enabled: boolean; readonly isViewed: (path: string) => boolean; readonly isStale: (path: string) => boolean; - readonly setFileViewed: (fileKey: string, path: string, viewed: boolean) => void; + readonly setFileViewed: ( + fileKey: string, + path: string, + viewed: boolean, + revision: string | undefined, + ) => void; } // Header portals are memoized on the render props; the viewed toggle reads its state through @@ -178,10 +183,12 @@ const FileViewedToggleContext = createContext(null function FileViewedToggle({ fileKey, path, + revision, stat, }: { readonly fileKey: string; readonly path: string; + readonly revision: string | undefined; readonly stat: ReactNode; }) { const viewedFiles = use(FileViewedToggleContext); @@ -199,7 +206,9 @@ function FileViewedToggle({ viewedFiles.setFileViewed(fileKey, path, next === true)} + onCheckedChange={(next) => + viewedFiles.setFileViewed(fileKey, path, next === true, revision) + } /> {stale ? ( @@ -679,8 +688,13 @@ function PullRequestCodeTab({ return next; }); - const setFileViewed = (fileKey: string, path: string, viewed: boolean) => { - setViewed(path, viewed); + const setFileViewed = ( + fileKey: string, + path: string, + viewed: boolean, + revision: string | undefined, + ) => { + setViewed(path, viewed, revision); setToggledFiles((current) => toggleFileDiffFoldForViewed(fileKey, viewed, effectiveFoldOverride, current), ); @@ -818,6 +832,7 @@ function PullRequestCodeTab({ { - return [...overlay].map(([path, viewed]) => ({ path, viewed })); + presses: ReadonlyMap, +): ReadonlyArray<{ readonly path: string; readonly viewed: boolean; readonly revision?: string }> { + return [...presses].map(([path, { viewed, revision }]) => + revision === undefined ? { path, viewed } : { path, viewed, revision }, + ); } diff --git a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts index f1fc4bb9f689d..6377ab4a3d49c 100644 --- a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts +++ b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts @@ -16,6 +16,7 @@ import { toFileViewedBatch, toFileViewedStates, type FileViewedOverlay, + type FileViewedPress, type FileViewedStates, } from "./pullRequestFilesViewed.logic"; @@ -27,7 +28,7 @@ export interface PullRequestFilesViewedView { readonly enabled: boolean; readonly isViewed: (path: string) => boolean; readonly isStale: (path: string) => boolean; - readonly setViewed: (path: string, viewed: boolean) => void; + readonly setViewed: (path: string, viewed: boolean, revision: string | undefined) => void; readonly truncated: boolean; readonly error: string | null; readonly refresh: () => void; @@ -51,7 +52,7 @@ export function usePullRequestFilesViewed(options: { reportFailure: false, }); - const queued = useRef>(new Map()); + const queued = useRef>(new Map()); const sentBy = useRef>(new Map()); const requests = useRef(0); const flushTimer = useRef | null>(null); @@ -139,13 +140,15 @@ export function usePullRequestFilesViewed(options: { }, [scopeKey]); // Handed to every file header the viewer draws; a new identity would rebuild all of them. - const setViewed = useStableCallback((path: string, viewed: boolean) => { - setOverlay((current) => new Map(current).set(path, viewed)); - queued.current.set(path, viewed); - if (flushTimer.current !== null) clearTimeout(flushTimer.current); - const target = { environmentId, reference }; - flushTimer.current = setTimeout(() => flush(target), FLUSH_DELAY_MS); - }); + const setViewed = useStableCallback( + (path: string, viewed: boolean, revision: string | undefined) => { + setOverlay((current) => new Map(current).set(path, viewed)); + queued.current.set(path, { viewed, revision }); + if (flushTimer.current !== null) clearTimeout(flushTimer.current); + const target = { environmentId, reference }; + flushTimer.current = setTimeout(() => flush(target), FLUSH_DELAY_MS); + }, + ); return { enabled, diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index a68f5394b0dd1..dc025df30e80f 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -716,6 +716,11 @@ export const PullRequestSetFilesViewedInput = Schema.Struct({ Schema.Struct({ path: FilePath, viewed: Schema.Boolean, + /** + * The version of the file the reader was shown, as the diff's `index` line names it. + * Absent where the diff names none; the server then records what the head has at the press. + */ + revision: Schema.optional(TrimmedNonEmptyString.check(Schema.isMaxLength(256))), }), ).check(Schema.isMaxLength(MAX_FILES_VIEWED_PRESSES)), }); From f1e6b708200d5dcb607bd9e3e6c539f23118087e Mon Sep 17 00:00:00 2001 From: Victor Vogel Date: Fri, 23 Oct 2026 06:12:45 -0500 Subject: [PATCH 4/9] fix(web): a viewed tick in one commit's diff records the head's version One commit's diff names that commit's blob, not the head's. Sending it made any file a later commit changed read back as Changed right after the tick. Commit-scoped presses now send no version and keep the press-time head read. Co-Authored-By: Claude Opus 5.5 --- apps/server/src/pullRequest/PullRequestService.test.ts | 2 +- apps/web/src/components/pullRequest/PullRequestCodeTab.tsx | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 355dd2f6ec850..153851878fc49 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -3465,7 +3465,7 @@ it.effect("clears a file against what the head has now, not what it had a moment }), ); -// The diff names each file's version on its `index` line, which is what a reader's press sends. +// A diff with `index` lines, as Forgejo and Bitbucket send. The GitLab project only selects environment-kept marks; GitLab's own patches name no version. const environmentViewedDiffService = (revisions: Map) => makeService({ projects: [ diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 9974624d366a3..f7697b00c04c1 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -832,7 +832,8 @@ function PullRequestCodeTab({ Date: Mon, 26 Oct 2026 11:05:33 +0000 Subject: [PATCH 5/9] refactor(pull-request): simplify viewed revision regression coverage --- .../pullRequest/PullRequestService.test.ts | 76 +++++-------------- .../src/pullRequest/pullRequestViewedFiles.ts | 3 - .../pullRequestFilesViewed.logic.ts | 1 - packages/contracts/src/pullRequest.ts | 5 +- 4 files changed, 20 insertions(+), 65 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index b7ffb4a88cddc..4552e73ac6cdd 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -31,7 +31,6 @@ import type { SourceControlProvider } from "@t3tools/source-control-core/server/ import { isSshRemoteUrl } from "@t3tools/shared/sourceControl"; import * as SourceControlRateLimit from "@t3tools/source-control-core/server/SourceControlRateLimit"; import { fakePullRequestProvider } from "@t3tools/source-control-testing/fakeSourceControlDriver"; -import { parseDiffFileRevisions } from "@t3tools/source-control-core/server/diffFileRevisions"; import { PullRequestProviderError, type ProviderChangeRequest, @@ -3685,78 +3684,41 @@ it.effect("clears a file against what the head has now, not what it had a moment }), ); -// A diff with `index` lines, as Forgejo and Bitbucket send. The GitLab project only selects environment-kept marks; GitLab's own patches name no version. -const environmentViewedDiffService = (revisions: Map) => - makeService({ - projects: [ - project({ - id: "p1", - title: "on gitlab", - workspaceRoot: "/a", - repository: "group/project", - provider: SourceControlProviderKind.make("gitlab"), - }), - ], - providers: [ - { - ...environmentViewedProvider(revisions, []), - getDiff: () => - Effect.sync(() => ({ - patch: [ - "diff --git a/src/a.ts b/src/a.ts", - `index 0000000..${revisions.get("src/a.ts")} 100644`, - "--- a/src/a.ts", - "+++ b/src/a.ts", - "@@ -0,0 +1 @@", - "+export const a = 1;", - "", - ].join("\n"), - truncated: false, - nextCursor: null, - })), - }, - ], - }); - -const shownRevision = (patch: string) => parseDiffFileRevisions(patch).get("src/a.ts") ?? ""; - -it.effect("records the version the reader was shown, not one pushed after the diff was drawn", () => +it.effect("records shown revisions until the reader ticks the pushed version", () => Effect.gen(function* () { - const revisions = new Map([["src/a.ts", "3464df0"]]); - const service = yield* environmentViewedDiffService(revisions); + const asked: Array> = []; + const service = yield* environmentViewedService( + new Map([ + ["src/a.ts", "pushed-a"], + ["src/b.ts", "current-b"], + ]), + asked, + ); - const shown = shownRevision((yield* service.diff(GITLAB_REFERENCE)).patch); - revisions.set("src/a.ts", "e9ed029"); yield* service.setFilesViewed({ ...GITLAB_REFERENCE, - files: [{ path: "src/a.ts", viewed: true, revision: shown }], + files: [ + { path: "src/a.ts", viewed: true, revision: "shown-a" }, + { path: "src/b.ts", viewed: true }, + ], }); + assert.deepStrictEqual(asked, [["src/b.ts"]]); yield* service.invalidate({ reference: GITLAB_REFERENCE }); - assert.deepStrictEqual((yield* service.filesViewed(GITLAB_REFERENCE)).files, [ { path: "src/a.ts", state: "dismissed" }, + { path: "src/b.ts", state: "viewed" }, ]); - }), -); -it.effect("records a pushed version as seen once the diff shows it", () => - Effect.gen(function* () { - const revisions = new Map([["src/a.ts", "3464df0"]]); - const service = yield* environmentViewedDiffService(revisions); - - yield* service.diff(GITLAB_REFERENCE); - revisions.set("src/a.ts", "e9ed029"); - yield* service.invalidate({ reference: GITLAB_REFERENCE }); - const shown = shownRevision((yield* service.diff(GITLAB_REFERENCE)).patch); + const reads = asked.length; yield* service.setFilesViewed({ ...GITLAB_REFERENCE, - files: [{ path: "src/a.ts", viewed: true, revision: shown }], + files: [{ path: "src/a.ts", viewed: true, revision: "pushed-a" }], }); + assert.strictEqual(asked.length, reads); yield* service.invalidate({ reference: GITLAB_REFERENCE }); - - assert.strictEqual(shown, "e9ed029"); assert.deepStrictEqual((yield* service.filesViewed(GITLAB_REFERENCE)).files, [ { path: "src/a.ts", state: "viewed" }, + { path: "src/b.ts", state: "viewed" }, ]); }), ); diff --git a/apps/server/src/pullRequest/pullRequestViewedFiles.ts b/apps/server/src/pullRequest/pullRequestViewedFiles.ts index 5d465166f2ea3..efb0948f7c16d 100644 --- a/apps/server/src/pullRequest/pullRequestViewedFiles.ts +++ b/apps/server/src/pullRequest/pullRequestViewedFiles.ts @@ -259,9 +259,6 @@ export const make = (dependencies: Dependencies) => { input: PullRequestSetFilesViewedInput, ): Effect.fn.Return { const viewer = yield* requiredViewerOf(project, "setFilesViewed"); - // A press that names the version it was shown records that version, so a push landing - // after the diff was drawn reads as changed rather than as seen. Only a press that names - // none asks the host what the head has. const cleared = input.files .filter((file) => file.viewed && file.revision === undefined) .map((file) => file.path); diff --git a/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts b/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts index 4dcc784af5625..89874004a9220 100644 --- a/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts @@ -60,7 +60,6 @@ export function revertFileViewedOverlay( export interface FileViewedPress { readonly viewed: boolean; - /** The version the diff on screen named for the file, which the server records as seen. */ readonly revision: string | undefined; } diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index ade1e682ef2a4..87d439978aba0 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -711,10 +711,7 @@ export const PullRequestSetFilesViewedInput = Schema.Struct({ Schema.Struct({ path: FilePath, viewed: Schema.Boolean, - /** - * The version of the file the reader was shown, as the diff's `index` line names it. - * Absent where the diff names none; the server then records what the head has at the press. - */ + /** The displayed revision; omitted clients fall back to a fresh head read. */ revision: Schema.optional(TrimmedNonEmptyString.check(Schema.isMaxLength(256))), }), ).check(Schema.isMaxLength(MAX_FILES_VIEWED_PRESSES)), From 079989be6bbe55d730a2fdefce2f419c82bdcdb1 Mon Sep 17 00:00:00 2001 From: maria-rcks Date: Mon, 26 Oct 2026 11:19:17 +0000 Subject: [PATCH 6/9] fix(pull-request): refresh comparisons after displayed revision marks --- .../src/pullRequest/PullRequestService.test.ts | 16 +++++++--------- .../server/src/pullRequest/PullRequestService.ts | 13 +++++++++++-- .../src/pullRequest/pullRequestViewedFiles.ts | 4 ++-- 3 files changed, 20 insertions(+), 13 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 4552e73ac6cdd..81b49ffe0654a 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -3687,13 +3687,11 @@ it.effect("clears a file against what the head has now, not what it had a moment it.effect("records shown revisions until the reader ticks the pushed version", () => Effect.gen(function* () { const asked: Array> = []; - const service = yield* environmentViewedService( - new Map([ - ["src/a.ts", "pushed-a"], - ["src/b.ts", "current-b"], - ]), - asked, - ); + const revisions = new Map([ + ["src/a.ts", "pushed-a"], + ["src/b.ts", "current-b"], + ]); + const service = yield* environmentViewedService(revisions, asked); yield* service.setFilesViewed({ ...GITLAB_REFERENCE, @@ -3710,12 +3708,12 @@ it.effect("records shown revisions until the reader ticks the pushed version", ( ]); const reads = asked.length; + revisions.set("src/a.ts", "pushed-a-again"); yield* service.setFilesViewed({ ...GITLAB_REFERENCE, - files: [{ path: "src/a.ts", viewed: true, revision: "pushed-a" }], + files: [{ path: "src/a.ts", viewed: true, revision: "pushed-a-again" }], }); assert.strictEqual(asked.length, reads); - yield* service.invalidate({ reference: GITLAB_REFERENCE }); assert.deepStrictEqual((yield* service.filesViewed(GITLAB_REFERENCE)).files, [ { path: "src/a.ts", state: "viewed" }, { path: "src/b.ts", state: "viewed" }, diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 6a9c721e3a127..91163f9383ece 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -1919,7 +1919,14 @@ export const make = Effect.gen(function* () { // 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. - Effect.ensuring(Effect.sync(() => bumpFilesViewedEpoch(ref))), + Effect.ensuring( + Effect.sync(() => { + bumpFilesViewedEpoch(ref); + if (input.files.some((file) => file.viewed && file.revision !== undefined)) { + bumpEpoch(fileRevisionEpochs, ref); + } + }), + ), ), ), ); @@ -2791,6 +2798,7 @@ export const make = Effect.gen(function* () { const filesViewedEpoch = (ref: PullRequestRef) => filesViewedEpochs.get(refScope(ref)) ?? 0; const bumpFilesViewedEpoch = (ref: PullRequestRef) => bumpEpoch(filesViewedEpochs, ref); + const fileRevisionEpochs = new Map(); let everyFileRevisionEpoch = 0; const viewedFiles = ViewedFiles.make({ filesViewedStore, @@ -2799,7 +2807,8 @@ export const make = Effect.gen(function* () { toPullRequestError, runFork, refEpoch, - fileRevisionsEpoch: () => everyFileRevisionEpoch, + fileRevisionsEpoch: (ref) => + `${everyFileRevisionEpoch}:${fileRevisionEpochs.get(refScope(ref)) ?? 0}`, }); const filtersOfKey = ( diff --git a/apps/server/src/pullRequest/pullRequestViewedFiles.ts b/apps/server/src/pullRequest/pullRequestViewedFiles.ts index efb0948f7c16d..b0e343d528e1c 100644 --- a/apps/server/src/pullRequest/pullRequestViewedFiles.ts +++ b/apps/server/src/pullRequest/pullRequestViewedFiles.ts @@ -30,7 +30,7 @@ const MAX_FILE_REVISION_PATHS = 1_000; interface FileRevisionsDependencies { readonly runFork: (effect: Effect.Effect) => unknown; readonly refEpoch: (ref: PullRequestRef) => number; - readonly fileRevisionsEpoch: () => number; + readonly fileRevisionsEpoch: (ref: PullRequestRef) => string; readonly toPullRequestError: ( operation: string, ) => (error: PullRequestProviderError) => PullRequestError; @@ -50,7 +50,7 @@ const makeFileRevisions = (dependencies: FileRevisionsDependencies) => { const fileRevisionsKey = (project: SupportedProject, ref: PullRequestRef) => [ refEpoch({ ...ref, host: project.host, repository: project.repository }), - fileRevisionsEpoch(), + fileRevisionsEpoch({ ...ref, host: project.host, repository: project.repository }), ref.projectId, project.repository.trim().toLowerCase(), ref.number, From 63749ff80f79fc6b185a8a79b2f37a3c65167424 Mon Sep 17 00:00:00 2001 From: maria-rcks Date: Mon, 26 Oct 2026 11:26:29 +0000 Subject: [PATCH 7/9] fix: let bitbucket revision comparisons read fresh pushes --- .../server/BitbucketPullRequestApi.test.ts | 18 ++++++++++ .../src/server/BitbucketPullRequestApi.ts | 33 ++----------------- 2 files changed, 20 insertions(+), 31 deletions(-) diff --git a/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.test.ts b/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.test.ts index bf5ea9d944738..355199aa569de 100644 --- a/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.test.ts +++ b/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.test.ts @@ -248,6 +248,24 @@ layer("BitbucketPullRequestApi.layer", (it) => { }), ); + it.effect("reads the pushed revision on consecutive comparison reads", () => + Effect.gen(function* () { + for (const revision of ["2222222", "3333333"]) { + mockedRequest.mockReturnValueOnce( + Effect.succeed( + response( + `diff --git a/a.ts b/a.ts\nindex 1111111..${revision} 100644\n@@ -1 +1 @@\n-a\n+b\n`, + ), + ), + ); + } + const api = yield* BitbucketPullRequestApi.BitbucketPullRequestApi; + const input = { repository: "acme/web", number: 73, paths: ["a.ts"] }; + assert.strictEqual((yield* api.getFileRevisions(input)).revisions.get("a.ts"), "2222222"); + assert.strictEqual((yield* api.getFileRevisions(input)).revisions.get("a.ts"), "3333333"); + }), + ); + it.effect("says nothing about the files past the end of a patch it could not read whole", () => Effect.gen(function* () { mockedRequest.mockReturnValueOnce( diff --git a/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.ts b/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.ts index de4b625bd9cbc..b9a97f0c5fa71 100644 --- a/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.ts +++ b/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.ts @@ -1,8 +1,5 @@ -import * as Cache from "effect/Cache"; import * as Context from "effect/Context"; -import * as Duration from "effect/Duration"; import * as Effect from "effect/Effect"; -import * as Exit from "effect/Exit"; import * as Layer from "effect/Layer"; import * as Result from "effect/Result"; import * as Schema from "effect/Schema"; @@ -121,12 +118,6 @@ const MAX_LIST_PAGES = 10; const CONVERSATION_PAGE_SIZE = 50; const CONVERSATION_PAGES = 10; const DIFF_MAX_BYTES = 8 * 1024 * 1024; -/** Must expire before the caller's version cache so refreshes reach Bitbucket. */ -const REVISION_PATCH_TTL = Duration.seconds(5); -const REVISION_PATCH_CAPACITY = 16; -const encodeRevisionKey = Schema.encodeSync( - Schema.fromJsonString(Schema.Tuple([Schema.String, Schema.Number])), -); export interface BitbucketPullRequestBatch { readonly items: ReadonlyArray; readonly truncated: boolean; @@ -511,23 +502,6 @@ export const make = Effect.gen(function* () { ), ); - // Cache revisions, not large patch bodies that need reparsing. - const revisionPatches = yield* Cache.makeWith( - (key: string) => { - const [repository, number] = JSON.parse(key) as [string, number]; - return pullRequestDiff({ repository, number }).pipe( - Effect.map((diff) => ({ - revisions: parseDiffFileRevisions(diff.patch), - truncated: diff.truncated, - })), - ); - }, - { - capacity: REVISION_PATCH_CAPACITY, - timeToLive: (exit) => (Exit.isSuccess(exit) ? REVISION_PATCH_TTL : Duration.zero), - }, - ); - return BitbucketPullRequestApi.of({ getViewer: Effect.fn("BitbucketPullRequestApi.getViewer")(function () { return bitbucket.request({ method: "GET", url: "/user" }).pipe( @@ -601,12 +575,9 @@ export const make = Effect.gen(function* () { getFileRevisions: Effect.fn("BitbucketPullRequestApi.getFileRevisions")(function* (input) { if (input.paths.length === 0) return { revisions: new Map(), complete: false }; - return yield* Cache.get( - revisionPatches, - encodeRevisionKey([input.repository, input.number]), - ).pipe( + return yield* pullRequestDiff(input).pipe( Effect.map((diff) => { - const revisions = new Map(diff.revisions); + const revisions = new Map(parseDiffFileRevisions(diff.patch)); // A truncated patch cannot identify absent paths as deleted. if (!diff.truncated) { for (const path of input.paths) { From 5f60c38861a5bc61254a6ef23077bee5060da451 Mon Sep 17 00:00:00 2001 From: maria-rcks Date: Tue, 27 Oct 2026 05:53:34 +0000 Subject: [PATCH 8/9] fix(web): settle viewed presses after completed reads --- .../pullRequest/filesViewedStore.test.ts | 11 ++----- .../pullRequest/filesViewedStore.ts | 32 +++++++------------ 2 files changed, 15 insertions(+), 28 deletions(-) diff --git a/apps/web/src/components/pullRequest/filesViewedStore.test.ts b/apps/web/src/components/pullRequest/filesViewedStore.test.ts index 5cd193b70a0f8..5303720c7b0c2 100644 --- a/apps/web/src/components/pullRequest/filesViewedStore.test.ts +++ b/apps/web/src/components/pullRequest/filesViewedStore.test.ts @@ -71,7 +71,8 @@ function setup(initial: PullRequestFilesViewedResult | null = answer(false)) { describe("createFilesViewedStore", () => { it("batches presses and holds them until a read that could have seen them", async () => { - using t = setup(); + const unchanged = { files: [{ path: "a.ts", state: "dismissed" as const }], truncated: false }; + using t = setup(unchanged); const scope = t.store.scope(first); const release = scope.subscribe(() => {}); scope.setViewed("a.ts", true, "shown-revision"); @@ -88,10 +89,7 @@ describe("createFilesViewedStore", () => { expect(scope.getSnapshot().get("a.ts")).toBe(true); // A push landed between the write and the next read: the host's answer wins. - t.answerFor(first, { - files: [{ path: "a.ts", state: "dismissed" }], - truncated: false, - }); + t.answerFor(first, unchanged); expect(scope.getSnapshot().size).toBe(0); release(); }); @@ -205,9 +203,6 @@ describe("createFilesViewedStore", () => { await vi.advanceTimersByTimeAsync(400); t.settles[0]?.(AsyncResult.success(undefined)); await vi.advanceTimersByTimeAsync(0); - // The same answer published again is not a new read. - t.answerFor(first, agreeing); - expect(scope.getSnapshot().get("a.ts")).toBe(true); t.answerFor(first, answer(false)); expect(scope.getSnapshot().size).toBe(0); release(); diff --git a/apps/web/src/components/pullRequest/filesViewedStore.ts b/apps/web/src/components/pullRequest/filesViewedStore.ts index d50e5185810a4..527b7beb96b72 100644 --- a/apps/web/src/components/pullRequest/filesViewedStore.ts +++ b/apps/web/src/components/pullRequest/filesViewedStore.ts @@ -8,7 +8,6 @@ import type { PullRequestFilesViewedResult, PullRequestRef, } from "@t3tools/contracts"; -import * as Option from "effect/Option"; import { AsyncResult } from "effect/reactivity"; import { appAtomRegistry } from "~/rpc/atomRegistry"; @@ -37,7 +36,7 @@ type FilesViewedBatch = ReadonlyArray<{ export interface FilesViewedStoreDeps { readonly flushDelayMs: number; - /** Calls `onAnswer` with the host's current answer at once, then with every later one. */ + /** Calls `onAnswer` with the current answer, then after each completed read. */ readonly subscribeAnswers: ( target: FilesViewedTarget, onAnswer: (answer: PullRequestFilesViewedResult | null) => void, @@ -68,13 +67,8 @@ interface FilesViewedScope { readonly sentBy: Map; requests: number; timer: ReturnType | null; - answer: PullRequestFilesViewedResult | null; - /** - * The host's answer as it stood when a completed write was acknowledged, per path. The first - * answer that differs from it is the first read that could have seen the write, which is what - * retires the press rather than the host happening to agree with it. - */ - readonly answeredFrom: Map; + // Completed reads retire acknowledged presses even when equal data is structurally shared. + readonly answeredFrom: Set; } /** @@ -95,12 +89,11 @@ export function createFilesViewedStore(deps: FilesViewedStoreDeps) { const isLive = (key: string, scope: FilesViewedScope) => scopes.get(key) === scope; const settle = (scope: FilesViewedScope, answer: PullRequestFilesViewedResult | null) => { - if (answer === scope.answer) return; - scope.answer = answer; + if (answer === null) return; const pending = new Set([...scope.queued.keys(), ...scope.sentBy.keys()]); const answered = new Set(); - for (const [path, from] of scope.answeredFrom) { - if (pending.has(path) || from === answer) continue; + for (const path of scope.answeredFrom) { + if (pending.has(path)) continue; answered.add(path); scope.answeredFrom.delete(path); } @@ -141,7 +134,7 @@ export function createFilesViewedStore(deps: FilesViewedStoreDeps) { } // Answered for from the next read on, whatever it says. A push landing between the write // and that read comes back as `dismissed`, and the press must not stand over it. - for (const path of mine) scope.answeredFrom.set(path, scope.answer); + for (const path of mine) scope.answeredFrom.add(path); deps.refresh(scope.target); }); }; @@ -160,10 +153,8 @@ export function createFilesViewedStore(deps: FilesViewedStoreDeps) { if (scope === undefined) { const listeners = new Set<() => void>(); let created: FilesViewedScope | undefined; - let initial: PullRequestFilesViewedResult | null = null; const unsubscribeAnswers = deps.subscribeAnswers(target, (answer) => { - if (created === undefined) initial = answer; - else settle(created, answer); + if (created !== undefined) settle(created, answer); }); created = { target, @@ -174,8 +165,7 @@ export function createFilesViewedStore(deps: FilesViewedStoreDeps) { sentBy: new Map(), requests: 0, timer: null, - answer: initial, - answeredFrom: new Map(), + answeredFrom: new Set(), }; scope = created; scopes.set(key, scope); @@ -243,7 +233,9 @@ const filesViewedStore = createFilesViewedStore({ subscribeAnswers: (target, onAnswer) => appAtomRegistry.subscribe( filesViewedAtom(target), - (result) => onAnswer(Option.getOrNull(AsyncResult.value(result))), + (result) => { + if (AsyncResult.isSuccess(result) && !result.waiting) onAnswer(result.value); + }, { immediate: true }, ), send: ({ environmentId, reference }, files) => From e3220952f63b79d342d356c959dd44d9c2679ccb Mon Sep 17 00:00:00 2001 From: maria-rcks Date: Fri, 30 Oct 2026 00:49:43 +0000 Subject: [PATCH 9/9] fix(web): settle viewed marks only from server answers --- .../pullRequest/filesViewedStore.ts | 18 ++++----- apps/web/src/state/pullRequests.ts | 37 +++++++++++++++++++ .../client-runtime/src/state/pullRequests.ts | 2 + 3 files changed, 46 insertions(+), 11 deletions(-) diff --git a/apps/web/src/components/pullRequest/filesViewedStore.ts b/apps/web/src/components/pullRequest/filesViewedStore.ts index 527b7beb96b72..1ddd675e771a7 100644 --- a/apps/web/src/components/pullRequest/filesViewedStore.ts +++ b/apps/web/src/components/pullRequest/filesViewedStore.ts @@ -8,10 +8,12 @@ import type { PullRequestFilesViewedResult, PullRequestRef, } from "@t3tools/contracts"; -import { AsyncResult } from "effect/reactivity"; import { appAtomRegistry } from "~/rpc/atomRegistry"; -import { pullRequestEnvironment } from "~/state/pullRequests"; +import { + pullRequestEnvironment, + subscribePullRequestFilesViewedAnswers, +} from "~/state/pullRequests"; import { toastManager } from "../ui/toast"; import { @@ -36,7 +38,7 @@ type FilesViewedBatch = ReadonlyArray<{ export interface FilesViewedStoreDeps { readonly flushDelayMs: number; - /** Calls `onAnswer` with the current answer, then after each completed read. */ + /** Calls `onAnswer` only after completed server reads, excluding persisted values. */ readonly subscribeAnswers: ( target: FilesViewedTarget, onAnswer: (answer: PullRequestFilesViewedResult | null) => void, @@ -230,14 +232,8 @@ const filesViewedAtom = ({ environmentId, reference }: FilesViewedTarget) => const filesViewedStore = createFilesViewedStore({ flushDelayMs: 400, - subscribeAnswers: (target, onAnswer) => - appAtomRegistry.subscribe( - filesViewedAtom(target), - (result) => { - if (AsyncResult.isSuccess(result) && !result.waiting) onAnswer(result.value); - }, - { immediate: true }, - ), + subscribeAnswers: ({ environmentId, reference }, onAnswer) => + subscribePullRequestFilesViewedAnswers({ environmentId, input: reference }, onAnswer), send: ({ environmentId, reference }, files) => runAtomCommand( appAtomRegistry, diff --git a/apps/web/src/state/pullRequests.ts b/apps/web/src/state/pullRequests.ts index c8ac23ced0584..59a5c04e7af57 100644 --- a/apps/web/src/state/pullRequests.ts +++ b/apps/web/src/state/pullRequests.ts @@ -10,6 +10,7 @@ import { type EnvironmentId, type ProjectId, type PullRequestChecksState, + type PullRequestFilesViewedResult, type PullRequestListInput, type PullRequestFilterOptionsInput, type PullRequestListStatsInput, @@ -366,8 +367,44 @@ export function restoredPullRequestList( return restorePullRequestList(snapshotStorage(), targets); } +const filesViewedAnswerListeners = new Map< + string, + Set<(answer: PullRequestFilesViewedResult) => void> +>(); + +function filesViewedAnswerKey({ environmentId, input }: EnvironmentQueryTarget) { + return JSON.stringify([ + environmentId, + input.projectId, + input.host, + input.expectedAccountId, + input.allowStale, + input.repository, + input.number, + ]); +} + +export function subscribePullRequestFilesViewedAnswers( + target: EnvironmentQueryTarget, + onAnswer: (answer: PullRequestFilesViewedResult) => void, +) { + const key = filesViewedAnswerKey(target); + let listeners = filesViewedAnswerListeners.get(key); + if (listeners === undefined) filesViewedAnswerListeners.set(key, (listeners = new Set())); + listeners.add(onAnswer); + return () => { + listeners.delete(onAnswer); + if (listeners.size === 0) filesViewedAnswerListeners.delete(key); + }; +} + // Answers are stamped on arrival: older servers leave no other ordering for same-dated reads. export const pullRequestEnvironment = createPullRequestEnvironmentAtoms(connectionAtomRuntime, { + filesViewed: (target, answer) => + Effect.sync(() => { + for (const listener of filesViewedAnswerListeners.get(filesViewedAnswerKey(target)) ?? []) + listener(answer); + }), list: ({ environmentId, input }, answer, registry) => Effect.map(Clock.currentTimeMillis, (observedAt) => { rememberPullRequestListAnswer(snapshotStorage(), { environmentId, input }, answer); diff --git a/packages/client-runtime/src/state/pullRequests.ts b/packages/client-runtime/src/state/pullRequests.ts index ca5fb6d7a77d5..15550957e8fb9 100644 --- a/packages/client-runtime/src/state/pullRequests.ts +++ b/packages/client-runtime/src/state/pullRequests.ts @@ -148,6 +148,7 @@ export interface PullRequestAnswerHandlers { readonly list?: QueryAnswerHandler; readonly listStats?: QueryAnswerHandler; readonly detail?: QueryAnswerHandler; + readonly filesViewed?: QueryAnswerHandler; readonly checks?: QueryAnswerHandler; readonly reviewGuide?: QueryAnswerHandler; } @@ -422,6 +423,7 @@ export function createPullRequestEnvironmentAtoms( }), filesViewed: createEnvironmentRpcQueryAtomFamily(runtime, { label: "environment-data:pull-requests:files-viewed", + ...(onAnswer.filesViewed === undefined ? {} : { onAnswer: onAnswer.filesViewed }), persistence: persistQuery(PullRequestFilesViewedResult), tag: WS_METHODS.pullRequestsFilesViewed, execute: (input) => routedRequest(WS_METHODS.pullRequestsFilesViewed, input),