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

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

@@ -3658,6 +3658,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

@@ -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<string, number>();
   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 = (

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

@@ -175,7 +175,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;
 }
 
 // Context updates viewed toggles without invalidating memoized header portals.
@@ -184,10 +189,12 @@ const FileViewedToggleContext = createContext<FileViewedToggleState | null>(null
 function FileViewedToggle({
   fileKey,
   path,
+  revision,
   stat,
 }: {
   readonly fileKey: string;
   readonly path: string;
+  readonly revision: string | undefined;
   readonly stat: ReactNode;
 }) {
   const viewedFiles = use(FileViewedToggleContext);
@@ -205,7 +212,9 @@ function FileViewedToggle({
         <Checkbox
           aria-label={stale ? "Changed" : "Viewed"}
           checked={viewed}
-          onCheckedChange={(next) => viewedFiles.setFileViewed(fileKey, path, next === true)}
+          onCheckedChange={(next) =>
+            viewedFiles.setFileViewed(fileKey, path, next === true, revision)
+          }
         />
         {stale ? (
           <Tooltip>
@@ -682,8 +691,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),
     );
@@ -820,6 +834,8 @@ function PullRequestCodeTab({
       <FileViewedToggle
         fileKey={item.id}
         path={path}
+        // One commit's diff names that commit's version, not the head's; its press falls back to the head read.
+        revision={commit === null ? item.fileDiff.newObjectId : undefined}
         stat={
           <PullRequestDiffStat
             additions={additions}

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,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";
@@ -21,18 +20,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` with the current answer, then after each completed read. */
   readonly subscribeAnswers: (
     target: FilesViewedTarget,
     onAnswer: (answer: PullRequestFilesViewedResult | null) => void,
@@ -49,7 +53,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();
@@ -59,17 +63,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>;
 }
 
 /**
@@ -90,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<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);
     }
@@ -136,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);
     });
   };
@@ -155,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,
@@ -169,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);
@@ -204,10 +199,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);
@@ -238,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) =>