Allow hot reload refresh script injection for 404 status code #51027
Conversation
ilonatommy
added
Area-AspNetCore
|
This PR is targeting |
There was a problem hiding this comment.
Pull Request Overview
This PR allows the hot reload refresh script to be injected for 404 status codes in addition to the existing 200 and 500 codes. The change addresses an issue where the browser refresh functionality wasn't working for 404 pages during development.
Key changes:
- Extended status code filtering to include 404 responses
- Added debug logging when script injection is skipped
- Improved test coverage with parameterized tests for both supported and unsupported scenarios
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/BuiltInTools/BrowserRefresh/ResponseStreamWrapper.cs |
Updated status code filtering logic to include 404 and added logging for skipped injections |
src/BuiltInTools/BrowserRefresh/BrowserRefreshMiddleware.cs |
Added new logging method for script injection skip events |
test/Microsoft.AspNetCore.Watch.BrowserRefresh.Tests/BrowserRefreshMiddlewareTest.cs |
Replaced single test with comprehensive parameterized tests covering multiple status codes and content types |
|
Thanks for your PR, @@ilonatommy. |
| response.Headers.Remove(HeaderNames.ContentEncoding); | ||
|
|
||
| _pipe = new Pipe(); | ||
| var gzipStream = new GZipStream(_pipe.Reader.AsStream(leaveOpen: true), CompressionMode.Decompress, leaveOpen: true); |
There was a problem hiding this comment.
Not changed in the PR, but do we want to dispose this stream at some point?
There was a problem hiding this comment.
We could store it same way as _baseStream and use the existing disposal methods to clean up after it.
There was a problem hiding this comment.
Merging to fix it asap.
|
/backport to release/10.0.1xx |
|
Started backporting to release/10.0.1xx: https://github.com/dotnet/sdk/actions/runs/18097996562 |
3 participants
Fixes dotnet/aspnetcore#63831.
Tested locally on the issue's reproduction. The change fixes hot reload actions after the re-execution with
UseStatusCodePagesWithReExecuteheppened.Changes:
before:
after (404 does not log it after the fix, it's only to show how logging would be visible to the user):
Allow refresh script injection for 404 status codes. This PR's goal is to fix the reported issue. I do not fully understand the filtering mechanism and it's possible that it requires more changes than just adding 404 code. Dan's question is valid here and further changes might be discussed and submitted later.
Cover the filtering mechanism with corresponding unit tests. We used to test only 200 case. I replaced this test with injection OK / injection not OK tests.