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.

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

feat: improve user experience for nested NodeExecutions #77

Merged
merged 4 commits into from
Jun 30, 2020
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
9 changes: 9 additions & 0 deletions src/common/formatters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,3 +144,12 @@ export function ensureUrlWithProtocol(url: string): string {
}
return url;
}

/** Formats a number into a string with leading zeros to ensure a consistent
* width.
* Example: 1 will be '01'
* 10 will be '10'
*/
export function leftPaddedNumber(value: number, length: number): string {
return value.toString().padStart(length, '0');
}
24 changes: 22 additions & 2 deletions src/common/test/formatters.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { millisecondsToDuration } from 'common/utils';
import { Admin } from 'flyteidl';
import {
subSecondString,
unknownValueString,
Expand All @@ -10,10 +9,10 @@ import {
dateFromNow,
dateWithFromNow,
ensureUrlWithProtocol,
fixedRateToString,
formatDate,
formatDateLocalTimezone,
formatDateUTC,
leftPaddedNumber,
millisecondsToHMS,
protobufDurationToHMS
} from '../formatters';
Expand Down Expand Up @@ -177,3 +176,24 @@ describe('ensureUrlWithProtocol', () => {
})
);
});

describe('leftPaddedNumber', () => {
const cases: [number, number, string][] = [
[1, 0, '1'],
[10, 0, '10'],
[0, 1, '0'],
[0, 2, '00'],
[1, 1, '1'],
[1, 2, '01'],
[1, 3, '001'],
[10, 1, '10'],
[10, 2, '10'],
[10, 3, '010']
];

cases.forEach(([value, width, expected]) =>
it(`should produce ${expected} with input (${value}, ${width})`, () => {
expect(leftPaddedNumber(value, width)).toEqual(expected);
})
);
});
8 changes: 4 additions & 4 deletions src/components/Cache/createCache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ function hasId(value: Object): value is HasIdObject {
* strictly equal (===). The cache provides methods for getting/setting values
* by key and for merging values or arrays of values with any existing items.
*/
export interface ValueCache {
export interface ValueCache<ValueType = object> {
/** Retrieve an item by key */
get(key: EntityKey): object | undefined;
get(key: EntityKey): ValueType | undefined;
/** Check existence of an item by key */
has(key: EntityKey): boolean;
/** Merge an array of values. If the items have an `id` property, its value
Expand All @@ -30,9 +30,9 @@ export interface ValueCache {
* performed. For arrays, the value is _replaced_.
* @returns The merged value
*/
mergeValue(key: EntityKey, value: object): object;
mergeValue(key: EntityKey, value: ValueType): ValueType;
/** Set an item value by key. Replaces any existing value. */
set(key: EntityKey, value: object): object;
set(key: EntityKey, value: ValueType): ValueType;
}

type Cacheable = object | any[];
Expand Down
3 changes: 2 additions & 1 deletion src/components/Cache/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ import * as objectHash from 'object-hash';
export function getCacheKey(id: any[] | object | string) {
return typeof id === 'string' || typeof id === 'symbol'
? id
: objectHash(id);
: // We only want to compare own properties, not .__proto__, .constructor, etc.
objectHash(id, { respectType: false });
}
26 changes: 16 additions & 10 deletions src/components/Executions/ExecutionDetails/ExecutionDetails.tsx
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
import { WaitForData, withRouteParams } from 'components/common';
import {
RefreshConfig,
useDataRefresher,
useWorkflowExecution
} from 'components/hooks';
import { RefreshConfig, useDataRefresher } from 'components/hooks';
import { Execution } from 'models';
import * as React from 'react';
import { executionRefreshIntervalMs } from '../constants';
import { ExecutionContext, ExecutionDataCacheContext } from '../contexts';
import { useExecutionDataCache } from '../useExecutionDataCache';
import { useWorkflowExecution } from '../useWorkflowExecution';
import { executionIsTerminal } from '../utils';
import { ExecutionContext } from './contexts';
import { ExecutionDetailsAppBarContent } from './ExecutionDetailsAppBarContent';
import { ExecutionNodeViews } from './ExecutionNodeViews';

Expand All @@ -35,15 +33,23 @@ export const ExecutionDetailsContainer: React.FC<ExecutionDetailsRouteParams> =
domain: domainId,
name: executionId
};

const { fetchable, terminateExecution } = useWorkflowExecution(id);
const dataCache = useExecutionDataCache();
const { fetchable, terminateExecution } = useWorkflowExecution(
id,
dataCache
);
useDataRefresher(id, fetchable, executionRefreshConfig);
const contextValue = { terminateExecution, execution: fetchable.value };
const contextValue = {
terminateExecution,
execution: fetchable.value
};
return (
<WaitForData {...fetchable}>
<ExecutionContext.Provider value={contextValue}>
<ExecutionDetailsAppBarContent execution={fetchable.value} />
<ExecutionNodeViews execution={fetchable.value} />
<ExecutionDataCacheContext.Provider value={dataCache}>
<ExecutionNodeViews execution={fetchable.value} />
</ExecutionDataCacheContext.Provider>
</ExecutionContext.Provider>
</WaitForData>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { NodeDetailsProps } from 'components/WorkflowGraph';
import { useStyles as useBaseStyles } from 'components/WorkflowGraph/NodeDetails/styles';

import { LiteralMapViewer } from 'components/Literals';
import { ExecutionContext } from '../contexts';
import { ExecutionContext } from '../../contexts';

/** Details panel renderer for the start/input node in a graph. Displays the
* top level `WorkflowExecution` inputs.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { SectionHeader, WaitForData } from 'components/common';
import { useCommonStyles } from 'components/common/styles';
import { useWorkflowExecutionData } from 'components/hooks';
import { LiteralMapViewer, RemoteLiteralMapViewer } from 'components/Literals';
import { NodeDetailsProps } from 'components/WorkflowGraph';
import { useStyles as useBaseStyles } from 'components/WorkflowGraph/NodeDetails/styles';
import { emptyLiteralMapBlob, Execution } from 'models';
import * as React from 'react';
import { ExecutionContext } from '../contexts';
import { ExecutionContext } from '../../contexts';
import { useWorkflowExecutionData } from '../../useWorkflowExecution';

const RemoteExecutionOutputs: React.FC<{ execution: Execution }> = ({
execution
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
TaskNodeDetails
} from 'components/WorkflowGraph';
import { useStyles as useBaseStyles } from 'components/WorkflowGraph/NodeDetails/styles';
import { NodeExecutionsContext } from '../contexts';
import { NodeExecutionsContext } from '../../contexts';
import { NodeExecutionData } from '../NodeExecutionData';
import { NodeExecutionInputs } from '../NodeExecutionInputs';
import { NodeExecutionOutputs } from '../NodeExecutionOutputs';
Expand Down
22 changes: 14 additions & 8 deletions src/components/Executions/ExecutionDetails/ExecutionNodeViews.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { WaitForData } from 'components/common';
import { useTabState } from 'components/hooks/useTabState';
import { Execution } from 'models';
import * as React from 'react';
import { NodeExecutionsRequestConfigContext } from '../contexts';
import { ExecutionFilters } from '../ExecutionFilters';
import { useNodeExecutionFiltersState } from '../filters/useExecutionFiltersState';
import { NodeExecutionsTable } from '../Tables/NodeExecutionsTable';
Expand Down Expand Up @@ -43,10 +44,11 @@ export const ExecutionNodeViews: React.FC<ExecutionNodeViewsProps> = ({
const filterState = useNodeExecutionFiltersState();
const tabState = useTabState(tabIds, tabIds.nodes);

const { workflow, nodeExecutions } = useWorkflowExecutionState(
execution,
filterState.appliedFilters
);
const {
workflow,
nodeExecutions,
nodeExecutionsRequestConfig
} = useWorkflowExecutionState(execution, filterState.appliedFilters);

return (
<WaitForData {...workflow}>
Expand All @@ -61,10 +63,14 @@ export const ExecutionNodeViews: React.FC<ExecutionNodeViewsProps> = ({
<ExecutionFilters {...filterState} />
</div>
<WaitForData {...nodeExecutions}>
<NodeExecutionsTable
{...nodeExecutions}
moreItemsAvailable={false}
/>
<NodeExecutionsRequestConfigContext.Provider
value={nodeExecutionsRequestConfig}
>
<NodeExecutionsTable
{...nodeExecutions}
moreItemsAvailable={false}
/>
</NodeExecutionsRequestConfigContext.Provider>
</WaitForData>
</>
)}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
import { DetailsPanel } from 'components/common';
import { WorkflowGraph } from 'components/WorkflowGraph';
import { keyBy } from 'lodash';
import { endNodeId, startNodeId } from 'models';
import { endNodeId, NodeExecution, startNodeId } from 'models';
import { Workflow } from 'models/Workflow';
import * as React from 'react';
import { DetailedNodeExecution } from '../types';
import { NodeExecutionsContext } from './contexts';
import { NodeExecutionsContext } from '../contexts';
import { useDetailedNodeExecutions } from '../useDetailedNodeExecutions';
import { NodeExecutionDetails } from './NodeExecutionDetails';
import { TaskExecutionNodeRenderer } from './TaskExecutionNodeRenderer/TaskExecutionNodeRenderer';

export interface ExecutionWorkflowGraphProps {
nodeExecutions: DetailedNodeExecution[];
nodeExecutions: NodeExecution[];
workflow: Workflow;
}

Expand All @@ -19,9 +19,10 @@ export const ExecutionWorkflowGraph: React.FC<ExecutionWorkflowGraphProps> = ({
nodeExecutions,
workflow
}) => {
const detailedNodeExecutions = useDetailedNodeExecutions(nodeExecutions);
const nodeExecutionsById = React.useMemo(
() => keyBy(nodeExecutions, 'id.nodeId'),
[nodeExecutions]
() => keyBy(detailedNodeExecutions, 'id.nodeId'),
[detailedNodeExecutions]
);
const [selectedNodes, setSelectedNodes] = React.useState<string[]>([]);
const onNodeSelectionChanged = (newSelection: string[]) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ const useStyles = makeStyles((theme: Theme) => {
content: {
overflowY: 'auto'
},
displayId: {
marginBottom: theme.spacing(1)
},
header: {
borderBottom: `${theme.spacing(1)}px solid ${theme.palette.divider}`
},
Expand All @@ -60,8 +63,7 @@ const useStyles = makeStyles((theme: Theme) => {
title: {
alignItems: 'flex-start',
display: 'flex',
justifyContent: 'space-between',
marginBottom: theme.spacing(1)
justifyContent: 'space-between'
}
};
});
Expand Down Expand Up @@ -189,6 +191,16 @@ export const NodeExecutionDetails: React.FC<NodeExecutionDetailsProps> = ({
<Close />
</IconButton>
</Typography>
<Typography
className={classnames(
commonStyles.textWrapped,
styles.displayId
)}
variant="subtitle1"
color="textSecondary"
>
{execution.displayId}
</Typography>
{statusContent}
<ExecutionTypeDetails execution={execution} />
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { TaskNodeRenderer } from 'components/WorkflowGraph/TaskNodeRenderer';
import { NodeExecutionPhase } from 'models/Execution/enums';
import { DAGNode } from 'models/Graph';

import { NodeExecutionsContext } from '../contexts';
import { NodeExecutionsContext } from '../../contexts';
import { StatusIndicator } from './StatusIndicator';

/** Renders DAGNodes with colors based on their node type, as well as dots to
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { render, waitFor } from '@testing-library/react';
import { mockAPIContextValue } from 'components/data/__mocks__/apiContext';
import { APIContext } from 'components/data/apiContext';
import {
DetailedNodeExecution,
NodeExecutionDisplayType
} from 'components/Executions/types';
import { createMockNodeExecutions } from 'models/Execution/__mocks__/mockNodeExecutionsData';
import { listTaskExecutions } from 'models/Execution/api';
import * as React from 'react';
import { NodeExecutionDetails } from '../NodeExecutionDetails';

describe('NodeExecutionDetails', () => {
let execution: DetailedNodeExecution;
let mockListTaskExecutions: jest.Mock<ReturnType<
typeof listTaskExecutions
>>;
beforeEach(() => {
const { executions } = createMockNodeExecutions(1);
execution = {
...executions[0],
displayType: NodeExecutionDisplayType.PythonTask,
displayId: 'com.flyte.testTask',
cacheKey: 'abcdefg'
};
mockListTaskExecutions = jest.fn().mockResolvedValue({ entities: [] });
});

const renderComponent = () =>
render(
<APIContext.Provider
value={mockAPIContextValue({
listTaskExecutions: mockListTaskExecutions
})}
>
<NodeExecutionDetails execution={execution} />
</APIContext.Provider>
);

it('renders displayId', async () => {
const { queryByText } = renderComponent();
await waitFor(() => {});
expect(queryByText(execution.displayId)).toBeInTheDocument();
});
});
2 changes: 1 addition & 1 deletion src/components/Executions/ExecutionInputsOutputsModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ import { Dialog, DialogContent, Tab, Tabs } from '@material-ui/core';
import { makeStyles, Theme } from '@material-ui/core/styles';
import { WaitForData } from 'components/common';
import { ClosableDialogTitle } from 'components/common/ClosableDialogTitle';
import { useWorkflowExecutionData } from 'components/hooks';
import { LiteralMapViewer, RemoteLiteralMapViewer } from 'components/Literals';
import { emptyLiteralMapBlob, Execution } from 'models';
import * as React from 'react';
import { useWorkflowExecutionData } from './useWorkflowExecution';

const useStyles = makeStyles((theme: Theme) => ({
content: {
Expand Down
Loading