Conversation
…sChanged is called during search
# Conflicts: # src/BootstrapBlazor/BootstrapBlazor.csproj
Contributor
Reviewer's GuideAdds a pre-close callback pipeline to Modal so callers can asynchronously decide whether a modal is allowed to close, wires this into the JS bootstrap modal behavior, and does minor cleanup/formatting around Modal, ModalDialog, and Dialog components. Sequence diagram for the new modal pre-close (OnClosingAsync) pipelinesequenceDiagram
actor User
participant BrowserJS as Browser_JS_modal
participant BlazorJS as Blazor_JSInterop
participant Modal as Modal_component
participant OnClosing as OnClosingAsync_callback
User->>BrowserJS: Trigger close (Escape, backdrop click, or close button)
BrowserJS->>BrowserJS: modal.close()
BrowserJS->>BlazorJS: invokeMethodAsync BeforeCloseCallback
BlazorJS->>Modal: BeforeCloseCallback()
activate Modal
alt OnClosingAsync is assigned
Modal->>OnClosing: Invoke OnClosingAsync()
activate OnClosing
OnClosing-->>Modal: Task<bool> result
deactivate OnClosing
else OnClosingAsync is null
Modal-->>BlazorJS: default true
end
Modal-->>BlazorJS: bool allowClose
deactivate Modal
BlazorJS-->>BrowserJS: bool allowClose
alt allowClose is true
BrowserJS->>BrowserJS: modal.hide()
BrowserJS->>BlazorJS: invokeMethodAsync CloseCallback
BlazorJS->>Modal: CloseCallback()
Modal-->>Modal: Execute OnCloseAsync if assigned
else allowClose is false
BrowserJS-->>User: Modal remains open
end
Updated class diagram for Modal pre-close callback supportclassDiagram
class Modal {
+Func~Task~ OnCloseAsync
+Func~Task~bool~~ OnClosingAsync
+Task Close()
+Task~bool~ BeforeCloseCallback()
+void RegisterOnClosingCallback(Func~Task~bool~~ onClosingCallback)
+void UnRegisterOnClosingCallback(Func~Task~bool~~ onClosingCallback)
}
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 1 issue, and left some high level feedback:
- The new
RegisterOnClosingCallback/UnRegisterOnClosingCallbackAPIs layer multipleFunc<Task<bool>>ontoOnClosingAsyncvia+=/-=, but the currentBeforeCloseCallbackinvocation only uses the final delegate’sboolresult; if multiple callbacks are expected, consider explicitly aggregating the invocation list (e.g., AND/OR semantics) or restrictingOnClosingAsyncto a single handler to avoid surprising behavior. - Both the JS-side
modal.close()and the C#Close()method now callBeforeCloseCallback, but they do so independently; it may be worth centralizing the pre-close check into a single code path (either always via JS or always via C#) to ensure all close flows behave identically and avoid future divergence.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `RegisterOnClosingCallback`/`UnRegisterOnClosingCallback` APIs layer multiple `Func<Task<bool>>` onto `OnClosingAsync` via `+=`/`-=`, but the current `BeforeCloseCallback` invocation only uses the final delegate’s `bool` result; if multiple callbacks are expected, consider explicitly aggregating the invocation list (e.g., AND/OR semantics) or restricting `OnClosingAsync` to a single handler to avoid surprising behavior.
- Both the JS-side `modal.close()` and the C# `Close()` method now call `BeforeCloseCallback`, but they do so independently; it may be worth centralizing the pre-close check into a single code path (either always via JS or always via C#) to ensure all close flows behave identically and avoid future divergence.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/Modal/Modal.razor.cs:91` </location>
<code_context>
+ /// <para lang="en">Callback Method Before Closing. Return true to close, false to prevent closing</para>
+ /// </summary>
+ [Parameter]
+ public Func<Task<bool>>? OnClosingAsync { get; set; }
+
/// <summary>
</code_context>
<issue_to_address>
**issue (bug_risk):** Using a multicast Func<Task<bool>> means only the last registered callback's result is honored.
Because this is a `Func<Task<bool>>` property that callers extend with `+=`/`-=`, it becomes a multicast delegate. When invoked, only the result of the last registered callback is used. If multiple callbacks are expected to collectively determine whether closing is allowed, this won’t work as intended. Instead, keep an internal collection of callbacks and explicitly combine their results (e.g., logical AND over all returned values).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7632 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 749 749
Lines 33017 33040 +23
Branches 4581 4583 +2
=========================================
+ Hits 33017 33040 +23
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 pull request adds an OnClosingAsync parameter to the Modal component, allowing consumers to intercept and potentially prevent modal closure by returning false from the callback. This callback is invoked before the modal closes in response to user interactions such as pressing the Escape key, clicking the backdrop, or clicking close buttons.
Changes:
- Added
OnClosingAsyncparameter to Modal component that returns a boolean to allow or prevent closing - Modified JavaScript handlers for Escape key and backdrop clicks to check the callback before closing
- Updated Modal.Close() method to invoke the callback before hiding the modal
- Added RegisterOnClosingCallback/UnRegisterOnClosingCallback methods for programmatic callback management
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/BootstrapBlazor/Components/Modal/Modal.razor.cs | Added OnClosingAsync parameter, BeforeCloseCallback JSInvokable method, updated Close() method, and added registration methods |
| src/BootstrapBlazor/Components/Modal/Modal.razor.js | Added modal.close() function that checks BeforeCloseCallback; updated Escape and backdrop handlers to use it |
| src/BootstrapBlazor/Components/Modal/ModalDialog.razor | Formatting changes only (attribute line breaks) |
| src/BootstrapBlazor/Components/Dialog/Dialog.razor.cs | Removed incorrect [NotNull] attributes from nullable fields |
| src/BootstrapBlazor/Components/Dialog/Dialog.razor | Formatting changes only (attribute organization) |
| src/BootstrapBlazor/BootstrapBlazor.csproj | Version bump from 10.3.1-beta06 to 10.3.1 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+290
to
+293
| public void RegisterOnClosingCallback(Func<Task<bool>> onClosingCallback) | ||
| { | ||
| OnClosingAsync += onClosingCallback; | ||
| } |
There was a problem hiding this comment.
When using delegate composition with OnClosingAsync += onClosingCallback, C# multicast delegates will only return the result of the last delegate in the invocation list. If multiple callbacks are registered using RegisterOnClosingCallback, only the last registered callback's return value will determine whether the modal closes. All callbacks will execute, but intermediate return values are discarded. This could lead to unexpected behavior where an earlier callback returns false to prevent closing, but a later callback returns true and allows it. Consider using a cache-based approach similar to _shownCallbackCache where you can iterate through all callbacks and properly aggregate their results (e.g., close only if all callbacks return true).
Comment on lines
+86
to
+92
| /// <summary> | ||
| /// <para lang="zh">关闭之前回调方法 返回 true 时关闭弹窗 返回 false 时阻止关闭弹窗</para> | ||
| /// <para lang="en">Callback Method Before Closing. Return true to close, false to prevent closing</para> | ||
| /// </summary> | ||
| [Parameter] | ||
| public Func<Task<bool>>? OnClosingAsync { get; set; } | ||
|
|
There was a problem hiding this comment.
The new OnClosingAsync parameter and related functionality lack test coverage. The existing test file has comprehensive tests for other callbacks like OnShownAsync (see ShownCallbackAsync_Ok test) and RegisterShownCallback (see RegisterShownCallback_Ok test). Consider adding tests to verify: (1) OnClosingAsync is invoked when closing the modal, (2) returning false prevents the modal from closing, (3) returning true allows the modal to close, (4) the callback is invoked for all close triggers (escape key, backdrop click, close button), and (5) RegisterOnClosingCallback and UnRegisterOnClosingCallback work correctly.
This was referenced Feb 10, 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
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 #7631
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add support for asynchronously vetoing modal closure and wire it through UI and JavaScript interactions.
New Features:
Enhancements: