FEATURE: basic PresenceChannel support (PR #13803)

This starts building presence channel support into Discourse which can be used to track online status of users

Work in progress, not ready for a merge

GitHub

@SamSaffron this works because we’re manually calling .auto_leave in the spec. However, in production, I’m not sure what would cause .auto_leave to be triggered :thinking:

An example scenario would be:

  • User A starts replying to a topic
  • User B is viewing the topic, and receives an user A entered notification on the messagebus
  • User A closes their browser, without sending an explicit leave event
  • User B will never receive a user B left messagebus event, even after the timeout has elapsed :cry:

If some other user were to start replying, or something else called channel.count, then things would get cleaned up. But I don’t think we can rely on that.


One solution would be some kind of persistent ruby thread, or regular sidekiq job, which iterates through all known channels and calls auto_leave. For that, we would also need to maintain a list of known channels.

I’ll do some thinking and see if we can come up with a cleaner solution, but just wanted to check if you had any ideas.

I was thinking we have a regular recurring task that calls auto-leave. Auto leave can be somewhat delayed. Running this recurring task in sidekiq once a minute would probably do it. Vast majority of the time we will get leave messages cause we can hook in to browser window close, even using a web worker for the extra time buffer. That said, we will certainly miss some leaves so we have to handle it.

To keep stuff cheap it could keep a list of “active presence channels” if that is empty no need to call auto leave, only do so on active channels.

Another thing to keep in mind is that auto_leave is called for enter/leave/count/etc … so some of this may just fix itself automatically next time someone loads the page?

Implemented in the latest commit. For the ‘list of active presence channels’ I’ve used a ranked list, where the rank is the ‘next expiry time’ in the channel, so it should scale pretty well. Plus, if things have already been cleaned up by enter/leave/count events, it doesn’t duplicate work.

we need to think through permissions here.

I guess we need to define an API that sets up a security domain for a channel. I would imagine we would use category based permission for “topic” presence, and user based permission for messages. Not sure how many extra permutations we will need.

@SamSaffron I think this is now in a mergeable state. I’m sure we’ll need more tweaks as we develop various use-cases, but the fundamentals are all done. Once we merge it, I’ll open a separate PR to make a start on migrating the core discourse-presence plugin to the new patterns.

JS LGTM, lots of things are going on and we are kinda new to using async/await so much, but given it’s not going to be used when merged I feel like this is very good foundations.

this is nice

I do wonder why params.require() is letting us down and what we should do to fix this.

also nice!

nice, this keeps this call pretty cheap.

thanks for adding this!

This is enormous, but from my perspective approved. The patterns you are using are great.

Congrats this is a huge piece of work!

note I can not approve cause github does not allow me to, but I am sure someone else can.

For me, the main annoyance is that params.require() doesn’t allow you to specify the data type. e.g. here we want a string, but params.require does nothing to prevent users from sending us an integer, or an array, in that parameter.

1 Like