fix(prs): refresh viewed files after partial batch failures

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

@@ -6064,6 +6064,56 @@ it.effect("names the signed-in account in the detail, and says nothing where the
   }),
 );
 
+it.effect("host viewed write errors discard cached marks after a partial mutation", () =>
+  Effect.gen(function* () {
+    const states = new Map<string, "viewed" | "unviewed">([
+      ["a.ts", "unviewed"],
+      ["b.ts", "unviewed"],
+    ]);
+    const service = yield* makeService({
+      projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })],
+      providers: [
+        fakeProvider(SourceControlProviderKind.make("github"), {
+          capabilities: {
+            ...fakeProvider(SourceControlProviderKind.make("github")).capabilities,
+            viewedFiles: "host",
+          },
+          getFilesViewed: () =>
+            Effect.sync(() => ({
+              files: [...states].map(([path, state]) => ({ path, state })),
+              truncated: false,
+            })),
+          setFilesViewed: () =>
+            Effect.gen(function* () {
+              states.set("a.ts", "viewed");
+              return yield* new PullRequestProviderError({
+                provider: SourceControlProviderKind.make("github"),
+                operation: "setFilesViewed",
+                reason: "failed",
+                detail: "Second mutation refused",
+              });
+            }),
+        }),
+      ],
+    });
+    const ref = { projectId: "p1" as ProjectId, repository: "acme/web", number: 10 };
+    assert.deepStrictEqual((yield* service.filesViewed(ref)).files, [
+      { path: "a.ts", state: "unviewed" },
+      { path: "b.ts", state: "unviewed" },
+    ]);
+    yield* Effect.flip(
+      service.setFilesViewed({
+        ...ref,
+        files: [...states.keys()].map((path) => ({ path, viewed: true })),
+      }),
+    );
+    assert.deepStrictEqual((yield* service.filesViewed(ref)).files, [
+      { path: "a.ts", state: "viewed" },
+      { path: "b.ts", state: "unviewed" },
+    ]);
+  }),
+);
+
 it.effect("keeps the diff cached across a file being ticked off", () =>
   Effect.gen(function* () {
     let diffReads = 0;

apps/server/src/pullRequest/PullRequestService.ts

@@ -2011,7 +2011,7 @@ 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. Only this reader's own bookkeeping is forgotten.
-          Effect.tap(() => Effect.sync(() => bumpFilesViewedEpoch(ref))),
+          Effect.ensuring(Effect.sync(() => bumpFilesViewedEpoch(ref))),
         ),
       ),
     );

apps/web/src/components/pullRequest/usePullRequestFilesViewed.test.tsx

@@ -4,6 +4,7 @@ import {
   type PullRequestFilesViewedResult,
   type PullRequestRef,
 } from "@t3tools/contracts";
+import * as Cause from "effect/Cause";
 import { AsyncResult } from "effect/reactivity";
 import { act, StrictMode } from "react";
 import { create, type ReactTestRenderer } from "react-test-renderer";
@@ -41,11 +42,17 @@ const reference: PullRequestRef = {
   repository: "acme/web",
   number: 42,
 };
-const paths = ["a.ts"];
+const paths = ["a.ts", "b.ts"];
 
 /** What the host answers, as a fresh object each time: a read is only a read if it is a new one. */
 function answer(state: "unviewed" | "viewed" | "dismissed"): PullRequestFilesViewedResult {
-  return { files: [{ path: "a.ts", state }], truncated: false };
+  return {
+    files: [
+      { path: "a.ts", state },
+      { path: "b.ts", state: "unviewed" },
+    ],
+    truncated: false,
+  };
 }
 
 let renderer: ReactTestRenderer | null = null;
@@ -159,3 +166,26 @@ describe("a mark the host has not answered for yet", () => {
     expect(view().isStale("a.ts")).toBe(false);
   });
 });
