fix: marking a file viewed records the version you were shown

apps/server/src/pullRequest/PullRequestService.test.ts

@@ -3717,6 +3717,69 @@ 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("records shown revisions until the reader ticks the pushed version", () =>
+  Effect.gen(function* () {
+    const asked: Array<ReadonlyArray<string>> = [];
+    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,
+      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" },
+    ]);
+
+    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-again" }],
+    });
+    assert.strictEqual(asked.length, reads);
+    assert.deepStrictEqual((yield* service.filesViewed(GITLAB_REFERENCE)).files, [
+      { path: "src/a.ts", state: "viewed" },
+      { path: "src/b.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(), []);

apps/server/src/pullRequest/PullRequestService.ts

@@ -1922,7 +1922,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);
+              }
+            }),
+          ),
         ),
       ),
     );
@@ -2811,6 +2818,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<string, number>();
   let everyFileRevisionEpoch = 0;
   const viewedFiles = ViewedFiles.make({
     filesViewedStore,
@@ -2819,7 +2827,8 @@ export const make = Effect.gen(function* () {
     toPullRequestError,
     runFork,
     refEpoch,
-    fileRevisionsEpoch: () => everyFileRevisionEpoch,
+    fileRevisionsEpoch: (ref) =>
+      `${everyFileRevisionEpoch}:${fileRevisionEpochs.get(refScope(ref)) ?? 0}`,
   });
 
   const filtersOfKey = (

apps/server/src/pullRequest/pullRequestViewedFiles.ts

@@ -30,7 +30,7 @@ const MAX_FILE_REVISION_PATHS = 1_000;
 interface FileRevisionsDependencies {
   readonly runFork: (effect: Effect.Effect<void>) => 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,
@@ -133,10 +133,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);
       // Refresh independently; it outlives the answered caller.
       return Effect.sync(() => {
@@ -260,7 +259,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
@@ -279,7 +280,7 @@ export const make = (dependencies: Dependencies) => {
           // Missing paths have null baselines; empty revision means confirmed deletion.
           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

@@ -722,7 +722,10 @@ function PullRequestCodeTab({
       extendRange ? lastViewedFileKey : null,
     );
     setLastViewedFileKey(fileKey);
-    for (const file of range) setViewed(file.path, viewed);
+    for (const file of range) {
+      // Commit diffs name the commit's version rather than the current head.
+      setViewed(file.path, viewed, commit === null ? file.fileDiff.newObjectId : undefined);
+    }
     setToggledFiles((current) =>
       range.reduce(
         (next, file) =>

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

@@ -71,25 +71,25 @@ 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);
+    scope.setViewed("a.ts", true, "shown-revision");
     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-revision" },
+    ]);
 
     t.settles[0]?.(AsyncResult.success(undefined));
     await vi.advanceTimersByTimeAsync(0);
     expect(t.refresh).toHaveBeenCalledWith(first);
     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();
   });
@@ -203,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();

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 {
@@ -20,18 +22,23 @@ 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;
-  /** Calls `onAnswer` with the host's current answer at once, then with every later one. */
+  /** Calls `onAnswer` only after completed server reads, excluding persisted values. */
   readonly subscribeAnswers: (
     target: FilesViewedTarget,
     onAnswer: (answer: PullRequestFilesViewedResult | null) => void,
@@ -48,7 +55,7 @@ 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;
+  readonly setViewed: (path: string, viewed: boolean, revision?: string) => void;
 }
 
 const NO_OVERLAY: FileViewedOverlay = new Map();
@@ -58,17 +65,12 @@ 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;
-  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<string, PullRequestFilesViewedResult | null>;
+  // Completed reads retire acknowledged presses even when equal data is structurally shared.
+  readonly answeredFrom: Set<string>;
 }
 
 /**
@@ -89,12 +91,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<string>();
-    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);
     }
@@ -135,7 +136,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);
     });
   };
@@ -154,10 +155,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,
@@ -168,8 +167,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);
@@ -203,10 +201,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);
@@ -234,12 +232,8 @@ const filesViewedAtom = ({ environmentId, reference }: FilesViewedTarget) =>
 
 const filesViewedStore = createFilesViewedStore({
   flushDelayMs: 400,
-  subscribeAnswers: (target, onAnswer) =>
-    appAtomRegistry.subscribe(
-      filesViewedAtom(target),
-      (result) => onAnswer(AsyncResult.getOrElse(result, () => null)),
-      { immediate: true },
-    ),
+  subscribeAnswers: ({ environmentId, reference }, onAnswer) =>
+    subscribePullRequestFilesViewedAnswers({ environmentId, input: reference }, onAnswer),
   send: ({ environmentId, reference }, files) =>
     runAtomCommand(
       appAtomRegistry,

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 | undefined) => void;
   readonly truncated: boolean;
   readonly error: string | null;
   readonly refresh: () => void;