diff --git a/frontend/src/cart/Cart.tsx b/frontend/src/cart/Cart.tsx index b968c36..3dfd2b4 100644 --- a/frontend/src/cart/Cart.tsx +++ b/frontend/src/cart/Cart.tsx @@ -25,18 +25,16 @@ import { fetchConfig, SiteConfig } from '../api'; import { loadPaypalSdk } from '../paypal'; import { useCart } from './CartContext'; import { useCustomerAuth } from '../customer/CustomerAuthContext'; +import { useNow } from './useNow'; +import { timeRemaining, isExpiringSoon, hasLapsedItem } from './reservation'; + +// The display has one-minute resolution, so half a minute keeps it honest +// without being busy. Once a second would be wasted work. +const COUNTDOWN_INTERVAL_MS = 30_000; const { Header, Content } = Layout; const { Title, Text } = Typography; -function timeRemaining(expiresAt: string): string { - const diffMs = new Date(expiresAt).getTime() - Date.now(); - if (diffMs <= 0) return 'expiring…'; - const hours = Math.floor(diffMs / (1000 * 60 * 60)); - const mins = Math.floor((diffMs % (1000 * 60 * 60)) / (1000 * 60)); - return `${hours}h ${mins}m left`; -} - export default function Cart() { const [items, setItems] = useState([]); const [addresses, setAddresses] = useState([]); @@ -71,6 +69,27 @@ export default function Cart() { useEffect(() => { if (customer) loadAll(); }, [customer]); + // Only ticks while there is something to count down, so an empty cart does + // not wake React forever. + const now = useNow(COUNTDOWN_INTERVAL_MS, items.length > 0); + const lapsed = hasLapsedItem(items, now); + + // Past the deadline the item is still held until the server's sweep releases + // it, and the client has no way to know when that lands. Refetching on each + // tick while anything is lapsed means the row clears within one interval of + // the release rather than sitting at "expiring…" until the page is reloaded. + useEffect(() => { + if (!lapsed) return; + loadAll(); + // The header badge counts held items too, so it goes stale in exactly the + // same way. Safe as a dependency: CartContext memoizes it with an empty + // dependency list, so it is stable across renders and cannot re-trigger + // this effect on its own. + refreshCartContext(); + // `now` is what paces this: it changes once per tick, and re-running while + // an item is lapsed is the point. + }, [lapsed, now, refreshCartContext]); + // compute the total const total = items.reduce((sum, i) => sum + i.price_cents, 0); @@ -174,8 +193,8 @@ export default function Cart() { avatar={item.images[0] && } title={item.name} description={ - - {timeRemaining(item.expires_at)} + + {timeRemaining(item.expires_at, now)} } /> diff --git a/frontend/src/cart/reservation.ts b/frontend/src/cart/reservation.ts new file mode 100644 index 0000000..a37ba31 --- /dev/null +++ b/frontend/src/cart/reservation.ts @@ -0,0 +1,51 @@ +/** + * How a held item's remaining time is described, and when it is worth warning + * about. + * + * Kept apart from the component so both can be exercised without rendering a + * cart, and because the warning rule below is a judgement rather than a + * formatting detail. + */ + +/** Warn over the last tenth of the hold, whatever the hold happens to be. */ +const WARNING_FRACTION = 0.1; + +export function timeRemaining(expiresAt: string, now: number): string { + const diffMs = new Date(expiresAt).getTime() - now; + // Not "expired": the server releases items on a sweep every few minutes, so + // between the deadline and that sweep the item is genuinely still held. This + // claims imminence, which is true, rather than completion, which is not ours + // to say. + if (diffMs <= 0) return 'expiring…'; + const hours = Math.floor(diffMs / (1000 * 60 * 60)); + const mins = Math.floor((diffMs % (1000 * 60 * 60)) / (1000 * 60)); + return `${hours}h ${mins}m left`; +} + +/** + * Whether this reservation is close enough to lapsing to say so in red. + * + * Derived from the item's own `added_at` and `expires_at` rather than from a + * constant. The hold is admin-configurable since #136 and accepts values as low + * as half an hour, so the previous hardcoded "final hour" meant every item was + * red from the moment it was reserved on any setting below one hour — a warning + * that is always on is not a warning. + * + * Reading it from the item also means an admin changing the setting does not + * retroactively relabel a reservation that was granted under the old one. + */ +export function isExpiringSoon(addedAt: string, expiresAt: string, now: number): boolean { + const expires = new Date(expiresAt).getTime(); + const holdMs = expires - new Date(addedAt).getTime(); + const remainingMs = expires - now; + if (remainingMs <= 0) return true; + // A nonsensical hold (equal or inverted timestamps) should not make every + // item permanently urgent; fall back to not warning rather than always. + if (holdMs <= 0) return false; + return remainingMs <= holdMs * WARNING_FRACTION; +} + +/** True once any held item is past its deadline and awaiting the server sweep. */ +export function hasLapsedItem(items: { expires_at: string }[], now: number): boolean { + return items.some((item) => new Date(item.expires_at).getTime() - now <= 0); +} diff --git a/frontend/src/cart/useNow.ts b/frontend/src/cart/useNow.ts new file mode 100644 index 0000000..838fadd --- /dev/null +++ b/frontend/src/cart/useNow.ts @@ -0,0 +1,34 @@ +import { useEffect, useState } from 'react'; + +/** + * The current time, as state that advances on an interval. + * + * Returns a value rather than merely forcing a re-render, and that is the point. + * A component that calls `Date.now()` while rendering produces output that + * depends on the clock, which React is entitled to assume it does not — the + * `react-hooks/purity` rule says so, and it was the only instance in this + * codebase precisely because it was the only place doing it. Reading `now` from + * state makes render a function of its inputs again. + * + * It also fixes the defect underneath that rule. Nothing scheduled a re-render, + * so the cart's countdown was a still photograph of the moment the page loaded, + * and the red warning in the final stretch could only appear by accident — the + * component had already rendered before the final stretch began. + * + * `active` exists so an idle page is not waking React forever: an empty cart has + * nothing to count down, so it does not tick at all. + */ +export function useNow(intervalMs: number, active: boolean): number { + const [now, setNow] = useState(() => Date.now()); + + useEffect(() => { + if (!active) return; + // Set immediately as well as on the interval: mounting with `active` already + // true would otherwise show a value up to intervalMs stale. + setNow(Date.now()); + const timer = setInterval(() => setNow(Date.now()), intervalMs); + return () => clearInterval(timer); + }, [intervalMs, active]); + + return now; +} diff --git a/frontend/tests/e2e/cart-countdown.spec.ts b/frontend/tests/e2e/cart-countdown.spec.ts new file mode 100644 index 0000000..cc1d7d3 --- /dev/null +++ b/frontend/tests/e2e/cart-countdown.spec.ts @@ -0,0 +1,74 @@ +import { test, expect, createItem, uniqueSuffix } from './fixtures'; + +/** + * The countdown is the one part of this app whose output depends on the wall + * clock, so these use Playwright's clock control rather than waiting in real + * time. Installing it also makes the assertions deterministic: without it, "the + * text changed" would depend on where in the minute the test happened to start. + */ +test.describe('The cart reservation countdown', () => { + test('counts down on its own, with no interaction', async ({ page, customer, cart }) => { + const name = `Held c${uniqueSuffix()}`; + const item = await createItem(page.request, { name, price: '80' }); + expect((await page.request.post(`/api/cart/items/${item.id}`)).status()).toBe(201); + + // Installed before navigating: the interval is created on mount, and a clock + // swapped in afterwards would not be the one it is scheduled against. + await page.clock.install(); + await cart.goto(); + + const before = await cart.remaining(name).textContent(); + expect(before).toMatch(/left/); + + // Two minutes, which is four ticks of the 30s interval and enough to move a + // display with one-minute resolution. + await page.clock.fastForward('02:00'); + + // The assertion the issue exists for: the text changes with nothing + // clicked, reloaded or navigated. Before this, it was a still photograph of + // the moment the page rendered. + await expect(cart.remaining(name)).not.toHaveText(before ?? ''); + }); + + test('warns near the end of the hold rather than for the whole of it', async ({ + page, + customer, + cart + }) => { + const name = `Waning c${uniqueSuffix()}`; + const item = await createItem(page.request, { name, price: '80' }); + expect((await page.request.post(`/api/cart/items/${item.id}`)).status()).toBe(201); + + await page.clock.install(); + await cart.goto(); + + // Freshly reserved, so it is not urgent. antd renders `type="danger"` as a + // class rather than an attribute, which is why this asserts on the class. + await expect(cart.remaining(name)).not.toHaveClass(/ant-typography-danger/); + + // Past 90% of the default 24-hour hold, so inside the final tenth. The old + // hardcoded "final hour" threshold would have been silent here. + await page.clock.fastForward('22:00:00'); + await expect(cart.remaining(name)).toHaveClass(/ant-typography-danger/); + }); + + test('says the hold is expiring once it is past, rather than freezing', async ({ + page, + customer, + cart + }) => { + const name = `Lapsing c${uniqueSuffix()}`; + const item = await createItem(page.request, { name, price: '80' }); + expect((await page.request.post(`/api/cart/items/${item.id}`)).status()).toBe(201); + + await page.clock.install(); + await cart.goto(); + + // Past the deadline. The server releases held items on a sweep every few + // minutes, so between the deadline and that sweep the item is genuinely + // still held — "expiring…" claims imminence rather than completion. + await page.clock.fastForward('25:00:00'); + + await expect(cart.remaining(name)).toHaveText('expiring…'); + }); +}); diff --git a/frontend/tests/e2e/fixtures.ts b/frontend/tests/e2e/fixtures.ts index 17286e0..cc3a406 100644 --- a/frontend/tests/e2e/fixtures.ts +++ b/frontend/tests/e2e/fixtures.ts @@ -16,6 +16,7 @@ import { AdminTaxonomy } from './pages/AdminTaxonomy'; import { AdminCustomers } from './pages/AdminCustomers'; import { AdminEmails } from './pages/AdminEmails'; import { AdminSettings } from './pages/AdminSettings'; +import { CartPage } from './pages/CartPage'; import { uniqueEmail } from './support/api'; // Re-exported so specs can import everything from here — expect, Page, @@ -37,6 +38,7 @@ export { AdminTaxonomy } from './pages/AdminTaxonomy'; export { AdminCustomers } from './pages/AdminCustomers'; export { AdminEmails } from './pages/AdminEmails'; export { AdminSettings } from './pages/AdminSettings'; +export { CartPage } from './pages/CartPage'; const NYC_OUTPUT = path.resolve(__dirname, '..', '..', '.nyc_output'); const collectingCoverage = process.env.COVERAGE === 'true'; @@ -70,6 +72,7 @@ interface Pages { adminCustomers: AdminCustomers; adminEmails: AdminEmails; adminSettings: AdminSettings; + cart: CartPage; } interface Data { @@ -147,6 +150,9 @@ export const test = base.extend({ adminSettings: async ({ page }, use) => { await use(new AdminSettings(page)); }, + cart: async ({ page }, use) => { + await use(new CartPage(page)); + }, adminApi: async ({ playwright, baseURL }, use) => { const context = await playwright.request.newContext({ baseURL }); diff --git a/frontend/tests/e2e/pages/CartPage.ts b/frontend/tests/e2e/pages/CartPage.ts new file mode 100644 index 0000000..2b1979b --- /dev/null +++ b/frontend/tests/e2e/pages/CartPage.ts @@ -0,0 +1,35 @@ +import { Locator, Page } from '@playwright/test'; + +/** + * The cart, where the reservation countdown lives. + * + * The countdown is the reason this page object exists: it is the one place in + * the app whose output depends on the clock, so a test has to be able to name + * the text without re-deriving the locator each time. + */ +export class CartPage { + readonly continueShoppingButton: Locator; + readonly emptyNotice: Locator; + + constructor(private readonly page: Page) { + this.continueShoppingButton = page.getByRole('button', { name: 'Continue Shopping' }); + this.emptyNotice = page.getByText('Your cart is empty'); + } + + async goto(): Promise { + await this.page.goto('/cart'); + } + + row(itemName: string): Locator { + return this.page.getByRole('listitem').filter({ hasText: itemName }); + } + + /** The "2h 15m left" / "expiring…" line for one held item. */ + remaining(itemName: string): Locator { + return this.row(itemName).getByText(/left|expiring/); + } + + removeButton(itemName: string): Locator { + return this.row(itemName).getByRole('button', { name: 'Remove' }); + } +}