fix: undo slider creation (#938)

v3/cypress/e2e/slider.spec.ts

@@ -198,22 +198,22 @@ context("Slider UI", () => {
     slider.getVariableValue(2).should("contain", initialSliderValue)
     slider.checkPlayButtonIsPaused(2)
   })
-  it("creates sliders with incrementing names when existing ones are closed", () => {
+  it("reuses slider names after existing ones are closed", () => {
     c.closeComponent("slider")
     c.checkComponentDoesNotExist("slider")
     c.createFromToolshelf("slider")
 
-    c.getComponentTitle("slider").should("contain", "v2")
-    slider.getVariableName().should("have.text", "v2")
+    c.getComponentTitle("slider").should("contain", "v1")
+    slider.getVariableName().should("have.text", "v1")
     slider.getVariableValue().should("contain", initialSliderValue)
     slider.checkPlayButtonIsPaused()
 
     c.closeComponent("slider")
     c.checkComponentDoesNotExist("slider")
     c.createFromToolshelf("slider")
 
-    c.getComponentTitle("slider").should("contain", "v3")
-    slider.getVariableName().should("have.text", "v3")
+    c.getComponentTitle("slider").should("contain", "v1")
+    slider.getVariableName().should("have.text", "v1")
     slider.getVariableValue().should("contain", initialSliderValue)
     slider.checkPlayButtonIsPaused()
 

v3/src/components/axis/hooks/use-axis.test.tsx

@@ -1,27 +1,38 @@
 /* eslint-disable testing-library/no-node-access */
 import { renderHook } from "@testing-library/react"
+import { Instance, types } from "mobx-state-tree"
 import React from "react"
 import { SliderAxisLayout } from "../../slider/slider-layout"
 import { AxisLayoutContext } from "../models/axis-layout-context"
 import { INumericAxisModel, NumericAxisModel } from "../models/axis-model"
 import {IUseAxis, useAxis} from "./use-axis"
-import { AxisProviderContext, IAxisProvider } from "./use-axis-provider-context"
+import { AxisProviderContext } from "./use-axis-provider-context"
 
-describe("useNumericAxis", () => {
+const TestAxisProvider = types.model("TestAxisProvider", {
+  axis: NumericAxisModel
+})
+.views(self => ({
+  getAxis() {
+    return self.axis
+  },
+  getNumericAxis() {
+    return self.axis
+  }
+}))
+interface ITestAxisProvider extends Instance<typeof TestAxisProvider> {}
 
-  let provider: IAxisProvider
-  let layout: SliderAxisLayout
+describe("useAxis", () => {
+
+  let provider: ITestAxisProvider
   let axisModel: INumericAxisModel
+  let layout: SliderAxisLayout
   let axisElt: SVGGElement
   let useAxisOptions: IUseAxis
 
   beforeEach(() => {
-    provider = {
-      getAxis: () => axisModel,
-      getNumericAxis: () => axisModel
-    }
+    provider = TestAxisProvider.create({ axis: { place: "bottom", min: 0, max: 10 }})
+    axisModel = provider.axis
     layout = new SliderAxisLayout()
-    axisModel = NumericAxisModel.create({ place: "bottom", min: 0, max: 10 })
     axisElt = document.createElementNS("http://www.w3.org/2000/svg", "g")
     useAxisOptions = { axisPlace: "bottom", centerCategoryLabels: true }
   })

v3/src/components/axis/hooks/use-axis.ts

@@ -1,7 +1,8 @@
 import {ScaleBand, ScaleLinear, scaleLinear, scaleOrdinal} from "d3"
-import {autorun, reaction} from "mobx"
+import {reaction} from "mobx"
 import {isAlive} from "mobx-state-tree"
 import {useCallback, useEffect, useRef} from "react"
+import {MobXAutorun} from "../../../utilities/mobx-autorun"
 import {AxisPlace, axisGap} from "../axis-types"
 import {useAxisLayoutContext} from "../models/axis-layout-context"
 import {IAxisModel, isNumericAxisModel} from "../models/axis-model"
@@ -101,16 +102,16 @@ export const useAxis = ({ axisPlace, axisTitle = "", centerCategoryLabels }: IUs
   // update d3 scale and axis when axis domain changes
   useEffect(function installDomainSync() {
     if (isNumeric) {
-      const disposer = autorun(() => {
-        const _axisModel = axisProvider.getNumericAxis?.(axisPlace)
+      const mobXAutorun = new MobXAutorun(() => {
+        const _axisModel = axisProvider?.getNumericAxis?.(axisPlace)
         if (_axisModel && !isAlive(_axisModel)) {
           console.warn("useAxis.installDomainSync skipping sync of defunct axis model")
           return
         }
         _axisModel?.domain && multiScale?.setNumericDomain(_axisModel?.domain)
         layout.setDesiredExtent(axisPlace, computeDesiredExtent())
-      }, { name: "useAxis.installDomainSync" })
-      return () => disposer()
+      }, { name: "useAxis.installDomainSync" }, axisProvider)
+      return () => mobXAutorun.dispose()
     }
     // Note axisModelChanged as a dependent. Shouldn't be necessary.
   }, [axisModelChanged, isNumeric, multiScale, axisPlace, layout, computeDesiredExtent, axisProvider])

v3/src/components/axis/hooks/use-sub-axis.ts

@@ -1,13 +1,14 @@
 import {BaseType, drag, format, ScaleLinear, select, Selection} from "d3"
-import {autorun, reaction} from "mobx"
-import {isAlive} from "mobx-state-tree"
+import {reaction} from "mobx"
 import {MutableRefObject, useCallback, useEffect, useMemo, useRef} from "react"
 import {transitionDuration} from "../../data-display/data-display-types"
 import {AxisBounds, AxisPlace, axisPlaceToAxisFn, AxisScaleType, otherPlace} from "../axis-types"
 import {useAxisLayoutContext} from "../models/axis-layout-context"
 import {isCategoricalAxisModel, isNumericAxisModel} from "../models/axis-model"
 import {isVertical} from "../../axis-graph-shared"
 import {between} from "../../../utilities/math-utils"
+import {MobXAutorun} from "../../../utilities/mobx-autorun"
+import {isAliveSafe} from "../../../utilities/mst-utils"
 import {kAxisTickLength} from "../../graph/graphing-types"
 import {DragInfo, collisionExists, computeBestNumberOfTicks, getCategoricalLabelPlacement,
   getCoordFunctions, IGetCoordFunctionsProps} from "../axis-utils"
@@ -55,7 +56,7 @@ export const useSubAxis = ({
 
     renderSubAxis = useCallback(() => {
       const _axisModel = axisProvider.getAxis?.(axisPlace)
-      if (_axisModel && !isAlive(_axisModel)) {
+      if (!isAliveSafe(_axisModel)) {
         console.warn("useSubAxis.renderSubAxis skipping rendering of defunct axis model")
         return
       }
@@ -350,9 +351,9 @@ export const useSubAxis = ({
 
   // update d3 scale and axis when axis domain changes
   useEffect(function installDomainSync() {
-    const disposer = autorun(() => {
-      const _axisModel = axisProvider.getAxis?.(axisPlace)
-      if (_axisModel && isAlive(_axisModel)) {
+    const mobXAutorun = new MobXAutorun(() => {
+      const _axisModel = axisProvider?.getAxis?.(axisPlace)
+      if (isAliveSafe(_axisModel)) {
         if (isNumericAxisModel(_axisModel)) {
           const {domain} = _axisModel || {}
           layout.getAxisMultiScale(axisPlace)?.setNumericDomain(domain)
@@ -362,8 +363,8 @@ export const useSubAxis = ({
       else if (_axisModel) {
         console.warn("useSubAxis.installDomainSync skipping sync of defunct axis model")
       }
-    }, { name: "useSubAxis.installDomainSync" })
-    return () => disposer()
+    }, { name: "useSubAxis.installDomainSync" }, axisProvider)
+    return () => mobXAutorun.dispose()
   }, [axisPlace, axisProvider, layout, renderSubAxis])
 
   // Refresh when category set, if any, changes

v3/src/components/slider/slider-component.tsx

@@ -1,7 +1,7 @@
-import React, { CSSProperties, useEffect, useMemo, useRef, useState } from "react"
-import { useResizeDetector } from "react-resize-detector"
 import { Flex, Editable, EditablePreview, EditableInput, Button } from "@chakra-ui/react"
 import { observer } from "mobx-react-lite"
+import React, { CSSProperties, useEffect, useMemo, useRef, useState } from "react"
+import { useResizeDetector } from "react-resize-detector"
 import PlayIcon from "../../assets/icons/icon-play.svg"
 import PauseIcon from "../../assets/icons/icon-pause.svg"
 import { SliderAxisLayout } from "./slider-layout"
@@ -11,6 +11,7 @@ import { Axis } from "../axis/components/axis"
 import { AxisProviderContext } from "../axis/hooks/use-axis-provider-context"
 import { AxisLayoutContext } from "../axis/models/axis-layout-context"
 import { InstanceIdContext, useNextInstanceId } from "../../hooks/use-instance-id-context"
+import { isAliveSafe } from "../../utilities/mst-utils"
 import { ITileBaseProps } from "../tiles/tile-base-props"
 import { CodapSliderThumb } from "./slider-thumb"
 import { EditableSliderValue } from "./editable-slider-value"
@@ -20,19 +21,14 @@ import './slider.scss'
 const kAxisMargin = 30
 
 export const SliderComponent = observer(function SliderComponent({ tile } : ITileBaseProps) {
-  const sliderModel = isSliderModel(tile?.content) ? tile?.content : undefined
+  const sliderModel = isAliveSafe(tile?.content) && isSliderModel(tile?.content) ? tile?.content : undefined
   const instanceId = useNextInstanceId("slider")
   const layout = useMemo(() => new SliderAxisLayout(), [])
   const {width, height, ref: sliderRef} = useResizeDetector()
   const [running, setRunning] = useState(false)
   const animationRef = useRef(false)
   const multiScale = layout.getAxisMultiScale("bottom")
 
-  const axisProvider = useMemo(() => ({
-    getAxis: () => sliderModel?.axis,
-    getNumericAxis: () => sliderModel?.axis
-  }), [sliderModel?.axis])
-
   // width and positioning
   useEffect(() => {
     if ((width != null) && (height != null)) {
@@ -53,15 +49,15 @@ export const SliderComponent = observer(function SliderComponent({ tile } : ITil
     appRef.current = document.querySelector(".app")
   }, [])
 
-  if (!isSliderModel(sliderModel)) return null
+  if (!sliderModel) return null
 
   const handleSliderNameInput = (name: string) => {
     sliderModel.setName(name)
   }
 
   return (
     <InstanceIdContext.Provider value={instanceId}>
-      <AxisProviderContext.Provider value={axisProvider}>
+      <AxisProviderContext.Provider value={sliderModel}>
         <AxisLayoutContext.Provider value={layout}>
           <div className={kSliderClass} ref={sliderRef}>
             <Flex className="slider-control">

v3/src/components/slider/slider-model.ts

@@ -1,6 +1,6 @@
 import { reaction } from "mobx"
 import { addDisposer, Instance, types} from "mobx-state-tree"
-import { NumericAxisModel } from "../axis/models/axis-model"
+import { INumericAxisModel, NumericAxisModel } from "../axis/models/axis-model"
 import { GlobalValue } from "../../models/global/global-value"
 import { IGlobalValueManager, kGlobalValueManagerType } from "../../models/global/global-value-manager"
 import { applyUndoableAction } from "../../models/history/apply-undoable-action"
@@ -32,6 +32,12 @@ export const SliderModel = TileContentModel
     get value() {
       return self.globalValue.value
     },
+    getAxis(): INumericAxisModel {
+      return self.axis
+    },
+    getNumericAxis(): INumericAxisModel {
+      return self.axis
+    },
     get domain() {
       return self.axis.domain
     },
@@ -118,12 +124,9 @@ export const SliderModel = TileContentModel
         }, { name: "SliderModel [sharedModelManager]", fireImmediately: true }
       ))
     },
-    beforeDestroy() {
-      // destroying the slider component removes the underlying global value
-      // unfortunately, removing the global value here invalidates the reference and leads to MST warnings.
-      // TODO: figure out a mechanism to remove the slider model and its global value safely
-      // for now, the global value stays in the registry when the slider model is removed
-      // self.globalValue && self.globalValueManager?.removeValue(self.globalValue)
+    destroyGlobalValue() {
+      // the underlying global value should be removed when the slider model is destroyed
+      self.globalValue && self.globalValueManager?.removeValue(self.globalValue)
     },
     updateAfterSharedModelChanges(sharedModel?: ISharedModel) {
       // nothing to do

v3/src/components/slider/slider-thumb.tsx

@@ -2,6 +2,7 @@ import { clsx } from "clsx"
 import { observer } from "mobx-react-lite"
 import React, {CSSProperties, useEffect, useState, useRef} from "react"
 import { ISliderModel } from "./slider-model"
+import { isAliveSafe } from "../../utilities/mst-utils"
 import { useAxisLayoutContext } from "../axis/models/axis-layout-context"
 import { useSliderAnimation } from "./use-slider-animation"
 import ThumbIcon from "../../assets/icons/icon-thumb.svg"
@@ -18,20 +19,22 @@ interface IProps {
 // offset from left edge of thumb to center of thumb
 const kThumbOffset = 10
 
-export const CodapSliderThumb = observer(function CodapSliderThumb({sliderContainer, sliderModel,
-    running, setRunning} : IProps) {
+export const CodapSliderThumb = observer(function CodapSliderThumb({
+  sliderContainer, sliderModel: _sliderModel, running, setRunning
+} : IProps) {
+  const sliderModel = isAliveSafe(_sliderModel) ? _sliderModel : undefined
   const layout = useAxisLayoutContext()
   const scale = layout.getAxisMultiScale("bottom")
   const [isDragging, setIsDragging] = useState(false)
   // offset from center of thumb to pointerDown
   const downOffset = useRef(0)
-  const thumbPos = (scale?.getScreenCoordinate({cell: 0, data: sliderModel.value}) ?? 0) - kThumbOffset
+  const thumbPos = (scale?.getScreenCoordinate({cell: 0, data: sliderModel?.value ?? 0}) ?? 0) - kThumbOffset
   const thumbStyle: CSSProperties = {
     left: thumbPos
   }
 
   // forces thumbnail to rerender when axis bounds change
-  sliderModel.axis.domain // eslint-disable-line no-unused-expressions
+  sliderModel?.axis.domain // eslint-disable-line no-unused-expressions
 
   useEffect(() => {
     const containerX = sliderContainer?.getBoundingClientRect().x
@@ -46,7 +49,7 @@ export const CodapSliderThumb = observer(function CodapSliderThumb({sliderContai
     const handlePointerMove = (e: PointerEvent) => {
       const sliderValue = getSliderValueFromEvent(e)
       if (sliderValue != null) {
-        sliderModel.setDynamicValue(sliderValue)
+        sliderModel?.setDynamicValue(sliderValue)
       }
       e.preventDefault()
       e.stopImmediatePropagation()
@@ -55,7 +58,7 @@ export const CodapSliderThumb = observer(function CodapSliderThumb({sliderContai
     const handlePointerUp = (e: PointerEvent) => {
       const sliderValue = getSliderValueFromEvent(e)
       if (sliderValue != null) {
-        sliderModel.applyUndoableAction(
+        sliderModel?.applyUndoableAction(
           () => sliderModel.setValue(sliderValue),
           "DG.Undo.slider.change", "DG.Redo.slider.change")
       }

v3/src/components/slider/slider-title-bar.tsx

@@ -1,14 +1,30 @@
-import React from "react"
+import { addDisposer } from "mobx-state-tree"
+import React, { useCallback } from "react"
 import { observer } from "mobx-react-lite"
 import { ComponentTitleBar  } from "../component-title-bar"
+import { isAliveSafe } from "../../utilities/mst-utils"
 import t from "../../utilities/translation/translate"
 import { ITileTitleBarProps } from "../tiles/tile-base-props"
 import { isSliderModel } from "./slider-model"
 
-export const SliderTitleBar = observer(function SliderTitleBar({ tile, ...others }: ITileTitleBarProps) {
-  const sliderModel = isSliderModel(tile?.content) ? tile?.content : undefined
-  const getTitle = () => tile?.title || sliderModel?.name || t("DG.DocumentController.sliderTitle")
+export const SliderTitleBar = observer(function SliderTitleBar({ tile, onCloseTile, ...others }: ITileTitleBarProps) {
+  const { content } = tile || {}
+
+  const getTitle = useCallback(() => {
+    const { title } = tile || {}
+    const sliderModel = isAliveSafe(content) && isSliderModel(content) ? content : undefined
+    const { name } = sliderModel || {}
+    return title || name || t("DG.DocumentController.sliderTitle")
+  }, [content, tile])
+
+  const handleCloseTile = useCallback((tileId: string) => {
+    const sliderModel = isAliveSafe(content) && isSliderModel(content) ? content : undefined
+    // when tile is closed by user, destroy the underlying global value as well
+    sliderModel && addDisposer(sliderModel, () => sliderModel.destroyGlobalValue())
+    onCloseTile?.(tileId)
+  }, [content, onCloseTile])
+
   return (
-    <ComponentTitleBar tile={tile} getTitle={getTitle} {...others} />
+    <ComponentTitleBar tile={tile} getTitle={getTitle} onCloseTile={handleCloseTile} {...others} />
   )
 })

v3/src/components/slider/slider-types.ts

@@ -21,4 +21,10 @@ export const kDefaultAnimationMode = "onceOnly"
 
 export const kDefaultAnimationRate = 20 // frames/second
 
+export const kAnimationDefaults = {
+  animationMode: kDefaultAnimationMode,
+  animationDirection: kDefaultAnimationDirection,
+  animationRate: kDefaultAnimationRate
+}
+
 export type FixValueFn = (value: number) => number