Propagate room join errors to the UI #1007
Conversation
Dispatch so we can set the state in RoomViewStore. Show the error when the room join fails (unsure if it's better to do this from the component or the store). Remove unused joinError from roomview.
src/stores/RoomViewStore.js
Outdated
| } | ||
|
|
||
| _joinedRoom(payload) { | ||
| if (payload.room_id === this._state.roomId) { |
There was a problem hiding this comment.
Evil! We must not update state conditionally but instead trust the payload!
src/stores/RoomViewStore.js
Outdated
| } | ||
|
|
||
| _joinRoomError(payload) { | ||
| if (payload.room_id === this._state.roomId) { |
As apparently doing it confitionally is bad
|
done! |
src/stores/RoomViewStore.js
Outdated
| MatrixClientPeg.get().joinRoom(this._state.roomId, payload.opts).done(() => { | ||
| dis.dispatch({ | ||
| action: 'joined_room', | ||
| room_id: this._state.roomId, |
There was a problem hiding this comment.
This isn't used anymore
There was a problem hiding this comment.
This isn't, is it? Since we need to update joining to false.
There was a problem hiding this comment.
But we don't need the room_id to do that
src/stores/RoomViewStore.js
Outdated
| joinError: err, | ||
| dis.dispatch({ | ||
| action: 'join_room_error', | ||
| room_id: this._state.roomId, |
There was a problem hiding this comment.
Agreed that nothing reads this error, but if it's there then it should probably be kept up to date. We could remove the join error though (unless we need it in RoomView by moving the Modal there)
There was a problem hiding this comment.
I was referring to the room_id
| }); | ||
| const msg = err.message ? err.message : JSON.stringify(err); | ||
| const ErrorDialog = sdk.getComponent("dialogs.ErrorDialog"); | ||
| Modal.createDialog(ErrorDialog, { |
There was a problem hiding this comment.
It feels quite wrong to create modals in the store. I would at least move this to _onRoomViewStoreUpdate of RoomView, but I am surprised that we weren't already doing this somewhere else...
There was a problem hiding this comment.
We were doing it in RoomView but it was removed - I wasn't sure whether the intention was to have the thing doing the joining display these errors or the view that caused the join: if the former, you'd get dialogs pop up even if you navigated away from the room view, if the latter, you could get multiple dialogs if multiple things were listening to the dispatch.
There was a problem hiding this comment.
IRL we decided that the value added by moving the location of the dialog creation is probably quite low.
The store should probably not be doing API calls and it should probably be dumb in that it should just accept state changes via dispatches that it trusts fully but that have originated elsewhere.
Dispatch so we can set the state in RoomViewStore. Show the error
when the room join fails (unsure if it's better to do this from
the component or the store). Remove unused joinError from roomview.