-
Notifications
You must be signed in to change notification settings - Fork 4
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 <HomeSelector>
race condition
#751
Conversation
Coverage Report
File Coverage
|
Visit the preview URL for this PR (updated for commit 7825bba): https://roar-staging--pr751-fix-homeselector-loa-j3kmclgx.web.app (expires Thu, 29 Aug 2024 17:10:51 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 2631e9c58fd0104ecbfddd72a62245ddac467460 |
Test summaryRun details
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
Do you think that it makes sense to have SUPER_ADMIN
as a third userType to return? Based on the presence of the super_admin
key in the user document.
We do have some additional checks and renders throughout the code base that rely on differentiating between an ADMIN
and a SUPER_ADMIN
. Would adding that distinction in this PR reduce complexity down the road?
I also noticed the initial "flashing" of the empty state, when administrations are initialized but not yet loaded. Hopefully we can address this, as well.
This also happens when switching from a student account to an admin account, and vice versa. The navbar seems to store the previous state of the user and displays a student username on the admin account, and the admin username on the student account. I suspect this is related to the navbar PR that you are working on.
Just some thoughts, otherwise I approve.
Thanks for the review @ksmontville! 100% with you on extending the
Regarding the above, you could you please confirm: when you mean "switching between student and admin account", you mean you log out, log in again and the name is incorrect in the navbar, correct? If so that is currently expected as I still need to implement query invalidation on the main PR so that should be addressed/fixed shortly |
Proposed changes
This PR fixes a race condition in the
<HomeSelector>
that would cause the user type checkisAdmin
to switch back and forth betweentrue
andfalse
during page load, effectively causing unwanted renders and side effects on the homepage. This issue seems to only be reproducible after upgrading to the latest@tanstack/vue-query
version as done in the base branch.With the proposed changes, the improved loading of the homepage provides a smoother user experience by preventing the flashing of unwanted components and slightly better performance due the asynchronously import of the homepage components.
Before
before.mov
After
after.mov
Note
The empty-state placeholder is, unfortunately, still flashing in the updated version. This is due to the nature of
isLoading
initially evaluating tofalse
when a TanStack query is initially disabled. This is currently being addressed and will be fixed separately. Additionally, we should address the navbar loading delay as reported in: yeatmanlab/roar/issues/322Types of changes
Checklist
Justification of missing checklist items
n/a
Further comments
n/a