From 427f178a75721de11a0fb33aa8084ba226c1460e Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Sun, 9 Aug 2026 14:55:31 -0700 Subject: [PATCH] fix(mobile): load the repo list in GitHub Project mode (#12972) --- mobile/app/h/[hostId]/tasks.tsx | 201 +++++++------- .../github-project-repo-list-load.test.ts | 118 ++++++++ .../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 +++++ .../src/tasks/hosted-repo-selection.test.ts | 36 +++ mobile/src/tasks/hosted-repo-selection.ts | 27 ++ mobile/src/tasks/use-host-repo-list.test.tsx | 253 ++++++++++++++++++ mobile/src/tasks/use-host-repo-list.ts | 129 +++++++++ 10 files changed, 857 insertions(+), 95 deletions(-) create mode 100644 mobile/src/tasks/github-project-repo-list-load.test.ts 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/hosted-repo-selection.test.ts create mode 100644 mobile/src/tasks/hosted-repo-selection.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 0794c317e..677ded5aa 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,9 @@ 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 { isHostedTaskRepo, reconcileRepoSelection } from '../../../src/tasks/hosted-repo-selection' import { extractLinearIssueReadItems, type LinearMobileIssue @@ -1118,22 +1122,6 @@ async function mapWithConcurrency( return results } -function isHostedTaskRepo(repo: RepoSummary): boolean { - return repo.kind !== 'folder' -} - -function reconcileRepoSelection( - repos: RepoSummary[], - persisted: string[] | null | undefined -): Set { - if (!persisted || persisted.length === 0) { - return new Set() - } - const availableIds = new Set(repos.filter(isHostedTaskRepo).map((repo) => repo.id)) - const selected = persisted.filter((id) => availableIds.has(id)) - return selected.length === 0 ? new Set() : new Set(selected) -} - function createLinearTask(issue: LinearIssue): TaskItem { return { key: `linear:${issue.workspaceId ?? 'workspace'}:${issue.id}`, @@ -2084,10 +2072,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 +2378,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 +2492,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 +2504,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,8 +2622,31 @@ 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. + const resetGitHubItemsState = useCallback(() => { + setGithubRepoSources({}) + setGithubPages([]) + setGithubCurrentPage(0) + setGithubTotalCount(null) + setGithubSourceErrors([]) + setGithubSourceFallbacks([]) + }, []) + + // 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({}) + resetGitHubItemsState() + } + 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( @@ -2877,7 +2901,7 @@ export default function MobileTasksScreen() { // pair but must not receive the newer task-specific method calls. setTasksSupportState({ kind: 'unsupported', client }) setItems([]) - setGithubPages([]) + resetGitHubItemsState() setGithubProjectTable(null) setShowLinearWorkspacePicker(false) setShowLinearTeamPicker(false) @@ -3035,32 +3059,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) { @@ -3251,24 +3269,26 @@ export default function MobileTasksScreen() { } try { if (provider !== 'github' || githubMode !== 'items') { - setGithubPages([]) - setGithubCurrentPage(0) - setGithubTotalCount(null) - setGithubSourceErrors([]) - setGithubSourceFallbacks([]) - } - if (provider === 'github' && githubMode === 'project') { - setItems([]) - return + resetGitHubItemsState() } if (provider === 'linear' && !linearConnected) { setItems([]) return } - const currentRepos = reposRef.current.length > 0 ? reposRef.current : await loadRepos() + // 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 } + // Why: project mode fetches no work items, but its rows are matched against + // the repo list, so it must not return before loadRepos() has run. + if (provider === 'github' && githubMode === 'project') { + setItems([]) + return + } if (provider === 'github' || provider === 'gitlab') { const supportedRepos = currentRepos.filter(isHostedTaskRepo) const queriedRepos = @@ -3280,11 +3300,7 @@ export default function MobileTasksScreen() { return } setItems([]) - setGithubPages([]) - setGithubCurrentPage(0) - setGithubTotalCount(null) - setGithubSourceErrors([]) - setGithubSourceFallbacks([]) + resetGitHubItemsState() return } if (provider === 'github') { @@ -3320,11 +3336,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}` }) @@ -3341,11 +3352,6 @@ export default function MobileTasksScreen() { ) return } - setGithubPages([]) - setGithubCurrentPage(0) - setGithubTotalCount(null) - setGithubSourceErrors([]) - setGithubSourceFallbacks([]) const results = await mapWithConcurrency( queriedRepos, GITHUB_REPO_CONCURRENCY, @@ -3429,8 +3435,7 @@ export default function MobileTasksScreen() { return } setItems([]) - setGithubSourceErrors([]) - setGithubSourceFallbacks([]) + resetGitHubItemsState() setError(err instanceof Error ? err.message : 'Failed to load tasks') } finally { if (isCurrent()) { @@ -3451,7 +3456,9 @@ export default function MobileTasksScreen() { linearConnected, linearFilter, linearOrderBy, - loadRepos, + // resetGitHubItemsState is useCallback([]), so its identity never changes + // and listing it here would only cost a line against the max-lines budget. + repoListEnsureLoaded, provider, selectedLinearTeamIds, selectedLinearWorkspaceId, @@ -3890,6 +3897,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 @@ -8120,22 +8140,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: { @@ -8691,10 +8704,10 @@ export default function MobileTasksScreen() { return } if (provider === 'github' && githubMode === 'project') { - void loadGitHubProjectTable({ queryOverride: appliedGithubProjectSearch }) + refreshGitHubProject() return } - void loadTasks({ silent: true }) + refreshTasks() }} > @@ -9278,9 +9291,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 ( @@ -9485,7 +9496,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 ( @@ -9584,7 +9595,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 new file mode 100644 index 000000000..96658c3df --- /dev/null +++ b/mobile/src/tasks/github-project-repo-list-load.test.ts @@ -0,0 +1,118 @@ +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') + +/** Why: `slice(start, -1)` silently trims one byte instead of failing, so a + * missing end marker would let every assertion below pass vacuously. */ +function block(startMarker: string, endMarker: string): string { + const start = source.indexOf(startMarker) + expect(start, `tasks.tsx must contain ${startMarker}`).toBeGreaterThan(-1) + const end = source.indexOf(endMarker, start) + expect(end, `${startMarker} must be followed by ${endMarker}`).toBeGreaterThan(start) + return source.slice(start, end) +} + +function loadTasksBody(): string { + return block('const loadTasks = useCallback(', 'const connectLinearAccount = useCallback(') +} + +describe('mobile GitHub Project repo list loading', () => { + // 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 body = loadTasksBody() + 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( + repoAwait + ) + }) + + it('still bails out before any work-item fetch in project mode', () => { + 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 body = block('const githubProjectRepoSlugReady = useMemo(', ' )') + expect(body).toContain('hasSettledHostRepoList(repoList.state)') + expect(body).toContain('[githubRepoSlugCache, hostedRepos, repoList.state]') + }) + + // Regression: readiness is only useful if the renderer consults it. Without + // this the board renders "No project items" before the repo list arrives, and + // the memo assertion above stays green. + it('gates the empty state on readiness in the renderer', () => { + const gate = block( + "githubMode === 'project' ? (", + 'No project items' + ) + expect(gate, 'the empty state must sit behind the readiness spinner').toContain( + 'githubProjectTable && !githubProjectRepoSlugReady ? (' + ) + }) + + it('re-reads the host and retries failed slug lookups on refresh', () => { + expect(block('const refreshTasks = useCallback(', '}, [')).toContain('repoListReload()') + + const projectBody = block('const refreshGitHubProject = useCallback(', '}, [') + 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', () => { + const body = block('if (boundClient !== client) {', '\n }') + expect(body).toContain('setItems([])') + expect(body).toContain('setGithubRepoSlugCache({})') + 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', () => { + expect(block('clientRef.current = client', '}, [client])')).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 58c44a578..620cf4f11 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 1e9cd6199..06a4f94e8 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 000000000..c386eeec1 --- /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 000000000..ed758a883 --- /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/hosted-repo-selection.test.ts b/mobile/src/tasks/hosted-repo-selection.test.ts new file mode 100644 index 000000000..86796cf96 --- /dev/null +++ b/mobile/src/tasks/hosted-repo-selection.test.ts @@ -0,0 +1,36 @@ +import { describe, expect, it } from 'vitest' +import { isHostedTaskRepo, reconcileRepoSelection } from './hosted-repo-selection' + +const repos = [ + { id: 'a', kind: 'worktree' }, + { id: 'b', kind: 'worktree' }, + { id: 'folder', kind: 'folder' } +] + +describe('isHostedTaskRepo', () => { + it('excludes folder workspaces and keeps everything else', () => { + expect(repos.filter(isHostedTaskRepo).map((repo) => repo.id)).toEqual(['a', 'b']) + expect(isHostedTaskRepo({ id: 'no-kind' })).toBe(true) + }) +}) + +describe('reconcileRepoSelection', () => { + it('treats an absent or empty persisted selection as "all repos"', () => { + expect(reconcileRepoSelection(repos, null)).toEqual(new Set()) + expect(reconcileRepoSelection(repos, [])).toEqual(new Set()) + }) + + it('drops ids this host no longer has', () => { + expect(reconcileRepoSelection(repos, ['a', 'gone'])).toEqual(new Set(['a'])) + }) + + // An empty set means "all repos", so a selection that survives nothing widens + // back out rather than filtering every row away. + it('widens back to all repos when nothing in the selection survives', () => { + expect(reconcileRepoSelection(repos, ['gone'])).toEqual(new Set()) + }) + + it('never selects a folder workspace', () => { + expect(reconcileRepoSelection(repos, ['folder'])).toEqual(new Set()) + }) +}) diff --git a/mobile/src/tasks/hosted-repo-selection.ts b/mobile/src/tasks/hosted-repo-selection.ts new file mode 100644 index 000000000..b3815bdad --- /dev/null +++ b/mobile/src/tasks/hosted-repo-selection.ts @@ -0,0 +1,27 @@ +/** Structural shape both helpers need. Kept minimal so the Tasks route's own + * RepoSummary satisfies it without importing anything back. */ +export type HostedRepoCandidate = { + id: string + kind?: string | null +} + +/** Folder workspaces have no hosted provider behind them, so they cannot back a + * GitHub/GitLab task or a Project board row. */ +export function isHostedTaskRepo(repo: HostedRepoCandidate): boolean { + return repo.kind !== 'folder' +} + +/** Narrows a persisted repo-id selection to what this host actually has. + * An empty result means "all repos", which is also what an empty persisted + * selection means, so a selection whose repos have all disappeared widens back + * out rather than silently matching nothing. */ +export function reconcileRepoSelection( + repos: readonly HostedRepoCandidate[], + persisted: readonly string[] | null | undefined +): Set { + if (!persisted || persisted.length === 0) { + return new Set() + } + const availableIds = new Set(repos.filter(isHostedTaskRepo).map((repo) => repo.id)) + return new Set(persisted.filter((id) => availableIds.has(id))) +} 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 000000000..053095aa3 --- /dev/null +++ b/mobile/src/tasks/use-host-repo-list.test.tsx @@ -0,0 +1,253 @@ +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: { 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' }]) + }) + + // 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) + 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 000000000..a1cabbaf1 --- /dev/null +++ b/mobile/src/tasks/use-host-repo-list.ts @@ -0,0 +1,129 @@ +import { + useCallback, + useEffect, + useLayoutEffect, + useMemo, + useReducer, + useRef, + useState +} 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) + const stateRef = useRef(state) + + // 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' }) + } + + // 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 + }) + + useLayoutEffect(() => { + if (boundKeyRef.current === clientKey) { + return + } + boundKeyRef.current = clientKey + reposRef.current = [] + inFlightRef.current = null + // Retires every request issued for the previous client. + requestIdRef.current += 1 + }, [clientKey]) + + // Why: consumers hold these in dependency arrays, so they must never change + // identity. Everything they read lives in refs, so they never need to. + const reload = useCallback(async (): Promise => { + const fetchNow = fetchRef.current + if (!fetchNow) { + return [] + } + if (inFlightRef.current) { + return inFlightRef.current + } + // Why: only the request issued for the currently bound client may commit. + // A slow response from the previous host would otherwise land afterwards + // and pin its repos as this host's authoritative list. + const requestKey = boundKeyRef.current + const requestId = requestIdRef.current + 1 + requestIdRef.current = requestId + const request = (async (): Promise => { + dispatch({ type: 'requested' }) + try { + const repos = await fetchNow() + // Why: A -> B -> A reuses the same client, so matching the key alone + // would let a stale request for A overwrite a newer result for A. + if (boundKeyRef.current !== requestKey || requestIdRef.current !== requestId) { + return [] + } + reposRef.current = repos + dispatch({ type: 'resolved', repos }) + return repos + } catch (err) { + if (boundKeyRef.current === requestKey && requestIdRef.current === requestId) { + dispatch({ + type: 'failed', + error: err instanceof Error ? err.message : 'Unknown error' + }) + } + throw err + } finally { + if (requestIdRef.current === requestId) { + inFlightRef.current = null + } + } + })() + inFlightRef.current = request + return request + }, []) + + // Why: within one event React has not rendered the `requested` dispatch yet, so + // the status still reads `loaded`. Join the in-flight request instead of + // handing back the list it is about to replace. + const ensureLoaded = useCallback( + (): Promise => + inFlightRef.current ?? + (needsHostRepoListFetch(stateRef.current) ? reload() : Promise.resolve(reposRef.current)), + [reload] + ) + + return useMemo(() => ({ state, ensureLoaded, reload }), [ensureLoaded, reload, state]) +}