Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: crash on delete DataSet #1531

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions v3/src/components/case-tile-common/hide-show-menu-list.tsx
Original file line number Diff line number Diff line change
@@ -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"
Expand All @@ -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(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
19 changes: 11 additions & 8 deletions v3/src/models/data/filtered-cases.ts
Original file line number Diff line number Diff line change
@@ -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"
Expand All @@ -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<string>()
private onActionDisposers: Array<() => void>
private disposers: Array<() => void>

constructor({ source, collectionID, casesArrayNumber = 0, filter, onSetCaseValues }: IProps) {
this.source = source
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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__)
}
Expand Down
8 changes: 5 additions & 3 deletions v3/src/utilities/mst-autorun.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,18 @@ import { SetRequired } from "type-fest"
MobX `autorun` API, except that passing a name is required.
*/
type IAutorunOptionsWithName = SetRequired<IAutorunOptions, "name">
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
function disposer() {
_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
}
7 changes: 4 additions & 3 deletions v3/src/utilities/mst-reaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,16 @@ type IReactionOptionsWithName<T, fireImmediately extends boolean> =

export function mstReaction<T, fireImmediately extends boolean>(
accessor: () => T, effect: (args: T) => void,
options: IReactionOptionsWithName<T, fireImmediately>, model: IAnyStateTreeNode) {
options: IReactionOptionsWithName<T, fireImmediately>, models: IAnyStateTreeNode | IAnyStateTreeNode[]) {
// install reaction
let _disposer: IReactionDisposer | undefined = reaction(accessor, effect, options)
// returned disposer prevents MobX disposer from being called multiple times
function disposer() {
_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
}
Loading