codeant-ai-for-open-source[bot] commented on code in PR #38865:
URL: https://github.com/apache/superset/pull/38865#discussion_r2994408549


##########
superset-frontend/src/dataMask/reducer.ts:
##########
@@ -245,35 +246,39 @@ const dataMaskReducer = produce(
           if (!isChartCustomizationItem(item)) {
             return;
           }
+          if (isDynamicTitleCustomization(item)) {
+            return;
+          }
+          const customizationItem = item as ChartCustomization;
 
-          const customizationFilterId = item.id;
+          const customizationFilterId = customizationItem.id;
           const dataMask = loadedDataMask || {};
 
           cleanState[customizationFilterId] = {
             ...getInitialDataMask(customizationFilterId),
-            ...item.defaultDataMask,
+            ...customizationItem.defaultDataMask,
             ...dataMask[customizationFilterId],
           };
 
           if (
             draft[customizationFilterId] &&
-            item.defaultDataMask &&
+            customizationItem.defaultDataMask &&
             !areObjectsEqual(
-              item.defaultDataMask,
+              customizationItem.defaultDataMask,
               draft[customizationFilterId],
               { ignoreUndefined: true },
             )
           ) {
             cleanState[customizationFilterId] = {
               ...cleanState[customizationFilterId],
-              ...item.defaultDataMask,
+              ...customizationItem.defaultDataMask,
             };
           }

Review Comment:
   **Suggestion:** The `if` block that compares 
`customizationItem.defaultDataMask` to `draft[customizationFilterId]` and then 
re-applies `defaultDataMask` will almost always evaluate as "not equal", 
causing default values to be reapplied over any persisted data mask for that 
customization on every hydrate, effectively discarding saved user state for 
chart customizations. [logic error]
   
   <details>
   <summary><b>Severity Level:</b> Major ⚠️</summary>
   
   ```mdx
   - ❌ Chart customization plugins may lose persisted state on hydrate.
   - ⚠️ Users can see default customization instead of last selection.
   ```
   </details>
   
   ```suggestion
             // customizationItem.defaultDataMask is already applied above when 
initializing cleanState
   ```
   <details>
   <summary><b>Steps of Reproduction ✅ </b></summary>
   
   ```mdx
   1. Open any dashboard in the UI, which triggers `hydrateDashboard` in
   `src/dashboard/actions/hydrate.ts:104-113`, passing `dataMask` from the 
backend and
   `dashboard.metadata.chart_customization_config` into the Redux store.
   
   2. Ensure the dashboard metadata contains at least one non-dynamic-title 
chart
   customization with an `id` and a `defaultDataMask` (these customizations are 
read via
   `selectChartCustomizationConfiguration` in
   `src/dashboard/components/nativeFilters/state.ts:98-125`).
   
   3. Dispatch the `HYDRATE_DASHBOARD` action so `dataMaskReducer` in
   `src/dataMask/reducer.ts:200-219` runs its `HYDRATE_DASHBOARD` case, iterates
   `chartCustomizationConfig`, and initializes 
`cleanState[customizationFilterId]` with
   `getInitialDataMask`, `customizationItem.defaultDataMask`, and the backend
   `loadedDataMask[customizationFilterId]` (lines 55-62).
   
   4. When there is already a `draft[customizationFilterId]` entry (previous 
in-memory
   `DataMask` for that customization), the condition at 
`src/dataMask/reducer.ts:64-71`
   compares `customizationItem.defaultDataMask` against the full
   `draft[customizationFilterId]` object; this almost always fails equality and 
re-applies
   `defaultDataMask` over `cleanState` at lines 73-76, overriding any 
overlapping fields from
   `loadedDataMask[customizationFilterId]` and effectively discarding persisted 
state for
   that customization on hydrate.
   ```
   </details>
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset-frontend/src/dataMask/reducer.ts
   **Line:** 263:276
   **Comment:**
        *Logic Error: The `if` block that compares 
`customizationItem.defaultDataMask` to `draft[customizationFilterId]` and then 
re-applies `defaultDataMask` will almost always evaluate as "not equal", 
causing default values to be reapplied over any persisted data mask for that 
customization on every hydrate, effectively discarding saved user state for 
chart customizations.
   
   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.
   ```
   </details>
   <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38865&comment_hash=721a49fbb6a9d16aa083ace92ce0f8fe26979b2d46b4d0fdc531258f1cff38e5&reaction=like'>👍</a>
 | <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38865&comment_hash=721a49fbb6a9d16aa083ace92ce0f8fe26979b2d46b4d0fdc531258f1cff38e5&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]

Reply via email to