Add Support for Emoji Reactions (Replaces #1980) #2127

neatchee · 2023-03-08T05:49:45Z

Originally #1980

Rebases to current glitch-soc/main
All feedback from Add Support for Emoji Reactions #1980 implemented
Additionally, keep emoji picker within screen bounds

This has been running in production without issue at https://urusai.social for several weeks now.
Urusai's repo is here: https://github.com/neatchee/mastodon

Plastikmensch · 2023-03-08T13:24:35Z

Not a review, but this reintroduces some of the deleted js locale files, which have been replaced with json files.

neatchee · 2023-03-08T21:23:45Z

Oops! Thought I fixed that in the rebase...

…

On Wed, Mar 8, 2023 at 5:24 AM Plastikmensch ***@***.***> wrote: Not a review, but this reintroduces some of the deleted js locale files, which have been replaced with json files. — Reply to this email directly, view it on GitHub <#2127 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABA3WHYUCJQHIREABXF4M23W3CCB5ANCNFSM6AAAAAAVTLJXFU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- Brian "Neatchee" Resnik

github-actions · 2023-03-15T08:00:39Z

This pull request has merge conflicts that must be resolved before it can be merged.

kescherCode · 2023-04-03T20:33:12Z

Highly suggest this e4c411e to be added.

kescherCode · 2023-04-03T20:47:44Z

Ah, I messed that commit up. Apply 9c4e060 afterwards.

Plastikmensch · 2023-04-03T21:53:18Z

Highly suggest this e4c411e to be added.

Hmh. Maybe something went wrong while porting, but integer_cast_setting doesn't prevent "e" in numbers anymore.

Edit: Not important anymore, since "e" doesn't cause any errors anymore

neatchee · 2023-04-03T22:05:42Z

Apologies if this should be obvious, as I'm not really a Ruby/React dev:
Is the intent of this patch to provide a user-facing setting for the number of visible reactions per status?

I'm not sure if I agree with exposing this setting to the end-user. Certainly not without enforcing a server-supplied maximum value.

I am generally opposed to any changes that will fragment user experience without a clear advantage to the end-user. This also increases the test surface area as we'll need to make sure nothing breaks visually across any of the UI - desktop and mobile - regardless of what the user sets this value to.

Can you make a compelling argument for why a user would want to make fewer reactions visible? More than a single line of reactions visible?

kescherCode · 2023-04-03T22:28:43Z

@neatchee this was part of the original patch, I merely ported it.

neatchee · 2023-04-03T23:08:34Z

Oh. Huh. Okay. I'll get it merged when I can

…

On Mon, Apr 3, 2023, 3:28 PM Jeremy Kescher ***@***.***> wrote: @neatchee <https://github.com/neatchee> this was part of the original patch, I merely ported it. — Reply to this email directly, view it on GitHub <#2127 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABA3WH2OUIHQTCE5B27V6PDW7NFKNANCNFSM6AAAAAAVTLJXFU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

github-actions · 2023-04-04T22:25:42Z

This pull request has resolved merge conflicts and is ready for review.

greyidol · 2023-04-28T16:37:55Z

This doesn't seem to work on Husky. I would like to be able to add emoji reactions from Husky.

Plastikmensch · 2023-04-28T16:44:11Z

This doesn't seem to work on Husky. I would like to be able to add emoji reactions from Husky.

Husky has to implement the ability to add reactions.

kescherCode · 2023-04-28T16:49:35Z

I think Husky has this ability - but since it's a fork of Tusky specifically for Pleroma, it would need adjustment for this PR.

Plastikmensch · 2023-04-28T16:52:30Z

I think Husky has this ability - but since it's a fork of Tusky specifically for Pleroma, it would need adjustment for this PR.

Yes, most likely they don't check Mastodon instances for availability of this feature.

greyidol · 2023-04-28T17:58:46Z

Yes, most likely they don't check Mastodon instances for availability of this feature.

It doesn't check. It just displays the emoji reaction button on every post, on every instance. I was hoping that this PR could implement the endpoint that Husky uses for emoji reactions, for compatibility's sake.

kescherCode · 2023-04-28T18:14:07Z

It is not the server's responsibility to implement the API as the client expects for another software's implementation.

TheEssem · 2023-04-30T03:45:30Z

Would it be possible to implement the ability to see who reacted to a post? Not showing what users reacted with while also showing who boosted/liked a post can cause confusion in quite a few contexts.

neatchee · 2023-04-30T03:56:06Z

That has been discussed as a separate change. It's desired. Just not done

…

On Sat, Apr 29, 2023, 8:45 PM Essem ***@***.***> wrote: Would it be possible to implement the ability to see who reacted to a post? Not showing what users reacted with while also showing who boosted/liked a post can cause confusion in quite a few contexts. — Reply to this email directly, view it on GitHub <#2127 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABA3WH5LCXPHBCFFAQ4N7NDXDXN6LANCNFSM6AAAAAAVTLJXFU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

github-actions · 2023-04-30T11:09:17Z

This pull request has merge conflicts that must be resolved before it can be merged.

Reticulmz · 2023-05-03T06:58:43Z

Fedibird uses the value "emoji_reaction" in the "fedibird_capabilities" key in the "/api/v1/instance" response to notify the client of the feature
SubwayTooter, for example, can use the emoji reaction if this value exists.

moyitpro · 2023-05-05T21:51:05Z

I noticed a bug with the latest Glitch SOC update. The reactions on threaded posts are misaligned.

Plastikmensch · 2023-05-05T23:43:05Z

I noticed a bug with the latest Glitch SOC update. The reactions on threaded posts are misaligned.

Ah, it's missing the margin.
adding .reactions-bar like this in flavours/glitch/styles/components/status.scss fixes that:

// line 1034
&--in-thread {
    border-bottom: 0;

    .status__content,
    .status__action-bar,
    .reactions-bar {
      margin-left: 46px + 10px;
      width: calc(100% - (46px + 10px));
    }
  }

(This also needs to be done in vanilla styles)

InvoxiPlayGames · 2023-05-07T04:24:33Z

There's a bug in this in which someone can react to a post that's not visible to them (for example, a blocked account)

neatchee · 2023-05-07T04:29:15Z

Couple questions about that: * Via direct API call? Or also via UI if the user was blocked after seeing the post? * Does the user receive a notification that the blocked user reacted to them? Will need to think about ideal behavior in that scenario since a "post does not exist" page isn't feasible

…

On Sat, May 6, 2023, 9:24 PM Emma ***@***.***> wrote: There's a bug in this in which someone can react to a post that's not visible to them (for example, a blocked account) — Reply to this email directly, view it on GitHub <#2127 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABA3WH3ND6FHJDMUANYVGNLXE4PYXANCNFSM6AAAAAAVTLJXFU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

InvoxiPlayGames · 2023-05-07T05:06:51Z

Via direct API call? Or also via UI if the user was blocked after seeing the post?

Yes via the API, however I see no reason it wouldn't work via UI so long as the client hasn't received the updates that lead the post to disappear when blocked.

Does the user receive a notification that the blocked user reacted to them?

No.

Will need to think about ideal behavior in that scenario

The /:id/favourite and /:id/reblog endpoints return a 404 for any post you can't see, for any reason (block, post privacy, etc) - I assume that would be the behaviour to replicate here as well.

neatchee · 2023-05-07T22:50:44Z

Uh, the custom_emojis_controller commit should not be in here. That was only concerning a broken merge I introduced in my fork.

Actually, a bunch of unrelated commits landed in here. Why is a broken version.rb change here :D

..........What the hell did I do....

neatchee · 2023-05-07T23:10:44Z

oh ok. I see.

The custom_emojis_controller commit was pulled in because it was apparently also missed on this side?

As for broken version.rb that's just a mistake on my part during conflict resolution.

Are there other commits you see in there that shouldn't?

kescherCode · 2023-05-07T23:22:05Z

yeah, 34f9e54 should not be in this PR, either (because it was another Catstodon specific commit)

Actually, it wasn't a bunch, it was mostly the same commit from multiple rebases being shown after each other lol.

This reverts commit 2a8b66a.

This reverts commit 34f9e54.

neatchee · 2023-05-07T23:32:28Z

I'm not gonna deal with rewinding the log again >.> Just reverting.

github-actions · 2023-05-08T16:02:04Z

This pull request has merge conflicts that must be resolved before it can be merged.

github-actions · 2023-05-08T22:08:01Z

This pull request has resolved merge conflicts and is ready for review.

Plastikmensch

This is only a surface level review for now.
Gawd this one is too big and went through too many changes...

Another thing:
reaction should be added to NON_EMAIL_TYPES in app/services/notify_service.rb as e-mail notifications for reactions are currently not supported.

Plastikmensch · 2023-05-09T10:03:15Z

app/controllers/api/v1/custom_emojis_controller.rb

@@ -2,6 +2,7 @@

 class Api::V1::CustomEmojisController < Api::BaseController
  vary_by '', unless: :disallow_unauthenticated_api_access?
+  skip_before_action :require_authenticated_user!, unless: :whitelist_mode?


Isn't this also a catstodon specific thing to allow anyone to copy custom emojis? (@kescherCode )

It is and should not be in here.

Plastikmensch · 2023-05-09T10:06:35Z

app/javascript/mastodon/initial_state.js

Why is visibleReactions missing from this file?

Plastikmensch · 2023-05-09T10:11:25Z

app/javascript/flavours/glitch/components/status_reactions.js

This file should have the .jsx extension (or better yet rewritten for .tsx)

Plastikmensch · 2023-05-09T10:13:35Z

app/javascript/flavours/glitch/containers/status_container.js

+import AccountContainer from 'flavours/glitch/containers/account_container';
+import Spoilers from '../components/spoilers';
+import Icon from 'flavours/glitch/components/icon';


Wrong merge conflict resolution?

Plastikmensch · 2023-05-09T10:14:57Z

app/javascript/flavours/glitch/initial_state.js

visibleReactions is also missing here.

Plastikmensch · 2023-05-09T10:23:04Z

app/lib/activitypub/activity/emoji_react.rb

+              @account.reacted?(original_status, name)
+
+    custom_emoji = nil
+    if name =~ /^:.*:$/


Suggested change

if name =~ /^:.*:$/

/^:.*:$/.match?(name)

Plastikmensch · 2023-05-09T10:23:48Z

app/lib/activitypub/activity/like.rb

+    return false if name.nil?
+
+    custom_emoji = nil
+    if name =~ /^:.*:$/


Suggested change

if name =~ /^:.*:$/

if /^:.*:$/.match?(name)

Plastikmensch · 2023-05-09T10:29:29Z

spec/fabricators/status_reaction_fabricator.rb

+Fabricator(:status_reaction) do
+  account      nil
+  status       nil
+  name         "MyString"
+  custom_emoji nil
+end


Suggested change

Fabricator(:status_reaction) do

account nil

status nil

name "MyString"

custom_emoji nil

end

# frozen_string_literal: true

Fabricator(:status_reaction) do

account

status

name '👍'

custom_emoji nil

end

At least making the fabricator usable.

Hmh this seems to cause a failure:

1) The status_reaction factory is valid Failure/Error: expect(factory).to be_valid expected #<StatusReaction id: 1, account_id: 110339817053204859, status_id: 110339817053191929, name: "👍", custom_emoji_id: nil, created_at: "2023-05-09 17:06:33.686302000 +0000", updated_at: "2023-05-09 17:06:33.686302000 +0000"> to be valid, but got errors: Limit of different reactions reached # ./spec/fabricators_spec.rb:9:in `block (3 levels) in <top (required)>'

