Copilot commented on code in PR #35968:
URL: https://github.com/apache/superset/pull/35968#discussion_r2496869311
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -141,6 +141,25 @@ 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
+ .replace(/%/g, 'percent')
+ .replace(/#/g, 'hash')
+ .replace(/△/g, 'delta')
+ .replace(/\s+/g, '_')
+ .replace(/[^a-zA-Z0-9_-]/g, '_')
+ .replace(/_+/g, '_'); // Collapse consecutive underscores
Review Comment:
The function doesn't handle leading/trailing underscores that could result
from the replacements. For example, `sanitizeHeaderId('% metric')` produces
`'percent_metric'` (correct), but `sanitizeHeaderId('%')` produces `'percent'`
(correct), yet `sanitizeHeaderId('@')` produces `'_'` (a lone underscore).
Consider adding `.replace(/^_+|_+$/g, '')` at the end to trim leading/trailing
underscores for edge cases.
```suggestion
.replace(/_+/g, '_') // Collapse consecutive underscores
.replace(/^_+|_+$/g, ''); // Trim leading/trailing underscores
```
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -141,6 +141,25 @@ 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
+ .replace(/%/g, 'percent')
+ .replace(/#/g, 'hash')
+ .replace(/△/g, 'delta')
+ .replace(/\s+/g, '_')
+ .replace(/[^a-zA-Z0-9_-]/g, '_')
+ .replace(/_+/g, '_'); // Collapse consecutive underscores
Review Comment:
The replacement order could be optimized. Line 158
`replace(/[^a-zA-Z0-9_-]/g, '_')` already converts most special characters
including `%`, `#`, and `△` to underscores. The specific replacements in lines
154-156 only have effect if they occur before line 158. Consider documenting
why these specific replacements are done first (likely for semantic meaning
like 'percent' instead of '_'), or move them after line 158 if they should
override the generic 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]
