refactor: DiffExpButtons (SCE-34) (#1140)

chanzuckerberg · Oct 24, 2024 · 186fea8 · 186fea8
1 parent d494730
commit 186fea8
Show file tree

Hide file tree

Showing 12 changed files with 197 additions and 119 deletions.
diff --git a/.infra/rdev/values.yaml b/.infra/rdev/values.yaml
@@ -2,7 +2,7 @@ stack:
   services:
     explorer:
       image:
-        tag: sha-064d4b58
+        tag: sha-0cc317df
       replicaCount: 1
       env:
         # env vars common to all deployment stages

diff --git a/client/src/components/DiffexNotice/style.ts b/client/src/components/DiffexNotice/style.ts
@@ -1,6 +1,6 @@
 import styled from "@emotion/styled";
 import Snackbar from "@mui/material/Snackbar";
-import { Alert } from "@mui/lab";
+import { Alert } from "@mui/material";
 
 export const StyledSnackbar = styled(Snackbar)`
   width: 423px;

diff --git a/client/src/components/MenuBar/components/DiffExpButtons/DiffExpButtons.tsx b/client/src/components/MenuBar/components/DiffExpButtons/DiffExpButtons.tsx
@@ -1,83 +1,57 @@
 import React from "react";
 import { connect } from "react-redux";
 import { ButtonGroup, AnchorButton, Tooltip } from "@blueprintjs/core";
-import actions from "actions";
+
 import * as globals from "~/globals";
 // @ts-expect-error ts-migrate(2307) FIXME: Cannot find module './menubar.css' or its correspo... Remove this comment to see the full error message
 import styles from "../../menubar.css";
 import CellSetButton from "./components/CellSetButton/CellSetButton";
+import {
+  DiffexpButtonsProps,
+  mapDispatchToProps,
+  mapStateToProps,
+} from "./types";
+import { useConnect } from "./connect";
+
+function DiffexpButtons(props: DiffexpButtonsProps): JSX.Element {
+  const { dispatch, differential, diffexpMayBeSlow, diffexpCellcountMax } =
+    props;
 
-// @ts-expect-error ts-migrate(1238) FIXME: Unable to resolve signature of class decorator whe... Remove this comment to see the full error message
-@connect((state) => ({
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any --- FIXME: disabled temporarily on migrate to TS.
-  differential: (state as any).differential,
-  diffexpMayBeSlow:
-    // eslint-disable-next-line @typescript-eslint/no-explicit-any --- FIXME: disabled temporarily on migrate to TS.
-    (state as any).config?.parameters?.["diffexp-may-be-slow"] ?? false,
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any --- FIXME: disabled temporarily on migrate to TS.
-  diffexpCellcountMax: (state as any).config?.limits?.diffexp_cellcount_max,
-}))
-class DiffexpButtons extends React.PureComponent {
-  // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types --- FIXME: disabled temporarily on migrate to TS.
-  computeDiffExp = () => {
-    // @ts-expect-error ts-migrate(2339) FIXME: Property 'dispatch' does not exist on type 'Readon... Remove this comment to see the full error message
-    const { dispatch, differential } = this.props;
-    if (differential.celllist1 && differential.celllist2) {
-      dispatch(
-        actions.requestDifferentialExpression(
-          differential.celllist1,
-          differential.celllist2
-        )
-      );
-    }
-  };
+  const {
+    computeDiffExp,
+    warnMaxSizeExceeded,
+    tipMessageWarn,
+    haveBothCellSets,
+    tipMessage,
+  } = useConnect({
+    dispatch,
+    differential,
+    diffexpMayBeSlow,
+    diffexpCellcountMax,
+  });
 
-  // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types --- FIXME: disabled temporarily on migrate to TS.
-  render() {
-    /* diffexp-related buttons may be disabled */
-    // @ts-expect-error ts-migrate(2339) FIXME: Property 'differential' does not exist on type 'Re... Remove this comment to see the full error message
-    const { differential, diffexpMayBeSlow, diffexpCellcountMax } = this.props;
-    const haveBothCellSets =
-      !!differential.celllist1 && !!differential.celllist2;
-    const haveEitherCellSet =
-      !!differential.celllist1 || !!differential.celllist2;
-    const slowMsg = diffexpMayBeSlow
-      ? " (CAUTION: large dataset - may take longer or fail)"
-      : "";
-    const tipMessage = `See top differentially expressed genes${slowMsg}`;
-    const tipMessageWarn = `The total number of cells for differential expression computation
-                            may not exceed ${diffexpCellcountMax}. Try reselecting new cell sets.`;
-    const warnMaxSizeExceeded =
-      haveEitherCellSet &&
-      !!diffexpCellcountMax &&
-      (differential.celllist1?.length ?? 0) +
-        (differential.celllist2?.length ?? 0) >
-        diffexpCellcountMax;
-    return (
-      <ButtonGroup className={styles.menubarButton}>
-        {/* @ts-expect-error ts-migrate(2769) FIXME: No overload matches this call. */}
-        <CellSetButton eitherCellSetOneOrTwo={1} />
-        {/* @ts-expect-error ts-migrate(2769) FIXME: No overload matches this call. */}
-        <CellSetButton eitherCellSetOneOrTwo={2} />
-        <Tooltip
-          content={warnMaxSizeExceeded ? tipMessageWarn : tipMessage}
-          position="bottom"
-          hoverOpenDelay={globals.tooltipHoverOpenDelayQuick}
-          intent={warnMaxSizeExceeded ? "danger" : "none"}
-        >
-          <AnchorButton
-            disabled={!haveBothCellSets || warnMaxSizeExceeded}
-            intent={warnMaxSizeExceeded ? "danger" : "primary"}
-            data-testid="diffexp-button"
-            loading={differential.loading}
-            icon="left-join"
-            fill
-            onClick={this.computeDiffExp}
-          />
-        </Tooltip>
-      </ButtonGroup>
-    );
-  }
+  return (
+    <ButtonGroup className={styles.menubarButton}>
+      <CellSetButton eitherCellSetOneOrTwo={1} />
+      <CellSetButton eitherCellSetOneOrTwo={2} />
+      <Tooltip
+        content={warnMaxSizeExceeded ? tipMessageWarn : tipMessage}
+        position="bottom"
+        hoverOpenDelay={globals.tooltipHoverOpenDelayQuick}
+        intent={warnMaxSizeExceeded ? "danger" : "none"}
+      >
+        <AnchorButton
+          disabled={!haveBothCellSets || warnMaxSizeExceeded}
+          intent={warnMaxSizeExceeded ? "danger" : "primary"}
+          data-testid="diffexp-button"
+          loading={differential.loading ?? false}
+          icon="left-join"
+          fill
+          onClick={computeDiffExp}
+        />
+      </Tooltip>
+    </ButtonGroup>
+  );
 }
 
-export default DiffexpButtons;
+export default connect(mapStateToProps, mapDispatchToProps)(DiffexpButtons);
diff --git a/...c/components/MenuBar/components/DiffExpButtons/components/CellSetButton/CellSetButton.tsx b/...c/components/MenuBar/components/DiffExpButtons/components/CellSetButton/CellSetButton.tsx
@@ -1,55 +1,42 @@
 import React from "react";
 import { AnchorButton, Tooltip } from "@blueprintjs/core";
 import { connect } from "react-redux";
-import actions from "actions";
 import { track } from "analytics";
 import { EVENTS } from "analytics/events";
 import { tooltipHoverOpenDelay } from "~/globals";
+import {
+  CellSetButtonProps,
+  mapDispatchToProps,
+  mapStateToProps,
+} from "./types";
+import { useConnect } from "./connect";
 
-// @ts-expect-error ts-migrate(1238) FIXME: Unable to resolve signature of class decorator whe... Remove this comment to see the full error message
-@connect((state) => ({
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any --- FIXME: disabled temporarily on migrate to TS.
-  differential: (state as any).differential,
-}))
-class CellSetButton extends React.PureComponent {
-  // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types --- FIXME: disabled temporarily on migrate to TS.
-  set() {
-    // @ts-expect-error ts-migrate(2339) FIXME: Property 'dispatch' does not exist on type 'Readon... Remove this comment to see the full error message
-    const { dispatch, eitherCellSetOneOrTwo } = this.props;
-    dispatch(actions.setCellSetFromSelection(eitherCellSetOneOrTwo));
-  }
+function CellSetButton(props: CellSetButtonProps) {
+  const { set, cellsSelected } = useConnect(props);
+  const { eitherCellSetOneOrTwo } = props;
 
-  // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types --- FIXME: disabled temporarily on migrate to TS.
-  render() {
-    // @ts-expect-error ts-migrate(2339) FIXME: Property 'differential' does not exist on type 'Re... Remove this comment to see the full error message
-    const { differential, eitherCellSetOneOrTwo } = this.props;
-    const cellListName = `celllist${eitherCellSetOneOrTwo}`;
-    const cellsSelected = differential[cellListName]
-      ? differential[cellListName].length
-      : 0;
-    return (
-      <Tooltip
-        content="Save current selection for differential expression computation"
-        position="bottom"
-        hoverOpenDelay={tooltipHoverOpenDelay}
-      >
-        <AnchorButton
-          type="button"
-          onClick={() => {
-            track(EVENTS.EXPLORER_CELLSET_BUTTON_CLICKED);
+  return (
+    <Tooltip
+      content="Save current selection for differential expression computation"
+      position="bottom"
+      hoverOpenDelay={tooltipHoverOpenDelay}
+    >
+      <AnchorButton
+        type="button"
+        onClick={() => {
+          track(EVENTS.EXPLORER_CELLSET_BUTTON_CLICKED);
 
-            this.set();
-          }}
-          data-testid={`cellset-button-${eitherCellSetOneOrTwo}`}
-        >
-          <span data-testid={`cellset-count-${eitherCellSetOneOrTwo}`}>
-            {cellsSelected}
-          </span>
-          {" cells"}
-        </AnchorButton>
-      </Tooltip>
-    );
-  }
+          set();
+        }}
+        data-testid={`cellset-button-${eitherCellSetOneOrTwo}`}
+      >
+        <span data-testid={`cellset-count-${eitherCellSetOneOrTwo}`}>
+          {cellsSelected}
+        </span>
+        {" cells"}
+      </AnchorButton>
+    </Tooltip>
+  );
 }
 
-export default CellSetButton;
+export default connect(mapStateToProps, mapDispatchToProps)(CellSetButton);
diff --git a/client/src/components/MenuBar/components/DiffExpButtons/components/CellSetButton/connect.ts b/client/src/components/MenuBar/components/DiffExpButtons/components/CellSetButton/connect.ts
@@ -0,0 +1,22 @@
+import actions from "actions";
+import { DifferentialState } from "reducers/differential";
+import { CellSetButtonProps } from "./types";
+
+export function useConnect({
+  dispatch,
+  differential,
+  eitherCellSetOneOrTwo,
+}: CellSetButtonProps) {
+  function set() {
+    dispatch(actions.setCellSetFromSelection(eitherCellSetOneOrTwo));
+  }
+
+  const cellListName: keyof DifferentialState = `celllist${eitherCellSetOneOrTwo}`;
+
+  const cellsSelected = differential[cellListName]?.length ?? 0;
+
+  return {
+    set,
+    cellsSelected,
+  };
+}
diff --git a/client/src/components/MenuBar/components/DiffExpButtons/components/CellSetButton/types.ts b/client/src/components/MenuBar/components/DiffExpButtons/components/CellSetButton/types.ts
@@ -0,0 +1,16 @@
+import { AppDispatch, RootState } from "reducers";
+import { DifferentialState } from "reducers/differential";
+
+export interface CellSetButtonProps {
+  differential: DifferentialState;
+  eitherCellSetOneOrTwo: 1 | 2;
+  dispatch: AppDispatch;
+}
+
+export const mapStateToProps = (state: RootState) => ({
+  differential: state.differential,
+});
+
+export const mapDispatchToProps = (dispatch: AppDispatch) => ({
+  dispatch,
+});
diff --git a/client/src/components/MenuBar/components/DiffExpButtons/connect.ts b/client/src/components/MenuBar/components/DiffExpButtons/connect.ts
@@ -0,0 +1,48 @@
+import actions from "actions";
+import { DiffexpButtonsProps } from "./types";
+import {
+  DIFFEXP_SLOW_MSG,
+  genTipMessageWarn,
+  genTipMessage,
+} from "./constants";
+
+export function useConnect({
+  dispatch,
+  differential,
+  diffexpMayBeSlow,
+  diffexpCellcountMax,
+}: DiffexpButtonsProps) {
+  function computeDiffExp() {
+    if (differential.celllist1 && differential.celllist2) {
+      dispatch(
+        actions.requestDifferentialExpression(
+          differential.celllist1,
+          differential.celllist2
+        )
+      ).catch((error) => {
+        console.error("Failed to request differential expression:", error);
+      });
+    }
+  }
+
+  const haveBothCellSets = !!differential.celllist1 && !!differential.celllist2;
+  const haveEitherCellSet =
+    !!differential.celllist1 || !!differential.celllist2;
+  const slowMsg = diffexpMayBeSlow ? DIFFEXP_SLOW_MSG : "";
+  const tipMessage = genTipMessage(slowMsg);
+  const tipMessageWarn = genTipMessageWarn(diffexpCellcountMax);
+  const warnMaxSizeExceeded =
+    haveEitherCellSet &&
+    !!diffexpCellcountMax &&
+    (differential.celllist1?.length ?? 0) +
+      (differential.celllist2?.length ?? 0) >
+      diffexpCellcountMax;
+
+  return {
+    computeDiffExp,
+    haveBothCellSets,
+    warnMaxSizeExceeded,
+    tipMessage,
+    tipMessageWarn,
+  };
+}
diff --git a/client/src/components/MenuBar/components/DiffExpButtons/constants.ts b/client/src/components/MenuBar/components/DiffExpButtons/constants.ts
@@ -0,0 +1,6 @@
+export const DIFFEXP_SLOW_MSG =
+  "(CAUTION: large dataset - may take longer or fail)";
+export const genTipMessage = (slowMsg: string) =>
+  `See top differentially expressed genes${slowMsg}`;
+export const genTipMessageWarn = (cellCountMax: number) =>
+  `The total number of cells for differential expression computation may not exceed ${cellCountMax}. Try reselecting new cell sets.`;
diff --git a/client/src/components/MenuBar/components/DiffExpButtons/types.ts b/client/src/components/MenuBar/components/DiffExpButtons/types.ts
@@ -0,0 +1,19 @@
+import { AppDispatch, RootState } from "reducers";
+import { DifferentialState } from "reducers/differential";
+
+export interface DiffexpButtonsProps {
+  differential: DifferentialState;
+  diffexpMayBeSlow: boolean;
+  diffexpCellcountMax: number;
+  dispatch: AppDispatch;
+}
+
+export const mapStateToProps = (state: RootState) => ({
+  differential: state.differential,
+  diffexpMayBeSlow: state.config?.parameters?.["diffexp-may-be-slow"] ?? false,
+  diffexpCellcountMax: state.config?.limits?.diffexp_cellcount_max ?? 0,
+});
+
+export const mapDispatchToProps = (dispatch: AppDispatch) => ({
+  dispatch,
+});
diff --git a/client/src/globals.ts b/client/src/globals.ts
@@ -69,6 +69,10 @@ export interface Config {
   };
   portalUrl: string;
   links: Record<ConfigLink, unknown>;
+  limits: {
+    column_request_max?: number;
+    diffexp_cellcount_max?: number;
+  };
 }
 
 /* shape of config library_versions */

diff --git a/client/src/reducers/config.ts b/client/src/reducers/config.ts
@@ -5,6 +5,7 @@ export interface ConfigState {
   features: Config["features"] | null;
   parameters: Config["parameters"] | null;
   displayNames: Config["displayNames"] | null;
+  limits: Config["limits"] | null;
   loading?: boolean;
   error?: Error | null;
   corpora_props?: Config["corpora_props"];
@@ -20,6 +21,7 @@ const ConfigReducer = (
     features: null,
     parameters: null,
     isDeepZoomSourceValid: true,
+    limits: null,
   },
   action: AnyAction
 ): ConfigState => {

diff --git a/client/src/reducers/differential.ts b/client/src/reducers/differential.ts
@@ -1,7 +1,7 @@
 import { AnyAction } from "redux";
 import { LabelArray } from "../util/dataframe";
 
-interface DifferentialState {
+export interface DifferentialState {
   loading: boolean | null;
   error: Error | string | null;
   celllist1: LabelArray | null;