fix(router): prevent multiple navigation at the same time

[aegenet]

Pull Request

📖 Description

When a user clicks quickly from one page to another, the content can be duplicated.

🎫 Issues

Resolves #1860.

👩‍💻 Reviewer Notes

I prevent multiple navigations from occurring simultaneously and store the most recent one. This ensures that users don't find it odd to end up on their last desired page.

I was delayed in creating this PR because I noticed another issue with the error handling in @aurelia/router. However, it requires a separate issue/PR, as it's not just linked to this current issue.

📑 Test Plan

• Added new integration/e2e tests for this specific case.

⏭ Next Steps

[codecov]

Codecov Report

Attention: Patch coverage is 90.90909% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 88.46%. Comparing base (06aa113) to head (84779a8).
Report is 2 commits behind head on master.

Files	Patch %	Lines
packages/router/src/router.ts	94.73%	1 Missing ⚠️
packages/router/src/utilities/runner.ts	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1895      +/-   ##
==========================================
+ Coverage   88.37%   88.46%   +0.09%     
==========================================
  Files         260      260              
  Lines       22847    22862      +15     
  Branches     5300     5301       +1     
==========================================
+ Hits        20190    20226      +36     
+ Misses       2657     2636      -21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bigopon]

@aegenet , thanks for the PR. It seems the fix does work, though not always. Sometimes the e2e tests passed, and sometimes it failed.

When some of the e2e tests failed, I can see this test result:

That test result means there's something that pulls back the URL from two-route to one-route, based on this code

    await Promise.all([
      page.waitForURL(`${baseURL}/pages/two-route`),
      page.click('#page-two-link'),
    ]);

    expect(page.url()).toBe(`${baseURL}/pages/two-route`); // <<<<<<< fails here

I've checked the fix, it seems ok, so I'm not sure what could be the reason.

cc @jwx

[bigopon]

When running locally, without the commits from master which has the changes at #1900, I also experienced the same failure, even though it's really rare. I'm not sure whether this is a playwright thing.

[aegenet]

@bigopon I remember there was an issue with the unit test. I think it's the same issue... and yes, it's the same: Playwright doesn't wait for the end of the navigation, so we need to wait for the end of all navigation before navigating to the last requested page.

[bigopon]

@aegenet thanks for checking that.

Playwright doesn't wait for the end of the navigation

Can this actually affect the test? Once the url has been changed to two-route, can it go back to one-route via something inside a navigation (or latest navigation), briefly or eventual back to two-route?

[aegenet]

Yes, I think, because we do a lot of navigation (page-one-link, page-two-link, & auth-link etc), so after, when we execute the following statement:

await Promise.all([
  page.waitForURL(`${baseURL}/pages/two-route`),
  page.click('#page-two-link'),
]);

The:

page.waitForURL(`${baseURL}/pages/two-route`)

can be swallowed by the previous loop "click"

At the moment, I can't reproduce the issue anymore with my last commit.

[bigopon]

That sounds like inside a navigation the URL would be updated somehow, I'm not sure on this, but the fix isn't doing any related to that behavior so I think it's ok. Thanks @aegenet.

[bigopon]

Nice work, thanks @aegenet

[bigopon]

If you wanna test this out, our dev version should be on npm in about 15 mins @aegenet

[aegenet]

It's ok with the dev version: https://gist.dumber.app/?gist=93e7586ed697b010ed48517e7ad3c423

No more 'clone wars', thanks for your review and involvement.

[bigopon]

Thank you too @aegenet for the work and the confirmation 🙏