Skip to content

Auto-transform the Bible HTML from getPassage so the consumer doesn't have extra steps#216

Open
cameronapak wants to merge 8 commits into
mainfrom
transform-bible-html
Open

Auto-transform the Bible HTML from getPassage so the consumer doesn't have extra steps#216
cameronapak wants to merge 8 commits into
mainfrom
transform-bible-html

Conversation

@cameronapak
Copy link
Copy Markdown
Collaborator

@cameronapak cameronapak commented Apr 20, 2026

Summary

Auto-transforms Bible HTML inside getPassage so consumers never need to call transformBibleHtml manually. Uses native DOMParser in browser, dynamic import('linkedom') on server. Added data-yv-transformed idempotency marker so double-transforms are a no-op.

5 files changed across core and ui: bible.ts (getHtmlAdapters + transform in getPassage), bible-html-transformer.ts (idempotency guard), bible-html-transformer.test.ts (3 idempotency tests), bible.test.ts (updated assertions for transformed output), verse.tsx (kept transform as XSS safety net for direct callers).

Verse.Html retains its transformBibleHtml call as defense-in-depth — the idempotency marker makes it a no-op for HTML that already went through getPassage.

All 609 tests pass (290 core, 258 hooks, 61 ui). Build, typecheck, lint green.

Context: Why transformBibleHtml Exists — And Where It May Not Be Needed

Test plan

  • Verify getPassage with format: 'html' returns transformed content (data-yv-transformed present)
  • Verify getPassage with format: 'text' returns raw content (no transformation)
  • Verify double-transform produces identical output (idempotency)
  • Verify footnotes are extracted into data-verse-footnote attributes
  • Verify Verse.Html still sanitizes raw HTML passed directly (XSS protection)

🤖 Generated with Claude Code

Greptile Summary

This PR auto-transforms Bible HTML inside getPassage so consumers no longer need to call transformBibleHtml manually. Runtime-specific DOM adapters (DOMParser in browser, dynamic import('jsdom') on server) are resolved inside getHtmlAdapters(); a data-yv-transformed idempotency marker ensures double-transforms are a no-op, and sanitization always runs first so XSS protection is preserved even for already-transformed HTML.

  • bible.ts gains getHtmlAdapters() and invokes transformBibleHtml at the end of getPassage (HTML format only, skipped when transform: false).
  • bible-html-transformer.ts adds the TRANSFORMED_ATTR constant and moves the idempotency check after sanitizeBibleHtmlDocument, correctly addressing the XSS concern from earlier review threads.
  • The optional peer dependency switches from linkedom to jsdom; the changeset, server wrapper, and lock file are updated consistently, though the PR description summary still mentions linkedom (leftover text).

Confidence Score: 5/5

Safe to merge; the core transform path is well-tested, sanitization runs unconditionally before the idempotency guard, and the jsdom fallback provides a clear error message when the optional peer dep is absent.

All 609 tests pass and the two open findings are a devDep/peer-dep version gap for @types/jsdom and a stale linkedom reference in the PR description body — neither affects runtime behavior or the generated changelog entry.

No files require special attention beyond the minor @types/jsdom version alignment in packages/core/package.json.

Important Files Changed

Filename Overview
packages/core/src/bible.ts Adds getHtmlAdapters() helper for runtime-agnostic DOM adapter resolution and auto-transforms HTML in getPassage; browser path uses native DOMParser, server path uses dynamic import('jsdom') with a clear error message fallback; adds transform?: boolean escape hatch as 6th positional parameter.
packages/core/src/bible-html-transformer.ts Adds TRANSFORMED_ATTR = 'data-yv-transformed' idempotency constant and guard; sanitization correctly runs before the idempotency check, so the previously-flagged XSS bypass concern is now addressed.
packages/core/package.json Replaces linkedom@^0.18.12 peer dependency with jsdom@^24.0.0 (optional); adds @types/jsdom@^28.0.1 as a devDependency — the major-version gap between the peer dep range (^24) and the type definitions (28.x) is worth verifying.
packages/core/src/tests/bible.test.ts Tests updated to assert data-yv-transformed presence for HTML format, absence for text format and transform: false, and footnote transformation; covers all new code paths.
packages/ui/src/components/verse.tsx Comment updated to reflect idempotency; Verse.Html still applies transformBibleHtml client-side as defense-in-depth; sanitization runs first in the transformer, so XSS protection is preserved even for already-transformed HTML.

Sequence Diagram

sequenceDiagram
    participant C as Consumer
    participant G as getPassage()
    participant A as getHtmlAdapters()
    participant T as transformBibleHtml()
    participant API as YouVersion API

    C->>G: "getPassage(versionId, usfm, html)"
    G->>API: "GET /v1/bibles/{id}/passages/{usfm}"
    API-->>G: "raw BiblePassage (untransformed HTML)"
    G->>A: getHtmlAdapters()
    alt Browser — DOMParser available
        A-->>G: "{ parseHtml: DOMParser, serializeHtml: body.innerHTML }"
    else Server — DOMParser absent
        A->>A: "import('jsdom') [Node module cache]"
        A-->>G: "{ parseHtml: JSDOM, serializeHtml: body.innerHTML }"
    end
    G->>T: "transformBibleHtml(passage.content, adapters)"
    T->>T: sanitizeBibleHtmlDocument(doc)
    T->>T: "check [data-yv-transformed] — idempotency guard"
    alt Already transformed
        T-->>G: "{ html } — sanitized, no structural changes"
    else Not yet transformed
        T->>T: "wrapVerseContent / footnotes / nbsp / tables"
        T->>T: "set data-yv-transformed on root element"
        T-->>G: "{ html } — fully transformed"
    end
    G-->>C: "BiblePassage { content: transformedHtml }"
Loading

Fix All in Claude Code Fix All in Cursor

Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
packages/core/package.json:50
The `@types/jsdom` devDependency is at `^28.0.1` while the optional peer dependency specifies `jsdom: ^24.0.0`. These two major versions have a significant gap — if the type signatures in `@types/jsdom@28` describe APIs not present in `jsdom@24` (the version consumers are expected to install), TypeScript consumers who import from `@youversion/platform-core/server` and use this package's types could see mismatches or unexpected type errors. The devDep and peer dep versions should target the same jsdom major.

```suggestion
    "@types/jsdom": "^24.0.0",
```

### Issue 2 of 2
.changeset/auto-transform-bible-html.md:7
**Changeset description references `jsdom` but PR description says `linkedom`**

The changeset body correctly says `import('jsdom')`, but the PR description's Summary section still reads "dynamic `import('linkedom')` on server". This will surface in the generated changelog entry and could confuse consumers comparing the PR description against the release notes. The changeset is the source of truth for the changelog, so this is likely harmless for the release, but worth correcting in the PR body for clarity.

Reviews (6): Last reviewed commit: "Merge branch 'main' into transform-bible..." | Re-trigger Greptile

getPassage now automatically sanitizes and transforms HTML content
before returning — verse wrapping, footnote extraction, nbsp, and
table fixes all happen at the root. Uses native DOMParser in browser,
dynamic import('linkedom') on server. Added data-yv-transformed
idempotency marker so double-transforms are a no-op.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 20, 2026

🦋 Changeset detected

Latest commit: 4494016

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@youversion/platform-core Minor
@youversion/platform-react-hooks Minor
@youversion/platform-react-ui Minor
vite-react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@cameronapak cameronapak changed the title feat(core): auto-transform Bible HTML in getPassage Auto-transform the Bible HTML from getPassage so the consumer doesn't have extra steps Apr 20, 2026
Comment thread packages/core/src/bible-html-transformer.ts Outdated
Comment thread packages/core/src/bible.ts Outdated
cameronapak and others added 2 commits April 20, 2026 14:10
Run XSS sanitization before idempotency check so data-yv-transformed
cannot bypass sanitizeBibleHtmlDocument. Add clear error message when
linkedom is missing on server instead of opaque module-not-found error.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

expect(result.html).not.toContain('onclick');
expect(result.html).toContain('<p>');
expect(result.html).toContain('<p');
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: if you're wondering why this tag is seemingly cut off, it's because the tags would contain a data attribute in this new PR, which would then make it where the tag is something like <p data-yv-attribute> versus <p> standalone

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this expect() call do regular expressions? we don't have pre tags yet, that I know of, but still... it'd be great to tighten this up if that's not too difficult.

Copy link
Copy Markdown
Member

@davidfedor davidfedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big picture: I love the idea of being helpful, without requiring the developer to have to make another call. My comments and questions are around whether this is the best way to do that. (Maybe it is! I'm not sure yet.)
I notice this would be blurring the lines between Core being merely an API helper-layer, but now it would be doing some of the prep-work of the UI (visualization layer). So at the least having that be optional seems wise.
I'm wondering if that parameter should default to do the transformation, or not... or whether we need to force the dev to make a choice (to attempt to force them to make an informed choice).

Comment thread packages/core/src/bible.ts Outdated
} catch {
throw new Error(
'Server-side HTML transformation requires "linkedom". ' +
'Install it as a dependency or pass format: "text" to skip transformation.',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be better if there was a supported way to get raw html (untransformed), for people who don't want to import linkedom or who (for whatever reason) want the original data. How about a new format option, "rawhtml" or something like that?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm writing this here because this error path is not something a builder will probably be excited to be in. The fix would mostly be elsewhere.)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... or add another parameter so that the format can stay "html". That feels like a better idea to me.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like what you're processing. I've added a new commit to have the escape hatch allowing users to intentionally seek raw html versus transformed: aece62a

This PR is ready for re-review and re-consideration @davidfedor

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things:

  • I'm curious, why linkedom as opposed to a more widely supported library like jsdom? Is there any risk of supply chain pollution with the newer library?
  • Have the docs been updated to reflect the need for a third party dependency?
  • Can the dependency be added as an optional peer dependency so it shows up in install logs?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ask as someone who is doing RSC data loading, and will need to have this work server-side :)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Bryson (@arinthros)!

Based on your comment, I've moved from linkedom to jsdom.

The YV dev docs will need to be updated immediatley after this (I will do that)

The dep has been added as an optional peer dep, so it can show up in install logs.

Anything else blocking this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arinthros following up ^

@davidfedor
Copy link
Copy Markdown
Member

(FYI I've asked for thoughts from Bryson H; not sure if he's got cycles to contribute or not)

cameronapak and others added 2 commits April 24, 2026 11:23
Add `transform` param to `getPassage` (default: true) so consumers can
receive untransformed HTML without needing linkedom on the server.
CSS now handles verse label spacing for raw HTML via ::after pseudo-element.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@cameronapak cameronapak requested a review from davidfedor April 24, 2026 16:51
@arinthros
Copy link
Copy Markdown

@davidfedor @cameronapak this looks good to me, I just don't see an "approve" button in my UI. Approved by me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants