fix(table-chart): fix missing table header IDs by sadpandajoe · Pull Request #35968 · apache/superset

sadpandajoe · 2025-11-04T05:24:57Z

SUMMARY

Table chart column headers were rendering with id="header-undefined" instead of meaningful IDs, breaking CSS customization and ARIA accessibility that worked in Superset 4.1.1.

Root Cause:
PR #31590 (Ant Design v5 migration) 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)}"

Column identifiers can contain CSS-unsafe characters (%, #, △, spaces), so added sanitizeHeaderId() helper to ensure valid CSS selectors and ARIA references.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

TESTING INSTRUCTIONS

Manual Testing:

Test regular table chart:

- Create a table chart with regular columns (non-time-comparison)
- Inspect the DOM (browser dev tools)
- Verify column headers have meaningful IDs like id="header-column_name"
- Verify NO headers have id="header-undefined"
- Verify table cells have matching aria-labelledby attributes

Test time-comparison table chart:

- Create a table chart with time comparison enabled
- Add metrics and enable comparison type
- Inspect the DOM
- Verify comparison columns have IDs like id="header-metric_1"
- Verify NO headers have id="header-undefined"

Test special characters:

- Create columns with special characters: %pct_nice, #count, etc.
- Verify IDs are sanitized: id="header-percentpct_nice", id="header-hashcount"
- Verify no spaces or invalid CSS characters in IDs

Test CSS customization (issue reporter's use case):

/* Should now work */
#header-column_name {
  background-color: red;
}

Automated Testing:

# Run table chart tests
npm test -- plugins/plugin-chart-table/test/TableChart.test.tsx

# Run pre-commit validation
pre-commit run --files plugins/plugin-chart-table/src/TableChart.tsx plugins/plugin-chart-table/test/TableChart.test.tsx

ADDITIONAL INFORMATION

Has associated issue: Fixes 【6.0.0-RC2】Table chart header IDs are not populated correctly #35783
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

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 <noreply@anthropic.com>

**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>

bito-code-review · 2025-11-04T05:25:06Z

Code Review Agent Run #67d15c

Actionable Suggestions - 0

Review Details

Files reviewed - 2 · Commit Range: 790fe5c..9fcddc5
- superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
- superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers a full AI review.
/pause - Pauses automatic reviews on this pull request.
/resume - Resumes automatic reviews.
/resolve - Marks all Bito-posted review comments as resolved.
/abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by

korbit-ai

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Status
	Redundant header ID sanitization in render loop ▹ view
	Inconsistent header ID generation fallback ▹ view

Files scanned

File Path	Reviewed
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

github-actions · 2025-11-04T06:03:54Z

🎪 Showtime is building environment on GHA for 9fcddc5

github-actions · 2025-11-04T06:18:36Z

🎪 Showtime deployed environment on GHA for 9fcddc5

• Environment: http://35.88.151.195:8080 (admin/admin)
• Lifetime: 48h auto-cleanup
• Updates: New commits create fresh environments automatically

Copilot

Pull Request Overview

This PR adds header ID sanitization to the TableChart component to ensure HTML id attributes and ARIA labels are valid and safe for CSS selectors. The change prevents issues with special characters (%, #, △) and spaces in column identifiers that could break accessibility features and CSS selectors.

Introduces sanitizeHeaderId() function to replace problematic characters in header IDs
Updates header id and aria-labelledby attributes to use sanitized IDs
Adds comprehensive test coverage for the sanitization function and validates proper ARIA attribute handling

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx	Implements `sanitizeHeaderId()` function and applies it to header and cell ARIA attributes
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx	Adds unit tests for `sanitizeHeaderId()` and integration tests verifying proper header ID generation and ARIA attribute references

Copilot · 2025-11-06T00:56:26Z


      it('render cell without color', () => {
-        const dataWithEmptyCell = testData.advanced.queriesData[0];
+        const dataWithEmptyCell = cloneDeep(testData.advanced.queriesData[0]);


[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.

The name captures that we are testing empty cell rendering rather than the cloning + pushing. Will probably not change unless if we think it's a merge blocker.

… feedback 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 <noreply@anthropic.com>

github-actions · 2025-11-06T20:12:54Z

🎪 Showtime deployed environment on GHA for f680611

• Environment: http://35.89.84.55:8080 (admin/admin)
• Lifetime: 48h auto-cleanup
• Updates: New commits create fresh environments automatically

github-actions · 2025-11-06T20:16:22Z

🎪 Showtime is building environment on GHA for 5407932

Copilot

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Copilot · 2025-11-06T20:22:12Z


      it('render cell without color', () => {
-        const dataWithEmptyCell = testData.advanced.queriesData[0];
+        const dataWithEmptyCell = cloneDeep(testData.advanced.queriesData[0]);


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.

The cloned data is used in the test, prevents mutation and things are working correctly. I don't thin this is a release blocker unless if the test becomes flakey.

Copilot · 2025-11-06T20:22:13Z

+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
+  );
+}


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.

Copilot · 2025-11-06T20:22:13Z

+  expect(sanitizeHeaderId('@@')).toBe('');
+  expect(sanitizeHeaderId('   ')).toBe('');
+  expect(sanitizeHeaderId('!@$')).toBe('');
+  expect(sanitizeHeaderId('!@#$')).toBe('hash'); // # → 'hash' is preserved (semantic replacement)


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)'.

Suggested change

expect(sanitizeHeaderId('!@#$')).toBe('hash'); // # → 'hash' is preserved (semantic replacement)

expect(sanitizeHeaderId('!@#$')).toBe('hash'); // # is replaced with 'hash' (semantic replacement)

github-actions · 2025-11-06T20:28:21Z

🎪 Showtime deployed environment on GHA for 5407932

• Environment: http://44.252.109.198:8080 (admin/admin)
• Lifetime: 48h auto-cleanup
• Updates: New commits create fresh environments automatically

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 <noreply@anthropic.com>

github-actions · 2025-11-06T20:45:16Z

🎪 Showtime is building environment on GHA for ebfb2a9

github-actions · 2025-11-06T20:56:12Z

🎪 Showtime deployed environment on GHA for ebfb2a9

• Environment: http://44.244.132.19:8080 (admin/admin)
• Lifetime: 48h auto-cleanup
• Updates: New commits create fresh environments automatically

sadpandajoe · 2025-11-06T20:57:28Z

@rusackas wondering if one of your changes/rules didn't get updated. I had added it tags for tests and they didn't fail. I caught it and updated the all to test now. But just a heads up.

Copilot

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

sadpandajoe and others added 2 commits November 3, 2025 16:35

pull-request-size Bot added the size/L label Nov 4, 2025

github-actions Bot added the plugins label Nov 4, 2025

dosubot Bot added the viz:charts:table Related to the Table chart label Nov 4, 2025

korbit-ai Bot suggested changes Nov 4, 2025

View reviewed changes

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

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

sadpandajoe added the 🎪 ⚡ showtime-trigger-start Create new ephemeral environment for this PR label Nov 4, 2025

github-actions Bot added 🎪 9fcddc5 🚦 building and removed 🎪 ⚡ showtime-trigger-start Create new ephemeral environment for this PR labels Nov 4, 2025

github-actions Bot added 🎪 9fcddc5 🚦 deploying and removed 🎪 9fcddc5 🚦 building labels Nov 4, 2025

sadpandajoe requested review from Copilot and geido November 4, 2025 18:26

Copilot AI reviewed Nov 6, 2025

View reviewed changes

github-actions Bot added dependencies:npm 🎪 af6b840 🚦 building labels Nov 6, 2025

github-actions Bot added 🎪 🎯 f680611 and removed 🎪 f680611 🚦 running labels Nov 6, 2025

github-actions Bot added 🎪 5407932 🚦 building labels Nov 6, 2025

sadpandajoe requested a review from Copilot November 6, 2025 20:18

Copilot AI reviewed Nov 6, 2025

View reviewed changes

github-actions Bot added 🎪 5407932 🚦 deploying and removed 🎪 5407932 🚦 building labels Nov 6, 2025

Copilot AI reviewed Nov 10, 2025

View reviewed changes

betodealmeida approved these changes Nov 12, 2025

View reviewed changes

sadpandajoe mentioned this pull request Nov 18, 2025

test(table): remove conditionals from TableChart tests #36149

Merged

9 tasks

dosubot Bot mentioned this pull request Nov 19, 2025

Table Headers with two metrics and time shift are bad aligned #36185

Open

3 tasks

dosubot Bot mentioned this pull request Jan 13, 2026

Wrong group names in Table Time Comparison when not showing all columns #37074

Closed

3 tasks

	expect(sanitizeHeaderId('!@#$')).toBe('hash'); // # → 'hash' is preserved (semantic replacement)
	expect(sanitizeHeaderId('!@#$')).toBe('hash'); // # is replaced with 'hash' (semantic replacement)

Conversation

sadpandajoe commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

bito-code-review Bot commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #67d15c

Uh oh!

korbit-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Nov 4, 2025

Uh oh!

github-actions Bot commented Nov 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

sadpandajoe Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Nov 6, 2025

Uh oh!

github-actions Bot commented Nov 6, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

sadpandajoe Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Nov 6, 2025

Uh oh!

github-actions Bot commented Nov 6, 2025

Uh oh!

github-actions Bot commented Nov 6, 2025

Uh oh!

sadpandajoe commented Nov 6, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sadpandajoe commented Nov 4, 2025 •

edited

Loading

bito-code-review Bot commented Nov 4, 2025 •

edited

Loading

korbit-ai Bot left a comment •

edited

Loading