Add getCachedResources command with background fetch and periodic refresh

[captaincrazybro]

Code Review Summary

Branch: feat-cached-resources

Base: origin/main

Date: 2026-05-14

Review model: Claude Sonnet 4.6

Files changed: 11

Overview

This branch adds a getCachedResources command to the platform-get-resources extension. On activation, the extension kicks off a background fetch (up to 10 retries, 1 second apart) and caches the result in both memory and persistent user storage. The new command returns the cache immediately if populated; if the cache is empty it performs an on-demand fetch. A 12-hour setInterval also fires fetchAndCacheResources periodically to keep the cache fresh. The Get Resources and Home web views were migrated from a live useData subscription to a one-time usePromise fetch using the new command.

Several correctness issues were found and fixed during the review: the error-feedback path was non-functional (dead isPlatformError branch plus unused localization strings for the planned retry UI), both web views had stale useData imports, the background-fetch guard had a TOCTOU window, and executionToken used a non-null assertion. Minor issues fixed include deduplicating the fetch-and-cache sequence into a fetchAndCacheResources helper, removing an unnecessary sleep helper, simplifying async/await wrappers, and adding an inner-mutex cache check.

API Changes

• 'platformGetResources.getCachedResources': () => Promise<DblResourceData[] | undefined> — new command added to papi-shared-types.CommandHandlers
• DblResourceData imported into the papi-shared-types module declaration (needed for the new command's return type)
• No changes to lib/platform-bible-react, lib/platform-bible-utils, or lib/papi-dts/papi.d.ts

Findings

Critical — Must address before merge

None.

Important — Should address before merge

• Dead isPlatformError branch + missing error UI in get-resources.web-view.tsx (fixed during review: replaced isPlatformError(resources) — which can never be true after the usePromise migration — with resources === undefined && !isLoadingResources; wired up %resources_fetch_failed% error label and a Retry button backed by a retryCount state variable that re-triggers usePromise)
• %resources_fetch_failed% and %resources_retry% defined in localizedStrings.json but never referenced (fixed during review: both strings are now used in the new error+retry UI)
• Stale useData import in get-resources.web-view.tsx and home.web-view.tsx (fixed during review: removed useData from the @papi/frontend/react import in both files)
• TOCTOU window in startBackgroundFetchResources between isLocked() and runExclusive() (fixed during review: replaced the fetchMutex.isLocked() guard with a module-level hasFetchStarted boolean set synchronously before entering runExclusive)
• executionToken declared with non-null assertion (!) (fixed during review: changed to ExecutionToken | undefined; both writeUserData call sites guarded with if (executionToken))

[All five important findings were fixed during the interview.]

Minor — Consider

• Local sleep function duplicates wait from platform-bible-utils (fixed during review: removed sleep, replaced with wait(1000))
• Fetch-and-cache logic duplicated between startBackgroundFetchResources and getCachedResources (fixed during review: extracted fetchAndCacheResources() helper that fetches the provider internally; all callers simplified)
• Unnecessary async () => await sendCommand(...) wrappers in both web views (fixed during review: simplified to () => papi.commands.sendCommand(...))
• Redundant fetch in getCachedResources when background fetch completes first (fixed during review: added if (cachedResources !== undefined) return cachedResources inside runExclusive callback)
• useData → usePromise behavioral change: was a live subscription, now a one-time fetch on mount (intentional — author confirmed this is the desired behavior for the cached-resources approach)
• No tests for the retry loop, persistence-load-on-startup, and on-demand fetch paths (acknowledged — consistent with the pre-existing lack of tests in this extension; not blocking)

Template Propagation

Shared Regions Modified

None.

Extension Config Changes

None.

Positive Observations

• New command is well-documented in platform-get-resources.d.ts with a clear @returns JSDoc covering both the success and undefined cases
• Mutex from platform-bible-utils correctly coordinates the background, on-demand, and scheduled fetch paths, preventing duplicate in-flight network requests
• Persisting the cache with papi.storage.readUserData / writeUserData means the UI can show stale-but-fast data immediately on next launch
• Background fetch is correctly registered as fire-and-forget with appropriately scoped eslint-disable comments
• New command and refresh interval both added to context.registrations.add(...), ensuring proper cleanup on deactivation
• Module-level mutable state follows the same pattern already established in the file

Interview Notes

Purpose: Implement a getCachedResources command — returns cached DBL resources if available; on startup, tries to fetch and cache; if called with no cache, also triggers an on-demand fetch. A 12-hour interval was added during review to periodically refresh the cache.

Key decisions:

• useData (live subscription) replaced with usePromise (one-time fetch on mount) — intentional; the cached-resource model means the UI doesn't need to react to live data updates
• hasFetchStarted flag used to prevent double background fetches — eliminates the window between lock check and lock acquisition
• executionToken persistence guards are best-effort — in-memory cache still populates if the token is unavailable
• 12-hour refresh interval uses fetchAndCacheResources directly (not getCachedResources) so it always re-fetches rather than no-oping on a populated cache

Author understanding: Clear throughout. The author explained the cache/retry model without prompting and made targeted decisions for every finding.

In-Review Quality Check

Check	Result
npm run typecheck	Passed
npm run lint	Failed → auto-fixed → passed
npm run format	Passed
npm test	Passed

Lint fixes applied (all in in-review files):

1. get-resources.web-view.tsx — Added explanatory comment before react-hooks/exhaustive-deps disable (retryCount is an intentional re-fetch trigger)
2. main.ts — Inline eslint-disable-line comments converted to eslint-disable-next-line with explanation comments
3. main.ts — no-continue: retry loop continue refactored to if (provider) { ... } guard block (later removed when provider fetch moved into helper)
4. main.ts — no-type-assertion: JSON.parse(cached) as DblResourceData[] refactored to typed assignment
5. main.ts — Added explanatory comment for floating-promise eslint-disable in setInterval callback
6. main.ts — Prettier formatting applied to context.registrations.add dispose object

Suggested Review Focus

• Base branch: Branch appears to target origin/main rather than origin/ai/main per the CLAUDE.md AI branch rules — verify the PR targets the correct base before merging
• useData → usePromise behavioral change: The UI now fetches once on mount rather than subscribing to live updates. If credentials become available after mount and the cache was empty, the UI won't refresh without a user-initiated retry — confirm this is acceptable
• 12-hour refresh interval behavior: The interval fires fetchAndCacheResources unconditionally every 12 hours under the mutex. Confirm the interval duration and the decision to always re-fetch (not use getCachedResources with its cache-hit early return)
• No tests: The retry loop, startup cache load, and on-demand fetch path have no test coverage — discuss whether to add before merge or track as follow-up

---

This change is 

[tombogle]

extensions/src/platform-get-resources/contributions/localizedStrings.json line 13 at r1 (raw file):

      "%resources_dialog_subtitle%": "Add resources to Home",
      "%resources_dialog_title%": "Get Resources",
      "%resources_fetch_failed%": "Opened Get Resources, but could not fetch the resource list",

The first clause communicates nothing the user doesn’t already know from:

• the dialog title,
• the visible UI,
• and the fact that the error is appearing there.

It also reads a bit like an internal log message rather than user-facing UI text. Also, we do not use the word "fetch" anywhere else in our UI. (FWIW, it is used a few times -- including in this context -- in P9.) It sounds developer-ish to me. I’d lean toward “load” rather than “fetch” for user-facing UI. We use "load" a lot in our UI. An alternative might be "access" or "connect to".

Paratext 9 already has a pretty good string that might serve us well:
Paratext was not able to connect to the DBL server to retrieve available resources.
Paratext no pudo conectarse al servidor DBL para obtener los recursos disponibles.

Other possibilities:

• Could not load the resource list.
• Unable to connect to the DBL server to retrieve available resources.
• Unable to access the DBL resource list. Check your internet connection and try again.
• Unable to access DBL resources. Check your internet connection and try again.
• Could not connect to the DBL server to retrieve resources.
• Unable to retrieve resources from the DBL server.
• Unable to retrieve the list of resources.

[tombogle]

extensions/src/platform-get-resources/src/types/platform-get-resources.d.ts line 73 at r1 (raw file):

     * @returns Cached DBL resources, or `undefined` if unavailable (credentials not configured or
     *   fetch failed)
     */

This could be stated more clearly to remove doubt about what happens in the case of a failed re-fetch:

Code snippet:

/**
 * Returns DBL resources from memory cache.
 *
 * If no cached value exists, attempts to fetch them.
 * Failed refresh attempts do NOT clear existing cached data.
 *
 * @returns Cached DBL resources, or `undefined` if none have been
 * successfully fetched.
 */

[tombogle]

@tombogle reviewed 6 files and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on captaincrazybro).

[tombogle]

extensions/src/platform-get-resources/src/home.web-view.tsx line 57 at r1 (raw file):

  const [resourcesList] = usePromise(
    useCallback(() => papi.commands.sendCommand('platformGetResources.getCachedResources'), []),

With this approach of using cached resources, what happens in the case where the Internet is down or (when we add a setting for this*) Internet activity is disabled, and then it later comes back or is re-enabled? Presumably the user can click retry manually, but is there any way we can detect that condition automatically and try to re-fetch?

• I assume we could/should be able to know when this setting changes, so that much should at least be possible.

[tombogle]

One fundamental question I have is whether fetchAndCacheResources should be fired unconditionally every 12 hours or only if there is some UI open that is actually trying to display a list of resources. My thought is that we really only need to refresh the cache on demand if it is older than 12 hours.
how-easy-and-easy-starry-gray.md