diff --git a/mobile/app/h/[hostId]/tasks.tsx b/mobile/app/h/[hostId]/tasks.tsx index 7b773374e3a..71abd94e924 100644 --- a/mobile/app/h/[hostId]/tasks.tsx +++ b/mobile/app/h/[hostId]/tasks.tsx @@ -2645,11 +2645,13 @@ export default function MobileTasksScreen() { setBoundClient(client) setItems([]) setGithubRepoSlugCache({}) - repoSelectionHydratedRef.current = false } useLayoutEffect(() => { clientRef.current = client + // Why: ref writes belong in the commit phase. Doing this during render would + // leak out of a concurrent render React later abandons. + repoSelectionHydratedRef.current = false }, [client]) const persistTaskResumeState = useCallback( diff --git a/mobile/src/tasks/github-project-repo-list-load.test.ts b/mobile/src/tasks/github-project-repo-list-load.test.ts index e8c8bc7a458..8cd4f99deb4 100644 --- a/mobile/src/tasks/github-project-repo-list-load.test.ts +++ b/mobile/src/tasks/github-project-repo-list-load.test.ts @@ -89,12 +89,21 @@ describe('mobile GitHub Project readiness and refresh', () => { // Regression: Expo reuses this screen for the next host, so an effect-based // reset runs a render too late and the previous host's rows show through. - it('clears the other client-scoped caches during render, not in an effect', () => { + it('clears the other client-scoped caches during render', () => { const start = source.indexOf('if (boundClient !== client) {') expect(start, 'the client-scoped reset must run during render').toBeGreaterThan(-1) const body = source.slice(start, source.indexOf('\n }', start)) expect(body).toContain('setItems([])') expect(body).toContain('setGithubRepoSlugCache({})') - expect(body).toContain('repoSelectionHydratedRef.current = false') + expect(body, 'a ref write here would leak from an abandoned render').not.toContain('.current =') + }) + + // ...and the ref half belongs in the commit phase, for the same reason. + it('resets the selection-hydration ref in the commit phase', () => { + const start = source.indexOf('clientRef.current = client') + expect(start, 'the client ref effect must exist').toBeGreaterThan(-1) + expect(source.slice(start, source.indexOf('}, [client])', start))).toContain( + 'repoSelectionHydratedRef.current = false' + ) }) }) diff --git a/mobile/src/tasks/use-host-repo-list.test.tsx b/mobile/src/tasks/use-host-repo-list.test.tsx index 5b35fb0a34c..053095aa39a 100644 --- a/mobile/src/tasks/use-host-repo-list.test.tsx +++ b/mobile/src/tasks/use-host-repo-list.test.tsx @@ -8,7 +8,7 @@ type Repo = { id: string } /** Renders the hook and exposes the latest resource plus a deferred fetcher so a * test can decide exactly when (and for which client) a response lands. */ function mountResource() { - const pending: Array<{ resolve: (repos: Repo[]) => void; reject: (err: Error) => void }> = [] + const pending: { resolve: (repos: Repo[]) => void; reject: (err: Error) => void }[] = [] let latest: HostRepoListResource | null = null let calls = 0 diff --git a/mobile/src/tasks/use-host-repo-list.ts b/mobile/src/tasks/use-host-repo-list.ts index 7ecbef9d69f..a1cabbaf161 100644 --- a/mobile/src/tasks/use-host-repo-list.ts +++ b/mobile/src/tasks/use-host-repo-list.ts @@ -1,4 +1,12 @@ -import { useMemo, useReducer, useRef } from 'react' +import { + useCallback, + useEffect, + useLayoutEffect, + useMemo, + useReducer, + useRef, + useState +} from 'react' import { hostRepoListReducer, initialHostRepoList, @@ -31,80 +39,91 @@ export function useHostRepoList( const inFlightRef = useRef | null>(null) const requestIdRef = useRef(0) const fetchRef = useRef(fetchRepos) - fetchRef.current = fetchRepos + const stateRef = useRef(state) - // Why: an effect runs too late. Expo reuses this screen for the next host, so - // a render between the swap and the effect would serve the old client's repos - // to the new one. Resetting here discards them before anything can read them. - if (boundKeyRef.current !== clientKey) { - boundKeyRef.current = clientKey - reposRef.current = [] - inFlightRef.current = null + // Why: discarding the previous client's list has to happen before anything can + // read it, and Expo reuses this screen for the next host. A state update during + // render is the supported way to do that; a ref write here is not, because a + // concurrent render React abandons would still have mutated it. + const [boundKey, setBoundKey] = useState(clientKey) + if (boundKey !== clientKey) { + setBoundKey(clientKey) dispatch({ type: 'reset' }) } - const stateRef = useRef(state) - stateRef.current = state + // Why: the async request guard reads these, so they are written in the commit + // phase - the earliest point a render is known to have survived. + useEffect(() => { + fetchRef.current = fetchRepos + stateRef.current = state + }) - const callbacksRef = useRef, 'ensureLoaded' | 'reload'> | null>( - null - ) - if (!callbacksRef.current) { - const reload = async (): Promise => { - const fetchNow = fetchRef.current - if (!fetchNow) { - return [] - } - if (inFlightRef.current) { - return inFlightRef.current - } - // Why: only the request issued for the currently bound client may commit. - // A slow response from the previous host would otherwise land afterwards - // and pin its repos as this host's authoritative list. - const requestKey = boundKeyRef.current - const requestId = requestIdRef.current + 1 - requestIdRef.current = requestId - const request = (async (): Promise => { - dispatch({ type: 'requested' }) - try { - const repos = await fetchNow() - // Why: A -> B -> A reuses the same client, so matching the key alone - // would let a stale request for A overwrite a newer result for A. - if (boundKeyRef.current !== requestKey || requestIdRef.current !== requestId) { - return [] - } - reposRef.current = repos - dispatch({ type: 'resolved', repos }) - return repos - } catch (err) { - if (boundKeyRef.current === requestKey && requestIdRef.current === requestId) { - dispatch({ - type: 'failed', - error: err instanceof Error ? err.message : 'Unknown error' - }) - } - throw err - } finally { - if (requestIdRef.current === requestId) { - inFlightRef.current = null - } + useLayoutEffect(() => { + if (boundKeyRef.current === clientKey) { + return + } + boundKeyRef.current = clientKey + reposRef.current = [] + inFlightRef.current = null + // Retires every request issued for the previous client. + requestIdRef.current += 1 + }, [clientKey]) + + // Why: consumers hold these in dependency arrays, so they must never change + // identity. Everything they read lives in refs, so they never need to. + const reload = useCallback(async (): Promise => { + const fetchNow = fetchRef.current + if (!fetchNow) { + return [] + } + if (inFlightRef.current) { + return inFlightRef.current + } + // Why: only the request issued for the currently bound client may commit. + // A slow response from the previous host would otherwise land afterwards + // and pin its repos as this host's authoritative list. + const requestKey = boundKeyRef.current + const requestId = requestIdRef.current + 1 + requestIdRef.current = requestId + const request = (async (): Promise => { + dispatch({ type: 'requested' }) + try { + const repos = await fetchNow() + // Why: A -> B -> A reuses the same client, so matching the key alone + // would let a stale request for A overwrite a newer result for A. + if (boundKeyRef.current !== requestKey || requestIdRef.current !== requestId) { + return [] } - })() - inFlightRef.current = request - return request - } - callbacksRef.current = { - // Why: within one event React has not rendered the `requested` dispatch - // yet, so the status still reads `loaded`. Join the in-flight request - // instead of handing back the list it is about to replace. - ensureLoaded: () => - inFlightRef.current ?? - (needsHostRepoListFetch(stateRef.current) ? reload() : Promise.resolve(reposRef.current)), - reload - } - } - // Why: consumers put this in dependency arrays. A fresh object per render - // would re-create their callbacks every render and spin their effects. - const callbacks = callbacksRef.current - return useMemo(() => ({ state, ...callbacks }), [callbacks, state]) + reposRef.current = repos + dispatch({ type: 'resolved', repos }) + return repos + } catch (err) { + if (boundKeyRef.current === requestKey && requestIdRef.current === requestId) { + dispatch({ + type: 'failed', + error: err instanceof Error ? err.message : 'Unknown error' + }) + } + throw err + } finally { + if (requestIdRef.current === requestId) { + inFlightRef.current = null + } + } + })() + inFlightRef.current = request + return request + }, []) + + // Why: within one event React has not rendered the `requested` dispatch yet, so + // the status still reads `loaded`. Join the in-flight request instead of + // handing back the list it is about to replace. + const ensureLoaded = useCallback( + (): Promise => + inFlightRef.current ?? + (needsHostRepoListFetch(stateRef.current) ? reload() : Promise.resolve(reposRef.current)), + [reload] + ) + + return useMemo(() => ({ state, ensureLoaded, reload }), [ensureLoaded, reload, state]) }