Improve PDF rendering for nested and split tables#171
Merged
Conversation
PDF table export was skipping nested table lines and some split-row fragments, which made imported DOCX table content disappear. Reuse a shared table row painter and align the PDF render-start rules with the Canvas renderer so visible table fragments are painted.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIntroduces documentation for PDF export parity lessons and a phased improvement plan. Refactors PDF table rendering to conditionally start based on split-row boundaries and row-range filtering. Adds support for nested table rendering with proper cell-content recursion. Improves merged-cell filtering and border clipping for paginated tables. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PdfPainter
participant TablePainter
participant NestedTable as Nested Block
Client->>PdfPainter: paintPage(pageLines)
PdfPainter->>PdfPainter: shouldStartTableRender(current line)?
alt Start Table Rendering
PdfPainter->>TablePainter: paintTablePageRange(table, pageStartRow, endRowIndex)
TablePainter->>TablePainter: paintTableRows(with clip if split)
TablePainter->>TablePainter: Filter merged cells by ownerRow
TablePainter->>TablePainter: Paint borders with clipped bounds
TablePainter->>PdfPainter: paintCellContent(callback with row range)
PdfPainter->>NestedTable: Detect nestedTable in cell
alt Nested Table Found
PdfPainter->>TablePainter: paintTableRows(nested table, full row range)
TablePainter->>TablePainter: Recurse: paintCellContent for nested cells
TablePainter-->>PdfPainter: Nested content rendered
end
PdfPainter-->>TablePainter: Content painted
else Skip (not a start boundary)
PdfPainter->>PdfPainter: Continue to next line
end
TablePainter-->>Client: Table rendered with parity
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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
🧹 Nitpick comments (1)
packages/docs/src/export/pdf-painter.ts (1)
786-794: ⚡ Quick winShare this predicate with the Canvas renderer instead of duplicating it.
This is now the same parity-critical rule that already exists in
packages/docs/src/view/doc-canvas.ts. Keeping two copies reintroduces the exact drift that caused the split-row regression in the first place, so it would be safer to move it into a shared helper and import it from both renderers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/export/pdf-painter.ts` around lines 786 - 794, Duplicate predicate shouldStartTableRender should be centralized into a single exported helper and imported where needed: extract the existing function (shouldStartTableRender) from pdf-painter.ts into a shared module (export it) and replace the local copy in pdf-painter.ts and the copy in doc-canvas.ts with imports from that shared helper; keep the exact logic (plIndex check, prev/blockIndex, pl.rowSplitOffset, prev.rowSplitOffset) and ensure the helper is exported with the same name and used by the table-rendering code paths in both PdfPainter and the Canvas renderer so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/docs/docs-pdf-export-parity.md`:
- Around line 175-178: Remove the "Nested tables inside cells" item from the
deferred/unsupported lists (the bullet that reads "Nested tables inside cells")
in this document and the other occurrence mentioned (the similar list at the
later block corresponding to lines 261-263), since nested-table rendering is
implemented; ensure both lists no longer mention nested tables and adjust
surrounding text if it implies the feature is unimplemented.
---
Nitpick comments:
In `@packages/docs/src/export/pdf-painter.ts`:
- Around line 786-794: Duplicate predicate shouldStartTableRender should be
centralized into a single exported helper and imported where needed: extract the
existing function (shouldStartTableRender) from pdf-painter.ts into a shared
module (export it) and replace the local copy in pdf-painter.ts and the copy in
doc-canvas.ts with imports from that shared helper; keep the exact logic
(plIndex check, prev/blockIndex, pl.rowSplitOffset, prev.rowSplitOffset) and
ensure the helper is exported with the same name and used by the table-rendering
code paths in both PdfPainter and the Canvas renderer so behavior remains
identical.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e362255-91b3-46f2-b998-76391c3d7b9b
📒 Files selected for processing (6)
docs/design/README.mddocs/design/docs/docs-pdf-export-parity.mddocs/tasks/active/20260501-pdf-export-parity-lessons.mddocs/tasks/active/20260501-pdf-export-parity-todo.mdpackages/docs/src/export/pdf-painter.tspackages/docs/src/export/pdf-table-painter.ts
kokodak
changed the title
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Collaborator
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
The parity doc lived under docs/design/ but it was really a phased in-progress plan — checklists, exit criteria, deferred items — which belongs in docs/tasks/active/. The architectural details it duplicated (pipeline, font strategy, table draw order) already live in docs-pdf-export.md. Consolidate the phased fixture plan, feature requirements, exit criteria, and risk list into the active todo so the design folder stays focused on stable architecture. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
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
2 participants
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.
Summary
Why
Imported DOCX documents can represent complex layouts with nested tables and long table cells. The previous PDF exporter skipped nested table lines and treated consecutive table
PageLines too broadly as duplicates, which caused some table content to disappear in exported PDFs.Linked Issues
Related to #113
Verification
CI automatically posts a verification summary comment on this PR with
per-lane results for both
verify:selfandverify:integration.Skip reason (if applicable):
Risk Assessment
Notes for Reviewers
Summary by CodeRabbit
Bug Fixes & Improvements
Bug Fixes
Tests