I was thinking about this and I reached a conclusion that we don't need to check whether we are connected to a keyserver. The reason for that is that when we receive notification it triggers redux update so that extendedUnreadCount selector is recalculated. This means that if we are disconnected from the keyserver then redux either reflects the state from the last time we were connected to the keyserver or we received a notif containing never state that triggered redux update. In either case we should the value from redux to calculate total unread count.

Nit: weird that you're mixing paradigms here... why not replace your for loop with a filter, so it matches with the map / flat approach later?

I'm not sure I follow this reasoning.

The behavior before was that the unread count could be updated in one of two ways:

• Directly from a notif. In that case, the unread count is updated by native code
• From this code. For a given keyserver, we only want to override the unread count we got from a notif if we are connected to that keyserver – otherwise, our local store may be more out-of-date than the most recent notif

I'm concerned that the user might open the app and have the badge count immediately update based on the app's out-of-date local data, which would be bad because it may be more out-of-date than the prior badge count (based on a notif).

I opted for this since in the following construction:

const removeKeyserversOperations = ops.filter(op => op.type === 'remove_keyservers')

flow doesn't understand that the array now consists of RemoveKeyserversOperation only but thinks that this array can contain any KeyserverStoreOperation. This breaks map used later. In order to use filter exclusively we need sth like this:

return ops
    .filter(op => op.type === 'remove_keyservers')
    .map(operation => operation.payload?.ids)
    .filter(Boolean)
    .flat();

Fot me personally it is even weirder since we are checking for values that we are sure that are truthy.

Can we name this something more descriptive, eg. perKeyserverUnreadCount?

Is this an unexpected condition? Should we use an invariant, or at least print an error?

This is just KeyserverInfos

Shouldn't this be messages for keyserverID? i.e. keyserverIDToMessageInfos[keyserverID]

extractKeyserverIDFromID returns a string, it's not possible for this to be falsy unless it's an empty string, which would mean that message.threadID was an empty string. I don't think this is possible?

We already have allUpdatesCurrentAsOff selector in this file that returns similar thing. I will update to allUnreadCounts to match convention.

Definitely - my mistake.

According to @inka explanation invariant seems to be best choice here.

In this case, we could consider making both transformations in the for loop instead. Also, I think that having a flat array isn't ideal, because it won't handle duplicated IDs properly.

This could be expressed using lodash

Can we define a separate constant for a separator and avoid including the separator in prefixes and suffixes?

We can consider simplifying this logic

What does -1 mean?

It is explained here.