From 790fe5c060fc33411ff08b6c536df832a5476f75 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Mon, 3 Nov 2025 16:35:44 -0800 Subject: [PATCH 1/5] fix(table-chart): fix table header IDs and accessibility (#35783) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Table chart column headers were rendering with `id="header-undefined"` instead of meaningful IDs, breaking CSS customization and ARIA accessibility. **Root Cause:** PR #31590 (Ant Design v5) changed header IDs from `column.key` to `column.originalLabel`, but `originalLabel` is only set for time-comparison columns (added in PR #32665), leaving regular columns with undefined values. **Solution:** Implemented defensive fallback with sanitization: - Headers: `id="header-${sanitizeHeaderId(column.originalLabel || column.key)}"` - Cells: `aria-labelledby="header-${sanitizeHeaderId(column.originalLabel || column.key)}"` **Sanitization Required:** Column identifiers can contain CSS-unsafe characters (%, #, △, spaces), so added sanitization function to ensure valid CSS selectors and ARIA references. **Changes:** - Added `sanitizeHeaderId()` helper function - Updated header `id` attribute with fallback + sanitization - Updated cell `aria-labelledby` with fallback + sanitization - Added 2 comprehensive tests (regular + time-comparison columns) - Fixed test fixture mutation issue **Tests verify:** - No "undefined" in any header IDs - All IDs are CSS-safe (no spaces or special characters) - All ARIA relationships valid (cells reference existing headers) - Works for both regular and time-comparison columns Fixes #35783 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../plugin-chart-table/src/TableChart.tsx | 17 ++- .../test/TableChart.test.tsx | 143 +++++++++++++++++- 2 files changed, 153 insertions(+), 7 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx index b182ad03bf9d..23b1cd4fc41a 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx +++ b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx @@ -141,6 +141,19 @@ function cellWidth({ return perc2; } +/** + * Sanitize a column identifier for use in HTML id attributes and CSS selectors. + * Replaces characters that are invalid in CSS selectors with safe alternatives. + */ +function sanitizeHeaderId(columnId: string): string { + return columnId + .replace(/%/g, 'percent') + .replace(/#/g, 'hash') + .replace(/△/g, 'delta') + .replace(/\s+/g, '_') + .replace(/[^a-zA-Z0-9_-]/g, '_'); +} + /** * Cell left margin (offset) calculation for horizontal bar chart elements * when alignPositiveNegative is not set @@ -969,7 +982,7 @@ export default function TableChart( } const cellProps = { - 'aria-labelledby': `header-${column.key}`, + 'aria-labelledby': `header-${sanitizeHeaderId(column.originalLabel || column.key)}`, role: 'cell', // show raw number in title in case of numeric values title: typeof value === 'number' ? String(value) : undefined, @@ -1056,7 +1069,7 @@ export default function TableChart( }, Header: ({ column: col, onClick, style, onDragStart, onDrop }) => ( { it('render advanced data', () => { render( - <> - - , - , + , ); const secondColumnHeader = screen.getByText('Sum of Num'); expect(secondColumnHeader).toBeInTheDocument(); @@ -467,7 +465,7 @@ describe('plugin-chart-table', () => { }); it('render cell without color', () => { - const dataWithEmptyCell = testData.advanced.queriesData[0]; + const dataWithEmptyCell = cloneDeep(testData.advanced.queriesData[0]); dataWithEmptyCell.data.push({ __timestamp: null, name: 'Noah', @@ -522,6 +520,141 @@ describe('plugin-chart-table', () => { expect(hasMetricHeaders).toBe(true); }); + it('should set meaningful header IDs for time-comparison columns', () => { + // Test time-comparison columns have proper IDs + // Uses originalLabel (e.g., "metric_1") which is sanitized for CSS safety + const props = transformProps(testData.comparison); + + const { container } = render(); + + const headers = screen.getAllByRole('columnheader'); + + // All headers should have IDs + const headersWithIds = headers.filter(header => header.id); + expect(headersWithIds.length).toBeGreaterThan(0); + + // None should have "header-undefined" + const undefinedHeaders = headersWithIds.filter(header => + header.id.includes('undefined'), + ); + expect(undefinedHeaders).toHaveLength(0); + + // Should have IDs based on sanitized originalLabel (e.g., "metric_1") + const hasMetricHeaders = headersWithIds.some( + header => + header.id.includes('metric_1') || header.id.includes('metric_2'), + ); + expect(hasMetricHeaders).toBe(true); + + // CRITICAL: Verify sanitization - no spaces or special chars in any header ID + headersWithIds.forEach(header => { + // IDs must not contain spaces (would break CSS selectors and ARIA) + expect(header.id).not.toMatch(/\s/); + // IDs must not contain special chars like %, #, △ + expect(header.id).not.toMatch(/[%#△]/); + // IDs should only contain valid characters: alphanumeric, underscore, hyphen + expect(header.id).toMatch(/^header-[a-zA-Z0-9_-]+$/); + }); + + // CRITICAL: Verify ALL cells reference valid headers (no broken ARIA) + const cellsWithLabels = container.querySelectorAll( + 'td[aria-labelledby]', + ); + cellsWithLabels.forEach(cell => { + const labelledBy = cell.getAttribute('aria-labelledby'); + if (labelledBy) { + // Check that the ID doesn't contain spaces (would be interpreted as multiple IDs) + expect(labelledBy).not.toMatch(/\s/); + // Check that the ID doesn't contain special characters + expect(labelledBy).not.toMatch(/[%#△]/); + // Verify the referenced header actually exists + const referencedHeader = container.querySelector( + `#${CSS.escape(labelledBy)}`, + ); + expect(referencedHeader).toBeTruthy(); + } + }); + }); + + it('should set meaningful header IDs for regular table columns', () => { + // Test regular (non-time-comparison) columns have proper IDs + // Uses fallback to column.key since originalLabel is undefined + const props = transformProps(testData.advanced); + + const { container } = render( + ProviderWrapper({ + children: , + }), + ); + + const headers = screen.getAllByRole('columnheader'); + + // Test 1: "name" column (regular string column) + const nameHeader = headers.find(header => + header.textContent?.includes('name'), + ); + expect(nameHeader).toBeDefined(); + expect(nameHeader?.id).toBe('header-name'); // Falls back to column.key + + // Verify cells reference this header correctly + const nameCells = container.querySelectorAll( + 'td[aria-labelledby="header-name"]', + ); + expect(nameCells.length).toBeGreaterThan(0); + + // Test 2: "sum__num" column (metric with verbose map "Sum of Num") + const sumHeader = headers.find(header => + header.textContent?.includes('Sum of Num'), + ); + expect(sumHeader).toBeDefined(); + expect(sumHeader?.id).toBe('header-sum__num'); // Falls back to column.key, not verbose label + + // Verify cells reference this header correctly + const sumCells = container.querySelectorAll( + 'td[aria-labelledby="header-sum__num"]', + ); + expect(sumCells.length).toBeGreaterThan(0); + + // Test 3: Verify NO headers have "undefined" in their ID + const undefinedHeaders = headers.filter(header => + header.id?.includes('undefined'), + ); + expect(undefinedHeaders).toHaveLength(0); + + // Test 4: Verify ALL headers have proper IDs (no missing IDs) + const headersWithIds = headers.filter(header => header.id); + expect(headersWithIds.length).toBe(headers.length); + + // Test 5: Verify ALL header IDs are properly sanitized + headersWithIds.forEach(header => { + // IDs must not contain spaces + expect(header.id).not.toMatch(/\s/); + // IDs must not contain special chars like % (from %pct_nice column) + expect(header.id).not.toMatch(/[%#△]/); + // IDs should only contain valid CSS selector characters + expect(header.id).toMatch(/^header-[a-zA-Z0-9_-]+$/); + }); + + // Test 6: Verify ALL cells reference valid headers (no broken ARIA) + const cellsWithLabels = container.querySelectorAll( + 'td[aria-labelledby]', + ); + cellsWithLabels.forEach(cell => { + const labelledBy = cell.getAttribute('aria-labelledby'); + if (labelledBy) { + // Verify no spaces (would be interpreted as multiple IDs) + expect(labelledBy).not.toMatch(/\s/); + // Verify no special characters + expect(labelledBy).not.toMatch(/[%#△]/); + // Verify the referenced header actually exists + const referencedHeader = container.querySelector( + `#${CSS.escape(labelledBy)}`, + ); + expect(referencedHeader).toBeTruthy(); + } + }); + }); + it('render cell bars properly, and only when it is toggled on in both regular and percent metrics', () => { const props = transformProps({ ...testData.raw, From 9fcddc545c4c1be782d16b60ebeb782d7b368d5d Mon Sep 17 00:00:00 2001 From: Joe Li Date: Mon, 3 Nov 2025 16:55:16 -0800 Subject: [PATCH 2/5] perf(table-chart): optimize header ID sanitization and add unit tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **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 --- .../plugin-chart-table/src/TableChart.tsx | 10 +++-- .../test/TableChart.test.tsx | 40 ++++++++++++++++++- 2 files changed, 46 insertions(+), 4 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx index 23b1cd4fc41a..7bd7a46d39e8 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx +++ b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx @@ -144,8 +144,9 @@ function cellWidth({ /** * Sanitize a column identifier for use in HTML id attributes and CSS selectors. * Replaces characters that are invalid in CSS selectors with safe alternatives. + * Exported for testing. */ -function sanitizeHeaderId(columnId: string): string { +export function sanitizeHeaderId(columnId: string): string { return columnId .replace(/%/g, 'percent') .replace(/#/g, 'hash') @@ -857,6 +858,9 @@ export default function TableChart( } } + // Cache sanitized header ID to avoid recomputing it multiple times + const headerId = sanitizeHeaderId(column.originalLabel || column.key); + return { id: String(i), // to allow duplicate column keys // must use custom accessor to allow `.` in column names @@ -982,7 +986,7 @@ export default function TableChart( } const cellProps = { - 'aria-labelledby': `header-${sanitizeHeaderId(column.originalLabel || column.key)}`, + 'aria-labelledby': `header-${headerId}`, role: 'cell', // show raw number in title in case of numeric values title: typeof value === 'number' ? String(value) : undefined, @@ -1069,7 +1073,7 @@ export default function TableChart( }, Header: ({ column: col, onClick, style, onDragStart, onDrop }) => ( { + test('should sanitize percent sign', () => { + expect(sanitizeHeaderId('%pct_nice')).toBe('percentpct_nice'); + }); + + test('should sanitize hash/pound sign', () => { + expect(sanitizeHeaderId('# metric_1')).toBe('hash_metric_1'); + }); + + test('should sanitize delta symbol', () => { + expect(sanitizeHeaderId('△ delta')).toBe('delta_delta'); + }); + + test('should replace spaces with underscores', () => { + expect(sanitizeHeaderId('Main metric_1')).toBe('Main_metric_1'); + expect(sanitizeHeaderId('multiple spaces')).toBe('multiple_spaces'); + }); + + test('should handle multiple special characters', () => { + expect(sanitizeHeaderId('% #△ test')).toBe('percent_hashdelta_test'); + expect(sanitizeHeaderId('% # △ test')).toBe('percent_hash_delta_test'); + }); + + test('should preserve alphanumeric, underscore, and hyphen', () => { + expect(sanitizeHeaderId('valid-name_123')).toBe('valid-name_123'); + }); + + test('should replace other special characters with underscore', () => { + expect(sanitizeHeaderId('col@name!test')).toBe('col_name_test'); + expect(sanitizeHeaderId('test.column')).toBe('test_column'); + }); + + test('should handle edge cases', () => { + expect(sanitizeHeaderId('')).toBe(''); + expect(sanitizeHeaderId('simple')).toBe('simple'); + }); +}); + describe('plugin-chart-table', () => { describe('transformProps', () => { it('should parse pageLength to pageSize', () => { From f68061103b759e8ad3142939289fa276c8dffd4c Mon Sep 17 00:00:00 2001 From: Joe Li Date: Wed, 5 Nov 2025 18:04:06 -0800 Subject: [PATCH 3/5] refactor(table-chart): improve header ID sanitization based on review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses code review feedback from PR #35968: - Collapse consecutive underscores in sanitized IDs for better readability - Use nullish coalescing (??) instead of OR (||) for more precise fallback logic - Add JSDoc documentation clarifying the need for ID prefixes - Add comprehensive unit tests for consecutive underscore handling These improvements enhance code quality and maintainability while preserving the existing functionality and passing all existing tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../plugin-chart-table/src/TableChart.tsx | 9 ++- .../test/TableChart.test.tsx | 67 ++++++++++--------- 2 files changed, 43 insertions(+), 33 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx index 7bd7a46d39e8..06b490516afb 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx +++ b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx @@ -144,6 +144,10 @@ function cellWidth({ /** * Sanitize a column identifier for use in HTML id attributes and CSS selectors. * Replaces characters that are invalid in CSS selectors with safe alternatives. + * + * Note: The returned value should be prefixed with a string (e.g., "header-") + * to ensure it forms a valid HTML ID (IDs cannot start with a digit). + * * Exported for testing. */ export function sanitizeHeaderId(columnId: string): string { @@ -152,7 +156,8 @@ export function sanitizeHeaderId(columnId: string): string { .replace(/#/g, 'hash') .replace(/△/g, 'delta') .replace(/\s+/g, '_') - .replace(/[^a-zA-Z0-9_-]/g, '_'); + .replace(/[^a-zA-Z0-9_-]/g, '_') + .replace(/_+/g, '_'); // Collapse consecutive underscores } /** @@ -859,7 +864,7 @@ export default function TableChart( } // Cache sanitized header ID to avoid recomputing it multiple times - const headerId = sanitizeHeaderId(column.originalLabel || column.key); + const headerId = sanitizeHeaderId(column.originalLabel ?? column.key); return { id: String(i), // to allow duplicate column keys diff --git a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx index eb175a954d7f..7e10e498d2ee 100644 --- a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx +++ b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx @@ -25,42 +25,47 @@ import DateWithFormatter from '../src/utils/DateWithFormatter'; import testData from './testData'; import { ProviderWrapper } from './testHelpers'; -describe('sanitizeHeaderId', () => { - test('should sanitize percent sign', () => { - expect(sanitizeHeaderId('%pct_nice')).toBe('percentpct_nice'); - }); +test('sanitizeHeaderId should sanitize percent sign', () => { + expect(sanitizeHeaderId('%pct_nice')).toBe('percentpct_nice'); +}); - test('should sanitize hash/pound sign', () => { - expect(sanitizeHeaderId('# metric_1')).toBe('hash_metric_1'); - }); +test('sanitizeHeaderId should sanitize hash/pound sign', () => { + expect(sanitizeHeaderId('# metric_1')).toBe('hash_metric_1'); +}); - test('should sanitize delta symbol', () => { - expect(sanitizeHeaderId('△ delta')).toBe('delta_delta'); - }); +test('sanitizeHeaderId should sanitize delta symbol', () => { + expect(sanitizeHeaderId('△ delta')).toBe('delta_delta'); +}); - test('should replace spaces with underscores', () => { - expect(sanitizeHeaderId('Main metric_1')).toBe('Main_metric_1'); - expect(sanitizeHeaderId('multiple spaces')).toBe('multiple_spaces'); - }); +test('sanitizeHeaderId should replace spaces with underscores', () => { + expect(sanitizeHeaderId('Main metric_1')).toBe('Main_metric_1'); + expect(sanitizeHeaderId('multiple spaces')).toBe('multiple_spaces'); +}); - test('should handle multiple special characters', () => { - expect(sanitizeHeaderId('% #△ test')).toBe('percent_hashdelta_test'); - expect(sanitizeHeaderId('% # △ test')).toBe('percent_hash_delta_test'); - }); +test('sanitizeHeaderId should handle multiple special characters', () => { + expect(sanitizeHeaderId('% #△ test')).toBe('percent_hashdelta_test'); + expect(sanitizeHeaderId('% # △ test')).toBe('percent_hash_delta_test'); +}); - test('should preserve alphanumeric, underscore, and hyphen', () => { - expect(sanitizeHeaderId('valid-name_123')).toBe('valid-name_123'); - }); +test('sanitizeHeaderId should preserve alphanumeric, underscore, and hyphen', () => { + expect(sanitizeHeaderId('valid-name_123')).toBe('valid-name_123'); +}); - test('should replace other special characters with underscore', () => { - expect(sanitizeHeaderId('col@name!test')).toBe('col_name_test'); - expect(sanitizeHeaderId('test.column')).toBe('test_column'); - }); +test('sanitizeHeaderId should replace other special characters with underscore', () => { + expect(sanitizeHeaderId('col@name!test')).toBe('col_name_test'); + expect(sanitizeHeaderId('test.column')).toBe('test_column'); +}); - test('should handle edge cases', () => { - expect(sanitizeHeaderId('')).toBe(''); - expect(sanitizeHeaderId('simple')).toBe('simple'); - }); +test('sanitizeHeaderId should handle edge cases', () => { + expect(sanitizeHeaderId('')).toBe(''); + expect(sanitizeHeaderId('simple')).toBe('simple'); +}); + +test('sanitizeHeaderId should collapse consecutive underscores', () => { + expect(sanitizeHeaderId('test @@ space')).toBe('test_space'); + expect(sanitizeHeaderId('col___name')).toBe('col_name'); + expect(sanitizeHeaderId('a b c')).toBe('a_b_c'); + expect(sanitizeHeaderId('test@@name')).toBe('test_name'); }); describe('plugin-chart-table', () => { @@ -645,11 +650,11 @@ describe('plugin-chart-table', () => { header.textContent?.includes('Sum of Num'), ); expect(sumHeader).toBeDefined(); - expect(sumHeader?.id).toBe('header-sum__num'); // Falls back to column.key, not verbose label + expect(sumHeader?.id).toBe('header-sum_num'); // Falls back to column.key, consecutive underscores collapsed // Verify cells reference this header correctly const sumCells = container.querySelectorAll( - 'td[aria-labelledby="header-sum__num"]', + 'td[aria-labelledby="header-sum_num"]', ); expect(sumCells.length).toBeGreaterThan(0); From 5407932cda149197898b763dd5176d9607c290e2 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Thu, 6 Nov 2025 12:12:27 -0800 Subject: [PATCH 4/5] refactor(table-chart): add comments and edge case handling for sanitizeHeaderId MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses additional code review feedback from PR #35968: - Add explanatory comments documenting why semantic replacements come first (preserves meaning in IDs: 'percentpct_nice' vs '_pct_nice') - Add leading/trailing underscore trimming to handle edge cases (e.g., '@col' → 'col' instead of '_col') - Add comprehensive edge case tests (4 new test blocks, 19 additional assertions) covering leading/trailing underscores and special-character-only inputs Tests: 45 total (13 unit + 2 integration + 30 existing) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../plugin-chart-table/src/TableChart.tsx | 20 +++++++---- .../test/TableChart.test.tsx | 34 +++++++++++++++++++ 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx index 06b490516afb..d92dd3bff36a 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx +++ b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx @@ -151,13 +151,19 @@ function cellWidth({ * Exported for testing. */ export function sanitizeHeaderId(columnId: string): string { - return columnId - .replace(/%/g, 'percent') - .replace(/#/g, 'hash') - .replace(/△/g, 'delta') - .replace(/\s+/g, '_') - .replace(/[^a-zA-Z0-9_-]/g, '_') - .replace(/_+/g, '_'); // Collapse consecutive underscores + return ( + columnId + // Semantic replacements first: preserve meaning in IDs for readability + // (e.g., 'percentpct_nice' instead of '_pct_nice') + .replace(/%/g, 'percent') + .replace(/#/g, 'hash') + .replace(/△/g, 'delta') + // Generic sanitization for remaining special characters + .replace(/\s+/g, '_') + .replace(/[^a-zA-Z0-9_-]/g, '_') + .replace(/_+/g, '_') // Collapse consecutive underscores + .replace(/^_+|_+$/g, '') // Trim leading/trailing underscores + ); } /** diff --git a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx index 7e10e498d2ee..6a8b687ddfed 100644 --- a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx +++ b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx @@ -68,6 +68,40 @@ test('sanitizeHeaderId should collapse consecutive underscores', () => { expect(sanitizeHeaderId('test@@name')).toBe('test_name'); }); +test('sanitizeHeaderId should remove leading underscores', () => { + expect(sanitizeHeaderId('@col')).toBe('col'); + expect(sanitizeHeaderId('!revenue')).toBe('revenue'); + expect(sanitizeHeaderId('@@test')).toBe('test'); + expect(sanitizeHeaderId(' leading_spaces')).toBe('leading_spaces'); +}); + +test('sanitizeHeaderId should remove trailing underscores', () => { + expect(sanitizeHeaderId('col@')).toBe('col'); + expect(sanitizeHeaderId('revenue!')).toBe('revenue'); + expect(sanitizeHeaderId('test@@')).toBe('test'); + expect(sanitizeHeaderId('trailing_spaces ')).toBe('trailing_spaces'); +}); + +test('sanitizeHeaderId should remove leading and trailing underscores', () => { + expect(sanitizeHeaderId('@col@')).toBe('col'); + expect(sanitizeHeaderId('!test!')).toBe('test'); + expect(sanitizeHeaderId(' spaced ')).toBe('spaced'); + expect(sanitizeHeaderId('@@multiple@@')).toBe('multiple'); +}); + +test('sanitizeHeaderId should handle inputs with only special characters', () => { + expect(sanitizeHeaderId('@')).toBe(''); + expect(sanitizeHeaderId('@@')).toBe(''); + expect(sanitizeHeaderId(' ')).toBe(''); + expect(sanitizeHeaderId('!@$')).toBe(''); + expect(sanitizeHeaderId('!@#$')).toBe('hash'); // # → 'hash' is preserved (semantic replacement) + // Semantic replacements should be preserved even when alone + expect(sanitizeHeaderId('%')).toBe('percent'); + expect(sanitizeHeaderId('#')).toBe('hash'); + expect(sanitizeHeaderId('△')).toBe('delta'); + expect(sanitizeHeaderId('% # △')).toBe('percent_hash_delta'); +}); + describe('plugin-chart-table', () => { describe('transformProps', () => { it('should parse pageLength to pageSize', () => { From ebfb2a990d2148d187d2f401ea221f79580b88bf Mon Sep 17 00:00:00 2001 From: Joe Li Date: Thu, 6 Nov 2025 12:43:58 -0800 Subject: [PATCH 5/5] docs(table-chart): improve comments and test consistency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses additional code review feedback: - Fix misleading comment in sanitizeHeaderId: clarify transformation example ('percentpct_nice' → '%pct_nice' → 'percentpct_nice' for accuracy) - Fix misleading use of "preserved" in test comments (characters are REPLACED with words, not preserved) - Replace all it() with test() for consistency with codebase standards (30 instances updated to follow CLAUDE.md guidelines) No functional changes - pure documentation and style improvements. Tests: 45 passed (13 unit + 2 integration + 30 existing) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../plugin-chart-table/src/TableChart.tsx | 2 +- .../test/TableChart.test.tsx | 68 +++++++++---------- 2 files changed, 35 insertions(+), 35 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx index d92dd3bff36a..ee575b6fb825 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx +++ b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx @@ -154,7 +154,7 @@ export function sanitizeHeaderId(columnId: string): string { return ( columnId // Semantic replacements first: preserve meaning in IDs for readability - // (e.g., 'percentpct_nice' instead of '_pct_nice') + // (e.g., '%pct_nice' → 'percentpct_nice' instead of '_pct_nice') .replace(/%/g, 'percent') .replace(/#/g, 'hash') .replace(/△/g, 'delta') diff --git a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx index 6a8b687ddfed..83de3752be37 100644 --- a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx +++ b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx @@ -94,8 +94,8 @@ test('sanitizeHeaderId should handle inputs with only special characters', () => expect(sanitizeHeaderId('@@')).toBe(''); expect(sanitizeHeaderId(' ')).toBe(''); expect(sanitizeHeaderId('!@$')).toBe(''); - expect(sanitizeHeaderId('!@#$')).toBe('hash'); // # → 'hash' is preserved (semantic replacement) - // Semantic replacements should be preserved even when alone + expect(sanitizeHeaderId('!@#$')).toBe('hash'); // # is replaced with 'hash' (semantic replacement) + // Semantic replacements produce readable output even when alone expect(sanitizeHeaderId('%')).toBe('percent'); expect(sanitizeHeaderId('#')).toBe('hash'); expect(sanitizeHeaderId('△')).toBe('delta'); @@ -104,7 +104,7 @@ test('sanitizeHeaderId should handle inputs with only special characters', () => describe('plugin-chart-table', () => { describe('transformProps', () => { - it('should parse pageLength to pageSize', () => { + test('should parse pageLength to pageSize', () => { expect(transformProps(testData.basic).pageSize).toBe(20); expect( transformProps({ @@ -120,13 +120,13 @@ describe('plugin-chart-table', () => { ).toBe(0); }); - it('should memoize data records', () => { + test('should memoize data records', () => { expect(transformProps(testData.basic).data).toBe( transformProps(testData.basic).data, ); }); - it('should memoize columns meta', () => { + test('should memoize columns meta', () => { expect(transformProps(testData.basic).columns).toBe( transformProps({ ...testData.basic, @@ -135,14 +135,14 @@ describe('plugin-chart-table', () => { ); }); - it('should format timestamp', () => { + test('should format timestamp', () => { // eslint-disable-next-line no-underscore-dangle const parsedDate = transformProps(testData.basic).data[0] .__timestamp as DateWithFormatter; expect(String(parsedDate)).toBe('2020-01-01 12:34:56'); expect(parsedDate.getTime()).toBe(1577882096000); }); - it('should process comparison columns when time_compare and comparison_type are set', () => { + test('should process comparison columns when time_compare and comparison_type are set', () => { const transformedProps = transformProps(testData.comparison); const comparisonColumns = transformedProps.columns.filter( col => @@ -164,7 +164,7 @@ describe('plugin-chart-table', () => { expect(comparisonColumns.some(col => col.label === '%')).toBe(true); }); - it('should not process comparison columns when time_compare is empty', () => { + test('should not process comparison columns when time_compare is empty', () => { const propsWithoutTimeCompare = { ...testData.comparison, rawFormData: { @@ -187,7 +187,7 @@ describe('plugin-chart-table', () => { expect(comparisonColumns.length).toBe(0); }); - it('should correctly apply column configuration for comparison columns', () => { + test('should correctly apply column configuration for comparison columns', () => { const transformedProps = transformProps(testData.comparisonWithConfig); const comparisonColumns = transformedProps.columns.filter( @@ -225,7 +225,7 @@ describe('plugin-chart-table', () => { expect(percentMetricConfig?.config).toEqual({ d3NumberFormat: '.3f' }); }); - it('should correctly format comparison columns using getComparisonColFormatter', () => { + test('should correctly format comparison columns using getComparisonColFormatter', () => { const transformedProps = transformProps(testData.comparisonWithConfig); const comparisonColumns = transformedProps.columns.filter( col => @@ -256,7 +256,7 @@ describe('plugin-chart-table', () => { expect(formattedPercentMetric).toBe('0.123'); }); - it('should set originalLabel for comparison columns when time_compare and comparison_type are set', () => { + test('should set originalLabel for comparison columns when time_compare and comparison_type are set', () => { const transformedProps = transformProps(testData.comparison); // Check if comparison columns are processed @@ -343,7 +343,7 @@ describe('plugin-chart-table', () => { }); describe('TableChart', () => { - it('render basic data', () => { + test('render basic data', () => { render( , ); @@ -362,7 +362,7 @@ describe('plugin-chart-table', () => { expect(cells[8]).toHaveTextContent('N/A'); }); - it('render advanced data', () => { + test('render advanced data', () => { render( , ); @@ -379,7 +379,7 @@ describe('plugin-chart-table', () => { expect(cells[4]).toHaveTextContent('2.47k'); }); - it('render advanced data with currencies', () => { + test('render advanced data with currencies', () => { render( ProviderWrapper({ children: ( @@ -399,7 +399,7 @@ describe('plugin-chart-table', () => { expect(cells[4]).toHaveTextContent('$ 2.47k'); }); - it('render data with a bigint value in a raw record mode', () => { + test('render data with a bigint value in a raw record mode', () => { render( ProviderWrapper({ children: ( @@ -420,7 +420,7 @@ describe('plugin-chart-table', () => { expect(cells[3]).toHaveTextContent('1234567890123456789'); }); - it('render raw data', () => { + test('render raw data', () => { const props = transformProps({ ...testData.raw, rawFormData: { ...testData.raw.rawFormData }, @@ -437,7 +437,7 @@ describe('plugin-chart-table', () => { expect(cells[1]).toHaveTextContent('0'); }); - it('render raw data with currencies', () => { + test('render raw data with currencies', () => { const props = transformProps({ ...testData.raw, rawFormData: { @@ -462,7 +462,7 @@ describe('plugin-chart-table', () => { expect(cells[2]).toHaveTextContent('$ 0'); }); - it('render small formatted data with currencies', () => { + test('render small formatted data with currencies', () => { const props = transformProps({ ...testData.raw, rawFormData: { @@ -504,14 +504,14 @@ describe('plugin-chart-table', () => { expect(cells[2]).toHaveTextContent('$ 0.61'); }); - it('render empty data', () => { + test('render empty data', () => { render( , ); expect(screen.getByText('No records found')).toBeInTheDocument(); }); - it('render color with column color formatter', () => { + test('render color with column color formatter', () => { render( ProviderWrapper({ children: ( @@ -541,7 +541,7 @@ describe('plugin-chart-table', () => { expect(getComputedStyle(screen.getByTitle('2467')).background).toBe(''); }); - it('render cell without color', () => { + test('render cell without color', () => { const dataWithEmptyCell = cloneDeep(testData.advanced.queriesData[0]); dataWithEmptyCell.data.push({ __timestamp: null, @@ -582,7 +582,7 @@ describe('plugin-chart-table', () => { ); expect(getComputedStyle(screen.getByText('N/A')).background).toBe(''); }); - it('should display original label in grouped headers', () => { + test('should display original label in grouped headers', () => { const props = transformProps(testData.comparison); render(); @@ -597,7 +597,7 @@ describe('plugin-chart-table', () => { expect(hasMetricHeaders).toBe(true); }); - it('should set meaningful header IDs for time-comparison columns', () => { + test('should set meaningful header IDs for time-comparison columns', () => { // Test time-comparison columns have proper IDs // Uses originalLabel (e.g., "metric_1") which is sanitized for CSS safety const props = transformProps(testData.comparison); @@ -653,7 +653,7 @@ describe('plugin-chart-table', () => { }); }); - it('should set meaningful header IDs for regular table columns', () => { + test('should set meaningful header IDs for regular table columns', () => { // Test regular (non-time-comparison) columns have proper IDs // Uses fallback to column.key since originalLabel is undefined const props = transformProps(testData.advanced); @@ -732,7 +732,7 @@ describe('plugin-chart-table', () => { }); }); - it('render cell bars properly, and only when it is toggled on in both regular and percent metrics', () => { + test('render cell bars properly, and only when it is toggled on in both regular and percent metrics', () => { const props = transformProps({ ...testData.raw, rawFormData: { ...testData.raw.rawFormData }, @@ -782,7 +782,7 @@ describe('plugin-chart-table', () => { cells = document.querySelectorAll('td'); }); - it('render color with string column color formatter(operator begins with)', () => { + test('render color with string column color formatter(operator begins with)', () => { render( ProviderWrapper({ children: ( @@ -814,7 +814,7 @@ describe('plugin-chart-table', () => { ); }); - it('render color with string column color formatter (operator ends with)', () => { + test('render color with string column color formatter (operator ends with)', () => { render( ProviderWrapper({ children: ( @@ -843,7 +843,7 @@ describe('plugin-chart-table', () => { expect(getComputedStyle(screen.getByText('Joe')).background).toBe(''); }); - it('render color with string column color formatter (operator containing)', () => { + test('render color with string column color formatter (operator containing)', () => { render( ProviderWrapper({ children: ( @@ -872,7 +872,7 @@ describe('plugin-chart-table', () => { expect(getComputedStyle(screen.getByText('Joe')).background).toBe(''); }); - it('render color with string column color formatter (operator not containing)', () => { + test('render color with string column color formatter (operator not containing)', () => { render( ProviderWrapper({ children: ( @@ -903,7 +903,7 @@ describe('plugin-chart-table', () => { ); }); - it('render color with string column color formatter (operator =)', () => { + test('render color with string column color formatter (operator =)', () => { render( ProviderWrapper({ children: ( @@ -934,7 +934,7 @@ describe('plugin-chart-table', () => { ); }); - it('render color with string column color formatter (operator None)', () => { + test('render color with string column color formatter (operator None)', () => { render( ProviderWrapper({ children: ( @@ -967,7 +967,7 @@ describe('plugin-chart-table', () => { ); }); - it('render color with column color formatter to entire row', () => { + test('render color with column color formatter to entire row', () => { render( ProviderWrapper({ children: ( @@ -1003,7 +1003,7 @@ describe('plugin-chart-table', () => { ); }); - it('display text color using column color formatter', () => { + test('display text color using column color formatter', () => { render( ProviderWrapper({ children: ( @@ -1036,7 +1036,7 @@ describe('plugin-chart-table', () => { ); }); - it('display text color using column color formatter for entire row', () => { + test('display text color using column color formatter for entire row', () => { render( ProviderWrapper({ children: (