-
-
Notifications
You must be signed in to change notification settings - Fork 606
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: show spinner again while recovering from connection error #702
Conversation
I wonder if we should also show the spinner while in RECONNECTING state. If you sleep your laptop and open it again 3 days later and go straight back to riot, you might not have 3 sync fails to go from RECONNECTING to ERROR. If the catchup sync then takes long (Matthew had one that took 5 minutes, I guess also because of retry logic after 80 seconds), it'll be very confusing to not have a spinner. Downside would be that if a connection drop on a sync request, we'll show a spinner briefly... maybe that's better than the catchup-sync-without-spinner confusion? Wdyt @dbkr? |
FTR, closed matrix-org/matrix-react-sdk#2138 in favor of this approach. |
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.
Yeah, it makes sense to represent somehow the state where we know our current state is severely out of date (ie. when you've opened your laptop after a day). RECONNECTING
isn't that though - PREPARED
seems closer to the truth, although should we be adding another edge in that diagram from 'SYNCING' to 'PREPARED'?
src/sync.js
Outdated
@@ -780,6 +780,13 @@ SyncApi.prototype._onSyncError = function(err, syncOptions) { | |||
// instead, so that clients can observe this state | |||
// if they wish. | |||
this._startKeepAlives().then(() => { | |||
if (this.getSyncState() == 'ERROR') { |
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.
nitpick - triple equals
This is according to the state diagram in client.js. This will show a spinner at the bottom of a room again while the catchup sync is in progress, which seems to have broken at some point.
beccb8f
to
0d23d04
Compare
Now needs matrix-org/matrix-react-sdk#2143 on react-sdk side to fix the spinner. |
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.
I think this makes sense. Do you think this is a breaking change? I guess it has to be since a client has no idea what to do with states it doesn't know about (even though Riot would just have assumed it was 'syncing' so would have behaved as before).
@dbkr Yeah, good point, this might be a breaking change, but these changes would be released together with the already breaking changes for lazy loading (e.g. |
ah, yes - I'd forgotten about that one. Might be an idea to stick a 'breaking change' heading at the top of the changelog, or note them down somewhere, otherwise I'll forget. |
Pass through PREPARED state after error, when keepalive returns succes.
This is according to the state diagram in client.js.
This will show a spinner at the bottom of a room again
while the catchup sync is in progress,
which seems to have broken at some point.
Addresses item 6 (We no longer show a spinner when recovering after losing connectivity) of element-hq/element-web#7182