Thread reactions

[uberbryn]

Status

• WIP
• Ready for review
• Needs testing

Deploy after merge (delete what needn't be deployed)

• api
• hyperion (frontend)
• desktop
• mobile

Run database migrations (delete if no migration was added)
YES

Release notes for users (delete if codebase-only change)

• Allows users to "like" threads

Related issues (delete if you don't know of any)
Closes #

[spectrum-bot]

	Warnings
⚠️	These modified files do not have Flow enabled: mercury/queues/processReputationEvent.js src/views/dashboard/components/threadCommunityInfo.js src/views/notifications/components/notificationDropdownList.js src/views/notifications/utils.js src/views/titlebar/index.js

Generated by 🚫 dangerJS

[spectrum-bot]

	Warnings
⚠️	These modified files do not have Flow enabled: mercury/queues/processReputationEvent.js src/views/dashboard/components/threadCommunityInfo.js src/views/notifications/components/notificationDropdownList.js src/views/notifications/utils.js src/views/titlebar/index.js

Generated by 🚫 dangerJS

[mxstbr]

Overall LGTM, only concern is the Thread.reactions schema as noted below. Let's get something better in there that's not like the Message.reactions one, then I'm good with the backend side of things here apart from the nits!

[mxstbr]

nit: this should probably be ThreadReactionType?

[mxstbr]

This is probably not ideal? If I'm reading this correctly, it would mean that if I spam-click the thread reaction button you'll be sent millions of notifications, one for each click. We should only ever send one of 'em?

[brianlovin]

This is handled in athena. It works by looking up a previous notification of the same type on the same entity in the previous 30mins, then will de-duplicate actors. So the result is that it will mark the notification as "unread" for the thread author, but will not actually insert multiple notifications unless there is a 30 min gap in between.

[mxstbr]

nit: should be ThreadReactionTypes

[mxstbr]

this todo doesn't make any sense, you can't react to a dm thread?

[mxstbr]

this todo doesn't make any sense, you can't react to a dm thread?

[mxstbr]

I'm going to veto using the same schema as the message reactions because it doesn't let us show which users liked the thing. (you only see it in the notifications, but nobody else can see/check) Let's change the return type here to include that information somehow, (open to ideas) so we can in the future show who liked a thread or not in the UI.

[brianlovin]

Going to change this to be a separate type ThreadReactions which will expose:

count: Int
hasReacted: Boolean
reactors: [User]

Or something like that - we can actually extend the message reactions to have the same data model, but let's keep message reactions and thread reactions as separately defined types in case they diverge in the future.

[brianlovin]

Took a while, but finally got this working. @mxstbr the one trouble I ran into was debouncing the handleScroll method in that last commit - ideally we would debounce or throttle it so we aren't putting a ton of load on the browser just to determine whether or not to show the header (I found a 100ms debounce is probably a good threshold). But I ran into some issues with React removing synthetic events from the event pool. I tried using event.persist() from the docs (https://reactjs.org/docs/events.html) but couldn't get it to work - React kept throwing the error:

This synthetic event is reused for performance reasons. If you're seeing this, you're accessing the method `target` on a released/nullified synthetic event. This is a no-op function. If you must keep the original synthetic event around, use event.persist(). See https://facebook.github.io/react/docs/events.html#event-pooling for more information.

Any chance you could take a look? Without the debounce people's machines are gonna get real real hot.

[brianlovin]

Took a while, but finally got this working. @mxstbr the one trouble I ran into was debouncing the handleScroll method in that last commit - ideally we would debounce or throttle it so we aren't putting a ton of load on the browser just to determine whether or not to show the header (I found a 100ms debounce is probably a good threshold). But I ran into some issues with React removing synthetic events from the event pool. I tried using event.persist() from the docs (https://reactjs.org/docs/events.html) but couldn't get it to work - React kept throwing the error:

This synthetic event is reused for performance reasons. If you're seeing this, you're accessing the method `target` on a released/nullified synthetic event. This is a no-op function. If you must keep the original synthetic event around, use event.persist(). See https://facebook.github.io/react/docs/events.html#event-pooling for more information.

Any chance you could take a look? Without the debounce people's machines are gonna get real real hot.

[brianlovin]

Also @uberbryn @mxstbr - if you want to pull this and try it out, let me know what you think. In my opinion it's very very distracting to have this header jump in and out every time you change the thread in the inbox view. But it seems to be quite a challenge to work around that since we're doing so much scroll position handling every time a thread is changed.

[brianlovin]

Also @uberbryn @mxstbr - if you want to pull this and try it out, let me know what you think. In my opinion it's very very distracting to have this header jump in and out every time you change the thread in the inbox view. But it seems to be quite a challenge to work around that since we're doing so much scroll position handling every time a thread is changed.

[uberbryn]

Would be cool for it to be more chill when we jump your scroll position as you mentioned, but I also think it's fine as is - it really wouldn't have bothered me other than that you mentioned it.

Other solution I could think of would be transitioning it from 0 opacity to 1 so the motion isn't jumpy, but I really don't think it's that big of a deal. Would prefer shipping and refining later.

Great work, bud! Thanks for grabbing that!

[uberbryn]

Would be cool for it to be more chill when we jump your scroll position as you mentioned, but I also think it's fine as is - it really wouldn't have bothered me other than that you mentioned it.

Other solution I could think of would be transitioning it from 0 opacity to 1 so the motion isn't jumpy, but I really don't think it's that big of a deal. Would prefer shipping and refining later.

Great work, bud! Thanks for grabbing that!

[mxstbr]

Will take a look!

[mxstbr]

Will take a look!

[mxstbr]

We shouldn't be putting the scroll offset (which changes every single frame you scroll) into the state, this is what's causing the issues. This isn't even necessary, just don't put it into the state, compare scrollOffset against threadDetailHeight and only set the state if it changes. That way you don't even need to debounce which is kinda annoying for this behavior

I'm taking a look at refactoring this!

[mxstbr]

Needs an optimistic update on the like click—feels bad rn on slow networks/with the remote server overhead. Will also tackle.

[mxstbr]

Even though you split 'em up, this is still the same schema as the message reactions? I thought we agreed that we should do this one right from the start (to one that let's us show who liked a thread) so we wouldn't have to migrate to a different one in the future?

[brianlovin]

Right, see comment below.

[mxstbr]

Needs an optimistic update on the like click—feels bad rn on slow networks/with the remote server overhead. Will also tackle.

Nevermind, played around with this a bit and it's a pain in the butt. Let's not do this for now 😅 So the last bit here is coming up with a nicer schema that let's us show who liked a thread, right?

[mxstbr]

Needs an optimistic update on the like click—feels bad rn on slow networks/with the remote server overhead. Will also tackle.

Nevermind, played around with this a bit and it's a pain in the butt. Let's not do this for now 😅 So the last bit here is coming up with a nicer schema that let's us show who liked a thread, right?

[brianlovin]

Hm, the schema will be:

type ThreadReactions {
  count: Int!
  hasReacted: Boolean
  users: [User]
}

Right? We just add a field inside that type. users could follow the connection spec, or just be an array of user data (obviously will be problematic when we get to hundreds of reactions on a post). So then it would be:

type ReactionUsersConnection {
  pageInfo: PageInfo
  edges: [ReactionUserEdge]
}

type ReactionUserEdge {
  node: User
  cursor: String
}

type ThreadReactions {
  count: Int!
  hasReacted: Boolean
  users: ReactionUsersConnection
}

But again, all of this can be added later. Querying would not change except adding the field to the graphql query and extracting it on the frontend with thread.reactions.users.edges.map(({ node: user }) => ...)

[brianlovin]

Hm, the schema will be:

type ThreadReactions {
  count: Int!
  hasReacted: Boolean
  users: [User]
}

Right? We just add a field inside that type. users could follow the connection spec, or just be an array of user data (obviously will be problematic when we get to hundreds of reactions on a post). So then it would be:

type ReactionUsersConnection {
  pageInfo: PageInfo
  edges: [ReactionUserEdge]
}

type ReactionUserEdge {
  node: User
  cursor: String
}

type ThreadReactions {
  count: Int!
  hasReacted: Boolean
  users: ReactionUsersConnection
}

But again, all of this can be added later. Querying would not change except adding the field to the graphql query and extracting it on the frontend with thread.reactions.users.edges.map(({ node: user }) => ...)

[brianlovin]

Let me see if I can handle optimistic update real quick

[brianlovin]

Let me see if I can handle optimistic update real quick

[brianlovin]

Optimistic updating is pushed, @mxstbr this is ready for a final review!

[brianlovin]

Optimistic updating is pushed, @mxstbr this is ready for a final review!

[mxstbr]

That schema actually makes a lot of sense. This LGTM, let's ship it?!

[brianlovin]

LET'S SHIP IT! Gonna put it on alpha, run the migration, test, then do a prod cut.

[brianlovin]

LET'S SHIP IT! Gonna put it on alpha, run the migration, test, then do a prod cut.