Feature🚀: Ability to mention anyone in open chats and notify them

[sarthology]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update

Description

Pheww! After working and tweaking for more than a week this feature is finally out. This is description will be a bit longer, as this PR has some bug fixes as well. So Let's get started

1. Ability to mention someone in open group

2. Get a notification if you got mentioned

3. Ability to mention all in the group and Notify them

4. Mentions get converted into a clickable link to open info in Sidecar

Tweaks in the Notification system

5. Message count will increase if there is a new message in the open group.

You will get notified only if mentioned in the group

6. Message count will be decreased only if the unread message is read

7. Message chat will increase even if you are on the connect page

But no notification will be shown

Added to documentation?

• docs.dev.to
• readme
• no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

[rhymes]

I have a UX question here: isn't the @all mention in an open channel a possible avenue for abuse? Don't we all already get a notification everytime there's a new message in a channel?

[sarthology]

I have a UX question here: isn't the @all mention in an open channel a possible avenue for abuse?

It's a really good point, It can. But there are useful use cases also there to consider.

Don't we all already get a notification every time there's a new message in a channel?

Not in the case of an open channel.

[sarthology]

Small feature added

Ability to navigate through mention suggestion list using keyboard.

[rhymes]

I think I'm getting closer to the issue, see my analysis

[rhymes]

this can be cleared up a little bit, the two pusher trigger lines are really long.

My suggestion:

Suggested change

	if @chat_channel.group?
	Pusher.trigger("private-message-notifications-#{session_current_user_id}", "message-opened", { channel_type: @chat_channel.channel_type, adjusted_slug: @chat_channel.adjusted_slug }.to_json)
	else
	Pusher.trigger("private-message-notifications-#{session_current_user_id}", "message-opened", { channel_type: @chat_channel.channel_type, adjusted_slug: @chat_channel.adjusted_slug(current_user) }.to_json)
	end
	adjusted_slug = if @chat_channel.group?
	@chat_channel.adjusted_slug
	else
	@chat_channel.adjusted_slug(current_user)
	end
	Pusher.trigger(
	"private-message-notifications-#{session_current_user_id}",
	"message-opened",
	{ channel_type: @chat_channel.channel_type, adjusted_slug: adjusted_slug }.to_json,
	)

[rhymes]

BTW reading this method seems that it's going to work only for direct channels, not for the other ones.

We have three types of channels:

• open which are groups as General
• invite only
• direct

as only direct channels are named private-message-notifications-{USER_ID}, what would happen to the other channels with this method?

If you look at https://github.com/thepracticaldev/dev.to/blob/0fb38adf48277b9b3f2b86245ed3e8d21a596bb8/app/controllers/messages_controller.rb#L121 you'll notice how messages are routed on different channels by calling chat_channel.pusher_channels in some case.

That might also be why I get this error when I test it with an open channel locally:

 TypeError: "newMessages[activeChannelId][foundIndex] is undefined"
    value chat.jsx:736
    setState Preact
    value chat.jsx:731
chat.jsx:866:12
    value chat.jsx:866

I get foundIndex as -1 which means that the message sent from Ruby wasn't found by this JS callback's part:

          const foundIndex = messages[activeChannelId].findIndex(
            message => message.temp_id === response.message.temp_id,
          );

If all channels need to require Pusher authentication we need to renamed then private- something. Should they be renamed in the Pusher console as well as the DB? Because right now, my open channel for example, they have names like:

[14] pry(main)> cc.pusher_channels
=> "open-channel-5"

You can create your own open channel with ChatChannel.create_with_users(User.all, "open") to test this hypothesis

[sarthology]

I found the real issue and it's because

    this.setupChannels(chatChannels);

    const channelsForPusherSub = chatChannels.filter(
      this.channelTypeFilterFn('open'),
    );
    this.subscribeChannelsToPusher(
      channelsForPusherSub,
      channel => `open-channel-${channel.chat_channel_id}`,
    );

this bit of code is not working and because of that socket for open channel never get connected. That's why you are getting this error.

 TypeError: "newMessages[activeChannelId][foundIndex] is undefined"
    value chat.jsx:736
    setState Preact
    value chat.jsx:731
chat.jsx:866:12
    value chat.jsx:866

Channels are fetched after that code

    if (showChannelsList) {
      const filters =
        channelTypeFilter === 'all'
          ? {}
          : { filters: `channel_type:${channelTypeFilter}` };
      getChannels(
        '',
        activeChannelId,
        this.props,
        channelPaginationNum,
        filters,
        this.loadChannels,
      );
      getUnopenedChannelIds(this.markUnopenedChannelIds);
    }

Sending a fix soon.

[sarthology]

Also to explain how the sockets are handled using pusher in connect, take a look at this flow chart. This shows a message with mention works.

though I have ideas on how to make this more efficient in the future.

[rhymes]

Hi @sarthology, thanks for the info! It seems to work a little bit better, though now I have to refresh the page to see the sent message in the chat. See the recording below.

Your reply did not address the other concerns though, have you tested the PR with all types of channels? Because the code seems geared only towards private channels. See my notes above.

As you can see in the recording, when I type a message it sends POST /messages but the message is not added to the inline chat, it only appears when I refresh the whole page.

[sarthology]

Yes I have tested it with various channels. Also the new error is also not repopulating.

[benhalpern]

I think this is in a good place.

If there are outstanding issues I think we'll have a better time finding them in production because I think the few non-direct channels we have will enjoy this even if there are edge cases to work out, because those areas are still de facto beta in a sense.

So I'm going to merge this now to keep progress rolling along.