From 796004ba141b2ba03cac3f4774a2ea6c537fbf58 Mon Sep 17 00:00:00 2001 From: Katherine Jensen Date: Thu, 30 Jul 2026 10:20:56 -0700 Subject: [PATCH 1/6] PT-4299: heal project picker when layering PDPF registers after grace window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Open-project dropdown was intermittently empty at startup because internalGetMetadataWithRetries only retried within 30 s of process start. If the USJ-providing layering PDPF (Scripture Extender) registered after that window, the filtered getMetadataForAllProjects call returned [] and the picker had no way to learn it needed to re-fetch — onDidReloadExtensions and onDidChangeProjects do not fire for a single PDPF registration. Fix: subscribe use-project-picker-data to object:onDidCreateNetworkObject (already a public network event) and call refreshMetadata() whenever a pdpFactory object registers. The refresh issues a fresh getMetadataForAllProjects call with the factory now registered, so the project list heals independent of the 30-second bound. Two tests added (TDD RED→GREEN): - verifies allProjects heals after a late pdpFactory registration - verifies non-PDPF object registrations do NOT trigger a re-fetch Co-Authored-By: Claude Sonnet 4.6 --- .../use-project-picker-data.hook.test.ts | 78 +++++++++++++++++++ .../hooks/use-project-picker-data.hook.ts | 23 +++++- 2 files changed, 100 insertions(+), 1 deletion(-) diff --git a/src/renderer/hooks/use-project-picker-data.hook.test.ts b/src/renderer/hooks/use-project-picker-data.hook.test.ts index 57771cc470f..1a0d9a79bea 100644 --- a/src/renderer/hooks/use-project-picker-data.hook.test.ts +++ b/src/renderer/hooks/use-project-picker-data.hook.test.ts @@ -584,5 +584,83 @@ describe('useProjectPickerData', () => { expect(projectLookupService.getMetadataForAllProjects).toHaveBeenCalledTimes(2); }); + + it('re-fetches metadata when a PDP factory registers via object:onDidCreateNetworkObject (PT-4299 healing)', async () => { + // Reproduces the PT-4299 race: the 30-second startup grace window in + // internalGetMetadataWithRetries expires before the USJ-providing layering PDPF + // (Scripture Extender) registers. The initial metadata fetch returns empty because no + // factory providing platformScripture.USJ_Chapter has registered yet. When the PDPF + // later registers, the network object service emits object:onDidCreateNetworkObject. + // The hook must subscribe to that event and refresh when the new object is a pdpFactory, + // so the project list heals without waiting for an unrelated extension reload or + // project-list change event. + const { getNetworkEvent, projectLookupService } = await importMocks(); + let pdpfRegistrationCallback: ((details: { objectType: string }) => void) | undefined; + vi.mocked(getNetworkEvent).mockImplementation( + (eventName: string) => + vi.fn((cb: (details: { objectType: string }) => void) => { + if (eventName === 'object:onDidCreateNetworkObject') pdpfRegistrationCallback = cb; + return vi.fn(); + }) as never, + ); + + // First fetch: grace window expired, USJ-providing layering PDPF not yet registered. + vi.mocked(projectLookupService.getMetadataForAllProjects) + .mockResolvedValueOnce([] as never) + // Second fetch after PDPF registers: project list now available. + .mockResolvedValue( + metadataList([ + { id: 'p1', fullName: 'Full p1', name: 'Short p1', isEditable: true }, + ]) as never, + ); + + const { result } = renderHook(() => useProjectPickerData()); + await settle(result); + // Initial state: empty because the layering PDPF providing USJ_Chapter has not + // registered yet (grace window already expired). + expect(result.current.allProjects).toHaveLength(0); + + // The USJ-providing layering PDPF registers after the grace window. + expect(pdpfRegistrationCallback).toBeDefined(); + act(() => pdpfRegistrationCallback!({ objectType: 'pdpFactory' })); + + await settle(result); + // The hook must detect the PDPF registration and re-fetch metadata so the list heals. + expect(projectLookupService.getMetadataForAllProjects).toHaveBeenCalledTimes(2); + expect(result.current.allProjects).toHaveLength(1); + expect(result.current.allProjects[0].id).toBe('p1'); + }); + + it('does not re-fetch metadata when object:onDidCreateNetworkObject fires for a non-PDPF network object', async () => { + // Only PDP factory registrations should heal the project list. Registrations of other + // network object types (PDPs, services, etc.) must not trigger an unnecessary metadata + // fan-out, since those fan-outs are expensive (one PAPI round-trip per registered PDPF). + const { getNetworkEvent, projectLookupService } = await importMocks(); + let networkObjectCallback: ((details: { objectType: string }) => void) | undefined; + vi.mocked(getNetworkEvent).mockImplementation( + (eventName: string) => + vi.fn((cb: (details: { objectType: string }) => void) => { + if (eventName === 'object:onDidCreateNetworkObject') networkObjectCallback = cb; + return vi.fn(); + }) as never, + ); + vi.mocked(projectLookupService.getMetadataForAllProjects).mockResolvedValue( + metadataList([ + { id: 'p1', fullName: 'Full p1', name: 'Short p1', isEditable: true }, + ]) as never, + ); + + const { result } = renderHook(() => useProjectPickerData()); + await settle(result); + expect(projectLookupService.getMetadataForAllProjects).toHaveBeenCalledTimes(1); + + expect(networkObjectCallback).toBeDefined(); + // A non-PDPF network object registers (e.g. a data provider or a service). + act(() => networkObjectCallback!({ objectType: 'networkObject' })); + await settle(result); + + // Must NOT re-fetch: only PDPF registrations should invalidate the metadata cache. + expect(projectLookupService.getMetadataForAllProjects).toHaveBeenCalledTimes(1); + }); }); /* eslint-enable no-type-assertion/no-type-assertion */ diff --git a/src/renderer/hooks/use-project-picker-data.hook.ts b/src/renderer/hooks/use-project-picker-data.hook.ts index 32e838d5ec9..44e58c98a31 100644 --- a/src/renderer/hooks/use-project-picker-data.hook.ts +++ b/src/renderer/hooks/use-project-picker-data.hook.ts @@ -6,7 +6,11 @@ import { webViews } from '@renderer/services/papi-frontend.service'; import { projectLookupService } from '@shared/services/project-lookup.service'; import { normalizeProjectId } from '@shared/models/project-lookup.service-model'; import { type ProjectMetadata } from '@shared/models/project-metadata.model'; -import { type ProjectMetadataFilterOptions } from '@shared/models/project-data-provider-factory.interface'; +import { + PDP_FACTORY_OBJECT_TYPE, + type ProjectMetadataFilterOptions, +} from '@shared/models/project-data-provider-factory.interface'; +import { type NetworkObjectDetails } from '@shared/models/network-object.model'; import { EVENT_NAME_ON_DID_CLOSE_WEB_VIEW, EVENT_NAME_ON_DID_OPEN_WEB_VIEW, @@ -116,6 +120,23 @@ export function useProjectPickerData(): ProjectPickerData { // three sections refetch. const onDidChangeProjects = useMemo(() => getNetworkEvent('platform.onDidChangeProjects'), []); useEvent(onDidChangeProjects, refreshMetadata); + // Heal the project list when a PDP factory registers after the startup grace window expires + // (PT-4299). internalGetMetadataWithRetries only retries within 30 s of process start; if the + // USJ-providing layering PDPF (Scripture Extender) arrives after that window AND neither + // onDidReloadExtensions nor onDidChangeProjects fires, the picker stays empty until the user + // triggers an unrelated refresh. Subscribing here ensures a late-arriving PDPF always heals + // the list independent of the 30-second bound. + const onDidCreateNetworkObject = useMemo( + () => getNetworkEvent('object:onDidCreateNetworkObject'), + [], + ); + const onPdpFactoryRegistered = useCallback( + ({ objectType }: NetworkObjectDetails) => { + if (objectType === PDP_FACTORY_OBJECT_TYPE) refreshMetadata(); + }, + [refreshMetadata], + ); + useEvent(onDidCreateNetworkObject, onPdpFactoryRegistered); // Recent project IDs from the service — reactive, updates when user opens projects const [rawRecentIds, , isRecentIdsLoading] = useData( From 6d36d2944707110b6c2e20e260e80e9990549f71 Mon Sep 17 00:00:00 2001 From: Katherine Jensen Date: Thu, 30 Jul 2026 12:47:38 -0700 Subject: [PATCH 2/6] PT-4299: retry metadata fetch after getAvailableProjects RPC timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the extension host is overloaded at startup, a PDPF's getAvailableProjects RPC can time out (30 s JSON-RPC timeout). The rejected promise invalidates the cache entry but no recovery signal fires, so allProjects stays empty until an unrelated refresh. Add a retry-on-failure path: after getMetadataForAllProjects rejects, schedule a re-fetch after 5 s (METADATA_FETCH_RETRY_DELAY_MS). The extension host typically drains its queue within 1-2 s of the timeout, so the retry succeeds. Cap at MAX_METADATA_FETCH_RETRIES = 3 consecutive failures so a permanently unavailable host does not loop forever. This is the second of two fixes for PT-4299: 1. onDidCreateNetworkObject → refreshMetadata (late PDPF registration) 2. isRetryPending → 5 s timer → refreshMetadata (RPC timeout recovery) Co-authored-by: Claude Sonnet 4.6 --- .../use-project-picker-data.hook.test.ts | 88 ++++++++++++++++++- .../hooks/use-project-picker-data.hook.ts | 52 +++++++++-- 2 files changed, 133 insertions(+), 7 deletions(-) diff --git a/src/renderer/hooks/use-project-picker-data.hook.test.ts b/src/renderer/hooks/use-project-picker-data.hook.test.ts index 1a0d9a79bea..1e75d2a378d 100644 --- a/src/renderer/hooks/use-project-picker-data.hook.test.ts +++ b/src/renderer/hooks/use-project-picker-data.hook.test.ts @@ -2,7 +2,11 @@ import { renderHook, act } from '@testing-library/react'; import '@testing-library/jest-dom'; import { vi } from 'vitest'; import { EVENT_NAME_ON_DID_UPDATE_WEB_VIEW } from '@shared/services/web-view.service-model'; -import { useProjectPickerData, type ProjectPickerData } from './use-project-picker-data.hook'; +import { + useProjectPickerData, + type ProjectPickerData, + MAX_METADATA_FETCH_RETRIES, +} from './use-project-picker-data.hook'; // --- Mocks --- @@ -631,6 +635,88 @@ describe('useProjectPickerData', () => { expect(result.current.allProjects[0].id).toBe('p1'); }); + it('heals allProjects after a timed-out metadata fetch once the retry resolves (PT-4299 timeout recovery)', async () => { + // Reproduces the scenario observed in console logs where getAvailableProjects times out + // (JSON-RPC 30-second timeout) because the extension host is overloaded at startup. The + // late response is discarded ("Ignoring subsequent resolution"), leaving the picker empty. + // After METADATA_FETCH_RETRY_DELAY_MS the hook must issue a fresh fetch — by which time the + // extension host has drained its queue and responds quickly. + vi.useFakeTimers(); + try { + const { projectLookupService } = await importMocks(); + vi.mocked(projectLookupService.getMetadataForAllProjects) + .mockRejectedValueOnce(new Error('JSON-RPC Request timed out')) + .mockResolvedValue( + metadataList([ + { id: 'p1', fullName: 'Full p1', name: 'Short p1', isEditable: true }, + ]) as never, + ); + + const { result } = renderHook(() => useProjectPickerData()); + await settle(result); + + // First fetch timed out — picker is empty + expect(result.current.allProjects).toHaveLength(0); + expect(vi.mocked(projectLookupService.getMetadataForAllProjects)).toHaveBeenCalledTimes(1); + + // Advance past the retry delay + await act(async () => { + await vi.runAllTimersAsync(); + }); + await settle(result); + + // Retry resolved — picker healed + expect(vi.mocked(projectLookupService.getMetadataForAllProjects)).toHaveBeenCalledTimes(2); + expect(result.current.allProjects).toHaveLength(1); + expect(result.current.allProjects[0].id).toBe('p1'); + } finally { + vi.useRealTimers(); + } + }); + + it('stops retrying metadata after MAX_METADATA_FETCH_RETRIES consecutive failures', async () => { + // Guards against an infinite retry loop when the extension host is permanently unavailable. + // After MAX_METADATA_FETCH_RETRIES failures the hook must stop scheduling retries. + vi.useFakeTimers(); + try { + const { projectLookupService } = await importMocks(); + vi.mocked(projectLookupService.getMetadataForAllProjects).mockRejectedValue( + new Error('Persistent failure'), + ); + + const { result } = renderHook(() => useProjectPickerData()); + await settle(result); + + // Drive through all allowed retries + for (let i = 0; i < MAX_METADATA_FETCH_RETRIES; i += 1) { + // Each iteration must be sequential: advance the timer first, then drain the resulting + // async work before the next retry cycle starts. + // eslint-disable-next-line no-await-in-loop + await act(async () => { + await vi.runAllTimersAsync(); + }); + // Same reason as above: sequential drain after each timer advance. + // eslint-disable-next-line no-await-in-loop + await settle(result); + } + + const callCountAfterExhaustion = vi.mocked(projectLookupService.getMetadataForAllProjects) + .mock.calls.length; + expect(callCountAfterExhaustion).toBe(1 + MAX_METADATA_FETCH_RETRIES); + + // One more timer advance must NOT trigger an additional fetch + await act(async () => { + await vi.runAllTimersAsync(); + }); + await settle(result); + expect(vi.mocked(projectLookupService.getMetadataForAllProjects)).toHaveBeenCalledTimes( + callCountAfterExhaustion, + ); + } finally { + vi.useRealTimers(); + } + }); + it('does not re-fetch metadata when object:onDidCreateNetworkObject fires for a non-PDPF network object', async () => { // Only PDP factory registrations should heal the project list. Registrations of other // network object types (PDPs, services, etc.) must not trigger an unnecessary metadata diff --git a/src/renderer/hooks/use-project-picker-data.hook.ts b/src/renderer/hooks/use-project-picker-data.hook.ts index 44e58c98a31..7944a3ee16d 100644 --- a/src/renderer/hooks/use-project-picker-data.hook.ts +++ b/src/renderer/hooks/use-project-picker-data.hook.ts @@ -1,6 +1,6 @@ import { useData } from '@renderer/hooks/papi-hooks'; import { useEvent, usePromise } from 'platform-bible-react'; -import { useCallback, useMemo, useRef, useState } from 'react'; +import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { getNetworkEvent } from '@shared/services/network.service'; import { webViews } from '@renderer/services/papi-frontend.service'; import { projectLookupService } from '@shared/services/project-lookup.service'; @@ -30,6 +30,15 @@ import { type ProjectItem } from '@renderer/components/projects/project-picker.c * this interface via the Scripture Extender layering PDPF, so the current project and recent * projects (both always scripture or resource projects) resolve from the same filtered fetch. */ +/** How long to wait before retrying a failed metadata fetch. */ +const METADATA_FETCH_RETRY_DELAY_MS = 5 * 1000; +/** + * Maximum number of consecutive metadata fetch failures before the hook stops retrying. After this + * many failures the picker stays empty until the next external refresh signal (e.g. an extension + * reload or a PDPF registration). + */ +export const MAX_METADATA_FETCH_RETRIES = 3; + const PICKER_PROJECT_INTERFACE = 'platformScripture.USJ_Chapter'; const PICKER_METADATA_FILTER: ProjectMetadataFilterOptions = { includeProjectInterfaces: [PICKER_PROJECT_INTERFACE], @@ -102,6 +111,13 @@ export function useProjectPickerData(): ProjectPickerData { const [currentProjectError, setCurrentProjectError] = useState(undefined); const refreshMetadata = useCallback(() => setMetadataRefreshCounter((n) => n + 1), []); const refreshActiveEditor = useCallback(() => setWebViewRefreshCounter((n) => n + 1), []); + // When getMetadataForAllProjects rejects (e.g. a PDPF's getAvailableProjects RPC times out + // during startup), isRetryPending becomes true and the effect below schedules a re-fetch after + // METADATA_FETCH_RETRY_DELAY_MS — by which time the extension host has typically drained its + // queue and responds quickly. fetchRetryCountRef caps consecutive retries so a permanently + // unavailable host does not loop forever. + const [isRetryPending, setIsRetryPending] = useState(false); + const fetchRetryCountRef = useRef(0); const onDidOpenWebView = useMemo(() => getNetworkEvent(EVENT_NAME_ON_DID_OPEN_WEB_VIEW), []); useEvent(onDidOpenWebView, refreshActiveEditor); @@ -137,6 +153,18 @@ export function useProjectPickerData(): ProjectPickerData { [refreshMetadata], ); useEvent(onDidCreateNetworkObject, onPdpFactoryRegistered); + // After a failed metadata fetch, wait METADATA_FETCH_RETRY_DELAY_MS before trying again. + // The timer is cancelled if the hook unmounts before it fires, preventing state updates on an + // unmounted component and avoiding spurious fan-outs when the user closes the picker quickly. + useEffect(() => { + if (!isRetryPending) return undefined; + const timeout = setTimeout(() => { + fetchRetryCountRef.current += 1; + setIsRetryPending(false); + refreshMetadata(); + }, METADATA_FETCH_RETRY_DELAY_MS); + return () => clearTimeout(timeout); + }, [isRetryPending, refreshMetadata]); // Recent project IDs from the service — reactive, updates when user opens projects const [rawRecentIds, , isRecentIdsLoading] = useData( @@ -165,11 +193,23 @@ export function useProjectPickerData(): ProjectPickerData { counter: metadataRefreshCounter, promise: projectLookupService.getMetadataForAllProjects(PICKER_METADATA_FILTER), }; - // Invalidate this generation if its fetch rejects, but only if it's still the current entry so - // a newer generation isn't clobbered. - entry.promise.catch(() => { - if (metadataFetchRef.current === entry) metadataFetchRef.current = undefined; - }); + // On success: reset the retry counter so a future transient failure gets its own retry budget. + // On failure (caught at the end of the chain): invalidate this generation's cache entry (so the + // next getAllMetadata call issues a fresh fetch rather than awaiting the poisoned promise), then + // schedule a retry if the budget allows. + entry.promise + .then(() => { + fetchRetryCountRef.current = 0; + return undefined; + }) + .catch(() => { + if (metadataFetchRef.current === entry) { + metadataFetchRef.current = undefined; + if (fetchRetryCountRef.current < MAX_METADATA_FETCH_RETRIES) { + setIsRetryPending(true); + } + } + }); metadataFetchRef.current = entry; return entry.promise; }, [metadataRefreshCounter]); From dd109c8e359953f528fbdd51324c9bddb6315d99 Mon Sep 17 00:00:00 2001 From: Katherine Jensen Date: Thu, 30 Jul 2026 13:41:48 -0700 Subject: [PATCH 3/6] fix(PT-4299): harden retry/debounce logic after code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four correctness issues found during code review, all fixed together: 1. **Stale-generation guard on success** — the `.then()` success handler now checks `metadataFetchRef.current === entry` before resetting the retry budget, so a slow in-flight Gen A that resolves after Gen B+ has exhausted its cap can no longer silently re-open the retry window. 2. **isRetryPending not cleared on success** — the success handler now calls `setIsRetryPending(false)`, which triggers the retry-timer `useEffect` cleanup (`clearTimeout`). Previously, if a concurrent PDPF-triggered refresh healed the list while the 5-second retry timer was still running, the timer would fire anyway and issue a spurious redundant fan-out. 3. **Structural `.then().catch()` trap** — the `.then(cb).catch(err)` chain was replaced with `.then(onFulfilled, onRejected).catch(() => undefined)`. In the chained form the `.catch()` also sees exceptions thrown by the success handler; the two-argument form keeps the handlers independent. 4. **N rapid PDPF registrations → N fan-outs** — each `onDidCreateNetworkObject` event previously called `refreshMetadata()` directly. A 200 ms debounce timer (`pdpfRefreshTimerRef`) now collapses bursts (e.g. after extension reload) into a single `getMetadataForAllProjects` call. All four fixes are covered by new tests (23 pass). Lint and typecheck clean. Co-authored-by: Claude Sonnet 4.6 --- .../use-project-picker-data.hook.test.ts | 259 +++++++++++++++--- .../hooks/use-project-picker-data.hook.ts | 58 ++-- 2 files changed, 269 insertions(+), 48 deletions(-) diff --git a/src/renderer/hooks/use-project-picker-data.hook.test.ts b/src/renderer/hooks/use-project-picker-data.hook.test.ts index 1e75d2a378d..0d855d717b7 100644 --- a/src/renderer/hooks/use-project-picker-data.hook.test.ts +++ b/src/renderer/hooks/use-project-picker-data.hook.test.ts @@ -597,42 +597,52 @@ describe('useProjectPickerData', () => { // later registers, the network object service emits object:onDidCreateNetworkObject. // The hook must subscribe to that event and refresh when the new object is a pdpFactory, // so the project list heals without waiting for an unrelated extension reload or - // project-list change event. - const { getNetworkEvent, projectLookupService } = await importMocks(); - let pdpfRegistrationCallback: ((details: { objectType: string }) => void) | undefined; - vi.mocked(getNetworkEvent).mockImplementation( - (eventName: string) => - vi.fn((cb: (details: { objectType: string }) => void) => { - if (eventName === 'object:onDidCreateNetworkObject') pdpfRegistrationCallback = cb; - return vi.fn(); - }) as never, - ); - - // First fetch: grace window expired, USJ-providing layering PDPF not yet registered. - vi.mocked(projectLookupService.getMetadataForAllProjects) - .mockResolvedValueOnce([] as never) - // Second fetch after PDPF registers: project list now available. - .mockResolvedValue( - metadataList([ - { id: 'p1', fullName: 'Full p1', name: 'Short p1', isEditable: true }, - ]) as never, + // project-list change event. The refresh fires after PDPF_REGISTRATION_DEBOUNCE_MS so + // that a burst of registrations collapses to a single fan-out; fake timers advance past it. + vi.useFakeTimers(); + try { + const { getNetworkEvent, projectLookupService } = await importMocks(); + let pdpfRegistrationCallback: ((details: { objectType: string }) => void) | undefined; + vi.mocked(getNetworkEvent).mockImplementation( + (eventName: string) => + vi.fn((cb: (details: { objectType: string }) => void) => { + if (eventName === 'object:onDidCreateNetworkObject') pdpfRegistrationCallback = cb; + return vi.fn(); + }) as never, ); - const { result } = renderHook(() => useProjectPickerData()); - await settle(result); - // Initial state: empty because the layering PDPF providing USJ_Chapter has not - // registered yet (grace window already expired). - expect(result.current.allProjects).toHaveLength(0); + // First fetch: grace window expired, USJ-providing layering PDPF not yet registered. + vi.mocked(projectLookupService.getMetadataForAllProjects) + .mockResolvedValueOnce([] as never) + // Second fetch after PDPF registers: project list now available. + .mockResolvedValue( + metadataList([ + { id: 'p1', fullName: 'Full p1', name: 'Short p1', isEditable: true }, + ]) as never, + ); - // The USJ-providing layering PDPF registers after the grace window. - expect(pdpfRegistrationCallback).toBeDefined(); - act(() => pdpfRegistrationCallback!({ objectType: 'pdpFactory' })); + const { result } = renderHook(() => useProjectPickerData()); + await settle(result); + // Initial state: empty because the layering PDPF providing USJ_Chapter has not + // registered yet (grace window already expired). + expect(result.current.allProjects).toHaveLength(0); - await settle(result); - // The hook must detect the PDPF registration and re-fetch metadata so the list heals. - expect(projectLookupService.getMetadataForAllProjects).toHaveBeenCalledTimes(2); - expect(result.current.allProjects).toHaveLength(1); - expect(result.current.allProjects[0].id).toBe('p1'); + // The USJ-providing layering PDPF registers after the grace window. + expect(pdpfRegistrationCallback).toBeDefined(); + act(() => pdpfRegistrationCallback!({ objectType: 'pdpFactory' })); + + // Advance past the debounce delay so the buffered refresh fires. + await act(async () => { + await vi.advanceTimersByTimeAsync(250); + }); + await settle(result); + // The hook must detect the PDPF registration and re-fetch metadata so the list heals. + expect(projectLookupService.getMetadataForAllProjects).toHaveBeenCalledTimes(2); + expect(result.current.allProjects).toHaveLength(1); + expect(result.current.allProjects[0].id).toBe('p1'); + } finally { + vi.useRealTimers(); + } }); it('heals allProjects after a timed-out metadata fetch once the retry resolves (PT-4299 timeout recovery)', async () => { @@ -748,5 +758,190 @@ describe('useProjectPickerData', () => { // Must NOT re-fetch: only PDPF registrations should invalidate the metadata cache. expect(projectLookupService.getMetadataForAllProjects).toHaveBeenCalledTimes(1); }); + + it('cancels the pending retry timer when a concurrent refresh heals the list (PT-4299 spurious Fetch C)', async () => { + // Reproduces the race where Fetch A fails (isRetryPending=true, 5-second timer scheduled), + // then an external PDPF registration fires while the timer is still running. Fetch B (triggered + // by the PDPF) succeeds and heals the list. The timer must be cancelled so it does not fire a + // spurious Fetch C after healing — the fix is setIsRetryPending(false) in the success path of + // getAllMetadata, which triggers the useEffect cleanup that calls clearTimeout. + vi.useFakeTimers(); + try { + const { projectLookupService, getNetworkEvent } = await importMocks(); + let pdpfCallback: ((d: { objectType: string }) => void) | undefined; + vi.mocked(getNetworkEvent).mockImplementation( + (eventName: string) => + vi.fn((cb: (d: { objectType: string }) => void) => { + if (eventName === 'object:onDidCreateNetworkObject') pdpfCallback = cb; + return vi.fn(); + }) as never, + ); + vi.mocked(projectLookupService.getMetadataForAllProjects) + .mockRejectedValueOnce(new Error('JSON-RPC timed out')) // Fetch A fails + .mockResolvedValue( + metadataList([ + { id: 'p1', fullName: 'Full p1', name: 'Short p1', isEditable: true }, + ]) as never, + ); // Fetch B+ succeeds + + const { result } = renderHook(() => useProjectPickerData()); + await settle(result); + // Fetch A failed; isRetryPending=true, 5-second timer scheduled. + expect(vi.mocked(projectLookupService.getMetadataForAllProjects)).toHaveBeenCalledTimes(1); + expect(result.current.allProjects).toHaveLength(0); + + // PDPF registers → schedules debounced Fetch B. Advance past the debounce delay to fire it. + expect(pdpfCallback).toBeDefined(); + act(() => pdpfCallback!({ objectType: 'pdpFactory' })); + await act(async () => { + await vi.advanceTimersByTimeAsync(250); + }); + await settle(result); + // Fetch B healed the project list. + expect(vi.mocked(projectLookupService.getMetadataForAllProjects)).toHaveBeenCalledTimes(2); + expect(result.current.allProjects).toHaveLength(1); + + // Advance remaining timers (retry timer from Fetch A should have been cancelled by Fetch B's + // success path setting isRetryPending=false). + await act(async () => { + await vi.runAllTimersAsync(); + }); + await settle(result); + + // No spurious Fetch C: the list healed in Fetch B; no unnecessary fan-out after. + expect(vi.mocked(projectLookupService.getMetadataForAllProjects)).toHaveBeenCalledTimes(2); + } finally { + vi.useRealTimers(); + } + }); + + it('stale-generation success does not reset the retry budget for a newer failing generation (PT-4299 cap integrity)', async () => { + // Reproduces the race where a slow in-flight Gen A promise (up to 30 s in the service's + // startup retry loop) resolves after a newer Gen B+ has already exhausted part of its retry + // budget. Gen A's unguarded .then() must NOT reset fetchRetryCountRef.current, or subsequent + // Gen B+ failures get a fresh budget and can exceed MAX_METADATA_FETCH_RETRIES total retries. + vi.useFakeTimers(); + try { + const { projectLookupService, getNetworkEvent } = await importMocks(); + + // Gen A: manually controlled — we will resolve it late, after Gen B+ has been retrying. + let resolveGenA: (v: never) => void = () => {}; + const genAPromise = new Promise((res) => { + resolveGenA = res; + }); + + let pdpfCallback: ((d: { objectType: string }) => void) | undefined; + vi.mocked(getNetworkEvent).mockImplementation( + (eventName: string) => + vi.fn((cb: (d: { objectType: string }) => void) => { + if (eventName === 'object:onDidCreateNetworkObject') pdpfCallback = cb; + return vi.fn(); + }) as never, + ); + + vi.mocked(projectLookupService.getMetadataForAllProjects) + .mockReturnValueOnce(genAPromise as never) // Gen A: in-flight, will resolve late + .mockRejectedValue(new Error('Timeout')); // All subsequent generations fail + + const { result } = renderHook(() => useProjectPickerData()); + await settle(result); + // Gen A is in-flight. + expect(vi.mocked(projectLookupService.getMetadataForAllProjects)).toHaveBeenCalledTimes(1); + + // PDPF registers → schedules debounced Gen B. Advance past the debounce delay to fire it. + expect(pdpfCallback).toBeDefined(); + act(() => pdpfCallback!({ objectType: 'pdpFactory' })); + await act(async () => { + await vi.advanceTimersByTimeAsync(250); + }); + await settle(result); + // Gen B started and failed; isRetryPending=true. + expect(vi.mocked(projectLookupService.getMetadataForAllProjects)).toHaveBeenCalledTimes(2); + + // Advance through 2 retries (Gen C, Gen D) so the counter is partway through the budget. + for (let i = 0; i < 2; i += 1) { + // Sequential: advance timer then drain before the next retry cycle. + // eslint-disable-next-line no-await-in-loop + await act(async () => { + await vi.runAllTimersAsync(); + }); + // eslint-disable-next-line no-await-in-loop + await settle(result); + } + // Calls so far: GenA(1) + GenB(1) + retry1(1) + retry2(1) = 4 + expect(vi.mocked(projectLookupService.getMetadataForAllProjects)).toHaveBeenCalledTimes(4); + + // Gen A resolves late (stale). Without the generation guard, its .then() resets the counter + // to 0, allowing more than MAX_METADATA_FETCH_RETRIES total retries. With the fix, the guard + // sees metadataFetchRef.current !== entry-A and skips the reset. + await act(async () => { + resolveGenA([] as never); + }); + await settle(result); + + // Advance through what should be the LAST retry (retry 3 = Gen E). + await act(async () => { + await vi.runAllTimersAsync(); + }); + await settle(result); + // Calls: GenA + GenB + retry1 + retry2 + retry3(last) = 5 + expect(vi.mocked(projectLookupService.getMetadataForAllProjects)).toHaveBeenCalledTimes(5); + + // Budget exhausted — no more retries regardless of timer advances. + await act(async () => { + await vi.runAllTimersAsync(); + }); + await settle(result); + expect(vi.mocked(projectLookupService.getMetadataForAllProjects)).toHaveBeenCalledTimes(5); + } finally { + vi.useRealTimers(); + } + }); + + it('multiple rapid PDPF registrations trigger only one getMetadataForAllProjects fan-out (PT-4299 debounce)', async () => { + // When PDPFs re-register after extension reload they arrive as separate WebSocket messages, + // so React cannot batch the resulting refreshMetadata() calls. Each would normally fan out + // getMetadataForAllProjects to every registered PDPF; the fix debounces the PDPF-registration + // listener so a burst collapses to a single re-fetch. + vi.useFakeTimers(); + try { + const { projectLookupService, getNetworkEvent } = await importMocks(); + let pdpfCallback: ((d: { objectType: string }) => void) | undefined; + vi.mocked(getNetworkEvent).mockImplementation( + (eventName: string) => + vi.fn((cb: (d: { objectType: string }) => void) => { + if (eventName === 'object:onDidCreateNetworkObject') pdpfCallback = cb; + return vi.fn(); + }) as never, + ); + vi.mocked(projectLookupService.getMetadataForAllProjects).mockResolvedValue([] as never); + + const { result } = renderHook(() => useProjectPickerData()); + await settle(result); + expect(vi.mocked(projectLookupService.getMetadataForAllProjects)).toHaveBeenCalledTimes(1); + + // Fire 3 PDPF registrations synchronously within one act. Each callback cancels the + // previous debounce timer and schedules a new one; only the last timer survives. + // Using a single act ensures no timer fires between callbacks (async act can advance + // fake timers between awaits, which would defeat the debounce). + expect(pdpfCallback).toBeDefined(); + act(() => { + pdpfCallback!({ objectType: 'pdpFactory' }); + pdpfCallback!({ objectType: 'pdpFactory' }); + pdpfCallback!({ objectType: 'pdpFactory' }); + }); + + // Advance timers to fire the single surviving debounced refresh. + await act(async () => { + await vi.runAllTimersAsync(); + }); + await settle(result); + + // With debouncing: initial(1) + debounced-refresh(1) = 2 total calls, not 4. + expect(vi.mocked(projectLookupService.getMetadataForAllProjects)).toHaveBeenCalledTimes(2); + } finally { + vi.useRealTimers(); + } + }); }); /* eslint-enable no-type-assertion/no-type-assertion */ diff --git a/src/renderer/hooks/use-project-picker-data.hook.ts b/src/renderer/hooks/use-project-picker-data.hook.ts index 7944a3ee16d..8c338e4f9b0 100644 --- a/src/renderer/hooks/use-project-picker-data.hook.ts +++ b/src/renderer/hooks/use-project-picker-data.hook.ts @@ -38,6 +38,8 @@ const METADATA_FETCH_RETRY_DELAY_MS = 5 * 1000; * reload or a PDPF registration). */ export const MAX_METADATA_FETCH_RETRIES = 3; +/** Debounce window that collapses rapid-fire PDPF registrations into a single metadata fan-out. */ +const PDPF_REGISTRATION_DEBOUNCE_MS = 200; const PICKER_PROJECT_INTERFACE = 'platformScripture.USJ_Chapter'; const PICKER_METADATA_FILTER: ProjectMetadataFilterOptions = { @@ -146,13 +148,21 @@ export function useProjectPickerData(): ProjectPickerData { () => getNetworkEvent('object:onDidCreateNetworkObject'), [], ); + // Debounce to collapse N rapid-fire PDPF registrations (e.g. after extension reload) into a + // single getMetadataForAllProjects fan-out. Without debouncing, each registration arrives as a + // separate WebSocket message that React 18 cannot batch, producing N wasted fan-outs. + const pdpfRefreshTimerRef = useRef | undefined>(undefined); const onPdpFactoryRegistered = useCallback( ({ objectType }: NetworkObjectDetails) => { - if (objectType === PDP_FACTORY_OBJECT_TYPE) refreshMetadata(); + if (objectType !== PDP_FACTORY_OBJECT_TYPE) return; + clearTimeout(pdpfRefreshTimerRef.current); + pdpfRefreshTimerRef.current = setTimeout(refreshMetadata, PDPF_REGISTRATION_DEBOUNCE_MS); }, [refreshMetadata], ); useEvent(onDidCreateNetworkObject, onPdpFactoryRegistered); + // Cancel any pending debounced refresh on unmount to prevent state updates after teardown. + useEffect(() => () => clearTimeout(pdpfRefreshTimerRef.current), []); // After a failed metadata fetch, wait METADATA_FETCH_RETRY_DELAY_MS before trying again. // The timer is cancelled if the hook unmounts before it fires, preventing state updates on an // unmounted component and avoiding spurious fan-outs when the user closes the picker quickly. @@ -193,23 +203,39 @@ export function useProjectPickerData(): ProjectPickerData { counter: metadataRefreshCounter, promise: projectLookupService.getMetadataForAllProjects(PICKER_METADATA_FILTER), }; - // On success: reset the retry counter so a future transient failure gets its own retry budget. - // On failure (caught at the end of the chain): invalidate this generation's cache entry (so the - // next getAllMetadata call issues a fresh fetch rather than awaiting the poisoned promise), then - // schedule a retry if the budget allows. + // Attach success and failure handlers as the two arguments of .then() rather than as a + // .then().catch() chain. In the chained form, the .catch() sees both fetch rejections AND + // exceptions thrown by the success handler — silently mis-routing a future handler bug as a + // fetch failure. The two-argument form keeps the handlers independent: an exception in + // onFulfilled does not reach onRejected. The trailing .catch(() => undefined) satisfies the + // ESLint promise/catch-or-return rule (the plugin does not recognise the two-argument form + // as sufficient) and catches any exception from either handler; neither handler can throw in + // practice (ref reads + setState), so this is purely a safety net. entry.promise - .then(() => { - fetchRetryCountRef.current = 0; - return undefined; - }) - .catch(() => { - if (metadataFetchRef.current === entry) { - metadataFetchRef.current = undefined; - if (fetchRetryCountRef.current < MAX_METADATA_FETCH_RETRIES) { - setIsRetryPending(true); + .then( + () => { + // Guard: only reset retry state for the current generation so a stale in-flight promise + // that resolves late cannot corrupt the retry budget or spuriously cancel the timer of + // the generation currently in the cache. + if (metadataFetchRef.current === entry) { + fetchRetryCountRef.current = 0; + // Cancels any pending retry timer: setIsRetryPending(false) triggers the useEffect + // cleanup (clearTimeout) so the timer does not fire a redundant fan-out after healing. + setIsRetryPending(false); } - } - }); + return undefined; + }, + () => { + if (metadataFetchRef.current === entry) { + metadataFetchRef.current = undefined; + if (fetchRetryCountRef.current < MAX_METADATA_FETCH_RETRIES) { + setIsRetryPending(true); + } + } + return undefined; + }, + ) + .catch(() => undefined); metadataFetchRef.current = entry; return entry.promise; }, [metadataRefreshCounter]); From 8b8b6d019e5a81842fa1603a7a01fa2514b8e37e Mon Sep 17 00:00:00 2001 From: Katherine Jensen Date: Thu, 30 Jul 2026 14:32:20 -0700 Subject: [PATCH 4/6] =?UTF-8?q?fix(PT-4299):=20fix=20lint=20errors=20after?= =?UTF-8?q?=20rebase=20=E2=80=94=20promise=20param=20name=20and=20eslint-d?= =?UTF-8?q?isable=20comment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename Promise constructor param `res` → `resolve` to satisfy `promise/param-names` - Add missing justification comment above second `no-await-in-loop` disable in the stale-generation cap-integrity test loop Co-Authored-By: Claude Sonnet 4.6 --- src/renderer/hooks/use-project-picker-data.hook.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/renderer/hooks/use-project-picker-data.hook.test.ts b/src/renderer/hooks/use-project-picker-data.hook.test.ts index 0d855d717b7..89b0a4ec127 100644 --- a/src/renderer/hooks/use-project-picker-data.hook.test.ts +++ b/src/renderer/hooks/use-project-picker-data.hook.test.ts @@ -826,8 +826,8 @@ describe('useProjectPickerData', () => { // Gen A: manually controlled — we will resolve it late, after Gen B+ has been retrying. let resolveGenA: (v: never) => void = () => {}; - const genAPromise = new Promise((res) => { - resolveGenA = res; + const genAPromise = new Promise((resolve) => { + resolveGenA = resolve; }); let pdpfCallback: ((d: { objectType: string }) => void) | undefined; @@ -865,6 +865,7 @@ describe('useProjectPickerData', () => { await act(async () => { await vi.runAllTimersAsync(); }); + // Same reason as above: sequential drain after each timer advance. // eslint-disable-next-line no-await-in-loop await settle(result); } From 87da1d68b65c63848cf245f0ce169f994f666499 Mon Sep 17 00:00:00 2001 From: Katherine Jensen Date: Thu, 30 Jul 2026 16:15:42 -0700 Subject: [PATCH 5/6] fix: restore orphaned JSDoc to PICKER_PROJECT_INTERFACE The JSDoc describing which projectInterface a project must support was separated from PICKER_PROJECT_INTERFACE by the three retry/debounce constants inserted above it, leaving the comment orphaned and the constant undocumented. Moved the JSDoc to sit immediately above the constant it describes. Co-Authored-By: Claude Sonnet 4.6 Session-URL: --- .../hooks/use-project-picker-data.hook.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/renderer/hooks/use-project-picker-data.hook.ts b/src/renderer/hooks/use-project-picker-data.hook.ts index 8c338e4f9b0..7f5d82ba209 100644 --- a/src/renderer/hooks/use-project-picker-data.hook.ts +++ b/src/renderer/hooks/use-project-picker-data.hook.ts @@ -21,15 +21,6 @@ import { logger } from '@shared/services/logger.service'; import { findFirstEditorWebViewDefinition } from '@shared/models/web-view.model'; import { type ProjectItem } from '@renderer/components/projects/project-picker.component'; -/** - * `projectInterface` a project must support to belong in the picker: it can be opened in the - * scripture editor. This filter is applied service-side so the service's retry-until-non-empty - * startup grace period keeps retrying until a factory that provides this interface has registered - * (a bare unfiltered fetch settles as soon as any project - possibly a non-scripture one - appears, - * before the layering PDPF that provides this interface registers). Published resources also carry - * this interface via the Scripture Extender layering PDPF, so the current project and recent - * projects (both always scripture or resource projects) resolve from the same filtered fetch. - */ /** How long to wait before retrying a failed metadata fetch. */ const METADATA_FETCH_RETRY_DELAY_MS = 5 * 1000; /** @@ -41,6 +32,15 @@ export const MAX_METADATA_FETCH_RETRIES = 3; /** Debounce window that collapses rapid-fire PDPF registrations into a single metadata fan-out. */ const PDPF_REGISTRATION_DEBOUNCE_MS = 200; +/** + * `projectInterface` a project must support to belong in the picker: it can be opened in the + * scripture editor. This filter is applied service-side so the service's retry-until-non-empty + * startup grace period keeps retrying until a factory that provides this interface has registered + * (a bare unfiltered fetch settles as soon as any project - possibly a non-scripture one - appears, + * before the layering PDPF that provides this interface registers). Published resources also carry + * this interface via the Scripture Extender layering PDPF, so the current project and recent + * projects (both always scripture or resource projects) resolve from the same filtered fetch. + */ const PICKER_PROJECT_INTERFACE = 'platformScripture.USJ_Chapter'; const PICKER_METADATA_FILTER: ProjectMetadataFilterOptions = { includeProjectInterfaces: [PICKER_PROJECT_INTERFACE], From 1aa86767c377670960c2c665d68d53bf84786e4d Mon Sep 17 00:00:00 2001 From: Katherine Jensen Date: Fri, 31 Jul 2026 08:54:56 -0700 Subject: [PATCH 6/6] docs(PT-4299): make comments forward-facing per review feedback Remove backward-facing PT-4299 ticket references from code comments and test names in the project-picker healing hook, per Jolie's review. The descriptions of behavior and rationale remain; only the development-history ticket tags were stripped. Co-authored-by: Claude --- .../hooks/use-project-picker-data.hook.test.ts | 12 ++++++------ src/renderer/hooks/use-project-picker-data.hook.ts | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/renderer/hooks/use-project-picker-data.hook.test.ts b/src/renderer/hooks/use-project-picker-data.hook.test.ts index 89b0a4ec127..59bf5ba9ab8 100644 --- a/src/renderer/hooks/use-project-picker-data.hook.test.ts +++ b/src/renderer/hooks/use-project-picker-data.hook.test.ts @@ -589,8 +589,8 @@ describe('useProjectPickerData', () => { expect(projectLookupService.getMetadataForAllProjects).toHaveBeenCalledTimes(2); }); - it('re-fetches metadata when a PDP factory registers via object:onDidCreateNetworkObject (PT-4299 healing)', async () => { - // Reproduces the PT-4299 race: the 30-second startup grace window in + it('re-fetches metadata when a PDP factory registers via object:onDidCreateNetworkObject (healing)', async () => { + // Reproduces the race: the 30-second startup grace window in // internalGetMetadataWithRetries expires before the USJ-providing layering PDPF // (Scripture Extender) registers. The initial metadata fetch returns empty because no // factory providing platformScripture.USJ_Chapter has registered yet. When the PDPF @@ -645,7 +645,7 @@ describe('useProjectPickerData', () => { } }); - it('heals allProjects after a timed-out metadata fetch once the retry resolves (PT-4299 timeout recovery)', async () => { + it('heals allProjects after a timed-out metadata fetch once the retry resolves (timeout recovery)', async () => { // Reproduces the scenario observed in console logs where getAvailableProjects times out // (JSON-RPC 30-second timeout) because the extension host is overloaded at startup. The // late response is discarded ("Ignoring subsequent resolution"), leaving the picker empty. @@ -759,7 +759,7 @@ describe('useProjectPickerData', () => { expect(projectLookupService.getMetadataForAllProjects).toHaveBeenCalledTimes(1); }); - it('cancels the pending retry timer when a concurrent refresh heals the list (PT-4299 spurious Fetch C)', async () => { + it('cancels the pending retry timer when a concurrent refresh heals the list (spurious Fetch C)', async () => { // Reproduces the race where Fetch A fails (isRetryPending=true, 5-second timer scheduled), // then an external PDPF registration fires while the timer is still running. Fetch B (triggered // by the PDPF) succeeds and heals the list. The timer must be cancelled so it does not fire a @@ -815,7 +815,7 @@ describe('useProjectPickerData', () => { } }); - it('stale-generation success does not reset the retry budget for a newer failing generation (PT-4299 cap integrity)', async () => { + it('stale-generation success does not reset the retry budget for a newer failing generation (cap integrity)', async () => { // Reproduces the race where a slow in-flight Gen A promise (up to 30 s in the service's // startup retry loop) resolves after a newer Gen B+ has already exhausted part of its retry // budget. Gen A's unguarded .then() must NOT reset fetchRetryCountRef.current, or subsequent @@ -899,7 +899,7 @@ describe('useProjectPickerData', () => { } }); - it('multiple rapid PDPF registrations trigger only one getMetadataForAllProjects fan-out (PT-4299 debounce)', async () => { + it('multiple rapid PDPF registrations trigger only one getMetadataForAllProjects fan-out (debounce)', async () => { // When PDPFs re-register after extension reload they arrive as separate WebSocket messages, // so React cannot batch the resulting refreshMetadata() calls. Each would normally fan out // getMetadataForAllProjects to every registered PDPF; the fix debounces the PDPF-registration diff --git a/src/renderer/hooks/use-project-picker-data.hook.ts b/src/renderer/hooks/use-project-picker-data.hook.ts index 7f5d82ba209..febdee731b4 100644 --- a/src/renderer/hooks/use-project-picker-data.hook.ts +++ b/src/renderer/hooks/use-project-picker-data.hook.ts @@ -138,8 +138,8 @@ export function useProjectPickerData(): ProjectPickerData { // three sections refetch. const onDidChangeProjects = useMemo(() => getNetworkEvent('platform.onDidChangeProjects'), []); useEvent(onDidChangeProjects, refreshMetadata); - // Heal the project list when a PDP factory registers after the startup grace window expires - // (PT-4299). internalGetMetadataWithRetries only retries within 30 s of process start; if the + // Heal the project list when a PDP factory registers after the startup grace window expires. + // internalGetMetadataWithRetries only retries within 30 s of process start; if the // USJ-providing layering PDPF (Scripture Extender) arrives after that window AND neither // onDidReloadExtensions nor onDidChangeProjects fires, the picker stays empty until the user // triggers an unrelated refresh. Subscribing here ensures a late-arriving PDPF always heals