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>

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({

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)
+  })
+})

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)
       }
     }

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<string, (...args: unknown[]) => unknown>(),
   lookup: vi.fn(),
-  contents: vi.fn()
+  contents: new Map<number, EventEmitter & { id: number; isDestroyed: () => 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(() => {