+
+describe("a failed batch that the host partly applied", () => {
+  it("refreshes the host and accepts the applied marks", async () => {
+    setFilesViewed.mockResolvedValueOnce(
+      AsyncResult.failure(Cause.fail(new Error("partial batch"))),
+    );
+    // A fresh host read contains the first applied alias, while the refused alias stays unviewed.
+    host.refresh.mockImplementation(() => {
+      host.data = answer("viewed");
+      renderer!.update(
+        <StrictMode>
+          <Surface />
+        </StrictMode>,
+      );
+    });
+    view().setViewed("a.ts", true);
+    view().setViewed("b.ts", true);
+    await act(async () => vi.advanceTimersByTimeAsync(500));
+    expect(view().isViewed("a.ts")).toBe(true);
+    expect(view().isViewed("b.ts")).toBe(false);
+    expect(view().viewedCount).toBe(1);
+  });
+});

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

@@ -127,16 +127,17 @@ export function usePullRequestFilesViewed(options: {
       // The reader has moved on, and what is on screen now has nothing to do with this answer.
       if (scope.current !== sentFrom) return;
       if (result._tag === "Failure") {
-        // The host never heard these, so the ticks go back to whatever it last said. Only the
-        // paths this request still answers for: one pressed again since is waiting on a request
-        // of its own, or on the next flush, and that press is the one on screen.
+        // A failed batch can still have applied some paths. Revert to the last read until it is
+        // refreshed, and only the paths this request still answers for: one pressed again since
+        // is waiting on a request of its own, or on the next flush, and that press is on screen.
         const owned = new Set(mine.filter((path) => !queued.current.has(path)));
         setOverlay((current) => revertFileViewedOverlay(current, batch, owned));
         // Silent when nothing was still this request's to answer for, so nothing on screen went
         // back, and when the connection went away mid-flight, which the reader is already being
         // told about and which the host never refused.
         if (owned.size > 0 && !isAtomCommandInterrupted(result)) {
           toastManager.add({ type: "error", title: "Could not update viewed files" });
+          refresh();
         }
         return;
       }

packages/client-runtime/src/state/pullRequestRouting.ts

@@ -204,7 +204,7 @@ const invalidateTarget = Effect.fn("PullRequestRouting.invalidateTarget")(functi
     inputs,
     (input) =>
       registry.run(target, request(WS_METHODS.pullRequestsInvalidate, input)).pipe(
-        // GitHub already accepted the write. A stalled reader must not delay its confirmation.
+        // Best effort and bounded: a stalled reader must not delay the write's answer.
         Effect.timeoutOption("1 second"),
         Effect.orElseSucceed(() => undefined),
       ),
@@ -274,42 +274,40 @@ export function createPullRequestRouter() {
       ref.repository.toLowerCase(),
       String(ref.number),
     ]);
-    const finish = (operation: ReturnType<typeof request<T>>) =>
-      operation.pipe(
-        Effect.tap(() => {
-          if (!writes.has(tag) || used.size === 0) return Effect.void;
-          const targets = new Map<EnvironmentId, PullRequestRef[]>([
-            [origin.target.environmentId, [ref]],
-          ]);
-          for (const entry of used.values()) {
-            if (
-              entry.origin !== origin.target.environmentId ||
-              !matchesReference(entry.reference, ref)
-            )
-              continue;
-            for (const target of [...entry.targets, origin.target.environmentId]) {
-              const refs = targets.get(target) ?? [];
-              if (!refs.some((existing) => existing.host === entry.reference.host))
-                refs.push(entry.reference);
-              targets.set(target, refs);
-            }
-          }
-          return Effect.forEach(
-            targets,
-            ([target, refs]) =>
-              invalidateTarget(registry, origin.target.environmentId, target, [
-                ...refs.map((reference) => ({
-                  reference,
-                  ...(tag === WS_METHODS.pullRequestsSetFilesViewed
-                    ? { filesViewedOnly: true }
-                    : {}),
-                })),
-                ...(tag === WS_METHODS.pullRequestsSetFilesViewed ? [] : [{}]),
-              ]),
-            { concurrency: 4, discard: true },
-          );
-        }),
+    const invalidateReaders = Effect.suspend(() => {
+      if (!writes.has(tag) || used.size === 0) return Effect.void;
+      const targets = new Map<EnvironmentId, PullRequestRef[]>([
+        [origin.target.environmentId, [ref]],
+      ]);
+      for (const entry of used.values()) {
+        if (entry.origin !== origin.target.environmentId || !matchesReference(entry.reference, ref))
+          continue;
+        for (const target of [...entry.targets, origin.target.environmentId]) {
+          const refs = targets.get(target) ?? [];
+          if (!refs.some((existing) => existing.host === entry.reference.host))
+            refs.push(entry.reference);
+          targets.set(target, refs);
+        }
+      }
+      return Effect.forEach(
+        targets,
+        ([target, refs]) =>
+          invalidateTarget(registry, origin.target.environmentId, target, [
+            ...refs.map((reference) => ({
+              reference,
+              ...(tag === WS_METHODS.pullRequestsSetFilesViewed ? { filesViewedOnly: true } : {}),
+            })),
+            ...(tag === WS_METHODS.pullRequestsSetFilesViewed ? [] : [{}]),
+          ]),
+        { concurrency: 4, discard: true },
       );
+    });
+    // A failed viewed-files batch can still have applied some paths on the host, so its routed
+    // readers forget their marks either way. Other writes invalidate only after they succeed.
+    const finish = (operation: ReturnType<typeof request<T>>) =>
+      tag === WS_METHODS.pullRequestsSetFilesViewed
+        ? operation.pipe(Effect.ensuring(invalidateReaders))
+        : operation.pipe(Effect.tap(() => invalidateReaders));
     const alternatives = [];
     for (const [id, entry] of entries) {
       if (id === origin.target.environmentId) continue;

packages/client-runtime/src/state/pullRequests.test.ts

@@ -584,6 +584,70 @@ it.effect.each(["github", "gitlab", "bitbucket", "azure-devops"] as const)(
     ),
 );
 
+it.effect("forgets routed viewed marks when a viewed-files batch fails", () =>
+  Effect.scoped(
+    Effect.gen(function* () {
+      const reference = {
+        projectId: ProjectId.make("project-1"),
+        host: "github.com",
+        repository: "acme/web",
+        number: 7,
+      };
+      const invalidations: { environment: string; input: unknown }[] = [];
+      const identity = () =>
+        Effect.succeed({
+          host: reference.host,
+          provider: "github",
+          accountId: "123",
+          viewer: "maria-rcks",
+        });
+      const clientFor = (environment: string) =>
+        ({
+          [WS_METHODS.pullRequestsRouting]: identity,
+          [WS_METHODS.pullRequestsRoutingIdentity]: identity,
+          [WS_METHODS.pullRequestsFilesViewed]: () =>
+            Effect.succeed({ files: [{ path: "a.ts", state: "unviewed" }], truncated: false }),
+          // The host may have applied part of the batch before refusing the rest.
+          [WS_METHODS.pullRequestsSetFilesViewed]: () => Effect.fail("second path refused"),
+          [WS_METHODS.pullRequestsInvalidate]: (input: unknown) =>
+            Effect.sync(() => {
+              invalidations.push({ environment, input });
+            }),
+        }) as unknown as WsRpcProtocolClient;
+      const { environmentRegistry, supervisor } = yield* makeTestRuntime(
+        clientFor("origin"),
+        clientFor("local"),
+      );
+      const route = createPullRequestRouter();
+      const write = yield* Effect.gen(function* () {
+        yield* route(WS_METHODS.pullRequestsFilesViewed, reference);
+        return yield* Effect.exit(
+          route(WS_METHODS.pullRequestsSetFilesViewed, {
+            ...reference,
+            files: [
+              { path: "a.ts", viewed: true },
+              { path: "b.ts", viewed: true },
+            ],
+          }),
+        );
+      }).pipe(
+        Effect.provideService(EnvironmentRegistry.EnvironmentRegistry, environmentRegistry),
+        Effect.provideService(GitHubRoutingPermissions, trustedRouting),
+        Effect.provideService(RpcPermissionGuard, { authorize: () => Effect.void }),
+        Effect.provideService(EnvironmentSupervisor.EnvironmentSupervisor, supervisor),
+      );
+      expect(write._tag).toBe("Failure");
+      expect(invalidations).toContainEqual({
+        environment: "local",
+        input: { reference: expect.objectContaining(reference), filesViewedOnly: true },
+      });
+      for (const call of invalidations) {
+        expect(call.input).toMatchObject({ filesViewedOnly: true });
+      }
+    }),
+  ),
+);
+
 const TARGET = new PrimaryConnectionTarget({
   environmentId: EnvironmentId.make("environment-1"),
   label: "Test environment",