codeant-ai-for-open-source[bot] commented on PR #36344: URL: https://github.com/apache/superset/pull/36344#issuecomment-3604728809
## 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/36344/files#diff-8fd79e6d48d6232814ec9bdd08e115d078b6fa7f28cdecb0ef2cb44e087fe64eR37-R43'><strong>Possible Bug</strong></a><br>The internal `chartColumns` state is initialized from the incoming `value` prop but is never updated when `value` changes after mount. If the control framework or parent component updates `value` externally (e.g., resetting controls, loading a saved chart, or applying undo/redo), this component will keep showing the stale local copy and emit outdated data via `onChange`. This controlled vs. uncontrolled mismatch should be validated.<br> - [ ] <a href='https://github.com/apache/superset/pull/36344/files#diff-970f94aae8a1e1253f3cf6dffcfc355bb6f0ec79a39dce9926a9abfd772a6de8R113-R227'><strong>Possible Bug</strong></a><br>Null metric values are currently coerced to 0 in both the chart data and the tooltip, which can misrepresent missing data as actual zeros. This may violate the intended behavior of "filtering nulls before drawing charts" and could confuse users interpreting the visualization.<br> - [ ] <a href='https://github.com/apache/superset/pull/36344/files#diff-fdfa9f8e7f7148dfadcc8118f16dfdfbdc471a9a29131b054436371dd1a53b5fR65-R92'><strong>Scale Domain Edge Case</strong></a><br>When all values in the input data (or effectively after bounds are applied) are identical, the computed scale domain will have equal min and max, which can cause rendering issues or warnings in scale/plot libraries that expect a non-zero domain range. It would be good to confirm how the downstream charting components behave in this situation and potentially guard against a zero-length domain.<br> - [ ] <a href='https://github.com/apache/superset/pull/36344/files#diff-f81846cf1629aa98607b19561a1dfd9b9afd33be277586ac69ab9a0739f124c8R38-R54'><strong>Possible Bug</strong></a><br>`parseArrayValue` always returns a numeric array (including `[]` for unsupported inputs), so the `if (!Array.isArray(value))` guard is never triggered. This likely defeats the intent of returning "N/A" for non-numeric or invalid row data and can lead to silently rendering empty sparklines instead of a clearer fallback.<br> - [ ] <a href='https://github.com/apache/superset/pull/36344/files#diff-5cd906eb6a8433fe138238a7a90c1dcaf5575a35b21ae7cf9dc3566008ba94b0R68-R76'><strong>Possible Bug</strong></a><br>The test that toggles `showValues` creates only a shallow copy of `createMockParams` and then mutates the nested `col.config` object. This means the shared `createMockParams.col.config` is permanently modified for subsequent tests, which can lead to inter-test coupling and flaky behavior depending on test execution order. The setup should avoid mutating shared nested objects.<br> - [ ] <a href='https://github.com/apache/superset/pull/36344/files#diff-734772e3939b93bbad551373f7a785fd0ab1301c18c0490f21df8913e04f59baR68-R73'><strong>Code Smell</strong></a><br>The `Array.isArray` guard right after `parseArrayValue` is inconsistent with `parseArrayValue`'s implementation, which always returns an array. This makes the `N/A` branch effectively unreachable and suggests either a misunderstanding of `parseArrayValue`'s contract or a missing check for an empty numeric array instead.<br> - [ ] <a href='https://github.com/apache/superset/pull/36344/files#diff-f81846cf1629aa98607b19561a1dfd9b9afd33be277586ac69ab9a0739f124c8R58-R60'><strong>Possible Bug</strong></a><br>`chartColor` calls `rgbToHex(color)` when `color` is an object, but the underlying `rgbToHex` utility expects three numeric arguments (`red, green, blue`). If `color` is an RGB object or other non-string structure, this mismatch can cause runtime errors or incorrect colors. The reviewer should confirm the shape of `color` and adjust the call accordingly.<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]
