korbit-ai[bot] commented on code in PR #36045:
URL: https://github.com/apache/superset/pull/36045#discussion_r2505794798
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -630,7 +636,9 @@ const PropertiesModal = ({
onFinish={onFinish}
onFieldsChange={() => {
// Re-validate sections when form fields change
- setTimeout(() => validateSection('basic'), 100);
+ if (!isLoading) {
+ setTimeout(() => validateSection('basic'), 100);
Review Comment:
### Unnecessary setTimeout in form validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using setTimeout with a fixed 100ms delay for form validation creates
unnecessary performance overhead and potential race conditions.
###### Why this matters
The arbitrary delay adds latency to validation feedback and may cause
validation to run after the component has unmounted or state has changed,
leading to stale closures and wasted computation cycles.
###### Suggested change ∙ *Feature Preview*
Remove the setTimeout wrapper and call validateSection('basic') directly, or
use a debounced validation approach if frequent validation calls are a concern:
```typescript
if (!isLoading) {
validateSection('basic');
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f4746199-4e1d-4176-926e-36d40c347950/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f4746199-4e1d-4176-926e-36d40c347950?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f4746199-4e1d-4176-926e-36d40c347950?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f4746199-4e1d-4176-926e-36d40c347950?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f4746199-4e1d-4176-926e-36d40c347950)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:566e274c-fc78-4cce-a648-087ae4ec839d -->
[](566e274c-fc78-4cce-a648-087ae4ec839d)
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -588,18 +588,24 @@ const PropertiesModal = ({
// Validate basic section when title changes
useEffect(() => {
- validateSection('basic');
- }, [dashboardTitle, validateSection]);
+ if (!isLoading) {
+ validateSection('basic');
+ }
+ }, [dashboardTitle, validateSection, isLoading]);
Review Comment:
### Validation skipped during loading state <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The validation logic prevents validation from running during loading state,
but this creates a gap where validation may never occur if the component
unmounts or state changes while loading.
###### Why this matters
If the modal is closed or the component unmounts while `isLoading` is true,
the validation will never run, potentially leaving the form in an invalid state
without user feedback. This could lead to silent validation failures.
###### Suggested change ∙ *Feature Preview*
Add a cleanup effect to ensure validation runs when loading completes, or
validate immediately after loading state changes:
```typescript
useEffect(() => {
validateSection('basic');
}, [dashboardTitle, validateSection]);
// Separate effect for post-loading validation
useEffect(() => {
if (!isLoading) {
validateSection('basic');
validateSection('advanced');
validateSection('refresh');
}
}, [isLoading, validateSection]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a40b12ae-3ac9-4000-be96-93ffe07d67ab/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a40b12ae-3ac9-4000-be96-93ffe07d67ab?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a40b12ae-3ac9-4000-be96-93ffe07d67ab?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a40b12ae-3ac9-4000-be96-93ffe07d67ab?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a40b12ae-3ac9-4000-be96-93ffe07d67ab)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:81bab707-fd49-4a35-a415-d8006de94fdc -->
[](81bab707-fd49-4a35-a415-d8006de94fdc)
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -630,7 +636,9 @@ const PropertiesModal = ({
onFinish={onFinish}
onFieldsChange={() => {
// Re-validate sections when form fields change
- setTimeout(() => validateSection('basic'), 100);
+ if (!isLoading) {
+ setTimeout(() => validateSection('basic'), 100);
Review Comment:
### Inconsistent validation timing in field changes <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The setTimeout validation in onFieldsChange callback is not protected by the
isLoading check consistently with other validation calls, and the 100ms delay
is arbitrary without cleanup.
###### Why this matters
This creates inconsistent validation behavior where field changes can
trigger validation during loading state while other triggers cannot. The
setTimeout also lacks cleanup, potentially causing validation to run after
component unmount.
###### Suggested change ∙ *Feature Preview*
Use consistent validation logic and add proper cleanup:
```typescript
onFieldsChange={() => {
if (!isLoading) {
// Use a ref to track the timeout for cleanup
const timeoutId = setTimeout(() => validateSection('basic'), 100);
return () => clearTimeout(timeoutId);
}
}}
```
Or better yet, remove the arbitrary delay:
```typescript
onFieldsChange={() => {
if (!isLoading) {
validateSection('basic');
}
}}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/aafc27af-3e3e-4eec-a206-d776f759aebf/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/aafc27af-3e3e-4eec-a206-d776f759aebf?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/aafc27af-3e3e-4eec-a206-d776f759aebf?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/aafc27af-3e3e-4eec-a206-d776f759aebf?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/aafc27af-3e3e-4eec-a206-d776f759aebf)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:14756d24-c514-4b3d-bdf5-e2ed456d8d5d -->
[](14756d24-c514-4b3d-bdf5-e2ed456d8d5d)
--
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]
