Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/api/eventTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,26 @@
import type { Message, Stream, UserPresence } from './modelTypes';

export class EventTypes {
static alert_words: 'alert_words' = 'alert_words';
static delete_message: 'delete_message' = 'delete_message';
static heartbeat: 'heartbeat' = 'heartbeat';
static message: 'message' = 'message';
static muted_topics: 'muted_topics' = 'muted_topics';
static presence: 'presence' = 'presence';
static reaction: 'reaction' = 'reaction';
static realm_bot: 'realm_bot' = 'realm_bot';
static realm_emoji: 'realm_emoji' = 'realm_emoji';
static realm_filters: 'realm_filters' = 'realm_filters';
static realm_user: 'realm_user' = 'realm_user';
static stream: 'stream' = 'stream';
static submessage: 'submessage' = 'submessage';
static subscription: 'subscription' = 'subscription';
static typing: 'typing' = 'typing';
static update_display_settings: 'update_display_settings' = 'update_display_settings';
static update_global_notifications: 'update_global_notifications' = 'update_global_notifications';
static update_message: 'update_message' = 'update_message';
static update_message_flags: 'update_message_flags' = 'update_message_flags';
static user_group: 'user_group' = 'user_group';
static user_status: 'user_status' = 'user_status';
}
Comment thread
gnprice marked this conversation as resolved.

Expand Down
9 changes: 6 additions & 3 deletions src/api/messages/__tests__/migrateMessages-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import omit from 'lodash.omit';

import { migrateMessages } from '../getMessages';
import { identityOfAuth } from '../../../account/accountMisc';
import * as eg from '../../../__tests__/lib/exampleData';
import type { ServerMessage, ServerReaction } from '../getMessages';
import type { Message } from '../../modelTypes';
Expand All @@ -20,11 +21,13 @@ describe('migrateMessages', () => {
},
};

type CommonFields = $Diff<Message, { reactions: mixed }>;

const serverMessage: ServerMessage = {
// The `omit` shouldn't be necessary with Flow v0.111: "Spreads
// now overwrite properties instead of inferring unions"
// (https://medium.com/flow-type/spreads-common-errors-fixes-9701012e9d58).
...(omit(eg.streamMessage(), 'reactions'): $Diff<Message, { reactions: mixed }>),
...(omit(eg.streamMessage(), 'reactions'): CommonFields),
reactions: [serverReaction],
};

