PT-4326: Read freely-licensed texts when no project is open - #2774
katherinejensen00 wants to merge 5 commits into
Conversation
44420da to
5e09689
Compare
|
@Sebastian-ubs That doesn't look right. Can you attach a log please? |
|
sent to you... |
Review summarySound design, no blocking findings, and the central seam is the right shape. The gaps are at the edges: one untested enforcement layer, one broken Storybook story, and a few user-visible copy inconsistencies. Against PT-4326's acceptance criteria
Scope discipline is good: Commentaries is excluded via Pattern adherenceStrong. Both ADR entries land in correct Worth addressing before merge
Minor
Worth creditingThe three-layer exclusion argument is written down and each layer actually exists. The AI-assisted review (Claude Code) — findings verified against the code before posting. |
5e09689 to
4d54fc2
Compare
|
Rebased onto current @Sebastian-ubs — panels stuck on "Loading…" after projects were removed — fixed in "Treat a project id that cannot be found as no project". A restored layout (or the last-opened-project cache, or a recents entry) could name a project that is no longer on the machine, and every project-scoped hook waited forever on a data provider that never arrives. The editor and both reading panels now resolve the container project id through @jolierabideau — worth addressing before merge
Minor
Rebase notes — Verification — |
f67ea08 to
2d3bfce
Compare
|
Re-reviewed — thanks @katherinejensen00. All seven "worth addressing" items and the minor items from my earlier review are fixed or answered, and I checked each against the branch. Approved from my side. Before merging:
AI-assisted review (Claude Code) |
|
@katherinejensen00 🙂 Stuck on loading is gone. Problems
Likely out of scope
|
2d3bfce to
400d27f
Compare
|
@jolierabideau thanks for the re-review. The branch is now rebased onto current Conflict resolutions
Verification after the rebase: @Sebastian-ubs: when you have a moment, could you confirm the "stuck on Loading…" case from your screenshot is fixed on the current branch? |
400d27f to
7bc6883
Compare
|
@Sebastian-ubs thanks for testing again, and glad the stuck-on-Loading problem is gone. First, why A and B happened: the build you tried was from before my local rebase was pushed. It still had the old Requested changes
D, E, F (probably out of scope)
|
At best we should quickly meet tomorrow to talk about what would be a better / best action for this. I think I got a bit offtrack with the register suggestion, but also not sure, why a project not open would be the gatekeeper to only list freely available resources. |
7bc6883 to
8c02615
Compare
|
@Sebastian-ubs Since this is dealing with viewing open license resources, I don't think it will affect the project dropdown. |
Lets a user with no project open pick and read freely-licensed Bible texts in the two reading panels flanking the editor, instead of "No project selected." The panels already render resources project-independently; only the chosen- resource list was project-scoped. `useResourceReferenceSource` is the seam that swaps where that list comes from - the project's text-connection PDP when a project is open, an app-scoped hidden setting when none is - returning the same state shape either way, so everything downstream is unchanged. Ships with one curated text (WEB, Public Domain). The licence test for this first pass is Public Domain only; expanding it is a data change, not a code change. With an empty allowlist `HAS_FREE_RESOURCES` switches the whole entry point off, so the panels behave exactly as before. A non-free resource is unreachable rather than merely refused, in three layers: the picker is restricted before the dialog sees the catalog, the read path filters what is shown, and the setting validator refuses newly added non-free references while letting stored ones survive an allowlist narrowing. Locally downloaded resources are excluded entirely when there is no project, since nothing filters those. Navigation also had to change: without a project the toolbar's book/chapter control resolved no target and disabled, leaving a chosen text readable at exactly one reference. BCV navigation now falls back to a project-less editor, in Simple mode only. Decisions and rejected alternatives are recorded in adr-no-project-reading-choice-in-app-settings and adr-bcv-falls-back-to-projectless-editor. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
A restored layout, a recents entry, or the last-opened-project cache can name a project that is not on this machine. The reading panels then sat on their loading state forever, the editor showed a blank pane, and the toolbar picker an error card the user could not act on. Reported on the PR by a tester who emptied their projects folder. - useResolvedContainerProjectId resolves the container id to undefined once getMetadataForProject rejects; the editor and both reading panels read the resolved id, never the raw prop. - The project picker reports no current project for an id it cannot resolve, and keeps its error card for a metadata fetch that itself failed. - The Simple-mode switch confirms the cached project before building a layout around it (clearing the cache on a definite miss), and the recents walk skips candidates it cannot find. - Record the decision in the architecture-decision log. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fd86WHD1RQo5tE5xkANftn
- Test the no-project settings validator (shape, allowlist, grandfathering of stored ids, case-insensitive matching) and rename it to no-project-reference-list.utils.ts to match its sibling; use isDblResourceReference for the shape check. - Share one set of no-project strings between both panels (%webView_resourcePanel_noProject_*%) and give the pick button its ellipsis. - Title the Model Text tab "Text" with no project open, matching the panel body. - Restore main's [usj] deps on the model text panel's editor feed. - Drop the FREE_RESOURCE_IDS alias for the frozen allowlist. - Translate the hidden es setting labels, bring the remaining no-project es strings to usted, and order settings.json alphabetically. - Note case-insensitive matching in allowedResourceIds' TSDoc and regenerate papi.d.ts. - Rewrite comments that narrated earlier implementations. - Mock projectLookup and keep the real mergeResourceReferenceLists in the resource panel web view test, which the rebase onto main exposed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fd86WHD1RQo5tE5xkANftn
…n the free-text picker
Responds to review of the no-project reading panels.
Toolbar project picker:
- Always reads "Select project", never "No projects".
- With no projects on this computer, activating the trigger opens Home
directly instead of a popover holding only "More projects…". This is a
new opt-in ProjectSelector prop, shouldRunFooterActionWhenEmpty.
- The placeholder is muted until hover or open. The trigger no longer sets
bg-transparent, so the ghost variant's hover:bg-muted shows, the same as
the book/chapter control.
Free-text resource picker:
- The notice now says what the user can do: "You can use freely available
texts without a project. Register with an organization to access more
resources."
- It carries a Register button that opens the registration ("Account")
dialog. ResourcePickerDialog gains noticeAction; the dialog options gain
noticeCommandLabel / noticeCommand, mirroring the notification service's
clickCommand pair. The picker is cancelled before the command runs, so
the registration dialog is not left behind the modal.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… open Rebasing onto main brought in #2859's third install-failure message, "The model text is installed but couldn't be opened." The no-project panel swaps every "model text" message for a neutral one, so the rebase gave this message a no-project variant too ("The text is installed but couldn't be opened.", en and es). This pins that swap. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
8c02615 to
9372d7b
Compare
|
@Sebastian-ubs You're right that those two problems were real, but this branch didn't introduce them, and they're already fixed on main. What happened On 22 Sept, the project dropdown in the title bar was swapped out for a new one (#2801). Your "22 Sept main" build must have been made just before that change, so it still had the old dropdown. You can tell from the label: the old dropdown showed names as "World English Bible USA … (WEB)", and the new one shows "WEB - World English Bible USA …". The old dropdown had both bugs you saw:
Where things stand now
If you try a build from after 22 Sept (latest main or this branch), both problems should be gone. If you still see either one, please send me a screenshot and I'll look again. |







