[feature] Ephemeral messages (MSC2228) partial support

[EdGeraghty]

Types

• Fix (non-breaking change which fixes an issue)
• Feature (non-breaking change which adds functionality)
• Refactor (non-breaking change which improves code quality - QA thoroughly )
• Breaking Change (fix or feature that would cause existing functionality to not work as expected)
• Chore (updates that would not affect or change the code involving the app itself)

Changes

🔮 Features

Add support for synapse's partial MSC2228 implementation

🐛 Fixes

🔒 Security

🛠 Performance

📐 Refactoring

Media (if applicable)

QA

• Signup
• Login
• Logout
• Unencrypted Chat
• Encrypted Chat
• Group Chats
• Profile Changes
• Theming Changes
• Force Kill and Restart

Final Checklist

• All associated issues have been linked to PR
• All necessary changes made to the documentation
• Parties interested in these changes have been notified

[EdGeraghty]

[EdGeraghty]

[0x1a8510f2]

Some awesome work! One minor code-related question.

I was also wondering about the placement of the button/link to access the dialogue. Placing it down by the composer feels more natural IMO; perhaps as a timer icon next to the send button (very explicit) or as an option on holding down the send button for instance (less explicit, probably preferable given limited composer realestate, though that is currently reserved for SMS I believe)? Bear in mind though, that I'm no designer :P

