Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: crash on delete DataSet #1531

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

kswenson
Copy link
Member

@kswenson kswenson commented Oct 2, 2024

There were several things going on here:

  • The primary cause of the hang/crash was an infinite loop in the GraphDataConfigurationModel's attempt to synchronize its filteredCases array after its DataSet was destroyed.

Once that was fixed, there were still several clients that were attempting to access the defunct DataSet triggering warnings. These have also been fixed:

  • The FilteredCases class now responds to disposal of its DataSet, thus preventing further access.
  • The CaseTile's HideShowMenuList checks whether its DataSet is alive before accessing it. (We conventionally avoid isAlive in models, but in components there often isn't a better way.)
  • The CountAdornmentComponent was properly using mstAutorun, but the model it was attached to was the CountAdornmentModel, not the axis models that were also accessed within the autorun.
  • Therefore, mstAutorun and mstReaction have been extended to support an array of model dependencies or a single model and so now the CountAdornmentComponent can specify all of its model dependencies. (I have wondered for some time whether we would ever encounter a situation in which a single model dependency was insufficient for these utilities and now we have.)

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.09%. Comparing base (4f8ddcc) to head (7cfddb4).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1531      +/-   ##
==========================================
- Coverage   85.10%   85.09%   -0.01%     
==========================================
  Files         563      563              
  Lines       28149    28157       +8     
  Branches     7728     7240     -488     
==========================================
+ Hits        23955    23961       +6     
- Misses       3888     4040     +152     
+ Partials      306      156     -150     
Flag Coverage Δ
cypress 74.39% <93.75%> (-0.01%) ⬇️
jest 53.34% <94.44%> (+<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.

Copy link

cypress bot commented Oct 2, 2024

codap-v3    Run #4521

Run Properties:  status check passed Passed #4521  •  git commit 7cfddb46f8: fix: crash on delete DataSet
Project codap-v3
Branch Review 188236957-fix-delete-data-set
Run status status check passed Passed #4521
Run duration 08m 44s
Commit git commit 7cfddb46f8: fix: crash on delete DataSet
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 30
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 227
View all changes introduced in this branch ↗︎

@kswenson kswenson marked this pull request as ready for review October 2, 2024 20:01
@kswenson kswenson requested a review from bfinzer October 2, 2024 20:01
@kswenson kswenson added the v3 CODAP v3 label Oct 2, 2024
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.

1 participant