Skip to content

Fix unspecified XLSX paper size default - #144

Merged
shps951023 merged 1 commit into
mainfrom
fix/xlsx-default-a4
Sep 6, 2026
Merged

shps951023 merged 1 commit into
mainfrom
fix/xlsx-default-a4

Conversation

@shps951023

@shps951023 shps951023 commented Sep 6, 2026

Copy link
Copy Markdown
Member

Summary

  • default XLSX worksheets without an explicit paperSize to A4, matching the Microsoft 365 benchmark output
  • preserve explicit paperSize="1" as US Letter
  • add regression tests for both omitted and explicit paper-size behavior

Validation

  • dotnet test tests/MiniPdf.Tests: 188 passed
  • classic84_travel_destination_cards: MiniPdf and reference PNGs both 1241 x 1754
  • focused visual score improved from approximately 0.9582 to 0.9963

Summary by CodeRabbit

  • Bug Fixes
    • Excel sheets without a specified paper size now consistently use A4 dimensions by default.
    • Explicitly configured Letter-size sheets continue to generate Letter-sized pages.

Copilot AI lite review requested due to automatic review settings September 6, 2026 16:28

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Paper size propagation can inherit a stale firstExplicitPaperSize if a sheet’s PaperSize is normalized after the value is captured, potentially causing inconsistent paper sizes across sheets in the same workbook.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR aligns XLSX-to-PDF page sizing with Microsoft 365 output by defaulting worksheets that omit paperSize to A4, while keeping explicit paperSize="1" as US Letter, and adds regression coverage for both behaviors.

Changes:

  • Default missing XLSX paperSize to A4 during sheet read/normalization.
  • Add tests validating A4 default when paperSize is omitted and Letter when explicitly set.
  • Update inline documentation to reflect the new default behavior for unspecified paperSize.
File summaries
File Description
tests/MiniPdf.Tests/ExcelToPdfConverterTests.cs Adds regression tests for default A4 and explicit Letter paper size handling.
src/MiniPdf/ExcelReader.cs Adjusts paper size propagation/defaulting logic and updates related comments.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • 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 131 to 135
}
if (sheet.PaperSize == 0)
{
sheet.PaperSize = firstExplicitPaperSize ??
(sheet.Rows.Count >= 1000 && sheet.ColumnWidths.Count == 0 && sheet.DefaultColumnWidth <= 0f ? 9 : 1);
sheet.PaperSize = firstExplicitPaperSize ?? 9;
}
@shps951023
shps951023 merged commit f02a788 into main Sep 6, 2026
2 of 4 checks passed
@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 90f0df92-e4cd-41ee-a542-7207b291d423

📥 Commits

Reviewing files that changed from the base of the PR and between 96dc5ec and f26d8ed.

📒 Files selected for processing (2)
  • src/MiniPdf/ExcelReader.cs
  • tests/MiniPdf.Tests/ExcelToPdfConverterTests.cs

📝 Walkthrough

Walkthrough

Excel conversion now defaults unspecified worksheet paper sizes to A4. Explicit Letter settings remain supported. Tests verify the resulting A4 and Letter page dimensions.

Changes

Excel paper-size defaults

Layer / File(s) Summary
Paper-size default and validation
src/MiniPdf/ExcelReader.cs, tests/MiniPdf.Tests/ExcelToPdfConverterTests.cs
Unspecified paper sizes now use A4. The page-setup documentation reflects this default. Tests verify A4 output without a paper-size setting and 612×792-point Letter output with explicit Letter settings.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Suggested reviewers: enzosam

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/xlsx-default-a4

Comment @coderabbitai help to get the list of available commands.

@shps951023
shps951023 deleted the fix/xlsx-default-a4 branch September 16, 2026 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants