✨(y-provider) preserve custom blocks on HTML/markdown conversion#2296
Open
sylvinus wants to merge 1 commit into
Open
✨(y-provider) preserve custom blocks on HTML/markdown conversion#2296sylvinus wants to merge 1 commit into
sylvinus wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds four BlockNote specs (Callout, InterlinkingLinkInline, Pdf, UploadLoader), composes them into a docs-specific BlockNote schema (docsBlockNoteSchema) with exported Docs* type aliases, updates convertHandler to use the docs schema and typed partial-blocks, and adds tests covering Yjs→HTML/Markdown/BlockNote JSON conversion for the new blocks. It also reorders Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/servers/y-provider/src/blockSpecs/Pdf.ts`:
- Around line 31-45: The code injects block.props.url directly into iframe.src
and anchor.href (see iframe.setAttribute('src', block.props.url') and
link.setAttribute('href', block.props.url')), which risks unsafe schemes like
"javascript:". Before setting these attributes (in Pdf.ts where the iframe and
link are created), validate and normalize the URL scheme using a URL parser (new
URL(...) with a base for relative URLs) or a strict regex and only allow safe
schemes (e.g., "http", "https", "about", and optionally trusted data/* like
"data:application/pdf"); if the scheme is not allowed, do not set the src/href
and instead render a plain text fallback (or omit the element) to avoid
injecting executable URIs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3212ae83-1570-4254-8442-ffa029717184
📒 Files selected for processing (11)
CHANGELOG.mdsrc/frontend/apps/impress/src/features/docs/doc-export/__tests__/utilsHtmlAccessibility.test.tssrc/frontend/apps/impress/src/features/service-worker/__tests__/ApiPlugin.test.tsxsrc/frontend/servers/y-provider/__tests__/convert.test.tssrc/frontend/servers/y-provider/package.jsonsrc/frontend/servers/y-provider/src/blockSpecs/Callout.tssrc/frontend/servers/y-provider/src/blockSpecs/InterlinkingLinkInline.tssrc/frontend/servers/y-provider/src/blockSpecs/Pdf.tssrc/frontend/servers/y-provider/src/blockSpecs/UploadLoader.tssrc/frontend/servers/y-provider/src/blockSpecs/index.tssrc/frontend/servers/y-provider/src/handlers/convertHandler.ts
sylvinus
force-pushed
the
custom_blocks_export
branch
from
bb8663b to
400c2c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/frontend/apps/impress/src/features/service-worker/__tests__/ApiPlugin.test.tsx`:
- Line 63: Remove the redundant "as ArrayBuffer" casts when constructing
Response instances in the test — the buffer returned by
RequestSerializer.objectToArrayBuffer() is already an ArrayBuffer. Locate the
Response constructions (e.g., the new Response(bodyBuffer as ArrayBuffer, { ...
}) usages in ApiPlugin.test.tsx) and simply pass bodyBuffer without the "as
ArrayBuffer" type assertions; do the same for the second occurrence around the
other Response at the later assertion.
In `@src/frontend/servers/y-provider/__tests__/convert.test.ts`:
- Around line 304-530: The test suite is missing regression coverage that
uploadLoader blocks survive the /api/convert flow; add a new test (e.g. "POST
/api/convert Yjs to BlockNote JSON preserves uploadLoader block") that uses
ServerBlockNoteEditor.create and editor.blocksToYDoc to create a Yjs doc
containing a block with type: 'uploadLoader', sends it to POST /api/convert with
content-type 'application/vnd.yjs.doc' and accept 'application/json', and then
asserts the returned JSON body contains an object with type equal to
'uploadLoader' (similar to the existing pageBreak test pattern).
In `@src/frontend/servers/y-provider/src/blockSpecs/index.ts`:
- Around line 13-14: The comment warns that blockSpecs/index.ts must stay in
sync with the frontend BlockNoteEditor.tsx schema; enforce this by extracting
the canonical schema into a shared module (e.g., export a single schema object
from a new shared package) and have both BlockNoteEditor.tsx and
blockSpecs/index.ts import that shared symbol instead of duplicating it, or if
extracting is not feasible add a type-level or runtime test (a unit test that
imports both schemas and asserts deep equality of the exported schema symbols)
to fail CI on drift; if neither option is possible, add a clear runbook/ADR
referencing the BlockNoteEditor.tsx and blockSpecs/index.ts symbols and the
manual sync steps.
- Around line 15-28: The blockSpecs configuration in docsBlockNoteSchema
incorrectly calls spec factories as functions; change the entries in
BlockNoteSchema.create (the blockSpecs object where defaultBlockSpecs is spread)
to use the spec objects directly by replacing CalloutBlock(), PdfBlock(), and
UploadLoaderBlock() with CalloutBlock, PdfBlock, and UploadLoaderBlock
respectively so they match how InterlinkingLinkInline is used.
In `@src/frontend/servers/y-provider/src/blockSpecs/InterlinkingLinkInline.ts`:
- Line 24: The href builder interlinkingHref currently interpolates docId
directly into a path, which can produce broken routes for docIds with reserved
characters; update the interlinkingHref helper (and any other call sites that
compose paths from docId in this file such as the render usages around the
inline link construction) to encode the id using encodeURIComponent before
inserting it into `/docs/${...}/`, and ensure any places that previously used
raw docId now call interlinkingHref so encoding is centralized and consistent.
In `@src/frontend/servers/y-provider/src/blockSpecs/Pdf.ts`:
- Around line 47-49: The iframe title is only set when block.props.name exists,
leaving unlabeled iframes for unnamed PDFs; update the code in Pdf (around where
iframe.setAttribute is called) to always set a title by using block.props.name
if present or a sensible fallback like "PDF preview" (or "PDF preview:
<id/name>" if available), i.e., compute a title variable from block.props.name
|| 'PDF preview' and call iframe.setAttribute('title', title) so every iframe
has an accessible title.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b56e7884-b344-4d2e-95dc-fa7cb8b14501
📒 Files selected for processing (11)
CHANGELOG.mdsrc/frontend/apps/impress/src/features/docs/doc-export/__tests__/utilsHtmlAccessibility.test.tssrc/frontend/apps/impress/src/features/service-worker/__tests__/ApiPlugin.test.tsxsrc/frontend/servers/y-provider/__tests__/convert.test.tssrc/frontend/servers/y-provider/package.jsonsrc/frontend/servers/y-provider/src/blockSpecs/Callout.tssrc/frontend/servers/y-provider/src/blockSpecs/InterlinkingLinkInline.tssrc/frontend/servers/y-provider/src/blockSpecs/Pdf.tssrc/frontend/servers/y-provider/src/blockSpecs/UploadLoader.tssrc/frontend/servers/y-provider/src/blockSpecs/index.tssrc/frontend/servers/y-provider/src/handlers/convertHandler.ts
sylvinus
force-pushed
the
custom_blocks_export
branch
from
400c2c7 to
31413a4
Compare
sylvinus
force-pushed
the
custom_blocks_export
branch
from
31413a4 to
aa290aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/frontend/servers/y-provider/src/blockSpecs/index.ts (1)
19-21:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAvoid invoking custom block specs during schema registration.
Line 19-21 currently call
CalloutBlock(),PdfBlock(), andUploadLoaderBlock(). If those exports are spec objects (not factories), this will fail at runtime and break custom-block conversion.Proposed fix
export const docsBlockNoteSchema = withPageBreak( BlockNoteSchema.create({ blockSpecs: { ...defaultBlockSpecs, - callout: CalloutBlock(), - pdf: PdfBlock(), - uploadLoader: UploadLoaderBlock(), + callout: CalloutBlock, + pdf: PdfBlock, + uploadLoader: UploadLoaderBlock, },#!/bin/bash set -euo pipefail # Verify whether these exports are functions or spec objects. rg -nP "export\\s+(const|function)\\s+(CalloutBlock|PdfBlock|UploadLoaderBlock)\\b" src/frontend/servers/y-provider/src/blockSpecs rg -nP "(CalloutBlock\\(|PdfBlock\\(|UploadLoaderBlock\\()" src/frontend/servers/y-provider/src/blockSpecs/index.ts # Helpful signal: object-style spec declarations (then invocation should be removed). rg -nP "const\\s+(CalloutBlock|PdfBlock|UploadLoaderBlock)\\s*=\\s*create\\w+Spec\\(" src/frontend/servers/y-provider/src/blockSpecs🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/servers/y-provider/src/blockSpecs/index.ts` around lines 19 - 21, The current registration invokes CalloutBlock(), PdfBlock(), and UploadLoaderBlock which will break if those exports are spec objects rather than factory functions; update the blockSpecs registration to reference the spec objects directly by removing the parentheses (use CalloutBlock, PdfBlock, UploadLoaderBlock) in the object literal, or if they are intended to be factories, ensure the exported symbols (CalloutBlock, PdfBlock, UploadLoaderBlock) are functions and keep invocation consistent—locate the usages in the blockSpecs index.ts and the corresponding exports to make them match.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/servers/y-provider/__tests__/convert.test.ts`:
- Around line 397-424: Add a negative-path test alongside the existing "POST
/api/convert Yjs to HTML with PDF block" that uses ServerBlockNoteEditor.create
and editor.blocksToYDoc to produce a PDF block whose props.url is an unsafe
value like "javascript:alert(1)"; POST that Yjs document to the /api/convert
endpoint with the same headers and assert the response is 200 but does NOT
contain an <iframe> or any src/href with the unsafe "javascript:" URL (i.e.,
ensure no iframe/src or link/href includes "javascript:"); use the same test
harness (initApp(), origin, apiKey) and assertions pattern so the new test
verifies the URL-safety guard is enforced.
In `@src/frontend/servers/y-provider/src/blockSpecs/InterlinkingLinkInline.ts`:
- Around line 30-43: The render implementation for InterlinkingLinkInline only
hides the link when disabled but not when unresolved (missing docId), causing
editor-visible text that toExternalHTML later omits; update render (in the
InterlinkingLinkInline block spec) to treat unresolved links the same as
disabled by checking if disabled || !docId and returning a non-link span (or
empty DOM) when true, otherwise proceed to create the anchor with
href/data-doc-id and title as before so editor and external export behavior
align.
---
Duplicate comments:
In `@src/frontend/servers/y-provider/src/blockSpecs/index.ts`:
- Around line 19-21: The current registration invokes CalloutBlock(),
PdfBlock(), and UploadLoaderBlock which will break if those exports are spec
objects rather than factory functions; update the blockSpecs registration to
reference the spec objects directly by removing the parentheses (use
CalloutBlock, PdfBlock, UploadLoaderBlock) in the object literal, or if they are
intended to be factories, ensure the exported symbols (CalloutBlock, PdfBlock,
UploadLoaderBlock) are functions and keep invocation consistent—locate the
usages in the blockSpecs index.ts and the corresponding exports to make them
match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2722269e-77f3-4c65-bcb2-8a7951ef4499
📒 Files selected for processing (9)
CHANGELOG.mdsrc/frontend/servers/y-provider/__tests__/convert.test.tssrc/frontend/servers/y-provider/package.jsonsrc/frontend/servers/y-provider/src/blockSpecs/Callout.tssrc/frontend/servers/y-provider/src/blockSpecs/InterlinkingLinkInline.tssrc/frontend/servers/y-provider/src/blockSpecs/Pdf.tssrc/frontend/servers/y-provider/src/blockSpecs/UploadLoader.tssrc/frontend/servers/y-provider/src/blockSpecs/index.tssrc/frontend/servers/y-provider/src/handlers/convertHandler.ts
sylvinus
force-pushed
the
custom_blocks_export
branch
from
aa290aa to
122832a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/frontend/servers/y-provider/src/blockSpecs/InterlinkingLinkInline.ts (1)
30-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign unresolved-link handling in
renderwith external export.At Line 32,
rendershould also suppress unresolved links (!docId) to matchtoExternalHTML; otherwise visible editor content can disappear during conversion.Proposed fix
render: (inlineContent) => { const { disabled, docId, title } = inlineContent.props; - if (disabled) { + if (disabled || !docId) { return { dom: document.createElement('span') }; } const dom = document.createElement('a'); dom.setAttribute('data-inline-content-type', 'interlinkingLinkInline'); - if (docId) { - dom.setAttribute('href', interlinkingHref(docId)); - dom.setAttribute('data-doc-id', docId); - } + dom.setAttribute('href', interlinkingHref(docId)); + dom.setAttribute('data-doc-id', docId); dom.textContent = title || docId; return { dom }; },Also applies to: 46-51
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/servers/y-provider/src/blockSpecs/InterlinkingLinkInline.ts` around lines 30 - 43, The render function for InterlinkingLinkInline currently renders an anchor even when docId is missing, causing editor-visible links that toExternalHTML later treats as unresolved; update render (in InterlinkingLinkInline.render) to mirror toExternalHTML by returning a neutral element (e.g., document.createElement('span')) when !docId or when props.disabled is true, and ensure any other rendering branch around the later block (the similar code at the 46-51 region) applies the same unresolved-link suppression so unresolved links are not emitted as anchors; keep using interlinkingHref only when docId exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/servers/y-provider/__tests__/convert.test.ts`:
- Around line 398-400: The test's Markdown link assertion is too loose because
it checks a substring without the closing parenthesis; update the assertion that
references response.text to include the full link token with the closing ')' so
it reads something like expect(response.text).toContain('[Other
doc](/docs/00000000-0000-0000-0000-000000000123/)') to ensure the serialized
link is complete and well-formed.
- Around line 571-572: The test currently only checks block types via the local
variable `types` and `expect(types).toContain('uploadLoader')`; update it to
locate the actual `uploadLoader` block in `response.body` (e.g., cast to any[]
and use .find(b => b.type === 'uploadLoader')) and assert its payload/metadata
fields instead of just the type—check the presence and values of the expected
properties (for example ensure `props`/`metadata` object exists and contains the
expected keys and values) so the test fails if `uploadLoader` loses or mutates
its required data.
---
Duplicate comments:
In `@src/frontend/servers/y-provider/src/blockSpecs/InterlinkingLinkInline.ts`:
- Around line 30-43: The render function for InterlinkingLinkInline currently
renders an anchor even when docId is missing, causing editor-visible links that
toExternalHTML later treats as unresolved; update render (in
InterlinkingLinkInline.render) to mirror toExternalHTML by returning a neutral
element (e.g., document.createElement('span')) when !docId or when
props.disabled is true, and ensure any other rendering branch around the later
block (the similar code at the 46-51 region) applies the same unresolved-link
suppression so unresolved links are not emitted as anchors; keep using
interlinkingHref only when docId exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a4fce11-671f-4c08-87ae-45084df7ce3d
📒 Files selected for processing (9)
CHANGELOG.mdsrc/frontend/servers/y-provider/__tests__/convert.test.tssrc/frontend/servers/y-provider/package.jsonsrc/frontend/servers/y-provider/src/blockSpecs/Callout.tssrc/frontend/servers/y-provider/src/blockSpecs/InterlinkingLinkInline.tssrc/frontend/servers/y-provider/src/blockSpecs/Pdf.tssrc/frontend/servers/y-provider/src/blockSpecs/UploadLoader.tssrc/frontend/servers/y-provider/src/blockSpecs/index.tssrc/frontend/servers/y-provider/src/handlers/convertHandler.ts
Wire the docs BlockNote schema (callout, pdf, uploadLoader, interlinking link, page break) into the conversion editor so /api/convert no longer drops or mangles these blocks.
sylvinus
force-pushed
the
custom_blocks_export
branch
from
122832a to
55345bd
Compare
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wire the docs BlockNote schema (callout, pdf, uploadLoader, interlinking link, page break) into the conversion editor so /api/convert no longer drops or mangles these blocks.
This is important for the Docs as CMS usecase, it is surprising for users to see these Docs blocks silently ignored in the output.