Expand All @@ -33,7 +36,7 @@ describe('migrateMessages', () => {
const expectedOutput: Message[] = [
{
// The `omit` shouldn't be necessary with Flow v0.111.
...(omit(serverMessage, 'reactions'): $Diff<ServerMessage, { reactions: mixed }>),
...(omit(serverMessage, 'reactions'): CommonFields),
reactions: [
{
user_id: reactingUser.user_id,
Expand All @@ -45,7 +48,7 @@ describe('migrateMessages', () => {
},
];

const actualOutput: Message[] = migrateMessages(input);
const actualOutput: Message[] = migrateMessages(input, identityOfAuth(eg.selfAuth));

test('Replace user object with `user_id`', () => {
expect(actualOutput.map(m => m.reactions)).toEqual(expectedOutput.map(m => m.reactions));
Expand Down
10 changes: 6 additions & 4 deletions src/api/messages/getMessages.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
/* @flow strict-local */
import type { Auth, ApiResponseSuccess } from '../transportTypes';
import type { Identity } from '../../types';
import type { Message, Narrow } from '../apiTypes';
import type { Reaction } from '../modelTypes';
import { apiGet } from '../apiFetch';
import { identityOfAuth } from '../../account/accountMisc';

type ApiResponseMessages = {|
...ApiResponseSuccess,
Expand Down Expand Up @@ -41,7 +43,7 @@ type ServerApiResponseMessages = {|
|};

/** Exported for tests only. */
export const migrateMessages = (messages: ServerMessage[]): Message[] =>
export const migrateMessages = (messages: ServerMessage[], identity: Identity): Message[] =>
messages.map(message => {
const { reactions, ...restMessage } = message;
return {
Expand All @@ -56,11 +58,11 @@ export const migrateMessages = (messages: ServerMessage[]): Message[] =>
};
});

const migrateResponse = response => {
const migrateResponse = (response, identity: Identity) => {
const { messages, ...restResponse } = response;
return {
...restResponse,
messages: migrateMessages(messages),
messages: migrateMessages(messages, identity),
};
};

Expand Down Expand Up @@ -91,5 +93,5 @@ export default async (
apply_markdown: true,
use_first_unread_anchor: useFirstUnread,
});
return migrateResponse(response);
return migrateResponse(response, identityOfAuth(auth));
};
10 changes: 5 additions & 5 deletions src/events/__tests__/eventActions--test.js
Original file line number Diff line number Diff line change
@@ -1,26 +1,26 @@
import { responseToActions } from '../eventActions';
import { eventsToActions } from '../eventActions';

console.log = () => {}; // eslint-disable-line

describe('responseToActions', () => {
describe('eventsToActions', () => {
test('empty response return no actions', () => {
const events = [];
const actions = responseToActions({}, events);
const actions = eventsToActions({}, events);

expect(actions).toBeEmpty();
});

test('filter out unknown event types and some known ones', () => {
const events = [{ type: 'some unknown type' }, { type: 'heartbeat' }];
const actions = responseToActions({}, events);
const actions = eventsToActions({}, events);

expect(actions).toBeEmpty();
});

test('when known events process and return actions', () => {
const event = { type: 'presence' };
const events = [event];
const actions = responseToActions({}, events);
const actions = eventsToActions({}, events);

expect(actions).toHaveLength(1);
});
Expand Down
27 changes: 0 additions & 27 deletions src/events/__tests__/eventMiddleware-test.js

This file was deleted.

57 changes: 57 additions & 0 deletions src/events/doEventActionSideEffects.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/* @flow strict-local */
// import { Vibration } from 'react-native';

import { AppState } from 'react-native';
import type { GlobalState, GetState, Dispatch, Message } from '../types';
import type { EventAction } from '../actionTypes';
import { EVENT_NEW_MESSAGE } from '../actionConstants';
import { isHomeNarrow, isMessageInNarrow } from '../utils/narrow';
import { getActiveAccount, getChatScreenParams, getOwnEmail } from '../selectors';
import { playMessageSound } from '../utils/sound';
import { NULL_ARRAY } from '../nullObjects';

/**
* React to incoming `MessageEvent`s.
*/
const messageEvent = (state: GlobalState, message: Message): void => {
const flags = message.flags ?? NULL_ARRAY;

if (AppState.currentState !== 'active') {
return;
}

const isPrivateMessage = Array.isArray(message.display_recipient);
const isMentioned = flags.includes('mentioned') || flags.includes('wildcard_mentioned');
if (!(isPrivateMessage || isMentioned)) {
return;
}

const activeAccount = getActiveAccount(state);
const { narrow } = getChatScreenParams(state);
const isUserInSameNarrow =
activeAccount
&& narrow !== undefined // chat screen is not at top
&& !isHomeNarrow(narrow)
&& isMessageInNarrow(message, narrow, activeAccount.email);
const isSenderSelf = getOwnEmail(state) === message.sender_email;
if (!isUserInSameNarrow && !isSenderSelf) {
playMessageSound();
// Vibration.vibrate();
}
};

/**
* React to actions dispatched for Zulip server events.
*
* To be dispatched after the event actions are dispatched.
*/
export default (action: EventAction) => async (dispatch: Dispatch, getState: GetState) => {
const state = getState();
switch (action.type) {
case EVENT_NEW_MESSAGE: {
messageEvent(state, action.message);
break;
}
default:
}
};
25 changes: 15 additions & 10 deletions src/events/eventActions.js
Original file line number Diff line number Diff line change
@@ -1,34 +1,32 @@
/* @flow strict-local */
import { batchActions } from 'redux-batched-actions';

import type { EventAction } from '../actionTypes';
import type { Action, Dispatch, GeneralEvent, GetState, GlobalState } from '../types';
import * as api from '../api';
import { logout } from '../account/accountActions';
import { deadQueue } from '../session/sessionActions';
import eventToAction from './eventToAction';
import eventMiddleware from './eventMiddleware';
import doEventActionSideEffects from './doEventActionSideEffects';
import { tryGetAuth } from '../selectors';
import actionCreator from '../actionCreator';
import { BackoffMachine } from '../utils/async';
import { ApiError } from '../api/apiErrors';

/** Convert an `/events` response into a sequence of our Redux actions. */
export const responseToActions = (
export const eventsToActions = (
state: GlobalState,
events: $ReadOnlyArray<GeneralEvent>,
): Action[] =>
): EventAction[] =>
events
.map(event => {
eventMiddleware(state, event);
return eventToAction(state, event);
})
.map(event => eventToAction(state, event))
.filter(action => {
if (action.type === 'ignore') {
return false;
}

if (!action || !action.type || action.type === 'unknown') {
console.log('Can not handle event', action.event); // eslint-disable-line
if (action.type === 'unknown') {
console.log('Cannot handle event', action.event); // eslint-disable-line
return false;
}

Expand Down Expand Up @@ -73,11 +71,18 @@ export const startEventPolling = (queueId: number, eventId: number) => async (
break;
}

const actions = responseToActions(getState(), events);
const actions = eventsToActions(getState(), events);

actionCreator(dispatch, actions, getState());
dispatchOrBatch(dispatch, actions);

actions.forEach(action => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// These side effects should not be moved to reducers, which
// are explicitly not the place for side effects (see
// https://redux.js.org/faq/actions).
dispatch(doEventActionSideEffects(action));
});

lastEventId = Math.max.apply(null, [lastEventId, ...events.map(x => x.id)]);
} catch (e) {
if (e.httpStatus === 401) {
Expand Down
63 changes: 0 additions & 63 deletions src/events/eventMiddleware.js

This file was deleted.

9 changes: 8 additions & 1 deletion src/events/eventToAction.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,15 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => {

case 'message':
return {
...event,
type: EVENT_NEW_MESSAGE,
id: event.id,
message: {
...event.message,
// Move `flags` key from `event` to `event.message` for consistency, and
// default to an empty array if `event.flags` is not set.
flags: event.message.flags ?? event.flags ?? [],
},
local_message_id: event.local_message_id,
caughtUp: state.caughtUp,
ownEmail: getOwnEmail(state),
};
Expand Down
Loading