Hm, that's odd indeed. I'll not apply this for now.

Yeah, I don't know how to fix that.
The fabricator itself works, it's just the fabricators_spec which fails

Plastikmensch · 2023-05-09T11:05:46Z

db/migrate/20230215074425_move_emoji_reaction_settings.rb

This migration should only be needed for people who already merged this into their forks.
Though, that is basically everyone working on this ^^

github-actions · 2023-05-09T12:51:01Z

This pull request has merge conflicts that must be resolved before it can be merged.

neatchee · 2023-05-09T18:36:48Z

Would either of you like to take over this PR? :) As mentioned above, I'm not really a Ruby dev, I just wanted the feature and it was stagnant. Happy to keep applying feedback but figured I'd offer it up :D

…

On Tue, May 9, 2023, 11:22 AM Jeremy Kescher ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/controllers/api/v1/custom_emojis_controller.rb <#2127 (comment)>: > @@ -2,6 +2,7 @@ class Api::V1::CustomEmojisController < Api::BaseController vary_by '', unless: :disallow_unauthenticated_api_access? + skip_before_action :require_authenticated_user!, unless: :whitelist_mode? It is and should not be in here. — Reply to this email directly, view it on GitHub <#2127 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABA3WH4O7DIOQF6BWBPDN53XFKDOXANCNFSM6AAAAAAVTLJXFU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Plastikmensch · 2023-05-09T18:53:12Z

