Skip to content

Commit

Permalink
Medium bug fixes (#283)
Browse files Browse the repository at this point in the history
Fixes bugs in the medium severity column. Each commit should have a
description of the bug it fixes and what a link to the issue

## Meets all label

This is a hacky fix that we needed to do to add the `Meets All` label
under the available files filter. It works by using some React state to
control whether the label should be visible or not, and we update the
state before we update the query parameter. We do this separately
because updating the query parameter state also triggers a refetch,
meaning the label won't be visible until

## Optimize revalidation + Default data loaders

This should hopefully fix the amount of flashing errors we see on the
frontend. The first thing I did was optimize revalidation by adding a
[shouldRevalidate()](https://remix.run/docs/en/main/route/should-revalidate)
for various pages so that we only refetch data when a required parameter
has changed. In the past, we would always refetch even if it wasn't
related to the query. For example, opening the metadata drawer or
switching tabs on the download modal all triggered refetches.

The second thing I did was add default data for the mock data used when
loading the tables. Before to create mock data, we were passing empty
objects and type casting them as the data object:


https://github.com/chanzuckerberg/cryoet-data-portal/blob/5bc0f0b607b2959f6acfb017b64ba48d25a49e7f/frontend/packages/data-portal/app/components/Run/AnnotationTable.tsx#L21

Now we provide empty defaults to the fields that are specified as
required in the interface:


https://github.com/chanzuckerberg/cryoet-data-portal/blob/388c816bd2a3524174471fe245ac25c58e7674c2/frontend/packages/data-portal/app/components/Run/AnnotationTable.tsx#L21-L37

This should fix the flashing errors because the errors were originally
caused because the frontend was trying to access data that didn't exist.
This usually happens whenever the page is refetching data and for a
split second, it tries rendering the invalid loading data. By reducing
the refetching + adding default data, we should:

1. Reduce the amount of refetches while using the app so that the
flashing errors don't occur
2. Add default data so that all data access is safe and using data that
exists

## Fix filters

To fix the filters, I used the `_and` and `_or` fields to implement the
desired logic. Every filter is pushed into a filters array with its
specified filter condition:


https://github.com/chanzuckerberg/cryoet-data-portal/blob/388c816bd2a3524174471fe245ac25c58e7674c2/frontend/packages/data-portal/app/routes/browse-data.datasets.tsx#L121-L130

We implement the filters with an OR condition by nesting an `_or` field:


https://github.com/chanzuckerberg/cryoet-data-portal/blob/388c816bd2a3524174471fe245ac25c58e7674c2/frontend/packages/data-portal/app/routes/browse-data.datasets.tsx#L216-L227


https://github.com/chanzuckerberg/cryoet-data-portal/blob/388c816bd2a3524174471fe245ac25c58e7674c2/frontend/packages/data-portal/app/routes/browse-data.datasets.tsx#L249-L251
  • Loading branch information
codemonkey800 authored Dec 15, 2023
1 parent 5bc0f0b commit ac81d47
Show file tree
Hide file tree
Showing 14 changed files with 368 additions and 59 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,19 @@ export function AnnotatedObjectsList({
? range(0, ANNOTATED_OBJECTS_MAX).map((val) => (
<Skeleton key={`skeleton-${val}`} variant="rounded" />
))
: annotatedObjects.map((obj) => (
<li
className="text-ellipsis line-clamp-1 break-all capitalize"
key={obj}
>
{obj}
</li>
))
: annotatedObjects
.slice()
.sort((val1, val2) =>
val1.toLowerCase().localeCompare(val2.toLocaleLowerCase()),
)
.map((obj) => (
<li
className="text-ellipsis line-clamp-1 break-all capitalize"
key={obj}
>
{obj}
</li>
))

return (
<List>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,12 @@ import { inQualityScoreRange } from 'app/utils/tiltSeries'

type Run = GetDatasetByIdQuery['datasets'][number]['runs'][number]

const LOADING_RUNS = range(0, MAX_PER_PAGE).map(() => ({}) as Run)
const LOADING_RUNS = range(0, MAX_PER_PAGE).map<Run>(() => ({
id: 0,
name: '',
tiltseries_aggregate: {},
tomogram_voxel_spacings: [],
}))

export function RunsTable() {
const { isLoadingDebounced } = useIsLoading()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useMemo } from 'react'
import { useEffect, useMemo, useState } from 'react'

import {
BooleanFilter,
Expand All @@ -7,6 +7,7 @@ import {
} from 'app/components/Filters'
import { QueryParams } from 'app/constants/query'
import { useDatasetFilter } from 'app/hooks/useDatasetFilter'
import { useI18n } from 'app/hooks/useI18n'
import { i18n } from 'app/i18n'
import {
AvailableFilesFilterOption,
Expand All @@ -28,6 +29,9 @@ const AVAILABLE_FILES_OPTIONS: AvailableFilesFilterOption[] = [
{ value: 'tomogram', label: i18n.tomogram },
]

const AVAILABLE_FILES_CLASS_NAME = 'select-available-files'
const MEETS_ALL_LABEL_ID = 'meets-all'

export function IncludedContentsFilterSection() {
const {
updateValue,
Expand Down Expand Up @@ -55,6 +59,34 @@ export function IncludedContentsFilterSection() {
[numberOfRuns],
)

const { t } = useI18n()

const [showMeetsAll, setShowMeetsAll] = useState(availableFiles.length > 0)

// Really hacky way to get a label to show above the selected chips for the
// SDS ComplexFilter component. We need to do this because the SDS does not
// yet provide a way to add a label.
// TODO Update upstream component to support chip labels
useEffect(() => {
const meetsAllNode = document.getElementById(
MEETS_ALL_LABEL_ID,
) as HTMLSpanElement | null

if (showMeetsAll && !meetsAllNode) {
const filterButtonNode = document.querySelector(
`.${AVAILABLE_FILES_CLASS_NAME} > button`,
)

const meetsAll = document.createElement('div')
meetsAll.id = MEETS_ALL_LABEL_ID
meetsAll.textContent = t('meetsAll')

filterButtonNode?.insertAdjacentElement('afterend', meetsAll)
} else if (!showMeetsAll && meetsAllNode) {
meetsAllNode.remove()
}
}, [showMeetsAll, t])

return (
<FilterSection title={i18n.includedContents}>
<BooleanFilter
Expand All @@ -70,7 +102,13 @@ export function IncludedContentsFilterSection() {
options={AVAILABLE_FILES_OPTIONS}
value={availableFilesOptions}
label={i18n.availableFiles}
onChange={(options) => updateValue(QueryParams.AvailableFiles, options)}
onChange={(options) => {
setShowMeetsAll((options?.length ?? 0) > 0)
updateValue(QueryParams.AvailableFiles, options)
}}
title={`${t('resultsMustIncludeAllFileTypes')}:`}
popperClassName="max-w-[244px]"
className={AVAILABLE_FILES_CLASS_NAME}
/>

<SelectFilter
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,10 @@
padding-left: theme('padding.sds-s');
}
}

:global(#meets-all) {
@apply text-sds-body-xs leading-sds-body-xs text-sds-gray-500;

letter-spacing: 0.3px;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { isArray, isEqual } from 'lodash-es'
import { useCallback, useMemo } from 'react'

import { BaseFilterOption } from 'app/types/filter'
import { cns } from 'app/utils/cns'

import styles from './SelectFilter.module.css'

Expand All @@ -18,18 +19,26 @@ export function SelectFilter<
Option extends BaseFilterOption,
Multiple extends boolean = false,
>({
className,
groupBy: groupByProp,
label,
multiple,
onChange,
options,
popperClassName,
search,
title,
value,
}: {
className?: string
groupBy?: (option: Value<Option, Multiple>) => string
label: string
multiple?: Multiple
onChange(value: Value<Option, Multiple>): void
options: Option[]
popperClassName?: string
search?: boolean
title?: string
value?: Value<Option, Multiple>
}) {
const labelValueMap = useMemo(
Expand Down Expand Up @@ -80,14 +89,26 @@ export function SelectFilter<
return null
}, [convertBaseOptionToSdsOption, value])

const groupBy = groupByProp
? (option: DefaultAutocompleteOption) =>
groupByProp(convertSdsOptionToBaseOption(option))
: undefined

return (
<ComplexFilter
className={styles.filter}
className={cns(styles.filter, className)}
value={sdsValue}
label={label}
search={search}
multiple={multiple}
options={sdsOptions}
DropdownMenuProps={{
groupBy,
title,
PopperBaseProps: {
className: popperClassName,
},
}}
onChange={(nextOptions) => {
if (isEqual(nextOptions, sdsValue)) {
return
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,23 @@ import { I18nKeys } from 'app/types/i18n'
import { getAnnotationTitle } from 'app/utils/annotation'
import { cnsNoMerge } from 'app/utils/cns'

const LOADING_ANNOTATIONS = range(0, MAX_PER_PAGE).map(() => ({}) as Annotation)
const LOADING_ANNOTATIONS = range(0, MAX_PER_PAGE).map<Annotation>(() => ({
annotation_method: '',
author_affiliations: [],
authors_aggregate: {},
authors: [],
confidence_precision: 0,
deposition_date: '',
files: [],
ground_truth_status: false,
https_path: '',
object_count: 0,
object_id: '',
object_name: '',
release_date: '',
s3_path: '',
shape_type: '',
}))

function ConfidenceValue({ value }: { value: number }) {
const { t } = useI18n()
Expand Down
1 change: 1 addition & 0 deletions frontend/packages/data-portal/app/constants/query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export enum QueryParams {
ObjectName = 'object',
ObjectShapeType = 'object_shape',
Organism = 'organism',
Page = 'page',
PortalId = 'portal_id',
ReconstructionMethod = 'reconstruction_method',
ReconstructionSoftware = 'reconstruction_software',
Expand Down
6 changes: 6 additions & 0 deletions frontend/packages/data-portal/app/root.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ export async function loader({ request }: LoaderFunctionArgs) {
})
}

export function shouldRevalidate() {
// Data returned from the root is pretty static, so we can disable
// revalidation forever to disable re-fetching root data.
return false
}

const Document = withEmotionCache(
({ children, title }: DocumentProps, emotionCache) => {
const clientStyleData = useContext(ClientStyleContext)
Expand Down
Loading

0 comments on commit ac81d47

Please sign in to comment.