DEV: Update discourse-presence plugin to use new PresenceChannel system (PR #14519)

O I just noticed this and I’m sure there is a good reason but I’m curious why we have to subscribe to the channel when the composer is in the replying state and not the whisper state?

Minor but we don’t need to filter when a user is anon as well.

Minor but this will never be called for an anon user so we don’t need this.currentUser?.id

@davidtaylorhq Sorry I was reviewing the PR again and I think this might be a security concern for PMs. If a group is accidentally added to a PM and removed shortly after, there is a chance that users in the removed group can still see the presence for the PM even though the group has been removed. The stars will really have to align for that to happen but I think it is still a valid security concern we should handle.

We display the users who are currently whispering or replying in the top right of the composer. So if you’re whispering, you will also be shown users who are replying. And if you’re replying, you’ll also be shown users that are replying.

This matches the current behavior, and I think it makes sense from a UX point of view. It’s useful to know if someone is whispering something while I’m drafting a reply. (e.g. I can then find out what they are whispering about before posting my reply)

The composer-presence-display component will never be loaded by anon (it’s embedded in the composer), so I don’t think it’s worth introducing another code path here.

    return users?.filter((u) => u.id !== this.currentUser.id);
      ?.filter((u) => u.id !== this.currentUser.id)

Yeah I agree with the concern here. I looked into invalidating the cache following PM participant changes - that’s pretty achievable.

However, it becomes much more difficult once you start thinking about regular topics/posts. When category permissions are changed, we’d have to invalidate the cache for every single topic/post in that category, which could easily be 100,000+ PresenceChannels (and therefore 100,000+ redis keys to DEL)

Instead, I am wondering whether we should reduce the 2 minute config cache down to 10 seconds. That way, it still helps alleviate postgres load during high-traffic events, but also means that permission changes will take effect much more quickly.

When category permissions are changed, we’d have to invalidate the cache for every single topic/post in that category, which could easily be 100,000+ PresenceChannels (and therefore 100,000+ redis keys to DEL)

Do we need to invalidate all the caches upfront? I thought we will just not use the cache when a new client joins the channel.

How would we know to skip the cache, without introducing any DB lookups?

Sorry not sure what I was thinking since the cache key is just the channel name. Actually, is there a way we can delete based on a prefix? We’ll only need to run delete on keys are that present instead of picking all the topic and post ids and trying to delete them whether they exists or not in the DB.