From 5ef97bef211316819ccbe6cf2f3a73b72d537445 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Mon, 24 Aug 2026 08:56:45 -0500 Subject: [PATCH] fix(cart): make the reservation countdown tick, and warn against the real hold (#97) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cart showed "2h 15m left" and turned it red in the final hour. Neither updated. Both readings happened during render from the wall clock, and nothing scheduled a re-render — no setInterval anywhere in the file — so the number a customer read was whatever it was when the page loaded, and the warning colour could only appear by accident, because the component had already rendered before the final stretch began. That matters more here than in most shops: every item is one of a kind, so a lapsed reservation is not "buy it later", it is someone else buying the only one. New useNow hook returns the time as state rather than merely forcing a re-render, and that is the point. A component reading Date.now() while rendering produces output that depends on the clock, which React is entitled to assume it does not — react-hooks/purity says so, and this was the only instance in the codebase precisely because it was the only place doing it. Reading `now` from state makes render a function of its inputs again, so the rule is satisfied rather than suppressed. It ticks every 30s, which matches the display's one-minute resolution, and only while the cart holds something, so an empty cart is not waking React forever. A second defect the issue did not mention. The red warning was hardcoded to the final hour, but the hold became admin-configurable in #136 and accepts values as low as half an hour — so on any setting below an hour every item was red from the moment it was reserved, and a warning that is always on is not a warning. It now keys off the last tenth of the item's own added_at-to-expires_at span. Reading it from the item rather than from the setting also means an admin changing the value does not retroactively relabel a reservation granted under the old one. At zero the row keeps saying "expiring…" and the page refetches on each tick while anything is lapsed, so it clears within one interval of the server's sweep actually releasing it. The client cannot know when that lands — the sweep runs every few minutes — so the wording claims imminence, which is true, rather than completion, which is not ours to say. The header badge is refreshed alongside, since it counts held items and goes stale the same way. Verified against the unfixed component, not just the fixed one: two of the three new tests fail on the old code. The third documents the "expiring…" wording rather than the fix, and passes either way — worth having, but it is not evidence. The tests use Playwright's clock control rather than waiting in real time, which also makes them deterministic: without it, "the text changed" would depend on where in the minute the run happened to start. Refs #97 --- frontend/src/cart/Cart.tsx | 39 +++++++++--- frontend/src/cart/reservation.ts | 51 ++++++++++++++++ frontend/src/cart/useNow.ts | 34 +++++++++++ frontend/tests/e2e/cart-countdown.spec.ts | 74 +++++++++++++++++++++++ frontend/tests/e2e/fixtures.ts | 6 ++ frontend/tests/e2e/pages/CartPage.ts | 35 +++++++++++ 6 files changed, 229 insertions(+), 10 deletions(-) create mode 100644 frontend/src/cart/reservation.ts create mode 100644 frontend/src/cart/useNow.ts create mode 100644 frontend/tests/e2e/cart-countdown.spec.ts create mode 100644 frontend/tests/e2e/pages/CartPage.ts 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' }); + } +} -- 2.54.0