-
Notifications
You must be signed in to change notification settings - Fork 7
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
Revert safe reset refactor #2219
Conversation
This reverts commit f1d189a.
✅ Deploy Preview for decent-interface-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@tomstuart123 Could you checkout the description on this PR please and test to confirm the fix on your end? |
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.
Kinda sad but well
We need some refactoring into our state management 🤣
@DarksightKellar thanks man, very clear description I just jumped between Safe's via the homepage-->DAOhome-->DAO roles--> homepage --> etc. This worked 100% of the time on 6 tries. I was able - as you mentioned to replicate the edge case you mentioned with the search bar. However, I can confirm that this is fine. To replicate it you need to move super fast in the UI (i.e. power users) and even then it does reload automatically eventually Approved |
Question @DarksightKellar / @decentdao/engineering - will this reverted change of 'call' location have any other user impact? Or was it just a general 'code efficiency' / 'logical place' reason ? |
@tomstuart123 Yeah this revert just sent back logic to the way it was already on develop. Plus the "fix" on top of that. The move (that ended up not working out) was meant to patch up edge case resetting logic flaws that do exist right now regardless and were discovered while fixing the original issue. For example, having the reset logic exist in the homepage means that going to the homepage is the only to reliably reset safe state. If the "homepage" changes at some point in the future, that affects this guarantee. |
In a previous commit, we moved the calls to
resetSafeState
andresetHatsStore
from the home page to the dao controller component, as that felt like the more logical place to put it.While the argument is still valid, this just led to more bugs due to the brittle nature of the flow of safe state changes.
We have tried moving the
reset
s higher up the hierarchy intoLayout/index.tsx
with slightly better result, but far from good enough.That said, the decision was made to revert this move for now. This fixes the original bug described in #2084, with the caveat that there are edge cases around switching safes instantly using the search bar, when already on the roles page of the current safe.
This will have to do for now as the most common UX flows to end up on different safes should be fine. I do plan to continue to dig into the original safe state issues in the meantime until more pressing issues get on the radar.