mirror of
https://github.com/stablyai/orca.git
synced 2026-10-21 00:03:46 +00:00
fix(mobile): keep ref writes out of the render phase
CI's React Doctor gate (check:react-doctor:changed, which is separate from check:code-quality:changed) flagged 15 render-phase ref mutations. This is the concurrent-render hazard review also raised: React may abandon a render, and a ref write from it leaks anyway. - The resource now tracks its bound client in state, so the reset during render is a state update - the supported pattern - instead of a ref write. The refs its async guard reads are written in the commit phase, and rebinding retires every request issued for the previous client. - ensureLoaded/reload become useCallback instead of a lazily populated ref, keeping the stable identity consumers depend on. - The route resets repoSelectionHydratedRef in its existing layout effect rather than during render. Also replaces Array<T> with T[] in the new test, per the same gate.
This commit is contained in:
@@ -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(
|
||||
|
||||
@@ -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'
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<Repo> | null = null
|
||||
let calls = 0
|
||||
|
||||
|
||||
@@ -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<Repo>(
|
||||
const inFlightRef = useRef<Promise<Repo[]> | 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<Pick<HostRepoListResource<Repo>, 'ensureLoaded' | 'reload'> | null>(
|
||||
null
|
||||
)
|
||||
if (!callbacksRef.current) {
|
||||
const reload = async (): Promise<Repo[]> => {
|
||||
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<Repo[]> => {
|
||||
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<Repo[]> => {
|
||||
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<Repo[]> => {
|
||||
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<Repo[]> =>
|
||||
inFlightRef.current ??
|
||||
(needsHostRepoListFetch(stateRef.current) ? reload() : Promise.resolve(reposRef.current)),
|
||||
[reload]
|
||||
)
|
||||
|
||||
return useMemo(() => ({ state, ensureLoaded, reload }), [ensureLoaded, reload, state])
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user