ui: adobe's own font files, visible button edges, and a refresh that keeps contrast

packages/ui/src/app/Shell.test.tsx

@@ -1,11 +1,14 @@
 import { ClientProvider } from '@/api/context'
+import { ApiError } from '@/api/errors'
 import { fakeClient } from '@/api/fake'
 import { render, screen, waitFor, within } from '@testing-library/react'
 import userEvent from '@testing-library/user-event'
+import type { ReactNode } from 'react'
 import { MemoryRouter } from 'react-router'
 import { describe, expect, it } from 'vitest'
 import { Shell } from './Shell'
 import { RefreshProvider } from './refresh'
+import { useLoad } from './useLoad'
 
 const STATUS = {
   newestHour: null,
@@ -15,6 +18,20 @@ const STATUS = {
   alerts: 0,
 } as const
 
+/** A stand-in for a screen's own load, so a test can control it independently of Freshness's. */
+function Loader({
+  id = 'load',
+  load,
+  dep = 0,
+}: {
+  id?: string
+  load: (signal: AbortSignal) => Promise<string>
+  dep?: number
+}) {
+  const r = useLoad(load, [dep])
+  return <p>{`${id}: ${r.state}`}</p>
+}
+
 function show(at = '/links') {
   const client = fakeClient({ status: () => Promise.resolve(STATUS) })
   render(
@@ -31,6 +48,21 @@ function show(at = '/links') {
   return client
 }
 
+/** Renders Shell with a caller-built client and children, for scenarios `show()` can't express. */
+function showWith(client: ReturnType<typeof fakeClient>, children: ReactNode) {
+  render(
+    <MemoryRouter initialEntries={['/links']}>
+      <ClientProvider client={client}>
+        <RefreshProvider>
+          <Shell email="admin@example.com" onSignOut={() => {}}>
+            {children}
+          </Shell>
+        </RefreshProvider>
+      </ClientProvider>
+    </MemoryRouter>,
+  )
+}
+
 describe('the shell', () => {
   it('links every screen and marks the one that is open', () => {
     show('/links')
@@ -84,7 +116,156 @@ describe('the shell', () => {
   it('reloads what is on screen when Refresh is pressed', async () => {
     const client = show()
     await screen.findByText('No clicks have reached the reports yet.')
-    await userEvent.setup().click(screen.getByRole('button', { name: 'Refresh' }))
+    const button = await screen.findByRole('button', { name: 'Refresh' })
+    await userEvent.setup().click(button)
     await waitFor(() => expect(client.calls.filter((c) => c.method === 'status')).toHaveLength(2))
   })
+
+  // Refreshing is one true signal for "something on this screen is loading",
+  // not a proxy for one particular request — a round is the simplest case of
+  // it, driven the same way every other load is.
+  it('shows Refreshing… and disables itself while a round is in flight', async () => {
+    let resolveSecond: ((s: typeof STATUS) => void) | undefined
+    let calls = 0
+    const client = fakeClient({
+      status: (() => {
+        calls += 1
+        if (calls === 1) return Promise.resolve(STATUS)
+        return new Promise<typeof STATUS>((resolve) => {
+          resolveSecond = resolve
+        })
+      }) as never,
+    })
+    showWith(client, <p>screen</p>)
+    await screen.findByText('No clicks have reached the reports yet.')
+    const button = await screen.findByRole('button', { name: 'Refresh' })
+    await userEvent.setup().click(button)
+    expect(await screen.findByRole('button', { name: 'Refreshing…' })).toBeDisabled()
+    resolveSecond?.(STATUS)
+    await waitFor(() => expect(screen.getByRole('button', { name: 'Refresh' })).toBeEnabled())
+  })
+
+  // The dimmed content it replaced was itself a contrast failure; the button
+  // base fades every disabled control to disabled:opacity-50, so this state
+  // needs its own override rather than inheriting that fade.
+  it('draws Refreshing… at full contrast, not the disabled fade', async () => {
+    let resolveSecond: ((s: typeof STATUS) => void) | undefined
+    let calls = 0
+    const client = fakeClient({
+      status: (() => {
+        calls += 1
+        if (calls === 1) return Promise.resolve(STATUS)
+        return new Promise<typeof STATUS>((resolve) => {
+          resolveSecond = resolve
+        })
+      }) as never,
+    })
+    showWith(client, <p>screen</p>)
+    await screen.findByText('No clicks have reached the reports yet.')
+    const button = await screen.findByRole('button', { name: 'Refresh' })
+    await userEvent.setup().click(button)
+    const refreshing = await screen.findByRole('button', { name: 'Refreshing…' })
+    expect(refreshing).not.toHaveClass('disabled:opacity-50')
+    expect(refreshing).toHaveClass('disabled:opacity-100')
+    resolveSecond?.(STATUS)
+  })
+
+  // A 429 or any other refusal is still a load settling, not a load that
+  // never happened: it must release the button the same as a success does.
+  it('re-enables Refresh once a failed reload settles, not only a successful one', async () => {
+    let rejectSecond: ((err: ApiError) => void) | undefined
+    let calls = 0
+    const client = fakeClient({
+      status: (() => {
+        calls += 1
+        if (calls === 1) return Promise.resolve(STATUS)
+        return new Promise<typeof STATUS>((_resolve, reject) => {
+          rejectSecond = reject
+        })
+      }) as never,
+    })
+    showWith(client, <p>screen</p>)
+    await screen.findByText('No clicks have reached the reports yet.')
+    const button = await screen.findByRole('button', { name: 'Refresh' })
+    await userEvent.setup().click(button)
+    expect(await screen.findByRole('button', { name: 'Refreshing…' })).toBeDisabled()
+    rejectSecond?.(new ApiError(429, 'rate_limited', 'slow down'))
+    await waitFor(() => expect(screen.getByRole('button', { name: 'Refresh' })).toBeEnabled())
+  })
+
+  // #46's proxy read only Freshness's own load; a slower report load left the
+  // button re-enabled while the numbers under it were still on the old
+  // window. The flag now has to outlast whichever load on the screen is
+  // slowest, not just the one in the header.
+  it('keeps Refreshing… when a slower load is still going after the status load has answered', async () => {
+    const client = fakeClient({ status: () => Promise.resolve(STATUS) })
+    let resolveReport: ((v: string) => void) | undefined
+    const report = () =>
+      new Promise<string>((resolve) => {
+        resolveReport = resolve
+      })
+    showWith(client, <Loader id="report" load={report} />)
+    await screen.findByText('No clicks have reached the reports yet.')
+    expect(screen.getByRole('button', { name: 'Refreshing…' })).toBeDisabled()
+    resolveReport?.('done')
+    await waitFor(() => expect(screen.getByRole('button', { name: 'Refresh' })).toBeEnabled())
+  })
+
+  // A window change, a filter or a search never touches `round`: they change
+  // a screen's own load deps instead. The button has to answer to that too,
+  // not only to a press of Refresh.
+  it('shows Refreshing… when a load restarts from something other than Refresh, such as a window change', async () => {
+    const client = fakeClient({ status: () => Promise.resolve(STATUS) })
+    let calls = 0
+    let resolveSecond: ((v: string) => void) | undefined
+    const load = () => {
+      calls += 1
+      if (calls === 1) return Promise.resolve('first')
+      return new Promise<string>((resolve) => {
+        resolveSecond = resolve
+      })
+    }
+    const tree = (dep: number) => (
+      <MemoryRouter initialEntries={['/links']}>
+        <ClientProvider client={client}>
+          <RefreshProvider>
+            <Shell email="admin@example.com" onSignOut={() => {}}>
+              <Loader load={load} dep={dep} />
+            </Shell>
+          </RefreshProvider>
+        </ClientProvider>
+      </MemoryRouter>
+    )
+    const { rerender } = render(tree(1))
+    await screen.findByText('No clicks have reached the reports yet.')
+    await waitFor(() => expect(screen.getByRole('button', { name: 'Refresh' })).toBeEnabled())
+    rerender(tree(2))
+    expect(await screen.findByRole('button', { name: 'Refreshing…' })).toBeDisabled()
+    resolveSecond?.('second')
+    await waitFor(() => expect(screen.getByRole('button', { name: 'Refresh' })).toBeEnabled())
+  })
+
+  // A load that fails must clear only its own slot: the button stays down
+  // until the slowest of them settles, whatever any one of them answered.
+  it('does not re-enable Refresh while one load is still going, even once another has failed', async () => {
+    const client = fakeClient({ status: () => Promise.resolve(STATUS) })
+    let resolveSlow: ((v: string) => void) | undefined
+    const failing = () => Promise.reject(new ApiError(429, 'rate_limited', 'slow down'))
+    const slow = () =>
+      new Promise<string>((resolve) => {
+        resolveSlow = resolve
+      })
+    showWith(
+      client,
+      <>
+        <Loader id="failing" load={failing} />
+        <Loader id="slow" load={slow} />
+      </>,
+    )
+    await screen.findByText('No clicks have reached the reports yet.')
+    await screen.findByText('failing: error')
+    expect(screen.getByRole('button', { name: 'Refreshing…' })).toBeDisabled()
+    resolveSlow?.('done')
+    await waitFor(() => expect(screen.getByRole('button', { name: 'Refresh' })).toBeEnabled())
+  })
 })

packages/ui/src/app/Shell.tsx

@@ -26,7 +26,7 @@ export function Shell({
   onSignOut,
   children,
 }: { email: string; onSignOut: () => void; children: ReactNode }) {
-  const { refresh } = useRefresh()
+  const { refresh, refreshing } = useRefresh()
   return (
     <div className="grid min-h-dvh grid-rows-[auto_1fr_auto] md:grid-cols-[13rem_1fr] md:grid-rows-[1fr_auto]">
       <nav
@@ -59,8 +59,19 @@ export function Shell({
           <Freshness />
           <div className="flex items-center gap-2">
             <span className="text-xs text-muted-foreground">{`Times in ${browserZone()}`}</span>
-            <Button type="button" variant="outline" size="sm" onClick={refresh}>
-              Refresh
+            <Button
+              type="button"
+              variant="outline"
+              size="sm"
+              onClick={refresh}
+              disabled={refreshing}
+              // The cue that replaced the dimmed content must not itself be
+              // dimmed: the button base fades every disabled control to
+              // disabled:opacity-50, so this overrides it back to full
+              // contrast while "Refreshing…" is the reason it is disabled.
+              className="disabled:opacity-100"
+            >
+              {refreshing ? 'Refreshing…' : 'Refresh'}
             </Button>
             <ThemeToggle />
             <Button type="button" variant="ghost" size="sm" onClick={onSignOut} title={email}>

packages/ui/src/app/refresh.test.tsx

@@ -16,6 +16,21 @@ function Round() {
   )
 }
 
+function Refreshing() {
+  const { refreshing, beginLoad, endLoad } = useRefresh()
+  return (
+    <>
+      <p>{refreshing ? 'refreshing' : 'idle'}</p>
+      <button type="button" onClick={beginLoad}>
+        begin
+      </button>
+      <button type="button" onClick={endLoad}>
+        end
+      </button>
+    </>
+  )
+}
+
 describe('refreshing', () => {
   it('counts a press of Refresh', async () => {
     render(
@@ -65,6 +80,39 @@ describe('refreshing', () => {
     expect(screen.getByRole('button')).toHaveTextContent('round 0')
   })
 
+  it('starts idle, and goes busy once something begins loading', async () => {
+    render(
+      <RefreshProvider>
+        <Refreshing />
+      </RefreshProvider>,
+    )
+    expect(screen.getByText('idle')).toBeInTheDocument()
+    await act(async () => screen.getByRole('button', { name: 'begin' }).click())
+    expect(screen.getByText('refreshing')).toBeInTheDocument()
+    await act(async () => screen.getByRole('button', { name: 'end' }).click())
+    expect(screen.getByText('idle')).toBeInTheDocument()
+  })
+
+  // The count, not a single flag: a second load still outstanding must keep
+  // the flag up once the first of the two ends, the way one screen's several
+  // loads share one Refresh button.
+  it('stays busy while a second load is still outstanding after the first ends', async () => {
+    render(
+      <RefreshProvider>
+        <Refreshing />
+      </RefreshProvider>,
+    )
+    const begin = screen.getByRole('button', { name: 'begin' })
+    const end = screen.getByRole('button', { name: 'end' })
+    await act(async () => begin.click())
+    await act(async () => begin.click())
+    expect(screen.getByText('refreshing')).toBeInTheDocument()
+    await act(async () => end.click())
+    expect(screen.getByText('refreshing')).toBeInTheDocument()
+    await act(async () => end.click())
+    expect(screen.getByText('idle')).toBeInTheDocument()
+  })
+
   it('stops listening once unmounted', () => {
     const windowRemove = vi.spyOn(window, 'removeEventListener')
     const documentRemove = vi.spyOn(document, 'removeEventListener')

packages/ui/src/app/refresh.tsx

@@ -9,9 +9,18 @@ import {
   useState,
 } from 'react'
 
-const RefreshContext = createContext<{ round: number; refresh: () => void }>({
+const RefreshContext = createContext<{
+  round: number
+  refresh: () => void
+  refreshing: boolean
+  beginLoad: () => void
+  endLoad: () => void
+}>({
   round: 0,
   refresh: () => {},
+  refreshing: false,
+  beginLoad: () => {},
+  endLoad: () => {},
 })
 
 /** How long away before coming back to the tab reloads what is on screen. */
@@ -22,18 +31,31 @@ export const FOCUS_REFRESH_AFTER_MS = 60_000
  * does coming back to the tab after a minute away — never a timer, because a
  * timer keeps a session alive for as long as a tab is open and spends report
  * slots on a page nobody is reading.
+ *
+ * `refreshing` is a second, independent counter: how many loads are
+ * currently in flight, anywhere on the current screen. `useLoad` calls
+ * `beginLoad`/`endLoad` itself around every request it makes, whether that
+ * request started from a Refresh press, a window change, a filter or a
+ * search, or the screen's own first load — so the Refresh button, which
+ * reads `refreshing`, has one true signal for "something is loading" rather
+ * than a proxy for one particular load. A failed load still calls `endLoad`,
+ * the same as a successful one: only the count of what is still in flight
+ * decides the flag, never which of them succeeded.
  */
 export function RefreshProvider({
   children,
   now = Date.now,
   focusAfterMs = FOCUS_REFRESH_AFTER_MS,
 }: { children: ReactNode; now?: () => number; focusAfterMs?: number }) {
   const [round, setRound] = useState(0)
+  const [busy, setBusy] = useState(0)
   const last = useRef(now())
   const refresh = useCallback(() => {
     last.current = now()
     setRound((r) => r + 1)
   }, [now])
+  const beginLoad = useCallback(() => setBusy((n) => n + 1), [])
+  const endLoad = useCallback(() => setBusy((n) => Math.max(0, n - 1)), [])
   useEffect(() => {
     const back = () => {
       if (document.visibilityState === 'hidden') return
@@ -46,7 +68,10 @@ export function RefreshProvider({
       document.removeEventListener('visibilitychange', back)
     }
   }, [now, focusAfterMs, refresh])
-  const value = useMemo(() => ({ round, refresh }), [round, refresh])
+  const value = useMemo(
+    () => ({ round, refresh, refreshing: busy > 0, beginLoad, endLoad }),
+    [round, refresh, busy, beginLoad, endLoad],
+  )
   return <RefreshContext.Provider value={value}>{children}</RefreshContext.Provider>
 }