fix: Calculated attributes don't always update on graphs (CODAP-1439)#2657
Conversation
codap-v3
|
||||||||||||||||||||||||||||
| Project |
codap-v3
|
| Branch Review |
main
|
| Run status |
|
| Run duration | 08m 19s |
| Commit |
|
| Committer | null |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
0
|
|
|
82
|
|
|
0
|
|
|
380
|
| View all changes introduced in this branch ↗︎ | |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2657 +/- ##
===========================================
+ Coverage 71.74% 87.46% +15.72%
===========================================
Files 800 800
Lines 45800 45811 +11
Branches 11209 11648 +439
===========================================
+ Hits 32860 40070 +7210
+ Misses 12925 5728 -7197
+ Partials 15 13 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes CODAP-1439 by ensuring graphs immediately refresh point positions and grow-fit numeric axis domains when calculated/formula attribute values change, including additional left-y (y+) and rightNumeric (y2) axes.
Changes:
- Add
plottedAttributeIDsto include all plotted attributes (including y+/y2) and use it to avoid skipping relevant formula-driven updates. - Expand
numericAttrsto include additional left-y/y2 numeric attributes so their value changes invalidate numeric-value caches. - Introduce
growNumericAxesToFit()and acasesChangeCount-driven reaction to grow-fit numeric axes after value changes (and reuse it for add-cases behavior).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| v3/src/components/graph/models/graph-data-configuration-model.ts | Adds plottedAttributeIDs and expands numeric-attribute tracking for cache invalidation (y+/y2). |
| v3/src/components/graph/models/graph-data-configuration-model.test.ts | Adds unit coverage for plottedAttributeIDs including y+/y2 IDs. |
| v3/src/components/graph/models/graph-content-model.ts | Adds grow-only axis fitting helper and reaction to refit numeric axes after case value changes. |
| v3/src/components/graph/models/graph-content-model.test.ts | Adds reaction-driven rescale tests using setComputedCaseValues (formula path), including y2-only coverage and cache prepopulation. |
| v3/src/components/graph/hooks/use-plot.ts | Switches computed-value refresh filtering from uniqueAttributes to plottedAttributeIDs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The casesChangeCount rescale reaction called growNumericAxesToFit on every value change, formula recalc, and case removal. For clamped count/percent axes (bar charts and histograms), setNiceDomain refit them tightly, so a user- or plugin-set count-axis max was silently discarded on any such change, and removeCases could shrink the axis. - Add a growOnly option to setNiceDomain; when set, the clamp branch no longer opts into shrinking, so setDomain's grow-only clamp preserves a larger existing bound. growNumericAxesToFit now passes growOnly. - Drop the redundant growNumericAxesToFit call from the addCases handler; addCases bumps casesChangeCount, so the reaction already covers it. The handler now only resyncs a binned plot's primary axis. - Add tests: grow-only clamp behavior in setNiceDomain, and a bar chart count axis that keeps a user-set max across a value change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kswenson
left a comment
There was a problem hiding this comment.
👍 Looks good -- the following review was developed in conjunction with Claude Code 🤖.
Approving. The two-part fix is sound: the use-plot guard now filters against plottedAttributeIDs (a strict superset of uniqueAttributes, so it can only allow more refreshes through), and the casesChangeCount rescale reaction is correctly ordered — MST transaction batching guarantees it reads post-invalidation values. I also confirmed the numericAttrs broadening has a contained blast radius (its sole consumer is the getCellKeys reaction, which doesn't depend on .role).
During review I pushed one follow-up commit (43c73bb, with the author's OK) addressing three findings:
- Grow-only for clamped axes. The new rescale reaction ran
growNumericAxesToFiton every value change, formula recalc, andremoveCases. For clamped count/percent axes (bar charts, histograms),setNiceDomainrefit them tightly, so a user- or plugin-set count-axis max was silently discarded on any such change (andremoveCasescould shrink the axis). Added agrowOnlyoption tosetNiceDomain;growNumericAxesToFitnow passes it so those axes are grown, never shrunk. The bar-chart/plot-model resync paths (which don't pass the flag) still tight-fit as before. - Redundant refit on
addCases. Dropped thegrowNumericAxesToFit()call in theaddCaseshandler —addCasesbumpscasesChangeCount, so the reaction already covers it. The handler now only resyncs a binned plot's primary axis. - Tests. Added
setNiceDomaingrow-only unit tests and a bar-chart count-axis regression test (verified it fails without the grow-only fix).
670 graph + axis Jest tests pass locally; typecheck and lint clean.
Fixes CODAP-1439. When a calculated (formula) attribute was plotted on a graph, changing its formula didn't reliably update the graph:
Both symptoms are fixed: the dots move immediately, and the numeric axes grow to fit values that move outside the current view.
These changes also fix CODAP-1444: Graph with computed values as second y attribute doesn't update when slider is dragged
Root causes
Dots not moving (2nd-y / y2). The value-change responder in
use-plot.tsskips refreshing when a formula recalc affects no plotted attribute, checked againstuniqueAttributes. But the graph'sattributeDescriptionsoverride omits the additional left-y and the y2/rightNumericattributes, souniqueAttributesnever contained them and their recalcs were filtered out.No rescale on value change. Nothing refit axis domains when values changed — only
addCasesdid. Compounding this,numericValuesForRolereads a manually-invalidated cache (numericValuesForAttribute) that is only cleared byclearCasesCache, which fires from a reaction keyed onnumericAttrs— andnumericAttrsexcluded the 2nd-y/y2 attributes, so their value changes never invalidated the cache. A naïve synchronous rescale also reads this cache before the invalidation reaction runs.Changes
plottedAttributeIDs(new view) — union ofuniqueAttributesandyAttributeIDs; the value-change responder now filters against it so 2nd-y/y2 recalcs move the dots.numericAttrsnow includes the additional left-y and y2 numeric attributes, so their value changes invalidate the cases cache (fixing cache coherence for the rescale and other consumers such as adornments).growNumericAxesToFit()(new grow-only action) grows numeric axes to fit current data without shrinking, so user/plugin-set bounds are preserved and a binned plot's primary axis is left to its own bin sync. It's driven by a reaction oncasesChangeCount, whichclearCasesCachebumps after invalidating the cache — guaranteeing the rescale reads post-change values. TheaddCaseshandler now shares this helper.Rescale is grow-only (never shrinks) — consistent with the existing
addCasesbehavior — so it fits outliers without discarding manually adjusted bounds.Testing
plottedAttributeIDs(includes 2nd-y/y2 thatuniqueAttributesomits).setComputedCaseValuesformula path: left-y grows, grow-only leaves in-range domains unchanged, and a y2-only attribute grows its axis (with the cache pre-populated, reproducing the reported scenario).Manual verification
Confirmed against the shared document from the story: editing the "calculated values" formula now updates and rescales all graphs without a manual resize.