Render styled XLSX grids and charts in Go - #176
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughChangesXLSX styled rendering and chart support
Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant XLSXConverter
participant XLSXRenderer
participant ChartParser
participant PDF
XLSXConverter->>XLSXRenderer: route styled worksheet
XLSXRenderer->>ChartParser: load worksheet chart relationships
ChartParser-->>XLSXRenderer: return chart data and anchors
XLSXRenderer->>PDF: render worksheet grid and charts
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues remain across worksheet selection, formulas, chart rendering, pagination, merges, and number formatting.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR adds styled XLSX grid and clustered-chart rendering in Go, including formulas, formatting, pagination, and benchmark updates.
Changes:
- Added style-aware worksheet and chart rendering.
- Added formula, merge, dimension, and number-format support.
- Added regression tests and refreshed benchmark evidence.
File summaries
| File | Description |
|---|---|
minipdf-go/xlsx.go |
Selects XLSX rendering paths. |
minipdf-go/xlsx_render.go |
Parses and renders styled worksheet grids. |
minipdf-go/xlsx_chart.go |
Parses and renders embedded charts. |
minipdf-go/pdf.go |
Adds horizontal text scaling. |
minipdf-go/office_test.go |
Adds XLSX rendering tests. |
artifacts/go-benchmark/issue/xlsx/report/comparison_report.md |
Updates benchmark report. |
artifacts/go-benchmark/issue/xlsx/report/comparison_report.json |
Updates comparison metrics. |
artifacts/go-benchmark/issue/xlsx/report/benchmark_coverage.json |
Refreshes benchmark coverage data. |
Review details
Suppressed comments (4)
minipdf-go/xlsx_chart.go:397
- The parsed
series.categoriesare never used here; only the number of values is read. Consequently the rendered chart has no category labels, such asDay 1throughDay 5in the canonical chart fixture, even after chart dispatch is fixed. Emit the category values under each group and clip them to the chart band.
categories := 0
for _, series := range chart.series {
categories = max(categories, len(series.values))
}
minipdf-go/xlsx_chart.go:413
- A zero or negative series value is forced to a positive 0.5-point bar, and the axis maximum is initialized at zero, so clustered-column charts with negative or zero data are rendered with incorrect bars and no negative range. Compute the data range and draw bars around a zero baseline instead of clamping every value to a positive height.
height := max(0.5, series.values[category]/axisMaximum*plotHeight)
barX := plotLeft + float64(category)*groupWidth + groupPadding + float64(seriesIndex)*barWidth
clip.rect(barX, plotBottom, barWidth, height, colors[seriesIndex%len(colors)])
minipdf-go/xlsx_chart.go:133
- A worksheet can legally contain a drawing made only of shapes and have no drawing relationship part. In that case
files.read(drawingRelationshipsName)returns an error and aborts conversion, even though there are no charts to render. Treat a missing drawing.relspart as an unsupported drawing with zero charts, while still propagating errors for a part that exists but cannot be read.
drawingRelationshipsName := xlsxRelationshipPartName(drawingName)
drawingRelationshipsData, err := files.read(drawingRelationshipsName)
if err != nil {
return nil, err
minipdf-go/xlsx_render.go:487
- These loops mark every cell in every merge before checking whether the merge intersects the current band. A large valid merged range, including a whole-sheet merge, can therefore cause enormous work and allocation on every page; clip the merge to
rowsandcolumnsbefore populatingmerged.
for _, merge := range grid.merges {
for row := merge.startRow; row <= merge.endRow; row++ {
for column := merge.startColumn; column <= merge.endColumn; column++ {
merged[[2]int{row, column}] = true
}
- Files reviewed: 8/17 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+35
to
+39
| if needsCalculatedXLSXGrid(worksheetXML) { | ||
| needsGridRendering = true | ||
| break | ||
| } | ||
| } |
Comment on lines
+160
to
+164
| width = grid.columnOffset(anchor.To.Column) - grid.columnOffset(anchor.From.Column) | ||
| } | ||
| if height <= 0 { | ||
| height = grid.rowOffset(anchor.To.Row) - grid.rowOffset(anchor.From.Row) | ||
| } |
Comment on lines
+132
to
+142
| formulaCount := 0 | ||
| for _, cell := range worksheet.Cells { | ||
| if cell.Formula == nil { | ||
| continue | ||
| } | ||
| formulaCount++ | ||
| if cell.Value != nil && strings.TrimSpace(*cell.Value) != "" { | ||
| return false | ||
| } | ||
| } | ||
| return formulaCount > 0 |
Comment on lines
+430
to
+432
| if grid.maxRow == 0 || grid.maxColumn == 0 { | ||
| document.AddPage(grid.pageSize.Width, grid.pageSize.Height) | ||
| return |
| endColumn := min(merge.endColumn, columns.end) | ||
| startRow := max(merge.startRow, rows.start) | ||
| endRow := min(merge.endRow, rows.end) | ||
| renderXLSXCell(page, cell, xPositions[startColumn], yPositions[endRow+1], xPositions[endColumn+1]-xPositions[startColumn], yPositions[startRow]-yPositions[endRow+1], styleAt(styles, cell.style), merge.startColumn >= columns.start && merge.startColumn <= columns.end) |
Comment on lines
+709
to
+711
| func formatXLSXValue(value string, numeric bool, numberFormat int) string { | ||
| if !numeric || numberFormat != 4 { | ||
| return value |
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
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
Closes #175
Validation
go vet ./...go test ./...go test -race ./...dotnet build src/MiniPdf/MiniPdf.csproj --configuration ReleaseBusiness expense budget1: 4/4 pages, text similarity1.0000, overall0.9670in the canonical three-page comparison and0.9734across all four pagesThis follows merged PR #174; the branch contains only the post-merge Go rendering work and benchmark evidence relative to
main.Summary by CodeRabbit
New Features
Bug Fixes