Compare perf: Fix missing unit in grouped predicate entry #3987
+3
−3
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
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a bug in the compare performance view where the unit (e.g. "ms") was missing for grouped predicate entries. The changes update the function signature and its calls to ensure that the unit is provided.
- Updated renderOptionalValue signature to reflect the expected unit parameter.
- Modified calls to renderOptionalValue to supply the unit from metric.unit.
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
| @@ -81,7 +81,7 @@ class ComparisonDataset { | |||
| } | |||
| } | |||
|
|
|||
| function renderOptionalValue(x: Optional<number>, unit?: string) { | |||
| function renderOptionalValue(x: Optional<number>, unit: string | undefined) { | |||
There was a problem hiding this comment.
If the 'unit' parameter is intended to be mandatory as mentioned in the PR description, consider updating its type to 'string' instead of 'string | undefined' and ensure that all call sites provide a valid string.
Suggested change
| function renderOptionalValue(x: Optional<number>, unit: string | undefined) { | |
| function renderOptionalValue(x: Optional<number>, unit: string) { |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
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 a minor bug in the compare performance view, where the "ms" unit was not displayed on a grouped predicate entry.
Since the optional parameter
unitis now provided at all call sites, and its absence was the cause of the bug, I also decided to make the parameter mandatory.Before: missing unit
The unit was missing next to the first
2in this screenshot:After: unit is there
(sorry, I didn't have the same query history item available for both screenshots)