-
Notifications
You must be signed in to change notification settings - Fork 357
Stop overriding the homeserver when restoring a Client
#5753
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
Stop overriding the homeserver when restoring a Client
#5753
Conversation
This isn't necessary and overrides the existing data previously saved by the SDK, resulting in losing data such as the `Client::server` (the discovery server URL). In turn, this caused the app to be unable to refresh the server info in some homeservers.
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5753 +/- ##
========================================
Coverage 79.65% 79.65%
========================================
Files 2434 2434
Lines 65454 65453 -1
Branches 8341 8341
========================================
Hits 52136 52136
+ Misses 10341 10340 -1
Partials 2977 2977 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bmarty
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!
| passphrase = sessionData.passphrase, | ||
| slidingSyncType = ClientBuilderSlidingSync.Restored, | ||
| ) | ||
| .homeserverUrl(sessionData.homeserverUrl) |
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 for the fix!
So it seems that the application is storing the actual homeserver Url (CS baseUrl) and this is not really expected. I was looking at the other usage of homeserverUrl (to see if it could be just removed) and I see that the only other usage of homeserverUrl is here, and toSession is invoked at line 73 below.
So it is actually OK that the CS baseUrl is stored?
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.
The other usage is actually not used by the SDK. Maybe some cleanup can be done SDK side later.
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.
The other usage is actually not used by the SDK. Maybe some cleanup can be done SDK side later.
This was implemented in matrix-org/matrix-rust-sdk#5874.



Content
Removes the
setHomeserverUrlcall when restoring aClientfrom itsSessionData.Motivation and context
This call isn't necessary and overrides the existing data previously saved by the SDK, resulting in losing data such as the
Client::server(the discovery server URL).In turn, this caused the app to be unable to refresh the server info in some homeservers. Sadly, this means users that are already logged in will be affected by this issue since their configured URLs would have been overridden after the first
Clientrestoration, so they won't be able to refresh their server info until they log out and in again.Tests
element.ioor some other HS whose server URL doesn't match their HS URL (the host for the/.well-known/matrix/clientdoesn't match the actual HS).Also, if right after restoring the
Clientyou add a log containing theClient.server()value, it should display the server URL instead of null.Tested devices
Checklist