Skip to content

Conversation

@EnxDev
Copy link
Contributor

@EnxDev EnxDev commented Dec 4, 2025

SUMMARY

Clearing previously set min and max values in a gauge chart sets the data labels to 0 instead of getting default values.

Now we:

  1. Validate that minVal and maxVal are valid numbers

  2. Treat null, undefined, empty strings, NaN, and Infinity as invalid

  3. Fall back to calculated defaults when values are invalid

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:

519715981-cf840003-21fa-46cd-a246-7c8076594574.mp4

AFTER:

519716143-5b7e2b23-5e2c-4709-9be4-fd71d8ffeda8.mp4

TESTING INSTRUCTIONS

  1. Create a Gauge Chart with the Vehicle Sales dataset
  2. Set something like this:
  3. Go to Customize and assign a Min value like 150000 and a Max as 2000000
  4. Click on Update Chart
  5. Remove the Min a Max values you previously assigned.

Expected results
You should now get the default values of the chart

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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

@geido geido marked this pull request as ready for review December 4, 2025 16:18
@dosubot dosubot bot added the viz:charts:gauge Related to the Gauge chart label Dec 4, 2025
@geido geido added testenv-up and removed viz:charts:gauge Related to the Gauge chart labels Dec 4, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2025

⚠️ DEPRECATED WORKFLOW ⚠️

@geido This workflow is deprecated! Please use the new Superset Showtime system instead:

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

@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2025

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

@geido geido added the 🎪 ⚡ showtime-trigger-start Create new ephemeral environment for this PR label Dec 4, 2025
@github-actions github-actions bot added 🎪 b98359d 🚦 building Environment b98359d status: building 🎪 b98359d 📅 2025-12-04T16-53 Environment b98359d created at 2025-12-04T16-53 🎪 b98359d ⌛ 48h Environment b98359d expires after 48h 🎪 b98359d 🤡 geido Environment b98359d requested by geido and removed 🎪 ⚡ showtime-trigger-start Create new ephemeral environment for this PR labels Dec 4, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2025

🎪 Showtime is building environment on GHA for b98359d

@github-actions github-actions bot added 🎪 b98359d 🚦 deploying Environment b98359d status: deploying 🎪 b98359d 🚦 running Environment b98359d status: running 🎪 🎯 b98359d Active environment pointer - b98359d is receiving traffic 🎪 b98359d 🌐 54.188.234.208:8080 Environment b98359d URL: http://54.188.234.208:8080 (click to visit) and removed 🎪 b98359d 🚦 building Environment b98359d status: building 🎪 b98359d 🚦 deploying Environment b98359d status: deploying 🎪 b98359d 🚦 running Environment b98359d status: running 🎪 🎯 b98359d Active environment pointer - b98359d is receiving traffic labels Dec 4, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2025

🎪 Showtime deployed environment on GHA for b98359d

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

Copilot finished reviewing on behalf of sadpandajoe December 4, 2025 18:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a bug where clearing previously set min and max values in a gauge chart incorrectly sets the data labels to 0 instead of falling back to calculated default values.

Key Changes:

  • Introduced a validation function isValidNumber to properly validate min/max values before using them
  • Modified min/max calculation logic to fall back to calculated defaults when values are invalid (null, undefined, empty strings, NaN, or Infinity)
  • Added comprehensive test coverage for various min/max validation scenarios

Reviewed changes

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

File Description
superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts Added isValidNumber helper function to validate min/max values and updated min/max calculation logic to use validation before applying user-provided values
superset-frontend/plugins/plugin-chart-echarts/test/Gauge/transformProps.test.ts Added 15 new test cases covering various scenarios: null values, empty strings, invalid strings, numeric strings, negative values, zero values, and different splitNumber configurations

const max = maxVal ?? calculateMax(transformedData);
const isValidNumber = (
val: number | null | undefined | string,
): val is number => {
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

The type predicate val is number is inaccurate since the function returns true when val is a valid numeric string (e.g., '10'). While lines 234 and 237 handle this correctly with Number(minVal) and Number(maxVal), the type guard is misleading.

Consider changing the return type to accurately reflect what the function validates:

const isValidNumber = (
  val: number | null | undefined | string,
): val is number | string => {

Or, alternatively, be more precise and only accept actual numbers in the type guard:

const isValidNumber = (
  val: number | null | undefined | string,
): boolean => {
Suggested change
): val is number => {
): boolean => {

Copilot uses AI. Check for mistakes.
});
});

describe('Min/Max calculation and axis labels', () => {
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

The test suite is missing coverage for important edge cases that the implementation handles:

  1. minVal or maxVal set to NaN
  2. minVal or maxVal set to Infinity or -Infinity
  3. minVal or maxVal explicitly set to undefined

These cases are validated in the implementation (lines 228-230 of transformProps.ts), but should have corresponding test cases to ensure they fall back to calculated defaults correctly.

Consider adding tests like:

it('should calculate min/max from data when minVal is NaN', () => {
  const formData: SqlaFormData = {
    ...baseFormData,
    minVal: NaN as any,
    maxVal: 100,
  };
  // ... test that min is calculated from data
});

it('should calculate min/max from data when maxVal is Infinity', () => {
  const formData: SqlaFormData = {
    ...baseFormData,
    minVal: 0,
    maxVal: Infinity as any,
  };
  // ... test that max is calculated from data
});

Copilot uses AI. Check for mistakes.
@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2025

⚠️ DEPRECATED WORKFLOW ⚠️

@dpgaspar This workflow is deprecated! Please use the new Superset Showtime system instead:

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

@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2025

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

@geido geido merged commit 236e000 into master Dec 5, 2025
125 of 155 checks passed
@geido geido deleted the enxdev/fix/gauge-min-max branch December 5, 2025 17:16
@sadpandajoe sadpandajoe added the v6.0 Label added by the release manager to track PRs to be included in the 6.0 branch label Dec 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🎪 b98359d 🤡 geido Environment b98359d requested by geido 🎪 b98359d 🚦 running Environment b98359d status: running 🎪 b98359d ⌛ 48h Environment b98359d expires after 48h 🎪 b98359d 🌐 54.188.234.208:8080 Environment b98359d URL: http://54.188.234.208:8080 (click to visit) 🎪 b98359d 📅 2025-12-04T16-53 Environment b98359d created at 2025-12-04T16-53 plugins size/L testenv-up v6.0 Label added by the release manager to track PRs to be included in the 6.0 branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants