PT-4731: Show Biblica license notice on restricted texts - #2864
katherinejensen00 wants to merge 6 commits into
Conversation
91e8947 to
de1fb36
Compare
Show a dismissible licence banner on Biblica's traditionally licensed
(non-Open) texts, and keep them from being picked as a model or base text.
Detection (C#)
- BiblicaLicensing: Biblica's list of 56 DBL ids (rights holder Biblica,
not open access) is the authority; a copyright rule ("Biblica, Inc" /
"Biblica®", not "Biblica® Open", OBTT exempted by id) also counts, so an
installed Biblica text added to the DBL later is covered. A survey of all
1,819 resources in Platform.Bible's list shows the rule and the list agree.
- New read-only computed project setting platformScripture.copyrightNotice:
none | notification (PT9's "Notification:" copyright banner, e.g. ESV) |
restrictedLicense (with the years of the text's copyright statement).
- DblResourceData.IsRestrictedAsModelText for catalog rows;
ProjectSummary.IsRestrictedAsBase for Manage Books.
Banner (platform-scripture-editor)
- CopyrightNoticeBanner in Simple Column 1 and Column 3 and the Power-mode
editor: bold name, clamped to two lines with Show more (re-measured when
a hidden tab becomes visible), More info… opens Biblica's terms with a
link to biblica.com/permissions, dismissal kept per pane and per text.
- Text Collection cells get an info icon carrying the notice.
- Strings live in platform-scripture and are translated into all 33 of its
locales (AI drafts).
Pickers
- ResourcePickerDialog gains getDisabledReason; the Column 1 model text
picker and Team layout's model text picker grey out restricted texts
with a tooltip. Already-selected ones can still be deselected.
- Manage Books "Create based on" shows restricted texts but not as choices.
Also: Storybook stories for the banner and the picker's disabled rows,
rebuilt platform-bible-react/-utils dist and papi.d.ts, and
adr-biblica-license-notice-computed-setting.
Enhanced Resources is not covered: an Enhanced Resource is not a ScrText
project, so the setting cannot reach it.
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Restriction - One check, BiblicaLicensing.IsRestricted(scrText), used by the notice setting and Manage Books. It applies only to resources, so a Biblica team's own editable project (or Biblica's master project, whose DBL id matches the list) is never flagged. It reads the typed, entity-decoded plain-text copyright, survives a malformed DBL id, and matches more spellings (Biblica after a non-Latin letter, Arabic and full-width commas, "Biblica Open" without ®). - The notice now carries its names (name, fullName falling back to the short name), so panes read one setting and a missing full name no longer shows "*Name Missing*". It is built through factories, and CopyrightNoticeKind has its own file. - Pickers no longer rely on a flag baked into the cached DBL catalog: a new listModelTextRestrictions call (no network needed) returns Biblica's ids and the installed restricted resources, and platform-get-resources stamps isRestrictedAsModelText on every catalog and local row it serves. This covers an old persisted cache, offline sessions, local rows and Biblica texts added to the DBL after the list was made. Banner and details - Dismissal matches Paratext 9: a pane remembers only the text whose notice was dismissed, so switching to another text and back shows it again. The banner and grid icon are keyed by project, so a switch never shows the previous text's notice. - The details dialog is modal, returns focus to its opener, has a working width and a localized Close, and announces its text. The permissions URL is a real link, opened in the browser through platform.openWindow. - role="note" instead of a live region; the dismiss button has a tooltip and a clearer label; the grid icon has a short label and its tooltip is one paragraph; "Show more" re-measures when the wording changes. - Text Collection: clicks, keys, right-clicks and Escape inside the details dialog no longer reach the verse row. - Model texts stored as project references get their notice too. - The Power/Simple editor asks every project, since a "Notification:" copyright shows its banner on any text, as in Paratext 9. Pickers - ResourcePickerDialog disabled rows: reachable like other rows, name plus reason as description, lock icon, no hover highlight, no selection by click, Enter or Space; shared row cells extracted. - ProjectSelector disabled rows keep pointer events so their reason tooltip opens, and carry the reason as their description. - The picker tooltip is a core string (%resourcePicker_restrictedModelText_tooltip%). Also: notice strings sorted in every locale; wiring tests for the web views, Team layout, Manage Books and the restriction list; stories for the grid icon, grid cells and the model text panel; the Storybook stub no longer crashes the grid stories; rebuilt platform-bible-react/-utils dist; ADR adr-biblica-license-notice-computed-setting updated to the final design. Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Correct the CopyrightNotice and copyrightNotice setting TSDoc: restrictedLicense applies only to resources, notification to any project whose copyright starts with "Notification:" - Drop the ellipsis from "More info" in all locales; it opens an informational dialog - Give the ProjectSelector disabled reason the tooltip's own foreground so it meets contrast on the inverted tooltip background - Rename the disabled-reason key to %restrictedModelOrBaseText_disabledReason%, since Manage Books uses it too - Hide the bound-but-closed Open button on disabled ProjectSelector rows - Make BiblicaLicensing.IsOnRestrictedList internal - Share one helper for the notice's message format between the banner and the message utils - Move BIBLICA_PERMISSIONS_URL to the const file; name utils files .utils - Note that the details wording is Biblica's own and not to be reworded - Add a story for the Notification details dialog - Make the model-text restrictions cache free of `this`, and extract the resync decision and bounded wait-then-stamp from platform-get-resources' main.ts into tested utils - Use a sample reason in the ResourcePickerDialog disabled-rows story and logical-direction classes in its row cells Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The resource tests read the notice through the project data provider, which refuses to open a resource without a valid Paratext registration. CI machines have none, so all eight failed there while passing on registered developer machines. Ask CopyrightNotice.FromScrText directly for the resource cases; the editable-project tests still cover the provider routing the setting. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CopyrightNotice.fullName already falls back to the short name on the C# side, as its type documents, so the dialog's `fullName || name` repeated that guarantee and tripped the project-name adoption sweep. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rebased onto main after PT-4585 (#2849) moved the zoom markers inside the editor scroll boxes and reworked the resource cell's right-click menu: - Portalled right-click test and copyright stories use the new cell props (required zoomArea, menuStrings fixture, Copy-only menu) - Grid test's verse-text ids no longer match the /^cell-/ cell count - Rebuilt platform-bible-react and platform-bible-utils dist Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
de1fb36 to
5d0046e
Compare
lyonsil
left a comment
There was a problem hiding this comment.
Review of the Biblica license notice and the model-text restrictions: 11 findings. Each comment shows the severity it settled at and a status; checked and confirmed means the finding was verified against the code at this head, and needs a human call means it could not be settled either way and is yours to judge. Findings that do not sit on a line this PR changed are listed below instead of inline.
Findings that could not be attached to a line in this diff
#9 (low · checked and confirmed) extensions/src/platform-scripture-editor/src/scripture-text-grid/resource-cell-view.component.tsx:393 - Drag-selecting text inside a cell's copyright details window runs the Text Collection grid's column-drag start handler, recording that column as the one being dragged although no column is being moved.
What happens: the copyright details dialog is portalled to document.body, but React still bubbles its synthetic events up the component tree. In chapter context copyrightIndicator renders inside the draggable data-cell-header div (resource-cell-view.component.tsx:424), whose onDragStart is headerDrag?.onDragStart with no containment check (:394). A text drag inside the dialog therefore calls headerDrag.onDragStart, which sets draggedIdRef (scripture-text-grid.component.tsx:220-222).
Why it matters: the dialog's dragend also bubbles to onDragEnd (:395) and clears that state, so nothing moves today; the cost is a reorder handler running on events that are not its own. The sibling contextmenu handler in this file already carries the guard for the same reason (:335). Only reachable when a restricted or Notification text sits in a reorderable chapter-view grid.
Fix: In resource-cell-view.component.tsx, replace onDragStart={headerDrag?.onDragStart} on the data-cell-header div (:394) with a handler that calls headerDrag.onDragStart() only when event.target instanceof Node && event.currentTarget.contains(event.target) (the guard handleCellContextMenu uses at :335), and leave it undefined when headerDrag is. Add a test in resource-cell-view.component.test.tsx next to the existing portalled-window guard test that fires a dragstart from inside a portalled child of copyrightIndicator and asserts headerDrag.onDragStart is not called (and is called for a drag on the header itself). The change must be forward-facing in any comment it adds.
How this was checked: The copyright details dialog is a Radix Dialog portalled to document.body (dialog.tsx:34-40, 128), so React bubbles its drag events through the component tree. In the chapter-context branch the indicator is a child of the data-cell-header div (resource-cell-view.component.tsx:424), whose onDragStart={headerDrag?.onDragStart} (:394) has no containment check, so a text drag in the dialog runs the grid's onDragStart and sets draggedIdRef (scripture-text-grid.component.tsx:220-222). The effect is small: the dragend from the dialog also bubbles to onDragEnd (:395), which clears the ref, and the drop-target ring is suppressed for the dragged column itself (:218-219). The same PR already guards the sibling contextmenu handler at :335 for this reason, and this PR introduced the indicator that makes the path reachable.
Not attached to a line: not on a changed line; cannot be posted inline
#10 (low · checked and confirmed) extensions/src/platform-scripture-editor/src/scripture-text-grid/scripture-text-grid.component.tsx:433 - Drag-selecting text inside a cell's copyright details window runs the Text Collection grid's reorder drag handlers, which record that cell as the one being dragged and accept drags over it, although no cell is being moved.
What happens: the copyright details dialog is portalled to document.body, but React still bubbles its synthetic events up the component tree. On the draggable listitem only onClick has a containment guard (scripture-text-grid.component.tsx:413-420); onDragStart, onDragOver and onDrop are wired straight to reorder (:433-437), and the ResourceColumn wrapper wires onDragOver and onDrop the same way (resource-column.component.tsx:73-74). A text drag inside the dialog sets draggedIdRef (scripture-text-grid.component.tsx:220-222), and onDragOver calls preventDefault and setDragOverId (:226-229).
Why it matters: no drop ring shows, because isDropTarget excludes the dragged cell (:218-219), and the dialog's dragend clears the state (:223-226), so nothing moves today; the cost is reorder handlers running on events that are not their own, on an element whose click handler already guards against exactly this. Only reachable when a restricted or Notification text sits in a reorderable grid.
Fix: Wrap reorder.onDragStart, reorder.onDragOver and reorder.onDrop at both scripture-text-grid.component.tsx:433-437 and resource-column.component.tsx:73-74 so each runs only when event.target instanceof Node && event.currentTarget.contains(event.target), matching the guard the listitem's onClick uses at :413-420; leave onDragEnd as is. Add tests in scripture-text-grid.component.test.tsx (next to the existing click-guard test) and in the resource-column tests that fire dragstart/dragover/drop from a portalled child and assert no reorder state change or onReorder call, while a drag on the cell itself still reorders.
How this was checked: In the verse-row listitem, onDragStart, onDragOver and onDrop are wired directly to reorder (scripture-text-grid.component.tsx:433-437) with no containment check, while this PR added one only to onClick (:413-420) and a target check to onKeyDown. The dialog is portalled but its synthetic drag events bubble to this listitem, so a text drag in the dialog sets draggedIdRef (:220-222), onDragOver calls preventDefault and setDragOverId (:226-229). The ring is not shown: isDropTarget excludes the dragged resource (:218-219), and the dialog sits over the grid so only the owning cell receives the events; dragend bubbles to onDragEnd (:223-226) and clears state. The same unguarded onDragOver/onDrop also sit on the ResourceColumn wrapper (resource-column.component.tsx:73-74), which the dialog bubbles into for chapter and aligned views.
Not attached to a line: not on a changed line; cannot be posted inline
#11 (low · checked and confirmed) src/shared/data/keyboard-shortcuts.data.ts:539 - The keyboard shortcuts catalog still describes the Text Collection's Enter/Space and Escape shortcuts without the conditions this change added to when they fire.
What happens: Enter/Space now activate a cell only when the listitem itself has focus (scripture-text-grid.component.tsx:425), and Escape now ignores presses inside an open dialog, decided in the new chapter-context-escape.utils.ts. The catalog entries scripture-text-grid-open-chapter-context and scripture-text-grid-close-chapter-context (keyboard-shortcuts.data.ts:539-566) were not touched, and the Escape entry's locations does not list the new utils file.
Why it matters: documentation only, with no behavior effect; .claude/rules/keyboard-shortcuts-catalog.md asks that each changed handler's context and locations stay accurate.
Fix: update the context text of both entries, and add extensions/src/platform-scripture-editor/src/scripture-text-grid/chapter-context-escape.utils.ts to the Escape entry's locations.
How this was checked: The PR diff does not touch src/shared/data/keyboard-shortcuts.data.ts. The Enter/Space handler in scripture-text-grid.component.tsx:421-431 now returns early unless event.target === event.currentTarget, so a control inside the row keeps its own Enter/Space. The Escape decision moved into the new chapter-context-escape.utils.ts, imported at scripture-text-grid.web-view.tsx:70, and ignores presses inside [role="dialog"]/[role="alertdialog"]. The catalog entries at keyboard-shortcuts.data.ts:539-547 and 560-566 still say only "Scripture Text Grid web view", and the Escape entry lists only the web-view file. Documentation only, no behavior effect.
Not attached to a line: not on a changed line; cannot be posted inline
(AI-assisted, with my guidance)
| ProjectSettingsNames.PT_VERSIFICATION, | ||
| "4" | ||
| ), | ||
| IsRestrictedAsBase: BiblicaLicensing.IsRestricted(scrText) |
There was a problem hiding this comment.
#3 - low · checked and confirmed
One resource whose copyright or full-name setting throws when read makes the whole Manage Books project list fail to load.
What happens: ProjectSummary.FromScrText now calls BiblicaLicensing.IsRestricted (ProjectSummary.cs:112), which reads Settings.CopyrightPlainText through GetPlainTextCopyright (BiblicaLicensing.cs:152) and Settings.FullName without a guard; only ReadDblId catches (:162). ProjectFilterService.ToProjectListResult runs Select(ProjectSummary.FromScrText) over every project (ProjectFilterService.cs:131), so one exception fails the whole request. The comment in this factory says its lookups are kept cheap and exception-free, and ListModelTextRestrictions wraps the same call per project for this reason (DblDownloadableDataProvider.cs:176-188).
Why it matters: rare, but when it happens Create, Copy and Delete in Manage Books are all unavailable.
Fix: in ProjectSummary.FromScrText, call BiblicaLicensing.IsRestricted through a guard that logs and returns false on an exception, matching ListModelTextRestrictions, and add a ProjectFilterServiceTests case with a resource whose settings throw.
How this was checked: ProjectSummary.FromScrText (c-sharp/ManageBooks/ProjectSummary.cs:112, added by this PR) calls BiblicaLicensing.IsRestricted, which for any resource reads Settings.CopyrightPlainText via GetPlainTextCopyright and Settings.FullName with no guard (BiblicaLicensing.cs:141-153); only ReadDblId catches (:155-165), and its own comment says a resource's zipped DBL metadata can be unreadable. ProjectFilterService.ToProjectListResult maps every project through FromScrText with a plain Select (ProjectFilterService.cs:131), so one throw fails the whole list request. The sibling ListModelTextRestrictions wraps the same call per project in try/catch for exactly this reason (DblDownloadableDataProvider.cs:176-188). This was traced in the code, not reproduced with a resource whose copyright read actually throws, so how often it happens is unknown.
| private static partial Regex BiblicaOpen(); | ||
|
|
||
| /// <summary>Four decimal digits in any script</summary> | ||
| [GeneratedRegex(@"\d{4}")] |
There was a problem hiding this comment.
#4 - low · checked and confirmed
The restricted-license details can quote a wrong copyright year when the copyright statement contains a number of five or more digits.
What happens: FourDigits is \d{4} with no digit boundaries (BiblicaLicensing.cs:127-128). GetCopyrightYears scans the text after each © (:215-226), so a run such as 20112 or an ISBN fragment yields a four-digit slice in the 1900-2099 range, which is kept as a year (:230-231).
Why it matters: the years are inserted into Biblica's required legal wording (%platformScripture_copyrightNotice_restrictedLicense_details%). The surveyed texts pass today, so this only bites on a new or updated copyright statement.
Fix: Change the FourDigits pattern at BiblicaLicensing.cs:127 to (?<!\d)\d{4}(?!\d), so only a run of exactly four digits is read as a year. Add a BiblicaLicensingTests case that fails without the change, for example GetCopyrightYears("© Biblica Inc. 20112, 1984") returning "1984", and update the FourDigits summary to say it matches exactly four digits.
How this was checked: The FourDigits regex at c-sharp/Projects/DigitalBibleLibrary/BiblicaLicensing.cs:127 is the bare pattern \d{4} with no digit boundaries. GetCopyrightYears (:209-237) runs it over the text from the first © onward, and Matches yields the first four digits of any longer run, so "© 20112" gives the year 2011 and "© 19781984" gives 1978 and 1984. The 1900-2099 range check at :230 does not reject these slices. The whole file is new in this PR. Existing tests (BiblicaLicensingTests.cs:214-253) have no case with a run of five or more digits, so nothing pins the behavior. It only bites on a copyright statement containing such a run, and the surveyed texts pass.
| }); | ||
| } | ||
|
|
||
| async function getCachedResources(): Promise<DblResourceCatalog> { |
There was a problem hiding this comment.
#1 - medium · checked and confirmed
A resource picker or Team Layout opened in the first seconds after startup can offer restricted Biblica texts as selectable model texts, and they stay selectable until that dialog is closed and reopened.
What happens: getCachedResources (main.ts:386-394) waits at most MODEL_TEXT_RESTRICTIONS_WAIT_MS (2000, main.ts:51) for the first restrictions fetch. When getWithin times out it resolves undefined (model-text-restrictions.utils.ts:126-137), and applyModelTextRestrictions returns the rows unstamped (:16-18). resource-picker.dialog.tsx:62 and team-layout.dialog.tsx:122 fetch the catalog once through useRetryablePromise, so getRestrictedModelTextReason sees isRestrictedAsModelText as undefined for the rest of that dialog's life.
Why it matters: the pickers fail open whenever the C# DBL provider takes longer than 2s to answer, which is most likely on a cold start, and the license restriction is the point of this feature.
Fix: A row whose restriction status is still unknown must not be selectable, and the dialog must pick up the status once it is known. In getCachedResources (main.ts:386-394), mark the available result as restrictions-pending when getWithin returns undefined (add an optional field to the available arm of DblResourceCatalog in types/platform-get-resources.d.ts:153, with TSDoc, and set it in applyModelTextRestrictionsToCatalog, model-text-restrictions.utils.ts:41-48). In resource-picker.dialog.tsx (:62) and team-layout.dialog.tsx (:122), while that flag is set, disable restricted-candidate rows (fail closed: report a pending reason from getRestrictedModelTextReason, restricted-model-text.utils.ts:19) and call the existing refetch from useRetryablePromise at a bounded delay until the flag clears. Both dialogs must change, since they fetch independently. Add a test in each dialog's test file (resource-picker.dialog.test.tsx, team-layout.dialog.test.tsx) that serves a pending catalog, then a stamped one, and asserts the restricted row is unselectable before and after the refetch as appropriate, plus an extension-side test that getCachedResources flags a timed-out read.
How this was checked: getCachedResources (main.ts:386-394) passes the 2000 ms bound (main.ts:51, added by this PR) to applyModelTextRestrictionsToCatalogWithin, which uses cache.getWithin (model-text-restrictions.utils.ts:126-137). On timeout getWithin resolves undefined and applyModelTextRestrictions returns rows unstamped (:16-18), so isRestrictedAsModelText is undefined and getRestrictedModelTextReason (restricted-model-text.utils.ts:19-27) returns no reason, leaving the row selectable. Both resource-picker.dialog.tsx:62-70 and team-layout.dialog.tsx:118-125 call getCachedResources once through useRetryablePromise with no refetch trigger other than the error/notReady retry, and the unstamped catalog is a normal available result, so it is never refetched until the dialog is reopened. The wait includes papi.dataProviders.get for the C# provider (main.ts:200-203), which can exceed 2 s while the provider registers; a failed fetch (undefined) produces the same fail-open. The existing test at model-text-restrictions.utils.test.ts:408-425 pins the unstamped-after-bound behavior but nothing covers the dialogs recovering.
Other findings in this file: #5, #6
| async function getCachedResources(): Promise<DblResourceCatalog> { | ||
| // Waits, boundedly, for the first restrictions fetch so a picker opened early is not fail-open; | ||
| // after that the restrictions are in memory and this adds nothing. | ||
| return applyModelTextRestrictionsToCatalogWithin( |
There was a problem hiding this comment.
#5 - low · checked and confirmed
Home, Get Resources and the Text Collection can take up to 2 seconds longer to load their resource lists at startup, waiting for a restriction flag they never read.
What happens: every platformGetResources.getCachedResources call now goes through applyModelTextRestrictionsToCatalogWithin with a 2000 ms wait (main.ts:389-393), whoever the caller is: home.web-view.tsx:70, get-resources.web-view.tsx:39 and scripture-text-grid.web-view.tsx:263 all reach it, and none of them reads isRestrictedAsModelText.
Why it matters: startup latency on the most common screens, paid only while the first restrictions fetch is outstanding.
Fix: Bound the wait only for callers that read isRestrictedAsModelText. Recommended: add an optional argument to 'platformGetResources.getCachedResources' (platform-get-resources.d.ts:219, with TSDoc) and to its handler in main.ts:386, so the 2000 ms applyModelTextRestrictionsToCatalogWithin wait runs only when it is set; pass it from resource-picker.dialog.tsx:62 and team-layout.dialog.tsx:122, and leave home.web-view.tsx, get-resources.web-view.tsx, scripture-text-grid.web-view.tsx and use-dbl-resource-catalog.hook.ts calling it bare (those rows are then returned without waiting, restrictions are still applied once known). Regenerate lib/papi-dts/papi.d.ts with npm run build:types, and add a test that a bare getCachedResources call resolves before a never-settling restrictions fetch while the opted-in call still waits (it must fail if the opt-in is removed).
How this was checked: getCachedResources (main.ts:386-394) wraps every call in applyModelTextRestrictionsToCatalogWithin(..., 2000), which runs restrictionsCache.getWithin alongside the catalog read in a Promise.all (model-text-restrictions.utils.ts:180-190). getWithin returns at once only when known is set; otherwise it races the first listModelTextRestrictions call (a lazy fetch via papi.dataProviders.get('platformGetResources.dblResourcesProvider'), main.ts:200-205) against the 2000 ms timeout. So until the first fetch lands, even a cache-hit catalog read is held back by up to 2 s. home.web-view.tsx:70, get-resources.web-view.tsx:39, scripture-text-grid.web-view.tsx:263 and use-dbl-resource-catalog.hook.ts:94 all call it with no argument, and none reads isRestrictedAsModelText. The only consumers of the flag are the resource picker (model-text-panel.web-view.tsx:172, which passes disableRestrictedModelTexts: true) and team-layout.dialog.tsx:105-107. The cost is limited to the first call(s) while the provider is slow, then it is free.
Other findings in this file: #1, #6
|
|
||
| const allMetadata = await getLocalProjectMetadata(); | ||
| return buildLocalNonDblResources(allMetadata, dblCatalog); | ||
| return await applyModelTextRestrictionsWithin( |
There was a problem hiding this comment.
#6 - low · checked and confirmed
At startup, the resource picker's Installed list of non-DBL resources waits as long as the model-text restrictions fetch takes, not the intended 2 seconds at most.
What happens: getLocalNonDblResources waits for the model-text restrictions three times in a row. getCachedResources (main.ts:409) waits up to MODEL_TEXT_RESTRICTIONS_WAIT_MS (2000). ensureInstalledFlagsSynced (main.ts:412) then runs syncModelTextRestrictionsAfterFlagSync (main.ts:307), whose sync(false) is ensureLoaded: it awaits the fetch already in flight, or starts a new one after a failure, with no time limit (model-text-restrictions.utils.ts:112-115). Only after that does applyModelTextRestrictionsWithin (main.ts:420-424) apply its own 2000 ms bound.
Why it matters: the 2-second bound does not limit this list. With a slow or failing backend at startup, the Installed section of the resource picker stays empty until the restrictions fetch settles.
Fix: Make getLocalNonDblResources wait for the model-text restrictions at most once, for MODEL_TEXT_RESTRICTIONS_WAIT_MS in total. Changing getCachedResources() at main.ts:409 to readCatalog() is not enough: ensureInstalledFlagsSynced() at main.ts:412 still awaits the restrictions fetch with no bound through syncModelTextRestrictionsAfterFlagSync (main.ts:307). Keep that unbounded restrictions step out of the awaited path here (for example by awaiting only the flag sync and leaving the restrictions sync in the background), and keep the single bounded applyModelTextRestrictionsWithin call at main.ts:420-424 so rows are still stamped. Add a test in extensions/src/platform-get-resources/ with a never-resolving restrictions fetch asserting the list resolves within about MODEL_TEXT_RESTRICTIONS_WAIT_MS; it must fail without the change.
How this was checked: getLocalNonDblResources waits on the restrictions three times in sequence: getCachedResources bounded to 2s (main.ts:409, 386-393), then ensureInstalledFlagsSynced (main.ts:412), whose sync step awaits the restrictions fetch with no bound (main.ts:307-310 via sync/ensureLoaded in model-text-restrictions.utils.ts:111-124), then applyModelTextRestrictionsWithin with a further 2s bound (main.ts:420-424). If the first fetch is still outstanding, the 2s bound is moot because the sync waits for it to finish; if it failed, getWithin starts a retry (utils.ts:126-137) and adds up to 2s more. The wait is new in this PR (the 420-424 call and the restrictions step in the sync). It only affects the startup window with a slow or failing backend.
Other findings in this file: #1, #5
| * @param flagSync `isRefreshRequested`: whether the sync was asked to refresh everything it can; | ||
| * `isAnyFlagChanged`: whether it changed any resource's flags. | ||
| */ | ||
| export function syncModelTextRestrictionsAfterFlagSync( |
There was a problem hiding this comment.
#2 - medium · checked and confirmed
A restricted Biblica text added mid-session outside the DBL catalog (copied in by hand, or installed by Paratext 9) stays selectable as a model text until the next install, uninstall or resource-flag refresh.
What happens: such a resource becomes a local non-DBL row whose dblEntryUid is its own projectId, so only the restrictions' projectIds can match it (model-text-restrictions.utils.ts:28-29). platform.onDidChangeProjects calls syncAfterInFlight(false) (main.ts:706), and syncFlags reports isAnyFlagChanged only when a catalog row's flags change (main.ts:271-272). syncModelTextRestrictionsAfterFlagSync therefore calls sync(false) (:155), which is ensureLoaded and returns the cached restrictions without asking the backend again, so projectIds never gains the new project.
Why it matters: the restriction goes unenforced for exactly the texts that only the backend's copyright rule can catch.
Fix: In the platform.onDidChangeProjects listener in extensions/src/platform-get-resources/src/main.ts (:700-707), refresh the restrictions as well as the flags. Call modelTextRestrictions.refresh() after syncAfterInFlight(false) has settled, so a sync that already started does not overwrite it with an older read. Do not switch the call to syncAfterInFlight(true), because that adds an updateAvailable backend round trip the adjacent comment excludes. Add a test that fails without the change: either extract the listener's body into a function in model-text-restrictions.utils.ts that can be exercised directly, or mount main.ts's activation with a mocked network event. The test should load the restrictions, then add a local row whose project id appears only in a second backend answer, fire the project-change event, and assert the row is stamped. If the comment above the listener documents sync cost, extend it to say the restrictions are refetched too.
How this was checked: The restrictions are matched for a local non-DBL row only through projectIds (model-text-restrictions.utils.ts:28-29), and the backend builds that list by scanning installed projects at call time (DblDownloadableDataProvider.cs:179-201). platform.onDidChangeProjects calls syncAfterInFlight(false) (main.ts:706). That sync reaches syncModelTextRestrictionsAfterFlagSync (main.ts:307), which calls sync(false) only when no refresh was requested and no catalog flag changed (utils.ts:155). sync(false) is ensureLoaded, which returns the cached value when it is known (utils.ts:112-115). A hand-copied non-catalog project has no catalog row, so syncFlags reports no change and the new project id never reaches the cache. getLocalNonDblResources then stamps from that stale cache via getWithin (main.ts:421-423, utils.ts:127). The stale list is replaced at the next install or uninstall that changes a flag, or the next platformGetResources.refreshResourceFlags call (main.ts:353, 621-623), each of which refetches.
| function CopyrightNoticeDetails({ notice, localizedStrings }: CopyrightNoticeDetailsProps) { | ||
| const opensInBrowserId = useId(); | ||
|
|
||
| if (notice.kind === 'notification') |
There was a problem hiding this comment.
#7 - low · checked and confirmed
For a "Notification:" copyright of a single paragraph, More info opens a window that shows only its title and a Close button.
What happens: CopyrightNotice.FromScrText builds details from the non-empty lines after the first (CopyrightNotice.cs:100-104), which is "" for a one-paragraph copyright. CopyrightNoticeDetails splits and filters that to no paragraphs (copyright-details-dialog.component.tsx:36-46). CopyrightNoticeBanner still renders the dialog with its More info trigger (copyright-notice-banner.component.tsx:102-110), and CopyrightNoticeIndicatorView still wraps its Info button in the dialog (copyright-notice-indicator.component.tsx:35-55). The banner test already builds such a notice (copyright-notice-banner.component.test.tsx:137) but never opens the dialog.
Why it matters: a dead-end window for any one-paragraph Notification copyright, in the banner and in a narrow Text Collection cell alike.
Fix: Add a shared predicate next to ShowableCopyrightNotice in copyright-notice-message.utils.tsx (for example hasCopyrightDetails(notice), true for restrictedLicense and for notification with non-empty notice.details) and use it at both sites. In CopyrightNoticeBanner (copyright-notice-banner.component.tsx:102-110), render the CopyrightDetailsDialog with its More info button only when it is true. In CopyrightNoticeIndicatorView (copyright-notice-indicator.component.tsx:35-55), when it is false render the Info Button directly inside TooltipTrigger with no dialog, keeping its aria-label and event.stopPropagation() onClick, so the tooltip still shows the notice. Add a banner test that a notification notice with details: '' shows no More info button, and an indicator test that the same notice shows the tooltip trigger but opens no dialog on click.
How this was checked: CopyrightNotice.FromScrText (c-sharp/Projects/CopyrightNotice.cs:96-111) builds Details from lines after the first, trimmed and non-empty, so a one-paragraph "Notification:" copyright yields "". CopyrightNoticeDetails (copyright-details-dialog.component.tsx:36-46) splits and filters that to zero paragraphs, so the dialog body is empty beneath the title (:107-112) and the Close footer. CopyrightNoticeBanner renders CopyrightDetailsDialog with its More info trigger unconditionally (copyright-notice-banner.component.tsx:102-110), and CopyrightNoticeIndicatorView wraps its button in the dialog unconditionally (copyright-notice-indicator.component.tsx:35-55). The empty-details notification is a real shape: the banner test already builds one with details '' (copyright-notice-banner.component.test.tsx:137) but never opens or asserts on the dialog. The indicator's tooltip shows the full notice sentence (indicator.component.tsx:56-58), so hiding its dialog would not hide the notice.
| @@ -0,0 +1,11 @@ | |||
| /** Windows Escape closes by itself, such as a cell's copyright details */ | |||
There was a problem hiding this comment.
#8 - low · checked and confirmed
In the Text Collection's chapter view, pressing Escape in a cell's right-click menu or zoom dropdown closes the whole chapter view instead of just the menu.
What happens: the grid listens for keydown on window in the capture phase (scripture-text-grid.web-view.tsx:237), which runs before Radix's own Escape handling, and calls stopPropagation. isEscapeForChapterContext exempts only targets inside DIALOG_SELECTOR, [role="dialog"], [role="alertdialog"] (chapter-context-escape.utils.ts:2). Radix menus use role="menu", so Escape pressed in one reaches handleCloseChapterContext.
Why it matters: the gap predates this change, which rewrote this function to exempt dialogs; any menu opened from a chapter-view cell is still affected.
Fix: In chapter-context-escape.utils.ts, extend the selector (DIALOG_SELECTOR) to also match [role="menu"] and rename it (for example ESCAPE_DISMISSIBLE_LAYER_SELECTOR) so it names any open layer that Escape dismisses first; update the comment on the constant and the TSDoc on isEscapeForChapterContext to say "window or menu". Add a menu case to the it.each in chapter-context-escape.utils.test.ts asserting Escape pressed on an item inside a role="menu" element returns false (this test fails without the selector change). Keep the keydown handler at scripture-text-grid.web-view.tsx:229-238 unchanged.
How this was checked: The window-level capture keydown listener in scripture-text-grid.web-view.tsx:229-238 calls stopPropagation on any Escape that isEscapeForChapterContext (chapter-context-escape.utils.ts:2-11) does not exempt, and only [role="dialog"], [role="alertdialog"] targets are exempt. Each ResourceCell with a usable state renders a Radix DropdownMenu for the right-click menu (resource-cell-view.component.tsx:477-492) and another for the zoom "options" dropdown (:425-459). Radix gives both content elements role="menu" and moves focus into them, so Escape there is swallowed before Radix's document-level Escape handler (the in-code comment at resource-cell-view.component.tsx:498 says "Radix handles Escape on the document") and the chapter split closes instead. Reachable whenever a chapter split is open in verse view and a menu is opened from any cell (verse column or chapter pane). Before this PR every Escape closed the split; the PR added the dialog exemption but not menus, so the gap is not a regression. No Select/listbox is rendered in these cells, so only role="menu" is needed.
Code Review Summary
Branch: pt-4731-biblica-texts-special-message
Base: origin/main
Date: 2026-09-24
Review model: Claude Opus 5.5
Files changed: 98. About 1,800 lines are production code; the rest is tests, stories, translations, test fixtures and generated files.
Summary
What users see. Biblica licenses some texts (NIV, NVI and others) as "reference text" resources only: they may not be translated or used as the basis for a new translation. When one of these resources is open, Simple and Power mode now show a dismissible banner with Biblica's wording.
Second feature (the ticket's nice-to-have). These texts are greyed out, with a reason, in the pickers. They can't be chosen as a model text (resource picker, Team Layout) or as the base of a new book (Manage Books "Create based on"). An existing selection is kept, and its banner shows.
How it works. One C# check,
BiblicaLicensing.IsRestricted, decides what is restricted. It applies to resources only: the DBL id must be on Matt T's list of 56 texts, or the copyright must say "Biblica, Inc." without "Biblica Open". That check reaches the UI by two paths:platformScripture.copyrightNotice.listModelTextRestrictions, which platform-get-resources stamps onto rows asisRestrictedAsModelText, andProjectSummary.IsRestrictedAsBasefor Manage Books.Suggested reading order
c-sharp/Projects/DigitalBibleLibrary/BiblicaLicensing.csIsRestricted: the id list, the copyright rule and the Open-text exemptions. Tests:BiblicaLicensingTests.cs, backed by a survey of 1,819 DBL texts inTestData/.c-sharp/Projects/CopyrightNotice.cs,ParatextProjectDataProvider.csnone/notification/restrictedLicensevalue, which is read-only. The TS type isCopyrightNoticeinplatform-scripture.d.ts.extensions/src/platform-scripture-editor/src/copyright-notice/project-copyright-notice.component.tsxreads the setting and handles dismissal. It renderscopyright-notice-banner,copyright-details-dialogand, in narrow cells,copyright-notice-indicator.platform-scripture-editor.web-view.tsx,model-text-panel.*,resource-text-panel.*,scripture-text-grid/resource-cell*chapter-context-escape.utils.ts).ModelTextRestrictions.cs→platform-get-resources/src/model-text-restrictions.utils.ts→src/renderer/components/dialogs/restricted-model-text.utils.ts→resource-picker.dialog.tsx,team-layout.*ResourcePickerDialog's newgetDisabledReason.c-sharp/ManageBooks/ProjectSummary.cs→manage-books-dialog.component.tsx→ProjectSelectorIsRestrictedAsBasebecomes adisabledReason.ProjectSelectordisabled rows can now show their tooltip.Safe to skim:
lib/*/dist/,lib/papi-dts/papi.d.ts.localizedStrings.json.TestData/*.jsonfiles.adr-biblica-license-notice-computed-setting.Decisions worth a second look
How to try it
Bundled Extensions/platform-scripture-editor/CopyrightNoticeBannerhas 7 stories, including the dialog, a right-to-left name and a "Notification:" text.Full review notes: API changes, findings, interview and quality checks
API Changes
lib/platform-bible-utils(resources.model.ts):DblResourceDatagains an optionalisRestrictedAsModelText?: booleanfield.lib/platform-bible-react(experimentalResourcePickerDialog):ResourcePickerDialogPropsgains an optionalgetDisabledReason?: (resource: DblResourceData) => string | undefined. When it returns a reason for a row in the "Installed" or "Available to Download" section, that row is dimmed, shows a lock icon and a tooltip, and cannot be picked.lib/platform-bible-react(experimentalProjectSelector): no signature change. Behavior changes:tw:data-[disabled=true]:pointer-events-auto), so theirdisabledReasontooltip can open.disabledReasonis exposed to screen readers througharia-describedby.ProjectSelectorProject.disabledReasonwas updated to match.isBoundButClosedJSDoc says so.papi.d.ts(renderer/components/dialogs/dialog-definition.model, reached through the dialog types):ResourcePickerDialogOptionsgains an optionaldisableRestrictedModelTexts?: boolean. It matches the source insrc/renderer/components/dialogs/dialog-definition.model.tsand looks regenerated, not hand-edited.extensions/src/platform-scripture/src/types/platform-scripture.d.ts:CopyrightNotice, a union overkind:none,notificationandrestrictedLicense.CopyrightNoticeis added to thepapi-shared-typesimport.'platformScripture.copyrightNotice': CopyrightNoticeinProjectSettingTypes. It is computed and read-only; C# throwsInvalidOperationExceptionon a write.extensions/src/platform-get-resources/src/types/platform-get-resources.d.ts:ModelTextRestrictions({ dblIds: string[]; projectIds: string[] }).listModelTextRestrictions(): Promise<ModelTextRestrictions>onIDblResourcesProvider.getCachedResourcesandgetLocalNonDblResources: docs now say each row carriesisRestrictedAsModelText, and that a read can wait up to 2 seconds for the first restrictions fetch.%restrictedModelOrBaseText_disabledReason%(en, es).Findings
Critical — Must address before merge
None.
Important — Should address before merge
CopyrightNoticeandplatformScripture.copyrightNoticeTSDoc said a non-resource project always getsnone, but a "Notification:" copyright shows on any project, as in Paratext 9. (fixed during review: both docs now sayrestrictedLicenseapplies only to resources,notificationto any project whose copyright starts with "Notification:", and otherwisenone.)The picker tooltip, "This text is disabled from being selected because of licensing terms which prohibit its use as a model or base for a new translation.", is one 25-word sentence, longer than the Tooltips guideline allows.(Author kept it: this is Ian's wording from the ticket.)ProjectSelectordisabled-reason line usedtw:text-muted-foregroundon the tooltip's invertedbg-foreground: about 4.0:1 contrast in light theme and 2.5:1 in dark, below AA. This branch is the first to show text in that line. (fixed during review: droppedtw:text-muted-foregroundso the reason inherits the tooltip's own foreground; italics kept;pbrrebuilt.)Author response: the author fixed three of the four during the interview and kept Ian's tooltip wording on purpose.
Minor — Consider
BiblicaLicensing.IsOnRestrictedListwaspublic, and its doc described picker callers that don't exist. (fixed during review: madeinternaland reworded the doc; the tests reach it throughInternalsVisibleTo.)messageFormatOfduplicated the choice of string key per notice kind informatCopyrightNoticeMessage. (fixed during review: one exportedgetCopyrightNoticeMessageFormathelper is used by both. A deliberate break of the re-measure dependency makes its test fail.)%resourcePicker_restrictedModelText_tooltip%was also used in Manage Books, which is not a resource picker, and it is a screen-reader description as well as a tooltip. (fixed during review: renamed to%restrictedModelOrBaseText_disabledReason%in en and es and at every call site. The constant was renamed to match. A repo-wide grep for the old key is empty.)M3 (other part): the flag has three names,(Author kept them: each names the role in its own context, a resource row "as a model text" and a project "as a base". Unifying them would mean renaming across C# and TypeScript for little gain.)isRestrictedAsModelText,isRestrictedAsBaseanddisableRestrictedModelTexts.ResourcePickerDialogWithDisabledRowsstory disabled ESV and NLT, which aren't Biblica texts, with a hardcoded copy of the Biblica reason. (fixed during review: it now uses the sample reason "Not available for this translation", documented as consumer-supplied.)BIBLICA_PERMISSIONS_URLlived in the message utils rather than the folder's const file, and the folder mixed.utiland.utilsfile names. (fixed during review: moved the URL tocopyright-notice.const.ts, and renamedopen-in-browser.util.tstoopen-in-browser.utils.tswithgit mv.)ProjectRowView,hasDisabledReasonrepeated a term ofhasExtraTooltipContent. (fixed during review:hasDisabledReasonis declared first and reused.)createModelTextRestrictionsCacheusedthis, so destructuringsyncorgetWithinwould break. (fixed during review: they are now closure-local functions. A new test destructures and calls them; it failed before the change.)M8: Enhanced Resources already has its own copyright ribbon and dialog, so this is a second, parallel copyright UI.(Author: for information only. Consolidating would mean moving both intoplatform-bible-react, which is out of scope; it's a follow-up if a third surface appears.)M9: The pickers treat restricted texts differently. The resource picker shows a lock icon, the row is reachable by keyboard and the tooltip opens on focus. Manage Books' "Based on"(Author: fixing it means changing how the sharedProjectSelectorrow has no icon and a hover-only tooltip, and cmdk skips disabled items with the keyboard.ProjectSelectorhandles disabled rows for every consumer. Follow-up.)M10: The narrow-cell indicator's tooltip shows the full banner, about 50 words.(Author kept it: in a narrow Text Collection cell the tooltip is the only place the banner wording appears. The dialog shows Biblica's terms, a different text, so shortening it would drop part of the required notice.)M11: Once a pane's banner is dismissed, the only way back to it is to switch texts.(Author: deliberate Paratext 9 parity. Switching to another text and back shows the banner again. Adding a "see it again" control would invent a requirement.)ResourceRowCellsandDisabledResourceRowused physical-direction classes. (fixed during review: switched tope-*,ps-*andtext-end.)NotificationDetailsOpenstory.)main.tswiring was untested, both the "resync when a flag changed" decision and the 2-second wait-then-stamp. (fixed during review: extracted intosyncModelTextRestrictionsAfterFlagSync,applyModelTextRestrictionsWithinandapplyModelTextRestrictionsToCatalogWithininmodel-text-restrictions.utils.ts, with 7 new tests, each of which fails when its code is deliberately broken.main.tsnow just calls them.)M17:(Author kept it: the wait only happens until the first fetch lands, a local C# call that normally takes milliseconds, and 2 seconds is the ceiling. Without it, a picker opened early could offer a restricted text; making it opt-in would add API for a startup-only edge case.)getCachedResourcesandgetLocalNonDblResourcescan wait up to 2 seconds for the first restrictions fetch, even for callers that don't read the flag.ProjectSelectorrows now take pointer events, so a disabled, bound-but-closed row's "Open" button would become clickable. (fixed during review: the Open button is not rendered on a disabled row. A new test failed before the fix.)Author response: the author asked for the reviewer's recommendation on each item, accepted fixing M1–M7, M12–M16 and M18, including the key rename in M3, and chose to skip M8–M11 and M17 for the reasons given.
Template Propagation
Shared Regions Modified
None.
Extension Config Changes
extensions/src/platform-scripture/contributions/localizedStrings.json: extension-specific, no propagation neededextensions/src/platform-scripture/contributions/projectSettings.json: extension-specific, no propagation neededPositive Observations
PB_IS_PUBLISHEDexactly: a get branch, a rejected set, aProjectSettingsNamesconstant and a hiddenprojectSettings.jsoncontribution. The C# wire shape is pinned byCopyrightNotice_SerializesToTheShapeTheFrontEndReads.CopyrightNoticetells consumers to treat an unknownkindasnone, so the union can grow safely.ResizeObservercatches up when the tab is shown, as.claude/rules/cross-view-sync-hidden-views.mdrequires. A test pins it.<bdi>, and the permissions URL is forced left-to-right.dir="auto".SyncBlockedBannerand sits outside the scrolling area. It clamps to two lines, offering Show more only when the text overflows. Narrow Text Collection cells get an indicator instead.copyright-notice-strings.test.tschecks that each locale has every key with the same placeholders.ListModelTextRestrictions;IsRestrictedAsBase.resolveModelTextProjectIdis now shared by the model-text panel and its web view, andResourceRowCellsavoids duplicating the picker row markup.adr-biblica-license-notice-computed-setting, is in byte-order slug position and records the rejected alternatives.Interview Notes
Purpose (author): "Show a banner on traditionally published Biblica texts explaining the copyright information."
Design explanation (Step 3.3): The author explained that Matt T provided the list of resources that must show the Biblica notice, and that the list was backed up programmatically by checking each resource's copyright; a text on the list gets the banner. The assistant filled in the code-level detail and the author accepted it:
The copyright rule matched exactly the 55 listed texts among the 1,819 surveyed.
Scope decision, Paratext 9 parity: The author first expected the Biblica notice to show on editable projects too. After discussing it, they agreed to keep Paratext 9's behaviour:
The TSDoc was corrected to say exactly this.
Deliberate choices for the reviewer:
ProjectSummary.IsRestrictedAsBase.%restrictedModelOrBaseText_disabledReason%exists in en and es only, like the otherresourcePicker_*keys.platform.openWindow. The web views'allowPopupswas deliberately not widened.Unresolved items: none. The author could explain every finding.
In-Review Quality Check
Every check passes, and none of them needed a fix.
npm run typecheck: pass.npm run linttakes over 40 minutes locally, so it was scoped to the changed files.--check: clean on all changed files.npm test: pass.web-view.component.test.tsx,comment-list.storiesandcomment-thread.stories.dotnet test c-sharp-tests/: 2,401 passed, 0 failed.Rebased onto main (
bbd52be5ac3, 16 new commits). Nine files conflicted, and main's changes were kept in each:ProjectSelector's indicator tooltips.fullName.Two of main's tests needed updating for this branch: one
ProjectSelectortest's query, and a source-contract test that now pins the banner between the resource selector and the zoom area.After the rebase:
Suggested Review Focus
BiblicaLicensing.IsRestrictedLicense:The author explained the list at a product level; walk through the fallback rule together.
Paratext 9 parity scope. Is showing "Notification:" banners on editable projects, and the Biblica notice on resources only, the product decision we want?
Second feature: disabling restricted texts in the model and base pickers. Review its public API (
getDisabledReason,disableRestrictedModelTexts,listModelTextRestrictions,isRestrictedAsModelText), and confirm that UI-only enforcement is acceptable.ProjectSelectordisabled rows now take pointer events. This is a shared component, so check other consumers.The up-to-2-second first wait in
getCachedResourcesandgetLocalNonDblResources(M17).Follow-ups to file. The Manage Books keyboard reachability for disabled rows (M9), and a shared copyright UI in
platform-bible-react(M8).AI-assisted — Claude Code (Claude Opus 5.5)
🤖 Generated with Claude Code
This change is