codeant-ai-for-open-source[bot] commented on code in PR #41350:
URL: https://github.com/apache/superset/pull/41350#discussion_r3569841592
##########
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:
##########
@@ -821,3 +822,89 @@ test('temporal x coltype wires the time formatter and Time
axis', () => {
expect(typeof label).toBe('string');
expect(label).not.toMatch(/NaN/);
});
+
+describe('Tooltip time grain wiring', () => {
Review Comment:
**Suggestion:** Flatten this new grouped block into standalone `test()`
cases unless shared setup is truly required, to comply with the
no-new-`describe()` rule. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The added block introduces a new `describe()` wrapper around only two tests,
with no clear shared setup that requires grouping. This fits the rule against
introducing `describe()` when plain `test()` cases would suffice.
</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=86c5501db3484e65b23bc4894f230531&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=86c5501db3484e65b23bc4894f230531&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/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts
**Line:** 826:826
**Comment:**
*Custom Rule: Flatten this new grouped block into standalone `test()`
cases unless shared setup is truly required, to comply with the
no-new-`describe()` rule.
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%2F41350&comment_hash=cdcd77ee7aa21d8cfff75048f9ee83a3f5746dc6c82103e6e5396c2ef13f3e4f&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41350&comment_hash=cdcd77ee7aa21d8cfff75048f9ee83a3f5746dc6c82103e6e5396c2ef13f3e4f&reaction=dislike'>๐</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:
##########
@@ -821,3 +822,89 @@ test('temporal x coltype wires the time formatter and Time
axis', () => {
expect(typeof label).toBe('string');
expect(label).not.toMatch(/NaN/);
});
+
+describe('Tooltip time grain wiring', () => {
+ test('dashboard-level extraFormData time grain overrides the chart-level
grain in the tooltip', () => {
+ const ts = Date.UTC(2021, 0, 7);
+ const temporalRows = [{ __timestamp: ts, metric: 100 }];
+ const temporalQueryData = createTestQueryData(temporalRows, {
+ colnames: ['__timestamp', 'metric'],
+ coltypes: [GenericDataType.Temporal, GenericDataType.Numeric],
+ label_map: { __timestamp: ['__timestamp'], metric: ['metric'] },
+ });
+
+ const chartProps = createEchartsTimeseriesTestChartProps<
+ EchartsMixedTimeseriesFormData,
+ EchartsMixedTimeseriesProps
+ >({
+ ...MIXED_TIMESERIES_CHART_PROPS_DEFAULTS,
+ defaultQueriesData: [temporalQueryData, temporalQueryData],
+ formData: {
+ ...formData,
+ x_axis: '__timestamp',
+ metrics: ['metric'],
+ metricsB: ['metric'],
+ groupby: [],
+ groupbyB: [],
+ // The chart itself is configured with a Day grain...
+ timeGrainSqla: TimeGranularity.DAY,
+ // ...but a dashboard-level filter/override resolves to Month.
+ extraFormData: { time_grain_sqla: TimeGranularity.MONTH },
+ },
+ queriesData: [temporalQueryData, temporalQueryData],
+ });
+
+ const { echartOptions } = transformProps(chartProps);
+ const tooltipFormatter = (echartOptions as any).tooltip.formatter;
+
+ const result = tooltipFormatter({
+ value: [ts, 100],
+ seriesName: 'metric',
+ });
+
+ // Month grain (the dashboard override) should win, so the tooltip title
+ // reads "Jan 2021" rather than the Day-grain "2021-01-07".
+ expect(result).toContain('Jan');
+ expect(result).toContain('2021');
+ expect(result).not.toContain('2021-01-07');
+ });
+
+ test('chart-level time grain drives the tooltip when there is no dashboard
override', () => {
+ const ts = Date.UTC(2021, 0, 7);
+ const temporalRows = [{ __timestamp: ts, metric: 100 }];
+ const temporalQueryData = createTestQueryData(temporalRows, {
+ colnames: ['__timestamp', 'metric'],
+ coltypes: [GenericDataType.Temporal, GenericDataType.Numeric],
+ label_map: { __timestamp: ['__timestamp'], metric: ['metric'] },
+ });
+
+ const chartProps = createEchartsTimeseriesTestChartProps<
+ EchartsMixedTimeseriesFormData,
+ EchartsMixedTimeseriesProps
+ >({
+ ...MIXED_TIMESERIES_CHART_PROPS_DEFAULTS,
+ defaultQueriesData: [temporalQueryData, temporalQueryData],
+ formData: {
+ ...formData,
+ x_axis: '__timestamp',
+ metrics: ['metric'],
+ metricsB: ['metric'],
+ groupby: [],
+ groupbyB: [],
+ timeGrainSqla: TimeGranularity.YEAR,
+ },
+ queriesData: [temporalQueryData, temporalQueryData],
+ });
+
+ const { echartOptions } = transformProps(chartProps);
+ const tooltipFormatter = (echartOptions as any).tooltip.formatter;
Review Comment:
**Suggestion:** Replace the second `any` cast with a concrete typed shape
for `echartOptions.tooltip` to preserve type safety in this new test path.
[custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is the same `any` usage as above, just in the second new test. It is a
direct TypeScript `any` cast in changed code, so it violates the no-any-types
rule.
</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=23f0429d2fed42afad1ff8d3c86e7caa&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=23f0429d2fed42afad1ff8d3c86e7caa&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/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts
**Line:** 900:900
**Comment:**
*Custom Rule: Replace the second `any` cast with a concrete typed shape
for `echartOptions.tooltip` to preserve type safety in this new test path.
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%2F41350&comment_hash=52aed38e98883fcb00ddeb3c063e0e06be6c9e40f3f7e9318dfced0c33e1cc20&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41350&comment_hash=52aed38e98883fcb00ddeb3c063e0e06be6c9e40f3f7e9318dfced0c33e1cc20&reaction=dislike'>๐</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -1799,3 +1799,69 @@ describe('Tooltip with long labels', () => {
expect(result).toContain('599616000000');
});
});
+
+describe('Tooltip time grain wiring', () => {
+ test('dashboard-level extraFormData time grain overrides the chart-level
grain in the tooltip', () => {
+ const ts = Date.UTC(2021, 0, 7);
+ const chartProps = createTestChartProps({
+ formData: {
+ granularity_sqla: 'ds',
+ richTooltip: false,
+ // The chart itself is configured with a Day grain...
+ timeGrainSqla: TimeGranularity.DAY,
+ // ...but a dashboard-level filter/override resolves to Month.
+ extraFormData: { time_grain_sqla: TimeGranularity.MONTH },
+ },
+ queriesData: [
+ createTestQueryData([{ __timestamp: ts, sales: 100 }], {
+ colnames: ['__timestamp', 'sales'],
+ coltypes: [GenericDataType.Temporal, GenericDataType.Numeric],
+ }),
+ ],
+ });
+
+ const transformedProps = transformProps(chartProps);
+ const tooltipFormatter = (transformedProps.echartOptions as any).tooltip
+ .formatter;
+
+ const result = tooltipFormatter({
+ value: [ts, 100],
+ seriesName: 'sales',
+ });
+
+ // Month grain (the dashboard override) should win, so the tooltip title
+ // reads "Jan 2021" rather than the Day-grain "2021-01-07".
+ expect(result).toContain('Jan');
+ expect(result).toContain('2021');
+ expect(result).not.toContain('2021-01-07');
+ });
+
+ test('chart-level time grain drives the tooltip when there is no dashboard
override', () => {
+ const ts = Date.UTC(2021, 0, 7);
+ const chartProps = createTestChartProps({
+ formData: {
+ granularity_sqla: 'ds',
+ richTooltip: false,
+ timeGrainSqla: TimeGranularity.YEAR,
+ },
+ queriesData: [
+ createTestQueryData([{ __timestamp: ts, sales: 100 }], {
+ colnames: ['__timestamp', 'sales'],
+ coltypes: [GenericDataType.Temporal, GenericDataType.Numeric],
+ }),
+ ],
+ });
+
+ const transformedProps = transformProps(chartProps);
+ const tooltipFormatter = (transformedProps.echartOptions as any).tooltip
+ .formatter;
+
+ const result = tooltipFormatter({
+ value: [ts, 100],
+ seriesName: 'sales',
+ });
+
+ expect(result).toContain('2021');
+ expect(result).not.toContain('2021-01-07');
+ });
+});
Review Comment:
**Suggestion:** Remove the new `describe()` wrapper and keep these as direct
`test()` cases unless nested grouping is strictly required. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The PR adds a new `describe()` wrapper around only two `test()` cases in a
test file; that is exactly the pattern targeted by the
use-test-instead-of-describe rule.
</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=92ccd3874c6048979d70c81d7d426040&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=92ccd3874c6048979d70c81d7d426040&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/plugin-chart-echarts/test/Timeseries/transformProps.test.ts
**Line:** 1803:1867
**Comment:**
*Custom Rule: Remove the new `describe()` wrapper and keep these as
direct `test()` cases unless nested grouping is strictly required.
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%2F41350&comment_hash=8b03e862d499c2d4a39010779bda95ee2b90a295fb72ba8e5f9a95e5e6f86cd8&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41350&comment_hash=8b03e862d499c2d4a39010779bda95ee2b90a295fb72ba8e5f9a95e5e6f86cd8&reaction=dislike'>๐</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:
##########
@@ -821,3 +822,89 @@ test('temporal x coltype wires the time formatter and Time
axis', () => {
expect(typeof label).toBe('string');
expect(label).not.toMatch(/NaN/);
});
+
+describe('Tooltip time grain wiring', () => {
+ test('dashboard-level extraFormData time grain overrides the chart-level
grain in the tooltip', () => {
+ const ts = Date.UTC(2021, 0, 7);
+ const temporalRows = [{ __timestamp: ts, metric: 100 }];
+ const temporalQueryData = createTestQueryData(temporalRows, {
+ colnames: ['__timestamp', 'metric'],
+ coltypes: [GenericDataType.Temporal, GenericDataType.Numeric],
+ label_map: { __timestamp: ['__timestamp'], metric: ['metric'] },
+ });
+
+ const chartProps = createEchartsTimeseriesTestChartProps<
+ EchartsMixedTimeseriesFormData,
+ EchartsMixedTimeseriesProps
+ >({
+ ...MIXED_TIMESERIES_CHART_PROPS_DEFAULTS,
+ defaultQueriesData: [temporalQueryData, temporalQueryData],
+ formData: {
+ ...formData,
+ x_axis: '__timestamp',
+ metrics: ['metric'],
+ metricsB: ['metric'],
+ groupby: [],
+ groupbyB: [],
+ // The chart itself is configured with a Day grain...
+ timeGrainSqla: TimeGranularity.DAY,
+ // ...but a dashboard-level filter/override resolves to Month.
+ extraFormData: { time_grain_sqla: TimeGranularity.MONTH },
+ },
+ queriesData: [temporalQueryData, temporalQueryData],
+ });
+
+ const { echartOptions } = transformProps(chartProps);
+ const tooltipFormatter = (echartOptions as any).tooltip.formatter;
Review Comment:
**Suggestion:** Replace the `any` cast with a concrete tooltip option type
(or a narrowly typed interface) so formatter access stays type-safe.
[custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The rule forbids new or changed TypeScript code using `any`. This line
explicitly casts `echartOptions` to `any`, so the violation is real and
directly matches the rule.
</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=08c99103c23a458fa1c24ca6c3bb522e&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=08c99103c23a458fa1c24ca6c3bb522e&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/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts
**Line:** 858:858
**Comment:**
*Custom Rule: Replace the `any` cast with a concrete tooltip option
type (or a narrowly typed interface) so formatter access stays type-safe.
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%2F41350&comment_hash=9adc281a776187e1d26bd43a069bcae393d7892d2c75bf76e0849a8043666a64&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41350&comment_hash=9adc281a776187e1d26bd43a069bcae393d7892d2c75bf76e0849a8043666a64&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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
