Fix widgets re-appearing after being deleted #1958
Conversation
* ScalarMessaging onMessage was getting the current room ID by listening for view_and remembering the room id or alias, and so having to look up the alias if it was alias. We have RoomViewStore for this. * Move waitForUserWidget into WidgetUtils * s/require/import/
Widgets would sometimes briefly re-appear after having been deleted. This was because of the following race: * User presses delete, send POST req, we set `deleting`. Widget hides. * POST request completes, we unset `deleting` so widget unhides. * State event comes down sync so widget hides again. This fixes this by introducing `waitForRoomWidget` and using it to wait until the state event comes down the sync until clearing the `deleting` flag. Since we now have `waitForRoomWidget`, this also uses it when adding a widget so the 'widget saved' appears at the same time the widget does.
src/WidgetUtils.js
Outdated
| * false to wait for it to be deleted. | ||
| * @returns {Promise} that resolves when the widget is available | ||
| */ | ||
| static waitForRoomWidget(widgetId, roomId, add) { |
There was a problem hiding this comment.
Any chance the shared logic between this and waitForUserWidget could be factored out? They both do very similar things on slightly different inputs..
There was a problem hiding this comment.
Yeah, I considered this but I think all of the bits of these functions are subtly different, plus there's already the fact that both of them wait for either addition or deletion. I think in this case having two less complex functions might be better than having one more complex one.
There was a problem hiding this comment.
I'm not necessarily suggesting one function; but I notice that
eventsInIntendedStateis very similar toeventInIntendedState(and not just in name)- they both add a listener to matrix client
- they both set a timeout
- the timeout is 10 seconds with both
- the timeout is cleared when the event is received by the matrix client
Actually, I noticed they've already diverged slightly:
https://github.com/matrix-org/matrix-react-sdk/pull/1958/files/36574ca0fb1b0a14a5886a953b3d6dbaad2ea7d2..3e6b3215cfe15532e19fd48288caf9fe23fd465e#diff-18c7dfd796a5f668feeee73e460716f3R125
gets the event from the account data again instead of reusing startingAccountDataEvent, whereas waitForRoomWidget reuses startingWidgetEvents.
If we want to be able to not care about the divergence between them, I think now is the best time to apply paranoia.
There was a problem hiding this comment.
Not sure what you mean - they both re-fetch the current state:
matrix-react-sdk/src/WidgetUtils.js
Line 119 in 3e6b321
matrix-react-sdk/src/WidgetUtils.js
Line 178 in 3e6b321
There was a problem hiding this comment.
Oh. I'm a crank apparently.
IRL we discussed and concluded that these two things are indeed subtly different, pretty much by consequence of the underlying API being used subtly differently depending on whether the widget is in account data or whether it is in a room.
| * @param {string} widgetId The ID of the widget to wait for | ||
| * @param {boolean} add True to wait for the widget to be added, | ||
| * false to wait for it to be deleted. | ||
| * @returns {Promise} that resolves when the widget is available |
There was a problem hiding this comment.
If it's deleted, presumably this is not true
src/WidgetUtils.js
Outdated
| @@ -100,7 +100,8 @@ export default class WidgetUtils { | |||
| * @param {string} widgetId The ID of the widget to wait for | |||
| * @param {boolean} add True to wait for the widget to be added, | |||
| * false to wait for it to be deleted. | |||
| * @returns {Promise} that resolves when the widget is available | |||
| * @returns {Promise} that resolves when the widget is the the | |||
| * requested state according to the `add` param | |||
There was a problem hiding this comment.
This doesn't make sense. "is in the" maybe?
src/WidgetUtils.js
Outdated
| @@ -146,7 +147,8 @@ export default class WidgetUtils { | |||
| * @param {string} roomId The ID of the room to wait for the widget in | |||
| * @param {boolean} add True to wait for the widget to be added, | |||
| * false to wait for it to be deleted. | |||
| * @returns {Promise} that resolves when the widget is available | |||
| * @returns {Promise} that resolves when the widget is the the | |||
| * requested state according to the `add` param | |||
Fix fixed comment
|
Following discussion on #1958 (comment) LGTM |
Widgets would sometimes briefly re-appear after having been deleted.
This was because of the following race:
deleting. Widget hides.deletingso widget unhides.This fixes this by introducing
waitForRoomWidgetand using it towait until the state event comes down the sync until clearing the
deletingflag.Since we now have
waitForRoomWidget, this also uses it when addinga widget so the 'widget saved' appears at the same time the widget
does.
The first commit also does a little refactoring and fixing (see commit message).