codeant-ai-for-open-source[bot] commented on code in PR #41551: URL: https://github.com/apache/superset/pull/41551#discussion_r3707430658
########## superset-frontend/src/features/versionHistory/SaveGroupItem.tsx: ########## @@ -0,0 +1,386 @@ +/** + * 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 { KeyboardEvent, useState } from 'react'; +import { t, tn } from '@apache-superset/core/translation'; +import { styled, useTheme } from '@apache-superset/core/theme'; +import { Button, Dropdown, Icons, Tag } from '@superset-ui/core/components'; +import type { SaveGroup, VersionedEntityType } from './types'; +import { + formatAuthor, + formatVersionDateTimeShort, + groupHeadline, +} from './display'; +import ActionRow from './ActionRow'; + +/** + * The first chart save serializes the full form_data and can fan out + * into dozens of records; cap the initially visible rows per group. + */ +// TODO(version-history): backend workaround — remove when upstream stops +// exploding the full form_data into per-field records on the first save. +const VISIBLE_RECORD_LIMIT = 10; + +// The highlighted container gains inner padding but extends outward by +// the same amount (negative margin) so its text stays column-aligned +// with non-highlighted neighbors. +const Container = styled.div<{ isPreviewed: boolean }>` + ${({ theme, isPreviewed }) => { + const inset = isPreviewed ? theme.sizeUnit * 3 : 0; + return ` + border-bottom: 1px solid ${theme.colorBorderSecondary}; + background-color: ${isPreviewed ? theme.colorPrimaryBg : 'transparent'}; + border-radius: ${isPreviewed ? theme.borderRadius : 0}px; + padding: ${theme.sizeUnit * 2}px ${inset}px ${theme.sizeUnit * 4}px; + margin: 0 ${-inset}px; + `; + }} +`; + +const Header = styled.div<{ hasRecords: boolean }>` + ${({ theme, hasRecords }) => ` + display: flex; + align-items: flex-start; + gap: ${theme.sizeUnit * 2}px; + padding: ${theme.sizeUnit * 3}px 0; + cursor: ${hasRecords ? 'pointer' : 'default'}; + `} +`; + +const HeaderText = styled.div` + ${({ theme }) => ` + flex: 1; + min-width: 0; + display: flex; + flex-direction: column; + gap: ${theme.sizeUnit * 2}px; + `} +`; + +const HeadlineRow = styled.div` + ${({ theme }) => ` + display: flex; + align-items: center; + gap: ${theme.sizeUnit * 2}px; + min-width: 0; + `} +`; + +const Headline = styled.div` + ${({ theme }) => ` + font-size: ${theme.fontSize}px; + line-height: ${theme.lineHeight}; + color: ${theme.colorText}; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + `} +`; + +const Meta = styled.div` + ${({ theme }) => ` + color: ${theme.colorTextTertiary}; + font-size: ${theme.fontSizeSM}px; + line-height: ${theme.lineHeightSM}; + `} +`; + +// Icons and trailing controls center within the first text line (one +// line-height tall) so they track the headline, not the middle of a +// two-line header block. +const IconWrapper = styled.span` + ${({ theme }) => ` + color: ${theme.colorTextSecondary}; + display: flex; + align-items: center; + height: ${theme.fontSize * theme.lineHeight}px; + `} +`; + +const ChevronWrapper = styled.span` + ${({ theme }) => ` + color: ${theme.colorTextTertiary}; + display: flex; + align-items: center; + height: ${theme.fontSize * theme.lineHeight}px; + `} +`; + +const KebabSlot = styled.span` + ${({ theme }) => ` + display: flex; + align-items: center; + height: ${theme.fontSize * theme.lineHeight}px; + `} +`; + +// Icon-only trigger: neutral icon color instead of the link-button blue. +const KebabButton = styled(Button)` + ${({ theme }) => ` + && { + color: ${theme.colorTextTertiary}; + } + &&:hover, + &&:focus { + color: ${theme.colorText}; + } + `} +`; + +const ExpanderRow = styled.div` + ${({ theme }) => ` + padding-left: ${theme.sizeUnit * 8}px; + `} +`; + +export interface SaveGroupItemProps { + entityType: VersionedEntityType; + group: SaveGroup; + /** The newest self save: it IS the live state, not a historical one. */ + isCurrent: boolean; + canRestore: boolean; + isPreviewed: boolean; + onPreview: (group: SaveGroup) => void; + /** Leave an active historical preview (back to the live version). */ + onExitPreview?: () => void; + onRestore: (group: SaveGroup) => void; + onOpenAsNew: (group: SaveGroup) => void; +} + +function GroupKebab({ + entityType, + group, + isCurrent, + canRestore, + onRestore, + onOpenAsNew, +}: Pick< + SaveGroupItemProps, + | 'entityType' + | 'group' + | 'isCurrent' + | 'canRestore' + | 'onRestore' + | 'onOpenAsNew' +>) { + const theme = useTheme(); + if (group.versionUuid == null) { + // Both actions name a specific version; a group the server returned + // without one has nothing for them to act on, and the container + // handlers would silently no-op. No kebab beats a dead menu. + return null; + } + const itemStyle = { + height: theme.controlHeightLG, + paddingLeft: theme.sizeUnit * 6, + paddingRight: theme.sizeUnit * 6, + display: 'flex', + alignItems: 'center', + }; + const menuItems = [ + // Restoring the live version is a no-op; offer it only on history. + ...(isCurrent || !canRestore + ? [] + : [ Review Comment: **Suggestion:** The current-version marker is unavailable when the dedicated newest-self probe fails, because `newestGroup` becomes `null` and every group is passed as `isCurrent={false}`. This condition then exposes Restore for the live version (and allows it to be previewed), misleading the user and potentially issuing a no-op restore. Treat an unknown current version as non-restorable until the probe succeeds, rather than using `!isCurrent` alone. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Live versions can show misleading Restore actions. - ⚠️ Live versions may become previewable after probe failure. - ⚠️ Restore can issue an unnecessary already-at-this-version request. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d0b71627d86f478397505dae15c4092f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d0b71627d86f478397505dae15c4092f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/features/versionHistory/SaveGroupItem.tsx **Line:** 197:199 **Comment:** *Logic Error: The current-version marker is unavailable when the dedicated newest-self probe fails, because `newestGroup` becomes `null` and every group is passed as `isCurrent={false}`. This condition then exposes Restore for the live version (and allows it to be previewed), misleading the user and potentially issuing a no-op restore. Treat an unknown current version as non-restorable until the probe succeeds, rather than using `!isCurrent` alone. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41551&comment_hash=db97e9490f25e160f131635656e2ab0d65f939d62c9399d69c68de6a3ddffa13&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41551&comment_hash=db97e9490f25e160f131635656e2ab0d65f939d62c9399d69c68de6a3ddffa13&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]