Would either of you like to take over this PR? :) As mentioned above, I'm not really a Ruby dev, I just wanted the feature and it was stagnant. Happy to keep applying feedback but figured I'd offer it up :D

I really shouldn't put more work upon myself, but I would be willing to take over.
Already considered it when the old PR became stagnant.

kescherCode · 2023-05-09T18:55:06Z

As I actively maintain this in my own fork, I'd be happy to take over.

neatchee · 2023-05-09T18:56:26Z

If @kescherCode can't take it, would it be easier on you if you provide instruction and I implement to the best of my ability? Even if that means a longer review? I don't mind at all, I just don't want to make this harder than it needs to be

…

On Tue, May 9, 2023, 11:53 AM Plastikmensch ***@***.***> wrote: Would either of you like to take over this PR? :) As mentioned above, I'm not really a Ruby dev, I just wanted the feature and it was stagnant. Happy to keep applying feedback but figured I'd offer it up :D I really shouldn't put more work upon myself, but I would be willing to take over. Already considered it when the old PR became stagnant. — Reply to this email directly, view it on GitHub <#2127 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABA3WHZLKQNGR35L5OXCH7DXFKHCFANCNFSM6AAAAAAVTLJXFU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Plastikmensch · 2023-05-09T19:47:49Z

If @kescherCode can't take it, would it be easier on you if you provide instruction and I implement to the best of my ability? Even if that means a longer review? I don't mind at all, I just don't want to make this harder than it needs to be

