feat(INetworkMonitorService): add INetworkMonitorService service #6407
Conversation
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
Reviewer's GuideThis PR introduces a new INetworkMonitorService abstraction with a default JS-interop implementation to centralize network state monitoring and callback management, refactors the existing client-side module and indicator component to use the service, and adds sample pages, DI registration, menu integration, and updated tests. Sequence diagram for registering a network state change callbacksequenceDiagram
participant Component
participant INetworkMonitorService
participant DefaultNetowrkMonitorService
participant JSRuntime
participant JS (Browser)
Component->>INetworkMonitorService: RegisterStateChangedCallback(this, callback)
INetworkMonitorService->>DefaultNetowrkMonitorService: RegisterStateChangedCallback(this, callback)
DefaultNetowrkMonitorService->>JSRuntime: LoadModuleByName("net")
JSRuntime-->>DefaultNetowrkMonitorService: JSModule
DefaultNetowrkMonitorService->>JS (Browser): init({Invoke, OnNetworkStateChangedCallback})
JS (Browser)-->>DefaultNetowrkMonitorService: (on network state change) TriggerNetworkStateChanged(state)
DefaultNetowrkMonitorService->>Component: callback(state)
ER diagram for NetworkMonitorState changeserDiagram
NETWORK_MONITOR_STATE {
string NetworkType
double Downlink
int RTT
}
Class diagram for INetworkMonitorService and DefaultNetowrkMonitorServiceclassDiagram
class INetworkMonitorService {
+Task<NetworkMonitorState> GetNetworkMonitorState(CancellationToken token)
+Task RegisterStateChangedCallback(IComponent component, Func<NetworkMonitorState, Task> callback)
+void UnregisterStateChangedCallback(IComponent component)
}
class DefaultNetowrkMonitorService {
-JSModule? _module
-JSModule? _networkModule
-IJSRuntime _runtime
-DotNetObjectReference<DefaultNetowrkMonitorService> _interop
-ConcurrentDictionary<IComponent, Func<NetworkMonitorState, Task>> _callbacks
-bool _init
-SemaphoreSlim _semaphoreSlim
+DefaultNetowrkMonitorService(IJSRuntime jsRuntime)
+Task<NetworkMonitorState> GetNetworkMonitorState(CancellationToken token)
+Task RegisterStateChangedCallback(IComponent component, Func<NetworkMonitorState, Task> callback)
+void UnregisterStateChangedCallback(IComponent component)
+Task TriggerNetworkStateChanged(NetworkMonitorState state)
+ValueTask DisposeAsync()
}
INetworkMonitorService <|.. DefaultNetowrkMonitorService
Class diagram for NetworkMonitorIndicator refactorclassDiagram
class NetworkMonitorIndicator {
+INetworkMonitorService NetworkMonitorService
-NetworkMonitorState _state
-string _networkTypeString
-string _downlinkString
-string _rttString
+Task OnInitializedAsync()
+Task OnNetworkStateChanged(NetworkMonitorState state)
+void Dispose()
}
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}
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
- Rename the typo in DefaultNetowrkMonitorService to DefaultNetworkMonitorService for consistent naming across the API and file names.
- Unify the casing of the RTT property in getNetworkInfo (rTT vs rtt) to match the NetworkMonitorState model and avoid potential mapping issues.
- The _indicators field in NetworkMonitorIndicator is no longer used after moving to the service-based model—consider removing it to clean up unused state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Rename the typo in DefaultNetowrkMonitorService to DefaultNetworkMonitorService for consistent naming across the API and file names.
- Unify the casing of the RTT property in getNetworkInfo (rTT vs rtt) to match the NetworkMonitorState model and avoid potential mapping issues.
- The _indicators field in NetworkMonitorIndicator is no longer used after moving to the service-based model—consider removing it to clean up unused state.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/wwwroot/modules/net.js:29` </location>
<code_context>
- Data.set(id, {
- onlineStateChanged,
- offlineStateChanged
+ registerBootstrapBlazorModule("NetworkMonitor", null, () => {
+ EventHandler.on(window, 'online', onlineStateChanged);
+ EventHandler.on(window, 'offline', offlineStateChanged);
</code_context>
<issue_to_address>
Event handler registration is now deferred to module registration.
Please verify that deferring event handler registration does not cause race conditions or missed events if the module registers after the page load.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/wwwroot/modules/net.js:49` </location>
<code_context>
updateState(navigator.connection);
}
-
-export async function dispose(id) {
- const nt = Data.get(id);
- Data.remove(id);
-
- if (nt) {
- const { onlineStateChanged, offlineStateChanged } = nt;
- EventHandler.off(window, 'online', onlineStateChanged);
- EventHandler.off(window, 'offline', offlineStateChanged);
- }
-}
</code_context>
<issue_to_address>
The 'dispose' function has been removed, which may lead to event handler leaks.
Without 'dispose', 'online' and 'offline' event handlers are never unregistered, which can cause memory leaks if the module is re-initialized. Please assess if a cleanup method is still required.
</issue_to_address>
### Comment 3
<location> `test/UnitTest/Components/NetworkMonitorIndicatorTest.cs:11` </location>
<code_context>
{
[Fact]
- public async Task NetworkMonitorIndicator_Ok()
+ public void NetworkMonitorIndicator_Ok()
{
var cut = Context.RenderComponent<NetworkMonitorIndicator>(pb =>
</code_context>
<issue_to_address>
Test method NetworkMonitorIndicator_Ok is now synchronous but previously tested async state changes.
Ensure tests still simulate network state changes and verify UI updates, as the previous async checks are no longer present.
</issue_to_address>
### Comment 4
<location> `src/BootstrapBlazor/Components/NetworkMonitor/NetworkMonitorIndicator.razor.cs:86` </location>
<code_context>
+ return Task.CompletedTask;
+ }
+
+ private void Dispose(bool disposing)
+ {
+ if (disposing)
</code_context>
<issue_to_address>
Consider simplifying the Dispose implementation by removing the Dispose(bool) pattern and finalizer suppression, and directly unregistering the callback in Dispose().
```csharp
// Remove the full Dispose(bool) pattern and finalizer suppression.
// Just implement IDisposable.Dispose() and call UnregisterStateChangedCallback.
public partial class NetworkMonitorIndicator : IDisposable
{
// ... existing code ...
protected override async Task OnInitializedAsync()
{
await base.OnInitializedAsync();
await NetworkMonitorService.RegisterStateChangedCallback(this, OnNetworkStateChanged);
}
private async Task OnNetworkStateChanged(NetworkMonitorState state)
{
_state = state;
await InvokeAsync(StateHasChanged);
}
// Replace this:
// private void Dispose(bool disposing)
// {
// if (disposing)
// {
// NetworkMonitorService.UnregisterStateChangedCallback(this);
// }
// }
//
// public void Dispose()
// {
// Dispose(true);
// GC.SuppressFinalize(this);
// }
// With this:
public void Dispose()
{
NetworkMonitorService.UnregisterStateChangedCallback(this);
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| Data.set(id, { | ||
| onlineStateChanged, | ||
| offlineStateChanged | ||
| registerBootstrapBlazorModule("NetworkMonitor", null, () => { |
There was a problem hiding this comment.
issue (bug_risk): Event handler registration is now deferred to module registration.
Please verify that deferring event handler registration does not cause race conditions or missed events if the module registers after the page load.
| navigator.connection.onchange = e => { | ||
| updateState(e.target); | ||
| } | ||
|
|
There was a problem hiding this comment.
issue (bug_risk): The 'dispose' function has been removed, which may lead to event handler leaks.
Without 'dispose', 'online' and 'offline' event handlers are never unregistered, which can cause memory leaks if the module is re-initialized. Please assess if a cleanup method is still required.
| { | ||
| [Fact] | ||
| public async Task NetworkMonitorIndicator_Ok() | ||
| public void NetworkMonitorIndicator_Ok() |
There was a problem hiding this comment.
issue (testing): Test method NetworkMonitorIndicator_Ok is now synchronous but previously tested async state changes.
Ensure tests still simulate network state changes and verify UI updates, as the previous async checks are no longer present.
| await InvokeAsync(StateHasChanged); | ||
| } | ||
|
|
||
| private void Dispose(bool disposing) |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the Dispose implementation by removing the Dispose(bool) pattern and finalizer suppression, and directly unregistering the callback in Dispose().
// Remove the full Dispose(bool) pattern and finalizer suppression.
// Just implement IDisposable.Dispose() and call UnregisterStateChangedCallback.
public partial class NetworkMonitorIndicator : IDisposable
{
// ... existing code ...
protected override async Task OnInitializedAsync()
{
await base.OnInitializedAsync();
await NetworkMonitorService.RegisterStateChangedCallback(this, OnNetworkStateChanged);
}
private async Task OnNetworkStateChanged(NetworkMonitorState state)
{
_state = state;
await InvokeAsync(StateHasChanged);
}
// Replace this:
// private void Dispose(bool disposing)
// {
// if (disposing)
// {
// NetworkMonitorService.UnregisterStateChangedCallback(this);
// }
// }
//
// public void Dispose()
// {
// Dispose(true);
// GC.SuppressFinalize(this);
// }
// With this:
public void Dispose()
{
NetworkMonitorService.UnregisterStateChangedCallback(this);
}
}
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 #6406
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add a new network monitoring service with JS interop, refactor the network module and indicator component to use this service, and include sample demos and tests for real-time network state updates.
New Features:
Enhancements:
Documentation:
Tests: