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 useStore composable losing reactivity when used in uxStore #4937

Closed
wants to merge 3 commits into from

Conversation

cstns
Copy link
Contributor

@cstns cstns commented Dec 18, 2024

Description

Main train of thought after reproducing the issue locally and noticing that the mainNavContexts was undefined which prevented the mainNav's nearestContextualMenu watcher to set the mainNavContext due to it's safeguard is that the useStore composable is losing store reactivity and causing the uxStore mainNavContexts getter to fail.

Related Issue(s)

closes #4797

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Includes a DB migration? -> add the area:migration label

@cstns cstns self-assigned this Dec 18, 2024
@cstns cstns requested a review from joepavitt December 18, 2024 10:44
const store = useStore()
export default function usePermissions (store = null) {
if (store !== null) {
store = store.state
Copy link
Contributor

Choose a reason for hiding this comment

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

just for readability and consistency, can we make this variable state please?

Copy link

codecov bot commented Dec 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.59%. Comparing base (33ac8f7) to head (b9b17f0).
Report is 56 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4937      +/-   ##
==========================================
- Coverage   78.64%   78.59%   -0.05%     
==========================================
  Files         324      325       +1     
  Lines       15265    15296      +31     
  Branches     3507     3512       +5     
==========================================
+ Hits        12005    12022      +17     
- Misses       3260     3274      +14     
Flag Coverage Δ
backend 78.59% <ø> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@@ -37,7 +37,7 @@ const getters = {
return state.tours.education
},
mainNavContexts: function (state, getters, rootState, rootGetters) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should these be store and rootStore?

Given that the input to usePermissions is store, but you're passing a variable called rootState?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would depend on the context where they are used, I used the naming vuex describes them in their api. It would be better if i'd change the argument name in the composable to rootState to match it

@joepavitt
Copy link
Contributor

Can confirm this hasn't fixed the issue for me. To reproduce consistently:

  • Sign out
  • Sign in as a Platform Admin
  • Go straight to Admin Settings

@joepavitt
Copy link
Contributor

Debugging this myself, and found that in MainNav.vue, the watch for nearestContextualMenu is firing correctly, but this.mainNavContexts is undefined.

Investigating further...

@cstns
Copy link
Contributor Author

cstns commented Dec 18, 2024

Very nice catch! Back to the drawing board but at least now i have something to work with

@joepavitt
Copy link
Contributor

If I hardcode:

mainNavContexts: function (state, getters, rootState, rootGetters) {
        return {}
}

Then the mainNavContexts is no longer undefined, so I'm guesisng Vue is catching an error in the getter and returning undefined, even though I've put breakpoints and console logs in that egtter, and none of them are firing 🤷

@cstns
Copy link
Contributor Author

cstns commented Dec 18, 2024

That's what i though i was fixing, i assumed that the permissions mixin was failing but I was working blindfolded until now. Now that I can reproduce it consistently i can get to the bottom of it easier

@joepavitt
Copy link
Contributor

joepavitt commented Dec 18, 2024

Found the issue:

Navigating Direct for First Time:

Screenshot 2024-12-18 at 20 58 11

On the Refresh (Working)

Screenshot 2024-12-18 at 20 58 14

The back button isn't getting loaded correctly into the context.

I've also verified this is occurring when you go to the "User Settings" for the exact same reason.

@joepavitt
Copy link
Contributor

Boils down to a few circular dependencies going on, as well as race conditions between multiple watchers. This vuex architecture to manage this is incredibly messy

@joepavitt
Copy link
Contributor

Calling this.setMainNavBackButton(menu) in the nearestContextMenu handler does at least make the side menu render, but it does so without a "Back" button.

I need to call that a night, but you have enough to go on in order to get this fixed in the morning hopefully.

@joepavitt
Copy link
Contributor

Fix pushed in #4949 this can be closed

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.

Sidebar sometimes fails to update when switching to admin/user views
2 participants