PT-4403: Show honest book-not-found message in resource panels - #2706
PT-4403: Show honest book-not-found message in resource panels#2706jolierabideau wants to merge 2 commits into
Conversation
jolierabideau
changed the title
The Bible text tab (Simple mode) collapsed every PDP error to `undefined`, so a book missing from the selected resource left `Editorial` rendering the previous chapter's content or its "Enter some Scripture..." placeholder — an edit prompt in a read-only resource. Detect the book-not-found error and render the same localized message the editable editor already shows for read-only projects. The resource-selector dropdown stays mounted so the user can switch to a resource that has the book, and the message sits outside the `dir` wrapper since it is UI-locale text rather than resource text. Other PDP errors are now logged instead of silently swallowed. Extracts the detection (and the regex coupled to MissingBookException.cs) out of platform-scripture-editor.web-view.tsx into `isBookNotFoundError` in platform-scripture-editor.utils.ts, so both tabs share one source of truth and the check is unit-testable. No new localized strings: reuses %webView_platformScriptureEditor_error_bookNotFoundResource%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Render the missing-book message through the shared `EmptyState` component in all three Scripture panels, so the same sentence is worded, styled, and announced to screen readers identically. Gains the `role="status"` live region the swapped-in-place region needs. - Extend the fix to the Model text panel, which had the identical bug via a different path: awaiting `getChapterUSJ` rejects rather than returning a `PlatformError`, so the load's catch cleared the USJ and still rendered the read-only editor. - Broaden `isBookNotFoundError` to recognize both shapes the PDPs surface (a returned `PlatformError` and a thrown `Error`), keeping one source of truth for the check. - Branch on `isBookNotFoundError` before logging: a book the resource lacks is ordinary navigation, so it logs at `debug`, while genuine PDP failures keep `error`. Previously the two tabs disagreed (`warn` vs `error`) and logged expected navigation as a fault. - Collapse the resource panel to a single `useMemo` deriving both `usjFromPdp` and `bookExists`, matching the editable editor's shape and removing a duplicate `isPlatformError` evaluation. - Pin the C#/TypeScript contract with `MissingBookExceptionTests`, so rewording `MissingBookException`'s message fails a test instead of silently breaking detection. - Add tests for the Model text panel's render fork and for the thrown-`Error` shape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-URL: <session URL>
5dfaf29 to
56d0254
Compare
Review summaryThe change is real and the diagnosis is right — a book missing from a resource genuinely does come back from the PDP as a Three things I'd hold the merge for:
Everything below is anchored to a line this PR changed. I'll add the inline comments separately. Blocking1.
Concretely: on a resource lacking Genesis, navigate GEN → MAT. This isn't a hypothesis about the hook — 2. Same missing guard — and here 3.
When the effective model text switches from resource A (which lacks the book) to resource B: the render at 461 passes (B is defined), the spinner guard at 466 is skipped ( 4. This panel resolves project references too — Important5. The accessibility rationale here is contradicted by guidance PT-4111 added. From
That text was added in commit
6. Two standards prohibit this pattern, and the PR now pins it with a test:
However — I checked whether the prescribed alternative actually works, and it currently doesn't: 7. "there is no structured error code to key off" isn't the real reason — 8.
9. All four new panel tests reject with To be fair: 10. "awaiting 11.
12. On the surface sweep flagged in the PR body: Minor13. 14. 15. 16. 17. 18. 19. 20. PR description & commits — Medium (convention). Three unfilled placeholders shipped: the body ends with the literal Checked and explicitly not an issueChased these and they don't hold — recording them so they don't come back in another review round:
Items 1–4 are the ones I'd block on; the rest are yours to weigh. AI-assisted review — session |
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
Thanks, Jolie, for improving the missing book error messaging! I posted inline comments to make it easier to follow what is being talked about, but feel free to respond in bulk.
@katherinejensen00 reviewed 9 files and all commit messages, and made 21 comments.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on jolierabideau).
-- commits at r1:
NIT 20. PR description & commits — Medium (convention). Three unfilled placeholders shipped: the body ends with the literal AI-assisted — session; commit 56d0254 ends with Session-URL: ; commit ca61b14 has no Session-URL: line. CLAUDE.md → "Git & PR Conventions" requires real URLs. Also still in the published body: "Screenshot files are in .review/screenshots/ (gitignored) — attach before-full-window.png and after-full-window.png here", under a Screenshots table with no screenshots.
c-sharp-tests/MissingBookExceptionTests.cs line 5 at r1 (raw file):
using Paranext.DataProvider; namespace TestParanextDataProvider
NIT 17. c-sharp-tests/MissingBookExceptionTests.cs:5 — Low. Block-scoped namespace; .editorconfig:19 sets csharp_style_namespace_declarations = file_scoped:warning.
c-sharp-tests/MissingBookExceptionTests.cs line 12 at r1 (raw file):
/// <remarks> /// The Scripture editor extension detects "this book is not in this project/resource" by /// pattern-matching the message text of the error the PDP returns — there is no structured
NIT 7. c-sharp-tests/MissingBookExceptionTests.cs:12 — Severity: Low-Medium
▎ "there is no structured error code to key off" isn't the real reason — c-sharp/PlatformErrorCodes.cs exists with NotFound, and c-sharp-tests/ManageBooks/PlatformErrorCodesTests.cs:14 names MissingBookException.cs as its neighbour. The accurate reason is the one above: the code is discarded in the router before it reaches TS. Worth correcting, because this comment is the artifact that will justify the next string-match.
c-sharp-tests/MissingBookExceptionTests.cs line 29 at r1 (raw file):
/// <c>platform-scripture-editor.utils.ts</c>. /// </summary> private const string ExtensionBookNotFoundRegex = @"Book number \d+ not found in project";
NIT 15. c-sharp-tests/MissingBookExceptionTests.cs:29 — Low. The pin is weaker than the docstring claims. Editing the TS regex is caught (by utils.test.ts:2597), but a coordinated edit of the regex and its test literal still drifts silently. Also, \d isn't the same in both engines (.NET matches Unicode Nd, JS matches [0-9]), so "character-for-character identical" doesn't guarantee semantically identical.
c-sharp-tests/MissingBookExceptionTests.cs line 66 at r1 (raw file):
[Test] public void BookNumAndProjectId_ArePreserved()
NIT 16. c-sharp-tests/MissingBookExceptionTests.cs:66 — Low. BookNumAndProjectId_ArePreserved is the exact pattern Testing-Guide.md:242 prohibits ("Constructor assignments — new User(name).name === name — Tautological"). I checked: MissingBookException.BookNum and .ProjectId are read nowhere in the repo. Separately, :32 is strictly contained in :51's {1, 40, 66} loop. Two of the four tests can go.
extensions/src/platform-scripture-editor/src/model-text-panel.component.tsx line 196 at r1 (raw file):
// `undefined` means "not yet fetched" so we can show the loading state, matching the original. const [isUsjLoading, setIsUsjLoading] = useState(false); // A book missing from the resource makes `getChapterUSJ` reject rather than return empty USJ.
NIT 13. model-text-panel.component.tsx:196 — Low. Pre-existing, but adjacent and now claimed by this comment: usj is not cleared when a load starts, so the usj === undefined && isUsjLoading guard at 466 is false and the editor renders the previous chapter for the whole round trip. Same root cause as #1/#3.
extensions/src/platform-scripture-editor/src/model-text-panel.component.tsx line 200 at r1 (raw file):
// read-only editor, which then shows either the previous chapter's text or its "enter some // Scripture" placeholder — an edit prompt in a resource the user cannot edit. const [bookExists, setBookExists] = useState(true);
- extensions/src/platform-scripture-editor/src/model-text-panel.component.tsx:200 — Severity: High
▎ bookExists is never reset when resourceProjectId changes, and the reset that does exist runs too late.
▎
▎ setIsUsjLoading(true) is at line 208, inside a passive effect — it runs after paint. So when the effective model text switches from resource A (which lacks the book) to resource B: the render at 461 passes (B is defined), the spinner guard at 466 is skipped (isUsjLoading is still false), and 478 fires — painting "This book does not exist in this resource" about resource B before a single byte has been requested from it, and pushing that false claim into a role="status" region. One line fixes it: setBookExists(true) alongside line 208.
extensions/src/platform-scripture-editor/src/model-text-panel.component.tsx line 226 at r1 (raw file):
// logged quietly; every other failure is a real error. Matches both Scripture tabs. const bookNotFound = isBookNotFoundError(e); if (bookNotFound) logger?.debug(`Book not found in model text: ${getErrorMessage(e)}`);
NIT 11. extensions/src/platform-scripture-editor/src/model-text-panel.component.tsx:226 — Severity: Low-Medium
▎ logger.debug is compiled out of packaged builds — src/main/global-this.model.ts:27 sets the level to isPackaged ? 'info' : 'debug'. So in production there is no trace of a book-not-found at all. That may well be what you want, but the PR's verification note ("verified in main.log: the new messages appear at [debug]") was necessarily done in a dev build and doesn't demonstrate the packaged behavior. Worth a deliberate choice between debug and info.
extensions/src/platform-scripture-editor/src/model-text-panel.component.tsx line 226 at r1 (raw file):
// logged quietly; every other failure is a real error. Matches both Scripture tabs. const bookNotFound = isBookNotFoundError(e); if (bookNotFound) logger?.debug(`Book not found in model text: ${getErrorMessage(e)}`);
NIT 14. model-text-panel.component.tsx:226 — Low. logger is now used inside the effect but isn't in the deps; it's covered by a blanket eslint-disable whose comment only justifies excluding scrRef.verseNum. Safe in practice (stable module object) — worth extending that comment rather than leaving it implied.
extensions/src/platform-scripture-editor/src/model-text-panel.component.tsx line 228 at r1 (raw file):
if (bookNotFound) logger?.debug(`Book not found in model text: ${getErrorMessage(e)}`); else logger?.error(`Error getting USJ for the model text panel: ${getErrorMessage(e)}`); setBookExists(!bookNotFound);
NIT 8. extensions/src/platform-scripture-editor/src/model-text-panel.component.tsx:228 — Severity: Medium
▎ setBookExists(!bookNotFound) means every non-book-not-found failure sets bookExists = true, falls through to the read-only editor with usj === undefined, and shows the "Enter some Scripture…" prompt — the exact dishonesty your comment at lines 196-199 describes, for a transient network error on a resource the user cannot edit. The new test at model-text-panel.component.test.tsx:321 then asserts findByTestId('editorial') for a 'Network request failed' rejection, which pins that behavior as correct. The negative control is worth keeping; consider asserting a neutral/error state rather than the editor.
extensions/src/platform-scripture-editor/src/model-text-panel.component.tsx line 228 at r1 (raw file):
if (bookNotFound) logger?.debug(`Book not found in model text: ${getErrorMessage(e)}`); else logger?.error(`Error getting USJ for the model text panel: ${getErrorMessage(e)}`); setBookExists(!bookNotFound);
NIT 9. extensions/src/platform-scripture-editor/src/model-text-panel.component.test.tsx:288 — Severity: Medium
▎ All four new panel tests reject with new Error(…). Production rejects with a PlatformError plain object — network.service.ts:285 throws newPlatformError(...), and every return path in platform-error.ts:141-176 is an object literal, never an Error instance. So these tests exercise the instanceof Error clause, and the clause that actually runs in the app has no panel-level coverage.
▎
▎ To be fair to the PR: platform-scripture-editor.utils.test.ts:2597 does cover the PlatformError shape, so the predicate itself is tested — this is test fidelity, not a coverage hole. Swapping the mocks to newPlatformError('JSON-RPC Request error (-32000): Book number 1 not found in project project-web.') closes it. (Testing-Guide.md:186 — "over-mocking that hides real integration issues, giving false confidence.")
extensions/src/platform-scripture-editor/src/model-text-panel.component.tsx line 478 at r1 (raw file):
// Scripture tabs, so the identical sentence is worded, styled, and announced to screen readers // the same way in every panel that can hit this state. if (!bookExists) {
NIT 19. model-text-panel.component.tsx:478 — Low. The missing-book branch drops the panel header, so the user can't see which model text is missing the book, and the 42px tab alignment goes with it.
extensions/src/platform-scripture-editor/src/model-text-panel.component.tsx line 485 at r1 (raw file):
className="tw:text-center" message={ localizedStrings['%webView_platformScriptureEditor_error_bookNotFoundResource%'] ?? ''
NIT 18. model-text-panel.component.tsx:485 — Low. ?? '' renders a blank
if the key is missing. The other two call sites degrade differently (raw %key%). Worth one consistent choice across the three.
extensions/src/platform-scripture-editor/src/platform-scripture-editor.utils.ts line 1167 at r1 (raw file):
// `c-sharp-tests/MissingBookExceptionTests.cs` duplicates this pattern and fails if that message is // reworded, so the two sides cannot drift silently — update both together. const BOOK_NOT_FOUND_REGEX = /Book number \d+ not found in project/;
- extensions/src/platform-scripture-editor/src/platform-scripture-editor.utils.ts:1167 — Severity: Medium
▎ Two standards prohibit this pattern, and the PR now pins it with a test.
▎
▎ Code-Style-Guide.md:238-255 — "Never compare variables against hardcoded English strings", with the literal counter-example if (errorMessage.includes("not found")) → if (errorCode === ErrorCode.NotFound), and "This applies to … string includes/matching." Paranext-Core-Patterns.md:478 — "String-matching on ex.Message in TS — read the platformErrorCode property off the caught PlatformError instead."
▎
▎ However — I checked whether the prescribed alternative actually works, and it currently doesn't: src/main/services/rpc-server.ts:287 rebuilds every forwarded error as createErrorResponse(message, code, id), and src/shared/data/rpc.model.ts:114 emits { code, message } with no data field, so platformErrorCode is stripped before network.service.ts:274 can read it. So keeping the regex is defensible today — but that's the reason to record, and it deserves its own ticket. Please don't take this as a request to change the approach in this PR.
extensions/src/platform-scripture-editor/src/platform-scripture-editor.utils.ts line 1175 at r1 (raw file):
* * Handles both shapes the scripture PDPs surface, because the two call styles fail differently: * `useProjectData` hands back a `PlatformError` in place of the data, while awaiting
NIT 10. extensions/src/platform-scripture-editor/src/platform-scripture-editor.utils.ts:1175 — Severity: Low-Medium
▎ "awaiting getChapterUSJ directly rejects with a thrown Error" is not accurate, per #9. The same claim appears in the test name at platform-scripture-editor.utils.test.ts:2617, in the PR body's "Suggested Review Focus", and in commit 56d0254. The instanceof Error branch is harmless defensive code, but the comment presents it as the production path.
extensions/src/platform-scripture-editor/src/platform-scripture-editor.utils.ts line 1182 at r1 (raw file):
* its methods, which may be a `PlatformError` or an `Error`. */ export function isBookNotFoundError(possibleError: unknown): boolean {
NIT 12. extensions/src/platform-scripture-editor/src/platform-scripture-editor.utils.ts:1182 — Severity: Low-Medium
▎ On the surface sweep you flagged: scripture-text-grid/resource-cell.utils.ts:26 maps any PlatformError to 'failed', which the grid renders as a download failure. So after this PR, a book missing from a resource reads "This book does not exist in this resource" in three panels and "Resource unavailable / download failed" in the grid. Now that isBookNotFoundError is exported and shared, plugging in a fourth consumer is a few lines — worth doing here or splitting out with a ticket, but not leaving as an open question.
extensions/src/platform-scripture-editor/src/platform-scripture-editor.web-view.tsx line 1296 at r1 (raw file):
// A book missing from the project is ordinary navigation, not a fault, so it is logged // quietly; every other PDP failure is a real error. Both Scripture tabs branch the same way. const bookNotFound = isBookNotFoundError(usjFromPdpPossiblyError);
- extensions/src/platform-scripture-editor/src/platform-scripture-editor.web-view.tsx:1296 — Severity: High
▎ Same missing guard, and here isUsjFromPdpLoading is already destructured at line 1273 and already used for this purpose at line 1322 — it's just not applied to bookExists. Same failure: navigate from a missing book to a present one and BookNotAvailableView / the resource message stays up over the loading content.
extensions/src/platform-scripture-editor/src/resource-text-panel.web-view.tsx line 434 at r1 (raw file):
// chapter's content or its "enter some scripture" placeholder — neither is honest for a read-only // resource. Derived in one pass, like the editable Scripture tab. const [usjFromPdp, bookExists] = useMemo<[Usj | undefined, boolean]>(() => {
- extensions/src/platform-scripture-editor/src/resource-text-panel.web-view.tsx:434 — Severity: High
▎ bookExists is derived from a possibly-stale PDP value, so the message outlives the condition.
▎
▎ useProjectData's underlying useData does not reset its value when the selector changes — it keeps the previous selector's value until the new subscription's first update lands. The isLoading flag returned at line 412 is discarded here. Concretely: on a resource lacking Genesis, navigate GEN → MAT. usjPossiblyError still holds Genesis's PlatformError, so bookExists stays false and the panel keeps asserting "This book does not exist in this resource" about Matthew for the whole round trip.
▎
▎ This isn't a hypothesis about the hook — platform-scripture-editor.web-view.tsx:1313-1319 documents exactly this trap in your own words ("doesn't reset its value back to the default when the selector … changes"), and guards isBlankChapter with isUsjFromPdpLoading on the next line. Please destructure the third tuple element here and gate the memo the same way.
extensions/src/platform-scripture-editor/src/resource-text-panel.web-view.tsx line 658 at r1 (raw file):
that does contain the book. The message sits outside the `dir` wrapper because it is UI-locale text, not resource text. `EmptyState` (rather than bare text) because this region swaps content in place as the user navigates, so its `role="status"` is what
- extensions/src/platform-scripture-editor/src/resource-text-panel.web-view.tsx:658 — Severity: Medium
▎ The accessibility rationale here is contradicted by guidance PT-4111 added.
▎
▎ lib/platform-bible-react/src/stories/shadcn-ui/empty.stories.tsx:214: "assistive tech announces mutations to a live region that is already in the accessibility tree. Mounting the region and its text in one commit … typically announces nothing in NVDA or JAWS. Keep the role="status" element mounted across the transition and swap only its text." I traced that text to commit 35a09c0, "fix: address PT-4111 review findings" — #2691, the PR this one rebased onto.
▎
▎ EmptyState mounts
together with its text in a branch that unmounts the editor, so per that guidance it likely announces nothing. BookNotAvailableView has the same structural issue and compensates with the part that actually works — document.hasFocus() → regionRef.focus() on a tabIndex={-1} wrapper (book-not-available-view.component.tsx:82-89) — which none of the three new surfaces have. Either add the focus repair, or soften the comments. Right now three comments assert something the repo says is false. Same note applies at platform-scripture-editor.web-view.tsx:1979 and model-text-panel.component.tsx:476.
extensions/src/platform-scripture-editor/src/resource-text-panel.web-view.tsx line 676 at r1 (raw file):
className="tw:text-center" message={ localizedStrings['%webView_platformScriptureEditor_error_bookNotFoundResource%']
- extensions/src/platform-scripture-editor/src/resource-text-panel.web-view.tsx:676 — Severity: Medium-High
▎ This panel resolves project references too — isProjectReference(selectedRef) → resourceProjectId = selectedRef.id at line 344 — but the message is hardcoded to %…bookNotFoundResource%, i.e. "This book does not exist in this resource." When the selection is a project, that's the wrong noun. %webView_platformScriptureEditor_error_bookNotFoundProject% already exists in this extension's localizedStrings.json in both locales, and the editable editor solves this by branching on platform.isPublished. For a PR about telling the user the truth, this one is worth catching.
2 participants
Code Review Summary
Branch:
pt-4403-honest-message-when-book-does-not-existBase:
origin/mainDate: 2026-08-21
Review model: Claude Opus 5
Files changed: 8
Overview
A book missing from the selected resource comes back from the scripture PDP as an error, not as
empty USJ. Every panel that displayed resource text collapsed that error to "no data" and then
rendered the read-only editor anyway — so the user saw either the previous chapter's text or an
"Enter some Scripture…" placeholder. Both are dishonest: the first is stale content presented as
current, the second is an edit prompt in a resource the user cannot edit.
This change detects the book-not-found error and renders the existing localized message
(
%webView_platformScriptureEditor_error_bookNotFoundResource%) instead. The detection — and theregex coupled to
MissingBookException.cs— is extracted intoisBookNotFoundErrorinplatform-scripture-editor.utils.tsso all three Scripture surfaces share one unit-testable sourceof truth. The resource selector stays mounted so the user can switch to a resource that has the
book, and the message sits outside the
dirwrapper because it is UI-locale text, not resource text.During review the fix was extended from the Bible text tab to the Model text panel, which had
the identical bug via a different code path, and the message was converged onto the shared
EmptyStatecomponent in the resource panels so the same sentence is worded, styled, and announcedto screen readers identically everywhere.
Rebased onto PT-4111 (#2691)
#2691 landed first and rewrote the editable editor's
!bookExistsbranch into a mode-awareBookNotAvailableView(Power zero-state with a Manage books action, Simple "ask your projectadministrator" copy) behind an
isResourcefork. This PR was rebased on top of it and its ownEmptyStatewrapper for that branch was dropped as superseded — #2691's version is strictlyricher and is what ships.
What survives the rebase is everything #2691 does not cover:
panel), which PT-4111: Show a book-not-available view with a Manage books action #2691 never touches;
isBookNotFoundErrorinplatform-scripture-editor.utils.ts, replacing the editor's localbookNotFoundRegex, so all surfaces share one unit-testable predicate;debugfor a book the text simply lacks,errorretained for genuine PDPfaults);
MissingBookExceptionTests, pinning the C#↔TS contract.Two small edits close the consistency gap #2691 left, which NN 5A ("consistent across all tabs")
calls for directly:
platform-scripture-editor.web-view.tsx— theisResourcebranch rendered bare text in a<div>with no live region, the only missing-book surface that announced nothing. NowEmptyState, matching the resource panels.book-not-available-view.component.tsx— Simple mode's<span>now carriestw:text-sm tw:text-muted-foreground, matchingEmptyDescriptionin its own Power branch andEmptyStateeverywhere else. It stays aspanrather than becoming anEmptyState: therole="status"live region and the focus repair live on the wrapperdiv, andEmptyStatecannot accept either, so converting it would have been an accessibility regression. Typography
was the only thing actually diverging.
API Changes
None. No public API surfaces changed.
lib/platform-bible-react/,lib/platform-bible-utils/: unchangedlib/papi-dts/papi.d.ts: unchanged (no regeneration needed; no hand edits)extensions/src/*/src/types/*.d.ts: unchangedisBookNotFoundErroris extension-internal (not a declaration file, notre-exported through
@papi/), consumed only withinplatform-scripture-editor.Findings
Critical — Must address before merge
None.
Important — Should address before merge
<div>, skipping theEmptyStatecomponent (
resource-text-panel.web-view.tsx). The region swaps content in place as theuser navigates, so with no live region a screen reader user got silence where scripture used
to be. Flagged independently by both the UX and style passes; a sibling web view in the same
extension (
scripture-text-grid.web-view.tsx:545) already usedEmptyState.(fixed during review: replaced with
<EmptyState id="resource-text-panel-book-not-found" className="tw:text-center" />, gainingrole="status", the standardtw:text-sm tw:text-muted-foregroundtreatment, and adata-testid. Verified live in therunning app: the rendered node is
<p role="status" data-testid="resource-text-panel-book-not-found">.)[Author response: Author chose to adopt
EmptyStateimmediately. The same treatment was thenapplied to the editable editor and the Model text panel so all three panels match.]
Minor — Consider
logger.warnin the resource panel vslogger.errorin the editable editor for the same class of PDP failure, undercutting the"one source of truth" goal. (fixed during review: both now branch identically. Investigated
what was actually pre-existing rather than guessing — the editable editor's
logger.errorwasthe only prior pattern; the resource panel previously logged nothing at all, so the
warnwas new in this PR, not an existing convention.)
navigation to a missing book emitted a warning/error. (fixed during review: branches on
isBookNotFoundErrorfirst —logger.debugfor the expected missing book,logger.errorretained for genuine PDP failures, so the pre-existing severity for real faults is not
downgraded. Verified in
~/Library/Logs/Electron/main.log: the new messages appear at[debug]and there are zero error-level book-not-found entries.)isPlatformErrorevaluated twice on the same value inresource-text-panel.web-view.tsx(once for
usjFromPdp, again inside thebookExistsmemo), where the editable editor derivesboth in one pass. (fixed during review: collapsed to a single
const [usjFromPdp, bookExists] = useMemo<[Usj | undefined, boolean]>(...), matching theeditable editor's shape. The resource panel's
undefined-on-error semantics were preserved —unlike the editor's
defaultUsj— because theif (usjFromPdp)guard downstream depends onit.)
tw:flex-1 … tw:p-8 tw:text-centervs
tw:h-full tw:px-4with no centering — so the same sentence sat in a different placedepending on which tab you were in. (fixed during review, then superseded by PT-4111: Show a book-not-available view with a Manage books action #2691, which
replaced that whole branch. The in-review
EmptyStatewrapper was dropped in the rebase; thecross-surface consistency it was after is now delivered by the two edits described under
"Rebased onto PT-4111" above.)
BOOK_NOT_FOUND_REGEXis coupled toMissingBookException.cs's message text, comments pointed both ways, but nothing failed ifsomeone reworded the C#. (fixed during review: added
c-sharp-tests/MissingBookExceptionTests.cs(4 tests) pinning the exact wording and theregex the extension applies, and cross-referenced the comments on both sides.
Falsifiability checked: temporarily rewording the C# message to
"Book {n} is missing from {id}."failed 3 of the 4 tests; original restored.)"render the message instead of the editor" decision was not. (fixed during review for the
Model text panel: 4 tests added to the existing
model-text-panel.component.test.tsxcovering the message replacing the editor, the
role="status"live region, the negativecontrol (an unrelated failure still renders the editor), and recovery when navigating back to
a book the resource has. Falsifiability checked: disabling the render branch failed 3 of
the 4, with the negative control correctly still passing.
resource-text-panel.web-view.tsxstill has no test file of its own — see Suggested Review Focus.)
loggercall insideuseMemois a render-time side effect (double-fires under StrictMode)(Dismissed: consistent with the pre-existing editable editor, both files now do it
identically, and the common case is now
debugso the noise is minimal. Moving it to auseEffectwould split the log from the derivation it explains.)A shared(Dismissed: the three call sites differ in{ usj, bookExists }helper would make "one source of truth" cover the wholeguard → log → branch dance, not just the regex
fallback (
defaultUsjvsundefinedvs React state) and in log wording, so the helper wouldneed enough parameters that it would not clearly pay for itself.)
The message says what is missing but not what to do next(Dismissed: the changedeliberately reuses the existing string rather than coining a new one, and rewording would
affect all three panels. Per the style guide a meaning change would need a new key.)
The borrowed localization key is prepended above the alphabetized(Dismissed: the comment above it already marks it as borrowed from another web view's%webView_resourcePanel_*block
namespace, which is the information a reader needs.)
Editorialis unmounted while the book is missing, so returning to a book that existsremounts a fresh editor whose content depends on the
setUsjeffect refiring(Dismissed as a known, pre-existing coupling: it mirrors the editable editor's existing
!bookExistsearly return, and holds as long asuseProjectDatareturns a new objectidentity per chapter. The Model text panel's equivalent recovery path is now covered by a
test.)
[Author response: Author asked for the log-level disagreement, the double evaluation, the
cross-tab styling divergence, and the unenforceable C# contract to be fixed, and asked that the
pre-existing pattern be checked rather than assumed. The remaining five were dismissed with
reasoning recorded above.]
Template Propagation
Shared Regions Modified
None. The only
#region shared withmarker in this extension is insrc/webpack-env.d.ts, whichis not in the diff. (The new
#region PDP Error Detectionis a plain region marker, not a sharedone.)
Extension Config Changes
None. No
package.json,tsconfig.json,webpack.config.ts,.eslintrc*, CI, or dependencychanges.
Positive Observations
MissingBookException.cscoupling comment moved together into the utilsmodule, so the fragile string stayed documented at its new home — and it was renamed to
BOOK_NOT_FOUND_REGEX, matching the neighboringPERMISSIONS_EXCEPTION_REGEXconvention thatthe old
bookNotFoundRegexdid not follow.isBookNotFoundErrorwas placed in the module that already hosts this extension's shared purehelpers, respecting that file's documented extension-host import boundary — a real trap given the
header comment there.
extension-host-import-boundary.test.tspasses.undefined/in-flight and plain-USJ cases, and the tests coverexactly those branches including the "unrelated
PlatformError" negative.enandesentries alreadypresent, and an inline comment explains why a
platformScriptureEditor-prefixed key appears inother panels.
dir={options.textDirection}wrapper — with a comment saying why— is exactly right for a mixed-direction panel: resource text keeps its own direction while the
UI sentence follows the interface locale.
of stranding them.
retained log produce the same
bookExistsresult as before.Interview Notes
Stated purpose: make the Bible text tab honest when the selected resource lacks the current
book, instead of showing stale content or an edit prompt in a read-only resource.
Key decisions the author made during review:
EmptyStatefinding, the author chose to adopt the shared component rather than defend thebare
<div>.follow code patterns" rather than accepting a proposed level. That check changed the outcome: the
investigation showed
logger.errorwas the only pre-existing pattern and that thelogger.warnwas introduced by this PR, so
errorwas retained for genuine faults instead of being downgradedto
warnacross both files.isBookNotFoundErrorso the expected case logs quietly.specifically on consistency-with-existing-patterns grounds.
chose to fix it in this PR rather than defer it, accepting the scope increase.
Author understanding: the author demonstrated clear understanding of the change throughout and
drove several decisions rather than accepting suggestions. The "what was pre-existing?" question in
particular caught an assumption that would otherwise have silently changed existing behavior. No
areas were deferred to AI and no uncertainty was expressed.
Unresolved items: none.
Verification Performed
Beyond the standard checks, three behavioral claims in this PR were verified against the running
app rather than inferred from the diff:
SANIAS (a New Testament–only Sanskrit resource) navigated Matthew 1:1 → Genesis 1:1, the same
navigation on both
mainand this branch. Before: the panel showed Matthew content while thetoolbar read Genesis 1:1. After: "This book does not exist in this resource." Screenshots are in
.review/screenshots/.<p role="status" data-testid="…-book-not-found" class="tw:text-sm tw:text-muted-foreground tw:text-center">, and the editor is confirmed unmounted.~/Library/Logs/Electron/main.logshows the new messages at[debug]with zero error-level book-not-found entries.Both new test groups were mutation-checked (deliberately break the code, confirm the tests fail,
restore) rather than merely observed passing.
In-Review Quality Check
All checks pass after the in-review changes:
npm run typechecknpm run lintimport/no-duplicates) are inmodel-text-panel.component.tsx— a file this branch edits — but they predate it: their line numbers merely shifted from 28/34 to 34/40 because the new imports pushed them down. They concern a duplicateresource-reference.utilsimport unrelated to this change, so they were left alone rather than folded into this PR.csharpiernpm testdotnet test c-sharp-tests/No fixes were needed to satisfy the quality gate; no unfixable failures.
Suggested Review Focus
model-text-panel.component.tsx.Confirm you are happy with that breadth in one PR versus splitting the Model text panel into a
follow-up.
book-not-available-view'sSimple
<span>and the editor'sisResourcebranch). They are deliberate and small, but theytouch code reviewed days ago under a different ticket — worth confirming you want them here
rather than as a separate follow-up.
isBookNotFoundErrorcontract: it now accepts both aPlatformError(whatuseProjectDatareturns) and a thrownError(what awaitinggetChapterUSJrejects with),because the Model text panel fails via the second path. Worth a look at whether widening the
guard to
possibleError instanceof Erroris the right boundary, versus two separatepredicates.
resource-text-panel.web-view.tsxstill has no test file, so its render fork is coveredonly by the manual verification above. The Model text panel's equivalent fork is now
unit-tested; consider whether the resource panel warrants the same.
loggeris not in the Model text panel's load-effect dependency array.exhaustive-depsis already suppressed on that effect for an unrelated documented reason, and the PAPI logger is
a stable module-level object, so this is believed safe — but it is a new use of a prop inside
that effect and worth a second pair of eyes.
in both
platform-scripture-editor.utils.tsandMissingBookExceptionTests.cs. A sharedconstant is not possible across the language boundary; confirm the duplicate-with-a-failing-test
approach is the tradeoff you want.
of the editable editor are now consistent. One known hole is
scripture-text-grid.web-view.tsx,which has no book-not-found handling at all (its only
EmptyStateis "no resources selected").It is Power-only and setting-gated, so it may be out of scope — worth a deliberate decision
rather than an omission.
Screenshots
main)Screenshot files are in
.review/screenshots/(gitignored) — attachbefore-full-window.pngandafter-full-window.pnghere.AI-assisted — session
This change is