CODAP-1117: fix MobX constraint violation in Collection.completeCaseGroups

v3/src/models/data/collection.test.ts

@@ -257,14 +257,22 @@ describe("CollectionModel", () => {
       itemIdToCaseIdsMap.clear()
 
       root.collections.forEach((collection, index) => {
+        if (!newCaseIds) {
+          // full invalidation for rebuild path
+          collection.invalidateCaseGroups()
+        }
         // update the cases
         collection.updateCaseGroups()
       })
 
       root.collections.forEach((collection, index) => {
+        if (newCaseIds) {
+          // additive invalidation for append path
+          collection.invalidateCaseGroupsForNewCases(newCaseIds)
+        }
         // sort child collection cases into groups
         const parentCaseGroups = index > 0 ? root.collections[index - 1].caseGroups : undefined
-        collection.completeCaseGroups(parentCaseGroups, newCaseIds)
+        collection.completeCaseGroups(parentCaseGroups)
       })
     }
     validateCases()
@@ -425,4 +433,93 @@ describe("CollectionModel", () => {
     expect(c1.groupKeyCaseIds.get(makeGroupKey(["c"]))).toBe("case5")
     expect(c1.groupKeyCaseIds.get(makeGroupKey(["c", "d"]))).toBe("case6")
   })
+
+  it("invalidateCaseGroups followed by invalidateCaseGroupsForNewCases still results in full rebuild", () => {
+    const c1 = CollectionModel.create({ name: "c1" })
+    const itemData: IItemData = {
+      itemIds: () => ["i0", "i1"],
+      isHidden: () => false,
+      getValue: (itemId: string) => itemId,
+      addItemInfo: () => null,
+      invalidate: () => null
+    }
+    syncCollectionLinks([c1], itemData)
+
+    // initial build (rebuild path since _needsFullRebuild starts true)
+    c1.updateCaseGroups()
+    c1.completeCaseGroups(undefined)
+    expect(c1.cases.length).toBe(2)
+
+    // full invalidation followed by additive — should still rebuild
+    c1.invalidateCaseGroups()
+    c1.invalidateCaseGroupsForNewCases(["i1"])
+    c1.updateCaseGroups()
+    c1.completeCaseGroups(undefined)
+    // rebuild produces full set, not just the "new" case
+    expect(c1.cases.length).toBe(2)
+  })
+
+  it("empty newCaseIds falls through to rebuild path", () => {
+    const c1 = CollectionModel.create({ name: "c1" })
+    let items = ["i0", "i1"]
+    const itemData: IItemData = {
+      itemIds: () => items,
+      isHidden: () => false,
+      getValue: (itemId: string) => itemId,
+      addItemInfo: () => null,
+      invalidate: () => null
+    }
+    syncCollectionLinks([c1], itemData)
+
+    // initial full build
+    c1.updateCaseGroups()
+    c1.completeCaseGroups(undefined)
+    expect(c1.cases.length).toBe(2)
+
+    // simulate the scenario where items are re-added (e.g. undo of delete)
+    // and updateCaseGroups returns empty newCaseIds because the groupKey
+    // mappings already exist in groupKeyCaseIds
+    items = ["i0", "i1", "i2"]
+    c1.updateCaseGroups()
+    // pass empty array — should fall through to REBUILD, not take APPEND (which would append nothing)
+    c1.invalidateCaseGroupsForNewCases([])
+    c1.completeCaseGroups(undefined)
+    // rebuild should pick up all 3 items
+    expect(c1.cases.length).toBe(3)
+    expect(c1.caseGroups.length).toBe(3)
+  })
+
+  it("additive invalidation appends correctly without full rebuild", () => {
+    const c1 = CollectionModel.create({ name: "c1" })
+    let items = ["i0", "i1"]
+    const itemData: IItemData = {
+      itemIds: () => items,
+      isHidden: () => false,
+      getValue: (itemId: string) => itemId,
+      addItemInfo: () => null,
+      invalidate: () => null
+    }
+    syncCollectionLinks([c1], itemData)
+
+    // initial full build
+    c1.invalidateCaseGroups()
+    c1.updateCaseGroups()
+    c1.completeCaseGroups(undefined)
+    expect(c1.cases.length).toBe(2)
+    expect(c1.caseGroups.length).toBe(2)
+
+    // add a new item and use additive path
+    items = ["i0", "i1", "i2"]
+    c1.updateCaseGroups(["i2"])
+    // find the case id for i2
+    let i2CaseId: string | undefined
+    c1.caseIdToGroupKeyMap.forEach((groupKey, caseId) => {
+      if (groupKey === "i2") i2CaseId = caseId
+    })
+    c1.invalidateCaseGroupsForNewCases(i2CaseId ? [i2CaseId] : [])
+    c1.completeCaseGroups(undefined)
+    // should have appended the new case
+    expect(c1.cases.length).toBe(3)
+    expect(c1.caseGroups.length).toBe(3)
+  })
 })

v3/src/models/data/collection.ts

