Skip to content

Commit

Permalink
presence: replace all presence-reporting logic
Browse files Browse the repository at this point in the history
Replace the existing presence-reporting logic with HeartbeatComponent,
instantiated as part of a new zero-display React component located
under AppEventHandlers. In particular, strip the throttling logic from
`reportPresence`.

This fixes at least three issues:

* If the user logged in and out, or otherwise performed an additional
  "initial fetch" without killing the app, the presence timer would be
  started multiple times. (The negative effects of this were
  previously concealed by the throttling logic.)

* `idle` events were not treated differently from `active` events by
  the throttling logic, and would almost invariably be swallowed.

* If the user went idle (e.g. by hitting the Home button), the
  presence timer would still run, and would still attempt to send
  `active` notifications. (These attempts were often suppressed while
  the app was in the background, but this was never guaranteed.)

Fixes the original version of zulip#3699; the next commit will complete the
fix for that issue.

This is also work towards zulip#3659.  Unfortunately, there is no
user-visible change yet: modifications will be needed on the Zulip
server side as well.
  • Loading branch information
rk-for-zulip authored and gnprice committed Mar 6, 2020
1 parent ff65ced commit 6efc449
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 23 deletions.
17 changes: 8 additions & 9 deletions src/boot/AppEventHandlers.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,8 @@ import type { Dispatch, Orientation as OrientationT } from '../types';
import { connect } from '../react-redux';
import { getUnreadByHuddlesMentionsAndPMs } from '../selectors';
import { handleInitialNotification, NotificationListener } from '../notification';
import {
appOnline,
appOrientation,
appState,
initSafeAreaInsets,
reportPresence,
} from '../actions';
import { appOnline, appOrientation, appState, initSafeAreaInsets } from '../actions';
import PresenceHeartbeat from '../presence/PresenceHeartbeat';

/**
* Part of the interface from react-native-netinfo.
Expand Down Expand Up @@ -90,7 +85,6 @@ class AppEventHandlers extends PureComponent<Props> {
/** For the type, see docs: https://facebook.github.io/react-native/docs/appstate */
handleAppStateChange = (state: 'active' | 'background' | 'inactive') => {
const { dispatch, unreadCount } = this.props;
dispatch(reportPresence(state === 'active'));
dispatch(appState(state === 'active'));
if (state === 'background' && Platform.OS === 'android') {
NativeModules.BadgeCountUpdaterModule.setBadgeCount(unreadCount);
Expand Down Expand Up @@ -131,7 +125,12 @@ class AppEventHandlers extends PureComponent<Props> {
}

render() {
return <View style={styles.wrapper}>{this.props.children}</View>;
return (
<>
<PresenceHeartbeat />
<View style={styles.wrapper}>{this.props.children}</View>
</>
);
}
}

Expand Down
4 changes: 0 additions & 4 deletions src/message/fetchActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import { tryUntilSuccessful } from '../utils/async';
import { initNotifications } from '../notification/notificationActions';
import { addToOutbox, sendOutbox } from '../outbox/outboxActions';
import { realmInit } from '../realm/realmActions';
import { reportPresence } from '../users/usersActions';
import { startEventPolling } from '../events/eventActions';
import { logout } from '../account/accountActions';

Expand Down Expand Up @@ -222,9 +221,6 @@ const fetchTopMostNarrow = () => async (dispatch: Dispatch, getState: GetState)
* we want to do when starting up, or regaining a network connection.
*/
export const doInitialFetch = () => async (dispatch: Dispatch, getState: GetState) => {
dispatch(reportPresence());
setInterval(() => dispatch(reportPresence()), 60 * 1000);

dispatch(initialFetchStart());
const auth = getAuth(getState());

Expand Down
72 changes: 72 additions & 0 deletions src/presence/PresenceHeartbeat.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
// @flow strict-local
import { PureComponent } from 'react';
import { AppState } from 'react-native';
import type { Auth, Dispatch } from '../types';

import { connect } from '../react-redux';
import { tryGetAuth } from '../account/accountsSelectors';
import { reportPresence } from '../actions';
import Heartbeat from './heartbeat';

type Props = $ReadOnly<{|
dispatch: Dispatch,
auth: Auth | void,
|}>;

/**
* Component providing a recurrent `presence` signal.
*/
// The name "PureComponent" is potentially misleading here, as this component is
// in no reasonable sense "pure" -- its entire purpose is to emit network calls
// for their observable side effects as a side effect of being rendered.
//
// (This is merely a misnomer on React's part, rather than a functional error. A
// `PureComponent` is simply one which never updates except when its props have
// changed -- which is exactly what we want.)
class PresenceHeartbeat extends PureComponent<Props> {
/** Callback for Heartbeat object. */
onHeartbeat = (state: boolean) => {
// N.B.: If `auth` changes, we do not send out a final `false` presence
// status for the previous `auth`. It's the responsibility of our logout
// handler to determine whether that's necessary.
//
// (TODO: ensure that our logout handlers actually do that.)
if (this.props.auth) {
this.props.dispatch(reportPresence(state));
}
};

heartbeat: Heartbeat = new Heartbeat(this.onHeartbeat, 1000 * 60);

componentDidMount() {
this.updateHeartbeatState(); // conditional start
AppState.addEventListener('change', this.updateHeartbeatState);
}

componentWillUnmount() {
AppState.removeEventListener('change', this.updateHeartbeatState);
this.heartbeat.stop(); // unconditional stop
}

// React to any state change.
updateHeartbeatState = () => {
// heartbeat.toState is idempotent
this.heartbeat.toState(AppState.currentState === 'active' && !!this.props.auth);
};

// React to props changes.
//
// Not dependent on `render()`'s return value, although the docs may not yet
// be clear on that. See: https://github.com/reactjs/reactjs.org/pull/1230.
componentDidUpdate() {
this.updateHeartbeatState();
}

render() {
return null;
}
}

export default connect(state => ({
auth: tryGetAuth(state),
}))(PresenceHeartbeat);
10 changes: 0 additions & 10 deletions src/users/usersActions.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
/* @flow strict-local */
import differenceInSeconds from 'date-fns/difference_in_seconds';
import * as typing_status from '@zulip/shared/js/typing_status';

import type { Dispatch, GetState, Narrow } from '../types';
Expand All @@ -9,8 +8,6 @@ import { getAuth, tryGetAuth } from '../selectors';
import { isPrivateOrGroupNarrow, caseNarrowPartial } from '../utils/narrow';
import { getAllUsersByEmail } from './userSelectors';

let lastReportPresence = new Date(0);

export const reportPresence = (isActive: boolean = true, newUserInput: boolean = false) => async (
dispatch: Dispatch,
getState: GetState,
Expand All @@ -20,13 +17,6 @@ export const reportPresence = (isActive: boolean = true, newUserInput: boolean =
return; // not logged in
}

const now = new Date();
if (differenceInSeconds(now, lastReportPresence) < 60) {
// TODO throttle properly; probably fold setInterval logic in here
return;
}
lastReportPresence = now;

const response = await api.reportPresence(auth, isActive, newUserInput);
dispatch({
type: PRESENCE_RESPONSE,
Expand Down

0 comments on commit 6efc449

Please sign in to comment.