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
36 changes: 36 additions & 0 deletions frontend/src/__tests__/dashboard.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,42 @@ describe('Dashboard Module', () => {
expect(summary?.innerHTML).toContain('Failed to load dashboard');
});

// Issue #344 T3: skeleton lifecycle — skeleton renders synchronously
// at fetch start, then is replaced by the success render (clean
// handoff) or torn down on error so it never sits next to a stale
// error message.
test('error path tears down the loading skeleton', async () => {
(api.getDashboardSummary as jest.Mock).mockRejectedValue(new Error('API Error'));
console.error = jest.fn();

await loadDashboard();

const summary = document.getElementById('summary');
expect(summary?.querySelector('.skeleton-tile')).toBeNull();
expect(summary?.dataset['skeletonActive']).toBeUndefined();
});

test('success path replaces the loading skeleton with KPI tiles', async () => {
(api.getDashboardSummary as jest.Mock).mockResolvedValue({
potential_monthly_savings: 1000,
total_recommendations: 5,
active_commitments: 3,
committed_monthly: 500,
current_coverage: 70,
target_coverage: 80,
ytd_savings: 5000,
by_service: {}
});
(api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [] });

await loadDashboard();

const summary = document.getElementById('summary');
// Real KPI tiles render — skeleton placeholders are gone.
expect(summary?.querySelectorAll('.kpi-tile').length).toBeGreaterThan(0);
expect(summary?.querySelector('.skeleton-tile')).toBeNull();
});

