Skip to content

PT-4299: heal project picker when layering PDPF registers after grace window - #2623

Merged
katherinejensen00 merged 6 commits into
mainfrom
pt-4299-project-picker-empty-race
Jul 31, 2026
Merged

PT-4299: heal project picker when layering PDPF registers after grace window#2623
katherinejensen00 merged 6 commits into
mainfrom
pt-4299-project-picker-empty-race

Conversation

@katherinejensen00

@katherinejensen00 katherinejensen00 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Summary

  • PT-4299: Fixes the intermittent empty project picker after first-run. The setup wizard burns through the service's 30-second PDPF grace window, leaving the picker empty with no recovery path. Two new mechanisms in useProjectPickerData heal it: a debounced onDidCreateNetworkObject subscription (handles late factory registration) and a 5-second retry with a 3-failure cap (handles RPC timeout during startup congestion).
  • First-run wizard steps: Adds sync consent (PT-4178) and sync progress (PT-4179) steps to the first-run wizard, and completes the shell/stepper navigation scaffolding (PT-4257).
  • New platform components: WizardStepper and DestructiveKeyConfirmation added to platform-bible-react. Keyboard-name utility (getLocalizeKeyForPhysicalKey) added to platform-bible-utils.
  • Tab icon fix: Selected Simple-mode tabs no longer show an invisible white icon on light backgrounds.

Details

PT-4299 — Empty project picker after first-run wizard

Root cause: The service retries getMetadataForAllProjects for 30 seconds after startup, waiting for the Scripture Extender PDPF (which provides the USJ_Chapter interface the picker filters by) to register. The first-run wizard is long enough to exhaust that window. When the user finally opens the picker, the grace period is over, and there is no signal to trigger a recovery fetch.

Two failure modes, two fixes:

Failure mode Mechanism Key constants
RPC timeout (extension host overloaded) 5 s retry, max 3 attempts METADATA_FETCH_RETRY_DELAY_MS, MAX_METADATA_FETCH_RETRIES
Successful-but-stale empty fetch (PDPF registered late) Subscribe onDidCreateNetworkObject, debounce 200 ms, re-fetch PDPF_REGISTRATION_DEBOUNCE_MS

Key file: src/renderer/hooks/use-project-picker-data.hook.ts
Tests: src/renderer/hooks/use-project-picker-data.hook.test.ts (23 tests, 7 new)


First-run wizard

Commit What it adds
PT-4178 Sync consent step — asks user to consent to background data sync
PT-4179 Sync progress step — shows sync progress during first run
PT-4257 Wizard stepper + shell navigation — routes between steps, tracks completion state

The auto-sync blocking service (src/renderer/services/auto-sync-blocking-service.ts) gates automatic sync during the wizard. first-run-store.ts / first-run.model.ts hold the wizard state.


Platform components and utilities

Addition Location Purpose
WizardStepper lib/platform-bible-react/…/wizard-stepper/ Step indicator used by the first-run wizard
DestructiveKeyConfirmation lib/platform-bible-react/…/destructive-key-confirmation* Confirm-before-delete UX (used by the two-step verse-number delete tooltip)
getLocalizeKeyForPhysicalKey, NameablePhysicalKey lib/platform-bible-utils/src/keyboard-util.ts Map physical key names to localized display strings
Tab icon fix lib/platform-bible-react/src/hooks/use-tab-icon-selection.hook.ts lightSelected (white) was invisible on plain-light tab headers; pickTabIconUrl now returns lightUnselected regardless of selection state in light theme

Infrastructure / cleanup

  • PT-4214/PT-4210: Declares the sync-block seam (getAutoSyncBlocking, onSyncWriteLockChanged) and adds a breakSyncLock stub as public API.
  • PT-4020: Improved tooltip styling for the two-step "Delete Verse Number" confirmation.
  • C# SendReceiveBlockNotifierService: New service notifying when send/receive is blocked.
  • jsdom CSS-parse fix: Suppresses CSS-parse console dumps that were hanging Linux CI unit tests (vitest.setup.ts).
  • Spanish localization docs: Decisions distilled into Localization-Guide.md.

