Move scroll indicator from set/map to standalone

Signed-off-by: Everett Ross <reverett@uber.com>

packages/jaeger-ui/src/components/TracePage/ScrollManager.tsx

@@ -131,8 +131,15 @@ export default class ScrollManager {
     }
     const { duration, spans, startTime: traceStartTime } = this._trace;
     const isUp = direction < 0;
-    const boundaryRow = isUp ? xrs.getTopRowIndexVisible() : xrs.getBottomRowIndexVisible();
-    const spanIndex = xrs.mapRowIndexToSpanIndex(startRow != null ? startRow : boundaryRow);
+    let boundaryRow: number;
+    if (startRow != null) {
+      boundaryRow = startRow;
+    } else if (isUp) {
+      boundaryRow = xrs.getTopRowIndexVisible();
+    } else {
+      boundaryRow = xrs.getBottomRowIndexVisible();
+    }
+    const spanIndex = xrs.mapRowIndexToSpanIndex(boundaryRow);
     if ((spanIndex === 0 && isUp) || (spanIndex === spans.length - 1 && !isUp)) {
       return;
     }

packages/jaeger-ui/src/components/TracePage/TracePageHeader/TracePageSearchBar.css

@@ -23,12 +23,12 @@ limitations under the License.
 }
 
 .TracePageSearchBar--bar {
-  max-width: 100%;
-  transition: max-width 1.5s;
+  max-width: 20rem;
+  transition: max-width 0.5s;
 }
 
