Re: [PR] fix(ag-grid): keep basic conditional formatting aligned after sort [superset]

["bito-code-review[bot] (via GitHub)" <gi...@apache.org>]

bito-code-review[bot] commented on PR #41390:
URL: https://github.com/apache/superset/pull/41390#issuecomment-4902580159

   <!-- Bito Reply -->
   The flagged issue is correct. Using a fixed string key for `Object.defineProperty` can indeed cause collisions if the user's dataset already contains a field named `__basicColorFormatters__`. To resolve this, you should use a `Symbol` as the key, which is guaranteed to be unique and will not collide with any user-defined string keys.
   
   Here is the corrected implementation for `superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts`:
   
   ```typescript
   // Define a unique symbol for the formatter key
   export const BASIC_COLOR_FORMATTERS_ROW_KEY = Symbol('basicColorFormatters');
   
   // ... inside transformProps ...
     if (basicColorFormatters) {
       passedData.forEach((row, index) => {
         Object.defineProperty(row, BASIC_COLOR_FORMATTERS_ROW_KEY, {
           value: basicColorFormatters[index],
           enumerable: false,
           configurable: true,
           writable: true,
         });
       });
     }
   ```
   
   I have checked the PR comments, but there are no other comments available in the provided context to address. If you have other comments you would like me to review, please provide them.
   
   **superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts**
   ```
   if (basicColorFormatters) {
       passedData.forEach((row, index) => {
         Object.defineProperty(row, BASIC_COLOR_FORMATTERS_ROW_KEY, {
           value: basicColorFormatters[index],
           enumerable: false,
           configurable: true,
           writable: true,
         });
       });
     }
   ```


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