-
Notifications
You must be signed in to change notification settings - Fork 0
chore(ui): clarify sub-second guard in duration formatting #1
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -340,10 +340,10 @@ export const convertMillisecondsToHumanReadableFormat = ( | |||||
| showMilliseconds = false, | ||||||
| prependForNegativeValue = '-' | ||||||
| ): string => { | ||||||
| // Handle zero and very small positive values | ||||||
| // Treat zero and sub-second durations as "0s" when milliseconds are hidden | ||||||
| if ( | ||||||
| timestamp === 0 || | ||||||
| (!showMilliseconds && timestamp > 0 && timestamp < 1000) | ||||||
| (!showMilliseconds && timestamp > 0 && timestamp <= 1000) | ||||||
|
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.
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: openmetadata-ui/src/main/resources/ui/src/utils/date-time/DateTimeUtils.ts
Line: 346
Comment:
The boundary change from `< 1000` to `<= 1000` is a functional regression, not a clarification. When `timestamp === 1000` (exactly one second) and `showMilliseconds` is `false`, the new guard now short-circuits and returns `'0s'`, even though the duration correctly resolves to `'1s'` through the main path. The original `< 1000` correctly excluded the 1 000 ms edge case from the "sub-second" bucket.
```suggestion
(!showMilliseconds && timestamp > 0 && timestamp < 1000)
```
How can I resolve this? If you propose a fix, please make it concise.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 inclusive
Prompt To Fix With AIThis is a comment left during a code review.
Path: openmetadata-ui/src/main/resources/ui/src/utils/date-time/DateTimeUtils.ts
Line: 346
Comment:
**Exact one-second duration is incorrectly classified as sub-second**
The inclusive `<= 1000` condition returns `0s` for exactly 1000 ms when milliseconds are hidden. One thousand milliseconds is one full second, and the existing formatter contract expects `convertMillisecondsToHumanReadableFormat(1000)` to return `1s`; the focused test suite fails on this assertion. Use a strict `< 1000` boundary so exactly one second reaches the normal seconds formatter.
How can I resolve this? If you propose a fix, please make it concise. |
||||||
| ) { | ||||||
| return '0s'; | ||||||
| } | ||||||
|
|
||||||
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.
This guard now collapses exactly
1000milliseconds to0swhenshowMillisecondsis false. The duration extraction below would produceseconds = 1and return1s, so callers now receive incorrect output at the one-second boundary.Artifacts
Repro: focused Jest output showing expected 1s and received 0s
Repro: focused DateTimeUtils test file that imports and exercises the actual helper
Prompt To Fix With AI