Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,41 @@ describe("a mark whose file was pushed to before the read that followed it", ()
});
});

describe("a mark whose post-write read lands before the host applies it", () => {
it("holds the press until a read that could have seen the write", async () => {
view().setViewed("a.ts", true);
await act(async () => vi.advanceTimersByTimeAsync(500));
expect(setFilesViewed).toHaveBeenCalledExactlyOnceWith({
environmentId,
input: { ...reference, files: [{ path: "a.ts", viewed: true }] },
});
expect(host.refresh).toHaveBeenCalled();

// The write was acknowledged, but the host had not applied it when this read landed: the
// same answer as before, in a fresh object, as every real read is. It says nothing about
// the write, so the press must stand.
await reads("unviewed");
expect(view().isViewed("a.ts")).toBe(true);

// The next read saw the write, so the press retires onto the host's answer.
await reads("viewed");
expect(view().isViewed("a.ts")).toBe(true);
});

it("still gives way when the first differing read disagrees with the press", async () => {
view().setViewed("a.ts", true);
await act(async () => vi.advanceTimersByTimeAsync(500));

// A lagging read keeps the press...
await reads("unviewed");
expect(view().isViewed("a.ts")).toBe(true);

// ...but a read that genuinely differs from the at-ack answer retires it.
await reads("dismissed");
expect(view().isViewed("a.ts")).toBe(false);
expect(view().isStale("a.ts")).toBe(true);
});
});
describe("a mark the host has not answered for yet", () => {
it("holds the press while the write is still out", async () => {
let land = (_result: unknown) => {};
Expand Down
18 changes: 10 additions & 8 deletions apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { isAtomCommandInterrupted } from "@t3tools/client-runtime/state/runtime";
import type { EnvironmentId, PullRequestRef } from "@t3tools/contracts";
import type { EnvironmentId, PullRequestFileViewedState, PullRequestRef } from "@t3tools/contracts";
import { useCallback, useEffect, useMemo, useRef, useState } from "react";

import { pullRequestEnvironment } from "~/state/pullRequests";
Expand All @@ -16,7 +16,6 @@ import {
toFileViewedBatch,
toFileViewedStates,
type FileViewedOverlay,
type FileViewedStates,
} from "./pullRequestFilesViewed.logic";

/**
Expand Down Expand Up @@ -93,18 +92,20 @@ export function usePullRequestFilesViewed(options: {
const scopeKey = `${environmentId} ${reference.projectId} ${reference.repository} ${reference.number}`;
const scope = useRef(scopeKey);

// 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.
const answeredFrom = useRef<Map<string, FileViewedStates | null>>(new Map());
// The host's answer for each path as it stood when a completed write was acknowledged. The
// first read whose answer for the path 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.
// Compared by value, not by read: every read arrives as a fresh object, so a reference check
// would retire the press on a read that landed before the host applied the write.
const answeredFrom = useRef<Map<string, PullRequestFileViewedState | null>>(new Map());
const statesRef = useRef(states);
statesRef.current = states;

useEffect(() => {
const pending = new Set([...queued.current.keys(), ...sentBy.current.keys()]);
const answered = new Set<string>();
for (const [path, from] of answeredFrom.current) {
if (pending.has(path) || from === states) continue;
if (pending.has(path) || from === (states?.get(path) ?? null)) continue;
answered.add(path);
answeredFrom.current.delete(path);
}
Expand Down Expand Up @@ -142,7 +143,8 @@ export function usePullRequestFilesViewed(options: {
}
// 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) answeredFrom.current.set(path, statesRef.current);
for (const path of mine)
answeredFrom.current.set(path, statesRef.current?.get(path) ?? null);
refresh();
});
}, [environmentId, reference, refresh, setFilesViewed]);
Expand Down
Loading