vaadin / flow

Vaadin Flow is a Java framework binding Vaadin web components to Java. This is part of Vaadin 10+.
Apache License 2.0
619 stars 167 forks source link

fix: queued navigate with React Router #19985

Closed platosha closed 1 month ago

platosha commented 1 month ago

Fixes #19839

github-actions[bot] commented 1 month ago

Test Results

1 139 files  + 2  1 139 suites  +2   1h 10m 2s :stopwatch: + 2m 16s 7 405 tests + 6  7 355 :white_check_mark: + 6  50 :zzz: ±0  0 :x: ±0  7 735 runs   - 31  7 677 :white_check_mark:  - 29  58 :zzz:  - 2  0 :x: ±0 

Results for commit ad8f36dd. ± Comparison against base commit 8b7f0b1e.

:recycle: This comment has been updated with latest results.

caalador commented 1 month ago

Seems that when we are not logged in and click the Navigate to admin view in 1 second button in the tests we do navigate to /admin then /my/login/page which seems fine but then we get one more navigate to /my/login/page which does a proceed on the blocker even though we have a server roundtrip going which then calls proceed again after the last call already let the blocker proceed and is already in the unblocked state

caalador commented 1 month ago

Actually it seems it is /admin goes to the server and then /my/login/page releases the blocker before the /admin returns. Blocking the last my/login call still seems to create the blocker state fault.

caalador commented 1 month ago

Just as a test dopping any blocker requests when a server roundtrip is waiting does fix the error, but I could expect this to have some drawbacks for other requests in the set requiring a roundtrip when multiple changes are made

diff --git forkSrcPrefix/flow-server/src/main/resources/com/vaadin/flow/server/frontend/Flow.tsx forkDstPrefix/flow-server/src/main/resources/com/vaadin/flow/server/frontend/Flow.tsx
index b073570de0e7926bcdb2b37ec34abc815e8d1297..c03527c624525b9b362e96e8570510344f197bbb 100644
--- forkSrcPrefix/flow-server/src/main/resources/com/vaadin/flow/server/frontend/Flow.tsx
+++ forkDstPrefix/flow-server/src/main/resources/com/vaadin/flow/server/frontend/Flow.tsx
@@ -257,6 +257,7 @@ function Flow() {
     const {pathname, search, hash} = useLocation();
     const navigated = useRef<boolean>(false);
     const fromAnchor = useRef<boolean>(false);
+    const serverRoundtrip = useRef<boolean>(false);
     const containerRef = useRef<RouterContainer | undefined>(undefined);
     const queuedNavigate = useQueuedNavigate();

@@ -306,11 +307,15 @@ function Flow() {
     const vaadinNavigateEventHandler = useCallback((event: CustomEvent<{state: unknown, url: string, replace?: boolean, callback: boolean}>) => {
         const path = '/' + event.detail.url;
         navigated.current = !event.detail.callback;
+        if(event.detail.replace && path === window.location.pathname) {
+            return;
+        }
         queuedNavigate(path, { state: event.detail.state, replace: event.detail.replace});
     }, [navigate]);

     const redirect = useCallback((path: string) => {
         return (() => {
+            serverRoundtrip.current = false;
             navigate(path, {replace: true});
         });
     }, [navigate]);
@@ -339,12 +344,16 @@ function Flow() {

     useEffect(() => {
         if (blocker.state === 'blocked') {
+            if(serverRoundtrip.current) {
+                return;
+            }
             // Do not skip server round-trip if navigation originates from a click on a link
             if (navigated.current && !fromAnchor.current) {
                 blocker.proceed();
                 return;
             }
             fromAnchor.current = false;
+            serverRoundtrip.current = true;
             const {pathname, search} = blocker.location;
             const routes = ((window as any)?.Vaadin?.routesConfig || []) as AgnosticRouteObject[];
             let matched = matchRoutes(Array.from(routes), pathname);
@@ -357,10 +366,12 @@ function Flow() {
                         prevent() {
                             blocker.reset();
                             navigated.current = false;
+                            serverRoundtrip.current = false;
                         },
                         redirect,
                         continue() {
                             blocker.proceed();
+                            serverRoundtrip.current = false;
                         }
                     }, router);
                 navigated.current = true;
@@ -376,13 +387,16 @@ function Flow() {
                             containerRef.current.serverConnected = (cancel) => {
                                 if (cancel) {
                                     blocker.reset();
+                                    serverRoundtrip.current = false;
                                 } else {
                                     blocker.proceed();
+                                    serverRoundtrip.current = false;
                                 }
                             }
                         } else {
                             // permitted navigation: proceed with the blocker
                             blocker.proceed();
+                            serverRoundtrip.current = false;
                         }
                     });
             }
sonarcloud[bot] commented 1 month ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

vaadin-bot commented 1 month ago

Hi @platosha and @caalador, when i performed cherry-pick to this commit to 24.4, i have encountered the following issue. Can you take a look and pick it manually? Error Message: Error: Command failed: git cherry-pick 9ceb7d7278ed01f040fdb366672d21b989711d63 error: could not apply 9ceb7d7278... fix: queued navigate with React Router (#19985) hint: After resolving the conflicts, mark them with hint: "git add/rm ", then run hint: "git cherry-pick --continue". hint: You can instead skip this commit with "git cherry-pick --skip". hint: To abort and get back to the state before "git cherry-pick", hint: run "git cherry-pick --abort".