EnxDev commented on PR #41490: URL: https://github.com/apache/superset/pull/41490#issuecomment-4851444746
## EnxDev's Review Agent โ apache/superset#41490 ยท HEAD 3b96e90 **lgtm** โ clean, test-first bug fix; color logic extracted into a shared util and reused unchanged. Two unmentioned standalone-chart behavior changes worth a note, both low risk. This is a refactor + fix: the color-resolution branches move verbatim from `CategoricalDeckGLContainer`'s `addColor` into `addColorToFeatures`, and the same util is then applied to color-aware sublayers (`deck_scatter`/`deck_path`/`deck_arc`) in `Multi.tsx`. I diffed the extracted branches against the originals โ fixed/categorical/breakpoint cases are byte-equivalent. Two deltas: ### ๐ด Functional - **`utils/addColor.ts:111`** ยท _Low_ โ the `default` case changed from `return []` to `return data`. In the standalone container, `addColor` is called with `selectedColorScheme = fd.color_scheme_type` (`CategoricalDeckGLContainer.tsx:247`); when that's unset/unrecognized, the old code dropped every feature (blank map), the new code returns features untouched so the layer renders points in its fallback color. The Multi path can't hit this (the call site guards on `color_scheme_type` being truthy), so the change is container-only and arguably an improvement โ but it's a real behavior shift for any saved scatter with no `color_scheme_type`. Confirm `color_scheme_type` always defaults for saved charts; if it can be blank, this is observable. **regression test:** call `addColorToFeatures(data, fd)` with no/unknown `color_scheme_type` and assert the intended outcome (the existing "unrecognized scheme returns features unchanged" test pins the new behavior but not the container's ol d empty-array contract). ### ๐ก Should-fix - **`CategoricalDeckGLContainer.tsx:73`** โ `getCategories` now keys the legend on `fd.slice_id` instead of `fd.sliceId`. This is the right call (it aligns the legend with the point-color path, which always used `slice_id`, and is the canonical `form_data` key โ already settled with reviewers). But it also changes legend swatch colors for existing standalone categorical scatter charts whose scale was previously seeded with `undefined`. It's an unmentioned standalone change with no direct test. Worth a line in the PR description and a test asserting legend and point colors resolve from the same `slice_id`. ### ๐ Praise - `utils/addColor.ts` โ extracting the branches into one tested util (and the `Multi.color.test.tsx` red-on-master pin) is the right shape; the fix is correctly scoped to color-aware layers and leaves others untouched. Re the `color_picker` alpha drop in the categorical branch (raised by a bot): agreed it's lifted verbatim from master and out of scope here. <!-- enxdev-review-agent:3b96e90 --> _Reviewed by EnxDev's Review Agent โ @rusackas ยท HEAD 3b96e90._ -- 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]
