From ababe1a20defd3baf387edd275df95b03cb6fb65 Mon Sep 17 00:00:00 2001 From: Marco polo Date: Thu, 13 Jun 2024 13:19:28 -1000 Subject: [PATCH] Fix duplicate code location error toasts + Add status filter to code locations page (#22540) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary & Motivation Recent changes to the WorkspaceContext where we split fetched by location caused us show the code location error toast multiple times. Once for each code location update after we've detected an errored code location. The fix is to instead fire the effect off of the errored code locations and preserve the reference if they haven't changed. Also adding a "state" filter to the code locations page for use in the error toast so that if you have 132 locations you can easily see which ones failed. ## How I Tested These Changes Loaded an org with 132 code locations and saw a single error toast. Clicked on "view definitions" and saw it took me to the code location page with status = Failed as the filter. Screenshot 2024-06-13 at 4 25 03 PM Screenshot 2024-06-13 at 4 24 53 PM --- .../src/instance/CodeLocationsPage.tsx | 50 +++++------ .../src/instance/flattenCodeLocationRows.tsx | 44 ++++++++-- .../instance/useCodeLocationPageFilters.tsx | 74 ++++++++++++++++ .../src/nav/useCodeLocationsStatus.tsx | 87 ++++++++++++++----- .../workspace/VirtualizedCodeLocationRow.tsx | 5 +- 5 files changed, 208 insertions(+), 52 deletions(-) create mode 100644 js_modules/dagster-ui/packages/ui-core/src/instance/useCodeLocationPageFilters.tsx diff --git a/js_modules/dagster-ui/packages/ui-core/src/instance/CodeLocationsPage.tsx b/js_modules/dagster-ui/packages/ui-core/src/instance/CodeLocationsPage.tsx index 6a0e3ef7adb5f..1a3fcc4f62d22 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/instance/CodeLocationsPage.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/instance/CodeLocationsPage.tsx @@ -3,12 +3,11 @@ import * as React from 'react'; import {InstancePageContext} from './InstancePageContext'; import {InstanceTabs} from './InstanceTabs'; -import {flattenCodeLocationRows} from './flattenCodeLocationRows'; +import {useCodeLocationPageFilters} from './useCodeLocationPageFilters'; import {useTrackPageView} from '../app/analytics'; import {useDocumentTitle} from '../hooks/useDocumentTitle'; import {ReloadAllButton} from '../workspace/ReloadAllButton'; import {RepositoryLocationsList} from '../workspace/RepositoryLocationsList'; -import {WorkspaceContext} from '../workspace/WorkspaceContext'; const SEARCH_THRESHOLD = 10; @@ -16,18 +15,8 @@ export const CodeLocationsPageContent = () => { useTrackPageView(); useDocumentTitle('Code locations'); - const {locationEntries, loading} = React.useContext(WorkspaceContext); - - const [searchValue, setSearchValue] = React.useState(''); - - const onChangeSearch = React.useCallback((e: React.ChangeEvent) => { - setSearchValue(e.target.value); - }, []); - - const queryString = searchValue.toLocaleLowerCase(); - const {flattened, filtered} = React.useMemo(() => { - return flattenCodeLocationRows(locationEntries, queryString); - }, [locationEntries, queryString]); + const {activeFiltersJsx, flattened, button, loading, filtered, onChangeSearch, searchValue} = + useCodeLocationPageFilters(); const entryCount = flattened.length; const showSearch = entryCount > SEARCH_THRESHOLD; @@ -47,17 +36,20 @@ export const CodeLocationsPageContent = () => { flex={{direction: 'row', justifyContent: 'space-between', alignItems: 'center'}} style={{height: '64px'}} > - {showSearch ? ( - - ) : ( - {subheadingText()} - )} + + {button} + {showSearch ? ( + + ) : ( + {subheadingText()} + )} + {showSearch ?
{`${entryCount} code locations`}
: null} @@ -68,6 +60,14 @@ export const CodeLocationsPageContent = () => { codeLocations={filtered} searchValue={searchValue} /> + {activeFiltersJsx.length ? ( + + {activeFiltersJsx} + + ) : null} ); }; diff --git a/js_modules/dagster-ui/packages/ui-core/src/instance/flattenCodeLocationRows.tsx b/js_modules/dagster-ui/packages/ui-core/src/instance/flattenCodeLocationRows.tsx index b9d7513e2dd71..392b0b1ee93e8 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/instance/flattenCodeLocationRows.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/instance/flattenCodeLocationRows.tsx @@ -1,4 +1,7 @@ -import {CodeLocationRowType} from '../workspace/VirtualizedCodeLocationRow'; +import { + CodeLocationRowStatusType, + CodeLocationRowType, +} from '../workspace/VirtualizedCodeLocationRow'; import {WorkspaceLocationNodeFragment} from '../workspace/types/WorkspaceQueries.types'; const flatten = (locationEntries: WorkspaceLocationNodeFragment[]) => { @@ -6,20 +9,48 @@ const flatten = (locationEntries: WorkspaceLocationNodeFragment[]) => { const all: CodeLocationRowType[] = []; for (const locationNode of locationEntries) { const {locationOrLoadError} = locationNode; + let status: CodeLocationRowStatusType; + if (locationNode.loadStatus === 'LOADING') { + if (locationOrLoadError) { + status = 'Updating'; + } else { + status = 'Loading'; + } + } else { + if (locationOrLoadError?.__typename === 'PythonError') { + status = 'Failed'; + } else { + status = 'Loaded'; + } + } if (!locationOrLoadError || locationOrLoadError?.__typename === 'PythonError') { - all.push({type: 'error' as const, node: locationNode}); + all.push({type: 'error' as const, node: locationNode, status}); } else { locationOrLoadError.repositories.forEach((repo) => { - all.push({type: 'repository' as const, codeLocation: locationNode, repository: repo}); + all.push({ + type: 'repository' as const, + codeLocation: locationNode, + repository: repo, + status, + }); }); } } return all; }; -const filterBySearch = (flattened: CodeLocationRowType[], searchValue: string) => { +const filterRows = ( + flattened: CodeLocationRowType[], + searchValue: string, + filters: CodeLocationFilters, +) => { const queryString = searchValue.toLocaleLowerCase(); return flattened.filter((row) => { + if (filters.status?.length) { + if (!filters.status.includes(row.status)) { + return false; + } + } if (row.type === 'error') { return row.node.name.toLocaleLowerCase().includes(queryString); } @@ -30,12 +61,15 @@ const filterBySearch = (flattened: CodeLocationRowType[], searchValue: string) = }); }; +export type CodeLocationFilters = Partial<{status: CodeLocationRowStatusType[]}>; + export const flattenCodeLocationRows = ( locationEntries: WorkspaceLocationNodeFragment[], searchValue: string = '', + filters: CodeLocationFilters = {}, ) => { const flattened = flatten(locationEntries); - const filtered = filterBySearch(flattened, searchValue); + const filtered = filterRows(flattened, searchValue, filters); return { flattened, diff --git a/js_modules/dagster-ui/packages/ui-core/src/instance/useCodeLocationPageFilters.tsx b/js_modules/dagster-ui/packages/ui-core/src/instance/useCodeLocationPageFilters.tsx new file mode 100644 index 0000000000000..0c502fcc814bb --- /dev/null +++ b/js_modules/dagster-ui/packages/ui-core/src/instance/useCodeLocationPageFilters.tsx @@ -0,0 +1,74 @@ +import React, {useCallback, useContext, useMemo, useState} from 'react'; + +import {CodeLocationFilters, flattenCodeLocationRows} from './flattenCodeLocationRows'; +import {useQueryPersistedState} from '../hooks/useQueryPersistedState'; +import {TruncatedTextWithFullTextOnHover} from '../nav/getLeftNavItemsForOption'; +import {useFilters} from '../ui/Filters'; +import {useStaticSetFilter} from '../ui/Filters/useStaticSetFilter'; +import {CodeLocationRowStatusType} from '../workspace/VirtualizedCodeLocationRow'; +import {WorkspaceContext} from '../workspace/WorkspaceContext'; + +export const useCodeLocationPageFilters = () => { + const {locationEntries, loading} = useContext(WorkspaceContext); + + const [searchValue, setSearchValue] = useState(''); + + const onChangeSearch = useCallback((e: React.ChangeEvent) => { + setSearchValue(e.target.value); + }, []); + + const queryString = searchValue.toLocaleLowerCase(); + + const [filters, setFilters] = useQueryPersistedState({ + encode: ({status}) => ({ + status: status?.length ? JSON.stringify(status) : undefined, + }), + decode: (qs) => { + return { + status: qs.status ? JSON.parse(qs.status) : [], + }; + }, + }); + + const {flattened, filtered} = useMemo(() => { + return flattenCodeLocationRows(locationEntries, queryString, filters); + }, [locationEntries, queryString, filters]); + + const statusFilter = useStaticSetFilter({ + name: 'Status', + icon: 'tag', + allValues: useMemo( + () => + (['Failed', 'Loaded', 'Updating', 'Loading'] as const).map((value) => ({ + key: value, + value, + match: [value], + })), + [], + ), + menuWidth: '300px', + renderLabel: ({value}) => { + return ; + }, + getStringValue: (value) => value, + state: filters.status, + onStateChanged: (values) => { + setFilters({status: Array.from(values)}); + }, + matchType: 'all-of', + canSelectAll: false, + allowMultipleSelections: true, + }); + + const {button, activeFiltersJsx} = useFilters({filters: [statusFilter]}); + + return { + button, + activeFiltersJsx, + onChangeSearch, + loading, + flattened, + filtered, + searchValue, + }; +}; diff --git a/js_modules/dagster-ui/packages/ui-core/src/nav/useCodeLocationsStatus.tsx b/js_modules/dagster-ui/packages/ui-core/src/nav/useCodeLocationsStatus.tsx index 98e523672ea8d..0ba91a5008265 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/nav/useCodeLocationsStatus.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/nav/useCodeLocationsStatus.tsx @@ -1,5 +1,5 @@ import {Box, ButtonLink, Colors} from '@dagster-io/ui-components'; -import {useCallback, useContext, useLayoutEffect, useRef, useState} from 'react'; +import {useCallback, useContext, useLayoutEffect, useMemo, useRef, useState} from 'react'; import {useHistory} from 'react-router-dom'; import {atom, useRecoilValue} from 'recoil'; import styled from 'styled-components'; @@ -7,6 +7,7 @@ import styled from 'styled-components'; import {showSharedToaster} from '../app/DomUtils'; import {RepositoryLocationLoadStatus} from '../graphql/types'; import {StatusAndMessage} from '../instance/DeploymentStatusType'; +import {CodeLocationRowStatusType} from '../workspace/VirtualizedCodeLocationRow'; import {WorkspaceContext} from '../workspace/WorkspaceContext'; import {CodeLocationStatusQuery} from '../workspace/types/WorkspaceQueries.types'; @@ -24,7 +25,7 @@ export const codeLocationStatusAtom = atom( }); export const useCodeLocationsStatus = (): StatusAndMessage | null => { - const {locationEntries, data} = useContext(WorkspaceContext); + const {locationEntries, loading, data} = useContext(WorkspaceContext); const [previousEntriesById, setPreviousEntriesById] = useState(null); const history = useHistory(); @@ -33,35 +34,71 @@ export const useCodeLocationsStatus = (): StatusAndMessage | null => { const [showSpinner, setShowSpinner] = useState(false); - const onClickViewButton = useCallback(() => { - historyRef.current.push('/locations'); + const onClickViewButton = useCallback((statuses: CodeLocationRowStatusType[]) => { + historyRef.current.push(`/locations?status=${JSON.stringify(statuses)}`); }, []); // Reload the workspace, but don't toast about it. + const previousErroredLocationEntries = useRef(null); + let erroredLocationEntries = useMemo( + () => + Object.values(data) + .map((entry) => { + if (entry.__typename === 'PythonError') { + return entry.__typename; + } + if (entry.locationOrLoadError?.__typename === 'PythonError') { + return entry.updatedTimestamp; + } + return null; + }) + .filter(Boolean), + [data], + ); + if ( + !previousErroredLocationEntries.current || + previousErroredLocationEntries.current.length !== erroredLocationEntries.length || + previousErroredLocationEntries.current?.some( + (entry, index) => entry !== erroredLocationEntries[index], + ) + ) { + previousErroredLocationEntries.current = erroredLocationEntries; + } else { + // We need preserve the previous reference to avoid firing the error layout effect more than necessary. + // This is due to their being multiple updates to `data` due to locations being fetched individually + // and each updating `data` as they come in. + erroredLocationEntries = previousErroredLocationEntries.current; + } + // Reload the workspace, and show a success or error toast upon completion. useLayoutEffect(() => { - const anyErrors = Object.values(data).some( - (entry) => - entry.__typename === 'PythonError' || - entry.locationOrLoadError?.__typename === 'PythonError', - ); - - const showViewButton = !alreadyViewingCodeLocations(); + if (loading) { + return; + } - if (anyErrors) { + if (erroredLocationEntries.length) { showSharedToaster({ intent: 'warning', message: (
Definitions loaded with errors
- {showViewButton ? : null} + { + onClickViewButton(['Failed']); + }} + /> ), icon: 'check_circle', }); } + }, [erroredLocationEntries, loading, onClickViewButton]); + useLayoutEffect(() => { + if (loading) { + return; + } const anyLoading = Object.values(data).some( (entry) => entry.__typename === 'WorkspaceLocationEntry' && @@ -70,14 +107,12 @@ export const useCodeLocationsStatus = (): StatusAndMessage | null => { if (!anyLoading) { setShowSpinner(false); } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [data, onClickViewButton]); + }, [loading, data]); const codeLocationStatusQueryData = useRecoilValue(codeLocationStatusAtom); useLayoutEffect(() => { const isFreshPageload = previousEntriesById === null; - const showViewButton = !alreadyViewingCodeLocations(); // Given the previous and current code locations, determine whether to show a) a loading spinner // and/or b) a toast indicating that a code location is being reloaded. @@ -95,7 +130,11 @@ export const useCodeLocationsStatus = (): StatusAndMessage | null => { message: (
Definitions reloaded
- {showViewButton ? : null} + { + onClickViewButton([]); + }} + /> ), icon: 'check_circle', @@ -181,7 +220,11 @@ export const useCodeLocationsStatus = (): StatusAndMessage | null => { message: ( {toastContent()} - {showViewButton ? : null} + { + onClickViewButton([]); + }} + /> ), icon: 'add_circle', @@ -210,7 +253,11 @@ export const useCodeLocationsStatus = (): StatusAndMessage | null => { ) : ( Updating {currentlyLoading.length} code locations )} - {showViewButton ? : null} + { + onClickViewButton([]); + }} + /> ), icon: 'refresh', @@ -246,8 +293,6 @@ export const useCodeLocationsStatus = (): StatusAndMessage | null => { return null; }; -const alreadyViewingCodeLocations = () => document.location.pathname.endsWith('/locations'); - const ViewCodeLocationsButton = ({onClick}: {onClick: () => void}) => { return ( diff --git a/js_modules/dagster-ui/packages/ui-core/src/workspace/VirtualizedCodeLocationRow.tsx b/js_modules/dagster-ui/packages/ui-core/src/workspace/VirtualizedCodeLocationRow.tsx index 109e95b561d7a..139e9cf925c91 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/workspace/VirtualizedCodeLocationRow.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/workspace/VirtualizedCodeLocationRow.tsx @@ -17,13 +17,16 @@ import {workspacePathFromAddress} from './workspacePath'; import {TimeFromNow} from '../ui/TimeFromNow'; import {HeaderCell, HeaderRow, RowCell} from '../ui/VirtualizedTable'; +export type CodeLocationRowStatusType = 'Failed' | 'Updating' | 'Loaded' | 'Loading'; + export type CodeLocationRowType = | { type: 'repository'; codeLocation: WorkspaceLocationNodeFragment; repository: WorkspaceRepositoryFragment; + status: CodeLocationRowStatusType; } - | {type: 'error'; node: WorkspaceLocationNodeFragment}; + | {type: 'error'; node: WorkspaceLocationNodeFragment; status: CodeLocationRowStatusType}; const TEMPLATE_COLUMNS = '3fr 1fr 1fr 240px 160px';