Conversation
Contributor
Reviewer's GuideAdds value synchronization and validation behavior to CardUpload and related upload components so that form validation is triggered correctly when files are added or deleted, introduces a reusable UploadValidateItem model and helper extension, and extends unit tests to cover the new validation behavior and type. Sequence diagram for CardUpload file deletion triggering validationsequenceDiagram
actor User
participant CardUpload
participant UploadBase
participant EditForm
participant ValidateForm
User->>CardUpload: Click delete on file card
CardUpload->>UploadBase: OnFileDelete(uploadFile)
UploadBase->>UploadBase: Remove from UploadFiles and DefaultFileList
UploadBase->>UploadBase: UpdateValue(Files)
UploadBase->>EditForm: Notify bound field value changed
EditForm->>ValidateForm: Trigger field validation
ValidateForm->>ValidateForm: ValidateAsync for upload field
ValidateForm-->>User: Updated validation messages
Class diagram for updated upload validation componentsclassDiagram
class UploadBase_TValue_ {
<<abstract>>
- List~UploadFile~ UploadFiles
- List~UploadFile~ DefaultFileList
- object _filesCache
- Type ValueType
- TValue CurrentValue
- List~UploadFile~ Files
+ Task OnFileChange(InputFileChangeEventArgs args)
+ Task~bool~ OnFileDelete(UploadFile item)
- void UpdateValue(List~UploadFile~ items)
}
class FileListUploadBase_TValue_ {
<<abstract>>
}
class AvatarUpload {
+ bool IsInValidOnAddItem
+ string AddId
- IJSObjectReference ValidateModule
- List~ValidationResult~ _results
+ Task ToggleMessage(IReadOnlyCollection~ValidationResult~ results)
}
class CardUpload {
+ bool IsInValidOnAddItem
+ string AddId
- IJSObjectReference ValidateModule
- List~ValidationResult~ _results
+ Task ToggleMessage(IReadOnlyCollection~ValidationResult~ results)
}
class UploadValidateItem {
<<record struct>>
+ string Id
+ string ErrorMessage
}
class ValidateContextExtensions {
+ ValidationResult GetValidationResult(ValidationContext context)
+ List~UploadValidateItem~ GetInvalidItems(IReadOnlyCollection~ValidationResult~ source, bool isInValidOnAddItem, string newId)
}
UploadBase_TValue_ <|-- FileListUploadBase_TValue_
FileListUploadBase_TValue_ <|-- AvatarUpload
FileListUploadBase_TValue_ <|-- CardUpload
ValidateContextExtensions ..> UploadValidateItem : uses
AvatarUpload ..> UploadValidateItem : uses via GetInvalidItems
CardUpload ..> UploadValidateItem : uses via GetInvalidItems
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}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
ValidateForm.razor.cs, theif (propertyValue is string)branch inside the file-upload validation block is empty, which is confusing; either implement the intended handling for string values or remove the branch to keep the logic clear. - In
ValidateContextExtensions.GetInvalidItems, consider using the positional constructornew UploadValidateItem(newId, source.First().ErrorMessage)(and similarly in theSelectprojection) instead ofnew UploadValidateItem() { Id = ..., ErrorMessage = ... }, which is awkward for areadonly record structand may conflict with its intended immutability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ValidateForm.razor.cs`, the `if (propertyValue is string)` branch inside the file-upload validation block is empty, which is confusing; either implement the intended handling for string values or remove the branch to keep the logic clear.
- In `ValidateContextExtensions.GetInvalidItems`, consider using the positional constructor `new UploadValidateItem(newId, source.First().ErrorMessage)` (and similarly in the `Select` projection) instead of `new UploadValidateItem() { Id = ..., ErrorMessage = ... }`, which is awkward for a `readonly record struct` and may conflict with its intended immutability.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/ValidateForm/ValidateForm.razor.cs:512-516` </location>
<code_context>
{
// 未选择文件
- propertyValue = null;
+ if (propertyValue is string)
+ {
+
+ }
+ else if (propertyValue is IEnumerable)
+ {
+ propertyValue = null;
</code_context>
<issue_to_address>
**issue (bug_risk):** The empty `string` branch introduces differing behavior for string vs collection properties and may leave stale values on "no file selected".
Previously, `propertyValue` was always set to `null` when no file was selected; now only `IEnumerable` values are cleared, while `string` values are left unchanged because the `string` branch is a no-op. This can cause stale string values to be revalidated when no file is chosen. To keep behavior consistent, consider also setting `propertyValue` to `null` (or another clear default) for the `string` case, or refactor so both types are handled in the same way.
</issue_to_address>
### Comment 2
<location> `test/UnitTest/Components/UploadCardTest.cs:161-170` </location>
<code_context>
+ public async Task CardUpload_ValidateForm_Ok()
</code_context>
<issue_to_address>
**issue (testing):** Add explicit assertions about validation state after deleting the file to prove the new behavior.
Right now the test exercises upload, validation, and the disabled toggle, but it never verifies the validation result after the delete + submit flow, which is the main behavior added in this PR. After the final `form.Submit()` call, add an assertion for the expected validation state (for example, checking the invalid flag and/or the `is-invalid` class) so the test actually proves that deleting the file makes the form invalid again.
</issue_to_address>
### Comment 3
<location> `test/UnitTest/Components/UploadAvatarTest.cs:290-299` </location>
<code_context>
}
+ [Fact]
+ public void UploadValidateItem_Ok()
+ {
+ var type = Type.GetType("BootstrapBlazor.Components.UploadValidateItem, BootstrapBlazor");
+ Assert.NotNull(type);
+
+ var instance = Activator.CreateInstance(type, ["addId", "mock_ErrorMessage"]);
+ var propertyInfo = type.GetProperty("Id");
+ Assert.NotNull(propertyInfo);
+
+ var v = propertyInfo.GetValue(instance, null);
+ Assert.Equal("addId", v);
+
+ propertyInfo = type.GetProperty("ErrorMessage");
+ Assert.NotNull(propertyInfo);
+
+ v = propertyInfo.GetValue(instance, null);
+ Assert.Equal("mock_ErrorMessage", v);
+ }
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests around GetInvalidItems to verify mapping of ValidationResult to UploadValidateItem, especially for the add-item case.
The new logic in `ValidateContextExtensions.GetInvalidItems` (used by both `AvatarUpload` and `CardUpload`) isn’t covered by this test. Please add focused tests for `GetInvalidItems` that exercise both `isInValidOnAddItem == true` (with `newId`) and `false` (mapping from `MemberNames`/`ErrorMessage`), including edge cases like multiple `ValidationResult`s and missing `MemberNames`, so the mapping behavior is well-specified and guarded against regressions.
Suggested implementation:
```csharp
[Fact]
public void UploadValidateItem_Ok()
```
To fully implement the requested tests for `ValidateContextExtensions.GetInvalidItems`, please add the following test methods (e.g., directly after `UploadValidateItem_Ok` in `UploadAvatarTest.cs`). They are written to use reflection so they do not depend on the internal visibility of `ValidateContext` or `GetInvalidItems`:
```csharp
[Fact]
public void GetInvalidItems_MapsValidationResults_ToUploadValidateItems()
{
// Arrange
var validateContextType = Type.GetType("BootstrapBlazor.Components.ValidateContext, BootstrapBlazor");
Assert.NotNull(validateContextType);
var validateContext = Activator.CreateInstance(validateContextType);
Assert.NotNull(validateContext);
var validationResultsProperty = validateContextType.GetProperty("ValidationResults");
Assert.NotNull(validationResultsProperty);
// Build List<ValidationResult> via reflection
var validationResultType = typeof(ValidationResult);
var listType = typeof(List<>).MakeGenericType(validationResultType);
var validationResults = (IList)Activator.CreateInstance(listType)!;
// ValidationResult("Name is required", new[] { "Name" })
var vr1 = Activator.CreateInstance(
validationResultType,
"Name is required",
new[] { "Name" });
Assert.NotNull(vr1);
validationResults.Add(vr1);
// ValidationResult("Age is invalid", new[] { "Age" })
var vr2 = Activator.CreateInstance(
validationResultType,
"Age is invalid",
new[] { "Age" });
Assert.NotNull(vr2);
validationResults.Add(vr2);
// ValidationResult with no member names to verify edge behavior
var vr3 = Activator.CreateInstance(
validationResultType,
"General error",
(IEnumerable<string>?)null);
Assert.NotNull(vr3);
validationResults.Add(vr3);
validationResultsProperty.SetValue(validateContext, validationResults);
var extensionsType = Type.GetType("BootstrapBlazor.Components.ValidateContextExtensions, BootstrapBlazor");
Assert.NotNull(extensionsType);
var method = extensionsType.GetMethod("GetInvalidItems", BindingFlags.Public | BindingFlags.Static);
Assert.NotNull(method);
// Act: isInValidOnAddItem == false, so mapping should come from MemberNames/ErrorMessage
var result = method.Invoke(
null,
new object?[]
{
validateContext,
false,
null // newId is not used in this mode
});
Assert.NotNull(result);
var uploadValidateItemType = Type.GetType("BootstrapBlazor.Components.UploadValidateItem, BootstrapBlazor");
Assert.NotNull(uploadValidateItemType);
var idProperty = uploadValidateItemType.GetProperty("Id");
var errorMessageProperty = uploadValidateItemType.GetProperty("ErrorMessage");
Assert.NotNull(idProperty);
Assert.NotNull(errorMessageProperty);
var enumerable = ((IEnumerable)result!).Cast<object>().ToList();
// We expect at least the items corresponding to vr1 and vr2
Assert.True(enumerable.Count >= 2);
var item1 = enumerable[0];
var item2 = enumerable[1];
Assert.Equal("Name", idProperty.GetValue(item1));
Assert.Equal("Name is required", errorMessageProperty.GetValue(item1));
Assert.Equal("Age", idProperty.GetValue(item2));
Assert.Equal("Age is invalid", errorMessageProperty.GetValue(item2));
// If GetInvalidItems includes results with no MemberNames, assert expected behavior
if (enumerable.Count >= 3)
{
var item3 = enumerable[2];
var id3 = idProperty.GetValue(item3) as string;
var msg3 = errorMessageProperty.GetValue(item3) as string;
// Behavior for missing MemberNames is implementation-defined; assert it is stable:
// - Id is either null or empty
Assert.True(string.IsNullOrEmpty(id3));
Assert.Equal("General error", msg3);
}
}
[Fact]
public void GetInvalidItems_AddItem_UsesNewIdAndFirstErrorMessage()
{
// Arrange
var validateContextType = Type.GetType("BootstrapBlazor.Components.ValidateContext, BootstrapBlazor");
Assert.NotNull(validateContextType);
var validateContext = Activator.CreateInstance(validateContextType);
Assert.NotNull(validateContext);
var validationResultsProperty = validateContextType.GetProperty("ValidationResults");
Assert.NotNull(validationResultsProperty);
var validationResultType = typeof(ValidationResult);
var listType = typeof(List<>).MakeGenericType(validationResultType);
var validationResults = (IList)Activator.CreateInstance(listType)!;
// Multiple ValidationResult entries – add-mode should map correctly
var vr1 = Activator.CreateInstance(
validationResultType,
"Add item error 1",
new[] { "Field1" });
var vr2 = Activator.CreateInstance(
validationResultType,
"Add item error 2",
new[] { "Field2" });
Assert.NotNull(vr1);
Assert.NotNull(vr2);
validationResults.Add(vr1);
validationResults.Add(vr2);
validationResultsProperty.SetValue(validateContext, validationResults);
var extensionsType = Type.GetType("BootstrapBlazor.Components.ValidateContextExtensions, BootstrapBlazor");
Assert.NotNull(extensionsType);
var method = extensionsType.GetMethod("GetInvalidItems", BindingFlags.Public | BindingFlags.Static);
Assert.NotNull(method);
var uploadValidateItemType = Type.GetType("BootstrapBlazor.Components.UploadValidateItem, BootstrapBlazor");
Assert.NotNull(uploadValidateItemType);
var idProperty = uploadValidateItemType.GetProperty("Id");
var errorMessageProperty = uploadValidateItemType.GetProperty("ErrorMessage");
Assert.NotNull(idProperty);
Assert.NotNull(errorMessageProperty);
var newId = "new-add-id";
// Act: isInValidOnAddItem == true, newId should be used for Id
var result = method.Invoke(
null,
new object?[]
{
validateContext,
true,
newId
});
// Assert
Assert.NotNull(result);
var enumerable = ((IEnumerable)result!).Cast<object>().ToList();
Assert.True(enumerable.Count >= 1);
var first = enumerable[0];
var id = idProperty.GetValue(first);
var msg = errorMessageProperty.GetValue(first) as string;
// In add-item mode, Id should map from newId regardless of MemberNames
Assert.Equal(newId, id);
// Guard behavior: we expect the error message to come from the first ValidationResult
Assert.Equal("Add item error 1", msg);
}
```
These tests:
1. Exercise `GetInvalidItems` with `isInValidOnAddItem == false`, verifying mapping from `ValidationResult.MemberNames` and `ValidationResult.ErrorMessage`, including an edge case with missing `MemberNames`.
2. Exercise `GetInvalidItems` with `isInValidOnAddItem == true`, verifying that `newId` is used for the `UploadValidateItem.Id` and that the first `ValidationResult`’s message is used.
If the actual signature of `GetInvalidItems` or the structure of `ValidateContext` differs (e.g., different property names or parameters), you will need to adjust:
- The type names passed to `Type.GetType(...)`.
- The property name used instead of `"ValidationResults"`.
- The parameter list in the `method.Invoke(...)` calls.
</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
+512
to
+516
| if (propertyValue is string) | ||
| { | ||
|
|
||
| } | ||
| else if (propertyValue is IEnumerable) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The empty string branch introduces differing behavior for string vs collection properties and may leave stale values on "no file selected".
Previously, propertyValue was always set to null when no file was selected; now only IEnumerable values are cleared, while string values are left unchanged because the string branch is a no-op. This can cause stale string values to be revalidated when no file is chosen. To keep behavior consistent, consider also setting propertyValue to null (or another clear default) for the string case, or refactor so both types are handled in the same way.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7434 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 748 749 +1
Lines 32807 32845 +38
Branches 4552 4563 +11
=========================================
+ Hits 32807 32845 +38
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:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds validation triggering functionality when files are deleted in the CardUpload component, addressing issue #7433. The implementation ensures that form validation is re-evaluated after a file deletion, allowing required field constraints to be properly enforced.
Key changes:
- Added
UpdateValue()call on file deletion to trigger validation through the form binding mechanism - Refactored validation error handling by introducing
UploadValidateItemrecord struct and moving duplicate code to an extension method - Enhanced test coverage with comprehensive validation scenarios for file upload and deletion workflows
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/BootstrapBlazor/Components/Upload/UploadBase.cs |
Extracted UpdateValue() method and called it after file deletion to trigger validation |
src/BootstrapBlazor/Components/Upload/CardUpload.razor.cs |
Refactored validation logic to use new GetInvalidItems() extension method |
src/BootstrapBlazor/Components/Upload/AvatarUpload.razor.cs |
Refactored validation logic to use new GetInvalidItems() extension method |
src/BootstrapBlazor/Components/Upload/UploadValidateItem.cs |
New record struct to encapsulate validation item data |
src/BootstrapBlazor/Extensions/ValidateContextExtensions.cs |
Added GetInvalidItems() extension method to centralize validation item creation logic |
src/BootstrapBlazor/Components/ValidateForm/ValidateForm.razor.cs |
Updated file upload validation logic to handle different property types when no files are uploaded |
src/BootstrapBlazor/Components/Upload/FileListUploadBase.cs |
Fixed incorrect class name in XML documentation comment |
test/UnitTest/Components/UploadCardTest.cs |
Added comprehensive test covering file upload, deletion, and validation scenarios |
test/UnitTest/Components/UploadAvatarTest.cs |
Added unit test for UploadValidateItem record struct |
src/BootstrapBlazor/BootstrapBlazor.csproj |
Version bumped to 10.1.5-beta05 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await cut.InvokeAsync(() => items[0].Click()); | ||
|
|
||
| form = cut.Find("form"); | ||
| await cut.InvokeAsync(() => form.Submit()); |
There was a problem hiding this comment.
The test case is missing an assertion after submitting the form at line 224. After deleting the file (line 221) and submitting the form (line 224), the test should verify that the form is now invalid again since the Files field is marked as Required and should be empty after deletion. Consider adding an assertion like Assert.True(invalid) after line 224 to verify the validation behavior.
| propertyValue = null; | ||
| if (propertyValue is string) | ||
| { | ||
|
|
There was a problem hiding this comment.
This empty code block appears to be intentional but lacks documentation. When the propertyValue is a string type and no files are uploaded, this block does nothing, which means the string value is validated as-is. Consider adding a comment explaining why string values don't need special handling in this case, or if this is incomplete logic that needs to be implemented.
Suggested change
| // 字符串类型在未选择文件时无需特殊处理，直接按原值进行数据注解验证 |
Comment on lines
+47
to
+48
| ? [new UploadValidateItem() { Id = newId, ErrorMessage = source.First().ErrorMessage }] | ||
| : source.Select(i => new UploadValidateItem() { Id = i.MemberNames.FirstOrDefault(), ErrorMessage = i.ErrorMessage }).ToList(); |
There was a problem hiding this comment.
The object initializer syntax used here is inconsistent with the positional constructor defined in the record struct. The record struct UploadValidateItem(string? Id, string? ErrorMessage) uses positional parameters, so instances should be created using the constructor: new UploadValidateItem(newId, source.First().ErrorMessage) instead of using object initializer syntax. The same applies to the second case where it should be new UploadValidateItem(i.MemberNames.FirstOrDefault(), i.ErrorMessage).
Suggested change
| ? [new UploadValidateItem() { Id = newId, ErrorMessage = source.First().ErrorMessage }] | |
| : source.Select(i => new UploadValidateItem() { Id = i.MemberNames.FirstOrDefault(), ErrorMessage = i.ErrorMessage }).ToList(); | |
| ? [new UploadValidateItem(newId, source.First().ErrorMessage)] | |
| : source.Select(i => new UploadValidateItem(i.MemberNames.FirstOrDefault(), i.ErrorMessage)).ToList(); |
This was referenced Dec 29, 2025
This was referenced Jan 7, 2026
This was referenced Jan 21, 2026
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 #7433
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Ensure upload components properly integrate with form validation, including when files are removed, and centralize upload validation item handling.
New Features:
Bug Fixes:
Enhancements:
Tests: