-
-
Notifications
You must be signed in to change notification settings - Fork 362
feat(NetworkMonitor): add NetworkMonitor component #6405
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideIntroduces a network‐monitoring UI and service component using JS interop to track online/offline and connection metrics, integrates it into the main layout with new styles and localization, and covers functionality with unit tests. Sequence diagram for network state change handlingsequenceDiagram
participant Browser
participant JSModule as net.js
participant NetworkMonitor
participant NetworkMonitorIndicator
Browser->>JSModule: Network state changes (online/offline or connection info)
JSModule->>NetworkMonitor: invokeMethodAsync(TriggerOnlineStateChanged/TriggerNetworkStateChanged)
NetworkMonitor->>NetworkMonitorIndicator: OnNetworkStateChanged callback
NetworkMonitorIndicator->>NetworkMonitorIndicator: Update UI state
Class diagram for new NetworkMonitor componentsclassDiagram
class NetworkMonitorState {
+bool IsOnline
+string? NetworkType
+double? Downlink
+int RTT
}
class NetworkMonitor {
+Func<NetworkMonitorState, Task>? OnNetworkStateChanged
+List<string>? Indicators
-NetworkMonitorState _state
+Task TriggerOnlineStateChanged(bool online)
+Task TriggerNetworkStateChanged(NetworkMonitorState state)
}
class NetworkMonitorIndicator {
+string? Title
+Placement PopoverPlacement
+string? Trigger
-NetworkMonitorState _state
-List<string> _indicators
-string _networkTypeString
-string _downlinkString
-string _rttString
+Task OnNetworkStateChanged(NetworkMonitorState state)
}
NetworkMonitorIndicator --> NetworkMonitor : uses
NetworkMonitorIndicator --> NetworkMonitorState : has
NetworkMonitor --> NetworkMonitorState : has
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
- Implement a Dispose/OnAfterRender cleanup in the NetworkMonitor component to call the JS ‘dispose’ method and remove event listeners when the component is destroyed.
- Add a feature‐detection guard or fallback path for browsers that don’t support navigator.connection to prevent runtime errors in unsupported environments.
- Replace the hardcoded Task.Delay in tests with a more deterministic synchronization mechanism (e.g. awaiting state changes or using event callbacks) to avoid flaky timing issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Implement a Dispose/OnAfterRender cleanup in the NetworkMonitor component to call the JS ‘dispose’ method and remove event listeners when the component is destroyed.
- Add a feature‐detection guard or fallback path for browsers that don’t support navigator.connection to prevent runtime errors in unsupported environments.
- Replace the hardcoded Task.Delay in tests with a more deterministic synchronization mechanism (e.g. awaiting state changes or using event callbacks) to avoid flaky timing issues.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/wwwroot/modules/net.js:12` </location>
<code_context>
+ downlink, networkType: effectiveType, rTT: rtt
+ });
+ }
+ navigator.connection.onchange = e => {
+ updateState(e.target);
+ }
</code_context>
<issue_to_address>
Potential lack of feature detection for navigator.connection.
Add a check to ensure navigator.connection and its onchange property exist before using them to prevent errors in unsupported browsers.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/wwwroot/modules/net.js:7` </location>
<code_context>
+export function init(id, options) {
+ const { invoke, onlineStateChangedCallback, onNetworkStateChangedCallback, indicators } = options;
+ const updateState = nt => {
+ const { downlink, effectiveType, rtt } = nt;
+ invoke.invokeMethodAsync(onNetworkStateChangedCallback, {
+ downlink, networkType: effectiveType, rTT: rtt
+ });
+ }
</code_context>
<issue_to_address>
Inconsistent property casing for 'rTT' in the object sent to .NET.
The property name should be 'RTT' to match the C# class and avoid deserialization problems.
</issue_to_address>
### Comment 3
<location> `src/BootstrapBlazor/Components/NetworkMonitor/NetworkMonitorIndicator.razor:15` </location>
<code_context>
+ <div>@_state.NetworkType</div>
+ </div>
+ <div class="bb-nt-item">
+ <span>Downlink:</span>
+ <div>@_state.Downlink M/s</div>
+ </div>
+ <div class="bb-nt-item">
</code_context>
<issue_to_address>
Unit for downlink speed may be misleading.
Update the label to 'Mbps' to accurately reflect the standard unit for downlink speed.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
<span>Downlink:</span>
<div>@_state.Downlink M/s</div>
=======
<span>Downlink:</span>
<div>@_state.Downlink Mbps</div>
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `src/BootstrapBlazor/Components/NetworkMonitor/NetworkMonitorIndicator.razor:11` </location>
<code_context>
+ <Template>
+ <div class="bb-nt-main">
+ <div class="bb-nt-item">
+ <span>NetworkType:</span>
+ <div>@_state.NetworkType</div>
+ </div>
+ <div class="bb-nt-item">
</code_context>
<issue_to_address>
Hardcoded labels may not be localized.
Consider replacing the hardcoded labels with the existing localized strings from the code-behind to support localization.
Suggested implementation:
```
<span>@NetworkTypeText:</span>
```
```
<span>@DownlinkText:</span>
```
```
<span>@RTTText:</span>
```
Make sure that `NetworkTypeText`, `DownlinkText`, and `RTTText` are defined as localized string properties in the code-behind file (NetworkMonitorIndicator.razor.cs or @code block), and that they retrieve the appropriate localized values.
</issue_to_address>
### Comment 5
<location> `test/UnitTest/Components/NetworkMonitorIndicatorTest.cs:19` </location>
<code_context>
+ });
+
+ var com = cut.FindComponent<NetworkMonitor>();
+ await cut.InvokeAsync(() => com.Instance.TriggerNetworkStateChanged(new NetworkMonitorState
+ {
+ IsOnline = false,
+ NetworkType = "4g",
+ Downlink = 10.0,
+ RTT = 50
+ }));
+ Assert.DoesNotContain("offline", cut.Markup);
+
+ await cut.InvokeAsync(() => com.Instance.TriggerOnlineStateChanged(true));
</code_context>
<issue_to_address>
Missing test for the 'offline' class when the network is offline.
Please add a test to check that the 'offline' class appears in the markup when the network is offline, to ensure the UI updates correctly.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
await cut.InvokeAsync(() => com.Instance.TriggerNetworkStateChanged(new NetworkMonitorState
{
IsOnline = false,
NetworkType = "4g",
Downlink = 10.0,
RTT = 50
}));
Assert.DoesNotContain("offline", cut.Markup);
await cut.InvokeAsync(() => com.Instance.TriggerOnlineStateChanged(true));
Assert.DoesNotContain("offline", cut.Markup);
=======
await cut.InvokeAsync(() => com.Instance.TriggerNetworkStateChanged(new NetworkMonitorState
{
IsOnline = false,
NetworkType = "4g",
Downlink = 10.0,
RTT = 50
}));
Assert.Contains("offline", cut.Markup);
await cut.InvokeAsync(() => com.Instance.TriggerOnlineStateChanged(true));
Assert.DoesNotContain("offline", cut.Markup);
>>>>>>> REPLACE
</suggested_fix>
### Comment 6
<location> `test/UnitTest/Components/NetworkMonitorIndicatorTest.cs:45` </location>
<code_context>
+ });
+ });
+
+ await cut.InvokeAsync(() => cut.Instance.TriggerOnlineStateChanged(false));
+ Assert.NotNull(state);
+ Assert.False(state.IsOnline);
+ }
+}
</code_context>
<issue_to_address>
Missing test for TriggerNetworkStateChanged with various NetworkMonitorState values.
Please add tests covering different NetworkMonitorState property combinations to verify correct callback and state updates across all scenarios.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
}
+}
=======
}
[Theory]
[InlineData(true, true)]
[InlineData(true, false)]
[InlineData(false, true)]
[InlineData(false, false)]
public async Task NetworkMonitor_TriggerNetworkStateChanged_UpdatesStateCorrectly(bool isOnline, bool isConnectionExpensive)
{
NetworkMonitorState? callbackState = null;
var cut = Context.RenderComponent<NetworkMonitor>(pb =>
{
pb.Add(a => a.OnNetworkStateChanged, v =>
{
callbackState = v;
return Task.CompletedTask;
});
});
var testState = new NetworkMonitorState
{
IsOnline = isOnline,
IsConnectionExpensive = isConnectionExpensive
};
await cut.InvokeAsync(() => cut.Instance.TriggerNetworkStateChanged(testState));
Assert.NotNull(callbackState);
Assert.Equal(isOnline, callbackState!.IsOnline);
Assert.Equal(isConnectionExpensive, callbackState.IsConnectionExpensive);
}
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/BootstrapBlazor/Components/NetworkMonitor/NetworkMonitorIndicator.razor
Outdated
Show resolved
Hide resolved
src/BootstrapBlazor/Components/NetworkMonitor/NetworkMonitorIndicator.razor
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6405 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 718 722 +4
Lines 31726 31790 +64
Branches 4478 4482 +4
=========================================
+ Hits 31726 31790 +64
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:
|
Link issues
fixes #6404
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add network monitoring support by creating NetworkMonitor and NetworkMonitorIndicator components with accompanying JS interop module, integrate them into the layout with styling, and include unit tests
New Features:
Enhancements:
Tests: