Conversation
Reviewer's GuideAdds a new OnGetStreamAsync callback to PdfReader to support loading PDF content from a stream, and updates initialization to optionally pass the PDF bytes to the JS interop instead of only using a URL. Sequence diagram for PdfReader initialization with optional streamsequenceDiagram
participant BlazorRuntime
participant PdfReader
participant OnGetStreamAsync as OnGetStreamAsyncDelegate
participant JSInterop as JavaScriptInterop
BlazorRuntime->>PdfReader: InvokeInitAsync()
activate PdfReader
PdfReader->>PdfReader: GetPdfStreamDataAsync()
activate PdfReader
alt OnGetStreamAsync is set
PdfReader->>OnGetStreamAsync: Invoke()
activate OnGetStreamAsync
OnGetStreamAsync-->>PdfReader: Stream
deactivate OnGetStreamAsync
PdfReader->>PdfReader: CopyToAsync(memoryStream)
PdfReader-->>PdfReader: pdfBytes
else OnGetStreamAsync is null
PdfReader-->>PdfReader: pdfBytes is null
end
deactivate PdfReader
PdfReader->>JSInterop: init(Id, Interop, { Url, Data = pdfBytes, ... })
deactivate PdfReader
Class diagram for updated PdfReader componentclassDiagram
class PdfReader {
+string? Url
+object? FitMode
+bool EnableThumbnails
+int CurrentPage
+Func~Task~? OnPrintingAsync
+Func~Task`1~? OnGetStreamAsync
+Task InvokeInitAsync()
+Task RotateLeft()
+Task RotateRight()
-Task`1 GetPdfStreamDataAsync()
}
class OnGetStreamAsyncDelegate {
<<delegate>>
+Task StreamInvoker()
}
PdfReader --> OnGetStreamAsyncDelegate : uses
Flow diagram for GetPdfStreamDataAsync logicflowchart TD
A[GetPdfStreamDataAsync start] --> B{OnGetStreamAsync is not null}
B -- Yes --> C[Create MemoryStream]
C --> D[Invoke OnGetStreamAsync]
D --> E[Get Stream result]
E --> F[Copy stream to MemoryStream]
F --> G[Convert MemoryStream to byte array]
G --> H[Return pdfBytes]
B -- No --> I[Set pdfBytes to null]
I --> H[Return pdfBytes]
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 there - I've reviewed your changes - here's some feedback:
- The XML remarks say the stream callback is only used when
Urlis not provided, butGetPdfStreamDataAsynccurrently callsOnGetStreamAsyncunconditionally; consider short-circuiting whenUrlis set to avoid unnecessary stream work and keep behavior aligned with the docs. - In
GetPdfStreamDataAsync, the stream returned fromOnGetStreamAsyncis never disposed; wrapping it in ausing/await using(or allowing the callback to handle disposal explicitly by convention) would avoid potential resource leaks. - You can simplify
GetPdfStreamDataAsyncby returning directly from the happy path (e.g.,return memoryStream.ToArray();) and eliminating the separatepdfBytesvariable and trailingreturn, which makes the method slightly clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The XML remarks say the stream callback is only used when `Url` is not provided, but `GetPdfStreamDataAsync` currently calls `OnGetStreamAsync` unconditionally; consider short-circuiting when `Url` is set to avoid unnecessary stream work and keep behavior aligned with the docs.
- In `GetPdfStreamDataAsync`, the stream returned from `OnGetStreamAsync` is never disposed; wrapping it in a `using`/`await using` (or allowing the callback to handle disposal explicitly by convention) would avoid potential resource leaks.
- You can simplify `GetPdfStreamDataAsync` by returning directly from the happy path (e.g., `return memoryStream.ToArray();`) and eliminating the separate `pdfBytes` variable and trailing `return`, which makes the method slightly clearer.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.cs:284-287` </location>
<code_context>
+ private async Task<byte[]?> GetPdfStreamDataAsync()
+ {
+ byte[]? pdfBytes = null;
+ if (OnGetStreamAsync != null)
+ {
+ using var memoryStream = new MemoryStream();
+ var stream = await OnGetStreamAsync();
+ await stream.CopyToAsync(memoryStream);
+ pdfBytes = memoryStream.ToArray();
</code_context>
<issue_to_address>
**issue (bug_risk):** Dispose the stream returned by OnGetStreamAsync to avoid resource leaks.
The stream returned from OnGetStreamAsync is never disposed. If it’s a FileStream or similar unmanaged resource, this can leak handles until finalization. Wrap the returned stream in a using/await using block, or clearly document that the delegate must return a non-resource stream (e.g., MemoryStream). For example:
```csharp
if (OnGetStreamAsync != null)
{
await using var stream = await OnGetStreamAsync();
using var memoryStream = new MemoryStream();
await stream.CopyToAsync(memoryStream);
pdfBytes = memoryStream.ToArray();
}
```
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.cs:240-244` </location>
<code_context>
- TriggerScaleChanged = OnScaleChangedAsync != null,
- TriggerRotationChanged = OnRotationChanged != null,
- });
+ var _data = await GetPdfStreamDataAsync();
+ await InvokeVoidAsync("init", Id, Interop, new
+ {
+ Url,
+ Data = _data,
+ FitMode,
+ EnableThumbnails,
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid calling OnGetStreamAsync when Url is provided to align with the documented precedence and reduce unnecessary work.
Per the XML remarks, Url should take precedence and the stream callback should only be used when Url is not provided. The current code always calls GetPdfStreamDataAsync, which can incur unnecessary I/O when Url is set and ultimately used by the JS layer.
You could short-circuit the stream loading:
```csharp
protected override async Task InvokeInitAsync()
{
byte[]? data = null;
if (string.IsNullOrEmpty(Url))
{
data = await GetPdfStreamDataAsync();
}
await InvokeVoidAsync("init", Id, Interop, new
{
Url,
Data = data,
// ...
});
}
```
This aligns with the documented precedence and avoids unnecessary work when Url is provided.
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.cs:281-292` </location>
<code_context>
+ if (OnGetStreamAsync != null)
+ {
+ using var memoryStream = new MemoryStream();
+ var stream = await OnGetStreamAsync();
+ await stream.CopyToAsync(memoryStream);
+ pdfBytes = memoryStream.ToArray();
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Guard against OnGetStreamAsync returning null to avoid a possible NullReferenceException.
Since `OnGetStreamAsync` is an extension point, an implementation might legitimately (or accidentally) return null, causing `await stream.CopyToAsync(memoryStream)` to throw. Consider handling the null case explicitly (e.g. return null or throw a descriptive exception) before calling `CopyToAsync` to avoid an opaque NRE and make callback issues easier to diagnose.
```suggestion
private async Task<byte[]?> GetPdfStreamDataAsync()
{
byte[]? pdfBytes = null;
if (OnGetStreamAsync != null)
{
var stream = await OnGetStreamAsync();
if (stream is null)
{
throw new InvalidOperationException("OnGetStreamAsync returned null. A non-null stream is required to read PDF data.");
}
using var memoryStream = new MemoryStream();
await stream.CopyToAsync(memoryStream);
pdfBytes = memoryStream.ToArray();
}
return pdfBytes;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR adds support for loading PDF documents via stream data in the PdfReader component, providing an alternative to URL-based loading. The implementation adds a new OnGetStreamAsync callback parameter that can be used to supply PDF data as a stream when a URL is not available.
Key Changes:
- Added
OnGetStreamAsyncparameter to enable PDF loading from streams - Modified
InvokeInitAsyncto retrieve PDF data from the callback and pass it to JavaScript - Incremented package version from 10.0.11 to 10.0.12
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| PdfReader.razor.cs | Added OnGetStreamAsync parameter, refactored InvokeInitAsync to support stream-based PDF loading, and implemented GetPdfStreamDataAsync helper method |
| BootstrapBlazor.PdfReader.csproj | Bumped version to 10.0.12 |
Comments suppressed due to low confidence (1)
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.cs:206
- Equality checks on floating point values can yield unexpected results.
if (_currentRotation != CurrentRotation)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+281
to
+292
| private async Task<byte[]?> GetPdfStreamDataAsync() | ||
| { | ||
| byte[]? pdfBytes = null; | ||
| if (OnGetStreamAsync != null) | ||
| { | ||
| using var memoryStream = new MemoryStream(); | ||
| var stream = await OnGetStreamAsync(); | ||
| await stream.CopyToAsync(memoryStream); | ||
| pdfBytes = memoryStream.ToArray(); | ||
| } | ||
| return pdfBytes; | ||
| } |
There was a problem hiding this comment.
The stream returned by OnGetStreamAsync is not being disposed. When a callback returns a Stream, the caller is typically responsible for disposing it. The stream should be wrapped in a using statement to ensure proper resource cleanup and prevent memory leaks.
| TriggerScaleChanged = OnScaleChangedAsync != null, | ||
| TriggerRotationChanged = OnRotationChanged != null, | ||
| }); | ||
| var _data = await GetPdfStreamDataAsync(); |
There was a problem hiding this comment.
The GetPdfStreamDataAsync method is called unconditionally and will execute even when Url is provided. According to the documentation comment on OnGetStreamAsync (line 133), the callback should only be invoked when Url is not provided. This wastes resources and doesn't align with the documented behavior. The method should only be called when Url is null or empty.
Suggested change
| var _data = await GetPdfStreamDataAsync(); | |
| object? _data = null; | |
| if (string.IsNullOrEmpty(Url)) | |
| { | |
| _data = await GetPdfStreamDataAsync(); | |
| } |
Comment on lines
+238
to
+255
| protected override async Task InvokeInitAsync() | ||
| { | ||
| Url, | ||
| FitMode, | ||
| EnableThumbnails, | ||
| CurrentPage, | ||
| TriggerPagesInit = OnPagesInitAsync != null, | ||
| TriggerPagesLoaded = OnPagesLoadedAsync != null, | ||
| TriggerPageChanged = OnPageChangedAsync != null, | ||
| TriggerTowPagesOnViewChanged = OnTwoPagesOneViewAsync != null, | ||
| TriggerScaleChanged = OnScaleChangedAsync != null, | ||
| TriggerRotationChanged = OnRotationChanged != null, | ||
| }); | ||
| var _data = await GetPdfStreamDataAsync(); | ||
| await InvokeVoidAsync("init", Id, Interop, new | ||
| { | ||
| Url, | ||
| Data = _data, | ||
| FitMode, | ||
| EnableThumbnails, | ||
| CurrentPage, | ||
| TriggerPagesInit = OnPagesInitAsync != null, | ||
| TriggerPagesLoaded = OnPagesLoadedAsync != null, | ||
| TriggerPageChanged = OnPageChangedAsync != null, | ||
| TriggerTowPagesOnViewChanged = OnTwoPagesOneViewAsync != null, | ||
| TriggerScaleChanged = OnScaleChangedAsync != null, | ||
| TriggerRotationChanged = OnRotationChanged != null, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The JavaScript code (PdfReader.razor.js line 19-21) checks if options.url is null and returns early, which means when only Data is provided without a Url, the PDF will not load. The JavaScript code needs to be updated to handle the case where options.data is provided instead of options.url. Currently, this feature will not work as intended.
| if (OnGetStreamAsync != null) | ||
| { | ||
| using var memoryStream = new MemoryStream(); | ||
| var stream = await OnGetStreamAsync(); |
There was a problem hiding this comment.
The stream returned by OnGetStreamAsync is not null-checked before use. If the callback returns null, this will cause a NullReferenceException when calling CopyToAsync. Add a null check after awaiting OnGetStreamAsync to handle this case gracefully.
Suggested change
| var stream = await OnGetStreamAsync(); | |
| var stream = await OnGetStreamAsync(); | |
| if (stream == null) | |
| { | |
| return null; | |
| } |
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 #812
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add support for initializing PdfReader with a PDF data stream in addition to a URL and pass the resolved PDF bytes into the JS init call.
New Features:
Enhancements: