Copilot commented on code in PR #35968:
URL: https://github.com/apache/superset/pull/35968#discussion_r2500711947
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -467,7 +542,7 @@ describe('plugin-chart-table', () => {
});
it('render cell without color', () => {
- const dataWithEmptyCell = testData.advanced.queriesData[0];
+ const dataWithEmptyCell = cloneDeep(testData.advanced.queriesData[0]);
Review Comment:
Using `cloneDeep` prevents mutation of shared test data, but this may create
a shallow reference issue. The variable `dataWithEmptyCell` is assigned a deep
clone of `queriesData[0]`, but then the code pushes to `dataWithEmptyCell.data`
which should work correctly. However, the subsequent usage in `transformProps`
should verify this clone is used. Please verify the test props construction
uses this cloned data rather than the original `testData.advanced`.
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -141,6 +141,31 @@ 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.
+ *
+ * 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 {
+ 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
+ );
+}
Review Comment:
The sanitization logic has an ordering issue. Line 163 replaces all
non-alphanumeric characters (except underscore and hyphen) with underscores,
which means semantic replacements from lines 158-160 can introduce characters
that are then immediately replaced. For example, `sanitizeHeaderId('%')`
replaces `%` with `'percent'` (✓), but if a column name is `'%'`, it becomes
`'percent'` correctly. However, consider `'%@'`: it becomes `'percent@'` →
`'percent_'` → `'percent'`. This works, but the flow could be clearer. The
logic is correct but the comment on line 157 is misleading - it says the
semantic replacement avoids `'_pct_nice'`, but actually `'%pct_nice'` →
`'percentpct_nice'` → `'percentpct_nice'` (no underscores between 'percent' and
'pct'). Consider updating the comment to accurately reflect the behavior.
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -18,12 +18,90 @@
*/
import '@testing-library/jest-dom';
import { render, screen } from '@superset-ui/core/spec';
-import TableChart from '../src/TableChart';
+import { cloneDeep } from 'lodash';
+import TableChart, { sanitizeHeaderId } from '../src/TableChart';
import transformProps from '../src/transformProps';
import DateWithFormatter from '../src/utils/DateWithFormatter';
import testData from './testData';
import { ProviderWrapper } from './testHelpers';
+test('sanitizeHeaderId should sanitize percent sign', () => {
+ expect(sanitizeHeaderId('%pct_nice')).toBe('percentpct_nice');
+});
+
+test('sanitizeHeaderId should sanitize hash/pound sign', () => {
+ expect(sanitizeHeaderId('# metric_1')).toBe('hash_metric_1');
+});
+
+test('sanitizeHeaderId should sanitize delta symbol', () => {
+ expect(sanitizeHeaderId('△ delta')).toBe('delta_delta');
+});
+
+test('sanitizeHeaderId should replace spaces with underscores', () => {
+ expect(sanitizeHeaderId('Main metric_1')).toBe('Main_metric_1');
+ expect(sanitizeHeaderId('multiple spaces')).toBe('multiple_spaces');
+});
+
+test('sanitizeHeaderId should handle multiple special characters', () => {
+ expect(sanitizeHeaderId('% #△ test')).toBe('percent_hashdelta_test');
+ expect(sanitizeHeaderId('% # △ test')).toBe('percent_hash_delta_test');
+});
+
+test('sanitizeHeaderId should preserve alphanumeric, underscore, and hyphen',
() => {
+ expect(sanitizeHeaderId('valid-name_123')).toBe('valid-name_123');
+});
+
+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('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');
+});
+
+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)
Review Comment:
The comment states '# → 'hash' is preserved (semantic replacement)' but this
is misleading. The `#` is not 'preserved' - it's replaced with the word 'hash'.
The comment should say '# → 'hash' replacement' or '# is replaced with 'hash'
(semantic replacement)'.
```suggestion
expect(sanitizeHeaderId('!@#$')).toBe('hash'); // # is replaced with
'hash' (semantic replacement)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