test('uses current provider filter', async () => {
(state.getCurrentProvider as jest.Mock).mockReturnValue('aws');
(api.getDashboardSummary as jest.Mock).mockResolvedValue({
Expand Down
175 changes: 175 additions & 0 deletions frontend/src/__tests__/skeleton.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
/**
* Skeleton helper tests (issue #344 T3).
*
* Verifies the show / teardown lifecycle and that the helpers produce
* the DOM shape each call site expects (tile / row / box).
*/

import {
skeletonBox,
skeletonText,
skeletonTile,
skeletonRow,
showSkeletonTiles,
showSkeletonRows,
showSkeletonBlock,
teardownSkeleton,
isSkeletonActive,
} from '../lib/skeleton';

describe('skeleton primitives', () => {
test('skeletonBox renders a div with .skeleton + width/height styles', () => {
const el = skeletonBox('80%', '1rem');
expect(el.tagName).toBe('DIV');
expect(el.classList.contains('skeleton')).toBe(true);
expect(el.getAttribute('aria-hidden')).toBe('true');
expect(el.style.width).toBe('80%');
expect(el.style.height).toBe('1rem');
});

test('skeletonText emits N lines with a shorter final line by default', () => {
const group = skeletonText(3);
expect(group.children).toHaveLength(3);
const last = group.children[2] as HTMLElement;
expect(last.style.width).toBe('60%');
const first = group.children[0] as HTMLElement;
expect(first.style.width).toBe('100%');
});

test('skeletonText with widthVaries=false makes every line full-width', () => {
const group = skeletonText(2, false);
expect(group.children).toHaveLength(2);
Array.from(group.children).forEach((child) => {
expect((child as HTMLElement).style.width).toBe('100%');
});
});

test('skeletonText clamps non-positive line counts to 1', () => {
expect(skeletonText(0).children).toHaveLength(1);
expect(skeletonText(-3).children).toHaveLength(1);
});

test('skeletonTile has title + value + detail rows in tile shape', () => {
const tile = skeletonTile();
expect(tile.classList.contains('kpi-tile')).toBe(true);
expect(tile.classList.contains('skeleton-tile')).toBe(true);
expect(tile.children).toHaveLength(3);
});

test('skeletonRow renders a <tr> with N <td>s, each containing a skeleton', () => {
const tr = skeletonRow(5);
expect(tr.tagName).toBe('TR');
expect(tr.classList.contains('skeleton-row')).toBe(true);
expect(tr.children).toHaveLength(5);
Array.from(tr.children).forEach((td) => {
expect(td.tagName).toBe('TD');
expect(td.querySelector('.skeleton')).not.toBeNull();
});
});

test('skeletonRow clamps non-positive col counts to 1', () => {
expect(skeletonRow(0).children).toHaveLength(1);
expect(skeletonRow(-2).children).toHaveLength(1);
});
});

describe('skeleton show / teardown lifecycle', () => {
let container: HTMLElement;

beforeEach(() => {
container = document.createElement('div');
document.body.appendChild(container);
});

afterEach(() => {
document.body.removeChild(container);
});

test('showSkeletonTiles fills container with N tile skeletons + marks active', () => {
showSkeletonTiles(container, 4);
expect(container.querySelectorAll('.skeleton-tile')).toHaveLength(4);
expect(isSkeletonActive(container)).toBe(true);
});

test('showSkeletonRows fills container with N row skeletons + table shell', () => {
showSkeletonRows(container, 8, 6);
expect(container.querySelector('table.skeleton-table')).not.toBeNull();
expect(container.querySelectorAll('tr.skeleton-row')).toHaveLength(8);
expect(container.querySelectorAll('tr.skeleton-row td')).toHaveLength(8 * 6);
expect(isSkeletonActive(container)).toBe(true);
});

test('showSkeletonBlock fills container with one sized shimmer block', () => {
showSkeletonBlock(container, '100%', '12rem');
const skel = container.querySelector('.skeleton') as HTMLElement;
expect(skel).not.toBeNull();
expect(skel.style.width).toBe('100%');
expect(skel.style.height).toBe('12rem');
expect(isSkeletonActive(container)).toBe(true);
});

test('teardownSkeleton wipes children when active + clears the marker', () => {
showSkeletonTiles(container, 3);
teardownSkeleton(container);
expect(container.children).toHaveLength(0);
expect(isSkeletonActive(container)).toBe(false);
});

test('teardownSkeleton is a no-op when no skeleton is active', () => {
// Container has real content (post-render); teardown must NOT wipe it.
const real = document.createElement('p');
real.textContent = 'rendered';
container.appendChild(real);
teardownSkeleton(container);
expect(container.children).toHaveLength(1);
expect(container.firstChild).toBe(real);
});

test('successive show* calls swap the placeholder without leaking prior children', () => {
showSkeletonTiles(container, 4);
showSkeletonRows(container, 5, 3);
expect(container.querySelectorAll('.skeleton-tile')).toHaveLength(0);
expect(container.querySelectorAll('tr.skeleton-row')).toHaveLength(5);
});

// PR #346 CR follow-up: the previous implementation called
// teardownSkeleton (which is conditional on the marker) so a first
// render where the container already held real content would APPEND
// skeletons below it instead of replacing them. The show* helpers
// must always clear children unconditionally.
test('show* helpers replace existing real content unconditionally on first render', () => {
const stale = document.createElement('p');
stale.textContent = 'previous content (no skeleton marker)';
container.appendChild(stale);
expect(container.children).toHaveLength(1);

showSkeletonTiles(container, 3);

// The pre-existing <p> must be gone, not still sitting at the top.
expect(container.querySelector('p')).toBeNull();
expect(container.querySelectorAll('.skeleton-tile')).toHaveLength(3);
expect(isSkeletonActive(container)).toBe(true);
});

test('showSkeletonRows also clears unmarked prior content', () => {
const stale = document.createElement('div');
stale.classList.add('error');
stale.textContent = 'old error';
container.appendChild(stale);

showSkeletonRows(container, 4, 5);

expect(container.querySelector('.error')).toBeNull();
expect(container.querySelectorAll('tr.skeleton-row')).toHaveLength(4);
});

test('showSkeletonBlock also clears unmarked prior content', () => {
const stale = document.createElement('canvas');
container.appendChild(stale);

showSkeletonBlock(container, '100%', '6rem');

expect(container.querySelector('canvas')).toBeNull();
expect(container.querySelectorAll('.skeleton')).toHaveLength(1);
});
});
23 changes: 21 additions & 2 deletions frontend/src/dashboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@
import { Chart, registerables } from 'chart.js';
import * as api from './api';
import * as state from './state';
import { formatCurrency, getDateParts, escapeHtml } from './utils';
import { formatCurrency, getDateParts } from './utils';
import { renderFreshness } from './freshness';
import type { DashboardSummary, UpcomingPurchase, ServiceSavings, LocalRecommendation } from './types';
import type { SavingsDataPoint } from './api';
import { showToast } from './toast';
import { confirmDialog } from './confirmDialog';
import { groupRecsByCell, pageLevelRange, formatSavingsRange } from './recommendations';
import { showSkeletonTiles, showSkeletonBlock, teardownSkeleton } from './lib/skeleton';

// Register Chart.js components
Chart.register(...registerables);
Expand Down Expand Up @@ -51,6 +52,15 @@ export function setupDashboardHandlers(): void {
* Load dashboard data
*/
export async function loadDashboard(): Promise<void> {
// Issue #344 T3: render skeletons synchronously before kicking off the
// fetch so the panels show "loading" intent instead of staying blank.
// The success render replaces children for a clean handoff; the catch
// block calls teardownSkeleton before rendering the error.
const summaryEl = document.getElementById('summary');
if (summaryEl) showSkeletonTiles(summaryEl, 4);
const upcomingEl = document.getElementById('upcoming-list');
if (upcomingEl) showSkeletonBlock(upcomingEl, '100%', '6rem');

try {
const currentProvider = state.getCurrentProvider();
const currentAccountIDs = state.getCurrentAccountIDs();
Expand Down Expand Up @@ -102,9 +112,18 @@ export async function loadDashboard(): Promise<void> {
console.error('Failed to load dashboard:', error);
const summary = document.getElementById('summary');
if (summary) {
teardownSkeleton(summary);
const err = error as Error;
summary.innerHTML = `<p class="error">Failed to load dashboard: ${escapeHtml(err.message)}</p>`;
while (summary.firstChild) summary.removeChild(summary.firstChild);
const p = document.createElement('p');
p.classList.add('error');
p.textContent = `Failed to load dashboard: ${err.message}`;
summary.appendChild(p);
}
// Clear the upcoming-list skeleton too — shimmer next to a dashboard
// error reads as a fresh fetch in-flight, which is misleading.
const upcoming = document.getElementById('upcoming-list');
if (upcoming) teardownSkeleton(upcoming);
}
}

Expand Down
11 changes: 11 additions & 0 deletions frontend/src/history.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { switchTab } from './navigation';
import { confirmDialog } from './confirmDialog';
import { showToast } from './toast';
import { getCurrentUser } from './state';
import { showSkeletonRows, teardownSkeleton } from './lib/skeleton';

const VALID_PROVIDERS: api.Provider[] = ['aws', 'azure', 'gcp'];

Expand Down Expand Up @@ -157,6 +158,15 @@ function snapDateInputsToPurchases(purchases: HistoryPurchase[]): void {
* Load history with filters
*/
export async function loadHistory(): Promise<void> {
// Issue #344 T3: skeleton rows for the purchase-history table. 8
// rows matches the typical first-page row count so the skeleton
// doesn't shrink dramatically when real data arrives. Column count
// (11) mirrors the rendered table headers in renderHistoryList:
// Status / Date / Provider / Service / Type / Region / Count /
// Term / Upfront Cost / Monthly Savings / Plan.
const listEl = document.getElementById('history-list');
if (listEl) showSkeletonRows(listEl, 8, 11);

Comment thread
coderabbitai[bot] marked this conversation as resolved.
try {
// Provider/account filters live in state.ts now (mutated by topbar chips).
const rawProvider = state.getCurrentProvider();
Expand All @@ -180,6 +190,7 @@ export async function loadHistory(): Promise<void> {
console.error('Failed to load history:', error);
const list = document.getElementById('history-list');
if (list) {
teardownSkeleton(list);
const err = error as Error;
list.innerHTML = `<p class="error">Failed to load history: ${escapeHtml(err.message)}</p>`;
}
Expand Down
Loading
Loading