fix(server): a file ticked Viewed before loading a push shows Changed

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"]]);

apps/server/src/pullRequest/pullRequestViewedFiles.ts

@@ -260,7 +260,9 @@ export const make = (dependencies: Dependencies) => {
       input: PullRequestSetFilesViewedInput,
     ): Effect.fn.Return<void, PullRequestError> {
       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,

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),
     );

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);

apps/web/src/components/pullRequest/filesViewedStore.ts

@@ -20,14 +20,19 @@ import {
   toFileViewedBatch,
   toFileViewedStates,
   type FileViewedOverlay,
+  type FileViewedPress,
 } from "./pullRequestFilesViewed.logic";
 
 export interface FilesViewedTarget {
   readonly environmentId: EnvironmentId;
   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<string, boolean>;
+  queued: Map<string, FileViewedPress>;
   readonly sentBy: Map<string, number>;
   requests: number;
   timer: ReturnType<typeof setTimeout> | 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);

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<string, FileViewedPress>,
+): ReadonlyArray<{ readonly path: string; readonly viewed: boolean; readonly revision?: string }> {
+  return [...presses].map(([path, { viewed, revision }]) =>
+    revision === undefined ? { path, viewed } : { path, viewed, revision },
+  );
 }

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;

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)),
 });