Bugfix in Entropy panel when window resizes

[jameshadfield]

The bug arose because window-resizes trigger a complete re-render of the entropy panel's SVG and this didn't correctly handle an edge case where the entropy data was stale (a concept introduced in #1879).

For specific steps to reproduce see #1895 (comment)

Closes #1895

[corneliusroemer]

Thanks @jameshadfield! I don't remember every resizing the window, that was definitely not a prerequisite of encountering the bug for me. It seems to me that there might be more cases where stale entropy panel can happen.

So I'd be hesitant to consider the bug fix without further testing. How can I best help there?

It's of course possible that resizing is just one of the things that trigger an underlying issue that has been fixed now.

[corneliusroemer]

Maybe you can just broaden the PR description to state that resizing was one way that this bug was hit, but in my experience there are others.

It seems that this fix here is general, improving null handling - the perf improvements resulted in bars now sometimes being null but this wasn't handled then. As Ivan said ok the issue, typescript might help here!

[corneliusroemer]

Considering the code alone, this is definitely an improvement, so I'm not at all against merging, I'm very much for merging, I'm just hesitant to consider the bug unequivocally fixed by this. It likely is but I'd like to do more testing for an extended period of time in real life (on next.nextstrain.org).

So we should probably merge, release, put on next as canary and then let it sit there for a while watching if anyone hits the whiteout. Then after say a week if no one hits it deploy to nextstrain.org

[corneliusroemer]

Approve with request to put this on next.nextstrain.org for a few days for testing for bug tonne fixed

[joverlee521]

So I'd be hesitant to consider the bug fix without further testing. How can I best help there?

@corneliusroemer it's possible to test PRs on downstream repos. I've added the preview on nextstrain.org label to trigger the test app for nextstrain.org.

You can try out the deployed test app at https://nextstrain-s-nextstrain-c6gsn5.herokuapp.com/ to see if you still run into the bug.

[corneliusroemer]

Thanks @joverlee521. It'll be hard to use this as daily driver like next.nextstrain.org for things like Usher so probably won't work in practice (given the bug happened in ways that I can't reproduce exactly).

The more I think about it the more confident I am that this is also the line of code that I hit. It's obviously a type error, no null handling.

We should just merge this and put on next.nextstrain.org 😄

[jameshadfield]

I'm just hesitant to consider the bug unequivocally fixed by this. It likely is but I'd like to do more testing for an extended period of time in real life (on next.nextstrain.org).

Agreed. I think there's a few things that will act synergistically here:

1. Individual fixes like this. Somewhat hard to cover all the use cases in such a dynamic app, so realistically this alone isn't enough.
2. Error boundaries so that when something slips through it'll just crash the panel and not Auspice. That's Error boundaries for panels #1897. (Doesn't help if the bug is not within react components, i.e. the rendering parts, but it's a big improvement nonetheless.)
3. Adding types throughout auspice which will enforce us to handle these things. TypeScript: Address strictNullChecks violations #1888 is important here.

[corneliusroemer]

Thanks! I'll test it eagerly as soon as new Auspice is on next.nextstrain.org

[victorlin]

next.nextstrain.org has been updated: nextstrain/nextstrain.org#1069