mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 00:03:45 +00:00
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.
This commit is contained in:
@@ -205,6 +205,7 @@ import {
|
||||
} from '@/components/task-page-cache-selectors'
|
||||
import { shouldHideTaskPageListChrome } from '@/components/task-page-list-chrome-visibility'
|
||||
import {
|
||||
buildSelectedReposKey,
|
||||
getTaskPagePerRepoLimit,
|
||||
resolveEmptyPageOutcome,
|
||||
taskPageToGitHubApiPage
|
||||
@@ -3206,20 +3207,10 @@ export default function TaskPage(): React.JSX.Element {
|
||||
[eligibleRepos, repoSelection]
|
||||
)
|
||||
|
||||
// Why: stable string key for selectedRepos — the repos store installs a fresh
|
||||
// array on every repos:changed event, so array-identity deps re-fire even when
|
||||
// the selection is unchanged. Includes the resolved GitHub source context so a
|
||||
// provider-identity change still re-keys everything the requests depend on.
|
||||
// Why: see buildSelectedReposKey — array-identity deps re-fire on every
|
||||
// repos:changed even when the selection is unchanged.
|
||||
const selectedReposKey = useMemo(
|
||||
() =>
|
||||
selectedRepos
|
||||
.map(
|
||||
(r) =>
|
||||
`${r.id}|${r.path}|${r.connectionId ?? ''}|${r.executionHostId ?? ''}|${JSON.stringify(
|
||||
getTaskPageRepoSourceContext(r, 'github')
|
||||
)}`
|
||||
)
|
||||
.join(','),
|
||||
() => buildSelectedReposKey(selectedRepos, (r) => getTaskPageRepoSourceContext(r, 'github')),
|
||||
[selectedRepos]
|
||||
)
|
||||
|
||||
@@ -3830,12 +3821,21 @@ export default function TaskPage(): React.JSX.Element {
|
||||
|
||||
// Why: keyed on selectedReposKey, not the selectedRepos array — a background
|
||||
// repos:changed refresh mid-flight would otherwise bump the generation and
|
||||
// silently discard the user's page navigation (#11485).
|
||||
// silently discard the user's page navigation (#11485). Mirrors every dep of
|
||||
// the fetch effect that resets page state, so a reset always invalidates
|
||||
// in-flight page requests.
|
||||
useEffect(() => {
|
||||
paginationGenerationRef.current += 1
|
||||
setPaginationLoading(false)
|
||||
setLoadingTargetPage(null)
|
||||
}, [selectedReposKey, appliedTaskSearch, workItemsInvalidationNonce])
|
||||
}, [
|
||||
selectedReposKey,
|
||||
appliedTaskSearch,
|
||||
workItemsInvalidationNonce,
|
||||
taskRefreshNonce,
|
||||
taskSource,
|
||||
githubMode
|
||||
])
|
||||
|
||||
// Why: the dialog's "Use" button routes through the same direct-launch flow as the row-level "Use" CTA so behavior is consistent regardless of entry point.
|
||||
const githubTaskDrawerWorkItem = useAppStore((s) => s.githubTaskDrawerWorkItem)
|
||||
@@ -6190,7 +6190,12 @@ export default function TaskPage(): React.JSX.Element {
|
||||
if (items.length === 0) {
|
||||
// Why: see resolveEmptyPageOutcome — a dead click needs feedback only
|
||||
// when something actually failed; a clean empty probe is end-of-data.
|
||||
const outcome = resolveEmptyPageOutcome({ target, failedCount, issueErrorTypes })
|
||||
const outcome = resolveEmptyPageOutcome({
|
||||
target,
|
||||
failedCount,
|
||||
issueErrorTypes,
|
||||
countedTotalPages
|
||||
})
|
||||
if (outcome.reason === 'window-unreachable') {
|
||||
toast.error(
|
||||
translate(
|
||||
@@ -6242,6 +6247,7 @@ export default function TaskPage(): React.JSX.Element {
|
||||
paginationLoading,
|
||||
selectedRepos,
|
||||
currentPage,
|
||||
countedTotalPages,
|
||||
appliedTaskSearch,
|
||||
fetchWorkItemsNextPage,
|
||||
githubPageSize,
|
||||
|
||||
@@ -2,6 +2,7 @@ import { describe, expect, it, vi } from 'vitest'
|
||||
import type { GitHubWorkItem } from '../../../shared/types'
|
||||
import {
|
||||
accumulateWorkItemPages,
|
||||
buildSelectedReposKey,
|
||||
getTaskPagePerRepoLimit,
|
||||
resolveEmptyPageOutcome,
|
||||
taskPageToGitHubApiPage,
|
||||
@@ -61,41 +62,95 @@ describe('numbered GitHub pagination', () => {
|
||||
})
|
||||
|
||||
describe('resolveEmptyPageOutcome', () => {
|
||||
const base = { failedCount: 0, issueErrorTypes: [] as const, countedTotalPages: null }
|
||||
|
||||
it('clamps and reports unreachable when the search window 422 is present', () => {
|
||||
expect(
|
||||
resolveEmptyPageOutcome({ target: 33, failedCount: 0, issueErrorTypes: ['validation_error'] })
|
||||
resolveEmptyPageOutcome({ ...base, target: 33, issueErrorTypes: ['validation_error'] })
|
||||
).toEqual({ reason: 'window-unreachable', clampTotalPagesTo: 33 })
|
||||
// A thrown repo alongside the 422 doesn't mask the window signal.
|
||||
// A real count is still clamped — the count over-advertising is the bug.
|
||||
expect(
|
||||
resolveEmptyPageOutcome({ target: 5, failedCount: 1, issueErrorTypes: ['validation_error'] })
|
||||
).toEqual({ reason: 'window-unreachable', clampTotalPagesTo: 5 })
|
||||
resolveEmptyPageOutcome({
|
||||
...base,
|
||||
target: 33,
|
||||
issueErrorTypes: ['validation_error'],
|
||||
countedTotalPages: 39
|
||||
})
|
||||
).toEqual({ reason: 'window-unreachable', clampTotalPagesTo: 33 })
|
||||
})
|
||||
|
||||
it('does not clamp on a window 422 when a sibling repo fetch threw', () => {
|
||||
// Repos advance independently; another repo's transient failure says
|
||||
// nothing about the healthy repos' remaining pages.
|
||||
expect(
|
||||
resolveEmptyPageOutcome({
|
||||
...base,
|
||||
target: 5,
|
||||
failedCount: 1,
|
||||
issueErrorTypes: ['validation_error']
|
||||
})
|
||||
).toEqual({ reason: 'window-unreachable', clampTotalPagesTo: null })
|
||||
})
|
||||
|
||||
it('reports a failure without clamping for transient or unclassified errors', () => {
|
||||
expect(resolveEmptyPageOutcome({ target: 5, failedCount: 2, issueErrorTypes: [] })).toEqual({
|
||||
expect(resolveEmptyPageOutcome({ ...base, target: 5, failedCount: 2 })).toEqual({
|
||||
reason: 'load-failed',
|
||||
clampTotalPagesTo: null
|
||||
})
|
||||
for (const type of ['permission_denied', 'not_found', 'rate_limited', 'unknown'] as const) {
|
||||
expect(
|
||||
resolveEmptyPageOutcome({ target: 5, failedCount: 0, issueErrorTypes: [type] })
|
||||
).toEqual({ reason: 'load-failed', clampTotalPagesTo: null })
|
||||
expect(resolveEmptyPageOutcome({ ...base, target: 5, issueErrorTypes: [type] })).toEqual({
|
||||
reason: 'load-failed',
|
||||
clampTotalPagesTo: null
|
||||
})
|
||||
}
|
||||
})
|
||||
|
||||
it('treats a clean empty probe as end-of-data and withdraws the speculative page', () => {
|
||||
expect(resolveEmptyPageOutcome({ target: 2, failedCount: 0, issueErrorTypes: [] })).toEqual({
|
||||
it('withdraws the speculative page on clean empty only while the count is unknown', () => {
|
||||
expect(resolveEmptyPageOutcome({ ...base, target: 2 })).toEqual({
|
||||
reason: 'end-of-data',
|
||||
clampTotalPagesTo: 2
|
||||
})
|
||||
// 0 means the count itself failed — treat like unknown.
|
||||
expect(resolveEmptyPageOutcome({ ...base, target: 2, countedTotalPages: 0 })).toEqual({
|
||||
reason: 'end-of-data',
|
||||
clampTotalPagesTo: 2
|
||||
})
|
||||
// A real count must not shrink: the PR list path swallows its own failures
|
||||
// into clean-empty results, and clamping would hide healthy pages silently.
|
||||
expect(resolveEmptyPageOutcome({ ...base, target: 2, countedTotalPages: 34 })).toEqual({
|
||||
reason: 'end-of-data',
|
||||
clampTotalPagesTo: null
|
||||
})
|
||||
// Never clamps below one advertised page.
|
||||
expect(resolveEmptyPageOutcome({ target: 0, failedCount: 0, issueErrorTypes: [] })).toEqual({
|
||||
expect(resolveEmptyPageOutcome({ ...base, target: 0 })).toEqual({
|
||||
reason: 'end-of-data',
|
||||
clampTotalPagesTo: 1
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
describe('buildSelectedReposKey', () => {
|
||||
const contextFor = (r: { id: string }) => ({ provider: 'github', repoId: r.id })
|
||||
|
||||
it('is stable across fresh arrays with identical contents', () => {
|
||||
const a = [{ id: 'r1', path: '/a', connectionId: null, executionHostId: null }]
|
||||
const b = [{ ...a[0] }]
|
||||
expect(buildSelectedReposKey(a, contextFor)).toBe(buildSelectedReposKey(b, contextFor))
|
||||
})
|
||||
|
||||
it('changes when any request-relevant field changes', () => {
|
||||
const repo = { id: 'r1', path: '/a', connectionId: null, executionHostId: null }
|
||||
const key = buildSelectedReposKey([repo], contextFor)
|
||||
expect(buildSelectedReposKey([{ ...repo, executionHostId: 'ssh:host' }], contextFor)).not.toBe(
|
||||
key
|
||||
)
|
||||
expect(buildSelectedReposKey([{ ...repo, path: '/b' }], contextFor)).not.toBe(key)
|
||||
expect(
|
||||
buildSelectedReposKey([repo], (r) => ({ provider: 'github', repoId: r.id, owner: 'new' }))
|
||||
).not.toBe(key)
|
||||
})
|
||||
})
|
||||
|
||||
describe('accumulateWorkItemPages', () => {
|
||||
it('drops the re-fetched boundary row that shares the previous page cursor', async () => {
|
||||
const boundary = item('r', 'issue:2', '2026-07-02')
|
||||
|
||||
@@ -45,26 +45,59 @@ export type EmptyPageOutcome = {
|
||||
* An empty page load has three distinct meanings, and only the caller-side
|
||||
* error channels can tell them apart (#11485):
|
||||
* - a `validation_error` is GitHub's 422 for pages past its 1000-result search
|
||||
* window — the page can never load, so stop advertising it;
|
||||
* window — the page can never load, so stop advertising it (unless a sibling
|
||||
* repo's fetch threw, which says nothing about the other repos' windows);
|
||||
* - any other per-repo error (thrown or issue-side envelope) may be transient
|
||||
* (rate limit, permissions), so surface it but keep the advertised count;
|
||||
* - no error at all is the designed end-of-data signal from probing the
|
||||
* speculative page `fallbackTotalPages` advertises while the count is
|
||||
* unknown — withdraw the phantom page silently.
|
||||
* - no error at all is end-of-data. That only warrants a clamp while the count
|
||||
* is unknown (null) or failed (0), to withdraw the speculative page
|
||||
* `fallbackTotalPages` advertises — a real count must not shrink, because a
|
||||
* provider that swallows its own failures (the PR list path) also produces
|
||||
* clean-empty results.
|
||||
*/
|
||||
export function resolveEmptyPageOutcome(args: {
|
||||
target: number
|
||||
failedCount: number
|
||||
issueErrorTypes: readonly ClassifiedError['type'][]
|
||||
countedTotalPages: number | null
|
||||
}): EmptyPageOutcome {
|
||||
const clamp = Math.max(1, Math.floor(args.target))
|
||||
if (args.issueErrorTypes.includes('validation_error')) {
|
||||
return { reason: 'window-unreachable', clampTotalPagesTo: clamp }
|
||||
return {
|
||||
reason: 'window-unreachable',
|
||||
clampTotalPagesTo: args.failedCount === 0 ? clamp : null
|
||||
}
|
||||
}
|
||||
if (args.failedCount > 0 || args.issueErrorTypes.length > 0) {
|
||||
return { reason: 'load-failed', clampTotalPagesTo: null }
|
||||
}
|
||||
return { reason: 'end-of-data', clampTotalPagesTo: clamp }
|
||||
const countUnknown = args.countedTotalPages === null || args.countedTotalPages === 0
|
||||
return { reason: 'end-of-data', clampTotalPagesTo: countUnknown ? clamp : null }
|
||||
}
|
||||
|
||||
/**
|
||||
* Stable identity string for a repo selection. The repos store installs a fresh
|
||||
* array on every repos:changed event, so effects keyed on array identity re-fire
|
||||
* with the selection unchanged — resetting pagination mid-click (#11485). The
|
||||
* key must cover every repo field the work-item requests read; the caller
|
||||
* supplies the resolved source context (which must stay free of timestamps).
|
||||
*/
|
||||
export function buildSelectedReposKey<
|
||||
T extends {
|
||||
id: string
|
||||
path: string
|
||||
connectionId?: string | null
|
||||
executionHostId?: string | null
|
||||
}
|
||||
>(repos: readonly T[], sourceContextFor: (repo: T) => unknown): string {
|
||||
return repos
|
||||
.map(
|
||||
(r) =>
|
||||
`${r.id}|${r.path}|${r.connectionId ?? ''}|${r.executionHostId ?? ''}|${JSON.stringify(
|
||||
sourceContextFor(r)
|
||||
)}`
|
||||
)
|
||||
.join(',')
|
||||
}
|
||||
|
||||
// Why: provider pages cannot spill truncated rows into the next page. Divide
|
||||
|
||||
@@ -1890,7 +1890,7 @@
|
||||
"noGithubSourceDetected": "No GitHub source detected for",
|
||||
"noGithubSourceDetectedHint": "it may have no GitHub remote, or the source could not be resolved.",
|
||||
"jiraLinkSourceUnavailable": "Couldn’t link this Jira issue. Reconnect Jira or pick the matching site, then try again.",
|
||||
"loadPageUnreachable": "Page {{value0}} could not be loaded from GitHub.",
|
||||
"loadPageUnreachable": "Page {{value0}} is beyond what GitHub search can return.",
|
||||
"loadPageFailed": "Page {{value0}} could not be loaded from GitHub."
|
||||
},
|
||||
"Terminal": {
|
||||
|
||||
@@ -6661,6 +6661,39 @@ describe('createGitHubSlice.fetchWorkItems source/error envelope', () => {
|
||||
})
|
||||
})
|
||||
|
||||
it('surfaces issue-side envelope errors as issueErrorTypes on next-page fetches', async () => {
|
||||
runtimeEnvironmentCall.mockResolvedValueOnce({
|
||||
id: 'rpc-work-items-page-422',
|
||||
ok: true,
|
||||
result: {
|
||||
items: [],
|
||||
sources: {
|
||||
issues: { owner: 'up', repo: 'r' },
|
||||
prs: null,
|
||||
originCandidate: { owner: 'up', repo: 'r' },
|
||||
upstreamCandidate: null
|
||||
},
|
||||
errors: {
|
||||
issues: { type: 'validation_error', message: 'only the first 1000 search results' }
|
||||
}
|
||||
},
|
||||
_meta: { runtimeId: 'remote-runtime' }
|
||||
})
|
||||
const store = createTestStore()
|
||||
store.setState({
|
||||
settings: { activeRuntimeEnvironmentId: 'env-1' },
|
||||
repos: [{ id: 'runtime-repo-id', path: '/server/repo', name: 'repo', kind: 'git' }]
|
||||
} as unknown as Partial<AppState>)
|
||||
|
||||
const result = await store
|
||||
.getState()
|
||||
.fetchWorkItemsNextPage([{ repoId: 'caller-repo-id', path: '/server/repo' }], 24, 100, '', 34)
|
||||
|
||||
// The window 422 travels on the envelope error channel, not failedCount —
|
||||
// resolveEmptyPageOutcome keys on this exact string (#11485).
|
||||
expect(result).toEqual({ items: [], failedCount: 0, issueErrorTypes: ['validation_error'] })
|
||||
})
|
||||
|
||||
it('routes work-item counts through the active runtime environment', async () => {
|
||||
runtimeEnvironmentCall.mockResolvedValueOnce({
|
||||
id: 'rpc-work-items-count',
|
||||
|
||||
Reference in New Issue
Block a user