Test plan

  • 7 new tests for PT-4299 (PDPF healing, timeout retry, retry cap, debounce collapse, spurious-Fetch-C cancellation, stale-generation integrity) — all RED before fix, GREEN after
  • All 23 use-project-picker-data.hook tests pass
  • WizardStepper and DestructiveKeyConfirmation have co-located unit tests
  • E2E first-run wizard overlay smoke test (e2e-tests/tests/isolated/first-run/)
  • TypeScript and ESLint clean on changed files (pre-existing buildInfo.json dev-env error on baseline, unrelated)

Code Review Summary

Branch: pt-4299-project-picker-empty-race

Base: origin/main

Date: 2026-07-30

Review model: Claude Sonnet 4.6

Files changed: 89

Overview

This branch fixes an intermittent empty/locked project picker that occurs when users work through the setup wizard on first run. The service has a 30-second startup grace period during which it retries getMetadataForAllProjects until the Scripture Extender PDPF (which provides the USJ_Chapter interface the picker filters by) registers. The setup wizard burns through that window — the user is clicking through onboarding steps while extensions load in the background. When they finally open the picker, the grace period has expired, Scripture Extender hasn't registered yet, and the filtered fetch returns empty with no way to recover.

Two mechanisms were added to useProjectPickerData to fix different failure modes: a debounced onDidCreateNetworkObject subscription that re-fetches when a PDPF eventually registers (heals a successful-but-stale empty fetch), and a 5-second retry with a cap of 3 attempts (heals a fetch that failed outright due to RPC timeout during startup congestion). The branch also includes the setup wizard UI work that exposed the race: first-run flow scaffolding, sync consent and sync progress steps, a WizardStepper component, a DestructiveKeyConfirmation component, keyboard utilities, C# SendReceive improvements, and e2e tests.

API Changes

  • lib/platform-bible-react: Added DestructiveKeyConfirmation (default export) and DestructiveKeyConfirmationProps type
  • lib/platform-bible-react: Added WizardStepper (default export) and WizardStepperProps type
  • lib/platform-bible-react: Modified pickTabIconUrl — for isTabSelected === true in light theme, the function now returns urls.lightUnselected rather than urls.lightSelected. The lightSelected field is retained in TabIconUrls for future hosts that give selected tabs a dark background, but is currently unused by the function. This is an intentional behavior change (commit 2d831dcd447 Fix white/invisible tab icon on selected Simple-mode tabs) — lightSelected is white, which is invisible against the plain light background every current host gives the active tab.
  • lib/platform-bible-utils: Added getLocalizeKeyForPhysicalKey() export
  • lib/platform-bible-utils: Added NameablePhysicalKey type export
  • @papi/core (via papi.d.ts): Added 'platform.syncOnStartup': boolean to SettingTypes

Findings

Critical — Must address before merge

None.

Important — Should address before merge

  • lib/platform-bible-react/src/hooks/use-tab-icon-selection.hook.tspickTabIconUrl behavior change for isTabSelected === true: previously returned urls.lightSelected, now returns urls.lightUnselected. Three internal callers supply a lightSelected URL that is now dead. (Intentional: commit 2d831dcd447 explicitly fixed invisible white icons on selected tabs in light theme. Every current host keeps the active tab header on a plain light background, making lightSelected (white) invisible. Change is documented in JSDoc and covered by tests. Author noted external extension developers are unlikely to have been affected during this development window.)
  • src/renderer/hooks/use-project-picker-data.hook.ts:24–32 — JSDoc for PICKER_PROJECT_INTERFACE was orphaned: the three new retry/debounce constants were inserted between the JSDoc and the constant it documents, so the JSDoc no longer attached to anything and PICKER_PROJECT_INTERFACE was left undocumented. (Fixed during review: moved the JSDoc block to sit immediately above PICKER_PROJECT_INTERFACE.)

