Skip to content

Change priority of re-execution handling and allow client to render NotFound contents #62178

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

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented May 30, 2025

Contributes to #62153.

Description

  • Priority of re-execution middleware and rendering NotFound contents.
Old Current
We used to detect the status code in the rendering method to avoid flushing the response and hand-over the processing to re-execution middleware. We no longer do that and we removed nulling-out the response content type because it was done only to force re-execution middleware to process the request further. If re-execution middleware is set up together with the default Router, then in all the cases we should render NotFoundPage contents and if page is not available, then we can render NotFound fragment if streaming has not started.
  • In case we are streaming, we should allow the client to use enhanced navigation to render NotFound contents.
Old Current
Inject JS-redirection into the page, which changed the url and introduced inconsistent behavior in comparison to non-streaming scenarios. If enhanced navigation is available, we rely on it to render the NotFound contents but prevent the url change. This will happen on JS-initiated navigations. If enhanced navigation is off or the navigation is initiated by the browser (e.g. mistype in the url bar) then we redirect to NotFound, which includes url change.
  • Customized NotFound url when streaming:
Old Current
We used to redirect to fixed "not-found". If Router has NotFoundPage type passed, it's getting rendered, otherwise we rely on the url passed to re-execution middleware. NotFound fragment cannot be rendered this way.
  • Moving the components common for interactivity and no-interactivity scenarios to TestContentPackage.We want the same components to be tested in global interactivity application (InteractivityTests.cs that is running Components.WasmMinimal.csproj) and per-component interactivity application (NoInteractivityTests.cs that is running Components.TestServer.csproj that is not loading WasmMinimal assembly). The neatest solution was to put the common components is an external razor library.
  • I decided to break the rule of "no old tests should be removed" because old PageThatSetsNotFound and StreamingSetNotFound components were doing the same job as new tests but the new version is doing it in a
    more unified way, taking into consideration both POST and GET request, with and without enhanced navigation.

NotFound drawio (1)

@ilonatommy ilonatommy added this to the 10.0-preview6 milestone May 30, 2025
@ilonatommy ilonatommy self-assigned this May 30, 2025
@ilonatommy ilonatommy added the area-blazor Includes: Blazor, Razor Components label May 30, 2025
@ilonatommy ilonatommy requested a review from javiercn May 30, 2025 13:26
@ilonatommy
Copy link
Member Author

ilonatommy commented May 30, 2025

image
it seems the tests cannot run rn, investigating

It might be connected to an issue with dotnet test, the previous attempt before dotnet mismatch finished with visible lack of test discovery:
image

@ilonatommy ilonatommy marked this pull request as ready for review June 2, 2025 07:42
@ilonatommy ilonatommy requested a review from a team as a code owner June 2, 2025 07:42
@ilonatommy ilonatommy added the * NO MERGE * Do not merge this PR as long as this label is present. label Jun 2, 2025
@ilonatommy
Copy link
Member Author

Adding no-merge until the tests won't be able to run fully. The PR can get an early review in the meantime.

@ilonatommy ilonatommy removed the * NO MERGE * Do not merge this PR as long as this label is present. label Jun 5, 2025
@ilonatommy
Copy link
Member Author

Failures connected with Router's streaming inferencing with enhanced navigation, e.g.:

  • RedirectEnhancedGetToInternalWithErrorBoundary - turning on streaming SSR (via [StreamRendering] on the ) swaps out Blazor’s normal “diff-and-patch” path for a streaming pipeline that replaces the rendered subtree each time new HTML arrives. It happens only when NavigationManager.NavigateTo is used in ErrorBoundary in component's body (removal of boundary does not break enhanced nav). The test relies on keeping references to DOM and assuming enhanced navigation did not allow removing them. I am not sure why the rendering change happens only with ErrorBoundary.
  • StopsProcessingStreamingOutputFromPreviousRequestAfterEnhancedNav, CanPerformStreamingRendering, CanNavigateToAnotherPageWhilePreservingCommonDOMElements - navigation with a NavLink in layout does not use enhanced navigation now but streaming.

Starting investigation on why the whole page, including constant elements, changes on streaming.

@ilonatommy
Copy link
Member Author

ilonatommy commented Jun 9, 2025

In RedirectEnhancedGetToInternalWithErrorBoundary case the difference is how fast we reach quiesce. When Router is streaming, we are not getting into quite state so fast (what is preventing us? Don't know yet):

if (!quiesceTask.IsCompletedSuccessfully)

and we trigger SendStreamingUpdatesAsync that, in contrast to EmitInitializersIfNecessary, starts the response. It's important in the context of how we handle the navigation. With Router that streams we fall into a _httpContext.Response.HasStarted=true path of processing navigation:
https://github.com/dotnet/aspnetcore/blob/1415d9bbe18c8ba97e9eb5a9618b5c149a261e3e/src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.EventDispatch.cs#L105C1-L116C10

Using HttpResponseStreamWriter explains why we loose references and change the way we render the page. Finding the root cause for prolonged quiesce wait could help eliminate this difference.

Edit:
forcing waitForQuiescence: true

waitForQuiescence: result.IsPost || isErrorHandlerOrReExecuted);

