Found a few issues with navigation guards and fixed them #11190

codyrancher · 2024-06-06T18:41:51Z

Summary

The from/to routes were transposed so I flipped them
Load initial settings wasn't receiving the store and the error was swallowed.
Fixed some error reporting instead of just swallowing exceptions
Removed unnecessary calls to fetchInitialSettings

While working on other changes I noticed that fetchInitialSettings was failing and was being swallowed by the try catch.

Checklist

The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
The PR has a Milestone
The PR template has been filled out
The PR has been self reviewed
The PR has a reviewer assigned
The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

- The from/to routes were transposed so I flipped them - Load initial settings wasn't receiving the store and the error was swallowed. - Fixed some error reporting instead of just swallowing exceptions

codyrancher · 2024-06-06T21:04:36Z

shell/config/router/navigation-guards/load-initial-settings.js

  try {
-    await fetchInitialSettings();


This was never actually doing the heavy lifting. All the extra fetchIninitialSettings were actually doing the work. I fixed this and removed the others.

In other words, we previously weren't fetching settings without a reference to the store?

codyrancher · 2024-06-06T21:05:02Z

shell/initialize/entry-helpers.js

@@ -294,7 +294,7 @@ export async function mountApp(appPartials, VueClass) {
    // Push the path and let route to be resolved
    router.push(path, undefined, (err) => {
      if (err) {
-        const errorHandler = vueApp.config.errorHandler || console.error; // eslint-disable-line no-console
+        const errorHandler = vueApp?.config?.errorHandler || console.error; // eslint-disable-line no-console


I've noticed a few times that this is being invoked without vueApp.config being defined.

codyrancher · 2024-06-06T21:06:22Z

shell/config/router/navigation-guards/load-initial-settings.js

-  } catch (ex) {}
+    await fetchInitialSettings(store);
+  } catch (e) {
+    console.error('Failed fetching initial settings', e); // eslint-disable-line no-console


I left this blank because it was a part of the original code but it I accidentally changed the original code and this prevented me from noticing since the error wasn't emitted anywhere.

shell/config/router/navigation-guards/attempt-first-login.js

rak-phillip · 2024-06-07T22:33:52Z

shell/config/router/navigation-guards/load-initial-settings.js

  try {
-    await fetchInitialSettings();


In other words, we previously weren't fetching settings without a reference to the store?

rak-phillip · 2024-06-07T22:35:49Z

shell/pages/auth/login.vue

@@ -147,8 +146,6 @@ export default {
      // For newer versions this will return all settings if you are somehow logged in,
      // and just the public ones if you aren't.
      try {
-        await fetchInitialSettings(this.$store);


Do we want to remove these instances of fetchInitialSettings()? I'm considering if they exist to act as a "refresh" in the event that something might have changed behind the scenes.

For fetchInitialSettings to act as a refresh instead of using our cache the dispatch methods used from within would need to be passed opt: { force: true }. The second block is also actively skipped if it has ever been fetch since it's looking at generation in the if statement.

Neither dispatch method seems to use this option so I figured that these usages aren't actually refreshing anything. If we'd like to put them back in for comfort I'm fine to do it but the above is why I decided to remove it. Let me know which you'd prefer.

Your explanation helps to quell my fears and I'm not seeing a scenario where removing these instances causes a regression while testing this change. I think that we can leave them out.

@codyrancher @rak-phillip

Settings are a special case. Requests when unauthed will result in a partial response, and when authed the full set.

To resolve this the UI uses load: _MULTI resulting in the response being cached in the store... but not mark as having all results. So any subsequent calls to findAll will still make a http request (without specifying force).

fetchInitialSettings was put in place because we were fetching settings multiple times in different scenarios with the wrong props (resulting in multiple blocking requests on every fresh load).

Given the move of fetching settings outside of the authenticated middlware in #11165 it looks ok to remove these additional ones from brand, setup and login. However we need to validate

setup flow works fine

no additional settings requests are made (if unauthed 1 then another on log in, or only a single one on load when authed)

parts that depends on settings continue to work (for example the brand is correctly applied on setup and logging pages)

@richard-cox thanks for the info. A few questions about the additional testing:

setup flow works fine

Are the tests in dashboard/cypress/e2e/tests/setup/rancher-setup.spec.ts sufficient for doing this?

parts that depends on settings continue to work (for example the brand is correctly applied on setup and logging pages)

Do you mean something else other than setting up rancher (like in the tests I referenced above) when you say setup?

What branding will be available before you've setup rancher? Which logging pages are you referring to?

I have already verified that colors and favicons still work on the login page pre auth.

Quick note:
I've just verified that this is working as expected:

no additional settings requests are made (if unauthed 1 then another on log in, or only a single one on load when authed)

Are the tests in dashboard/cypress/e2e/tests/setup/rancher-setup.spec.ts sufficient for doing this?

Good point! I wish whoever put them in remembered he put them in 🤦‍♂️ Apologies for churn.

Do you mean something else other than setting up rancher (like in the tests I referenced above) when you say setup?

Maybe we can skip this one. Branding (/v3/settings/ui-brand) will be applied either at install so will be seen in setup, or once setup in the UI (and setup is not returned to).

What branding will be available before you've setup rancher? Which logging pages are you referring to?

I have already verified that colors and favicons still work on the login page pre auth.

That's the one

no additional settings requests are made (if unauthed 1 then another on log in, or only a single one on load when authed)

Perfect.

I think all bases are covered then, and it's really nice removing all those extra calls.

codyrancher · 2024-06-07T22:47:47Z

In other words, we previously weren't fetching settings without a reference to the store?

I don't know why I can't reply inline... This is mostly true, we weren't fetching settings from the intended location. It was getting called from the other locations which I removed in this PR. Those other call sites helped mask the issue that this wasn't getting called properly.

rancher-ui-project-bot bot assigned codyrancher Jun 6, 2024

Found a few issues with navigation guards and fixed them

ccfc473

- The from/to routes were transposed so I flipped them - Load initial settings wasn't receiving the store and the error was swallowed. - Fixed some error reporting instead of just swallowing exceptions

codyrancher force-pushed the navguard-fixes branch from d81e2ba to ccfc473 Compare June 6, 2024 19:11

codyrancher added this to the v2.9.0 milestone Jun 6, 2024

codyrancher commented Jun 6, 2024

View reviewed changes

codyrancher requested a review from rak-phillip June 6, 2024 21:06

codyrancher marked this pull request as ready for review June 6, 2024 21:07

richard-cox self-requested a review June 7, 2024 04:58

codyrancher removed the request for review from richard-cox June 7, 2024 16:02

rak-phillip reviewed Jun 7, 2024

View reviewed changes

codyrancher requested a review from rak-phillip June 7, 2024 22:50

rak-phillip approved these changes Jun 7, 2024

View reviewed changes

codyrancher merged commit 75ffa34 into rancher:master Jun 7, 2024
34 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Found a few issues with navigation guards and fixed them #11190

Found a few issues with navigation guards and fixed them #11190

codyrancher commented Jun 6, 2024 •

edited

Loading

codyrancher Jun 6, 2024

rak-phillip Jun 7, 2024

codyrancher Jun 6, 2024

codyrancher Jun 6, 2024

rak-phillip Jun 7, 2024

rak-phillip Jun 7, 2024

codyrancher Jun 7, 2024 •

edited

Loading

rak-phillip Jun 7, 2024

richard-cox Jun 10, 2024

codyrancher Jun 10, 2024

richard-cox Jun 10, 2024

codyrancher commented Jun 7, 2024

Found a few issues with navigation guards and fixed them #11190

Found a few issues with navigation guards and fixed them #11190

Conversation

codyrancher commented Jun 6, 2024 • edited Loading

Summary

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codyrancher Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codyrancher commented Jun 7, 2024

codyrancher commented Jun 6, 2024 •

edited

Loading

codyrancher Jun 7, 2024 •

edited

Loading