From 7cfddb46f8260ea9da7e5025f548a39c479d3c55 Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Wed, 2 Oct 2024 12:46:04 -0700 Subject: [PATCH] fix: crash on delete DataSet --- .../case-tile-common/hide-show-menu-list.tsx | 3 +++ .../count/count-adornment-component.tsx | 2 +- .../models/graph-data-configuration-model.ts | 2 +- v3/src/models/data/filtered-cases.ts | 19 +++++++++++-------- v3/src/utilities/mst-autorun.ts | 8 +++++--- v3/src/utilities/mst-reaction.ts | 7 ++++--- 6 files changed, 25 insertions(+), 16 deletions(-) diff --git a/v3/src/components/case-tile-common/hide-show-menu-list.tsx b/v3/src/components/case-tile-common/hide-show-menu-list.tsx index 48915d5473..fc0e1b7fbb 100644 --- a/v3/src/components/case-tile-common/hide-show-menu-list.tsx +++ b/v3/src/components/case-tile-common/hide-show-menu-list.tsx @@ -1,5 +1,6 @@ import { useDisclosure } from "@chakra-ui/react" import { observer } from "mobx-react-lite" +import { isAlive } from "mobx-state-tree" import React from "react" import { useCaseMetadata } from "../../hooks/use-case-metadata" import { useDataSetContext } from "../../hooks/use-data-set-context" @@ -13,6 +14,8 @@ export const HideShowMenuList = observer(function HideShowMenuList() { const caseMetadata = useCaseMetadata() const formulaModal = useDisclosure() + if (data && !isAlive(data)) return null + const handleSetAsideCases = (itemIds: string[], deselect: boolean) => { if (data && itemIds.length) { data.applyModelChange(() => { diff --git a/v3/src/components/graph/adornments/count/count-adornment-component.tsx b/v3/src/components/graph/adornments/count/count-adornment-component.tsx index bd9dcc45e2..541be23f5b 100644 --- a/v3/src/components/graph/adornments/count/count-adornment-component.tsx +++ b/v3/src/components/graph/adornments/count/count-adornment-component.tsx @@ -146,7 +146,7 @@ export const CountAdornment = observer(function CountAdornment(props: IAdornment getAxisDomains(xAxis, yAxis) subPlotRegionBoundariesRef.current = subPlotRegionBoundaries() plotCaseCounts() - }, { name: "Count.refreshBoundariesAndCaseCounts" }, model) + }, { name: "Count.refreshBoundariesAndCaseCounts" }, [model, xAxis, yAxis]) }, [model, plotCaseCounts, subPlotRegionBoundaries, xAxis, yAxis]) useEffect(function refreshOnSubPlotRegionChange() { diff --git a/v3/src/components/graph/models/graph-data-configuration-model.ts b/v3/src/components/graph/models/graph-data-configuration-model.ts index a5390db2c9..64109690b8 100644 --- a/v3/src/components/graph/models/graph-data-configuration-model.ts +++ b/v3/src/components/graph/models/graph-data-configuration-model.ts @@ -695,7 +695,7 @@ export const GraphDataConfigurationModel = DataConfigurationModel self.filteredCases.pop()?.destroy() } // add any required filteredCases - while (self.filteredCases.length < filteredCasesRequired) { + while (self.dataset && self.filteredCases.length < filteredCasesRequired) { self._addNewFilteredCases() } }, { name: "GraphDataConfigurationModel yAttrDescriptions reaction", equals: comparer.structural } diff --git a/v3/src/models/data/filtered-cases.ts b/v3/src/models/data/filtered-cases.ts index b893c51a3e..5e7be7e894 100644 --- a/v3/src/models/data/filtered-cases.ts +++ b/v3/src/models/data/filtered-cases.ts @@ -1,5 +1,5 @@ import { action, computed, makeObservable, observable } from "mobx" -import { ISerializedActionCall } from "mobx-state-tree" +import { addDisposer, ISerializedActionCall } from "mobx-state-tree" import { typedId } from "../../utilities/js-utils" import { onAnyAction } from "../../utilities/mst-utils" import { IDataSet } from "./data-set" @@ -25,14 +25,14 @@ interface IProps { export class FilteredCases { public readonly id = typedId("FICA") - private source: IDataSet + private source?: IDataSet private collectionID: string | undefined private casesArrayNumber: number @observable private filter?: FilterFn private onSetCaseValues?: OnSetCaseValuesFn private prevCaseIdSet = new Set() - private onActionDisposers: Array<() => void> + private disposers: Array<() => void> constructor({ source, collectionID, casesArrayNumber = 0, filter, onSetCaseValues }: IProps) { this.source = source @@ -43,14 +43,15 @@ export class FilteredCases { makeObservable(this) - this.onActionDisposers = [ + this.disposers = [ + addDisposer(source, () => this.source = undefined), onAnyAction(this.source, this.handleBeforeAction, { attachAfter: false }), // runs before the action onAnyAction(this.source, this.handleAction, { attachAfter: true }), // runs after the action ] } destroy() { - this.onActionDisposers.forEach(disposer => disposer()) + this.disposers.forEach(disposer => disposer()) } @computed @@ -61,10 +62,12 @@ export class FilteredCases { // cases when cases are inserted, but that would be more code to write/maintain and running // the filter function over an array of cases should be quick so rather than succumb to the // temptation of premature optimization, let's wait to see whether it becomes a bottleneck. - const rawCases = this.collectionID ? this.source.getCasesForCollection(this.collectionID) : this.source.items + const rawCases = this.collectionID + ? this.source?.getCasesForCollection(this.collectionID) ?? [] + : this.source?.items ?? [] return rawCases .map(aCase => aCase.__id__) - .filter(id => !this.filter || this.filter(this.source, id, this.casesArrayNumber)) + .filter(id => !this.filter || (this.source && this.filter(this.source, id, this.casesArrayNumber))) } @computed @@ -124,7 +127,7 @@ export class FilteredCases { cases.forEach(aCase => { // compare the pre-/post-change filter state of the affected cases const wasIncluded = this.prevCaseIdSet.has(aCase.__id__) - const nowIncluded = !this.filter || this.filter(this.source, aCase.__id__) + const nowIncluded = !this.filter || (this.source && this.filter(this.source, aCase.__id__)) if (wasIncluded === nowIncluded) { changed.push(aCase.__id__) } diff --git a/v3/src/utilities/mst-autorun.ts b/v3/src/utilities/mst-autorun.ts index 53cf16a86a..464e059b70 100644 --- a/v3/src/utilities/mst-autorun.ts +++ b/v3/src/utilities/mst-autorun.ts @@ -9,8 +9,9 @@ import { SetRequired } from "type-fest" MobX `autorun` API, except that passing a name is required. */ type IAutorunOptionsWithName = SetRequired +type IAnyModels = IAnyStateTreeNode | IAnyStateTreeNode[] -export function mstAutorun(fn: () => void, options: IAutorunOptionsWithName, model: IAnyStateTreeNode) { +export function mstAutorun(fn: () => void, options: IAutorunOptionsWithName, models: IAnyModels) { // install autorun let _disposer: IReactionDisposer | undefined = autorun(fn, options) // returned disposer prevents MobX disposer from being called multiple times @@ -18,7 +19,8 @@ export function mstAutorun(fn: () => void, options: IAutorunOptionsWithName, mod _disposer?.() _disposer = undefined } - // dispose of autorun if the model it depends on is destroyed - addDisposer(model, disposer) + // dispose of autorun if the model(s) it depends on is/are destroyed + const _models = Array.isArray(models) ? models : [models] + _models.forEach(model => model && addDisposer(model, disposer)) return disposer } diff --git a/v3/src/utilities/mst-reaction.ts b/v3/src/utilities/mst-reaction.ts index 36307f3d51..c272a48650 100644 --- a/v3/src/utilities/mst-reaction.ts +++ b/v3/src/utilities/mst-reaction.ts @@ -13,7 +13,7 @@ type IReactionOptionsWithName = export function mstReaction( accessor: () => T, effect: (args: T) => void, - options: IReactionOptionsWithName, model: IAnyStateTreeNode) { + options: IReactionOptionsWithName, models: IAnyStateTreeNode | IAnyStateTreeNode[]) { // install reaction let _disposer: IReactionDisposer | undefined = reaction(accessor, effect, options) // returned disposer prevents MobX disposer from being called multiple times @@ -21,7 +21,8 @@ export function mstReaction( _disposer?.() _disposer = undefined } - // dispose of the reaction if the model it depends on is destroyed - addDisposer(model, disposer) + // dispose of the reaction if the model(s) it depends on is/are destroyed + const _models = Array.isArray(models) ? models : [models] + _models.forEach(model => model && addDisposer(model, disposer)) return disposer }