Skip to content

Conversation

@ArgoZhang
Copy link
Member

@ArgoZhang ArgoZhang commented Nov 11, 2025

Link issues

fixes #7093

Summary By Copilot

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • Merge the latest code from the main branch

Summary by Sourcery

Streamline the Message component's rendering and interop layers, add per-message force delay support, and update tests and samples accordingly.

New Features:

  • Introduce ForceDelay option to override global message delay per message

Enhancements:

  • Refactor Message.razor to use a unified GetMessages() method and eliminate duplicated placement branches
  • Update C# interop methods to clear and dismiss individual messages by ID and simplify state management
  • Simplify JS interop by removing unused callback parameters, making close handlers async, and streamlining item removal logic
  • Enhance sample page with dynamic placement selection via radio list and use an incremental counter for message content

Tests:

  • Add a ForceDelay_Ok unit test and update existing tests to clear and dismiss by message ID

Chores:

  • Remove redundant entries in exclusion.dic

Copilot AI review requested due to automatic review settings November 11, 2025 03:28
@bb-auto bb-auto bot added the enhancement New feature or request label Nov 11, 2025
@bb-auto bb-auto bot added this to the 10.0.0 milestone Nov 11, 2025
@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Nov 11, 2025

Reviewer's Guide

Consolidated Message component rendering logic, optimized JS interop and event handling, refactored per-item clear/dismiss operations, introduced ForceDelay override with dynamic default delay support, and updated unit tests and sample demos accordingly.

Sequence diagram for per-item clear and dismiss operations

sequenceDiagram
    participant JS as "Message.razor.js"
    participant Message as "Message (C#)"
    participant User as actor "User"
    User->>JS: Clicks dismiss button
    JS->>Message: Dismiss(alertId)
    Message->>Message: Remove MessageOption by id
    Message->>Message: StateHasChanged()
    Message-->>JS: (async completion)
    User->>JS: Message auto-hides
    JS->>Message: Clear(msgId)
    Message->>Message: Remove MessageOption by id
    Message->>Message: StateHasChanged()
Loading

Class diagram for updated Message component logic

classDiagram
    class Message {
        -List<MessageOption> _messages
        -string _msgId
        +void Clear(string id)
        +ValueTask Dismiss(string id)
        +void SetPlacement(Placement placement)
        +List<MessageOption> GetMessages()
        +Task Show(MessageOption option)
    }
    class MessageOption {
        +string Content
        +string Icon
        +bool ShowDismiss
        +bool IsAutoHide
        +int Delay
        +RenderFragment ChildContent
        +Func<Task> OnDismiss
        +bool ShowShadow
        +MessageShowMode ShowMode
    }
    Message "1" *-- "*" MessageOption : manages
    class Placement {
        <<enum>>
        Top
        Bottom
    }
    MessageOption --> Placement : uses
Loading

Class diagram for dynamic default delay configuration

classDiagram
    class BootstrapBlazorOptions {
        +int MessageDelay
    }
    class IOptionsMonitor {
        +BootstrapBlazorOptions CurrentValue
    }
    class Message {
        +int DefaultDelay
    }
    Message --> IOptionsMonitor : uses for delay
    IOptionsMonitor --> BootstrapBlazorOptions : provides
Loading

File-Level Changes

Change Details Files
Consolidate rendering logic in Message.razor
  • Added GetMessages() to unify ordering based on Placement
  • Replaced dual if/for branches with single foreach over GetMessages()
  • Removed duplicated markup for icon, content, and dismiss button
src/BootstrapBlazor/Components/Message/Message.razor
src/BootstrapBlazor/Components/Message/Message.razor.cs
Simplify JS interop and event handling
  • Removed callback parameter from init signature and invocation
  • Streamlined show/close logic to update items and invoke Clear directly
  • Updated click handler to await Dismiss and properly filter msg.items
src/BootstrapBlazor/Components/Message/Message.razor.js
Refactor Clear and Dismiss to per-item operations
  • Changed Clear to accept an id and remove only the specified message
  • Converted Dismiss to ValueTask, remove message and trigger StateHasChanged
  • Removed bulk-clear logic and aligned JS init accordingly
src/BootstrapBlazor/Components/Message/Message.razor.cs
Introduce ForceDelay override and dynamic default delay
  • Implemented ForceDelay flag to override per-message Delay
  • Injected IOptionsMonitor for dynamic default delay
  • Added unit tests for ForceDelay behavior and runtime delay updates
src/BootstrapBlazor/Components/Message/Message.razor.cs
test/UnitTest/Components/MessageTest.cs
Enhance sample Messages component for runtime placement
  • Added placement selection via RadioList and _placement binding
  • Introduced _count for incremental message content
  • Replaced static placement calls with dynamic Placement property
