fix: MST warning on date axis

kswenson commented 3 weeks ago

The useSubAxis hook makes use of an AxisHelper instance, which was being created/stored in a local variable using useMemo(). When transitioning from an empty axis to a date axis, a MobX reaction within useSubAxis was being triggered before the useSubAxis hook was re-rendered to update the axis helper instance, which meant that a stale axis helper connected to a defunct axis model was being called.

Instead of storing the axis helper instance in a local variable we now store it in a WeakMap keyed by the AxisModel, and look it up when needed so that the axis helper used will always be the one associated with the correct axis model. Because it's a WeakMap, axis helpers associated with defunct axis models will automatically be garbage-collected, so we don't need to worry about cleanup.

Separately, there was another reaction in the useAxis hook which was turned into a mstReaction to avoid another MST warning on destruction of the axis model.

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 85.29412% with 5 lines in your changes missing coverage. Please review.

Project coverage is 84.79%. Comparing base (46787cf) to head (7eaa312). Report is 2 commits behind head on main.

Files	Patch %	Lines
v3/src/components/axis/hooks/use-sub-axis.ts	86.66%	4 Missing :warning:
...3/src/components/axis/helper-models/axis-helper.ts	0.00%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1412 +/- ## ========================================== - Coverage 84.79% 84.79% -0.01% ========================================== Files 539 539 Lines 26612 26624 +12 Branches 7206 7260 +54 ========================================== + Hits 22566 22576 +10 - Misses 3740 3742 +2 Partials 306 306 ``` | [Flag](https://app.codecov.io/gh/concord-consortium/codap/pull/1412/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=concord-consortium) | Coverage Δ | | |---|---|---| | [cypress](https://app.codecov.io/gh/concord-consortium/codap/pull/1412/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=concord-consortium) | `71.44% <87.09%> (+0.01%)` | :arrow_up: | | [jest](https://app.codecov.io/gh/concord-consortium/codap/pull/1412/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=concord-consortium) | `53.26% <11.76%> (-0.02%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=concord-consortium#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cypress[bot] commented 3 weeks ago

Test summary

204 • 0 • 29 • 0 • 1

Run details


Project	codap-v3
Status	Passed
Commit	12fe766989
Started	Aug 18, 2024 5:23 PM
Ended	Aug 18, 2024 5:32 PM
Duration	08:19 💡
OS	Linux Ubuntu -
Browser	Chrome 127

View run in Cypress Cloud ➡️

Flakiness

	cypress/e2e/bivariate-adornments.spec.ts		1
1	Graph adornments > adds squares of residuals squares to the plot when the Squares of Residuals checkbox is checked

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud

kswenson commented 3 weeks ago

👍🏻LGTM This PR appears not only to have fixed the MST warnings issue but also to have fixed the problem I observed when switching attributes in a scatterplot. Cool. (I'll be asking you about that!)

I didn't see the infinite loop behavior you described after I fixed the first issue with the helper confusion, so I assume it was another symptom of using a stale helper. 🤷

concord-consortium / codap