codeant-ai-for-open-source[bot] commented on code in PR #39461:
URL: https://github.com/apache/superset/pull/39461#discussion_r3411613459
##########
superset-frontend/src/components/CopyToClipboard/index.tsx:
##########
@@ -16,126 +16,153 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { Component, cloneElement, ReactElement } from 'react';
+import {
+ cloneElement,
+ isValidElement,
+ type KeyboardEvent,
+ ReactElement,
+ useCallback,
+} from 'react';
import { t } from '@apache-superset/core/translation';
import { css, SupersetTheme } from '@apache-superset/core/theme';
import copyTextToClipboard from 'src/utils/copy';
import { Tooltip } from '@superset-ui/core/components';
import withToasts from '../MessageToasts/withToasts';
import type { CopyToClipboardProps } from './types';
-const defaultProps: Partial<CopyToClipboardProps> = {
- copyNode: <span>{t('Copy')}</span>,
- onCopyEnd: () => {},
- shouldShowText: true,
- wrapped: true,
- tooltipText: t('Copy to clipboard'),
- hideTooltip: false,
-};
+function CopyToClip({
+ copyNode = <span>{t('Copy')}</span>,
+ onCopyEnd = () => {},
+ shouldShowText = true,
+ wrapped = true,
+ tooltipText = t('Copy to clipboard'),
+ hideTooltip = false,
+ disabled,
+ getText,
+ text,
+ addSuccessToast,
+ addDangerToast,
+}: CopyToClipboardProps) {
+ const copyToClipboard = useCallback(
+ (textToCopy: Promise<string>) => {
+ copyTextToClipboard(() => textToCopy)
+ .then(() => {
+ addSuccessToast(t('Copied to clipboard!'));
+ })
+ .catch(() => {
+ addDangerToast(
+ t(
+ 'Sorry, your browser does not support copying. Use Ctrl / Cmd +
C!',
+ ),
+ );
+ })
+ .finally(() => {
+ if (onCopyEnd) onCopyEnd();
+ });
+ },
+ [addSuccessToast, addDangerToast, onCopyEnd],
+ );
-class CopyToClip extends Component<CopyToClipboardProps> {
- static defaultProps = defaultProps;
-
- constructor(props: CopyToClipboardProps) {
- super(props);
- this.copyToClipboard = this.copyToClipboard.bind(this);
- this.onClick = this.onClick.bind(this);
- }
-
- onClick() {
- if (this.props.disabled) {
+ const onClick = useCallback(() => {
+ if (disabled) {
return;
}
- if (this.props.getText) {
- this.props.getText((d: string) => {
- this.copyToClipboard(Promise.resolve(d));
+ if (getText) {
+ getText((d: string) => {
+ copyToClipboard(Promise.resolve(d));
});
} else {
- this.copyToClipboard(Promise.resolve(this.props.text || ''));
+ copyToClipboard(Promise.resolve(text || ''));
}
- }
-
- getDecoratedCopyNode() {
- const copyNode = this.props.copyNode as ReactElement;
- const { disabled } = this.props;
- return cloneElement(copyNode, {
- style: {
- ...copyNode.props.style,
- cursor: disabled ? 'not-allowed' : 'pointer',
- },
- onClick: disabled ? undefined : this.onClick,
- 'aria-disabled': disabled || undefined,
- tabIndex: disabled ? -1 : copyNode.props.tabIndex,
- });
- }
+ }, [disabled, getText, text, copyToClipboard]);
- copyToClipboard(textToCopy: Promise<string>) {
- copyTextToClipboard(() => textToCopy)
- .then(() => {
- this.props.addSuccessToast(t('Copied to clipboard!'));
- })
- .catch(() => {
- this.props.addDangerToast(
- t(
- 'Sorry, your browser does not support copying. Use Ctrl / Cmd +
C!',
- ),
- );
- })
- .finally(() => {
- if (this.props.onCopyEnd) this.props.onCopyEnd();
+ const getDecoratedCopyNode = useCallback(() => {
+ const cursor = disabled ? 'not-allowed' : 'pointer';
+ if (isValidElement(copyNode)) {
+ const node = copyNode as ReactElement;
+ return cloneElement(node, {
+ style: {
+ ...node.props.style,
+ cursor,
+ },
+ onClick: disabled ? undefined : onClick,
Review Comment:
**Suggestion:** Replacing the child element's click handler with `onClick`
drops any existing `copyNode` click behavior, which can break caller logic (for
example preventing default navigation or custom analytics). Compose with the
original handler instead of overwriting it. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Future copy buttons lose custom click behavior.
- ⚠️ Analytics handlers on copyNode would never execute.
- ⚠️ Affects SqlLab, dashboard, and explore copy interactions.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect `CopyToClipboard` implementation in
superset-frontend/src/components/CopyToClipboard/index.tsx:79–92; when
`copyNode` is a
ReactElement, `cloneElement(node, { ...style, cursor, onClick: disabled ?
undefined :
onClick, 'aria-disabled': ..., tabIndex: ... })` is used, unconditionally
setting
`onClick`.
2. Note that several callers already pass custom React elements as
`copyNode` (e.g.
URLShortLinkButton at
superset-frontend/src/dashboard/components/URLShortLinkButton/index.tsx:11–16,
TableElement at
superset-frontend/src/SqlLab/components/TableElement/index.tsx:122–137,
DataTableControl at
superset-frontend/src/explore/components/DataTableControl/index.tsx:8–27),
even though
they currently omit `onClick`.
3. If any of these callers adds a meaningful `onClick` to `copyNode` (for
analytics,
navigation, or side effects), React will still execute
`getDecoratedCopyNode()`
(CopyToClipboard:79–114), and `cloneElement` at line 83–89 will overwrite
the original
`node.props.onClick` with its own `onClick` handler.
4. At runtime, clicking the rendered copy control calls only
`CopyToClipboard`'s internal
`onClick` (lines 66–77, which performs clipboard copy) while the caller's
original
`copyNode` click handler is never invoked, silently breaking any caller
logic attached to
that handler without errors.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d8fe6c0ea86f4019903672c807aabb22&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d8fe6c0ea86f4019903672c807aabb22&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/components/CopyToClipboard/index.tsx
**Line:** 88:88
**Comment:**
*Api Mismatch: Replacing the child element's click handler with
`onClick` drops any existing `copyNode` click behavior, which can break caller
logic (for example preventing default navigation or custom analytics). Compose
with the original handler instead of overwriting it.
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%2F39461&comment_hash=15245a6f2eccabd67fa04d57e6515e29ca0b4d1ac03904a2f42b51336b45e250&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39461&comment_hash=15245a6f2eccabd67fa04d57e6515e29ca0b4d1ac03904a2f42b51336b45e250&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-ui-core/src/components/ModalTrigger/index.tsx:
##########
@@ -110,7 +113,9 @@ export const ModalTrigger = forwardRef(
className={className}
show={showModal}
onHide={close}
- name={modalTitle}
+ // `name` is used for data-test / telemetry and must be a string;
+ // `title` accepts arbitrary ReactNode for rich rendering.
+ name={typeof modalTitle === 'string' ? modalTitle : undefined}
Review Comment:
**Suggestion:** When `modalTitle` is a React node, `name` becomes
`undefined`, so the underlying modal falls back to the generic
`data-test="antd-modal"` and loses the stable per-modal identifier used by
telemetry/selectors. Add a dedicated string prop (for example `modalName`) and
pass that to `name` so rich titles do not erase modal identity. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Rich-title modals get generic data-test "antd-modal".
- ⚠️ Telemetry cannot distinguish individually named modals.
- ⚠️ End-to-end tests lose stable modal selectors.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note that `ModalTrigger`
(superset-frontend/packages/superset-ui-core/src/components/ModalTrigger/index.tsx:24–31,
112–120) declares `modalTitle?: ReactNode` and passes `name={typeof
modalTitle ===
'string' ? modalTitle : undefined}` to `Modal`.
2. Observe `Modal` implementation
(superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx:228–256,
87–109) where `wrapProps` is set to `{'data-test': `${name ||
'antd'}-modal`, ...}` so
when `name` is falsy the data-test falls back to `"antd-modal"`.
3. Use `ModalTrigger` from any caller (for example `renderControls` in
superset-frontend/src/SqlLab/components/TableElement/index.tsx:58–80) but
pass a rich
ReactNode title as encouraged by the new comment (lines 27–30 in
ModalTrigger), e.g.
`<ControlHeader title="Keys for table" />` instead of a plain string.
4. When the trigger node is clicked, `open()` in ModalTrigger (lines 84–88)
sets
`showModal` to true, rendering `<Modal>` with `name={undefined}`; inside
`Modal` line 108
sets `data-test="antd-modal"`, so this modal no longer has a stable,
specific `data-test`
identifier for telemetry or automated selectors even though a distinct title
ReactNode was
provided.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=15fca8579efa4d6096d478489e099cdd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=15fca8579efa4d6096d478489e099cdd&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/packages/superset-ui-core/src/components/ModalTrigger/index.tsx
**Line:** 118:118
**Comment:**
*Api Mismatch: When `modalTitle` is a React node, `name` becomes
`undefined`, so the underlying modal falls back to the generic
`data-test="antd-modal"` and loses the stable per-modal identifier used by
telemetry/selectors. Add a dedicated string prop (for example `modalName`) and
pass that to `name` so rich titles do not erase modal identity.
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%2F39461&comment_hash=3719c63cd0433116af8b3e74714be5c58c79c0025ac62a3583d86b312932aeb2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39461&comment_hash=3719c63cd0433116af8b3e74714be5c58c79c0025ac62a3583d86b312932aeb2&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]
