Merge pull request #164 from amelioro/set-no-unchecked-index-access

Set no unchecked index access

web/docs/state-management.md

@@ -38,3 +38,8 @@ These are functions that subscribe to changes in the state
     return useTopicStore((state) => state.diagrams[diagramId]);
   };
   ```
+
+### Zombie child issue
+
+- hooks should generally not throw errors, particularly due to the [zombie child issue](https://github.com/pmndrs/zustand/issues/302)
+  - e.g. if a node is deleted from state, the node's component will have a re-render triggered due to the state change, but hooks looking for that node will not find it; in this case, the hook should just return something that doesn't error - the node's component will soon be removed from the DOM by re-render of the parent Diagram component

web/src/common/components/Menu/Menu.tsx

@@ -3,10 +3,12 @@
 import { MenuItem, type MenuItemProps, Menu as MuiMenu } from "@mui/material";
 import { Children, isValidElement } from "react";
 
+import { errorWithData } from "../../errorHandling";
+
 export const addCloseOnClick = (closeMenu: () => void, children: React.ReactNode) => {
   return Children.map(children, (child) => {
     if (!isValidElement(child) || child.type !== MenuItem) {
-      throw new Error("Menu children must be MenuItems");
+      throw errorWithData("Menu children must be MenuItems", child);
     }
 
     const childProps = child.props as MenuItemProps;

web/src/common/errorHandling.ts

@@ -0,0 +1,11 @@
+/**
+ * Creates an error with the stringified data.
+ *
+ * Main intent here is to be able to easily log objects related to an error, without needing to know the relevant properties.
+ */
+// eslint-disable-next-line functional/functional-parameters, @typescript-eslint/no-explicit-any
+export const errorWithData = (message: string, ...data: any[]) => {
+  return new Error(
+    data.length > 0 ? message + "\n\nrelated data:\n" + JSON.stringify(data) : message
+  );
+};

web/src/modules/topic/components/CriteriaTable/CriteriaTable.tsx

@@ -9,6 +9,7 @@ import MaterialReactTable, {
 } from "material-react-table";
 import React, { useState } from "react";
 
+import { errorWithData } from "../../../../common/errorHandling";
 import { useCriterionSolutionEdges, useNode, useNodeChildren } from "../../store/nodeHooks";
 import { closeTable } from "../../store/viewActions";
 import { Edge, Node, problemDiagramId } from "../../utils/diagram";
@@ -33,7 +34,9 @@ const buildRows = (rowNodes: Node[], columnNodes: Node[], edges: Edge[]): RowDat
       ...Object.fromEntries(
         columnNodes.map((columnNode) => {
           const edge = getConnectingEdge(rowNode, columnNode, edges);
-          if (!edge) throw new Error(`No edge found between ${rowNode.id} and ${columnNode.id}`);
+          if (!edge) {
+            throw errorWithData(`No edge found between ${rowNode.id} and ${columnNode.id}`, edges);
+          }
 
           return [columnNode.id, edge];
         })

web/src/modules/topic/components/Diagram/Diagram.styles.tsx

@@ -10,12 +10,14 @@ import "reactflow/dist/style.css";
  * nodes (z-index: 0);
  */
 export const zIndex = {
-  impliedEdge: -1,
+  background: -1,
   svgWhenAnyArguableSelected: 1,
-  arguableWhenNeighborSelected: 2,
-  arguableWhenSelected: 3,
+  secondary: 2,
+  primary: 3,
 };
 
+export type Spotlight = "primary" | "secondary" | "normal" | "background";
+
 interface FlowProps {
   isAnyArguableSelected: boolean;
 }

web/src/modules/topic/components/Node/EditableNode.styles.tsx

@@ -46,6 +46,7 @@ interface StyledTextareaProps {
 }
 
 export const StyledTextareaAutosize = styled(TextareaAutosize)<StyledTextareaProps>`
+  padding: 0;
   border: 0;
   resize: none;
   outline: none;

web/src/modules/topic/components/Node/FlowNode.styles.tsx

