Skip to content

Commit 9fcddc5

Browse files
sadpandajoeclaude
andcommitted
perf(table-chart): optimize header ID sanitization and add unit tests
**Performance improvement:** - Cache sanitized header ID value to avoid recomputing it twice per column - Compute once at column definition, use for both header `id` and cell `aria-labelledby` **Test coverage:** - Added 8 unit tests for `sanitizeHeaderId()` function - Tests verify all sanitization rules: %, #, △, spaces, and other special chars - Provides faster diagnostics if sanitization logic changes in future **Changes:** - Export `sanitizeHeaderId()` for testing - Cache sanitized value in `headerId` variable - Added comprehensive unit test suite Tests: 40 passed (8 new unit tests + 32 existing) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 790fe5c commit 9fcddc5

2 files changed

Lines changed: 46 additions & 4 deletions

File tree

superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,9 @@ function cellWidth({
144144
/**
145145
* Sanitize a column identifier for use in HTML id attributes and CSS selectors.
146146
* Replaces characters that are invalid in CSS selectors with safe alternatives.
147+
* Exported for testing.
147148
*/
148-
function sanitizeHeaderId(columnId: string): string {
149+
export function sanitizeHeaderId(columnId: string): string {
149150
return columnId
150151
.replace(/%/g, 'percent')
151152
.replace(/#/g, 'hash')
@@ -857,6 +858,9 @@ export default function TableChart<D extends DataRecord = DataRecord>(
857858
}
858859
}
859860

861+
// Cache sanitized header ID to avoid recomputing it multiple times
862+
const headerId = sanitizeHeaderId(column.originalLabel || column.key);
863+
860864
return {
861865
id: String(i), // to allow duplicate column keys
862866
// must use custom accessor to allow `.` in column names
@@ -982,7 +986,7 @@ export default function TableChart<D extends DataRecord = DataRecord>(
982986
}
983987

984988
const cellProps = {
985-
'aria-labelledby': `header-${sanitizeHeaderId(column.originalLabel || column.key)}`,
989+
'aria-labelledby': `header-${headerId}`,
986990
role: 'cell',
987991
// show raw number in title in case of numeric values
988992
title: typeof value === 'number' ? String(value) : undefined,
@@ -1069,7 +1073,7 @@ export default function TableChart<D extends DataRecord = DataRecord>(
10691073
},
10701074
Header: ({ column: col, onClick, style, onDragStart, onDrop }) => (
10711075
<th
1072-
id={`header-${sanitizeHeaderId(column.originalLabel || column.key)}`}
1076+
id={`header-${headerId}`}
10731077
title={t('Shift + Click to sort by multiple columns')}
10741078
className={[className, col.isSorted ? 'is-sorted' : ''].join(' ')}
10751079
style={{

superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,50 @@
1919
import '@testing-library/jest-dom';
2020
import { render, screen } from '@superset-ui/core/spec';
2121
import { cloneDeep } from 'lodash';
22-
import TableChart from '../src/TableChart';
22+
import TableChart, { sanitizeHeaderId } from '../src/TableChart';
2323
import transformProps from '../src/transformProps';
2424
import DateWithFormatter from '../src/utils/DateWithFormatter';
2525
import testData from './testData';
2626
import { ProviderWrapper } from './testHelpers';
2727

28+
describe('sanitizeHeaderId', () => {
29+
test('should sanitize percent sign', () => {
30+
expect(sanitizeHeaderId('%pct_nice')).toBe('percentpct_nice');
31+
});
32+
33+
test('should sanitize hash/pound sign', () => {
34+
expect(sanitizeHeaderId('# metric_1')).toBe('hash_metric_1');
35+
});
36+
37+
test('should sanitize delta symbol', () => {
38+
expect(sanitizeHeaderId('△ delta')).toBe('delta_delta');
39+
});
40+
41+
test('should replace spaces with underscores', () => {
42+
expect(sanitizeHeaderId('Main metric_1')).toBe('Main_metric_1');
43+
expect(sanitizeHeaderId('multiple spaces')).toBe('multiple_spaces');
44+
});
45+
46+
test('should handle multiple special characters', () => {
47+
expect(sanitizeHeaderId('% #△ test')).toBe('percent_hashdelta_test');
48+
expect(sanitizeHeaderId('% # △ test')).toBe('percent_hash_delta_test');
49+
});
50+
51+
test('should preserve alphanumeric, underscore, and hyphen', () => {
52+
expect(sanitizeHeaderId('valid-name_123')).toBe('valid-name_123');
53+
});
54+
55+
test('should replace other special characters with underscore', () => {
56+
expect(sanitizeHeaderId('col@name!test')).toBe('col_name_test');
57+
expect(sanitizeHeaderId('test.column')).toBe('test_column');
58+
});
59+
60+
test('should handle edge cases', () => {
61+
expect(sanitizeHeaderId('')).toBe('');
62+
expect(sanitizeHeaderId('simple')).toBe('simple');
63+
});
64+
});
65+
2866
describe('plugin-chart-table', () => {
2967
describe('transformProps', () => {
3068
it('should parse pageLength to pageSize', () => {

0 commit comments

Comments
 (0)