korbit-ai[bot] commented on code in PR #33126:
URL: https://github.com/apache/superset/pull/33126#discussion_r2042912097
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -486,51 +488,69 @@ const config: ControlPanelConfig = {
return true;
},
mapStateToProps(explore, _, chart) {
- const timeComparisonStatus = !isEmpty(
- explore?.controls?.time_compare?.value,
- );
-
+ const timeComparisonValue =
+ explore?.controls?.time_compare?.value;
const { colnames: _colnames, coltypes: _coltypes } =
chart?.queriesResponse?.[0] ?? {};
let colnames: string[] = _colnames || [];
let coltypes: GenericDataType[] = _coltypes || [];
const childColumnMap: Record<string, boolean> = {};
+ const timeComparisonColumnMap: Record<string, boolean> = {};
- if (timeComparisonStatus) {
+ if (!isEmpty(timeComparisonValue)) {
/**
* Replace numeric columns with sets of comparison columns.
*/
const updatedColnames: string[] = [];
const updatedColtypes: GenericDataType[] = [];
- colnames.forEach((colname, index) => {
- if (coltypes[index] === GenericDataType.Numeric) {
- const comparisonColumns =
- generateComparisonColumns(colname);
- comparisonColumns.forEach((name, idx) => {
- updatedColnames.push(name);
- updatedColtypes.push(
- ...generateComparisonColumnTypes(4),
- );
-
- if (idx === 0 && name.startsWith('Main ')) {
- childColumnMap[name] = false;
- } else {
- childColumnMap[name] = true;
- }
- });
- } else {
- updatedColnames.push(colname);
- updatedColtypes.push(coltypes[index]);
- childColumnMap[colname] = false;
- }
- });
+ colnames
+ .filter(
+ colname =>
+ last(colname.split('__')) !== timeComparisonValue,
+ )
+ .forEach((colname, index) => {
+ if (
+ explore.form_data.metrics?.some(
+ metric => getMetricLabel(metric) === colname,
+ ) ||
+ explore.form_data.percent_metrics?.some(
+ (metric: QueryFormMetric) =>
+ getMetricLabel(metric) === colname,
+ )
+ ) {
Review Comment:
### Complex Nested Metric Check Condition <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Complex nested condition checking metrics and percent_metrics with
duplicated logic makes the code hard to follow.
###### Why this matters
The duplicated logic and nested structure makes it difficult to understand
the condition at a glance and increases cognitive load.
###### Suggested change ∙ *Feature Preview*
```typescript
const isMetricColumn = (colname: string) => {
const { metrics = [], percent_metrics = [] } = explore.form_data;
return [...metrics, ...percent_metrics]
.some(metric => getMetricLabel(metric) === colname);
}
// Usage
if (isMetricColumn(colname)) {
// ... rest of the code
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b307422f-a685-46ad-b3fb-f54c78438bf5/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b307422f-a685-46ad-b3fb-f54c78438bf5?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b307422f-a685-46ad-b3fb-f54c78438bf5?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b307422f-a685-46ad-b3fb-f54c78438bf5?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b307422f-a685-46ad-b3fb-f54c78438bf5)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e77ae596-3541-4188-90b3-83e0e62a5570 -->
[](e77ae596-3541-4188-90b3-83e0e62a5570)
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -486,51 +488,69 @@
return true;
},
mapStateToProps(explore, _, chart) {
- const timeComparisonStatus = !isEmpty(
- explore?.controls?.time_compare?.value,
- );
-
+ const timeComparisonValue =
+ explore?.controls?.time_compare?.value;
const { colnames: _colnames, coltypes: _coltypes } =
chart?.queriesResponse?.[0] ?? {};
let colnames: string[] = _colnames || [];
let coltypes: GenericDataType[] = _coltypes || [];
const childColumnMap: Record<string, boolean> = {};
+ const timeComparisonColumnMap: Record<string, boolean> = {};
- if (timeComparisonStatus) {
+ if (!isEmpty(timeComparisonValue)) {
/**
* Replace numeric columns with sets of comparison columns.
*/
const updatedColnames: string[] = [];
const updatedColtypes: GenericDataType[] = [];
- colnames.forEach((colname, index) => {
- if (coltypes[index] === GenericDataType.Numeric) {
- const comparisonColumns =
- generateComparisonColumns(colname);
- comparisonColumns.forEach((name, idx) => {
- updatedColnames.push(name);
- updatedColtypes.push(
- ...generateComparisonColumnTypes(4),
- );
-
- if (idx === 0 && name.startsWith('Main ')) {
- childColumnMap[name] = false;
- } else {
- childColumnMap[name] = true;
- }
- });
- } else {
- updatedColnames.push(colname);
- updatedColtypes.push(coltypes[index]);
- childColumnMap[colname] = false;
- }
- });
+ colnames
+ .filter(
+ colname =>
+ last(colname.split('__')) !== timeComparisonValue,
+ )
+ .forEach((colname, index) => {
+ if (
+ explore.form_data.metrics?.some(
+ metric => getMetricLabel(metric) === colname,
+ ) ||
+ explore.form_data.percent_metrics?.some(
+ (metric: QueryFormMetric) =>
+ getMetricLabel(metric) === colname,
+ )
+ ) {
+ const comparisonColumns =
+ generateComparisonColumns(colname);
+ comparisonColumns.forEach((name, idx) => {
+ updatedColnames.push(name);
+ updatedColtypes.push(
+ ...generateComparisonColumnTypes(4),
+ );
+ timeComparisonColumnMap[name] = true;
+ if (idx === 0 && name.startsWith('Main ')) {
+ childColumnMap[name] = false;
+ } else {
+ childColumnMap[name] = true;
+ }
Review Comment:
### Complex MapStateToProps Function <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The mapStateToProps function is too large and handles multiple
responsibilities, making it difficult to maintain and test.
###### Why this matters
Complex functions with multiple responsibilities violate the Single
Responsibility Principle and make the code harder to understand, maintain, and
test. This increases the likelihood of bugs when modifications are needed.
###### Suggested change ∙ *Feature Preview*
Extract the column processing logic into separate functions:
```typescript
const processTimeComparisonColumns = (colnames: string[], coltypes:
GenericDataType[], timeComparisonValue: string, metrics: any[]) => {
// Handle time comparison column processing
return { updatedColnames, updatedColtypes, childColumnMap,
timeComparisonColumnMap };
};
const mapStateToProps = (explore, _, chart) => {
const timeComparisonValue = explore?.controls?.time_compare?.value;
const { colnames, coltypes } = chart?.queriesResponse?.[0] ?? {};
return {
columnsPropsObject: !isEmpty(timeComparisonValue)
? processTimeComparisonColumns(colnames, coltypes,
timeComparisonValue, explore.form_data.metrics)
: { colnames, coltypes, childColumnMap: {}, timeComparisonColumnMap:
{} }
};
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ca1a8f4-8600-4c45-be70-884e45bcf071/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ca1a8f4-8600-4c45-be70-884e45bcf071?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ca1a8f4-8600-4c45-be70-884e45bcf071?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ca1a8f4-8600-4c45-be70-884e45bcf071?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ca1a8f4-8600-4c45-be70-884e45bcf071)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a21e6aeb-c5fc-444e-9e81-86071af1826f -->
[](a21e6aeb-c5fc-444e-9e81-86071af1826f)
--
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]
