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

fix: stale selected item reference in NE details panel #115

Merged
merged 2 commits into from
Nov 6, 2020
Merged

fix: stale selected item reference in NE details panel #115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d22a70a
fix: stale selected item reference in NE details panel
schottra Nov 5, 2020
54b2918
refactor: syntax cleanup
schottra Nov 5, 2020
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
58 changes: 44 additions & 14 deletions src/components/Executions/Tables/test/NodeExecutionsTable.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import {
fireEvent,
getAllByRole,
getByText,
getByTitle,
render,
screen,
waitFor
} from '@testing-library/react';
import { mockAPIContextValue } from 'components/data/__mocks__/apiContext';
Expand Down Expand Up @@ -45,6 +47,7 @@ import {
listTaskExecutionChildren,
listTaskExecutions
} from 'models/Execution/api';
import { NodeExecutionPhase } from 'models/Execution/enums';
import { mockTasks } from 'models/Task/__mocks__/mockTaskData';
import * as React from 'react';
import { makeIdentifier } from 'test/modelUtils';
Expand Down Expand Up @@ -137,20 +140,19 @@ describe('NodeExecutionsTable', () => {
};
});

const renderTable = () =>
render(
<APIContext.Provider value={apiContext}>
<NodeExecutionsRequestConfigContext.Provider
value={requestConfig}
>
<ExecutionContext.Provider value={executionContext}>
<ExecutionDataCacheContext.Provider value={dataCache}>
<NodeExecutionsTable {...props} />
</ExecutionDataCacheContext.Provider>
</ExecutionContext.Provider>
</NodeExecutionsRequestConfigContext.Provider>
</APIContext.Provider>
);
const Table = (props: NodeExecutionsTableProps) => (
<APIContext.Provider value={apiContext}>
<NodeExecutionsRequestConfigContext.Provider value={requestConfig}>
<ExecutionContext.Provider value={executionContext}>
<ExecutionDataCacheContext.Provider value={dataCache}>
<NodeExecutionsTable {...props} />
</ExecutionDataCacheContext.Provider>
</ExecutionContext.Provider>
</NodeExecutionsRequestConfigContext.Provider>
</APIContext.Provider>
);

const renderTable = () => render(<Table {...props} />);

it('renders task name for task nodes', async () => {
const { queryAllByText } = renderTable();
Expand Down Expand Up @@ -413,4 +415,32 @@ describe('NodeExecutionsTable', () => {
})
);
});

describe('when rendering the DetailsPanel', () => {
const selectFirstNode = async (container: HTMLElement) => {
const { nodeId } = mockNodeExecutions[0].id;
const nodeNameAnchor = await waitFor(() =>
getByText(container, nodeId)
);
fireEvent.click(nodeNameAnchor);
// Wait for Details Panel to render and then for the nodeId header
const detailsPanel = await waitFor(() =>
screen.getByTestId('details-panel')
);
await waitFor(() => getByText(detailsPanel, nodeId));
return detailsPanel;
};
it('should render updated state if selected nodeExecution object changes', async () => {
mockNodeExecutions[0].closure.phase = NodeExecutionPhase.RUNNING;
// Render table, click first node
const { container, rerender } = renderTable();
const detailsPanel = await selectFirstNode(container);
await waitFor(() => getByText(detailsPanel, 'Running'));

props.value = cloneDeep(mockNodeExecutions);
props.value[0].closure.phase = NodeExecutionPhase.FAILED;
rerender(<Table {...props} />);
await waitFor(() => getByText(detailsPanel, 'Failed'));
});
});
});
21 changes: 17 additions & 4 deletions src/components/Executions/Tables/useNodeExecutionsTableState.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,23 @@ export function useNodeExecutionsTableState({
[value]
);

const [
selectedExecution,
setSelectedExecution
] = useState<DetailedNodeExecution | null>(null);
const [selectedExecutionKey, setSelectedExecutionKey] = useState<
string | null
>(null);

const selectedExecution = useMemo(
() =>
executions.find(
({ cacheKey }) => cacheKey === selectedExecutionKey
) || null,
Comment on lines +23 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

If .find doesn't find a match it will return undefined. Curious to know if there is a meaningful difference between returning undefined vs null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Short answer: No, not really. The consuming code should handle the undefined case in the same manner as the null case.

Longer answer: It's a matter of preference.

I prefer using null when explicitly setting a value to the "not defined" case and letting it be undefined when it truly was never defined or set. There are also cases where updating a value to be undefined is a little difficult or error-prone (like merging objects or attempting to delete properties). Setting to null feels cleaner in these cases.

To reflect that, the type of the returned value in the state is DetailedNodeExecution | null. It's initialized to null and set to null when the selection is cleared. So allowing it to be undefined here would cause a type error.

[executions, selectedExecutionKey]
);

const setSelectedExecution = useMemo(
() => (newValue: DetailedNodeExecution | null) =>
setSelectedExecutionKey(newValue?.cacheKey ?? null),
[setSelectedExecutionKey]
);

return {
executions,
Expand Down
1 change: 1 addition & 0 deletions src/components/common/DetailsPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export const DetailsPanel: React.FC<DetailsPanelProps> = ({
return (
<Drawer
anchor="right"
data-testid="details-panel"
ModalProps={{
className: styles.modal,
hideBackdrop: true,
Expand Down