Summary
Lets a user with no project open pick and read freely-licensed Bible texts in the two reading
panels flanking the editor, instead of staring at "No project selected." Sub-task of PT-4323 (the
interim fix for users who finish setup with no projects).
The panels already render resources project-independently — only the chosen-resource list was
project-scoped. So the core of this PR is one seam that swaps where that list comes from, and
everything downstream is unchanged.
Ships with exactly one text (WEB, Public Domain). The curated allowlist is deliberately tiny;
the licence test for this first pass is Public Domain only. Expanding it is a data change, not a
code change.
Why review this
Not part of the current epic. It unblocks PT-4323's "user finishes setup with nothing to read"
case, and it is worth reviewing now because it is what studio testing needs in order to exercise
the no-project reading flow at all.
Where to start
Read these five in order — they are the whole idea. The other 32 files are tests, strings, stories
and call-site updates.
free-resources.const.tsuse-resource-reference-source.hook.tsresource-panel-readiness.utils.tsregistrationRequiredstate. Both panels share it, so they cannot drift.no-project-reference-list.validator.tsnavigation-target.util.tsTwo ADRs record the decisions and the rejected alternatives:
adr-no-project-reading-choice-in-app-settingsandadr-bcv-falls-back-to-projectless-editor.Details
How the exclusion guarantee is enforced (three layers)
A non-free resource must be unreachable, not merely refused:
allowedResourceIdsnarrows the catalog before the dialog seesit, so the language filter and total count stay consistent with what is selectable. It carries a
notice, because the dialog builds its own explanation of a short list from the fetch results,which this narrowing does not touch.
allowlist narrowing, so a later, wider list restores them rather than having destroyed them.
With no project, locally-downloaded resources are excluded from the panel's row list entirely —
nothing filters those, so including them would bypass all three layers.
Why an app-scoped hidden setting, not web-view state
The project path persists picks in the
platformScripture.textConnectionSettingsPDP, which doesnot exist without a project.
useWebViewStateis not a substitute: Simple mode never persists itslayout, so a pick would be lost on every restart — for exactly the user this exists to help. Full
reasoning and the rejected
UserStateContributionalternative are in the ADR.Registration handling, and a trap worth knowing about
A missing Paratext registration makes the catalog unreachable in a way a retry cannot fix, so that
state offers a Register button instead of Try again.
Detecting it is not obvious:
getCachedResourcesresolvesundefinedrather than throwing, so thethrown-sentinel check never sees it. The catalog hook now probes
paratextRegistration.doesUserHaveValidRegistration, but only after the fetch has already failed— so the cold-start retry storm behind
adr-registration-validity-once-per-sessiondoes not apply.The tempting shortcut is wrong.
isGetDblResourcesAvailablelooks free, butDblResourcePasswordProvider.IsPasswordAvailablereturns false both for an invalid registrationand for a build with no DBL user-secrets — so acting on it tells a developer with a perfectly good
registration to go register. Noted in the code so nobody reaches for it later.
Known gaps and a CI issue this surfaced
extensions/package.jsonhas notypecheckscript, sonpm run typecheck --workspaces --if-presentsilently skips every extension. That hid a real type error in this branch (caught byreview, now fixed) and is a pre-existing CI gap worth addressing separately.
this was written on has no DBL credentials, so whether
9879dbb7cfe39e4dresolves to WEB andinstalls is exactly what studio testing should check first.
getMarkerMenuItems is not a functionfrom astale local
platform-editor, in a file this branch does not touch. Expected green in CI.Testing
npm test— 1,453 passing inplatform-scripture-editor; new coverage for the allowlist,the source seam, the readiness states, the picker restriction, and the navigation fallback
npm run lint— cleantsc -p extensions/tsconfig.jsondirectly, given the gap above)AI Involvement
AI-assisted throughout: implementation, tests, and docs were generated with Claude Code and reviewed
by me. Two
/code-review maxpasses were run against the branch and their findings addressed —including one bug that would have shipped (locally-downloaded resources bypassing the allowlist) and
a permanent spinner in the Bible Texts panel.
Risk Level
Low–Medium. Every new branch is gated behind
HAS_FREE_RESOURCES, so with an empty allowlist thepanels behave exactly as before. The project-open paths are unchanged apart from shared-component
signatures. The medium half is that this is the first code to run in the no-project state, and it has
not yet been exercised against a real catalog.
This change is