feat(WebClientService): improve performance#8038
Conversation
Reviewer's GuideRefactors WebClientService client info retrieval to use a direct JS interop return value with cancellation support instead of a JSInvokable callback/TaskCompletionSource, simplifies resource disposal, updates the JS client module accordingly, and adapts unit tests to the new async pattern while tightening JSModule timeout token handling. Sequence diagram for updated WebClientService client info retrievalsequenceDiagram
participant WebClientService
participant JSRuntime
participant JSModule
participant ClientJs
WebClientService->>JSRuntime: LoadModuleByName(client)
JSRuntime-->>WebClientService: JSModule
WebClientService->>JSModule: InvokeAsync_ClientInfo(ping, token, ip.axd)
JSModule->>ClientJs: ping(ip.axd)
ClientJs-->>JSModule: ClientInfo
JSModule-->>WebClientService: ClientInfo
WebClientService->>WebClientService: set _client and RequestUrl
WebClientService->>WebClientService: GetClientInfo returns ClientInfo
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.
Hey - I've left some high level feedback:
- GetClientInfo still relies on mutable shared state (_client) as a cache while also doing async work; consider refactoring to use a local ClientInfo instance and only assign to the field once at the end to avoid stale or partially populated state in concurrent calls.
- CancellationToken passed into GetClientInfo is treated the same as any other exception and effectively ignored after logging; if cancellation is expected to be meaningful, consider detecting OperationCanceledException and returning early or rethrowing instead of continuing with IP lookup and returning cached data.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- GetClientInfo still relies on mutable shared state (_client) as a cache while also doing async work; consider refactoring to use a local ClientInfo instance and only assign to the field once at the end to avoid stale or partially populated state in concurrent calls.
- CancellationToken passed into GetClientInfo is treated the same as any other exception and effectively ignored after logging; if cancellation is expected to be meaningful, consider detecting OperationCanceledException and returning early or rethrowing instead of continuing with IP lookup and returning cached data.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 #8038 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 766 766
Lines 34157 34154 -3
Branches 4698 4696 -2
=========================================
- Hits 34157 34154 -3
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:
|
There was a problem hiding this comment.
Pull request overview
This PR updates WebClientService to fetch ClientInfo by directly awaiting a JS module call that returns data, removing the previous callback/TaskCompletionSource-based flow to reduce overhead and simplify the interop path.
Changes:
- Refactor
WebClientService.GetClientInfoto callclient.ping(url)viaInvokeAsync<T>and return the result. - Update
client.jssoping(url)returns theClientInfopayload instead of calling back into .NET. - Simplify and stabilize unit tests by removing polling/
Task.Runpatterns and adding cancellation-token coverage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTest/Services/WebClientServiceTest.cs | Updates tests to mock ping returning ClientInfo; adds cancellation-token test; adjusts error-path test. |
| test/UnitTest/Services/ConnectionHubTest.cs | Removes usage of removed WebClientService.SetData test helper pattern. |
| src/BootstrapBlazor/wwwroot/modules/client.js | Changes ping to return the client info instead of invoking a .NET callback. |
| src/BootstrapBlazor/Utils/JSModule.cs | Minor disposal syntax change for timeout CTS in InvokeVoidAsync. |
| src/BootstrapBlazor/Services/WebClientService.cs | Refactors GetClientInfo to await JS return value; removes SetData JS callback pathway and interop reference. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Link issues
fixes #8037
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Refactor WebClientService client info retrieval to use direct JS interop return values instead of callback-based invocation and improve cancellation and timeout handling.
New Features:
Bug Fixes:
Enhancements:
Tests: