From ceb4018bf5783957762e17eba323b7a416eaac4d Mon Sep 17 00:00:00 2001 From: JCelento Date: Mon, 8 Jun 2026 12:47:51 -0300 Subject: [PATCH 1/2] fix(explore): drop inherit/custom time shifts when target viz can't honor them Converting a chart from Big Number / Table period-over-period (which offer "inherit" and "custom" time shifts) to a timeseries chart carried the inherit/custom value into the new chart's Time shift control, where it lingered as a tag the user had to remove manually. The timeseries advanced-analytics Time shift reuses the same `time_compare` key but does not offer those choices, and since the control is free-form it accepted them. standardizedFormData.transform now strips inherit/custom from `time_compare` when the target viz type's control does not list them as choices, and keeps them when it does (e.g. Table to Big Number). --- .../controlUtils/standardizedFormData.test.ts | 41 +++++++++++++++++++ .../controlUtils/standardizedFormData.ts | 31 ++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/superset-frontend/src/explore/controlUtils/standardizedFormData.test.ts b/superset-frontend/src/explore/controlUtils/standardizedFormData.test.ts index 9c2dc91d0b8d..90e804585e61 100644 --- a/superset-frontend/src/explore/controlUtils/standardizedFormData.test.ts +++ b/superset-frontend/src/explore/controlUtils/standardizedFormData.test.ts @@ -344,6 +344,47 @@ describe('should collect control values and create SFD', () => { ]); }); + test('strips inherit/custom time shifts when target viz does not support them', () => { + // Table/Big Number period-over-period offer "inherit" and "custom" time + // shifts; the timeseries advanced-analytics target (target_viz) does not. + // Carrying them over leaves un-removable tags on the new chart (SC-99170). + const store = { + ...sourceMockStore, + form_data: { + ...sourceMockFormData, + time_compare: ['1 year ago', 'inherit', 'custom'], + }, + }; + const sfd = new StandardizedFormData(store.form_data); + const { formData } = sfd.transform('target_viz', store); + // the free-form delta survives, the special markers are dropped + expect(formData.time_compare).toEqual(['1 year ago']); + }); + + test('preserves inherit/custom time shifts when target viz supports them', () => { + getChartControlPanelRegistry().registerValue('target_viz_inherit', { + controlPanelSections: [ + sections.timeComparisonControls({}), + { + label: 'transform controls', + controlSetRows: publicControls + .filter(c => c !== 'dashboardId' && c !== 'time_compare') + .map(control => [control]), + }, + ], + }); + const store = { + ...sourceMockStore, + form_data: { + ...sourceMockFormData, + time_compare: ['1 year ago', 'inherit'], + }, + }; + const sfd = new StandardizedFormData(store.form_data); + const { formData } = sfd.transform('target_viz_inherit', store); + expect(formData.time_compare).toEqual(['1 year ago', 'inherit']); + }); + test('should inherit standardizedFormData and memorizedFormData is LIFO', () => { // from source_viz to target_viz const sfd = new StandardizedFormData(sourceMockFormData); diff --git a/superset-frontend/src/explore/controlUtils/standardizedFormData.ts b/superset-frontend/src/explore/controlUtils/standardizedFormData.ts index 0462b9e3a1c6..0658619e74b3 100644 --- a/superset-frontend/src/explore/controlUtils/standardizedFormData.ts +++ b/superset-frontend/src/explore/controlUtils/standardizedFormData.ts @@ -156,6 +156,36 @@ export class StandardizedFormData { return controls; } + // Time shifts only meaningful for viz types whose "Time shift" control offers + // them (Big Number / Table period-over-period). Other viz types reuse the same + // `time_compare` key without these choices. + private static specialTimeShifts = ['inherit', 'custom']; + + // Drop `time_compare` markers the target viz can't honor so they don't carry + // over as un-removable tags when switching chart types. + static dropUnsupportedTimeShifts( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + controlsState: Record, + ): void { + const control = controlsState?.time_compare; + if (!control || !Array.isArray(control.value)) { + return; + } + const supportedChoices = new Set( + ensureIsArray(control.choices).map( + (choice: [string, string]) => choice[0], + ), + ); + const filtered = control.value.filter( + (shift: string) => + !StandardizedFormData.specialTimeShifts.includes(shift) || + supportedChoices.has(shift), + ); + if (filtered.length !== control.value.length) { + control.value = filtered.length ? filtered : null; + } + } + private getLatestFormData(vizType: string): QueryFormData { if (this.has(vizType)) { return this.get(vizType); @@ -215,6 +245,7 @@ export class StandardizedFormData { ...publicFormData, viz_type: targetVizType, }); + StandardizedFormData.dropUnsupportedTimeShifts(targetControlsState); const targetFormData = { // eslint-disable-next-line @typescript-eslint/no-explicit-any ...getFormDataFromControls(targetControlsState as any), From a38122ab57173867dad43d814c260032d85575e2 Mon Sep 17 00:00:00 2001 From: JCelento Date: Wed, 10 Jun 2026 10:44:29 -0300 Subject: [PATCH 2/2] refactor(explore): type-guard dropUnsupportedTimeShifts and cover null branch Address review feedback on #40865: - Replace the `any` controls-state param with `Record` plus runtime guards, so future control-shape changes can't silently bypass the time-shift filtering (also drops an `any` from this file). - Add a test for the all-unsupported case where every shift is stripped and `time_compare` clears to null, locking in the empty-result branch. - Register the inherit-supporting target viz in beforeAll for setup consistency. --- .../controlUtils/standardizedFormData.test.ts | 39 +++++++++++++------ .../controlUtils/standardizedFormData.ts | 19 +++++---- 2 files changed, 40 insertions(+), 18 deletions(-) diff --git a/superset-frontend/src/explore/controlUtils/standardizedFormData.test.ts b/superset-frontend/src/explore/controlUtils/standardizedFormData.test.ts index 90e804585e61..93e91d125a86 100644 --- a/superset-frontend/src/explore/controlUtils/standardizedFormData.test.ts +++ b/superset-frontend/src/explore/controlUtils/standardizedFormData.test.ts @@ -276,6 +276,18 @@ describe('should collect control values and create SFD', () => { metrics: formData.standardizedFormData.controls.metrics, }), }); + // a target viz whose "Time shift" control offers inherit/custom choices + getChartControlPanelRegistry().registerValue('target_viz_inherit', { + controlPanelSections: [ + sections.timeComparisonControls({}), + { + label: 'transform controls', + controlSetRows: publicControlFields + .filter(c => c !== 'time_compare') + .map(control => [control]), + }, + ], + }); }); test('should avoid to overlap', () => { @@ -362,17 +374,6 @@ describe('should collect control values and create SFD', () => { }); test('preserves inherit/custom time shifts when target viz supports them', () => { - getChartControlPanelRegistry().registerValue('target_viz_inherit', { - controlPanelSections: [ - sections.timeComparisonControls({}), - { - label: 'transform controls', - controlSetRows: publicControls - .filter(c => c !== 'dashboardId' && c !== 'time_compare') - .map(control => [control]), - }, - ], - }); const store = { ...sourceMockStore, form_data: { @@ -385,6 +386,22 @@ describe('should collect control values and create SFD', () => { expect(formData.time_compare).toEqual(['1 year ago', 'inherit']); }); + test('clears time_compare to null when every shift is unsupported', () => { + // When only special markers carry over and none survive, the control value + // must serialize to null (not an empty array) so the target chart shows an + // empty "Time shift" rather than a stray tag (SC-99170). + const store = { + ...sourceMockStore, + form_data: { + ...sourceMockFormData, + time_compare: ['inherit', 'custom'], + }, + }; + const sfd = new StandardizedFormData(store.form_data); + const { formData } = sfd.transform('target_viz', store); + expect(formData.time_compare).toBeNull(); + }); + test('should inherit standardizedFormData and memorizedFormData is LIFO', () => { // from source_viz to target_viz const sfd = new StandardizedFormData(sourceMockFormData); diff --git a/superset-frontend/src/explore/controlUtils/standardizedFormData.ts b/superset-frontend/src/explore/controlUtils/standardizedFormData.ts index 0658619e74b3..1adf00d1ee7a 100644 --- a/superset-frontend/src/explore/controlUtils/standardizedFormData.ts +++ b/superset-frontend/src/explore/controlUtils/standardizedFormData.ts @@ -164,20 +164,25 @@ export class StandardizedFormData { // Drop `time_compare` markers the target viz can't honor so they don't carry // over as un-removable tags when switching chart types. static dropUnsupportedTimeShifts( - // eslint-disable-next-line @typescript-eslint/no-explicit-any - controlsState: Record, + controlsState: Record, ): void { - const control = controlsState?.time_compare; + const control = controlsState?.time_compare as + | { value?: unknown; choices?: unknown } + | undefined; if (!control || !Array.isArray(control.value)) { return; } const supportedChoices = new Set( - ensureIsArray(control.choices).map( - (choice: [string, string]) => choice[0], - ), + ensureIsArray(control.choices) + .filter( + (choice): choice is [string, string] => + Array.isArray(choice) && typeof choice[0] === 'string', + ) + .map(choice => choice[0]), ); const filtered = control.value.filter( - (shift: string) => + (shift: unknown) => + typeof shift !== 'string' || !StandardizedFormData.specialTimeShifts.includes(shift) || supportedChoices.has(shift), );