mirror of
https://github.com/stablyai/orca.git
synced 2026-10-22 00:03:47 +00:00
perf(github): reuse workItemsCache identity on no-op refetch
A force refresh always remapped IPC rows and allocated a new cache entry, so TaskPage's useShallow selector treated every poll as a real change and remapped visible work-item rows. Reconcile nested rows structurally, bump fetchedAt on the existing entry when nothing changed, and only write a new entry when a row or meta field actually differs. Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
Neil
Orca
@@ -6300,6 +6300,151 @@ describe('createGitHubSlice.refreshGitHubForWorktree', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('createGitHubSlice.fetchWorkItems cache identity', () => {
|
||||
const nestedSources = {
|
||||
issues: { owner: 'acme', repo: 'widgets' },
|
||||
prs: { owner: 'acme', repo: 'widgets' },
|
||||
originCandidate: { owner: 'acme', repo: 'widgets' },
|
||||
upstreamCandidate: { owner: 'acme-upstream', repo: 'widgets' }
|
||||
}
|
||||
|
||||
function makeNestedWorkItem(
|
||||
overrides: Partial<GitHubWorkItem> & Pick<GitHubWorkItem, 'id' | 'number' | 'title'>
|
||||
): Omit<GitHubWorkItem, 'repoId'> {
|
||||
return {
|
||||
type: 'pr',
|
||||
state: 'open',
|
||||
url: `https://github.com/acme/widgets/pull/${overrides.number}`,
|
||||
labels: ['bug', 'needs-review'],
|
||||
updatedAt: '2026-05-22T00:00:00Z',
|
||||
author: 'octocat',
|
||||
authorAvatarUrl: 'https://avatars.example/octocat.png',
|
||||
branchName: 'feature/nested',
|
||||
baseRefName: 'main',
|
||||
headSha: 'abc123def456',
|
||||
prRepo: { owner: 'acme', repo: 'widgets', host: 'github.com' },
|
||||
additions: 12,
|
||||
deletions: 3,
|
||||
changedFiles: 2,
|
||||
reviewDecision: 'REVIEW_REQUIRED',
|
||||
reviewRequests: [
|
||||
{ login: 'reviewer', name: 'Reviewer', avatarUrl: 'https://avatars.example/reviewer.png' }
|
||||
],
|
||||
latestReviews: [
|
||||
{ login: 'reviewer', state: 'COMMENTED', avatarUrl: 'https://avatars.example/reviewer.png' }
|
||||
],
|
||||
assignees: [
|
||||
{ login: 'assignee', name: 'Assignee', avatarUrl: 'https://avatars.example/assignee.png' }
|
||||
],
|
||||
checksSummary: {
|
||||
state: 'pending',
|
||||
total: 4,
|
||||
passed: 1,
|
||||
failed: 0,
|
||||
pending: 3,
|
||||
neutral: 0
|
||||
},
|
||||
mergeable: 'MERGEABLE',
|
||||
...overrides
|
||||
}
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
resetRemoteRuntimeMocks()
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks()
|
||||
})
|
||||
|
||||
it('reuses the cache map, entry, and nested rows on a no-op force refetch', async () => {
|
||||
const store = createTestStore()
|
||||
const items = [
|
||||
makeNestedWorkItem({ id: 'pr:42', number: 42, title: 'First nested PR' }),
|
||||
makeNestedWorkItem({ id: 'pr:43', number: 43, title: 'Second nested PR' })
|
||||
]
|
||||
mockApi.gh.listWorkItems.mockImplementation(() =>
|
||||
Promise.resolve({
|
||||
items: structuredClone(items),
|
||||
sources: structuredClone(nestedSources)
|
||||
})
|
||||
)
|
||||
|
||||
let now = 1_700_000_000_000
|
||||
vi.spyOn(Date, 'now').mockImplementation(() => now)
|
||||
|
||||
await store.getState().fetchWorkItems('repo-id', '/repo', 24, '')
|
||||
const cacheKey = workItemsCacheKey('repo-id', 24, '')
|
||||
const previousCache = store.getState().workItemsCache
|
||||
const previousEntry = previousCache[cacheKey]
|
||||
const previousRows = previousEntry?.data
|
||||
expect(previousRows).toHaveLength(2)
|
||||
expect(previousRows?.[0]?.labels).toEqual(['bug', 'needs-review'])
|
||||
expect(previousRows?.[0]?.assignees?.[0]?.login).toBe('assignee')
|
||||
expect(previousRows?.[0]?.reviewRequests?.[0]?.login).toBe('reviewer')
|
||||
expect(previousRows?.[0]?.prRepo).toEqual({
|
||||
owner: 'acme',
|
||||
repo: 'widgets',
|
||||
host: 'github.com'
|
||||
})
|
||||
expect(previousRows?.[0]?.checksSummary?.state).toBe('pending')
|
||||
|
||||
now += 5_000
|
||||
await store.getState().fetchWorkItems('repo-id', '/repo', 24, '', { force: true })
|
||||
|
||||
const nextCache = store.getState().workItemsCache
|
||||
const nextEntry = nextCache[cacheKey]
|
||||
expect(nextCache).toBe(previousCache)
|
||||
expect(nextEntry).toBe(previousEntry)
|
||||
expect(nextEntry?.data).toBe(previousRows)
|
||||
expect(nextEntry?.data?.[0]).toBe(previousRows?.[0])
|
||||
expect(nextEntry?.data?.[1]).toBe(previousRows?.[1])
|
||||
expect(nextEntry?.sources).toBe(previousEntry?.sources)
|
||||
expect(nextEntry?.fetchedAt).toBe(now)
|
||||
expect(nextEntry?.fetchedAt).toBeGreaterThan(1_700_000_000_000)
|
||||
})
|
||||
|
||||
it('writes a new entry when a nested reviewRequests login changes but reuses the sibling row', async () => {
|
||||
const store = createTestStore()
|
||||
const first = makeNestedWorkItem({ id: 'pr:42', number: 42, title: 'First nested PR' })
|
||||
const second = makeNestedWorkItem({ id: 'pr:43', number: 43, title: 'Second nested PR' })
|
||||
mockApi.gh.listWorkItems.mockResolvedValueOnce({
|
||||
items: structuredClone([first, second]),
|
||||
sources: structuredClone(nestedSources)
|
||||
})
|
||||
|
||||
await store.getState().fetchWorkItems('repo-id', '/repo', 24, '')
|
||||
const cacheKey = workItemsCacheKey('repo-id', 24, '')
|
||||
const previousCache = store.getState().workItemsCache
|
||||
const previousEntry = previousCache[cacheKey]
|
||||
const previousRows = previousEntry?.data
|
||||
expect(previousRows).toHaveLength(2)
|
||||
|
||||
const changedFirst = structuredClone(first)
|
||||
const nextReviewer = changedFirst.reviewRequests?.[0]
|
||||
if (nextReviewer) {
|
||||
nextReviewer.login = 'reviewer-updated'
|
||||
}
|
||||
mockApi.gh.listWorkItems.mockResolvedValueOnce({
|
||||
items: [changedFirst, structuredClone(second)],
|
||||
sources: structuredClone(nestedSources)
|
||||
})
|
||||
|
||||
await store.getState().fetchWorkItems('repo-id', '/repo', 24, '', { force: true })
|
||||
|
||||
const nextCache = store.getState().workItemsCache
|
||||
const nextEntry = nextCache[cacheKey]
|
||||
expect(nextCache).not.toBe(previousCache)
|
||||
expect(nextEntry).not.toBe(previousEntry)
|
||||
expect(nextEntry?.data).not.toBe(previousRows)
|
||||
expect(nextEntry?.data?.[0]).not.toBe(previousRows?.[0])
|
||||
expect(nextEntry?.data?.[0]?.reviewRequests?.[0]?.login).toBe('reviewer-updated')
|
||||
expect(nextEntry?.data?.[1]).toBe(previousRows?.[1])
|
||||
expect(nextEntry?.sources).toBe(previousEntry?.sources)
|
||||
})
|
||||
})
|
||||
|
||||
describe('createGitHubSlice.fetchWorkItems source/error envelope', () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
|
||||
@@ -78,6 +78,7 @@ import { normalizeGitHubPRForBranchOutcome } from '../../../../shared/github-pr-
|
||||
import { restoreReactionOnSubject, setReactionOnSubject } from '@/lib/pr-comment-reactions'
|
||||
import { withGitHubCheckDetailsTimeout } from '@/runtime/github-check-details-timeout'
|
||||
import { getGitHubRepoLookupIndex } from './github-repo-lookup-index'
|
||||
import { areValuesEqual, reconcileCatalogRows } from './repo-identity-reconcile'
|
||||
|
||||
// ─── ProjectV2 cache types ────────────────────────────────────────────
|
||||
// Why: separate from CacheEntry<T> — project-view has a single GraphQL source (no issue/PR fallback) and a distinct error union.
|
||||
@@ -2739,15 +2740,57 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
||||
if (get().workItemsInvalidationNonce !== requestInvalidationNonce) {
|
||||
return items
|
||||
}
|
||||
set((s) => ({
|
||||
workItemsCache: withBoundedCacheEntry(s.workItemsCache, key, {
|
||||
data: items,
|
||||
fetchedAt: Date.now(),
|
||||
sources: envelope.sources,
|
||||
...(errorForCache ? { error: errorForCache } : {}),
|
||||
...(envelope.issueSourceFellBack ? { issueSourceFellBack: true } : {})
|
||||
})
|
||||
}))
|
||||
// Why: TaskPage useShallow-selects cache entry refs. A new { ...entry, fetchedAt }
|
||||
// still remaps every visible row. IPC structuredClone rebuilds nested records, so
|
||||
// data === previous.data never holds — reconcile structurally, then either mutate
|
||||
// fetchedAt in place or write one entry that keeps unchanged row/meta refs.
|
||||
set((s) => {
|
||||
const previousEntry = s.workItemsCache[key]
|
||||
const previousData = previousEntry?.data ?? []
|
||||
const reconciled = reconcileCatalogRows(
|
||||
previousData,
|
||||
items,
|
||||
(row) => `${row.repoId}\0${row.id}`
|
||||
)
|
||||
const nextFellBack = envelope.issueSourceFellBack ? true : undefined
|
||||
const sourcesUnchanged = areValuesEqual(previousEntry?.sources, envelope.sources)
|
||||
const errorUnchanged = areValuesEqual(previousEntry?.error, errorForCache)
|
||||
const fellBackUnchanged = previousEntry?.issueSourceFellBack === nextFellBack
|
||||
if (
|
||||
previousEntry &&
|
||||
reconciled === previousData &&
|
||||
sourcesUnchanged &&
|
||||
errorUnchanged &&
|
||||
fellBackUnchanged
|
||||
) {
|
||||
previousEntry.fetchedAt = Date.now()
|
||||
return {}
|
||||
}
|
||||
const previousSources = previousEntry?.sources
|
||||
const previousError = previousEntry?.error
|
||||
return {
|
||||
workItemsCache: withBoundedCacheEntry(s.workItemsCache, key, {
|
||||
data:
|
||||
previousEntry?.data != null && reconciled === previousEntry.data
|
||||
? previousEntry.data
|
||||
: reconciled.slice(),
|
||||
fetchedAt: Date.now(),
|
||||
sources:
|
||||
sourcesUnchanged && previousSources !== undefined
|
||||
? previousSources
|
||||
: envelope.sources,
|
||||
...(errorForCache
|
||||
? {
|
||||
error:
|
||||
errorUnchanged && previousError !== undefined
|
||||
? previousError
|
||||
: errorForCache
|
||||
}
|
||||
: {}),
|
||||
...(nextFellBack ? { issueSourceFellBack: true } : {})
|
||||
})
|
||||
}
|
||||
})
|
||||
return items
|
||||
} catch (err) {
|
||||
// Why: rethrow but keep the stale cache entry so the UI still renders while the user retries.
|
||||
|
||||
Reference in New Issue
Block a user