static-site[list-resources]: Fix crash in modal_draw() when bottoming out during radius calculations #1253
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… out during radius calculations
Fixes the algorithm which calculates an ideal radius for the points on
the beeswarm plot by ensuring that beeswarmData, beeswarmHeight, and
radius are all set to their last computed values when bottoming out as
well as when topping out. Simplifies state tracking instead of doubling
variables unnecessarily.
Previously when bottoming out on downward iterations, the algorithm
would leave beeswarmData unset, which caused errors like this from
inside d3:
TypeError: can't access property Symbol.iterator, items is undefined
This was triggered when many data points were close together causing
beeswarm heights to soar.
Bottoming out will mean that some points in the plot are cut off, i.e.
above the top of the chart. This seems preferable to a crash (or
unclickably tiny points). Future improvements could dynamically scale
the chart height in such cases, or alternatively the chart width to
spread out points horizontally and make the longer timeline scrollable.
Resolves: <#1252>
nextstrain-bot
temporarily deployed
to
nextstrain-s-trs-fix-pr-nwo4pg
victorlin
reviewed
Nov 12, 2025
Comment on lines
+102
to
+113
| beeswarmData = dodge(flatData, { | ||
| radius: radius * 2 + padding, | ||
| x: (d) => x(d["date"]), | ||
| }); | ||
| const nextBeeswarmHeight = d3.max(nextBeeswarmData.map((d) => d.y)); | ||
| beeswarmHeight = d3.max(beeswarmData.map((d) => d.y)); | ||
| const nextSpareHeight = | ||
| availBeeswarmHeight - nextBeeswarmHeight - nextRadius; | ||
| availBeeswarmHeight - beeswarmHeight - radius; | ||
| if (nextSpareHeight <= spareHeight && nextSpareHeight > 0) { | ||
| beeswarmData = nextBeeswarmData; | ||
| beeswarmHeight = nextBeeswarmHeight; | ||
| spareHeight = nextSpareHeight; | ||
| radius = nextRadius; | ||
| nextRadius += radiusJump; | ||
| radius += radiusJump; | ||
| } else { | ||
| nextRadius -= radiusJump; | ||
| radius -= radiusJump; |
Member
There was a problem hiding this comment.
This fixes the bug, but the radius seems off:
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes the algorithm which calculates an ideal radius for the points on the beeswarm plot by ensuring that beeswarmData, beeswarmHeight, and radius are all set to their last computed values when bottoming out as well as when topping out. Simplifies state tracking instead of doubling variables unnecessarily.
Previously when bottoming out on downward iterations, the algorithm would leave beeswarmData unset, which caused errors like this from inside d3:
This was triggered when many data points were close together causing beeswarm heights to soar.
Bottoming out will mean that some points in the plot are cut off, i.e. above the top of the chart. This seems preferable to a crash (or unclickably tiny points). Future improvements could dynamically scale the chart height in such cases, or alternatively the chart width to spread out points horizontally and make the longer timeline scrollable.
Resolves: #1252
Checklist