Improve Blazor reconnection experience after the server is restarted#11
Conversation
…flect change in ResumeCircuit
There was a problem hiding this comment.
Pull request overview
This PR improves the Blazor reconnection experience when the server is restarted or circuit state is no longer available. The changes allow the client to gracefully handle failed resume attempts by providing a retry mechanism instead of immediately reloading the page.
Changes:
- Modified client-side reconnection logic to show a resume-failed state with a retry button instead of automatically reloading the page
- Updated server-side ComponentHub to not send error messages to the client when circuit state is unavailable, treating it as a valid scenario
- Added comprehensive E2E tests for reconnection scenarios without server state
- Enhanced reconnection UI to support retrying failed resume attempts
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| ReconnectModal.razor | Reordered UI elements to show the Resume button in both paused and resume-failed states, updated error message text |
| ReconnectModal.razor.js | Changed catch block to show resume-failed state instead of reloading page |
| UserSpecifiedDisplay.ts | Made reconnect and remote properties public to support state tracking across reconnection attempts |
| DefaultReconnectionHandler.ts | Removed automatic pause state update after failed reconnect to avoid UI state conflicts |
| DefaultReconnectDisplay.ts | Updated display logic to show Resume button in failed resume state, removed line that hid rejoining animation |
| ComponentHub.cs | Removed error notification to client when circuit state is unavailable, treating it as valid scenario |
| ComponentHubTest.cs | Removed test assertion for error message that is no longer sent |
| RazorComponentEndpointsStartup.cs | Added configuration option to disable circuit persistence for testing |
| ServerReconnectionWithoutStateTest.cs | Added comprehensive E2E tests for reconnection scenarios without server state |
| Failed to resume the session.<br />Please reload the page. | ||
| Failed to resume the session.<br />Please retry or reload the page. | ||
| </p> | ||
| <button id="components-resume-button" class="components-pause-visible components-resume-failed-visible"> |
There was a problem hiding this comment.
The Resume button now has both components-pause-visible and components-resume-failed-visible classes, which means it will be displayed when either state is active. However, the CSS rules in ReconnectModal.razor.css show that these classes are mutually exclusive states (lines 14 and 15 in the CSS file control when each is shown). This could lead to the button being visible when it shouldn't be if both CSS classes are active simultaneously. Consider whether this multi-class approach aligns with the intended behavior or if separate buttons for pause and resume-failed states would be clearer.
| static readonly ReconnectStateChangedEventName = 'components-reconnect-state-changed'; | ||
|
|
||
| private reconnect = false; | ||
| reconnect = true; |
There was a problem hiding this comment.
The reconnect property was changed from private reconnect = false to public reconnect = true. This is a breaking change to the internal API. The default value change from false to true could affect existing behavior. Please ensure this change is intentional and that all code paths that rely on this property have been updated accordingly.
|
|
||
| this.reconnect = options?.type === 'reconnect'; | ||
|
|
||
| this.resumeButton.style.display = 'none'; |
There was a problem hiding this comment.
The resumeButton.style.display = 'none' is set in the show() method (line 99), but this may not be necessary since the button visibility should be controlled by the state transitions in update() and failed(). Adding this line creates redundancy and could make the code harder to maintain if button visibility logic needs to be changed in the future.
| // The circuit state cannot be retrieved. It might have been deleted or expired. | ||
| // We do not send an error to the client as this is a valid scenario | ||
| // that will be handled by the client reconnection logic. |
There was a problem hiding this comment.
The comment spans three lines and provides good explanation. However, the comment could be more concise. Consider: "Circuit state unavailable (deleted or expired). Client reconnection logic will handle this scenario."
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Text; |
There was a problem hiding this comment.
The file includes unused using directives. System.Collections.Generic and System.Text don't appear to be used in the code. These should be removed to keep the code clean.
| using OpenQA.Selenium.BiDi.Communication; | ||
| using OpenQA.Selenium.DevTools; |
There was a problem hiding this comment.
These using directives for OpenQA.Selenium.BiDi.Communication and OpenQA.Selenium.DevTools don't appear to be used in the test file. Consider removing them to reduce clutter.
| Browser.Exists(By.Id("components-reconnect-modal")) | ||
| .FindElements(By.CssSelector(".components-reconnect-container button")); | ||
|
|
||
| Assert.Equal(2, buttons.Count); |
There was a problem hiding this comment.
The test expects exactly 2 buttons to be present in the reconnect modal. This hard-coded expectation is fragile - if the UI is modified to add or remove buttons in the future, this test will break. Consider using more specific selectors to find the expected buttons (e.g., by ID) rather than relying on button count.
Benchmark PR from qodo-benchmark#32