Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
81 changes: 81 additions & 0 deletions frontend/src/__tests__/skeleton.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,10 @@ import {
showSkeletonTiles,
showSkeletonRows,
showSkeletonBlock,
showSkeletonText,
teardownSkeleton,
isSkeletonActive,
SKELETON_SLOW_MS,
} from '../lib/skeleton';

describe('skeleton primitives', () => {
Expand Down Expand Up @@ -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 = '<table><tbody><tr><td>real</td></tr></tbody></table>';
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.
Expand All @@ -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 = '<p>old content</p>';
Expand Down
5 changes: 3 additions & 2 deletions frontend/src/apikeys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -285,7 +285,8 @@ export async function showCreateKeyModal(): Promise<void> {
}

const permissionsContainer = document.getElementById('apikey-permissions-list');
if (permissionsContainer) permissionsContainer.innerHTML = '<p class="text-muted">Loading your permissions&hellip;</p>';
// 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.
Expand Down
67 changes: 67 additions & 0 deletions frontend/src/lib/skeleton.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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<HTMLElement, ReturnType<typeof setTimeout>>();

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.
Expand Down Expand Up @@ -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));
}

/**
Expand Down Expand Up @@ -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'];
Expand Down
19 changes: 9 additions & 10 deletions frontend/src/riexchange.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -1689,12 +1689,14 @@ export async function loadAutomationSettings(): Promise<void> {
// 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();
Expand Down Expand Up @@ -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();
Expand Down
9 changes: 7 additions & 2 deletions frontend/src/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -1217,7 +1218,7 @@ function appendLegacyOverrideHint(cell: HTMLElement, o: api.AccountServiceOverri
}

export async function loadOverridesPanel(accountId: string, panel: HTMLElement, provider: AccountProvider): Promise<void> {
panel.textContent = 'Loading\u2026';
showSkeletonText(panel, 2); // issue #186: was a plain "Loading…" line
try {
const overrides = await api.listAccountServiceOverrides(accountId);
panel.textContent = '';
Expand Down Expand Up @@ -3065,7 +3066,11 @@ export async function loadGlobalSettings(): Promise<void> {
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');

Expand Down
26 changes: 26 additions & 0 deletions frontend/src/styles/base.css
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Expand Down
6 changes: 6 additions & 0 deletions frontend/src/styles/responsive.css
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
36 changes: 36 additions & 0 deletions frontend/tests-e2e/loading-fallback.spec.ts
Original file line number Diff line number Diff line change
@@ -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<void>((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);
});
Loading
Loading