Minor — Consider

  • src/renderer/hooks/use-project-picker-data.hook.ts:40MAX_METADATA_FETCH_RETRIES is exported from a renderer-internal hook file whose only consumer is the co-located test file. Could be unexported with a test-local constant instead. (Author chose to leave the export as-is — it makes the retry-cap test self-documenting without a magic number.)

Template Propagation

Shared Regions Modified

None.

Extension Config Changes

None.

Positive Observations

  • Comprehensive test coverage for the race fix. Seven new tests cover PDPF-registration healing, RPC-timeout recovery, retry cap, debounce collapse, spurious-Fetch-C cancellation, and stale-generation cap integrity — each named after the exact race scenario it defends and with a comment explaining the production race. Tests use fake timers and are fully deterministic.
  • Two-argument .then(onFulfilled, onRejected) form. Replaces the .then().catch() chain to keep the handlers structurally independent — an exception in the success handler cannot be silently mis-routed to the rejection handler. The inline comment explaining why is excellent.
  • Generation guard on both promise paths. The if (metadataFetchRef.current === entry) check in both the success and failure handlers prevents a slow in-flight promise from corrupting the retry budget or cancelling a newer generation's timer after the fact.
  • Clean separation of refresh signals. Metadata cache invalidation (PDPF registration, extension reload, project list change) is decoupled from active-editor re-derivation (web view open/update/close), keeping the expensive getMetadataForAllProjects fan-out isolated.
  • Proper cleanup on unmount. Both useEffect cleanups call clearTimeout, preventing state updates after teardown for both the retry timer and the debounce timer.
  • settle() helper pattern. Replaces waitFor with a deterministic microtask drain, avoiding flaky CI timing dependencies.
  • New components have tests and accessibility consideration. DestructiveKeyConfirmation includes a role="status" live region for screen readers. WizardStepper has a co-located test and Storybook story.
  • papi.d.ts is properly regenerated, not hand-edited.

Interview Notes

Stated purpose: Fix the intermittent empty/locked project picker that occurs specifically after users run through the setup wizard on first startup.

Author's design explanation: The setup wizard takes time — users click through onboarding steps while extensions load in the background. The service has a 30-second grace period after startup where it keeps retrying until the Scripture Extender PDPF (which provides the USJ_Chapter interface the picker filters by) registers. The wizard burns through that window before the user even opens the picker. When they finally do, the grace period is expired, Scripture Extender hasn't registered yet, and the filtered fetch returns empty with no way to recover.

The two mechanisms fix different flavors of that:

  • PDPF subscription: when Scripture Extender eventually registers, the hook sees the onDidCreateNetworkObject event, waits 200 ms to batch any burst of registrations, then re-fetches. Late factory → healed list. (Fixes a successful-but-stale empty fetch.)
  • Retry: handles the case where the RPC itself fails during startup congestion — the extension host queue is full, the call times out, and rather than staying empty forever the hook waits 5 seconds and tries again (up to 3 times). (Fixes a failed fetch.)

On the pickTabIconUrl finding: author initially thought it was a bad rebase, but after reviewing commit 2d831dcd447 and the updated tests, confirmed it was an intentional fix for invisible white icons on selected tabs in light theme.

On MAX_METADATA_FETCH_RETRIES export: author chose to leave it — it makes the retry-cap test self-documenting.

In-Review Quality Check

One change was made during the review (JSDoc move in use-project-picker-data.hook.ts). Quality check results:

  • typecheck: pre-existing failure (Cannot find module '../../../release/app/buildInfo.json' in src/main/services/app.service-host.ts) — build artifact not present in the worktree; reproduced identically on the base commit. Unrelated to this PR.
  • lint: same pre-existing buildInfo.json resolution failure. Unrelated.
  • prettier: clean — no formatting changes needed.
  • tests (23/23): all pass.

