fix(ui): Fix last release bubble bucket #86956
Open
+76
−17
Conversation
This file contains 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
github-actions
bot
added
the
Scope: Frontend
❌ 4 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
billyvg
force-pushed
the
fix-ui-release-bubbles-missing-most-recent-bucket
branch
from
9080779 to
0be4c82
Compare
billyvg
force-pushed
the
fix-ui-release-bubbles-missing-most-recent-bucket
branch
from
6aeb6eb to
6317e09
Compare
billyvg
commented
Mar 13, 2025
| currentBucket.releases.push(release); | ||
| } else if (releaseTs > maxTime) { |
There was a problem hiding this comment.
I suppose we don't necessarily need this based on our assumption about releases being properly constrained to the correct timeframe, but this was to prevent adding releases with date < minTs into the last bucket.
| // the last bucket, as the last buckets width can be less than the | ||
| // others. (i.e. if the total time difference is not perfectly divible by | ||
| // `desiredBuckets`, the last bucket will be the remainder) | ||
| if (bucketEndTs && releaseTs <= bucketEndTs) { |
There was a problem hiding this comment.
You were kinda right about this @gggritso
There was a problem hiding this comment.
@billyvg 😂 what did I say about this, I don't remember!
There was a problem hiding this comment.
I dug it up to refer to: #85270 (comment) -- we in fact do want to include releases more recent than the time series. maxTime is equivalent to the latest plotted date -- but the data at that point includes [maxTime, now], but the releases bucket was only including up to maxTime
gggritso
approved these changes
Mar 14, 2025
| // and for its last bucket, it's ending boundary is `maxTime` to | ||
| // correspond to the chart. We cannot change `maxTime` for the buckets | ||
| // otherwise we will draw bubbles past the chart data and it would look | ||
| // broken. |
There was a problem hiding this comment.
Okay I have a question about this. How broken would it look? You're doing a lot of work to jam the latest releases in the last bucket (which is completely fine) but I'm curious what happens if you just don't. If you leave things as-is, and let the final bubble extend the X axis to the current timestamp, what happens? Does it add some blank space on the right side of the chart? Or something worse?
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.
The last release bubble has some more recent releases filtered out because the meaning of
maxTimediffers between the time series plots and the release buckets:This PR changes the bucketing logic so it adds releases >
maxTimeto the last bucket. It assumes we fetch the correct releases outside of the buckets.We will have to fix the tooltip to reflect this without changing
maxTime.ref #85775