CODAP-1117/1178: fix MobX constraint violations and stale computeds in DataSet

[kswenson]

Summary

Fixes MobX constraint violations and stale computed bugs in the DataSet .extend() block, completing the work started in PRs #2393 (cachedFnWithArgsFactory) and #2398 (Collection.completeCaseGroups). Also fixes the Sampler plugin stale cache bug (CODAP-1178).

CODAP-1117: MobX constraint violations

• Replace 3 observable.box variables (_validationCount, _isValidCases, _isValidItemIds) with plain variables + a single _caseValidationVersion observable counter, following the same pattern as the Collection _cacheVersion fix
• Move invalidateCases() and invalidateItemIds() from views to actions
• Fix stale MobX computed: isValidCases, isValidItemIds, and validationCount getters now read _caseValidationVersion.get() to establish an observable dependency, and invalidateCases()/invalidateItemIds() bump the counter
• Defer selection cleanup from validateCases() to setValidCases(), consolidating all observable writes from views into a single runInAction
• Fix use-rows.ts reaction to always evaluate validationCount first so MobX dependency tracking is maintained

CODAP-1178: Sampler plugin stale cache bug

• Fix stale items/itemIds computeds caused by MobX dependency oscillation: after an append operation, _validateItemIds() is a no-op (cache valid), so MobX drops all tracked dependencies and permanently seals the cached array reference. Subsequent deletes/inserts are invisible.
• Fix: read _caseValidationVersion.get() in the items and itemIds getters -- same one-line pattern used for all other getters in the .extend() block

Root cause details

CODAP-1117 (stale isValidCases): The isValidCases view getter read a plain let variable with no observable dependency. When observed by a reaction, MobX cached it permanently. invalidateCases() set the variable to false, but MobX never re-evaluated the computed, so validateCases() short-circuited and hierarchical table operations returned stale data.

CODAP-1178 (stale items/itemIds): The items/itemIds getters call _validateItemIds(), which reads self._itemIds when the cache is invalid but is a no-op when valid. After an append, MobX re-evaluates during the no-op, tracks zero dependencies, and seals the computed. This was a heisenbug -- debug logging accidentally read self._itemIds.length outside untracked(), creating a dependency that prevented the oscillation.

Fixes CODAP-1117 and CODAP-1178.

Test plan

• All 2396 Jest tests pass (278 suites)
• Cypress smoke test passes (4/4 including "verify Mammals" hierarchical test)
• Cypress hierarchical table test passes (7/7 + 1 pre-existing skip)
• Manual verification: Sampler plugin Clear Data + re-sample works correctly
• New test: MobX reaction fires after invalidateCases + validateCases
• New test: MobX reaction fires after moveAttributeToNewCollection
• New test: deferred selection cleanup flushes in setValidCases
• New test: Collection view getter reactivity via version counter
• New test: prepareSnapshot forces validation when invalidated
• New test: implicit validation via getCasesForCollection
• New test: items getter survives dependency oscillation (CODAP-1178)
• Verified test fails without fix, passes with fix

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 87.17949% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.96%. Comparing base (b58e598) to head (ea356b2).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
v3/src/models/data/data-set.ts	86.11%	5 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #2401    +/-   ##
========================================
  Coverage   85.96%   85.96%            
========================================
  Files         768      768            
  Lines       42884    42899    +15     
  Branches    10262    10675   +413     
========================================
+ Hits        36865    36878    +13     
+ Misses       6007     6006     -1     
- Partials       12       15     +3     

Flag	Coverage Δ
cypress	69.70% <86.84%> (-0.01%)	⬇️
jest	58.15% <87.17%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cypress]

codap-v3    Run #10747

Run Properties:   Passed #10747  •  7303ffa213: Increment the build number

Project	codap-v3
Branch Review	main
Run status	Passed #10747
Run duration	02m 08s
Commit	7303ffa213: Increment the build number
Committer	github-actions[bot]
View all properties for this run ↗︎

---

Test results
Failures	0
Flaky	0
Pending	0
Skipped	0
Passing	4
View all changes introduced in this branch ↗︎

[Copilot]

Pull request overview

This PR updates DataSet case/item-id validation caching to avoid MobX constraint violations by replacing multiple observable cache guards with plain variables and a single observable “version” counter, aligning with the recently introduced Collection._cacheVersion pattern.

Changes:

• Replace observable.box cache guards in DataSet.extend() with plain variables + _caseValidationVersion observable counter.
• Move invalidateCases() / invalidateItemIds() to actions and adjust validation/selection cleanup behavior.
• Update case-table useRows reaction logic and add regression tests around MobX reaction behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
v3/src/models/data/data-set.ts	Reworks validation cache/invalidation logic; adds version counter and deferred selection cleanup.
v3/src/models/data/data-set-collections.test.ts	Adds tests asserting MobX reactions fire correctly across invalidation/validation and hierarchy changes.
v3/src/models/data/collection.test.ts	Adds reaction regression test for the collection version-counter approach.
v3/src/components/case-table/use-rows.ts	Updates reaction expression to maintain dependency tracking with new non-observable guards.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

This PR refactors DataSet cache/validation tracking to avoid MobX “observable writes during computed/view evaluation” and fixes stale computed behavior that could permanently seal cached results (notably impacting the Sampler plugin). It also updates a useRows reaction pattern to preserve MobX dependency tracking, and adds Jest tests to cover the new reactivity guarantees.

Changes:

• Replaces multiple observable.box cache guards in DataSet.extend() with plain variables + a single observable version counter used to keep computeds/reactors reactive.
• Moves invalidation helpers into actions and defers selection cleanup so observable writes are consolidated into a single runInAction flush after validation.
• Updates use-rows.ts reaction derivation and adds new Jest tests for DataSet/Collection MobX reactivity and the Sampler oscillation scenario.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
v3/src/models/data/data-set.ts	Refactors case/item-id validation caches and invalidation, introduces _caseValidationVersion, and defers selection cleanup to a controlled flush.
v3/src/components/case-table/use-rows.ts	Ensures the validation reaction always reads validationCount first to maintain MobX dependencies.
v3/src/models/data/data-set-collections.test.ts	Adds tests for invalidate/validate reactivity, selection cleanup deferral, snapshot validation behavior, and the Sampler oscillation regression.
v3/src/models/data/collection.test.ts	Adds a regression test ensuring reactions fire via Collection’s cache version counter.

Comments suppressed due to low confidence (1)

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

• appendItemIdsToCache() mutates _cachedItemIds/_cachedItems in place. Since itemIds/items are MobX computed getters returning these arrays by reference, observers (e.g. React observer renders or reaction(() => data.items.length)) may not be notified on subsequent appends—especially after the first cache-valid re-evaluation drops the _itemIds dependency and the computed becomes effectively tied only to _caseValidationVersion (which isn’t bumped here). Consider (1) updating the cache via replacement (new array references, like Collection’s append path) and (2) bumping an observable version counter when the cache is updated so appends reliably trigger observers.

      appendItemIdsToCache(itemIds: string[]) {
        if (_isValidItemIds) {
          const visibleIds = itemIds.filter(itemId =>
            !self.setAsideItemIdsSet.has(itemId) && !self.filteredOutItemIds.has(itemId)
          )
          _cachedItemIds.push(...visibleIds)
          _cachedItems.push(...visibleIds.map(id => ({ __id__: id })))
          // Invalidate the hash caches so they recompute from the updated _cachedItemIds.
          // The invalidation is observable, so MobX reactions tracking the hashes will fire.
          _itemIdsHash.invalidate()
          _itemIdsOrderedHash.invalidate()
        }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[emcelroy]

Looks good 👍