codeant-ai-for-open-source[bot] commented on code in PR #37679: URL: https://github.com/apache/superset/pull/37679#discussion_r2813596537
########## superset-frontend/src/dashboard/components/ExportDashboardDataModal/ChartSelector.tsx: ########## @@ -0,0 +1,181 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { styled, t } from '@apache-superset/core'; +import { Checkbox } from '@superset-ui/core/components'; + +const Container = styled.div` + ${({ theme }) => ` + padding: ${theme.sizeUnit * 4}px 0; + `} +`; + +const HeaderRow = styled.div` + ${({ theme }) => ` + display: flex; + justify-content: space-between; + align-items: center; + margin-bottom: ${theme.sizeUnit * 3}px; + padding: 0 ${theme.sizeUnit * 4}px; + `} +`; + +const SelectionInfo = styled.div` + ${({ theme }) => ` + color: ${theme.colorTextSecondary}; + font-size: ${theme.sizeUnit * 3.5}px; + `} +`; + +const Actions = styled.div` + ${({ theme }) => ` + display: flex; + gap: ${theme.sizeUnit * 2}px; + `} +`; + +const ActionLink = styled.a` + ${({ theme }) => ` + color: ${theme.colorPrimary}; + cursor: pointer; + font-size: ${theme.sizeUnit * 3.5}px; + + &:hover { + text-decoration: underline; + } + `} +`; + +const ChartList = styled.div` + ${({ theme }) => ` + max-height: 400px; + overflow-y: auto; + border: 1px solid ${theme.colorBorder}; + border-radius: ${theme.sizeUnit}px; + `} +`; + +const ChartItem = styled.div<{ disabled?: boolean }>` + ${({ theme, disabled }) => ` + display: flex; + align-items: center; + padding: ${theme.sizeUnit * 3}px ${theme.sizeUnit * 4}px; + border-bottom: 1px solid ${theme.colorBorderSecondary}; + cursor: ${disabled ? 'not-allowed' : 'pointer'}; + opacity: ${disabled ? 0.5 : 1}; + + &:last-child { + border-bottom: none; + } + + &:hover { + background-color: ${disabled ? 'transparent' : theme.colorBgTextHover}; + } + `} +`; + +const ChartName = styled.span` + ${({ theme }) => ` + margin-left: ${theme.sizeUnit * 2}px; + font-size: ${theme.sizeUnit * 3.5}px; + `} +`; + +const ChartMeta = styled.span` + ${({ theme }) => ` + margin-left: auto; + color: ${theme.colorTextSecondary}; + font-size: ${theme.sizeUnit * 3}px; + `} +`; + +export interface ChartInfo { + id: number; + name: string; + vizType?: string; +} + +export interface ChartSelectorProps { + charts: ChartInfo[]; + selectedChartIds: Set<number>; + onSelectionChange: (selectedIds: Set<number>) => void; + disabled?: boolean; +} + +export const ChartSelector = ({ + charts, + selectedChartIds, + onSelectionChange, + disabled = false, +}: ChartSelectorProps) => { + const handleSelectAll = () => { + const allIds = new Set(charts.map(c => c.id)); + onSelectionChange(allIds); + }; + + const handleDeselectAll = () => { + onSelectionChange(new Set()); + }; + + const handleToggleChart = (chartId: number) => { + if (disabled) return; + + const newSelection = new Set(selectedChartIds); + if (newSelection.has(chartId)) { + newSelection.delete(chartId); + } else { + newSelection.add(chartId); + } + onSelectionChange(newSelection); + }; + + return ( + <Container> + <HeaderRow> + <SelectionInfo> + {t('%s of %s charts selected', selectedChartIds.size, charts.length)} + </SelectionInfo> + <Actions> + <ActionLink onClick={handleSelectAll}>{t('Select all')}</ActionLink> + <span>|</span> + <ActionLink onClick={handleDeselectAll}> + {t('Deselect all')} + </ActionLink> + </Actions> + </HeaderRow> + + <ChartList> + {charts.map(chart => ( + <ChartItem + key={chart.id} + onClick={() => handleToggleChart(chart.id)} + disabled={disabled} + > + <Checkbox + checked={selectedChartIds.has(chart.id)} + onChange={() => handleToggleChart(chart.id)} Review Comment: **Suggestion:** Clicking directly on the checkbox will trigger both the checkbox's `onChange` handler and the parent row's `onClick` handler, calling the toggle function twice and leaving the selection unchanged; you should prevent the row click from firing when the checkbox itself is clicked so the toggle only happens once. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Clicking checkbox doesn't change selection due to double toggle. - ⚠️ Confusing UX in dashboard bulk data export modal. ``` </details> ```suggestion onClick={event => event.stopPropagation()} ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the Superset frontend with this PR and open any dashboard that exposes the bulk data export modal which renders `ChartSelector` (`superset-frontend/src/dashboard/components/ExportDashboardDataModal/ChartSelector.tsx:120-181`). 2. Ensure the modal passes at least one chart into the `charts` prop so the list rendering block at lines 162-178 (the `{charts.map(chart => ( ... ))}` section) is visible. 3. In the rendered list, click directly on the checkbox for a chart row; this triggers the row's `onClick={() => handleToggleChart(chart.id)}` at line 166 via event bubbling and the `Checkbox`'s `onChange={() => handleToggleChart(chart.id)}` at line 171. 4. Observe that the chart's selection state in the parent (driven by `selectedChartIds` passed into `ChartSelector`) ends up unchanged or flickers, because `handleToggleChart` (lines 135-144) is executed twice for the same `chartId`, adding the id and then immediately removing it (or vice versa). ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/ExportDashboardDataModal/ChartSelector.tsx **Line:** 171:171 **Comment:** *Logic Error: Clicking directly on the checkbox will trigger both the checkbox's `onChange` handler and the parent row's `onClick` handler, calling the toggle function twice and leaving the selection unchanged; you should prevent the row click from firing when the checkbox itself is clicked so the toggle only happens once. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37679&comment_hash=70a03127df3a6b98690ed43f6931f86db8b392b44b6e090e93f986fc01862c6a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37679&comment_hash=70a03127df3a6b98690ed43f6931f86db8b392b44b6e090e93f986fc01862c6a&reaction=dislike'>👎</a> -- 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]
