-
Notifications
You must be signed in to change notification settings - Fork 41
Implement client logout and add logout button #432
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
Implement client logout and add logout button #432
Conversation
Signed-off-by: Alvin <zyzzz0928@gmail.com>
Due to the age of PR #293 and substantial changes in the codebase, resolving merge conflicts became quite challenging. I've contacted the original author, Guocork, who also suggested creating a new PR. Therefore, I've created a fresh PR implementing the same functionality with updated code and comprehensive testing. Could you please review this new implementation? |
Signed-off-by: Alvin <zyzzz0928@gmail.com>
kevinaboos
left a comment
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.
Thanks Alvin, great start on this feature!
I left some comments below. In general, make sure to name things carefully --- try to give variables and types very specific names that cannot be misinterpreted. (I know naming is hard, so it'll take some time to get that right. No worries.)
Thank you for the review. I will try to improve the naming conventions and continue working on it. |
Signed-off-by: Alvin <zyzzz0928@gmail.com>
Signed-off-by: Alvin <zyzzz0928@gmail.com>
Signed-off-by: Alvin <zyzzz0928@gmail.com>
Signed-off-by: Alvin <zyzzz0928@gmail.com>
Signed-off-by: Alvin <zyzzz0928@gmail.com>
Signed-off-by: Alvin <zyzzz0928@gmail.com>
273f1f1 to
734536b
Compare
Signed-off-by: Alvin <zyzzz0928@gmail.com>
This makes it easier to clean up, which is needed for PR project-robius#432. We now pass out `Handle`s instead of strong references to the runtime itself, which is fine because we do want to be able to drop the runtime and forcibly shutdown all background async tasks.
This makes it easier to clean up, which is needed for PR project-robius#432. We now pass out `Handle`s instead of strong references to the runtime itself, which is fine because we do want to be able to drop the runtime and forcibly shutdown all background async tasks.
…POINT_OF_NO_RETURN to state mechine
kevinaboos
left a comment
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.
Just one more comment about the usage of the set_logout_*() functions.
kevinaboos
left a comment
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.
Alrighty, everything looks good to me! I haven't thoroughly tested this, but that's okay since it's a little-used feature that can always be worked around by restarting the app.
I assume you have tested it frequently during the development of this; if any problems show up in my usage, I'll file an issue.
Thanks so much for all of your hard work on this! I know it's tedious responding to all of my comments, but it's important for us to ensure high code quality for this repo as a "flagship" app showing off Makepad + Robius functionality.
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.
Pull Request Overview
This PR implements comprehensive client logout functionality to resolve error handling during logout processes. The implementation adds a robust state machine-based logout system that properly manages sync service termination and resource cleanup.
Key Changes:
- Implemented state machine-based logout flow with proper error handling
- Added logout confirmation modal with progress feedback
- Modified static global resources to use
Mutex<Option<T>>for safe cleanup
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/sliding_sync.rs |
Core logout implementation with state machine integration and resource cleanup |
src/logout/logout_state_machine.rs |
Complete logout state machine with progress tracking and error recovery |
src/logout/logout_confirm_modal.rs |
UI modal for logout confirmation with progress updates |
src/settings/account_settings.rs |
Integration of logout button with confirmation modal |
src/app.rs |
App state cleanup and logout action handling |
| Various cache modules | Added cache clearing functions for proper resource cleanup |
Comments suppressed due to low confidence (1)
src/sliding_sync.rs:1
- The variable
rtshadowsrt_handleand creates unnecessary confusion. Both variables hold the same handle value, but using different names for the same thing reduces code clarity.
use anyhow::{anyhow, bail, Result};
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>


Closes #277. Replaces #293 and #494.
Based on PR #293, I found an error message during logout:
2024-12-24T07:58:55.514036Z ERROR matrix_sdk_ui::sync_service: Error while processing encryption in sync service: Something wrong happened in sliding sync: the server returned an error: [401 / M_UNKNOWN_TOKEN] Invalid access token passed.Root cause:
The error is related to SYNC_SERVICE handling during logout process.
Changes: