From 8c12cf3096bccc94e3cb794a2c315dbbd6747512 Mon Sep 17 00:00:00 2001 From: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com> Date: Sun, 25 Oct 2026 23:48:35 -0700 Subject: [PATCH 1/4] refactor(mobile): pull request list records answers where they land The mobile pull request list copied every list answer back into its pages atom from a passive effect, copied every stats answer into a stats atom from another, and tracked load/refresh state with refs and a layout effect that aborted in-flight requests on scope change. List head and stats answers are now recorded by the composerPullRequests onAnswer handlers (adds a listStats handler slot in client-runtime), the pages and stats atoms move to state/pull-requests, and page/refresh loads are per-scope Atom.fn atoms, so leaving a scope interrupts its load. Effects: usePullRequestList.ts 3 -> 0. state/pull-requests.ts 0 -> 0. Co-Authored-By: Claude Opus 5.5 --- .../pull-requests/usePullRequestList.ts | 305 ++++++++++-------- apps/mobile/src/state/pull-requests.ts | 45 ++- .../client-runtime/src/state/pullRequests.ts | 4 + 3 files changed, 212 insertions(+), 142 deletions(-) diff --git a/apps/mobile/src/features/pull-requests/usePullRequestList.ts b/apps/mobile/src/features/pull-requests/usePullRequestList.ts index 5656e1d18ab5f..7affcbe33d0d7 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestList.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestList.ts @@ -1,11 +1,18 @@ 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, + pullRequestStatKey as statKey, +} from "../../state/pull-requests"; import { appendPullRequestListPage, mergeEnvironmentPullRequestPages, @@ -17,139 +24,142 @@ import { 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 loaded pages, with the head the list query holds now (cached or answered) on top. */ +function currentPages(read: Read, target: ListTarget): PullRequestListPages | null { + const head = Option.getOrNull(AsyncResult.value(read(composerPullRequests.list(target)))); + const cached = read(pullRequestListPagesFamily(pullRequestListPagesKey(target))); + return head ? pullRequestListHead(cached, head) : cached; +} + 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]); +} - 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 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); +} + +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 +209,16 @@ export function usePullRequestList(targets: readonly ListTarget[]) { ); const visibleRows = visible.key === scopeKey && visible.rows.length ? visible.rows : data.entries.slice(0, 8); + // Decided on the rows without the stats below, so a cached answer shown offline does not + // unmount the query that holds it. + const unstatted = 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 = unstatted.get(scopedPullRequestListKey(row)) ?? row; + return row.environmentId === environmentId && base.additions + base.deletions === 0; + }) .map(({ projectId, host, repository, number }) => ({ projectId, host, @@ -223,22 +239,29 @@ 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 shownStats = new Map( + counts.flatMap(({ environmentId, result }) => + AsyncResult.isSuccess(result) + ? result.value.stats.map((row) => [`${environmentId}:${statKey(row)}`, row] as const) + : [], + ), + ); + const entries = shownStats.size + ? data.entries.map((entry) => { + const stat = shownStats.get(`${entry.environmentId}:${statKey(entry)}`); + return stat && entry.additions + entry.deletions === 0 + ? { ...entry, additions: stat.additions, deletions: stat.deletions } + : entry; + }) + : data.entries; 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) => + registry.update(pullRequestListPagesFamily(pullRequestListPagesKey(target)), (previous) => previous ? { ...previous, @@ -269,10 +292,10 @@ export function usePullRequestList(targets: readonly ListTarget[]) { }), [sources], ); - const listing = useMemo(() => ({ ...data, errors: [...data.errors, ...errors] }), [data, errors]); + const listing = { ...data, entries, errors: [...data.errors, ...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 +303,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..6892d9700283c 100644 --- a/apps/mobile/src/state/pull-requests.ts +++ b/apps/mobile/src/state/pull-requests.ts @@ -2,9 +2,52 @@ 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, + 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 pullRequestStatKey = ( + row: Pick, +) => `${row.projectId}:${row.repository}:${row.number}`; -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", From c2dc39493073c1d9f6d07d0ed7ee92f9180bdf2b Mon Sep 17 00:00:00 2001 From: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com> Date: Sun, 25 Oct 2026 23:54:31 -0700 Subject: [PATCH 2/4] fix(mobile): pull request list keeps entry identity across renders The cached-stats overlay rebuilt the entries array on every render whenever a stats answer was on screen, which fed PullRequestsScreen's facets setState into a render loop. The overlay is now a pure helper that returns the same list when no row changes, memoized on the list and the stats answers, and the listing object is memoized again. Co-Authored-By: Claude Opus 5.5 --- .../pull-requests/pullRequestList.test.ts | 38 +++++++++++++++++ .../features/pull-requests/pullRequestList.ts | 33 +++++++++++++++ .../pull-requests/usePullRequestList.ts | 41 ++++++++++--------- apps/mobile/src/state/pull-requests.ts | 5 +-- 4 files changed, 93 insertions(+), 24 deletions(-) diff --git a/apps/mobile/src/features/pull-requests/pullRequestList.test.ts b/apps/mobile/src/features/pull-requests/pullRequestList.test.ts index 59a0f723f7720..3ce5b0b2cfc02 100644 --- a/apps/mobile/src/features/pull-requests/pullRequestList.test.ts +++ b/apps/mobile/src/features/pull-requests/pullRequestList.test.ts @@ -19,6 +19,7 @@ import { pullRequestListInput, scopedPullRequestListKey, pullRequestListRowsAreEqual, + withShownPullRequestStats, withoutScratchProjectScope, } from "./pullRequestList"; @@ -387,3 +388,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..a9974cb29c35a 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, diff --git a/apps/mobile/src/features/pull-requests/usePullRequestList.ts b/apps/mobile/src/features/pull-requests/usePullRequestList.ts index 7affcbe33d0d7..b54c045928136 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestList.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestList.ts @@ -11,7 +11,6 @@ import { pullRequestListPagesFamily, pullRequestListPagesKey, pullRequestListStatsFamily, - pullRequestStatKey as statKey, } from "../../state/pull-requests"; import { appendPullRequestListPage, @@ -19,7 +18,9 @@ import { nextPullRequestListInput, pullRequestListHead, pullRequestListKey, + pullRequestStatKey as statKey, scopedPullRequestListKey, + withShownPullRequestStats, type EnvironmentPullRequestListEntry, type PullRequestListPages, } from "./pullRequestList"; @@ -209,14 +210,16 @@ export function usePullRequestList(targets: readonly ListTarget[]) { ); const visibleRows = visible.key === scopeKey && visible.rows.length ? visible.rows : data.entries.slice(0, 8); - // Decided on the rows without the stats below, so a cached answer shown offline does not - // unmount the query that holds it. - const unstatted = new Map(data.entries.map((row) => [scopedPullRequestListKey(row), row])); + // 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) => { - const base = unstatted.get(scopedPullRequestListKey(row)) ?? row; + const base = withoutCachedOverlay.get(scopedPullRequestListKey(row)) ?? row; return row.environmentId === environmentId && base.additions + base.deletions === 0; }) .map(({ projectId, host, repository, number }) => ({ @@ -241,21 +244,16 @@ export function usePullRequestList(targets: readonly ListTarget[]) { const counts = useAtomValue(statsAtom); // Server answers land in pullRequestListStatsFamily; these cover a cached answer, which is // all an offline environment has. - const shownStats = new Map( - counts.flatMap(({ environmentId, result }) => - AsyncResult.isSuccess(result) - ? result.value.stats.map((row) => [`${environmentId}:${statKey(row)}`, row] as const) - : [], - ), + const entries = useMemo( + () => + withShownPullRequestStats( + data.entries, + counts.flatMap(({ environmentId, result }) => + AsyncResult.isSuccess(result) ? [{ environmentId, stats: result.value.stats }] : [], + ), + ), + [data.entries, counts], ); - const entries = shownStats.size - ? data.entries.map((entry) => { - const stat = shownStats.get(`${entry.environmentId}:${statKey(entry)}`); - return stat && entry.additions + entry.deletions === 0 - ? { ...entry, additions: stat.additions, deletions: stat.deletions } - : entry; - }) - : data.entries; const updateEntry = useCallback( (entry: EnvironmentPullRequestListEntry, patch: Partial) => { @@ -292,7 +290,10 @@ export function usePullRequestList(targets: readonly ListTarget[]) { }), [sources], ); - const listing = { ...data, entries, errors: [...data.errors, ...errors] }; + const listing = useMemo( + () => ({ ...data, entries, errors: [...data.errors, ...errors] }), + [data, entries, errors], + ); return { data: listing, entries, diff --git a/apps/mobile/src/state/pull-requests.ts b/apps/mobile/src/state/pull-requests.ts index 6892d9700283c..5d6b5980fa9b0 100644 --- a/apps/mobile/src/state/pull-requests.ts +++ b/apps/mobile/src/state/pull-requests.ts @@ -8,6 +8,7 @@ import { Atom } from "effect/reactivity"; import { connectionAtomRuntime } from "../connection/runtime"; import { pullRequestListHead, + pullRequestStatKey, type PullRequestListPages, } from "../features/pull-requests/pullRequestList"; @@ -26,10 +27,6 @@ export const pullRequestListStatsFamily = Atom.family((_environmentId: string) = Atom.make>(new Map()).pipe(Atom.setIdleTTL(5 * 60_000)), ); -export const pullRequestStatKey = ( - row: Pick, -) => `${row.projectId}:${row.repository}:${row.number}`; - export const composerPullRequests = createPullRequestEnvironmentAtoms(connectionAtomRuntime, { list: (target, value, registry) => target.input.cursors === undefined From 250f87106f27886f6aff825bd9dffe5a126d4eca Mon Sep 17 00:00:00 2001 From: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com> Date: Mon, 26 Oct 2026 14:10:38 -0700 Subject: [PATCH 3/4] fix(mobile): pull request row updates apply before the list's first answer is recorded Co-Authored-By: Claude Opus 5.5 --- .../pull-requests/pullRequestList.test.ts | 14 +++++++++++++ .../features/pull-requests/pullRequestList.ts | 16 ++++++++++++++ .../pull-requests/usePullRequestList.ts | 21 +++++++------------ 3 files changed, 38 insertions(+), 13 deletions(-) diff --git a/apps/mobile/src/features/pull-requests/pullRequestList.test.ts b/apps/mobile/src/features/pull-requests/pullRequestList.test.ts index 3ce5b0b2cfc02..f0a37296bceb2 100644 --- a/apps/mobile/src/features/pull-requests/pullRequestList.test.ts +++ b/apps/mobile/src/features/pull-requests/pullRequestList.test.ts @@ -13,6 +13,7 @@ import { mergeEnvironmentPullRequestPages, nextPullRequestListInput, PR_LIST_PAGE_SIZE, + patchPullRequestListEntry, pullRequestFilterChoices, pullRequestLabelColors, pullRequestListHead, @@ -116,6 +117,19 @@ describe("mobile PR list pagination", () => { mergePullRequestListEntries([entry(1)], [entry(2, { host: "enterprise.test" }), entry(2)]), ).toHaveLength(3); }); + it("keeps a patch made on head-only pages while that head is current", () => { + const head = page([entry(2), entry(1)], { repo: "first" }); + const patched = patchPullRequestListEntry( + pullRequestListHead(null, head), + entry(1), + { state: "merged" }, + 5, + ); + expect(pullRequestListHead(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" }), { diff --git a/apps/mobile/src/features/pull-requests/pullRequestList.ts b/apps/mobile/src/features/pull-requests/pullRequestList.ts index a9974cb29c35a..b98eb8187c842 100644 --- a/apps/mobile/src/features/pull-requests/pullRequestList.ts +++ b/apps/mobile/src/features/pull-requests/pullRequestList.ts @@ -176,6 +176,22 @@ export function pullRequestListHead( }; } +export function patchPullRequestListEntry( + pages: PullRequestListPages, + entry: PullRequestListEntry, + patch: Partial, + observedAt: number, +): PullRequestListPages { + return { + ...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 b54c045928136..83171e2d512a9 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestList.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestList.ts @@ -16,8 +16,8 @@ import { appendPullRequestListPage, mergeEnvironmentPullRequestPages, nextPullRequestListInput, + patchPullRequestListEntry, pullRequestListHead, - pullRequestListKey, pullRequestStatKey as statKey, scopedPullRequestListKey, withShownPullRequestStats, @@ -259,18 +259,13 @@ export function usePullRequestList(targets: readonly ListTarget[]) { (entry: EnvironmentPullRequestListEntry, patch: Partial) => { const target = targets.find((target) => target.environmentId === entry.environmentId); if (!target) return; - registry.update(pullRequestListPagesFamily(pullRequestListPagesKey(target)), (previous) => - previous - ? { - ...previous, - entries: previous.entries.map((row) => - pullRequestListKey(row) === pullRequestListKey(entry) - ? { ...row, ...patch, observedAt: Date.now() } - : row, - ), - } - : previous, - ); + // Patch what the list shows: before any answer is recorded, the head alone supplies the rows. + const pages = currentPages((atom) => registry.get(atom), target); + if (pages) + registry.set( + pullRequestListPagesFamily(pullRequestListPagesKey(target)), + patchPullRequestListEntry(pages, entry, patch, Date.now()), + ); registry.refresh(composerPullRequests.list(target)); }, [registry, targets], From e3e1a9eaa6f824e46bc01fec89b2c19dab9be79e Mon Sep 17 00:00:00 2001 From: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com> Date: Mon, 26 Oct 2026 14:13:25 -0700 Subject: [PATCH 4/4] test(mobile): cover patching a pull request row before the list records an answer Co-Authored-By: Claude Opus 5.5 --- .../pull-requests/pullRequestList.test.ts | 12 +++---- .../features/pull-requests/pullRequestList.ts | 32 +++++++++++++------ .../pull-requests/usePullRequestList.ts | 31 +++++++++++------- 3 files changed, 46 insertions(+), 29 deletions(-) diff --git a/apps/mobile/src/features/pull-requests/pullRequestList.test.ts b/apps/mobile/src/features/pull-requests/pullRequestList.test.ts index f0a37296bceb2..7ac4f7f1643a4 100644 --- a/apps/mobile/src/features/pull-requests/pullRequestList.test.ts +++ b/apps/mobile/src/features/pull-requests/pullRequestList.test.ts @@ -19,6 +19,7 @@ import { pullRequestListHead, pullRequestListInput, scopedPullRequestListKey, + shownPullRequestListPages, pullRequestListRowsAreEqual, withShownPullRequestStats, withoutScratchProjectScope, @@ -117,15 +118,10 @@ describe("mobile PR list pagination", () => { mergePullRequestListEntries([entry(1)], [entry(2, { host: "enterprise.test" }), entry(2)]), ).toHaveLength(3); }); - it("keeps a patch made on head-only pages while that head is current", () => { + 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( - pullRequestListHead(null, head), - entry(1), - { state: "merged" }, - 5, - ); - expect(pullRequestListHead(patched, head).entries.map((row) => row.state)).toEqual([ + const patched = patchPullRequestListEntry(null, head, entry(1), { state: "merged" }, 5); + expect(shownPullRequestListPages(patched, head)?.entries.map((row) => row.state)).toEqual([ "open", "merged", ]); diff --git a/apps/mobile/src/features/pull-requests/pullRequestList.ts b/apps/mobile/src/features/pull-requests/pullRequestList.ts index b98eb8187c842..0b0f7cfd4e005 100644 --- a/apps/mobile/src/features/pull-requests/pullRequestList.ts +++ b/apps/mobile/src/features/pull-requests/pullRequestList.ts @@ -176,20 +176,32 @@ 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( - pages: PullRequestListPages, + recorded: PullRequestListPages | null, + head: PullRequestListResult | null, entry: PullRequestListEntry, patch: Partial, observedAt: number, -): PullRequestListPages { - return { - ...pages, - entries: pages.entries.map((row) => - pullRequestListKey(row) === pullRequestListKey(entry) - ? { ...row, ...patch, observedAt } - : row, - ), - }; +): 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( diff --git a/apps/mobile/src/features/pull-requests/usePullRequestList.ts b/apps/mobile/src/features/pull-requests/usePullRequestList.ts index 83171e2d512a9..73dad15cafd2f 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestList.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestList.ts @@ -18,6 +18,7 @@ import { nextPullRequestListInput, patchPullRequestListEntry, pullRequestListHead, + shownPullRequestListPages, pullRequestStatKey as statKey, scopedPullRequestListKey, withShownPullRequestStats, @@ -28,11 +29,16 @@ import { type ListTarget = { readonly environmentId: EnvironmentId; readonly input: PullRequestListInput }; type Read = (atom: Atom.Atom) => A; -/** The loaded pages, with the head the list query holds now (cached or answered) on top. */ +/** 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 { - const head = Option.getOrNull(AsyncResult.value(read(composerPullRequests.list(target)))); - const cached = read(pullRequestListPagesFamily(pullRequestListPagesKey(target))); - return head ? pullRequestListHead(cached, head) : cached; + return shownPullRequestListPages( + read(pullRequestListPagesFamily(pullRequestListPagesKey(target))), + currentHead(read, target), + ); } const sourcesFamily = Atom.family((key: string) => { @@ -259,13 +265,16 @@ export function usePullRequestList(targets: readonly ListTarget[]) { (entry: EnvironmentPullRequestListEntry, patch: Partial) => { const target = targets.find((target) => target.environmentId === entry.environmentId); if (!target) return; - // Patch what the list shows: before any answer is recorded, the head alone supplies the rows. - const pages = currentPages((atom) => registry.get(atom), target); - if (pages) - registry.set( - pullRequestListPagesFamily(pullRequestListPagesKey(target)), - patchPullRequestListEntry(pages, entry, patch, Date.now()), - ); + 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],