From e610d0961fca5c28a4dd00335f5a9eb0802ddd1e Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Thu, 23 Apr 2026 13:08:04 -0400 Subject: [PATCH 1/3] =?UTF-8?q?fix(dashboard):=20429=20UX=20polish=20?= =?UTF-8?q?=E2=80=94=20coherent=20failure=20state,=20a11y,=20skeleton=20lo?= =?UTF-8?q?ad=20(#2937,=20#2938,=20#2939)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three retrospective follow-ups from PR #2933: #2937 (P1) — post-auto-retry state is now coherent + accessible: - The "you've refreshed too quickly" state is driven by a real countdown off the second 429's Retry-After, falling back to 60s when absent. Button stays disabled for the full window instead of re-enabling while the copy says to wait. - Countdown container gets role="status", aria-live="polite", aria-atomic="true". Text updates throttled to 10s intervals + the final 5s + terminal transition so screen readers don't hear every tick. #2938 (P2) — no stale session state: - autoRetriedAgents now clears when a retry succeeds. A 9am 429 no longer taints a 5pm retry. - New retryInFlight Set prevents a click-plus-countdown race from firing retryAgentCard twice. #2939 (P3) — polish: - visibilitychange → visible forces a tick so background-throttled tabs don't show stuck countdown values on return. - Page renders BEFORE agent fetches: cards appear in list order in their "not yet checked" skeleton state immediately, then hydrate in place via a new swapAgentCard helper as each worker resolves. - Shared `cancellation` flag lets workers stop hammering the network when the 15s timeout fires. - "Retrying now…" beat before auto-retry fires so the transition reads as an action. - Middleware: new parseRetryAfterSeconds helper rejects 0 / negatives / non-finite (was `parseInt() || undefined` which swallowed 0). Comment noting the HTTP-date form is legal but express-rate-limit only emits delta-seconds today. - rate-limit-retry-after.test.ts: 5 parse tests + supertest assertion that the 429 body carries retryAfter. Typecheck clean. 1946 server + 631 root unit tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/dashboard-429-polish.md | 22 ++ server/public/dashboard-agents.html | 265 ++++++++++++------ server/src/middleware/rate-limit.ts | 34 ++- .../tests/unit/rate-limit-retry-after.test.ts | 79 ++++++ 4 files changed, 308 insertions(+), 92 deletions(-) create mode 100644 .changeset/dashboard-429-polish.md create mode 100644 server/tests/unit/rate-limit-retry-after.test.ts diff --git a/.changeset/dashboard-429-polish.md b/.changeset/dashboard-429-polish.md new file mode 100644 index 0000000000..ccc111a7f2 --- /dev/null +++ b/.changeset/dashboard-429-polish.md @@ -0,0 +1,22 @@ +--- +--- + +Close #2937, #2938, #2939 — the three retrospective follow-ups from PR #2933 (dashboard 429 UX). + +**#2937 (P1) — coherent + accessible failure state.** +- The post-auto-retry "you've refreshed too quickly" copy is now driven by a real countdown off the second response's `Retry-After`, falling back to 60s when absent. Button stays disabled for the full window instead of re-enabling while the copy says to wait. +- Countdown container gets `role="status"`, `aria-live="polite"`, `aria-atomic="true"`. Text updates are throttled to every 10s + the final 5s + the terminal transition so screen readers aren't shouted at every tick. + +**#2938 (P2) — no stale session state.** +- `autoRetriedAgents` now clears when a retry succeeds. A 9am 429 no longer taints a 5pm retry. +- New `retryInFlight` Set prevents a click-plus-countdown race from firing `retryAgentCard` twice. + +**#2939 (P3) — polish.** +- `visibilitychange → visible` forces a countdown tick so background-throttled tabs don't show stuck values. +- Initial page render happens BEFORE the agent fetches. Cards appear in list order immediately in their "not yet checked" skeleton state, then hydrate in place as each worker resolves. Fixes the scrambled loading order and the "dead blank page" feel on large agent lists. +- A shared `cancellation` flag lets workers stop hammering the network when the 15s timeout fires. +- "Retrying now…" beat before auto-retry fires so the transition reads. +- Middleware: extracted `parseRetryAfterSeconds` helper that rejects 0, negatives, and non-finite values (replaces `|| undefined` which swallowed 0). Added comment noting the HTTP-date form is technically legal but express-rate-limit only emits delta-seconds. +- New `rate-limit-retry-after.test.ts` — 5 tests on the parse helper + a supertest that verifies the 429 body surfaces `retryAfter` when the header is set. + +No production behavior changes beyond what's called out above. 1946 server + 631 root unit tests pass. diff --git a/server/public/dashboard-agents.html b/server/public/dashboard-agents.html index c03ba4a51e..a23a161a51 100644 --- a/server/public/dashboard-agents.html +++ b/server/public/dashboard-agents.html @@ -849,65 +849,68 @@ }); } - // Fetch compliance data for org's agents with a concurrency cap - // so a large agent list doesn't self-DOS the per-agent read - // limiter (#2804). Each agent fans out to 2 server routes + // Fetch compliance data for org's agents with a concurrency + // cap so a large agent list doesn't self-DOS the per-agent + // read limiter (#2804). Each agent fans out to 2 server routes // (agentReadRateLimiter, 240/min) + 1 auth-status call, so // holding 6 agents in-flight keeps the burst well under the // cap while still feeling fast for typical (1-10 agent) users. - // A hard 15s ceiling still guards against any single fetch - // hanging the entire page — unresolved agents fall through to - // the per-card "not yet checked" / error state. + // + // Render-in-place pattern (#2939): cards appear in the user's + // list order *immediately* in their skeleton "not yet checked" + // state, then hydrate as each fetchAgentState resolves. That + // replaces the old flow where the whole page sat blank until + // the entire batch (or the 15s timeout) resolved — for a + // 60-agent list over a slow connection that's the difference + // between "instant" and "dead for 14 seconds." + // + // A shared `cancelled` flag lets workers stop hammering the + // network if the user navigates away or the 15s ceiling + // fires. Workers always harvest results into the shared + // complianceMap, even after cancellation — a late success + // that arrived before cancel is still useful. const AGENT_FETCH_CONCURRENCY = 6; + const FETCH_TIMEOUT_MS = 15_000; const agentCompliance = new Map(); const agents = profileData?.profile?.agents || []; + const currentOrgId = currentOrg?.id || null; + + pageState.agents = agents; + pageState.complianceMap = agentCompliance; + pageState.orgId = currentOrgId; + pageState.hasApiAccess = !!profileData?.has_api_access; + pageState.brandHostingType = null; + renderPage(agents, agentCompliance, currentOrgId); + if (agents.length > 0) { const queue = agents.slice(); - const settled = []; + const cancellation = { cancelled: false }; async function worker() { - while (queue.length > 0) { + while (!cancellation.cancelled && queue.length > 0) { const agent = queue.shift(); if (!agent) break; try { - settled.push({ status: 'fulfilled', value: await fetchAgentState(agent) }); + const state = await fetchAgentState(agent); + if (cancellation.cancelled) break; + agentCompliance.set(state.url, state); + swapAgentCard(state.url); } catch (err) { - settled.push({ status: 'rejected', reason: err }); + console.error('Agent compliance fetch rejected:', err); } } } const workerCount = Math.min(AGENT_FETCH_CONCURRENCY, agents.length); const workers = Promise.all(Array.from({ length: workerCount }, worker)); - const timeout = new Promise(resolve => setTimeout(() => resolve(null), 15000)); + const timeout = new Promise(resolve => setTimeout(() => { + cancellation.cancelled = true; + resolve(null); + }, FETCH_TIMEOUT_MS)); const completed = await Promise.race([workers, timeout]); - if (completed !== null) { - for (const r of settled) { - if (r.status === 'fulfilled') { - agentCompliance.set(r.value.url, r.value); - } else { - console.error('Agent compliance fetch rejected:', r.reason); - } - } - } else { - console.warn('Agent compliance fetch timed out after 15s; rendering with partial data'); - // Even on timeout, harvest whatever workers managed to - // resolve before the deadline so the user sees partial - // results instead of nothing. - for (const r of settled) { - if (r.status === 'fulfilled') { - agentCompliance.set(r.value.url, r.value); - } - } + if (completed === null) { + console.warn('Agent compliance fetch timed out after 15s; in-flight workers cancelled, rendering with partial data'); } } - const currentOrgId = currentOrg?.id || null; - pageState.agents = agents; - pageState.complianceMap = agentCompliance; - pageState.orgId = currentOrgId; - pageState.hasApiAccess = !!profileData?.has_api_access; - pageState.brandHostingType = null; - renderPage(agents, agentCompliance, currentOrgId); - // Brand hosting type gates the "Public" visibility option. Fetched // asynchronously — the cards re-render once the answer lands. const primaryBrandDomain = profileData?.profile?.primary_brand_domain; @@ -1111,41 +1114,51 @@

Agents

if (loadError) { const isRateLimited = loadError.code === 429; - // Rate-limit rendering has three modes (#2804): - // - first 429 with a known retry time → live countdown + - // auto-retry-once when the countdown hits zero. - // - first 429 with no known retry time → generic "Retry in - // a moment" text, manual retry only. - // - post-auto-retry second 429 → "You've refreshed too - // quickly" guidance, retry button disabled for the - // remaining window. + // Rate-limit rendering (#2804 / #2937): + // - first 429 → live countdown, auto-retry once at zero. + // - second 429 after auto-retry → same countdown mechanics + // but guidance copy changes and we do NOT auto-retry + // again. If the second response didn't carry a + // Retry-After, fall back to 60s so the button disables + // meaningfully. + // - 429 with no retry hint AND no prior auto-retry → plain + // "retry in a moment", button stays enabled. + const SECOND_429_FALLBACK_SEC = 60; const autoRetried = pageState.autoRetriedAgents?.has(agent.url); - const retryAfterSeconds = isRateLimited && typeof loadError.retryAfterSeconds === 'number' + const rawRetryAfterSeconds = isRateLimited && typeof loadError.retryAfterSeconds === 'number' ? loadError.retryAfterSeconds : null; - const retryAtMs = retryAfterSeconds !== null ? Date.now() + retryAfterSeconds * 1000 : null; + const effectiveRetryAfterSeconds = rawRetryAfterSeconds !== null + ? rawRetryAfterSeconds + : isRateLimited && autoRetried ? SECOND_429_FALLBACK_SEC : null; + const retryAtMs = effectiveRetryAfterSeconds !== null + ? Date.now() + effectiveRetryAfterSeconds * 1000 + : null; let errMsg; if (!isRateLimited) { errMsg = 'Couldn\'t load compliance data. The service may be temporarily unavailable.'; - } else if (autoRetried) { - errMsg = 'You\'ve refreshed too quickly — wait a minute before trying again.'; - } else if (retryAfterSeconds !== null) { - // Replaced live by the countdown interval below. The - // initial value is rendered server-side so the user sees - // a number even before the first tick fires. - errMsg = `Rate-limited — retry in ${retryAfterSeconds}s…`; + } else if (autoRetried && effectiveRetryAfterSeconds !== null) { + errMsg = `You've refreshed too quickly — retry in ${effectiveRetryAfterSeconds}s…`; + } else if (effectiveRetryAfterSeconds !== null) { + errMsg = `Rate-limited — retry in ${effectiveRetryAfterSeconds}s…`; } else { errMsg = 'Rate-limited — retry in a moment.'; } - // Disable the retry button until the countdown expires or - // (if we already auto-retried) for the rest of the known - // rate-limit window. - const retryDisabled = isRateLimited && (autoRetried || retryAtMs !== null); - const retryLabel = retryDisabled && retryAtMs !== null - ? `Retry in ${retryAfterSeconds}s` + // Disable the retry button whenever we have a known window; + // the single page-level countdown interval re-enables it + // (and re-renders) when the countdown expires. + const retryDisabled = isRateLimited && retryAtMs !== null; + const retryLabel = retryDisabled + ? `Retry in ${effectiveRetryAfterSeconds}s` : 'Retry'; + // The countdown only auto-retries on the FIRST 429 — second + // 429 is purely a waiting-room; the button re-enables at + // zero for a manual retry. That gap is encoded in the + // `data-rate-limited-auto` flag so the interval logic stays + // simple. + const shouldAutoRetryAtZero = retryAtMs !== null && !autoRetried; return `
@@ -1159,7 +1172,11 @@

Agents

-
+
${escapeHtml(errMsg)}
${visibilitySelectorHtml} @@ -1280,36 +1297,66 @@

Agents

return days + 'd ago'; } + // Re-entry guard for retryAgentCard: set while a retry is in + // flight, cleared when the re-render completes. Prevents a stale + // click from racing the countdown auto-trigger (#2938). + const retryInFlight = new Set(); + + // Swap a single agent's card in the DOM with freshly-rendered HTML + // from renderAgentsSection. Used by both retry paths and the + // skeleton-hydrate flow at initial load (#2939). Accepts + // `{ clearAutoRetried }`: callers performing a successful retry + // pass true so a long-lived session doesn't carry "auto-retried" + // state from a window that has long since expired (#2938). + function swapAgentCard(agentUrl, { clearAutoRetried = false } = {}) { + if (clearAutoRetried) pageState.autoRetriedAgents.delete(agentUrl); + const cardId = 'agent-' + encodeURIComponent(agentUrl).replace(/%/g, '_'); + const card = document.getElementById(cardId); + if (!card) return; + const agent = pageState.agents.find(a => a.url === agentUrl) || { url: agentUrl }; + const newHtml = renderAgentsSection([agent], pageState.complianceMap, pageState.orgId); + const wrapper = document.createElement('div'); + wrapper.innerHTML = newHtml.trim(); + const newCard = wrapper.firstElementChild; + if (newCard) card.replaceWith(newCard); + } + // Retry a single agent's compliance fetch after a load error. Shared // between the explicit Retry button click and the post-countdown // auto-retry triggered by the rate-limit UI. `auto` is true when - // called from the countdown, which marks the agent as having - // auto-retried so a subsequent 429 freezes the card with - // "you've refreshed too quickly" guidance instead of looping. + // called from the countdown (adds to autoRetriedAgents so a second + // 429 renders guidance copy). On successful retry, we clear the + // auto-retried flag so stale session state doesn't linger (#2938). async function retryAgentCard(agentUrl, cardId, { auto = false } = {}) { + if (retryInFlight.has(agentUrl)) return; + retryInFlight.add(agentUrl); const card = document.getElementById(cardId); - if (!card) return; + if (!card) { + retryInFlight.delete(agentUrl); + return; + } const retryBtn = card.querySelector('.agent-reload-btn'); if (retryBtn) { retryBtn.disabled = true; - retryBtn.textContent = 'Retrying...'; + retryBtn.textContent = 'Retrying…'; } if (auto) pageState.autoRetriedAgents.add(agentUrl); try { const state = await fetchAgentState({ url: agentUrl }); pageState.complianceMap.set(agentUrl, state); - const agent = pageState.agents.find(a => a.url === agentUrl) || { url: agentUrl }; - const newHtml = renderAgentsSection([agent], pageState.complianceMap, pageState.orgId); - const wrapper = document.createElement('div'); - wrapper.innerHTML = newHtml.trim(); - const newCard = wrapper.firstElementChild; - if (newCard) card.replaceWith(newCard); + // Successful refresh (no loadError, or at least a non-429 + // response) → clear any auto-retry marker so a future 429 + // later in the session starts from a clean slate (#2938). + const cleanedUp = !state.loadError || state.loadError.code !== 429; + swapAgentCard(agentUrl, { clearAutoRetried: cleanedUp }); } catch (err) { console.error('Retry failed:', err); if (retryBtn) { retryBtn.disabled = false; retryBtn.textContent = 'Retry'; } + } finally { + retryInFlight.delete(agentUrl); } } @@ -1324,9 +1371,18 @@

Agents

// Live countdown + auto-retry for rate-limited cards. One page-level // interval scans for `[data-rate-limited-until]` elements every - // second, updates their text to "Retry in Ns…", and fires a - // single auto-retry when the countdown hits zero (#2804). - setInterval(function() { + // second, updates their text, and fires an auto-retry when the + // countdown hits zero IF the node's `data-rate-limited-auto` flag + // is set (first 429 only — second 429 is a waiting-room; the + // button just re-enables for a manual retry). #2804 / #2937. + // + // Announcements are throttled: screen readers shouldn't hear every + // tick of the countdown. `aria-live="polite"` on the node lets + // the browser debounce, but the text itself changes every second. + // We also track `lastAnnouncedSec` per-node so we only change + // textContent at meaningful intervals (every 10s + the last 5s + // + the terminal state). + function tickRateLimitCountdowns() { const nodes = document.querySelectorAll('[data-rate-limited-until]'); if (nodes.length === 0) return; const now = Date.now(); @@ -1334,28 +1390,65 @@

Agents

const until = parseInt(node.dataset.rateLimitedUntil || '', 10); if (!Number.isFinite(until)) continue; const remainingSec = Math.max(0, Math.ceil((until - now) / 1000)); + const card = node.closest('.agent-compliance-card'); + const retryBtn = card?.querySelector('.agent-reload-btn'); + const autoRetried = card?.dataset.autoRetried === '1'; + const leadingCopy = autoRetried + ? 'You\'ve refreshed too quickly — retry in' + : 'Rate-limited — retry in'; + if (remainingSec > 0) { - node.textContent = `Rate-limited — retry in ${remainingSec}s…`; - // Update the button label too so disabled button reflects - // the live countdown instead of a stale "Retry in 30s". - const card = node.closest('.agent-compliance-card'); - const retryBtn = card?.querySelector('.agent-reload-btn'); + // Throttle textContent updates for a11y — screen readers + // announce changes; every-second updates are noisy. Only + // update on 10s boundaries, the final 5s, or when the + // card was just rendered. + const lastAnnounced = parseInt(node.dataset.lastAnnouncedSec || '', 10); + const shouldAnnounce = + !Number.isFinite(lastAnnounced) || + remainingSec <= 5 || + remainingSec % 10 === 0 || + Math.abs(lastAnnounced - remainingSec) >= 10; + if (shouldAnnounce) { + node.textContent = `${leadingCopy} ${remainingSec}s…`; + node.dataset.lastAnnouncedSec = String(remainingSec); + } if (retryBtn && retryBtn.disabled) { retryBtn.textContent = `Retry in ${remainingSec}s`; } continue; } - // Countdown expired. Clear the attribute so we don't fire - // multiple retries if the re-render is slow, then trigger. + + // Countdown expired. Clear attributes so a stuck render + // doesn't trigger multiple retries. const cardId = node.dataset.cardId; - const card = document.getElementById(cardId); - const agentUrl = card?.querySelector('.agent-reload-btn')?.dataset.agentUrl; + const agentUrl = retryBtn?.dataset.agentUrl; + const shouldAutoRetry = node.dataset.rateLimitedAuto === '1'; node.removeAttribute('data-rate-limited-until'); - if (cardId && agentUrl) { + node.removeAttribute('data-rate-limited-auto'); + + if (shouldAutoRetry && cardId && agentUrl) { + // A small "Retrying now…" beat before the card swap so the + // transition reads as an action, not a sudden DOM change. + // The swap in retryAgentCard will overwrite this immediately + // on success or replace the whole node on failure. + node.textContent = 'Retrying now…'; retryAgentCard(agentUrl, cardId, { auto: true }); + } else if (retryBtn) { + // Second-429 waiting-room: just re-enable the button so + // the user can manually try again. + retryBtn.disabled = false; + retryBtn.textContent = 'Retry'; } } - }, 1000); + } + setInterval(tickRateLimitCountdowns, 1000); + + // Hidden tabs throttle setInterval, so a user flipping back sees + // a stuck countdown that then jumps. Force a tick on visible so + // the display is current (#2939). + document.addEventListener('visibilitychange', function() { + if (document.visibilityState === 'visible') tickRateLimitCountdowns(); + }); // Connect agent toggle document.addEventListener('click', function(e) { diff --git a/server/src/middleware/rate-limit.ts b/server/src/middleware/rate-limit.ts index 3cd0edf032..40e19ca65a 100644 --- a/server/src/middleware/rate-limit.ts +++ b/server/src/middleware/rate-limit.ts @@ -5,6 +5,28 @@ import { CachedPostgresStore } from './pg-rate-limit-store.js'; const logger = createLogger('rate-limit'); +/** + * Parse a `Retry-After` header value into delta-seconds. Handles both + * the number form (express-rate-limit with `standardHeaders: true` + * emits seconds as a number) and the string form. Returns `undefined` + * for anything that doesn't look like a positive integer — including + * zero, which we treat as "no meaningful wait" rather than exposing + * a degenerate countdown value. + * + * Callers surface the value in the 429 body as a proxy-stripped + * fallback for the header (#2804). + */ +export function parseRetryAfterSeconds(raw: number | string | string[] | undefined): number | undefined { + if (typeof raw === 'number') { + return Number.isFinite(raw) && raw > 0 ? raw : undefined; + } + if (typeof raw === 'string') { + const parsed = parseInt(raw, 10); + return Number.isFinite(parsed) && parsed > 0 ? parsed : undefined; + } + return undefined; +} + /** * Generate a rate limit key from request, preferring user ID over IP. * Uses proper IPv6 subnet masking when falling back to IP addresses. @@ -233,12 +255,12 @@ export const agentReadRateLimiter = rateLimit({ // headers, and the dashboard needs SOMETHING to key its countdown // off. `retryAfter` is seconds-to-retry, matching the header's // delta-seconds format. - const retryAfterHeader = res.getHeader('Retry-After'); - const retryAfter = typeof retryAfterHeader === 'number' - ? retryAfterHeader - : typeof retryAfterHeader === 'string' - ? parseInt(retryAfterHeader, 10) || undefined - : undefined; + // + // The HTTP spec (RFC 9110 §10.2.3) also allows an HTTP-date here, + // but express-rate-limit only emits delta-seconds — so a parseInt + // is sufficient. If that ever changes (e.g., we swap limiter + // libraries), the fallback below would need a second parse path. + const retryAfter = parseRetryAfterSeconds(res.getHeader('Retry-After')); res.status(429).json({ error: 'Too many requests', message: 'Agent dashboard read rate limit exceeded. Please try again in a moment.', diff --git a/server/tests/unit/rate-limit-retry-after.test.ts b/server/tests/unit/rate-limit-retry-after.test.ts new file mode 100644 index 0000000000..e2554d34f8 --- /dev/null +++ b/server/tests/unit/rate-limit-retry-after.test.ts @@ -0,0 +1,79 @@ +import { describe, it, expect } from 'vitest'; +import express from 'express'; +import request from 'supertest'; +import { parseRetryAfterSeconds, agentReadRateLimiter } from '../../src/middleware/rate-limit.js'; + +/** + * Tests for the retryAfter fallback field we surface on the 429 body + * from `agentReadRateLimiter` (#2804/#2939). Reverse proxies sometimes + * strip non-standard response headers; the dashboard client falls back + * to the JSON body when the `Retry-After` header is missing. + */ + +describe('parseRetryAfterSeconds', () => { + it('accepts positive integers', () => { + expect(parseRetryAfterSeconds(30)).toBe(30); + expect(parseRetryAfterSeconds('15')).toBe(15); + }); + + it('rejects zero — the client treats a zero countdown as "no hint" rather than a degenerate tick', () => { + expect(parseRetryAfterSeconds(0)).toBeUndefined(); + expect(parseRetryAfterSeconds('0')).toBeUndefined(); + }); + + it('rejects negatives and non-finite numbers', () => { + expect(parseRetryAfterSeconds(-5)).toBeUndefined(); + expect(parseRetryAfterSeconds(Number.NaN)).toBeUndefined(); + expect(parseRetryAfterSeconds(Infinity)).toBeUndefined(); + }); + + it('rejects non-numeric strings', () => { + expect(parseRetryAfterSeconds('soon')).toBeUndefined(); + expect(parseRetryAfterSeconds('')).toBeUndefined(); + }); + + it('returns undefined for unexpected shapes (array, undefined)', () => { + expect(parseRetryAfterSeconds(undefined)).toBeUndefined(); + expect(parseRetryAfterSeconds(['30', '60'])).toBeUndefined(); + }); +}); + +describe('agentReadRateLimiter 429 body', () => { + // Exercise the actual middleware through a tiny express app so the + // assertion lives at the same layer production depends on. This is + // cheap — the limiter uses a cached Postgres store by default, but + // in tests we use a trivial in-memory store by monkey-patching the + // store interface isn't exposed here. Instead we just overwhelm + // the limiter by setting a very low max via the existing limiter + // and checking that the 429 body carries retryAfter. + // + // The live limiter has max=240/min; we can't easily reach that in a + // unit test. Instead, mount it on a dummy route and fire 241 reqs + // so the 429 fires on the last one. + function buildApp() { + const app = express(); + app.get('/ping', agentReadRateLimiter, (_req, res) => res.status(200).json({ ok: true })); + return app; + } + + it('includes `retryAfter` (seconds) in the body when the header is set', async () => { + const app = buildApp(); + // Race past the 240/min cap. Same IP so the limiter keys identically. + // We do this serially because express-rate-limit's in-flight + // tracking can be fiddly with parallel supertest calls and the + // test point is the 429 body, not concurrent behavior. + let last: Awaited>; + for (let i = 0; i < 241; i++) { + last = await request(app).get('/ping'); + } + expect(last!.status).toBe(429); + expect(last!.body.error).toBe('Too many requests'); + expect(typeof last!.body.retryAfter).toBe('number'); + expect(last!.body.retryAfter).toBeGreaterThan(0); + // Header and body should agree. + const headerSeconds = parseInt(last!.headers['retry-after'] ?? '', 10); + if (Number.isFinite(headerSeconds) && headerSeconds > 0) { + expect(last!.body.retryAfter).toBe(headerSeconds); + } + }, 30_000); +}); From 9d11a4740b96e83a6122f6658429a9216899b754 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Thu, 23 Apr 2026 13:16:45 -0400 Subject: [PATCH 2/3] fix(dashboard): expert-review follow-ups on #2941 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All Must-Fix from the DX review + Should-Fix items from code/testing: - **aria scope narrowed.** `aria-atomic="true"` on the full countdown container was re-announcing the entire string ("Still rate-limited — retry in 30s…") every tick. Split into a static prefix outside the live region and a nested live region holding just the dynamic number. Screen readers now hear just "30 seconds" at 10s intervals. (DX Must-Fix) - **Card height stability.** Added `min-height: 120px` to `.agent-compliance-card` so the skeleton → hydrated transition doesn't cause a reflow cascade. The rate-limit card and the fully-passed card differ substantially in intrinsic height; reserving a minimum dampens the jump for users with `prefers-reduced-motion`. (DX Must-Fix) - **Less blameful copy.** `You've refreshed too quickly` → `Still rate-limited`. The user didn't refresh — the page auto- retried on their behalf. (DX Should-Fix) - **`cleanedUp` → `shouldClearAutoRetried`** with a one-line comment clarifying why a non-429 error also clears the marker (window elapsed, not limiter's problem; fresh state for the next 429). (code Should-Fix) - **Defensive warn in `swapAgentCard`** when the card isn't in the DOM. Shouldn't happen in the current flow but would silently hide the bug class if a future refactor moved render behind a microtask. (code Should-Fix) - **Test: reset the limiter in `beforeEach`.** `agentReadRateLimiter` is a module-singleton; the counter persists across tests and can silently break the 241-request assumption in future suites. `resetKey('::ffff:127.0.0.1')` scrubs it. (testing Should-Fix) - **Test: unconditional header/body cross-check.** If `Retry-After` is missing or malformed, fail loudly rather than silently skip the assertion. (testing Should-Fix) Deferred as follow-ups (none are bugs today): - 250ms delay before retry so "Retrying now…" announcement lands before DOM replacement (interaction effect, hard to test). - AbortController on in-flight fetches when cancellation fires (currently just lets responses drop on the floor). - `pageshow` listener for bfcache-restore on mobile Safari. 1946 server + 631 root unit tests pass, typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- server/public/dashboard-agents.html | 106 ++++++++++++------ .../tests/unit/rate-limit-retry-after.test.ts | 49 ++++---- 2 files changed, 102 insertions(+), 53 deletions(-) diff --git a/server/public/dashboard-agents.html b/server/public/dashboard-agents.html index a23a161a51..cd7f5fe7c5 100644 --- a/server/public/dashboard-agents.html +++ b/server/public/dashboard-agents.html @@ -72,6 +72,15 @@ border: var(--border-1) solid var(--color-border); border-radius: var(--radius-lg); margin-bottom: var(--space-3); + /* + * Skeleton → hydrated transition can change the card's intrinsic + * height substantially (rate-limited card ≈ header + 1 line; + * fully-passed card ≈ header + headline + track pills + history + * sparkline). Reserving a minimum height dampens the reflow + * cascade on list hydration — especially material for users + * with prefers-reduced-motion (#2939 DX review). + */ + min-height: 120px; } .agent-compliance-card:last-child { margin-bottom: 0; } @@ -1135,15 +1144,30 @@

Agents

? Date.now() + effectiveRetryAfterSeconds * 1000 : null; - let errMsg; + // Rate-limit copy splits into static prefix (outside the + // live region — doesn't re-announce every tick) + dynamic + // countdown number (inside the aria-live region). Screen + // readers hear "12 seconds" at 10s intervals instead of the + // full "Still rate-limited — retry in 12s" each tick. + // "Still rate-limited" (vs the earlier "you've refreshed too + // quickly") is accurate without blaming the user for the + // auto-retry they didn't ask for. + let staticPrefix; + let countdownSuffix; + let plainMsg = ''; if (!isRateLimited) { - errMsg = 'Couldn\'t load compliance data. The service may be temporarily unavailable.'; - } else if (autoRetried && effectiveRetryAfterSeconds !== null) { - errMsg = `You've refreshed too quickly — retry in ${effectiveRetryAfterSeconds}s…`; + plainMsg = 'Couldn\'t load compliance data. The service may be temporarily unavailable.'; + staticPrefix = ''; + countdownSuffix = ''; } else if (effectiveRetryAfterSeconds !== null) { - errMsg = `Rate-limited — retry in ${effectiveRetryAfterSeconds}s…`; + staticPrefix = autoRetried + ? 'Still rate-limited — retry in ' + : 'Rate-limited — retry in '; + countdownSuffix = `${effectiveRetryAfterSeconds}s…`; } else { - errMsg = 'Rate-limited — retry in a moment.'; + plainMsg = 'Rate-limited — retry in a moment.'; + staticPrefix = ''; + countdownSuffix = ''; } // Disable the retry button whenever we have a known window; @@ -1155,11 +1179,16 @@

Agents

: 'Retry'; // The countdown only auto-retries on the FIRST 429 — second // 429 is purely a waiting-room; the button re-enables at - // zero for a manual retry. That gap is encoded in the - // `data-rate-limited-auto` flag so the interval logic stays - // simple. + // zero for a manual retry. const shouldAutoRetryAtZero = retryAtMs !== null && !autoRetried; + // When we have a countdown: static text + nested live region + // holding just the number. When we don't (plain error): a + // single paragraph, no live region. + const bodyHtml = retryAtMs !== null + ? `
${escapeHtml(staticPrefix)}${escapeHtml(countdownSuffix)}
` + : `
${escapeHtml(plainMsg)}
`; + return `
@@ -1172,13 +1201,7 @@

Agents

-
- ${escapeHtml(errMsg)} -
+ ${bodyHtml} ${visibilitySelectorHtml}
`; } @@ -1312,7 +1335,14 @@

Agents

if (clearAutoRetried) pageState.autoRetriedAgents.delete(agentUrl); const cardId = 'agent-' + encodeURIComponent(agentUrl).replace(/%/g, '_'); const card = document.getElementById(cardId); - if (!card) return; + if (!card) { + // This branch shouldn't fire in today's flow — initial render + // inserts cards synchronously before the worker pool awaits. + // Log loudly if it does, because silent returns hide the bug + // class if anyone later moves the render behind a microtask. + console.warn('swapAgentCard: card not found for', agentUrl); + return; + } const agent = pageState.agents.find(a => a.url === agentUrl) || { url: agentUrl }; const newHtml = renderAgentsSection([agent], pageState.complianceMap, pageState.orgId); const wrapper = document.createElement('div'); @@ -1344,11 +1374,14 @@

Agents

try { const state = await fetchAgentState({ url: agentUrl }); pageState.complianceMap.set(agentUrl, state); - // Successful refresh (no loadError, or at least a non-429 - // response) → clear any auto-retry marker so a future 429 - // later in the session starts from a clean slate (#2938). - const cleanedUp = !state.loadError || state.loadError.code !== 429; - swapAgentCard(agentUrl, { clearAutoRetried: cleanedUp }); + // Clear any prior auto-retry marker UNLESS we hit another 429 + // — keeping the marker on a back-to-back 429 is what drives + // the second-429 "Still rate-limited" UI. Anything else (a + // success or a non-429 failure) means the limiter window has + // elapsed or was never the problem; wipe the marker so a + // later 429 this session starts fresh (#2938). + const shouldClearAutoRetried = !state.loadError || state.loadError.code !== 429; + swapAgentCard(agentUrl, { clearAutoRetried: shouldClearAutoRetried }); } catch (err) { console.error('Retry failed:', err); if (retryBtn) { @@ -1383,6 +1416,10 @@

Agents

// textContent at meaningful intervals (every 10s + the last 5s // + the terminal state). function tickRateLimitCountdowns() { + // The live region is the narrow holding only the + // countdown suffix ("30s…"). The static prefix ("Rate-limited + // — retry in ") is outside the live region so screen readers + // don't re-announce the same context string on every tick. const nodes = document.querySelectorAll('[data-rate-limited-until]'); if (nodes.length === 0) return; const now = Date.now(); @@ -1392,16 +1429,12 @@

Agents

const remainingSec = Math.max(0, Math.ceil((until - now) / 1000)); const card = node.closest('.agent-compliance-card'); const retryBtn = card?.querySelector('.agent-reload-btn'); - const autoRetried = card?.dataset.autoRetried === '1'; - const leadingCopy = autoRetried - ? 'You\'ve refreshed too quickly — retry in' - : 'Rate-limited — retry in'; if (remainingSec > 0) { // Throttle textContent updates for a11y — screen readers - // announce changes; every-second updates are noisy. Only - // update on 10s boundaries, the final 5s, or when the - // card was just rendered. + // announce changes on polite regions; every-second updates + // are noisy. Announce at 10s boundaries, the final 5s, or + // when the node was just rendered. const lastAnnounced = parseInt(node.dataset.lastAnnouncedSec || '', 10); const shouldAnnounce = !Number.isFinite(lastAnnounced) || @@ -1409,9 +1442,12 @@

Agents

remainingSec % 10 === 0 || Math.abs(lastAnnounced - remainingSec) >= 10; if (shouldAnnounce) { - node.textContent = `${leadingCopy} ${remainingSec}s…`; + node.textContent = `${remainingSec}s…`; node.dataset.lastAnnouncedSec = String(remainingSec); } + // Retry button label updates every tick regardless of the + // a11y throttle — the button isn't in a live region, so + // there's no announcement cost. (Intentional asymmetry.) if (retryBtn && retryBtn.disabled) { retryBtn.textContent = `Retry in ${remainingSec}s`; } @@ -1429,13 +1465,15 @@

Agents

if (shouldAutoRetry && cardId && agentUrl) { // A small "Retrying now…" beat before the card swap so the // transition reads as an action, not a sudden DOM change. - // The swap in retryAgentCard will overwrite this immediately - // on success or replace the whole node on failure. + // retryAgentCard's swap will replace the whole card shortly. node.textContent = 'Retrying now…'; retryAgentCard(agentUrl, cardId, { auto: true }); } else if (retryBtn) { - // Second-429 waiting-room: just re-enable the button so - // the user can manually try again. + // Second-429 waiting-room: re-enable the button for manual + // retry. Leave the "Still rate-limited" prefix visible — + // clearing only the dynamic span so the static context + // stays present without a stale time. + node.textContent = 'now'; retryBtn.disabled = false; retryBtn.textContent = 'Retry'; } diff --git a/server/tests/unit/rate-limit-retry-after.test.ts b/server/tests/unit/rate-limit-retry-after.test.ts index e2554d34f8..f8ecaeaebb 100644 --- a/server/tests/unit/rate-limit-retry-after.test.ts +++ b/server/tests/unit/rate-limit-retry-after.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, beforeEach } from 'vitest'; import express from 'express'; import request from 'supertest'; import { parseRetryAfterSeconds, agentReadRateLimiter } from '../../src/middleware/rate-limit.js'; @@ -40,28 +40,34 @@ describe('parseRetryAfterSeconds', () => { describe('agentReadRateLimiter 429 body', () => { // Exercise the actual middleware through a tiny express app so the - // assertion lives at the same layer production depends on. This is - // cheap — the limiter uses a cached Postgres store by default, but - // in tests we use a trivial in-memory store by monkey-patching the - // store interface isn't exposed here. Instead we just overwhelm - // the limiter by setting a very low max via the existing limiter - // and checking that the 429 body carries retryAfter. - // - // The live limiter has max=240/min; we can't easily reach that in a - // unit test. Instead, mount it on a dummy route and fire 241 reqs - // so the 429 fires on the last one. + // assertion lives at the same layer production depends on. + // `agentReadRateLimiter` is a module-singleton with a cached + // Postgres store — the increment hot path is in-memory, so no real + // DB is needed, but the counter persists across tests. `resetKey` + // scrubs the per-key counter in `beforeEach` so each test starts + // from zero regardless of what ran earlier in the suite. function buildApp() { const app = express(); app.get('/ping', agentReadRateLimiter, (_req, res) => res.status(200).json({ ok: true })); return app; } - it('includes `retryAfter` (seconds) in the body when the header is set', async () => { + // Supertest hits loopback; the limiter's keyGenerator falls back to + // `::ffff:127.0.0.1` (IPv4-mapped IPv6) when no req.user is present. + const LOOPBACK_KEY = '::ffff:127.0.0.1'; + beforeEach(async () => { + await agentReadRateLimiter.resetKey(LOOPBACK_KEY); + // Also clear the raw IPv4 key as a belt-and-braces in case + // Node/Express ever flips the default representation. + await agentReadRateLimiter.resetKey('127.0.0.1'); + }); + + it('includes `retryAfter` (seconds) in the body matching the Retry-After header', async () => { const app = buildApp(); // Race past the 240/min cap. Same IP so the limiter keys identically. - // We do this serially because express-rate-limit's in-flight - // tracking can be fiddly with parallel supertest calls and the - // test point is the 429 body, not concurrent behavior. + // Serial rather than parallel — express-rate-limit's in-flight + // tracking is more deterministic, and the assertion is about the + // 429 body shape, not concurrent behavior. let last: Awaited>; for (let i = 0; i < 241; i++) { last = await request(app).get('/ping'); @@ -70,10 +76,15 @@ describe('agentReadRateLimiter 429 body', () => { expect(last!.body.error).toBe('Too many requests'); expect(typeof last!.body.retryAfter).toBe('number'); expect(last!.body.retryAfter).toBeGreaterThan(0); - // Header and body should agree. + + // Unconditional cross-check: the body's `retryAfter` must equal + // the `Retry-After` header's delta-seconds. If the header is + // malformed or missing, this assertion fails loudly rather than + // silently skipping — either outcome would be a bug the test + // needs to catch. const headerSeconds = parseInt(last!.headers['retry-after'] ?? '', 10); - if (Number.isFinite(headerSeconds) && headerSeconds > 0) { - expect(last!.body.retryAfter).toBe(headerSeconds); - } + expect(Number.isFinite(headerSeconds)).toBe(true); + expect(headerSeconds).toBeGreaterThan(0); + expect(last!.body.retryAfter).toBe(headerSeconds); }, 30_000); }); From 4f8d9eb06e45679138938fd73d3fe508cf00f637 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Thu, 23 Apr 2026 14:00:20 -0400 Subject: [PATCH 3/3] =?UTF-8?q?fix(dashboard):=20restructure=20rate-limit?= =?UTF-8?q?=20copy=20=E2=80=94=20live=20span=20owns=20the=20full=20clause?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second expert review pass flagged two grammar regressions from my earlier aria-scope narrowing: - "Rate-limited — retry in " + span="Retrying now…" → rendered as "Rate-limited — retry in Retrying now…" - "Still rate-limited — retry in " + span="now" → rendered as "Still rate-limited — retry in now" Root cause: the static prefix carried a dangling preposition ("retry in ") that only made sense when the span was a time. Every other transition the span needed to support read as broken English. Fix: split at the status LABEL, not mid-sentence. Static part is now just "Rate-limited" / "Still rate-limited" — a short noun phrase that reads fine next to anything. The live span owns the em-dash and everything after: " — retry in 30s…", " — retrying now…", " — try again". Every transition is a grammatical self-contained clause. Also dropped the 120px min-height per DX feedback. It was a token gesture — too low to prevent the skeleton → hydrated grow jump, and adding awkward empty space to short rate-limited cards. Layout-shift prevention would need skeleton cards sized to match hydrated ones, which is a different refactor. 1946 server + 631 root unit tests pass, typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- server/public/dashboard-agents.html | 76 ++++++++++++++--------------- 1 file changed, 37 insertions(+), 39 deletions(-) diff --git a/server/public/dashboard-agents.html b/server/public/dashboard-agents.html index cd7f5fe7c5..fc474299cd 100644 --- a/server/public/dashboard-agents.html +++ b/server/public/dashboard-agents.html @@ -72,15 +72,6 @@ border: var(--border-1) solid var(--color-border); border-radius: var(--radius-lg); margin-bottom: var(--space-3); - /* - * Skeleton → hydrated transition can change the card's intrinsic - * height substantially (rate-limited card ≈ header + 1 line; - * fully-passed card ≈ header + headline + track pills + history - * sparkline). Reserving a minimum height dampens the reflow - * cascade on list hydration — especially material for users - * with prefers-reduced-motion (#2939 DX review). - */ - min-height: 120px; } .agent-compliance-card:last-child { margin-bottom: 0; } @@ -1144,29 +1135,31 @@

Agents

? Date.now() + effectiveRetryAfterSeconds * 1000 : null; - // Rate-limit copy splits into static prefix (outside the - // live region — doesn't re-announce every tick) + dynamic - // countdown number (inside the aria-live region). Screen - // readers hear "12 seconds" at 10s intervals instead of the - // full "Still rate-limited — retry in 12s" each tick. - // "Still rate-limited" (vs the earlier "you've refreshed too - // quickly") is accurate without blaming the user for the - // auto-retry they didn't ask for. - let staticPrefix; + // Rate-limit copy splits into a stable status LABEL (the + // short, never-changing "Rate-limited" / "Still rate-limited" + // — outside the live region) and a dynamic SUFFIX that owns + // everything after it ("— retry in 30s…" / "— retrying now…" + // / "— try again" — inside the live region, so screen readers + // announce the whole updated clause as a unit). Earlier this + // tried to put the static text "retry in " in the prefix, + // which then left dangling prepositions when the span + // rewrote to "Retrying now…" or "now" — the expert reviewers + // flagged both. Owning the whole clause from the preposition + // onward keeps the countdown updates grammatical through + // every transition. + let statusLabel; let countdownSuffix; let plainMsg = ''; if (!isRateLimited) { plainMsg = 'Couldn\'t load compliance data. The service may be temporarily unavailable.'; - staticPrefix = ''; + statusLabel = ''; countdownSuffix = ''; } else if (effectiveRetryAfterSeconds !== null) { - staticPrefix = autoRetried - ? 'Still rate-limited — retry in ' - : 'Rate-limited — retry in '; - countdownSuffix = `${effectiveRetryAfterSeconds}s…`; + statusLabel = autoRetried ? 'Still rate-limited' : 'Rate-limited'; + countdownSuffix = ` — retry in ${effectiveRetryAfterSeconds}s…`; } else { plainMsg = 'Rate-limited — retry in a moment.'; - staticPrefix = ''; + statusLabel = ''; countdownSuffix = ''; } @@ -1182,11 +1175,13 @@

Agents

// zero for a manual retry. const shouldAutoRetryAtZero = retryAtMs !== null && !autoRetried; - // When we have a countdown: static text + nested live region - // holding just the number. When we don't (plain error): a - // single paragraph, no live region. + // Structure: static status LABEL outside the live region, + // dynamic clause (from the em-dash onward) inside it. The + // live span owns the transition between "— retry in 30s…", + // "— retrying now…", and "— try again" so every variant + // reads cleanly next to the static label. const bodyHtml = retryAtMs !== null - ? `
${escapeHtml(staticPrefix)}${escapeHtml(countdownSuffix)}
` + ? `
${escapeHtml(statusLabel)}${escapeHtml(countdownSuffix)}
` : `
${escapeHtml(plainMsg)}
`; return ` @@ -1416,10 +1411,13 @@

Agents

// textContent at meaningful intervals (every 10s + the last 5s // + the terminal state). function tickRateLimitCountdowns() { - // The live region is the narrow holding only the - // countdown suffix ("30s…"). The static prefix ("Rate-limited - // — retry in ") is outside the live region so screen readers - // don't re-announce the same context string on every tick. + // The live region is the owning the dynamic clause from + // the em-dash onward (" — retry in 30s…" / " — retrying now…" + // / " — try again"). The static status label ("Rate-limited" / + // "Still rate-limited") sits outside the live region so screen + // readers don't re-announce it on every tick, but the span + // always carries a grammatical self-contained clause — no + // dangling prepositions when it rewrites to "retrying now…". const nodes = document.querySelectorAll('[data-rate-limited-until]'); if (nodes.length === 0) return; const now = Date.now(); @@ -1442,7 +1440,7 @@

Agents

remainingSec % 10 === 0 || Math.abs(lastAnnounced - remainingSec) >= 10; if (shouldAnnounce) { - node.textContent = `${remainingSec}s…`; + node.textContent = ` — retry in ${remainingSec}s…`; node.dataset.lastAnnouncedSec = String(remainingSec); } // Retry button label updates every tick regardless of the @@ -1463,17 +1461,17 @@

Agents

node.removeAttribute('data-rate-limited-auto'); if (shouldAutoRetry && cardId && agentUrl) { - // A small "Retrying now…" beat before the card swap so the + // A small "retrying now…" beat before the card swap so the // transition reads as an action, not a sudden DOM change. // retryAgentCard's swap will replace the whole card shortly. - node.textContent = 'Retrying now…'; + node.textContent = ' — retrying now…'; retryAgentCard(agentUrl, cardId, { auto: true }); } else if (retryBtn) { // Second-429 waiting-room: re-enable the button for manual - // retry. Leave the "Still rate-limited" prefix visible — - // clearing only the dynamic span so the static context - // stays present without a stale time. - node.textContent = 'now'; + // retry. The span rewrites to a grammatical clause ("— try + // again") so the card reads "Still rate-limited — try again" + // until the user clicks. + node.textContent = ' — try again'; retryBtn.disabled = false; retryBtn.textContent = 'Retry'; }