Conversation
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
There was a problem hiding this comment.
Pull request overview
Adds Markdown-to-PDF rendering support to GenPDFAction so generated PDFs can represent structured content (headings, lists, code blocks, tables) instead of plain text.
Changes:
- Parse
contentas Markdown usinggomarkdown/markdownand render the AST intogofpdf. - Introduce a dedicated Markdown renderer (
genpdf_markdown.go) with table and inline formatting support. - Add tests intended to cover Markdown content, special characters, and tables for PDF generation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| services/actions/genpdf.go | Parses content as Markdown and routes rendering through the new Markdown-to-PDF renderer; updates action description. |
| services/actions/genpdf_markdown.go | New Markdown AST walker/renderer for gofpdf (headings, lists, code blocks, tables, inline styles). |
| services/actions/genpdf_test.go | Adds new test cases for Markdown content, special characters, and tables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| left, _, _, _ := pdf.GetMargins() | ||
| words := strings.Fields(s) | ||
| for i, word := range words { | ||
| wordW := pdf.GetStringWidth(word) | ||
| spaceW := 0.0 | ||
| if i > 0 { | ||
| spaceW = pdf.GetStringWidth(" ") | ||
| } | ||
| x := pdf.GetX() | ||
| // If this word (and preceding space) would overflow, start a new line first. | ||
| if i > 0 { | ||
| if x+spaceW+wordW > maxW && x > left { | ||
| pdf.Ln(lineHt) | ||
| x = pdf.GetX() | ||
| } else { | ||
| pdf.CellFormat(spaceW, lineHt, " ", "", 0, "", false, 0, "") | ||
| x = pdf.GetX() | ||
| } | ||
| } else if wordW > 0 && x+wordW > maxW && x > left { | ||
| pdf.Ln(lineHt) | ||
| x = pdf.GetX() | ||
| } | ||
| // Single word longer than line width: use MultiCell so it wraps. | ||
| if wordW > maxW-left { | ||
| pdf.MultiCell(0, lineHt, word, "", "", false) | ||
| } else { | ||
| if x+wordW > maxW && x > left { | ||
| pdf.Ln(lineHt) |
There was a problem hiding this comment.
cellWrap treats maxW as if it were an absolute X coordinate (it compares pdf.GetX()+... > maxW), but maxW is computed as a width (pageW - left - right). This makes wrapping occur too early (effectively subtracting the left margin twice) and can also mis-handle the long-word branch (wordW > maxW-left). Consider passing an absolute right boundary (e.g., maxX := left + maxW) or computing available width from the current X position when wrapping.
| pdf.Ln(pdfBlockMargin) | ||
| case *ast.HorizontalRule: | ||
| pdf.Ln(pdfBlockMargin) | ||
| pdf.Line(pdf.GetX(), pdf.GetY(), pdf.GetX()+190, pdf.GetY()) |
There was a problem hiding this comment.
The horizontal rule is drawn with a hard-coded width (+190), which ignores the configured page size and margins and can render past the right margin (or be too short if margins/page size differ). Consider computing the end X using the page width and right margin (or using pdf.GetPageSize() / current margins) instead of a constant.
| pdf.Line(pdf.GetX(), pdf.GetY(), pdf.GetX()+190, pdf.GetY()) | |
| x := pdf.GetX() | |
| _, _, right, _ := pdf.GetMargins() | |
| pageWidth, _ := pdf.GetPageSize() | |
| endX := pageWidth - right | |
| pdf.Line(x, pdf.GetY(), endX, pdf.GetY()) |
| pdf.SetFont("Arial", "", 12) | ||
| pdf.CellFormat(8, pdfLineHeight, bullet, "", 0, "", false, 0, "") | ||
| for inner := ast.GetFirstChild(item); inner != nil; inner = ast.GetNextNode(inner) { | ||
| renderBlock(pdf, tr, inner) | ||
| } |
There was a problem hiding this comment.
List items render the bullet with a fixed-width CellFormat, then rely on cellWrap/pdf.Ln() for wrapping. Since Ln() resets X to the left margin, wrapped lines of a long list item will start at the page left margin rather than aligning under the text after the bullet (and can visually collide with the bullet area). Consider introducing an explicit indent (e.g., increase left margin / set X after Ln) while rendering the list item's content so wrapped lines remain aligned.
| It("generates PDF with markdown content and renders structure", func() { | ||
| content := "# Section\n\n**Bold** and *italic* and `code`.\n\n- Item one\n- Item two" | ||
| result, err := action.Run(ctx, sharedState, types.ActionParams{ | ||
| "content": content, | ||
| }) | ||
|
|
||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(result.Result).To(ContainSubstring("PDF generated and saved to:")) | ||
| paths := result.Metadata[actions.MetadataPDFs].([]string) | ||
| Expect(paths).To(HaveLen(1)) | ||
| Expect(paths[0]).To(BeAnExistingFile()) | ||
| info, err := os.Stat(paths[0]) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(info.Size()).To(BeNumerically(">", 0)) | ||
| }) |
There was a problem hiding this comment.
These new tests only assert that a non-empty PDF file is produced; they don't verify that Markdown is actually rendered (e.g., headings/lists are not left as raw #, **, backticks) or that special characters round-trip correctly. This means the tests can pass even if rendering regresses. Consider extracting text from the generated PDF (there is already a github.com/dslipak/pdf dependency in go.mod) and asserting expected output/content for each case.
This PR introduces a parser from markdown to the pdf elements so the PDF action can finally generate readable PDFs.
This pull request introduces Markdown support for PDF generation in the
GenPDFAction, allowing structured content such as headings, lists, code blocks, and tables to be rendered in the output PDF. The action now parses and renders Markdown content, with comprehensive tests added to verify correct rendering and handling of special characters.Markdown rendering enhancements
gomarkdown/markdownlibraries ingenpdf.go, enabling structured content rendering in PDFs.Test coverage improvements
genpdf_test.goto verify PDF generation with Markdown content, special characters, and tables, ensuring correct rendering and file output.