From 6d1092cb7b41c1c9543b783b832ffd299b390a2d Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Fri, 7 Aug 2026 04:12:43 -0400 Subject: [PATCH] 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. --- mobile/app/h/[hostId]/tasks.tsx | 137 ++++++----- .../github-project-repo-list-load.test.ts | 94 ++++++-- .../tasks/github-project-repo-match.test.ts | 35 +++ mobile/src/tasks/github-project-repo-match.ts | 19 ++ mobile/src/tasks/host-repo-list.test.ts | 71 ++++++ mobile/src/tasks/host-repo-list.ts | 63 ++++++ mobile/src/tasks/use-host-repo-list.test.tsx | 212 ++++++++++++++++++ mobile/src/tasks/use-host-repo-list.ts | 104 +++++++++ 8 files changed, 650 insertions(+), 85 deletions(-) create mode 100644 mobile/src/tasks/host-repo-list.test.ts create mode 100644 mobile/src/tasks/host-repo-list.ts create mode 100644 mobile/src/tasks/use-host-repo-list.test.tsx create mode 100644 mobile/src/tasks/use-host-repo-list.ts diff --git a/mobile/app/h/[hostId]/tasks.tsx b/mobile/app/h/[hostId]/tasks.tsx index a47b616eda7..dc356259f3e 100644 --- a/mobile/app/h/[hostId]/tasks.tsx +++ b/mobile/app/h/[hostId]/tasks.tsx @@ -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(null) - const reposRef = useRef([]) const loadGenerationRef = useRef(0) const taskResumeRef = useRef({}) - const [repos, setRepos] = useState([]) + const repoList = useHostRepoList( + 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('github') const [visibleProviders, setVisibleProviders] = useState(() => 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 => { - 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 => { 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={() => } 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 ? ( 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 8065dfd7c95..f4d7fac3412 100644 --- a/mobile/src/tasks/github-project-repo-list-load.test.ts +++ b/mobile/src/tasks/github-project-repo-list-load.test.ts @@ -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') + }) }) diff --git a/mobile/src/tasks/github-project-repo-match.test.ts b/mobile/src/tasks/github-project-repo-match.test.ts index 58c44a57830..620cf4f11bf 100644 --- a/mobile/src/tasks/github-project-repo-match.test.ts +++ b/mobile/src/tasks/github-project-repo-match.test.ts @@ -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) + }) }) diff --git a/mobile/src/tasks/github-project-repo-match.ts b/mobile/src/tasks/github-project-repo-match.ts index 1e9cd61996a..06a4f94e802 100644 --- a/mobile/src/tasks/github-project-repo-match.ts +++ b/mobile/src/tasks/github-project-repo-match.ts @@ -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 +): Record { + 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 = diff --git a/mobile/src/tasks/host-repo-list.test.ts b/mobile/src/tasks/host-repo-list.test.ts new file mode 100644 index 00000000000..c386eeec165 --- /dev/null +++ b/mobile/src/tasks/host-repo-list.test.ts @@ -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, + ...actions: Parameters>[1][] +): HostRepoListState { + return actions.reduce(hostRepoListReducer, state) +} + +const idle = initialHostRepoList() + +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(idle, { type: 'reset' })).toBe(idle) + const loading = reduce(idle, { type: 'requested' }) + expect(hostRepoListReducer(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) + }) +}) diff --git a/mobile/src/tasks/host-repo-list.ts b/mobile/src/tasks/host-repo-list.ts new file mode 100644 index 00000000000..ed758a883a7 --- /dev/null +++ b/mobile/src/tasks/host-repo-list.ts @@ -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 = { + 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 = + | { type: 'reset' } + | { type: 'requested' } + | { type: 'resolved'; repos: Repo[] } + | { type: 'failed'; error: string } + +export const IDLE_HOST_REPO_LIST: HostRepoListState = { + status: 'idle', + repos: [], + error: '' +} + +export function initialHostRepoList(): HostRepoListState { + return IDLE_HOST_REPO_LIST as HostRepoListState +} + +export function hostRepoListReducer( + state: HostRepoListState, + action: HostRepoListAction +): HostRepoListState { + switch (action.type) { + case 'reset': + return state.status === 'idle' ? state : initialHostRepoList() + 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): 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): boolean { + return state.status !== 'loaded' +} diff --git a/mobile/src/tasks/use-host-repo-list.test.tsx b/mobile/src/tasks/use-host-repo-list.test.tsx new file mode 100644 index 00000000000..cbc2a186038 --- /dev/null +++ b/mobile/src/tasks/use-host-repo-list.test.tsx @@ -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 | null = null + let calls = 0 + + function Probe({ clientKey, connected = true }: { clientKey: unknown; connected?: boolean }) { + latest = useHostRepoList( + clientKey, + connected + ? () => { + calls += 1 + return new Promise((resolve, reject) => pending.push({ resolve, reject })) + } + : null + ) + return null + } + + let renderer!: ReturnType + act(() => { + renderer = create(createElement(Probe, { clientKey: 'client-a' })) + }) + return { + get resource(): HostRepoListResource { + 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' }]) + }) +}) diff --git a/mobile/src/tasks/use-host-repo-list.ts b/mobile/src/tasks/use-host-repo-list.ts new file mode 100644 index 00000000000..334048c7124 --- /dev/null +++ b/mobile/src/tasks/use-host-repo-list.ts @@ -0,0 +1,104 @@ +import { useMemo, useReducer, useRef } from 'react' +import { + hostRepoListReducer, + initialHostRepoList, + needsHostRepoListFetch, + type HostRepoListState +} from './host-repo-list' + +export type HostRepoListResource = { + state: HostRepoListState + /** Cached repos when this client already answered, otherwise one fetch. */ + ensureLoaded: () => Promise + /** Discards the cache so an explicit refresh re-reads the host. */ + reload: () => Promise +} + +/** 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( + clientKey: unknown, + fetchRepos: (() => Promise) | null +): HostRepoListResource { + const [state, dispatch] = useReducer( + hostRepoListReducer, + undefined, + initialHostRepoList + ) + const boundKeyRef = useRef(clientKey) + const reposRef = useRef([]) + const inFlightRef = useRef | 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, '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() + 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]) +}