From 7b2996dc84ca85ce427d2121185f7e96f7f3d290 Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Wed, 28 Oct 2026 08:08:17 -0700 Subject: [PATCH] perf(github): look up refresh renderer owners by ID (#35015) * Use targeted owner lookups for PR refresh visibility * test(github): update document lifetime native lookup mock * test(github): collapse PR refresh owner lookup matrix into one guard --------- Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> --- .../pr-refresh-coordinator-test-mocks.ts | 25 +++- src/main/github/pr-refresh-visibility.test.ts | 126 ++++++++++++++++++ src/main/github/pr-refresh-visibility.ts | 9 +- ...ithub-pr-refresh-document-lifetime.test.ts | 8 +- 4 files changed, 155 insertions(+), 13 deletions(-) create mode 100644 src/main/github/pr-refresh-visibility.test.ts diff --git a/src/main/github/pr-refresh-coordinator-test-mocks.ts b/src/main/github/pr-refresh-coordinator-test-mocks.ts index 5c674077c8a..619cac5549e 100644 --- a/src/main/github/pr-refresh-coordinator-test-mocks.ts +++ b/src/main/github/pr-refresh-coordinator-test-mocks.ts @@ -1,10 +1,17 @@ import { vi, type Mock } from 'vitest' import { isWslUncPath } from '../../shared/wsl-paths' +type PRRefreshWebContentsMock = { + id: number + isDestroyed: () => boolean + send?: Mock +} + export type PRRefreshCoordinatorMocks = { sendMock: Mock sendToTrustedUIRendererMock: Mock - getAllWebContentsMock: Mock + getAllWebContentsMock: Mock<() => PRRefreshWebContentsMock[]> + webContentsFromIdMock: Mock<(id: number) => PRRefreshWebContentsMock | undefined> getPRForBranchOutcomeMock: Mock getOriginGitHubApiRepositoryMock: Mock getRateLimitMock: Mock @@ -19,6 +26,7 @@ export function createPRRefreshCoordinatorMocks(): PRRefreshCoordinatorMocks { sendMock: vi.fn(), sendToTrustedUIRendererMock: vi.fn(), getAllWebContentsMock: vi.fn(), + webContentsFromIdMock: vi.fn(), getPRForBranchOutcomeMock: vi.fn(), getOriginGitHubApiRepositoryMock: vi.fn(), getRateLimitMock: vi.fn(), @@ -28,12 +36,18 @@ export function createPRRefreshCoordinatorMocks(): PRRefreshCoordinatorMocks { } } -export type ElectronModuleMock = { webContents: { getAllWebContents: Mock } } +export type ElectronModuleMock = { + webContents: { + getAllWebContents: PRRefreshCoordinatorMocks['getAllWebContentsMock'] + fromId: PRRefreshCoordinatorMocks['webContentsFromIdMock'] + } +} export function electronModuleMock(mocks: PRRefreshCoordinatorMocks): ElectronModuleMock { return { webContents: { - getAllWebContents: mocks.getAllWebContentsMock + getAllWebContents: mocks.getAllWebContentsMock, + fromId: mocks.webContentsFromIdMock } } } @@ -86,6 +100,7 @@ export function resetPRRefreshCoordinatorMocks(mocks: PRRefreshCoordinatorMocks) sendMock, sendToTrustedUIRendererMock, getAllWebContentsMock, + webContentsFromIdMock, getPRForBranchOutcomeMock, getOriginGitHubApiRepositoryMock, getRateLimitMock, @@ -102,6 +117,10 @@ export function resetPRRefreshCoordinatorMocks(mocks: PRRefreshCoordinatorMocks) sendMock(channel, payload) }) getAllWebContentsMock.mockReset() + webContentsFromIdMock.mockReset() + webContentsFromIdMock.mockImplementation((id) => + getAllWebContentsMock().find((contents) => contents.id === id) + ) getPRForBranchOutcomeMock.mockReset() getOriginGitHubApiRepositoryMock.mockReset() getOriginGitHubApiRepositoryMock.mockResolvedValue({ diff --git a/src/main/github/pr-refresh-visibility.test.ts b/src/main/github/pr-refresh-visibility.test.ts new file mode 100644 index 00000000000..a1865a4736b --- /dev/null +++ b/src/main/github/pr-refresh-visibility.test.ts @@ -0,0 +1,126 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { coordinatorMocks, moduleMocks } = await vi.hoisted(async () => { + const moduleMocks = await import('./pr-refresh-coordinator-test-mocks') + return { coordinatorMocks: moduleMocks.createPRRefreshCoordinatorMocks(), moduleMocks } +}) +vi.mock('electron', () => moduleMocks.electronModuleMock(coordinatorMocks)) + +import { makeCandidate } from './pr-refresh-coordinator-test-harness' +import { refreshKey } from './pr-refresh-candidate-policy' +import { PRRefreshVisibility } from './pr-refresh-visibility' + +const { getAllWebContentsMock, webContentsFromIdMock } = coordinatorMocks + +describe('PR refresh owner liveness', () => { + beforeEach(() => moduleMocks.resetPRRefreshCoordinatorMocks(coordinatorMocks)) + afterEach(() => vi.useRealTimers()) + + it('checks only recorded owners by ID instead of a native census', () => { + const registry = new Map( + Array.from({ length: 1_000 }, (_, index) => { + const id = index + 1 + return [id, { id, isDestroyed: () => false }] as const + }) + ) + getAllWebContentsMock.mockImplementation(() => Array.from(registry.values())) + webContentsFromIdMock.mockImplementation((id) => registry.get(id)) + const candidates = Array.from({ length: 100 }, (_, index) => + makeCandidate({ branch: `feature/${index}`, cacheKey: `/repo::feature/${index}` }) + ) + const visibility = new PRRefreshVisibility() + visibility.report(candidates, 1, 1) + + expect(candidates.every((candidate) => visibility.has(refreshKey(candidate)))).toBe(true) + expect(visibility.report(candidates, 2, 1)).toBe(true) + expect(visibility.windowCount).toBe(1) + expect(getAllWebContentsMock).not.toHaveBeenCalled() + expect(webContentsFromIdMock.mock.calls.every(([id]) => id === 1)).toBe(true) + }) + + it('does no native work without recorded owners', () => { + const visibility = new PRRefreshVisibility() + expect(visibility.has(refreshKey(makeCandidate()))).toBe(false) + expect(visibility.windowCount).toBe(0) + expect(getAllWebContentsMock).not.toHaveBeenCalled() + expect(webContentsFromIdMock).not.toHaveBeenCalled() + }) + + it('prunes missing and destroyed owners while checking surviving owners afresh', () => { + const destroyed = vi.fn(() => false) + const registry = new Map([ + [1, { id: 1, isDestroyed: () => false }], + [2, { id: 2, isDestroyed: destroyed }], + [3, { id: 3, isDestroyed: () => false }] + ]) + getAllWebContentsMock.mockImplementation(() => Array.from(registry.values())) + webContentsFromIdMock.mockImplementation((id) => registry.get(id)) + const visibility = new PRRefreshVisibility() + const candidate = makeCandidate() + for (const id of registry.keys()) { + visibility.report([candidate], 1, id) + } + registry.delete(1) + destroyed.mockReturnValue(true) + webContentsFromIdMock.mockClear() + expect(visibility.has(refreshKey(candidate))).toBe(true) + expect(visibility.windowCount).toBe(1) + expect(webContentsFromIdMock.mock.calls).toEqual([[1], [2], [3]]) + registry.delete(3) + webContentsFromIdMock.mockClear() + expect(visibility.has(refreshKey(candidate))).toBe(false) + expect(webContentsFromIdMock.mock.calls).toEqual([[3]]) + expect(visibility.windowCount).toBe(0) + }) + + it('prunes an old owner at the next report boundary', () => { + const registry = new Map([ + [1, { id: 1, isDestroyed: () => false }], + [2, { id: 2, isDestroyed: () => false }] + ]) + getAllWebContentsMock.mockImplementation(() => Array.from(registry.values())) + webContentsFromIdMock.mockImplementation((id) => registry.get(id)) + const visibility = new PRRefreshVisibility() + const first = makeCandidate() + const second = makeCandidate({ branch: 'feature/second', cacheKey: '/repo::feature/second' }) + visibility.report([first], 1, 1) + registry.delete(1) + expect(visibility.report([second], 1, 2)).toBe(true) + expect(visibility.windowCount).toBe(1) + expect(visibility.has(refreshKey(first))).toBe(false) + expect(visibility.has(refreshKey(second))).toBe(true) + }) + + it('retains generation, cached fields and surviving selection ownership', () => { + const registry = new Map([ + [1, { id: 1, isDestroyed: () => false }], + [2, { id: 2, isDestroyed: () => false }] + ]) + getAllWebContentsMock.mockImplementation(() => Array.from(registry.values())) + webContentsFromIdMock.mockImplementation((id) => registry.get(id)) + const visibility = new PRRefreshVisibility() + const candidate = makeCandidate({ cachedFetchedAt: 1, cachedHeadOid: 'old-head' }) + const key = refreshKey(candidate) + visibility.report([{ ...candidate, isSelected: true }], 2, 1) + visibility.report([candidate], 1, 2) + visibility.update({ ...candidate, cachedFetchedAt: 3, cachedHeadOid: 'new-head' }) + expect(visibility.report([], 1, 1)).toBe(false) + expect(visibility.candidate(key, candidate)).toMatchObject({ + isSelected: true, + cachedFetchedAt: 3, + cachedHeadOid: 'new-head' + }) + expect(visibility.report([candidate], 3, 1)).toBe(true) + expect(visibility.candidate(key, candidate)).toMatchObject({ + isSelected: false, + cachedFetchedAt: 3, + cachedHeadOid: 'new-head' + }) + visibility.report([{ ...candidate, isSelected: true }], 2, 2) + expect(visibility.clearWindow(1)).toBe(true) + expect(visibility.has(key)).toBe(true) + expect(visibility.candidate(key, candidate)).toMatchObject({ isSelected: true }) + registry.delete(2) + expect(visibility.has(key)).toBe(false) + }) +}) diff --git a/src/main/github/pr-refresh-visibility.ts b/src/main/github/pr-refresh-visibility.ts index 15dab59db1e..bf163f7d408 100644 --- a/src/main/github/pr-refresh-visibility.ts +++ b/src/main/github/pr-refresh-visibility.ts @@ -84,14 +84,9 @@ export class PRRefreshVisibility { } private pruneDestroyedWindows(): void { - const liveWindowIds = new Set( - webContents - .getAllWebContents() - .filter((contents) => !contents.isDestroyed()) - .map((contents) => contents.id) - ) for (const windowId of this.visibleByWindow.keys()) { - if (!liveWindowIds.has(windowId)) { + const contents = webContents.fromId(windowId) + if (!contents || contents.isDestroyed()) { this.visibleByWindow.delete(windowId) } } diff --git a/src/main/ipc/github-pr-refresh-document-lifetime.test.ts b/src/main/ipc/github-pr-refresh-document-lifetime.test.ts index 30543cbb5fe..cea64ae837e 100644 --- a/src/main/ipc/github-pr-refresh-document-lifetime.test.ts +++ b/src/main/ipc/github-pr-refresh-document-lifetime.test.ts @@ -5,14 +5,14 @@ import type { PRRefreshOutcome } from '../../shared/github/pull-request-refresh- const { handlers, lookup, contents } = vi.hoisted(() => ({ handlers: new Map unknown>(), lookup: vi.fn(), - contents: vi.fn() + contents: new Map boolean }>() })) vi.mock('electron', () => ({ ipcMain: { handle: (channel: string, handler: (...args: unknown[]) => unknown) => handlers.set(channel, handler) }, - webContents: { getAllWebContents: contents } + webContents: { fromId: (id: number) => contents.get(id) } })) vi.mock('../github/client', () => ({ getPRForBranch: vi.fn(), getPRForBranchOutcome: lookup })) vi.mock('../github/pr-refresh-rate-limit-gate', () => ({ @@ -44,7 +44,8 @@ async function setup() { const coordinator = await import('../github/pr-refresh-coordinator') const owner = sender(17) const other = sender(18) - contents.mockReturnValue([owner, other]) + contents.set(owner.id, owner) + contents.set(other.id, other) // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: Visible refresh registration reads only this store fixture's repo list. const store = { getRepos: () => [] } as never // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: Every PR is already counted, so the observer reads only this stats method. @@ -64,6 +65,7 @@ describe('automatic PR refresh document ownership', () => { vi.useFakeTimers() vi.setSystemTime(1_700_000_000_000) handlers.clear() + contents.clear() lookup.mockReset().mockImplementation(async () => found()) }) afterEach(() => {