Fix crash when there are undefined references in the state tree e.g. when a track is deleted but still referred to by a session #1597
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1597 +/- ##
==========================================
+ Coverage 59.18% 59.20% +0.01%
==========================================
Files 438 438
Lines 19927 19948 +21
Branches 4680 4686 +6
==========================================
+ Hits 11794 11810 +16
- Misses 7839 7844 +5
Partials 294 294
Continue to review full report at Codecov.
|
|
can you check the diff for this being changed? also I will just pontificate for a moment: the change to rootmodel means this logic is centralized which is nice in some ways but in other ways, any user of say @jbrowse/react-linear-genome-view will not get this functionality and thus needs to duplicate it in their own app there are a number of types of things like this where our jbrowse 2 app has nice features, but embedded mode kind of leaves some stuff to be wanted. this is another thing about our general messaging about embedded mode: it's not necessarily a batteries included product |
|
meant to say: can you check the diff for the CLI README being changed? just curious if that's a real thing or some artifact |
|
I think thats an artifact, didnt touch anything related to the CLI |
|
maybe these commits need to be rebased onto master? that CLI readme thing shouldn't be in this PR |
peterkxie
force-pushed
the
560_fix_jbweb_crash
branch
from
94bed29 to
ad9a2fd
Compare
|
rebased the branch, CLI seems to be gone from diff |
| const parent = getContainingView(self) as DotplotViewModel | ||
| autorun(reaction => { | ||
| if (parent.initialized) { | ||
| makeAbortableReaction( | ||
| self as any, | ||
| renderBlockData, | ||
| renderBlockEffect as any, | ||
| { | ||
| name: `${self.type} ${self.id} rendering`, | ||
| delay: 1000, | ||
| fireImmediately: true, | ||
| }, | ||
| this.setLoading, | ||
| this.setRendered, | ||
| this.setError, | ||
| ) | ||
| reaction.dispose() | ||
| } | ||
| }) |
There was a problem hiding this comment.
This nested autorun-reaction thing is a little frightening, I'm not sure what it might do in corner cases.
What if you added the if(parent.initialized) logic to the renderBlockData and renderBlockEffect functions instead?
garrettjstevens
added
the
needs label triage
peterkxie
added
bug
|
@peterkxie i think this code is ok and ready to merge. the dotplot view is simply a flaky test #1221 in order to properly fix #1221 I think what we would want to do is insert console.log statements at many positions leading up to where it is generating the prerendered_canvas, and see why it gets stuck and doesn't get there |
cmdcolin
force-pushed
the
560_fix_jbweb_crash
branch
from
9152109 to
37e4d54
Compare
cmdcolin
changed the title
Uses @rbuels #1353 fix that filters out undefined references at loadtime while making it work with dotplot. Autorun is added to dotplot model to only making the abortable action after the view is initialized. Closes #560