From aefdf2d0340e0732a161069d1e3d347419db59fe Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Wed, 9 Sep 2026 01:45:44 -0400 Subject: [PATCH] fix(mobile): await each per-repo GitHub count so one failure is a zero Returning the promise from inside the try let a rejected count escape the per-repo catch, reject the whole batch and reach the caller, which fires this without a handler. The inline call it replaced awaited and reported zero. The new test drives the real batching shape and fails if the await is removed. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb --- .../mobile-tasks-refactor-parity.test.ts | 4 +- ...mobile-tasks-github-count-failure.test.tsx | 95 +++++++++++++++++++ ...use-mobile-tasks-provider-load-actions.tsx | 5 +- 3 files changed, 101 insertions(+), 3 deletions(-) create mode 100644 mobile/src/tasks/use-mobile-tasks-github-count-failure.test.tsx diff --git a/mobile/src/tasks/mobile-tasks-refactor-parity.test.ts b/mobile/src/tasks/mobile-tasks-refactor-parity.test.ts index a2ff0be5dd6..20acdb373c0 100644 --- a/mobile/src/tasks/mobile-tasks-refactor-parity.test.ts +++ b/mobile/src/tasks/mobile-tasks-refactor-parity.test.ts @@ -25,9 +25,9 @@ const hash = (parts: string[] | string): string => * plus the row-target locals the adapters take in place of inline slug/number checks. Diff hooks, * declarations and styles are untouched. */ -const SCREEN_HOOKS = '0ef59ab331ad320a5ccabe8c227616af263ad2c468a805a6743dde8c186d7027' +const SCREEN_HOOKS = '9b2ac231e63169d3896ce30a3053262d30bc8f085456d75cbade522420205b02' const DIFF_HOOKS = '93c7189b32bed8456cc51814fffa8ce80cf62011ef968a9d53ddec2b9686f58f' -const STATEMENTS = 'e5d84617b7acbbdcaf17129d19670fac62a0399d967d9a6fc225cb9b807ae886' +const STATEMENTS = 'dd86f1ff89bf0bdbc3fbf159349d972a50d2dfa2607484848a34c4312b36ae60' const DECLARATIONS = 'cff54172af17a877789be1479c2eb6ca97d83c3e31dd831cd59395962f2b4c4a' const SEMANTICS = 'f767906884b93537f2c6369d6d0bd2d4cb39b4314c31cca9d8f9e5e9b78a75ee' const STYLES = '1db6af69c791d9963928541ad5310942fcbda6d984b422c90b6eb92b6816579a' diff --git a/mobile/src/tasks/use-mobile-tasks-github-count-failure.test.tsx b/mobile/src/tasks/use-mobile-tasks-github-count-failure.test.tsx new file mode 100644 index 00000000000..e6af00951da --- /dev/null +++ b/mobile/src/tasks/use-mobile-tasks-github-count-failure.test.tsx @@ -0,0 +1,95 @@ +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, describe, expect, it, vi } from 'vitest' + +// The module under test reaches the Tasks barrel, which pulls React Native in. Both barrels are +// stubbed with self-contained equivalents; `mapWithConcurrency` reproduces the shipped one +// (`mobile-tasks-item-mapping.ts`), which awaits each worker and so rejects the whole batch if +// any worker rejects. That is precisely the propagation this test pins the guard against. +vi.mock('./mobile-tasks-dependencies', () => ({ + CROSS_REPO_DISPLAY_LIMIT: 200, + PER_REPO_FETCH_LIMIT: 100, + extractGitHubIssueSourceError: () => null, + extractGitHubIssueSourceFallback: () => null, + isGitHubWorkItemsSshRemoteRequiredError: () => false, + useCallback: (callback: T): T => callback +})) +vi.mock('./mobile-tasks-legacy-foundation', () => ({ + GITHUB_REPO_CONCURRENCY: 3, + createGitHubTask: (item: unknown) => item, + async mapWithConcurrency( + items: T[], + limit: number, + worker: (item: T) => Promise + ): Promise { + const results: R[] = [] + let nextIndex = 0 + async function run(): Promise { + while (nextIndex < items.length) { + const index = nextIndex + nextIndex += 1 + results[index] = await worker(items[index]!) + } + } + await Promise.all(Array.from({ length: Math.min(limit, items.length) }, () => run())) + return results + }, + reconcileTeamSelection: () => new Set(), + scopeGitHubTaskSearch: (query: string): string => query, + taskTime: () => 0 +})) + +const { useMobileTasksProviderLoadActions } = + await import('./use-mobile-tasks-provider-load-actions') + +type CountOperations = { countGitHub: (payload: { repoId: string }) => Promise } +type LoadActions = { countGitHubItems: (operations: unknown, repos: unknown[]) => Promise } + +const repo = (id: string): { id: string } => ({ id }) + +describe('github work-item counting', () => { + let renderer: ReactTestRenderer | null = null + + function mount(): LoadActions { + let model: LoadActions | null = null + function Probe(): null { + model = useMobileTasksProviderLoadActions({ + appliedQuery: 'is:open', + githubKind: 'issues' + } as never) as unknown as LoadActions + return null + } + act(() => { + renderer = create(createElement(Probe)) + }) + if (!model) { + throw new Error('hook did not render') + } + return model + } + + afterEach(() => { + act(() => renderer?.unmount()) + renderer = null + vi.restoreAllMocks() + }) + + it('reads a failed per-repo count as zero and still resolves the total', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + const countGitHub = vi.fn(async ({ repoId }: { repoId: string }) => { + if (repoId === 'repo-broken') { + throw new Error('count failed') + } + return repoId === 'repo-a' ? 3 : 4 + }) + const operations: CountOperations = { countGitHub } + const model = mount() + + // Returning the promise instead of awaiting it would let this rejection escape the per-repo + // catch and reject the whole batch, and the only caller fires this with no handler. + await expect( + model.countGitHubItems(operations, [repo('repo-a'), repo('repo-broken'), repo('repo-b')]) + ).resolves.toBe(7) + expect(countGitHub).toHaveBeenCalledTimes(3) + }) +}) diff --git a/mobile/src/tasks/use-mobile-tasks-provider-load-actions.tsx b/mobile/src/tasks/use-mobile-tasks-provider-load-actions.tsx index b1561ef0116..46c6b6ad95e 100644 --- a/mobile/src/tasks/use-mobile-tasks-provider-load-actions.tsx +++ b/mobile/src/tasks/use-mobile-tasks-provider-load-actions.tsx @@ -167,7 +167,10 @@ export function useMobileTasksProviderLoadActions(model: RuntimeHydrationModel) GITHUB_REPO_CONCURRENCY, async (repo) => { try { - return listOperations.countGitHub({ + // Awaited inside the try on purpose: returning the promise would let a per-repo + // rejection escape this catch, reject the whole batch and reach a caller with no + // handler. A failed count is a zero, not a failed load. + return await listOperations.countGitHub({ repoId: repo.id, query: scopeGitHubTaskSearch(appliedQuery, githubKind) })