Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add uiFind icons, scroll to first match #367

Merged
merged 14 commits into from
May 3, 2019
Merged

add uiFind icons, scroll to first match #367

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a0e296a
WIP - add uiFind icons, scroll to first match
everett980 Apr 2, 2019
ebf9473
Use componentDidUpdate instead of setTimeout
everett980 Apr 11, 2019
37f9a20
Clarify focusUiFind function name, fix existing
everett980 Apr 11, 2019
bb66f09
Add scrollToFirstVisibleSpan tests
everett980 Apr 11, 2019
79f04af
Add scrollToFirstVisibleSpan tests
everett980 Apr 11, 2019
79eb6df
Bolster test coverage for files in PR
everett980 Apr 12, 2019
8a268c2
Improve TracePageSearchBar styling, choose icons
everett980 Apr 15, 2019
5a146ef
Move scroll indicator from set/map to standalone
everett980 Apr 23, 2019
43a8ca5
Update CopyIcon to support spanID deep link
everett980 Apr 24, 2019
5801d3e
Fix CopyIcon css, Swap in copy-to-clipboard
everett980 Apr 25, 2019
73e8691
Improve VirtualizedTraceView shouldComponentUpdate
everett980 Apr 25, 2019
7651dbc
Merge branch 'master' into issue-361-and-360-improve-uiFind-interactions
tiffon Apr 26, 2019
9b9f5c0
Remove unnecessary function calls, improve names
everett980 May 2, 2019
cdbdd9d
Remove nonfunctional focusUiFindMatches call
everett980 May 3, 2019
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
15 changes: 12 additions & 3 deletions packages/jaeger-ui/src/components/TracePage/ScrollManager.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import _get from 'lodash/get';
import _findIndex from 'lodash/findIndex';
import _isUndefined from 'lodash/isUndefined';

import { TNil } from '../../types';
import { Span, SpanReference, Trace } from '../../types/trace';

Expand Down Expand Up @@ -120,7 +124,7 @@ export default class ScrollManager {
this._scroller.scrollTo(y);
}

