From cbf300ca6af6d9e126d0fd4297b5eaba1d05b5da Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 12 Aug 2026 22:31:39 -0700 Subject: [PATCH] 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 --- src/renderer/src/store/slices/github.test.ts | 145 +++++++++++++++++++ src/renderer/src/store/slices/github.ts | 61 ++++++-- 2 files changed, 197 insertions(+), 9 deletions(-) diff --git a/src/renderer/src/store/slices/github.test.ts b/src/renderer/src/store/slices/github.test.ts index 5e5b0d61ccb..dfcd76ec5d1 100644 --- a/src/renderer/src/store/slices/github.test.ts +++ b/src/renderer/src/store/slices/github.test.ts @@ -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 & Pick + ): Omit { + 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() diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index 7ab87e09731..bd8f7b02625 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -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 — project-view has a single GraphQL source (no issue/PR fallback) and a distinct error union. @@ -2739,15 +2740,57 @@ export const createGitHubSlice: StateCreator = (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.