You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "durgaprasadml (via GitHub)" <gi...@apache.org> on 2026/07/10 01:06:37 UTC
[PR] fix(nvd3): invoke shiftMetric() in TimePivot formDataOverrides [superset]
durgaprasadml opened a new pull request, #41916:
URL: https://github.com/apache/superset/pull/41916
### SUMMARY
This PR fixes the **Time-series Period Pivot** chart configuration issue where the required **Metric** control was rendered in a non-interactive state, preventing users from selecting a metric and causing chart execution to fail with the error:
> Metric 'None' does not exist
### Root Cause
The `formDataOverrides` configuration in the TimePivot control panel assigned the `shiftMetric` function itself instead of invoking it.
Before:
```ts
metric: getStandardizedControls().shiftMetric,
```
This stored a function reference as the metric value instead of the transformed metric object expected by the control and query pipeline.
After:
```ts
metric: getStandardizedControls().shiftMetric(),
```
Invoking `shiftMetric()` correctly initializes the metric value, allowing the Metric control to function as expected and ensuring a valid metric is included in the generated query.
### Changes
- Invoke `shiftMetric()` in `TimePivot/controlPanel.ts` instead of assigning the function reference.
- Add a regression test to verify:
- `shiftMetric()` is invoked.
- The returned metric is assigned to `formData`.
- Future regressions are detected during CI.
Fixes #41884
---
### BEFORE
- Metric field appears as required.
- Metric selector is disabled and cannot be interacted with.
- Users cannot add or select a metric.
- Running the query results in:
```
Metric 'None' does not exist
```
### AFTER
- Metric selector is interactive.
- Users can select a metric normally.
- A valid metric is included in the generated query.
- The Time-series Period Pivot chart renders successfully.
---
### TESTING INSTRUCTIONS
1. Start Superset locally.
2. Open **Explore**.
3. Select any dataset.
4. Choose the **Time-series Period Pivot** visualization.
5. Verify that the **Metric** control is enabled.
6. Select a metric.
7. Click **Run**.
8. Confirm that the chart renders successfully without the `Metric 'None' does not exist` error.
9. Run the frontend test suite and verify that the new regression test passes.
---
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #41884
- [ ] Required feature flags
- [x] 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
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(nvd3): invoke shiftMetric() in TimePivot formDataOverrides [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41916:
URL: https://github.com/apache/superset/pull/41916#issuecomment-4931020258
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(nvd3): invoke shiftMetric() in TimePivot formDataOverrides [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #41916:
URL: https://github.com/apache/superset/pull/41916
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(nvd3): invoke shiftMetric() in TimePivot formDataOverrides [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41916:
URL: https://github.com/apache/superset/pull/41916#discussion_r3555768537
##########
superset-frontend/plugins/legacy-preset-chart-nvd3/test/TimePivot/controlPanel.test.ts:
##########
@@ -0,0 +1,55 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { SqlaFormData } from '@superset-ui/core';
+import * as ChartControls from '@superset-ui/chart-controls';
+import controlPanel from '../../src/TimePivot/controlPanel';
+
+const { __mockShiftMetric } = ChartControls as any;
Review Comment:
**Suggestion:** Replace the `any` cast with a concrete typed shape for the mocked chart-controls module so type safety is preserved. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The file uses `as any`, which directly violates the no-any-types rule for changed TypeScript code. A concrete type should be used instead of an untyped cast.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 16)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1e735a258dce4435baf4bd99efb174ed&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1e735a258dce4435baf4bd99efb174ed&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/legacy-preset-chart-nvd3/test/TimePivot/controlPanel.test.ts
**Line:** 23:23
**Comment:**
*Custom Rule: Replace the `any` cast with a concrete typed shape for the mocked chart-controls module so type safety is preserved.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41916&comment_hash=e4e64c0ce1eab9a58ff1f5e6a84cf21cb38a721db84b2894c42221b284f244ff&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41916&comment_hash=e4e64c0ce1eab9a58ff1f5e6a84cf21cb38a721db84b2894c42221b284f244ff&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(nvd3): invoke shiftMetric() in TimePivot formDataOverrides [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41916:
URL: https://github.com/apache/superset/pull/41916#discussion_r3555980985
##########
superset-frontend/plugins/legacy-preset-chart-nvd3/test/TimePivot/controlPanel.test.ts:
##########
@@ -0,0 +1,57 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { SqlaFormData } from '@superset-ui/core';
+import * as ChartControls from '@superset-ui/chart-controls';
+import controlPanel from '../../src/TimePivot/controlPanel';
+
+const { __mockShiftMetric } = ChartControls as typeof ChartControls & {
+ __mockShiftMetric: jest.Mock;
+};
+
+jest.mock('@superset-ui/core', () => ({
+ ...jest.requireActual('@superset-ui/core'),
+ t: (str: string) => str,
+}));
+
+jest.mock('@superset-ui/chart-controls', () => {
+ const original = jest.requireActual('@superset-ui/chart-controls');
+ const mockShiftMetric = jest.fn(() => 'shiftedMetric');
+ return {
+ ...original,
+ getStandardizedControls: () => ({
+ shiftMetric: mockShiftMetric,
+ }),
+ __mockShiftMetric: mockShiftMetric,
+ };
+});
+
+describe('TimePivot Control Panel Config', () => {
+ test('should override formData metric using getStandardizedControls', () => {
+ const dummyFormData = { someProp: 'test' } as unknown as SqlaFormData;
+ const newFormData = controlPanel.formDataOverrides!(dummyFormData);
+
+ // The original properties are spread correctly.
+ expect(newFormData.someProp).toBe('test');
+ // The metric property should be replaced by the output of shiftMetric.
+ expect(newFormData.metric).toBe('shiftedMetric');
+
+ // Ensure that the mockShiftMetric function was called.
+ expect(__mockShiftMetric).toHaveBeenCalled();
+ });
+});
Review Comment:
**Suggestion:** Remove the unnecessary suite wrapper and keep this as a single top-level test case since only one assertion flow is being tested. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This new test file contains only one test case inside a single `describe()` wrapper, so the suggestion correctly identifies the rule violation targeted by llmcfg-use-test-instead-of-describe.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.github/copilot-instructions.md (line 43)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2a74f0814bf142328af323c1d7ffa41c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2a74f0814bf142328af323c1d7ffa41c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/legacy-preset-chart-nvd3/test/TimePivot/controlPanel.test.ts
**Line:** 44:57
**Comment:**
*Custom Rule: Remove the unnecessary suite wrapper and keep this as a single top-level test case since only one assertion flow is being tested.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41916&comment_hash=d2ac7602dc53a42558a64257b9bf7f5db77c74b43e80ad2fd693e25e7af3313d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41916&comment_hash=d2ac7602dc53a42558a64257b9bf7f5db77c74b43e80ad2fd693e25e7af3313d&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(nvd3): invoke shiftMetric() in TimePivot formDataOverrides [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #41916:
URL: https://github.com/apache/superset/pull/41916#issuecomment-4931068999
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/41916?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 64.57%. Comparing base ([`bea5f8d`](https://app.codecov.io/gh/apache/superset/commit/bea5f8d4a446344685cacf2144e790448ac07eb8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`2c3fc7e`](https://app.codecov.io/gh/apache/superset/commit/2c3fc7e6ab25cdeee4f0c9941674ea328dfd131f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #41916 +/- ##
=======================================
Coverage 64.56% 64.57%
=======================================
Files 2718 2718
Lines 151497 151497
Branches 34804 34804
=======================================
+ Hits 97821 97822 +1
+ Misses 51850 51849 -1
Partials 1826 1826
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/41916/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/41916/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `69.78% <100.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/41916?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(nvd3): invoke shiftMetric() in TimePivot formDataOverrides [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41916:
URL: https://github.com/apache/superset/pull/41916#issuecomment-4931032718
<!-- Bito Reply -->
The suggestion to replace the `any` cast with a concrete type is correct and improves type safety. You can resolve this by defining an interface for the mocked module structure instead of using `as any`.
Here is the corrected implementation for the test file:
```typescript
// Replace line 53 with a concrete type definition
interface MockedChartControls {
__mockShiftMetric: jest.Mock;
}
const { __mockShiftMetric } = ChartControls as MockedChartControls;
```
There are no other comments on this PR to address. Would you like me to apply this change to the test file?
**superset-frontend/plugins/legacy-preset-chart-nvd3/test/TimePivot/controlPanel.test.ts**
```
interface MockedChartControls {
__mockShiftMetric: jest.Mock;
}
const { __mockShiftMetric } = ChartControls as MockedChartControls;
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(nvd3): invoke shiftMetric() in TimePivot formDataOverrides [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41916:
URL: https://github.com/apache/superset/pull/41916#issuecomment-4932294119
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org