fix: UI not reflecting 2FA TOTP and email status change immediately #35185
Conversation
…g/disabling TOTP/email
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: f799be9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 35 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
.changeset/clever-shirts-talk.md
Outdated
| '@rocket.chat/meteor': patch | ||
| --- | ||
|
|
||
| Written code to update UI to immediately reflect 2FA status change after enabling/disabling TOTP/email |
There was a problem hiding this comment.
| Written code to update UI to immediately reflect 2FA status change after enabling/disabling TOTP/email | |
| Fixes 2FA status change not reflected in UI when enabling/disabling TOTP/emailcode |
There was a problem hiding this comment.
Thank you, Sir. And yes, from next time, I will write the changeset summary in the manner you mentioned.
| const user = useUser(); | ||
|
|
||
| const isEnabled = user?.services?.email2fa?.enabled; | ||
| const [isEnabled, setIsEnabled] = useState(user?.services?.email2fa?.enabled); |
There was a problem hiding this comment.
Hi, can you change the isEnabled and setIsEnabled for something like isEmail2faEnabled/setIsEmail2faEnabled for a better const description name? Thanks!
There was a problem hiding this comment.
Yes, Sir. I have renamed it to isEmail2faEnabled/setIsEmail2faEnabled. Thank you, and from now on, I will keep this in mind while defining and naming variables.
|
|
||
| const handleEnable = useCallback(async () => { | ||
| await enable2faAction(); | ||
| setIsEnabled(true); |
There was a problem hiding this comment.
How about to use the same technique used into TOTP file for "is enabling" state control? This will avoid any double click into the action button if a connection drop/delay happens... ;)
There was a problem hiding this comment.
Yes, Sir. Thank you for the suggestion. I’ve added a registeringEmail2fa state to track the action process, preventing multiple clicks if there's a delay or connection issue. I’ve also included error handling, and right now, I'm only dispatching the error as a toast message, assuming we get an error message if enabling/disabling email 2FA fails.
If you prefer a different approach, please let me know, and I’ll make the changes accordingly
|
Thanks for the contribution! Small but important catches. |
|
This solution fixes the issue at hand but isn't a proper fix. This issue arises because the user's info isn't synced between the client and server. After enabling/disabling the 2FA options, the server is updated but the client still has old info. A better fix for this would be to fix this root problem, syncing the data between client and server. Please check my implementation on PR #35188 and feel free to ask any questions that you might have. Thanks for the contribution. 🚀 |
Yes, Sir, I will check your PR, and I will definitely ask you questions if I don't understand something. Thank you for this. 🙏 |
|
Fixed by #35188 Thanks for the contribution. 🚀 |
3 participants
Proposed changes (including videos)
This pull request addresses the issue where the UI does not immediately reflect changes in the 2FA status (both TOTP and email) after enabling or disabling them. The state is updated correctly in the backend, but the frontend UI fails to re-render, requiring a page reload to display the updated status. The fix ensures that the UI reflects the new 2FA status immediately after the change is made without requiring a page reload.
2FA TOTP Video:
2fa.totp.mp4
2FA Email Video:
2fa.email.mp4
Issue(s)
Closes #35184
Steps to test or reproduce
Further comments
To resolve the issue, I added a state update and re-render logic to the UI component after enabling or disabling 2FA. This ensures that the UI synchronizes with the backend immediately. No page reload is needed after the action is performed.