codeant-ai-for-open-source[bot] commented on PR #36535: URL: https://github.com/apache/superset/pull/36535#issuecomment-3642741320
## 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/36535/files#diff-1554dae79940b5b23a67ba94b0a38b37d36e984fb0e9ff077d2fc6c45e9f0619R30-R33'><strong>Global state risk</strong></a><br>A module-level mutable registry (`dashboardColorRegistry`) is added and shared across all dashboards and palettes. This can cause color assignments to leak across different dashboards, lead to unexpected color collisions, and grow over time with no eviction/reset, causing memory growth and surprising behavior.<br> - [ ] <a href='https://github.com/apache/superset/pull/36535/files#diff-1554dae79940b5b23a67ba94b0a38b37d36e984fb0e9ff077d2fc6c45e9f0619R48-R51'><strong>Fallback selection logic</strong></a><br>The fallback color uses `palette[dashboardColorRegistry.usedColors.size % palette.length]`. Because `usedColors` is global and may contain colors from other palettes or dashboards, the fallback index can be incorrect and may select a color that's already in use. There is also no attempt to prefer an unused color before falling back.<br> - [ ] <a href='https://github.com/apache/superset/pull/36535/files#diff-1554dae79940b5b23a67ba94b0a38b37d36e984fb0e9ff077d2fc6c45e9f0619R30-R52'><strong>No reset / namespacing API</strong></a><br>There's no public API to clear or namespace `dashboardColorRegistry`. Tests, hot-reload, or switching dashboards could leave stale mappings in memory and produce inconsistent color assignments across sessions.<br> - [ ] <a href='https://github.com/apache/superset/pull/36535/files#diff-9f7ed3fe71ad1ca6d1d4f0b643353d6a4169eb1e0e9a8e0ac625808bcfc11b6bR32-R46'><strong>Palette compatibility / fallback</strong></a><br>The function destructures `colorBounds` into `[minColor, maxColor]` but then uses three entries in the color range. If callers ever provide a 3-color palette, the current code ignores the middle palette color. Conversely, if callers provide less-than-expected values, the mid color should have a clear fallback. This should be handled explicitly.<br> - [ ] <a href='https://github.com/apache/superset/pull/36535/files#diff-9f7ed3fe71ad1ca6d1d4f0b643353d6a4169eb1e0e9a8e0ac625808bcfc11b6bR46-R46'><strong>Hardcoded mid color</strong></a><br>The function now hardcodes the mid-range color as `#B3B3B3`. This reduces configurability and can cause inconsistencies if other parts of the app or different palettes expect a different neutral mid color. Consider centralizing the mid color (or supporting a 3-color `colorBounds`) so themes/palettes remain consistent.<br> - [ ] <a href='https://github.com/apache/superset/pull/36535/files#diff-9f7ed3fe71ad1ca6d1d4f0b643353d6a4169eb1e0e9a8e0ac625808bcfc11b6bR10-R10'><strong>License header typo</strong></a><br>The Apache license URL in the file header appears to have a stray "color" suffix appended to it ("LICENSE-2.0color"). This is most likely an accidental edit and should be corrected to the canonical license URL.<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]
