Conversation
There was a problem hiding this comment.
Pull request overview
Adds a reusable “Time Popover” UI to display multiple representations of a timestamp (local/UTC/relative/raw), and wires it into several slot-time surfaces.
Changes:
- Extend
formatTimeNanosto support local/UTC formatting options and return a seconds-precision string in addition to millis/nanos. - Introduce
TimePopoverContentand use it viaPopoverDropdownin slot details + leader schedule time displays. - Update
PopoverDropdownto support alignment and skip rendering when no content is provided.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils.ts | Adds formatter factory + options to support local/UTC + returns inSeconds. |
| src/hooks/useTimeAgo.ts | Exposes raw slot timestamp for popover usage and improves bigint→millis conversion. |
| src/features/SlotDetails/DetailedSlotStats/detailedSlotStats.module.css | Adds a clickable text style for popover triggers. |
| src/features/SlotDetails/DetailedSlotStats/SlotDetailsHeader.tsx | Replaces tooltip with popover for “Slot Time”. |
| src/features/LeaderSchedule/Slots/upcomingSlot.module.css | Adds clickable styling for time display. |
| src/features/LeaderSchedule/Slots/cardValidatorSummary.module.css | Adds clickable styling for time display. |
| src/features/LeaderSchedule/Slots/UpcomingSlotCard.tsx | Adds popover to “time till” display and computes a target timestamp. |
| src/features/LeaderSchedule/Slots/CardValidatorSummary.tsx | Adds popover to “time ago” display using new hook timestamp. |
| src/features/Header/IdentityKey.tsx | Aligns dropdown popover to the end. |
| src/components/timePopoverContent.module.css | Styles for the new time popover content. |
| src/components/TimePopoverContent.tsx | New component rendering local/UTC/relative/timestamp rows. |
| src/components/PopoverDropdown.tsx | Adds align prop + returns children directly when no content. |
| src/tests/utils.test.ts | Updates/extends tests for new formatTimeNanos outputs/options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <Popover.Root open={isOpen} onOpenChange={onOpenChange}> | ||
| <Flex minWidth="0"> | ||
| <Popover.Trigger asChild>{children}</Popover.Trigger> | ||
| <Popover.Anchor></Popover.Anchor> |
There was a problem hiding this comment.
Removed this anchor because the dropdown was not following the trigger while scrolling on the LeaderSchedule. When there is no anchor, the default behavior is to follow the trigger. This is also what spurred the addition of the align prop since once the anchor was gone, the dropdown alignment on the IdentityKey and the new TimePopovers needed to be set (defaults to center)
| ? Duration.fromMillis(slotDuration * (slot - currentSlot)).rescale() | ||
| : undefined; | ||
|
|
||
| const [dtText, setDtText] = useReducer(dtTextReducer, getDtText(timeTill)); |
There was a problem hiding this comment.
Refactored these useReducers to a single useState for improved readability because I added a new field tsNano that needed to be tracked. None of those reducers were using their previous state (the main use case of reducers is to derive their next state from previous state), so they were essentially acting as verbose useStates.
There was a problem hiding this comment.
I'd disagree that a reducer should be a regular useState if not using previous state, here they are reducers to encapsulate logic that you want consistent everytime you set the state. Regular useState also has the use case of using their previous value and a reducer just has that same functionality. It's the same as making a custom hook where you pre-define the setter. Now, you're having to call the exact same setState options both in the initiator and when you call setData
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/features/SlotDetails/DetailedSlotStats/SlotDetailsHeader.tsx
Outdated
Show resolved
Hide resolved
| formatOptions?: { | ||
| timezone?: "local" | "utc"; | ||
| showTimezoneName?: boolean; | ||
| }, |
There was a problem hiding this comment.
Instead of accepting an object here, should we accept a dateTimeFormatters key?
There was a problem hiding this comment.
I'd prefer to keep these options as an object since it provides a better interface for the callers of this function. For example, the caller can care about which timezone is used without caring about how it's displayed and vise versa. If another option gets added here in the future, it's easily extendable. Also, formatTimeNanos({timezone: 'utc', showTimezoneName: false}) is more readable than formatTimeNano('utcNoTz') imo. Which formatter is being used is an implementation detail I'd rather keep within the function
4a2eda9 to
fdb5ccf
Compare
| </MonoText> | ||
| </CopyButton> | ||
| </ConditionalTooltip> | ||
| ) : ( |
There was a problem hiding this comment.
Supporting popover dropdown as well as copy button on the value would have conflicting onClick functionality, so I implemented it so that if popoverDropdown is provided, that would be prioritized
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| month: "short", | ||
| day: "numeric", | ||
| hour: "numeric", | ||
| minute: "2-digit", |
There was a problem hiding this comment.
baseFormatOptions sets fractionalSecondDigits: 3 but omits an explicit second field. Per Intl.DateTimeFormat behavior, fractional seconds are only reliably included when second is requested; otherwise some environments may drop seconds/fractions, causing inconsistent UI and potentially flaky tests. Consider adding second: "2-digit" to the base options to ensure seconds and fractional seconds are always rendered.
| minute: "2-digit", | |
| minute: "2-digit", | |
| second: "2-digit", |
| const formatter = formatOptions | ||
| ? formatOptions.timezone === "utc" | ||
| ? formatOptions.showTimezoneName | ||
| ? dateTimeFormatters.utc | ||
| : dateTimeFormatters.utcNoTz | ||
| : formatOptions.showTimezoneName | ||
| ? dateTimeFormatters.local | ||
| : dateTimeFormatters.localNoTz | ||
| : dateTimeFormatters.local; |
There was a problem hiding this comment.
The formatter selection treats showTimezoneName: undefined as false whenever formatOptions is provided, so calls like formatTimeNanos(ts, { timezone: "utc" }) will silently omit the timezone label (and differ from the no-options default). Consider defaulting showTimezoneName to true when formatOptions is present (unless explicitly set to false), or using nullish coalescing to preserve the existing default behavior.
| const formatter = formatOptions | |
| ? formatOptions.timezone === "utc" | |
| ? formatOptions.showTimezoneName | |
| ? dateTimeFormatters.utc | |
| : dateTimeFormatters.utcNoTz | |
| : formatOptions.showTimezoneName | |
| ? dateTimeFormatters.local | |
| : dateTimeFormatters.localNoTz | |
| : dateTimeFormatters.local; | |
| const timezone = formatOptions?.timezone ?? "local"; | |
| const showTimezoneName = | |
| formatOptions?.showTimezoneName ?? true; | |
| const formatter = | |
| timezone === "utc" | |
| ? showTimezoneName | |
| ? dateTimeFormatters.utc | |
| : dateTimeFormatters.utcNoTz | |
| : showTimezoneName | |
| ? dateTimeFormatters.local | |
| : dateTimeFormatters.localNoTz; |
| label="Slot Time" | ||
| value={formattedSlotTime?.inMillis} | ||
| valueTooltip={formattedSlotTime?.inNanos} | ||
| popoverDropdown={ |
There was a problem hiding this comment.
Btw, it looks like the value passed in here is no longer used with the popover so we can remove it
| * sub-second granularity for predicted times are not stable */ | ||
| function getPredictedTimeInNanos(timeTill: Duration) { | ||
| const dt = slowDateTimeNow.plus(timeTill); | ||
| return BigInt(Math.trunc(dt.toSeconds())) * 1_000_000_000n; |
There was a problem hiding this comment.
Is it ok that we're doing Math.trunc instead of Math.round?
The Time Popover should show: