You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "kgabryje (via GitHub)" <gi...@apache.org> on 2025/01/29 12:14:06 UTC

[PR] fix: Filters badge disappeared [superset]

kgabryje opened a new pull request, #32025:
URL: https://github.com/apache/superset/pull/32025

   ### SUMMARY
   Fix filters badge not appearring after adding a native filter
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   Before:
   
   ![image](https://github.com/user-attachments/assets/ddb1fcbd-363a-4fca-a60d-cd1731cf2a64)
   
   After:
   
   ![image](https://github.com/user-attachments/assets/da783a41-e7ea-4039-98a6-3f3e023843b8)
   
   
   ### TESTING INSTRUCTIONS
   1. Add a native filter and set some value
   2. Verify that the charts in scope of that filter have a filters badge
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] 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: Filters badge disappeared [superset]

Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32025:
URL: https://github.com/apache/superset/pull/32025#discussion_r1933775498


##########
superset-frontend/src/dashboard/components/FiltersBadge/index.tsx:
##########
@@ -175,10 +175,10 @@ export const FiltersBadge = ({ chartId }: FiltersBadgeProps) => {
       setDashboardIndicators(indicatorsInitialState);
     } else if (prevChartStatus !== 'success') {
       if (
-        chart?.queriesResponse?.rejected_filters !==
-          prevChart?.queriesResponse?.rejected_filters ||
-        chart?.queriesResponse?.applied_filters !==
-          prevChart?.queriesResponse?.applied_filters ||
+        chart?.queriesResponse?.[0]?.rejected_filters !==
+          prevChart?.queriesResponse?.[0]?.rejected_filters ||
+        chart?.queriesResponse?.[0]?.applied_filters !==
+          prevChart?.queriesResponse?.[0]?.applied_filters ||

Review Comment:
   ### Missing empty array check for queriesResponse <sub>![category Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The code assumes queriesResponse array will always have at least one element without proper validation, which could lead to runtime errors if the array is empty.
   
   ###### Why this matters
   Accessing index [0] of an empty array will return undefined and could cause the component to fail silently or behave incorrectly when comparing filters.
   
   ###### Suggested change ∙ *Feature Preview*
   Add a length check before accessing the first element:
   ```typescript
   if (
           (chart?.queriesResponse?.length > 0 &&
             chart.queriesResponse[0]?.rejected_filters) !==
             (prevChart?.queriesResponse?.length > 0 &&
               prevChart.queriesResponse[0]?.rejected_filters) ||
           (chart?.queriesResponse?.length > 0 &&
             chart.queriesResponse[0]?.applied_filters) !==
             (prevChart?.queriesResponse?.length > 0 &&
               prevChart.queriesResponse[0]?.applied_filters)
   ```
   
   
   </details>
   
   <sub>💡 Does this comment miss the mark? [Tell us why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2020b553-6612-4dd9-bf9b-f4d799eac9cf?suggestedFixEnabled=true) and Korbit will adapt to your team’s feedback.
   💬 Chat with Korbit by mentioning @korbit-ai.
   </sub>
   
   <!--- korbi internal id:5755d8e1-e446-40a7-af3a-ad09afb94c61 -->
   



-- 
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: Filters badge disappeared [superset]

Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #32025:
URL: https://github.com/apache/superset/pull/32025#discussion_r1933877396


##########
superset-frontend/src/dashboard/components/FiltersBadge/index.tsx:
##########
@@ -175,10 +175,10 @@ export const FiltersBadge = ({ chartId }: FiltersBadgeProps) => {
       setDashboardIndicators(indicatorsInitialState);
     } else if (prevChartStatus !== 'success') {
       if (
-        chart?.queriesResponse?.rejected_filters !==
-          prevChart?.queriesResponse?.rejected_filters ||
-        chart?.queriesResponse?.applied_filters !==
-          prevChart?.queriesResponse?.applied_filters ||
+        chart?.queriesResponse?.[0]?.rejected_filters !==
+          prevChart?.queriesResponse?.[0]?.rejected_filters ||
+        chart?.queriesResponse?.[0]?.applied_filters !==
+          prevChart?.queriesResponse?.[0]?.applied_filters ||

Review Comment:
   You're correct. Python and Java throws errors for this use case but Javascript does not.



-- 
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: Filters badge disappeared [superset]

Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #32025:
URL: https://github.com/apache/superset/pull/32025#discussion_r1933866961


##########
superset-frontend/src/dashboard/components/FiltersBadge/index.tsx:
##########
@@ -175,10 +175,10 @@ export const FiltersBadge = ({ chartId }: FiltersBadgeProps) => {
       setDashboardIndicators(indicatorsInitialState);
     } else if (prevChartStatus !== 'success') {
       if (
-        chart?.queriesResponse?.rejected_filters !==
-          prevChart?.queriesResponse?.rejected_filters ||
-        chart?.queriesResponse?.applied_filters !==
-          prevChart?.queriesResponse?.applied_filters ||
+        chart?.queriesResponse?.[0]?.rejected_filters !==
+          prevChart?.queriesResponse?.[0]?.rejected_filters ||
+        chart?.queriesResponse?.[0]?.applied_filters !==
+          prevChart?.queriesResponse?.[0]?.applied_filters ||

Review Comment:
   If it's an empty array, then `queriesResponse[0]` should return undefined, which is handled by `[0]?.`. Or am I missing something?



-- 
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: Filters badge disappeared [superset]

Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje merged PR #32025:
URL: https://github.com/apache/superset/pull/32025


-- 
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: Filters badge disappeared [superset]

Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #32025:
URL: https://github.com/apache/superset/pull/32025#discussion_r1933819965


##########
superset-frontend/src/dashboard/components/FiltersBadge/index.tsx:
##########
@@ -175,10 +175,10 @@ export const FiltersBadge = ({ chartId }: FiltersBadgeProps) => {
       setDashboardIndicators(indicatorsInitialState);
     } else if (prevChartStatus !== 'success') {
       if (
-        chart?.queriesResponse?.rejected_filters !==
-          prevChart?.queriesResponse?.rejected_filters ||
-        chart?.queriesResponse?.applied_filters !==
-          prevChart?.queriesResponse?.applied_filters ||
+        chart?.queriesResponse?.[0]?.rejected_filters !==
+          prevChart?.queriesResponse?.[0]?.rejected_filters ||
+        chart?.queriesResponse?.[0]?.applied_filters !==
+          prevChart?.queriesResponse?.[0]?.applied_filters ||

Review Comment:
   @kgabryje I believe Korbit is correct here. It's complaining that `chart?.queriesResponse` might be an empty array which is not handled by a null check.



-- 
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: Filters badge disappeared [superset]

Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #32025:
URL: https://github.com/apache/superset/pull/32025#discussion_r1933812755


##########
superset-frontend/src/dashboard/components/FiltersBadge/index.tsx:
##########
@@ -175,10 +175,10 @@ export const FiltersBadge = ({ chartId }: FiltersBadgeProps) => {
       setDashboardIndicators(indicatorsInitialState);
     } else if (prevChartStatus !== 'success') {
       if (
-        chart?.queriesResponse?.rejected_filters !==
-          prevChart?.queriesResponse?.rejected_filters ||
-        chart?.queriesResponse?.applied_filters !==
-          prevChart?.queriesResponse?.applied_filters ||
+        chart?.queriesResponse?.[0]?.rejected_filters !==
+          prevChart?.queriesResponse?.[0]?.rejected_filters ||
+        chart?.queriesResponse?.[0]?.applied_filters !==
+          prevChart?.queriesResponse?.[0]?.applied_filters ||

Review Comment:
   Handled with null check



-- 
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: Filters badge disappeared [superset]

Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #32025:
URL: https://github.com/apache/superset/pull/32025#discussion_r1933877396


##########
superset-frontend/src/dashboard/components/FiltersBadge/index.tsx:
##########
@@ -175,10 +175,10 @@ export const FiltersBadge = ({ chartId }: FiltersBadgeProps) => {
       setDashboardIndicators(indicatorsInitialState);
     } else if (prevChartStatus !== 'success') {
       if (
-        chart?.queriesResponse?.rejected_filters !==
-          prevChart?.queriesResponse?.rejected_filters ||
-        chart?.queriesResponse?.applied_filters !==
-          prevChart?.queriesResponse?.applied_filters ||
+        chart?.queriesResponse?.[0]?.rejected_filters !==
+          prevChart?.queriesResponse?.[0]?.rejected_filters ||
+        chart?.queriesResponse?.[0]?.applied_filters !==
+          prevChart?.queriesResponse?.[0]?.applied_filters ||

Review Comment:
   You're correct. Python and Java throw errors for this use case but Javascript does not.



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