Skip to content
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

fix: multiple fast navigate calls #20446

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

Conversation

caalador
Copy link
Contributor

Fix issue where a slow connection
and fast navigate calls throws
exception due to faulty blocker
state change.

Fixes #20404

Fix issue where a slow connection
and fast `navigate` calls throws
exception due to faulty blocker
state change.

Fixes #20404
Copy link

github-actions bot commented Nov 12, 2024

Test Results

1 156 files  ±0  1 156 suites  ±0   1h 36m 8s ⏱️ + 3m 55s
7 498 tests ±0  7 445 ✅ ±0  53 💤 ±0  0 ❌ ±0 
7 843 runs  +5  7 780 ✅ +5  63 💤 ±0  0 ❌ ±0 

Results for commit 7848db7. ± Comparison against base commit 76d5e58.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@mshabarov mshabarov left a comment

Choose a reason for hiding this comment

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

The patch looks good.
Smoke tested it, couldn't spot anything weird.
However, I couldn't reproduce the original issue.

Another pair of eyes would be beneficial.

@caalador
Copy link
Contributor Author

Easiest way to replicate original issue is to have a hilla layout and 2 flow layouts, setting the network throttling to slow 4g , then navigating to a flow view from a flow view through the layout menu and clicking 2 times on the menu item.

Copy link
Contributor

@mshabarov mshabarov left a comment

Choose a reason for hiding this comment

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

We talked with @platosha on Friday, this patch slightly changes how navigation works: with a slow network, when you first time click on the menu link and immediately click on the second one, the application will ignore the second navigation and basically all other navigations until the first one completes.
Maybe it's better to queue these navigations and apply the latest in the end. Flow navigation works like this IIRC.
@platosha promised to see how we can do it.
@caalador do you think it's better to queue like I described?

@caalador
Copy link
Contributor Author

We could push the navigate to queuedNavigate instead of canceling the navigation. I think we have enough information to do that.

The issue is that when the programmer uses react-router navigate() directly when on a Flow view it bypasses all the features for navgation we have except for the blocker and this will start multiple blockers at the same time setting the blocker state multiple times for the same instance as the latter one overwrites the previous one without checking if it is blocking already or not.

@mshabarov mshabarov self-requested a review November 18, 2024 12:46
Copy link

sonarcloud bot commented Nov 18, 2024

Copy link
Contributor

@mshabarov mshabarov left a comment

Choose a reason for hiding this comment

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

Thanks for the tip, the exception has eventually appeared for me and after the latest changes I don't see it anymore and the multiple navigations are queued and handled as I expected - the last click wins in the end.

I think we can try adding a test that simulates the same steps, I wonder if these codes works:

public class UseBlockerIT extends ChromeDeviceTest {

    @Test
    public void test() {
        ChromeDriver driver = (ChromeDriver) getDriver();
        DevTools devTools = driver.getDevTools();
        devTools.send(Network.enable(Optional.empty(), Optional.empty(), Optional.empty()));
        devTools.send(Network.emulateNetworkConditions(
                false, // offline
                100,   // latency (ms)
                750000, // download through (bits per second)
                250000, // upload throughput (bits per second)
                Optional.of(ConnectionType.CELLULAR4G), // connection type
                Optional.empty(),
                Optional.empty(),
                Optional.empty()
        ));
        // open a page
        // click twice
        // check for errors
        // check the latest navigation
    }
}

If Flow repo is not a suitable place for it, could we add it to platform test module?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Javascript error: Invalid blocker state transition
3 participants