Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notification Panel: Include Encrypted rooms (part 1: 24 hour POC) #25383

Open
daniellekirkwood opened this issue May 16, 2023 · 6 comments
Open
Assignees
Labels
A-Notif-Panel A-Notifications O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience S-Minor Impairs non-critical functionality or suitable workarounds exist T-Enhancement Z-Chronic

Comments

@daniellekirkwood
Copy link
Contributor

daniellekirkwood commented May 16, 2023

Your use case

The notification panel in Element web currently only displays unencrypted ping/highlight messages (mentions and keywords). The panel is extremely useful to people and internally we had a discussion here

In the future we want the notification panel to:

  • Include all messages you were mentioned or keyword-ed in, regardless of encrypted or unencrypted
  • Move the access point (it's wrong on so many levels today 😂 )

Adding encrypted rooms to this list may take some work so we'd like to experiment with ways we might be able to do this. This issue is only for the first experiment we've discussed.


Can we add encrypted rooms to the notifications panel?

  • Yes, but performance will take a hit unless we make some changes to spec

Our suggestion:

  • Have the notifications panel only keep messages from the last 24 hours. This reduces the load and therefore shouldn't have too much of a negative impact on the product. (In the future we should aim to remove or lengthen this cap, but for the purposes of testing how we'd do this, it's a good compromise for now)

Open questions

Things we're thinking about:

  • If we shorten the unencrypted rooms 'memory' in the panel to only 24 hours user's may get upset
  • If we add the two lists together users may get confused

To be discussed:

  • Could we have a drop down at the top of the panel (similar to the threads panel) that let's users choose between 'forever unencrypted' and '24 hour encrypted' while we're in this experimental phase?
  • Or, could we have a labs flag and measure how many people turn it on and are willing to only see the last 24 hours in order to get all their rooms?
  • Or, could we extend the 24 hours to longer (maybe 3 or 4 days) and what would the performance impact/change be?

Spec land:

  • What changes would we have to make server-side to help the client provide this to users?
@daniellekirkwood daniellekirkwood added S-Minor Impairs non-critical functionality or suitable workarounds exist A-Notifications T-Enhancement Z-Chronic O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience labels May 16, 2023
@daniellekirkwood daniellekirkwood self-assigned this May 16, 2023
@daniellekirkwood
Copy link
Contributor Author

@giomfo @weeman1337

here's the issue we were talking about yesterday with my 'open questions' that i'd like us to discuss.

WDYT?

@t3chguy
Copy link
Member

t3chguy commented May 16, 2023

Duplicate of #6874

@t3chguy t3chguy marked this as a duplicate of #6874 May 16, 2023
@t3chguy t3chguy closed this as completed May 16, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 16, 2023
@daniellekirkwood
Copy link
Contributor Author

it's not a duplicate.

@daniellekirkwood daniellekirkwood changed the title Notification Panel: Include Encrypted rooms (part 1) Notification Panel: Include Encrypted rooms (part 1: 24 hour POC) May 16, 2023
@t3chguy
Copy link
Member

t3chguy commented Aug 4, 2023

The /notifications API should be sufficient for this. Currently we call it with only=highlight which makes it exclude encrypted notifications as by default those only trigger notify push actions and highlight only once decrypted.

We could drop the only=highlight and perform a local filtering.
Synapse drops record of non-highlight notifications after a day so it'd match the 24 hr POC. This isn't spec conforming, the spec says

This API is used to paginate through the list of events that the user has been, or would have been notified about.

It does not have any note which permits a server to prune this resultant set.


A local filtering isn't trivial, either we slow down the pagination of this panel to allow for decryption or we allow things to be in the list slightly out of order to make it feel faster

Could we have a drop down at the top of the panel (similar to the threads panel) that let's users choose between 'forever unencrypted' and '24 hour encrypted' while we're in this experimental phase?

This would certainly be within the realm of possibilities except we can't guarantee the 24 hour encrypted - other server implementations may only support 1h or with no limit at all.

POC:
image
image

@giomfo
Copy link
Member

giomfo commented Aug 4, 2023

Note: MSC4028 may help on this request (see here)

@t3chguy
Copy link
Member

t3chguy commented Aug 4, 2023

@giomfo yes that will be needed to support M&K-overriden Encrypted rooms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Notif-Panel A-Notifications O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience S-Minor Impairs non-critical functionality or suitable workarounds exist T-Enhancement Z-Chronic
Projects
None yet
Development

No branches or pull requests

3 participants