Re: [PR] fix(chart): prevent duplicate measures when switching between Line and Waterfall charts [superset]

Mon, 21 Jul 2025 04:53:43 -0700 


korbit-ai[bot] commented on code in PR #34250:
URL: https://github.com/apache/superset/pull/34250#discussion_r2218974213


##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx:
##########
@@ -160,6 +161,11 @@ const config: ControlPanelConfig = {
       multi: false,
     },
   },
+  formDataOverrides: formData => ({
+    ...formData,
+    metric: getStandardizedControls().shiftMetric(), // 标准化 metric 字段
+    groupby: getStandardizedControls().popAllColumns(), // 标准化 groupby 字段

Review Comment:
   ### Inconsistent Comment Language <sub>![category 
Readability](https://img.shields.io/badge/Readability-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   Comments are written in Chinese (标准化 metric 字段, 标准化 groupby 字段) while the 
rest of the codebase is in English
   
   
   ###### Why this matters
   Mixed language comments make the code harder to understand for team members 
who don't read Chinese and breaks consistency with the rest of the English 
codebase
   
   ###### Suggested change ∙ *Feature Preview*
   ```typescript
       metric: getStandardizedControls().shiftMetric(), // standardize metric 
field
       groupby: getStandardizedControls().popAllColumns(), // standardize 
groupby field
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8fd8c361-a3e1-45e1-bb96-a5bbefc0fc3e/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8fd8c361-a3e1-45e1-bb96-a5bbefc0fc3e?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8fd8c361-a3e1-45e1-bb96-a5bbefc0fc3e?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8fd8c361-a3e1-45e1-bb96-a5bbefc0fc3e?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8fd8c361-a3e1-45e1-bb96-a5bbefc0fc3e)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:e1e5bdad-11e0-43bd-a66e-afbe0fb19891 -->
   
   
   [](e1e5bdad-11e0-43bd-a66e-afbe0fb19891)



##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx:
##########
@@ -160,6 +161,11 @@ const config: ControlPanelConfig = {
       multi: false,
     },
   },
+  formDataOverrides: formData => ({
+    ...formData,
+    metric: getStandardizedControls().shiftMetric(), // 标准化 metric 字段
+    groupby: getStandardizedControls().popAllColumns(), // 标准化 groupby 字段
+  }),

Review Comment:
   ### Inefficient Standardized Controls Instance Creation <sub>![category 
Design](https://img.shields.io/badge/Design-0d9488)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The code creates a new instance of standardized controls on every call to 
formDataOverrides, which is inefficient and violates the DRY principle.
   
   
   ###### Why this matters
   Creating new instances unnecessarily can impact performance and memory 
usage, especially if this override function is called frequently during form 
updates.
   
   ###### Suggested change ∙ *Feature Preview*
   Extract the standardized controls creation to a constant outside the 
formDataOverrides function:
   ```typescript
   const standardizedControls = getStandardizedControls();
   
   const config: ControlPanelConfig = {
     // ...
     formDataOverrides: formData => ({
       ...formData,
       metric: standardizedControls.shiftMetric(),
       groupby: standardizedControls.popAllColumns(),
     }),
   };
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6bc5de3b-a871-4ef8-8ba8-fb719028ea3f/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6bc5de3b-a871-4ef8-8ba8-fb719028ea3f?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6bc5de3b-a871-4ef8-8ba8-fb719028ea3f?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6bc5de3b-a871-4ef8-8ba8-fb719028ea3f?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6bc5de3b-a871-4ef8-8ba8-fb719028ea3f)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:cbbc8c91-bf9f-480a-be59-766a93258349 -->
   
   
   [](cbbc8c91-bf9f-480a-be59-766a93258349)



-- 
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]

Reply via email to