mirror of
https://github.com/stablyai/orca.git
synced 2026-10-26 16:02:42 +00:00
refactor(mobile): make the repo list a client-scoped resource
Two review rounds on the ad-hoc follow-ups kept finding new defects, all tracing to one cause: the repo list was loose component state fetched from inside loadTasks, keyed to nothing, with no request-generation guard and invalidation duplicated across refresh controls. An empty array could not say whether the host has no repos or was never asked, and a plain array could not say which client answered. Replace it with a resource: - host-repo-list.ts holds the state machine (idle/loading/loaded/error) so "no repos" and "not fetched" are distinguishable. Project readiness asks it instead of inferring from `loading`/`refreshing`. - use-host-repo-list.ts binds it to the client. It resets during render, not in an effect, because Expo reuses this screen for the next host and an effect runs a render too late. Only the request issued for the bound client may commit, so a slow response from the previous host can no longer become this host's answer. Concurrent callers share one request. - reload() is the single invalidation point, so both project refresh controls and the items/Linear ones re-read the host the same way. - The other client-scoped caches reset in the same render guard. The logic is now unit- and render-tested rather than asserted against route source text; the source test keeps only the wiring it cannot see.
This commit is contained in:
@@ -80,6 +80,7 @@ import {
|
||||
import { shouldResolveHostedReviewStartPoint } from '../../../src/tasks/hosted-review-start-point'
|
||||
import { getLinkedWorkItemSuggestedName } from '../../../src/tasks/mobile-workspace-name'
|
||||
import {
|
||||
dropFailedGitHubRepoSlugEntries,
|
||||
filterGitHubProjectRowsForRepos,
|
||||
findRepoForGitHubProjectRepository,
|
||||
type GitHubRepoSlugCacheEntry
|
||||
@@ -132,6 +133,8 @@ import {
|
||||
resolveVisibleTaskProvider,
|
||||
type TaskProvider
|
||||
} from '../../../src/tasks/mobile-task-providers'
|
||||
import { hasSettledHostRepoList } from '../../../src/tasks/host-repo-list'
|
||||
import { useHostRepoList } from '../../../src/tasks/use-host-repo-list'
|
||||
import {
|
||||
extractLinearIssueReadItems,
|
||||
type LinearMobileIssue
|
||||
@@ -2084,10 +2087,22 @@ export default function MobileTasksScreen() {
|
||||
const reconnectAttempts = useReconnectAttempt(hostId)
|
||||
const lastConnectedAt = useLastConnectedAt(hostId)
|
||||
const clientRef = useRef<RpcClient | null>(null)
|
||||
const reposRef = useRef<RepoSummary[]>([])
|
||||
const loadGenerationRef = useRef(0)
|
||||
const taskResumeRef = useRef<TaskResumeState>({})
|
||||
const [repos, setRepos] = useState<RepoSummary[]>([])
|
||||
const repoList = useHostRepoList<RepoSummary>(
|
||||
client,
|
||||
client && connState === 'connected'
|
||||
? async () => {
|
||||
const response = await client.sendRequest('repo.list')
|
||||
if (!isSuccess(response)) {
|
||||
throw new Error(response.error.message)
|
||||
}
|
||||
return (response.result as { repos: RepoSummary[] }).repos
|
||||
}
|
||||
: null
|
||||
)
|
||||
const repos = repoList.state.repos
|
||||
const { ensureLoaded: repoListEnsureLoaded, reload: repoListReload } = repoList
|
||||
const [provider, setProvider] = useState<TaskProvider>('github')
|
||||
const [visibleProviders, setVisibleProviders] = useState<TaskProvider[]>(() =>
|
||||
normalizeVisibleTaskProviders(undefined)
|
||||
@@ -2378,13 +2393,17 @@ export default function MobileTasksScreen() {
|
||||
) as RepoSummary | null,
|
||||
[activeGitHubProjectHost, githubRepoSlugCache, hostedRepos]
|
||||
)
|
||||
// Why: `every` is vacuously true on an empty repo list, so readiness has to ask
|
||||
// the resource whether that list is real yet. Otherwise the board renders
|
||||
// "No project items" for a board whose repos simply have not arrived.
|
||||
const githubProjectRepoSlugReady = useMemo(
|
||||
() =>
|
||||
hasSettledHostRepoList(repoList.state) &&
|
||||
hostedRepos.every((repo) => {
|
||||
const cached = githubRepoSlugCache[repo.id]
|
||||
return cached !== undefined && cached.path === repo.path
|
||||
}),
|
||||
[githubRepoSlugCache, hostedRepos]
|
||||
[githubRepoSlugCache, hostedRepos, repoList.state]
|
||||
)
|
||||
const visibleGitHubProjectRows = useMemo(
|
||||
() =>
|
||||
@@ -2488,13 +2507,10 @@ export default function MobileTasksScreen() {
|
||||
throw new Error(response.error.message)
|
||||
}
|
||||
const result = response.result as GitHubOwnerRepo | null
|
||||
return {
|
||||
repoId: repo.id,
|
||||
path: repo.path,
|
||||
repository: result
|
||||
}
|
||||
return { repoId: repo.id, entry: { path: repo.path, repository: result } }
|
||||
} catch {
|
||||
return { repoId: repo.id, path: repo.path, repository: null }
|
||||
// Cached so readiness settles; `failed` marks it for retry on refresh.
|
||||
return { repoId: repo.id, entry: { path: repo.path, repository: null, failed: true } }
|
||||
}
|
||||
}).then((entries) => {
|
||||
if (cancelled) {
|
||||
@@ -2503,7 +2519,7 @@ export default function MobileTasksScreen() {
|
||||
setGithubRepoSlugCache((current) => {
|
||||
const next = { ...current }
|
||||
for (const entry of entries) {
|
||||
next[entry.repoId] = { path: entry.path, repository: entry.repository }
|
||||
next[entry.repoId] = entry.entry
|
||||
}
|
||||
return next
|
||||
})
|
||||
@@ -2621,6 +2637,17 @@ export default function MobileTasksScreen() {
|
||||
|
||||
// Why: task-loading effects use this as a stale-client guard, so the ref
|
||||
// must be current before those passive effects can run after commit.
|
||||
// Why: Expo reuses this screen for the next host, so an effect reset runs a
|
||||
// render too late and the previous host's rows show under the new one. The
|
||||
// repo list resets itself; these are the other client-scoped caches.
|
||||
const [boundClient, setBoundClient] = useState(client)
|
||||
if (boundClient !== client) {
|
||||
setBoundClient(client)
|
||||
setItems([])
|
||||
setGithubRepoSlugCache({})
|
||||
repoSelectionHydratedRef.current = false
|
||||
}
|
||||
|
||||
useLayoutEffect(() => {
|
||||
clientRef.current = client
|
||||
}, [client])
|
||||
@@ -3035,32 +3062,26 @@ export default function MobileTasksScreen() {
|
||||
setProvider(resolveVisibleTaskProvider(provider, visibleProviders))
|
||||
}, [provider, visibleProviders])
|
||||
|
||||
const loadRepos = useCallback(async (): Promise<RepoSummary[]> => {
|
||||
if (!client || connState !== 'connected') {
|
||||
return []
|
||||
// Selection follows the list rather than the fetch, so it reconciles the same
|
||||
// way no matter which caller triggered the load.
|
||||
useEffect(() => {
|
||||
if (repoList.state.status !== 'loaded') {
|
||||
return
|
||||
}
|
||||
const response = await client.sendRequest('repo.list')
|
||||
if (!isSuccess(response)) {
|
||||
throw new Error(response.error.message)
|
||||
}
|
||||
const result = response.result as { repos: RepoSummary[] }
|
||||
reposRef.current = result.repos
|
||||
setRepos(result.repos)
|
||||
if (!repoSelectionHydratedRef.current) {
|
||||
repoSelectionHydratedRef.current = true
|
||||
setSelectedRepoIds(reconcileRepoSelection(result.repos, defaultRepoSelectionRef.current))
|
||||
} else {
|
||||
setSelectedRepoIds((current) => {
|
||||
if (current.size === 0) {
|
||||
return current
|
||||
}
|
||||
const availableIds = new Set(result.repos.filter(isHostedTaskRepo).map((repo) => repo.id))
|
||||
const next = new Set([...current].filter((id) => availableIds.has(id)))
|
||||
return next.size === current.size ? current : next
|
||||
})
|
||||
setSelectedRepoIds(reconcileRepoSelection(repos, defaultRepoSelectionRef.current))
|
||||
return
|
||||
}
|
||||
return result.repos
|
||||
}, [client, connState])
|
||||
setSelectedRepoIds((current) => {
|
||||
if (current.size === 0) {
|
||||
return current
|
||||
}
|
||||
const availableIds = new Set(repos.filter(isHostedTaskRepo).map((repo) => repo.id))
|
||||
const next = new Set([...current].filter((id) => availableIds.has(id)))
|
||||
return next.size === current.size ? current : next
|
||||
})
|
||||
}, [repoList.state.status, repos])
|
||||
|
||||
const loadLinearContext = useCallback(async (): Promise<void> => {
|
||||
if (!client || connState !== 'connected' || !tasksSupported) {
|
||||
@@ -3261,7 +3282,7 @@ export default function MobileTasksScreen() {
|
||||
setItems([])
|
||||
return
|
||||
}
|
||||
const currentRepos = reposRef.current.length > 0 ? reposRef.current : await loadRepos()
|
||||
const currentRepos = await repoListEnsureLoaded()
|
||||
if (!isCurrent()) {
|
||||
return
|
||||
}
|
||||
@@ -3322,11 +3343,6 @@ export default function MobileTasksScreen() {
|
||||
return
|
||||
}
|
||||
if (provider === 'gitlab' && gitlabView === 'todos') {
|
||||
setGithubPages([])
|
||||
setGithubCurrentPage(0)
|
||||
setGithubTotalCount(null)
|
||||
setGithubSourceErrors([])
|
||||
setGithubSourceFallbacks([])
|
||||
const response = await requestClient.sendRequest('gitlab.todos', {
|
||||
repo: `id:${queriedRepos[0]!.id}`
|
||||
})
|
||||
@@ -3343,11 +3359,6 @@ export default function MobileTasksScreen() {
|
||||
)
|
||||
return
|
||||
}
|
||||
setGithubPages([])
|
||||
setGithubCurrentPage(0)
|
||||
setGithubTotalCount(null)
|
||||
setGithubSourceErrors([])
|
||||
setGithubSourceFallbacks([])
|
||||
const results = await mapWithConcurrency(
|
||||
queriedRepos,
|
||||
GITHUB_REPO_CONCURRENCY,
|
||||
@@ -3453,7 +3464,7 @@ export default function MobileTasksScreen() {
|
||||
linearConnected,
|
||||
linearFilter,
|
||||
linearOrderBy,
|
||||
loadRepos,
|
||||
repoListEnsureLoaded,
|
||||
provider,
|
||||
selectedLinearTeamIds,
|
||||
selectedLinearWorkspaceId,
|
||||
@@ -3892,6 +3903,19 @@ export default function MobileTasksScreen() {
|
||||
tasksSupported
|
||||
])
|
||||
|
||||
// Why: a refresh must re-read the host, not replay the cached list, or a repo
|
||||
// added since this screen mounted can never appear.
|
||||
const refreshTasks = useCallback(() => {
|
||||
void repoListReload().catch(() => {})
|
||||
void loadTasks({ silent: true })
|
||||
}, [loadTasks, repoListReload])
|
||||
|
||||
const refreshGitHubProject = useCallback(() => {
|
||||
setGithubRepoSlugCache(dropFailedGitHubRepoSlugEntries)
|
||||
refreshTasks()
|
||||
void loadGitHubProjectTable({ queryOverride: appliedGithubProjectSearch })
|
||||
}, [appliedGithubProjectSearch, loadGitHubProjectTable, refreshTasks])
|
||||
|
||||
useEffect(() => {
|
||||
if (!taskStateHydrated) {
|
||||
return
|
||||
@@ -8122,22 +8146,15 @@ export default function MobileTasksScreen() {
|
||||
if (!isSuccess(response)) {
|
||||
throw new Error(response.error.message)
|
||||
}
|
||||
setRepos((current) =>
|
||||
current.map((candidate) =>
|
||||
candidate.id === repo.id
|
||||
? { ...candidate, issueSourcePreference: preference }
|
||||
: candidate
|
||||
)
|
||||
)
|
||||
reposRef.current = reposRef.current.map((candidate) =>
|
||||
candidate.id === repo.id ? { ...candidate, issueSourcePreference: preference } : candidate
|
||||
)
|
||||
// Why: the host owns issueSourcePreference, so re-read the list instead of
|
||||
// patching the cached copy and hoping the two stay in step.
|
||||
await repoListReload().catch(() => {})
|
||||
await loadTasks({ silent: true })
|
||||
} catch (err) {
|
||||
setError(err instanceof Error ? err.message : 'Failed to update issue source')
|
||||
}
|
||||
},
|
||||
[client, loadTasks, taskUiReady]
|
||||
[client, loadTasks, repoListReload, taskUiReady]
|
||||
)
|
||||
|
||||
const renderCommentComposer = (args: {
|
||||
@@ -8693,7 +8710,7 @@ export default function MobileTasksScreen() {
|
||||
return
|
||||
}
|
||||
if (provider === 'github' && githubMode === 'project') {
|
||||
void loadGitHubProjectTable({ queryOverride: appliedGithubProjectSearch })
|
||||
refreshGitHubProject()
|
||||
return
|
||||
}
|
||||
void loadTasks({ silent: true })
|
||||
@@ -9280,9 +9297,7 @@ export default function MobileTasksScreen() {
|
||||
ItemSeparatorComponent={() => <View style={styles.separator} />}
|
||||
contentContainerStyle={[styles.list, { paddingBottom: spacing.lg + insets.bottom }]}
|
||||
refreshing={githubProjectLoading}
|
||||
onRefresh={() =>
|
||||
void loadGitHubProjectTable({ queryOverride: appliedGithubProjectSearch })
|
||||
}
|
||||
onRefresh={refreshGitHubProject}
|
||||
renderItem={({ item: entry }) => {
|
||||
if (entry.type === 'group') {
|
||||
return (
|
||||
@@ -9487,7 +9502,7 @@ export default function MobileTasksScreen() {
|
||||
}
|
||||
contentContainerStyle={[styles.list, { paddingBottom: spacing.lg + insets.bottom }]}
|
||||
refreshing={refreshing}
|
||||
onRefresh={() => void loadTasks({ silent: true })}
|
||||
onRefresh={refreshTasks}
|
||||
renderItem={({ item: entry }) => {
|
||||
if (entry.type === 'section') {
|
||||
return (
|
||||
@@ -9586,7 +9601,7 @@ export default function MobileTasksScreen() {
|
||||
}
|
||||
contentContainerStyle={[styles.list, { paddingBottom: spacing.lg + insets.bottom }]}
|
||||
refreshing={refreshing}
|
||||
onRefresh={() => void loadTasks({ silent: true })}
|
||||
onRefresh={refreshTasks}
|
||||
ListFooterComponent={
|
||||
provider === 'github' && githubMode === 'items' && githubCanShowPagination ? (
|
||||
<View style={styles.paginationFooter}>
|
||||
|
||||
@@ -1,10 +1,13 @@
|
||||
import { readFileSync } from 'node:fs'
|
||||
import { describe, expect, it } from 'vitest'
|
||||
|
||||
/** Wiring assertions only. The repo-list behaviour itself (caching, client
|
||||
* scoping, stale responses, retry) is covered behaviourally in
|
||||
* host-repo-list.test.ts and use-host-repo-list.test.tsx. What cannot be
|
||||
* reached from there is how this 15k-line route consumes the resource, so
|
||||
* these pin the consumption points the original bug lived in. */
|
||||
const source = readFileSync(new URL('../../app/h/[hostId]/tasks.tsx', import.meta.url), 'utf8')
|
||||
|
||||
// loadTasks is the only path that calls loadRepos, so the ordering inside it is
|
||||
// what decides whether Project rows have repos to match against.
|
||||
function loadTasksBody(): string {
|
||||
const start = source.indexOf('const loadTasks = useCallback(')
|
||||
expect(start, 'loadTasks must exist in the tasks route').toBeGreaterThan(-1)
|
||||
@@ -13,29 +16,14 @@ function loadTasksBody(): string {
|
||||
return source.slice(start, end)
|
||||
}
|
||||
|
||||
function offsets(): {
|
||||
repoLoad: number
|
||||
projectReturn: number
|
||||
workItemFetch: number
|
||||
} {
|
||||
const body = loadTasksBody()
|
||||
return {
|
||||
repoLoad: body.indexOf('await loadRepos()'),
|
||||
projectReturn: body.indexOf("provider === 'github' && githubMode === 'project'"),
|
||||
workItemFetch: body.indexOf("provider === 'github' || provider === 'gitlab'")
|
||||
}
|
||||
}
|
||||
|
||||
describe('mobile GitHub Project repo list loading', () => {
|
||||
it('keeps loadRepos on the loadTasks path', () => {
|
||||
expect(offsets().repoLoad, 'loadTasks must still load the repo list').toBeGreaterThan(-1)
|
||||
})
|
||||
|
||||
// Regression: project mode returned before loadRepos(), so hostedRepos stayed
|
||||
// empty, filterGitHubProjectRowsForRepos matched nothing, and the board read
|
||||
// "No project items" until another mode populated repos as a side effect.
|
||||
// Regression (#12966): project mode returned before the repo list was ever
|
||||
// loaded, so hostedRepos stayed empty and every board row was filtered out.
|
||||
it('loads the repo list before project mode bails out', () => {
|
||||
const { repoLoad, projectReturn } = offsets()
|
||||
const body = loadTasksBody()
|
||||
const repoLoad = body.indexOf('await repoListEnsureLoaded()')
|
||||
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(projectReturn, 'project mode must bail out after the repo list load').toBeGreaterThan(
|
||||
repoLoad
|
||||
@@ -43,10 +31,68 @@ describe('mobile GitHub Project repo list loading', () => {
|
||||
})
|
||||
|
||||
it('still bails out before any work-item fetch in project mode', () => {
|
||||
const { projectReturn, workItemFetch } = offsets()
|
||||
const body = loadTasksBody()
|
||||
const projectReturn = body.indexOf("provider === 'github' && githubMode === 'project'")
|
||||
const workItemFetch = body.indexOf("provider === 'github' || provider === 'gitlab'")
|
||||
expect(workItemFetch, 'the hosted work-item fetch must still exist').toBeGreaterThan(-1)
|
||||
expect(workItemFetch, 'project mode must not reach the issue/PR fetch').toBeGreaterThan(
|
||||
projectReturn
|
||||
)
|
||||
})
|
||||
|
||||
it('keeps the repo list in the resource rather than local state', () => {
|
||||
expect(source).toContain('const repos = repoList.state.repos')
|
||||
expect(source, 'a second copy of the list would drift from the resource').not.toContain(
|
||||
'const [repos, setRepos]'
|
||||
)
|
||||
expect(source).not.toContain('reposRef')
|
||||
})
|
||||
})
|
||||
|
||||
describe('mobile GitHub Project readiness and refresh', () => {
|
||||
// `every` is vacuously true on an empty list, so readiness must ask the
|
||||
// resource whether that list is real yet rather than infer it.
|
||||
it('derives slug readiness from the resource status', () => {
|
||||
const start = source.indexOf('const githubProjectRepoSlugReady = useMemo(')
|
||||
expect(start, 'slug readiness memo must exist').toBeGreaterThan(-1)
|
||||
const body = source.slice(start, source.indexOf(' )', start))
|
||||
expect(body).toContain('hasSettledHostRepoList(repoList.state)')
|
||||
expect(body).toContain('[githubRepoSlugCache, hostedRepos, repoList.state]')
|
||||
})
|
||||
|
||||
it('re-reads the host and retries failed slug lookups on refresh', () => {
|
||||
const start = source.indexOf('const refreshTasks = useCallback(')
|
||||
expect(start, 'refresh must be a single shared callback').toBeGreaterThan(-1)
|
||||
expect(source.slice(start, source.indexOf('}, [', start))).toContain('repoListReload()')
|
||||
|
||||
const project = source.indexOf('const refreshGitHubProject = useCallback(')
|
||||
expect(project, 'project refresh must be a single shared callback').toBeGreaterThan(-1)
|
||||
const projectBody = source.slice(project, source.indexOf('}, [', project))
|
||||
expect(projectBody).toContain('dropFailedGitHubRepoSlugEntries')
|
||||
expect(projectBody).toContain('refreshTasks()')
|
||||
expect(projectBody).toContain('loadGitHubProjectTable(')
|
||||
})
|
||||
|
||||
it('routes every refresh control through those callbacks', () => {
|
||||
expect(source).toContain('onRefresh={refreshTasks}')
|
||||
expect(source).toContain('onRefresh={refreshGitHubProject}')
|
||||
expect(source, 'no refresh control may call loadTasks directly').not.toContain(
|
||||
'onRefresh={() => void loadTasks('
|
||||
)
|
||||
})
|
||||
|
||||
it('marks a failed slug lookup as retryable rather than resolved', () => {
|
||||
expect(source).toContain('repository: null, failed: true')
|
||||
})
|
||||
|
||||
// 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', () => {
|
||||
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')
|
||||
})
|
||||
})
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import {
|
||||
dropFailedGitHubRepoSlugEntries,
|
||||
filterGitHubProjectRowsForRepos,
|
||||
findRepoForGitHubProjectRepository,
|
||||
normalizeGitHubRepositorySlug
|
||||
@@ -255,4 +256,38 @@ describe('GitHub project repo matching', () => {
|
||||
)
|
||||
).toBeNull()
|
||||
})
|
||||
|
||||
// Regression: a transient github.repoSlug error used to be cached as a
|
||||
// resolved "no repository", which filtered that repo's rows out forever.
|
||||
it('leaves a failed slug lookup matchable by the path fallback', () => {
|
||||
expect(
|
||||
findRepoForGitHubProjectRepository(
|
||||
'stablyai/orca',
|
||||
[{ id: 'repo-1', path: '/Users/me/stablyai/orca', displayName: 'orca' }],
|
||||
{ 'repo-1': { path: '/Users/me/stablyai/orca', repository: null, failed: true } }
|
||||
)
|
||||
).toEqual({ id: 'repo-1', path: '/Users/me/stablyai/orca', displayName: 'orca' })
|
||||
})
|
||||
})
|
||||
|
||||
describe('dropFailedGitHubRepoSlugEntries', () => {
|
||||
it('drops only the entries a retry could still resolve', () => {
|
||||
expect(
|
||||
dropFailedGitHubRepoSlugEntries({
|
||||
'repo-1': { path: '/a', repository: { owner: 'stablyai', repo: 'orca' } },
|
||||
'repo-2': { path: '/b', repository: null, failed: true },
|
||||
'repo-3': { path: '/c', repository: null }
|
||||
})
|
||||
).toEqual({
|
||||
'repo-1': { path: '/a', repository: { owner: 'stablyai', repo: 'orca' } },
|
||||
'repo-3': { path: '/c', repository: null }
|
||||
})
|
||||
})
|
||||
|
||||
// Why: the cache is a slug-effect dependency, so a fresh object on every
|
||||
// refresh would re-run the effect even when there is nothing to retry.
|
||||
it('returns the same object when nothing failed', () => {
|
||||
const cache = { 'repo-1': { path: '/a', repository: null } }
|
||||
expect(dropFailedGitHubRepoSlugEntries(cache)).toBe(cache)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -15,6 +15,25 @@ export type GitHubProjectRepoMatch = {
|
||||
export type GitHubRepoSlugCacheEntry = {
|
||||
path: string
|
||||
repository: { owner: string; repo: string; host?: string } | null
|
||||
/** Resolution failed rather than resolving to "no repository". Cached so the
|
||||
* board stops waiting on it, but dropped on refresh so it is retried. */
|
||||
failed?: boolean
|
||||
}
|
||||
|
||||
/** Why: a transient `github.repoSlug` error would otherwise be cached forever as
|
||||
* an unresolved repo, filtering its rows out of every future board render. */
|
||||
export function dropFailedGitHubRepoSlugEntries(
|
||||
slugsByRepoId: Record<string, GitHubRepoSlugCacheEntry | undefined>
|
||||
): Record<string, GitHubRepoSlugCacheEntry | undefined> {
|
||||
const retryable = Object.entries(slugsByRepoId).filter(([, entry]) => entry?.failed === true)
|
||||
if (retryable.length === 0) {
|
||||
return slugsByRepoId
|
||||
}
|
||||
const next = { ...slugsByRepoId }
|
||||
for (const [repoId] of retryable) {
|
||||
delete next[repoId]
|
||||
}
|
||||
return next
|
||||
}
|
||||
|
||||
type CachedSlugState =
|
||||
|
||||
@@ -0,0 +1,71 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import {
|
||||
hasSettledHostRepoList,
|
||||
hostRepoListReducer,
|
||||
initialHostRepoList,
|
||||
needsHostRepoListFetch,
|
||||
type HostRepoListState
|
||||
} from './host-repo-list'
|
||||
|
||||
type Repo = { id: string }
|
||||
|
||||
function reduce(
|
||||
state: HostRepoListState<Repo>,
|
||||
...actions: Parameters<typeof hostRepoListReducer<Repo>>[1][]
|
||||
): HostRepoListState<Repo> {
|
||||
return actions.reduce(hostRepoListReducer<Repo>, state)
|
||||
}
|
||||
|
||||
const idle = initialHostRepoList<Repo>()
|
||||
|
||||
describe('hostRepoListReducer', () => {
|
||||
it('keeps the last good list while a reload is in flight', () => {
|
||||
const loaded = reduce(idle, { type: 'requested' }, { type: 'resolved', repos: [{ id: 'a' }] })
|
||||
expect(reduce(loaded, { type: 'requested' })).toEqual({
|
||||
status: 'loading',
|
||||
repos: [{ id: 'a' }],
|
||||
error: ''
|
||||
})
|
||||
})
|
||||
|
||||
it('keeps the last good list when a reload fails, and records why', () => {
|
||||
const loaded = reduce(idle, { type: 'resolved', repos: [{ id: 'a' }] })
|
||||
expect(reduce(loaded, { type: 'requested' }, { type: 'failed', error: 'offline' })).toEqual({
|
||||
status: 'error',
|
||||
repos: [{ id: 'a' }],
|
||||
error: 'offline'
|
||||
})
|
||||
})
|
||||
|
||||
it('clears a previous host completely on reset', () => {
|
||||
const loaded = reduce(idle, { type: 'resolved', repos: [{ id: 'a' }] })
|
||||
expect(reduce(loaded, { type: 'reset' })).toEqual(idle)
|
||||
})
|
||||
|
||||
it('returns the identical state when a reset or repeat request changes nothing', () => {
|
||||
expect(hostRepoListReducer<Repo>(idle, { type: 'reset' })).toBe(idle)
|
||||
const loading = reduce(idle, { type: 'requested' })
|
||||
expect(hostRepoListReducer<Repo>(loading, { type: 'requested' })).toBe(loading)
|
||||
})
|
||||
})
|
||||
|
||||
describe('host repo list readiness', () => {
|
||||
// Regression (#12966): an empty list read as "this host has no repos", so the
|
||||
// Project board filtered every row against [] and rendered "No project items".
|
||||
it('does not treat an unfetched or in-flight list as settled', () => {
|
||||
expect(hasSettledHostRepoList(idle)).toBe(false)
|
||||
expect(hasSettledHostRepoList(reduce(idle, { type: 'requested' }))).toBe(false)
|
||||
})
|
||||
|
||||
it('settles on success and on failure, so the UI never waits forever', () => {
|
||||
expect(hasSettledHostRepoList(reduce(idle, { type: 'resolved', repos: [] }))).toBe(true)
|
||||
expect(hasSettledHostRepoList(reduce(idle, { type: 'failed', error: 'nope' }))).toBe(true)
|
||||
})
|
||||
|
||||
it('re-fetches after a failure but not after an empty success', () => {
|
||||
expect(needsHostRepoListFetch(idle)).toBe(true)
|
||||
expect(needsHostRepoListFetch(reduce(idle, { type: 'failed', error: 'nope' }))).toBe(true)
|
||||
// A host with no repos is a real answer; asking again on every load is waste.
|
||||
expect(needsHostRepoListFetch(reduce(idle, { type: 'resolved', repos: [] }))).toBe(false)
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,63 @@
|
||||
/** The host repo list as a client-scoped resource.
|
||||
*
|
||||
* Why this is a state machine rather than a `repos` array plus a "did we fetch
|
||||
* it" flag: the Tasks screen is reused across hosts and `forceReconnect` swaps
|
||||
* the client underneath it, so a bare array cannot say *which* client answered,
|
||||
* and an empty array cannot say whether the host has no repos or was never
|
||||
* asked. Both ambiguities silently filtered every GitHub Project row (#12966).
|
||||
*/
|
||||
|
||||
export type HostRepoListStatus = 'idle' | 'loading' | 'loaded' | 'error'
|
||||
|
||||
export type HostRepoListState<Repo> = {
|
||||
status: HostRepoListStatus
|
||||
/** Last successful response. Retained across a reload so refreshing does not
|
||||
* blank the list, and cleared only by `reset`. */
|
||||
repos: Repo[]
|
||||
error: string
|
||||
}
|
||||
|
||||
export type HostRepoListAction<Repo> =
|
||||
| { type: 'reset' }
|
||||
| { type: 'requested' }
|
||||
| { type: 'resolved'; repos: Repo[] }
|
||||
| { type: 'failed'; error: string }
|
||||
|
||||
export const IDLE_HOST_REPO_LIST: HostRepoListState<never> = {
|
||||
status: 'idle',
|
||||
repos: [],
|
||||
error: ''
|
||||
}
|
||||
|
||||
export function initialHostRepoList<Repo>(): HostRepoListState<Repo> {
|
||||
return IDLE_HOST_REPO_LIST as HostRepoListState<Repo>
|
||||
}
|
||||
|
||||
export function hostRepoListReducer<Repo>(
|
||||
state: HostRepoListState<Repo>,
|
||||
action: HostRepoListAction<Repo>
|
||||
): HostRepoListState<Repo> {
|
||||
switch (action.type) {
|
||||
case 'reset':
|
||||
return state.status === 'idle' ? state : initialHostRepoList<Repo>()
|
||||
case 'requested':
|
||||
return state.status === 'loading' ? state : { ...state, status: 'loading', error: '' }
|
||||
case 'resolved':
|
||||
return { status: 'loaded', repos: action.repos, error: '' }
|
||||
case 'failed':
|
||||
return { ...state, status: 'error', error: action.error }
|
||||
}
|
||||
}
|
||||
|
||||
/** Whether a fetch has finished, successfully or not. Callers gate their empty
|
||||
* state on this: "no repos yet" must not render as "this host has no repos". */
|
||||
export function hasSettledHostRepoList(state: HostRepoListState<unknown>): boolean {
|
||||
return state.status === 'loaded' || state.status === 'error'
|
||||
}
|
||||
|
||||
/** Whether `loadTasks` still needs to fetch before it can trust `repos`. An
|
||||
* in-flight request does not count as fetched, so a caller that needs the list
|
||||
* now awaits its own request rather than reading a stale array. */
|
||||
export function needsHostRepoListFetch(state: HostRepoListState<unknown>): boolean {
|
||||
return state.status !== 'loaded'
|
||||
}
|
||||
@@ -0,0 +1,212 @@
|
||||
import { createElement } from 'react'
|
||||
import { act, create } from 'react-test-renderer'
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import { useHostRepoList, type HostRepoListResource } from './use-host-repo-list'
|
||||
|
||||
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 }> = []
|
||||
let latest: HostRepoListResource<Repo> | null = null
|
||||
let calls = 0
|
||||
|
||||
function Probe({ clientKey, connected = true }: { clientKey: unknown; connected?: boolean }) {
|
||||
latest = useHostRepoList<Repo>(
|
||||
clientKey,
|
||||
connected
|
||||
? () => {
|
||||
calls += 1
|
||||
return new Promise<Repo[]>((resolve, reject) => pending.push({ resolve, reject }))
|
||||
}
|
||||
: null
|
||||
)
|
||||
return null
|
||||
}
|
||||
|
||||
let renderer!: ReturnType<typeof create>
|
||||
act(() => {
|
||||
renderer = create(createElement(Probe, { clientKey: 'client-a' }))
|
||||
})
|
||||
return {
|
||||
get resource(): HostRepoListResource<Repo> {
|
||||
if (!latest) {
|
||||
throw new Error('probe never rendered')
|
||||
}
|
||||
return latest
|
||||
},
|
||||
get callCount(): number {
|
||||
return calls
|
||||
},
|
||||
pending,
|
||||
rerender() {
|
||||
act(() => {
|
||||
renderer.update(createElement(Probe, { clientKey: 'client-a' }))
|
||||
})
|
||||
},
|
||||
rebind(clientKey: unknown, connected = true) {
|
||||
act(() => {
|
||||
renderer.update(createElement(Probe, { clientKey, connected }))
|
||||
})
|
||||
},
|
||||
async settle(index: number, repos: Repo[]) {
|
||||
await act(async () => {
|
||||
pending[index]!.resolve(repos)
|
||||
await Promise.resolve()
|
||||
})
|
||||
},
|
||||
async fail(index: number, message: string) {
|
||||
await act(async () => {
|
||||
pending[index]!.reject(new Error(message))
|
||||
await Promise.resolve()
|
||||
})
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
describe('useHostRepoList', () => {
|
||||
it('fetches once and then serves the cached list', async () => {
|
||||
const probe = mountResource()
|
||||
let first: Repo[] = []
|
||||
await act(async () => {
|
||||
void probe.resource.ensureLoaded().then((repos) => {
|
||||
first = repos
|
||||
})
|
||||
})
|
||||
await probe.settle(0, [{ id: 'a' }])
|
||||
expect(first).toEqual([{ id: 'a' }])
|
||||
expect(probe.resource.state.status).toBe('loaded')
|
||||
|
||||
await act(async () => {
|
||||
await probe.resource.ensureLoaded()
|
||||
})
|
||||
expect(probe.callCount).toBe(1)
|
||||
})
|
||||
|
||||
// Regression (#12966): a host with no repos must not be re-fetched forever.
|
||||
it('treats an empty response as a real answer', async () => {
|
||||
const probe = mountResource()
|
||||
await act(async () => {
|
||||
void probe.resource.ensureLoaded()
|
||||
})
|
||||
await probe.settle(0, [])
|
||||
await act(async () => {
|
||||
await probe.resource.ensureLoaded()
|
||||
})
|
||||
expect(probe.callCount).toBe(1)
|
||||
expect(probe.resource.state.status).toBe('loaded')
|
||||
})
|
||||
|
||||
it('collapses concurrent callers into one request', async () => {
|
||||
const probe = mountResource()
|
||||
await act(async () => {
|
||||
void probe.resource.ensureLoaded()
|
||||
void probe.resource.ensureLoaded()
|
||||
})
|
||||
expect(probe.callCount).toBe(1)
|
||||
})
|
||||
|
||||
it('retries after a failure instead of caching it', async () => {
|
||||
const probe = mountResource()
|
||||
await act(async () => {
|
||||
void probe.resource.ensureLoaded().catch(() => {})
|
||||
})
|
||||
await probe.fail(0, 'offline')
|
||||
expect(probe.resource.state.status).toBe('error')
|
||||
expect(probe.resource.state.error).toBe('offline')
|
||||
await act(async () => {
|
||||
void probe.resource.ensureLoaded().catch(() => {})
|
||||
})
|
||||
expect(probe.callCount).toBe(2)
|
||||
})
|
||||
|
||||
// Regression: the screen is reused across hosts, so the previous host's list
|
||||
// must never answer for the new one.
|
||||
it('drops the cached list as soon as the client changes', async () => {
|
||||
const probe = mountResource()
|
||||
await act(async () => {
|
||||
void probe.resource.ensureLoaded()
|
||||
})
|
||||
await probe.settle(0, [{ id: 'from-host-a' }])
|
||||
expect(probe.resource.state.repos).toEqual([{ id: 'from-host-a' }])
|
||||
|
||||
probe.rebind('client-b')
|
||||
expect(probe.resource.state.status).toBe('idle')
|
||||
expect(probe.resource.state.repos).toEqual([])
|
||||
|
||||
await act(async () => {
|
||||
void probe.resource.ensureLoaded()
|
||||
})
|
||||
expect(probe.callCount).toBe(2)
|
||||
})
|
||||
|
||||
// Regression: navigation keeps the old client alive, so its slow repo.list can
|
||||
// still resolve after the swap. It must not become the new host's answer.
|
||||
it('discards a response that arrives after the client changed', async () => {
|
||||
const probe = mountResource()
|
||||
await act(async () => {
|
||||
void probe.resource.ensureLoaded()
|
||||
})
|
||||
probe.rebind('client-b')
|
||||
await probe.settle(0, [{ id: 'from-host-a' }])
|
||||
|
||||
expect(probe.resource.state.status).toBe('idle')
|
||||
expect(probe.resource.state.repos).toEqual([])
|
||||
|
||||
await act(async () => {
|
||||
void probe.resource.ensureLoaded()
|
||||
})
|
||||
await probe.settle(1, [{ id: 'from-host-b' }])
|
||||
expect(probe.resource.state.repos).toEqual([{ id: 'from-host-b' }])
|
||||
})
|
||||
|
||||
it('stays idle with no connection instead of caching an empty answer', async () => {
|
||||
const probe = mountResource()
|
||||
probe.rebind('client-a', false)
|
||||
await act(async () => {
|
||||
expect(await probe.resource.ensureLoaded()).toEqual([])
|
||||
})
|
||||
expect(probe.callCount).toBe(0)
|
||||
expect(probe.resource.state.status).toBe('idle')
|
||||
})
|
||||
|
||||
// Regression: the resource used to be a fresh object per render, so consumers
|
||||
// that held it in a dependency array re-created their callbacks every render
|
||||
// and their effects re-fired forever ("Maximum update depth exceeded").
|
||||
it('keeps a stable identity across renders that change nothing', async () => {
|
||||
const probe = mountResource()
|
||||
const first = probe.resource
|
||||
probe.rerender()
|
||||
expect(probe.resource).toBe(first)
|
||||
expect(probe.resource.ensureLoaded).toBe(first.ensureLoaded)
|
||||
expect(probe.resource.reload).toBe(first.reload)
|
||||
|
||||
await act(async () => {
|
||||
void probe.resource.ensureLoaded()
|
||||
})
|
||||
await probe.settle(0, [{ id: 'a' }])
|
||||
// A load changes state, so the object may differ, but the callbacks must not.
|
||||
expect(probe.resource.ensureLoaded).toBe(first.ensureLoaded)
|
||||
expect(probe.resource.reload).toBe(first.reload)
|
||||
const loaded = probe.resource
|
||||
probe.rerender()
|
||||
expect(probe.resource).toBe(loaded)
|
||||
})
|
||||
|
||||
it('re-reads the host on an explicit reload', async () => {
|
||||
const probe = mountResource()
|
||||
await act(async () => {
|
||||
void probe.resource.ensureLoaded()
|
||||
})
|
||||
await probe.settle(0, [{ id: 'a' }])
|
||||
await act(async () => {
|
||||
void probe.resource.reload()
|
||||
})
|
||||
expect(probe.callCount).toBe(2)
|
||||
// The previous list stays visible while the reload is in flight.
|
||||
expect(probe.resource.state.repos).toEqual([{ id: 'a' }])
|
||||
await probe.settle(1, [{ id: 'a' }, { id: 'b' }])
|
||||
expect(probe.resource.state.repos).toEqual([{ id: 'a' }, { id: 'b' }])
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,104 @@
|
||||
import { useMemo, useReducer, useRef } from 'react'
|
||||
import {
|
||||
hostRepoListReducer,
|
||||
initialHostRepoList,
|
||||
needsHostRepoListFetch,
|
||||
type HostRepoListState
|
||||
} from './host-repo-list'
|
||||
|
||||
export type HostRepoListResource<Repo> = {
|
||||
state: HostRepoListState<Repo>
|
||||
/** Cached repos when this client already answered, otherwise one fetch. */
|
||||
ensureLoaded: () => Promise<Repo[]>
|
||||
/** Discards the cache so an explicit refresh re-reads the host. */
|
||||
reload: () => Promise<Repo[]>
|
||||
}
|
||||
|
||||
/** Binds the repo list to `clientKey`. Pass `fetchRepos: null` while there is no
|
||||
* usable connection; the resource then stays idle instead of caching an empty
|
||||
* answer that a later client would inherit. */
|
||||
export function useHostRepoList<Repo>(
|
||||
clientKey: unknown,
|
||||
fetchRepos: (() => Promise<Repo[]>) | null
|
||||
): HostRepoListResource<Repo> {
|
||||
const [state, dispatch] = useReducer(
|
||||
hostRepoListReducer<Repo>,
|
||||
undefined,
|
||||
initialHostRepoList<Repo>
|
||||
)
|
||||
const boundKeyRef = useRef(clientKey)
|
||||
const reposRef = useRef<Repo[]>([])
|
||||
const inFlightRef = useRef<Promise<Repo[]> | null>(null)
|
||||
const requestIdRef = useRef(0)
|
||||
const fetchRef = useRef(fetchRepos)
|
||||
fetchRef.current = fetchRepos
|
||||
|
||||
// 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
|
||||
dispatch({ type: 'reset' })
|
||||
}
|
||||
|
||||
const stateRef = useRef(state)
|
||||
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()
|
||||
if (boundKeyRef.current !== requestKey) {
|
||||
return []
|
||||
}
|
||||
reposRef.current = repos
|
||||
dispatch({ type: 'resolved', repos })
|
||||
return repos
|
||||
} catch (err) {
|
||||
if (boundKeyRef.current === requestKey) {
|
||||
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
|
||||
}
|
||||
callbacksRef.current = {
|
||||
ensureLoaded: () =>
|
||||
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])
|
||||
}
|
||||
Reference in New Issue
Block a user