Apply BitFileInput improvements (#12742)#12744
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughBitFileInput adds validation options, previews, removal callbacks, browser file handling, accessibility attributes, customizable styles, color and size variants, expanded demos, and tests covering the new behavior. ChangesBitFileInput improvements
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant BitFileInput
participant JavaScriptFileInput
participant Browser
User->>BitFileInput: Select, drop, or paste files
BitFileInput->>JavaScriptFileInput: Initialize input and drag/drop handlers
JavaScriptFileInput->>Browser: Assign files and dispatch change
Browser->>BitFileInput: Return file metadata and preview URLs
BitFileInput->>BitFileInput: Validate files and invoke OnChange
User->>BitFileInput: Remove a file
BitFileInput->>JavaScriptFileInput: Remove file and revoke preview URL
BitFileInput->>BitFileInput: Invoke OnRemove and OnChange
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 8 minutes. |
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.ts (1)
15-25: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winIndex can collide after
removeFile+ append.
lastIndexis derived from the current count of items forid, not the max existingindex. After a file is removed mid-list and then more files are appended, the new item'sindexcan collide with a remaining item'sindex(differentfileId, sameid+index). This metadata is returned to the Blazor side, so any consumer relying on index for ordering/keys can get duplicate/inconsistent values.Repro: append 3 files (indices 0,1,2) → remove the item at index 1 → append 1 more file → new item gets index
0 + 2 = 2, colliding with the surviving item at index 2.🐛 Proposed fix — derive next index from max existing index, not count
- const lastIndex = append ? FileInput._fileInputs.filter(f => f.id === id).length : 0; + const existingItems = FileInput._fileInputs.filter(f => f.id === id); + const lastIndex = append && existingItems.length > 0 + ? Math.max(...existingItems.map(f => f.index)) + 1 + : 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.ts` around lines 15 - 25, Update the `lastIndex` calculation in the file-mapping flow to derive the next index from the maximum existing `index` for the same `id`, rather than the current item count; use zero as the starting offset when no items exist. Preserve the existing `index + lastIndex` assignment and all other file metadata behavior.
🧹 Nitpick comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.razor (1)
34-35: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAvoid setting both
aria-labelandaria-labelledbysimultaneously.When
AriaLabelis set and the label button is rendered, both attributes end up on the<input>. Per spec, aria-labelledby will take precedence over aria-label if both are applied, making the user-suppliedAriaLabelsilently ignored in that case.♻️ Proposed fix
- aria-label="`@AriaLabel`" - aria-labelledby="@(LabelTemplate is null && HideLabel is false ? _buttonId : null)" + aria-label="@(LabelTemplate is null && HideLabel is false ? null : AriaLabel)" + aria-labelledby="@(LabelTemplate is null && HideLabel is false ? _buttonId : null)"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.razor` around lines 34 - 35, Update the accessibility attributes on the input in BitFileInput so aria-labelledby is assigned only when AriaLabel is not set, while preserving the existing label-button condition. Ensure AriaLabel takes precedence and never renders simultaneously with aria-labelledby.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.razor`:
- Around line 76-89: Update the remove-button markup in BitFileInput so it is
disabled and cannot invoke RemoveFile(file) when IsEnabled is false. Preserve
the existing enabled-state behavior, styling, and accessibility attributes.
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.razor.cs`:
- Around line 252-256: Update RemoveFile to return immediately when IsEnabled is
false, before performing any file-removal logic. Keep the existing empty-file
guard and removal behavior unchanged when the component is enabled.
- Around line 252-281: Update RemoveFile after mutating _files to explicitly
reset each remaining file’s IsValid state and MaxCountErrorMessage, then reapply
the MaxCount validation by position so only files beyond MaxCount remain
invalid. Ensure this recalculation occurs before invoking OnChange and
StateHasChanged, while preserving the existing removal callbacks and JavaScript
operations.
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.ts`:
- Around line 45-66: Update onDragEnter and onDragLeave to also return
immediately when inputElement.disabled is true, preventing drag highlight
classes and dragCounter changes for disabled inputs while preserving the
existing file checks and enabled-input behavior.
---
Outside diff comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.ts`:
- Around line 15-25: Update the `lastIndex` calculation in the file-mapping flow
to derive the next index from the maximum existing `index` for the same `id`,
rather than the current item count; use zero as the starting offset when no
items exist. Preserve the existing `index + lastIndex` assignment and all other
file metadata behavior.
---
Nitpick comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.razor`:
- Around line 34-35: Update the accessibility attributes on the input in
BitFileInput so aria-labelledby is assigned only when AriaLabel is not set,
while preserving the existing label-button condition. Ensure AriaLabel takes
precedence and never renders simultaneously with aria-labelledby.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0fa0f64f-63d1-4da0-b8e9-4874782194c8
📒 Files selected for processing (12)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.scsssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInputClassStyles.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInputInfo.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInputJsRuntimeExtensions.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/FileInput/BitFileInputDemo.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/FileInput/BitFileInputDemo.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/FileInput/BitFileInputDemo.razor.samples.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/FileInput/BitFileInputDemo.razor.scsssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/FileInput/BitFileInputTests.cs
| @if (ShowRemoveButton) | ||
| { | ||
| var removeIcon = BitIconInfo.From(RemoveButtonIcon, RemoveButtonIconName ?? "Delete"); | ||
|
|
||
| <button @onclick="() => RemoveFile(file)" | ||
| type="button" | ||
| title="Remove" | ||
| aria-label="@($"Remove {file.Name}")" | ||
| style="@Styles?.RemoveButton" | ||
| class="bit-fin-rbt @Classes?.RemoveButton"> | ||
| <i style="@Styles?.RemoveIcon" class="@removeIcon?.GetCssClasses() @Classes?.RemoveIcon" aria-hidden="true" /> | ||
| </button> | ||
| } | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove button isn't gated by IsEnabled.
The remove button has no disabled attribute and its @onclick="() => RemoveFile(file)" isn't guarded, so files can be removed while the component is disabled. See consolidated comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.razor`
around lines 76 - 89, Update the remove-button markup in BitFileInput so it is
disabled and cannot invoke RemoveFile(file) when IsEnabled is false. Preserve
the existing enabled-state behavior, styling, and accessibility attributes.
| public async Task RemoveFile(BitFileInputInfo? fileInfo = null) | ||
| { | ||
| if (_files.Any() is false) return; | ||
|
|
||
| if (fileInfo is null) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
RemoveFile doesn't check IsEnabled, unlike Browse()/ReadContentAsync().
Browse() and ReadContentAsync() both early-return if (IsEnabled is false). RemoveFile skips this guard, so files can still be removed programmatically (and via the remove button, see BitFileInput.razor) while the component is disabled. See consolidated comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.razor.cs`
around lines 252 - 256, Update RemoveFile to return immediately when IsEnabled
is false, before performing any file-removal logic. Keep the existing empty-file
guard and removal behavior unchanged when the component is enabled.
| public async Task RemoveFile(BitFileInputInfo? fileInfo = null) | ||
| { | ||
| if (_files.Any() is false) return; | ||
|
|
||
| if (fileInfo is null) | ||
| { | ||
| var removedFiles = _files.ToArray(); | ||
|
|
||
| _files.Clear(); | ||
|
|
||
| await _js.BitFileInputClear(UniqueId); | ||
|
|
||
| foreach (var file in removedFiles) | ||
| { | ||
| await OnRemove.InvokeAsync(file); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| _files.Remove(fileInfo); | ||
| if (_files.Remove(fileInfo) is false) return; | ||
|
|
||
| await _js.BitFileInputRemoveFile(UniqueId, fileInfo.FileId); | ||
|
|
||
| await OnRemove.InvokeAsync(fileInfo); | ||
| } | ||
|
|
||
| await OnChange.InvokeAsync([.. _files]); | ||
|
|
||
| StateHasChanged(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
RemoveFile doesn't recompute MaxCount validity after mutating _files.
HandleOnChange invalidates files beyond MaxCount by position (_files.Skip(MaxCount)), but RemoveFile never re-runs this check. After a single-file removal, files that should now fall back within the limit stay marked invalid (stale MaxCountErrorMessage), while the actual current excess may go unflagged. Note ValidateFile never resets IsValid = true, so simply re-invoking it won't fix this — the reset needs to be explicit.
Example: 5 files, MaxCount = 3 → files at index 3,4 invalid. Remove file at index 0 → new list [F1,F2,F3,F4] (count 4, only F4 should now be invalid), but F3 stays incorrectly invalid since it was never reset.
🐛 Proposed fix
public async Task RemoveFile(BitFileInputInfo? fileInfo = null)
{
if (_files.Any() is false) return;
if (fileInfo is null)
{
var removedFiles = _files.ToArray();
_files.Clear();
await _js.BitFileInputClear(UniqueId);
foreach (var file in removedFiles)
{
await OnRemove.InvokeAsync(file);
}
}
else
{
if (_files.Remove(fileInfo) is false) return;
await _js.BitFileInputRemoveFile(UniqueId, fileInfo.FileId);
await OnRemove.InvokeAsync(fileInfo);
}
+ var maxCountMessage = MaxCountErrorMessage ?? "The maximum number of files is exceeded";
+ foreach (var file in _files.Where(f => f.Message == maxCountMessage))
+ {
+ file.IsValid = true;
+ file.Message = null;
+ }
+
+ if (MaxCount > 0 && _files.Count > MaxCount)
+ {
+ foreach (var file in _files.Skip(MaxCount))
+ {
+ if (file.IsValid is false) continue;
+
+ file.IsValid = false;
+ file.Message = maxCountMessage;
+ }
+ }
+
await OnChange.InvokeAsync([.. _files]);
StateHasChanged();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async Task RemoveFile(BitFileInputInfo? fileInfo = null) | |
| { | |
| if (_files.Any() is false) return; | |
| if (fileInfo is null) | |
| { | |
| var removedFiles = _files.ToArray(); | |
| _files.Clear(); | |
| await _js.BitFileInputClear(UniqueId); | |
| foreach (var file in removedFiles) | |
| { | |
| await OnRemove.InvokeAsync(file); | |
| } | |
| } | |
| else | |
| { | |
| _files.Remove(fileInfo); | |
| if (_files.Remove(fileInfo) is false) return; | |
| await _js.BitFileInputRemoveFile(UniqueId, fileInfo.FileId); | |
| await OnRemove.InvokeAsync(fileInfo); | |
| } | |
| await OnChange.InvokeAsync([.. _files]); | |
| StateHasChanged(); | |
| } | |
| public async Task RemoveFile(BitFileInputInfo? fileInfo = null) | |
| { | |
| if (_files.Any() is false) return; | |
| if (fileInfo is null) | |
| { | |
| var removedFiles = _files.ToArray(); | |
| _files.Clear(); | |
| await _js.BitFileInputClear(UniqueId); | |
| foreach (var file in removedFiles) | |
| { | |
| await OnRemove.InvokeAsync(file); | |
| } | |
| } | |
| else | |
| { | |
| if (_files.Remove(fileInfo) is false) return; | |
| await _js.BitFileInputRemoveFile(UniqueId, fileInfo.FileId); | |
| await OnRemove.InvokeAsync(fileInfo); | |
| } | |
| var maxCountMessage = MaxCountErrorMessage ?? "The maximum number of files is exceeded"; | |
| foreach (var file in _files.Where(f => f.Message == maxCountMessage)) | |
| { | |
| file.IsValid = true; | |
| file.Message = null; | |
| } | |
| if (MaxCount > 0 && _files.Count > MaxCount) | |
| { | |
| foreach (var file in _files.Skip(MaxCount)) | |
| { | |
| if (file.IsValid is false) continue; | |
| file.IsValid = false; | |
| file.Message = maxCountMessage; | |
| } | |
| } | |
| await OnChange.InvokeAsync([.. _files]); | |
| StateHasChanged(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.razor.cs`
around lines 252 - 281, Update RemoveFile after mutating _files to explicitly
reset each remaining file’s IsValid state and MaxCountErrorMessage, then reapply
the MaxCount validation by position so only files beyond MaxCount remain
invalid. Ensure this recalculation occurs before invoking OnChange and
StateHasChanged, while preserving the existing removal callbacks and JavaScript
operations.
| function onDragEnter(e: DragEvent) { | ||
| e.preventDefault(); | ||
| if (!hasFiles(e)) return; | ||
|
|
||
| dragCounter++; | ||
| dropZoneElement.classList.add(...dragClasses); | ||
| } | ||
|
|
||
| function onDragLeave(e: DragEvent) { | ||
| function onDragOver(e: DragEvent) { | ||
| e.preventDefault(); | ||
| } | ||
|
|
||
| function onDrop(e: DragEvent) { | ||
| function onDragLeave(e: DragEvent) { | ||
| e.preventDefault(); | ||
| inputElement.files = e.dataTransfer!.files; | ||
| if (!hasFiles(e)) return; | ||
|
|
||
| dragCounter--; | ||
| if (dragCounter <= 0) { | ||
| dragCounter = 0; | ||
| dropZoneElement.classList.remove(...dragClasses); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Dragging highlight shown even when the input is disabled.
onDragEnter/onDragLeave only gate on hasFiles(e), not inputElement.disabled. So a disabled BitFileInput still shows the "dragging" highlight on drag-over, even though onDrop silently no-ops for disabled inputs. This misleads users into thinking the drop will succeed.
🐛 Proposed fix — skip the highlight when disabled
function onDragEnter(e: DragEvent) {
e.preventDefault();
- if (!hasFiles(e)) return;
+ if (inputElement.disabled || !hasFiles(e)) return;
dragCounter++;
dropZoneElement.classList.add(...dragClasses);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function onDragEnter(e: DragEvent) { | |
| e.preventDefault(); | |
| if (!hasFiles(e)) return; | |
| dragCounter++; | |
| dropZoneElement.classList.add(...dragClasses); | |
| } | |
| function onDragLeave(e: DragEvent) { | |
| function onDragOver(e: DragEvent) { | |
| e.preventDefault(); | |
| } | |
| function onDrop(e: DragEvent) { | |
| function onDragLeave(e: DragEvent) { | |
| e.preventDefault(); | |
| inputElement.files = e.dataTransfer!.files; | |
| if (!hasFiles(e)) return; | |
| dragCounter--; | |
| if (dragCounter <= 0) { | |
| dragCounter = 0; | |
| dropZoneElement.classList.remove(...dragClasses); | |
| } | |
| } | |
| function onDragEnter(e: DragEvent) { | |
| e.preventDefault(); | |
| if (inputElement.disabled || !hasFiles(e)) return; | |
| dragCounter++; | |
| dropZoneElement.classList.add(...dragClasses); | |
| } | |
| function onDragOver(e: DragEvent) { | |
| e.preventDefault(); | |
| } | |
| function onDragLeave(e: DragEvent) { | |
| e.preventDefault(); | |
| if (!hasFiles(e)) return; | |
| dragCounter--; | |
| if (dragCounter <= 0) { | |
| dragCounter = 0; | |
| dropZoneElement.classList.remove(...dragClasses); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileInput/BitFileInput.ts` around
lines 45 - 66, Update onDragEnter and onDragLeave to also return immediately
when inputElement.disabled is true, preventing drag highlight classes and
dragCounter changes for disabled inputs while preserving the existing file
checks and enabled-input behavior.
1 participant
closes #12742
Summary by CodeRabbit