Auto-transform the Bible HTML from getPassage so the consumer doesn't have extra steps#216
Auto-transform the Bible HTML from getPassage so the consumer doesn't have extra steps#216cameronapak wants to merge 5 commits intomainfrom
Conversation
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 detectedLatest commit: 20c1599 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
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'); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
davidfedor
left a comment
There was a problem hiding this comment.
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).
| } catch { | ||
| throw new Error( | ||
| 'Server-side HTML transformation requires "linkedom". ' + | ||
| 'Install it as a dependency or pass format: "text" to skip transformation.', |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
(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.)
There was a problem hiding this comment.
... or add another parameter so that the format can stay "html". That feels like a better idea to me.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
A few things:
- I'm curious, why
linkedomas opposed to a more widely supported library likejsdom? 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?
There was a problem hiding this comment.
I ask as someone who is doing RSC data loading, and will need to have this work server-side :)
|
(FYI I've asked for thoughts from Bryson H; not sure if he's got cycles to contribute or not) |
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>
Summary
Auto-transforms Bible HTML inside
getPassageso consumers never need to calltransformBibleHtmlmanually. Uses nativeDOMParserin browser, dynamicimport('linkedom')on server. Addeddata-yv-transformedidempotency 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.Htmlretains itstransformBibleHtmlcall as defense-in-depth — the idempotency marker makes it a no-op for HTML that already went throughgetPassage.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
getPassagewithformat: 'html'returns transformed content (data-yv-transformedpresent)getPassagewithformat: 'text'returns raw content (no transformation)data-verse-footnoteattributesVerse.Htmlstill sanitizes raw HTML passed directly (XSS protection)🤖 Generated with Claude Code
Greptile Summary
This PR auto-transforms Bible HTML inside
getPassageso consumers no longer calltransformBibleHtmlmanually, with atransform: falseescape hatch for raw-HTML or server-only use cases. Thedata-yv-transformedidempotency marker makesVerse.Html's existing sanitization call a no-op for pre-transformed content, and a clear error is thrown whenlinkedomis absent on the server.Confidence Score: 4/5
Safe to merge after addressing the two P2 items; no P0/P1 issues introduced in this PR
Both remaining findings are P2: the idempotency marker fallback is an edge case that doesn't affect the Bible API's always-wrapped HTML in practice, and the CSS :has() compatibility gap only impacts transform: false consumers on Firefox < 121. Score of 4 rather than 5 to signal these are worth a quick fix before shipping, but neither is blocking.
packages/core/src/bible-html-transformer.ts (marker placement fallback) and packages/core/src/styles/bible-reader.css (:has() browser support)
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["getPassage(versionId, usfm, format)"] --> B{"format == html\nAND transform !== false?"} B -- No --> C[Return raw API response] B -- Yes --> D[getHtmlAdapters] D --> E{"DOMParser available\nin globalThis?"} E -- Yes --> F[Browser: native DOMParser adapter] E -- No --> G["Server: dynamic import linkedom"] G -- error --> H["Throw: install linkedom\nor use transform: false"] G -- ok --> I[linkedom DOMParser adapter] F --> J[transformBibleHtml] I --> J J --> K{"doc.querySelector\ndata-yv-transformed?"} K -- Found --> L[sanitize only, return early] K -- Not found --> M["sanitize → wrapVerseContent\n→ footnotes → nbsp → tables"] M --> N["Set data-yv-transformed on\nfirstElementChild"] N --> O[Return transformed passage] L --> O P["Verse.Html browser"] --> Q["transformBibleHtml via useMemo"] Q --> KPrompt To Fix All With AI
Reviews (3): Last reviewed commit: "chore: update changeset with transform o..." | Re-trigger Greptile