@gnprice ping for a review.

I think the word processing for /me would be better in processAlertWords itself

That wouldn't be very true to the function name though, wouldn't it? This has nothing to do with alert words :)

The style of display would be better off exactly the same as the one displayed in the webapp IMO -- maybe some trickery with display: inline or :after, and CSS variables would achieve that?

No need for tricky CSS, it can be achieved by relatively straight-forward CSS... and then we need to support another way of rendering messages that is barely used and barely needed (the feature request has been open for years, the PR for a half, and no one is really asking about this, this is a 'barely worth it PR', IMHO).

Also, to reiterate, Slack do render it exactly like this PR. I would argue the web app needs to revert to this rendering too (for simplicity and maintainability).

Slack do render it exactly like this PR

Oh I wasn't aware of that, I misread the commit. Sorry about that.

This has nothing to do with alert words

/me isn't an alert word? I was under the impression that it was, my mistake. Even then though, processing another potential word would be cumbersome. Perhaps adding both of the processing to one function (e.g. processMessageContent(content)) in messageAsHtml would be better for the sake of modularity.

/me isn't an alert word? I was under the impression that it was, my mistake.

Honest error. I barely know what they are :)

https://zulipchat.com/help/add-an-alert-word

Thanks @borisyankov for this PR and @ishammahajan for the review!

On the logic: in the webapp code (thanks for the link!), it has this:

    _maybe_format_me_message: function (message_container) {
        if (message_container.msg.is_me_message) {
            // Slice the '<p>/me ' off the front, and '</p>' off the first line
            // 'p' tag is sliced off to get sender in the same line as the
            // first line of the message
            var msg_content = message_container.msg.content;
            var p_index = msg_content.indexOf('</p>');
            message_container.status_message = msg_content.slice('<p>/me '.length, p_index) +
                                                msg_content.slice(p_index + '</p>'.length);

Note in particular the is_me_message check.

So, I think we should consult that; it looks like this version doesn't. I think that will even mitigate to some extent the brittleness that you point out in the error message 🙂

Looks like we have the plumbing to sling it around; it's a flag.

zulipchat.com/help/add-an-alert-word

Helpful link! :)
Looks like I'll be getting a lot more notifications now 😛.

So, I think we should consult that; it looks like this version doesn't. I think that will even mitigate to some extent the brittleness that you point out in the error message 🙂

Looks like we have the plumbing to sling it around; it's a flag.

I am not sure what you mean.

So, I think we should consult that; it looks like this version doesn't. I think that will even mitigate to some extent the brittleness that you point out in the error message slightly_smiling_face
Looks like we have the plumbing to sling it around; it's a flag.

I am not sure what you mean.

In the webapp, the corresponding logic only fires if the is_me_message flag is set on the message.

Digging a bit more to see what that works out to... it's set from the function Message.is_status_message in the server... and in particular that rejects any message with a newline in the raw Bugdown content. So e.g. a message

/me writes a second
line

will not be treated as a me-message.

(There actually seems to be a bug where the webapp's local echo disagrees with the server -- the former does treat it as a me-message.)

I don't love the implementation of this feature, and don't have a strong view on the edge cases of what should count as a me-message; but of course whatever we do, the behavior should agree across platforms.

So to do that here, we just need to consult the is_me_message flag on the message. Which I believe we already have in our flags data structure, alongside the other flags.

Quick update -- I reported this:

(There actually seems to be a bug where the webapp's local echo disagrees with the server -- the former does treat it as a me-message.)

as zulip/zulip#12450, and it was fixed in zulip/zulip#12619 and in particular zulip/zulip@c03efd297. The backend logic was updated to match the webapp frontend.

Looking briefly back at the code in this PR, it's based on the webapp frontend, not the backend, so hopefully it's already aligned with the behavior that the backend and webapp frontend now share.

Closing because this implementation direction doesn't work. The issue remains open, and can be fixed with the approach described at #3306 (comment) above.

(Which basically just comes down to: do what the issue description at #1773 already says to do.)