Refactor DateTimeRangePicker again (#1247)

* hella basic refetch interval control

* do it on the metrics page instead

* get rid of refetch interval in favor of hard-coded 10s interval

* fix up bad behavior due to not clearing interval when changing preset

* all right forget the immediate: true thing

* useDateTimeRange

* go back to the hook model

* don't do the refetch buttons in this branch

Author: david-crespo

app/components/form/fields/DateTimeRangePicker.spec.tsx

@@ -10,18 +10,27 @@ import { DateTimeRangePicker, dateForInput } from './DateTimeRangePicker'
 const now = new Date(2020, 1, 1)
 
 function renderLastDay() {
-  const onChange = vi.fn()
+  const setStartTime = vi.fn()
+  const setEndTime = vi.fn()
   render(
     <DateTimeRangePicker
       initialPreset="lastDay"
       startTime={subDays(now, 1)}
       endTime={now}
-      onChange={onChange}
+      setStartTime={setStartTime}
+      setEndTime={setEndTime}
     />
   )
-  return { onChange }
+  return { setStartTime, setEndTime }
 }
 
+beforeAll(() => {
+  vi.useFakeTimers()
+  vi.setSystemTime(now)
+
+  return () => vi.useRealTimers()
+})
+
 describe('useDateTimeRangePicker', () => {
   it.each([
     ['lastHour', subHours(now, 1)],
@@ -30,13 +39,13 @@ describe('useDateTimeRangePicker', () => {
     ['lastWeek', subDays(now, 7)],
     ['last30Days', subDays(now, 30)],
   ])('sets initial start and end', (preset, start) => {
-    const onChange = vi.fn()
     render(
       <DateTimeRangePicker
         initialPreset={preset as RangeKey}
         startTime={start}
         endTime={now}
-        onChange={onChange}
+        setStartTime={() => {}}
+        setEndTime={() => {}}
       />
     )
 
@@ -52,36 +61,33 @@ it.each([
   ['Last week', subDays(now, 7)],
   ['Last 30 days', subDays(now, 30)],
 ])('choosing a preset sets the times', (option, start) => {
-  vi.useFakeTimers()
-  vi.setSystemTime(now)
-
-  const { onChange } = renderLastDay()
+  const { setStartTime, setEndTime } = renderLastDay()
 
   clickByRole('button', 'Choose a time range')
   clickByRole('option', option)
 
-  expect(onChange).toBeCalledWith(start, now)
-
-  vi.useRealTimers()
+  expect(setStartTime).toBeCalledWith(start)
+  expect(setEndTime).toBeCalledWith(now)
 })
 
 describe('custom mode', () => {
   it('enables datetime inputs', () => {
-    const { onChange } = renderLastDay()
+    const { setStartTime, setEndTime } = renderLastDay()
 
     expect(screen.getByLabelText('Start time')).toBeDisabled()
 
     clickByRole('button', 'Choose a time range')
     clickByRole('option', 'Custom...')
 
-    expect(onChange).not.toBeCalled()
+    expect(setStartTime).not.toBeCalled()
+    expect(setEndTime).not.toBeCalled()
     expect(screen.getByLabelText('Start time')).toBeEnabled()
     expect(screen.getByRole('button', { name: 'Reset' })).toHaveClass('visually-disabled')
     expect(screen.getByRole('button', { name: 'Load' })).toHaveClass('visually-disabled')
   })
 
   it('clicking load after changing date changes range', async () => {
-    const { onChange } = renderLastDay()
+    const { setStartTime, setEndTime } = renderLastDay()
 
     expect(screen.getByLabelText('Start time')).toHaveValue(dateForInput(subDays(now, 1)))
     expect(screen.getByLabelText('End time')).toHaveValue(dateForInput(now))
@@ -98,15 +104,17 @@ describe('custom mode', () => {
     fireEvent.change(endInput, { target: { value: '2020-01-17T00:00' } })
 
     // changing the input value without clicking Load doesn't do anything
-    expect(onChange).not.toBeCalled()
+    expect(setStartTime).not.toBeCalled()
+    expect(setEndTime).not.toBeCalled()
 
-    // clicking Load calls onChange
+    // clicking Load calls setTime with the new range
     clickByRole('button', 'Load')
-    expect(onChange).toBeCalledWith(new Date(2020, 0, 15), new Date(2020, 0, 17))
+    expect(setStartTime).toBeCalledWith(new Date(2020, 0, 15))
+    expect(setEndTime).toBeCalledWith(new Date(2020, 0, 17))
   })
 
   it('clicking reset after changing inputs resets inputs', async () => {
-    const { onChange } = renderLastDay()
+    const { setStartTime, setEndTime } = renderLastDay()
 
     expect(screen.getByLabelText('Start time')).toHaveValue(dateForInput(subDays(now, 1)))
     expect(screen.getByLabelText('End time')).toHaveValue(dateForInput(now))
@@ -127,8 +135,8 @@ describe('custom mode', () => {
     expect(startInput).toHaveValue('2020-01-31T00:00')
     expect(endInput).toHaveValue('2020-02-01T00:00')
 
-    // onChange is never called
-    expect(onChange).not.toBeCalled()
+    expect(setStartTime).not.toBeCalled()
+    expect(setEndTime).not.toBeCalled()
   })
 
   it('shows error for invalid range', () => {

app/components/form/fields/DateTimeRangePicker.tsx

@@ -1,5 +1,5 @@
 import { format, subDays, subHours } from 'date-fns'
-import { useMemo, useState } from 'react'
+import { useCallback, useMemo, useState } from 'react'
 
 import { Listbox, useInterval } from '@oxide/ui'
 import { Button, TextInput } from '@oxide/ui'
@@ -32,31 +32,25 @@ const computeStart: Record<RangeKey, (now: Date) => Date> = {
 //   - list of presets is hard-coded
 //   - initial preset can't be "custom"
 
-type Props = {
-  initialPreset: RangeKey
-  /**
-   * if set and range is a relative preset, update the range to have `endTime`
-   * of now every X ms
-   */
-  slideInterval?: number
-  startTime: Date
-  endTime: Date
-  onChange: (startTime: Date, endTime: Date) => void
-}
-
-export function useDateTimeRangePickerState(initialPreset: RangeKey) {
-  // default endTime is now, i.e., mount time
+/**
+ * Exposes `startTime` and `endTime` plus the whole set of picker UI controls as
+ * a JSX element to render. When we're using a relative preset like last N
+ * hours, automatically slide the window forward live by updating the range to
+ * have `endTime` of _now_ every `SLIDE_INTERVAL` ms.
+ */
+export function useDateTimeRangePicker(initialPreset: RangeKey) {
   const now = useMemo(() => new Date(), [])
 
   const [startTime, setStartTime] = useState(computeStart[initialPreset](now))
   const [endTime, setEndTime] = useState(now)
 
-  function onChange(newStart: Date, newEnd: Date) {
-    setStartTime(newStart)
-    setEndTime(newEnd)
-  }
+  const props = { initialPreset, startTime, endTime, setStartTime, setEndTime }
 
-  return { startTime, endTime, onChange }
+  return {
+    startTime,
+    endTime,
+    dateTimeRangePicker: <DateTimeRangePicker {...props} />,
+  }
 }
 
 function validateRange(startTime: Date, endTime: Date): string | null {
@@ -67,17 +61,24 @@ function validateRange(startTime: Date, endTime: Date): string | null {
   return null
 }
 
-/**
- * Exposes `startTime` and `endTime` plus the whole set of picker UI controls as
- * a JSX element to render.
- */
+/** Interval for sliding range forward when using a relative time preset */
+const SLIDE_INTERVAL = 10_000
+
+type DateTimeRangePickerProps = {
+  initialPreset: RangeKey
+  startTime: Date
+  endTime: Date
+  setStartTime: (startTime: Date) => void
+  setEndTime: (endTime: Date) => void
+}
+
 export function DateTimeRangePicker({
   initialPreset,
-  slideInterval,
   startTime,
   endTime,
-  onChange,
-}: Props) {
+  setStartTime,
+  setEndTime,
+}: DateTimeRangePickerProps) {
   const [preset, setPreset] = useState<RangeKeyAll>(initialPreset)
 
   // needs a separate pair of values because they can be edited without
@@ -92,22 +93,29 @@ export function DateTimeRangePicker({
 
   const enableInputs = preset === 'custom'
 
-  /** Set the input values and call the passed-on onChange with the new times */
-  function setTimesForPreset(newPreset: RangeKey) {
-    const now = new Date()
-    const newStartTime = computeStart[newPreset](now)
-    onChange(newStartTime, now)
-    setStartTimeInput(newStartTime)
-    setEndTimeInput(now)
-  }
-
-  useInterval(
-    () => {
-      if (preset !== 'custom') setTimesForPreset(preset)
+  // could handle this in a useEffect that looks at `preset`, but that would
+  // also run on initial render, which is silly. Instead explicitly call it on
+  // preset change and in useInterval.
+  const setRange = useCallback(
+    (preset: RangeKeyAll) => {
+      if (preset !== 'custom') {
+        const now = new Date()
+        const newStartTime = computeStart[preset](now)
+        setStartTime(newStartTime)
+        setEndTime(now)
+        setStartTimeInput(newStartTime)
+        setEndTimeInput(now)
+      }
     },
-    slideInterval && preset !== 'custom' ? slideInterval : null
+    [setStartTime, setEndTime]
   )
 
+  useInterval({
+    fn: () => setRange(preset),
+    delay: preset !== 'custom' ? SLIDE_INTERVAL : null,
+    key: preset, // force a render which clears current interval
+  })
+
   return (
     <form className="flex h-20 gap-4">
       <Listbox
@@ -118,8 +126,9 @@ export function DateTimeRangePicker({
         items={rangePresets}
         onChange={(item) => {
           if (item) {
-            setPreset(item.value as RangeKeyAll)
-            if (item.value !== 'custom') setTimesForPreset(item.value as RangeKey)
+            const newPreset = item.value as RangeKeyAll
+            setPreset(newPreset)
+            setRange(newPreset)
           }
         }}
       />
@@ -170,7 +179,10 @@ export function DateTimeRangePicker({
       {enableInputs && (
         <Button
           disabled={!customInputsDirty || !!error}
-          onClick={() => onChange(startTimeInput, endTimeInput)}
+          onClick={() => {
+            setStartTime(startTimeInput)
+            setEndTime(endTimeInput)
+          }}
         >
           Load
         </Button>

app/pages/SiloUtilizationPage.tsx

@@ -5,7 +5,7 @@ import { Divider, Listbox, PageHeader, PageTitle, Snapshots24Icon } from '@oxide
 import { bytesToGiB } from '@oxide/util'
 
 import { SystemMetric } from 'app/components/SystemMetric'
-import { DateTimeRangePicker, useDateTimeRangePickerState } from 'app/components/form'
+import { useDateTimeRangePicker } from 'app/components/form'
 
 const DEFAULT_SILO_ID = '001de000-5110-4000-8000-000000000000'
 const ALL_PROJECTS = '|ALL_PROJECTS|'
@@ -32,12 +32,7 @@ export function SiloUtilizationPage() {
     { enabled: !!orgName }
   )
 
-  const initialPreset = 'lastHour'
-  const {
-    startTime,
-    endTime,
-    onChange: onTimeChange,
-  } = useDateTimeRangePickerState(initialPreset)
+  const { startTime, endTime, dateTimeRangePicker } = useDateTimeRangePicker('lastHour')
 
   const orgItems = useMemo(() => {
     const items = orgs?.items.map(toListboxItem) || []
@@ -109,13 +104,7 @@ export function SiloUtilizationPage() {
           )}
         </div>
 
-        <DateTimeRangePicker
-          initialPreset={initialPreset}
-          slideInterval={5000}
-          startTime={startTime}
-          endTime={endTime}
-          onChange={onTimeChange}
-        />
+        {dateTimeRangePicker}
       </div>
       {/* TODO: this divider is supposed to go all the way across */}
       <Divider className="mb-6" />

app/pages/project/instances/instance/tabs/MetricsTab.tsx

@@ -6,7 +6,7 @@ import { useApiQuery } from '@oxide/api'
 import { Listbox, Spinner } from '@oxide/ui'
 
 import { TimeSeriesAreaChart } from 'app/components/TimeSeriesChart'
-import { DateTimeRangePicker, useDateTimeRangePickerState } from 'app/components/form'
+import { useDateTimeRangePicker } from 'app/components/form'
 import { useRequiredParams } from 'app/hooks'
 
 type DiskMetricParams = {
@@ -69,12 +69,7 @@ function DiskMetric({
 function DiskMetrics({ disks }: { disks: Disk[] }) {
   const { orgName, projectName } = useRequiredParams('orgName', 'projectName')
 
-  const initialPreset = 'lastDay'
-  const {
-    startTime,
-    endTime,
-    onChange: onTimeChange,
-  } = useDateTimeRangePickerState(initialPreset)
+  const { startTime, endTime, dateTimeRangePicker } = useDateTimeRangePicker('lastDay')
 
   invariant(disks.length > 0, 'DiskMetrics should not be rendered with zero disks')
   const [diskName, setDiskName] = useState<string>(disks[0].name)
@@ -86,9 +81,6 @@ function DiskMetrics({ disks }: { disks: Disk[] }) {
   return (
     <>
       <div className="mt-8 mb-4 flex justify-between">
-        {/* TODO: using a Formik field here feels like overkill, but we've built
-            ListboxField to require that, i.e., there's no way to get the nice worked-out
-            layout from ListboxField without using Formik. Something to think about. */}
         <Listbox
           className="w-48"
           aria-label="Choose disk"
@@ -101,13 +93,7 @@ function DiskMetrics({ disks }: { disks: Disk[] }) {
           }}
           defaultValue={diskName}
         />
-        <DateTimeRangePicker
-          initialPreset={initialPreset}
-          slideInterval={5000}
-          startTime={startTime}
-          endTime={endTime}
-          onChange={onTimeChange}
-        />
+        {dateTimeRangePicker}
       </div>
 
       {/* TODO: separate "Reads" from "(count)" so we can

app/pages/system/CapacityUtilizationPage.tsx

@@ -5,7 +5,7 @@ import { Divider, Listbox, PageHeader, PageTitle, Snapshots24Icon } from '@oxide
 import { bytesToGiB } from '@oxide/util'
 
 import { SystemMetric } from 'app/components/SystemMetric'
-import { DateTimeRangePicker, useDateTimeRangePickerState } from 'app/components/form'
+import { useDateTimeRangePicker } from 'app/components/form'
 
 const FLEET_ID = '001de000-1334-4000-8000-000000000000'
 const DEFAULT_SILO_ID = '001de000-5110-4000-8000-000000000000'
@@ -20,11 +20,7 @@ export function CapacityUtilizationPage() {
   const { data: silos } = useApiQuery('siloList', {})
 
   const initialPreset = 'lastHour'
-  const {
-    startTime,
-    endTime,
-    onChange: onTimeChange,
-  } = useDateTimeRangePickerState(initialPreset)
+  const { startTime, endTime, dateTimeRangePicker } = useDateTimeRangePicker(initialPreset)
 
   const siloItems = useMemo(() => {
     const items = silos?.items.map((silo) => ({ label: silo.name, value: silo.id })) || []
@@ -66,13 +62,7 @@ export function CapacityUtilizationPage() {
           {/* TODO: need a button to clear the silo */}
         </div>
 
-        <DateTimeRangePicker
-          initialPreset={initialPreset}
-          slideInterval={5000}
-          startTime={startTime}
-          endTime={endTime}
-          onChange={onTimeChange}
-        />
+        {dateTimeRangePicker}
       </div>
       {/* TODO: this divider is supposed to go all the way across */}
       <Divider className="mb-6" />