More prep commits for new avatar_url handling (#4157)

[chrisbobbe]

Following #4171, here's another chunk of work that I think it would be helpful to land, to make it easier to focus on the changes to start handling avatar URLs at the edge. It includes changes to eventMiddleware as discussed around here.

[gnprice]

Thanks @chrisbobbe ! I like these changes. Some comments on specifics below.

[gnprice]

This has a subtle effect on behavior which it's not obvious to me whether it matters. Previously, we're dispatching the clearTypingNotification() before we dispatch the plain EVENT_TYPING_START action (which happens in dispatchOrBatch.) After this change, we're dispatching the plain EVENT_TYPING_START action first and then the clearTypingNotification() (via doEventActionSideEffects.)

Maybe that's fine, but we should check explicitly that we think that's fine.

I guess the same thing is true a few commits earlier, of the message sound vs. the EVENT_NEW_MESSAGE action. I'm pretty sure that one is fine -- the sound doesn't affect anything else, and the logic for whether we play the sound doesn't depend on anything that'd get updated by reducers on an EVENT_NEW_MESSAGE.

(Definitely agreed that this is a much better place for it! If the order matters, probably just move the side effects to come before dispatchOrBatch. If later we have some that need to come before and others after for some reason, we can have two different functions handling the different side effects.)

[chrisbobbe]

If later we have some that need to come before and others after for some reason, we can have two different functions handling the different side effects

Or, I guess, move these side effects into a new custom Redux Middleware; in there, we can choose when to call next.

[chrisbobbe]

This has a subtle effect on behavior which it's not obvious to me whether it matters.

Hmmm, right. I assumed it wouldn't matter, because clearTypingNotification starts off by doing nothing for 15 seconds. But there's another thing to think about: clearTypingNotification isn't dispatched unconditionally:

    case EVENT_TYPING_START:
      if (Object.keys(state.typing).length === 0) {
        dispatch(clearTypingNotification());
      }
      break;

I'll try and see if I can understand what's going on with that condition; it seems kind of wrong, like "If there are no typing notifications, clear the typing notifications".

[chrisbobbe]

OK, I resolved this confusion by reimplementing the client-side timer for the typing state; take a look, and see what you think.

[gnprice]

BTW this is part of the work for #4157.

(I see you've mentioned that in the title -- but a GitHub quirk is it doesn't parse titles for issue references, so it doesn't turn that into a link nor into a backlink from the mentioned issue.)

[chrisbobbe]

Thanks for the review! I just resolved a conflict and addressed your comments.

In particular, I reimplemented the logic around clearTypingNotification; I think it caused both of us some unnecessary confusion.

Hopefully I made it better; I think it makes more sense this way. I may have gotten so deep into it that I lost some control of the narrative of the branch, so if you're not sure what's going on, feel free to ping me rather than give yourself a headache trying to guess at what I was trying to do. 🙂 But I thought it best to let you take a look sooner rather than later, @gnprice.

[gnprice]

Thanks for the revisions!

Some comments below on the expiring-typing-notifications part. Everything else LGTM.

One thought I have on looking again at that code as we currently have it: it sure does seem to say, as you wrote, "if there are no typing notifications, clear the typing notifications". Which would mean that it never has any effect.

Given that, I think a good strategy for getting the rest of this merged is probably:

• just delete that code -- there's no regression in doing so since it already didn't work at all
• open an issue for the fact that we should be using timers to expire typing notifications after some interval if we don't hear again about them.

Then for the implementation of that issue, following up on some thoughts below, I think we ideally want something like:

• For each active typing notification -- a conversation plus a particular sender -- we have a time representing the last start event we heard, and also a timer, and we keep track of the timer ID.
• When we hear another start event, we bump the time, and also cancel the timer and make a new one.
• When we hear a stop event, we delete that record, and also cancel the timer.
• When the timer fires, we delete the record.

This way when a timer fires we can simply act on it unconditionally, and not recheck the time; if the time had been bumped (by a later start event), we'd have cancelled the timer. Rechecking the time makes me a bit nervous that we'll ignore a timer firing and then not have another one to fire later, and so leave the typing notification around indefinitely.

A consequence of having to manage cancelling timers and making new ones is that that part doesn't belong in a reducer, rather in something like a thunk action. (Though the timer IDs might naturally go in the Redux state, to be right next to the timestamps.) That probably pushes this particular reducer back toward being a relatively simple data layer that just acts on straightforward "set this" and "delete that" actions.

[gnprice]

Hmm, interesting solution!

I think this is not actually needed for the present use case. IIUC the TYPING_CLIENT_TIMER_EXPIRED action effectively just tells the reducer "hey BTW time T has come and gone, I heard a rumor you might want to update your state at key X at that time, take a look and see." And the reducer won't actually change anything unless it turns out it did indeed want to make that change at time T.

I could imagine cases where this is helpful for something else; say perhaps where we have some code make an API request and then dispatch an action with the resulting data, though that may not be a great example because we hope to cover those more systematically by canceling fetches on logout or account switch.

Perhaps keep this around in your back pocket for if we spot another case where we want to use it?

[chrisbobbe]

Are you leaning toward it being fine to let the state get updated as normal, without checking whether the same user that received the EVENT_TYPING_START is still logged in?

Maybe that's fine; I guess if there's a rapid user switch and they happen to have a conversation with the exact same people, it wouldn't feel like a glitch if the new user's state was informed by the previous user's state. (A person is either typing in a conversation, or they're not.) And, thinking about privacy of the knowledge that a different account has an active conversation with the same people as the currently active account, it's more reasonable to suppose the two accounts are owned by the same person than it would be if we were building a web app (which people will log into on public computers).

Unfortunately, we can't leave the decision to ignore (or treat differently) actions for no-longer-active accounts up to the individual reducers. typingReducer doesn't know anything about the accounts state, which we'd need to determine the active account.

[gnprice]

And, thinking about privacy of the knowledge that a different account has an active conversation with the same people as the currently active account, it's more reasonable to suppose the two accounts are owned by the same person than it would be if we were building a web app (which people will log into on public computers).

Right -- there's no privacy issue with information moving around the app. A potential privacy issue only arises when sending information to the server (or outside the app in general, but for us that almost always means to the server.)

Maybe that's fine; I guess if there's a rapid user switch and they happen to have a conversation with the exact same people, it wouldn't feel like a glitch if the new user's state was informed by the previous user's state.

Right but I think even more than that is true here: because the reducer checks the timestamp, the only information the reducer is really relying on from the action is the fact that the current time is at least what the action says it is. If the reducer gets an action which was really intended for the old account but ends up acting on it, then it must also have been the case that it was already late enough that it ought to be getting any moment now an action with the same effect that was intended for the new account.

[gnprice]

FWIW I think we're actually pretty safe from this particular risk... because the server API systematically uses snake_case, not camelCase like this name. 🙂

(Though that spreading of raw events is certainly a thing we want to fix for other reasons!)

[gnprice]

This isn't a pure refactor of the `eventTypingStop` handler, but it
makes it better by fixing a small bug. Before this change, if I'm in
a group PM with many people, and several are typing at once, an
EVENT_TYPING_STOP for a single person will clear the typing state of
*everyone* who was typing, not just the person who stopped typing.

Huh, really? This code looks to me like it correctly handled that case. What am I missing?

[gnprice]

Another thing that occurs to me on looking at this code, and in light of your later commits about TYPING_CLIENT_TIMER_EXPIRED:

It doesn't seem to make a lot of sense to bump time here. (Which we do both before and after this commit.) Effectively we're saying that if people A and B were typing, and then B stops typing... that will push back the time at which we should decide A isn't typing any more if we haven't heard another start from them.

And in fact because we only have the one "expired" action dispatched per start, we'll ignore that action and just keep thinking they're still typing forever.

In order to accurately track all this, I think what we really need is a separate time per sender -- namely the time we last heard of a typing start from them.

[gnprice]

Some chat discussion here. Merging all the changes apart from the typing-status changes:

78e00a9 eventActions [nfc]: Remove unnecessary checks in responseToActions.
ef3c7fd eventToAction: Make shell a little crunchier for message events.
f2b5b71 eventMiddleware: Stop mutating events.
871742a eventMiddleware: Rename; run on actions instead of events.
2c5d03c eventActions [nfc]: Rename responseToActions to eventsToActions.
bf504a9 doEventActionSideEffects [nfc]: Turn into an action creator.
6217f89 fetchActions tests: Mock raw server message for fetch response.
eb2fd33 eventTypes [nfc]: Synchronize EventTypes class with eventToAction.
99f959f migrateMessages tests [nfc]: Make type alias for something we write twice.
c72ec2c getMessages [nfc]: Plumb identity down to migrateMessages.

I definitely still want to see that area cleaned up, but we need some more iteration on how to do so.

[chrisbobbe]

Sounds good, thanks for the review!