-.TracePageSearchBar--bar:not(:focus-within) {
-  max-width: 20rem;
+.TracePageSearchBar--bar:focus-within {
+  max-width: 100%;
 }
 
 .TracePageSearchBar--count {
@@ -43,3 +43,7 @@ limitations under the License.
 .TracePageSearchBar--btn.is-disabled {
   opacity: 0.5;
 }
+
+.TracePageSearchBar--btn.locate-btn {
+  padding: 1px 8px 4px;
+}

packages/jaeger-ui/src/components/TracePage/TracePageHeader/TracePageSearchBar.tsx

@@ -68,7 +68,7 @@ export function TracePageSearchBarFn(props: TracePageSearchBarProps & { forwarde
         {navigable && (
           <>
             <Button
-              className={cx(btnClass, 'ant-btn-icon-only')}
+              className={cx(btnClass, 'locate-btn')}
               disabled={!textFilter}
               htmlType="button"
               onClick={focusUiFindMatches}

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.test.js

@@ -32,6 +32,7 @@ describe('<VirtualizedTraceViewImpl>', () => {
   const props = {
     childrenHiddenIDs: new Set(),
     childrenToggle: jest.fn(),
+    clearShouldScrollToFirstUiFindMatch: jest.fn(),
     currentViewRangeTime: [0.25, 0.75],
     detailLogItemToggle: jest.fn(),
     detailLogsToggle: jest.fn(),
@@ -44,6 +45,7 @@ describe('<VirtualizedTraceViewImpl>', () => {
     scrollToFirstVisibleSpan: jest.fn(),
     setSpanNameColumnWidth: jest.fn(),
     setTrace: jest.fn(),
+    shouldScrollToFirstUiFindMatch: false,
     spanNameColumnWidth: 0.5,
     trace,
     uiFind: 'uiFind',
@@ -345,32 +347,46 @@ describe('<VirtualizedTraceViewImpl>', () => {
     });
   });
 
-  describe('handles needToScroll', () => {
+  describe('shouldScrollToFirstUiFindMatch', () => {
+    const propsWithTruthShouldScrollToFirstUiFindMatch = { ...props, shouldScrollToFirstUiFindMatch: true };
+
     beforeEach(() => {
       props.scrollToFirstVisibleSpan.mockReset();
+      props.clearShouldScrollToFirstUiFindMatch.mockReset();
     });
 
-    [[Set, 'childrenHiddenIDs'], [Map, 'detailStates']].forEach(([ConstructorFn, propName]) => {
-      it(`calls props.scrollToFirstVisibleSpan if ${propName} changes and ${propName}.needToScroll is true`, () => {
-        expect(props.scrollToFirstVisibleSpan).not.toHaveBeenCalled();
+    it('calls props.scrollToFirstVisibleSpan if shouldScrollToFirstUiFindMatch is true', () => {
+      expect(props.scrollToFirstVisibleSpan).not.toHaveBeenCalled();
+      expect(props.clearShouldScrollToFirstUiFindMatch).not.toHaveBeenCalled();
 
-        const collectionThatNeedsToScroll = new ConstructorFn();
-        collectionThatNeedsToScroll.needToScroll = true;
+      wrapper.setProps(propsWithTruthShouldScrollToFirstUiFindMatch);
+      expect(props.scrollToFirstVisibleSpan).toHaveBeenCalledTimes(1);
+      expect(props.clearShouldScrollToFirstUiFindMatch).toHaveBeenCalledTimes(1);
+    });
 
-        wrapper.setProps({ [propName]: collectionThatNeedsToScroll });
-        expect(props.scrollToFirstVisibleSpan).toHaveBeenCalledTimes(1);
+    describe('shouldComponentUpdate', () => {
+      it('returns true if props.shouldScrollToFirstUiFindMatch is unchanged', () => {
+        expect(wrapper.instance().shouldComponentUpdate(props)).toBe(true);
+      });
 
-        wrapper.setProps({ [propName]: collectionThatNeedsToScroll });
-        expect(props.scrollToFirstVisibleSpan).toHaveBeenCalledTimes(1);
+      it('returns true if props.shouldScrollToFirstUiFindMatch changes to true', () => {
+        expect(wrapper.instance().shouldComponentUpdate(propsWithTruthShouldScrollToFirstUiFindMatch)).toBe(
+          true
+        );
+      });
 
-        const differentCollectionThatAlsoNeedsToScroll = new ConstructorFn();
-        differentCollectionThatAlsoNeedsToScroll.needToScroll = true;
-        wrapper.setProps({ [propName]: differentCollectionThatAlsoNeedsToScroll });
-        expect(props.scrollToFirstVisibleSpan).toHaveBeenCalledTimes(2);
+      it('returns true if props.shouldScrollToFirstUiFindMatch changes to false and another props change', () => {
+        const propsWithOtherDifferenceAndTrueshouldScrollToFirstUiFindMatch = {
+          ...propsWithTruthShouldScrollToFirstUiFindMatch,
+          clearShouldScrollToFirstUiFindMatch: () => {},
+        };
+        wrapper.setProps(propsWithOtherDifferenceAndTrueshouldScrollToFirstUiFindMatch);
+        expect(wrapper.instance().shouldComponentUpdate(props)).toBe(true);
+      });
 
-        const differentCollectionThatDoesNotNeedToScroll = new ConstructorFn();
-        wrapper.setProps({ [propName]: differentCollectionThatDoesNotNeedToScroll });
-        expect(props.scrollToFirstVisibleSpan).toHaveBeenCalledTimes(2);
+      it('returns false if props.shouldScrollToFirstUiFindMatch changes to false and no other props change', () => {
+        wrapper.setProps(propsWithTruthShouldScrollToFirstUiFindMatch);
+        expect(wrapper.instance().shouldComponentUpdate(props)).toBe(false);
       });
     });
   });

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.tsx

@@ -59,6 +59,7 @@ type TVirtualizedTraceViewOwnProps = {
 
 type TDispatchProps = {
   childrenToggle: (spanID: string) => void;
+  clearShouldScrollToFirstUiFindMatch: () => void;
   detailLogItemToggle: (spanID: string, log: Log) => void;
   detailLogsToggle: (spanID: string) => void;
   detailProcessToggle: (spanID: string) => void;
@@ -135,6 +136,7 @@ function getCssClasses(currentViewRange: [number, number]) {
 }
 
 // export from tests
+// eslint-disable-next-line react/no-redundant-should-component-update
 export class VirtualizedTraceViewImpl extends React.PureComponent<VirtualizedTraceViewProps> {
   clippingCssClasses: string;
   listView: ListView | TNil;
@@ -159,6 +161,25 @@ export class VirtualizedTraceViewImpl extends React.PureComponent<VirtualizedTra
     setTrace(trace, uiFind);
   }
 
+  shouldComponentUpdate(nextProps: VirtualizedTraceViewProps) {
+    if (
+      !nextProps.shouldScrollToFirstUiFindMatch &&
+      nextProps.shouldScrollToFirstUiFindMatch !== this.props.shouldScrollToFirstUiFindMatch
+    ) {
+      const nextPropKeys = Object.keys(nextProps);
+      for (let i = 0; i < nextPropKeys.length; i += 1) {
+        if (
+          nextProps[nextPropKeys[i] as keyof VirtualizedTraceViewProps] !==
+            this.props[nextPropKeys[i] as keyof VirtualizedTraceViewProps] &&
+          nextPropKeys[i] !== 'shouldScrollToFirstUiFindMatch'
+        )
+          return true;
+      }
+      return false;
+    }
+    return true;
+  }
+
   componentWillUpdate(nextProps: VirtualizedTraceViewProps) {
     const { childrenHiddenIDs, detailStates, registerAccessors, trace, currentViewRangeTime } = this.props;
     const {
@@ -191,19 +212,15 @@ export class VirtualizedTraceViewImpl extends React.PureComponent<VirtualizedTra
     }
   }
 
-  componentDidUpdate(prevProps: VirtualizedTraceViewProps) {
+  componentDidUpdate() {
     const {
-      detailStates: currDetailStates,
-      childrenHiddenIDs: currChildrenHiddenIds,
+      shouldScrollToFirstUiFindMatch,
+      clearShouldScrollToFirstUiFindMatch,
       scrollToFirstVisibleSpan,
     } = this.props;
-    const { detailStates: prevDetailStates, childrenHiddenIDs: prevChildrenHiddenIds } = prevProps;
-
-    if (
-      (currChildrenHiddenIds.needToScroll && currChildrenHiddenIds !== prevChildrenHiddenIds) ||
-      (currDetailStates.needToScroll && currDetailStates !== prevDetailStates)
-    ) {
+    if (shouldScrollToFirstUiFindMatch) {
       scrollToFirstVisibleSpan();
+      clearShouldScrollToFirstUiFindMatch();
     }
   }
 
@@ -431,7 +448,7 @@ function mapStateToProps(state: ReduxState): TTraceTimeline & TExtractUiFindFrom
 
 /* istanbul ignore next */
 function mapDispatchToProps(dispatch: Dispatch<ReduxState>): TDispatchProps {
-  return bindActionCreators(actions, dispatch) as any;
+  return (bindActionCreators(actions, dispatch) as any) as TDispatchProps;
 }
 
 export default withRouter(

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/duck.test.js

@@ -111,15 +111,13 @@ describe('TraceTimelineViewer/duck', () => {
     });
 
     it('indicates the need to scroll iff there are uiFindMatches', () => {
-      expect(state.detailStates.needToScroll).toBe(true);
-      expect(state.childrenHiddenIDs.needToScroll).toBe(true);
+      expect(state.shouldScrollToFirstUiFindMatch).toBe(true);
 
       filterSpansSpy.mockReturnValue(new Set());
       const singleSpecStore = createStore(reducer, newInitialState());
       singleSpecStore.dispatch(action);
       const singleSpecState = singleSpecStore.getState();
-      expect(singleSpecState.detailStates.needToScroll).toBeUndefined();
-      expect(singleSpecState.childrenHiddenIDs.needToScroll).toBeUndefined();
+      expect(singleSpecState.shouldScrollToFirstUiFindMatch).toBe(false);
     });
 
     it('returns existing state if uiFind is falsy', () => {
@@ -138,6 +136,10 @@ describe('TraceTimelineViewer/duck', () => {
   });
 
   describe('setTrace', () => {
+    beforeEach(() => {
+      filterSpansSpy.mockClear();
+    });
+
     const setTraceAction = actions.setTrace(trace);
 
     it('retains all state when setting to the same traceID', () => {
@@ -175,22 +177,37 @@ describe('TraceTimelineViewer/duck', () => {
 
     it('calls calculateHiddenIdsAndDetailStates iff a truthy uiFind is provided', () => {
       store.dispatch(setTraceAction);
-      let state = store.getState();
-      expect(state.childrenHiddenIDs).toEqual(new Set());
+      expect(filterSpansSpy).not.toHaveBeenCalled();
 
-      store.dispatch(actions.setTrace(trace, null));
-      state = store.getState();
-      expect(state.childrenHiddenIDs).toEqual(new Set());
+      store.dispatch(actions.setTrace(Object.assign({}, trace, { traceID: `${trace.traceID}_1` }), null));
+      expect(filterSpansSpy).not.toHaveBeenCalled();
 
-      store.dispatch(actions.setTrace(trace, undefined));
-      state = store.getState();
-      expect(state.childrenHiddenIDs).toEqual(new Set());
+      store.dispatch(actions.setTrace(Object.assign({}, trace, { traceID: `${trace.traceID}_2` }), ''));
+      expect(filterSpansSpy).not.toHaveBeenCalled();
 
-      store.dispatch(actions.setTrace(trace, 'truthy uiFind string'));
-      state = store.getState();
-      expect(state.childrenHiddenIDs).toEqual(new Set());
+      store.dispatch(
+        actions.setTrace(Object.assign({}, trace, { traceID: `${trace.traceID}_3` }), 'truthy uiFind string')
+      );
+      expect(filterSpansSpy).toHaveBeenCalledTimes(1);
+    });
+  });
+
+  describe('clearShouldScrollToFirstUiFindMatch', () => {
+    const clearShouldScrollToFirstUiFindMatchAction = actions.clearShouldScrollToFirstUiFindMatch();
+
+    it('returns existing state if current state does not indicate need to scroll', () => {
+      const state = store.getState();
+      store.dispatch(clearShouldScrollToFirstUiFindMatchAction);
+      expect(store.getState()).toBe(state);
+    });
+
+    it('sets state.shouldScrollToFirstUiFindMatch to false if it is currently true', () => {
+      const state = store.getState();
+      state.shouldScrollToFirstUiFindMatch = true;
+      expect(store.getState().shouldScrollToFirstUiFindMatch).toBe(true);
+      store.dispatch(clearShouldScrollToFirstUiFindMatchAction);
+      expect(store.getState().shouldScrollToFirstUiFindMatch).toBe(false);
     });
-    // describe('calls focus sometmisth', () => {
   });
 
   describe('toggles children and details', () => {

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/duck.tsx

@@ -51,6 +51,7 @@ export function newInitialState(): TTraceTimeline {
     childrenHiddenIDs: new Set(),
     detailStates: new Map(),
     hoverIndentGuideIds: new Set(),
+    shouldScrollToFirstUiFindMatch: false,
     spanNameColumnWidth: 0.25,
     traceID: null,
   };
@@ -59,6 +60,7 @@ export function newInitialState(): TTraceTimeline {
 export const actionTypes = generateActionTypes('@jaeger-ui/trace-timeline-viewer', [
   'ADD_HOVER_INDENT_GUIDE_ID',
   'CHILDREN_TOGGLE',
+  'CLEAR_SHOULD_SCROLL_TO_FIRST_UI_FIND_MATCH',
   'COLLAPSE_ALL',
   'COLLAPSE_ONE',
   'DETAIL_TOGGLE',
@@ -77,6 +79,7 @@ export const actionTypes = generateActionTypes('@jaeger-ui/trace-timeline-viewer
 const fullActions = createActions<TActionTypes>({
   [actionTypes.ADD_HOVER_INDENT_GUIDE_ID]: (spanID: string) => ({ spanID }),
   [actionTypes.CHILDREN_TOGGLE]: (spanID: string) => ({ spanID }),
+  [actionTypes.CLEAR_SHOULD_SCROLL_TO_FIRST_UI_FIND_MATCH]: () => ({}),
   [actionTypes.COLLAPSE_ALL]: (spans: Span[]) => ({ spans }),
   [actionTypes.COLLAPSE_ONE]: (spans: Span[]) => ({ spans }),
   [actionTypes.DETAIL_LOG_ITEM_TOGGLE]: (spanID: string, logItem: Log) => ({ logItem, spanID }),
@@ -96,8 +99,9 @@ export const actions = (fullActions as any).jaegerUi.traceTimelineViewer as TTim
 
 function calculateHiddenIdsAndDetailStates(uiFind: string, spans: Span[]) {
   const spansMap = new Map();
-  const childrenHiddenIDs: Set<string> & { needToScroll?: true } = new Set();
-  const detailStates: Map<string, DetailState> & { needToScroll?: true } = new Map();
+  const childrenHiddenIDs: Set<string> = new Set();
+  const detailStates: Map<string, DetailState> = new Map();
+  let shouldScrollToFirstUiFindMatch: boolean = false;
 
   spans.forEach(span => {
     spansMap.set(span.spanID, span);
@@ -110,12 +114,12 @@ function calculateHiddenIdsAndDetailStates(uiFind: string, spans: Span[]) {
       detailStates.set(spanID, new DetailState());
       spanAncestorIds(span).forEach(ancestorID => childrenHiddenIDs.delete(ancestorID));
     });
-    childrenHiddenIDs.needToScroll = true;
-    detailStates.needToScroll = true;
+    shouldScrollToFirstUiFindMatch = true;
   }
   return {
     childrenHiddenIDs,
     detailStates,
+    shouldScrollToFirstUiFindMatch,
   };
 }
 
@@ -127,6 +131,13 @@ function focusUiFindMatches(state: TTraceTimeline, { uiFind, trace }: TTraceUiFi
   };
 }
 
+function clearShouldScrollToFirstUiFindMatch(state: TTraceTimeline) {
+  if (state.shouldScrollToFirstUiFindMatch) {
+    return { ...state, shouldScrollToFirstUiFindMatch: false };
+  }
+  return state;
+}
+
 function setTrace(state: TTraceTimeline, { uiFind, trace }: TTraceUiFindValue) {
   const { traceID, spans } = trace;
   if (traceID === state.traceID) {
@@ -280,6 +291,9 @@ export default handleActions(
   {
     [actionTypes.ADD_HOVER_INDENT_GUIDE_ID]: guardReducer(addHoverIndentGuideId),
     [actionTypes.CHILDREN_TOGGLE]: guardReducer(childrenToggle),
+    [actionTypes.CLEAR_SHOULD_SCROLL_TO_FIRST_UI_FIND_MATCH]: guardReducer(
+      clearShouldScrollToFirstUiFindMatch
+    ),
     [actionTypes.COLLAPSE_ALL]: guardReducer(collapseAll),
     [actionTypes.COLLAPSE_ONE]: guardReducer(collapseOne),
     [actionTypes.DETAIL_LOGS_TOGGLE]: guardReducer(detailLogsToggle),

packages/jaeger-ui/src/types/TTraceTimeline.tsx

@@ -16,9 +16,10 @@ import DetailState from '../components/TracePage/TraceTimelineViewer/SpanDetail/
 import TNil from './TNil';
 
 type TTraceTimeline = {
-  childrenHiddenIDs: Set<string> & { needToScroll?: true };
-  detailStates: Map<string, DetailState> & { needToScroll?: true };
+  childrenHiddenIDs: Set<string>;
+  detailStates: Map<string, DetailState>;
   hoverIndentGuideIds: Set<string>;
+  shouldScrollToFirstUiFindMatch: boolean;
   spanNameColumnWidth: number;
   traceID: string | TNil;
 };