Conversation
Reviewer's GuideAdds stream-based loading support and progress demonstration to the PdfReader sample by introducing an OnGetStreamAsync callback, toggling between URL and stream sources, and wiring new sample buttons for different PDFs. Sequence diagram for PdfReader stream-based loading with OnGetStreamAsyncsequenceDiagram
actor User
participant PdfReaders
participant PdfReader
participant WebHostEnvironment
participant FileSystem
User->>PdfReaders: click Sample-Stream button
PdfReaders->>PdfReaders: GetSampleStream()
PdfReaders->>PdfReaders: _url = ""
PdfReaders->>PdfReaders: _streamFileName = "sample.pdf"
PdfReader->>PdfReaders: OnGetStreamAsync()
activate PdfReaders
PdfReaders->>PdfReaders: check _streamFileName
alt _streamFileName is empty
PdfReaders-->>PdfReader: return Stream.Null
else _streamFileName is set
PdfReaders->>WebHostEnvironment: get WebRootPath
WebHostEnvironment-->>PdfReaders: WebRootPath
PdfReaders->>FileSystem: File.OpenRead(WebRootPath + "/samples/" + _streamFileName)
FileSystem-->>PdfReaders: Stream
PdfReaders-->>PdfReader: return Stream
end
deactivate PdfReaders
PdfReader->>PdfReader: render PDF pages from returned stream
Updated class diagram for PdfReaders sample componentclassDiagram
class PdfReaders {
- bool _showTwoPagesOneView
- bool _enableThumbnails
- bool _showDownload
- bool _showToolbar
- string _url
- string _streamFileName
+ Task~Stream~ OnGetStreamAsync()
+ void GetTestStream()
+ void GetSampleStream()
}
Flow diagram for selecting PDF source in PdfReaders sampleflowchart LR
Start([Start])
B{User clicks button}
BU[Sample-Url]
BV[Sample2-Url]
BS[Sample-Stream]
BT[Sample2-Stream]
C[Set _url to ./samples/sample.pdf]
D[Set _url to ./samples/sample2.pdf]
E[Set _url to empty]
F[Set _streamFileName to sample.pdf]
G[Set _streamFileName to sample2.pdf]
H{Is _url empty?}
I[PdfReader loads PDF from Url]
J[PdfReader calls OnGetStreamAsync]
K{Is _streamFileName empty?}
L[Return Stream.Null]
M[Open file at WebRootPath/samples/_streamFileName]
N[Return file Stream to PdfReader]
End([End])
Start --> B
B --> BU
B --> BV
B --> BS
B --> BT
BU --> C --> H
BV --> D --> H
BS --> E --> F --> H
BT --> E --> G --> H
H -->|No| I --> End
H -->|Yes| J --> K
K -->|Yes| L --> End
K -->|No| M --> N --> End
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 and found some issues that need to be addressed.
- The Razor button
OnClicklambdas use double quotes inside a double-quoted attribute (e.g.,OnClick="@(() => _url = "./samples/sample.pdf")"), which will break parsing; use single quotes around the path or interpolate differently to avoid nested double quotes. OnGetStreamAsyncis marked async but only callsTask.Yield()and then uses synchronousFile.OpenRead; consider removingasync/Task.Yield()or switching to truly asynchronous file IO (e.g.,FileStreamwithuseAsync: true) for clarity and consistency.- The file names and sample paths (
sample.pdf,sample2.pdf,./samples/...) are duplicated across methods and the markup; consider centralizing these into constants or a small configuration to reduce the risk of typos and ease future changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Razor button `OnClick` lambdas use double quotes inside a double-quoted attribute (e.g., `OnClick="@(() => _url = "./samples/sample.pdf")"`), which will break parsing; use single quotes around the path or interpolate differently to avoid nested double quotes.
- `OnGetStreamAsync` is marked async but only calls `Task.Yield()` and then uses synchronous `File.OpenRead`; consider removing `async`/`Task.Yield()` or switching to truly asynchronous file IO (e.g., `FileStream` with `useAsync: true`) for clarity and consistency.
- The file names and sample paths (`sample.pdf`, `sample2.pdf`, `./samples/...`) are duplicated across methods and the markup; consider centralizing these into constants or a small configuration to reduce the risk of typos and ease future changes.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor.Server/Components/Samples/PdfReaders.razor:42-43` </location>
<code_context>
</BootstrapInputGroup>
</div>
+ <div class="col-12">
+ <Button Color="Color.Primary" OnClick="@(() => _url = "./samples/sample.pdf")" class="me-2">Sample-Url</Button>
+ <Button Color="Color.Primary" OnClick="@(() => _url = "./samples/sample2.pdf")" class="me-2">Sample2-Url</Button>
+ <Button Color="Color.Danger" OnClick="GetSampleStream" class="me-2">Sample-Stream</Button>
+ <Button Color="Color.Danger" OnClick="GetTestStream">Sample2-Stream</Button>
</code_context>
<issue_to_address>
**issue (bug_risk):** The inline lambdas for `OnClick` have invalid string quoting and will not compile.
The nested double quotes in the `OnClick` lambdas break Razor/C# parsing. Use a different quoting strategy, e.g. escape the inner quotes (`@(() => _url = "./samples/sample.pdf")`) or switch to single quotes inside the string: `@(() => _url = './samples/sample.pdf')`.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor.Server/Components/Samples/PdfReaders.razor.cs:21-25` </location>
<code_context>
private bool _enableThumbnails = true;
private bool _showDownload = true;
private bool _showToolbar = true;
- private string _url = "./samples/sample.pdf";
+ private string _url = "sample.pdf";
+ private string _streamFileName = "";
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The initial URL value uses a different path pattern than the button click handlers, which may be confusing or inconsistent.
The initial `_url` now points to `"sample.pdf"` while the buttons use `"./samples/sample.pdf"` and `"./samples/sample2.pdf"`. If `PdfReader` resolves these differently, the first load could fail or behave differently from the buttons. Consider using a consistent path pattern (e.g., all with `./samples/...` or all without) based on where the files actually reside.
```suggestion
private bool _enableThumbnails = true;
private bool _showDownload = true;
private bool _showToolbar = true;
private string _url = "./samples/sample.pdf";
private string _streamFileName = "";
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| <Button Color="Color.Primary" OnClick="@(() => _url = "./samples/sample.pdf")" class="me-2">Sample-Url</Button> | ||
| <Button Color="Color.Primary" OnClick="@(() => _url = "./samples/sample2.pdf")" class="me-2">Sample2-Url</Button> |
There was a problem hiding this comment.
issue (bug_risk): The inline lambdas for OnClick have invalid string quoting and will not compile.
The nested double quotes in the OnClick lambdas break Razor/C# parsing. Use a different quoting strategy, e.g. escape the inner quotes (@(() => _url = "./samples/sample.pdf")) or switch to single quotes inside the string: @(() => _url = './samples/sample.pdf').
| private bool _enableThumbnails = true; | ||
| private bool _showDownload = true; | ||
| private bool _showToolbar = true; | ||
| private string _url = "./samples/sample.pdf"; | ||
| private string _url = "sample.pdf"; | ||
| private string _streamFileName = ""; | ||
|
|
There was a problem hiding this comment.
suggestion (bug_risk): The initial URL value uses a different path pattern than the button click handlers, which may be confusing or inconsistent.
The initial _url now points to "sample.pdf" while the buttons use "./samples/sample.pdf" and "./samples/sample2.pdf". If PdfReader resolves these differently, the first load could fail or behave differently from the buttons. Consider using a consistent path pattern (e.g., all with ./samples/... or all without) based on where the files actually reside.
| private bool _enableThumbnails = true; | |
| private bool _showDownload = true; | |
| private bool _showToolbar = true; | |
| private string _url = "./samples/sample.pdf"; | |
| private string _url = "sample.pdf"; | |
| private string _streamFileName = ""; | |
| private bool _enableThumbnails = true; | |
| private bool _showDownload = true; | |
| private bool _showToolbar = true; | |
| private string _url = "./samples/sample.pdf"; | |
| private string _streamFileName = ""; |
There was a problem hiding this comment.
Pull request overview
This pull request adds stream-based loading functionality to the PdfReader component demo, replacing the previous download-based approach. The changes demonstrate how to load PDF files from streams rather than just URLs, upgrading to version 10.0.15 of the BootstrapBlazor.PdfReader package.
Key Changes:
- Replaced
OnDownloadAsynccallback withOnGetStreamAsyncto support stream-based PDF loading - Added UI buttons to demonstrate both URL-based and stream-based PDF loading modes
- Updated BootstrapBlazor.PdfReader package from version 10.0.12 to 10.0.15
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/BootstrapBlazor.Server/Components/Samples/PdfReaders.razor.cs | Implements stream loading functionality with OnGetStreamAsync method and helper methods to switch between URL and stream modes |
| src/BootstrapBlazor.Server/Components/Samples/PdfReaders.razor | Adds buttons to toggle between different PDF samples using URL or stream loading approaches |
| src/BootstrapBlazor.Server/BootstrapBlazor.Server.csproj | Updates PdfReader package version to 10.0.15 to support new streaming capabilities |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| var file = Path.Combine(WebHostEnvironment.WebRootPath, "samples", "sample.pdf"); | ||
| await DownloadService.DownloadFromFileAsync($"sample_{DateTime.Now:yyyyMMddHHmmss}.pdf", file); | ||
| await Task.Yield(); |
There was a problem hiding this comment.
The call to await Task.Yield() appears unnecessary in this context. Task.Yield() is typically used to force asynchronous yielding in tight loops or to prevent blocking the synchronization context. Since the rest of the method is synchronous (File.OpenRead is synchronous), this adds no value and makes the code less clear. Consider removing it or using Task.FromResult if the method needs to remain async for interface compliance.
| await Task.Yield(); |
| private void GetTestStream() | ||
| { | ||
| _url = ""; | ||
| _streamFileName = "sample2.pdf"; | ||
| } |
There was a problem hiding this comment.
The method name GetTestStream is ambiguous. It's unclear what "Test" refers to in this context - it appears to load "sample2.pdf" which doesn't obviously relate to "test". Consider renaming to something more descriptive like GetSample2Stream or LoadSecondSampleStream to better reflect its purpose.
| </div> | ||
| <div class="col-12"> | ||
| <Button Color="Color.Primary" OnClick="@(() => _url = "./samples/sample.pdf")" class="me-2">Sample-Url</Button> | ||
| <Button Color="Color.Primary" OnClick="@(() => _url = "./samples/sample2.pdf")" class="me-2">Sample2-Url</Button> |
There was a problem hiding this comment.
The button label "Sample2-Url" is inconsistent with the naming used elsewhere. Consider using more descriptive labels that indicate the actual difference between the samples, or maintain consistency by using "Sample 2" (with a space) instead of "Sample2" to match the pattern if "Sample-Url" represents "Sample 1".
| <Button Color="Color.Primary" OnClick="@(() => _url = "./samples/sample2.pdf")" class="me-2">Sample2-Url</Button> | |
| <Button Color="Color.Primary" OnClick="@(() => _url = "./samples/sample2.pdf")" class="me-2">Sample 2-Url</Button> |
| private void GetTestStream() | ||
| { | ||
| _url = ""; | ||
| _streamFileName = "sample2.pdf"; | ||
| } | ||
|
|
||
| private void GetSampleStream() | ||
| { | ||
| _url = ""; | ||
| _streamFileName = "sample.pdf"; |
There was a problem hiding this comment.
The GetTestStream and GetSampleStream methods have duplicated logic - both clear the URL and set a stream filename. Consider consolidating this into a single parameterized method like LoadStreamFile(string fileName) to reduce code duplication and improve maintainability.
| private void GetTestStream() | |
| { | |
| _url = ""; | |
| _streamFileName = "sample2.pdf"; | |
| } | |
| private void GetSampleStream() | |
| { | |
| _url = ""; | |
| _streamFileName = "sample.pdf"; | |
| private void LoadStreamFile(string fileName) | |
| { | |
| _url = ""; | |
| _streamFileName = fileName; | |
| } | |
| private void GetTestStream() | |
| { | |
| LoadStreamFile("sample2.pdf"); | |
| } | |
| private void GetSampleStream() | |
| { | |
| LoadStreamFile("sample.pdf"); |
| var stream = File.OpenRead(Path.Combine(WebHostEnvironment.WebRootPath, "samples", _streamFileName)); | ||
| return stream; |
There was a problem hiding this comment.
The file stream opened by File.OpenRead is not being properly disposed of. When OnGetStreamAsync is called, it opens a file stream and returns it to the caller. However, there's no guarantee that the caller will dispose of the stream. This can lead to file handle leaks and resource exhaustion over time. Consider implementing IDisposable pattern in the component or ensuring the stream is properly disposed after use.
| var stream = File.OpenRead(Path.Combine(WebHostEnvironment.WebRootPath, "samples", _streamFileName)); | |
| return stream; | |
| var filePath = Path.Combine(WebHostEnvironment.WebRootPath, "samples", _streamFileName); | |
| var memoryStream = new MemoryStream(); | |
| using (var fileStream = File.OpenRead(filePath)) | |
| { | |
| await fileStream.CopyToAsync(memoryStream); | |
| } | |
| memoryStream.Position = 0; | |
| return memoryStream; |
| return Stream.Null; | ||
| } | ||
|
|
||
| var stream = File.OpenRead(Path.Combine(WebHostEnvironment.WebRootPath, "samples", _streamFileName)); |
There was a problem hiding this comment.
The method constructs a file path using _streamFileName without validating or sanitizing the input. While this is a demo/sample file, it sets a potentially dangerous precedent. An attacker could potentially manipulate _streamFileName to include path traversal sequences (e.g., "../../../sensitive.pdf") to access files outside the intended directory. Consider validating the filename to ensure it doesn't contain directory traversal characters or use Path.GetFileName to extract only the filename component.
| var stream = File.OpenRead(Path.Combine(WebHostEnvironment.WebRootPath, "samples", _streamFileName)); | |
| var stream = File.OpenRead(Path.Combine(WebHostEnvironment.WebRootPath, "samples", Path.GetFileName(_streamFileName))); |
| var stream = File.OpenRead(Path.Combine(WebHostEnvironment.WebRootPath, "samples", _streamFileName)); | ||
| return stream; |
There was a problem hiding this comment.
The File.OpenRead call does not handle potential exceptions such as FileNotFoundException, UnauthorizedAccessException, or IOException. If the file doesn't exist or cannot be accessed, this will result in an unhandled exception that could crash the component or display unhelpful errors to users. Consider wrapping this in a try-catch block and returning an appropriate error response or Stream.Null with proper error logging.
| var stream = File.OpenRead(Path.Combine(WebHostEnvironment.WebRootPath, "samples", _streamFileName)); | |
| return stream; | |
| try | |
| { | |
| var stream = File.OpenRead(Path.Combine(WebHostEnvironment.WebRootPath, "samples", _streamFileName)); | |
| return stream; | |
| } | |
| catch (FileNotFoundException ex) | |
| { | |
| Console.Error.WriteLine($"File not found: {ex.Message}"); | |
| return Stream.Null; | |
| } | |
| catch (UnauthorizedAccessException ex) | |
| { | |
| Console.Error.WriteLine($"Access denied: {ex.Message}"); | |
| return Stream.Null; | |
| } | |
| catch (IOException ex) | |
| { | |
| Console.Error.WriteLine($"IO error: {ex.Message}"); | |
| return Stream.Null; | |
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7322 +/- ##
=======================================
Coverage 99.98% 99.98%
=======================================
Files 746 746
Lines 32561 32561
Branches 4500 4500
=======================================
Hits 32556 32556
Misses 1 1
Partials 4 4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Link issues
fixes #7321
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add stream-based loading support to the PdfReader sample and expose UI controls to switch between URL and stream sources.
New Features:
Enhancements: