From 55b615f96031a5c750fb65acdfc62d55c6e1c7cd Mon Sep 17 00:00:00 2001
From: Safia Abdalla <safia@microsoft.com>
Date: Wed, 22 Jul 2020 15:54:55 -0700
Subject: [PATCH] Don't render route component if OnNavigateAsync task
 in-progress

---
 .../Components/src/Routing/Router.cs          | 31 ++++++++++++-------
 .../test/E2ETest/Tests/RoutingTest.cs         | 23 ++++++++++++++
 .../RouterTest/TestRouterWithOnNavigate.razor | 17 +++++++++-
 3 files changed, 59 insertions(+), 12 deletions(-)

diff --git a/src/Components/Components/src/Routing/Router.cs b/src/Components/Components/src/Routing/Router.cs
index ab95ee2c6334..8470101904a9 100644
--- a/src/Components/Components/src/Routing/Router.cs
+++ b/src/Components/Components/src/Routing/Router.cs
@@ -152,6 +152,19 @@ private void RefreshRouteTable()
 
         internal virtual void Refresh(bool isNavigationIntercepted)
         {
+            // If an `OnNavigateAsync` task is currently in progress, then wait
+            // for it to complete before rendering. Note: because _previousOnNavigateTask
+            // is initialized to a CompletedTask on initialization, this will still
+            // allow first-render to complete successfully.
+            if (_previousOnNavigateTask.Status != TaskStatus.RanToCompletion)
+            {
+                if (Navigating != null)
+                {
+                    _renderHandle.Render(Navigating);
+                }
+                return;
+            }
+
             RefreshRouteTable();
 
             var locationPath = NavigationManager.ToBaseRelativePath(_locationAbsolute);
@@ -248,19 +261,15 @@ internal async Task RunOnNavigateWithRefreshAsync(string path, bool isNavigation
             var previousTask = _previousOnNavigateTask;
             var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
             _previousOnNavigateTask = tcs.Task;
-            try
-            {
-                // And pass an indicator for the previous task to the currently running one.
-                var shouldRefresh = await RunOnNavigateAsync(path, previousTask);
-                if (shouldRefresh)
-                {
-                    Refresh(isNavigationIntercepted);
-                }
-            }
-            finally
+
+            // And pass an indicator for the previous task to the currently running one.
+            var shouldRefresh = await RunOnNavigateAsync(path, previousTask);
+            tcs.SetResult();
+            if (shouldRefresh)
             {
-                tcs.SetResult();
+                Refresh(isNavigationIntercepted);
             }
+
         }
 
         private void OnLocationChanged(object sender, LocationChangedEventArgs args)
diff --git a/src/Components/test/E2ETest/Tests/RoutingTest.cs b/src/Components/test/E2ETest/Tests/RoutingTest.cs
index 8c369e336165..ea90f9ebb20a 100644
--- a/src/Components/test/E2ETest/Tests/RoutingTest.cs
+++ b/src/Components/test/E2ETest/Tests/RoutingTest.cs
@@ -567,6 +567,29 @@ public void OnNavigate_CanRenderUIForExceptions()
             Assert.NotNull(errorUiElem);
         }
 
+        [Fact]
+        public void OnNavigate_DoesNotRenderWhileOnNavigateExecuting()
+        {
+            var app = Browser.MountTestComponent<TestRouterWithOnNavigate>();
+
+            // Navigate to a route
+            SetUrlViaPushState("/WithParameters/name/Abc");
+
+            // Click the button to trigger a re-render
+            var button = app.FindElement(By.Id("trigger-rerender"));
+            button.Click();
+
+            // Assert that the parameter route didn't render
+            Browser.DoesNotExist(By.Id("test-info"));
+
+            // Navigate to another page to cancel the previous `OnNavigateAsync`
+            // task and trigger a re-render on its completion
+            SetUrlViaPushState("/LongPage1");
+
+            // Confirm that the route was rendered
+            Browser.Equal("This is a long page you can scroll.", () => app.FindElement(By.Id("test-info")).Text);
+        }
+
         private long BrowserScrollY
         {
             get => (long)((IJavaScriptExecutor)Browser).ExecuteScript("return window.scrollY");
diff --git a/src/Components/test/testassets/BasicTestApp/RouterTest/TestRouterWithOnNavigate.razor b/src/Components/test/testassets/BasicTestApp/RouterTest/TestRouterWithOnNavigate.razor
index 5baa82fc005f..7b0c289b56e6 100644
--- a/src/Components/test/testassets/BasicTestApp/RouterTest/TestRouterWithOnNavigate.razor
+++ b/src/Components/test/testassets/BasicTestApp/RouterTest/TestRouterWithOnNavigate.razor
@@ -1,5 +1,9 @@
 @using Microsoft.AspNetCore.Components.Routing
 
+@using System.Threading
+
+<button @onclick="TriggerRerender" id="trigger-rerender">Trigger Rerender</button>
+
 <Router AppAssembly="@typeof(BasicTestApp.Program).Assembly" OnNavigateAsync="@OnNavigateAsync">
     <Navigating>
         <div style="padding: 20px;background-color:blue;color:white;" id="loading-banner">
@@ -21,7 +25,8 @@
     {
         { "LongPage1", new Func<NavigationContext, Task>(TestLoadingPageShows) },
         { "LongPage2", new Func<NavigationContext, Task>(TestOnNavCancel) },
-        { "Other", new Func<NavigationContext, Task>(TestOnNavException) }
+        { "Other", new Func<NavigationContext, Task>(TestOnNavException) },
+        {"WithParameters/name/Abc", new Func<NavigationContext, Task>(TestRefreshHandling)}
     };
 
     private async Task OnNavigateAsync(NavigationContext args)
@@ -50,4 +55,14 @@
         await Task.CompletedTask;
         throw new Exception("This is an uncaught exception.");
     }
+
+    public static async Task TestRefreshHandling(NavigationContext args)
+    {
+        await Task.Delay(Timeout.Infinite, args.CancellationToken);
+    }
+
+    private void TriggerRerender()
+    {
+        Console.WriteLine("Nothing to see here, just an even to trigger a re-render...");
+    }
 }