NickAkhmetov/CAT-777 Processed Data section #3495

NickAkhmetov · 2024-08-01T19:23:46Z

Summary

This PR implements the Processed Data section for unified views.

TODOs

Handle vislifted datasets by treating supports as processed datasets
Sort processed datasets - internal datasets on top, then sort by publication status
~~Handle height difference between data products and files to avoid layout shift~~ - since this shift only occurs on user interaction, let's leave it alone for now and correct if needed.
Handle versioned datasets - may require further discussion
Make helper panel use the same logic as table of contents for positioning
Make non-published datasets' accordions collapsed by default
Functional workspace menu options
~~Tooltips for status icons~~ I believe this was done upstream and just needs to be merged in; if not, it's a finishing touch.

Future improvements not in scope of this PR

Unified protocols section (tabbed view with any protocols from datasets)
Improved analysis details section (cards or flowchart instead of wall of links)
Combined metadata table
Redirect from processed dataset pages

Design Documentation/Original Tickets

Subtask: https://hms-dbmi.atlassian.net/browse/CAT-777
Task: https://hms-dbmi.atlassian.net/browse/CAT-678
Designs: https://www.figma.com/design/GUz7Z37Q3AAMiRyDy7YzWd/Unified-Detail-Pages?node-id=334-42649&m=dev

Testing

See screenshots in accordions below.

Screenshots/Video

CODEX

H&E Stained Microscopy

