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 @@ -252,7 +252,8 @@ describe('regression: group edit no longer drops/widens unrepresentable permissi
const resourceSelect = document.querySelector('.perm-resource') as HTMLSelectElement;

// No breakout payload parsed as a live element anywhere in the modal.
expect(document.querySelectorAll('img, script, svg, iframe, style').length).toBe(0);
// svg.icon is the Remove button's own trash icon (src/icons.ts), not injected markup.
expect(document.querySelectorAll('img, script, svg:not(.icon), iframe, style').length).toBe(0);

// No extra <option> was smuggled in via the closing-tag breakout: the
// action select is ALL_ACTIONS + the empty placeholder + the one
Expand Down Expand Up @@ -485,7 +486,8 @@ describe('regression: group edit preserves constraints.accounts (#1629)', () =>
group.permissions = [{ action: 'view', resource: 'history', constraints: { accounts: [payload] } }];
await openEdit(group);

expect(document.querySelectorAll('img, script, svg, iframe, style').length).toBe(0);
// svg.icon is the Remove button's own trash icon (src/icons.ts), not injected markup.
expect(document.querySelectorAll('img, script, svg:not(.icon), iframe, style').length).toBe(0);
expect(accountsInput().value).toBe(payload);

await clickSave();
Expand Down
18 changes: 12 additions & 6 deletions frontend/src/__tests__/groups.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@
document.body.innerHTML = `
<div id="groups-list"></div>
`;
userState.setAllUsers(mockUsers as any);

Check warning on line 107 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type
groupState.setCurrentEditingGroup(null);
});

Expand Down Expand Up @@ -165,7 +165,7 @@
// Set up users so group-1 has exactly 1 member
userState.setAllUsers([
{ id: 'user-1', email: 'admin@test.com', groups: ['00000000-0000-5000-8000-000000000001', 'group-1'], mfa_enabled: true },
] as any);

Check warning on line 168 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type

const group = mockGroups[0];
if (!group) throw new Error('Test data missing');
Expand Down Expand Up @@ -242,12 +242,18 @@
groupList.renderGroups([xssGroup]);

const container = document.getElementById('groups-list');
// Verify script tag is escaped (not executable)
expect(container?.innerHTML).not.toContain('<script>');
expect(container?.innerHTML).toContain('&lt;script&gt;');
// Verify img tag is escaped (not executable)
expect(container?.innerHTML).not.toContain('<img');
expect(container?.innerHTML).toContain('&lt;img');
// The name and description must stay text: no element is created
// from them anywhere in the card.
expect(container?.querySelector('script, img')).toBeNull();
expect(container?.querySelector('h4')?.textContent).toBe(xssGroup.name);
expect(container?.querySelector('.group-description')?.textContent).toBe(xssGroup.description);
// The icon buttons carry the name in aria-label. The attribute must
// round-trip the raw string, so a quote cannot end it early. (innerHTML
// serialises attribute values without escaping < and >, which is why
// this checks the DOM rather than the serialised markup.)
const edit = container?.querySelector('.edit-group-btn');
expect(edit?.getAttribute('aria-label')).toBe(`Edit group ${xssGroup.name}`);
expect(edit?.getAttributeNames().sort()).toEqual(['aria-label', 'class', 'data-group-id', 'title']);
});

