PT-4686: Keep Text Collection resources resolvable offline - #2869
katherinejensen00 wants to merge 5 commits into
Conversation
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
@irahopkinson+AI made 9 comments.
Reviewable status: 0 of 39 files reviewed, 8 unresolved discussions (waiting on katherinejensen00).
a discussion (no related file):
I reviewed the offline catalog changes across the C# provider, platform-get-resources and the Text Collection grid, and left 9 findings: 1 medium and 8 low. Eight are inline; the ninth is below, because it is about a file this PR does not change. Each shows the severity I settled on, and each has been checked against the code at d27bacc809d. Several of the fixes are marked optional or give a documented alternative, so treat the low ones as your call.
#5 - low · checked and confirmed · extensions/src/platform-scripture-editor/src/model-text-panel.web-view.tsx:72
The Model Text panel still shows installed DBL resources as unavailable while offline, because the disk fallback was added only to the Text Collection grid.
What happens: The rule "resolve DBL references from disk when the catalog can't" lives in useDblInstallLookup, which only the grid uses. The Model Text panel calls useDblResourceCatalog() (model-text-panel.web-view.tsx:72), which still resolves only through catalog rows plus getLocalNonDblResources.
Why it matters: This change does not make the Model Text panel worse offline: where it used to read a silently empty "ready" catalog, it now gets hasCatalogError and shows the retry state it already wires up (:204-205). The gap is documented as deferred (the "known sibling gap not addressed here" line in adr-dbl-empty-catalog-is-a-failed-fetch, and "Not in this PR"), but no ticket tracks it, so the deferral is not searchable.
Fix: Do not build the fallback in this PR. File a follow-up ticket for giving useDblResourceCatalog consumers the disk-scan fallback, and change the line in adr-dbl-empty-catalog-is-a-failed-fetch from "a known sibling gap not addressed here" to "a known sibling gap, tracked in PT-XXXX" with the real key. The follow-up must use one shared hook for both the grid and the Model Text panel, with a Model Text panel test (a rejected catalog plus an installed DBL resource on disk) that fails without the fallback.
(AI-assisted, with my guidance)
c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 252 at r1 (raw file):
// exception to the front end's log. Capturing suppresses AlertCapture's own console // fallback, so the `finally` below logs every captured alert itself, on every exit path. using var alertScope = AlertCapture.StartCapture();
#2 - low · checked and confirmed
During the DBL catalog fetch, any yes/no question ParatextData raises is now answered "yes" instead of "no".
What happens: FetchResourcesCore now opens AlertCapture.StartCapture() (:252) around GetInstallableDBLResources, which is handed a DblProjectDeleter and DblMigrationOperations (:197-203). Inside a scope AlertCapture.ShowInternal returns AlertResult.Positive; with no scope, as on this path before, it printed to the console and returned Negative.
Why it matters: Only a prompt with buttons is affected, and nothing in this repo raises one on the DBL path: the alerts described here are failure notices (offline, server error, bad registration), and answering Positive under a capture scope is already the convention in the manage-books orchestrators (CopyBooksOrchestrator.cs:726, CreateBooksOrchestrator.cs:105, ImportBooksOrchestrator.cs:677). The risk is that ParatextData asks a question inside the fetch, which cannot be checked from this repo.
Fix: Before merge, confirm that InstallableDBLResource.GetInstallableDBLResources and its DblProjectDeleter / DblMigrationOperations callbacks never call Alert.Show with Yes/No or OK/Cancel buttons; if they do not, leave the capture scope as it is. If ParatextData does raise a question there, add an AlertCapture.StartCapture(AlertResult answer) overload that stores the answer on AlertScope and returns it from ShowInternal for captured prompts, pass AlertResult.Negative from FetchResourcesCore, and add an NUnit test that calls Alert.Show with AlertButtons.YesNo inside that scope (driving it through Alert.Implementation as ParatextGlobalsAlertInstallTests.cs:62 does) and asserts Negative. The test must fail with the plain StartCapture().
Other findings in this file: #3
c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 264 at r1 (raw file):
alertScope.Entries ); _resources = fetched.Where(r => DblResourceWhiteList.IsValidResource(r)).ToList();
#3 - low · checked and confirmed
When the whitelist filters out every catalog row, the backend treats the catalog as loaded while the front end treats it as failed, so the grid shows "Couldn't check" where the disk could have answered and Retry cannot fix it.
What happens: RequireFetchedCatalog checks the unfiltered list. FetchResourcesCore then assigns _resources = fetched.Where(DblResourceWhiteList.IsValidResource) (:264), which can be empty, and sets _hasFetchedResources = true (:270). resolveDblCatalog rejects that same empty list (dbl-catalog.utils.ts:44-45). With _hasFetchedResources set, the disk-scan branch (:442) is off, and ProjectInstallStatus over the empty _resources (:479) returns {}.
Why it matters: The whitelist (DblResourceWhiteList.cs:7) holds about 1,860 DBL ids, so a registered user's catalog with none of them is close to unreachable. When it happens the failure is contained: only references with no catalog row and no disk answer show "Couldn't check", always with the Retry banner, and the state clears on the next fetch that includes a whitelisted row.
Fix: In FetchResourcesCore (DblDownloadableDataProvider.cs:259-270), filter with DblResourceWhiteList.IsValidResource into a local list first. If that list is empty, throw an exception whose message says the catalog contained no compatible resources (not the unreachable-DBL message), before assigning _resources or _hasFetchedResources; assign both only after the check passes. Add a C# test beside GetDblResources_AnUnreachableFirstFetchLeavesInstallStatusAnsweringFromDisk (DblResourcesDataProviderTests.cs:560) that fetches a non-empty catalog containing only a non-whitelisted uid, asserts GetDblResources throws, then asserts RecomputeDblResourcesInstallStatus still answers from disk for an installed whitelisted uid; confirm it fails without the change. Run the existing DBL provider tests and adjust any that fetch only non-whitelisted uids and expect success.
Other findings in this file: #2
extensions/src/platform-scripture-editor/src/resource-text-panel.web-view.tsx line 221 at r1 (raw file):
// Neither an empty referenced list nor a failed catalog may hide the downloaded extras: they are // read straight off disk, so they can be correct even while the catalog fetch is down. if (listReadiness === 'empty' || listReadiness === 'catalogError') {
#1 - medium · checked and confirmed
With the DBL catalog failing, a Bible texts panel set to a DBL resource silently shows the first downloaded resource instead, and saves that as the user's choice.
What happens: With no catalog, resolveReferenced (downloaded-resources.utils.ts:141-142) drops the saved dbl:<uid> reference, while downloaded extras still come back as project: rows from downloadedToRow (:191-199). matchesSelectedResourceId (resource-selection.utils.ts:33-40) matches neither, so resolveResourceSelection falls back to firstUsable (:98-104). The override at :221-224 now turns catalogError into configured whenever any downloaded row exists, so that row renders with no error or retry, and the effect at :257-260 writes its id over selectedResourceId.
Why it matters: The overwrite of the saved selection already happened before this change, but the user saw the error view; the override is what makes the swap silent. It is reachable whenever the catalog fetch fails and at least one other resource is downloaded. When the saved resource happens to be the first usable row, the panel shows the right text, just under a project: id.
Fix: In resource-text-panel.web-view.tsx, keep the catalogError to configured override at :221 only when selectedResourceId is undefined or some row in filteredResources satisfies matchesSelectedResourceId(row, selectedResourceId); otherwise leave readiness as catalogError so the error-and-retry view shows. Do not use selection.selectedRow for this check: it falls back to rows[0] (resource-selection.utils.ts:74) and is computed after the override. In the effect at :257, skip only the fallback write (a nextSelectedResourceId produced when no row matches the saved id) while hasCatalogError is true, keeping the pending-pick commit and the legacy bare-id migration working. Add resource-text-panel.web-view.test.tsx cases: a saved dbl: selection, a failed catalog and one unrelated downloaded resource shows the error view and never calls setSelectedResourceId; a saved project: id that is downloaded still renders.
extensions/src/platform-scripture-editor/src/scripture-text-grid/use-dbl-install-lookup.hook.ts line 17 at r1 (raw file):
throw new Error('The DBL resources provider is not available to report installed resources'); // An empty map may only mean the provider is busy for a moment. return retryUntil(
#7 - low · checked and confirmed
A grid cell can stay on "Couldn't check whether this resource is installed" for a resource that is installed, because the disk check gives up after 2.5 seconds of a busy backend and never asks again on its own.
What happens: fetchInstalledDblResources retries 5 times, 500 ms apart (:17-20). RecomputeDblResourcesInstallStatus returns [] whenever _providerGate is contended (DblDownloadableDataProvider.cs:457-459). The lookup then reads as unanswered, and with hasCatalogError the cell shows unverified. The ask memo is keyed only on [isNeeded, hasCatalogSettled] (:64), so nothing re-asks until the catalog settles again, and a removal made in another panel is never picked up.
Why it matters: The window is narrow: the gate is held for seconds only by a catalog download, an install or an uninstall (DblDownloadableDataProvider.cs:389-392), and this grid asks only after its own catalog fetch settles. The user is not stuck, because the Retry banner shows with every unverified cell and its onRetry re-runs the check (scripture-text-grid.web-view.tsx:290-292), and an install re-asks through refreshCounter.
Fix: Optional hardening. In useDblInstallLookup, re-ask when platform.onDidChangeProjects fires, subscribing with useEvent as use-resource-picker-resources.hook.ts:80-85 does, so removals made in other panels are picked up. Add a hook test that fires the event and asserts a second recomputeDblResourcesInstallStatus call. If you also re-ask while the lookup is unanswered and hasCatalogError is set, wait at least 30 seconds between attempts as .claude/rules/architecture/provider-lookups-fan-out.md requires, cancel the timer on unmount, and add a fake-timer test that fails without it. Either re-ask must keep the answer.ask === ask guard at :69.
Other findings in this file: #8, #9
extensions/src/platform-scripture-editor/src/scripture-text-grid/use-dbl-install-lookup.hook.ts line 50 at r1 (raw file):
); const ask = useMemo(() => {
#9 - low · checked and confirmed
Each Retry runs the disk check twice, and offline the first run makes five calls in 2.5 seconds to a backend that is busy with the catalog fetch and can only answer "busy".
What happens: hasCatalogSettled flipping to false builds an ask (:50-64) that runs at once against the held _providerGate. A contended Monitor.TryEnter returns [] (DblDownloadableDataProvider.cs:457-459), so retryUntil spends all 5 attempts (:17-20). The flip back to true then builds a second ask.
Why it matters: Each wasted call is a cheap IPC round trip that returns immediately, so the cost is small. The early ask is also deliberate: the hook's doc comment (:27-29) promises a disk answer while the catalog is still loading, which is what lets an installed resource render on first mount offline.
Fix: Leave the first-mount ask while unsettled as it is. If the wasted calls on a Retry need trimming, skip only the ask for the settled-to-unsettled flip when the previous answer is already non-empty and current; otherwise keep the ask and accept five cheap contended calls. Any change must keep answer.ask === ask as the staleness guard, preserve the first-mount case where an unsettled catalog still gets a disk answer, and add a hook test that fails without it.
Other findings in this file: #7, #8
extensions/src/platform-scripture-editor/src/scripture-text-grid/use-dbl-install-lookup.hook.ts line 64 at r1 (raw file):
// runs, so ask again once it settles — after a Retry or an install, too. // eslint-disable-next-line react-hooks/exhaustive-deps }, [isNeeded, hasCatalogSettled]);
#8 - low · checked and confirmed
Clicking Retry turns every grid cell that was resolved from disk into a "Resource is loading…" spinner for as long as the catalog fetch takes, and offline that is the whole failing fetch.
What happens: refetch sets hasSettled false (use-retryable-promise.hook.ts:118-122). The ask memo is keyed on [isNeeded, hasCatalogSettled] (:64), so it builds a new ask, the answer.ask === ask guard (:69) discards the previous answer, and toInstallLookup returns pending (grid-resources.utils.ts:60). Every reference without a catalog row becomes checking (:144), which ResourceCell renders as a spinner, unmounting the editor (resource-cell.component.tsx:122-125).
Why it matters: The PR description accepts a brief spinner after an install, where the disk has changed and the old answer is genuinely stale. Retry is not mentioned there, and on Retry the disk has not changed, so the offline user the PR targets loses scroll position and caret for the length of a failing fetch for no reason.
Fix: Keep the current answer.ask === ask guard for any ask created by an install (refreshCounter change) or an isNeeded change. Pass refreshCounter into useDblInstallLookup (it is not passed today, scripture-text-grid.web-view.tsx:288-292) and retain the last answered lookup only across asks that differ solely by the hasCatalogSettled flip (the Retry path), replacing it when the newer ask answers. Update the test at use-dbl-install-lookup.hook.test.ts:148-163 to exercise the install trigger, and add a test that an answered lookup followed by a settle flip with refreshCounter unchanged still resolves the project. If the Retry spinner is accepted instead, extend the PR description's "Brief spinner" note to cover Retry and say it lasts as long as the catalog fetch.
Other findings in this file: #7, #9
extensions/src/platform-scripture-editor/src/scripture-text-grid/grid-resources.utils.ts line 73 at r1 (raw file):
uid: string, ): string | undefined { const key = Object.keys(installedProjectIds).find((candidate) =>
#6 - low · checked and confirmed
Resolving each grid reference scans every key of the install map, which reaches about 1,800 entries once the catalog has loaded, instead of using a lower-cased index.
What happens: findInstalledProjectId (:69-77) runs Object.keys(installedProjectIds).find(isSameDblEntryUid) per reference, and isSameDblEntryUid lower-cases both sides (dbl-resource-lookup.utils.ts:8-9). Once the backend has the catalog, ProjectInstallStatus returns a key for every catalogued uid. findCachedDblResource (:22) scans the catalog the same way, while indexDblResourcesByUid (downloaded-resources.utils.ts:91-95) already builds a lower-cased Map.
Why it matters: The cost is negligible today: toGridResources is memoized (scripture-text-grid.web-view.tsx:325-329), a grid has a handful of references, and the offline disk map names only installed projects. This is duplication more than a performance problem.
Fix: Optional at this size. If you do it, build the lookups once per toGridResources call rather than per reference. For the install map, build a Map from Object.keys(installedProjectIds) keyed by key.toLowerCase() that keeps the first key seen (if (!map.has(lower)) map.set(lower, key)), so the result matches today's Array.find, and look references up with reference.id.toLowerCase(). If you also index the catalog, note that indexDblResourcesByUid is last-wins, so either keep first-wins semantics or confirm catalog uids are unique. The existing grid-resources.utils.test.ts cases cover the behaviour; add one for a mixed-case duplicate key only if you change the tie-break.
extensions/src/platform-get-resources/src/main.ts line 357 at r1 (raw file):
// A failed catalog fetch must not cost the local list: for an offline user with no cached // catalog, these rows are the only resources the picker can offer. try {
#4 - low · checked and confirmed
Picking a downloaded DBL resource from a panel while the catalog is failing saves it as a plain project reference, not a DBL resource reference.
What happens: The new inner try (main.ts:357-361) swallows the catalog failure, then cachedResources ?? [] (:371) makes buildLocalNonDblResources(allMetadata, []) (:374) exclude nothing (get-local-non-dbl-resources.utils.ts:56-66). Every read-only project, DBL-installed ones included, becomes a row with dblEntryUid === projectId, which isNonDblResource (resource-reference.utils.ts:15) takes as the marker for a ProjectReference.
Why it matters: Listing local resources when the catalog fails is intentional (the comments at :355-356 and :367-370, and the adr-dbl-empty-catalog-is-a-failed-fetch entry), and before this change the picker offered nothing offline. A ProjectReference still resolves offline by project id, so the text displays. What is lost is that a reference picked in this window never gets DBL update or Get Resources status once the catalog returns.
Fix: Either keep DBL-installed rows out of the list, or record the trade-off. To keep them out: in getLocalNonDblResources (main.ts), when cachedResources is undefined, call the existing readInstallStatus() (main.ts:151-162) and drop from allMetadata any project whose id appears among its values, before buildLocalNonDblResources. Treat undefined or an empty map as "no answer" and exclude nothing. Add a main.test.ts case where the catalog fetch throws and readInstallStatus reports a project as DBL-installed, which must fail without the change, and a second where the status is empty and the project is still listed. To keep the behaviour instead, add a line to the Consequences of adr-dbl-empty-catalog-is-a-failed-fetch saying a resource picked while the catalog is failing is saved as a plain project reference and gets no DBL update status.
- Bible texts panel: a saved DBL selection with no row while the catalog is failing keeps the catalog-error view, and the fallback to another row is no longer saved over it (#1). Applies to an empty referenced list too, where a downloaded DBL pick is saved as `dbl:`. - C#: a catalog the compatibility whitelist empties is rejected like an unreachable one, before `_resources`/`_hasFetchedResources` are written, so install status keeps answering from disk (#3). - Text Collection install lookup: an answer holds across a Retry (no spinner, and no disk scan against the provider the refetch holds); only an install from the grid or a change of references discards it; a project added, removed or renamed anywhere re-asks; a slow earlier ask cannot overwrite a newer answer (#7, #8, #9). - ADR: record the picker trade-off while the catalog is failing (#4) and track the Model Text panel gap in PT-4814 (#5). #2 verified against ParatextData 9.5.0.24: nothing on the catalog fetch path raises a Yes/No or OK/Cancel prompt, so the capture scope stays. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
@katherinejensen00 made 1 comment.
Reviewable status: 0 of 39 files reviewed, 8 unresolved discussions.
a discussion (no related file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
I reviewed the offline catalog changes across the C# provider, platform-get-resources and the Text Collection grid, and left 9 findings: 1 medium and 8 low. Eight are inline; the ninth is below, because it is about a file this PR does not change. Each shows the severity I settled on, and each has been checked against the code at
d27bacc809d. Several of the fixes are marked optional or give a documented alternative, so treat the low ones as your call.
#5 - low · checked and confirmed ·
extensions/src/platform-scripture-editor/src/model-text-panel.web-view.tsx:72The Model Text panel still shows installed DBL resources as unavailable while offline, because the disk fallback was added only to the Text Collection grid.
What happens: The rule "resolve DBL references from disk when the catalog can't" lives in
useDblInstallLookup, which only the grid uses. The Model Text panel callsuseDblResourceCatalog()(model-text-panel.web-view.tsx:72), which still resolves only through catalog rows plusgetLocalNonDblResources.Why it matters: This change does not make the Model Text panel worse offline: where it used to read a silently empty "ready" catalog, it now gets
hasCatalogErrorand shows the retry state it already wires up (:204-205). The gap is documented as deferred (the "known sibling gap not addressed here" line inadr-dbl-empty-catalog-is-a-failed-fetch, and "Not in this PR"), but no ticket tracks it, so the deferral is not searchable.Fix: Do not build the fallback in this PR. File a follow-up ticket for giving
useDblResourceCatalogconsumers the disk-scan fallback, and change the line inadr-dbl-empty-catalog-is-a-failed-fetchfrom "a known sibling gap not addressed here" to "a known sibling gap, tracked in PT-XXXX" with the real key. The follow-up must use one shared hook for both the grid and the Model Text panel, with a Model Text panel test (a rejected catalog plus an installed DBL resource on disk) that fails without the fallback.(AI-assisted, with my guidance)
#1: Fixed. Readiness now keeps catalogError when the saved selection matches no row, and the fallback write is skipped while the catalog has failed; committing a pick and the bare-id migration still go through. It also applies when nothing is referenced, since a downloaded DBL resource picked online is saved as dbl: too. Tests cover all three cases and fail without the change.
#2: Checked by decompiling ParatextData 9.5.0.24. GetInstallableDBLResources, DblParatextApi.GetResourceList, RESTClient and ConvertXmlResponseToInstallableDblResources raise only AlertButtons.Ok notices, and our DblMigrationOperations callback raises none; DblProjectDeleter isn't called during a fetch. Leaving the capture scope as it is.
#3: Fixed as suggested. The whitelist result is checked before _resources and _hasFetchedResources are written, with a test that disk install status still answers afterwards.
#4: Kept the behaviour and recorded it in the ADR's Consequences. Hiding those rows would leave an offline user unable to pick an installed resource at all, and it wouldn't close the gap, because the Bible texts panel's downloaded rows also become project references without a catalog.
#5: Filed PT-4814 (one shared hook, plus a Model Text panel test) and cited it in the ADR.
#6: Leaving as is. It's memoized over a handful of references, and a Map would add code without a measurable gain.
#7: Added the onDidChangeProjects re-ask with a test. I didn't add the timed re-ask; the Retry banner already covers that case.
#8 / #9: Fixed. An answer now belongs to a generation keyed on isNeeded and refreshCounter. A Retry keeps it (no spinner) and skips the disk check while the catalog refetch holds the backend; an install still discards it. An empty "busy" answer never replaces a real one, and a slow earlier ask can't overwrite a newer answer. Each rule has a test that fails without it.
Offline, ParatextData's GetInstallableDBLResources reports an unreachable DBL by raising an alert and returning an empty list. The C# provider accepted that as the catalog, and platform-get-resources persisted it over the cached one, so the Text Collection -- which resolves DBL references only through the catalog -- showed "Resource not installed" for installed resources while the Bible texts tab still showed them. C# (DblResourcesDataProvider): - RequireFetchedCatalog rejects an empty or 401 fetch; _resources and _hasFetchedResources are written only after validation, so a failed fetch keeps the loaded catalog and install status keeps answering from disk. The 401 path no longer wipes _resources before throwing. - Alerts raised during a fetch are captured into the error and logged on every exit path. Injectable fetcher for tests. platform-get-resources: - resolveDblCatalog rejects an empty catalog; parsePersistedCatalog discards an empty persisted one. - The startup retry loop stops on a thrown attempt; concurrent on-demand reads share one fetch; the local non-DBL list survives a catalog failure. Text Collection (Scripture Text Grid): - useDblInstallLookup asks the backend which DBL entries are installed on disk for references the catalog cannot resolve. An empty install map is never an answer; a catalog row decides while the disk cannot; only the current ask's answer is used. - New "Couldn't check whether this text is installed" cell state, shown only when the catalog failed, always with a retry banner above the grid (inside a persistent status region, focus kept during a retry). - A catalog row marked installed with an empty project id no longer renders as a cell. Bible texts panel: when the catalog fails, rows that resolved without it win over the catalog-error view. Also: Storybook stories for the new states, an ADR entry (adr-dbl-empty-catalog-is-a-failed-fetch) plus amendment notes on two related entries. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Document that the DblResources getter rejects when the DBL is unreachable instead of resolving an empty list - Test main.ts: single-flight on-demand catalog fetch, a fresh fetch after a rejection, and the local list surviving a catalog failure - Move the Text Collection retry state into useCatalogRetryState, tested - Say "resource" rather than "text" in the couldn't-check cell line - Match DBL uids case-insensitively in both catalog and disk lookups - Share the retry banner's string keys; merge the duplicated readiness branches in the Bible texts panel; take toGridResources options as an object; widen the downloading state's doc; name the whitelist cause in the empty-catalog error; note which C# members are internal for tests Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Bible texts panel: a saved DBL selection with no row while the catalog is failing keeps the catalog-error view, and the fallback to another row is no longer saved over it (#1). Applies to an empty referenced list too, where a downloaded DBL pick is saved as `dbl:`. - C#: a catalog the compatibility whitelist empties is rejected like an unreachable one, before `_resources`/`_hasFetchedResources` are written, so install status keeps answering from disk (#3). - Text Collection install lookup: an answer holds across a Retry (no spinner, and no disk scan against the provider the refetch holds); only an install from the grid or a change of references discards it; a project added, removed or renamed anywhere re-asks; a slow earlier ask cannot overwrite a newer answer (#7, #8, #9). - ADR: record the picker trade-off while the catalog is failing (#4) and track the Model Text panel gap in PT-4814 (#5). #2 verified against ParatextData 9.5.0.24: nothing on the catalog fetch path raises a Yes/No or OK/Cancel prompt, so the capture scope stays. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PT-4184's verse-aligned Grid view (#2781) added resource-column.component.tsx, importing GridResource from resource-cell.component. This branch moved that type to grid-resources.utils, so the merge failed typecheck:workspaces with TS2614 even though neither side did alone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
0c8a89c to
c560886
Compare
Picking a published resource (e.g. HBKFRA) as the current project left the Text Collection on the previous project, still showing its texts and still offering them in the project picker. The Comments tab errored on the resource. - updateRelatedTextCollectionPanel follows every project, resources included. - The grid never binds its settings to a published resource. useTextCollectionSources asks the new useTextCollectionBinding hook, which binds a project only once its own platform.isPublished reading says it is not one (tagged per project, so a previous project's answer is never used). A resource gets no textConnectionPdp, and every Text Collection write goes through it, so the mount-time overlay init and View Options cannot write Extensions/UserSettings-<user>.xml into a resource's folder. This also covers an unbound grid seeding itself onto a resource. - The grid says "Resources don't have a Text Collection. Switch to a project to see its texts." and publishes no navigable projects for a resource. - The Comments panel says "Resources don't have comments." instead of waiting on a comments provider a resource never registers. - Amends adr-column-3-panels-are-told-their-project; the Checks side panel keeps its own resource skip. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
2 participants
Summary
Problem: with the internet disconnected, the Text Collection showed "Resource not installed" for DBL resources that are installed. The Bible texts tab showed the same resources fine.
Root cause: when ParatextData can't reach the DBL, it returns an empty resource list instead of throwing. We accepted that as the real catalog and saved it over the good cached one. The Text Collection finds installed resources only through that catalog, so everything looked uninstalled.
Fix: an empty result now means "no answer", never "nothing exists".
Out of scope: the internet-settings framework (PT-1596, PT-3283).
Review guide
Start with these; they hold all the logic:
c-sharp/.../DblDownloadableDataProvider.csRequireFetchedCatalogthrows on an empty or 401 fetch.FetchResourcesCorewrites state only after that check, so install status keeps answering from disk offline.platform-get-resources/src/dbl-catalog.utils.ts,resources-cache.util.ts,main.tsdbl-catalog.utils.tsrejects an empty catalog, andresources-cache.util.tsdiscards a saved empty one. Inmain.ts, concurrent reads share one fetch, the startup retry loop stops after an attempt that threw, and the local resource list survives a catalog failure.scripture-text-grid/grid-resources.utils.tstoGridResourcesdecides each cell, in this order: catalog row → disk answer → catalog row says not installed → checking (spinner) → "couldn't check" (catalog failed) or "not installed".scripture-text-grid/use-dbl-install-lookup.hook.tsscripture-text-grid.web-view.tsxrole="status"region) and the navigable-project publish gate.resource-text-panel.web-view.tsxThese can be skimmed:
catalog-retry-banner.*,use-catalog-retry-state.hook.ts, and the new'unverified'state inresource-cell*.dbl-resource-lookup.utils.tsgains a case-insensitiveisSameDblEntryUid.localizedStrings.json.AlertCapture.WriteCapturedToConsole.adr-dbl-empty-catalog-is-a-failed-fetch, plus amendment notes on two related entries.Behavior changes worth knowing
getDblResourcesandgetCachedResourcesnow reject offline (with no cached catalog) instead of resolving an empty list. Every caller in this repo handles the rejection, and Studio doesn't call either one. The.d.tsdocuments it.findCachedDblResourcenow matches DBL uids case-insensitively. That also affects the resource panels and the Model Text panel.Testing
main.test.ts) and the Text Collection.typecheck:core's missing generatedrelease/app/buildInfo.json, a local-environment gap.npm testfails only inthird-party-notices, which needs a builtextensions/dist.cachedDblResourcesto[]and relaunch offline: it still renders in both tabs.Not in this PR
CatalogRetryBannerandRowWithUnresolvedCellsstories.Full review summary (findings, decisions, interview notes)
Code Review Summary
Branch: pt-4686-text-collection-offline
Base: origin/main
Date: 2026-09-28
Review model: Claude Opus 5.5
Files changed: 33
Overview
This branch fixes one consequence of missing internet on the Text Collection surface. A locally installed DBL resource that the Bible texts tab displays correctly showed "Resource not installed" or "No text for this verse" in the Text Collection while offline. It does not touch the internet-settings framework (PT-1596 and PT-3283 remain out of scope). The ticket's earlier framing as a server-switch problem was wrong.
The root cause is how ParatextData reports an unreachable DBL: it raises an alert and returns an empty list instead of throwing. The C# provider accepted that empty list as the catalog. platform-get-resources then saved it over the good cached catalog. The Text Collection resolves DBL references only through that catalog, so every installed resource looked missing.
The fix treats an empty result as "no answer" in three layers:
The Bible texts tab now shows the rows it resolved without the catalog, instead of a catalog-error view. Stories cover the new states, and an ADR records the decision.
API Changes
lib/platform-bible-react,lib/platform-bible-utilsandlib/papi-dts/papi.d.tsdid not change.extensions/src/platform-get-resources/src/types/platform-get-resources.d.ts:'platformGetResources.getCachedResources'changed. It now says "Concurrent calls share one fetch", and@throwsnames an unreachable DBL with no cached catalog as a rejection case.DblResourcesdata type changed during the review. It now says the getter rejects when the DBL can't be reached or the registration is invalid, and that an empty list never stands in for "offline". The signatures are unchanged.IDblResourcesProvider.getDblResources(theDblResourcesdata type, backed by C#DblResourcesDataProvider.GetDblResources):[]._resourcesand then threw.platformGetResources.getCachedResources:{ status: 'available', resources: [] }and save that empty list.platformGetResources.getLocalNonDblResources: it now returns the local list even when the catalog fetch fails. Before, it returned[].GridResourcemoved togrid-resources.utils.tsand gainedunresolvedReason.ResourceCellStategained'unverified'.shouldStopBackgroundFetchacceptsundefined.toGridResourcesnow takes an options object.DblResourceDataandGetDblResourceswent fromprivatetointernalso tests can reach them.Findings
Critical — Must address before merge
DblResourcesgetter now rejects offline instead of resolving[], but its public declaration didn't say so.getDblResourcesandgetCachedResourcesare reachable by any extension throughpapi.dataProviders.get(...). (fixed during review: added a TSDoc note onDblResourcesinplatform-get-resources.d.tssaying it rejects when the DBL can't be reached or the registration is invalid, and never resolves an empty list to mean "offline". The behavior change also gets a line in the PR description for consumers outside this repo.)The author confirmed the rejection is intended: it is the fix itself. It is recorded in the commit message and in
adr-dbl-empty-catalog-is-a-failed-fetch, which rejects the in-between option ("an empty catalog is never a true answer for a configured user"). The author also pointed out that the change is narrower than the finding suggested:getDblResourcescould already reject before this branch, on a 401 or a missing registration.getCachedResourcesalready declared@throws; offline is a new case of an existing contract.The author checked paratext-10-studio (branch
pt-4685-sba-sync-listand origin/main2844f50) and found no call to either getter or to the provider. Studio's Send/Receive auto-download calls ParatextData'sGetInstallableDBLResourcesdirectly and already reads an empty list as "DBL unavailable". Studio's repo patch still applies cleanly; the only file it shares with this branch isplatform-scripture-editor/contributions/localizedStrings.json. paratext-bible-extensions and paratext-bible-internal-extensions were not searched.Important — Should address before merge
main.tsbehaviors the offline fix depends on had no tests: the single-flight on-demandgetCachedResources, including its.finallyreset, andgetLocalNonDblResourcessurviving a failed catalog. (fixed during review: added three tests toplatform-get-resources/src/main.test.ts, using the scaffolding main added since the plan was written. They check that concurrent calls with no cache make one DBL fetch and reject together, that the next call after a rejection fetches again, and that local non-DBL rows are returned when the DBL fetch rejects. Each test was confirmed to fail when the behavior it guards is removed. The first test fails that way only by timing out after 15s, not on an assertion.)The author asked for the tests to be added in this PR.
Minor — Consider
isRetryingCatalogin the grid web view had no test. It is the state that keeps the Retry banner mounted through a retry. (fixed during review: moved it into auseCatalogRetryStatehook inscripture-text-grid/use-catalog-retry-state.hook.ts, with a 5-caserenderHooktest. The web view's behavior is unchanged.)The banner reuses "Couldn't load the list of available resources.", so a user may not connect it to the "couldn't check" cells(a UX judgment, not a defect. It is flagged for UX review in the PR, using theCatalogRetryBannerandRowWithUnresolvedCellsstories.)resource-text-panel.web-view.tsx: thecatalogErrorandemptyreadiness branches had identical bodies. (fixed during review: merged them into one condition with one comment covering both reasons.)findInstalledProjectIdmatched case-insensitively, whilefindCachedDblResourcematched exactly. (fixed during review: both now use a sharedisSameDblEntryUidhelper indbl-resource-lookup.utils.ts, with tests. This makes the catalog lookup case-insensitive for all six callers offindCachedDblResource, including the resource panels and the Model Text panel, which was agreed.)CATALOG_ERROR_KEYandCATALOG_RETRY_KEYfrom the web view. (fixed during review: they now live incatalog-retry-banner.const.ts, which both import.)checkingunresolved reason maps to thedownloadingcell state, but that state's doc said only "data is still loading". (fixed during review: theResourceCellStatedoc now also covers "the install check is still in flight". The state was not renamed.)toGridResourcestook two optional positional parameters, so the call site passed a bare boolean. (fixed during review: changed to an options object,{ installLookup?, hasCatalogError? }, with the same defaults. The call site, all tests and the@paramdocs were updated.)GetDblResourcesandDblResourceDatabecameinternalso tests can reach them, with no note saying so. (fixed during review: added a one-line note on each saying it is internal so tests can reach it.)The new C# "DBL unreachable" failure throws a plain(the TypeScript side recognizes the 401 case by its message text, and nothing would consume a typed exception. Adding one would change a cross-process contract with no user and break with the file's existing convention.)Exceptionrather than a typed onetoGridResourcesreads a uid that is absent from an answered disk map as "not installed", although the C# contract says absent means "not reported"InstallLookupTSDoc. The alternative would show "couldn't check" plus a banner for every resource that genuinely isn't installed while offline, which is the more common case. The cost is that an installed resource whose settings file can't be read shows "not installed" offline.)The startup retry loop now stops after the first thrown attempt instead of trying up to 10 times(intended and documented in theshouldStopBackgroundFetchTSDoc and the ADR. With a saved catalog, one startup failure keeps that catalog until the 12-hour refresh or the next launch. No code change; this goes in the PR description.)The author asked for recommendations and accepted them all: fix #1, #2, #4 to #9 and #13; leave #3, #10 and #11 open with the reasons above; put #12 in the PR description.
Template Propagation
Shared Regions Modified
None.
Extension Config Changes
None.
Positive Observations
RequireFetchedCatalogvalidates before_resourcesor_hasFetchedResourcesis written. A failed fetch therefore keeps the loaded catalog and never marks it fetched, which is what lets install status answer from disk offline. The 401 path no longer wipes the catalog.GetDblResources. Alerts are logged on every exit path.useDblInstallLookupcovers a busy provider, asking again after the catalog settles, and never presenting a previous ask's answer as current (answer.ask === ask).toGridResources,toInstallLookupandneedsInstallLookuphave extensive tests that fail when their logic breaks.retryUntilfrom platform-bible-utils,AlertCapture.RedactPathsForLog, the existingcatalogUnavailableandretrystrings, and the same defaultButtonasRetryableErrorView, so retry controls look the same everywhere.role="status"region, so it is announced when it appears.role="alert"is removed so live regions don't nest.aria-disabled, so keyboard focus survives a retry, and the banner stays mounted while the retry runs.aria-hidden.unverifiedstate in chapter and verse modes, an extendedPartialFailureRow,RowWithUnresolvedCells, and a fullCatalogRetryBannerset.eslint-disableis justified.Interview Notes
Purpose (author's words): "This PR covers a specific consequence of missing internet on the Text Collection surface: a locally-installed resource that displays correctly on the Bible texts tab shows 'No text for this verse' / 'Resource not installed' in Text Collection when internet is disconnected. It does not cover the internet-settings framework itself (PT-1596, PT-3283 remain out of scope), and it is no longer framed as a server-switch problem — that framing was incorrect."
Critical finding: see the Findings section. The author explained that the rejection is the fix, is recorded in the ADR and commit message, and is narrower than stated. They checked the Studio repo themselves and confirmed it is not a consumer. They asked for the doc note.
Design explanation (Step 3.3): the author walked through the non-obvious part clearly and accurately.
[]on any DBL failure, and the branch treats "empty means no answer" at each layer:RequireFetchedCatalogthrows before any state is written;resolveDblCatalogandparsePersistedCatalogreject or discard an empty catalog;fetchInstalledDblResourcesretries on an empty map, because the provider may just be busy.toGridResources:usePromisekeeps showing the previous answer, so each answer is tagged with the ask that produced it and used only if that ask is current.hasCatalogSettledis a trigger-only dependency: while the catalog fetch runs it holds the provider's lock, so the hook asks again once the catalog settles.Minor findings: the author asked which to do to "get the code as right as possible" and accepted all the recommendations.
Anything else: the author had nothing further to flag.
The author showed clear understanding of the changes and did not defer any area to AI or express uncertainty.
In-Review Quality Check
lib/.npm run typecheck:typecheck:erb,typecheck:e2eandtypecheck:workspacespass.typecheck:corefails only onsrc/main/services/app.service-host.ts, which can't find the generatedrelease/app/buildInfo.json. That file is missing from this local environment and unrelated to the branch. There are no errors in branch-changed files.extensions/, found nothing and applied no fixes. The fullnpm run lintwasn't run because it takes 40+ minutes locally.dotnet csharpier --checkis clean.npm test(full suite): 6262 passed, 17 skipped, 5 failed. All 5 failures are in.erb/scripts/third-party-notices/: 4 indegradation.test.ts, plus 1 timeout inderived-invariants.test.ts. They need a builtextensions/dist, which this environment doesn't have, and the branch touches none of those files.DblResourcesDataProviderTestsandAlertCapturetests pass, 43/43.No fixes were needed.
Suggested Review Focus
getDblResourcesandgetCachedResourcesnow reject offline instead of resolving empty. Agree that "an empty catalog is a failed fetch" (ADRadr-dbl-empty-catalog-is-a-failed-fetch). Every caller in this repo handles the rejection, and Studio is not a caller. paratext-bible-extensions and paratext-bible-internal-extensions were not checked.toGridResources(grid-resources.utils.ts): "unverified" appears only when the catalog failed, always with the Retry banner. A catalog row wins over "checking".useDblInstallLookup(use-dbl-install-lookup.hook.ts): the rule that only the current ask's answer is used (answer.ask === ask), and the trigger-onlyhasCatalogSettleddependency.findCachedDblResourceis now case-insensitive. This also affects the resource panels, the Model Text panel and long-name lookups.resource-text-panel.web-view.tsx): resolved rows win over a failed catalog, and navigable ids are published only when the catalog is ready or rows won.cachedDblResources = []), with both the Text Collection and the Bible texts tab open;6279ca87427since the base (5439a887718). Rebase before merge.AI-assisted — session
🤖 Generated with Claude Code
This change is