From bc4b6e1f35d0b266be982cc0587f67ff42750376 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Thu, 12 Nov 2026 09:42:05 +0000 Subject: [PATCH 1/2] test(server): a viewed tick names the version the reader was shown Fails on main: a press after a push the reader has not loaded stores the pushed version, so the file reads Viewed instead of Changed (#35053). The press carries the displayed revision, which the contract does not accept yet. AI-assisted; verified with the tests and tools listed above. --- .../pullRequest/PullRequestService.test.ts | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index f54c77007c313..7045cb1e9f791 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -4321,6 +4321,26 @@ it.effect("leaves a mark alone when the host could not say what the head has of }), ); +it.effect("reports a file pushed to after the diff the reader ticked was drawn as changed", () => + Effect.gen(function* () { + const revisions = new Map([["src/a.ts", "shown-a"]]); + const service = yield* environmentViewedService(revisions, []); + + yield* service.filesViewed(GITLAB_REFERENCE); + yield* TestClock.adjust("61 seconds"); + revisions.set("src/a.ts", "pushed-a"); + yield* service.setFilesViewed({ + ...GITLAB_REFERENCE, + files: [{ path: "src/a.ts", viewed: true, revision: "shown-a" }], + }); + yield* service.invalidate({ reference: GITLAB_REFERENCE }); + + assert.deepStrictEqual((yield* service.filesViewed(GITLAB_REFERENCE)).files, [ + { path: "src/a.ts", state: "dismissed" }, + ]); + }), +); + it.effect("forgets what the head had of a marked file once a mutation moves the head", () => Effect.gen(function* () { const revisions = new Map([["src/a.ts", "blob-a"]]); From bfa0c8fda9722a4090d7a7284875efade92a8050 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Thu, 12 Nov 2026 09:52:00 +0000 Subject: [PATCH 2/2] fix(server): a file ticked Viewed before loading a push shows Changed A Viewed press sent only the file path, so the server stored whatever version the head had at the press. A push landing after the diff was drawn was saved as seen and the file never showed Changed (#35053). The web Code tab now sends the file's version from the diff it showed (the `index` line's new object id) with the press, and the server stores that instead of reading the head. Presses without one (commit diffs, diffs without index lines, mobile, older clients) keep reading the head. AI-assisted; verified with the tests and tools listed above. --- .../src/pullRequest/pullRequestViewedFiles.ts | 8 +++++--- .../pullRequest/PullRequestCodeTab.tsx | 5 ++++- .../pullRequest/filesViewedStore.test.ts | 6 ++++-- .../components/pullRequest/filesViewedStore.ts | 16 +++++++++++----- .../pullRequest/pullRequestFilesViewed.logic.ts | 13 ++++++++++--- .../pullRequest/usePullRequestFilesViewed.ts | 2 +- packages/contracts/src/pullRequest.ts | 2 ++ 7 files changed, 37 insertions(+), 15 deletions(-) diff --git a/apps/server/src/pullRequest/pullRequestViewedFiles.ts b/apps/server/src/pullRequest/pullRequestViewedFiles.ts index 596808eb46a07..ee956dfa0fe9c 100644 --- a/apps/server/src/pullRequest/pullRequestViewedFiles.ts +++ b/apps/server/src/pullRequest/pullRequestViewedFiles.ts @@ -260,7 +260,9 @@ 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); + const cleared = input.files + .filter((file) => file.viewed && file.revision === undefined) + .map((file) => file.path); const revisions = cleared.length === 0 ? null @@ -276,10 +278,10 @@ export const make = (dependencies: Dependencies) => { yield* filesViewedStore .set({ ...filesViewedScope(project, input.number, viewer), - // Missing paths have null baselines; empty revision means confirmed deletion. + // Shown revisions win over the head; missing paths have null baselines; empty means deleted. 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 8518bfe75071c..febf82441f514 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -825,7 +825,10 @@ function PullRequestCodeTab({ extendRange ? lastViewedFileKey : null, ); setLastViewedFileKey(fileKey); - for (const file of range) setViewed(file.path, viewed); + for (const file of range) { + // A commit's diff names that commit's version of the file, not the head's. + setViewed(file.path, viewed, commit === null ? file.fileDiff.newObjectId : undefined); + } setFoldChoices((current) => range.reduce((next, file) => withFileDiffFold(next, file.fileKey, viewed), current), ); diff --git a/apps/web/src/components/pullRequest/filesViewedStore.test.ts b/apps/web/src/components/pullRequest/filesViewedStore.test.ts index 9633e1306689d..3caef2d754103 100644 --- a/apps/web/src/components/pullRequest/filesViewedStore.test.ts +++ b/apps/web/src/components/pullRequest/filesViewedStore.test.ts @@ -74,11 +74,13 @@ describe("createFilesViewedStore", () => { using t = setup(); const scope = t.store.scope(first); const release = scope.subscribe(() => {}); - scope.setViewed("a.ts", true); + scope.setViewed("a.ts", true, "shown-a"); expect(scope.getSnapshot().get("a.ts")).toBe(true); await vi.advanceTimersByTimeAsync(400); expect(t.send).toHaveBeenCalledTimes(1); - expect(t.send.mock.calls[0]?.[1]).toEqual([{ path: "a.ts", viewed: true }]); + expect(t.send.mock.calls[0]?.[1]).toEqual([ + { path: "a.ts", viewed: true, revision: "shown-a" }, + ]); t.settles[0]?.(AsyncResult.success(undefined)); await vi.advanceTimersByTimeAsync(0); diff --git a/apps/web/src/components/pullRequest/filesViewedStore.ts b/apps/web/src/components/pullRequest/filesViewedStore.ts index cae490ded8d66..6ebafcd929b96 100644 --- a/apps/web/src/components/pullRequest/filesViewedStore.ts +++ b/apps/web/src/components/pullRequest/filesViewedStore.ts @@ -20,6 +20,7 @@ import { toFileViewedBatch, toFileViewedStates, type FileViewedOverlay, + type FileViewedPress, } from "./pullRequestFilesViewed.logic"; export interface FilesViewedTarget { @@ -27,7 +28,11 @@ export interface FilesViewedTarget { readonly reference: PullRequestRef; } -type FilesViewedBatch = ReadonlyArray<{ readonly path: string; readonly viewed: boolean }>; +type FilesViewedBatch = ReadonlyArray<{ + readonly path: string; + readonly viewed: boolean; + readonly revision?: string; +}>; export interface FilesViewedStoreDeps { readonly flushDelayMs: number; @@ -48,7 +53,8 @@ export interface FilesViewedScopeHandle { /** Holds the scope open while subscribed; the last reader to leave sends its last presses. */ readonly subscribe: (listener: () => void) => () => void; readonly getSnapshot: () => FileViewedOverlay; - readonly setViewed: (path: string, viewed: boolean) => void; + /** `revision` is the file's version in the diff the press was made on, when it names one. */ + readonly setViewed: (path: string, viewed: boolean, revision?: string) => void; } const NO_OVERLAY: FileViewedOverlay = new Map(); @@ -58,7 +64,7 @@ interface FilesViewedScope { readonly listeners: Set<() => void>; readonly unsubscribeAnswers: () => void; overlay: FileViewedOverlay; - queued: Map; + queued: Map; readonly sentBy: Map; requests: number; timer: ReturnType | null; @@ -203,10 +209,10 @@ export function createFilesViewedStore(deps: FilesViewedStoreDeps) { const handle: FilesViewedScopeHandle = { subscribe: (listener) => acquire(key, owned, handle, listener), getSnapshot: () => scopes.get(key)?.overlay ?? NO_OVERLAY, - setViewed: (path, viewed) => { + setViewed: (path, viewed, revision) => { const scope = scopes.get(key); if (scope === undefined) return; - scope.queued.set(path, viewed); + scope.queued.set(path, { viewed, revision }); publish(scope, new Map(scope.overlay).set(path, viewed)); if (scope.timer !== null) clearTimeout(scope.timer); scope.timer = setTimeout(() => flush(key, scope), deps.flushDelayMs); diff --git a/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts b/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts index 05f5fdc6dceda..89874004a9220 100644 --- a/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts @@ -58,8 +58,15 @@ export function revertFileViewedOverlay( return next.size === overlay.size ? overlay : next; } +export interface FileViewedPress { + readonly viewed: boolean; + readonly revision: string | undefined; +} + export function toFileViewedBatch( - overlay: FileViewedOverlay, -): ReadonlyArray<{ readonly path: string; readonly viewed: boolean }> { - 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 f71b135b6044d..22a03fb528959 100644 --- a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts +++ b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts @@ -15,7 +15,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) => void; readonly truncated: boolean; readonly error: string | null; readonly refresh: () => void; diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index 014b26272af73..7604b1f077413 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -790,6 +790,8 @@ export const PullRequestSetFilesViewedInput = Schema.Struct({ Schema.Struct({ path: FilePath, viewed: Schema.Boolean, + /** The file's version in the diff the reader was shown; without it the head's is recorded. */ + revision: Schema.optional(TrimmedNonEmptyString.check(Schema.isMaxLength(256))), }), ).check(Schema.isMaxLength(MAX_FILES_VIEWED_PRESSES)), });