Enable Display of Bridge Errors

[V02460]

This PR is part of the Reliable Bridges GSoC project. It marks messages which triggered a bridge error with an error message. This is done by adding a BridgeError view that is appended via EventTile component to every message that is affected. Besides the information that an error occured, the originating network and the affected users are displayed. A lab flag is added to enable testing of the feature on the way to its standardization.

---

@jryans: This implements MSC2162

[turt2live]

@V02460 is this ready for our review or still a work in progress?

[V02460]

Besides a small thing I have to check about the regexes (probably not a word), it's ready for review :)

edit: Regex handling is fine for now.

[turt2live]

@V02460 can we get screenshots in the top post for what this looks like? Screenshots help us figure out what it looks like and what it should look like when we break it.

This will also need design oversight from @nadonomy, but I'll throw this into the queue as code review from the team rather than design oversight.

[V02460]

Added the screenshot of how it currently looks.

I don't take any special pride in the design and I think it still needs love. Especially the colors are hardcoded, which will probably break some themes.

[t3chguy]

Instead of function binds, please use arrow functions on the class as per https://github.com/matrix-org/matrix-react-sdk/blob/master/code_style.md#react

[bwindels]

Nice work! Some suggestions about styling and receiving updates.

[V02460]

That's how it's looking right now

[nadonomy]

Hey there! Apologies for the late review on this PR (I've been away on holiday).

Couple of small requests:

1. We should just use the same ( ! ) icon we use to decorate e2e error messages, firstly for consistency and secondly as the icon in these screenshots seem to be not rendering correctly
2. I'm lacking some context (so I'll defer to @turt2live on this for final say) but if we're expecting users to action the failures (e.g. by sending messages again) we should be using $warning-color for the error messages themselves too
3. The error messages are unnecessarily verbose. Can we update the copy to defer to the issues more immediately? Users don't want to wade through unnecessary language to fix problems. Feedback on the strings below:

"Not delivered to people on %(networkName)s (%(affectedUsers)s)": "Not delivered to people on %(networkName)s (%(affectedUsers)s)"

• Messages are delivered to rooms, not people, better to say "Can't send messages" or similar?

"It took so long. Gave up sending to people on %(networkName)s (%(affectedUsers)s)": "It took so long. Gave up sending to people on %(networkName)s (%(affectedUsers)s)"

• Too verbose. "It took so long. Gave up sending to people on..." -> "Couldn't send messages to"

"Unexpected error while sending to people on %(networkName)s (%(affectedUsers)s)":

• Remove "people on".

"Unexpected error while sending to people on %(networkName)s (%(affectedUsers)s)",

• Remove "people on".

"%(networkName)s did not deliver the message to the people there (%(affectedUsers)s)": "%(networkName)s did not deliver the message to the people there (%(affectedUsers)s)",

• Too verbose. Can just be "Can't deliver to %(networkName)s."

"Was not understood by %(networkName)s, so people there didn't get this message (%(affectedUsers)s)": "Was not understood by %(networkName)s, so people there didn't get this message (%(affectedUsers)s)",

• Too verbose. Can just be "Can't deliver to %(networkName)s.".

[V02460]

Thanks for the review :)

1. The e2e (round) warning icon lies at res/img/e2e/warning.svg while the normal (triangle) warning icon lies at res/img/warning.svg. I can try to fix the exclamation mark for the current icon. If I should use the other icon, somebody would need to tell me how to move it around, so that the path name is not e2e-related anymore.
2. It's planned to be actionable in a later iteration. I would still proceed with changing the color now.
3. Knew that the text wasn't great – will gladly update 😁 (Note: I think your copy & paste went somewhat wrong there, you might want to recheck that what you wrote is what you mean)

[V02460]

@nadonomy Another issue is a room with many people in it. It is probably not the best idea to put 1000+ people into the error message in plain sight. What would be best there? Mouse hover, add a ellipse after a few, something entirely different?

[nadonomy]

@nadonomy Another issue is a room with many people in it. It is probably not the best idea to put 1000+ people into the error message in plain sight. What would be best there? Mouse hover, add a ellipse after a few, something entirely different?

Great question! Stacking/batching them definitely sounds like a good idea. Can you clarify who these error messages are exposed to? All users? Room admins? Bridge admins?

[bwindels]

@nadonomy Another issue is a room with many people in it. It is probably not the best idea to put 1000+ people into the error message in plain sight. What would be best there? Mouse hover, add a ellipse after a few, something entirely different?

The idea being that a subset of the members in the room are bridged users, and we want to be specific about who didn't get the message?

[V02460]

Everyone will see the same message. It's documenting for the room that not all participants got the message.

(A retry option which might be added later would probably only be visible for the sender)

@bwindels Yes :)

[nadonomy]

Everyone

@V02460 just want to clarify here— so if Alice sends a message which produces a bridge error, all other participants in the room will see the error?

[V02460]

Yes, everyone will see it, except those affected by the failure on the foreign network.

It should be visible if a message does not reach everybody. The intention is to have it say: Be aware, this message didn't reach all its destined recipients. (Don't be mad at them if they don't follow your party invitation)

[nadonomy]

Hey @V02460 just a quick update on this— I'm putting together a document on the UX considerations of having the errors this chatty to help drive resolution on what we want, how to unblock this PR, and what else we want to action relatedly outside of this PR.

There'll be some conversation around how we want to batch errors, handling that consistently with other areas of the client (e.g. e2e & message send errors), as well as if this should impact any server/bridge implementations. I'll loop you in to the relevant doc when I have it up and ready to share.

[nadonomy]

@V02460 another update on this. We've been discussing internally the best path forward. Outside of this PR we'll continue to iterate on this in the following direction:

• Expose errors to users depending on room power level, manageable in settings with sensible defaults
• Batch multiple errors together (like a console) to improve signal:noise ratio

So I'm happy to green light the current implementation from my side as it's feature flagged, and we don't want this branch to get too stale.

[jryans]

I believe this needs another round of code review now that design has signed off.

[jryans]

Ah, after looking back at the MSC, it seems there are few open questions to resolve there first, so that seems like the best next step here.

[turt2live]

(also merge conflicts)

[Half-Shot]

Yep, we're waiting on Kai's return. I'll pick this up in their absence if we need to.

[ericmigi]

Any movement on this recently? Just an enquiring fan :)

[V02460]

Thank you for asking! I am currently trying to get back to the project and make some progress again.

I would really like to have this PR in Riot Web labs, as it would work with the current implementation of the Discord bridge. Further changes to the MSC will probably not fundamentally change the workings of this code, so even if the MSC is not there yet, it would make it easier collecting some real world experience with the feature.

So, if the Riot Web team agrees, I would merge the latest develop and try to get it in a mergable state.

@nadonomy Could you explain to me the rationale behind this?

Expose errors to users depending on room power level, manageable in settings with sensible defaults

[ericmigi]

@V02460 cool! I hope it gets through :)

[ericmigi]

monthly bump up! Let's get this into Riot :)

[Half-Shot]

This is effectively blocked on MSC2162, which in turn is blocked on MSC2346, which is blocked on the spec team.

[ericmigi]

@Half-Shot Ah thanks, good to know. I'll keep track of those.

[Half-Shot]

I'm going to close this, as I suspect it has bit rotted, and we're not actively working on this at the moment.