mirror of
https://github.com/stablyai/orca.git
synced 2026-10-21 00:03:46 +00:00
fix(mobile): commit only the newest repo-list request, join in-flight
Round-4 review of the resource found four real defects: - The commit guard compared only the client key. A -> B -> A reuses the same client object, so a stale request for A could overwrite a newer result for A. Only the newest request may now commit. - refreshTasks calls reload() and loadTasks() in the same event, and React has not rendered the `requested` dispatch yet, so ensureLoaded read `loaded` and returned the very list the reload was replacing. It now joins the in-flight request, which also collapses the double repo.list the issue-source handler could issue. - The non-project header refresh still called loadTasks directly, so it reused the cached list while pull-to-refresh re-read the host. - Connected Linear waited on a repo list it never reads, so a slow or failed repo.list delayed or cleared the issue list. Linear now starts the fetch for the composer without blocking on it.
This commit is contained in:
@@ -3282,7 +3282,11 @@ export default function MobileTasksScreen() {
|
||||
setItems([])
|
||||
return
|
||||
}
|
||||
const currentRepos = await repoListEnsureLoaded()
|
||||
// Why: Linear issues do not need the repo list, only the composer does, so
|
||||
// start the fetch either way but never make Linear wait on it.
|
||||
const repoListRequest = repoListEnsureLoaded()
|
||||
void repoListRequest.catch(() => {})
|
||||
const currentRepos = provider === 'linear' ? [] : await repoListRequest
|
||||
if (!isCurrent()) {
|
||||
return
|
||||
}
|
||||
@@ -8713,7 +8717,7 @@ export default function MobileTasksScreen() {
|
||||
refreshGitHubProject()
|
||||
return
|
||||
}
|
||||
void loadTasks({ silent: true })
|
||||
refreshTasks()
|
||||
}}
|
||||
>
|
||||
<RefreshCw size={16} color={taskUiReady ? colors.textSecondary : colors.textMuted} />
|
||||
|
||||
@@ -21,12 +21,14 @@ describe('mobile GitHub Project repo list loading', () => {
|
||||
// loaded, so hostedRepos stayed empty and every board row was filtered out.
|
||||
it('loads the repo list before project mode bails out', () => {
|
||||
const body = loadTasksBody()
|
||||
const repoLoad = body.indexOf('await repoListEnsureLoaded()')
|
||||
const repoLoad = body.indexOf('const repoListRequest = repoListEnsureLoaded()')
|
||||
const repoAwait = body.indexOf('await repoListRequest')
|
||||
const projectReturn = body.indexOf("provider === 'github' && githubMode === 'project'")
|
||||
expect(repoLoad, 'loadTasks must load the repo list').toBeGreaterThan(-1)
|
||||
expect(projectReturn, 'project mode must still short-circuit loadTasks').toBeGreaterThan(-1)
|
||||
expect(repoAwait, 'the repo list must be awaited, not just started').toBeGreaterThan(repoLoad)
|
||||
expect(projectReturn, 'project mode must bail out after the repo list load').toBeGreaterThan(
|
||||
repoLoad
|
||||
repoAwait
|
||||
)
|
||||
})
|
||||
|
||||
|
||||
@@ -161,6 +161,47 @@ describe('useHostRepoList', () => {
|
||||
expect(probe.resource.state.repos).toEqual([{ id: 'from-host-b' }])
|
||||
})
|
||||
|
||||
// Regression: A -> B -> A reuses the same client object, so a client-key match
|
||||
// alone let a stale request for A overwrite a newer result for A.
|
||||
it('discards a stale response after returning to the original client', async () => {
|
||||
const probe = mountResource()
|
||||
await act(async () => {
|
||||
void probe.resource.ensureLoaded()
|
||||
})
|
||||
probe.rebind('client-b')
|
||||
probe.rebind('client-a')
|
||||
await act(async () => {
|
||||
void probe.resource.ensureLoaded()
|
||||
})
|
||||
await probe.settle(1, [{ id: 'new-a' }])
|
||||
expect(probe.resource.state.repos).toEqual([{ id: 'new-a' }])
|
||||
|
||||
await probe.settle(0, [{ id: 'stale-a' }])
|
||||
expect(probe.resource.state.repos).toEqual([{ id: 'new-a' }])
|
||||
})
|
||||
|
||||
// Regression: a refresh calls reload() and loadTasks() in the same event, and
|
||||
// React has not rendered the `requested` dispatch yet, so ensureLoaded saw
|
||||
// `loaded` and returned the very list the reload was replacing.
|
||||
it('joins an in-flight reload instead of serving the list it will replace', async () => {
|
||||
const probe = mountResource()
|
||||
await act(async () => {
|
||||
void probe.resource.ensureLoaded()
|
||||
})
|
||||
await probe.settle(0, [{ id: 'stale' }])
|
||||
|
||||
let joined: Repo[] = []
|
||||
await act(async () => {
|
||||
void probe.resource.reload()
|
||||
void probe.resource.ensureLoaded().then((repos) => {
|
||||
joined = repos
|
||||
})
|
||||
})
|
||||
expect(probe.callCount).toBe(2)
|
||||
await probe.settle(1, [{ id: 'fresh' }])
|
||||
expect(joined).toEqual([{ id: 'fresh' }])
|
||||
})
|
||||
|
||||
it('stays idle with no connection instead of caching an empty answer', async () => {
|
||||
const probe = mountResource()
|
||||
probe.rebind('client-a', false)
|
||||
|
||||
@@ -68,14 +68,16 @@ export function useHostRepoList<Repo>(
|
||||
dispatch({ type: 'requested' })
|
||||
try {
|
||||
const repos = await fetchNow()
|
||||
if (boundKeyRef.current !== requestKey) {
|
||||
// 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) {
|
||||
if (boundKeyRef.current === requestKey && requestIdRef.current === requestId) {
|
||||
dispatch({
|
||||
type: 'failed',
|
||||
error: err instanceof Error ? err.message : 'Unknown error'
|
||||
@@ -92,8 +94,12 @@ export function useHostRepoList<Repo>(
|
||||
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: () =>
|
||||
needsHostRepoListFetch(stateRef.current) ? reload() : Promise.resolve(reposRef.current),
|
||||
inFlightRef.current ??
|
||||
(needsHostRepoListFetch(stateRef.current) ? reload() : Promise.resolve(reposRef.current)),
|
||||
reload
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user