codeant-ai-for-open-source[bot] commented on code in PR #38581:
URL: https://github.com/apache/superset/pull/38581#discussion_r3015754210
##########
superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx:
##########
@@ -760,6 +772,197 @@ describe('ResultSet', () => {
},
);
+ test('should show CSV button with granular can_export_data permission when
flag is ON', async () => {
+ mockIsFeatureEnabled.mockImplementation(
+ (flag: FeatureFlag) => flag === FeatureFlag.GranularExportControls,
+ );
+ const { queryByTestId } = setup(
+ mockedProps,
+ mockStore({
+ ...initialState,
+ user: {
+ ...user,
+ roles: {
+ sql_lab: [['can_export_data', 'Superset']],
+ },
+ },
+ sqlLab: {
+ ...initialState.sqlLab,
+ queries: {
+ [queries[0].id]: queries[0],
+ },
+ },
+ }),
+ );
+ const csvButton = queryByTestId('export-csv-button');
+ expect(csvButton).toBeInTheDocument();
+ expect(csvButton).toBeEnabled();
+ mockIsFeatureEnabled.mockReset();
+ });
+
+ test('should disable CSV button when granular flag is ON and user lacks
can_export_data', async () => {
+ mockIsFeatureEnabled.mockImplementation(
+ (flag: FeatureFlag) => flag === FeatureFlag.GranularExportControls,
+ );
+ const { queryByTestId } = setup(
+ mockedProps,
+ mockStore({
+ ...initialState,
+ user: {
+ ...user,
+ roles: {
+ sql_lab: [],
+ },
+ },
+ sqlLab: {
+ ...initialState.sqlLab,
+ queries: {
+ [queries[0].id]: queries[0],
+ },
+ },
+ }),
+ );
+ const csvButton = queryByTestId('export-csv-button');
+ expect(csvButton).toBeInTheDocument();
+ expect(csvButton).toBeDisabled();
+ mockIsFeatureEnabled.mockReset();
+ });
+
+ test('should show clipboard button with granular can_copy_clipboard
permission when flag is ON', async () => {
+ mockIsFeatureEnabled.mockImplementation(
+ (flag: FeatureFlag) => flag === FeatureFlag.GranularExportControls,
+ );
+ const { queryByTestId } = setup(
+ mockedProps,
+ mockStore({
+ ...initialState,
+ user: {
+ ...user,
+ roles: {
+ sql_lab: [['can_copy_clipboard', 'Superset']],
+ },
+ },
+ sqlLab: {
+ ...initialState.sqlLab,
+ queries: {
+ [queries[0].id]: queries[0],
+ },
+ },
+ }),
+ );
+ expect(queryByTestId('copy-to-clipboard-button')).toBeInTheDocument();
+ mockIsFeatureEnabled.mockReset();
+ });
+
+ test('should disable clipboard button when granular flag is ON and user
lacks can_copy_clipboard', async () => {
+ mockIsFeatureEnabled.mockImplementation(
+ (flag: FeatureFlag) => flag === FeatureFlag.GranularExportControls,
+ );
+ const { queryByTestId } = setup(
+ mockedProps,
+ mockStore({
+ ...initialState,
+ user: {
+ ...user,
+ roles: {
+ sql_lab: [['can_export_data', 'Superset']],
+ },
+ },
+ sqlLab: {
+ ...initialState.sqlLab,
+ queries: {
+ [queries[0].id]: queries[0],
+ },
+ },
+ }),
+ );
+ const clipboardButton = queryByTestId('copy-to-clipboard-button');
+ expect(clipboardButton).toBeInTheDocument();
+ expect(clipboardButton).toBeDisabled();
+ mockIsFeatureEnabled.mockReset();
+ });
+
+ test('should use legacy can_export_csv for both CSV and clipboard when
granular flag is OFF', async () => {
+ mockIsFeatureEnabled.mockReturnValue(false);
+ const { queryByTestId } = setup(
+ mockedProps,
+ mockStore({
+ ...initialState,
+ user: {
+ ...user,
+ roles: {
+ sql_lab: [['can_export_csv', 'SQLLab']],
+ },
+ },
+ sqlLab: {
+ ...initialState.sqlLab,
+ queries: {
+ [queries[0].id]: queries[0],
+ },
+ },
+ }),
+ );
+ expect(queryByTestId('export-csv-button')).toBeInTheDocument();
+ expect(queryByTestId('copy-to-clipboard-button')).toBeInTheDocument();
Review Comment:
**Suggestion:** This legacy fallback test only checks that both controls are
rendered, but both controls are rendered even when disabled. That means
regressions in legacy `can_export_csv` permission behavior would go undetected.
Assert enabled state for both controls. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Legacy can_export_csv mapping for SQL Lab unvalidated enablement.
- ⚠️ CSV and clipboard actions could silently disable for admins.
```
</details>
```suggestion
const csvButton = queryByTestId('export-csv-button');
const clipboardButton = queryByTestId('copy-to-clipboard-button');
expect(csvButton).toBeInTheDocument();
expect(csvButton).toBeEnabled();
expect(clipboardButton).toBeInTheDocument();
expect(clipboardButton).toBeEnabled();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset-frontend/src/hooks/usePermissions.ts` at lines 37–60, note
that when
`GranularExportControls` is disabled, both `canExportDataSqlLab` and
`canCopyClipboardSqlLab` fall back to the legacy SQL Lab permission
`can_export_csv` via
`canExportCsvSqlLab`.
2. In `superset-frontend/src/SqlLab/components/ResultSet/index.tsx` lines
388–455, the CSV
and clipboard buttons use `disabled={!canExportData}` and
`disabled={!canCopyClipboard}`
respectively, so with the feature flag off and `can_export_csv` granted,
both buttons
should be enabled for SQL Lab users.
3. Find the test `"should use legacy can_export_csv for both CSV and
clipboard when
granular flag is OFF"` in `ResultSet.test.tsx` at lines 885–908; it forces
`mockIsFeatureEnabled.mockReturnValue(false)` (line 886) and configures the
Redux store so
`user.roles.sql_lab = [['can_export_csv', 'SQLLab']]` (lines 892–895).
4. The test currently only asserts that both buttons are present in the DOM
(lines
905–906) and then resets the feature flag mock (907); if a regression caused
`canExportDataSqlLab` or `canCopyClipboardSqlLab` to be computed as `false`
under the
legacy path, resulting in disabled buttons for authorized users, this test
would still
pass because it never validates that either control is enabled.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx
**Line:** 905:906
**Comment:**
*Logic Error: This legacy fallback test only checks that both controls
are rendered, but both controls are rendered even when disabled. That means
regressions in legacy `can_export_csv` permission behavior would go undetected.
Assert enabled state for both controls.
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%2F38581&comment_hash=737af7ebfe9a4eb774178e487c61a5ef47eeaa84dc1f8d6ae1736c91b5e4569a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38581&comment_hash=737af7ebfe9a4eb774178e487c61a5ef47eeaa84dc1f8d6ae1736c91b5e4569a&reaction=dislike'>👎</a>
##########
superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx:
##########
@@ -760,6 +772,197 @@ describe('ResultSet', () => {
},
);
+ test('should show CSV button with granular can_export_data permission when
flag is ON', async () => {
+ mockIsFeatureEnabled.mockImplementation(
+ (flag: FeatureFlag) => flag === FeatureFlag.GranularExportControls,
+ );
+ const { queryByTestId } = setup(
+ mockedProps,
+ mockStore({
+ ...initialState,
+ user: {
+ ...user,
+ roles: {
+ sql_lab: [['can_export_data', 'Superset']],
+ },
+ },
+ sqlLab: {
+ ...initialState.sqlLab,
+ queries: {
+ [queries[0].id]: queries[0],
+ },
+ },
+ }),
+ );
+ const csvButton = queryByTestId('export-csv-button');
+ expect(csvButton).toBeInTheDocument();
+ expect(csvButton).toBeEnabled();
+ mockIsFeatureEnabled.mockReset();
+ });
+
+ test('should disable CSV button when granular flag is ON and user lacks
can_export_data', async () => {
+ mockIsFeatureEnabled.mockImplementation(
+ (flag: FeatureFlag) => flag === FeatureFlag.GranularExportControls,
+ );
+ const { queryByTestId } = setup(
+ mockedProps,
+ mockStore({
+ ...initialState,
+ user: {
+ ...user,
+ roles: {
+ sql_lab: [],
+ },
+ },
+ sqlLab: {
+ ...initialState.sqlLab,
+ queries: {
+ [queries[0].id]: queries[0],
+ },
+ },
+ }),
+ );
+ const csvButton = queryByTestId('export-csv-button');
+ expect(csvButton).toBeInTheDocument();
+ expect(csvButton).toBeDisabled();
+ mockIsFeatureEnabled.mockReset();
+ });
+
+ test('should show clipboard button with granular can_copy_clipboard
permission when flag is ON', async () => {
+ mockIsFeatureEnabled.mockImplementation(
+ (flag: FeatureFlag) => flag === FeatureFlag.GranularExportControls,
+ );
+ const { queryByTestId } = setup(
+ mockedProps,
+ mockStore({
+ ...initialState,
+ user: {
+ ...user,
+ roles: {
+ sql_lab: [['can_copy_clipboard', 'Superset']],
+ },
+ },
+ sqlLab: {
+ ...initialState.sqlLab,
+ queries: {
+ [queries[0].id]: queries[0],
+ },
+ },
+ }),
+ );
+ expect(queryByTestId('copy-to-clipboard-button')).toBeInTheDocument();
Review Comment:
**Suggestion:** This test only verifies that the clipboard button exists,
but the button is always rendered and can still be disabled. As written, it can
pass even when `can_copy_clipboard` permission handling is broken. Assert
enabled state to validate the permission contract. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ SQL Lab clipboard permission regressions may escape unit tests.
- ⚠️ Granular export controls less validated for clipboard behavior.
```
</details>
```suggestion
const clipboardButton = queryByTestId('copy-to-clipboard-button');
expect(clipboardButton).toBeInTheDocument();
expect(clipboardButton).toBeEnabled();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset-frontend/src/SqlLab/components/ResultSet/index.tsx` and
locate the
copy-to-clipboard button in `renderControls()` at lines 434–454, where
`disabled={!canCopyClipboard}` determines whether the button is enabled.
2. Inspect `superset-frontend/src/hooks/usePermissions.ts` at lines 24–60:
when the
`GranularExportControls` feature flag is enabled
(`isFeatureEnabled(FeatureFlag.GranularExportControls)`),
`canCopyClipboardSqlLab` is
derived from the `can_copy_clipboard` permission and returned as
`canCopyClipboardSqlLab`
(aliased to `canCopyClipboard` in `ResultSet`).
3. In the test file
`superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx`, find
the test
`"should show clipboard button with granular can_copy_clipboard permission
when flag is
ON"` at lines 831–855; it enables the `GranularExportControls` flag and sets
`user.roles.sql_lab = [['can_copy_clipboard', 'Superset']]` (lines 831–852),
then only
asserts
`expect(queryByTestId('copy-to-clipboard-button')).toBeInTheDocument();` (line
853).
4. Because this test never asserts `expect(clipboardButton).toBeEnabled()`,
a regression
that incorrectly sets `canCopyClipboardSqlLab` to `false` (making the button
disabled
despite the permission) would still leave this test passing, allowing a real
SQL Lab
clipboard permission bug to slip through CI.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx
**Line:** 853:853
**Comment:**
*Logic Error: This test only verifies that the clipboard button exists,
but the button is always rendered and can still be disabled. As written, it can
pass even when `can_copy_clipboard` permission handling is broken. Assert
enabled state to validate the permission contract.
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%2F38581&comment_hash=011ace98ffa4b71a811d8d1e5a3ee74299305a6fc46e3bfc10739861f1463465&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38581&comment_hash=011ace98ffa4b71a811d8d1e5a3ee74299305a6fc46e3bfc10739861f1463465&reaction=dislike'>👎</a>
##########
superset-frontend/src/SqlLab/components/ResultSet/index.tsx:
##########
@@ -427,30 +431,33 @@ const ResultSet = ({
}}
/>
)}
- {canExportData && (
- <CopyToClipboard
- text={prepareCopyToClipboardTabularData(
- data,
- columns.map(c => c.column_name),
- )}
- wrapped={false}
- copyNode={
- <Button
- buttonSize="small"
- variant="text"
- color="primary"
- icon={<Icons.CopyOutlined iconSize="m" />}
- tooltip={t('Copy to Clipboard')}
- aria-label={t('Copy to Clipboard')}
- data-test="copy-to-clipboard-button"
- />
- }
- hideTooltip
- onCopyEnd={() =>
- logAction(LOG_ACTIONS_SQLLAB_COPY_RESULT_TO_CLIPBOARD, {})
- }
- />
- )}
+ <CopyToClipboard
+ text={prepareCopyToClipboardTabularData(
+ data,
+ columns.map(c => c.column_name),
+ )}
Review Comment:
**Suggestion:** The clipboard payload is always materialized even when the
user cannot copy, which can serialize very large result sets unnecessarily and
cause avoidable UI slowdowns. Only build the clipboard text when copy
permission is granted. [performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ SQL Lab result header recomputes huge clipboard strings unnecessarily.
- ⚠️ Extra CPU and memory for users without clipboard permission.
- ⚠️ Potential UI lag on large result sets in SQL Lab.
```
</details>
```suggestion
text={
canCopyClipboard
? prepareCopyToClipboardTabularData(
data,
columns.map(c => c.column_name),
)
: ''
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable the `GRANULAR_EXPORT_CONTROLS` feature flag so that SQL Lab uses
granular
clipboard permissions, as wired in `usePermissions`
(`superset-frontend/src/hooks/usePermissions.ts:49-60`, where
`granularExport` toggles
`canCopyClipboardSqlLab` based on `can_copy_clipboard`).
2. Create or use a role that has SQL Lab access and export permissions
(`can_export_csv`
on `SQLLab` or `can_export_data` on `Superset`) but explicitly lacks
`can_copy_clipboard`
on `Superset`, so `canCopyClipboardSqlLab` resolves to `false` for that user
(`usePermissions.ts:46-60`).
3. Log in as that restricted user, open SQL Lab, and run a query that
returns a large
result set (e.g. tens of thousands of rows with many columns) so that
`query.results.data`
and `query.results.columns` are large when `ResultSet` renders
(`superset-frontend/src/SqlLab/components/ResultSet/index.tsx`,
`renderControls` function
around hunk lines 343-383 where `data` and `columns` are derived from
`query.results`).
4. Observe that on every render of `renderControls` (e.g. initial render,
typing in the
filter input, or other state changes), the `CopyToClipboard` component at
`ResultSet/index.tsx` hunk lines 435-441 calls
`prepareCopyToClipboardTabularData(data,
columns.map(...))` to eagerly build a full tab-delimited string for the
entire result set,
even though `canCopyClipboard` is `false` and the underlying button is
disabled. This
unnecessary O(rows × columns) loop and large string allocation can be seen in
`prepareCopyToClipboardTabularData`
(`superset-frontend/src/utils/common.ts:106-127`) and
can cause noticeable main-thread work and UI slowness for large results,
despite the user
not being allowed to copy.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/ResultSet/index.tsx
**Line:** 435:438
**Comment:**
*Performance: The clipboard payload is always materialized even when
the user cannot copy, which can serialize very large result sets unnecessarily
and cause avoidable UI slowdowns. Only build the clipboard text when copy
permission is granted.
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%2F38581&comment_hash=1489118a5e665d3c709deb4c11fb79cbf941a03032bd7ccf54f04bcdbce918b3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38581&comment_hash=1489118a5e665d3c709deb4c11fb79cbf941a03032bd7ccf54f04bcdbce918b3&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]
