Skip to content

Commit

Permalink
[dagit] Fix JS error on repo reload (#8297)
Browse files Browse the repository at this point in the history
### Summary & Motivation

There is currently a JS error that occurs when reloading a single repo from the left nav while viewing the Instance Overview page. It appears to be due to an Apollo bug: when the store is emptied after reloading the repo, the queries in `InstanceOverviewPage` resolve to `{}` instead of `undefined` while data is being repopulated.

Because our code currently checks whether `data` is truthy in order to determine whether there are values to extract from it, this results in a JS error because fields expected to be there (and typechecked as such) are missing.

To resolve this, use the `loading` values instead. As far as I know, we shouldn't really have to do that, but I don't know if this has been fixed in recent versions of `@apollo/client`.

I also fixed some spacing and truncation issues on the repo name at the foot of the left nav.

### How I Tested These Changes

View Dagit, reload the current repo via left nav while viewing Instance Overview. Verify that the page reloads and populates properly, with no JS errors.
  • Loading branch information
hellendag committed Jun 9, 2022
1 parent 09194c3 commit 79ea5d9
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ export const InstanceOverviewPage = () => {
fetchPolicy: 'network-only',
notifyOnNetworkStatusChange: true,
});
const {data: lastTenRunsData} = queryResultLastRuns;
const {data: lastTenRunsData, loading: lastTenRunsLoading} = queryResultLastRuns;

const refreshState = useMergedRefresh(
useQueryRefreshAtInterval(queryResultLastRuns, FIFTEEN_SECONDS),
Expand All @@ -129,7 +129,7 @@ export const InstanceOverviewPage = () => {
return a.job.name.toLocaleLowerCase().localeCompare(b.job.name.toLocaleLowerCase());
};

if (data && data?.workspaceOrError.__typename === 'Workspace') {
if (!loading && data?.workspaceOrError.__typename === 'Workspace') {
for (const locationEntry of data.workspaceOrError.locationEntries) {
if (
locationEntry.__typename === 'WorkspaceLocationEntry' &&
Expand Down Expand Up @@ -182,7 +182,7 @@ export const InstanceOverviewPage = () => {
neverRan.sort(sortFn);

return {failed, inProgress, queued, succeeded, neverRan};
}, [data]);
}, [data, loading]);

const filteredJobs = React.useMemo(() => {
const searchToLower = searchValue.toLocaleLowerCase();
Expand Down Expand Up @@ -211,7 +211,7 @@ export const InstanceOverviewPage = () => {
}

const flattened: {[key: string]: RunTimeFragment[]} = {};
if (lastTenRunsData && lastTenRunsData?.workspaceOrError.__typename === 'Workspace') {
if (!lastTenRunsLoading && lastTenRunsData?.workspaceOrError.__typename === 'Workspace') {
for (const locationEntry of lastTenRunsData.workspaceOrError.locationEntries) {
if (
locationEntry.__typename === 'WorkspaceLocationEntry' &&
Expand All @@ -231,7 +231,7 @@ export const InstanceOverviewPage = () => {
}

return flattened;
}, [lastTenRunsData]);
}, [lastTenRunsData, lastTenRunsLoading]);

const filteredJobsFlattened: JobItem[] = React.useMemo(() => {
return Object.values(filteredJobs).reduce((accum, jobList) => {
Expand Down
18 changes: 12 additions & 6 deletions js_modules/dagit/packages/core/src/nav/RepoNavItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,11 @@ export const RepoNavItem: React.FC<Props> = (props) => {
return <span style={{color: Colors.Gray700}}>No repositories</span>;
}
if (allRepos.length === 1) {
return <SingleRepoSummary repo={allRepos[0]} />;
return <SingleRepoSummary repo={allRepos[0]} onlyRepo />;
}
if (selected.length === 1) {
const selectedRepo = Array.from(selected)[0];
return <SingleRepoSummary repo={selectedRepo} />;
return <SingleRepoSummary repo={selectedRepo} onlyRepo={false} />;
}
return <span>{`${selected.length} of ${allRepos.length} shown`}</span>;
};
Expand Down Expand Up @@ -84,22 +84,28 @@ export const RepoNavItem: React.FC<Props> = (props) => {
</Box>
</DialogFooter>
</Dialog>
<Button onClick={() => setOpen(true)}>Filter</Button>
<Box margin={{left: 4}}>
<Button onClick={() => setOpen(true)}>Filter</Button>
</Box>
</>
) : null}
</Box>
</Box>
);
};

const SingleRepoSummary: React.FC<{repo: RepoSelectorOption}> = ({repo}) => {
const SingleRepoSummary: React.FC<{repo: RepoSelectorOption; onlyRepo: boolean}> = ({
repo,
onlyRepo,
}) => {
const repoAddress = buildRepoAddress(repo.repository.name, repo.repositoryLocation.name);
const {canReloadRepositoryLocation} = usePermissions();
return (
<Group direction="row" spacing={4} alignItems="center">
<SingleRepoNameLink
to={workspacePathFromAddress(repoAddress)}
title={repoAddressAsString(repoAddress)}
$onlyRepo={onlyRepo}
>
{repoAddress.name}
</SingleRepoNameLink>
Expand Down Expand Up @@ -148,10 +154,10 @@ const SummaryText = styled.div`
line-height: 32px;
`;

const SingleRepoNameLink = styled(Link)`
const SingleRepoNameLink = styled(Link)<{$onlyRepo: boolean}>`
color: ${Colors.Gray900};
display: block;
max-width: 234px;
max-width: ${({$onlyRepo}) => ($onlyRepo ? '248px' : '192px')};
overflow-x: hidden;
text-overflow: ellipsis;
transition: color 100ms linear;
Expand Down

0 comments on commit 79ea5d9

Please sign in to comment.