I actually don't know, though it can be hard for me to give instructions sometimes.
I don't really mind who takes over, but I would really like this being split into frontend and backend changes, as this is so large that I actually dread looking at this.

kescherCode · 2023-05-09T19:53:54Z

I won't really split this into backend and frontend because that would really mess with the history of the existing commits...

Plastikmensch · 2023-05-09T20:02:17Z

I won't really split this into backend and frontend because that would really mess with the history of the existing commits...

Yeah... :/

kescherCode · 2023-05-09T20:44:32Z

One more migration that didn't belong here in the first place: db/migrate/20221218015350_fix_foreign_keys_status_reactions.rb. Catstodon-specific, or for folks having merged an older version of the initial emoji reaction migration.

neatchee · 2023-05-09T21:41:08Z

We're off to PR number three. #2221

Closing this one out. <3

neatchee mentioned this pull request Mar 8, 2023

Add Support for Emoji Reactions #1980

Closed

github-actions bot added the rebase needed 🚧 label Mar 15, 2023

neatchee force-pushed the feat/emoji_reactions branch from ff157b2 to 59848e2 Compare April 4, 2023 22:25

github-actions bot removed the rebase needed 🚧 label Apr 4, 2023

github-actions bot added the rebase needed 🚧 label Apr 30, 2023