@@ -3,7 +3,7 @@ import { css } from "@mui/material";
 
 import { Node } from "../../utils/diagram";
 import { Orientation } from "../../utils/layout";
-import { zIndex } from "../Diagram/Diagram.styles";
+import { Spotlight, zIndex } from "../Diagram/Diagram.styles";
 import { AddNodeButtonGroup } from "../Node/AddNodeButtonGroup";
 import { EditableNode } from "./EditableNode";
 
@@ -74,42 +74,38 @@ export const AddNodeButtonGroupChild = styled(StyledAddNodeButtonGroup)<{
 `;
 
 interface NodeProps {
-  node: Node;
-  isNeighborSelected: boolean;
-  isEdgeSelected: boolean;
-  isAnyArguableSelected: boolean;
+  spotlight: Spotlight;
 }
 
 const options = {
-  shouldForwardProp: (prop: string) =>
-    !["isNeighborSelected", "isEdgeSelected", "isAnyArguableSelected"].includes(prop),
+  shouldForwardProp: (prop: string) => !["spotlight"].includes(prop),
 };
 
 export const StyledEditableNode = styled(EditableNode, options)<NodeProps>`
-  ${({ theme, node, isNeighborSelected, isEdgeSelected, isAnyArguableSelected }) => {
-    if (node.selected) {
+  ${({ theme, spotlight }) => {
+    if (spotlight === "primary") {
       return css``;
-    } else if (isNeighborSelected || isEdgeSelected) {
+    } else if (spotlight === "secondary") {
       return css`
         border-color: ${theme.palette.info.main};
-        z-index: ${zIndex.arguableWhenNeighborSelected};
+        z-index: ${zIndex.secondary};
       `;
-    } else if (isAnyArguableSelected) {
+    } else if (spotlight === "background") {
       return css`
         opacity: 0.5;
       `;
     }
   }};
 `;
 
-export const nodeStyles = (node: Node, isNeighborSelected: boolean) => {
+export const nodeStyles = (node: Node, spotlight: Spotlight) => {
   return css`
     // reactflow sets z-index on its node wrapper, so we can't just set z-index on our node div
     .react-flow__node[data-id="${node.id}"] {
-      z-index: ${node.selected
-        ? zIndex.arguableWhenSelected
-        : isNeighborSelected
-        ? zIndex.arguableWhenNeighborSelected
+      z-index: ${spotlight === "primary"
+        ? zIndex.primary
+        : spotlight === "secondary"
+        ? zIndex.secondary
         : 0} !important; // !important to override because reactflow sets z-index via style attribute
     }
   `;

web/src/modules/topic/components/Node/FlowNode.tsx

@@ -7,6 +7,7 @@ import { useDiagramType } from "../../store/store";
 import { Node, orientations } from "../../utils/diagram";
 import { NodeType } from "../../utils/node";
 import { NodeProps } from "../Diagram/Diagram";
+import { Spotlight } from "../Diagram/Diagram.styles";
 import {
   AddNodeButtonGroupChild,
   AddNodeButtonGroupParent,
@@ -37,13 +38,21 @@ export const FlowNode = (flowNode: NodeProps) => {
   const orientation = orientations[diagramType];
   const node = convertToNode(flowNode);
 
+  const spotlight: Spotlight = node.selected
+    ? "primary"
+    : isNeighborSelected || isEdgeSelected
+    ? "secondary"
+    : isAnyArguableSelected
+    ? "background"
+    : "normal";
+
   return (
     <>
-      <Global styles={nodeStyles(node, isNeighborSelected)} />
+      <Global styles={nodeStyles(node, spotlight)} />
 
       <HoverBridgeDiv />
 
-      <NodeHandle node={node} direction="parent" orientation={orientation} />
+      <NodeHandle node={node} direction="parent" orientation={orientation} spotlight={spotlight} />
       {/* should this use react-flow's NodeToolbar? seems like it'd automatically handle positioning */}
       <AddNodeButtonGroupParent
         fromNodeId={flowNode.id}
@@ -52,20 +61,15 @@ export const FlowNode = (flowNode: NodeProps) => {
         orientation={orientation}
       />
 
-      <StyledEditableNode
-        node={node}
-        isNeighborSelected={isNeighborSelected}
-        isEdgeSelected={isEdgeSelected}
-        isAnyArguableSelected={isAnyArguableSelected}
-      />
+      <StyledEditableNode node={node} spotlight={spotlight} />
 
       <AddNodeButtonGroupChild
         fromNodeId={flowNode.id}
         fromNodeType={node.type}
         as="child"
         orientation={orientation}
       />
-      <NodeHandle node={node} direction="child" orientation={orientation} />
+      <NodeHandle node={node} direction="child" orientation={orientation} spotlight={spotlight} />
     </>
   );
 };

web/src/modules/topic/components/Node/NodeHandle.styles.tsx

@@ -2,12 +2,15 @@ import { css } from "@emotion/react";
 import styled from "@emotion/styled";
 import { Handle } from "reactflow";
 
+import { Spotlight } from "../Diagram/Diagram.styles";
+
 interface Props {
   hasHiddenComponents: boolean;
+  spotlight: Spotlight;
 }
 
 const options = {
-  shouldForwardProp: (prop: string) => !["hasHiddenComponents"].includes(prop),
+  shouldForwardProp: (prop: string) => !["hasHiddenComponents", "spotlight"].includes(prop),
 };
 
 export const StyledHandle = styled(Handle, options)<Props>`
@@ -23,5 +26,13 @@ export const StyledHandle = styled(Handle, options)<Props>`
         `;
       }
     }}
+
+    ${({ spotlight }) => {
+      if (spotlight === "background") {
+        return css`
+          opacity: 0.5;
+        `;
+      }
+    }}
   }
 `;

web/src/modules/topic/components/Node/NodeHandle.tsx

@@ -8,6 +8,7 @@ import { toggleShowNeighbors } from "../../store/viewActions";
 import { Node, RelationDirection } from "../../utils/diagram";
 import { Orientation } from "../../utils/layout";
 import { hideableNodeTypes, nodeDecorations } from "../../utils/node";
+import { Spotlight } from "../Diagram/Diagram.styles";
 import { StyledHandle } from "./NodeHandle.styles";
 
 const tooltipItems = (
@@ -54,9 +55,10 @@ interface Props {
   node: Node;
   direction: RelationDirection;
   orientation: Orientation;
+  spotlight: Spotlight;
 }
 
-export const NodeHandle = ({ node, direction, orientation }: Props) => {
+export const NodeHandle = ({ node, direction, orientation, spotlight }: Props) => {
   const directedNeighbors = useNeighbors(node.id, direction, node.data.diagramId);
 
   const shownNodesTooltips = tooltipItems(node.id, directedNeighbors, direction, true);
@@ -81,6 +83,7 @@ export const NodeHandle = ({ node, direction, orientation }: Props) => {
         type={type}
         position={position}
         hasHiddenComponents={hiddenNodesTooltips.length > 0}
+        spotlight={spotlight}
       />
     </Tooltip>
   );

web/src/modules/topic/components/Score/ScorePie.tsx

@@ -4,6 +4,7 @@ import { useState } from "react";
 import { PieChart } from "react-minimal-pie-chart";
 import { Data } from "react-minimal-pie-chart/types/commonTypes";
 
+import { errorWithData } from "../../../../common/errorHandling";
 import { setScore } from "../../store/actions";
 import { ArguableType, Score, possibleScores } from "../../utils/diagram";
 import { scoreColors } from "./Score";
@@ -54,7 +55,11 @@ export const ScorePie = ({ circleDiameter, arguableId, arguableType }: Props) =>
         startAngle={-90 - 360 / possibleScores.length / 2} // shift first slice to top center
         onMouseOver={(_, dataIndex) => setHovered(dataIndex)}
         onMouseOut={(_) => setHovered(undefined)}
-        onClick={(_, dataIndex) => setScore(arguableId, arguableType, data[dataIndex].key as Score)}
+        onClick={(_, dataIndex) => {
+          const segmentData = data[dataIndex];
+          if (!segmentData) throw errorWithData(`invalid pie segment dataIndex ${dataIndex}`, data);
+          setScore(arguableId, arguableType, segmentData.key as Score);
+        }}
         background="white"
       />
     </CircleDiv>

web/src/modules/topic/components/ScoreEdge/ScoreEdge.styles.tsx

@@ -1,38 +1,30 @@
 import { css } from "@emotion/react";
 import styled from "@emotion/styled";
 
-import { zIndex } from "../Diagram/Diagram.styles";
+import { Spotlight, zIndex } from "../Diagram/Diagram.styles";
 
 interface PathProps {
-  isEdgeSelected: boolean;
-  isNodeSelected: boolean;
-  isAnyArguableSelected: boolean;
-  isImplied: boolean;
+  spotlight: Spotlight;
 }
 
 const pathOptions = {
-  shouldForwardProp: (prop: string) =>
-    !["isEdgeSelected", "isNodeSelected", "isAnyArguableSelected", "isImplied"].includes(prop),
+  shouldForwardProp: (prop: string) => !["spotlight"].includes(prop),
 };
 
 export const StyledPath = styled("path", pathOptions)<PathProps>`
   &.react-flow__edge-path {
-    ${({ theme, isEdgeSelected, isNodeSelected, isAnyArguableSelected, isImplied }) => {
-      if (isEdgeSelected) {
+    ${({ theme, spotlight }) => {
+      if (spotlight === "primary") {
         return css`
           opacity: 1;
         `;
-      } else if (isNodeSelected) {
+      } else if (spotlight === "secondary") {
         return css`
           // marker should be styled too but reactflow's default only styles this, and that's probably
           // because marker styles need to be specified when creating the marker element, without css
           stroke: ${theme.palette.info.main};
         `;
-      } else if (isAnyArguableSelected) {
-        return css`
-          opacity: 0.5;
-        `;
-      } else if (isImplied) {
+      } else if (spotlight === "background") {
         return css`
           opacity: 0.5;
         `;
@@ -44,22 +36,11 @@ export const StyledPath = styled("path", pathOptions)<PathProps>`
 interface DivProps {
   labelX: number;
   labelY: number;
-  isEdgeSelected: boolean;
-  isNodeSelected: boolean;
-  isAnyArguableSelected: boolean;
-  isImplied: boolean;
+  spotlight: Spotlight;
 }
 
 const divOptions = {
-  shouldForwardProp: (prop: string) =>
-    ![
-      "labelX",
-      "labelY",
-      "isEdgeSelected",
-      "isNodeSelected",
-      "isAnyArguableSelected",
-      "isImplied",
-    ].includes(prop),
+  shouldForwardProp: (prop: string) => !["labelX", "labelY", "spotlight"].includes(prop),
 };
 
 export const StyledDiv = styled("div", divOptions)<DivProps>`
@@ -81,25 +62,21 @@ export const StyledDiv = styled("div", divOptions)<DivProps>`
     transform: translate(-50%, -50%) translate(${labelX}px, ${labelY}px);
   `}
 
-  ${({ theme, isEdgeSelected, isNodeSelected, isAnyArguableSelected, isImplied }) => {
-    if (isEdgeSelected) {
+  ${({ theme, spotlight }) => {
+    if (spotlight === "primary") {
       return css`
         border-color: #555;
-        z-index: ${zIndex.arguableWhenSelected};
+        z-index: ${zIndex.primary};
       `;
-    } else if (isNodeSelected) {
+    } else if (spotlight === "secondary") {
       return css`
         border-color: ${theme.palette.info.main};
-        z-index: ${zIndex.arguableWhenNeighborSelected};
-      `;
-    } else if (isAnyArguableSelected) {
-      return css`
-        opacity: 0.5;
+        z-index: ${zIndex.secondary};
       `;
-    } else if (isImplied) {
+    } else if (spotlight === "background") {
       return css`
         opacity: 0.5;
-        z-index: ${zIndex.impliedEdge};
+        z-index: ${zIndex.background};
       `;
     }
   }}