Guard the too-large check against a platform error with no message

[jasonleenaylor]

Follow-up on #274, taking the robustness question @alex-rawlings-yyc left open in his review rather than answering it with an argument.

isPlatformError in platform-bible-utils is:

return !!error && typeof error === 'object' && 'platformErrorVersion' in error;

It tests one property. So a value that passes it is not proven to carry a message, even though PlatformError declares message: string as required, and isPt9TooLargeError then read .includes off it.

Not reachable from the platform. Every return path in newPlatformError sets message, and there is a deliberate Object.defineProperty(platformError, 'message', { enumerable: true }) so it survives the iframe serialization boundary. The property is engineered to be present.

Guarded anyway because of where the call sits. isPt9TooLargeError runs inside the catch in runPt9Import. A throw from there escapes the handler, so a failed import would surface as an unhandled rejection instead of the reported failure the user is supposed to see. That is a bad trade against one typeof check.

The RESOURCE_EXHAUSTED code check short-circuits first and is untouched. The plain-Error branch needs nothing, since Error guarantees a string message through its prototype.

The regression test asserts both that the call does not throw and that it returns false. It fails with a TypeError against the unfixed code.

Verification

jest      2304 passed / 2304, 87 suites
prettier  clean
eslint    48 problems, identical to the unmodified base
tsc       3 errors, identical to the unmodified base

The eslint and tsc counts are not zero and are not from this change. Both are Cannot find type definition file / import/no-unresolved against papi-dts, platform-bible-react and platform-bible-utils, because ../paranext-core/lib/* is not built in this checkout. I measured the unmodified base to confirm the counts are unchanged; jest passes because it mocks those modules.

Targets main, since #274 squash-merged while this was being verified. The commit was replayed onto main so the diff is the two files rather than all of #274 again.

---

This change is 

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: e724894d-2cd9-4e32-96c0-d8cfda59a28b

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[alex-rawlings-yyc]

@alex-rawlings-yyc reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on jasonleenaylor).

---

src/__tests__/utils/pt9-import-error.test.ts line 33 at r1 (raw file):

  it('rejects a platform error carrying no message instead of throwing out of the catch', () => {
    // isPlatformError only looks for platformErrorVersion, so this shape passes it while the

Same three issues as the comment in the source file: it explains isPlatformError's internals, names runPt9Import, and narrates the .includes mechanics the assertions below already show. The part worth keeping is why the fixture is shaped oddly — it omits a field the type declares as required — which is reasoning the code can't show:

    // Narrows as a platform error despite omitting the message the type declares as required.

---

src/utils/pt9-import-error.ts line 16 at r1 (raw file):

export function isPt9TooLargeError(error: unknown): boolean {
  if (isPlatformError(error))
    // isPlatformError only checks for platformErrorVersion, so a value that passes it is not

This explains how isPlatformError and newPlatformError work internally, but neither is what the reader of this function needs — per comment-rules, that's documenting the collaborator rather than the contract, and newPlatformError isn't called here at all. Naming runPt9Import documents a consumer, which goes stale silently. And "Every newPlatformError path does set it, but…" is an argument for why the guard exists despite being unreachable — that belongs in the PR description (where it already reads well), not in the code.

What the next reader genuinely can't see is that the narrowing proves less than PlatformError's declared message: string suggests, so the typeof check isn't redundant. Keeping the catch point makes it clear this isn't dead defensive code, phrased so it survives a rename of the caller:

    // The narrowing proves less than the type does: a value can satisfy it without carrying the
    // declared message, and this runs where a throw would escape the surrounding catch.

[jasonleenaylor]

@jasonleenaylor made 2 comments.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on alex-rawlings-yyc).

---

src/__tests__/utils/pt9-import-error.test.ts line 33 at r1 (raw file):

Previously, alex-rawlings-yyc (Alex Rawlings) wrote…

Same three issues as the comment in the source file: it explains isPlatformError's internals, names runPt9Import, and narrates the .includes mechanics the assertions below already show. The part worth keeping is why the fixture is shaped oddly — it omits a field the type declares as required — which is reasoning the code can't show:

    // Narrows as a platform error despite omitting the message the type declares as required.

Done.

---

src/utils/pt9-import-error.ts line 16 at r1 (raw file):

Previously, alex-rawlings-yyc (Alex Rawlings) wrote…

This explains how isPlatformError and newPlatformError work internally, but neither is what the reader of this function needs — per comment-rules, that's documenting the collaborator rather than the contract, and newPlatformError isn't called here at all. Naming runPt9Import documents a consumer, which goes stale silently. And "Every newPlatformError path does set it, but…" is an argument for why the guard exists despite being unreachable — that belongs in the PR description (where it already reads well), not in the code.

What the next reader genuinely can't see is that the narrowing proves less than PlatformError's declared message: string suggests, so the typeof check isn't redundant. Keeping the catch point makes it clear this isn't dead defensive code, phrased so it survives a rename of the caller:

    // The narrowing proves less than the type does: a value can satisfy it without carrying the
    // declared message, and this runs where a throw would escape the surrounding catch.

Done.

[alex-rawlings-yyc]

@alex-rawlings-yyc reviewed 2 files and all commit messages, and resolved 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).