From 727552aa2b116055a1f4068784c61270ed957d2e Mon Sep 17 00:00:00 2001 From: Alex Southwell Date: Fri, 2 Oct 2026 19:33:42 +1000 Subject: [PATCH 1/6] fix(prs): refresh viewed files after partial batch failures --- .../pullRequest/PullRequestService.test.ts | 47 +++++++++++++++++++ .../src/pullRequest/PullRequestService.ts | 2 +- .../usePullRequestFilesViewed.test.tsx | 34 +++++++++++++- .../pullRequest/usePullRequestFilesViewed.ts | 7 +-- 4 files changed, 84 insertions(+), 6 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index d8fd366916ab..ac0f688fe48e 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -5695,6 +5695,53 @@ 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([ + ["a.ts", "unviewed"], + ["b.ts", "unviewed"], + ]); + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + capabilities: { ...fakeProvider("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: "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; diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 1e65926e84f4..20454be91942 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -1933,7 +1933,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))), ), ), ); diff --git a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.test.tsx b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.test.tsx index 6f221bf7d363..87c2befb6122 100644 --- a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.test.tsx +++ b/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( + + + , + ); + }); + 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); + }); +}); diff --git a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts index c2ee3f4c0bf4..2a542f673e0c 100644 --- a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts +++ b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts @@ -127,9 +127,9 @@ 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 @@ -137,6 +137,7 @@ export function usePullRequestFilesViewed(options: { // 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; } From b9072862862c46a96dd324b0d0354ba26270812e Mon Sep 17 00:00:00 2001 From: SAPHID Date: Wed, 14 Oct 2026 03:39:10 +0000 Subject: [PATCH 2/6] test(server): brand the provider kind in the partial viewed-write test Main now types source-control provider kinds as a branded string, so the new test builds them with SourceControlProviderKind.make like its neighbours. Co-Authored-By: Claude Opus 5.5 --- apps/server/src/pullRequest/PullRequestService.test.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index b8153b2c571d..793ce753eada 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -6073,8 +6073,11 @@ it.effect("host viewed write errors discard cached marks after a partial mutatio const service = yield* makeService({ projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], providers: [ - fakeProvider("github", { - capabilities: { ...fakeProvider("github").capabilities, viewedFiles: "host" }, + fakeProvider(SourceControlProviderKind.make("github"), { + capabilities: { + ...fakeProvider(SourceControlProviderKind.make("github")).capabilities, + viewedFiles: "host", + }, getFilesViewed: () => Effect.sync(() => ({ files: [...states].map(([path, state]) => ({ path, state })), @@ -6084,7 +6087,7 @@ it.effect("host viewed write errors discard cached marks after a partial mutatio Effect.gen(function* () { states.set("a.ts", "viewed"); return yield* new PullRequestProviderError({ - provider: "github", + provider: SourceControlProviderKind.make("github"), operation: "setFilesViewed", reason: "failed", detail: "Second mutation refused", From e3a2c8c8a3f518bcef40b9822f673934afefab0a Mon Sep 17 00:00:00 2001 From: SAPHID Date: Wed, 14 Oct 2026 03:46:08 +0000 Subject: [PATCH 3/6] fix(client): forget routed viewed marks when a viewed-files batch fails When pull request reads route to another environment, only a successful write invalidated that reader's cached marks, so the refresh after a partial failure still saw the stale ones. Viewed-files writes now invalidate routed readers on every outcome; other writes still do so only on success. Co-Authored-By: Claude Opus 5.5 --- .../src/state/pullRequestRouting.ts | 68 +++++++++---------- .../src/state/pullRequests.test.ts | 64 +++++++++++++++++ 2 files changed, 97 insertions(+), 35 deletions(-) diff --git a/packages/client-runtime/src/state/pullRequestRouting.ts b/packages/client-runtime/src/state/pullRequestRouting.ts index 7b7addaa8c72..900bf38cd0d9 100644 --- a/packages/client-runtime/src/state/pullRequestRouting.ts +++ b/packages/client-runtime/src/state/pullRequestRouting.ts @@ -274,42 +274,40 @@ export function createPullRequestRouter() { ref.repository.toLowerCase(), String(ref.number), ]); - const finish = (operation: ReturnType>) => - operation.pipe( - Effect.tap(() => { - if (!writes.has(tag) || used.size === 0) return Effect.void; - const targets = new Map([ - [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([ + [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>) => + 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; diff --git a/packages/client-runtime/src/state/pullRequests.test.ts b/packages/client-runtime/src/state/pullRequests.test.ts index 0217cfd00078..f83a6494a12b 100644 --- a/packages/client-runtime/src/state/pullRequests.test.ts +++ b/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", From 7429e1df04afe6fab0e358d740a1f1fc0bd1f3dc Mon Sep 17 00:00:00 2001 From: SAPHID Date: Wed, 14 Oct 2026 03:50:12 +0000 Subject: [PATCH 4/6] docs(client): describe routed reader invalidation as best effort It now also runs after a failed viewed-files batch, when the host may have accepted only part of the write. Co-Authored-By: Claude Opus 5.5 --- packages/client-runtime/src/state/pullRequestRouting.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/client-runtime/src/state/pullRequestRouting.ts b/packages/client-runtime/src/state/pullRequestRouting.ts index 900bf38cd0d9..5cc3b92cf9cf 100644 --- a/packages/client-runtime/src/state/pullRequestRouting.ts +++ b/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), ), From 2abfab8371ef7a8d42b2351ab079c67b7eca8e72 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Mon, 19 Oct 2026 07:04:45 +0000 Subject: [PATCH 5/6] test(prs): prove partial viewed writes refresh without evicting diffs --- .../server/src/pullRequest/PullRequestService.test.ts | 11 ++++++++++- .../usePullRequestFilesViewed.browser.test.tsx | 10 +++++----- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 31614a51013f..5eb0ea9660f7 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -6214,8 +6214,9 @@ 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", () => +it.effect("partial host viewed writes invalidate marks but keep the diff cached", () => Effect.gen(function* () { + let diffReads = 0; const states = new Map([ ["a.ts", "unviewed"], ["b.ts", "unviewed"], @@ -6228,6 +6229,11 @@ it.effect("host viewed write errors discard cached marks after a partial mutatio ...fakeProvider(SourceControlProviderKind.make("github")).capabilities, viewedFiles: "host", }, + getDiff: () => + Effect.sync(() => { + diffReads += 1; + return { patch: "@@", truncated: false, nextCursor: null }; + }), getFilesViewed: () => Effect.sync(() => ({ files: [...states].map(([path, state]) => ({ path, state })), @@ -6247,6 +6253,7 @@ it.effect("host viewed write errors discard cached marks after a partial mutatio ], }); const ref = { projectId: "p1" as ProjectId, repository: "acme/web", number: 10 }; + yield* service.diff(ref); assert.deepStrictEqual((yield* service.filesViewed(ref)).files, [ { path: "a.ts", state: "unviewed" }, { path: "b.ts", state: "unviewed" }, @@ -6257,6 +6264,8 @@ it.effect("host viewed write errors discard cached marks after a partial mutatio files: [...states.keys()].map((path) => ({ path, viewed: true })), }), ); + yield* service.diff(ref); + assert.strictEqual(diffReads, 1); assert.deepStrictEqual((yield* service.filesViewed(ref)).files, [ { path: "a.ts", state: "viewed" }, { path: "b.ts", state: "unviewed" }, diff --git a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.browser.test.tsx b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.browser.test.tsx index 281015513c09..b967849e9521 100644 --- a/apps/web/src/components/pullRequest/usePullRequestFilesViewed.browser.test.tsx +++ b/apps/web/src/components/pullRequest/usePullRequestFilesViewed.browser.test.tsx @@ -161,20 +161,20 @@ describe("a failed batch that the host partly applied", () => { 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( + root.render( , ); }); - view().setViewed("a.ts", true); - view().setViewed("b.ts", true); + act(() => { + 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); }); }); From 0544a3dffb48aaaa42ad578303bcf7c10404d2da Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Mon, 19 Oct 2026 07:12:21 +0000 Subject: [PATCH 6/6] refactor(prs): clarify viewed-write invalidation comments --- packages/client-runtime/src/state/pullRequestRouting.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/client-runtime/src/state/pullRequestRouting.ts b/packages/client-runtime/src/state/pullRequestRouting.ts index 36aadc5c7e14..594e56760a75 100644 --- a/packages/client-runtime/src/state/pullRequestRouting.ts +++ b/packages/client-runtime/src/state/pullRequestRouting.ts @@ -202,7 +202,7 @@ const invalidateTarget = Effect.fn("PullRequestRouting.invalidateTarget")(functi inputs, (input) => registry.run(target, request(WS_METHODS.pullRequestsInvalidate, input)).pipe( - // Best effort and bounded: a stalled reader must not delay the write's answer. + // A stalled reader must not delay the write's answer. Effect.timeoutOption("1 second"), Effect.orElseSucceed(() => undefined), ), @@ -300,8 +300,7 @@ export function createPullRequestRouter() { { 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. + // A failed viewed-files batch may have applied some paths. Other writes invalidate on success. const finish = (operation: ReturnType>) => tag === WS_METHODS.pullRequestsSetFilesViewed ? operation.pipe(Effect.ensuring(invalidateReaders))