@@ -479,29 +479,47 @@ export const CollectionModel = V2Model
   },
 }))
 .extend(self => {
-  const _caseGroups = observable.box<CaseInfo[]>([], { deep: false })
-  const _cases = observable.box<IGroupedCase[]>([], { deep: false })
-  const _nonEmptyCases = observable.box<IGroupedCase[]>([], { deep: false })
-  const _caseIdsHash = observable.box<number>(0)
-  const _caseIdsOrderedHash = observable.box<number>(0)
+  // Plain cached values (NOT observable — no MobX involvement)
+  let _caseGroups: CaseInfo[] = []
+  let _cases: IGroupedCase[] = []
+  let _nonEmptyCases: IGroupedCase[] = []
+  let _caseIdsHash = 0
+  let _caseIdsOrderedHash = 0
+
+  // Single observable version counter — incremented by completeCaseGroups() after updating
+  // plain variables, so that MobX reactions only fire when fresh data is available.
+  // Note: this is a pragmatic compromise — writing to an observable from a view via runInAction
+  // is not ideal, but avoids the circular read/write pattern of the old code (5 observable boxes
+  // read then written in the same view) and ensures reactions never see stale cached data.
+  const _cacheVersion = observable.box(0)
+
+  // Invalidation state (plain, not observable)
+  let _needsFullRebuild = true   // starts true: no valid data yet
+  let _pendingNewCaseIds: string[] | undefined
+
   return {
     views: {
       get caseGroups() {
-        return _caseGroups.get()
+        _cacheVersion.get()
+        return _caseGroups
       },
       get cases() {
-        return _cases.get()
+        _cacheVersion.get()
+        return _cases
       },
       get nonEmptyCases() {
-        return _nonEmptyCases.get()
+        _cacheVersion.get()
+        return _nonEmptyCases
       },
       get caseIdsHash() {
-        return _caseIdsHash.get()
+        _cacheVersion.get()
+        return _caseIdsHash
       },
       get caseIdsOrderedHash() {
-        return _caseIdsOrderedHash.get()
+        _cacheVersion.get()
+        return _caseIdsOrderedHash
       },
-      completeCaseGroups(parentCases: Maybe<CaseInfo[]>, newCaseIds?: string[]) {
+      completeCaseGroups(parentCases: Maybe<CaseInfo[]>) {
         if (parentCases) {
           self.caseIds.splice(0, self.caseIds.length)
           // sort cases by parent cases
@@ -522,44 +540,64 @@ export const CollectionModel = V2Model
           })
         }
 
-        let caseGroups = _caseGroups.get()
-        let cases = _cases.get()
-        let nonEmptyCases = _nonEmptyCases.get()
-        // append new cases to existing arrays
-        if (!parentCases && newCaseIds) {
+        const newCaseIds = _pendingNewCaseIds
+        _pendingNewCaseIds = undefined
+
+        // APPEND path: top-level collection with only new cases added
+        // Uses concatenation (not mutation) so consumers holding old references see a new array.
+        // Note: newCaseIds can be an empty array (e.g., when re-adding previously deleted items
+        // whose group key mappings still exist), so check length to fall through to REBUILD.
+        if (!parentCases && !_needsFullRebuild && newCaseIds?.length) {
           const newCaseGroups = newCaseIds.map(caseId => self.getCaseGroup(caseId))
-          caseGroups.push(...newCaseGroups.filter(group => !!group))
+          _caseGroups = [..._caseGroups, ...newCaseGroups.filter(group => !!group)]
 
           const newCases = newCaseGroups.map(caseGroup => caseGroup?.groupedCase)
-          cases.push(...newCases.filter(aCase => !!aCase))
+          _cases = [..._cases, ...newCases.filter(aCase => !!aCase)]
 
           const newNonEmptyCases = newCaseGroups.map(group => {
             return group && self.isNonEmptyCaseGroup(group) ? group.groupedCase : undefined
           }).filter(aCase => !!aCase)
-          nonEmptyCases.push(...newNonEmptyCases)
+          _nonEmptyCases = [..._nonEmptyCases, ...newNonEmptyCases]
         }
-        // rebuild arrays from scratch
+        // REBUILD path: full recompute from volatile state
         else {
-          caseGroups = self.caseIds
-                        .map(caseId => self.getCaseGroup(caseId))
-                        .filter(group => !!group)
+          _caseGroups = self.caseIds
+                          .map(caseId => self.getCaseGroup(caseId))
+                          .filter(group => !!group)
 
-          cases = caseGroups
-                    .map(group => group?.groupedCase)
-                    .filter(groupedCase => !!groupedCase)
+          _cases = _caseGroups
+                      .map(group => group?.groupedCase)
+                      .filter(groupedCase => !!groupedCase)
 
-          nonEmptyCases = caseGroups.map(group => {
+          _nonEmptyCases = _caseGroups.map(group => {
             return group && self.isNonEmptyCaseGroup(group) ? group.groupedCase : undefined
           }).filter(aCase => !!aCase)
+
+          _needsFullRebuild = false
         }
 
-        runInAction(() => {
-          _caseGroups.set(caseGroups)
-          _cases.set(cases)
-          _nonEmptyCases.set(nonEmptyCases)
-          _caseIdsHash.set(hashStringSet(self.caseIds))
-          _caseIdsOrderedHash.set(hashOrderedStringSet(self.caseIds))
-        })
+        _caseIdsHash = hashStringSet(self.caseIds)
+        _caseIdsOrderedHash = hashOrderedStringSet(self.caseIds)
+
+        // Signal to MobX that cached data has been updated.
+        // Must happen AFTER plain variables are updated so reactions see fresh data.
+        runInAction(() => _cacheVersion.set(_cacheVersion.get() + 1))
+      }
+    },
+    actions: {
+      // Full invalidation — signals complete rebuild needed.
+      // Called from DataSet.invalidateCases() (always in action context).
+      invalidateCaseGroups() {
+        _needsFullRebuild = true
+        _pendingNewCaseIds = undefined
+      },
+      // Additive invalidation — signals only new cases were added.
+      // Called from DataSet.validateCasesForNewItems() (action context).
+      // Only enables append path if no full rebuild is pending.
+      invalidateCaseGroupsForNewCases(newCaseIds: string[]) {
+        if (!_needsFullRebuild) {
+          _pendingNewCaseIds = newCaseIds
+        }
       }
     }
   }

v3/src/models/data/data-set-collections.test.ts

@@ -215,6 +215,38 @@ describe("DataSet collections", () => {
     expect(data.collections.length).toBe(1)
   })
 
+  it("recovers correctly when addCases is called while cases are already invalidated", () => {
+    // This simulates the undo scenario: removeCases triggers invalidateCases (via onPatch),
+    // then addCases re-adds items. The INSERT path triggers another invalidateCases, but the
+    // APPEND path calls validateCasesForNewItems which should early-return when isValidCases
+    // is already false, deferring to the full rebuild.
+    data.moveAttributeToNewCollection("aId")
+    expect(data.collections.length).toBe(2)
+    data.validateCases()
+    const parentCollection = data.collections[0]
+    const originalParentCaseCount = parentCollection.cases.length
+    const originalChildCaseCount = data.childCollection.cases.length
+    expect(originalParentCaseCount).toBe(3) // 3 groups by aId
+
+    // remove some cases
+    const casesToRemove = ["1-1-1", "1-1-2", "1-1-3"]
+    data.removeCases(casesToRemove)
+    data.validateCases()
+    expect(data.childCollection.cases.length).toBe(originalChildCaseCount - casesToRemove.length)
+
+    // now re-add them (simulating undo) — first invalidate, then add
+    data.invalidateCases()
+    data.addCases(casesToRemove.map(id => ({ __id__: id, aId: "1", bId: "1", cId: id.split("-")[2] })))
+    // at this point isValidCases is still false; validateCasesForNewItems should have
+    // early-returned without corrupting state
+    expect(data.isValidCases).toBe(false)
+
+    // full validation should produce correct results
+    data.validateCases()
+    expect(parentCollection.cases.length).toBe(originalParentCaseCount)
+    expect(data.childCollection.cases.length).toBe(originalChildCaseCount)
+  })
+
   it("doesn't take formula evaluated values into account when grouping", () => {
     const aAttr = data.attrFromID("aId")
     aAttr?.setDisplayExpression("foo * bar")

v3/src/models/data/data-set.ts

@@ -255,6 +255,8 @@ export const DataSet = V2UserTitleModel.named("DataSet").props({
         runInAction(() => {
           _isValidCases.set(false)
           _invalidateItemIds()
+          // invalidate each collection's case group cache
+          self.collections.forEach(c => c.invalidateCaseGroups())
         })
       },
       setValidCases() {
@@ -693,11 +695,17 @@ export const DataSet = V2UserTitleModel.named("DataSet").props({
     // Append new items to the itemIds/items cache
     self.appendItemIdsToCache(itemIds)
 
-    const newCaseIdsForCollections = new Map<string, string[]>()
+    // If a full invalidation is already pending (e.g., from an earlier insert in the same
+    // undo action), skip additive processing. The full rebuild will happen when validateCases()
+    // is next called. Without this check, completeCaseGroups() would run with incomplete volatile
+    // state and signal observers via _cacheVersion before the data is fully rebuilt.
+    if (!self.isValidCases) return
+
     self.collections.forEach((collection, index) => {
-      // update the cases
+      // update the cases (additive — only processes new itemIds)
       const { newCaseIds } = collection.updateCaseGroups(itemIds)
-      newCaseIdsForCollections.set(collection.id, newCaseIds)
+      // tell collection about new cases for additive completion
+      collection.invalidateCaseGroupsForNewCases(newCaseIds)
     })
     self.collections.forEach((collection, index) => {
       // complete the case groups, including sorting child collection cases into groups
@@ -1076,6 +1084,8 @@ export const DataSet = V2UserTitleModel.named("DataSet").props({
       },
       // should be called before retrieving snapshot (pre-serialization)
       prepareSnapshot() {
+        // ensure case validation is up to date before serializing
+        self.validateCases()
         // move volatile data into serializable properties
         withoutUndo({ noDirty: true, suppressWarning: true })
         self.collections.forEach(collection => collection.prepareSnapshot())