matrix-react-sdk
matrix-react-sdk copied to clipboard
matrix-org
Mark as Unread
This implements the 'mark as unread' feature, as per https://github.com/matrix-org/matrix-spec-proposals/pull/2867
Needs https://github.com/matrix-org/matrix-analytics-events/pull/90 (plus release of matrix-analytics-events)
Checklist
- [ ] Tests written for new code (and old code if feasible)
- [ ] Linter and other CI checks pass
- [ ] Sign-off given on the changes (see CONTRIBUTING.md)
Here's what your changelog entry will look like:
✨ Features
- Mark as Unread (#12254).
This is now ready, code-wise, just needs a new icon for the mark as unread menu item and whatever testing we want to do before merging.
Slightly concerned about the transition path here, once MSC2867 becomes canon and the account_data type changes to m.marked_unread. I can see potential solutions but I think it would be good to have them spelt out ahead of time.
Indeed: is there any plan to get MSC2867 merged, or is the intention for EW just to use unspecified features forever? The setting should probably be behind a labs flag until MSC2867 lands?
Are you sure it wouldn't be easier to get MSC2867 merged first? There seem to be enough impls out there to justify it and it would save the dance around the account_data entry.
So it turns out that the rust (mobile) impls will read either prefix but currently only write to the unstable one. We could do the same here, although would that could as 'using' the unstable prefix before the MSC is merged? It would mean it continues working seamlessly (assuming other clients do similar or change to doing so before we start writing the stable prefix).
I don't really see anything blocking the MSC, so it could probably be moved forward.
although would that count as 'using' the unstable prefix before the MSC is merged?
It would, really. If, as part of the standardisation process of the MSC, we decide that actually m.marked_unread should have a completely different shape/semantics to what is currently proposed, then your implementations which prematurely applied an interpretation on m.marked_unread will be broken. Maybe that's a risk you're willing to take, but "we've already written an implementation which depends on m.marked_unread having a given shape" will cut little ice on the spec side.
Implementation now updated to read both, with comments on the plan and that we're okay accepting that risk.
So I didn't add playwright tests as this was not too bad to get good coverage with regular tests, so I was biasing towards only adding them for things we couldn't cover with regular tests (since they are quite heavyweight and often flakey). Happy to write some though.
So I didn't add playwright tests as this was not too bad to get good coverage with regular tests, so I was biasing towards only adding them for things we couldn't cover with regular tests (since they are quite heavyweight and often flakey).
Yeah, the trouble is that the jest tests might be good for hitting our coverage metric, but they're pretty awful at actually testing that the application works. Ideally we could test that our components work together inside a Jest test rather than firing up a full playwright environment, but the architecture of the app doesn't make that easy.
Second split out PR: https://github.com/matrix-org/matrix-react-sdk/pull/12344
OK, main bits of this now split out & merged separately. The next biggest chunk would be analytics but hopefully this doesn't add a great deal of complexity here.
Thanks. I believe once I saw it mark a room as unread but then fail to update the UI to put the badge on, but all my attempt to repro it subsequently have failed. I'm not really sure how to proceed here. We could go ahead and merge, filing this as a bug so we're aware of it and can chase it later if we get more data, or otherwise perhaps organise another round of testing to see if anyone else can reproduce it?