From ff5ed3a74fd47dc8ba60741fe77b96c6c7ade006 Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Sat, 14 Feb 2026 23:58:34 -0800 Subject: [PATCH 1/7] CODAP-1117: fix MobX constraint violation in cachedFnWithArgsFactory Replace observable.map cache with observable version counters (per-key and global) paired with a plain Map, so the getter never writes to an observable. This follows the same pattern as observableCachedFnFactory from PR #2377, avoiding the anti-pattern of modifying observables during computed/view evaluation. Co-Authored-By: Claude Opus 4.6 --- v3/src/utilities/mst-utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/v3/src/utilities/mst-utils.ts b/v3/src/utilities/mst-utils.ts index 8a556d47f5..f9ca3afc7f 100644 --- a/v3/src/utilities/mst-utils.ts +++ b/v3/src/utilities/mst-utils.ts @@ -199,7 +199,7 @@ export function cachedFnWithArgsFactory any>( const gv = _globalVersion.get() const kv = _keyVersions.get(cacheKey) const cached = _cache.get(cacheKey) - if (cached?.globalVer !== gv || cached.keyVer !== kv) { + if (!cached || cached.globalVer !== gv || cached.keyVer !== kv) { const value = calculate(...args) _cache.set(cacheKey, { value, globalVer: gv, keyVer: kv }) return value From 763043be7d871e065404355fc37b81e866fc0eb4 Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Tue, 17 Feb 2026 08:04:50 -0800 Subject: [PATCH 2/7] CODAP-1117: fix lint in cachedFnWithArgsFactory; update CLAUDE.md Simplify cache-miss check (lint fix) and add note about running git commands from the repo root to avoid v3/v3 path errors. Co-Authored-By: Claude Opus 4.6 --- v3/src/utilities/mst-utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/v3/src/utilities/mst-utils.ts b/v3/src/utilities/mst-utils.ts index f9ca3afc7f..8a556d47f5 100644 --- a/v3/src/utilities/mst-utils.ts +++ b/v3/src/utilities/mst-utils.ts @@ -199,7 +199,7 @@ export function cachedFnWithArgsFactory any>( const gv = _globalVersion.get() const kv = _keyVersions.get(cacheKey) const cached = _cache.get(cacheKey) - if (!cached || cached.globalVer !== gv || cached.keyVer !== kv) { + if (cached?.globalVer !== gv || cached.keyVer !== kv) { const value = calculate(...args) _cache.set(cacheKey, { value, globalVer: gv, keyVer: kv }) return value From d594249e5d04f92271d5e1fe87b656d52e45eba8 Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Mon, 16 Feb 2026 20:25:12 -0800 Subject: [PATCH 3/7] CODAP-1117: fix MobX constraint violation in Collection.completeCaseGroups Replace 5 observable boxes with plain variables and a single version counter to eliminate observable writes from within a computed/view. Add invalidateCaseGroups() and invalidateCaseGroupsForNewCases() actions so that the version counter is only incremented from action contexts. Co-Authored-By: Claude Opus 4.6 --- v3/src/models/data/collection.test.ts | 69 ++++++++++++++++- v3/src/models/data/collection.ts | 104 +++++++++++++++++--------- v3/src/models/data/data-set.ts | 8 +- 3 files changed, 143 insertions(+), 38 deletions(-) diff --git a/v3/src/models/data/collection.test.ts b/v3/src/models/data/collection.test.ts index 595b1434e6..3ae2ea719b 100644 --- a/v3/src/models/data/collection.test.ts +++ b/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,63 @@ 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("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) + }) }) diff --git a/v3/src/models/data/collection.ts b/v3/src/models/data/collection.ts index 8778a6464c..7dbab076d8 100644 --- a/v3/src/models/data/collection.ts +++ b/v3/src/models/data/collection.ts @@ -479,29 +479,47 @@ export const CollectionModel = V2Model }, })) .extend(self => { - const _caseGroups = observable.box([], { deep: false }) - const _cases = observable.box([], { deep: false }) - const _nonEmptyCases = observable.box([], { deep: false }) - const _caseIdsHash = observable.box(0) - const _caseIdsOrderedHash = observable.box(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, newCaseIds?: string[]) { + completeCaseGroups(parentCases: Maybe) { if (parentCases) { self.caseIds.splice(0, self.caseIds.length) // sort cases by parent cases @@ -522,44 +540,62 @@ 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. + if (!parentCases && !_needsFullRebuild && newCaseIds) { 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 + } } } } diff --git a/v3/src/models/data/data-set.ts b/v3/src/models/data/data-set.ts index 2116980841..c688cc88a0 100644 --- a/v3/src/models/data/data-set.ts +++ b/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,11 @@ export const DataSet = V2UserTitleModel.named("DataSet").props({ // Append new items to the itemIds/items cache self.appendItemIdsToCache(itemIds) - const newCaseIdsForCollections = new Map() 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 From 006f92705c521bcad73f4afd73db0ead26dbb86b Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Mon, 16 Feb 2026 21:48:41 -0800 Subject: [PATCH 4/7] CODAP-1117: skip additive case processing when full rebuild is pending When undo restores deleted cases in multiple batches (insert + append), the first batch triggers invalidateCases() but the second batch's validateCasesForNewItems() would run completeCaseGroups() with incomplete volatile state, signaling observers before data was fully rebuilt. Co-Authored-By: Claude Opus 4.6 --- v3/src/models/data/data-set.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/v3/src/models/data/data-set.ts b/v3/src/models/data/data-set.ts index c688cc88a0..aa78d5316d 100644 --- a/v3/src/models/data/data-set.ts +++ b/v3/src/models/data/data-set.ts @@ -695,6 +695,12 @@ export const DataSet = V2UserTitleModel.named("DataSet").props({ // Append new items to the itemIds/items cache self.appendItemIdsToCache(itemIds) + // 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 (additive — only processes new itemIds) const { newCaseIds } = collection.updateCaseGroups(itemIds) From 1904aaae05e2ffcede9133bc84fa3702243b00d2 Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Tue, 17 Feb 2026 07:26:09 -0800 Subject: [PATCH 5/7] CODAP-1117: fix empty-array truthiness bug in Collection.completeCaseGroups When undoing "delete unselected cases", updateCaseGroups() can return an empty newCaseIds array (because groupKeyCaseIds still has old mappings). The APPEND path condition treated [] as truthy, appending nothing instead of falling through to the REBUILD path. Check newCaseIds?.length instead. Co-Authored-By: Claude Opus 4.6 --- v3/src/models/data/collection.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/v3/src/models/data/collection.ts b/v3/src/models/data/collection.ts index 7dbab076d8..d16ac7135f 100644 --- a/v3/src/models/data/collection.ts +++ b/v3/src/models/data/collection.ts @@ -545,7 +545,9 @@ export const CollectionModel = V2Model // APPEND path: top-level collection with only new cases added // Uses concatenation (not mutation) so consumers holding old references see a new array. - if (!parentCases && !_needsFullRebuild && newCaseIds) { + // 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 = [..._caseGroups, ...newCaseGroups.filter(group => !!group)] From bbbc076b69e3bfb84060b02e4b2530410c3a9d20 Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Tue, 17 Feb 2026 07:34:34 -0800 Subject: [PATCH 6/7] CODAP-1117: ensure case validation before snapshot serialization The early return in validateCasesForNewItems (which skips additive processing when a full rebuild is pending) means groupKeyCaseIds may not be populated when prepareSnapshot() is called. Adding a validateCases() call in prepareSnapshot() ensures caches are current. Co-Authored-By: Claude Opus 4.6 --- v3/src/models/data/data-set.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/v3/src/models/data/data-set.ts b/v3/src/models/data/data-set.ts index aa78d5316d..f490c48a64 100644 --- a/v3/src/models/data/data-set.ts +++ b/v3/src/models/data/data-set.ts @@ -1084,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()) From 8f98f5c1eb776f67eeb9296ff2178a5e6cea0c06 Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Tue, 17 Feb 2026 07:50:15 -0800 Subject: [PATCH 7/7] CODAP-1117: add tests for empty-array fallthrough and invalidated addCases - Collection: test that invalidateCaseGroupsForNewCases([]) falls through to the REBUILD path instead of the APPEND path (which would append nothing) - DataSet: test that addCases called while isValidCases is false (simulating undo of removeCases) correctly defers to full rebuild via validateCases() Co-Authored-By: Claude Opus 4.6 --- v3/src/models/data/collection.test.ts | 30 +++++++++++++++++ .../models/data/data-set-collections.test.ts | 32 +++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/v3/src/models/data/collection.test.ts b/v3/src/models/data/collection.test.ts index 3ae2ea719b..b4312fd335 100644 --- a/v3/src/models/data/collection.test.ts +++ b/v3/src/models/data/collection.test.ts @@ -459,6 +459,36 @@ describe("CollectionModel", () => { 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"] diff --git a/v3/src/models/data/data-set-collections.test.ts b/v3/src/models/data/data-set-collections.test.ts index 922adea02a..84e934d8b1 100644 --- a/v3/src/models/data/data-set-collections.test.ts +++ b/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")