codeant-ai-for-open-source[bot] commented on code in PR #41392: URL: https://github.com/apache/superset/pull/41392#discussion_r3534769732
########## superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/copyCellValue.ts: ########## @@ -0,0 +1,156 @@ +/** + * 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. + */ + +/** + * The Interactive Table selects the cell (not its text) on click (#106389). + * Because only AG Grid Community modules are registered, the Enterprise + * clipboard module is unavailable, so the browser's native text-selection was + * previously the *only* way to copy a cell value. To keep copy-a-value working, + * we provide a small Ctrl/Cmd+C handler that copies the focused cell's value. + */ + +/** + * Keyboard fields we read to detect a copy shortcut. AG Grid types the event as + * the DOM `Event`, so we accept that and read the keyboard fields optionally + * (at runtime a cell key-down carries a `KeyboardEvent`). + */ +export type CopyKeyboardEvent = Partial< + Pick<KeyboardEvent, 'key' | 'ctrlKey' | 'metaKey'> +> & + Partial<Event>; + +/** + * Minimal shape of the AG Grid `CellKeyDownEvent` we rely on. The event carries + * no pre-formatted value, so we re-run the column's `valueFormatter` ourselves. + */ +export interface CopyableCellParams { + event?: CopyKeyboardEvent | null; + value?: unknown; + colDef?: { + valueFormatter?: unknown; + } | null; + // Forwarded to the valueFormatter; Superset's reads `value` and `node`. + node?: unknown; + column?: unknown; + data?: unknown; + api?: unknown; + context?: unknown; +} + +/** True when the event is Ctrl+C (Win/Linux) or Cmd+C (macOS). */ +export function isCopyShortcut(event?: CopyKeyboardEvent | null): boolean { + if (!event) { + return false; + } + const key = (event.key ?? '').toLowerCase(); + return (event.ctrlKey === true || event.metaKey === true) && key === 'c'; +} + +/** + * Resolves the text to copy so it matches what the user sees: runs the column's + * `valueFormatter` (as the grid does when painting the cell) to copy the + * displayed value (`2,871`, `$2.87K`, `N/A`, ...). Falls back to the raw value + * when there is no formatter or it throws; null/undefined copy as empty string. + */ +export function getCellCopyText(params?: CopyableCellParams): string { + if (!params) { + return ''; + } + + const { valueFormatter } = params.colDef ?? {}; + if (typeof valueFormatter === 'function') { + try { + const formatted = valueFormatter({ + value: params.value, + node: params.node, + column: params.column, + colDef: params.colDef, + data: params.data, + api: params.api, + context: params.context, + }); + if (formatted !== null && formatted !== undefined) { + return String(formatted); + } + } catch { + // Fall through to the raw value below. + } + } + + const { value } = params; + if (value === null || value === undefined) { + return ''; + } + return String(value); +} + +/** + * Writes text to the clipboard, using the async Clipboard API when available + * and falling back to a hidden textarea + execCommand for non-secure contexts. + * Returns whether the write is believed to have succeeded. + */ +export async function writeTextToClipboard(text: string): Promise<boolean> { + try { + if ( + typeof navigator !== 'undefined' && + navigator.clipboard && + typeof navigator.clipboard.writeText === 'function' + ) { + await navigator.clipboard.writeText(text); + return true; + } + } catch { + // Fall through to the legacy fallback below. + } + + if (typeof document === 'undefined') { + return false; + } + + try { + const textarea = document.createElement('textarea'); + textarea.value = text; + textarea.setAttribute('readonly', ''); + textarea.style.position = 'fixed'; + textarea.style.top = '-1000px'; + textarea.style.opacity = '0'; + document.body.appendChild(textarea); + textarea.select(); + const succeeded = document.execCommand('copy'); + document.body.removeChild(textarea); + return succeeded; + } catch { + return false; + } Review Comment: **Suggestion:** The fallback clipboard path leaks a hidden textarea when `textarea.select()` or `document.execCommand('copy')` throws, because cleanup is only performed on the success path. Move DOM removal into a `finally` block (or equivalent guaranteed cleanup) so the temporary node is always removed even on exceptions. [missing cleanup] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Interactive table copy shortcut leaks hidden textarea nodes. - ⚠️ Long-running dashboards may accumulate unnecessary DOM elements. - ⚠️ Clipboard failure path not fully cleaned up. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open a dashboard or Explore view that renders the Interactive Table chart, which uses `AgGridDataTable` from `superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx` (see component definition at lines 120-153 and the `handleCellKeyDown` callback at lines 241-246 wiring `onCellKeyDown`). 2. Run the app in an environment where the async Clipboard API path is unavailable or failing, e.g. `navigator.clipboard` is undefined or `navigator.clipboard.writeText` rejects, which causes `writeTextToClipboard` in `superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/copyCellValue.ts:108-121` to fall through to the legacy `document.execCommand('copy')` fallback. 3. With the interactive table focused, press Ctrl+C (or Cmd+C on macOS) on a cell; `onCellKeyDown` on the grid (wired at `src/AgGridTable/index.tsx:520-528`) calls `handleCellKeyDown`, which invokes `copyCellValueOnKeyDown` (`src/utils/copyCellValue.ts:148-155`), which in turn calls `writeTextToClipboard` hitting the fallback block at lines 127-140 where a hidden textarea is appended and `document.execCommand('copy')` is invoked. 4. Simulate or encounter a runtime where `textarea.select()` or `document.execCommand('copy')` throws (mirroring the Jest test `writeTextToClipboard returns false when both clipboard paths fail` at `superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/copyCellValue.test.ts:39-52`, where `execCommand` is mocked to throw); the exception causes control to jump to the `catch` block at lines 138-140, which returns `false` without running `document.body.removeChild(textarea)`, leaving the hidden textarea in the DOM. Repeating the copy shortcut accumulates multiple hidden textarea elements, which can be observed in browser devtools under the document body. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=50728f5a7a884e15a98e20eaa7d7f15f&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=50728f5a7a884e15a98e20eaa7d7f15f&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/plugins/plugin-chart-ag-grid-table/src/utils/copyCellValue.ts **Line:** 133:140 **Comment:** *Missing Cleanup: The fallback clipboard path leaks a hidden textarea when `textarea.select()` or `document.execCommand('copy')` throws, because cleanup is only performed on the success path. Move DOM removal into a `finally` block (or equivalent guaranteed cleanup) so the temporary node is always removed even on exceptions. 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%2F41392&comment_hash=e74e4370db89db010daf12345222fd458a48a215fb9438529f861d1627f78641&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41392&comment_hash=e74e4370db89db010daf12345222fd458a48a215fb9438529f861d1627f78641&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]
