Skip to content

CODAP-1117: fix MobX constraint violation in cachedFnWithArgsFactory#2393

Merged
kswenson merged 3 commits intomainfrom
CODAP-1117-cachedFnWithArgsFactory
Feb 25, 2026
Merged

CODAP-1117: fix MobX constraint violation in cachedFnWithArgsFactory#2393
kswenson merged 3 commits intomainfrom
CODAP-1117-cachedFnWithArgsFactory

Conversation

@kswenson
Copy link
Member

@kswenson kswenson commented Feb 15, 2026

Summary

  • Fixes the MobX anti-pattern in cachedFnWithArgsFactory where the getter wrote to an observable.map during computed/view evaluation
  • Replaces the observable map cache with observable version counters (per-key + global) paired with a plain Map, so the getter only reads observables — matching the pattern established by observableCachedFnFactory in PR CODAP-1086: Fix formula recalculation when items are added via plugins #2377
  • Per-key versions (observable.map) handle invalidate(); a global version counter (observable.box) handles invalidateAll()
  • Adds tests verifying per-key reactivity isolation and invalidateAll() behavior

Addresses CODAP-1117

Test plan

  • Existing cachedFnWithArgsFactory tests pass
  • New tests verify invalidate() only triggers reactions for the affected key
  • New tests verify invalidateAll() triggers reactions for all keys
  • data-configuration-model tests pass (consumer of cachedFnWithArgsFactory)
  • CI passes (lint, build, unit tests)
  • Manual smoke test of graph functionality (primary consumer)

🤖 Generated with Claude Code

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 <noreply@anthropic.com>
@kswenson kswenson added the v3 CODAP v3 label Feb 15, 2026
@codecov
Copy link

codecov bot commented Feb 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.25%. Comparing base (2a3224a) to head (12ab8d6).
⚠️ Report is 91 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (2a3224a) and HEAD (12ab8d6). Click for more details.

HEAD has 15 uploads less than BASE
Flag BASE (2a3224a) HEAD (12ab8d6)
cypress 16 1
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2393       +/-   ##
===========================================
- Coverage   85.45%   69.25%   -16.21%     
===========================================
  Files         753      753               
  Lines       41500    41509        +9     
  Branches    10235    10235               
===========================================
- Hits        35462    28745     -6717     
- Misses       6028    12758     +6730     
+ Partials       10        6        -4     
Flag Coverage Δ
cypress 39.64% <100.00%> (-29.92%) ⬇️
jest 56.90% <100.00%> (+<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
Copy link

cypress bot commented Feb 15, 2026

codap-v3    Run #10449

Run Properties:  status check passed Passed #10449  •  git commit 7e3dd37d92: CODAP-1117: fix MobX constraint violation in cachedFnWithArgsFactory (#2393)
Project codap-v3
Branch Review main
Run status status check passed Passed #10449
Run duration 05m 22s
Commit git commit 7e3dd37d92: CODAP-1117: fix MobX constraint violation in cachedFnWithArgsFactory (#2393)
Committer Kirk Swenson
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 73
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 300
View all changes introduced in this branch ↗︎

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a MobX constraint violation in cachedFnWithArgsFactory by removing observable writes from within the cached getter, aligning the implementation with the version-counter pattern introduced for observableCachedFnFactory (PR #2377).

Changes:

  • Replaces the observable-map cache with a plain Map plus observable version counters (per-key + global) so the getter only reads observables.
  • Implements per-key invalidation via an observable map of key versions and global invalidation via an observable box counter.
  • Adds unit tests to verify per-key reaction isolation (invalidate()) and full invalidation behavior (invalidateAll()).

Reviewed changes

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

File Description
v3/src/utilities/mst-utils.ts Refactors cachedFnWithArgsFactory to use MobX version counters + non-observable cache storage to avoid observable writes during derivation.
v3/src/utilities/mst-utils.test.ts Adds MobX reaction-based tests to confirm invalidate() only affects the targeted key and invalidateAll() affects all keys.

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

@kswenson kswenson marked this pull request as ready for review February 15, 2026 08:17
@kswenson kswenson requested a review from scytacki February 15, 2026 08:17
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 <noreply@anthropic.com>
Copy link
Member

@scytacki scytacki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Just a comment about how the test is assuming synchronous reactions.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@kswenson kswenson merged commit 7e3dd37 into main Feb 25, 2026
10 checks passed
@kswenson kswenson deleted the CODAP-1117-cachedFnWithArgsFactory branch February 25, 2026 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v3 CODAP v3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants