Skip to content

Commit bfa0c8f

Browse files
fix(server): a file ticked Viewed before loading a push shows Changed
A Viewed press sent only the file path, so the server stored whatever version the head had at the press. A push landing after the diff was drawn was saved as seen and the file never showed Changed (#35053). The web Code tab now sends the file's version from the diff it showed (the `index` line's new object id) with the press, and the server stores that instead of reading the head. Presses without one (commit diffs, diffs without index lines, mobile, older clients) keep reading the head. AI-assisted; verified with the tests and tools listed above.
1 parent bc4b6e1 commit bfa0c8f

7 files changed

Lines changed: 37 additions & 15 deletions

File tree

‎apps/server/src/pullRequest/pullRequestViewedFiles.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,9 @@ export const make = (dependencies: Dependencies) => {
260260
input: PullRequestSetFilesViewedInput,
261261
): Effect.fn.Return<void, PullRequestError> {
262262
const viewer = yield* requiredViewerOf(project, "setFilesViewed");
263-
const cleared = input.files.filter((file) => file.viewed).map((file) => file.path);
263+
const cleared = input.files
264+
.filter((file) => file.viewed && file.revision === undefined)
265+
.map((file) => file.path);
264266
const revisions =
265267
cleared.length === 0
266268
? null
@@ -276,10 +278,10 @@ export const make = (dependencies: Dependencies) => {
276278
yield* filesViewedStore
277279
.set({
278280
...filesViewedScope(project, input.number, viewer),
279-
// Missing paths have null baselines; empty revision means confirmed deletion.
281+
// Shown revisions win over the head; missing paths have null baselines; empty means deleted.
280282
files: input.files.map((file) => ({
281283
path: file.path,
282-
revision: revisions?.get(file.path) ?? null,
284+
revision: file.revision ?? revisions?.get(file.path) ?? null,
283285
viewed: file.viewed,
284286
})),
285287
viewedAt,

‎apps/web/src/components/pullRequest/PullRequestCodeTab.tsx‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -825,7 +825,10 @@ function PullRequestCodeTab({
825825
extendRange ? lastViewedFileKey : null,
826826
);
827827
setLastViewedFileKey(fileKey);
828-
for (const file of range) setViewed(file.path, viewed);
828+
for (const file of range) {
829+
// A commit's diff names that commit's version of the file, not the head's.
830+
setViewed(file.path, viewed, commit === null ? file.fileDiff.newObjectId : undefined);
831+
}
829832
setFoldChoices((current) =>
830833
range.reduce((next, file) => withFileDiffFold(next, file.fileKey, viewed), current),
831834
);

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,11 +74,13 @@ describe("createFilesViewedStore", () => {
7474
using t = setup();
7575
const scope = t.store.scope(first);
7676
const release = scope.subscribe(() => {});
77-
scope.setViewed("a.ts", true);
77+
scope.setViewed("a.ts", true, "shown-a");
7878
expect(scope.getSnapshot().get("a.ts")).toBe(true);
7979
await vi.advanceTimersByTimeAsync(400);
8080
expect(t.send).toHaveBeenCalledTimes(1);
81-
expect(t.send.mock.calls[0]?.[1]).toEqual([{ path: "a.ts", viewed: true }]);
81+
expect(t.send.mock.calls[0]?.[1]).toEqual([
82+
{ path: "a.ts", viewed: true, revision: "shown-a" },
83+
]);
8284

8385
t.settles[0]?.(AsyncResult.success(undefined));
8486
await vi.advanceTimersByTimeAsync(0);

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

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,14 +20,19 @@ import {
2020
toFileViewedBatch,
2121
toFileViewedStates,
2222
type FileViewedOverlay,
23+
type FileViewedPress,
2324
} from "./pullRequestFilesViewed.logic";
2425

2526
export interface FilesViewedTarget {
2627
readonly environmentId: EnvironmentId;
2728
readonly reference: PullRequestRef;
2829
}
2930

30-
type FilesViewedBatch = ReadonlyArray<{ readonly path: string; readonly viewed: boolean }>;
31+
type FilesViewedBatch = ReadonlyArray<{
32+
readonly path: string;
33+
readonly viewed: boolean;
34+
readonly revision?: string;
35+
}>;
3136

3237
export interface FilesViewedStoreDeps {
3338
readonly flushDelayMs: number;
@@ -48,7 +53,8 @@ export interface FilesViewedScopeHandle {
4853
/** Holds the scope open while subscribed; the last reader to leave sends its last presses. */
4954
readonly subscribe: (listener: () => void) => () => void;
5055
readonly getSnapshot: () => FileViewedOverlay;
51-
readonly setViewed: (path: string, viewed: boolean) => void;
56+
/** `revision` is the file's version in the diff the press was made on, when it names one. */
57+
readonly setViewed: (path: string, viewed: boolean, revision?: string) => void;
5258
}
5359

5460
const NO_OVERLAY: FileViewedOverlay = new Map();
@@ -58,7 +64,7 @@ interface FilesViewedScope {
5864
readonly listeners: Set<() => void>;
5965
readonly unsubscribeAnswers: () => void;
6066
overlay: FileViewedOverlay;
61-
queued: Map<string, boolean>;
67+
queued: Map<string, FileViewedPress>;
6268
readonly sentBy: Map<string, number>;
6369
requests: number;
6470
timer: ReturnType<typeof setTimeout> | null;
@@ -203,10 +209,10 @@ export function createFilesViewedStore(deps: FilesViewedStoreDeps) {
203209
const handle: FilesViewedScopeHandle = {
204210
subscribe: (listener) => acquire(key, owned, handle, listener),
205211
getSnapshot: () => scopes.get(key)?.overlay ?? NO_OVERLAY,
206-
setViewed: (path, viewed) => {
212+
setViewed: (path, viewed, revision) => {
207213
const scope = scopes.get(key);
208214
if (scope === undefined) return;
209-
scope.queued.set(path, viewed);
215+
scope.queued.set(path, { viewed, revision });
210216
publish(scope, new Map(scope.overlay).set(path, viewed));
211217
if (scope.timer !== null) clearTimeout(scope.timer);
212218
scope.timer = setTimeout(() => flush(key, scope), deps.flushDelayMs);

‎apps/web/src/components/pullRequest/pullRequestFilesViewed.logic.ts‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,15 @@ export function revertFileViewedOverlay(
5858
return next.size === overlay.size ? overlay : next;
5959
}
6060

61+
export interface FileViewedPress {
62+
readonly viewed: boolean;
63+
readonly revision: string | undefined;
64+
}
65+
6166
export function toFileViewedBatch(
62-
overlay: FileViewedOverlay,
63-
): ReadonlyArray<{ readonly path: string; readonly viewed: boolean }> {
64-
return [...overlay].map(([path, viewed]) => ({ path, viewed }));
67+
presses: ReadonlyMap<string, FileViewedPress>,
68+
): ReadonlyArray<{ readonly path: string; readonly viewed: boolean; readonly revision?: string }> {
69+
return [...presses].map(([path, { viewed, revision }]) =>
70+
revision === undefined ? { path, viewed } : { path, viewed, revision },
71+
);
6572
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ export interface PullRequestFilesViewedView {
1515
readonly enabled: boolean;
1616
readonly isViewed: (path: string) => boolean;
1717
readonly isStale: (path: string) => boolean;
18-
readonly setViewed: (path: string, viewed: boolean) => void;
18+
readonly setViewed: (path: string, viewed: boolean, revision?: string) => void;
1919
readonly truncated: boolean;
2020
readonly error: string | null;
2121
readonly refresh: () => void;

‎packages/contracts/src/pullRequest.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -790,6 +790,8 @@ export const PullRequestSetFilesViewedInput = Schema.Struct({
790790
Schema.Struct({
791791
path: FilePath,
792792
viewed: Schema.Boolean,
793+
/** The file's version in the diff the reader was shown; without it the head's is recorded. */
794+
revision: Schema.optional(TrimmedNonEmptyString.check(Schema.isMaxLength(256))),
793795
}),
794796
).check(Schema.isMaxLength(MAX_FILES_VIEWED_PRESSES)),
795797
});

0 commit comments

Comments
 (0)