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

fix(#278): wrong nodeExecutions details are showed in node executions list #277

Merged
merged 4 commits into from
Feb 11, 2022
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
41 changes: 23 additions & 18 deletions src/components/Executions/ExecutionDetails/ExecutionNodeViews.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { DataError } from 'components/Errors/DataError';
import { useTabState } from 'components/hooks/useTabState';
import { secondaryBackgroundColor } from 'components/Theme/constants';
import { Execution, NodeExecution } from 'models/Execution/types';
import { NodeExecutionDetailsContextProvider } from '../contextProvider/NodeExecutionDetails';
import { NodeExecutionsRequestConfigContext } from '../contexts';
import { ExecutionFilters } from '../ExecutionFilters';
import { useNodeExecutionFiltersState } from '../filters/useExecutionFiltersState';
Expand Down Expand Up @@ -85,29 +86,33 @@ export const ExecutionNodeViews: React.FC<ExecutionNodeViewsProps> = ({
<Tab value={tabs.nodes.id} label={tabs.nodes.label} />
<Tab value={tabs.graph.id} label={tabs.graph.label} />
</Tabs>
<div className={styles.nodesContainer}>
{tabState.value === tabs.nodes.id && (
<>
<div className={styles.filters}>
<ExecutionFilters {...filterState} />
</div>
<NodeExecutionDetailsContextProvider
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only change here: wrapped Tabs content with NodeExecutionDetailsContextProvider. github compare just gone rogue :)

workflowId={execution.closure.workflowId}
>
<div className={styles.nodesContainer}>
{tabState.value === tabs.nodes.id && (
<>
<div className={styles.filters}>
<ExecutionFilters {...filterState} />
</div>
<WaitForQuery
errorComponent={DataError}
query={nodeExecutionsQuery}
>
{renderNodeExecutionsTable}
</WaitForQuery>
</>
)}
{tabState.value === tabs.graph.id && (
<WaitForQuery
errorComponent={DataError}
query={nodeExecutionsQuery}
>
{renderNodeExecutionsTable}
{renderExecutionLoader}
</WaitForQuery>
</>
)}
{tabState.value === tabs.graph.id && (
<WaitForQuery
errorComponent={DataError}
query={nodeExecutionsQuery}
>
{renderExecutionLoader}
</WaitForQuery>
)}
</div>
)}
</div>
</NodeExecutionDetailsContextProvider>
</>
);
};
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useEffect, useState } from 'react';
import { useEffect, useRef, useState } from 'react';
import { IconButton, Typography } from '@material-ui/core';
import { makeStyles, Theme } from '@material-ui/core/styles';
import Tab from '@material-ui/core/Tab';
Expand Down Expand Up @@ -32,7 +32,7 @@ import {
} from '../nodeExecutionQueries';
import { TaskExecutionsList } from '../TaskExecutionsList/TaskExecutionsList';
import { NodeExecutionDetails } from '../types';
import { useNodeExecutionDetails } from '../useNodeExecutionDetails';
import { useNodeExecutionContext } from '../contextProvider/NodeExecutionDetails';
import { NodeExecutionInputs } from './NodeExecutionInputs';
import { NodeExecutionOutputs } from './NodeExecutionOutputs';
import { NodeExecutionTaskDetails } from './NodeExecutionTaskDetails';
Expand Down Expand Up @@ -210,6 +210,14 @@ const NodeExecutionTabs: React.FC<{
const styles = useStyles();
const tabState = useTabState(tabIds, defaultTab);

if (tabState.value === tabIds.task && !taskTemplate) {
// Reset tab value, if task tab is selected, while no taskTemplate is avaible
// can happen when user switches between nodeExecutions without closing the drawer
tabState.onChange(() => {
/* */
}, defaultTab);
}

let tabContent: JSX.Element | null = null;
switch (tabState.value) {
case tabIds.executions: {
Expand Down Expand Up @@ -293,22 +301,43 @@ export const NodeExecutionDetailsPanelContent: React.FC<NodeExecutionDetailsProp
nodeExecutionId,
onClose
}) => {
const [mounted, setMounted] = useState(true);
const isMounted = useRef(false);
useEffect(() => {
isMounted.current = true;
return () => {
setMounted(false);
isMounted.current = false;
};
}, []);

const queryClient = useQueryClient();
const detailsContext = useNodeExecutionContext();

const [isReasonsVisible, setReasonsVisible] = React.useState(false);
const [dag, setDag] = React.useState<any>(null);
const [details, setDetails] = React.useState<
NodeExecutionDetails | undefined
>();

const nodeExecutionQuery = useQuery<NodeExecution, Error>({
...makeNodeExecutionQuery(nodeExecutionId),
// The selected NodeExecution has been fetched at this point, we don't want to
// issue an additional fetch.
staleTime: Infinity
});

React.useEffect(() => {
let isCurrent = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure I understand this? Won't this always be true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would change to false, whenever return() is called (in this case on component unmount)

detailsContext.getNodeExecutionDetails(nodeExecution).then(res => {
if (isCurrent) {
setDetails(res);
}
});

return () => {
isCurrent = false;
};
});

React.useEffect(() => {
setReasonsVisible(false);
}, [nodeExecutionId]);
Expand All @@ -326,7 +355,7 @@ export const NodeExecutionDetailsPanelContent: React.FC<NodeExecutionDetailsProp
);
if (workflowData) {
const keyedDag = transformWorkflowToKeyedDag(workflowData);
if (mounted) setDag(keyedDag);
if (isMounted.current) setDag(keyedDag);
}
};

Expand All @@ -347,15 +376,7 @@ export const NodeExecutionDetailsPanelContent: React.FC<NodeExecutionDetailsProp

const commonStyles = useCommonStyles();
const styles = useStyles();
const detailsQuery = useNodeExecutionDetails(nodeExecution);
const displayName = detailsQuery.data ? (
detailsQuery.data.displayName
) : (
<Skeleton />
);
const taskTemplate = detailsQuery.data
? detailsQuery.data.taskTemplate
: null;
const displayName = details?.displayName ?? <Skeleton />;

const isRunningPhase = React.useMemo(() => {
return (
Expand Down Expand Up @@ -400,17 +421,14 @@ export const NodeExecutionDetailsPanelContent: React.FC<NodeExecutionDetailsProp
<NodeExecutionCacheStatus
taskNodeMetadata={nodeExecution.closure.taskNodeMetadata}
/>
<ExecutionTypeDetails
details={detailsQuery.data}
execution={nodeExecution}
/>
<ExecutionTypeDetails details={details} execution={nodeExecution} />
</>
) : null;

const tabsContent = nodeExecution ? (
<NodeExecutionTabs
nodeExecution={nodeExecution}
taskTemplate={taskTemplate}
taskTemplate={details?.taskTemplate}
/>
) : null;
return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ import {
cacheStatusMessages,
viewSourceExecutionString
} from 'components/Executions/constants';
import { NodeExecutionDetailsContextProvider } from 'components/Executions/contextProvider/NodeExecutionDetails';
import { Core } from 'flyteidl';
import { basicPythonWorkflow } from 'mocks/data/fixtures/basicPythonWorkflow';
import { mockWorkflowId } from 'mocks/data/fixtures/types';
import { insertFixture } from 'mocks/data/insertFixture';
import { mockServer } from 'mocks/server';
import { NodeExecution, TaskNodeMetadata } from 'models/Execution/types';
Expand All @@ -17,6 +19,9 @@ import { makeIdentifier } from 'test/modelUtils';
import { createTestQueryClient } from 'test/utils';
import { NodeExecutionDetailsPanelContent } from '../NodeExecutionDetailsPanelContent';

jest.mock('components/Workflow/workflowQueries');
const { fetchWorkflow } = require('components/Workflow/workflowQueries');

describe('NodeExecutionDetails', () => {
let fixture: ReturnType<typeof basicPythonWorkflow.generate>;
let execution: NodeExecution;
Expand All @@ -27,16 +32,23 @@ describe('NodeExecutionDetails', () => {
execution =
fixture.workflowExecutions.top.nodeExecutions.pythonNode.data;
insertFixture(mockServer, fixture);
fetchWorkflow.mockImplementation(() =>
Promise.resolve(fixture.workflows.top)
);
queryClient = createTestQueryClient();
});

const renderComponent = () =>
render(
<MemoryRouter>
<QueryClientProvider client={queryClient}>
<NodeExecutionDetailsPanelContent
nodeExecutionId={execution.id}
/>
<NodeExecutionDetailsContextProvider
workflowId={mockWorkflowId}
>
<NodeExecutionDetailsPanelContent
nodeExecutionId={execution.id}
/>
</NodeExecutionDetailsContextProvider>
</QueryClientProvider>
</MemoryRouter>
);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { makeStyles, Theme } from '@material-ui/core/styles';
import { storiesOf } from '@storybook/react';
import { NodeExecutionDetailsContext } from 'components/Executions/contextProvider/NodeExecutionDetails';
import { makeNodeExecutionListQuery } from 'components/Executions/nodeExecutionQueries';
import { basicPythonWorkflow } from 'mocks/data/fixtures/basicPythonWorkflow';
import * as React from 'react';
Expand All @@ -19,6 +20,14 @@ const useStyles = makeStyles((theme: Theme) => ({
const fixture = basicPythonWorkflow.generate();
const workflowExecution = fixture.workflowExecutions.top.data;

const getNodeExecutionDetails = async () => {
return {
displayId: 'node0',
displayName: 'basic.byton.workflow.unique.task_name',
displayType: 'Python-Task'
};
};

const stories = storiesOf('Tables/NodeExecutionsTable', module);
stories.addDecorator(story => {
return <div className={useStyles().container}>{story()}</div>;
Expand All @@ -28,9 +37,21 @@ stories.add('Basic', () => {
makeNodeExecutionListQuery(useQueryClient(), workflowExecution.id)
);
return query.data ? (
<NodeExecutionsTable nodeExecutions={query.data} />
<NodeExecutionDetailsContext.Provider
value={{ getNodeExecutionDetails }}
>
<NodeExecutionsTable nodeExecutions={query.data} />
</NodeExecutionDetailsContext.Provider>
) : (
<div />
);
});
stories.add('With no items', () => <NodeExecutionsTable nodeExecutions={[]} />);
stories.add('With no items', () => {
return (
<NodeExecutionDetailsContext.Provider
value={{ getNodeExecutionDetails }}
>
<NodeExecutionsTable nodeExecutions={[]} />
</NodeExecutionDetailsContext.Provider>
);
});
2 changes: 1 addition & 1 deletion src/components/Executions/Tables/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export const workflowExecutionsTableColumnWidths = {

export const nodeExecutionsTableColumnWidths = {
duration: 100,
logs: 225,
logs: 100,
type: 144,
nodeId: 144,
name: 380,
Expand Down
Loading