Blazor supports DisplayName for models
#19
Conversation
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Javier Calvarro Nelson <[email protected]>
WalkthroughA new generic Changes
Sequence DiagramsequenceDiagram
actor User
participant DisplayName as DisplayName<br/>Component
participant Accessor as ExpressionMemberAccessor
participant Reflection as Reflection API
participant Render as Renderer
User->>DisplayName: SetParametersAsync(For = expr)
DisplayName->>DisplayName: Validate For provided
DisplayName->>Accessor: GetDisplayName(For expression)
Accessor->>Accessor: Parse expression tree
Accessor->>Reflection: Extract MemberInfo
Reflection-->>Accessor: MemberInfo
Accessor->>Reflection: Get DisplayAttribute
Reflection-->>Accessor: DisplayAttribute or null
Accessor->>Reflection: Get DisplayNameAttribute (if needed)
Reflection-->>Accessor: DisplayNameAttribute or null
Accessor-->>Accessor: Resolve name (Display > DisplayName > member name)
Accessor-->>DisplayName: Display name string
DisplayName->>DisplayName: Cache display name
DisplayName->>Render: Trigger render
Render->>Render: BuildRenderTree
Render-->>User: Display name rendered
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Components/Web/src/Forms/ExpressionMemberAccessor.cs`:
- Around line 26-51: The cache uses Expression instances as keys so
_memberInfoCache in GetMemberInfo<TValue> never hits across new lambda
instances; replace or augment the cache to use a structural equality comparer
(e.g., a LambdaExpressionComparer equivalent) or compute a stable key from the
expression (such as a serialized/structural representation) before caching, and
update the GetMemberInfo call to use that comparer/key so identical member
access expressions across invocations resolve to the same cached MemberInfo;
ensure the comparer is used when constructing _memberInfoCache or that the
computed key logic is uniquely derived from the expression tree (referencing
_memberInfoCache, GetMemberInfo, and the LambdaExpressionComparer concept).
In
`@src/ProjectTemplates/Web.ProjectTemplates/content/BlazorWeb-CSharp/BlazorWebCSharp.1/Components/Account/Pages/Login.razor`:
- Around line 33-39: Indentation inside the div.form-floating block is
inconsistent for the Password input elements; update the block containing the
InputText (type="password" bound to Input.Password), the <label> that uses
DisplayName For="() => Input.Password", and the ValidationMessage For="() =>
Input.Password" so each of those elements is indented one level deeper (matching
the Email block's indentation) inside the div.form-floating to maintain
consistent formatting.
🧹 Nitpick comments (5)
src/ProjectTemplates/Web.ProjectTemplates/content/BlazorWeb-CSharp/BlazorWebCSharp.1/Components/Account/Pages/Register.razor (1)
32-48: Minor inconsistency in label class attributes.The
ConfirmPasswordlabel (line 46) includesclass="form-label"while thePasswordlabels (lines 32, 39) do not. For consistent styling, consider adding the same class to all labels or removing it from theConfirmPasswordlabel to match.Compare with
ChangePassword.razorwhere all labels consistently useclass="form-label".♻️ Suggested fix for consistency
<div class="form-floating mb-3"> <InputText `@bind-Value`="Input.Email" id="Input.Email" class="form-control" autocomplete="username" aria-required="true" placeholder="[email protected]" /> - <label for="Input.Email"> + <label for="Input.Email" class="form-label"> <DisplayName For="() => Input.Email" /> </label> <ValidationMessage For="() => Input.Email" class="text-danger" /> </div> <div class="form-floating mb-3"> <InputText type="password" `@bind-Value`="Input.Password" id="Input.Password" class="form-control" autocomplete="new-password" aria-required="true" placeholder="password" /> - <label for="Input.Password"> + <label for="Input.Password" class="form-label"> <DisplayName For="() => Input.Password" /> </label> <ValidationMessage For="() => Input.Password" class="text-danger" /> </div>src/Components/Web/src/Forms/DisplayName.cs (1)
26-28: Minor: Missing blank line beforeAttachmethod.For consistency with typical C# formatting, add a blank line between the property and the interface method implementation.
Suggested fix
[Parameter, EditorRequired] public Expression<Func<TValue>>? For { get; set; } + /// <inheritdoc /> void IComponent.Attach(RenderHandle renderHandle)src/Components/Web/src/Forms/ExpressionMemberAccessor.cs (1)
15-16: Unused_displayNameCachefield should be removed or utilized.
_displayNameCacheis declared but never populated or read. Either implement caching for display names inGetDisplayName(MemberInfo)or remove this dead code.♻️ Option 1: Remove unused cache
internal static class ExpressionMemberAccessor { private static readonly ConcurrentDictionary<Expression, MemberInfo> _memberInfoCache = new(); - private static readonly ConcurrentDictionary<MemberInfo, string> _displayNameCache = new();♻️ Option 2: Implement display name caching
If caching display names was intended (which could improve performance for repeated lookups), update
GetDisplayName(MemberInfo)to use the cache:public static string GetDisplayName(MemberInfo member) { ArgumentNullException.ThrowIfNull(member); + return _displayNameCache.GetOrAdd(member, static m => + { + var displayAttribute = m.GetCustomAttribute<DisplayAttribute>(); + if (displayAttribute is not null) + { + var name = displayAttribute.GetName(); + if (name is not null) + { + return name; + } + } + + var displayNameAttribute = m.GetCustomAttribute<DisplayNameAttribute>(); + if (displayNameAttribute?.DisplayName is not null) + { + return displayNameAttribute.DisplayName; + } + + return m.Name; + }); - var displayAttribute = member.GetCustomAttribute<DisplayAttribute>(); - // ... rest of implementation }And update
ClearCacheto clear both caches:private static void ClearCache() { _memberInfoCache.Clear(); + _displayNameCache.Clear(); }src/Components/Web/test/Forms/DisplayNameTest.cs (2)
176-192: Helper method uses fragile frame lookups.
RenderAndGetOutputuses.First()to find frames, which could break if frame ordering changes or if there are multiple text frames. Consider using more specific filtering or index-based lookup relative to the component frame.♻️ Suggested improvement
private static async Task<string> RenderAndGetOutput(TestHostComponent rootComponent) { var testRenderer = new TestRenderer(); var componentId = testRenderer.AssignRootComponentId(rootComponent); await testRenderer.RenderRootComponentAsync(componentId); var batch = testRenderer.Batches.Single(); - var displayLabelComponentFrame = batch.ReferenceFrames - .First(f => f.FrameType == RenderTree.RenderTreeFrameType.Component && - f.Component is DisplayName<string> or DisplayName<int> or DisplayName<DateTime>); - - // Find the text content frame within the component - var textFrame = batch.ReferenceFrames - .First(f => f.FrameType == RenderTree.RenderTreeFrameType.Text); + // Find the text content rendered by the DisplayName component + var textFrame = batch.ReferenceFrames + .Single(f => f.FrameType == RenderTree.RenderTreeFrameType.Text); return textFrame.TextContent; }
183-185: Unused variabledisplayLabelComponentFrame.The
displayLabelComponentFramevariable is assigned but never used. If it's not needed for the assertion logic, consider removing it.♻️ Suggested fix
var batch = testRenderer.Batches.Single(); - var displayLabelComponentFrame = batch.ReferenceFrames - .First(f => f.FrameType == RenderTree.RenderTreeFrameType.Component && - f.Component is DisplayName<string> or DisplayName<int> or DisplayName<DateTime>); - // Find the text content frame within the component
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/Components/Web/src/Forms/DisplayName.cssrc/Components/Web/src/Forms/ExpressionMemberAccessor.cssrc/Components/Web/src/PublicAPI.Unshipped.txtsrc/Components/Web/test/Forms/DisplayNameTest.cssrc/Components/test/E2ETest/Tests/FormsTest.cssrc/Components/test/testassets/BasicTestApp/FormsTest/DisplayNameComponent.razorsrc/Components/test/testassets/BasicTestApp/Index.razorsrc/Components/test/testassets/BasicTestApp/Resources.fr.resxsrc/Components/test/testassets/BasicTestApp/Resources.resxsrc/Components/test/testassets/BasicTestApp/TestResources.cssrc/ProjectTemplates/Web.ProjectTemplates/content/BlazorWeb-CSharp/BlazorWebCSharp.1/Components/Account/Pages/ForgotPassword.razorsrc/ProjectTemplates/Web.ProjectTemplates/content/BlazorWeb-CSharp/BlazorWebCSharp.1/Components/Account/Pages/Login.razorsrc/ProjectTemplates/Web.ProjectTemplates/content/BlazorWeb-CSharp/BlazorWebCSharp.1/Components/Account/Pages/Manage/ChangePassword.razorsrc/ProjectTemplates/Web.ProjectTemplates/content/BlazorWeb-CSharp/BlazorWebCSharp.1/Components/Account/Pages/Manage/Email.razorsrc/ProjectTemplates/Web.ProjectTemplates/content/BlazorWeb-CSharp/BlazorWebCSharp.1/Components/Account/Pages/Register.razorsrc/ProjectTemplates/Web.ProjectTemplates/content/BlazorWeb-CSharp/BlazorWebCSharp.1/Components/Account/Pages/ResetPassword.razor
🧰 Additional context used
🧬 Code graph analysis (5)
src/Components/test/E2ETest/Tests/FormsTest.cs (1)
src/Shared/E2ETesting/WaitAssert.cs (1)
Equal(26-27)
src/Components/test/testassets/BasicTestApp/TestResources.cs (1)
src/Components/Web/test/Forms/DisplayNameTest.cs (1)
TestResources(228-231)
src/Components/Web/src/Forms/ExpressionMemberAccessor.cs (2)
src/Components/Shared/src/HotReloadManager.cs (1)
HotReloadManager(11-31)src/Components/Web/src/Forms/DisplayName.cs (1)
DisplayName(16-63)
src/Components/Web/src/Forms/DisplayName.cs (1)
src/Components/Web/src/Forms/ExpressionMemberAccessor.cs (4)
ExpressionMemberAccessor(13-87)ExpressionMemberAccessor(18-24)GetDisplayName(53-74)GetDisplayName(76-81)
src/Components/Web/test/Forms/DisplayNameTest.cs (1)
src/Components/Web/src/Forms/DisplayName.cs (3)
Task(35-57)DisplayName(16-63)BuildRenderTree(59-62)
🔇 Additional comments (19)
src/Components/test/testassets/BasicTestApp/Resources.resx (1)
123-125: LGTM!The new
ProductNameresource entry is correctly structured and follows the existing pattern. This provides the default English value for localization testing of the DisplayName feature.src/Components/test/testassets/BasicTestApp/Resources.fr.resx (1)
123-125: LGTM!The French translation for
ProductNameis correct and aligns with the corresponding entry in Resources.resx, enabling proper localization testing.src/ProjectTemplates/Web.ProjectTemplates/content/BlazorWeb-CSharp/BlazorWebCSharp.1/Components/Account/Pages/Manage/ChangePassword.razor (1)
23-39: LGTM!The
DisplayNamecomponent usage is consistent across all three password fields, with proper binding to the corresponding properties. TheInputModelclass has appropriate[Display]attributes (lines 101, 107, 111) that will be resolved by the component.src/Components/test/testassets/BasicTestApp/Index.razor (1)
30-30: LGTM!The new test selector option is correctly placed in alphabetical order and follows the established naming convention for test component references.
src/ProjectTemplates/Web.ProjectTemplates/content/BlazorWeb-CSharp/BlazorWebCSharp.1/Components/Account/Pages/Manage/Email.razor (1)
48-50: LGTM!The
DisplayNamecomponent is correctly bound toInput.NewEmail, and theInputModelclass includes the appropriate[Display(Name = "New email")]attribute (line 142) for resolution.src/Components/test/testassets/BasicTestApp/TestResources.cs (1)
1-11: LGTM!The wrapper class is correctly designed to expose resource properties as public statics, which is required by
DisplayAttribute.ResourceType. The comment clearly explains the rationale.src/Components/Web/src/Forms/DisplayName.cs (1)
45-54: LGTM on change detection logic.The reference equality check for expressions aligns with how
FieldIdentifieroperates. Even when expressions are new instances each render, the underlyingMemberInfocaching inExpressionMemberAccessorensures display name resolution remains efficient.src/ProjectTemplates/Web.ProjectTemplates/content/BlazorWeb-CSharp/BlazorWebCSharp.1/Components/Account/Pages/ForgotPassword.razor (1)
28-30: LGTM!The template correctly adopts the new
DisplayNamecomponent pattern. Using[Display(Name = "Email")]as the single source of truth for both the label and validation messages improves maintainability and consistency.Also applies to: 74-74
src/Components/Web/src/PublicAPI.Unshipped.txt (1)
43-46: LGTM!The public API surface is correctly documented. The
Forproperty signature correctly reflects its nullable expression type with non-nullableTValue.src/Components/test/E2ETest/Tests/FormsTest.cs (1)
558-582: LGTM!The E2E test comprehensively covers all display name resolution scenarios:
DisplayAttribute.NameDisplayNameAttribute- Attribute precedence (Display over DisplayName)
- Fallback to property name
- Resource-based localization
The assertions align with both the unit tests and the test component's expected behavior.
src/Components/test/testassets/BasicTestApp/FormsTest/DisplayNameComponent.razor (1)
1-35: LGTM!The test component is well-structured with clear inline documentation explaining the expected behavior for each scenario. The
Productmodel covers all attribute resolution paths, and the element IDs align correctly with the E2E test assertions.src/ProjectTemplates/Web.ProjectTemplates/content/BlazorWeb-CSharp/BlazorWebCSharp.1/Components/Account/Pages/ResetPassword.razor (2)
27-43: LGTM! Clean adoption of the DisplayName component.The migration from static label text to
<DisplayName For="..." />components is consistent and correctly binds to the corresponding input properties. This approach centralizes display name resolution and supports localization through the[Display]attribute.
95-115: Model annotations are consistent with the DisplayName usage.The
[Display(Name = "...")]attributes onPassword, andConfirmPasswordproperly support the new DisplayName component bindings in the markup.src/ProjectTemplates/Web.ProjectTemplates/content/BlazorWeb-CSharp/BlazorWebCSharp.1/Components/Account/Pages/Login.razor (2)
28-30: LGTM! DisplayName component correctly bound to Email property.
153-169: Model annotations are consistent with the DisplayName usage.src/Components/Web/src/Forms/ExpressionMemberAccessor.cs (1)
53-74: Display name resolution logic is correct.The attribute precedence (
DisplayAttribute>DisplayNameAttribute> member name) follows MVC conventions. UsingdisplayAttribute.GetName()correctly supports localization whenResourceTypeis specified.src/Components/Web/test/Forms/DisplayNameTest.cs (3)
13-34: Good test for required parameter validation.The test correctly verifies that
DisplayName<TValue>throws when theForparameter is missing, and validates the exception message contains relevant keywords.
36-123: Comprehensive test coverage for display name resolution.The tests properly cover:
- Fallback to property name when no attribute is present
DisplayAttributename resolutionDisplayNameAttributename resolution- Attribute precedence (Display > DisplayName)
228-231: Test localization uses a mock resource class rather than actual resource files.
TestResources.LocalizedDisplayNameis a static property returning a hardcoded string, which verifies thatDisplayAttribute.GetName()works withResourceTypebut doesn't test actual resource file lookup. This is acceptable for unit testing the component's behavior, though E2E tests should cover real resource-based localization.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| private static MemberInfo GetMemberInfo<TValue>(Expression<Func<TValue>> accessor) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(accessor); | ||
|
|
||
| return _memberInfoCache.GetOrAdd(accessor, static expr => | ||
| { | ||
| var lambdaExpression = (LambdaExpression)expr; | ||
| var accessorBody = lambdaExpression.Body; | ||
|
|
||
| if (accessorBody is UnaryExpression unaryExpression | ||
| && unaryExpression.NodeType == ExpressionType.Convert | ||
| && unaryExpression.Type == typeof(object)) | ||
| { | ||
| accessorBody = unaryExpression.Operand; | ||
| } | ||
|
|
||
| if (accessorBody is not MemberExpression memberExpression) | ||
| { | ||
| throw new ArgumentException( | ||
| $"The provided expression contains a {accessorBody.GetType().Name} which is not supported. " + | ||
| $"Only simple member accessors (fields, properties) of an object are supported."); | ||
| } | ||
|
|
||
| return memberExpression.Member; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, run the provided ast-grep pattern to find GetOrAdd usages
ast-grep --pattern $'GetOrAdd($_, static $_ => {
$$$
})'Repository: qodo-benchmark/aspnetcore-combined-coderabbit
Length of output: 71
🏁 Script executed:
# Find the _memberInfoCache field definition to see if it uses custom equality
fd "ExpressionMemberAccessor.cs" --exec cat -n {}Repository: qodo-benchmark/aspnetcore-combined-coderabbit
Length of output: 3561
🏁 Script executed:
# Find and examine the DisplayName.cs file around lines 44-52
fd "DisplayName.cs" --exec cat -n {}Repository: qodo-benchmark/aspnetcore-combined-coderabbit
Length of output: 2672
🏁 Script executed:
# Check if there's any usage of Expression equality or custom comparers
rg -t cs "EqualityComparer|Expression.*Equal|IEqualityComparer" --max-count=20Repository: qodo-benchmark/aspnetcore-combined-coderabbit
Length of output: 50401
🏁 Script executed:
# Examine the LambdaExpressionComparer mentioned in search results
fd "LambdaExpressionComparer.cs" --exec cat -n {}Repository: qodo-benchmark/aspnetcore-combined-coderabbit
Length of output: 4763
🏁 Script executed:
# Search for any tests related to ExpressionMemberAccessor caching
rg -t cs "ExpressionMemberAccessor|_memberInfoCache" --max-count=30Repository: qodo-benchmark/aspnetcore-combined-coderabbit
Length of output: 784
🏁 Script executed:
# Check if there are tests for DisplayName component behavior
rg -t cs "DisplayName.*GetDisplayName|GetDisplayName.*test" -i --max-count=20Repository: qodo-benchmark/aspnetcore-combined-coderabbit
Length of output: 1473
🏁 Script executed:
# Search for actual usage of GetDisplayName in components to understand real-world patterns
rg -t cs "GetDisplayName\(" --context=2 --max-count=15Repository: qodo-benchmark/aspnetcore-combined-coderabbit
Length of output: 26232
Expression-based cache key won't match across invocations without structural equality comparer.
The _memberInfoCache dictionary uses Expression objects as keys with the default equality comparer, which relies on reference equality. Since lambda expressions like () => model.Property create new instances on each invocation, the cache key will never match a previously cached result—each call produces a different reference.
A structural equality comparer (similar to the existing LambdaExpressionComparer in Mvc.ViewFeatures) would be needed to enable meaningful cache hits. The current implementation provides caching overhead without actual benefit in typical usage where new expression instances are created per component render or invocation.
This may be intentional if the cache is only meant to work within a component's lifetime where the same expression instance is reused, but this should be documented or reconsidered. The hot-reload cache clearing on line 22-23 suggests awareness of expression lifecycle issues.
🤖 Prompt for AI Agents
In `@src/Components/Web/src/Forms/ExpressionMemberAccessor.cs` around lines 26 -
51, The cache uses Expression instances as keys so _memberInfoCache in
GetMemberInfo<TValue> never hits across new lambda instances; replace or augment
the cache to use a structural equality comparer (e.g., a
LambdaExpressionComparer equivalent) or compute a stable key from the expression
(such as a serialized/structural representation) before caching, and update the
GetMemberInfo call to use that comparer/key so identical member access
expressions across invocations resolve to the same cached MemberInfo; ensure the
comparer is used when constructing _memberInfoCache or that the computed key
logic is uniquely derived from the expression tree (referencing
_memberInfoCache, GetMemberInfo, and the LambdaExpressionComparer concept).
| <div class="form-floating mb-3"> | ||
| <InputText type="password" @bind-Value="Input.Password" id="Input.Password" class="form-control" autocomplete="current-password" aria-required="true" placeholder="password" /> | ||
| <label for="Input.Password" class="form-label">Password</label> | ||
| <ValidationMessage For="() => Input.Password" class="text-danger" /> | ||
| <InputText type="password" @bind-Value="Input.Password" id="Input.Password" class="form-control" autocomplete="current-password" aria-required="true" placeholder="password" /> | ||
| <label for="Input.Password" class="form-label"> | ||
| <DisplayName For="() => Input.Password" /> | ||
| </label> | ||
| <ValidationMessage For="() => Input.Password" class="text-danger" /> | ||
| </div> |
There was a problem hiding this comment.
Indentation inconsistency in the Password input block.
The elements inside this div.form-floating are not indented consistently with the Email block above (lines 26-31). The InputText, label, and ValidationMessage should be indented one level deeper inside the div.
🔧 Suggested fix
<div class="form-floating mb-3">
- <InputText type="password" `@bind-Value`="Input.Password" id="Input.Password" class="form-control" autocomplete="current-password" aria-required="true" placeholder="password" />
- <label for="Input.Password" class="form-label">
- <DisplayName For="() => Input.Password" />
- </label>
- <ValidationMessage For="() => Input.Password" class="text-danger" />
+ <InputText type="password" `@bind-Value`="Input.Password" id="Input.Password" class="form-control" autocomplete="current-password" aria-required="true" placeholder="password" />
+ <label for="Input.Password" class="form-label">
+ <DisplayName For="() => Input.Password" />
+ </label>
+ <ValidationMessage For="() => Input.Password" class="text-danger" />
</div>📝 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.
| <div class="form-floating mb-3"> | |
| <InputText type="password" @bind-Value="Input.Password" id="Input.Password" class="form-control" autocomplete="current-password" aria-required="true" placeholder="password" /> | |
| <label for="Input.Password" class="form-label">Password</label> | |
| <ValidationMessage For="() => Input.Password" class="text-danger" /> | |
| <InputText type="password" @bind-Value="Input.Password" id="Input.Password" class="form-control" autocomplete="current-password" aria-required="true" placeholder="password" /> | |
| <label for="Input.Password" class="form-label"> | |
| <DisplayName For="() => Input.Password" /> | |
| </label> | |
| <ValidationMessage For="() => Input.Password" class="text-danger" /> | |
| </div> | |
| <div class="form-floating mb-3"> | |
| <InputText type="password" `@bind-Value`="Input.Password" id="Input.Password" class="form-control" autocomplete="current-password" aria-required="true" placeholder="password" /> | |
| <label for="Input.Password" class="form-label"> | |
| <DisplayName For="() => Input.Password" /> | |
| </label> | |
| <ValidationMessage For="() => Input.Password" class="text-danger" /> | |
| </div> |
🤖 Prompt for AI Agents
In
`@src/ProjectTemplates/Web.ProjectTemplates/content/BlazorWeb-CSharp/BlazorWebCSharp.1/Components/Account/Pages/Login.razor`
around lines 33 - 39, Indentation inside the div.form-floating block is
inconsistent for the Password input elements; update the block containing the
InputText (type="password" bound to Input.Password), the <label> that uses
DisplayName For="() => Input.Password", and the ValidationMessage For="() =>
Input.Password" so each of those elements is indented one level deeper (matching
the Email block's indentation) inside the div.form-floating to maintain
consistent formatting.
Benchmark PR from qodo-benchmark#84
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.