Moved nodesDeltaBuffer to a global state to fix the paused status ren…

…dering bug.
weaveworks · Jun 6, 2017 · f6100fe · f6100fe
1 parent fcb76db
commit f6100fe
Show file tree

Hide file tree

Showing 7 changed files with 177 additions and 143 deletions.
diff --git a/client/app/scripts/actions/app-actions.js b/client/app/scripts/actions/app-actions.js
@@ -6,9 +6,8 @@ import ActionTypes from '../constants/action-types';
 import { saveGraph } from '../utils/file-utils';
 import { updateRoute } from '../utils/router-utils';
 import {
-  bufferDeltaUpdate,
-  resumeUpdate,
-  resetUpdateBuffer,
+  isNodesDeltaPaused,
+  getUpdateBufferSize,
 } from '../utils/update-buffer-utils';
 import {
   doControlRequest,
@@ -43,6 +42,9 @@ import {
 
 const log = debug('scope:app-actions');
 
+// TODO: This shouldn't be exposed here as a global variable.
+let nodesDeltaBufferUpdateTimer = null;
+
 export function showHelp() {
   return { type: ActionTypes.SHOW_HELP };
 }
@@ -75,6 +77,30 @@ export function sortOrderChanged(sortedBy, sortedDesc) {
   };
 }
 
+function resetNodesDeltaBuffer() {
+  clearTimeout(nodesDeltaBufferUpdateTimer);
+  return { type: ActionTypes.CLEAR_NODES_DELTA_BUFFER };
+}
+
+function bufferDeltaUpdate(delta) {
+  return (dispatch, getState) => {
+    if (delta.add === null && delta.update === null && delta.remove === null) {
+      log('Discarding empty nodes delta');
+      return;
+    }
+
+    const bufferLength = 100;
+    if (getUpdateBufferSize(getState()) >= bufferLength) {
+      dispatch({ type: ActionTypes.CONSOLIDATE_NODES_DELTA_BUFFER });
+    }
+
+    dispatch({
+      type: ActionTypes.ADD_TO_NODES_DELTA_BUFFER,
+      delta,
+    });
+    log('Buffering node delta, new size', getUpdateBufferSize(getState()));
+  };
+}
 
 //
 // Networks
@@ -211,7 +237,7 @@ export function changeTopologyOption(option, value, topologyId, addOrRemove) {
     });
     updateRoute(getState);
     // update all request workers with new options
-    resetUpdateBuffer();
+    dispatch(resetNodesDeltaBuffer());
     const state = getState();
     getTopologies(activeTopologyOptionsSelector(state), dispatch);
     updateWebsocketChannel(state, dispatch);
@@ -383,15 +409,6 @@ export function clickRelative(nodeId, topologyId, label, origin) {
   };
 }
 
-export function clickResumeUpdate() {
-  return (dispatch, getState) => {
-    dispatch({
-      type: ActionTypes.CLICK_RESUME_UPDATE
-    });
-    resumeUpdate(getState);
-  };
-}
-
 function updateTopology(dispatch, getState) {
   const state = getState();
   // If we're in the resource view, get the snapshot of all the relevant node topologies.
@@ -400,7 +417,7 @@ function updateTopology(dispatch, getState) {
   }
   updateRoute(getState);
   // update all request workers with new options
-  resetUpdateBuffer();
+  dispatch(resetNodesDeltaBuffer());
   // NOTE: This is currently not needed for our static resource
   // view, but we'll need it here later and it's simpler to just
   // keep it than to redo the nodes delta updating logic.
@@ -452,7 +469,7 @@ export function websocketQueryTimestamp(queryTimestamp) {
     });
     updateWebsocketChannel(getState(), dispatch);
     // update all request workers with new options
-    resetUpdateBuffer();
+    dispatch(resetNodesDeltaBuffer());
   };
 }
 