it('should not render if container not found', () => {
Expand Down Expand Up @@ -289,7 +295,7 @@
it('should attach click handlers to delete buttons', async () => {
(api.deleteGroup as jest.Mock).mockResolvedValue(undefined);
mockConfirmDialog.mockResolvedValue(true);
userState.setAvailableGroups(mockGroups as any);

Check warning on line 298 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type

groupList.renderGroups(mockGroups);

Expand Down Expand Up @@ -856,7 +862,7 @@
document.body.innerHTML = `
<div id="groups-list"></div>
`;
userState.setAvailableGroups(mockGroups as any);

Check warning on line 865 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type
jest.clearAllMocks();
mockConfirmDialog.mockResolvedValue(true);
(api.deleteGroup as jest.Mock).mockResolvedValue(undefined);
Expand Down Expand Up @@ -929,22 +935,22 @@
</form>
`;
// Clear window properties
delete (window as any).openCreateGroupModal;

Check warning on line 938 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type
delete (window as any).closeGroupModal;

Check warning on line 939 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type
delete (window as any).addPermission;

Check warning on line 940 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type
jest.clearAllMocks();
});

describe('setupGroupHandlers', () => {
it('should expose openCreateGroupModal on window', () => {
groupHandlers.setupGroupHandlers();
expect((window as any).openCreateGroupModal).toBeDefined();

Check warning on line 947 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type
expect(typeof (window as any).openCreateGroupModal).toBe('function');

Check warning on line 948 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type
});

it('should expose closeGroupModal on window', () => {
groupHandlers.setupGroupHandlers();
expect((window as any).closeGroupModal).toBeDefined();

Check warning on line 953 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type
expect(typeof (window as any).closeGroupModal).toBe('function');
});

Expand Down
77 changes: 77 additions & 0 deletions frontend/src/__tests__/icons.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/**
* Tests for the shared SVG icon set (issue #751).
*/
import fs from 'fs';
import path from 'path';
import { createIcon, iconSvg, type IconName } from '../icons';
import { showToast } from '../toast';

const ALL: IconName[] = [
'alert-triangle', 'check', 'check-circle', 'chevron-down', 'chevron-right',
'chevron-up', 'chevrons-up-down', 'copy', 'download', 'edit', 'filter', 'info',
'pause', 'play', 'rotate-cw', 'settings', 'slash', 'trash-2', 'x', 'x-circle',
];

function parse(markup: string): SVGSVGElement {
const host = document.createElement('div');
host.innerHTML = markup;
return host.firstElementChild as SVGSVGElement;
}

describe('icons', () => {
it.each(ALL)('%s renders as a decorative currentColor SVG', (name) => {
const svg = parse(iconSvg(name));
expect(svg.tagName.toLowerCase()).toBe('svg');
expect(svg.getAttribute('class')).toBe(`icon icon-${name}`);
expect(svg.getAttribute('viewBox')).toBe('0 0 24 24');
expect(svg.getAttribute('stroke')).toBe('currentColor');
expect(svg.getAttribute('aria-hidden')).toBe('true');
expect(svg.getAttribute('focusable')).toBe('false');
expect(svg.childElementCount).toBeGreaterThan(0);
});

it.each(ALL)('createIcon(%s) builds the same SVG as iconSvg', (name) => {
const built = createIcon(name);
expect(built.namespaceURI).toBe('http://www.w3.org/2000/svg');
expect(built.outerHTML).toBe(parse(iconSvg(name)).outerHTML);
});

it('toasts use an SVG icon and an SVG close button for every kind', () => {
for (const kind of ['success', 'info', 'warning', 'error'] as const) {
showToast({ message: kind, kind, timeout: null });
}
const toasts = Array.from(document.querySelectorAll('.toast'));
expect(toasts).toHaveLength(4);
for (const t of toasts) {
expect(t.querySelector('.toast-icon svg.icon')).not.toBeNull();
expect(t.querySelector('.toast-icon')?.textContent).toBe('');
expect(t.querySelector('.toast-close svg.icon-x')).not.toBeNull();
}
document.body.replaceChildren();
});

// Regression guard: the platform-font glyphs replaced by the icon set
// must not creep back into the rendered UI. Comments and tests are
// allowed to name them.
it('the replaced Unicode icon glyphs no longer appear in UI source', () => {
const banned = /[⏸⏵✎⛛ⓘ⚙↻]|&#9432;|&#x26DB;|\\u26db|\\u2195/i;
const srcDir = path.join(__dirname, '..');
const offenders: string[] = [];
const walk = (dir: string): void => {
for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
const full = path.join(dir, entry.name);
if (entry.isDirectory()) {
if (entry.name !== '__tests__') walk(full);
continue;
}
if (!/\.(ts|html)$/.test(entry.name) || entry.name === 'docs.html') continue;
fs.readFileSync(full, 'utf8').split('\n').forEach((line, i) => {
const code = line.replace(/\/\/.*$/, '').replace(/^\s*\*.*$/, '');
if (banned.test(code)) offenders.push(`${path.relative(srcDir, full)}:${i + 1}`);
});
}
};
walk(srcDir);
expect(offenders).toEqual([]);
});
});
4 changes: 4 additions & 0 deletions frontend/src/__tests__/plans-permissions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@ describe('Plans page permission gating (issue #365)', () => {
const buttons = Array.from(pp.querySelectorAll('td.actions button'));
expect(buttons.length).toBeGreaterThanOrEqual(4);
for (const b of buttons) expect(b.getAttribute('aria-label')).toBeTruthy();
// Issue #751: actions use the shared SVG icon set, not platform-font
// glyphs, and the money-triggering Run now action carries a text label.
for (const b of buttons) expect(b.querySelector('svg.icon')).not.toBeNull();
expect(pp.querySelector('button[data-action="run"]')?.textContent?.trim()).toBe('Run now');
});
});

Expand Down
164 changes: 164 additions & 0 deletions frontend/src/__tests__/row-action-icons.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
/**
* Icon-only row actions on the admin page (issue #751 review feedback).
*
* API key Revoke/Delete, the group editor's permission Remove, and the
* users/groups Edit, Duplicate and Delete actions used to be
* chunky text buttons. They now render an SVG from the shared icon set, so
* the accessible name has to come from aria-label. These tests pin the icon,
* the name a screen reader (and getByRole) sees, the tooltip, and that the
* click handlers still fire.
*/
import { getByRole, queryByRole } from '@testing-library/dom';
import { loadApiKeys } from '../apikeys';
import { addPermission } from '../groups/groupModals';
import { renderGroups } from '../groups/groupList';
import { renderUsers } from '../users/userList';
import * as userState from '../users/state';

jest.mock('../api', () => ({
getApiKeys: jest.fn(),
getApiKeysUsageStats: jest.fn(),
revokeApiKey: jest.fn(),
deleteApiKey: jest.fn(),
}));

const mockConfirmDialog = jest.fn<Promise<boolean>, [unknown]>(() => Promise.resolve(true));
jest.mock('../confirmDialog', () => ({
confirmDialog: (opts: unknown) => mockConfirmDialog(opts),
}));

import * as api from '../api';

function expectIconButton(btn: HTMLElement, icon: string, name: string): void {
expect(btn.classList.contains('icon-btn')).toBe(true);
expect(btn.querySelector(`svg.icon.icon-${icon}`)).not.toBeNull();
// No visible text: the name comes only from aria-label, the tooltip from title.
expect(btn.textContent?.trim()).toBe('');
expect(btn.getAttribute('aria-label')).toBe(name);
expect(btn.getAttribute('title')).toBe(name);
}

describe('API key row actions', () => {
beforeEach(() => {
document.body.innerHTML = '<div id="apikeys-usage-summary"></div><div id="apikeys-list"></div>';
(api.getApiKeysUsageStats as jest.Mock).mockResolvedValue({
total_active: 1, total_requests_window: 0, total_requests_lifetime: 0, top_keys: [],
});
(api.getApiKeys as jest.Mock).mockResolvedValue({
api_keys: [
{ id: 'k-active', name: 'CI', key_prefix: 'abc', is_active: true, created_at: '2026-01-01T00:00:00Z' },
{ id: 'k-revoked', name: 'Old', key_prefix: 'def', is_active: false, created_at: '2026-01-01T00:00:00Z' },
],
});
mockConfirmDialog.mockClear();
});

it('render as slash and trash icon buttons with accessible names', async () => {
await loadApiKeys();
const rows = document.querySelectorAll<HTMLElement>('#apikeys-list tbody tr');
expect(rows).toHaveLength(2);

const revoke = getByRole(rows[0]!, 'button', { name: 'Revoke API key' });
const del = getByRole(rows[0]!, 'button', { name: 'Delete API key' });
expectIconButton(revoke, 'slash', 'Revoke API key');
expectIconButton(del, 'trash-2', 'Delete API key');
// Warning/danger colour variants are kept.
expect(revoke.classList.contains('btn-warning')).toBe(true);
expect(del.classList.contains('btn-danger')).toBe(true);
// Side by side in one non-wrapping group.
expect(revoke.parentElement).toBe(del.parentElement);
expect(revoke.parentElement?.classList.contains('row-actions')).toBe(true);

// A revoked key offers only Delete.
expect(queryByRole(rows[1]!, 'button', { name: /Revoke/ })).toBeNull();
expect(getByRole(rows[1]!, 'button', { name: 'Delete API key' })).toBeTruthy();
});

it('keep their data attributes and confirm-then-act handlers', async () => {
(api.revokeApiKey as jest.Mock).mockResolvedValue({});
(api.deleteApiKey as jest.Mock).mockResolvedValue({});
await loadApiKeys();
const row = document.querySelector<HTMLElement>('#apikeys-list tbody tr')!;
const revoke = getByRole(row, 'button', { name: /Revoke/ });
expect(revoke.dataset['keyId']).toBe('k-active');

revoke.click();
await new Promise(resolve => setTimeout(resolve, 0));
expect(mockConfirmDialog).toHaveBeenCalledTimes(1);
expect(api.revokeApiKey).toHaveBeenCalledWith('k-active');

getByRole(row, 'button', { name: /Delete/ }).click();
await new Promise(resolve => setTimeout(resolve, 0));
expect(mockConfirmDialog).toHaveBeenCalledTimes(2);
expect(api.deleteApiKey).toHaveBeenCalledWith('k-active');
});
});

describe('group editor permission Remove', () => {
beforeEach(() => {
document.body.innerHTML = '<div id="permissions-list"></div>';
});

it('is a trash icon button named "Remove permission" that still removes its row', () => {
addPermission({ action: 'view', resource: 'plans' });
addPermission({ action: 'update', resource: 'purchases' });
const list = document.getElementById('permissions-list')!;
expect(list.querySelectorAll('.permission-item')).toHaveLength(2);

const first = list.querySelector<HTMLElement>('.permission-item')!;
const remove = getByRole(first, 'button', { name: 'Remove permission' });
expectIconButton(remove, 'trash-2', 'Remove permission');
expect(remove.classList.contains('btn-danger')).toBe(true);
expect(remove.classList.contains('remove-permission-btn')).toBe(true);

remove.click();
expect(list.querySelectorAll('.permission-item')).toHaveLength(1);
expect(list.querySelector<HTMLSelectElement>('.perm-action')?.value).toBe('update');
});
});

describe('users table and group card actions', () => {
afterEach(() => {
document.body.replaceChildren();
});

it('users Edit/Delete are pencil and trash icon buttons with per-user names', () => {
document.body.innerHTML = '<div id="users-list"></div>';
renderUsers([
{ id: 'u1', email: 'alice@example.com', groups: [], mfa_enabled: false },
] as unknown as Parameters<typeof renderUsers>[0]);
const row = document.querySelector<HTMLElement>('#users-list tbody tr')!;
const edit = getByRole(row, 'button', { name: 'Edit user alice@example.com' });
const del = getByRole(row, 'button', { name: 'Delete user alice@example.com' });
expect(edit.querySelector('svg.icon-edit')).not.toBeNull();
expect(del.querySelector('svg.icon-trash-2')).not.toBeNull();
expect(edit.textContent?.trim()).toBe('');
expect(del.textContent?.trim()).toBe('');
expect(edit.getAttribute('title')).toBe('Edit user');
expect(del.getAttribute('title')).toBe('Delete user');
expect(del.classList.contains('btn-danger')).toBe(true);
expect(edit.parentElement?.classList.contains('row-actions')).toBe(true);
});

it('group Edit/Duplicate/Delete are icon buttons named after the group', () => {
document.body.innerHTML = '<div id="groups-list"></div>';
userState.setAllUsers([]);
renderGroups([
{ id: 'g1', name: 'Ops', description: '', permissions: [], allowed_accounts: [] },
] as unknown as Parameters<typeof renderGroups>[0]);
const card = document.querySelector<HTMLElement>('#groups-list .group-card')!;
const cases: Array<[string, string, string, string]> = [
['Edit group Ops', 'edit', 'Edit group', 'edit-group-btn'],
['Duplicate group Ops', 'copy', 'Duplicate group', 'duplicate-group-btn'],
['Delete group Ops', 'trash-2', 'Delete group', 'delete-group-btn'],
];
for (const [name, icon, title, cls] of cases) {
const btn = getByRole(card, 'button', { name });
expect(btn.querySelector(`svg.icon-${icon}`)).not.toBeNull();
expect(btn.textContent?.trim()).toBe('');
expect(btn.getAttribute('title')).toBe(title);
expect(btn.classList.contains(cls)).toBe(true);
expect(btn.dataset['groupId']).toBe('g1');
}
});
});
7 changes: 5 additions & 2 deletions frontend/src/apikeys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { escapeHtml, formatDateTime, formatRelativeTime } from './utils';
import { confirmDialog } from './confirmDialog';
import { friendlyError } from './api/errors';
import { showToast } from './toast';
import { iconSvg } from './icons';
import { openModal, closeModal } from './modal';
import { showSkeletonRows, showSkeletonText, teardownSkeleton } from './lib/skeleton';
import { parseLocalDateInputValue, toLocalDateInputValue } from './lib/local-date';
Expand Down Expand Up @@ -146,8 +147,10 @@ export function renderApiKeysList(): void {
<td class="apikeys-count-cell">${countTotal}</td>
<td>${key.expires_at ? formatDateTime(key.expires_at) : '<span class="text-muted">Never</span>'}</td>
<td>
${key.is_active && !isExpired ? `<button class="btn-small btn-warning revoke-key-btn" data-key-id="${escapeHtml(key.id)}">Revoke</button>` : ''}
<button class="btn-small btn-danger delete-key-btn" data-key-id="${escapeHtml(key.id)}">Delete</button>
<div class="row-actions">
${key.is_active && !isExpired ? `<button type="button" class="btn-small btn-warning icon-btn revoke-key-btn" data-key-id="${escapeHtml(key.id)}" aria-label="Revoke API key" title="Revoke API key">${iconSvg('slash')}</button>` : ''}
<button type="button" class="btn-small btn-danger icon-btn delete-key-btn" data-key-id="${escapeHtml(key.id)}" aria-label="Delete API key" title="Delete API key">${iconSvg('trash-2')}</button>
</div>
</td>
</tr>
`;
Expand Down
3 changes: 2 additions & 1 deletion frontend/src/confirmDialog.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/

import { FOCUSABLE_SELECTOR } from './modal';
import { createIcon } from './icons';

let dialogSeq = 0;

Expand Down Expand Up @@ -61,7 +62,7 @@ export function confirmDialog(opts: ConfirmDialogOptions): Promise<boolean> {
closeBtn.type = 'button';
closeBtn.className = 'modal-confirm-close';
closeBtn.setAttribute('aria-label', 'Close');
closeBtn.textContent = '×';
closeBtn.appendChild(createIcon('x'));
dialog.appendChild(closeBtn);

const titleEl = document.createElement('h3');
Expand Down
7 changes: 4 additions & 3 deletions frontend/src/federation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import { getFederationIaC } from './api';
import { showToast } from './toast';
import { createIcon } from './icons';

// ---------------------------------------------------------------------------
// Download helper
Expand Down Expand Up @@ -102,12 +103,12 @@ function makeFormatButton(opt: FormatOption, target: string, source: string): HT
// .btn-download class together signal the click action).
btn.setAttribute('aria-label', `Download ${opt.label} (${opt.sublabel}) for ${target.toUpperCase()}`);

// A leading ↓ glyph communicates "download" visually. aria-hidden so
// screen readers rely on the aria-label above, not the glyph.
// A leading download icon communicates "download" visually. aria-hidden
// so screen readers rely on the aria-label above, not the icon.
const iconSpan = document.createElement('span');
iconSpan.className = 'download-icon';
iconSpan.setAttribute('aria-hidden', 'true');
iconSpan.textContent = '↓';
iconSpan.appendChild(createIcon('download'));
btn.appendChild(iconSpan);

const labelSpan = document.createElement('span');
Expand Down
13 changes: 8 additions & 5 deletions frontend/src/groups/groupList.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { allUsers } from '../users/state';
import { escapeHtml } from '../users/utils';
import { openEditGroupModal, deleteGroup } from './groupActions';
import { openDuplicateGroupModal } from './groupModals';
import { iconSvg } from '../icons';
import { isSystemManagedGroup, SYSTEM_GROUP_NOTE } from './systemGroups';

/**
Expand Down Expand Up @@ -64,11 +65,13 @@ export function renderGroups(groups: APIGroup[]): void {
</div>
<div class="group-card-actions">
<span class="badge">${memberCount} member${memberCount !== 1 ? 's' : ''}</span>
${locked
? `<span class="badge badge-muted system-group-badge" title="${escapeHtml(SYSTEM_GROUP_NOTE)}">System group<span class="sr-only">. ${escapeHtml(SYSTEM_GROUP_NOTE)}</span></span>`
: `<button class="btn-small edit-group-btn" data-group-id="${escapeHtml(group.id)}">Edit</button>`}
<button class="btn-small duplicate-group-btn" data-group-id="${escapeHtml(group.id)}" title="Duplicate">Duplicate</button>
${locked ? '' : `<button class="btn-small btn-danger delete-group-btn" data-group-id="${escapeHtml(group.id)}">Delete</button>`}
<span class="row-actions">
${locked
? `<span class="badge badge-muted system-group-badge" title="${escapeHtml(SYSTEM_GROUP_NOTE)}">System group<span class="sr-only">. ${escapeHtml(SYSTEM_GROUP_NOTE)}</span></span>`
: `<button class="btn-small icon-btn edit-group-btn" data-group-id="${escapeHtml(group.id)}" aria-label="Edit group ${escapeHtml(group.name)}" title="Edit group">${iconSvg('edit')}</button>`}
<button class="btn-small icon-btn duplicate-group-btn" data-group-id="${escapeHtml(group.id)}" aria-label="Duplicate group ${escapeHtml(group.name)}" title="Duplicate group">${iconSvg('copy')}</button>
${locked ? '' : `<button class="btn-small btn-danger icon-btn delete-group-btn" data-group-id="${escapeHtml(group.id)}" aria-label="Delete group ${escapeHtml(group.name)}" title="Delete group">${iconSvg('trash-2')}</button>`}
</span>
</div>
</div>
<div class="group-card-body">
Expand Down
Loading
Loading