(I realise this is still a draft but I figured it's best to suggest design changes early)

[EdGeraghty]

Some awesome work!

😁 Thank you

I was also wondering about the placement of the button/link to access the dialogue. Placing it down by the composer feels more natural IMO; perhaps as a timer icon next to the send button (very explicit)

Funnily enough, this was my first instinct, too. I decided against it in the end, because I don't forsee needing to change it regularly enough to remove that click and clutter the bar. In e.g. Signal I just set and forget (apart from very odd occasions!) because it's as much housekeeping as anything.

Bear in mind, this is an unstable feature which is "hidden" in the Advanced Settings in the first place.

I also think this is the first client implementation...

[0x1a8510f2]

Yeah, that's reasonable. I'd say it's definitely fine to keep it there while it's unstable then, and possibly leave it there if we don't get too many questions about where it is once it becomes stable.

I also think this is the first client implementation

I think so too; part of the reason I'm so impressed at how fast we're going :P

[EdGeraghty]

I also think this is the first client implementation

I think so too; part of the reason I'm so impressed at how fast we're going :P

Move fast and... er... implement things :D

[ereio]

Awesome work here! 🚀

Let's change the name to Disappearing Messages in the UX, though we can continue to reference it as Ephemeral in the code since that's what the spec has.

When it doubt / as a general rule in Syphon, prefer on using Plain Language in the UX as much as possible even when it defies what Matrix names something.

https://accessibility.huit.harvard.edu/use-plain-language

[EdGeraghty]

Thank you 😊

I understand the need for simple language, but I'm just a little hesitant that people will assign more "clout" to it being called that than the current which is, basically, asking pretty please to servers & clients.

I'm thinking I'll create a dialog which goes into the details of the limitations, and I'll be happier with changing the name...

Just to be clear, I'm particularly skittish on this one, purely because we're the first implementation AFAIK

[ereio]

That makes sense and I'm willing to keep it ephemeral, but I think the dialog you mentioned is a good idea. If there's a confirmation dialog after attempting to toggle it in advanced settings describing what it is and isn't for now, I think it will temper peoples expectations and we could call it Disappearing Messages with the understanding that it takes several parties respecting it.

I'll try to push the "Advanced Settings" toggle out to dev today to better isolate these features from the typical user.

[EdGeraghty]

@ereio if you could give me some guidance on how I can use the async method for checking server support I'd appreciate it. It was simple for the Hidden Messages because changing the value is an async operation there in the first place...

[ereio]

Sorry in advance if I'm misunderstanding the ask here. You should be able to use the async / future based check you have here in either of the following:

• Loading the Chat Screen, probably in onMounted(), if it's possible a server could enable it per room

otherwise

• During the user session load and checking it globally if available by the server
  • Probably best to do this in onAuthStateChange since multiaccounts can be across servers
  • https://github.com/syphon-org/syphon/blob/dev/lib/store/auth/actions.dart#L238

Lmk if that answers your question. Happy to review this a bit more and add comments to guide you later today too!

[EdGeraghty]

Lmk if that answers your question. Happy to review this a bit more and add comments to guide you later today too!

I think it does 😅

Any code comments are more than welcome. I'd rather get this right 🙂

The support is per-homeserver rather than room. Right now, I'm planning to store the wanted per-message expiration millis in the Room object, client-side. Means we can have different per-room expirations, as it's sent in the message payload

[EdGeraghty]

I'm wondering if we're going to need something like the Mute Handler to expire messages in the client without manually refreshing rooms...

Don't think we can put it in the sync isolate as that would just mean turning off sync would break expiration...

[EdGeraghty]

It occurs to me that we could even check the participating homeservers in a given room for their support, too.

That way we can be even more explicit about support in a given room...

[0x1a8510f2]

It occurs to me that we could even check the participating homeservers in a given room for their support, too.

Great idea, but I don't think it's practical with the current state of the spec:

• Matrix generally tries to avoid sharing your IP with anyone other than your homeserver. Media requests and link previews, for instance, are proxied via your own HS. But AFAIK there is no way to implement this without making a request to each individual HS and therefore sharing your IP.
• Big rooms would produce a huge amount of requests which might be expensive on metered networks.
• This would only help in cases where a server has not implemented the feature and reports as such. It wouldn't help in case of an actively malicious server and could cause a false sense of security.

So, while I would love to have something like that, I think we'd have to write an MSC for it first (something like "query server capabilities over federation") which solves at least some of these issues.

On a separate note, I was wondering how well this works with MSC2285 (Hidden read receipts), because MSC2228 (Self destructing events) depends on read receipts to redact events after they are viewed. It seems neither MSC mentions the other which makes it difficult to see how they interact. Should I comment on one of them for clarification?

[EdGeraghty]

Great idea, but I don't think it's practical with the current state of the spec:

Damn. You're quite right, of course!

On a separate note, I was wondering how well this works with matrix-org/matrix-spec-proposals#2285 (Hidden read receipts)

That's an interesting point, given the refactoring has split the endpoint off for hidden RRs.

There's going to have to be a lot of testing on all of this...

[EdGeraghty]

Another thing to consider, as suggested by @notramo in the Dev #, is not hiding the toggle but throwing a "your homeserver does not support this" dialog when trying to click but it's... well... unsupported by your HS

[0x1a8510f2]

Another thing to consider [...]

I definitely see the upside. Just hiding the feature can cause confusion if one user can see features and another can't. This would also stop feature requests from users who can't find the feature because their HS does not support it. On the other hand, lots of disabled features clutter the UI. I don't have any strong preference personally. We could make it a setting but having settings for settings feels like a bit much :P

[EdGeraghty]

We could make it a setting but having settings for settings feels like a bit much :P

Well @ereio is moving Advanced Settings behind a toggle anyway, AIUI, so it's already gonna be hidden behind a toggle. I'm erring towards doing it this way, perhaps it'll nudge others to implement?

[EdGeraghty]

[0x1a8510f2]

Looks good! I like it :D

[EdGeraghty]

[EdGeraghty]

Unless I'm losing the plot, there's actually no way to interrogate the homeserver as to whether or not it has the enable_ephemeral_messages flag set true* (since it's not reflected in versions in any way I can see :/). The discussion around hiding the option may be moot anyway!

*The other option is to copy the test case of trying to expire a message in the past and see whether the hs honours it, but I can't think of a "nice" way to implement the failure case

[EdGeraghty]

@ereio I've reverted the stuff to do with the Message model and rebased on dev. Let's see what I can do!

[EdGeraghty]

And now I'm back to chasing my tail again. On my local branch I've started adding saveEphemeralMessages to the middleware and added class AddEphemeralMessages { ... } and ThunkAction<AppState> addEphemeralMessages({ ...}) to the state events, but I'm now at the edges of my understanding of the Syphon codebase again.

[EdGeraghty]

We should probably get upstream exposing it @ /_matrix/client/r0/capabilities

{"capabilities":{"m.room_versions":{"default":"9","available":{"1":"stable","2":"stable","3":"stable","4":"stable","5":"stable","6":"stable","org.matrix.msc2176":"unstable","7":"stable","8":"stable","9":"stable","org.matrix.msc2716v3":"unstable"},"org.matrix.msc3244.room_capabilities":{"knock":{"preferred":"7","support":["7","8","9","org.matrix.msc2716v3"]},"restricted":{"preferred":"9","support":["8","9"]}}},"m.change_password":{"enabled":true},"m.set_displayname":{"enabled":true},"m.set_avatar_url":{"enabled":true},"m.3pid_changes":{"enabled":true}}}

[EdGeraghty]

matrix-org/synapse#12524 (comment)

Unfortunately we wouldn't be happy to see this land in Synapse's mainline as things currently stand.

Exposing unstable features as a stop-gap to facilitate beta testing is one thing; however, as far as I'm aware there is no work currently being done to complete MSC2228, nor Synapse's implementation of it. Looking again at MSC2228, there's still a lot of unanswered questions - to the extent that if I were trying to write an implementation, I really wouldn't know where to begin; and I have no real idea how it relates to what was implemented in matrix-org/synapse#6409. Indeed, as things currently stand we're considering reverting matrix-org/synapse#6409 since it's clearly not ready for use.

If you or anyone else is interested in picking up MSC2228 and turning it into a finished proposal, that accurately reflects the implementation, then we'd be delighted. But without such a commitment, sorry, we can't support adding it to /capabilities (or /versions).

[EdGeraghty]

Unless and until someone has the bandwidth to deal with the verdict from the Matrix team, with immense sadness I'm going to close this attempt.