Suggested Review Focus

  • The two-mechanism design: why both PDPF subscription and retry were needed — they fix distinct failure modes (stale-successful vs. failed RPC). Worth confirming the reviewer understands the distinction so they can evaluate whether any third failure mode was missed.
  • Debounce timing (200 ms): was this value empirically determined or estimated? A burst of 3+ PDPF registrations needs to all land within the window for the debounce to collapse them.
  • Retry parameters (3 attempts, 5 s delay): are these calibrated against observed startup congestion durations, or conservative defaults? The reviewer may want to know if there's data behind them.

AI-assisted — session


This change is Reviewable

katherinejensen00 and others added 4 commits July 30, 2026 14:05
… window

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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…slint-disable comment

- 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 <noreply@anthropic.com>
@katherinejensen00
katherinejensen00 force-pushed the pt-4299-project-picker-empty-race branch from 918a7d0 to 8b8b6d0 Compare July 30, 2026 21:40
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 <noreply@anthropic.com>
Session-URL: <session URL>

@jolierabideau jolierabideau left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great Katherine! Very well commented and tested. Approving but left one comment about comments.

@jolierabideau reviewed 2 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on katherinejensen00).


src/renderer/hooks/use-project-picker-data.hook.ts line 142 at r1 (raw file):

  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

(I will get my PR for this in asap) Can we write forward-facing comments here and remove code reference

Prefer Forward-Facing Comments Over Backward-Facing Ones
A code comment earns its place only if it helps someone who reads or changes the code later and never saw this PR. Before keeping any comment you add or change, apply one test:
Strip the PR/development context — does the comment still help? If it only explains how the code reached its current state during this PR, cut it. If it points forward — to open work, a constraint, or rationale that can't be inlined — keep it.
Backward-facing — cut these
These record development history. They rot the moment the PR merges and mean nothing to a future reader:

Ticket/PR references for work done in this PR: PT-4214, PR #2561, review-finding IDs like
(review C61-2).
"Fixes error E1 found in a test pass", "addressed in review round 2".
Stage/epic tags: Stage U, Phase 3.
Dated development notes: found live in E2E 2026-07-16.
The right home for "this change was made because of PT-1234" is the commit message or PR body, not the source file.


src/renderer/hooks/use-project-picker-data.hook.test.ts line 593 at r1 (raw file):

  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

Same comment about forward-facing

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 <noreply@anthropic.com>

@katherinejensen00 katherinejensen00 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@katherinejensen00 made 2 comments.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on jolierabideau).


src/renderer/hooks/use-project-picker-data.hook.ts line 142 at r1 (raw file):

Previously, jolierabideau wrote…

(I will get my PR for this in asap) Can we write forward-facing comments here and remove code reference

Prefer Forward-Facing Comments Over Backward-Facing Ones
A code comment earns its place only if it helps someone who reads or changes the code later and never saw this PR. Before keeping any comment you add or change, apply one test:
Strip the PR/development context — does the comment still help? If it only explains how the code reached its current state during this PR, cut it. If it points forward — to open work, a constraint, or rationale that can't be inlined — keep it.
Backward-facing — cut these
These record development history. They rot the moment the PR merges and mean nothing to a future reader:

Ticket/PR references for work done in this PR: PT-4214, PR #2561, review-finding IDs like
(review C61-2).
"Fixes error E1 found in a test pass", "addressed in review round 2".
Stage/epic tags: Stage U, Phase 3.
Dated development notes: found live in E2E 2026-07-16.
The right home for "this change was made because of PT-1234" is the commit message or PR body, not the source file.

Good points. Fixed.


src/renderer/hooks/use-project-picker-data.hook.test.ts line 593 at r1 (raw file):

Previously, jolierabideau wrote…

Same comment about forward-facing

Fixed.

@katherinejensen00
katherinejensen00 merged commit c5dc0c6 into main Jul 31, 2026
6 of 7 checks passed
@katherinejensen00
katherinejensen00 deleted the pt-4299-project-picker-empty-race branch July 31, 2026 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants