From 8af2ff85f88e9e2283420df8fcdac2841c25e1ac Mon Sep 17 00:00:00 2001
From: Kirk Swenson <kswenson@users.noreply.github.com>
Date: Fri, 13 Sep 2024 18:03:20 -0700
Subject: [PATCH 1/4] feat: infrastructure for filter formula

---
 .../edit-filter-formula-modal.tsx             | 89 +++++++++++++++++++
 .../case-tile-common/hide-show-menu-list.tsx  | 23 ++++-
 v3/src/models/data/data-set.ts                | 25 +++++-
 v3/src/utilities/translation/lang/en-US.json5 |  4 +
 4 files changed, 138 insertions(+), 3 deletions(-)
 create mode 100644 v3/src/components/case-tile-common/edit-filter-formula-modal.tsx

diff --git a/v3/src/components/case-tile-common/edit-filter-formula-modal.tsx b/v3/src/components/case-tile-common/edit-filter-formula-modal.tsx
new file mode 100644
index 0000000000..7f9ba48e6d
--- /dev/null
+++ b/v3/src/components/case-tile-common/edit-filter-formula-modal.tsx
@@ -0,0 +1,89 @@
+import {
+  Button, FormControl, FormLabel, ModalBody, ModalCloseButton, ModalFooter, ModalHeader,
+  Textarea, Tooltip
+} from "@chakra-ui/react"
+import React, { useEffect, useState } from "react"
+import { observer } from "mobx-react-lite"
+import { useDataSetContext } from "../../hooks/use-data-set-context"
+import { t } from "../../utilities/translation/translate"
+import { CodapModal } from "../codap-modal"
+
+interface IProps {
+  isOpen: boolean
+  onClose: () => void
+}
+
+export const EditFilterFormulaModal = observer(function EditFormulaModal({ isOpen, onClose }: IProps) {
+  const data = useDataSetContext()
+  const [formula, setFormula] = useState("")
+
+  useEffect(() => {
+    setFormula(data?.filterFormula?.display || "")
+  }, [data?.filterFormula?.display])
+
+  const closeModal = () => {
+    onClose()
+  }
+
+  function applyFilterFormula() {
+    data?.applyModelChange(() => {
+      data.setFilterFormula(formula)
+    }, {
+      undoStringKey: "V3.Undo.hideShowMenu.changeFilterFormula",
+      redoStringKey: "V3.Redo.hideShowMenu.changeFilterFormula"
+    })
+    closeModal()
+  }
+
+  const handleFormulaChange = (e: React.ChangeEvent<HTMLTextAreaElement>) => setFormula(e.target.value)
+
+  const buttons = [{
+    label: t("DG.AttrFormView.cancelBtnTitle"),
+    tooltip: t("DG.AttrFormView.cancelBtnTooltip"),
+    onClick: closeModal
+  }, {
+    label: t("DG.AttrFormView.applyBtnTitle"),
+    onClick: applyFilterFormula,
+    default: true
+  }]
+
+  return (
+    <CodapModal
+      isOpen={isOpen}
+      onClose={onClose}
+      modalWidth={"400px"}
+      modalHeight={"180px"}
+    >
+      <ModalHeader h="30" className="codap-modal-header" fontSize="md" data-testid="codap-modal-header">
+        <div className="codap-modal-icon-container" />
+        <div className="codap-header-title" />
+        <ModalCloseButton onClick={onClose} data-testid="modal-close-button" />
+      </ModalHeader>
+      <ModalBody>
+        <FormControl display="flex" flexDirection="column" className="formula-form-control">
+          <FormLabel>{t("DG.AttrFormView.formulaPrompt")}
+            <Textarea size="xs" value={formula} onChange={handleFormulaChange}
+              onKeyDown={(e) => e.stopPropagation()} data-testid="attr-formula-input" />
+          </FormLabel>
+        </FormControl>
+      </ModalBody>
+      <ModalFooter mt="-5">
+        {
+          buttons.map((b, idx) => {
+            const key = `${idx}-${b.label}`
+            return (
+              <Tooltip key={idx} label={b.tooltip} h="20px" fontSize="12px" color="white" openDelay={1000}
+                placement="bottom" bottom="15px" left="15px" data-testid="modal-tooltip"
+              >
+                <Button key={key} size="xs" variant={`${b.default ? "default" : ""}`} ml="5" onClick={b.onClick}
+                      _hover={{backgroundColor: "#72bfca", color: "white"}} data-testid={`${b.label}-button`}>
+                  {b.label}
+                </Button>
+              </Tooltip>
+            )
+          })
+        }
+      </ModalFooter>
+    </CodapModal>
+  )
+})
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 b631dc7aca..ce39cefabb 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,14 +1,17 @@
+import { useDisclosure } from "@chakra-ui/react"
 import { observer } from "mobx-react-lite"
 import React from "react"
 import { useCaseMetadata } from "../../hooks/use-case-metadata"
 import { useDataSetContext } from "../../hooks/use-data-set-context"
 import { hideAttributeNotification } from "../../models/data/data-set-notifications"
 import { t } from "../../utilities/translation/translate"
