mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 08:03:39 +00:00
* fix(tasks): cap advertised GitHub pages at the search result window GitHub's Search API rejects requests past its first-1000-results window with HTTP 422, but totalPages was derived from the raw total_count, so the pagination bar advertised pages that could never load and clicks on them silently did nothing (#11485). Cap per-repo advertised pages at floor(1000 / perRepoLimit), and when a page load comes back empty, say so with a toast instead of ignoring the click — clamping the advertised count only when no fetch threw, so transient failures don't shrink the bar. * fix(tasks): key pagination resets on repo selection, not array identity The repos store installs a fresh array on every repos:changed event, so the pagination-reset effect fired on background refreshes and bumped the request generation, silently discarding any in-flight page navigation — clicking an unloaded page did nothing whenever a repo refresh landed during the fetch. Key the effect on the stable selection string instead. * fix(tasks): distinguish end-of-data, window 422s, and failures on empty pages Adversarial-review round 1 rework: - fetchWorkItemsNextPage now returns issue-side envelope error types — the channel the search-window 422 actually travels on (failedCount only counts thrown repo calls). - resolveEmptyPageOutcome (unit-tested) maps an empty page to window-unreachable (clamp + toast), load-failed (toast only; may be transient), or end-of-data (silently withdraw the speculative page the count-fallback advertises). - The work-items fetch effect is keyed on selectedReposKey too — its unconditional page reset re-fired on every repos:changed array identity, bouncing the user to page 1 mid-click. The key now includes the resolved GitHub source context so identity changes still re-dispatch. - Toasts carry stable ids so repeats replace instead of stack. - Cap comment documents the conservative PR-scope tail loss; cap tests pinned at shipped (36 → 27) and dividing (25 → 40) limits. * fix(tasks): withdraw the speculative page when the failed count is zero countedTotalPages of 0 comes from a swallowed count failure and routes totalPages through the fallback, so the clamp must replace it like null. * fix(tasks): tighten empty-page outcomes after round-2 review - en.json's loadPageUnreachable carried the pre-reword text, and the catalog beats the inline default — the two toasts were identical. - end-of-data clamps only while the count is unknown/failed: the PR list path swallows its own failures into clean-empty results, and clamping a real count silently hid healthy pages (worse than the pre-fix no-op). - A window 422 no longer clamps when a sibling repo's fetch threw. - The generation effect mirrors every fetch-effect dep that resets page state, so manual refresh/source switches invalidate in-flight clicks. - selectedReposKey extracted as buildSelectedReposKey with stability tests; envelope error types wire-tested through the store. * fix(tasks): clamp against the committed count, not the click-time closure Round-3 review: the count promise routinely resolves between click and response, so deciding the end-of-data clamp from the closure value let a stale null overwrite a real count. applyEmptyPageClamp now runs inside the functional updater against the committed value, never raises an earlier clamp, and a window 422 coinciding with a thrown sibling repo resolves as load-failed so the toast and the clamp always agree. * fix(tasks): only an all-window-422 empty page may clamp; harden count merges Round-4 review: a sibling repo's envelope 403/404 arrives with failedCount still 0, so the window branch now requires every error to be the window 422 (non-window validation errors are demoted at the store); the count resolution mins against an applied clamp instead of re-advertising withdrawn pages; the generation effect mirrors taskResumeApplied so its doc claim holds. * fix(tasks): split the proven window limit from the count slot Round-5 review: min-ing the count against an applied clamp pinned a SPECULATIVE end-of-data withdrawal that raced ahead of the count, permanently collapsing the bar for the generation. Proven window-422 limits now live in provenPageLimit (set once, only lowered, reset per generation); the count overwrites its own slot unconditionally; and deriveAdvertisedTotalPages (unit-tested for both arrival orders) caps the count-or-fallback estimate with the proven limit, floored at the loaded pages. * fix(tasks): surface PR-side list failures so they can't read as end-of-data Round-6 review: PartialWorkItemsResult had no PR error slot, so a swallowed gh pr list failure reached the renderer as a clean empty page — and with the count blocked (0) the speculative withdrawal deleted the pagination bar with no toast and no recovery (a regression vs main's silent no-op). PR-side errors now ride the envelope (errors.prs), demoted so they can never join the issue-only window-422 signal; errorTypes replaces issueErrorTypes; an empty page that a real count said should exist now toasts instead of looking dead. * test(tasks): cover the PR-error envelope end-to-end; neutral no-more-results toast Round-7 review: the two literal gh-utils mocks lacked classifyListPrsError (a PR-side rejection in those suites would TypeError instead of assert), and the producer half of the errors.prs contract had no main-side test — added both, plus a classifier contract test pinning the search-window phrase the renderer keys on. The refused-clamp toast now reads the committed count via a synchronous ref mirror instead of the click-time closure, and says 'No more results' — nothing failed on that branch. Both toast keys plus the new one are translated in es/ja/ko/zh. * fix(tasks): preserve final reachable GitHub search page * Extract GitHub search result window error pattern to shared constant Extract the 1000-result window detection pattern to a single source of truth so the classifier and consumer stay synchronized. The pattern is the only signal separating a permanently unreachable page from a transient validation failure, so drift or trimming silently demotes window 422s to generic failures and stops capping the advertised page count (#11485). --------- Co-authored-by: Jinjing <6427696+AmethystLiang@users.noreply.github.com>
318 lines
12 KiB
TypeScript
318 lines
12 KiB
TypeScript
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
|
import type * as GithubApiRepositoryModule from './github-api-repository'
|
|
|
|
const {
|
|
execFileAsyncMock,
|
|
ghExecFileAsyncMock,
|
|
getOwnerRepoMock,
|
|
getIssueOwnerRepoMock,
|
|
getOwnerRepoForRemoteMock,
|
|
resolveIssueSourceMock,
|
|
gitExecFileAsyncMock,
|
|
rateLimitGuardMock,
|
|
noteRateLimitSpendMock,
|
|
repositoryRateLimitGuardMock,
|
|
noteRepositoryRateLimitSpendMock,
|
|
spendsSharedGitHubComQuotaMock,
|
|
acquireMock,
|
|
releaseMock
|
|
} = vi.hoisted(() => ({
|
|
execFileAsyncMock: vi.fn(),
|
|
ghExecFileAsyncMock: vi.fn(),
|
|
getOwnerRepoMock: vi.fn(),
|
|
getIssueOwnerRepoMock: vi.fn(),
|
|
getOwnerRepoForRemoteMock: vi.fn(),
|
|
resolveIssueSourceMock: vi.fn(),
|
|
gitExecFileAsyncMock: vi.fn(),
|
|
rateLimitGuardMock: vi.fn((_bucket?: unknown) => ({ blocked: false })),
|
|
noteRateLimitSpendMock: vi.fn(),
|
|
repositoryRateLimitGuardMock: vi.fn((_repo: unknown, bucket: string) =>
|
|
rateLimitGuardMock(bucket)
|
|
),
|
|
noteRepositoryRateLimitSpendMock: vi.fn(),
|
|
spendsSharedGitHubComQuotaMock: vi.fn(() => true),
|
|
acquireMock: vi.fn(),
|
|
releaseMock: vi.fn()
|
|
}))
|
|
|
|
vi.mock('./gh-utils', () => ({
|
|
execFileAsync: execFileAsyncMock,
|
|
ghExecFileAsync: ghExecFileAsyncMock,
|
|
githubRepoContext: (
|
|
repoPath: string,
|
|
connectionId?: string | null,
|
|
localGitOptions: { wslDistro?: string } = {}
|
|
) => ({
|
|
repoPath,
|
|
connectionId: connectionId ?? null,
|
|
...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {})
|
|
}),
|
|
ghRepoExecOptions: (context: {
|
|
repoPath: string
|
|
connectionId?: string | null
|
|
wslDistro?: string
|
|
}) =>
|
|
context.connectionId
|
|
? {}
|
|
: { cwd: context.repoPath, ...(context.wslDistro ? { wslDistro: context.wslDistro } : {}) },
|
|
getOwnerRepo: getOwnerRepoMock,
|
|
getIssueOwnerRepo: getIssueOwnerRepoMock,
|
|
getOwnerRepoForRemote: getOwnerRepoForRemoteMock,
|
|
resolveIssueSource: resolveIssueSourceMock,
|
|
acquire: acquireMock,
|
|
release: releaseMock,
|
|
_resetOwnerRepoCache: vi.fn(),
|
|
classifyGhError: (stderr: string) => ({ type: 'unknown', message: stderr }),
|
|
classifyListIssuesError: (stderr: string) => ({ type: 'unknown', message: stderr }),
|
|
classifyListPrsError: (stderr: string) => ({ type: 'unknown', message: stderr })
|
|
}))
|
|
|
|
vi.mock('../git/runner', () => ({
|
|
gitExecFileAsync: gitExecFileAsyncMock
|
|
}))
|
|
|
|
vi.mock('./rate-limit', () => ({
|
|
rateLimitGuard: rateLimitGuardMock,
|
|
noteRateLimitSpend: noteRateLimitSpendMock,
|
|
getRateLimit: vi.fn(async () => ({ ok: false, error: 'not probed in tests' })),
|
|
// Mirror production: shared-scope calls delegate to the global guard/spend.
|
|
repositoryRateLimitGuard: repositoryRateLimitGuardMock,
|
|
noteRepositoryRateLimitSpend: noteRepositoryRateLimitSpendMock,
|
|
spendsSharedGitHubComQuota: spendsSharedGitHubComQuotaMock
|
|
}))
|
|
|
|
vi.mock('./github-api-repository', async (importOriginal) => {
|
|
const actual = await importOriginal<typeof GithubApiRepositoryModule>()
|
|
return {
|
|
...actual,
|
|
// Why: these suites drive source resolution through the legacy gh-utils
|
|
// mocks; bridge the hosted seams onto the same mocks.
|
|
resolveIssueGitHubApiRepositorySource: (
|
|
repoPath: string,
|
|
preference: unknown,
|
|
connectionId?: string | null,
|
|
localGitOptions?: unknown
|
|
) => resolveIssueSourceMock(repoPath, preference, connectionId, localGitOptions),
|
|
getIssueGitHubApiRepository: (repoPath: string, connectionId?: string | null) =>
|
|
getIssueOwnerRepoMock(repoPath, connectionId),
|
|
getOriginGitHubApiRepository: (
|
|
repoPath: string,
|
|
connectionId?: string | null,
|
|
localGitOptions?: unknown
|
|
) => getOwnerRepoMock(repoPath, connectionId, localGitOptions),
|
|
getGitHubApiRepositoryForRemote: (
|
|
repoPath: string,
|
|
remoteName: string,
|
|
connectionId?: string | null,
|
|
localGitOptions?: unknown
|
|
) =>
|
|
remoteName === 'origin'
|
|
? getOwnerRepoMock(repoPath, connectionId, localGitOptions)
|
|
: getOwnerRepoForRemoteMock(repoPath, remoteName, connectionId, localGitOptions)
|
|
}
|
|
})
|
|
|
|
import {
|
|
countWorkItems,
|
|
listWorkItems,
|
|
_resetMergeQueueCacheForTests,
|
|
_resetOwnerRepoCache
|
|
} from './client'
|
|
import { GITHUB_WORK_ITEMS_QUERY_MAX_BYTES } from '../../shared/github-work-items-query-bounds'
|
|
|
|
import { _resetOriginGitHubApiRepositoryCache } from './github-api-repository'
|
|
|
|
// The origin-repository cache is module-level state; reset it so slugs
|
|
// resolved by one test cannot leak into the next.
|
|
beforeEach(() => {
|
|
_resetOriginGitHubApiRepositoryCache()
|
|
})
|
|
// Why: split from client-work-items.test.ts to keep both suites under the
|
|
// max-lines cap; this file owns query/paging request-shaping cases.
|
|
describe('listWorkItems query paging', () => {
|
|
beforeEach(() => {
|
|
execFileAsyncMock.mockReset()
|
|
ghExecFileAsyncMock.mockReset()
|
|
getOwnerRepoMock.mockReset()
|
|
getIssueOwnerRepoMock.mockReset()
|
|
getOwnerRepoForRemoteMock.mockReset()
|
|
resolveIssueSourceMock.mockReset()
|
|
gitExecFileAsyncMock.mockReset()
|
|
rateLimitGuardMock.mockReset()
|
|
rateLimitGuardMock.mockReturnValue({ blocked: false })
|
|
noteRateLimitSpendMock.mockReset()
|
|
repositoryRateLimitGuardMock.mockClear()
|
|
noteRepositoryRateLimitSpendMock.mockReset()
|
|
spendsSharedGitHubComQuotaMock.mockReset()
|
|
spendsSharedGitHubComQuotaMock.mockReturnValue(true)
|
|
acquireMock.mockReset()
|
|
releaseMock.mockReset()
|
|
acquireMock.mockResolvedValue(undefined)
|
|
// Why: preference-aware `listWorkItems` calls `resolveIssueSource`.
|
|
// Route through the same `getIssueOwnerRepoMock` so existing tests that
|
|
// only set up `getIssueOwnerRepoMock` continue to work.
|
|
resolveIssueSourceMock.mockImplementation(async () => ({
|
|
source: await getIssueOwnerRepoMock(),
|
|
fellBack: false
|
|
}))
|
|
getOwnerRepoForRemoteMock.mockResolvedValue(null)
|
|
_resetOwnerRepoCache()
|
|
_resetMergeQueueCacheForTests()
|
|
})
|
|
|
|
it('returns zero for oversized count queries before resolving repo sources', async () => {
|
|
const secret = 'main-github-work-items-secret'
|
|
const oversizedQuery = secret + 'x'.repeat(GITHUB_WORK_ITEMS_QUERY_MAX_BYTES)
|
|
|
|
await expect(countWorkItems('/repo-root', oversizedQuery)).resolves.toBe(0)
|
|
|
|
expect(resolveIssueSourceMock).not.toHaveBeenCalled()
|
|
expect(getIssueOwnerRepoMock).not.toHaveBeenCalled()
|
|
expect(getOwnerRepoMock).not.toHaveBeenCalled()
|
|
expect(ghExecFileAsyncMock).not.toHaveBeenCalled()
|
|
expect(acquireMock).not.toHaveBeenCalled()
|
|
expect(releaseMock).not.toHaveBeenCalled()
|
|
})
|
|
|
|
it('uses an implicit WSL UNC cwd for search quota accounting', async () => {
|
|
const repository = { owner: 'acme', repo: 'widgets', host: 'github.com' }
|
|
const repoPath = String.raw`\\wsl.localhost\Ubuntu\home\me\widgets`
|
|
getIssueOwnerRepoMock.mockResolvedValueOnce(repository)
|
|
getOwnerRepoMock.mockResolvedValueOnce(repository)
|
|
ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '3' })
|
|
spendsSharedGitHubComQuotaMock.mockReturnValue(false)
|
|
|
|
await expect(countWorkItems(repoPath, 'is:issue is:open')).resolves.toBe(3)
|
|
|
|
const executionOptions = { cwd: repoPath, host: 'github.com' }
|
|
expect(spendsSharedGitHubComQuotaMock).toHaveBeenCalledWith(repository, executionOptions)
|
|
expect(repositoryRateLimitGuardMock).toHaveBeenCalledWith(
|
|
repository,
|
|
'search',
|
|
executionOptions
|
|
)
|
|
expect(noteRepositoryRateLimitSpendMock).toHaveBeenCalledWith(
|
|
repository,
|
|
'search',
|
|
1,
|
|
executionOptions
|
|
)
|
|
})
|
|
|
|
it('passes review-requested as a --search qualifier (gh CLI has no dedicated flag)', async () => {
|
|
getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
|
|
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
|
|
ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' })
|
|
|
|
await listWorkItems('/repo-root', 10, 'review-requested:@me is:open')
|
|
|
|
expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1)
|
|
expect(ghExecFileAsyncMock).toHaveBeenCalledWith(
|
|
expect.arrayContaining(['--search', 'is:pr is:open review-requested:@me sort:created-desc']),
|
|
{ cwd: '/repo-root' }
|
|
)
|
|
expect(ghExecFileAsyncMock).not.toHaveBeenCalledWith(
|
|
expect.arrayContaining(['--review-requested']),
|
|
expect.anything()
|
|
)
|
|
})
|
|
|
|
it('uses the requested numbered Search API page for issues', async () => {
|
|
getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
|
|
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
|
|
ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' })
|
|
|
|
await listWorkItems('/repo-root', 10, 'is:issue is:open', 2)
|
|
|
|
expect(ghExecFileAsyncMock).toHaveBeenCalledWith(
|
|
[
|
|
'api',
|
|
'--cache',
|
|
'120s',
|
|
`search/issues?q=${encodeURIComponent('repo:acme/widgets is:issue is:open')}&sort=created&order=desc&per_page=10&page=2`,
|
|
'--jq',
|
|
'.items'
|
|
],
|
|
{ cwd: '/repo-root' }
|
|
)
|
|
})
|
|
|
|
it('fetches and slices stable PR results for the requested numbered page', async () => {
|
|
getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
|
|
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
|
|
ghExecFileAsyncMock.mockResolvedValueOnce({
|
|
stdout: JSON.stringify(
|
|
[1, 2, 3, 4].map((number) => ({
|
|
number,
|
|
title: `PR ${number}`,
|
|
state: 'OPEN',
|
|
url: `https://github.com/acme/widgets/pull/${number}`,
|
|
labels: [],
|
|
updatedAt: `2026-07-0${number}T00:00:00Z`,
|
|
author: { login: 'octocat' },
|
|
isDraft: false,
|
|
headRefName: `feature/${number}`,
|
|
headRefOid: `head-${number}`,
|
|
baseRefName: 'main'
|
|
}))
|
|
)
|
|
})
|
|
|
|
const { items } = await listWorkItems('/repo-root', 2, 'is:pr is:open', 2)
|
|
|
|
expect(ghExecFileAsyncMock).toHaveBeenCalledWith(
|
|
expect.arrayContaining(['--limit', '4', '--search', 'is:pr is:open sort:created-desc']),
|
|
{ cwd: '/repo-root' }
|
|
)
|
|
expect(items.map((item) => item.number)).toEqual([4, 3])
|
|
})
|
|
|
|
it('lifts a swallowed PR-side failure onto errors.prs instead of reading as end-of-data', async () => {
|
|
getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
|
|
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
|
|
// Non-availability failure (plain 403): swallowed into [], must still surface.
|
|
ghExecFileAsyncMock.mockRejectedValueOnce(new Error('HTTP 403: Forbidden'))
|
|
|
|
const envelope = await listWorkItems('/repo-root', 10, 'is:pr is:open', 2)
|
|
|
|
expect(envelope.items).toEqual([])
|
|
expect(envelope.errors?.issues).toBeUndefined()
|
|
expect(envelope.errors?.prs).toEqual({
|
|
type: 'unknown',
|
|
message: expect.stringContaining('HTTP 403')
|
|
})
|
|
})
|
|
|
|
it('filters pull request rows out of issue Search API results', async () => {
|
|
getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
|
|
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
|
|
ghExecFileAsyncMock.mockResolvedValueOnce({
|
|
stdout: JSON.stringify([
|
|
{
|
|
number: 2,
|
|
title: 'PR-shaped search row',
|
|
state: 'open',
|
|
html_url: 'https://github.com/acme/widgets/pull/2',
|
|
labels: [],
|
|
updated_at: '2026-07-02T00:00:00Z',
|
|
user: { login: 'octocat' },
|
|
pull_request: { url: 'https://api.github.com/repos/acme/widgets/pulls/2' }
|
|
},
|
|
{
|
|
number: 1,
|
|
title: 'Issue row',
|
|
state: 'open',
|
|
html_url: 'https://github.com/acme/widgets/issues/1',
|
|
labels: [],
|
|
updated_at: '2026-07-01T00:00:00Z',
|
|
user: { login: 'octocat' }
|
|
}
|
|
])
|
|
})
|
|
|
|
const { items } = await listWorkItems('/repo-root', 10, 'is:issue is:open')
|
|
|
|
expect(items.map((item) => item.id)).toEqual(['issue:1'])
|
|
})
|
|
})
|