src/BootstrapBlazor.Server/Components/Samples/Messages.razor
src/BootstrapBlazor.Server/Components/Samples/Messages.razor.cs

Assessment against linked issues

Issue Objective Addressed Explanation
#7093 Improve the performance of the Message component.

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there - I've reviewed your changes and they look great!

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `src/BootstrapBlazor/Components/Message/Message.razor.js:48-49` </location>
<code_context>
     el.classList.add('show');

     const close = () => {
-        EventHandler.off(el, 'click')
-        el.classList.remove('show');
-        const hideHandler = setTimeout(function () {
</code_context>

<issue_to_address>
**suggestion:** The close function is now unused and can be removed for clarity.

Removing unused code helps maintain clarity and reduces potential confusion.
</issue_to_address>

### Comment 2
<location> `test/UnitTest/Components/MessageTest.cs:156-161` </location>
<code_context>
+            ForceDelay = true,
+            Delay = 2000
+        };
+        await cut.InvokeAsync(() => service.Show(option, cut.Instance));
+        Assert.Contains("data-bb-delay=\"2000\"", cut.Markup);
+
+        var alert = cut.Find(".alert");
</code_context>

<issue_to_address>
**suggestion (testing):** Add assertions to verify that messages are actually removed from the DOM after Clear is called.

Consider adding an assertion to confirm that the alert element is removed from the DOM after Clear is called, to fully validate the Clear functionality.

```suggestion
        await cut.InvokeAsync(() => cut.Instance.Clear(alert.Id));
        // Assert that the alert element is removed from the DOM after Clear is called
        Assert.Empty(cut.FindAll(".alert"));

        option.ForceDelay = false;
        await cut.InvokeAsync(() => service.Show(option, cut.Instance));
        Assert.Contains("data-bb-delay=\"4000\"", cut.Markup);
        await cut.InvokeAsync(() => cut.Instance.Clear(alert.Id));
        // Assert that the alert element is removed from the DOM after Clear is called
        Assert.Empty(cut.FindAll(".alert"));
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copilot finished reviewing on behalf of ArgoZhang November 11, 2025 03:30
@codecov
Copy link

codecov bot commented Nov 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (e7d93b0) to head (d894f8f).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #7094   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          745       745           
  Lines        32556     32542   -14     
  Branches      4512      4509    -3     
=========================================
- Hits         32556     32542   -14     
Flag Coverage Δ
BB 100.00% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the Message component to improve performance by simplifying the component lifecycle and reducing unnecessary re-renders. The changes streamline message management by removing the callback mechanism and making the Clear method accept a specific message ID instead of clearing all messages.

Key changes include:

  • Refactored JavaScript interop to remove the callback parameter and call Clear/Dismiss methods directly with message IDs
  • Modified the Razor template to eliminate duplicate code by using a single loop with conditional reversal logic
  • Updated the Clear method to remove individual messages by ID rather than clearing all messages

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/BootstrapBlazor/Components/Message/Message.razor.js Removed callback parameter from init, refactored close logic to call Clear with message ID, updated event handlers
src/BootstrapBlazor/Components/Message/Message.razor.cs Changed Clear to accept message ID parameter, updated Dismiss signature to ValueTask, added GetMessages helper method
src/BootstrapBlazor/Components/Message/Message.razor Simplified template by removing duplicate code and using GetMessages() for conditional ordering
test/UnitTest/Components/MessageTest.cs Updated test calls to wrap Clear in InvokeAsync, added ForceDelay_Ok test for delay behavior
src/BootstrapBlazor.Server/Components/Samples/Messages.razor.cs Updated demo to use message counter and consolidated message instance references
src/BootstrapBlazor.Server/Components/Samples/Messages.razor Enhanced demo UI with placement selector
exclusion.dic Added spelling exclusions for 'alertdialog' and 'blazorbootstrap'

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ArgoZhang
Copy link
Member Author

@sourcery-ai review

sourcery-ai[bot]
sourcery-ai bot previously approved these changes Nov 11, 2025
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there - I've reviewed your changes - here's some feedback:

  • The sample’s _items list is initialized with square brackets instead of a valid C# collection initializer (use {}), which will fail to compile.
  • The JS interop setup doesn’t clean up event handlers or Data entries on component disposal; consider implementing DisposeAsync to remove handlers and avoid memory leaks.
  • The fallback to a 1000ms delay when MessageDelay is set to zero can be surprising—consider making this default behavior more explicit in code or via a named constant.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The sample’s `_items` list is initialized with square brackets instead of a valid C# collection initializer (use `{}`), which will fail to compile.
- The JS interop setup doesn’t clean up event handlers or Data entries on component disposal; consider implementing `DisposeAsync` to remove handlers and avoid memory leaks.
- The fallback to a 1000ms delay when `MessageDelay` is set to zero can be surprising—consider making this default behavior more explicit in code or via a named constant.

## Individual Comments

### Comment 1
<location> `src/BootstrapBlazor/Components/Message/Message.razor.js:55-64` </location>
<code_context>
     };

-    EventHandler.on(el, 'click', '.btn-close', e => {
+    EventHandler.on(el, 'click', '.btn-close', async e => {
         e.preventDefault();
         e.stopPropagation();

         const alert = e.delegateTarget.closest('.alert');
         if (alert) {
+            EventHandler.off(el, 'click')
+            alert.classList.add("d-none");
+
             const alertId = alert.getAttribute('id');
-            msg.invoke.invokeMethodAsync('Dismiss', alertId);
+            msg.items = msg.items.filter(i => i.el.id !== alertId);
+            await msg.invoke.invokeMethodAsync('Dismiss', alertId);
         }
-        close();
</code_context>

<issue_to_address>
**issue (bug_risk):** Mixing async/await with non-async event handler registration may cause unhandled promise rejections.

Wrap the async handler logic in a try/catch block to prevent silent UI failures from unhandled promise rejections.
</issue_to_address>

### Comment 2
<location> `test/UnitTest/Components/MessageTest.cs:170-150` </location>
<code_context>
+        Assert.Contains("data-bb-delay=\"1000\"", cut.Markup);
+        await cut.InvokeAsync(() => cut.Instance.Clear(alert.Id));
+
+        options.CurrentValue.MessageDelay = 0;
+        await cut.InvokeAsync(() => service.Show(option, cut.Instance));
+        Assert.Contains("data-bb-delay=\"1000\"", cut.Markup);
+        await cut.InvokeAsync(() => cut.Instance.Clear(alert.Id));
</code_context>

<issue_to_address>
**suggestion (testing):** Suggestion: Add a test for negative MessageDelay values.

Add a test with a negative MessageDelay to verify the component's behavior and ensure it handles invalid input correctly.

Suggested implementation:

```csharp
        options.CurrentValue.MessageDelay = 1000;
        await cut.InvokeAsync(() => service.Show(option, cut.Instance));
        Assert.Contains("data-bb-delay=\"1000\"", cut.Markup);
        await cut.InvokeAsync(() => cut.Instance.Clear(alert.Id));

        // Negative MessageDelay test
        options.CurrentValue.MessageDelay = -500;
        await cut.InvokeAsync(() => service.Show(option, cut.Instance));
        // Assert that negative delay is handled (e.g., defaults to 0 or a minimum value)
        Assert.DoesNotContain("data-bb-delay=\"-500\"", cut.Markup);
        Assert.Contains("data-bb-delay=\"0\"", cut.Markup); // Adjust if the component defaults to another value
        await cut.InvokeAsync(() => cut.Instance.Clear(alert.Id));

```

If the component does not default negative values to 0, adjust the assertion to match the actual default value. You may also want to check the component's implementation to ensure negative values are handled gracefully.
</issue_to_address>

### Comment 3
<location> `src/BootstrapBlazor/Components/Message/Message.razor.cs:88` </location>
<code_context>
         }
     }

+    private List<MessageOption> GetMessages()
+    {
+        var messages = new List<MessageOption>(_messages);
</code_context>

<issue_to_address>
**issue (complexity):** Consider replacing the message list reversal and removal logic with a property and helper method to streamline rendering and message management.

Here’s one way to collapse the duplication and simplify the “reverse‐if‐bottom” logic without losing any behavior:

```csharp
// replace GetMessages()
private IEnumerable<MessageOption> MessagesForRender =>
    Placement == Placement.Bottom
        ? _messages.AsEnumerable().Reverse()
        : _messages;

// helper to remove + return the removed item
private MessageOption? RemoveMessageById(string id)
{
    var msg = _messages.FirstOrDefault(x => GetItemId(x) == id);
    if (msg != null)
        _messages.Remove(msg);
    return msg;
}

[JSInvokable]
public Task Clear(string id)
{
    RemoveMessageById(id);
    StateHasChanged();
    return Task.CompletedTask;
}

[JSInvokable]
public async ValueTask Dismiss(string id)
{
    var msg = RemoveMessageById(id);
    if (msg?.OnDismiss != null)
        await msg.OnDismiss();

    StateHasChanged();
}
```

And in your `.razor` just enumerate `MessagesForRender` instead of calling `GetMessages()`. This:

- Removes the extra copy/reverse helper.
- Unifies the “find & remove” logic into one small method.
- Still calls all callbacks and updates state exactly once.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@ArgoZhang ArgoZhang merged commit 8b8ef2c into main Nov 11, 2025
7 checks passed
@ArgoZhang ArgoZhang deleted the fix-message branch November 11, 2025 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf(Message): improve performance

2 participants