@@ -616,7 +633,7 @@ export function receiveNodesDelta(delta) {
 
     if (hasChanges || movingInTime) {
       if (state.get('updatePausedAt') !== null) {
-        bufferDeltaUpdate(delta);
+        dispatch(bufferDeltaUpdate(delta));
       } else {
         dispatch({
           type: ActionTypes.RECEIVE_NODES_DELTA,
@@ -627,6 +644,36 @@ export function receiveNodesDelta(delta) {
   };
 }
 
+function maybeUpdateFromNodesDeltaBuffer() {
+  return (dispatch, getState) => {
+    const state = getState();
+    if (isNodesDeltaPaused(state)) {
+      dispatch(resetNodesDeltaBuffer());
+    } else {
+      if (getUpdateBufferSize(state) > 0) {
+        const delta = state.get('nodesDeltaBuffer').first();
+        dispatch({ type: ActionTypes.POP_NODES_DELTA_BUFFER });
+        dispatch(receiveNodesDelta(delta));
+      }
+      if (getUpdateBufferSize(state) > 0) {
+        const feedInterval = 1000;
+        nodesDeltaBufferUpdateTimer = setTimeout(
+          () => dispatch(maybeUpdateFromNodesDeltaBuffer()),
+        feedInterval);
+      }
+    }
+  };
+}
+
+export function clickResumeUpdate() {
+  return (dispatch, getState) => {
+    dispatch({
+      type: ActionTypes.CLICK_RESUME_UPDATE
+    });
+    dispatch(maybeUpdateFromNodesDeltaBuffer(getState));
+  };
+}
+
 export function receiveNodesForTopology(nodes, topologyId) {
   return {
     type: ActionTypes.RECEIVE_NODES_FOR_TOPOLOGY,

diff --git a/client/app/scripts/components/pause-button.js b/client/app/scripts/components/pause-button.js
@@ -0,0 +1,49 @@
+import React from 'react';
+import moment from 'moment';
+import { connect } from 'react-redux';
+
+import { getUpdateBufferSize } from '../utils/update-buffer-utils';
+import { clickPauseUpdate, clickResumeUpdate } from '../actions/app-actions';
+
+
+class PauseButton extends React.Component {
+  render() {
+    const isPaused = this.props.updatePausedAt !== null;
+    const updateCount = this.props.updateCount;
+    const hasUpdates = updateCount > 0;
+    const title = isPaused ?
+      `Paused ${moment(this.props.updatePausedAt).fromNow()}` :
+      'Pause updates (freezes the nodes in their current layout)';
+    const action = isPaused ? this.props.clickResumeUpdate : this.props.clickPauseUpdate;
+    let label = '';
+    if (hasUpdates && isPaused) {
+      label = `Paused +${updateCount}`;
+    } else if (hasUpdates && !isPaused) {
+      label = `Resuming +${updateCount}`;
+    } else if (!hasUpdates && isPaused) {
+      label = 'Paused';
+    }
+
+    return (
+      <a className="button pause-button" onClick={action} title={title}>
+        {label !== '' && <span className="pause-text">{label}</span>}
+        <span className="fa fa-pause" />
+      </a>
+    );
+  }
+}
+
+function mapStateToProps(state) {
+  return {
+    updateCount: getUpdateBufferSize(state),
+    updatePausedAt: state.get('updatePausedAt'),
+  };
+}
+
+export default connect(
+  mapStateToProps,
+  {
+    clickPauseUpdate,
+    clickResumeUpdate,
+  }
+)(PauseButton);
diff --git a/client/app/scripts/components/timeline-control.js b/client/app/scripts/components/timeline-control.js
@@ -5,15 +5,9 @@ import classNames from 'classnames';
 import { connect } from 'react-redux';
 import { debounce } from 'lodash';
 
+import PauseButton from './pause-button';
 import TopologyTimestampInfo from './topology-timestamp-info';
-import { getUpdateBufferSize } from '../utils/update-buffer-utils';
-import {
-  clickPauseUpdate,
-  clickResumeUpdate,
-  websocketQueryTimestamp,
-  startMovingInTime,
-} from '../actions/app-actions';
-
+import { websocketQueryTimestamp, startMovingInTime } from '../actions/app-actions';
 import { TIMELINE_DEBOUNCE_INTERVAL } from '../constants/timer';
 
 
@@ -78,35 +72,28 @@ const sliderRanges = {
     getStart: () => moment().utc().startOf('year'),
     getEnd: () => moment().utc(),
   },
-  yesterday: {
-    label: 'Yesterday',
-    getStart: () => moment().utc().subtract(1, 'day').startOf('day'),
-    getEnd: () => moment().utc().subtract(1, 'day').endOf('day'),
-  },
-  previousWeek: {
-    label: 'Previous week',
-    getStart: () => moment().utc().subtract(1, 'week').startOf('week'),
-    getEnd: () => moment().utc().subtract(1, 'week').endOf('week'),
-  },
-  previousMonth: {
-    label: 'Previous month',
-    getStart: () => moment().utc().subtract(1, 'month').startOf('month'),
-    getEnd: () => moment().utc().subtract(1, 'month').endOf('month'),
-  },
-  previousYear: {
-    label: 'Previous year',
-    getStart: () => moment().utc().subtract(1, 'year').startOf('year'),
-    getEnd: () => moment().utc().subtract(1, 'year').endOf('year'),
-  },
+  // yesterday: {
+  //   label: 'Yesterday',
+  //   getStart: () => moment().utc().subtract(1, 'day').startOf('day'),
+  //   getEnd: () => moment().utc().subtract(1, 'day').endOf('day'),
+  // },
+  // previousWeek: {
+  //   label: 'Previous week',
+  //   getStart: () => moment().utc().subtract(1, 'week').startOf('week'),
+  //   getEnd: () => moment().utc().subtract(1, 'week').endOf('week'),
+  // },
+  // previousMonth: {
+  //   label: 'Previous month',
+  //   getStart: () => moment().utc().subtract(1, 'month').startOf('month'),
+  //   getEnd: () => moment().utc().subtract(1, 'month').endOf('month'),
+  // },
+  // previousYear: {
+  //   label: 'Previous year',
+  //   getStart: () => moment().utc().subtract(1, 'year').startOf('year'),
+  //   getEnd: () => moment().utc().subtract(1, 'year').endOf('year'),
+  // },
 };
 
-// <div className="column">
-//   {this.renderRangeOption(sliderRanges.yesterday)}
-//   {this.renderRangeOption(sliderRanges.previousWeek)}
-//   {this.renderRangeOption(sliderRanges.previousMonth)}
-//   {this.renderRangeOption(sliderRanges.previousYear)}
-// </div>
-
 class TimelineControl extends React.PureComponent {
   constructor(props, context) {
     super(props, context);
@@ -186,23 +173,6 @@ class TimelineControl extends React.PureComponent {
     const timeStatusClassName = classNames('time-status', { 'showing-current': showingCurrent });
     const toggleButtonClassName = classNames('button toggle', { selected: showTimelinePanel });
 
-    // pause button
-    const isPaused = this.props.updatePausedAt !== null;
-    const updateCount = getUpdateBufferSize();
-    const hasUpdates = updateCount > 0;
-    const pauseTitle = isPaused ?
-      `Paused ${moment(this.props.updatePausedAt).fromNow()}` :
-      'Pause updates (freezes the nodes in their current layout)';
-    const pauseAction = isPaused ? this.props.clickResumeUpdate : this.props.clickPauseUpdate;
-    let pauseLabel = '';
-    if (hasUpdates && isPaused) {
-      pauseLabel = `Paused +${updateCount}`;
-    } else if (hasUpdates && !isPaused) {
-      pauseLabel = `Resuming +${updateCount}`;
-    } else if (!hasUpdates && isPaused) {
-      pauseLabel = 'Paused';
-    }
-
     return (
       <div className="timeline-control">
         {showTimelinePanel && <div className="timeline-panel">
@@ -239,10 +209,7 @@ class TimelineControl extends React.PureComponent {
             <TopologyTimestampInfo />
             <span className="fa fa-clock-o" />
           </a>
-          <a className="button" onClick={pauseAction} title={pauseTitle}>
-            {pauseLabel !== '' && <span className="pause-text">{pauseLabel}</span>}
-            <span className="fa fa-pause" />
-          </a>
+          <PauseButton />
           {!showingCurrent && <a
             className="button jump-to-now"
             title="Jump to now"
@@ -255,17 +222,9 @@ class TimelineControl extends React.PureComponent {
   }
 }
 
-function mapStateToProps(state) {
-  return {
-    updatePausedAt: state.get('updatePausedAt'),
-  };
-}
-
 export default connect(
-  mapStateToProps,
+  null,
   {
-    clickPauseUpdate,
-    clickResumeUpdate,
     websocketQueryTimestamp,
     startMovingInTime,
   }

diff --git a/client/app/scripts/constants/action-types.js b/client/app/scripts/constants/action-types.js
@@ -20,6 +20,10 @@ const ACTION_TYPES = [
   'CLOSE_WEBSOCKET',
   'START_MOVING_IN_TIME',
   'WEBSOCKET_QUERY_TIMESTAMP',
+  'CLEAR_NODES_DELTA_BUFFER',
+  'CONSOLIDATE_NODES_DELTA_BUFFER',
+  'ADD_TO_NODES_DELTA_BUFFER',
+  'POP_NODES_DELTA_BUFFER',
   'DEBUG_TOOLBAR_INTERFERING',
   'DESELECT_NODE',
   'DO_CONTROL',

diff --git a/client/app/scripts/reducers/__tests__/root-test.js b/client/app/scripts/reducers/__tests__/root-test.js
@@ -2,10 +2,9 @@ import { is, fromJS } from 'immutable';
 import expect from 'expect';
 
 import { TABLE_VIEW_MODE } from '../../constants/naming';
-// Root reducer test suite using Jasmine matchers
 import { constructEdgeId } from '../../utils/layouter-utils';
-// import { isResourceViewModeSelector } from '../../selectors/topology';
 
+// Root reducer test suite using Jasmine matchers
 describe('RootReducer', () => {
   const ActionTypes = require('../../constants/action-types').default;
   const reducer = require('../root').default;
@@ -16,7 +15,6 @@ describe('RootReducer', () => {
   const activeTopologyOptionsSelector = topologySelectors.activeTopologyOptionsSelector;
   const getAdjacentNodes = topologyUtils.getAdjacentNodes;
   const isTopologyEmpty = topologyUtils.isTopologyEmpty;
-  const isNodesDisplayEmpty = topologyUtils.isNodesDisplayEmpty;
   const getUrlState = require('../../utils/router-utils').getUrlState;
 
   // fixtures

diff --git a/client/app/scripts/reducers/root.js b/client/app/scripts/reducers/root.js
@@ -22,6 +22,7 @@ import {
   isResourceViewModeSelector,
 } from '../selectors/topology';
 import { activeTopologyZoomCacheKeyPathSelector } from '../selectors/zooming';
+import { consolidatedBeginningOfNodesDeltaBuffer } from '../utils/update-buffer-utils';
 import { applyPinnedSearches } from '../utils/search-utils';
 import {
   findTopologyById,
@@ -63,6 +64,7 @@ export const initialState = makeMap({
   mouseOverNodeId: null,
   nodeDetails: makeOrderedMap(), // nodeId -> details
   nodes: makeOrderedMap(), // nodeId -> node
+  nodesDeltaBuffer: makeList(),
   nodesLoaded: false,
   // nodes cache, infrequently updated, used for search & resource view
   nodesByTopology: makeMap(), // topologyId -> nodes
@@ -364,6 +366,29 @@ export function rootReducer(state = initialState, action) {
       return state.set('websocketClosed', true);
     }
 
+    //
+    // nodes delta buffer
+    //
+
+    case ActionTypes.CLEAR_NODES_DELTA_BUFFER: {
+      return state.update('nodesDeltaBuffer', buffer => buffer.clear());
+    }
+
+    case ActionTypes.CONSOLIDATE_NODES_DELTA_BUFFER: {
+      const deltaUnion = consolidatedBeginningOfNodesDeltaBuffer(state);
+      state = state.setIn(['nodesDeltaBuffer', 0], deltaUnion);
+      state = state.deleteIn(['nodesDeltaBuffer', 1]);
+      return state;
+    }
+
+    case ActionTypes.POP_NODES_DELTA_BUFFER: {
+      return state.update('nodesDeltaBuffer', buffer => buffer.shift());
+    }
+
+    case ActionTypes.ADD_TO_NODES_DELTA_BUFFER: {
+      return state.update('nodesDeltaBuffer', buffer => buffer.push(action.delta));
+    }
+
     //
     // networks
     //