korbit-ai[bot] commented on code in PR #32514:
URL: https://github.com/apache/superset/pull/32514#discussion_r2031813398
##########
superset-frontend/src/dashboard/actions/nativeFilters.ts:
##########
@@ -122,7 +122,7 @@ export const setInScopeStatusOfFilters =
// need to update native_filter_configuration in the dashboard metadata
const metadata = cloneDeep(getState().dashboardInfo.metadata);
Review Comment:
### Inefficient Deep Clone of Entire Metadata <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Deep cloning the entire metadata object is inefficient when only the
native_filter_configuration needs to be modified.
###### Why this matters
The cloneDeep operation has O(n) complexity where n is the size of the
entire metadata object. This creates unnecessary overhead by copying all
metadata properties when only the filter configuration needs modification.
###### Suggested change ∙ *Feature Preview*
Only clone the required native_filter_configuration array and create a new
metadata object with spread operator:
```typescript
const { metadata } = getState().dashboardInfo;
const filterConfig = [...(metadata.native_filter_configuration || [])];
// After modifications
const newMetadata = { ...metadata, native_filter_configuration:
mergedFilterConfig };
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f88aa9eb-8ba8-43ca-a7ec-366ad71a1230/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f88aa9eb-8ba8-43ca-a7ec-366ad71a1230?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f88aa9eb-8ba8-43ca-a7ec-366ad71a1230?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f88aa9eb-8ba8-43ca-a7ec-366ad71a1230?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f88aa9eb-8ba8-43ca-a7ec-366ad71a1230)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6f634df5-4ec1-409a-8de3-c746b49d969b -->
[](6f634df5-4ec1-409a-8de3-c746b49d969b)
##########
superset-frontend/src/components/Select/index.tsx:
##########
@@ -20,3 +20,7 @@ import Select from './Select';
import AsyncSelect from './AsyncSelect';
export { Select, AsyncSelect };
+export * from './types';
+export * from './styles';
+export * from './CustomTag';
+export * from './constants';
Review Comment:
### Avoid wildcard exports for better API clarity <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using wildcard exports (*) makes the public API less explicit and can lead
to unintended exports. It also makes it harder to track dependencies and
maintain backwards compatibility.
###### Why this matters
When consumers import from this module, they won't know exactly which
symbols are available without checking the source files. This can lead to
accidental dependencies and make refactoring more difficult.
###### Suggested change ∙ *Feature Preview*
Replace wildcard exports with explicit named exports:
```typescript
export { type SelectProps, type OptionType } from './types';
export { selectStyles, tagStyles } from './styles';
export { CustomTag } from './CustomTag';
export { SELECT_ALL_VALUE, DEFAULT_OPTION } from './constants';
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d869960-5aa7-4f0b-8798-207f77abd0f8/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d869960-5aa7-4f0b-8798-207f77abd0f8?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d869960-5aa7-4f0b-8798-207f77abd0f8?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d869960-5aa7-4f0b-8798-207f77abd0f8?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d869960-5aa7-4f0b-8798-207f77abd0f8)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:73492b9d-9b7b-406d-90f7-bc20d582914d -->
[](73492b9d-9b7b-406d-90f7-bc20d582914d)
##########
superset-frontend/src/GlobalStyles.tsx:
##########
@@ -0,0 +1,88 @@
+/**
+ * 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 { css } from '@superset-ui/core';
+import { Global } from '@emotion/react';
+import 'react-js-cron/dist/styles.css';
+
+export const GlobalStyles = () => (
+ <Global
+ styles={theme => css`
+ body {
+ background-color: ${theme.colorBgBase};
+ }
+
+ h1,
+ h2,
+ h3,
+ h4,
+ h5,
+ h6,
+ strong,
+ th {
+ font-weight: ${theme.fontWeightStrong};
+ }
+ // CSS hack to resolve the issue caused by the invisible echart tooltip
on
+ // https://github.com/apache/superset/issues/30058
+ .echarts-tooltip[style*='visibility: hidden'] {
+ display: none !important;
+ }
+
+ // Ant Design is applying inline z-index styles causing troubles
+ // TODO: Remove z-indexes when Ant Design is fully upgraded to v5
+ // Prefer vanilla Ant Design z-indexes that should work out of the box
+ .antd5-dropdown,
+ .ant-dropdown,
+ .antd5-select-dropdown,
+ .antd5-modal-wrap,
+ .antd5-modal-mask,
+ .antd5-picker-dropdown,
+ .ant-popover,
+ .antd5-popover {
+ z-index: ${theme.zIndexPopupBase} !important;
+ }
+
+ // TODO: Remove when buttons have been upgraded to Ant Design 5.
+ // Check src/components/Modal for more info.
+ .column-config-popover {
+ & .antd5-input-number {
+ width: 100%;
+ }
+ && .btn-group svg {
+ line-height: 0;
+ top: 0;
+ }
+ & .btn-group > .btn {
+ padding: 5px 10px 6px;
+ }
Review Comment:
### Unexplained magic numbers in padding <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Magic numbers used in padding values (5px, 10px, 6px) without using theme
variables or explaining the specific values.
###### Why this matters
Hard-coded pixel values make it difficult to maintain consistent spacing and
adapt to theme changes across the application.
###### Suggested change ∙ *Feature Preview*
Use theme variables for consistent spacing:
```css
& .btn-group > .btn {
padding: ${theme.sizeUnit}px ${theme.sizeUnit * 2}px ${theme.sizeUnit +
1}px;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9b69d425-ade0-4800-b411-cbc39b8ecc4c/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9b69d425-ade0-4800-b411-cbc39b8ecc4c?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9b69d425-ade0-4800-b411-cbc39b8ecc4c?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9b69d425-ade0-4800-b411-cbc39b8ecc4c?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9b69d425-ade0-4800-b411-cbc39b8ecc4c)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9a380581-9b41-4d3b-bc3b-62c2da7dd14b -->
[](9a380581-9b41-4d3b-bc3b-62c2da7dd14b)
##########
superset-frontend/cypress-base/cypress/utils/index.ts:
##########
@@ -143,3 +143,29 @@ export function resize(selector: string) {
},
};
}
+
+export const setSelectSearchInput = (
+ $input: any,
+ value: string,
+ async = false,
+) => {
+ // Ant Design 5 Select crashes Chromium with type/click events when
showSearch is true.
+ // This copies the value directly to the input element as a workaround.
+ const nativeInputValueSetter = Object.getOwnPropertyDescriptor(
+ window.HTMLInputElement.prototype,
+ 'value',
+ )?.set;
Review Comment:
### Complex DOM manipulation needs inline explanation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The complex low-level DOM manipulation code lacks inline explanation of how
it bypasses standard event handling.
###### Why this matters
Without understanding the internal workings of HTMLInputElement and property
descriptors, developers may struggle to understand or modify this workaround
implementation.
###### Suggested change ∙ *Feature Preview*
```typescript
// Get the native setter for HTMLInputElement's value property
// This allows direct value setting, bypassing React's synthetic event
system
const nativeInputValueSetter = Object.getOwnPropertyDescriptor(
window.HTMLInputElement.prototype,
'value',
)?.set;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0cb3ddbd-b11f-4b62-aabf-97c15a0d149b/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0cb3ddbd-b11f-4b62-aabf-97c15a0d149b?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0cb3ddbd-b11f-4b62-aabf-97c15a0d149b?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0cb3ddbd-b11f-4b62-aabf-97c15a0d149b?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0cb3ddbd-b11f-4b62-aabf-97c15a0d149b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:cef7dfc7-dcdc-4fd9-865f-5444ff89e4fe -->
[](cef7dfc7-dcdc-4fd9-865f-5444ff89e4fe)
##########
superset-frontend/src/components/Select/Select.tsx:
##########
@@ -635,9 +647,8 @@ const Select = forwardRef(
onSelect={handleOnSelect}
onClear={handleClear}
placeholder={placeholder}
- showSearch={shouldShowSearch}
- showArrow
tokenSeparators={tokenSeparators}
+ virtual={fullSelectOptions.length > 20}
Review Comment:
### Arbitrary Virtual Scrolling Threshold <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Virtual scrolling threshold is hardcoded at 20 items without considering the
actual DOM height or performance impact.
###### Why this matters
Small lists with tall items might exceed viewport height before reaching 20
items, while lists with short items might not need virtualization at 20 items,
leading to suboptimal performance and unnecessary complexity.
###### Suggested change ∙ *Feature Preview*
Consider using a height-based approach or make the threshold configurable:
```typescript
const VIRTUAL_THRESHOLD = 100; // Can be made configurable via props
virtual={fullSelectOptions.length * ESTIMATED_ITEM_HEIGHT >
VIRTUAL_THRESHOLD}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/059a3187-c7b1-42d7-82ba-8b4e59aefde5/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/059a3187-c7b1-42d7-82ba-8b4e59aefde5?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/059a3187-c7b1-42d7-82ba-8b4e59aefde5?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/059a3187-c7b1-42d7-82ba-8b4e59aefde5?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/059a3187-c7b1-42d7-82ba-8b4e59aefde5)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:c4b277f1-1dc3-470c-bdbf-d27772414f8b -->
[](c4b277f1-1dc3-470c-bdbf-d27772414f8b)
##########
superset-frontend/cypress-base/cypress/e2e/explore/utils.ts:
##########
@@ -67,31 +71,40 @@ export function setFilter(filter: string, option: string) {
cy.wait('@filtering');
}
-export function saveChartToDashboard(dashboardName: string) {
+export function saveChartToDashboard(chartName: string, dashboardName: string)
{
interceptDashboardGet();
interceptUpdate();
interceptExploreGet();
+ interceptChartDashboardsGet();
cy.getBySel('query-save-button')
.should('be.enabled')
.should('not.be.disabled')
- .click();
- cy.getBySelLike('chart-modal').should('be.visible');
- cy.get(
- '[data-test="save-chart-modal-select-dashboard-form"] [aria-label="Select
a dashboard"]',
- )
- .first()
- .click();
- cy.get(
- '.ant-select-selection-search-input[aria-label="Select a dashboard"]',
- ).type(dashboardName, { force: true });
- cy.get(`.ant-select-item-option[title="${dashboardName}"]`).click();
- cy.getBySel('btn-modal-save').click();
-
- cy.wait('@update');
+ .click({ force: true });
+
+ cy.getBySel('save-modal-body')
+ .should('be.visible')
+ .then($modal => {
+ cy.wait(500);
Review Comment:
### Unreliable Fixed Wait Time <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using a hard-coded wait time of 500ms in the test could lead to flaky tests
as the timing might vary depending on system performance and network conditions.
###### Why this matters
Fixed wait times can cause tests to fail intermittently when the application
needs more or less time to respond, especially under different load conditions
or environments.
###### Suggested change ∙ *Feature Preview*
Replace the fixed wait with a more reliable wait condition that checks for
the actual state or element you're waiting for. For example:
```typescript
cy.wrap($modal)
.find('.antd5-select-selection-search-input[aria-label="Select a
dashboard"]')
.should('be.visible')
.type(dashboardName, { force: true });
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5a33ed84-659d-4a0a-86bd-46f3d848ad45/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5a33ed84-659d-4a0a-86bd-46f3d848ad45?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5a33ed84-659d-4a0a-86bd-46f3d848ad45?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5a33ed84-659d-4a0a-86bd-46f3d848ad45?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5a33ed84-659d-4a0a-86bd-46f3d848ad45)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a964d5e2-0230-4a31-ac7b-354c802d3e6b -->
[](a964d5e2-0230-4a31-ac7b-354c802d3e6b)
--
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]
