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: Make routes reactive to change of user #3859

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

DafyddLlyr
Copy link
Contributor

@DafyddLlyr DafyddLlyr commented Oct 27, 2024

What's the problem?

  • Routes with a route guard behave in a flakey manner, due to a race condition between initUserStore() and the route resolving.
  • This means that we can navigate to /global-settings after landing on the homepage, but if we refresh once on this page the route guard will block us incorrectly.
  • This is particularly noticeable on team settings and flow setting and has come up as an issue quite a few times. It's also super frustrating when working on any of these pages as a refresh of the dev environment throws a NotFound error

What's the solution?

  • Following the pattern outlined here in the react-navi docs, we ensure that the Route component is subscribed to changes in the user
  • This means, when Zustand changes the user, react-navi is aware (and is re-rendering). This avoids the race condition.

To test...

The following pages demonstrate examples of the race condition described above. On staging, a refresh (or a few) will give non-deterministic results. On the Pizza, we can refresh and reload the pages reliably.

Staging ❌ Pizza ✅
Global Settings Cell
Admin Panel Cell
Barnet / Team Members Cell

Next steps...

  • I'd like to move the hasJWT() logic into the user store and tie it into the current initUserStore() method - should be a quick follow on PR
  • A helper function to actually act as an auth guard on routes would be nice - we currently have repeated blocks of the following code which can be refactored -
const isAuthorised = useStore.getState().user?.isPlatformAdmin;
if (!isAuthorised)
 throw new NotFoundError(
   `User does not have access to ${req.originalUrl}`,
 );

@DafyddLlyr DafyddLlyr marked this pull request as draft October 27, 2024 10:24
Copy link

github-actions bot commented Oct 27, 2024

Pizza

Deployed 5533585 to https://3859.planx.pizza.

Useful links:

@DafyddLlyr
Copy link
Contributor Author

So this is working as expected locally once I'm logged in - testing on the Pizza though showed that this change has broken the login flow however! 🙃

Working on a fix!

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

Successfully merging this pull request may close these issues.

1 participant