Fix widgets getting stuck in loading states #31314
Conversation
(Rather than storing just the raw ClientWidgetApi objects.)
|
This is failing the test coverage check at the moment because SonarCloud thinks the renamed files are entirely new code. This probably could benefit from a new test or two anyway targeting the actual regression, but I'm out of time until next week and would like this to at least gather review feedback in the meantime, please :) |
BillCarsonFr
left a comment
BillCarsonFr
left a comment
There was a problem hiding this comment.
Great work catching this!
I find the renaming great, and thanks for documenting existing code 🙏
I just have a few comments.
| this.messaging = new WidgetMessaging(this.widget, props); | ||
| WidgetMessagingStore.instance.storeMessaging(this.widget, props.room?.roomId, this.messaging); | ||
| } catch (e) { | ||
| logger.log("Failed to construct widget", e); |
There was a problem hiding this comment.
could raise the level to error() ?
| // we register listeners for both cases. Note that due to React strict | ||
| // mode, the messaging could even be aborted and replaced by an entirely | ||
| // new messaging while we are waiting here! | ||
| while (!messaging?.widgetApi) { |
There was a problem hiding this comment.
I am a bit lost, if messaging is undefined this is then a while(true) what is breaking the loop?
| await super.start(); | ||
| this.messaging!.on(`action:${ElementWidgetActions.JoinCall}`, this.onJoin); | ||
| this.messaging!.on(`action:${ElementWidgetActions.HangupCall}`, this.onHangup); | ||
| this.widgetApi!.on(`action:${ElementWidgetActions.JoinCall}`, this.onJoin); |
There was a problem hiding this comment.
existing code, but wouldn't it be better to test if this.messaging! is null and throw a specific error instead of unwrapping? or start() ensures it is created?
| }); | ||
| this.messaging.on("capabilitiesNotified", () => this.emit("capabilitiesNotified")); | ||
| this.messaging.on(`action:${WidgetApiFromWidgetAction.OpenModalWidget}`, this.onOpenModal); | ||
| this.widgetApi.on(`action:${WidgetApiFromWidgetAction.OpenModalWidget}`, this.onOpenModal); |
There was a problem hiding this comment.
What about the capabilitiesNotified that was there previously? it was not needed?
| public abstract readonly STUCK_DEVICE_TIMEOUT_MS: number; | ||
|
|
||
| private _messaging: ClientWidgetApi | null = null; | ||
| private _widgetApi: ClientWidgetApi | null = null; |
There was a problem hiding this comment.
Finally we have dont do the magic "call the ClientWidgetApi messaging pattern anymore. I never liked sth of class ClientWidgetApi be called messaging.
| * set of callbacks. | ||
| */ | ||
| // TODO: Consider alternative designs for matrix-widget-api? | ||
| // Replace with matrix-rust-sdk? |
There was a problem hiding this comment.
Should we add another comment that we would need to add thread support to the rust sdk for this but it would highly reduce maintenance burden.
| } | ||
|
|
||
| /** | ||
| * A running instance of a widget, associated with an iframe and a messaging transport. |
There was a problem hiding this comment.
Can this get a bit more verbose. Especially the realtion between the widgetDriver and the WidgetMessaging is sth that might be hard to grasp for ppl trying to learn this code?
I feel like a complete list of the resposibilites of this class would be very nice.
- generate url (via template)
- allocate driver (delegate class required for the matrix-widget-api class to do cs api and client navigation tasks. (for widget-> element client communication)
- feed information into the widget (manually do all element client -> widget communication) (
updateTheme,setViewedRoomId, ...)
^ this summary also documents that the two classes WidgetDriver WidgetMessaging are doing very similar things just split by direction. It makes it easier to reason about unifying this seperation.
It would make a lot of sense, that the driver is a bidirectional interface (containing functions that do things on behave of the widget AND set up listeners on the client and call functions on the widget api (or emit) on specific actions.)
4 participants
Depends on #31354
We allow widgets to remain "always on screen" by moving between a PiP view and an AppTile view as you switch rooms. But apparently the lifecycle of a StopGapWidget object was such that it would be disposed of and reconstructed every time the AppTile component was remounted, even while presenting the very same widget. This meant that whenever you switched rooms, causing a widget to move into a PiP, the client would forget some of the widget's state (like which iframe it was associated with, and whether the postmessage API had been started).
As far as I can tell, EW's widget code has always been brittle in this way, but the forgotten state didn't cause any visible impact until the upgrade to React 19 and some related fixes that I made. After those changes, switching rooms would cause the client to forget state that would prevent it from stopping widgets properly, causing issues for when you later try to launch the same widget again.
I believe the right way to fix this situation is to make StopGapWidget persist between React components, just as we persist the iframe element. In other words, put it in a store. But it felt really redundant to create a StopGapWidgetStore on top of all the other widget stores that we have knocking around…
So, the proposed solution: Rename the StopGapWidget class to WidgetMessaging, and store these objects in the WidgetMessagingStore. (Today that store holds ClientWidgetApi objects, but if WidgetMessaging objects are persistent, then they can just hold their own ClientWidgetApi without any help.) I used this renaming opportunity to also give the driver class a more proper name and add some much-needed doc comments to things.
Closes #30838
Closes #31065