_scrollToVisibleSpan(direction: 1 | -1) {
_scrollToVisibleSpan(direction: 1 | -1, startRow?: number) {
const xrs = this._accessors;
/* istanbul ignore next */
if (!xrs) {
Expand All @@ -131,7 +135,8 @@ export default class ScrollManager {
}
const { duration, spans, startTime: traceStartTime } = this._trace;
const isUp = direction < 0;
const boundaryRow = isUp ? xrs.getTopRowIndexVisible() : xrs.getBottomRowIndexVisible();
const boundaryRow = _isUndefined(startRow) ? (isUp ? xrs.getTopRowIndexVisible() : xrs.getBottomRowIndexVisible()): startRow;
everett980 marked this conversation as resolved.
Show resolved Hide resolved
console.log(boundaryRow);
const spanIndex = xrs.mapRowIndexToSpanIndex(boundaryRow);
if ((spanIndex === 0 && isUp) || (spanIndex === spans.length - 1 && !isUp)) {
return;
Expand Down Expand Up @@ -184,7 +189,7 @@ export default class ScrollManager {

// If there are hidden children, scroll to the last visible span
if (childrenAreHidden) {
let isFallbackHidden: boolean | TNil;
let isFallbackHidden: boolean;
tiffon marked this conversation as resolved.
Show resolved Hide resolved
do {
const { isHidden, parentIDs } = isSpanHidden(spans[nextSpanIndex], childrenAreHidden, spansMap);
if (isHidden) {
Expand Down Expand Up @@ -255,6 +260,10 @@ export default class ScrollManager {
this._scrollToVisibleSpan(-1);
};

scrollToFirstVisibleSpan = () => {
this._scrollToVisibleSpan(1, 0);
tiffon marked this conversation as resolved.
Show resolved Hide resolved
};

destroy() {
this._trace = undefined;
this._scroller = undefined as any;
Expand Down
3 changes: 3 additions & 0 deletions packages/jaeger-ui/src/components/TracePage/TracePageHeader/TracePageHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import './TracePageHeader.css';
type TracePageHeaderEmbedProps = {
canCollapse: boolean;
clearSearch: () => void;
focusUiFind: () => void;
hideMap: boolean;
hideSummary: boolean;
linkToStandalone: string;
Expand Down Expand Up @@ -95,6 +96,7 @@ export function TracePageHeaderFn(props: TracePageHeaderEmbedProps & { forwarded
const {
canCollapse,
clearSearch,
focusUiFind,
forwardedRef,
hideMap,
hideSummary,
Expand Down Expand Up @@ -164,6 +166,7 @@ export function TracePageHeaderFn(props: TracePageHeaderEmbedProps & { forwarded
{showShortcutsHelp && <KeyboardShortcutsHelp className="ub-mr2" />}
<TracePageSearchBar
clearSearch={clearSearch}
focusUiFind={focusUiFind}
nextResult={nextResult}
prevResult={prevResult}
ref={forwardedRef}
Expand Down
10 changes: 9 additions & 1 deletion packages/jaeger-ui/src/components/TracePage/TracePageHeader/TracePageSearchBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,13 @@ type TracePageSearchBarProps = {
prevResult: () => void;
nextResult: () => void;
clearSearch: () => void;
focusUiFind: () => void;
resultCount: number;
navigable: boolean;
};

export function TracePageSearchBarFn(props: TracePageSearchBarProps & { forwardedRef: React.Ref<Input> }) {
const { clearSearch, forwardedRef, navigable, nextResult, prevResult, resultCount, textFilter } = props;
const { clearSearch, focusUiFind, forwardedRef, navigable, nextResult, prevResult, resultCount, textFilter } = props;

const count = textFilter ? <span className="TracePageSearchBar--count">{resultCount}</span> : null;

Expand Down Expand Up @@ -70,6 +71,13 @@ export function TracePageSearchBarFn(props: TracePageSearchBarProps & { forwarde
icon="down"
onClick={nextResult}
/>
<Button
className={navigationBtnClass}
disabled={navigationBtnDisabled}
htmlType="button"
icon="search"
onClick={focusUiFind}
/>
<Button
className={btnClass}
disabled={!textFilter}
Expand Down
12 changes: 5 additions & 7 deletions packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// limitations under the License.

import React from 'react';
import { Divider, Tooltip } from 'antd';
import { Button, Divider, Tooltip } from 'antd';

import AccordianKeyValues from './AccordianKeyValues';
import AccordianLogs from './AccordianLogs';
Expand Down Expand Up @@ -113,13 +113,11 @@ export default function SpanDetail(props: SpanDetailProps) {
timestamp={traceStartTime}
/>
)}

<small className="SpanDetail--debugInfo">
<Tooltip title="Click ID to add to filter">
<span className="SpanDetail--debugLabel" data-label="SpanID:" />{' '}
<button className="SpanDetail--debugValue" type="button" onClick={() => addToUiFind(spanID)}>
{spanID}
</button>
<span className="SpanDetail--debugLabel" data-label="SpanID:" />{' '}
{spanID}
<Tooltip placement="topRight" title="Click to add to filter for deep linking">
<Button className="SpanDetail--debugValue" htmlType="button" icon="down" onClick={() => addToUiFind(spanID)}/>
tiffon marked this conversation as resolved.
Show resolved Hide resolved
</Tooltip>
</small>
</div>
Expand Down
5 changes: 5 additions & 0 deletions packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ type RowState = {
type TVirtualizedTraceViewOwnProps = {
currentViewRangeTime: [number, number];
findMatchesIDs: Set<string> | TNil;
scrollToFirstVisibleSpan: () => void;
registerAccessors: (accesors: Accessors) => void;
trace: Trace;
};
Expand Down Expand Up @@ -158,6 +159,10 @@ export class VirtualizedTraceViewImpl extends React.PureComponent<VirtualizedTra
setTrace(trace, uiFind);
}

componentDidMount() {
setTimeout(this.props.scrollToFirstVisibleSpan);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Without the setTimeout it scrolls to the bottom. I think the rows may not have height in the initial render.
@tiffon do you have any insight on why this would scroll too far without the setTimeout

Copy link
Member

Choose a reason for hiding this comment

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

Looks like the constructor calls setTrace(...). When visiting the URL, directly, the setTrace(...) applies the find matching.

When coming from the search page the setTimeout has no effect; the find is already applied to the redux state via the reducer that changed the state to be viewing the trace.

So, seems like the rows are rendered but they're not collapsed, yet, when visiting a trace directly from a URL. Probably it's scrolling to the position of the row, but then the non-matching rows are collapsed and the target scroll position ends up getting truncated with the result of being scrolled to the bottom.

}

componentWillUpdate(nextProps: VirtualizedTraceViewProps) {
const { childrenHiddenIDs, detailStates, registerAccessors, trace, currentViewRangeTime } = this.props;
const {
Expand Down
66 changes: 41 additions & 25 deletions packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/duck.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,47 +68,38 @@ export const actionTypes = generateActionTypes('@jaeger-ui/trace-timeline-viewer
'DETAIL_LOG_ITEM_TOGGLE',
'EXPAND_ALL',
'EXPAND_ONE',
'FOCUS_UI_FIND',
'REMOVE_HOVER_INDENT_GUIDE_ID',
'SET_SPAN_NAME_COLUMN_WIDTH',
'SET_TRACE',
]);

const fullActions = createActions<TActionTypes>({
[actionTypes.SET_TRACE]: (trace: Trace, uiFind: string | TNil) => ({ trace, uiFind }),
[actionTypes.SET_SPAN_NAME_COLUMN_WIDTH]: (width: number) => ({ width }),
[actionTypes.ADD_HOVER_INDENT_GUIDE_ID]: (spanID: string) => ({ spanID }),
[actionTypes.CHILDREN_TOGGLE]: (spanID: string) => ({ spanID }),
[actionTypes.EXPAND_ALL]: () => ({}),
[actionTypes.EXPAND_ONE]: (spans: Span[]) => ({ spans }),
[actionTypes.COLLAPSE_ALL]: (spans: Span[]) => ({ spans }),
[actionTypes.COLLAPSE_ONE]: (spans: Span[]) => ({ spans }),
[actionTypes.DETAIL_TOGGLE]: (spanID: string) => ({ spanID }),
[actionTypes.DETAIL_TAGS_TOGGLE]: (spanID: string) => ({ spanID }),
[actionTypes.DETAIL_PROCESS_TOGGLE]: (spanID: string) => ({ spanID }),
[actionTypes.DETAIL_LOGS_TOGGLE]: (spanID: string) => ({ spanID }),
[actionTypes.DETAIL_LOG_ITEM_TOGGLE]: (spanID: string, logItem: Log) => ({ logItem, spanID }),
[actionTypes.ADD_HOVER_INDENT_GUIDE_ID]: (spanID: string) => ({ spanID }),
[actionTypes.DETAIL_LOGS_TOGGLE]: (spanID: string) => ({ spanID }),
[actionTypes.EXPAND_ALL]: () => ({}),
[actionTypes.EXPAND_ONE]: (spans: Span[]) => ({ spans }),
[actionTypes.DETAIL_PROCESS_TOGGLE]: (spanID: string) => ({ spanID }),
[actionTypes.DETAIL_TAGS_TOGGLE]: (spanID: string) => ({ spanID }),
[actionTypes.DETAIL_TOGGLE]: (spanID: string) => ({ spanID }),
[actionTypes.FOCUS_UI_FIND]: (trace: Trace, uiFind: string | TNil) => ({ trace, uiFind }),
[actionTypes.REMOVE_HOVER_INDENT_GUIDE_ID]: (spanID: string) => ({ spanID }),
[actionTypes.SET_SPAN_NAME_COLUMN_WIDTH]: (width: number) => ({ width }),
[actionTypes.SET_TRACE]: (trace: Trace, uiFind: string | TNil) => ({ trace, uiFind }),
});

export const actions = (fullActions as any).jaegerUi.traceTimelineViewer as TTimelineViewerActions;

function setTrace(state: TTraceTimeline, { uiFind, trace }: TTraceUiFindValue) {
const { traceID, spans } = trace;
if (traceID === state.traceID) {
return state;
}
const { spanNameColumnWidth } = state;

if (!uiFind) {
// No filter, so we're done
return { ...newInitialState(), spanNameColumnWidth, traceID };
}
// There is a filter, so collapse all rows except matches and their ancestors; show details for matches
function calculateHiddenIdsAndDetailStates(uiFind: string, spans: Span[]) {
everett980 marked this conversation as resolved.
Show resolved Hide resolved
const spansMap = new Map();
const childrenHiddenIDs = new Set();
const detailStates = new Map();

spans.forEach((span: Span) => {
spans.forEach(span => {
spansMap.set(span.spanID, span);
childrenHiddenIDs.add(span.spanID);
});
Expand All @@ -121,14 +112,38 @@ function setTrace(state: TTraceTimeline, { uiFind, trace }: TTraceUiFindValue) {
});
}
return {
...newInitialState(),
spanNameColumnWidth,
childrenHiddenIDs,
detailStates,
traceID,
};
}

function focusUiFind(state: TTraceTimeline, { uiFind, trace }: TTraceUiFindValue) {
if (!uiFind) return state;
return {
...state,
...calculateHiddenIdsAndDetailStates(uiFind, trace.spans),
};
}


function setTrace(state: TTraceTimeline, { uiFind, trace }: TTraceUiFindValue) {
const { traceID, spans } = trace;
if (traceID === state.traceID) {
return state;
}
const { spanNameColumnWidth } = state;
const newState = { ...newInitialState(), spanNameColumnWidth, traceID };
everett980 marked this conversation as resolved.
Show resolved Hide resolved

if (uiFind) {
Object.assign(
newState,
calculateHiddenIdsAndDetailStates(uiFind, spans),
);
}

return newState;
}

function setColumnWidth(state: TTraceTimeline, { width }: TWidthValue): TTraceTimeline {
return { ...state, spanNameColumnWidth: width };
}
Expand Down Expand Up @@ -278,6 +293,7 @@ export default handleActions(
[actionTypes.DETAIL_TOGGLE]: guardReducer(detailToggle),
[actionTypes.EXPAND_ALL]: guardReducer(expandAll),
[actionTypes.EXPAND_ONE]: guardReducer(expandOne),
[actionTypes.FOCUS_UI_FIND]: guardReducer(focusUiFind),
[actionTypes.REMOVE_HOVER_INDENT_GUIDE_ID]: guardReducer(removeHoverIndentGuideId),
[actionTypes.SET_SPAN_NAME_COLUMN_WIDTH]: guardReducer(setColumnWidth),
[actionTypes.SET_TRACE]: guardReducer(setTrace),
Expand Down
1 change: 1 addition & 0 deletions packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ type TDispatchProps = {
type TProps = TDispatchProps & {
registerAccessors: (accessors: Accessors) => void;
findMatchesIDs: Set<string> | TNil;
scrollToFirstVisibleSpan: () => void;
spanNameColumnWidth: number;
trace: Trace;
updateNextViewRangeTime: (update: ViewRangeTimeUpdate) => void;
Expand Down
19 changes: 14 additions & 5 deletions packages/jaeger-ui/src/components/TracePage/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import { TEv } from './TraceGraph/types';
import { trackSlimHeaderToggle } from './TracePageHeader/TracePageHeader.track';
import TracePageHeader from './TracePageHeader';
import TraceTimelineViewer from './TraceTimelineViewer';
import { actions as timelineActions } from './TraceTimelineViewer/duck';
import { TUpdateViewRangeTimeFunction, ViewRange, ViewRangeTimeUpdate } from './types';
import { getLocation, getUrl } from './url';
import ErrorMessage from '../common/ErrorMessage';
Expand All @@ -49,6 +50,7 @@ import * as jaegerApiActions from '../../actions/jaeger-api';
import { getUiFindVertexKeys } from '../TraceDiff/TraceDiffGraph/traceDiffGraphUtils';
import { fetchedState } from '../../constants';
import { FetchedTrace, ReduxState, TNil } from '../../types';
import { Trace } from '../../types/trace';
import { TraceArchive } from '../../types/archive';
import { EmbeddedState } from '../../types/embedded';
import filterSpans from '../../utils/filter-spans';
Expand All @@ -60,6 +62,7 @@ type TDispatchProps = {
acknowledgeArchive: (id: string) => void;
archiveTrace: (id: string) => void;
fetchTrace: (id: string) => void;
focusUiFind: (trace: Trace, uiFind: string | TNil) => void;
};

type TOwnProps = {
Expand Down Expand Up @@ -307,7 +310,7 @@ export class TracePageImpl extends React.PureComponent<TProps, TState> {
}

render() {
const { archiveEnabled, archiveTraceState, embedded, id, searchUrl, uiFind, trace } = this.props;
const { archiveEnabled, archiveTraceState, embedded, focusUiFind, id, searchUrl, uiFind, trace } = this.props;
const { slimView, traceGraphView, headerHeight, viewRange } = this.state;
if (!trace || trace.state === fetchedState.LOADING) {
return <LoadingIndicator className="u-mt-vast" centered />;
Expand All @@ -317,7 +320,6 @@ export class TracePageImpl extends React.PureComponent<TProps, TState> {
return <ErrorMessage className="ub-m3" error={trace.error || 'Unknown error'} />;
}

// $FlowIgnore because flow believes Set<string> cannot be assigned to Set<string | number>
let findCount = 0;
let graphFindMatches;
let spanFindMatches;
Expand All @@ -330,10 +332,15 @@ export class TracePageImpl extends React.PureComponent<TProps, TState> {
findCount = spanFindMatches ? spanFindMatches.size : 0;
}
}
// = ? : null;
// const spanFindMatches = !traceGraphView ? this._filterSpans(uiFind || '', _get(trace, 'data.spans')) : null;

const isEmbedded = Boolean(embedded);
const headerProps = {
focusUiFind: () => {
if (trace.data) {
focusUiFind(trace.data, uiFind);
setTimeout(this._scrollManager.scrollToFirstVisibleSpan);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The scroll destination changes as a result of dispatching focusUiFind. This setTimeout achieves the desired result but definitely seems like the wrong way to achieve this.
@tiffon how would you recommend scrolling to the first match only after the rows have been expanded/collapsed and had their children shown/hidden?

Copy link
Member

Choose a reason for hiding this comment

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

That's a tough one, maybe via componentDidUpdate() with an instance property indicating whether or not the component should scroll.

Also, I recommend turning this into a method instead of creating a new function with every render.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately an instance property wouldn't work because the component that updates as a result of focusUiFind is not TracePage/index.tsx. I got around this by adding a property to the detailStates Map and childrenHiddenIDs Set made by calculateHiddenIdsAndDetailStates in TracePage/duck.tsx and reading that property in componentDidUpdate in TraceTimelineViewer/VirtualizedTraceView.tsx
It's slightly unorthodox in principle, but not so bad in practice.
Here is the commit that solely addresses setTimeout
TODO: more tests and icons

}
},
slimView,
textFilter: uiFind,
traceGraphView,
Expand Down Expand Up @@ -382,6 +389,7 @@ export class TracePageImpl extends React.PureComponent<TProps, TState> {
<section style={{ paddingTop: headerHeight }}>
<TraceTimelineViewer
registerAccessors={this._scrollManager.setAccessors}
scrollToFirstVisibleSpan={this._scrollManager.scrollToFirstVisibleSpan}
findMatchesIDs={spanFindMatches}
trace={data}
updateNextViewRangeTime={this.updateNextViewRangeTime}
Expand Down Expand Up @@ -421,7 +429,8 @@ export function mapStateToProps(state: ReduxState, ownProps: TOwnProps): TReduxP
export function mapDispatchToProps(dispatch: Dispatch<ReduxState>): TDispatchProps {
const { fetchTrace } = bindActionCreators(jaegerApiActions, dispatch);
const { archiveTrace, acknowledge: acknowledgeArchive } = bindActionCreators(archiveActions, dispatch);
return { acknowledgeArchive, archiveTrace, fetchTrace };
const { focusUiFind } = bindActionCreators(timelineActions, dispatch);
return { acknowledgeArchive, archiveTrace, fetchTrace, focusUiFind };
}

export default connect(mapStateToProps, mapDispatchToProps)(TracePageImpl);