Skip to content

Commit

Permalink
recent-pms: Add a model file, including state type and reducer.
Browse files Browse the repository at this point in the history
This takes the data from `recent_private_conversations` (which we
just started requesting in the previous commit) in the initial fetch,
and then maintains it as new messages arrive.

We make good use of our new support for Immutable.js data structures.
Instead of maintaining the data in the exact same format as it came
over the wire as JSON, we arrange it on receipt into a data structure
that's optimized for efficiently maintaining and using.

The organization of the code is slightly different from our usual
existing practice:

 * The state type is defined right in the same file as the reducer
   which updates it.

 * Also in that file are some related stateless helper functions:
   in particular `usersOfKey`, which code elsewhere in the app can
   use to help interpret the data provided by this model.

 * The file is named in a general way like "thingModel", rather than
   our usual "thingReducer", and the reducer is a named rather than
   default export.

I think this is helpful here for keeping together code that
logically goes together in that it has shared internal knowledge
and invariants, like the format of PmConversationKey strings.

I think something similar would be helpful for a lot of our model
code (including most of what's in reducers and selectors), but I'm
in no rush to do a sweeping refactor; we'll want to experiment first
and find the arrangement that works best.

This version doesn't put any selector code in this file.  The
existing selector in the neighboring `pmConversationsSelectors.js`
doesn't really belong here, because it mixes together a number of
different areas of the app model, notably the unread counts.  After
some future refactoring to separate that out, there may be a more
focused selector that naturally lives here... or it may be that no
selector is needed, because it'd just be accessing a property on the
state type, and we can simplify by just saying that directly.
  • Loading branch information
gnprice committed Jan 9, 2021
1 parent 0ca375e commit 7c697fb
Show file tree
Hide file tree
Showing 5 changed files with 339 additions and 1 deletion.
2 changes: 2 additions & 0 deletions src/boot/reducers.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import narrows from '../chat/narrowsReducer';
import messages from '../message/messagesReducer';
import mute from '../mute/muteReducer';
import outbox from '../outbox/outboxReducer';
import { reducer as pmConversations } from '../pm-conversations/pmConversationsModel';
import presence from '../presence/presenceReducer';
import realm from '../realm/realmReducer';
import session from '../session/sessionReducer';
Expand Down Expand Up @@ -46,6 +47,7 @@ const reducers = {
narrows,
mute,
outbox,
pmConversations,
presence,
realm,
session,
Expand Down
2 changes: 1 addition & 1 deletion src/boot/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export const storeKeys: Array<$Keys<GlobalState>> = [
*/
// prettier-ignore
export const cacheKeys: Array<$Keys<GlobalState>> = [
'flags', 'messages', 'mute', 'narrows', 'realm', 'streams',
'flags', 'messages', 'mute', 'narrows', 'pmConversations', 'realm', 'streams',
'subscriptions', 'unread', 'userGroups', 'users',
];

Expand Down
144 changes: 144 additions & 0 deletions src/pm-conversations/__tests__/pmConversationsModel-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
/* @flow strict-local */
import Immutable from 'immutable';

import { usersOfKey, keyOfExactUsers, reducer } from '../pmConversationsModel';
import * as eg from '../../__tests__/lib/exampleData';

describe('usersOfKey', () => {
for (const [desc, ids] of [
['self-1:1', []],
['other 1:1 PM', [123]],
['group PM', [123, 345, 567]],
]) {
test(desc, () => {
expect(usersOfKey(keyOfExactUsers(ids))).toEqual(ids);
});
}
});

describe('reducer', () => {
const initialState = reducer(undefined, ({ type: eg.randString() }: $FlowFixMe));
const someKey = keyOfExactUsers([eg.makeUser().user_id]);
const someState = { map: Immutable.Map([[someKey, 123]]), sorted: Immutable.List([someKey]) };

describe('REALM_INIT', () => {
test('no data (old server)', () => {
/* eslint-disable-next-line no-unused-vars */
const { recent_private_conversations, ...rest } = eg.action.realm_init.data;
expect(reducer(someState, { ...eg.action.realm_init, data: rest })).toEqual(initialState);
});

test('works in normal case', () => {
// Includes self-1:1, other 1:1, and group PM thread.
// Out of order.
const recent_private_conversations = [
{ user_ids: [], max_message_id: 234 },
{ user_ids: [1], max_message_id: 123 },
{ user_ids: [2, 1], max_message_id: 345 }, // user_ids out of order
];
const expected = {
map: Immutable.Map([['', 234], ['1', 123], ['1,2', 345]]),
sorted: Immutable.List(['1,2', '', '1']),
};
expect(
reducer(someState, {
...eg.action.realm_init,
data: { ...eg.action.realm_init.data, recent_private_conversations },
}),
).toEqual(expected);
});
});

describe('MESSAGE_FETCH_COMPLETE', () => {
const [user1, user2] = [eg.makeUser({ user_id: 1 }), eg.makeUser({ user_id: 2 })];
const msg = (id, otherUsers) => eg.pmMessageFromTo(eg.selfUser, otherUsers, { id });
const action = messages => ({ ...eg.action.message_fetch_complete, messages });

test('works', () => {
let state = initialState;
state = reducer(
state,
action([msg(45, [user1]), msg(123, [user1, user2]), eg.streamMessage(), msg(234, [user1])]),
);
expect(state).toEqual({
map: Immutable.Map([['1', 234], ['1,2', 123]]),
sorted: Immutable.List(['1', '1,2']),
});

const newState = reducer(state, action([eg.streamMessage()]));
expect(newState).toBe(state);

state = reducer(
state,
action([
msg(345, [user2]),
msg(159, [user2]),
msg(456, [user1, user2]),
msg(102, [user1, user2]),
]),
);
expect(state).toEqual({
map: Immutable.Map([['1', 234], ['1,2', 456], ['2', 345]]),
sorted: Immutable.List(['1,2', '2', '1']),
});
});
});

describe('EVENT_NEW_MESSAGE', () => {
const actionGeneral = message => ({ ...eg.eventNewMessageActionBase, message });
const [user1, user2] = [eg.makeUser({ user_id: 1 }), eg.makeUser({ user_id: 2 })];
const action = (id, otherUsers) =>
actionGeneral(eg.pmMessageFromTo(eg.selfUser, otherUsers, { id }));

// We'll start from here to test various updates.
const baseState = (() => {
let state = initialState;
state = reducer(state, action(234, [user1]));
state = reducer(state, action(123, [user1, user2]));
return state;
})();

test('(check base state)', () => {
// This is here mostly for checked documentation of what's in
// baseState, to help in reading the other test cases.
expect(baseState).toEqual({
map: Immutable.Map([['1', 234], ['1,2', 123]]),
sorted: Immutable.List(['1', '1,2']),
});
});

test('stream message -> do nothing', () => {
const state = reducer(baseState, actionGeneral(eg.streamMessage()));
expect(state).toBe(baseState);
});

test('new conversation, newest message', () => {
const state = reducer(baseState, action(345, [user2]));
expect(state).toEqual({
map: Immutable.Map([['1', 234], ['1,2', 123], ['2', 345]]),
sorted: Immutable.List(['2', '1', '1,2']),
});
});

test('new conversation, not newest message', () => {
const state = reducer(baseState, action(159, [user2]));
expect(state).toEqual({
map: Immutable.Map([['1', 234], ['1,2', 123], ['2', 159]]),
sorted: Immutable.List(['1', '2', '1,2']),
});
});

test('existing conversation, newest message', () => {
const state = reducer(baseState, action(345, [user1, user2]));
expect(state).toEqual({
map: Immutable.Map([['1', 234], ['1,2', 345]]),
sorted: Immutable.List(['1,2', '1']),
});
});

test('existing conversation, not newest in conversation', () => {
const state = reducer(baseState, action(102, [user1, user2]));
expect(state).toBe(baseState);
});
});
});
190 changes: 190 additions & 0 deletions src/pm-conversations/pmConversationsModel.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
/* @flow strict-local */
import Immutable from 'immutable';
import invariant from 'invariant';
import {
ACCOUNT_SWITCH,
EVENT_NEW_MESSAGE,
LOGIN_SUCCESS,
LOGOUT,
MESSAGE_FETCH_COMPLETE,
REALM_INIT,
} from '../actionConstants';

import type { Action, Message, Outbox } from '../types';
import { recipientsOfPrivateMessage } from '../utils/recipient';

//
//
// Keys.
//

/** The key identifying a PM conversation in this data structure. */
// User IDs, excluding self, sorted numerically, joined with commas.
export opaque type PmConversationKey = string;

/** PRIVATE. Exported only for tests. */
// Input must have the exact right (multi-)set of users. Needn't be sorted.
export function keyOfExactUsers(ids: number[]): PmConversationKey {
return ids.sort((a, b) => a - b).join(',');
}

// Input may contain self or not, and needn't be sorted.
function keyOfUsers(ids: number[], ownUserId: number): PmConversationKey {
return keyOfExactUsers(ids.filter(id => id !== ownUserId));
}

// Input must indeed be a PM, else throws.
function keyOfPrivateMessage(msg: Message | Outbox, ownUserId: number): PmConversationKey {
return keyOfUsers(recipientsOfPrivateMessage(msg).map(r => r.id), ownUserId);
}

/** The users in the conversation, other than self. */
export function usersOfKey(key: PmConversationKey): number[] {
return key ? key.split(',').map(s => Number.parseInt(s, 10)) : [];
}

//
//
// State and reducer.
//

/**
* The list of recent PM conversations, plus data to efficiently maintain it.
*
* This gets initialized from the `recent_private_conversations` data
* structure in the `/register` response (aka our initial fetch), and then
* kept up to date as we learn about new or newly-fetched messages.
*/
// (Compare the webapp's implementation, in static/js/pm_conversations.js.)
export type PmConversationsState = {|
// The latest message ID in each conversation.
map: Immutable.Map<PmConversationKey, number>,

// The keys of the map, sorted by latest message descending.
sorted: Immutable.List<PmConversationKey>,
|};

const initialState: PmConversationsState = { map: Immutable.Map(), sorted: Immutable.List() };

// Insert the key at the proper place in the sorted list.
//
// Optimized, taking O(1) time, for the case where that place is the start,
// because that's the common case for a new message. May take O(n) time in
// general.
function insertSorted(sorted, map, key, msgId) {
const i = sorted.findIndex(k => {
const id = map.get(k);
invariant(id !== undefined, 'pm-conversations: key in sorted should be in map');
return id < msgId;
});

// Immutable.List is a deque, with O(1) shift/unshift as well as push/pop.
if (i === 0) {
return sorted.unshift(key);
}
if (i < 0) {
// (This case isn't common and isn't here to optimize, though it happens
// to be fast; it's just that `sorted.insert(-1, key)` wouldn't work.)
return sorted.push(key);
}
return sorted.insert(i, key);
}

// Insert the message into the state.
//
// Can take linear time in general. That sounds inefficient...
// but it's what the webapp does, so must not be catastrophic. 🤷
// (In fact the webapp calls `Array#sort`, which takes at *least*
// linear time, and may be 𝛳(N log N).)
//
// Optimized for the EVENT_NEW_MESSAGE case; for REALM_INIT and
// FETCH_MESSAGES_COMPLETE, if we find we want to optimize them, the first
// thing we'll want to do is probably to batch the work and skip this
// function.
//
// For EVENT_NEW_MESSAGE, the point of the event is that we're learning
// about the message in real time immediately after it was sent -- so the
// overwhelmingly common case is that the message is newer than any existing
// message we know about. (*) That's therefore the case we optimize for,
// particularly in the helper `insertSorted`.
//
// (*) The alternative is possible, but requires some kind of race to occur;
// e.g., we get a FETCH_MESSAGES_COMPLETE that includes the just-sent
// message 1002, and only after that get the event about message 1001, sent
// moments earlier. The event queue always delivers events in order, so
// even the race is possible only because we fetch messages outside of the
// event queue.
function insert(
state: PmConversationsState,
key: PmConversationKey,
msgId: number,
): PmConversationsState {
/* eslint-disable padded-blocks */
let { map, sorted } = state;
const prev = map.get(key);
// prettier-ignore
if (prev === undefined) {
// The conversation is new. Add to both `map` and `sorted`.
map = map.set(key, msgId);
return { map, sorted: insertSorted(sorted, map, key, msgId) };

} else if (prev >= msgId) {
// The conversation already has a newer message. Do nothing.
return state;

} else {
// The conversation needs to be (a) updated in `map`...
map = map.set(key, msgId);

// ... and (b) moved around in `sorted` to keep the list sorted.
const i = sorted.indexOf(key);
invariant(i >= 0, 'pm-conversations: key in map should be in sorted');
sorted = sorted.delete(i); // linear time, ouch
return { map, sorted: insertSorted(sorted, map, key, msgId) };
}
}

// Insert the message into the state.
//
// See `insert` for discussion of the time complexity.
function insertMessage(state, message, ownUserId) {
if (message.type !== 'private') {
return state;
}
return insert(state, keyOfPrivateMessage(message, ownUserId), message.id);
}

export function reducer(state: PmConversationsState = initialState, action: Action) {
switch (action.type) {
case LOGOUT:
case LOGIN_SUCCESS:
case ACCOUNT_SWITCH:
return initialState;

case REALM_INIT: {
// TODO optimize; this is quadratic (but so is the webapp's version?!)
let st = initialState;
for (const r of action.data.recent_private_conversations ?? []) {
st = insert(st, keyOfExactUsers(r.user_ids), r.max_message_id);
}
return st;
}

case MESSAGE_FETCH_COMPLETE: {
// TODO optimize; this is quadratic (but so is the webapp's version?!)
let st = state;
for (const m of action.messages) {
st = insertMessage(st, m, action.ownUserId);
}
return st;
}

case EVENT_NEW_MESSAGE: {
const { message, ownUser } = action;
return insertMessage(state, message, ownUser.user_id);
}

default:
return state;
}
}
2 changes: 2 additions & 0 deletions src/reduxTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import type {
} from './api/apiTypes';
import type { Narrow } from './utils/narrow';
import type { SessionState } from './session/sessionReducer';
import type { PmConversationsState } from './pm-conversations/pmConversationsModel';

export type * from './actionTypes';

Expand Down Expand Up @@ -358,6 +359,7 @@ export type GlobalState = {|
mute: MuteState,
narrows: NarrowsState,
outbox: OutboxState,
pmConversations: PmConversationsState,
presence: PresenceState,
realm: RealmState,
session: SessionState,
Expand Down

0 comments on commit 7c697fb

Please sign in to comment.