From 371b52d9dad76876f84609a4cd0f80eaa757b69c Mon Sep 17 00:00:00 2001 From: maria Date: Mon, 21 Sep 2026 14:39:40 -0300 Subject: [PATCH] feat(web): answer pull request actions on the row at once (#12843) Co-authored-by: Claude Fable 5.1 --- .../pullRequest/PullRequestDetailPanel.tsx | 11 +- .../pullRequest/pullRequestList.logic.test.ts | 94 +++++++++++ .../pullRequest/pullRequestList.logic.ts | 126 +++++++++++++++ apps/web/src/routes/_chat.pull-requests.tsx | 151 ++++++++++++++++-- 4 files changed, 367 insertions(+), 15 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index f19e41599043..a02cb52173bd 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -458,7 +458,12 @@ export function PullRequestDetailPanel({ * An action changed this pull request on the host, so a list showing it is now out of date. * Told rather than assumed: only the page knows whether it is showing one. */ - onActed?: () => void; + /** + * Each host action as it goes: "sent" the moment it leaves, so a list can answer before the + * host does; "done" or "failed" when the host has spoken. Undefined for one the caller cannot + * name, which is only ever "done". + */ + onActed?: (action?: PullRequestAction, phase?: "sent" | "done" | "failed") => void; /** Page-owned detail columns use this to clear the selected pull request. */ onClose?: () => void; /** @@ -961,6 +966,7 @@ export function PullRequestDetailPanel({ method?: PullRequestMergeMethod, updateMethod?: PullRequestUpdateMethod, ) => { + onActed?.(action, "sent"); const result = await runAction({ environmentId, input: { @@ -988,6 +994,7 @@ export function PullRequestDetailPanel({ title: ACTION_FAILURE_LABELS[action], description: readableFailure(failure, hint), }); + onActed?.(action, "failed"); return false; } toastManager.add({ type: "success", title: ACTION_SUCCESS_LABELS[action] }); @@ -1001,7 +1008,7 @@ export function PullRequestDetailPanel({ } else { refreshDetail(); } - onActed?.(); + onActed?.(action, "done"); return true; }; diff --git a/apps/web/src/components/pullRequest/pullRequestList.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestList.logic.test.ts index 9154e38026d2..357ea20ad76d 100644 --- a/apps/web/src/components/pullRequest/pullRequestList.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestList.logic.test.ts @@ -32,6 +32,10 @@ import { resolveQueryEnvironmentIds, resolveSelectedEnvironmentId, type EnvironmentPullRequestEntry, + applyPullRequestOverrides, + pullRequestOverrideAfterAction, + reusePullRequestEntries, + settlePullRequestOverrides, } from "./pullRequestList.logic"; import { pullRequestListPreferences, @@ -1600,3 +1604,93 @@ describe("the priority groups against a paginated feed", () => { ).toEqual([6123]); }); }); + +describe("pull request list overrides", () => { + const entry = (number: number, state: "open" | "closed" | "merged") => + ({ + host: "github.com", + repository: "pingdotgg/t3code", + number, + state, + isDraft: false, + updatedAt: "2026-07-01T00:00:00Z", + labels: [], + }) as unknown as PullRequestListEntry; + const key = (row: { number: number }) => `#${row.number}`; + + it("maps the actions that change a row's state and nothing else", () => { + const now = new Date("2026-07-02T00:00:00Z"); + expect(pullRequestOverrideAfterAction(entry(1, "open"), "close", now, 7)).toEqual({ + state: "closed", + updatedAt: "2026-07-02T00:00:00.000Z", + token: 7, + at: now.getTime(), + }); + expect(pullRequestOverrideAfterAction(entry(1, "closed"), "reopen", now, 1)?.state).toBe( + "open", + ); + expect(pullRequestOverrideAfterAction(entry(1, "open"), "merge", now, 1)?.state).toBe("merged"); + expect(pullRequestOverrideAfterAction(entry(1, "open"), "draft", now, 1)?.isDraft).toBe(true); + expect(pullRequestOverrideAfterAction(entry(1, "open"), "update-branch", now, 1)).toBeNull(); + }); + + it("writes the override over the row and drops it from a list whose state it left", () => { + const rows = [entry(1, "open"), entry(2, "open")]; + const overrides = new Map([ + ["#1", { state: "closed" as const, updatedAt: "2026-07-03T00:00:00Z", token: 1, at: 0 }], + ]); + expect( + applyPullRequestOverrides(rows, overrides, key, "open").map((row) => row.number), + ).toEqual([2]); + const all = applyPullRequestOverrides(rows, overrides, key, "all"); + expect(all.map((row) => [row.number, row.state])).toEqual([ + [1, "closed"], + [2, "open"], + ]); + expect(applyPullRequestOverrides(rows, new Map(), key, "open")).toBe(rows); + }); + + it("hands back the held object for a row a refresh did not change", () => { + const previous = [entry(1, "open"), entry(2, "open")]; + const next = [{ ...entry(1, "open") }, { ...entry(2, "open"), state: "merged" as const }]; + const reused = reusePullRequestEntries(previous, next, key); + expect(reused[0]).toBe(previous[0]); + expect(reused[1]).toBe(next[1]); + expect( + reusePullRequestEntries(previous, [{ ...entry(1, "open") }, { ...entry(2, "open") }], key), + ).toBe(previous); + }); +}); + +describe("pull request list override settlement", () => { + const entry = (number: number, state: "open" | "closed" | "merged") => + ({ number, state, isDraft: false, labels: [] }) as unknown as PullRequestListEntry; + const key = (row: { number: number }) => `#${row.number}`; + + it("keeps an override until an answer agrees with it", () => { + const at = 1_000_000; + const closed = { state: "closed" as const, updatedAt: "2026-07-03T00:00:00Z", token: 1, at }; + const overrides = new Map([["#1", closed]]); + // A read from before the action still says open: the override stands. + expect(settlePullRequestOverrides(overrides, [entry(1, "open")], key, at + 5_000)).toBe( + overrides, + ); + // Absent from the answer says nothing: the row may live in another group or page. + expect(settlePullRequestOverrides(overrides, [entry(2, "open")], key, at + 5_000).size).toBe(1); + // Present as closed: confirmed. + expect(settlePullRequestOverrides(overrides, [entry(1, "closed")], key, at + 5_000).size).toBe( + 0, + ); + // Present as open a good while later: the host's news, which outranks the note. + expect(settlePullRequestOverrides(overrides, [entry(1, "open")], key, at + 90_000).size).toBe( + 0, + ); + }); + + it("does not hand back the old order when only the order changed", () => { + const previous = [entry(1, "open"), entry(2, "open")]; + const swapped = reusePullRequestEntries(previous, [entry(2, "open"), entry(1, "open")], key); + expect(swapped).not.toBe(previous); + expect(swapped.map((row) => row.number)).toEqual([2, 1]); + }); +}); diff --git a/apps/web/src/components/pullRequest/pullRequestList.logic.ts b/apps/web/src/components/pullRequest/pullRequestList.logic.ts index f001e33829c5..f4cb3a31f46f 100644 --- a/apps/web/src/components/pullRequest/pullRequestList.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestList.logic.ts @@ -9,6 +9,7 @@ import { } from "@t3tools/contracts"; import type { ProjectId, + PullRequestAction, PullRequestActor, PullRequestDiffStat, PullRequestInvolvement, @@ -16,6 +17,7 @@ import type { PullRequestListCursors, PullRequestListFilters, PullRequestListState, + PullRequestState, } from "@t3tools/contracts"; import { toSortableTimestamp } from "../../lib/threadSort"; @@ -1140,3 +1142,127 @@ export function withDiffStat< const stat = statsByRow.get(pullRequestDiffStatKey(entry)); return stat === undefined ? entry : { ...entry, ...stat }; } + +/** + * What a row should say the moment an action is sent, before any host has answered. The host + * is the record and a later read replaces this, but the reader pressed the button and should + * see the row answer at once: a closed pull request leaves an "open" list on the click, not + * after the reads that follow. + */ +export interface PullRequestListOverride { + readonly state: PullRequestState; + readonly isDraft?: boolean; + readonly updatedAt: string; + /** Which action wrote it, so a failure takes back its own note and not a later one's. */ + readonly token: number; + /** When it was written, in the reader's clock. */ + readonly at: number; +} + +export function pullRequestOverrideAfterAction( + entry: Pick, + action: PullRequestAction, + now: Date, + token: number, +): PullRequestListOverride | null { + const stamp = { updatedAt: now.toISOString(), token, at: now.getTime() }; + switch (action) { + case "close": + return { state: "closed", ...stamp }; + case "reopen": + return { state: "open", ...stamp }; + case "merge": + return { state: "merged", ...stamp }; + case "draft": + return { state: entry.state, isDraft: true, ...stamp }; + case "ready": + return { state: entry.state, isDraft: false, ...stamp }; + default: + return null; + } +} + +/** The rows with their pending answers written over them, and the ones the list's state filter no longer holds dropped. */ +export function applyPullRequestOverrides( + entries: ReadonlyArray, + overrides: ReadonlyMap, + keyOf: (entry: Entry) => string, + state: PullRequestListState, +): ReadonlyArray { + if (overrides.size === 0) return entries; + const out: Entry[] = []; + for (const entry of entries) { + const override = overrides.get(keyOf(entry)); + if (override === undefined) { + out.push(entry); + continue; + } + if (state !== "all" && override.state !== state) continue; + out.push({ ...entry, ...override }); + } + return out; +} + +/** + * A fresh answer with the rows it did not change handed back as the objects already held, so a + * memoized row whose data is the same does not render again. Every refresh otherwise rebuilds + * every entry, and a hundred rows repaint for the one that moved. + */ +export function reusePullRequestEntries( + previous: ReadonlyArray, + next: ReadonlyArray, + keyOf: (entry: Entry) => string, +): ReadonlyArray { + if (previous.length === 0) return next; + const held = new Map(previous.map((entry) => [keyOf(entry), entry])); + let reused = 0; + const out = next.map((entry) => { + const before = held.get(keyOf(entry)); + if (before !== undefined && JSON.stringify(before) === JSON.stringify(entry)) { + reused += 1; + return before; + } + return entry; + }); + return reused === next.length && + previous.length === next.length && + previous.every((entry, index) => keyOf(entry) === keyOf(next[index]!)) + ? previous + : out; +} + +/** How long a read that disagrees is taken for a stale one rather than for news. */ +const PULL_REQUEST_OVERRIDE_TRUST_MS = 60_000; + +/** + * The overrides an answer has confirmed, dropped; the rest kept. A read that started before + * the action can land after it and still say the old thing, so an override is not cleared + * because an answer arrived but because the answer agrees: the row is there in the state the + * override said. A row that is absent says nothing — the authored and reviewing groups are read + * apart from the feed, and a page is only a page — so absence never confirms. A row present in + * another state is taken for a stale read for a minute, and for the host's news after that, + * which is how a pull request reopened elsewhere comes back. + */ +export function settlePullRequestOverrides( + overrides: ReadonlyMap, + answered: ReadonlyArray, + keyOf: (entry: Entry) => string, + now: number, +): ReadonlyMap { + if (overrides.size === 0) return overrides; + const byKey = new Map(answered.map((entry) => [keyOf(entry), entry])); + const kept = new Map(); + for (const [key, override] of overrides) { + const row = byKey.get(key); + if (row === undefined) { + kept.set(key, override); + continue; + } + const agrees = + row.state === override.state && + (override.isDraft === undefined || row.isDraft === override.isDraft); + const outranked = now - override.at > PULL_REQUEST_OVERRIDE_TRUST_MS; + if (!agrees && !outranked) kept.set(key, override); + } + return kept.size === overrides.size ? overrides : kept; +} diff --git a/apps/web/src/routes/_chat.pull-requests.tsx b/apps/web/src/routes/_chat.pull-requests.tsx index 485fa7718e3e..59368351359d 100644 --- a/apps/web/src/routes/_chat.pull-requests.tsx +++ b/apps/web/src/routes/_chat.pull-requests.tsx @@ -4,6 +4,7 @@ import { pullRequestHostOf, resolveEnvironmentMachineKind } from "@t3tools/contr import type { EnvironmentId, ProjectId, + PullRequestAction, PullRequestInvolvement, PullRequestListCursors, PullRequestListFilters, @@ -77,6 +78,11 @@ import { type PullRequestStatsPolicy, type PullRequestStatsScope, type PullRequestPartitionsSnapshot, + applyPullRequestOverrides, + type PullRequestListOverride, + pullRequestOverrideAfterAction, + reusePullRequestEntries, + settlePullRequestOverrides, } from "../components/pullRequest/pullRequestList.logic"; import { pullRequestListPreferences, @@ -914,6 +920,36 @@ function PullRequestsRouteView() { key: string; entries: ReadonlyArray; } | null>(null); + // What the reader just did to a row, shown before any host confirms it. A closed pull request + // leaves an "open" list on the click; the reads that follow are slow, and until one lands the + // row says what was asked of it. Cleared when a whole-page answer arrives after the action. + const [overrides, setOverrides] = useState>( + () => new Map(), + ); + const overrideToken = useRef(0); + /** Writes the action's outcome onto the row; the token names this write for a later rollback. */ + const overrideEntry = ( + entry: EnvironmentPullRequestEntry, + action: PullRequestAction, + ): number | null => { + const token = ++overrideToken.current; + const override = pullRequestOverrideAfterAction(entry, action, new Date(), token); + if (override === null) return null; + setOverrides((current) => new Map(current).set(pullRequestEntryKey(entry), override)); + return token; + }; + /** A rollback for one write only: a later action's note over the same row is left alone. */ + const revertOverride = (key: string, token: number | null) => { + if (token === null) return; + setOverrides((current) => { + if (current.get(key)?.token !== token) return current; + const next = new Map(current); + next.delete(key); + return next; + }); + }; + /** The detail panel's own writes, by row, so its failure takes back its own note. */ + const detailOverrideTokens = useRef(new Map()); // A reload recreates the registry the queries live in, so with nothing held the page would // cold-start into skeletons even though almost every row is unchanged. The last answer for // this set of environments is kept across reloads and hydrated here as the carried rows: they @@ -1077,8 +1113,20 @@ function PullRequestsRouteView() { // since the last read at the bottom of the page — below rows a week older — where "the // latest" is exactly what a refresh was for. The host answers in the order the page // reads, so its order stands; a row that moved was updated, and moving is the news. - return { key: filterKey, entries: rankPullRequestMatches(answered.entries, sentParsed.text) }; + return { + key: filterKey, + entries: reusePullRequestEntries( + previous.entries, + rankPullRequestMatches(answered.entries, sentParsed.text), + pullRequestEntryKey, + ), + }; }); + // The host's word outranks the reader's, once it has actually said it: an override is + // cleared by an answer that agrees with it, not by any answer that happens to land. + setOverrides((current) => + settlePullRequestOverrides(current, answered.entries, pullRequestEntryKey, Date.now()), + ); }, [ answered, filterKey, @@ -1455,10 +1503,34 @@ function PullRequestsRouteView() { setStatsByRow((previous) => mergePullRequestDiffStats(previous, stats)); }, [statsQuery.stats]); const displayGroups = useMemo(() => { - const enriched = groups.map((group) => ({ - ...group, - entries: group.entries.map((entry) => withDiffStat(entry, statsByRow)), - })); + // The reader's pending answers go on here, after grouping: the authored and reviewing + // groups are read separately from the feed, and a row closed a moment ago has to leave + // whichever group it was in. + const enriched = groups.map((group) => { + const answered = applyPullRequestOverrides( + group.entries.map((entry) => withDiffStat(entry, statsByRow)), + overrides, + pullRequestEntryKey, + search.state, + ); + // A row whose draft flag just changed has to pass the local filters again: a draft + // filter that let it in may not let the new one in. + return { + ...group, + entries: + hasLocalFilters && overrides.size > 0 + ? answered.filter( + (entry) => + !overrides.has(pullRequestEntryKey(entry)) || + matchesPullRequestFilters( + entry, + localFilters, + pullRequestEntryViewer(entry, viewers), + ), + ) + : answered, + }; + }); // Searching keeps its relevance order and priority groups unless the reader explicitly asks // for another sort. The readiness queue is the default browse order, not a way to bury a // closer text match. @@ -1470,7 +1542,29 @@ function PullRequestsRouteView() { entry.additions + entry.deletions > 0 || statsByRow.has(pullRequestDiffStatKey(entry)), search.involvement, ); - }, [groups, search.involvement, sort, statsByRow, typedParsed.text]); + }, [ + groups, + hasLocalFilters, + localFilters, + overrides, + search.involvement, + search.state, + sort, + statsByRow, + typedParsed.text, + viewers, + ]); + /** What is actually on screen once the reader's pending answers are on the rows. */ + const shownCount = displayGroups.reduce((count, group) => count + group.entries.length, 0); + const heldPullRequestsBySurface = useMemo( + () => + new Map( + groups.flatMap((group) => + group.entries.map((entry) => [pullRequestListEntryId(entry), entry] as const), + ), + ), + [groups], + ); const listedPullRequestsBySurface = useMemo( () => new Map( @@ -1645,7 +1739,7 @@ function PullRequestsRouteView() { // so that case waits with the skeletons rather than answering for the hosts. A search says so // in its own words and is left to. const carriedToNothing = - showingCarried && listQuery.isPending && entries.length === 0 && typedQuery.length === 0; + showingCarried && listQuery.isPending && shownCount === 0 && typedQuery.length === 0; const listBody = ( <> {!capabilityKnown ? ( @@ -1657,7 +1751,7 @@ function PullRequestsRouteView() { /> ) : firstLoad ? ( - ) : listQuery.error && entries.length === 0 ? ( + ) : listQuery.error && shownCount === 0 ? ( ) : carriedToNothing ? ( - ) : entries.length === 0 ? ( + ) : shownCount === 0 ? ( 0} refreshing={refreshing} @@ -1724,7 +1818,7 @@ function PullRequestsRouteView() { )} - {listQuery.error && entries.length > 0 ? ( + {listQuery.error && shownCount > 0 ? (
{listQuery.error} Showing the last pull requests loaded.