![image](https://github.com/user-attachments/assets/cfb6938e-0789-4af1-a0eb-04417c96b1ab)

Visium

Donor

Sample

Organ

Publication

Collapsible section demo with disappearing actions:

Screen.Recording.2024-08-16.122217.mp4

Checklist

Code follows the project's coding standards
- Lint checks pass locally
- New CHANGELOG-your-feature-name-here.md is present in the root directory, describing the change(s) in full sentences.
Unit tests covering the new feature have been added
All existing tests pass
Any relevant documentation in JIRA/Confluence has been updated to reflect the new feature
Any new functionalities have appropriate analytics functionalities added

Additional Notes

I'm open to ideas re: unit tests to add.

… support for partial multifetch

…o analysis details, add section descriptions, ensure hash links work with `document.querySelector`, add preliminary logic for scrolling to lazy loaded pieces,

…ontext

… from dataset page, remove vitessce conf generation from dataset page path, enable providing marker genes and parent uuids to .vitessce.json paths

…test

austenem

This looks great! Most of these are just questions/small stylistic suggestions.

austenem · 2024-08-09T15:56:32Z

context/app/static/js/components/detailPage/DatasetRelationships/nodeTypes.tsx

  return (
    <NodeTemplate
      rounded
      target
      icon={nodeIcons.componentDataset}
-      href={`#${data.name}-section`}
+      href={datasetDetails ? `#${datasetSectionId(datasetDetails, 'section')}` : undefined}


Could this be

href={datasetDetails && `#${datasetSectionId(datasetDetails, 'section')}`}`

?

I prefer to explicitly pass undefined in props when I can to be safe in case the false boolean value that the expression false && "string" gets toString()ed to "false" down the render chain. With that said, since this is repeated throughout this file, I'll make a helper function for this logic instead.

austenem · 2024-08-09T16:37:50Z

context/app/static/js/components/detailPage/AnalysisDetails/style.ts

+const PrimaryTextDivider = styled(Divider)(({ theme }) => ({
+  marginLeft: theme.spacing(0.5),
+  marginRight: theme.spacing(0.5),
+  height: '15px',


Could this height val be 1rem?

austenem · 2024-08-09T16:38:42Z

context/app/static/js/components/detailPage/BulkDataTransfer/BulkDataTransferSection.tsx

+        <SectionDescription>
+          This section explains how to download data in bulk from raw and processed datasets. Processed datasets have
+          separate download directories in Globus or dbGaP, distinct from the raw dataset.
+        </SectionDescription>


This text is longer and might be nice to have as a const.

austenem · 2024-08-09T16:39:27Z

context/app/static/js/components/detailPage/CollectionsSection/CollectionsSection.tsx

+      <SectionDescription>
+        Collections may contain references to either raw or processed datasets. If a processed dataset is not included
+        in any collection, there will be no corresponding tabs in the table below.
+      </SectionDescription>


Same here ^

austenem · 2024-08-09T17:54:20Z

context/app/static/js/components/detailPage/ProcessedData/HelperPanel/HelperPanel.tsx

+
+export default function HelperPanel() {
+  const currentDataset = useCurrentDataset();
+  // const panelOffset = useProcessedDataStore((state) => state.currentDatasetOffset);


This can be removed

austenem · 2024-08-09T20:41:34Z

context/app/static/js/components/detailPage/StatusIcon/style.ts

+) =>
+  styled(IconComponent)(({ theme, $noColor }) => ({
+    color: $noColor ? 'inherit' : theme.palette[status].main,
+    fontSize: '1em',
    marginRight: 3,


Could marginRight come from the theme?

This appears to be the value we have on main as well

austenem · 2024-08-09T21:41:27Z

context/app/static/js/pages/Dataset/Dataset.tsx

+          {shouldDisplaySection.files && <Files files={files} />}
+          {shouldDisplaySection['bulk-data-transfer'] && <BulkDataTransfer />}
+          {shouldDisplaySection.collections && <CollectionsSection collectionsData={collectionsData} />}
+          {shouldDisplaySection.contributors && <ContributorsTable contributors={contributors} title="Contributors" />}


Destructuring shouldDisplaySection might make this TSX slightly more readable

Good call; this has always presented a bit of a maintainability nightmare. I'm currently building out the collapsible detail page sections, which will have a shouldDisplay prop that will clean this up somewhat.

austenem · 2024-08-09T21:48:08Z

context/app/static/js/pages/Dataset/hooks.ts

  };

  const sectionsToDisplay = Object.entries(shouldDisplaySection).filter(([_k, v]) => v === true);

  return {
    // TODO: Improve the lookup for descendants to exclude anything with a missing pipeline name
-    ...getSectionFromString(pipeline ?? hubmap_id, `${hubmap_id}-section`),
-    items: sectionsToDisplay.map(([s]) => ({ ...getSectionFromString(s), hash: `${s}-${hubmap_id}` })),
+    ...getSectionFromString(pipeline ?? hubmap_id, datasetSectionId(hit._source, 'section')),


What is the naming convention behind variables _likeThis?

In this scenario - Search API responses return field-level data inside a _source field for each hit, since each hit also includes _id (the entity's UUID), _score (how closely the entity matches query parameters if using looser matching than must), _index (which search index the response came from, a constant value depending on the environment in our usecase), and _type ("doc" in our case).

In non-search API contexts, including the _ prefix for _first indicates that the variable is not used, and keeps ESLint from yelling at us for unused variables. For example, if you want to drop the first item in a list, you can destructure it like so:

const [_first, rest] = [1, 2, 3, 4, 5]

Ahh got it! Thanks for the explanation.

austenem · 2024-08-09T21:59:03Z

context/app/static/js/pages/Dataset/hooks.ts

+            // - original link pointed to older version of the dataset and the user needs to select it
+            toastError(
+              'Could not find the section you were looking for. The dataset you wish to view may have been updated to a new version.',
+            );


Maybe remove the language about the new version? I got this when I pasted a dataset link into a new page and it had a section hash appended to it, and the wording about new versions was a bit confusing. I think the first sentence accounts for all of these cases.

Good call, thanks for pointing it out!

I am planning to iterate on this by having each section independently check if its ID matches the hash of the window on initial load, but haven't yet gotten around to revisiting this functionality.

austenem · 2024-08-09T22:00:54Z

context/app/static/js/shared-styles/sections/TableOfContents/hooks.ts

-export { useThrottledOnScroll, useFindActiveIndex };
+function useAnimatedSidebarPosition() {
+  const { summaryInView } = useEntityStore((state: EntityStore) => state.summaryComponentObserver);
+  const initialHeightOffset = headerHeight + 16;


What is the 16 referring to here - where does this unit come from?

This was adding 1rem of padding to the top of the sidebar to keep it from touching the entity header; now that John's entity header work is merged in, I was able to replace this.

…ra provider, improve table of contents visualization section logic

…ummary

…ilPageSection` where logical

…cordion actions when collapsed

NickAkhmetov · 2024-08-15T20:55:02Z

This should be ready for re-review now; the remaining changes are related to fixing broken specs, which I'll be able to fix in the morning. I'll update the PR description with all necessary documentation tomorrow as well.

john-conroy

A lot to sift through, but nothing really stands out as potentially problematic. Amazing work!

john-conroy · 2024-08-19T14:03:39Z

context/app/static/js/components/detailPage/AnalysisDetails/AnalysisDetailsLink.tsx

+interface CWLPipelineLink {
+  hash: string;
+  name: string;
+  origin: string;
+}
+
+interface IngestPipelineLink {
+  hash: string;
+  origin: string;
+}
+
+interface ProvAnalysisDetailsLinkProps {
+  data: CWLPipelineLink | IngestPipelineLink;
+}


Should we move this into the Dataset type?

john-conroy · 2024-08-19T15:52:58Z

context/app/static/js/components/detailPage/DatasetRelationships/hooks.ts

+  if (result.assaytype === 'image_pyramid') {
+    return 'Image Pyramid Generation';
+  }


Is there a difference between this statement and checking for is_pyramid in the vitessce-hints?

There isn't, so I've changed this condition to match what we use in portal-visualization.

john-conroy · 2024-08-19T15:54:01Z

context/app/static/js/components/detailPage/BulkDataTransfer/PublicationBulkDataTransfer.tsx

+import { sectionIconMap } from 'js/shared-styles/icons/sectionIconMap';
+import BulkDataTransferPanels from './BulkDataTransferPanels';
+
+// Workaround for publication pages


Maybe a more descriptive comment? Not sure what caused the workaround.

john-conroy · 2024-08-19T16:02:21Z

context/app/static/js/components/detailPage/BulkDataTransfer/usePanelSet.tsx

  const { status: globusURLStatus, isLoading: globusURLIsLoading } = useFetchProtectedFile(uuid);
  const hasNoAccess = globusURLStatus === 403;
  const isNonConsortium = !isHubmapUser;

-  if (accessType !== 'Protected') {
+  if (!accessType || accessType !== 'Protected') {


Are there situations we don't expect accessType or is this just for type safety?

This was added to handle the initial loading state for accessType; since this was previously provided by flask data, it was never expected to be undefined, but since each descendant's bulk data transfer logic now gets handled independently, this is now loaded clientside for any processed descendants.

john-conroy · 2024-08-19T16:03:59Z

context/app/static/js/components/detailPage/CollectionsSection/CollectionsSectionContext.tsx

+
+import { createContext, useContext } from 'js/helpers/context';
+
+interface CollectionsSectionContextType {


Could it make more sense to have a generic section provider?

The logic in this provider is specific to the collections section:

If no collections are found for any of the datasets, the section should not be displayed

If the primary dataset is not found in any collections but the processed dataset is, the section should be displayed

If the primary dataset is found in a collection, but none of the processed datasets are, the section should be displayed as a single tab

I'm adding this explanation to the context as a comment, thanks for catching this!

john-conroy · 2024-08-19T16:06:33Z

context/app/static/js/components/detailPage/ProcessedData/HelperPanel/HelperPanel.tsx

+const noWrapStyles = {
+  overflow: 'hidden',
+  textOverflow: 'ellipsis',
+  display: '-webkit-box',
+  WebkitLineClamp: 3,
+  WebkitBoxOrient: 'vertical',
+};


Should we reuse the LineClamp component?

Genuinely forgot about that already existing despite being the one to add it 😆

john-conroy · 2024-08-19T16:09:22Z

context/app/static/js/components/detailPage/ProcessedData/HelperPanel/HelperPanel.tsx

+          onClick={() => {
+            track({
+              action: 'Start Creating Workspace',
+              label: currentDataset?.hubmap_id,
+            });
+            setOpenCreateWorkspace(true);
+            handleClose();
+          }}
+        >
+          <ListItemIcon>
+            <WorkspacesIcon color="primary" fontSize="1.5rem" />
+          </ListItemIcon>
+          Launch New Workspace
+        </MenuItem>
+        <MenuItem
+          onClick={() => {
+            track({
+              action: 'Start Adding Dataset to Existing Workspace',
+              label: currentDataset?.hubmap_id,
+            });
+            openEditWorkspaceDialog();
+            handleClose();
+          }}


Leverage useCallback for these onclicks? I feel like we've been consistent with that recently.

john-conroy · 2024-08-19T16:10:22Z

context/app/static/js/components/detailPage/ProcessedData/ProcessedDataset/DatasetTitle.tsx

+      {hubmap_id}
+      <SecondaryBackgroundTooltip title="Copy HuBMAP ID">
+        <IconButton
+          onClick={() => {


Should we also use useCallback here? I'm fine with either, but we should be consistent.

john-conroy · 2024-08-19T16:11:31Z

context/app/static/js/components/detailPage/ProcessedData/ProcessedDataset/DatasetTitle.tsx

+import React from 'react';
+import Typography from '@mui/material/Typography';
+import { useHandleCopyClick } from 'js/hooks/useCopyText';
+import { SecondaryBackgroundTooltip } from 'js/shared-styles/tooltips';
+import IconButton from '@mui/material/IconButton';
+import ContentCopyIcon from '@mui/icons-material/ContentCopyRounded';
+import Stack from '@mui/material/Stack';
+import StatusIcon from '../../StatusIcon';
+import { useProcessedDatasetContext } from './ProcessedDatasetContext';
+import VersionSelect from '../../VersionSelect';
+import { useTrackEntityPageEvent } from '../../useTrackEntityPageEvent';
+import SummaryJSONButton from '../../summary/SummaryJSONButton';


Not for here, but should we add an eslint rule for import order?

Yes, I definitely want to get to the bottom of why that's not autofixing right now 😓

john-conroy · 2024-08-19T16:16:22Z

context/app/static/js/components/detailPage/ProcessedData/hooks.ts

+import { useMemo } from 'react';
+import { ProcessedDatasetInfo, useProcessedDatasets } from 'js/pages/Dataset/hooks';
+
+function createdByCentralProcess(dataset: ProcessedDatasetInfo) {
+  return dataset.creation_action === 'Central Process';
+}
+
+function datasetIsPublished(dataset: ProcessedDatasetInfo) {
+  return dataset.status === 'Published';
+}
+
+export function useSortedSearchHits(datasets: ReturnType<typeof useProcessedDatasets>['searchHits']) {


A spec would be helpful here.

* Add changelog * NickAkhmetov/CAT-677 Unified Prov Graph (#3458) Co-authored-by: John Conroy <[email protected]> Co-authored-by: Tabassum Kakar <[email protected]> Co-authored-by: tkakar <[email protected]> Co-authored-by: Austen Money <[email protected]> fix table tabs (#3466) Fix incorrect URL structure in gene search results (#3467) * John conroy/update table of contents CAT-755 (#3473) * Convert table of contents to typescript * Convert route to ts and add boundaries * Fix styling so top works * Add icons map * Fix responsiveness * Handle sub links * Update marker color * Style table of contents * Add icons to table of contents * Add processed datasets for table of contents * Add loading state * Adjust padding per Tiffany feedback * Update style per Tiffany's feedback * Open toc accordions by default * Add visualization to processed data sections in toc * Add processed sections and refactor * Update changelog * Update organ page to use layout * Switch to derived data per tiffany's feedback * Use height props * Pull utils/hooks/types into files * Add a use callback for toc click handler * Pull find active index into hook * Consolidate styles * Use system props --------- Co-authored-by: John Conroy <[email protected]> * NickAkhmetov/CAT-776 Dataset Relationships diagram (#3481) * John conroy/summary bar (#3501) * Remove placeholder for undefined items * Create entity header action buttons * Add workspaces button * Add copy button * Always show entity header * Add basic view select chips * Fix opacity transition * Animation progress * Stash * Add check icon to view select * Add datasets relationship graph to summary bar * Delete unused gene components * Update summary components * Different heights for summary and diagram * Show entity header content when expanded * Use position sticky * Fix divider * Remove version select * Add citation and consortium to summary for datasets * Clamp description in expanded summary * Only show view buttons on large desktops * Show mapped status * Dedupe flask data and entity store assay metadata * Add icons to summary * Split out cases into components * Add icons * Add paper for relationship diagram * Show summary view for more entities * Integrate publication summary * Fix publication summary * Fix undefined * Fix tests * Fix offsets * Remove fit view * Fix comments from review * Fix data-testid propogation --------- Co-authored-by: John Conroy <[email protected]> * NickAkhmetov/CAT-777 Processed Data section (#3495) * make sure sample metadata doesn't get lost * hide empty dataset relationships box on dataset pages without processed descendants --------- Co-authored-by: John Conroy <[email protected]> Co-authored-by: Nikolay Akhmetov <[email protected]>

NickAkhmetov added 17 commits July 31, 2024 09:12

set up processed data section, pull analysis details out of prov, add…

5bab2ee

… support for partial multifetch

text styling adjustments to match designs

dea3cb2

add helper panel, add publication/creation date logic, add tooltips t…

4d88066

…o analysis details, add section descriptions, ensure hash links work with `document.querySelector`, add preliminary logic for scrolling to lazy loaded pieces,

sort the processed data section

ee78c1e

match table of contents and helper panel behavior

7503f55

only expand published datasets by default, reduce prop drilling via c…

e0519ca

…ontext

switch to 3 line clamp in helper panel

c7273ed

pull in image pyramids into processed data, remove base visualization…

e9cebb6

… from dataset page, remove vitessce conf generation from dataset page path, enable providing marker genes and parent uuids to .vitessce.json paths

display dataset relationships for vislifted datasets' pyramids

a213e72

fix node links

9dfab45

error toast on malformed hash

4ce7bc2

workspace menu functionalities

ca986f2

no need to collapse internal subsections

ea4289c

enable changing processed datasets' versions

d4f9097

fixes for image pyramid case

f706d97

remove version select from dataset summary spec

96b5424

Add tracking and changelog

043659c

NickAkhmetov marked this pull request as ready for review August 7, 2024 21:14

NickAkhmetov requested review from lchoy, tkakar, tsliaw, john-conroy and austenem August 8, 2024 13:21

NickAkhmetov added 5 commits August 8, 2024 10:41

Add old version alert to processed dataset

2ea5fcf

improve section href logic and consistency

8a41ab2

fix wording on old version alert, fix convertProvDataToNodesAndEdges …

1a93ad2

…test

bulk data transfer section

8cc2eca

collections section

6c1622f

austenem requested changes Aug 9, 2024

View reviewed changes

metadata section updates

0bbab47

NickAkhmetov added 9 commits August 13, 2024 15:22

add json button, make copy ID icon consistent, remove unnecessary ext…

a99287e

…ra provider, improve table of contents visualization section logic

regular dataset label

4b6166c

improve text alignment for dataset relationship diagram and rest of s…

d2cbbc3

…ummary

set up redirect toast

21a6a59

add collapsible detail sections and section icons

8344df3

Combine DetailPageSection and SectionHeader into `CollapsibleDeta…

96c6ebb

…ilPageSection` where logical

fix specs

6f8667f

add icons to other detail pages, simplify attribution

4987df4

add icons to other detail pages, convert/clean up organ page, hide ac…

9f9c156

…cordion actions when collapsed

NickAkhmetov marked this pull request as ready for review August 15, 2024 20:55

NickAkhmetov added 2 commits August 16, 2024 11:06

fix missing cdn url errors

91e0fd7

mock collapsible detail page section out

81efb7f

john-conroy approved these changes Aug 19, 2024

View reviewed changes

NickAkhmetov added 8 commits August 19, 2024 12:37

add explanatory comment to collections section context

fb74d02

reuse lineclamp component instead of duplicating it

1cbc62a

fix barrel imports

ad04b6d

add specs

c71383f

add explanatory comment for publication bulk data transfer section

9008a74

use vitessce hints instead of assayname

84dc88b

deduplicate dag prov types

180d146

hide helper panel if no dataset is currently visible, add animations

3170a72

NickAkhmetov requested a review from john-conroy August 19, 2024 19:04

NickAkhmetov added 4 commits August 19, 2024 15:28

type lint fix

70c5c45

CAT-831 Data loading improvements for provenance

38f29b3

fix test mock

2300131

adjust styles for error tile to match

8813366

NickAkhmetov merged commit dda7257 into unified-datasets Aug 20, 2024
5 checks passed

NickAkhmetov deleted the nickakhmetov/processed-datasets branch August 20, 2024 18:10


		import { createContext, useContext } from 'js/helpers/context';

		interface CollectionsSectionContextType {

NickAkhmetov/CAT-777 Processed Data section #3495

NickAkhmetov/CAT-777 Processed Data section #3495

Conversation

NickAkhmetov commented Aug 1, 2024 • edited Loading

Summary

TODOs

Future improvements not in scope of this PR

Design Documentation/Original Tickets

Testing

Screenshots/Video

Checklist

Additional Notes

austenem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickAkhmetov commented Aug 15, 2024

john-conroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickAkhmetov commented Aug 1, 2024 •

edited

Loading