diff --git a/apps/mobile/src/features/pull-requests/pullRequestList.test.ts b/apps/mobile/src/features/pull-requests/pullRequestList.test.ts index 59a0f723f7720..7ac4f7f1643a4 100644 --- a/apps/mobile/src/features/pull-requests/pullRequestList.test.ts +++ b/apps/mobile/src/features/pull-requests/pullRequestList.test.ts @@ -13,12 +13,15 @@ import { mergeEnvironmentPullRequestPages, nextPullRequestListInput, PR_LIST_PAGE_SIZE, + patchPullRequestListEntry, pullRequestFilterChoices, pullRequestLabelColors, pullRequestListHead, pullRequestListInput, scopedPullRequestListKey, + shownPullRequestListPages, pullRequestListRowsAreEqual, + withShownPullRequestStats, withoutScratchProjectScope, } from "./pullRequestList"; @@ -115,6 +118,14 @@ describe("mobile PR list pagination", () => { mergePullRequestListEntries([entry(1)], [entry(2, { host: "enterprise.test" }), entry(2)]), ).toHaveLength(3); }); + it("patches a row that only the list head supplies, and keeps it while that head is current", () => { + const head = page([entry(2), entry(1)], { repo: "first" }); + const patched = patchPullRequestListEntry(null, head, entry(1), { state: "merged" }, 5); + expect(shownPullRequestListPages(patched, head)?.entries.map((row) => row.state)).toEqual([ + "open", + "merged", + ]); + }); it("refreshes the head without dropping older loaded pages or rewinding their cursor", () => { const first = pullRequestListHead(null, page([entry(3)], { repo: "first" })); const continued = appendPullRequestListPage(first, page([entry(2)], { repo: "second" }), { @@ -387,3 +398,40 @@ describe("pullRequestFilterChoices", () => { expect(rows[50]).toBe("label-from-host"); }); }); + +describe("shown PR diff stats", () => { + const cups = EnvironmentId.make("cups"); + const stat = (number: number, additions: number, deletions: number) => ({ + projectId: "project" as PullRequestListEntry["projectId"], + repository: "org/repo", + number, + additions, + deletions, + }); + const rows = [ + { ...entry(1), environmentId: cups }, + { ...entry(2, { additions: 4, deletions: 1 }), environmentId: cups }, + ]; + + it("keeps the same list when the answers change no row", () => { + expect(withShownPullRequestStats(rows, [])).toBe(rows); + expect(withShownPullRequestStats(rows, [{ environmentId: cups, stats: [stat(1, 0, 0)] }])).toBe( + rows, + ); + expect(withShownPullRequestStats(rows, [{ environmentId: cups, stats: [stat(2, 9, 9)] }])).toBe( + rows, + ); + }); + + it("fills only rows without stats, in their own environment", () => { + const filled = withShownPullRequestStats(rows, [ + { environmentId: EnvironmentId.make("mbp"), stats: [stat(1, 7, 7)] }, + { environmentId: cups, stats: [stat(1, 3, 2), stat(2, 9, 9)] }, + ]); + expect(filled.map(({ additions, deletions }) => [additions, deletions])).toEqual([ + [3, 2], + [4, 1], + ]); + expect(filled[1]).toBe(rows[1]); + }); +}); diff --git a/apps/mobile/src/features/pull-requests/pullRequestList.ts b/apps/mobile/src/features/pull-requests/pullRequestList.ts index 8040c12d6d84d..0b0f7cfd4e005 100644 --- a/apps/mobile/src/features/pull-requests/pullRequestList.ts +++ b/apps/mobile/src/features/pull-requests/pullRequestList.ts @@ -3,6 +3,7 @@ import { ProjectId, resolvePullRequestAuthorFilter, type EnvironmentId, + type PullRequestDiffStat, type PullRequestListEntry, type PullRequestListInput, type PullRequestListResult, @@ -45,6 +46,38 @@ export type EnvironmentPullRequestListEntry = PullRequestListEntry & { export const scopedPullRequestListKey = (entry: EnvironmentPullRequestListEntry) => `${entry.environmentId}:${pullRequestListKey(entry)}`; +export const pullRequestStatKey = ( + row: Pick, +) => `${row.projectId}:${row.repository}:${row.number}`; + +/** + * Fills rows that have no diff stat yet from the stats answers on screen. Returns `entries` + * itself when no row changes, so the list keeps its identity across renders. + */ +export function withShownPullRequestStats( + entries: readonly EnvironmentPullRequestListEntry[], + answers: readonly { + readonly environmentId: EnvironmentId; + readonly stats: readonly PullRequestDiffStat[]; + }[], +): readonly EnvironmentPullRequestListEntry[] { + const shown = new Map( + answers.flatMap(({ environmentId, stats }) => + stats.map((row) => [`${environmentId}:${pullRequestStatKey(row)}`, row] as const), + ), + ); + if (!shown.size) return entries; + let changed = false; + const next = entries.map((entry) => { + const stat = shown.get(`${entry.environmentId}:${pullRequestStatKey(entry)}`); + if (!stat || entry.additions + entry.deletions !== 0) return entry; + if (stat.additions + stat.deletions === 0) return entry; + changed = true; + return { ...entry, additions: stat.additions, deletions: stat.deletions }; + }); + return changed ? next : entries; +} + export function pullRequestListRowsAreEqual( previous: EnvironmentPullRequestListEntry, next: EnvironmentPullRequestListEntry, @@ -143,6 +176,34 @@ export function pullRequestListHead( }; } +/** The recorded pages with the current list head on top; before any answer is recorded, the head alone. */ +export function shownPullRequestListPages( + recorded: PullRequestListPages | null, + head: PullRequestListResult | null, +): PullRequestListPages | null { + return head ? pullRequestListHead(recorded, head) : recorded; +} + +export function patchPullRequestListEntry( + recorded: PullRequestListPages | null, + head: PullRequestListResult | null, + entry: PullRequestListEntry, + patch: Partial, + observedAt: number, +): PullRequestListPages | null { + const pages = shownPullRequestListPages(recorded, head); + return ( + pages && { + ...pages, + entries: pages.entries.map((row) => + pullRequestListKey(row) === pullRequestListKey(entry) + ? { ...row, ...patch, observedAt } + : row, + ), + } + ); +} + export function pullRequestListInput( controls: PullRequestListControls, query: string, diff --git a/apps/mobile/src/features/pull-requests/usePullRequestList.ts b/apps/mobile/src/features/pull-requests/usePullRequestList.ts index 5656e1d18ab5f..73dad15cafd2f 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestList.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestList.ts @@ -1,155 +1,172 @@ import { RegistryContext, useAtomValue } from "@effect/atom-react"; import { executeAtomQuery, squashAtomCommandFailure } from "@t3tools/client-runtime/state/runtime"; -import type { EnvironmentId, PullRequestDiffStat, PullRequestListInput } from "@t3tools/contracts"; +import type { EnvironmentId, PullRequestListInput } from "@t3tools/contracts"; import * as Cause from "effect/Cause"; +import * as Effect from "effect/Effect"; import * as Option from "effect/Option"; -import { AsyncResult, Atom } from "effect/reactivity"; -import { use, useCallback, useEffect, useMemo, useRef, useState, useLayoutEffect } from "react"; -import { composerPullRequests } from "../../state/pull-requests"; +import { AsyncResult, Atom, type AtomRegistry } from "effect/reactivity"; +import { use, useCallback, useMemo, useState } from "react"; +import { + composerPullRequests, + pullRequestListPagesFamily, + pullRequestListPagesKey, + pullRequestListStatsFamily, +} from "../../state/pull-requests"; import { appendPullRequestListPage, mergeEnvironmentPullRequestPages, nextPullRequestListInput, + patchPullRequestListEntry, pullRequestListHead, - pullRequestListKey, + shownPullRequestListPages, + pullRequestStatKey as statKey, scopedPullRequestListKey, + withShownPullRequestStats, type EnvironmentPullRequestListEntry, type PullRequestListPages, } from "./pullRequestList"; -const pagesFamily = Atom.family((_key: string) => - Atom.make(null).pipe(Atom.setIdleTTL(5 * 60_000)), -); -const statsFamily = Atom.family((_environmentId: string) => - Atom.make>(new Map()).pipe(Atom.setIdleTTL(5 * 60_000)), -); -const statKey = (row: Pick) => - `${row.projectId}:${row.repository}:${row.number}`; - type ListTarget = { readonly environmentId: EnvironmentId; readonly input: PullRequestListInput }; +type Read = (atom: Atom.Atom) => A; + +/** The head the list query holds now, cached or answered. */ +function currentHead(read: Read, target: ListTarget) { + return Option.getOrNull(AsyncResult.value(read(composerPullRequests.list(target)))); +} + +function currentPages(read: Read, target: ListTarget): PullRequestListPages | null { + return shownPullRequestListPages( + read(pullRequestListPagesFamily(pullRequestListPagesKey(target))), + currentHead(read, target), + ); +} + const sourcesFamily = Atom.family((key: string) => { const targets: readonly ListTarget[] = JSON.parse(key); return Atom.make((get) => - targets.map((target) => { - const result = get(composerPullRequests.list(target)); - const head = Option.getOrNull(AsyncResult.value(result)); - const cached = get(pagesFamily(JSON.stringify(target))); - return { - target, - result, - pages: head ? pullRequestListHead(cached, head) : cached, - stats: get(statsFamily(target.environmentId)), - }; - }), + targets.map((target) => ({ + target, + result: get(composerPullRequests.list(target)), + pages: currentPages(get, target), + stats: get(pullRequestListStatsFamily(target.environmentId)), + })), ); }); -export function usePullRequestList(targets: readonly ListTarget[]) { - const registry = use(RegistryContext); - const scopeKey = JSON.stringify(targets); - const activeScope = useRef(scopeKey); - const sources = useAtomValue(sourcesFamily(scopeKey)); - const requests = useRef<{ page?: AbortController; refresh?: AbortController }>({}); - const [loading, setLoading] = useState({ - key: scopeKey, - page: false, - refresh: false, - error: null as string | null, +function continuations(registry: AtomRegistry.AtomRegistry, targets: readonly ListTarget[]) { + return targets.flatMap((target) => { + const pages = currentPages((atom) => registry.get(atom), target); + const input = pages && nextPullRequestListInput(pages, target.input); + return pages && input ? [{ target, pages, input }] : []; }); - const state = loading.key === scopeKey ? loading : { page: false, refresh: false, error: null }; - useLayoutEffect(() => { - activeScope.current = scopeKey; - return () => { - requests.current.page?.abort(); - requests.current.refresh?.abort(); - requests.current = {}; - }; - }, [scopeKey, activeScope]); - useEffect(() => { - for (const { target, pages } of sources) - if (pages) registry.set(pagesFamily(JSON.stringify(target)), pages); - }, [registry, sources]); +} + +async function loadNextPages( + registry: AtomRegistry.AtomRegistry, + targets: readonly ListTarget[], + retry: boolean, + signal: AbortSignal, +) { + const responses = await Promise.allSettled( + continuations(registry, targets).map(async ({ target, pages, input }) => { + const response = await executeAtomQuery( + registry, + composerPullRequests.list({ ...target, input }), + { + signal, + refresh: retry, + reportFailure: false, + }, + ); + if (signal.aborted) return; + if (response._tag === "Failure") throw squashAtomCommandFailure(response); + if (response.value.errors.length) + throw new Error(response.value.errors.map((error) => error.message).join("\n")); + const pagesAtom = pullRequestListPagesFamily(pullRequestListPagesKey(target)); + const next = appendPullRequestListPage( + currentPages((atom) => registry.get(atom), target) ?? pages, + response.value, + input, + ); + registry.set(pagesAtom, next); + if ( + input.cursors && + Object.keys(next.nextCursors).length && + JSON.stringify(Object.entries(input.cursors).sort()) === + JSON.stringify(Object.entries(next.nextCursors).sort()) + ) + throw new Error("Couldn't advance the list. Try again."); + }), + ); + return requestErrors(responses); +} - const loadMore = useCallback( - async (retry = false) => { - if (requests.current.page || requests.current.refresh || (!retry && state.error)) return; - const continuations = targets.flatMap((target) => { - const pagesAtom = pagesFamily(JSON.stringify(target)); - const pages = registry.get(pagesAtom); - const input = pages && nextPullRequestListInput(pages, target.input); - return pages && input ? [{ target, pagesAtom, pages, input }] : []; +async function refreshHeads( + registry: AtomRegistry.AtomRegistry, + targets: readonly ListTarget[], + signal: AbortSignal, +) { + const responses = await Promise.allSettled( + targets.map(async (target) => { + await composerPullRequests.invalidate.run(registry, { + environmentId: target.environmentId, + input: {}, }); - if (!continuations.length) return; - const controller = new AbortController(); - requests.current.page = controller; - setLoading({ key: scopeKey, page: true, refresh: false, error: null }); - const responses = await Promise.allSettled( - continuations.map(async ({ target, pagesAtom, pages, input }) => { - const response = await executeAtomQuery( - registry, - composerPullRequests.list({ ...target, input }), - { - signal: controller.signal, - refresh: retry, - reportFailure: false, - }, - ); - if (controller.signal.aborted || activeScope.current !== scopeKey) return; - if (response._tag === "Failure") throw squashAtomCommandFailure(response); - if (response.value.errors.length) - throw new Error(response.value.errors.map((error) => error.message).join("\n")); - const next = appendPullRequestListPage( - registry.get(pagesAtom) ?? pages, - response.value, - input, - ); - registry.set(pagesAtom, next); - if ( - input.cursors && - Object.keys(next.nextCursors).length && - JSON.stringify(Object.entries(input.cursors).sort()) === - JSON.stringify(Object.entries(next.nextCursors).sort()) - ) - throw new Error("Couldn't advance the list. Try again."); - }), + if (signal.aborted) return; + const response = await executeAtomQuery(registry, composerPullRequests.list(target), { + refresh: true, + signal, + reportFailure: false, + }); + if (signal.aborted) return; + if (response._tag === "Failure") throw squashAtomCommandFailure(response); + registry.set( + pullRequestListPagesFamily(pullRequestListPagesKey(target)), + pullRequestListHead(null, response.value), ); - if (requests.current.page === controller) delete requests.current.page; - if (!controller.signal.aborted && activeScope.current === scopeKey) - setLoading({ key: scopeKey, page: false, refresh: false, error: requestErrors(responses) }); - }, - [targets, registry, scopeKey, state.error, activeScope], + }), ); + return requestErrors(responses); +} - const onRefresh = async () => { - if (!targets.length || requests.current.refresh) return; - requests.current.page?.abort(); - delete requests.current.page; - const controller = new AbortController(); - requests.current.refresh = controller; - setLoading({ key: scopeKey, page: false, refresh: true, error: null }); - const responses = await Promise.allSettled( - targets.map(async (target) => { - await composerPullRequests.invalidate.run(registry, { - environmentId: target.environmentId, - input: {}, - }); - if (controller.signal.aborted) return; - const response = await executeAtomQuery(registry, composerPullRequests.list(target), { - refresh: true, - signal: controller.signal, - reportFailure: false, - }); - if (controller.signal.aborted || activeScope.current !== scopeKey) return; - if (response._tag === "Failure") throw squashAtomCommandFailure(response); - registry.set( - pagesFamily(JSON.stringify(target)), - pullRequestListHead(null, response.value), - ); - }), - ); - if (requests.current.refresh === controller) delete requests.current.refresh; - if (!controller.signal.aborted && activeScope.current === scopeKey) - setLoading({ key: scopeKey, page: false, refresh: false, error: requestErrors(responses) }); +// One load of each kind per list scope. The atoms are not kept alive, so leaving a scope (a +// filter change, unmount) interrupts its load and aborts the requests. +const pageLoadFamily = Atom.family((scopeKey: string) => + Atom.fn((retry: boolean, get) => + Effect.promise((signal) => loadNextPages(get.registry, JSON.parse(scopeKey), retry, signal)), + ), +); +const refreshLoadFamily = Atom.family((scopeKey: string) => + Atom.fn((_: void, get) => + Effect.promise((signal) => refreshHeads(get.registry, JSON.parse(scopeKey), signal)), + ), +); + +const settledError = (result: AsyncResult.AsyncResult) => + AsyncResult.isSuccess(result) && !result.waiting ? result.value : null; + +export function usePullRequestList(targets: readonly ListTarget[]) { + const registry = use(RegistryContext); + const scopeKey = JSON.stringify(targets); + const sources = useAtomValue(sourcesFamily(scopeKey)); + const pageAtom = pageLoadFamily(scopeKey); + const refreshAtom = refreshLoadFamily(scopeKey); + const page = useAtomValue(pageAtom); + const refresh = useAtomValue(refreshAtom); + // Starting one kind of load clears the other, so at most one holds an error. + const loadError = settledError(refresh) ?? settledError(page); + + const loadMore = (retry = false) => { + if (page.waiting || refresh.waiting || (!retry && loadError)) return; + if (!continuations(registry, targets).length) return; + registry.set(refreshAtom, Atom.Reset); + registry.set(pageAtom, retry); + }; + + const onRefresh = () => { + if (!targets.length || refresh.waiting) return; + registry.set(pageAtom, Atom.Reset); + registry.set(refreshAtom, undefined); }; const data = useMemo( @@ -199,10 +216,18 @@ export function usePullRequestList(targets: readonly ListTarget[]) { ); const visibleRows = visible.key === scopeKey && visible.rows.length ? visible.rows : data.entries.slice(0, 8); + // Visible rows carry the cached-answer overlay below. Decide which still need stats on the rows + // without it, so a cached answer shown offline does not unmount the query that holds it. + const withoutCachedOverlay = new Map( + data.entries.map((row) => [scopedPullRequestListKey(row), row]), + ); const statsKey = JSON.stringify( targets.flatMap(({ environmentId }) => { const refs = visibleRows - .filter((row) => row.environmentId === environmentId && row.additions + row.deletions === 0) + .filter((row) => { + const base = withoutCachedOverlay.get(scopedPullRequestListKey(row)) ?? row; + return row.environmentId === environmentId && base.additions + base.deletions === 0; + }) .map(({ projectId, host, repository, number }) => ({ projectId, host, @@ -223,33 +248,33 @@ export function usePullRequestList(targets: readonly ListTarget[]) { ); }, [statsKey]); const counts = useAtomValue(statsAtom); - useEffect(() => { - for (const { environmentId, result } of counts) { - if (result._tag !== "Success" || result.waiting) continue; - registry.update(statsFamily(environmentId), (previous) => { - const next = new Map(previous); - for (const row of result.value.stats) next.set(statKey(row), row); - return next; - }); - } - }, [registry, counts]); + // Server answers land in pullRequestListStatsFamily; these cover a cached answer, which is + // all an offline environment has. + const entries = useMemo( + () => + withShownPullRequestStats( + data.entries, + counts.flatMap(({ environmentId, result }) => + AsyncResult.isSuccess(result) ? [{ environmentId, stats: result.value.stats }] : [], + ), + ), + [data.entries, counts], + ); const updateEntry = useCallback( (entry: EnvironmentPullRequestListEntry, patch: Partial) => { const target = targets.find((target) => target.environmentId === entry.environmentId); if (!target) return; - registry.update(pagesFamily(JSON.stringify(target)), (previous) => - previous - ? { - ...previous, - entries: previous.entries.map((row) => - pullRequestListKey(row) === pullRequestListKey(entry) - ? { ...row, ...patch, observedAt: Date.now() } - : row, - ), - } - : previous, + const pagesAtom = pullRequestListPagesFamily(pullRequestListPagesKey(target)); + const read = (atom: Atom.Atom) => registry.get(atom); + const patched = patchPullRequestListEntry( + read(pagesAtom), + currentHead(read, target), + entry, + patch, + Date.now(), ); + if (patched) registry.set(pagesAtom, patched); registry.refresh(composerPullRequests.list(target)); }, [registry, targets], @@ -269,10 +294,13 @@ export function usePullRequestList(targets: readonly ListTarget[]) { }), [sources], ); - const listing = useMemo(() => ({ ...data, errors: [...data.errors, ...errors] }), [data, errors]); + const listing = useMemo( + () => ({ ...data, entries, errors: [...data.errors, ...errors] }), + [data, entries, errors], + ); return { data: listing, - entries: data.entries, + entries, error: errors.map((error) => error.message).join("\n") || null, isPending: sources.some(({ result }) => result.waiting) && !data.entries.length, continued: data.continued, @@ -280,9 +308,9 @@ export function usePullRequestList(targets: readonly ListTarget[]) { onRefresh, onViewableItemsChanged, updateEntry, - refreshing: state.refresh, - loadingMore: state.page, - loadMoreError: state.error, + refreshing: refresh.waiting, + loadingMore: page.waiting, + loadMoreError: loadError, hasMore: sources.some( ({ pages, target }) => pages && nextPullRequestListInput(pages, target.input), ), diff --git a/apps/mobile/src/state/pull-requests.ts b/apps/mobile/src/state/pull-requests.ts index d5b03a9a1bb28..5d6b5980fa9b0 100644 --- a/apps/mobile/src/state/pull-requests.ts +++ b/apps/mobile/src/state/pull-requests.ts @@ -2,9 +2,49 @@ import { createPullRequestEnvironmentAtoms, createPullRequestStackAtomFamily, } from "@t3tools/client-runtime/state/pull-requests"; +import type { EnvironmentId, PullRequestDiffStat, PullRequestListInput } from "@t3tools/contracts"; +import * as Effect from "effect/Effect"; +import { Atom } from "effect/reactivity"; import { connectionAtomRuntime } from "../connection/runtime"; +import { + pullRequestListHead, + pullRequestStatKey, + type PullRequestListPages, +} from "../features/pull-requests/pullRequestList"; + +export const pullRequestListPagesKey = (target: { + readonly environmentId: EnvironmentId; + readonly input: PullRequestListInput; +}) => JSON.stringify([target.environmentId, target.input]); + +/** The pages loaded past each listing's head, keyed by {@link pullRequestListPagesKey}. */ +export const pullRequestListPagesFamily = Atom.family((_key: string) => + Atom.make(null).pipe(Atom.setIdleTTL(5 * 60_000)), +); + +/** Diff stats each environment has answered so far, so rows keep them when they scroll away. */ +export const pullRequestListStatsFamily = Atom.family((_environmentId: string) => + Atom.make>(new Map()).pipe(Atom.setIdleTTL(5 * 60_000)), +); -export const composerPullRequests = createPullRequestEnvironmentAtoms(connectionAtomRuntime); +export const composerPullRequests = createPullRequestEnvironmentAtoms(connectionAtomRuntime, { + list: (target, value, registry) => + target.input.cursors === undefined + ? Effect.sync(() => + registry.update(pullRequestListPagesFamily(pullRequestListPagesKey(target)), (previous) => + pullRequestListHead(previous, value), + ), + ) + : Effect.void, + listStats: (target, value, registry) => + Effect.sync(() => + registry.update(pullRequestListStatsFamily(target.environmentId), (previous) => { + const next = new Map(previous); + for (const row of value.stats) next.set(pullRequestStatKey(row), row); + return next; + }), + ), +}); export const pullRequestStackAtom = createPullRequestStackAtomFamily( connectionAtomRuntime, composerPullRequests.refreshes, diff --git a/packages/client-runtime/src/state/pullRequests.ts b/packages/client-runtime/src/state/pullRequests.ts index 94b509280934d..6b675f36a7858 100644 --- a/packages/client-runtime/src/state/pullRequests.ts +++ b/packages/client-runtime/src/state/pullRequests.ts @@ -14,6 +14,8 @@ import { type PullRequestActor, type PullRequestActionInput, type PullRequestListInput, + type PullRequestListStatsInput, + type PullRequestListStatsResult, type PullRequestDetail, type PullRequestDiffInput, type PullRequestRef, @@ -144,6 +146,7 @@ export function createLinkedPullRequestSummaryAtomFamily( /** Lets a client record what each answer says where it lands, rather than watching the data. */ export interface PullRequestAnswerHandlers { readonly list?: QueryAnswerHandler; + readonly listStats?: QueryAnswerHandler; readonly detail?: QueryAnswerHandler; readonly checks?: QueryAnswerHandler; readonly reviewGuide?: QueryAnswerHandler; @@ -357,6 +360,7 @@ export function createPullRequestEnvironmentAtoms( tag: WS_METHODS.pullRequestsListStats, staleTimeMs: 60_000, refreshTrigger: ({ environmentId }) => refreshes({ environmentId, input: {} }), + ...(onAnswer.listStats === undefined ? {} : { onAnswer: onAnswer.listStats }), }), filterOptions: createEnvironmentRpcQueryAtomFamily(runtime, { label: "environment-data:pull-requests:filter-options",