Conversation
* feat(IpAddress): support paste function * refactor: 代码格式化 # Conflicts: # src/BootstrapBlazor/Components/IpAddress/IpAddress.razor.js
* refactor: 增加 JSInvoke 能力 * refactor: 更改样式 * refactor: 增加客户端更改 IP 回调方法 * test: 更新单元测试 * chore: bump version 10.1.3 # Conflicts: # src/BootstrapBlazor/BootstrapBlazor.csproj # src/BootstrapBlazor/Components/IpAddress/IpAddress.razor.scss # test/UnitTest/Components/IpAddressTest.cs
# Conflicts: # src/BootstrapBlazor/Services/DefaultZipArchiveService.cs # src/BootstrapBlazor/Services/IZipArchiveService.cs
# Conflicts: # test/UnitTest/Services/ZipArchiveServiceTest.cs
Contributor
Reviewer's GuideIntroduces a more flexible and async-capable ZIP archive service API (with entry-level configuration and directory/archive async methods), enhances IpAddress, Table, TreeView, Drawer, and CardUpload components with new behaviors and options, and extends the HTML-to-PDF service with PdfOptions support, updating tests accordingly. Sequence diagram for IpAddress IPv4 paste and JS interop updatesequenceDiagram
actor User
participant Browser
participant IpAddressJs as IpAddress_js
participant ModuleBase as BootstrapModuleComponentBase
participant IpComp as IpAddress_component
User->>Browser: Focus IPv4 input
ModuleBase->>Browser: Invoke init(Id, Interop)
Browser->>IpAddressJs: init(id, invoke)
IpAddressJs->>Browser: Attach keydown and paste handlers
User->>Browser: Paste text with IPv4
Browser->>IpAddressJs: paste event
IpAddressJs->>IpAddressJs: Extract IPv4 and split into octets
IpAddressJs->>Browser: Populate .ipv4-cell inputs and prevValues
IpAddressJs->>ModuleBase: invoke.invokeMethodAsync TriggerUpdate(v1, v2, v3, v4)
ModuleBase->>IpComp: [JSInvokable] TriggerUpdate(v1, v2, v3, v4)
IpComp->>IpComp: Set Value1..Value4
IpComp->>IpComp: UpdateValue()
IpComp->>Browser: UI re-renders with new IP value
Class diagram for updated ZIP archive and HTML-to-PDF servicesclassDiagram
direction LR
class IZipArchiveService {
<<interface>>
+Task~Stream~ ArchiveAsync(IEnumerable~ArchiveEntry~ entries, ArchiveOptions options)
+Task ArchiveAsync(string archiveFile, IEnumerable~ArchiveEntry~ entries, ArchiveOptions options)
+Task ArchiveDirectoryAsync(string archiveFile, string directoryName, CompressionLevel compressionLevel, bool includeBaseDirectory, Encoding encoding, CancellationToken token)
+Task~bool~ ExtractToDirectoryAsync(string archiveFile, string destinationDirectoryName, bool overwriteFiles, Encoding encoding, CancellationToken token)
+ZipArchiveEntry GetEntry(string archiveFile, string entryFile, bool overwriteFiles, Encoding encoding)
}
class DefaultZipArchiveService {
+Task~Stream~ ArchiveAsync(IEnumerable~ArchiveEntry~ entries, ArchiveOptions options)
+Task ArchiveAsync(string archiveFile, IEnumerable~ArchiveEntry~ entries, ArchiveOptions options)
+Task ArchiveDirectoryAsync(string archiveFile, string directoryName, CompressionLevel compressionLevel, bool includeBaseDirectory, Encoding encoding, CancellationToken token)
+Task~bool~ ExtractToDirectoryAsync(string archiveFile, string destinationDirectoryName, bool overwriteFiles, Encoding encoding, CancellationToken token)
+ZipArchiveEntry GetEntry(string archiveFile, string entryFile, bool overwriteFiles, Encoding encoding)
-static Task ArchiveFilesAsync(Stream stream, IEnumerable~ArchiveEntry~ entries, ArchiveOptions options)
}
class ArchiveEntry {
<<record struct>>
+string SourceFileName
+string EntryName
+CompressionLevel CompressionLevel
}
class ArchiveOptions {
+ZipArchiveMode Mode
+bool LeaveOpen
+Encoding Encoding
+CompressionLevel CompressionLevel
+Func~string, Task~Stream~~ ReadStreamAsync
}
class IHtml2Pdf {
<<interface>>
+Task~byte[]~ PdfDataAsync(string url, PdfOptions options)
+Task~Stream~ PdfStreamAsync(string url, PdfOptions options)
+Task~byte[]~ PdfDataFromHtmlAsync(string html, IEnumerable~string~ links, IEnumerable~string~ scripts, PdfOptions options)
+Task~Stream~ PdfStreamFromHtmlAsync(string html, IEnumerable~string~ links, IEnumerable~string~ scripts, PdfOptions options)
}
class DefaultHtml2PdfService {
-const string ErrorMessage
+Task~byte[]~ PdfDataAsync(string url, PdfOptions options)
+Task~Stream~ PdfStreamAsync(string url, PdfOptions options)
+Task~byte[]~ PdfDataFromHtmlAsync(string html, IEnumerable~string~ links, IEnumerable~string~ scripts, PdfOptions options)
+Task~Stream~ PdfStreamFromHtmlAsync(string html, IEnumerable~string~ links, IEnumerable~string~ scripts, PdfOptions options)
}
class PdfOptions {
+bool Landscape
}
IZipArchiveService <|.. DefaultZipArchiveService
DefaultZipArchiveService o--> ArchiveEntry
DefaultZipArchiveService o--> ArchiveOptions
IHtml2Pdf <|.. DefaultHtml2PdfService
DefaultHtml2PdfService o--> PdfOptions
Class diagram for updated UI components (IpAddress, DynamicElement, Table, TreeView, CardUpload)classDiagram
direction LR
class BootstrapModuleComponentBase {
#string Id
#object Interop
#Task InvokeVoidAsync(string identifier, object arg1, object arg2)
#Task InvokeInitAsync()
}
class IpAddress {
<<partial>>
+string ClassName
+string Value1
+string Value2
+string Value3
+string Value4
+void TriggerUpdate(int v1, int v2, int v3, int v4)
-void UpdateValue()
}
class DynamicElement {
+bool GenerateElement
+object Key
+RenderFragment ChildContent
+string TagName
+void BuildRenderTree(RenderTreeBuilder builder)
}
class TableTItem {
+Func~TItem, object~ OnGetRowKey
-object GetKeyByITem(TItem item)
-RenderFragment~TItem~ RenderRow
}
class TreeViewTItem {
+Func~TreeViewItemTItem, Task~ OnTreeItemClick
+Func~TreeViewItemTItem, Task~bool~~ OnBeforeTreeItemClick
-Task OnClick(TreeViewItemTItem item)
}
class TreeViewItemTItem {
+TItem Value
+bool IsExpanded
+bool IsActive
+bool CanTriggerClickNode(bool isDisabled, bool canExpandWhenDisabled)
}
class CardUploadTValue {
+RenderFragment~UploadFile~ IconTemplate
+RenderFragment~UploadFile~ ActionButtonTemplate
+bool ShowZoomButton
}
class UploadFile {
+string PrevUrl
+long Size
+string GetFileName()
+string ToFileSizeString()
}
BootstrapModuleComponentBase <|-- IpAddress
TableTItem o--> DynamicElement
TableTItem --> "1" DynamicElement : uses Key
TreeViewTItem o--> TreeViewItemTItem
CardUploadTValue o--> UploadFile
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
ArgoZhang
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- In
Table.razor, theDynamicElementrows now useKey="GetKeyByITem(item)", which passes the literal string as the key instead of the method result; this should be changed to bind the method result (and probably useitemas a fallback whenOnGetRowKeyis null) to preserve stable row identity. - In
DefaultZipArchiveService.ArchiveFilesAsync, directory entries are created usingPath.DirectorySeparatorCharin the entry name, which can introduce platform-specific separators into the ZIP and may not be handled consistently by all tools; consider normalizing to'/'in the stored entry paths. - When
options.ReadStreamAsyncis used inArchiveFilesAsync, the compression level is always taken fromoptions.CompressionLeveland ignoresArchiveEntry.CompressionLevel; if per-entry control is intended, you may want to mirror the file/dir branches and preferf.CompressionLevel ?? options.CompressionLevelthere as well.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Table.razor`, the `DynamicElement` rows now use `Key="GetKeyByITem(item)"`, which passes the literal string as the key instead of the method result; this should be changed to bind the method result (and probably use `item` as a fallback when `OnGetRowKey` is null) to preserve stable row identity.
- In `DefaultZipArchiveService.ArchiveFilesAsync`, directory entries are created using `Path.DirectorySeparatorChar` in the entry name, which can introduce platform-specific separators into the ZIP and may not be handled consistently by all tools; consider normalizing to `'/'` in the stored entry paths.
- When `options.ReadStreamAsync` is used in `ArchiveFilesAsync`, the compression level is always taken from `options.CompressionLevel` and ignores `ArchiveEntry.CompressionLevel`; if per-entry control is intended, you may want to mirror the file/dir branches and prefer `f.CompressionLevel ?? options.CompressionLevel` there as well.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/BaseComponents/DynamicElement.cs:128-130` </location>
<code_context>
builder.AddContent(8, ChildContent);
+ if (Key != null)
+ {
+ builder.SetKey(Key);
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Key is applied too late in the render sequence and won’t key the intended element.
In Blazor, `builder.SetKey` must be called immediately after `OpenElement`/`OpenComponent` and before any attributes or children so the key applies to that element. Placing it after `AddContent(8, ChildContent)` will key the next sequence instead of the wrapper `DynamicElement`. When `GenerateElement` is true, move `SetKey(Key)` to directly after `builder.OpenElement(0, TagName)` and before adding attributes/children, to preserve the behavior callers expect when replacing `@key` with `Key="..."`.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Components/Table/Table.razor:240` </location>
<code_context>
}
else
{
- <DynamicElement class="@GetRowClassString(item, "table-row")" @key="item"
+ <DynamicElement class="@GetRowClassString(item, "table-row")" Key="GetKeyByITem(item)"
TriggerContextMenu="ContextMenuZone != null" OnContextMenu="e => OnContextMenu(e, item)"
@ontouchstart="e => OnTouchStart(e, item)"
</code_context>
<issue_to_address>
**issue (bug_risk):** Row key expression is treated as a literal string instead of being evaluated.
Because `Key="GetKeyByITem(item)"` lacks `@`, Razor treats it as the literal string `"GetKeyByITem(item)"`, so all rows share the same key, breaking diffing and risking rendering issues. The same applies in the later `RenderRow` fragment. Please change these to `Key="@GetKeyByITem(item)"` (or an `@` block) so the method is invoked, and consider renaming `GetKeyByITem` → `GetKeyByItem` for clarity.
</issue_to_address>
### Comment 3
<location> `src/BootstrapBlazor/Components/Drawer/Drawer.razor.js:11-12` </location>
<code_context>
let height = 0;
let isVertical = false;
const drawerBody = el.querySelector('.drawer-body');
- const bar = el.querySelector('.drawer-bar');
+ const bar = [...drawerBody.children].find(i => i.classList.contains('drawer-bar'));
Drag.drag(bar,
e => {
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential runtime error if `.drawer-body` is not present in the drawer.
The previous `querySelector('.drawer-bar')` safely returned `null` when the element was missing. Now, `const drawerBody = el.querySelector('.drawer-body');` is followed by `drawerBody.children`, which will throw if `.drawer-body` is absent or renamed, and the same issue exists in `dispose`. Please add a null-check for `drawerBody` before accessing `children` or fall back to the original `querySelector` approach when `drawerBody` is null.
</issue_to_address>
### Comment 4
<location> `src/BootstrapBlazor/Services/DefaultZipArchiveService.cs:54-56` </location>
<code_context>
+ var entryName = f.EntryName;
+ if (!string.IsNullOrEmpty(entryName))
+ {
+ if (!entryName.EndsWith(Path.DirectorySeparatorChar))
+ {
+ entryName = $"{entryName}{Path.DirectorySeparatorChar}";
+ }
+ archive.CreateEntry(entryName, f.CompressionLevel ?? options.CompressionLevel);
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `Path.DirectorySeparatorChar` in ZIP entry names can create non-portable archives.
ZIP entry paths should always use `'/'` as the separator, independent of OS. Using `Path.DirectorySeparatorChar` will emit `'\'` on Windows and can break tools that expect standard ZIP semantics. Please normalize `entryName` to use `'/'` and append `"/"` explicitly for directory entries.
</issue_to_address>
### Comment 5
<location> `src/BootstrapBlazor/Services/DefaultZipArchiveService.cs:41-47` </location>
<code_context>
- var entry = archive.CreateEntry(Path.GetFileName(f), options.CompressionLevel);
- using var entryStream = entry.Open();
- await using var content = await options.ReadStreamAsync(f);
+ var entry = archive.CreateEntry(f.EntryName, options.CompressionLevel);
+ await using var content = await options.ReadStreamAsync(f.SourceFileName);
+ await using var entryStream = entry.Open();
await content.CopyToAsync(entryStream);
+ entryStream.Close();
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Per-entry `CompressionLevel` is ignored when `ReadStreamAsync` is used.
In this branch you always use `options.CompressionLevel`, whereas in the file/directory branches you honor `f.CompressionLevel ?? options.CompressionLevel`. To keep behavior consistent and allow per-entry overrides for streamed content, use `f.CompressionLevel ?? options.CompressionLevel` here as well if that override is intended to apply to all entry types.
```suggestion
if (options.ReadStreamAsync != null)
{
var entry = archive.CreateEntry(f.EntryName, f.CompressionLevel ?? options.CompressionLevel);
await using var content = await options.ReadStreamAsync(f.SourceFileName);
await using var entryStream = entry.Open();
await content.CopyToAsync(entryStream);
entryStream.Close();
```
</issue_to_address>
### Comment 6
<location> `test/UnitTest/Services/ZipArchiveServiceTest.cs:105-114` </location>
<code_context>
+ public async Task ArchiveAsync_Ok()
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen `ArchiveAsync_Ok` to validate archive contents (files and directory entries), not just file existence
The test currently only verifies that `test.zip` is created, but not that the new `ArchiveEntry`-based logic produced the expected archive structure. Please also:
- Open `test.zip` with `ZipFile.OpenRead` and assert that `test1/test.log` and `test2/test.log` exist with the expected contents (e.g., single byte `65`).
- Assert that directory entries for `test1` and `test2` are present as intended.
- Optionally validate that per-entry `CompressionLevel` is applied (e.g., differs from the default when specified).
This will exercise the new API end-to-end instead of only checking for file creation.
Suggested implementation:
```csharp
[Fact]
public async Task ArchiveAsync_Ok()
{
var fileName = Path.Combine(AppContext.BaseDirectory, "archive_test", "test.zip");
if (File.Exists(fileName))
{
File.Delete(fileName);
}
var root = AppContext.BaseDirectory;
var files = new string[]
{
```
```csharp
var files = new string[]
{
```
```csharp
var root = AppContext.BaseDirectory;
var files = new string[]
{
// existing file list initialization remains unchanged
};
await _service.ArchiveAsync(root, files, fileName);
Assert.True(File.Exists(fileName));
using var archive = ZipFile.OpenRead(fileName);
// directory entries
var dirTest1 = archive.GetEntry("test1/");
Assert.NotNull(dirTest1);
Assert.Equal(0, dirTest1.Length);
var dirTest2 = archive.GetEntry("test2/");
Assert.NotNull(dirTest2);
Assert.Equal(0, dirTest2.Length);
// file entries
var fileTest1 = archive.GetEntry("test1/test.log");
Assert.NotNull(fileTest1);
await using (var fileTest1Stream = fileTest1.Open())
await using (var ms1 = new MemoryStream())
{
await fileTest1Stream.CopyToAsync(ms1);
var bytes1 = ms1.ToArray();
Assert.Equal(1, bytes1.Length);
Assert.Equal(65, bytes1[0]);
}
var fileTest2 = archive.GetEntry("test2/test.log");
Assert.NotNull(fileTest2);
await using (var fileTest2Stream = fileTest2.Open())
await using (var ms2 = new MemoryStream())
{
await fileTest2Stream.CopyToAsync(ms2);
var bytes2 = ms2.ToArray();
Assert.Equal(1, bytes2.Length);
Assert.Equal(65, bytes2[0]);
}
```
1. Ensure `using System.IO.Compression;` is present at the top of `ZipArchiveServiceTest.cs` so `ZipFile` and `ZipArchive` are available.
2. Adjust the `_service.ArchiveAsync(root, files, fileName);` call if the actual service instance or method signature differs (e.g., different parameter order or service field name).
3. If the archive structure produced by the new `ArchiveEntry`-based logic uses different directory or file names than `"test1/"`, `"test2/"`, `"test1/test.log"`, and `"test2/test.log"`, update the `GetEntry` paths to match the actual archive layout.
4. If the test currently already calls `ArchiveAsync` and asserts only `File.Exists(fileName)`, replace that existing assertion block with the new archive-content assertions instead of duplicating the call/assertions.
</issue_to_address>
### Comment 7
<location> `src/BootstrapBlazor/Services/DefaultZipArchiveService.cs:35` </location>
<code_context>
}
- private static async Task ArchiveFilesAsync(Stream stream, IEnumerable<string> files, ArchiveOptions? options = null)
+ private static async Task ArchiveFilesAsync(Stream stream, IEnumerable<ArchiveEntry> entries, ArchiveOptions? options = null)
{
options ??= new ArchiveOptions();
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the new zip helpers to extract per-entry operations, centralize compression-level logic, and hide #if blocks behind small compat methods to keep the async APIs simpler and easier to follow.
You can reduce the added complexity without changing behavior by factoring out a few focused helpers and centralizing rules.
### 1. Split `ArchiveFilesAsync` into per‑case helpers
Right now `ArchiveFilesAsync` has three inline branches (stream, directory, file). Extracting them makes the loop easier to read and reduces nesting:
```csharp
private static async Task ArchiveFilesAsync(Stream stream, IEnumerable<ArchiveEntry> entries, ArchiveOptions? options = null)
{
options ??= new ArchiveOptions();
using var archive = new ZipArchive(stream, options.Mode, options.LeaveOpen, options.Encoding);
foreach (var entry in entries)
{
await ArchiveEntryAsync(archive, entry, options);
}
}
private static async Task ArchiveEntryAsync(ZipArchive archive, ArchiveEntry entry, ArchiveOptions options)
{
if (options.ReadStreamAsync != null)
{
await CreateEntryFromStreamAsync(archive, entry, options);
}
else if (Directory.Exists(entry.SourceFileName))
{
CreateDirectoryEntry(archive, entry, options);
}
else
{
CreateFileEntry(archive, entry, options);
}
}
```
Then move the inline branches into small helpers:
```csharp
private static async Task CreateEntryFromStreamAsync(ZipArchive archive, ArchiveEntry entry, ArchiveOptions options)
{
var zipEntry = archive.CreateEntry(entry.EntryName, GetCompressionLevel(entry, options));
await using var content = await options.ReadStreamAsync!(entry.SourceFileName);
await using var entryStream = zipEntry.Open();
await content.CopyToAsync(entryStream);
}
private static void CreateDirectoryEntry(ZipArchive archive, ArchiveEntry entry, ArchiveOptions options)
{
var entryName = entry.EntryName;
if (string.IsNullOrEmpty(entryName))
{
return;
}
if (!entryName.EndsWith(Path.DirectorySeparatorChar))
{
entryName = $"{entryName}{Path.DirectorySeparatorChar}";
}
archive.CreateEntry(entryName, GetCompressionLevel(entry, options));
}
private static void CreateFileEntry(ZipArchive archive, ArchiveEntry entry, ArchiveOptions options)
{
archive.CreateEntryFromFile(entry.SourceFileName, entry.EntryName, GetCompressionLevel(entry, options));
}
```
### 2. Centralize compression level logic
You currently ignore `ArchiveEntry.CompressionLevel` in the `ReadStreamAsync` branch and manually inline the `??` in other branches. A tiny helper both fixes the asymmetry and reduces cognitive load:
```csharp
private static CompressionLevel GetCompressionLevel(ArchiveEntry entry, ArchiveOptions options)
=> entry.CompressionLevel ?? options.CompressionLevel;
```
Use that in all branches (including the stream case) as shown above.
### 3. Remove redundant `Close` with `await using`
In the current code:
```csharp
await using var content = await options.ReadStreamAsync(f.SourceFileName);
await using var entryStream = entry.Open();
await content.CopyToAsync(entryStream);
entryStream.Close(); // redundant
```
`await using` already guarantees disposal, so you can safely drop the explicit `Close()`:
```csharp
await using var content = await options.ReadStreamAsync(f.SourceFileName);
await using var entryStream = entry.Open();
await content.CopyToAsync(entryStream);
```
(The helper extraction above already removes this redundancy.)
### 4. Hide conditional compilation in small compat helpers
The async directory/archive methods mix `#if` blocks and core logic. Moving the platform-specific bits into private helpers keeps the public methods small:
```csharp
public async Task ArchiveDirectoryAsync(string archiveFile, string directoryName,
CompressionLevel compressionLevel = CompressionLevel.Optimal,
bool includeBaseDirectory = false,
Encoding? encoding = null,
CancellationToken token = default)
{
if (!Directory.Exists(directoryName))
{
return;
}
var folder = Path.GetDirectoryName(archiveFile);
if (!string.IsNullOrEmpty(folder) && !Directory.Exists(folder))
{
Directory.CreateDirectory(folder);
}
await CreateFromDirectoryAsyncCompat(directoryName, archiveFile, compressionLevel, includeBaseDirectory, encoding, token);
}
private static Task CreateFromDirectoryAsyncCompat(string directoryName, string archiveFile,
CompressionLevel compressionLevel, bool includeBaseDirectory, Encoding? encoding, CancellationToken token)
{
#if NET10_0_OR_GREATER
return ZipFile.CreateFromDirectoryAsync(directoryName, archiveFile, compressionLevel, includeBaseDirectory, encoding, token);
#else
return Task.Run(() =>
{
token.ThrowIfCancellationRequested();
ZipFile.CreateFromDirectory(directoryName, archiveFile, compressionLevel, includeBaseDirectory, encoding);
}, token);
#endif
}
```
Similarly for `ExtractToDirectoryAsync`:
```csharp
public async Task<bool> ExtractToDirectoryAsync(string archiveFile, string destinationDirectoryName,
bool overwriteFiles = false, Encoding? encoding = null, CancellationToken token = default)
{
if (!Directory.Exists(destinationDirectoryName))
{
Directory.CreateDirectory(destinationDirectoryName);
}
await ExtractToDirectoryAsyncCompat(archiveFile, destinationDirectoryName, overwriteFiles, encoding, token);
return true;
}
private static Task ExtractToDirectoryAsyncCompat(string archiveFile, string destinationDirectoryName,
bool overwriteFiles, Encoding? encoding, CancellationToken token)
{
#if NET10_0_OR_GREATER
return ZipFile.ExtractToDirectoryAsync(archiveFile, destinationDirectoryName, encoding, overwriteFiles, token);
#else
return Task.Run(() =>
{
token.ThrowIfCancellationRequested();
ZipFile.ExtractToDirectory(archiveFile, destinationDirectoryName, encoding, overwriteFiles);
}, token);
#endif
}
```
These changes keep all the new features (per-entry metadata, async APIs, cancellation) intact while reducing method size, clarifying responsibilities, and centralizing rules.
</issue_to_address>
### Comment 8
<location> `src/BootstrapBlazor/Components/IpAddress/IpAddress.razor.js:16` </location>
<code_context>
}
-export function init(id) {
+export function init(id, invoke) {
const el = document.getElementById(id)
if (el === null) {
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the complex paste handling and keydown logic into small helper functions so that `init` focuses on wiring events and reads more linearly.
You can keep the new behavior while reducing `init`’s complexity by extracting the “heavy” inline logic into small helpers and flattening the `keydown` branching.
### 1. Extract paste parsing + DOM update
Move the IPv4 parsing and cell update into helpers so the event handler reads linearly:
```js
const parseIpv4FromText = raw => {
const ipRegex = /\b((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\b/
const match = raw.match(ipRegex)
if (!match) return null
return match[0].split('.').map(p => parseInt(p, 10))
}
const applyIpv4ToCells = (el, ip, parts) => {
const cells = el.querySelectorAll(".ipv4-cell")
const args = []
for (let i = 0; i < 4 && i < parts.length; i++) {
const num = parts[i]
cells[i].value = num.toString()
ip.prevValues[i] = cells[i].value
args.push(num)
}
return args
}
```
Then the paste handler becomes simpler:
```js
EventHandler.on(c, 'paste', e => {
e.preventDefault()
const raw = (e.clipboardData || window.clipboardData)?.getData('text') ?? ''
if (!raw) return
const parts = parseIpv4FromText(raw)
if (!parts) return
const args = applyIpv4ToCells(el, ip, parts)
invoke.invokeMethodAsync("TriggerUpdate", ...args)
})
```
Same functionality, significantly less cognitive load in `init`.
### 2. Split `keydown` responsibilities
You can pull the digit handling and navigation into small helpers and reduce the big `if/else` chain:
```js
const isDigitKey = e =>
(e.keyCode >= 48 && e.keyCode <= 57) || (e.keyCode >= 96 && e.keyCode <= 105)
const handleDigitInput = (c, index, e, ip) => {
ip.prevValues[index] = c.value
if (c.value === "0") {
c.value = ""
return
}
if (c.selectionStart !== c.selectionEnd) {
const selected = c.value.substring(c.selectionStart, c.selectionEnd)
const newVal = c.value.replace(selected, e.key)
if (Number(newVal) > 255) e.preventDefault()
} else {
if (Number(c.value + e.key) > 255) e.preventDefault()
}
}
const handleNavigationKeys = (el, c, index, e) => {
if (e.key === '.') {
e.preventDefault()
const next = selectCell(el, index + 1)
next.select()
return true
}
if (e.key === 'Backspace' && c.value.length === 0) {
c.value = "0"
selectCell(el, index - 1)
return true
}
const current = e.target
if (current.selectionStart === current.value.length &&
(e.key === 'Space' || e.key === 'ArrowRight')) {
e.preventDefault()
selectCell(el, index + 1)
return true
}
if (current.selectionStart === 0 && e.key === 'ArrowLeft') {
e.preventDefault()
selectCell(el, index - 1)
return true
}
return false
}
```
Then your handler becomes:
```js
EventHandler.on(c, 'keydown', e => {
if (isDigitKey(e)) {
handleDigitInput(c, index, e, ip)
return
}
if (handleNavigationKeys(el, c, index, e)) {
return
}
// ignore other keys (or explicitly allow if needed)
})
```
This keeps all current behaviors (digit validation, navigation, backspace behavior) but makes `init` mostly about wiring handlers and delegates the complex logic to focused functions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+128
to
+130
| if (Key != null) | ||
| { | ||
| builder.SetKey(Key); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Key is applied too late in the render sequence and won’t key the intended element.
In Blazor, builder.SetKey must be called immediately after OpenElement/OpenComponent and before any attributes or children so the key applies to that element. Placing it after AddContent(8, ChildContent) will key the next sequence instead of the wrapper DynamicElement. When GenerateElement is true, move SetKey(Key) to directly after builder.OpenElement(0, TagName) and before adding attributes/children, to preserve the behavior callers expect when replacing @key with Key="...".
| } | ||
| else | ||
| { | ||
| <DynamicElement class="@GetRowClassString(item, "table-row")" @key="item" |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Row key expression is treated as a literal string instead of being evaluated.
Because Key="GetKeyByITem(item)" lacks @, Razor treats it as the literal string "GetKeyByITem(item)", so all rows share the same key, breaking diffing and risking rendering issues. The same applies in the later RenderRow fragment. Please change these to Key="@GetKeyByITem(item)" (or an @ block) so the method is invoked, and consider renaming GetKeyByITem → GetKeyByItem for clarity.
Comment on lines
+54
to
+56
| if (!entryName.EndsWith(Path.DirectorySeparatorChar)) | ||
| { | ||
| entryName = $"{entryName}{Path.DirectorySeparatorChar}"; |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Using Path.DirectorySeparatorChar in ZIP entry names can create non-portable archives.
ZIP entry paths should always use '/' as the separator, independent of OS. Using Path.DirectorySeparatorChar will emit '\' on Windows and can break tools that expect standard ZIP semantics. Please normalize entryName to use '/' and append "/" explicitly for directory entries.
Comment on lines
41
to
+47
| if (options.ReadStreamAsync != null) | ||
| { | ||
| var entry = archive.CreateEntry(Path.GetFileName(f), options.CompressionLevel); | ||
| using var entryStream = entry.Open(); | ||
| await using var content = await options.ReadStreamAsync(f); | ||
| var entry = archive.CreateEntry(f.EntryName, options.CompressionLevel); | ||
| await using var content = await options.ReadStreamAsync(f.SourceFileName); | ||
| await using var entryStream = entry.Open(); | ||
| await content.CopyToAsync(entryStream); | ||
| entryStream.Close(); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Per-entry CompressionLevel is ignored when ReadStreamAsync is used.
In this branch you always use options.CompressionLevel, whereas in the file/directory branches you honor f.CompressionLevel ?? options.CompressionLevel. To keep behavior consistent and allow per-entry overrides for streamed content, use f.CompressionLevel ?? options.CompressionLevel here as well if that override is intended to apply to all entry types.
Suggested change
| if (options.ReadStreamAsync != null) | |
| { | |
| var entry = archive.CreateEntry(Path.GetFileName(f), options.CompressionLevel); | |
| using var entryStream = entry.Open(); | |
| await using var content = await options.ReadStreamAsync(f); | |
| var entry = archive.CreateEntry(f.EntryName, options.CompressionLevel); | |
| await using var content = await options.ReadStreamAsync(f.SourceFileName); | |
| await using var entryStream = entry.Open(); | |
| await content.CopyToAsync(entryStream); | |
| entryStream.Close(); | |
| if (options.ReadStreamAsync != null) | |
| { | |
| var entry = archive.CreateEntry(f.EntryName, f.CompressionLevel ?? options.CompressionLevel); | |
| await using var content = await options.ReadStreamAsync(f.SourceFileName); | |
| await using var entryStream = entry.Open(); | |
| await content.CopyToAsync(entryStream); | |
| entryStream.Close(); |
| } | ||
|
|
||
| private static async Task ArchiveFilesAsync(Stream stream, IEnumerable<string> files, ArchiveOptions? options = null) | ||
| private static async Task ArchiveFilesAsync(Stream stream, IEnumerable<ArchiveEntry> entries, ArchiveOptions? options = null) |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider refactoring the new zip helpers to extract per-entry operations, centralize compression-level logic, and hide #if blocks behind small compat methods to keep the async APIs simpler and easier to follow.
You can reduce the added complexity without changing behavior by factoring out a few focused helpers and centralizing rules.
1. Split ArchiveFilesAsync into per‑case helpers
Right now ArchiveFilesAsync has three inline branches (stream, directory, file). Extracting them makes the loop easier to read and reduces nesting:
private static async Task ArchiveFilesAsync(Stream stream, IEnumerable<ArchiveEntry> entries, ArchiveOptions? options = null)
{
options ??= new ArchiveOptions();
using var archive = new ZipArchive(stream, options.Mode, options.LeaveOpen, options.Encoding);
foreach (var entry in entries)
{
await ArchiveEntryAsync(archive, entry, options);
}
}
private static async Task ArchiveEntryAsync(ZipArchive archive, ArchiveEntry entry, ArchiveOptions options)
{
if (options.ReadStreamAsync != null)
{
await CreateEntryFromStreamAsync(archive, entry, options);
}
else if (Directory.Exists(entry.SourceFileName))
{
CreateDirectoryEntry(archive, entry, options);
}
else
{
CreateFileEntry(archive, entry, options);
}
}Then move the inline branches into small helpers:
private static async Task CreateEntryFromStreamAsync(ZipArchive archive, ArchiveEntry entry, ArchiveOptions options)
{
var zipEntry = archive.CreateEntry(entry.EntryName, GetCompressionLevel(entry, options));
await using var content = await options.ReadStreamAsync!(entry.SourceFileName);
await using var entryStream = zipEntry.Open();
await content.CopyToAsync(entryStream);
}
private static void CreateDirectoryEntry(ZipArchive archive, ArchiveEntry entry, ArchiveOptions options)
{
var entryName = entry.EntryName;
if (string.IsNullOrEmpty(entryName))
{
return;
}
if (!entryName.EndsWith(Path.DirectorySeparatorChar))
{
entryName = $"{entryName}{Path.DirectorySeparatorChar}";
}
archive.CreateEntry(entryName, GetCompressionLevel(entry, options));
}
private static void CreateFileEntry(ZipArchive archive, ArchiveEntry entry, ArchiveOptions options)
{
archive.CreateEntryFromFile(entry.SourceFileName, entry.EntryName, GetCompressionLevel(entry, options));
}2. Centralize compression level logic
You currently ignore ArchiveEntry.CompressionLevel in the ReadStreamAsync branch and manually inline the ?? in other branches. A tiny helper both fixes the asymmetry and reduces cognitive load:
private static CompressionLevel GetCompressionLevel(ArchiveEntry entry, ArchiveOptions options)
=> entry.CompressionLevel ?? options.CompressionLevel;Use that in all branches (including the stream case) as shown above.
3. Remove redundant Close with await using
In the current code:
await using var content = await options.ReadStreamAsync(f.SourceFileName);
await using var entryStream = entry.Open();
await content.CopyToAsync(entryStream);
entryStream.Close(); // redundantawait using already guarantees disposal, so you can safely drop the explicit Close():
await using var content = await options.ReadStreamAsync(f.SourceFileName);
await using var entryStream = entry.Open();
await content.CopyToAsync(entryStream);(The helper extraction above already removes this redundancy.)
4. Hide conditional compilation in small compat helpers
The async directory/archive methods mix #if blocks and core logic. Moving the platform-specific bits into private helpers keeps the public methods small:
public async Task ArchiveDirectoryAsync(string archiveFile, string directoryName,
CompressionLevel compressionLevel = CompressionLevel.Optimal,
bool includeBaseDirectory = false,
Encoding? encoding = null,
CancellationToken token = default)
{
if (!Directory.Exists(directoryName))
{
return;
}
var folder = Path.GetDirectoryName(archiveFile);
if (!string.IsNullOrEmpty(folder) && !Directory.Exists(folder))
{
Directory.CreateDirectory(folder);
}
await CreateFromDirectoryAsyncCompat(directoryName, archiveFile, compressionLevel, includeBaseDirectory, encoding, token);
}
private static Task CreateFromDirectoryAsyncCompat(string directoryName, string archiveFile,
CompressionLevel compressionLevel, bool includeBaseDirectory, Encoding? encoding, CancellationToken token)
{
#if NET10_0_OR_GREATER
return ZipFile.CreateFromDirectoryAsync(directoryName, archiveFile, compressionLevel, includeBaseDirectory, encoding, token);
#else
return Task.Run(() =>
{
token.ThrowIfCancellationRequested();
ZipFile.CreateFromDirectory(directoryName, archiveFile, compressionLevel, includeBaseDirectory, encoding);
}, token);
#endif
}Similarly for ExtractToDirectoryAsync:
public async Task<bool> ExtractToDirectoryAsync(string archiveFile, string destinationDirectoryName,
bool overwriteFiles = false, Encoding? encoding = null, CancellationToken token = default)
{
if (!Directory.Exists(destinationDirectoryName))
{
Directory.CreateDirectory(destinationDirectoryName);
}
await ExtractToDirectoryAsyncCompat(archiveFile, destinationDirectoryName, overwriteFiles, encoding, token);
return true;
}
private static Task ExtractToDirectoryAsyncCompat(string archiveFile, string destinationDirectoryName,
bool overwriteFiles, Encoding? encoding, CancellationToken token)
{
#if NET10_0_OR_GREATER
return ZipFile.ExtractToDirectoryAsync(archiveFile, destinationDirectoryName, encoding, overwriteFiles, token);
#else
return Task.Run(() =>
{
token.ThrowIfCancellationRequested();
ZipFile.ExtractToDirectory(archiveFile, destinationDirectoryName, encoding, overwriteFiles);
}, token);
#endif
}These changes keep all the new features (per-entry metadata, async APIs, cancellation) intact while reducing method size, clarifying responsibilities, and centralizing rules.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enhances the IZipArchiveService interface by adding a new ArchiveEntry record struct and refactoring archive methods to support more flexible file archiving with custom entry names and compression levels. The PR also includes several component improvements across IpAddress, CardUpload, TreeView, Table, and Drawer components, plus updates to the HTML2PDF service interface.
Key Changes:
- Introduced
ArchiveEntrystruct to allow custom archive entry names and per-file compression settings - Converted synchronous archive/extract methods to async with cancellation token support
- Added support for archiving directories as entries within zip files
- Enhanced IpAddress component with paste functionality and improved keyboard navigation
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/BootstrapBlazor/Services/IZipArchiveService.cs | Added ArchiveEntry struct and updated method signatures to use entries instead of file paths |
| src/BootstrapBlazor/Services/DefaultZipArchiveService.cs | Implemented updated interface with async methods and NET10 conditional compilation |
| test/UnitTest/Services/ZipArchiveServiceTest.cs | Updated tests to use ArchiveEntry and added comprehensive test coverage for new functionality |
| src/BootstrapBlazor/Components/IpAddress/* | Enhanced with paste support, improved keyboard navigation, and CSS class name update |
| src/BootstrapBlazor/Components/Upload/CardUpload.* | Added ActionButtonTemplate parameter for custom action buttons |
| src/BootstrapBlazor/Components/TreeView/TreeView.razor.cs | Added OnBeforeTreeItemClick callback for pre-click validation |
| src/BootstrapBlazor/Components/Table/Table.* | Added OnGetRowKey callback for custom row key generation |
| src/BootstrapBlazor/Components/Drawer/Drawer.razor.js | Fixed drawer-bar element selection to search within children |
| src/BootstrapBlazor/Components/BaseComponents/DynamicElement.cs | Added Key parameter support for Blazor rendering optimization |
| src/BootstrapBlazor/Components/BaseComponents/BootstrapModuleComponentBase.cs | Updated InvokeInitAsync to pass Interop reference to JS modules |
| src/BootstrapBlazor/Services/IHtml2Pdf.cs | Added PdfOptions parameter to all methods |
| src/BootstrapBlazor/Options/PdfOptions.cs | New class for PDF export configuration options |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -13,7 +13,7 @@ const selectCell = (el, index) => { | |||
| return c | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The function signature has been updated to include an 'invoke' parameter, but there is no JSDoc comment explaining what this parameter is for or how it should be used. Adding documentation would improve code maintainability.
Suggested change
| /** | |
| * Initializes the IPv4 address input component. | |
| * | |
| * @param {string} id The id of the root element that contains the IPv4 input cells. | |
| * @param {*} invoke An interop object that exposes `invokeMethodAsync`, used to notify the host | |
| * (for example, a .NET/Blazor component) when the IP address changes, such as | |
| * after a successful paste operation. | |
| */ |
| } | ||
|
|
||
| [Fact] | ||
| public async Task ZipArchive_Ok() |
There was a problem hiding this comment.
The test method name 'ZipArchive_Ok' is ambiguous and doesn't clearly describe what behavior is being tested. Consider renaming to something more descriptive like 'CreateZipArchiveWithDirectory_Ok' or 'CreateEntryFromFileAsync_Ok' to better indicate the test's purpose.
Suggested change
| public async Task ZipArchive_Ok() | |
| public async Task CreateEntryFromFileAsync_Ok() |
Comment on lines
+61
to
67
| // 删除文件夹 | ||
| Directory.Delete(destFolder, true); | ||
| Assert.False(Directory.Exists(destFolder)); | ||
|
|
||
| // 异步解压缩单元测试 | ||
| await archService.ExtractToDirectoryAsync(archiveFile, destFolder); | ||
| Assert.True(Directory.Exists(destFolder)); |
There was a problem hiding this comment.
The code for deleting the destination folder and re-extracting is duplicated from lines 54-59. This duplication makes the test harder to maintain and understand. The comment on line 65 states this is for "异步解压缩单元测试" (async decompression unit test), but it's testing the same method call. Consider removing the duplication or extracting it into a helper method if testing multiple scenarios is needed.
| }) | ||
|
|
||
| EventHandler.on(c, 'paste', e => { | ||
| e.preventDefault(); |
There was a problem hiding this comment.
The preventDefault() call should be placed after validation checks. If the clipboard data is empty or doesn't contain a valid IP address, the default paste behavior is still prevented, which could confuse users. Consider moving preventDefault() to after the validation on line 94 where it's confirmed the data is valid.
| return; | ||
| } | ||
|
|
||
| const ipRegex = /\b((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\b/; |
There was a problem hiding this comment.
The regex pattern for IP address validation is duplicated as a magic string. Consider extracting this to a named constant at the module level for better maintainability and potential reuse.
Comment on lines
+128
to
+131
| if (Key != null) | ||
| { | ||
| builder.SetKey(Key); | ||
| } |
There was a problem hiding this comment.
The SetKey method is called at the wrong position in the render tree. According to Blazor's rendering semantics, SetKey must be called immediately after OpenElement/OpenComponent and before any attributes or content are added. Currently, it's being called after content has been added (line 126), which means the key will not work as intended.
| Value3 = v3.ToString(); | ||
| Value4 = v4.ToString(); | ||
|
|
||
| UpdateValue(); |
There was a problem hiding this comment.
The TriggerUpdate method is missing accessibility for assistive technologies. When the IP address is updated via JavaScript (e.g., from paste), there's no indication to screen readers that the value has changed. Consider triggering a StateHasChanged call or firing an appropriate event to notify assistive technologies of the value change.
Suggested change
| UpdateValue(); | |
| UpdateValue(); | |
| InvokeAsync(StateHasChanged); |
| await using var content = await options.ReadStreamAsync(f); | ||
| var entry = archive.CreateEntry(f.EntryName, options.CompressionLevel); | ||
| await using var content = await options.ReadStreamAsync(f.SourceFileName); | ||
| await using var entryStream = entry.Open(); |
There was a problem hiding this comment.
Using 'await using' for the entryStream is unnecessary since entry.Open() returns a Stream synchronously, not an asynchronous disposable. Consider using 'using var entryStream = entry.Open();' instead for clarity and consistency with synchronous disposal patterns.
Suggested change
| await using var entryStream = entry.Open(); | |
| using var entryStream = entry.Open(); |
| /// </summary> | ||
| /// <returns></returns> | ||
| protected virtual Task InvokeInitAsync() => InvokeVoidAsync("init", Id); | ||
| protected virtual Task InvokeInitAsync() => InvokeVoidAsync("init", Id, Interop); |
There was a problem hiding this comment.
The InvokeInitAsync method now unconditionally passes the Interop parameter to all JavaScript init functions. This is a breaking change that could cause issues for existing JavaScript modules that don't expect this parameter. Consider making this conditional based on whether Interop is not null, or ensure backward compatibility by checking if the JS module accepts this parameter.
Suggested change
| protected virtual Task InvokeInitAsync() => InvokeVoidAsync("init", Id, Interop); | |
| protected virtual Task InvokeInitAsync() | |
| { | |
| // For backward compatibility, only pass Interop when it is available. | |
| return Interop is not null | |
| ? InvokeVoidAsync("init", Id, Interop) | |
| : InvokeVoidAsync("init", Id); | |
| } |
| if (OnBeforeTreeItemClick != null) | ||
| { | ||
| var ret = await OnBeforeTreeItemClick(item); | ||
| if (ret == false) |
There was a problem hiding this comment.
The expression 'A == false' can be simplified to '!A'.
Suggested change
| if (ret == false) | |
| if (!ret) |
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 #7371
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Introduce more flexible zip archiving APIs, extend HTML-to-PDF and UI components with new options and callbacks, and enhance keyboard/paste handling and key management for interactive components.
New Features:
Enhancements:
Tests: