[Feature] - Page Break#4
Conversation
…ions for each page in the document.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
WalkthroughAdds a PageBreakNode and slash-command, styles for the node, exports EditorRoot and tightens DocumentPDF typing, changes PDF rendering to flatten lists and split content into sections at page-breaks (rendering one PDF page per section), and removes the PDF export web worker. Changes
Sequence DiagramsequenceDiagram
actor User
participant SlashCommandMenu
participant Editor
participant DocumentTree
participant DocumentPDF
User->>SlashCommandMenu: Invoke "Page Break" command
SlashCommandMenu->>Editor: editor.update() → create & insert PageBreakNode
SlashCommandMenu->>Editor: Insert new paragraph after page-break and select it
Editor->>DocumentTree: Update document tree with PageBreakNode
User->>DocumentPDF: Request PDF render (main thread)
DocumentPDF->>DocumentTree: Read editor root (EditorRoot)
DocumentPDF->>DocumentPDF: Recursively flatten lists into leaf node stream
DocumentPDF->>DocumentPDF: Split stream into sections at page-break nodes
DocumentPDF->>DocumentPDF: Render one PDF Page per section (title only on first)
DocumentPDF->>User: Return generated PDF
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/components/DocumentPDF.tsx (1)
200-203: Consider using a cleaner pattern for guaranteeing at least one section.The current approach mutates a
constarray after filtering, which works but can be confusing. A cleaner approach would handle this inline.♻️ Suggested refactor
- // Remove empty sections (leading, trailing, or consecutive page-breaks). - // Always keep at least one section so the document has at least one page. - const sections = rawSections.filter((s) => s.length > 0); - if (sections.length === 0) sections.push([]); + // Remove empty sections (leading, trailing, or consecutive page-breaks). + // Always keep at least one section so the document has at least one page. + const nonEmpty = rawSections.filter((s) => s.length > 0); + const sections = nonEmpty.length > 0 ? nonEmpty : [[]];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/DocumentPDF.tsx` around lines 200 - 203, The code filters rawSections into sections then mutates that const by pushing an empty array; instead initialize sections to the filtered result or a fallback in one expression to avoid mutation: compute the filtered array from rawSections (reference rawSections and the current filter predicate) and assign sections to filtered if it has items otherwise to an array containing a single empty section (i.e., use a single const declaration with a conditional/fallback so sections is guaranteed non-empty without calling push).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/nodes/PageBreakNode.tsx`:
- Around line 118-125: The insertPageBreakWithParagraph function creates a
PageBreakNode then calls node.insertAfter(paragraph) while the new node is not
inserted into the editor tree; update insertPageBreakWithParagraph to first
insert the page break into the nearest root using $insertNodeToNearestRoot(node)
(like SlashCommandMenu.tsx does), then create the paragraph, call
node.insertAfter(paragraph), and finally paragraph.select() so both the page
break and paragraph are properly added and selected.
---
Nitpick comments:
In `@apps/web/src/components/DocumentPDF.tsx`:
- Around line 200-203: The code filters rawSections into sections then mutates
that const by pushing an empty array; instead initialize sections to the
filtered result or a fallback in one expression to avoid mutation: compute the
filtered array from rawSections (reference rawSections and the current filter
predicate) and assign sections to filtered if it has items otherwise to an array
containing a single empty section (i.e., use a single const declaration with a
conditional/fallback so sections is guaranteed non-empty without calling push).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62bd16e8-162b-47ba-b8d1-18ffff0405d1
📒 Files selected for processing (5)
apps/web/src/components/DocumentPDF.tsxapps/web/src/components/SlashCommandMenu.tsxapps/web/src/constants/editor.tsapps/web/src/index.cssapps/web/src/nodes/PageBreakNode.tsx
| export function insertPageBreakWithParagraph(editor: LexicalEditor): void { | ||
| editor.update(() => { | ||
| const node = $createPageBreakNode(); | ||
| const paragraph = $createParagraphNode(); | ||
| node.insertAfter(paragraph); | ||
| paragraph.select(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if insertPageBreakWithParagraph is used anywhere in the codebase
echo "Searching for usages of insertPageBreakWithParagraph..."
rg -n "insertPageBreakWithParagraph" --type ts --type tsxRepository: hawk-doc/hawkdoc
Length of output: 160
🏁 Script executed:
#!/bin/bash
# First, let's find and read the PageBreakNode.tsx file
echo "=== Finding PageBreakNode.tsx ==="
fd -e tsx -e ts "PageBreakNode" | head -20
echo -e "\n=== Reading PageBreakNode.tsx ==="
cat -n apps/web/src/nodes/PageBreakNode.tsxRepository: hawk-doc/hawkdoc
Length of output: 4201
🏁 Script executed:
#!/bin/bash
# Search for insertPageBreakWithParagraph usages (without file type issues)
echo "=== Searching for insertPageBreakWithParagraph usages ==="
rg -n "insertPageBreakWithParagraph"
echo -e "\n=== Searching for SlashCommandMenu.tsx ==="
fd -e tsx "SlashCommandMenu"
echo -e "\n=== Searching for page-break insertion patterns ==="
rg -n "PageBreakNode\|page-break" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -t tsx -t ts | head -40Repository: hawk-doc/hawkdoc
Length of output: 376
🏁 Script executed:
#!/bin/bash
# Read SlashCommandMenu.tsx
echo "=== Reading SlashCommandMenu.tsx ==="
wc -l apps/web/src/components/SlashCommandMenu.tsx
echo -e "\n=== Searching for page break handling in SlashCommandMenu ==="
rg -n -A 10 "page.break\|pagebreak\|PageBreak" apps/web/src/components/SlashCommandMenu.tsxRepository: hawk-doc/hawkdoc
Length of output: 208
🏁 Script executed:
#!/bin/bash
# Search for any page break insertion
echo "=== All PageBreakNode references ==="
rg -n "PageBreakNode\|page-break" --no-heading | grep -v node_modules
echo -e "\n=== Search for $insertNodeToNearestRoot usage ==="
rg -n "\$insertNodeToNearestRoot"
echo -e "\n=== Check SlashCommandMenu briefly for insertion patterns ==="
head -100 apps/web/src/components/SlashCommandMenu.tsx | grep -A 5 "editor.update\|insertNode"Repository: hawk-doc/hawkdoc
Length of output: 1587
🏁 Script executed:
#!/bin/bash
# Search for where $createPageBreakNode is used in SlashCommandMenu
echo "=== Searching for PageBreak insertion in SlashCommandMenu ==="
rg -n -B 3 -A 8 "\$createPageBreakNode" apps/web/src/components/SlashCommandMenu.tsx
echo -e "\n=== Also check what $insertNodeToNearestRoot does ==="
rg -n -B 2 -A 5 "insertNodeToNearestRoot" apps/web/src/components/SlashCommandMenu.tsx | head -30Repository: hawk-doc/hawkdoc
Length of output: 1099
Bug confirmed: insertPageBreakWithParagraph never inserts the page-break node into the editor tree.
The function creates a PageBreakNode and immediately calls node.insertAfter(paragraph) without first inserting the node into the document. This means the orphaned node's insertion will fail, and the paragraph will not be added to the editor.
The correct implementation pattern is shown in SlashCommandMenu.tsx (lines 189–195), which calls $insertNodeToNearestRoot(node) before node.insertAfter(paragraph).
🐛 Proposed fix
+import { $insertNodeToNearestRoot } from '@lexical/utils';
export function insertPageBreakWithParagraph(editor: LexicalEditor): void {
editor.update(() => {
const node = $createPageBreakNode();
+ $insertNodeToNearestRoot(node);
const paragraph = $createParagraphNode();
node.insertAfter(paragraph);
paragraph.select();
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function insertPageBreakWithParagraph(editor: LexicalEditor): void { | |
| editor.update(() => { | |
| const node = $createPageBreakNode(); | |
| const paragraph = $createParagraphNode(); | |
| node.insertAfter(paragraph); | |
| paragraph.select(); | |
| }); | |
| } | |
| import { $insertNodeToNearestRoot } from '@lexical/utils'; | |
| export function insertPageBreakWithParagraph(editor: LexicalEditor): void { | |
| editor.update(() => { | |
| const node = $createPageBreakNode(); | |
| $insertNodeToNearestRoot(node); | |
| const paragraph = $createParagraphNode(); | |
| node.insertAfter(paragraph); | |
| paragraph.select(); | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/nodes/PageBreakNode.tsx` around lines 118 - 125, The
insertPageBreakWithParagraph function creates a PageBreakNode then calls
node.insertAfter(paragraph) while the new node is not inserted into the editor
tree; update insertPageBreakWithParagraph to first insert the page break into
the nearest root using $insertNodeToNearestRoot(node) (like SlashCommandMenu.tsx
does), then create the paragraph, call node.insertAfter(paragraph), and finally
paragraph.select() so both the page break and paragraph are properly added and
selected.
…move unused insertPageBreakWithParagraph function
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/web/src/nodes/PageBreakNode.tsx (1)
88-90: Spreadsuper.exportJSON()for forward compatibility.The base
exportJSON()may include additional properties in future Lexical versions. Spreading ensures the serialized output remains compatible.♻️ Proposed fix
exportJSON(): SerializedPageBreakNode { - return { type: 'page-break', version: 1 }; + return { + ...super.exportJSON(), + type: 'page-break', + version: 1, + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/nodes/PageBreakNode.tsx` around lines 88 - 90, The exportJSON implementation on PageBreakNode should spread the base serialization to remain forward-compatible; update exportJSON in the PageBreakNode class to return an object that spreads super.exportJSON() and then overrides/adds { type: 'page-break', version: 1 } so any future properties from the base node are preserved (refer to exportJSON, PageBreakNode, and SerializedPageBreakNode to locate the method and shape).apps/web/src/components/DocumentPDF.tsx (1)
170-174: Consider usingEditorRootdirectly for stricter typing.The
objecttype provides minimal type safety. Since the component immediately casts toEditorRoot, using it directly in the props interface would catch mismatches at compile time.♻️ Proposed fix
interface DocumentPDFProps { - editorState: object; + editorState: EditorRoot; title: string; watermark?: string; }Then remove the cast on line 177:
export function DocumentPDF({ editorState, title, watermark }: DocumentPDFProps) { - const root = (editorState as EditorRoot).root; + const root = editorState.root; const children = root?.children ?? [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/DocumentPDF.tsx` around lines 170 - 174, Change the DocumentPDFProps.editorState type from object to EditorRoot and remove the runtime cast to EditorRoot inside the DocumentPDF component (the cast referenced on the original line 177); update the props interface: use EditorRoot directly (import EditorRoot from its module where the editor types live) so TypeScript validates the shape at compile time and you can delete the explicit cast within the DocumentPDF function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/components/DocumentPDF.tsx`:
- Around line 170-174: Change the DocumentPDFProps.editorState type from object
to EditorRoot and remove the runtime cast to EditorRoot inside the DocumentPDF
component (the cast referenced on the original line 177); update the props
interface: use EditorRoot directly (import EditorRoot from its module where the
editor types live) so TypeScript validates the shape at compile time and you can
delete the explicit cast within the DocumentPDF function.
In `@apps/web/src/nodes/PageBreakNode.tsx`:
- Around line 88-90: The exportJSON implementation on PageBreakNode should
spread the base serialization to remain forward-compatible; update exportJSON in
the PageBreakNode class to return an object that spreads super.exportJSON() and
then overrides/adds { type: 'page-break', version: 1 } so any future properties
from the base node are preserved (refer to exportJSON, PageBreakNode, and
SerializedPageBreakNode to locate the method and shape).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d22124b6-b95e-4110-8ba0-34c9be5a5d54
📒 Files selected for processing (2)
apps/web/src/components/DocumentPDF.tsxapps/web/src/nodes/PageBreakNode.tsx
…d update DocumentPDF import
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/workers/pdfExport.worker.ts (1)
1-6:⚠️ Potential issue | 🟠 MajorPDF export in a Web Worker violates coding guidelines.
The guidelines state that "PDF export runs on the main thread via
@react-pdf/renderer— never move to a Web Worker as it does not support required APIs." While the worker architecture is pre-existing (not introduced by this PR), this typing change extends the pattern.Consider migrating PDF export to the main thread in a follow-up to align with the documented architecture. Based on learnings: "PDF export runs on the main thread via react-pdf/renderer — never move to a Web Worker as it does not support required APIs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/workers/pdfExport.worker.ts` around lines 1 - 6, This file implements PDF export inside a Web Worker which violates the guideline that PDF export must run on the main thread using `@react-pdf/renderer`; move the export logic out of the worker into a main-thread module that calls pdf(...) with createElement(DocumentPDF, { ...editorState as EditorRoot, title }), generates the blob URL and returns it to callers, then replace any consumer code that spawns this worker to instead import and call the new main-thread export function; finally remove this worker and its imports (pdf, createElement, DocumentPDF, EditorRoot) or keep it only as a stub redirecting to the main-thread implementation in a follow-up PR.
🧹 Nitpick comments (1)
apps/web/src/workers/pdfExport.worker.ts (1)
27-27: Type assertion on parsed JSON lacks runtime validation.Using
as EditorRoottrusts the incoming JSON structure without verification. IfeditorStateJSONis malformed or missingroot.children, this could cause runtime errors during PDF rendering.Consider adding minimal validation or using a schema validator:
🛡️ Proposed validation
- const editorState = JSON.parse(editorStateJSON) as EditorRoot; + const parsed: unknown = JSON.parse(editorStateJSON); + if ( + typeof parsed !== 'object' || + parsed === null || + !('root' in parsed) || + typeof (parsed as EditorRoot).root?.children === 'undefined' + ) { + throw new Error('Invalid editor state structure'); + } + const editorState = parsed as EditorRoot;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/workers/pdfExport.worker.ts` at line 27, The code currently trusts JSON.parse(editorStateJSON) with a type assertion to EditorRoot; add runtime validation: wrap JSON.parse in try/catch to handle malformed JSON, verify the parsed object has a "root" property and that root.children is an array (or meets whatever minimal shape your renderer expects), and if validation fails throw or return a controlled error/path so pdf rendering doesn't crash. Update the usage around the const editorState = JSON.parse(editorStateJSON) as EditorRoot; location to perform these checks (referencing editorStateJSON and EditorRoot) and handle failures consistently (e.g., log and abort PDF export or fall back to an empty/validated editor state).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/web/src/workers/pdfExport.worker.ts`:
- Around line 1-6: This file implements PDF export inside a Web Worker which
violates the guideline that PDF export must run on the main thread using
`@react-pdf/renderer`; move the export logic out of the worker into a main-thread
module that calls pdf(...) with createElement(DocumentPDF, { ...editorState as
EditorRoot, title }), generates the blob URL and returns it to callers, then
replace any consumer code that spawns this worker to instead import and call the
new main-thread export function; finally remove this worker and its imports
(pdf, createElement, DocumentPDF, EditorRoot) or keep it only as a stub
redirecting to the main-thread implementation in a follow-up PR.
---
Nitpick comments:
In `@apps/web/src/workers/pdfExport.worker.ts`:
- Line 27: The code currently trusts JSON.parse(editorStateJSON) with a type
assertion to EditorRoot; add runtime validation: wrap JSON.parse in try/catch to
handle malformed JSON, verify the parsed object has a "root" property and that
root.children is an array (or meets whatever minimal shape your renderer
expects), and if validation fails throw or return a controlled error/path so pdf
rendering doesn't crash. Update the usage around the const editorState =
JSON.parse(editorStateJSON) as EditorRoot; location to perform these checks
(referencing editorStateJSON and EditorRoot) and handle failures consistently
(e.g., log and abort PDF export or fall back to an empty/validated editor
state).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51d3b974-110f-47c4-bdc6-e3775d63fbca
📒 Files selected for processing (2)
apps/web/src/components/DocumentPDF.tsxapps/web/src/workers/pdfExport.worker.ts
Description
PageBreakNodeto the Lexical editor with slash command support and reliable PDF export./page breakinserts a dashed visual divider.<Page>components at each break replacing the unreliable<View break />approach.Type of change
How to test
Screenshots
Checklist
npm run typecheck)npm run lint)devbranch (notmain)Summary by CodeRabbit
New Features
New Component
Style