diff --git a/frontend/src/__tests__/riexchange-automation-settings.test.ts b/frontend/src/__tests__/riexchange-automation-settings.test.ts index f157719b..79b28151 100644 --- a/frontend/src/__tests__/riexchange-automation-settings.test.ts +++ b/frontend/src/__tests__/riexchange-automation-settings.test.ts @@ -165,8 +165,11 @@ describe('loadAutomationSettings: loading and error stay inside the section fram expect(container.children).toHaveLength(1); expect(fieldset?.querySelector('legend')?.textContent).toBe('Exchange Automation'); expect(fieldset?.getAttribute('aria-busy')).toBe('true'); - expect(fieldset?.querySelector('.loading')?.textContent).toBe('Loading exchange automation settings...'); + // Issue #186: the shared skeleton, not a visible loading paragraph. expect(fieldset?.querySelector('.skeleton-group')).not.toBeNull(); + expect(fieldset?.querySelector('.skeleton-status')?.textContent).toBe('Loading…'); + expect(fieldset?.querySelector('p.loading')).toBeNull(); + expect(container.textContent).not.toContain('Loading settings...'); resolve(Promise.reject(new Error('Failed to fetch'))); (settings.isPermissionDeniedError as jest.Mock).mockReturnValue(false); diff --git a/frontend/src/__tests__/skeleton.test.ts b/frontend/src/__tests__/skeleton.test.ts index 767c8cad..c4fedf9f 100644 --- a/frontend/src/__tests__/skeleton.test.ts +++ b/frontend/src/__tests__/skeleton.test.ts @@ -13,8 +13,10 @@ import { showSkeletonTiles, showSkeletonRows, showSkeletonBlock, + showSkeletonText, teardownSkeleton, isSkeletonActive, + SKELETON_SLOW_MS, } from '../lib/skeleton'; describe('skeleton primitives', () => { @@ -174,6 +176,84 @@ describe('skeleton show / teardown lifecycle', () => { }); }); +describe('consistent loading pattern + slow fallback (issue #186)', () => { + let container: HTMLElement; + beforeEach(() => { + jest.useFakeTimers(); + container = document.createElement('div'); + document.body.appendChild(container); + }); + afterEach(() => { + jest.useRealTimers(); + container.remove(); + }); + + const slowNote = (): HTMLElement | null => container.querySelector('.skeleton-slow-note'); + + test('showSkeletonText renders N shimmer lines and marks active', () => { + showSkeletonText(container, 4); + expect(container.querySelectorAll('.skeleton-group .skeleton')).toHaveLength(4); + expect(isSkeletonActive(container)).toBe(true); + }); + + test.each([ + ['tiles', (c: HTMLElement) => showSkeletonTiles(c, 2)], + ['rows', (c: HTMLElement) => showSkeletonRows(c, 2, 2)], + ['block', (c: HTMLElement) => showSkeletonBlock(c, '1px', '1px')], + ['text', (c: HTMLElement) => showSkeletonText(c, 2)], + ])('%s skeleton carries one screen-reader "Loading…" status', (_name, show) => { + show(container); + show(container); + const status = container.querySelectorAll('.skeleton-status'); + expect(status).toHaveLength(1); + expect(status[0]!.getAttribute('role')).toBe('status'); + expect(status[0]!.classList.contains('sr-only')).toBe(true); + expect(status[0]!.textContent).toBe('Loading…'); + }); + + test('a skeleton still showing after the threshold gets a "Still loading" note', () => { + showSkeletonRows(container, 3, 3); + jest.advanceTimersByTime(SKELETON_SLOW_MS - 1); + expect(slowNote()).toBeNull(); + jest.advanceTimersByTime(1); + expect(slowNote()!.textContent).toBe('Still loading. This is taking longer than usual.'); + expect(slowNote()!.getAttribute('role')).toBe('status'); + // The shimmer stays; the note is added, not swapped in. + expect(container.querySelectorAll('tr.skeleton-row')).toHaveLength(3); + }); + + test('no note when the real content replaced the skeleton (innerHTML handoff)', () => { + showSkeletonRows(container, 3, 3); + container.innerHTML = '
real
'; + jest.advanceTimersByTime(SKELETON_SLOW_MS); + expect(slowNote()).toBeNull(); + }); + + test('no note after teardownSkeleton (error path)', () => { + showSkeletonText(container, 2); + teardownSkeleton(container); + jest.advanceTimersByTime(SKELETON_SLOW_MS); + expect(slowNote()).toBeNull(); + }); + + test('no note for a skeleton inside a hidden panel', () => { + container.classList.add('hidden'); + showSkeletonText(container, 2); + jest.advanceTimersByTime(SKELETON_SLOW_MS); + expect(slowNote()).toBeNull(); + }); + + test('re-showing restarts the clock and never stacks notes', () => { + showSkeletonTiles(container, 2); + jest.advanceTimersByTime(SKELETON_SLOW_MS - 1000); + showSkeletonTiles(container, 2); + jest.advanceTimersByTime(SKELETON_SLOW_MS - 1000); + expect(slowNote()).toBeNull(); + jest.advanceTimersByTime(1000); + expect(container.querySelectorAll('.skeleton-slow-note')).toHaveLength(1); + }); +}); + // Issue #729 (UX-34): the placeholders are aria-hidden, so the container // carries aria-busy while one is showing, and drops it once real content // (or nothing) replaces it, whichever path did the replacing. @@ -184,6 +264,7 @@ describe('skeleton aria-busy', () => { ['tiles', (c: HTMLElement): void => showSkeletonTiles(c, 2)], ['rows', (c: HTMLElement): void => showSkeletonRows(c, 2, 3)], ['block', (c: HTMLElement): void => showSkeletonBlock(c, '100%', '4rem')], + ['text', (c: HTMLElement): void => showSkeletonText(c, 2)], ])('show (%s) marks the container busy and keeps it busy after observers run', async (_name, show) => { const c = document.createElement('div'); c.innerHTML = '

old content

'; diff --git a/frontend/src/apikeys.ts b/frontend/src/apikeys.ts index 4a89ffa7..2f02d6a2 100644 --- a/frontend/src/apikeys.ts +++ b/frontend/src/apikeys.ts @@ -10,7 +10,7 @@ import { confirmDialog } from './confirmDialog'; import { friendlyError } from './api/errors'; import { showToast } from './toast'; import { openModal, closeModal } from './modal'; -import { showSkeletonRows, teardownSkeleton } from './lib/skeleton'; +import { showSkeletonRows, showSkeletonText, teardownSkeleton } from './lib/skeleton'; import { parseLocalDateInputValue, toLocalDateInputValue } from './lib/local-date'; // State for modal management @@ -285,7 +285,8 @@ export async function showCreateKeyModal(): Promise { } const permissionsContainer = document.getElementById('apikey-permissions-list'); - if (permissionsContainer) permissionsContainer.innerHTML = '

Loading your permissions…

'; + // Issue #186: skeleton instead of a "Loading your permissions…" line. + if (permissionsContainer) showSkeletonText(permissionsContainer, 3); // Open the modal before the permissions fetch resolves so the dialog // never sits closed while waiting on the network. diff --git a/frontend/src/lib/skeleton.ts b/frontend/src/lib/skeleton.ts index 582de643..e439eeb0 100644 --- a/frontend/src/lib/skeleton.ts +++ b/frontend/src/lib/skeleton.ts @@ -17,10 +17,21 @@ * All helpers use `createElement` only — no `innerHTML` — to match the * codebase's XSS posture from issue #340. * + * Issue #186: skeletons are the one loading pattern for panels (plain + * "Loading..." paragraphs were migrated to showSkeletonText). Every show* + * call also adds a screen-reader-only "Loading…" status, since the shimmer + * blocks are aria-hidden, and arms a slow-response fallback: if the + * skeleton is still on screen after SKELETON_SLOW_MS, a visible "Still + * loading" note is appended so a slow or stuck response does not look + * like a hang. + * * Accessibility (issue #729): the placeholders themselves are aria-hidden, * so the container carries `aria-busy="true"` while a skeleton is showing. * Success renders replace the children without calling back in here, so a * MutationObserver clears the flag once no placeholder is left. + * + * The two coexist: aria-busy marks the container, while the sr-only status + * and the slow note are what a screen reader actually hears. */ const SKELETON_SELECTOR = '.skeleton, .skeleton-group, .skeleton-tile, .skeleton-row, .skeleton-table'; @@ -46,6 +57,35 @@ function markBusy(container: HTMLElement): void { busyObservers.set(container, observer); } +/** How long a skeleton may show before the "Still loading" note appears. */ +export const SKELETON_SLOW_MS = 8_000; + +const SLOW_NOTE_TEXT = 'Still loading. This is taking longer than usual.'; + +const slowTimers = new WeakMap>(); + +function clearSlowTimer(container: HTMLElement): void { + const t = slowTimers.get(container); + if (t !== undefined) { + clearTimeout(t); + slowTimers.delete(container); + } +} + +/** + * True while the container still shows skeleton placeholders. Success + * renders replace the children (often via innerHTML, which leaves the + * data marker behind), so the shimmer elements themselves are the signal. + */ +function skeletonStillShown(container: HTMLElement): boolean { + return container.isConnected + && container.dataset['skeletonActive'] === '1' + && container.querySelector('.skeleton') !== null + // A placeholder whose panel was hidden (e.g. #settings-loading once the + // form shows) is not something the user is waiting on. + && container.closest('.hidden') === null; +} + /** * A single shimmer block. Width/height are CSS sizes (e.g. "60%", "1rem", * "12px"). Useful as a primitive inside higher-level helpers. @@ -123,7 +163,33 @@ export function skeletonRow(cols: number): HTMLTableRowElement { function activateSkeleton(container: HTMLElement): void { while (container.firstChild) container.removeChild(container.firstChild); container.dataset['skeletonActive'] = '1'; + + const status = document.createElement('span'); + status.className = 'sr-only skeleton-status'; + status.setAttribute('role', 'status'); + status.textContent = 'Loading…'; + container.appendChild(status); markBusy(container); + + clearSlowTimer(container); + slowTimers.set(container, setTimeout(() => { + slowTimers.delete(container); + if (!skeletonStillShown(container) || container.querySelector('.skeleton-slow-note')) return; + const note = document.createElement('p'); + note.className = 'skeleton-slow-note'; + note.setAttribute('role', 'status'); + note.textContent = SLOW_NOTE_TEXT; + container.appendChild(note); + }, SKELETON_SLOW_MS)); +} + +/** + * Replace the children of `container` with `lines` text-line skeletons — + * the panel/form counterpart of showSkeletonRows (issue #186). + */ +export function showSkeletonText(container: HTMLElement, lines: number): void { + activateSkeleton(container); + container.appendChild(skeletonText(lines)); } /** @@ -176,6 +242,7 @@ export function showSkeletonBlock(container: HTMLElement, width: string, height: * content. */ export function teardownSkeleton(container: HTMLElement): void { + clearSlowTimer(container); if (container.dataset['skeletonActive']) { while (container.firstChild) container.removeChild(container.firstChild); delete container.dataset['skeletonActive']; diff --git a/frontend/src/riexchange.ts b/frontend/src/riexchange.ts index d4f3f63a..52144a37 100644 --- a/frontend/src/riexchange.ts +++ b/frontend/src/riexchange.ts @@ -30,7 +30,7 @@ import type { Provider, } from './api'; import { openModal, closeModal } from './modal'; -import { showSkeletonRows, teardownSkeleton, skeletonText } from './lib/skeleton'; +import { showSkeletonRows, showSkeletonText, teardownSkeleton } from './lib/skeleton'; import { canAccess } from './permissions'; import { applyReadOnlySettings, isPermissionDeniedError } from './settings'; import { showToast } from './toast'; @@ -1689,12 +1689,14 @@ export async function loadAutomationSettings(): Promise { // Hold the section's place with its own titled frame while the config // loads (issue #190): a bare "Loading settings..." line between the GCP // block and Commitment Laddering read as the whole page half-loaded. - const loadingP = document.createElement('p'); - loadingP.className = 'loading'; - loadingP.textContent = 'Loading exchange automation settings...'; - const loadingShell = automationSectionShell(loadingP, skeletonText(3)); + // Inside the frame, the shared text skeleton (issue #186) stands in for + // the form: it brings the screen-reader "Loading…" status and the + // "Still loading" note, so no visible loading paragraph is needed. + const loadingBody = document.createElement('div'); + const loadingShell = automationSectionShell(loadingBody); loadingShell.setAttribute('aria-busy', 'true'); container.replaceChildren(loadingShell); + showSkeletonText(loadingBody, 4); try { const config = await api.getRIExchangeConfig(); @@ -1973,11 +1975,8 @@ async function loadExchangeHistory(generation: number = scopeGeneration): Promis const container = document.getElementById('ri-exchange-history-list'); if (!container) return; - container.textContent = ''; - const loadingP = document.createElement('p'); - loadingP.className = 'loading'; - loadingP.textContent = 'Loading exchange history...'; - container.appendChild(loadingP); + // Issue #186: skeleton rows shaped like the 7-column history table. + showSkeletonRows(container, 3, 7); try { const records = await api.getRIExchangeHistory(); diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index edc45829..071785ab 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -22,6 +22,7 @@ import { isServiceFilterError, } from './serviceFilters'; import { initLadderingSettings } from './ladder'; +import { showSkeletonText } from './lib/skeleton'; import { canonicalizeServiceConfigs, legacySettingsServiceTarget } from './lib/settings-service-ids'; import { buildLastUpdatedStamp } from './lib/last-updated'; @@ -1217,7 +1218,7 @@ function appendLegacyOverrideHint(cell: HTMLElement, o: api.AccountServiceOverri } export async function loadOverridesPanel(accountId: string, panel: HTMLElement, provider: AccountProvider): Promise { - panel.textContent = 'Loading\u2026'; + showSkeletonText(panel, 2); // issue #186: was a plain "Loading…" line try { const overrides = await api.listAccountServiceOverrides(accountId); panel.textContent = ''; @@ -3065,7 +3066,11 @@ export async function loadGlobalSettings(): Promise { const formEl = document.getElementById('global-settings-form'); const errorEl = document.getElementById('settings-error'); - if (loadingEl) loadingEl.classList.remove('hidden'); + if (loadingEl) { + loadingEl.classList.remove('hidden'); + // Issue #186: skeleton instead of the static "Loading settings..." text. + showSkeletonText(loadingEl, 6); + } if (formEl) formEl.classList.add('hidden'); if (errorEl) errorEl.classList.add('hidden'); diff --git a/frontend/src/styles/base.css b/frontend/src/styles/base.css index 681870ec..2efe48a6 100644 --- a/frontend/src/styles/base.css +++ b/frontend/src/styles/base.css @@ -295,6 +295,32 @@ body { margin-bottom: 0; } +/* Skeleton tables (showSkeletonRows). The shimmer boxes are 80% of an + * auto-sized cell; with nothing else in the cell, auto layout shrank the + * cells to their padding and the boxes to 0px, so on Opportunities the + * "skeleton" was a blank card that read as a hang (issue #186). Fixed + * layout spreads the columns over the full width and the min-width keeps + * every box visible on narrow viewports. */ +.skeleton-table { + width: 100%; + table-layout: fixed; +} + +.skeleton-row .skeleton { + min-width: 1.5rem; +} + +/* Slow-response fallback appended by lib/skeleton.ts after + * SKELETON_SLOW_MS (issue #186). Spans the full row when the skeleton + * container is a grid (Home KPI tiles, Plans cards). */ +.skeleton-slow-note { + grid-column: 1 / -1; + margin: var(--cudly-sp-3) 0 0; + color: var(--cudly-text-muted); + font-size: var(--cudly-fs-sm); + text-align: center; +} + @keyframes cudly-shimmer { 0% { background-position: 200% 0; } 100% { background-position: -200% 0; } diff --git a/frontend/src/styles/responsive.css b/frontend/src/styles/responsive.css index 9f5d8b20..8b5684cc 100644 --- a/frontend/src/styles/responsive.css +++ b/frontend/src/styles/responsive.css @@ -356,6 +356,12 @@ body.sidebar-open { overflow-x: auto; } +/* The loading skeleton has nothing to scroll; as a block its cells + * collapsed and the shimmer was invisible (issue #186). */ +#recommendations-list > table.skeleton-table { + display: table; +} + /* Wide-table scroll affordances (issue #185); lib/table-scroll.ts sets the * classes and variables. A scroller with columns cut off on one side fades * that edge out, so the clipped column reads as "more this way". The fade is diff --git a/frontend/tests-e2e/loading-fallback.spec.ts b/frontend/tests-e2e/loading-fallback.spec.ts new file mode 100644 index 00000000..6d5082fa --- /dev/null +++ b/frontend/tests-e2e/loading-fallback.spec.ts @@ -0,0 +1,36 @@ +import { test, expect } from '@playwright/test'; +import { mockApi, seedAuth, RECS, SUMMARY } from './fixtures/recs'; + +// Issue #186: the Opportunities skeleton is visible, a skeleton that sits for +// a long time gets a "Still loading" note so a slow response does not look +// like a hang, and the note goes away with the skeleton once data arrives. + +test('slow recommendations load shows a still-loading note, then the table', async ({ page }) => { + await seedAuth(page); + await mockApi(page); + let release: () => void = () => undefined; + const gate = new Promise((resolve) => { release = resolve; }); + await page.route(/\/api\/recommendations(?:\?.*)?$/, async (route) => { + await gate; + await route.fulfill({ json: { recommendations: RECS, summary: SUMMARY, regions: [] } }); + }); + await page.clock.install(); + await page.goto('/opportunities'); + + const list = page.locator('#recommendations-list'); + await expect(list.locator('.skeleton-row')).toHaveCount(5); + // The shimmer must actually be visible: the Opportunities table is + // display:block for scrolling, which used to collapse skeleton cells to 0px. + const box = await list.locator('.skeleton-row .skeleton').first().boundingBox(); + expect(box!.width).toBeGreaterThan(16); + await expect(list.locator('.skeleton-status')).toHaveText('Loading…'); + await expect(list.locator('.skeleton-slow-note')).toHaveCount(0); + + await page.clock.fastForward(8_000); + await expect(list.locator('.skeleton-slow-note')).toHaveText('Still loading. This is taking longer than usual.'); + + release(); + await expect(list.locator('tr.recommendation-row').first()).toBeVisible(); + await expect(list.locator('.skeleton-slow-note')).toHaveCount(0); + await expect(list.locator('.skeleton-status')).toHaveCount(0); +}); diff --git a/frontend/tests-e2e/purchasing-sections.spec.ts b/frontend/tests-e2e/purchasing-sections.spec.ts index f63124bf..a2fa0cd8 100644 --- a/frontend/tests-e2e/purchasing-sections.spec.ts +++ b/frontend/tests-e2e/purchasing-sections.spec.ts @@ -65,5 +65,9 @@ test('Exchange Automation loads inside its own titled frame', async ({ page }) = const frame = page.locator('#ri-exchange-automation-settings > fieldset'); await expect(frame.locator('legend')).toHaveText('Exchange Automation'); await expect(frame).toHaveAttribute('aria-busy', 'true'); - await expect(frame.locator('.loading')).toHaveText('Loading exchange automation settings...'); + // Issue #186: the shared skeleton (with its screen-reader status) fills the + // frame instead of a visible loading paragraph. + await expect(frame.locator('.skeleton-group')).toBeVisible(); + await expect(frame.locator('.skeleton-status')).toHaveText('Loading…'); + await expect(frame.locator('p.loading')).toHaveCount(0); });