Conversation
Reviewer's GuideThe PR enhances HTML-to-PDF generation by adding an optional PdfOptions parameter to all relevant methods, implementing a helper to translate these options into PuppeteerSharp.PdfOptions, and refining logging and header comments. Sequence diagram for PDF generation with PdfOptionssequenceDiagram
participant Client
participant DefaultPdfService
participant PuppeteerSharp
Client->>DefaultPdfService: PdfDataAsync(url, options)
DefaultPdfService->>PuppeteerSharp: NewPageAsync()
DefaultPdfService->>PuppeteerSharp: GoToAsync(url)
DefaultPdfService->>DefaultPdfService: GetOptions(options)
DefaultPdfService->>PuppeteerSharp: PdfDataAsync(PuppeteerSharp.PdfOptions)
DefaultPdfService-->>Client: PDF byte[]
Sequence diagram for PDF generation from HTML with PdfOptionssequenceDiagram
participant Client
participant DefaultPdfService
participant PuppeteerSharp
Client->>DefaultPdfService: PdfDataFromHtmlAsync(html, links, scripts, options)
DefaultPdfService->>PuppeteerSharp: NewPageAsync()
DefaultPdfService->>DefaultPdfService: AddStyleTagAsync(page, links)
DefaultPdfService->>DefaultPdfService: AddScriptTagAsync(page, scripts)
DefaultPdfService->>DefaultPdfService: GetOptions(options)
DefaultPdfService->>PuppeteerSharp: PdfDataAsync(PuppeteerSharp.PdfOptions)
DefaultPdfService-->>Client: PDF byte[]
Class diagram for updated DefaultPdfService with PdfOptions supportclassDiagram
class DefaultPdfService {
IWebProxy? WebProxy
+Task<byte[]> PdfDataAsync(string url, PdfOptions? options = null)
+Task<Stream> PdfStreamAsync(string url, PdfOptions? options = null)
+Task<byte[]> PdfDataFromHtmlAsync(string html, IEnumerable<string>? links = null, IEnumerable<string>? scripts = null, PdfOptions? options = null)
+Task<Stream> PdfStreamFromHtmlAsync(string html, IEnumerable<string>? links = null, IEnumerable<string>? scripts = null, PdfOptions? options = null)
-static PuppeteerSharp.PdfOptions GetOptions(PdfOptions? options)
-static Task AddStyleTagAsync(IPage page, IEnumerable<string>? links = null)
-static Task AddScriptTagAsync(IPage page, IEnumerable<string>? scripts = null)
-Task<IBrowser> LaunchBrowserAsync()
-void Log(Exception? exception, string? message, params object?[] args)
}
class PdfOptions {
+bool Landscape
}
DefaultPdfService --> PdfOptions : uses
DefaultPdfService --> PuppeteerSharp.PdfOptions : converts
PdfOptions --|> PuppeteerSharp.PdfOptions : mapped by GetOptions
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for customizable PDF generation options by introducing a PdfOptions parameter to the HTML-to-PDF conversion methods in the Html2Pdf component. This enhancement allows consumers to specify PDF generation settings like landscape orientation.
- Adds optional
PdfOptionsparameter to all PDF generation methods - Implements
GetOptionshelper method to convert customPdfOptionstoPuppeteerSharp.PdfOptions - Updates copyright headers and package version
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| DefaultPdfService.cs | Added PdfOptions parameter to all PDF generation methods and implemented conversion logic to PuppeteerSharp.PdfOptions |
| ServiceCollectionExtensions.cs | Updated copyright header |
| BootstrapBlazor.Html2Pdf.csproj | Updated version to 9.0.6-beta01 and BootstrapBlazor dependency version |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
95
to
+100
|
|
||
| private static PuppeteerSharp.PdfOptions GetOptions(PdfOptions? options) | ||
| { | ||
| return options == null ? new PuppeteerSharp.PdfOptions() : new PuppeteerSharp.PdfOptions | ||
| { | ||
| Landscape = options.Landscape |
There was a problem hiding this comment.
[nitpick] The GetOptions method only maps the Landscape property from PdfOptions to PuppeteerSharp.PdfOptions. If PdfOptions is intended to support additional properties in the future, this implementation will require updates. Consider documenting this mapping or using a more extensible pattern if additional options are planned.
Suggested change
| private static PuppeteerSharp.PdfOptions GetOptions(PdfOptions? options) | |
| { | |
| return options == null ? new PuppeteerSharp.PdfOptions() : new PuppeteerSharp.PdfOptions | |
| { | |
| Landscape = options.Landscape | |
| /// <summary> | |
| /// Maps <see cref="PdfOptions"/> to <see cref="PuppeteerSharp.PdfOptions"/>. | |
| /// | |
| /// NOTE: If new properties are added to <see cref="PdfOptions"/>, they must be mapped here as well. | |
| /// </summary> | |
| private static PuppeteerSharp.PdfOptions GetOptions(PdfOptions? options) | |
| { | |
| return options == null ? new PuppeteerSharp.PdfOptions() : new PuppeteerSharp.PdfOptions | |
| { | |
| Landscape = options.Landscape | |
| // TODO: Map additional properties from PdfOptions here as needed. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- GetOptions currently only maps the Landscape flag; please extend it to include all PdfOptions properties (e.g. margins, format, printBackground) to fully support customization.
- Refactor the Log method to early-return when LogLevel.Information is disabled, which will flatten the nested ifs and improve readability.
- Consider renaming GetOptions to something like ToPuppeteerPdfOptions or ConvertToPuppeteerSharpOptions for clearer intent of the helper method.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- GetOptions currently only maps the Landscape flag; please extend it to include all PdfOptions properties (e.g. margins, format, printBackground) to fully support customization.
- Refactor the Log method to early-return when LogLevel.Information is disabled, which will flatten the nested ifs and improve readability.
- Consider renaming GetOptions to something like ToPuppeteerPdfOptions or ConvertToPuppeteerSharpOptions for clearer intent of the helper method.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.Html2Pdf/Services/DefaultPdfService.cs:152` </location>
<code_context>
private void Log(Exception? exception, string? message, params object?[] args)
{
- if (args.Length != 0)
</code_context>
<issue_to_address>
**issue (complexity):** Consider removing the redundant log level check and simplifying the GetOptions method to reduce code nesting and verbosity.
```csharp
// ↓ Replace the verbose Log method...
private void Log(Exception? exception, string? message, params object?[] args)
{
- if (logger.IsEnabled(LogLevel.Information))
- {
- if (args.Length != 0)
- {
- logger.LogInformation(exception, "{Message} | Args: {Args}", message, args);
- }
- else
- {
- logger.LogInformation(exception, "{Message}", message);
- }
- }
+ // no need for IsEnabled guard – LogInformation already checks the level
+ if (args.Length > 0)
+ {
+ logger.LogInformation(exception, "{Message} | Args: {Args}", message, args);
+ }
+ else
+ {
+ logger.LogInformation(exception, "{Message}", message);
+ }
}
// …and make GetOptions an expression‐bodied helper
private static PuppeteerSharp.PdfOptions GetOptions(PdfOptions? options)
=> new()
{
Landscape = options?.Landscape ?? default
};
```
These changes
- remove the redundant `IsEnabled` check and flatten the nesting,
- shorten `GetOptions` to a single expression while preserving the mapping.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -161,13 +151,16 @@ private async Task<IBrowser> LaunchBrowserAsync() | |||
|
|
|||
| private void Log(Exception? exception, string? message, params object?[] args) | |||
There was a problem hiding this comment.
issue (complexity): Consider removing the redundant log level check and simplifying the GetOptions method to reduce code nesting and verbosity.
// ↓ Replace the verbose Log method...
private void Log(Exception? exception, string? message, params object?[] args)
{
- if (logger.IsEnabled(LogLevel.Information))
- {
- if (args.Length != 0)
- {
- logger.LogInformation(exception, "{Message} | Args: {Args}", message, args);
- }
- else
- {
- logger.LogInformation(exception, "{Message}", message);
- }
- }
+ // no need for IsEnabled guard – LogInformation already checks the level
+ if (args.Length > 0)
+ {
+ logger.LogInformation(exception, "{Message} | Args: {Args}", message, args);
+ }
+ else
+ {
+ logger.LogInformation(exception, "{Message}", message);
+ }
}
// …and make GetOptions an expression‐bodied helper
private static PuppeteerSharp.PdfOptions GetOptions(PdfOptions? options)
=> new()
{
Landscape = options?.Landscape ?? default
};These changes
- remove the redundant
IsEnabledcheck and flatten the nesting, - shorten
GetOptionsto a single expression while preserving the mapping.
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.
Link issues
fixes #631
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Enable PDF customization by accepting a PdfOptions parameter in Html2Pdf service methods, add a helper to map options for PuppeteerSharp, refine logging checks, and update license headers.
New Features:
Enhancements:
Chores: