Copilot commented on code in PR #35968:
URL: https://github.com/apache/superset/pull/35968#discussion_r2496673576
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -467,7 +503,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:
[nitpick] Using `cloneDeep` here is appropriate to prevent test pollution by
modifying the original test data. However, the variable name
`dataWithEmptyCell` is misleading - based on the test context at line 507-510,
this data will have a new row pushed to it, so it's not specifically 'empty
cell data' but rather 'modified query data'. Consider renaming to
`modifiedQueryData` or `queryDataWithExtraRow` for clarity.
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -141,6 +141,20 @@ 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.
+ * 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, '_');
Review Comment:
[nitpick] HTML id attributes have an additional constraint: they cannot
start with a digit. If a column key begins with a number (e.g.,
`'123_column'`), the resulting ID `'header-123_column'` would be valid because
of the `'header-'` prefix. However, the `sanitizeHeaderId` function doesn't
enforce this constraint itself. Consider either adding a comment explaining
this dependency on the `'header-'` prefix, or making the function more robust
by handling numeric prefixes (e.g., prepending `'col_'` if the ID starts with a
digit).
```suggestion
* Ensures the result does not start with a digit (prepends 'col_' if
needed).
* Exported for testing.
*/
export function sanitizeHeaderId(columnId: string): string {
let sanitized = columnId
.replace(/%/g, 'percent')
.replace(/#/g, 'hash')
.replace(/△/g, 'delta')
.replace(/\s+/g, '_')
.replace(/[^a-zA-Z0-9_-]/g, '_');
if (/^[0-9]/.test(sanitized)) {
sanitized = 'col_' + sanitized;
}
return sanitized;
```
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -141,6 +141,20 @@ 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.
+ * 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, '_');
Review Comment:
[nitpick] The sanitization logic can produce consecutive underscores. For
example, `'test @ # space'` would result in `'test_hash_space'` after line 152,
but `'test @@ space'` would result in `'test___space'` (each `@` becomes `_` on
line 155). Consider adding a final cleanup step like `.replace(/_+/g, '_')` to
collapse consecutive underscores into a single underscore for cleaner IDs.
```suggestion
.replace(/[^a-zA-Z0-9_-]/g, '_')
.replace(/_+/g, '_');
```
--
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]