Fix missed merge conflict text in version.rb

2a8b66a

neatchee added 3 commits May 7, 2023 16:26

Remove stale/missed references to makeCustomEmojiMap / EmojiMap

730878a

Revert "Fix missed merge conflict text in version.rb"

eb68f62

This reverts commit 2a8b66a.

Revert "Fix appearance/show.html.haml"

5bd0c35

This reverts commit 34f9e54.

Fix appearance/show.html.haml

f560ee4

github-actions bot added the rebase needed 🚧 label May 8, 2023

Merge remote-tracking branch 'glitch-soc/main' into feat/emoji_reactions

924e858

github-actions bot removed the rebase needed 🚧 label May 8, 2023

Plastikmensch suggested changes May 9, 2023

View reviewed changes

Plastikmensch reviewed May 9, 2023

View reviewed changes

github-actions bot added the rebase needed 🚧 label May 9, 2023

neatchee closed this May 9, 2023

greyidol mentioned this pull request Jul 29, 2023

Add Support for Emoji Reactions (Replaces #1980 and #2127) #2221

Closed

TheEssem mentioned this pull request Nov 10, 2023

Emoji reactions support, take 4 #2462

Open

Add Support for Emoji Reactions (Replaces #1980) #2127

Add Support for Emoji Reactions (Replaces #1980) #2127

Conversation

neatchee commented Mar 8, 2023 • edited Loading

Plastikmensch commented Mar 8, 2023

neatchee commented Mar 8, 2023 via email

github-actions bot commented Mar 15, 2023

kescherCode commented Apr 3, 2023

kescherCode commented Apr 3, 2023

Plastikmensch commented Apr 3, 2023 • edited Loading

neatchee commented Apr 3, 2023

kescherCode commented Apr 3, 2023

neatchee commented Apr 3, 2023 via email

github-actions bot commented Apr 4, 2023

greyidol commented Apr 28, 2023

Plastikmensch commented Apr 28, 2023

kescherCode commented Apr 28, 2023

Plastikmensch commented Apr 28, 2023

greyidol commented Apr 28, 2023 • edited Loading

kescherCode commented Apr 28, 2023

TheEssem commented Apr 30, 2023

neatchee commented Apr 30, 2023 via email

github-actions bot commented Apr 30, 2023

Reticulmz commented May 3, 2023 • edited Loading

moyitpro commented May 5, 2023

Plastikmensch commented May 5, 2023 • edited Loading

InvoxiPlayGames commented May 7, 2023

neatchee commented May 7, 2023 via email

InvoxiPlayGames commented May 7, 2023

neatchee commented May 7, 2023

neatchee commented May 7, 2023

kescherCode commented May 7, 2023 • edited Loading

neatchee commented May 7, 2023

github-actions bot commented May 8, 2023

github-actions bot commented May 8, 2023

Plastikmensch left a comment • edited Loading

Choose a reason for hiding this comment

Plastikmensch May 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 9, 2023

neatchee commented May 9, 2023 via email

Plastikmensch commented May 9, 2023

kescherCode commented May 9, 2023

neatchee commented May 9, 2023 via email

Plastikmensch commented May 9, 2023

kescherCode commented May 9, 2023

Plastikmensch commented May 9, 2023

kescherCode commented May 9, 2023

neatchee commented May 9, 2023

neatchee commented Mar 8, 2023 •

edited

Loading

Plastikmensch commented Apr 3, 2023 •

edited

Loading

greyidol commented Apr 28, 2023 •

edited

Loading

Reticulmz commented May 3, 2023 •

edited

Loading

Plastikmensch commented May 5, 2023 •

edited

Loading

kescherCode commented May 7, 2023 •

edited

Loading

Plastikmensch left a comment •

edited

Loading

Plastikmensch May 9, 2023 •

edited

Loading