+import { EditFilterFormulaModal } from "./edit-filter-formula-modal"
 import { IMenuItem, StdMenuList } from "./std-menu-list"
 
 export const HideShowMenuList = observer(function HideShowMenuList() {
   const data = useDataSetContext()
   const caseMetadata = useCaseMetadata()
+  const formulaModal = useDisclosure()
 
   const handleSetAsideCases = (itemIds: string[], deselect: boolean) => {
     if (data && itemIds.length) {
@@ -22,6 +25,14 @@ export const HideShowMenuList = observer(function HideShowMenuList() {
     }
   }
 
+  const handleEditFormulaOpen = () => {
+    formulaModal.onOpen()
+  }
+
+  const handleEditFormulaClose = () => {
+    formulaModal.onClose()
+  }
+
   const itemCount = data?.items.length ?? 0
   const selectionCount = data?.selection.size ?? 0
   const setAsideCount = data?.hiddenItemIds.length ?? 0
@@ -67,6 +78,13 @@ export const HideShowMenuList = observer(function HideShowMenuList() {
         }
       }
     },
+    {
+      itemKey: !data?.filterFormula?.empty
+                ? "V3.hideShowMenu.editFilterFormula"
+                : "V3.hideShowMenu.addFilterFormula",
+      isEnabled: () => !!data,
+      handleClick: handleEditFormulaOpen
+    },
     {
       itemKey: "DG.Inspector.attributes.showAllHiddenAttributesPlural",
       isEnabled: () => hiddenAttributeCount > 0,
@@ -98,6 +116,9 @@ export const HideShowMenuList = observer(function HideShowMenuList() {
   ]
 
   return (
-    <StdMenuList data-testid="hide-show-menu-list" menuItems={menuItems} />
+    <>
+      <StdMenuList data-testid="hide-show-menu-list" menuItems={menuItems} />
+      <EditFilterFormulaModal isOpen={formulaModal.isOpen} onClose={handleEditFormulaClose} />
+    </>
   )
 })
diff --git a/v3/src/models/data/data-set.ts b/v3/src/models/data/data-set.ts
index d8675a8f8d..521c6ffa61 100644
--- a/v3/src/models/data/data-set.ts
+++ b/v3/src/models/data/data-set.ts
@@ -58,6 +58,7 @@ import {
 } from "./data-set-types"
 // eslint-disable-next-line import/no-cycle
 import { isLegacyDataSetSnap, isOriginalDataSetSnap, isTempDataSetSnap } from "./data-set-conversion"
+import { Formula } from "../formula/formula"
 import { applyModelChange } from "../history/apply-model-change"
 import { withoutUndo } from "../history/without-undo"
 import { kAttrIdPrefix, kItemIdPrefix, typeV3Id, v3Id } from "../../utilities/codap-utils"
@@ -139,7 +140,8 @@ export const DataSet = V2Model.named("DataSet").props({
   // for serialization only, not for dynamic selection tracking
   snapSelection: types.array(types.string),
   // hidden by user, e.g. set-aside in CODAP
-  hiddenItemIds: types.array(types.string)
+  hiddenItemIds: types.array(types.string),
+  filterFormula: types.maybe(Formula)
 })
 .volatile(self => ({
   // map from attribute name to attribute id
@@ -274,8 +276,19 @@ export const DataSet = V2Model.named("DataSet").props({
     return caseInfo?.childItemIds.every(itemId => self.hiddenItemIdsSet.has(itemId)) ??
             self.hiddenItemIdsSet.has(caseOrItemId)
   },
-  get itemIds() {
+  // ids of items that have not been hidden (set aside) by user
+  get _unhiddenItemIds() {
     return self._itemIds.filter(itemId => !self.hiddenItemIdsSet.has(itemId))
+  },
+  isPassedByFilter(itemId: string) {
+    // evaluate filter for item (or retrieve cached value)
+    return true
+  }
+}))
+.views(self => ({
+  // ids of items that have not been hidden (set aside) or filtered by user
+  get itemIds() {
+    return self._unhiddenItemIds.filter(itemId => self.isPassedByFilter(itemId))
   }
 }))
 .views(self => ({
@@ -558,6 +571,14 @@ export const DataSet = V2Model.named("DataSet").props({
       // show all hidden cases/items
       self.hiddenItemIds.clear()
     }
+  },
+  setFilterFormula(display: string) {
+    if (!self.filterFormula) {
+      self.filterFormula = Formula.create({ display })
+    }
+    else {
+      self.filterFormula.setDisplayExpression(display)
+    }
   }
 }))
 .actions(self => ({
diff --git a/v3/src/utilities/translation/lang/en-US.json5 b/v3/src/utilities/translation/lang/en-US.json5
index 341605d40f..dc54634b14 100644
--- a/v3/src/utilities/translation/lang/en-US.json5
+++ b/v3/src/utilities/translation/lang/en-US.json5
@@ -40,6 +40,10 @@
     "V3.Redo.hideShowMenu.setAsideCases": "Redo setting aside cases",
     "V3.Undo.hideShowMenu.restoreSetAsideCases": "Undo restoring set aside cases",
     "V3.Redo.hideShowMenu.restoreSetAsideCases": "Redo restoring set aside cases",
+    "V3.hideShowMenu.addFilterFormula": "Add Filter Formula...",
+    "V3.hideShowMenu.editFilterFormula": "Edit Filter Formula...",
+    "V3.Undo.hideShowMenu.changeFilterFormula": "Undo changing filter formula",
+    "V3.Redo.hideShowMenu.changeFilterFormula": "Redo changing filter formula",
 
     "V3.Inspector.rescale.noRescale.toolTip": "All the data are already visible",
     "V3.Inspector.rescale.casePlot.toolTip": "Move the points to new random positions",

From 6a34f8741b2b39b439f74d9c061bf21aafc2f6b1 Mon Sep 17 00:00:00 2001
From: pjanik <janikpiotrek@gmail.com>
Date: Thu, 19 Sep 2024 17:40:14 +0200
Subject: [PATCH 2/4] feat: add dataset filter formula support, handle errors +
 tests [PT-187799307]

---
 .../edit-filter-formula-modal.tsx             |   8 +-
 v3/src/models/data/attribute.ts               |   5 +
 v3/src/models/data/data-set.ts                |  88 +++++++++---
 .../models/document/create-document-model.ts  |   4 +-
 .../formula/filter-formula-adapter.test.ts    | 127 ++++++++++++++++++
 .../models/formula/filter-formula-adapter.ts  | 115 ++++++++++++++++
 6 files changed, 328 insertions(+), 19 deletions(-)
 create mode 100644 v3/src/models/formula/filter-formula-adapter.test.ts
 create mode 100644 v3/src/models/formula/filter-formula-adapter.ts

diff --git a/v3/src/components/case-tile-common/edit-filter-formula-modal.tsx b/v3/src/components/case-tile-common/edit-filter-formula-modal.tsx
index 7f9ba48e6d..d9e666090a 100644
--- a/v3/src/components/case-tile-common/edit-filter-formula-modal.tsx
+++ b/v3/src/components/case-tile-common/edit-filter-formula-modal.tsx
@@ -58,8 +58,14 @@ export const EditFilterFormulaModal = observer(function EditFormulaModal({ isOpe
         <div className="codap-modal-icon-container" />
         <div className="codap-header-title" />
         <ModalCloseButton onClick={onClose} data-testid="modal-close-button" />
-      </ModalHeader>
+      </ModalHeader>=
       <ModalBody>
+        {
+          data?.filterFormulaError &&
+          <div className="formula-error" style={{ background: "rgb(254, 224, 228)", fontSize: "0.8em" }}>
+            {data.filterFormulaError}
+          </div>
+        }
         <FormControl display="flex" flexDirection="column" className="formula-form-control">
           <FormLabel>{t("DG.AttrFormView.formulaPrompt")}
             <Textarea size="xs" value={formula} onChange={handleFormulaChange}
diff --git a/v3/src/models/data/attribute.ts b/v3/src/models/data/attribute.ts
index ddf1668dfc..78781c50e8 100644
--- a/v3/src/models/data/attribute.ts
+++ b/v3/src/models/data/attribute.ts
@@ -307,6 +307,11 @@ export const Attribute = V2Model.named("Attribute").props({
       }
     }
     else {
+      // Note that this generates a warning that the formula is unregistered in an unexpected way.
+      // Originally, the formula manager was designed to handle only empty formulas. However,
+      // https://github.com/concord-consortium/codap/pull/1124 changed the behavior, and formula objects now get deleted
+      // from the parent model.
+      // TODO: handle it better, or remove the warning from the formula manager if it doesn't cause any issues
       this.clearFormula()
     }
   },
diff --git a/v3/src/models/data/data-set.ts b/v3/src/models/data/data-set.ts
index 521c6ffa61..9f0fff2b27 100644
--- a/v3/src/models/data/data-set.ts
+++ b/v3/src/models/data/data-set.ts
@@ -58,7 +58,7 @@ import {
 } from "./data-set-types"
 // eslint-disable-next-line import/no-cycle
 import { isLegacyDataSetSnap, isOriginalDataSetSnap, isTempDataSetSnap } from "./data-set-conversion"
-import { Formula } from "../formula/formula"
+import { Formula, IFormula } from "../formula/formula"
 import { applyModelChange } from "../history/apply-model-change"
 import { withoutUndo } from "../history/without-undo"
 import { kAttrIdPrefix, kItemIdPrefix, typeV3Id, v3Id } from "../../utilities/codap-utils"
@@ -164,7 +164,10 @@ export const DataSet = V2Model.named("DataSet").props({
   transactionCount: 0,
   // the id of the interactive frame handling this dataset
   // used by the Collaborative plugin
-  managingControllerId: ""
+  managingControllerId: "",
+  // cached result of filter formula evaluation for each item ID
+  filterFormulaResults: observable.map<string, boolean>(),
+  filterFormulaError: ""
 }))
 .extend(self => {
   const _validationCount = observable.box<number>(0)
@@ -270,25 +273,36 @@ export const DataSet = V2Model.named("DataSet").props({
   }
   return snap
 })
+.views(self => ({
+  isItemSetAside(itemId: string) {
+    return self.hiddenItemIdsSet.has(itemId)
+  },
+  isItemFilteredOut(itemId: string) {
+    // Note that if itemResult is undefined, it means the item has not been filtered out (e.g., there may not be any
+    /// filter formula), so it should be considered as having passed the filter.
+    return self.filterFormulaResults.get(itemId) === false
+  }
+}))
+.views(self => ({
+  isItemHidden(itemId: string) {
+    return self.isItemSetAside(itemId) || self.isItemFilteredOut(itemId)
+  }
+}))
 .views(self => ({
   isCaseOrItemHidden(caseOrItemId: string) {
     const caseInfo = self.caseInfoMap.get(caseOrItemId)
-    return caseInfo?.childItemIds.every(itemId => self.hiddenItemIdsSet.has(itemId)) ??
-            self.hiddenItemIdsSet.has(caseOrItemId)
-  },
-  // ids of items that have not been hidden (set aside) by user
-  get _unhiddenItemIds() {
-    return self._itemIds.filter(itemId => !self.hiddenItemIdsSet.has(itemId))
-  },
-  isPassedByFilter(itemId: string) {
-    // evaluate filter for item (or retrieve cached value)
-    return true
+    return caseInfo?.childItemIds.every(itemId => self.isItemHidden(itemId)) ??
+            self.isItemHidden(caseOrItemId)
   }
 }))
 .views(self => ({
+  // ids of items that have not been hidden (set aside) by user
+  get itemsNotSetAside() {
+    return self._itemIds.filter(itemId => !self.isItemSetAside(itemId))
+  },
   // ids of items that have not been hidden (set aside) or filtered by user
   get itemIds() {
-    return self._unhiddenItemIds.filter(itemId => self.isPassedByFilter(itemId))
+    return self._itemIds.filter(itemId => !self.isItemHidden(itemId))
   }
 }))
 .views(self => ({
@@ -303,6 +317,9 @@ export const DataSet = V2Model.named("DataSet").props({
   },
   get items(): readonly IItem[] {
     return self.itemIds.map(id => ({ __id__: id }))
+  },
+  get hasFilterFormula() {
+    return !!self.filterFormula && !self.filterFormula.empty
   }
 }))
 .views(self => ({
@@ -534,6 +551,14 @@ export const DataSet = V2Model.named("DataSet").props({
     return self.collections.find(coll => coll.hasCase(caseId))
   }
 }))
+.actions(self => ({
+  clearFilterFormula() {
+    self.filterFormula = undefined
+    self.filterFormulaResults.clear()
+    self.filterFormulaError = ""
+    self.invalidateCases()
+  }
+}))
 .actions(self => ({
   hideCasesOrItems(caseOrItemIds: string[]) {
     caseOrItemIds.forEach(id => {
@@ -573,12 +598,33 @@ export const DataSet = V2Model.named("DataSet").props({
     }
   },
   setFilterFormula(display: string) {
-    if (!self.filterFormula) {
-      self.filterFormula = Formula.create({ display })
+    if (display) {
+      if (!self.filterFormula) {
+        self.filterFormula = Formula.create({ display })
+      }
+      else {
+        self.filterFormula.setDisplayExpression(display)
+      }
+    } else {
+      // Note that this generates a warning that the formula is unregistered in an unexpected way.
+      // Originally, the formula manager was designed to handle only empty formulas. However,
+      // https://github.com/concord-consortium/codap/pull/1124 changed the behavior, and formula objects now get deleted
+      // from the parent model.
+      // TODO: handle it better, or remove the warning from the formula manager if it doesn't cause any issues
+      self.clearFilterFormula()
     }
-    else {
-      self.filterFormula.setDisplayExpression(display)
+  },
+  updateFilterFormulaResults(filterFormulaResults: { itemId: string, result: boolean }[], { replaceAll = false }) {
+    if (replaceAll) {
+      self.filterFormulaResults.clear()
     }
+    filterFormulaResults.forEach(({ itemId, result }) => {
+      self.filterFormulaResults.set(itemId, result)
+    })
+    self.invalidateCases()
+  },
+  setFilterFormulaError(error: string) {
+    self.filterFormulaError = error
   }
 }))
 .actions(self => ({
@@ -1256,3 +1302,11 @@ export const DataSet = V2Model.named("DataSet").props({
 
 export interface IDataSet extends Instance<typeof DataSet> {}
 export interface IDataSetSnapshot extends SnapshotIn<typeof DataSet> {}
+
+export interface IDataSetWithFilterFormula extends IDataSet {
+  filterFormula: IFormula
+}
+
+export function isFilterFormulaDataSet(dataSet?: IDataSet): dataSet is IDataSetWithFilterFormula {
+  return !!dataSet?.hasFilterFormula
+}
diff --git a/v3/src/models/document/create-document-model.ts b/v3/src/models/document/create-document-model.ts
index 18a2a7fb10..12ad157607 100644
--- a/v3/src/models/document/create-document-model.ts
+++ b/v3/src/models/document/create-document-model.ts
@@ -12,6 +12,7 @@ import { AttributeFormulaAdapter } from "../formula/attribute-formula-adapter"
 import { PlottedValueFormulaAdapter } from "../formula/plotted-value-formula-adapter"
 import { PlottedFunctionFormulaAdapter } from "../formula/plotted-function-formula-adapter"
 import { ISharedDataSet, SharedDataSet, kSharedDataSetType } from "../shared/shared-data-set"
+import { FilterFormulaAdapter } from "../formula/filter-formula-adapter"
 
 /**
  * Create a DocumentModel and add a new sharedModelManager into its environment
@@ -35,7 +36,8 @@ export const createDocumentModel = (snapshot?: IDocumentModelSnapshot) => {
     formulaManager.addAdapters([
       new AttributeFormulaAdapter(adapterApi),
       new PlottedValueFormulaAdapter(adapterApi),
-      new PlottedFunctionFormulaAdapter(adapterApi)
+      new PlottedFunctionFormulaAdapter(adapterApi),
+      new FilterFormulaAdapter(adapterApi)
     ])
 
     addDisposer(document, onAction(document, (call) => {
diff --git a/v3/src/models/formula/filter-formula-adapter.test.ts b/v3/src/models/formula/filter-formula-adapter.test.ts
new file mode 100644
index 0000000000..59552b0665
--- /dev/null
+++ b/v3/src/models/formula/filter-formula-adapter.test.ts
@@ -0,0 +1,127 @@
+import { IDataSet } from "../data/data-set"
+import { createDataSet } from "../data/data-set-conversion"
+import { FilterFormulaAdapter } from "./filter-formula-adapter"
+import { displayToCanonical } from "./utils/canonicalization-utils"
+import { getDisplayNameMap } from "./utils/name-mapping-utils"
+
+const AttrID = "AttrID"
+const AttrName = "foo"
+
+const getTestEnv = (filterFormula: string) => {
+  const dataSet = createDataSet({ name: "DataSet" })
+  dataSet.addAttribute({ id: AttrID, name: AttrName })
+  dataSet.addCases([{ __id__: "1", AttrID: 1 }, { __id__: "2", AttrID: 2 }, { __id__: "3", AttrID: 3 }])
+  dataSet.setFilterFormula(filterFormula)
+  const formula = dataSet.filterFormula!
+  const dataSets = new Map<string, IDataSet>([[dataSet.id, dataSet]])
+  const context = { dataSet, formula }
+  const extraMetadata = { dataSetId: dataSet.id }
+  const api = {
+    getDatasets: jest.fn(() => dataSets),
+    getGlobalValueManager: jest.fn(),
+    getFormulaExtraMetadata: jest.fn(() => extraMetadata),
+    getFormulaContext: jest.fn(() => context),
+  }
+  const adapter = new FilterFormulaAdapter(api)
+
+  const displayNameMap = getDisplayNameMap({ localDataSet: dataSet, dataSets })
+  formula.setCanonicalExpression(displayToCanonical(formula.display, displayNameMap))
+
+  return { adapter, api, dataSet, formula, context, extraMetadata }
+}
+
+describe("AttributeFormulaAdapter", () => {
+  describe("getAllFormulas", () => {
+    it("should return attribute formulas and extra metadata", () => {
+      const { adapter, extraMetadata, formula } = getTestEnv("foo < 2")
+      const formulas = adapter.getActiveFormulas()
+      expect(formulas.length).toBe(1)
+      expect(formulas[0].formula).toBe(formula)
+      expect(formulas[0].formula.display).toBe("foo < 2")
+      expect(formulas[0].extraMetadata).toEqual(extraMetadata)
+    })
+  })
+
+  describe("computeFormula", () => {
+    it("should return results for each provided case", () => {
+      const { adapter, context, extraMetadata } = getTestEnv("foo < 2")
+      const results = adapter.computeFormula(context, extraMetadata, "ALL_CASES")
+      expect(results).toEqual([
+        { itemId: "1", result: true },
+        { itemId: "2", result: false },
+        { itemId: "3", result: false },
+      ])
+    })
+
+    it("should ignore cases that are set aside by the user", () => {
+      const { adapter, context, extraMetadata } = getTestEnv("foo < 2")
+      const dataSet = context.dataSet
+      dataSet.hideCasesOrItems(["3"])
+      const results = adapter.computeFormula(context, extraMetadata, "ALL_CASES")
+      expect(results).toEqual([
+        { itemId: "1", result: true },
+        { itemId: "2", result: false }
+      ])
+    })
+
+    it("should not include set-aside cases in the aggregate function calculations", () => {
+      const { adapter, context, dataSet, extraMetadata } = getTestEnv("foo < max(foo)")
+      const results = adapter.computeFormula(context, extraMetadata, "ALL_CASES")
+      // max(foo) = 3 when all the cases are included
+      expect(results).toEqual([
+        { itemId: "1", result: true },
+        { itemId: "2", result: true },
+        { itemId: "3", result: false },
+      ])
+
+      dataSet.hideCasesOrItems(["3"])
+
+      const results2 = adapter.computeFormula(context, extraMetadata, "ALL_CASES")
+      // max(foo) = 2 when case 3 is set aside
+      expect(results2).toEqual([
+        { itemId: "1", result: true },
+        { itemId: "2", result: false }
+      ])
+    })
+  })
+
+  describe("recalculateFormula", () => {
+    it("should store results in DataSet filterFormulaResult map", () => {
+      const { adapter, dataSet, context, extraMetadata } = getTestEnv("foo < 2")
+      adapter.recalculateFormula(context, extraMetadata, "ALL_CASES")
+      expect(dataSet.filterFormulaResults.toJSON()).toEqual([
+        ["1", true],
+        ["2", false],
+        ["3", false],
+      ])
+    })
+
+    it("should update just provided cases when casesToRecalculateDesc is an array", () => {
+      const { adapter, dataSet, context, extraMetadata } = getTestEnv("foo < 2")
+      dataSet.filterFormulaResults.set("1", false)
+      dataSet.filterFormulaResults.set("2", true)
+
+      adapter.recalculateFormula(context, extraMetadata, [{ __id__: "1" }])
+      expect(dataSet.filterFormulaResults.toJSON()).toEqual([
+        ["1", true],
+        // Old values should be preserved
+        ["2", true]
+      ])
+    })
+
+    it("should clear previous filter formula error", () => {
+      const { adapter, dataSet, context, extraMetadata } = getTestEnv("foo < 2")
+      adapter.setFormulaError(context, extraMetadata, "test error")
+      adapter.recalculateFormula(context, extraMetadata, "ALL_CASES")
+      expect(dataSet.filterFormulaError).toEqual("")
+    })
+  })
+
+  describe("setError", () => {
+    it("should store error in DataSet case values", () => {
+      const { adapter, dataSet, context, extraMetadata } = getTestEnv("foo < 2")
+      adapter.setFormulaError(context, extraMetadata, "test error")
+      expect(dataSet.filterFormulaError).toEqual("test error")
+    })
+  })
+})
diff --git a/v3/src/models/formula/filter-formula-adapter.ts b/v3/src/models/formula/filter-formula-adapter.ts
new file mode 100644
index 0000000000..456f1531f7
--- /dev/null
+++ b/v3/src/models/formula/filter-formula-adapter.ts
@@ -0,0 +1,115 @@
+import { math } from "./functions/math"
+import { FormulaMathJsScope } from "./formula-mathjs-scope"
+import { formulaError } from "./utils/misc"
+import { ICase } from "../data/data-set-types"
+import { IFormula } from "./formula"
+import type {
+  IFormulaAdapterApi, IFormulaContext, IFormulaExtraMetadata, IFormulaManagerAdapter
+} from "./formula-manager"
+import { DEBUG_FORMULAS, debugLog } from "../../lib/debug"
+import { isFilterFormulaDataSet } from "../data/data-set"
+
+const FILTER_FORMULA_ADAPTER = "FilterFormulaAdapter"
+
+export class FilterFormulaAdapter implements IFormulaManagerAdapter {
+  type = FILTER_FORMULA_ADAPTER
+  api: IFormulaAdapterApi
+
+  constructor(api: IFormulaAdapterApi) {
+    this.api = api
+  }
+
+  getActiveFormulas(): ({ formula: IFormula, extraMetadata: IFormulaExtraMetadata })[] {
+    const result: ({ formula: IFormula, extraMetadata: IFormulaExtraMetadata })[] = []
+    this.api.getDatasets().forEach(dataSet => {
+      if (isFilterFormulaDataSet(dataSet)) {
+        result.push({
+          formula: dataSet.filterFormula,
+          extraMetadata: {
+            dataSetId: dataSet.id
+          }
+        })
+      }
+    })
+    return result
+  }
+
+  recalculateFormula(formulaContext: IFormulaContext, extraMetadata: IFormulaExtraMetadata,
+    casesToRecalculateDesc: ICase[] | "ALL_CASES" = "ALL_CASES") {
+    if (formulaContext.formula.empty) {
+      return
+    }
+
+    // Clear any previous error first.
+    this.setFormulaError(formulaContext, extraMetadata, "")
+
+    const dataSet = this.api.getDatasets().get(extraMetadata.dataSetId)
+    if (!dataSet) {
+      throw new Error(`Dataset with id "${extraMetadata.dataSetId}" not found`)
+    }
+    const results = this.computeFormula(formulaContext, extraMetadata, casesToRecalculateDesc)
+    if (results && results.length > 0) {
+      dataSet.updateFilterFormulaResults(results, { replaceAll: casesToRecalculateDesc === "ALL_CASES" })
+    }
+  }
+
+  computeFormula(formulaContext: IFormulaContext, extraMetadata: IFormulaExtraMetadata,
+    casesToRecalculateDesc: ICase[] | "ALL_CASES" = "ALL_CASES") {
+    const { formula, dataSet } = formulaContext
+    const { attributeId } = extraMetadata
+
+    let casesToRecalculate: ICase[] = []
+    if (casesToRecalculateDesc === "ALL_CASES") {
+      // If casesToRecalculate is not provided, recalculate all cases.
+      // Use itemsNotSetAside to exclude set-aside cases so they're not included in calculations,
+      // such as aggregate functions like mean.
+      casesToRecalculate = dataSet.itemsNotSetAside.map(id => dataSet.getItem(id)).filter(c => c) as ICase[]
+    } else {
+      casesToRecalculate = casesToRecalculateDesc
+    }
+    if (!casesToRecalculate || casesToRecalculate.length === 0) {
+      return
+    }
+
+    const caseIds = casesToRecalculate.map(c => c.__id__)
+
+    debugLog(DEBUG_FORMULAS, `[attr formula] recalculate "${formula.canonical}" for ${casesToRecalculate.length} cases`)
+
+    const formulaScope = new FormulaMathJsScope({
+      localDataSet: dataSet,
+      dataSets: this.api.getDatasets(),
+      globalValueManager: this.api.getGlobalValueManager(),
+      formulaAttrId: attributeId,
+      caseIds,
+      childMostCollectionCaseIds: caseIds
+    })
+
+    try {
+      const compiledFormula = math.compile(formula.canonical)
+      return casesToRecalculate.map((c, idx) => {
+        formulaScope.setCasePointer(idx)
+        const formulaValue = compiledFormula.evaluate(formulaScope)
+        // This is necessary for functions like `prev` that need to know the previous result when they reference
+        // its own attribute.
+        formulaScope.savePreviousResult(formulaValue)
+        return {
+          itemId: c.__id__,
+          result: !!formulaValue // only boolean values are supported
+        }
+      })
+    } catch (e: any) {
+      return this.setFormulaError(formulaContext, extraMetadata, formulaError(e.message))
+    }
+  }
+
+  // Error message is set as formula output, similarly as in CODAP V2.
+  setFormulaError(formulaContext: IFormulaContext, extraMetadata: IFormulaExtraMetadata, errorMsg: string) {
+    const { dataSet } = formulaContext
+    dataSet.setFilterFormulaError(errorMsg)
+  }
+
+  getFormulaError(formulaContext: IFormulaContext, extraMetadata: IFormulaExtraMetadata) {
+    // No custom errors yet.
+    return undefined
+  }
+}

From b79deafa26cd8b585f8f372c3dbfb4b2d8ca2945 Mon Sep 17 00:00:00 2001
From: Kirk Swenson <kswenson@users.noreply.github.com>
Date: Mon, 23 Sep 2024 15:52:31 -0700
Subject: [PATCH 3/4] chore: code review tweaks

---
 v3/src/components/case-table/use-rows.ts      |  2 +-
 .../case-tile-common/hide-show-menu-list.tsx  |  6 +-
 .../codap/create-codap-document.test.ts       |  2 +-
 v3/src/models/data/attribute.ts               |  5 --
 .../models/data/data-set-conversion.test.ts   | 24 +++++++-
 v3/src/models/data/data-set-conversion.ts     | 16 +++--
 v3/src/models/data/data-set.ts                | 58 +++++++++----------
 .../models/document/create-document-model.ts  |  6 +-
 v3/src/models/formula/formula-manager.ts      | 10 ++--
 9 files changed, 75 insertions(+), 54 deletions(-)

diff --git a/v3/src/components/case-table/use-rows.ts b/v3/src/components/case-table/use-rows.ts
index 5194a5ae04..8d0dbea6b7 100644
--- a/v3/src/components/case-table/use-rows.ts
+++ b/v3/src/components/case-table/use-rows.ts
@@ -135,7 +135,7 @@ export const useRows = () => {
 
     const onPatchDisposer = data && onPatch(data, ({ op, path, value }) => {
       // reset on any changes to items or hidden items
-      if (/(_itemIds|hiddenItemIds)(\/\d+)?$/.test(path)) {
+      if (/(_itemIds|setAsideItemIds)(\/\d+)?$/.test(path)) {
         resetRowCacheAndSyncRows()
       }
     })
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 ce39cefabb..382ee1a2d3 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
@@ -35,7 +35,7 @@ export const HideShowMenuList = observer(function HideShowMenuList() {
 
   const itemCount = data?.items.length ?? 0
   const selectionCount = data?.selection.size ?? 0
-  const setAsideCount = data?.hiddenItemIds.length ?? 0
+  const setAsideCount = data?.setAsideItemIds.length ?? 0
 
   const hiddenAttributes = data?.attributes.filter(attr => attr && caseMetadata?.isHidden(attr.id))
   const hiddenAttributeCount = hiddenAttributes?.length ?? 0
@@ -65,9 +65,9 @@ export const HideShowMenuList = observer(function HideShowMenuList() {
       itemLabel: () => t("DG.Inspector.setaside.restoreSetAsideCases", { vars: [setAsideCount] }),
       isEnabled: () => setAsideCount > 0,
       handleClick: () => {
-        if (data?.hiddenItemIds.length) {
+        if (data?.setAsideItemIds.length) {
           data.applyModelChange(() => {
-            const hiddenItems = [...data.hiddenItemIds]
+            const hiddenItems = [...data.setAsideItemIds]
             data.showHiddenCasesAndItems()
             data.setSelectedCases(hiddenItems)
           }, {
diff --git a/v3/src/models/codap/create-codap-document.test.ts b/v3/src/models/codap/create-codap-document.test.ts
index 564d762ef1..98b2a49171 100644
--- a/v3/src/models/codap/create-codap-document.test.ts
+++ b/v3/src/models/codap/create-codap-document.test.ts
@@ -89,7 +89,7 @@ describe("createCodapDocument", () => {
                 }
               },
               _itemIds: ["test-9", "test-10", "test-11"],
-              hiddenItemIds: [],
+              setAsideItemIds: [],
               collections: [{
                 id: "test-7",
                 name: "Cases",
diff --git a/v3/src/models/data/attribute.ts b/v3/src/models/data/attribute.ts
index 78781c50e8..ddf1668dfc 100644
--- a/v3/src/models/data/attribute.ts
+++ b/v3/src/models/data/attribute.ts
@@ -307,11 +307,6 @@ export const Attribute = V2Model.named("Attribute").props({
       }
     }
     else {
-      // Note that this generates a warning that the formula is unregistered in an unexpected way.
-      // Originally, the formula manager was designed to handle only empty formulas. However,
-      // https://github.com/concord-consortium/codap/pull/1124 changed the behavior, and formula objects now get deleted
-      // from the parent model.
-      // TODO: handle it better, or remove the warning from the formula manager if it doesn't cause any issues
       this.clearFormula()
     }
   },
diff --git a/v3/src/models/data/data-set-conversion.test.ts b/v3/src/models/data/data-set-conversion.test.ts
index e53ec2953b..5592966c30 100644
--- a/v3/src/models/data/data-set-conversion.test.ts
+++ b/v3/src/models/data/data-set-conversion.test.ts
@@ -2,7 +2,7 @@ import { IAttributeSnapshot } from "./attribute"
 import { ICollectionModelSnapshot } from "./collection"
 import { DataSet } from "./data-set"
 import {
-  IInitialItemsDataSetSnap, IOriginalDataSetSnap, IPreItemsDataSetSnap, ITempDataSetSnap
+  IHiddenItemIdsDataSetSnap, IInitialItemsDataSetSnap, IOriginalDataSetSnap, IPreItemsDataSetSnap, ITempDataSetSnap
 } from "./data-set-conversion"
 
 const kUngroupedCollectionName = "Collection Formerly Known As Ungrouped"
@@ -109,4 +109,26 @@ describe("DataSet conversions", () => {
     expect(data._itemIds.length).toBe(1)
     expect(data.itemIds.length).toBe(1)
   })
+
+  test("DataSet hiddenItemIds flat snapshot conversion", () => {
+    const data = DataSet.create({
+      name: "Data",
+      collections: [{
+        name: "Cases",
+        attributes: ["a1Id", "a2Id", "a3Id"]
+      }],
+      attributesMap: {
+        a1Id: { id: "a1Id", name: "a1", values: ["a1-0"] },
+        a2Id: { id: "a2Id", name: "a2", values: ["a2-0"] },
+        a3Id: { id: "a3Id", name: "a3", values: ["a3-0"] }
+      },
+      _itemIds: ["ITEM0"],
+      hiddenItemIds: ["ITEM0"]
+    } as IHiddenItemIdsDataSetSnap)
+    expect(data.attributesMap.size).toBe(3)
+    expect(data.attributes.length).toBe(3)
+    expect(data._itemIds.length).toBe(1)
+    expect(data.itemIds.length).toBe(1)
+    expect(data.setAsideItemIds.length).toBe(1)
+  })
 })
diff --git a/v3/src/models/data/data-set-conversion.ts b/v3/src/models/data/data-set-conversion.ts
index 6b007ab9d4..e9e0a56674 100644
--- a/v3/src/models/data/data-set-conversion.ts
+++ b/v3/src/models/data/data-set-conversion.ts
@@ -47,11 +47,19 @@ export function isInitialItemsDataSetSnap(snap: IDataSetSnapshot): snap is IInit
   return !("attributes" in snap) && !("ungrouped" in snap) && !("cases" in snap) && ("itemIds" in snap)
 }
 
-export type ILegacyDataSetSnap = (IOriginalDataSetSnap | ITempDataSetSnap |
-            IPreItemsDataSetSnap | IInitialItemsDataSetSnap) & { itemIds?: string[] }
+// hiddenItemIds => setAsideItemIds
+export interface IHiddenItemIdsDataSetSnap extends Omit<IInitialItemsDataSetSnap, "itemIds"> {
+  hiddenItemIds?: string[]
+}
+export function isHiddenItemIdsDataSetSnap(snap: IDataSetSnapshot): snap is IHiddenItemIdsDataSetSnap {
+  return "hiddenItemIds" in snap
+}
+
+export type ILegacyDataSetSnap = (IOriginalDataSetSnap | ITempDataSetSnap | IPreItemsDataSetSnap |
+            IInitialItemsDataSetSnap | IHiddenItemIdsDataSetSnap) & { itemIds?: string[], hiddenItemIds?: string[] }
 export function isLegacyDataSetSnap(snap: IDataSetSnapshot): snap is ILegacyDataSetSnap {
-  return isOriginalDataSetSnap(snap) || isTempDataSetSnap(snap) ||
-          isPreItemDataSetSnap(snap) || isInitialItemsDataSetSnap(snap)
+  return isOriginalDataSetSnap(snap) || isTempDataSetSnap(snap) || isPreItemDataSetSnap(snap) ||
+          isInitialItemsDataSetSnap(snap) || isHiddenItemIdsDataSetSnap(snap)
 }
 
 export function createDataSet(snap: IDataSetSnapshot | ILegacyDataSetSnap): IDataSet {
diff --git a/v3/src/models/data/data-set.ts b/v3/src/models/data/data-set.ts
index 9f0fff2b27..7d41f5c27f 100644
--- a/v3/src/models/data/data-set.ts
+++ b/v3/src/models/data/data-set.ts
@@ -140,7 +140,7 @@ export const DataSet = V2Model.named("DataSet").props({
   // for serialization only, not for dynamic selection tracking
   snapSelection: types.array(types.string),
   // hidden by user, e.g. set-aside in CODAP
-  hiddenItemIds: types.array(types.string),
+  setAsideItemIds: types.array(types.string),
   filterFormula: types.maybe(Formula)
 })
 .volatile(self => ({
@@ -152,9 +152,9 @@ export const DataSet = V2Model.named("DataSet").props({
   selection: observable.set<string>(),
   selectionChanges: 0,
   // MobX-observable set of hidden (set aside) item IDs
-  hiddenItemIdsSet: observable.set<string>(),
-  // copy of hiddenItemIds used for change-detection
-  hiddenItemIdsMirror: [] as string[],
+  setAsideItemIdsSet: observable.set<string>(),
+  // copy of setAsideItemIds used for change-detection
+  setAsideItemIdsMirror: [] as string[],
   // map from case ID to the CaseInfo it represents
   caseInfoMap: new Map<string, CaseInfo>(),
   // map from item ID to the child case containing it
@@ -221,7 +221,8 @@ export const DataSet = V2Model.named("DataSet").props({
   // convert legacy collections/attributes/cases implementation to current
   if (isLegacyDataSetSnap(snap)) {
     const {
-      collections: _collections = [], attributes: _legacyAttributes, ungrouped, cases, itemIds, ...others
+      collections: _collections = [], attributes: _legacyAttributes, ungrouped,
+      cases, itemIds, hiddenItemIds = [], ...others
     } = snap
 
     const attributeIds: string[] = []
@@ -269,13 +270,13 @@ export const DataSet = V2Model.named("DataSet").props({
 
     const _itemIds = cases?.map(({ __id__ }) => __id__) ?? itemIds ?? []
 
-    return { attributesMap, collections, _itemIds, ...others }
+    return { attributesMap, collections, _itemIds, setAsideItemIds: hiddenItemIds, ...others }
   }
   return snap
 })
 .views(self => ({
   isItemSetAside(itemId: string) {
-    return self.hiddenItemIdsSet.has(itemId)
+    return self.setAsideItemIdsSet.has(itemId)
   },
   isItemFilteredOut(itemId: string) {
     // Note that if itemResult is undefined, it means the item has not been filtered out (e.g., there may not be any
@@ -565,14 +566,14 @@ export const DataSet = V2Model.named("DataSet").props({
       const caseInfo = self.caseInfoMap.get(id)
       if (caseInfo) {
         caseInfo.childItemIds.forEach(itemId => {
-          if (!self.hiddenItemIdsSet.has(itemId)) {
-            self.hiddenItemIds.push(itemId)
+          if (!self.setAsideItemIdsSet.has(itemId)) {
+            self.setAsideItemIds.push(itemId)
           }
         })
       }
       else if (self.itemInfoMap.get(id)) {
-        if (!self.hiddenItemIdsSet.has(id)) {
-          self.hiddenItemIds.push(id)
+        if (!self.setAsideItemIdsSet.has(id)) {
+          self.setAsideItemIds.push(id)
         }
       }
     })
@@ -583,18 +584,18 @@ export const DataSet = V2Model.named("DataSet").props({
         const caseInfo = self.caseInfoMap.get(id)
         if (caseInfo) {
           caseInfo.childItemIds.forEach(itemId => {
-            const foundIndex = self.hiddenItemIds.findIndex(hiddenItemId => hiddenItemId === itemId)
-            if (foundIndex >= 0) self.hiddenItemIds.splice(foundIndex, 1)
+            const foundIndex = self.setAsideItemIds.findIndex(hiddenItemId => hiddenItemId === itemId)
+            if (foundIndex >= 0) self.setAsideItemIds.splice(foundIndex, 1)
           })
         }
         else if (self.itemInfoMap.get(id)) {
-          const foundIndex = self.hiddenItemIds.findIndex(hiddenItemId => hiddenItemId === id)
-          if (foundIndex >= 0) self.hiddenItemIds.splice(foundIndex, 1)
+          const foundIndex = self.setAsideItemIds.findIndex(hiddenItemId => hiddenItemId === id)
+          if (foundIndex >= 0) self.setAsideItemIds.splice(foundIndex, 1)
         }
       })
     } else {
       // show all hidden cases/items
-      self.hiddenItemIds.clear()
+      self.setAsideItemIds.clear()
     }
   },
   setFilterFormula(display: string) {
@@ -606,11 +607,6 @@ export const DataSet = V2Model.named("DataSet").props({
         self.filterFormula.setDisplayExpression(display)
       }
     } else {
-      // Note that this generates a warning that the formula is unregistered in an unexpected way.
-      // Originally, the formula manager was designed to handle only empty formulas. However,
-      // https://github.com/concord-consortium/codap/pull/1124 changed the behavior, and formula objects now get deleted
-      // from the parent model.
-      // TODO: handle it better, or remove the warning from the formula manager if it doesn't cause any issues
       self.clearFilterFormula()
     }
   },
@@ -1251,28 +1247,28 @@ export const DataSet = V2Model.named("DataSet").props({
       }))
 
       // when items are hidden/shown...
-      // Use MST's onPatch mechanism to respond to changes to the `hiddenItemIds`.
+      // Use MST's onPatch mechanism to respond to changes to the `setAsideItemIds`.
       // This will be called once for each item added/removed and will update related properties.
       addDisposer(self, onPatch(self, ({ op, path, value }) => {
         let match: RegExpExecArray | null = null
-        if ((op === "add" || op === "remove") && (match = /hiddenItemIds\/(\d+)$/.exec(path))) {
+        if ((op === "add" || op === "remove") && (match = /setAsideItemIds\/(\d+)$/.exec(path))) {
           const index = +match[1]
           if (op === "add") {
             const itemId = value
-            self.hiddenItemIdsMirror.splice(index, 0, itemId)
-            self.hiddenItemIdsSet.add(itemId)
+            self.setAsideItemIdsMirror.splice(index, 0, itemId)
+            self.setAsideItemIdsSet.add(itemId)
           }
           else {
-            const itemId = self.hiddenItemIdsMirror[index]
-            self.hiddenItemIdsMirror.splice(index, 1)
-            self.hiddenItemIdsSet.delete(itemId)
+            const itemId = self.setAsideItemIdsMirror[index]
+            self.setAsideItemIdsMirror.splice(index, 1)
+            self.setAsideItemIdsSet.delete(itemId)
           }
           self.invalidateCases()
         }
-        if ((op === "replace") && /hiddenItemIds$/.test(path)) {
+        if ((op === "replace") && /setAsideItemIds$/.test(path)) {
           const replacementArray: string[] = value
-          self.hiddenItemIdsMirror = [...replacementArray]
-          self.hiddenItemIdsSet.replace(observable.set<string>(replacementArray))
+          self.setAsideItemIdsMirror = [...replacementArray]
+          self.setAsideItemIdsSet.replace(observable.set<string>(replacementArray))
           self.invalidateCases()
         }
       }))
diff --git a/v3/src/models/document/create-document-model.ts b/v3/src/models/document/create-document-model.ts
index 12ad157607..3e2e919f61 100644
--- a/v3/src/models/document/create-document-model.ts
+++ b/v3/src/models/document/create-document-model.ts
@@ -9,10 +9,10 @@ import { IDocumentEnvironment } from "./document-environment"
 import { SharedModelDocumentManager } from "./shared-model-document-manager"
 import { FormulaManager } from "../formula/formula-manager"
 import { AttributeFormulaAdapter } from "../formula/attribute-formula-adapter"
+import { FilterFormulaAdapter } from "../formula/filter-formula-adapter"
 import { PlottedValueFormulaAdapter } from "../formula/plotted-value-formula-adapter"
 import { PlottedFunctionFormulaAdapter } from "../formula/plotted-function-formula-adapter"
 import { ISharedDataSet, SharedDataSet, kSharedDataSetType } from "../shared/shared-data-set"
-import { FilterFormulaAdapter } from "../formula/filter-formula-adapter"
 
 /**
  * Create a DocumentModel and add a new sharedModelManager into its environment
@@ -35,9 +35,9 @@ export const createDocumentModel = (snapshot?: IDocumentModelSnapshot) => {
     // initialize formula adapters after the document has been created
     formulaManager.addAdapters([
       new AttributeFormulaAdapter(adapterApi),
+      new FilterFormulaAdapter(adapterApi),
       new PlottedValueFormulaAdapter(adapterApi),
-      new PlottedFunctionFormulaAdapter(adapterApi),
-      new FilterFormulaAdapter(adapterApi)
+      new PlottedFunctionFormulaAdapter(adapterApi)
     ])
 
     addDisposer(document, onAction(document, (call) => {
diff --git a/v3/src/models/formula/formula-manager.ts b/v3/src/models/formula/formula-manager.ts
index 195a4c777b..9df532e5dc 100644
--- a/v3/src/models/formula/formula-manager.ts
+++ b/v3/src/models/formula/formula-manager.ts
@@ -1,5 +1,5 @@
 import { comparer, makeObservable, observable, reaction, action } from "mobx"
-import { addDisposer, isAlive } from "mobx-state-tree"
+import { addDisposer } from "mobx-state-tree"
 import { ICase } from "../data/data-set-types"
 import { CaseList } from "./formula-types"
 import { IDataSet } from "../data/data-set"
@@ -236,10 +236,6 @@ export class FormulaManager {
       if (!activeFormulas.has(formulaId)) {
         this.unregisterFormula(formulaId)
       }
-      if (!isAlive(metadata.formula)) {
-        console.warn(`Formula ${metadata.formula.display} unregistered in an unexpected way`)
-        this.unregisterFormula(formulaId)
-      }
     })
   }
 
@@ -307,6 +303,10 @@ export class FormulaManager {
     const { dataSet } = formulaContext
     const { formula, adapter } = formulaMetadata
     const { defaultArgument } = extraMetadata
+
+    // unregister formulas when they're destroyed
+    addDisposer(formula, () => this.unregisterFormula(formulaId))
+
     if (formula.empty) {
       return
     }

From a85ef8c30c66c22960a43dbff4e2385d8242596e Mon Sep 17 00:00:00 2001
From: pjanik <janikpiotrek@gmail.com>
Date: Tue, 24 Sep 2024 15:09:18 +0200
Subject: [PATCH 4/4] chore: remove unnecessary character

---
 .../components/case-tile-common/edit-filter-formula-modal.tsx   | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/v3/src/components/case-tile-common/edit-filter-formula-modal.tsx b/v3/src/components/case-tile-common/edit-filter-formula-modal.tsx
index d9e666090a..58061d8d72 100644
--- a/v3/src/components/case-tile-common/edit-filter-formula-modal.tsx
+++ b/v3/src/components/case-tile-common/edit-filter-formula-modal.tsx
@@ -58,7 +58,7 @@ export const EditFilterFormulaModal = observer(function EditFormulaModal({ isOpe
         <div className="codap-modal-icon-container" />
         <div className="codap-header-title" />
         <ModalCloseButton onClick={onClose} data-testid="modal-close-button" />
-      </ModalHeader>=
+      </ModalHeader>
       <ModalBody>
         {
           data?.filterFormulaError &&