codeant-ai-for-open-source[bot] commented on PR #36849: URL: https://github.com/apache/superset/pull/36849#issuecomment-3694732668
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/36849/files#diff-12adcc0dab4417a345f763565aee3cd0f657eece6142615492172abe69cfd3bdR33-R44'><strong>Input Validation</strong></a><br>The new helper `computeCollapsedMap` accepts `keys` and `depth` but does not validate that `keys` is an array of arrays nor that `depth` is a positive integer. Unexpected inputs (null, non-array keys, non-integer or out-of-range depth) could lead to no-op behavior or inadvertent state mutations. Consider validating inputs and normalizing `depth`.<br> - [ ] <a href='https://github.com/apache/superset/pull/36849/files#diff-380ab7f1cfa9e06b6c86b781ab6bd0f65be79a8a40125415363e270690e5a9d1R110-R111'><strong>Missing validation</strong></a><br>The new `defaultRowExpansionDepth` and `defaultColExpansionDepth` values are passed through directly from `formData` with no normalization or clamping. If these values are undefined, non-numeric, negative, or excessively large, downstream code that uses them to seed initial expanded/collapsed state may behave unexpectedly or suffer performance issues.<br> - [ ] <a href='https://github.com/apache/superset/pull/36849/files#diff-380ab7f1cfa9e06b6c86b781ab6bd0f65be79a8a40125415363e270690e5a9d1R186-R187'><strong>Potential performance impact</strong></a><br>Unbounded expansion depth values could cause heavy initial work when computing collapse/expand maps (e.g., iterating very deep trees). It's safer to cap the depth to a reasonable maximum (or to the available hierarchy depth) before returning the props.<br> - [ ] <a href='https://github.com/apache/superset/pull/36849/files#diff-12adcc0dab4417a345f763565aee3cd0f657eece6142615492172abe69cfd3bdR33-R44'><strong>Implicit-parent collapse</strong></a><br>The current implementation of `computeCollapsedMap` only collapses keys whose length exactly equals `depth` (it filters with `k.length === depth`). If parent keys are implicit (not present in `keys`) but only deeper child keys exist (e.g., only ['A','X','1'] and ['A','X','2']), the parent key ['A','X'] will not be collapsed even though the UI probably expects it to be. Add tests and/or update the function to derive parent keys from longer keys (slice first `depth` elements) so implicit parents are collapsed.<br> - [ ] <a href='https://github.com/apache/superset/pull/36849/files#diff-12adcc0dab4417a345f763565aee3cd0f657eece6142615492172abe69cfd3bdR107-R156'><strong>Initial state application</strong></a><br>`componentDidMount` calls `this.getBasePivotSettings()` which constructs a new `PivotData` and computes keys. This duplicates work done in render/caching and could be expensive. Also, `setState(updates)` may write empty `{}` collapsed maps when `computeCollapsedMap` returns empty objects — this can overwrite any prior state unintentionally. Consider reusing cached settings (if present) and only set state when collapsed maps are non-empty.<br> </td></tr> </table> -- 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]
