-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat: show dash instead of zero for missing data on evals page #7748
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
841c6c7
b8463b7
9943604
7c7fd6e
668859b
374e9ea
be1ef46
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,8 @@ | ||
| export const formatDuration = (durationMs: number) => { | ||
| export const formatDuration = (durationMs: number | null | undefined) => { | ||
| if (durationMs === null || durationMs === undefined) { | ||
| return "-" | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same null-checking pattern as the other formatters. Could we consider extracting this into a shared utility like |
||
| } | ||
|
|
||
| const seconds = Math.floor(durationMs / 1000) | ||
| const hours = Math.floor(seconds / 3600) | ||
| const minutes = Math.floor((seconds % 3600) / 60) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,8 @@ | ||
| export const formatTokens = (tokens: number, decimals = 0) => { | ||
| export const formatTokens = (tokens: number | null | undefined, decimals = 0) => { | ||
| if (tokens === null || tokens === undefined) { | ||
| return "-" | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The chart rendering code in evals.tsx still passes potentially null/undefined values to formatCurrency within the chart components. Is the Recharts library handling these gracefully, or should we add null checks before passing to the chart components? |
||
| } | ||
|
|
||
| if (tokens < 1000) { | ||
| return tokens.toString() | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be good to add unit tests for this new null/undefined handling behavior? I notice there aren't any tests for these formatting functions yet, and it would help ensure the "-" fallback works correctly across all edge cases.