Merge pull request 'fix(cart): make the reservation countdown tick, and warn against the real hold (#97)' (#152) from feature/97-cart-countdown into main

Reviewed-on: #152

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<CartItem[]>([]);
   const [addresses, setAddresses] = useState<ShippingAddress[]>([]);
@@ -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] && <img src={item.images[0].image_path} alt="" style={{ width: 60, height: 60, objectFit: 'cover' }} />}
                     title={item.name}
                     description={
-                      <Text type={new Date(item.expires_at).getTime() - Date.now() < 60 * 60 * 1000 ? 'danger' : 'secondary'}>
+                      <Text type={isExpiringSoon(item.added_at, item.expires_at, now) ? 'danger' : 'secondary'}>
-                        {timeRemaining(item.expires_at)}
+                        {timeRemaining(item.expires_at, now)}
                       </Text>
                     }
                   />

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);
+}

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;
+}

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…');
+  });
+});

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<Pages & Data & { collectCoverage: void }>({
   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 });

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<void> {
+    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' });
+  }
+}