Conversation
PuppeteerSharp bump version 20.2.2
Reviewer's GuideThe PR extends DefaultPdfService to support ARM browsers by introducing network proxy configuration, structured logging, and enhanced error handling around browser download, launch, and PDF creation. Sequence diagram for PDF generation with error handling and loggingsequenceDiagram
participant Client
participant DefaultPdfService
participant BrowserFetcher
participant Puppeteer
participant ILogger
Client->>DefaultPdfService: PdfDataAsync(url)
DefaultPdfService->>BrowserFetcher: DownloadAsync() [with WebProxy]
BrowserFetcher-->>DefaultPdfService: Browser downloaded
DefaultPdfService->>ILogger: Log (Ready to start downloading browser)
DefaultPdfService->>Puppeteer: LaunchAsync(options)
Puppeteer-->>DefaultPdfService: IBrowser
DefaultPdfService->>ILogger: Log (Start your browser)
DefaultPdfService->>IBrowser: NewPageAsync()
IBrowser-->>DefaultPdfService: IPage
DefaultPdfService->>IPage: GoToAsync(url)
DefaultPdfService->>IPage: PdfDataAsync()
IPage-->>DefaultPdfService: PDF bytes
DefaultPdfService-->>Client: PDF bytes
alt Exception occurs
DefaultPdfService->>ILogger: Log (Error generating PDF)
DefaultPdfService-->>Client: Exception thrown
end
Class diagram for updated DefaultPdfService with ARM and proxy supportclassDiagram
class DefaultPdfService {
+IWebProxy? WebProxy
+Task<byte[]> PdfDataAsync(string url)
+Task<Stream> PdfStreamAsync(string url)
+Task<byte[]> PdfDataFromHtmlAsync(string html, IEnumerable<string>? links = null, IEnumerable<string>? scripts = null)
+Task<Stream> PdfStreamFromHtmlAsync(string html, IEnumerable<string>? links = null, IEnumerable<string>? scripts = null)
-Task<IBrowser> LaunchBrowserAsync()
-void Log(Exception? exception, string? message, params object?[] args)
}
DefaultPdfService --|> IHtml2Pdf
class IHtml2Pdf {
<<interface>>
}
class ILogger~DefaultPdfService~ {
<<interface>>
}
DefaultPdfService o-- ILogger~DefaultPdfService~
class IWebProxy {
<<interface>>
}
DefaultPdfService o-- IWebProxy
Class diagram for BrowserFetcher proxy integrationclassDiagram
class BrowserFetcher {
+IWebProxy? WebProxy
+Task DownloadAsync()
}
BrowserFetcher o-- IWebProxy
DefaultPdfService --> BrowserFetcher
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
- The inline constructor parameter on the class declaration isn’t valid C#—you need a private readonly ILogger field and a proper constructor to accept and assign it.
- BrowserFetcher.DownloadAsync returns a revision string rather than a browser instance, so assigning its result to a browser variable won’t compile—you should call DownloadAsync for the revision and then use Puppeteer.LaunchAsync.
- You’re logging exceptions at Information level; use LogError or LogWarning for exception paths so failures are more visible in the logs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The inline constructor parameter on the class declaration isn’t valid C#—you need a private readonly ILogger field and a proper constructor to accept and assign it.
- BrowserFetcher.DownloadAsync returns a revision string rather than a browser instance, so assigning its result to a browser variable won’t compile—you should call DownloadAsync for the revision and then use Puppeteer.LaunchAsync.
- You’re logging exceptions at Information level; use LogError or LogWarning for exception paths so failures are more visible in the logs.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.Html2Pdf/Services/DefaultPdfService.cs:27` </location>
<code_context>
- await using var browser = await LaunchBrowserAsync();
- await using var page = await browser.NewPageAsync();
- await page.GoToAsync(url);
+ try
+ {
+ await using var browser = await LaunchBrowserAsync();
</code_context>
<issue_to_address>
Consider extracting a single helper method to handle try/catch/log logic for all PDF generation methods.
You can remove almost all of the duplicated try/catch/log boiler-plate by extracting a single “execute and log” helper and then passing in only the unique bits (navigate/set-up + PdfData vs PdfStream). For example:
```csharp
// add this helper to DefaultPdfService
private async Task<T> ExecuteAsync<T>(Func<IPage, Task<T>> work, string message, params object?[] args)
{
try
{
await using var browser = await LaunchBrowserAsync();
await using var page = await browser.NewPageAsync();
return await work(page);
}
catch (Exception ex)
{
_logger.LogError(ex, message, args);
throw;
}
}
```
Then each public method collapses down to a one-liner:
```csharp
public Task<byte[]> PdfDataAsync(string url) =>
ExecuteAsync(
async page => { await page.GoToAsync(url); return await page.PdfDataAsync(); },
"Error generating PDF from URL: {Url}", url);
public Task<Stream> PdfStreamAsync(string url) =>
ExecuteAsync(
async page => { await page.GoToAsync(url); return await page.PdfStreamAsync(); },
"Error generating PDF from URL: {Url}", url);
public Task<byte[]> PdfDataFromHtmlAsync(string html, IEnumerable<string>? links = null, IEnumerable<string>? scripts = null) =>
ExecuteAsync(
async page =>
{
await page.SetContentAsync(html);
await AddStyleTagAsync(page, links);
await AddScriptTagAsync(page, scripts);
return await page.PdfDataAsync();
},
"Error generating PDF from HTML content");
public Task<Stream> PdfStreamFromHtmlAsync(string html, IEnumerable<string>? links = null, IEnumerable<string>? scripts = null) =>
ExecuteAsync(
async page =>
{
await page.SetContentAsync(html);
await AddStyleTagAsync(page, links);
await AddScriptTagAsync(page, scripts);
return await page.PdfStreamAsync();
},
"Error generating PDF from HTML content");
```
This preserves all behavior, removes four-fold duplication, and keeps your logger usage consistent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| await using var browser = await LaunchBrowserAsync(); | ||
| await using var page = await browser.NewPageAsync(); | ||
| await page.GoToAsync(url); | ||
| try |
There was a problem hiding this comment.
issue (complexity): Consider extracting a single helper method to handle try/catch/log logic for all PDF generation methods.
You can remove almost all of the duplicated try/catch/log boiler-plate by extracting a single “execute and log” helper and then passing in only the unique bits (navigate/set-up + PdfData vs PdfStream). For example:
// add this helper to DefaultPdfService
private async Task<T> ExecuteAsync<T>(Func<IPage, Task<T>> work, string message, params object?[] args)
{
try
{
await using var browser = await LaunchBrowserAsync();
await using var page = await browser.NewPageAsync();
return await work(page);
}
catch (Exception ex)
{
_logger.LogError(ex, message, args);
throw;
}
}Then each public method collapses down to a one-liner:
public Task<byte[]> PdfDataAsync(string url) =>
ExecuteAsync(
async page => { await page.GoToAsync(url); return await page.PdfDataAsync(); },
"Error generating PDF from URL: {Url}", url);
public Task<Stream> PdfStreamAsync(string url) =>
ExecuteAsync(
async page => { await page.GoToAsync(url); return await page.PdfStreamAsync(); },
"Error generating PDF from URL: {Url}", url);
public Task<byte[]> PdfDataFromHtmlAsync(string html, IEnumerable<string>? links = null, IEnumerable<string>? scripts = null) =>
ExecuteAsync(
async page =>
{
await page.SetContentAsync(html);
await AddStyleTagAsync(page, links);
await AddScriptTagAsync(page, scripts);
return await page.PdfDataAsync();
},
"Error generating PDF from HTML content");
public Task<Stream> PdfStreamFromHtmlAsync(string html, IEnumerable<string>? links = null, IEnumerable<string>? scripts = null) =>
ExecuteAsync(
async page =>
{
await page.SetContentAsync(html);
await AddStyleTagAsync(page, links);
await AddScriptTagAsync(page, scripts);
return await page.PdfStreamAsync();
},
"Error generating PDF from HTML content");This preserves all behavior, removes four-fold duplication, and keeps your logger usage consistent.
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
1 participant
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 #491
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Enable ARM browser support and improve observability for HTML-to-PDF service by injecting logging, proxy configuration, and robust error handling.
Enhancements: