Skip to content

fix: Time Comparison Feature Reverts Metric Labels to Metric Keys in Table Charts#32665

Merged
geido merged 5 commits into
apache:masterfrom
fardin-developer:fix-metric-label-32621
Mar 25, 2025
Merged

fix: Time Comparison Feature Reverts Metric Labels to Metric Keys in Table Charts#32665
geido merged 5 commits into
apache:masterfrom
fardin-developer:fix-metric-label-32621

Conversation

@fardin-developer
Copy link
Copy Markdown
Contributor

@fardin-developer fardin-developer commented Mar 14, 2025

Fixes: #32621
FEATURE_CHART_PLUGINS_EXPERIMENTAL=true

SUMMARY

This PR addresses an issue where, with Time Comparison enabled in the Table Chart, the metric name ignores the defined label and instead uses the metric key. This results in an unfriendly display name in the UI.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

###Changes

  • Introduced originalLabel in the DataColumnMeta interface to retain the correct label.
  • Updated processComparisonColumns to store originalLabel before modifying the metric label.
  • Ensured renderGroupingHeaders retrieves and displays the originalLabel correctly in grouped headers.

BEFORE/AFTER SCREENSHOTS

Before

Screenshot 2025-03-14 at 11 33 05 AM

After

Screenshot 2025-03-14 at 12 29 16 PM

@dosubot dosubot Bot added the viz:charts:table Related to the Table chart label Mar 14, 2025
Copy link
Copy Markdown

@korbit-ai korbit-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.
Category Issue Fix Detected
Error Handling Missing error logging for originalLabel fallback ▹ view
Documentation Unclear relationship between label properties ▹ view
Files scanned
File Path Reviewed
superset-frontend/plugins/plugin-chart-table/src/types.ts
superset-frontend/plugins/plugin-chart-table/src/transformProps.ts
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.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

  • You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.
  • You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.
  • Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.
  • On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment.
  • Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

  • Check out our docs on how you can make Korbit work best for you and your team.
  • Customize Korbit for your organization through the Korbit Console.

Feedback and Support

Comment thread superset-frontend/plugins/plugin-chart-table/src/types.ts
Comment thread superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx Outdated
@fardin-developer fardin-developer changed the title Fix: Time Comparison Feature Reverts Metric Labels to Metric Keys in Table Charts fix: Time Comparison Feature Reverts Metric Labels to Metric Keys in Table Charts Mar 14, 2025
Comment on lines +612 to +615
// Use a proper logging function if available
console.debug(
`originalLabel not found for column at index ${value[0]}, using key as fallback`,
);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Use a proper logging function if available
console.debug(
`originalLabel not found for column at index ${value[0]}, using key as fallback`,
);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

Copy link
Copy Markdown
Member

@Antonio-RiveroMartnez Antonio-RiveroMartnez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding extra processing using the label + the comparison ones like we do for the key and not include a new property is also an option, but I have not strong preference and these changes look like a less risky fix. Thanks for taking care of this!

@geido geido added preset:bounty Issues that have been selected by Preset and have a bounty attached. hold! On hold labels Mar 17, 2025
@geido
Copy link
Copy Markdown
Member

geido commented Mar 17, 2025

This looks good @fardin-developer. Can you please add a test for this change?

@fardin-developer
Copy link
Copy Markdown
Contributor Author

This looks good @fardin-developer. Can you please add a test for this change?

Thank you, I’ll include a test with the PR.

@pull-request-size pull-request-size Bot added size/M and removed size/S labels Mar 21, 2025
@apache apache deleted a comment from github-actions Bot Mar 21, 2025
@apache apache deleted a comment from github-actions Bot Mar 21, 2025
@apache apache deleted a comment from github-actions Bot Mar 21, 2025
@apache apache deleted a comment from github-actions Bot Mar 21, 2025
@github-actions
Copy link
Copy Markdown
Contributor

@geido Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments

@github-actions
Copy link
Copy Markdown
Contributor

@geido Ephemeral environment spinning up at http://35.94.39.180:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.

@fardin-developer fardin-developer requested a review from geido March 21, 2025 15:25
@eschutho
Copy link
Copy Markdown
Member

Thanks @fardin-developer, I tested on the ephemeral and it looks great!

@fardin-developer
Copy link
Copy Markdown
Contributor Author

Thanks @fardin-developer, I tested on the ephemeral and it looks great!

Glad to hear it worked out! Thanks for the update.

@geido geido removed the hold! On hold label Mar 25, 2025
@geido geido merged commit 7d77dc4 into apache:master Mar 25, 2025
@michael-s-molina michael-s-molina added the v5.0 Label added by the release manager to track PRs to be included in the 5.0 branch label Mar 25, 2025
michael-s-molina pushed a commit that referenced this pull request Mar 25, 2025
…Table Charts (#32665)

Co-authored-by: Fardin Mustaque <fardinmustaque@Fardins-Mac-mini.local>
(cherry picked from commit 7d77dc4)
michael-s-molina pushed a commit that referenced this pull request Apr 14, 2025
…Table Charts (#32665)

Co-authored-by: Fardin Mustaque <fardinmustaque@Fardins-Mac-mini.local>
(cherry picked from commit 7d77dc4)
alexandrusoare pushed a commit to alexandrusoare/superset that referenced this pull request Jun 19, 2025
…Table Charts (apache#32665)

Co-authored-by: Fardin Mustaque <fardinmustaque@Fardins-Mac-mini.local>
(cherry picked from commit 7d77dc4)
@mistercrunch mistercrunch added 🍒 5.0.0 Cherry-picked to 5.0.0 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels labels Jul 29, 2025
sadpandajoe added a commit that referenced this pull request Nov 4, 2025
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>
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
…Table Charts (apache#32665)

Co-authored-by: Fardin Mustaque <fardinmustaque@Fardins-Mac-mini.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels plugins preset:bounty Issues that have been selected by Preset and have a bounty attached. size/M v5.0 Label added by the release manager to track PRs to be included in the 5.0 branch viz:charts:table Related to the Table chart 🍒 5.0.0 Cherry-picked to 5.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

With Time Comparison enabled, the metric name ignores the label and uses the metric key instead

7 participants