Skip to content

Wrap OnNavigateTo callback with GetErrorHandledTask for proper exception logging #62414

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Jun 19, 2025

Exceptions thrown in the OnNavigateTo callback were not being logged because the callback was passed directly to NavigationManager.Initialize() without error handling. When RemoteNavigationManager invokes this callback using the fire-and-forget pattern (_ = _onNavigateTo(absoluteUriString)), any exceptions are silently discarded.

This change wraps the OnNavigateTo callback with GetErrorHandledTask, following the same pattern already established for the OnNotFound handler in the same method.

Before:

((IHostEnvironmentNavigationManager)navigationManager)?.Initialize(
    GetContextBaseUri(httpContext.Request), 
    GetFullUri(httpContext.Request), 
    OnNavigateTo);

After:

((IHostEnvironmentNavigationManager)navigationManager)?.Initialize(
    GetContextBaseUri(httpContext.Request), 
    GetFullUri(httpContext.Request), 
    uri => 
    {
        _ = GetErrorHandledTask(OnNavigateTo(uri));
        return Task.CompletedTask;
    });

The GetErrorHandledTask method catches exceptions and logs them appropriately, ensuring that navigation errors are visible for debugging rather than being silently ignored.

Fixes #62413.


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Copy link
Contributor

Greetings human! You've submitted a PR that modifies code that is shared with https://github.com/dotnet/runtime . Please make sure you synchronize this code with the changes in that repo!

@github-actions github-actions bot added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label Jun 19, 2025
@Copilot Copilot AI changed the title [WIP] Make sure exceptions from OnNavigateTo are logged Wrap OnNavigateTo callback with GetErrorHandledTask for proper exception logging Jun 19, 2025
@Copilot Copilot AI requested a review from ilonatommy June 19, 2025 10:19
Copilot finished work on behalf of ilonatommy June 19, 2025 10:19
@ilonatommy
Copy link
Member

Make a test in src/Components/Components/test/NavigationManagerTest.cs for the case when OnNavigateTo would throw and check if it got logged.

@@ -83,7 +83,11 @@ internal async Task InitializeStandardComponentServicesAsync(
IFormCollection? form = null)
{
var navigationManager = httpContext.RequestServices.GetRequiredService<NavigationManager>();
((IHostEnvironmentNavigationManager)navigationManager)?.Initialize(GetContextBaseUri(httpContext.Request), GetFullUri(httpContext.Request), OnNavigateTo);
((IHostEnvironmentNavigationManager)navigationManager)?.Initialize(GetContextBaseUri(httpContext.Request), GetFullUri(httpContext.Request), uri =>
Copy link
Member

@ilonatommy ilonatommy Jun 19, 2025

Choose a reason for hiding this comment

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

Check out

void Initialize(string baseUri, string uri, Func<string, Task> onNavigateTo) =>
. We want to use Initialize(string baseUri, string uri, Func<string, Task> onNavigateTo)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use the correct Initialize(string baseUri, string uri, Func<string, Task> onNavigateTo) method signature. The lambda now directly returns the Task from GetErrorHandledTask(OnNavigateTo(uri)) instead of wrapping it and returning Task.CompletedTask. Fixed in commit 80512e6.

Copy link
Contributor Author

Copilot AI commented Jun 19, 2025

Make a test in src/Components/Components/test/NavigationManagerTest.cs for the case when OnNavigateTo would throw and check if it got logged.

I've added the requested test in NavigationManagerTest.cs. The test OnNavigateToCallback_WhenThrows_ShouldBeHandledGracefully verifies that exceptions from the OnNavigateTo callback are properly captured and handled gracefully without crashing the navigation system. This ensures that the error handling mechanism works correctly for the callback that was wrapped with GetErrorHandledTask in the original fix.

Fixed in commit 949150a.

@Copilot Copilot AI requested a review from ilonatommy June 19, 2025 10:40
Copilot finished work on behalf of ilonatommy June 19, 2025 10:40
Copilot finished work on behalf of ilonatommy June 19, 2025 10:50
@gfoidl gfoidl added area-blazor Includes: Blazor, Razor Components and removed needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically labels Jun 20, 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 Attention: Shared Code Modified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make sure exceptions from OnNavigateTo are logged
3 participants