fixes it but that's not a way to do it. It would break the scenarios where we really want to stream-in the changes.

@ilonatommy
Copy link
Member Author

ilonatommy commented Jun 9, 2025

In case of the other category of failing tests: with NavLink click, it's a matter of

public static RenderTreeDiff ComputeDiff(
method. We are entering it with oldTree as ampty array (why? Under investigation...) so logically, it builds the new view from scratch.
edit: this behavior is not influenced by Router streaming settings.

@ilonatommy
Copy link
Member Author

ilonatommy commented Jun 10, 2025

Functional test failures are connected with empty streaming markers added to the response, e.g. Renders_RoutingComponent:
expected: "Router component\n<p>Routed successfully</p>"
actual:

Router component
<!--bl:3--><!--bl:4--><!--bl:5--><!--bl:6--><p>Routed successfully</p><!--/bl:6--><!--/bl:5--><!--/bl:4--><!--/bl:3-->

@ilonatommy
Copy link
Member Author

The failing test have a difference in generated html, in component markers:
page 1st (with PageTitle):
image
page 2nd that we navigate to, (without PageTitle):
image

Because streaming state is inherited from parent component, PageTitle starts being in streaming state. Before adding StreamRendering to router was in non-streaming state and did not emit any marker in <!--/bl:{componentId}--> form. It can be fixed in tests, however it uncovers the consequences of using default, streaming router: all the components now will be streaming by default.

@ilonatommy ilonatommy requested a review from a team as a code owner June 11, 2025 10:16
@ilonatommy
Copy link
Member Author

ilonatommy commented Jun 13, 2025

Topics for to discuss in a follow-up:

  • Do we want to treat POST same as "response started"? Are there any good workarounds? Router would be able to render the page if we did not stop the renderer in this scenario.
  • Don't we want to artificially add enhanced-nav=on headers to requests going through "not found processing"? We would have to respect user's setting of enhanced nav but if they did not disable it, we could prevent redirection in browser navigation or form submission cases.

@ilonatommy ilonatommy requested a review from Copilot June 16, 2025 07:29
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request updates the re-execution middleware priority and enables custom NotFound content streaming for the Router along with adjustments in test assets and client-side navigation.

  • Adjusts the ordering and rendering logic for NotFound middleware and pages
  • Updates test pages and end-to-end tests to verify behavior under both streaming and non-streaming scenarios
  • Modifies enhanced navigation and streaming rendering in JS and endpoint rendering components

Reviewed Changes

Copilot reviewed 42 out of 42 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
Components.Shared/Index.razor Updates to test pages for NotFound behavior and navigation links
Components.Shared/ComponentThatSetsNotFound.razor Updated logic to trigger NotFound navigation
Components.Shared/ComponentThatPostsNotFound.razor Updated logic to trigger NotFound navigation on form submission
E2ETest files Adjusted tests to account for enhanced navigation and streaming variants
Web.JS files (NavigationEnhancement.ts, StreamingRendering.ts) Modifications to support URL change controls and streaming updates
Endpoints and Components files Updates to NotFound handling, including new constructor arguments and event dispatching

Comment on lines +51 to +58
// Attach pathFormat to HttpContext.Items early in the pipeline
context.Items[nameof(StatusCodePagesOptions)] = _options.PathFormat;

await _next(context);

// Remove pathFormat from HttpContext.Items after handler execution
context.Items.Remove(nameof(StatusCodePagesOptions));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not suggesting we change this right now, but maybe this should go into https://github.com/dotnet/aspnetcore/blob/main/src/Middleware/Diagnostics/src/StatusCodePage/StatusCodePagesFeature.cs instead.

The items dictionary is typically not used by the framework itself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that's public API 🫨

@ilonatommy ilonatommy enabled auto-merge (squash) June 16, 2025 13:51
@ilonatommy
Copy link
Member Author

ilonatommy commented Jun 16, 2025

Topics for to discuss in a follow-up:

  • Do we want to treat POST same as "response started"? Are there any good workarounds? Router would be able to render the page if we did not stop the renderer in this scenario.
  • Don't we want to artificially add enhanced-nav=on headers to requests going through "not found processing"? We would have to respect user's setting of enhanced nav but if they did not disable it, we could prevent redirection in browser navigation or form submission cases.

Most of it is addressed in this PR. Server always requests rendering NotFound contents using enhanced navigation, the client decides if it's possible based on user settings. Remaining: test POST request setting NotFound before and after event dispatch.

@ilonatommy ilonatommy merged commit 06ea51f into dotnet:main Jun 16, 2025
27 checks passed
@ilonatommy ilonatommy changed the title Change priority of re-execution handling and allow router to stream NotFound contents Change priority of re-execution handling and allow client to render NotFound contents Jun 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants