korbit-ai[bot] commented on code in PR #35201:
URL: https://github.com/apache/superset/pull/35201#discussion_r2361412933
##########
superset-frontend/src/pages/DashboardList/index.tsx:
##########
@@ -300,6 +300,14 @@ function DashboardList(props: DashboardListProps) {
);
}
+ const hasFavoritesOnPage = useMemo(
+ () =>
+ dashboards.length > 0 &&
+ Object.keys(favoriteStatus).length === dashboards.length &&
+ Object.values(favoriteStatus).some(status => status === true),
+ [dashboards.length, favoriteStatus],
+ );
Review Comment:
### Extract Complex Favorites Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The hasFavoritesOnPage calculation is complex and its purpose is not
immediately clear from the implementation. The logic for determining if there
are favorites on the page is embedded directly in the component.
###### Why this matters
Complex boolean logic embedded in components makes the code harder to
maintain and test. If this favorites logic needs to be reused or modified, it
would require changing the component directly.
###### Suggested change ∙ *Feature Preview*
Create a separate utility function to handle the favorites logic:
```typescript
const hasFavoritesOnPage = (dashboards: Dashboard[], favoriteStatus:
Record<number, boolean>) => {
if (dashboards.length === 0) return false;
if (Object.keys(favoriteStatus).length !== dashboards.length) return false;
return Object.values(favoriteStatus).some(status => status === true);
};
// In component:
const hasFavoritesOnCurrentPage = useMemo(
() => hasFavoritesOnPage(dashboards, favoriteStatus),
[dashboards, favoriteStatus]
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef18f5a6-41ee-4066-9e11-ea96279f4e38/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef18f5a6-41ee-4066-9e11-ea96279f4e38?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef18f5a6-41ee-4066-9e11-ea96279f4e38?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef18f5a6-41ee-4066-9e11-ea96279f4e38?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef18f5a6-41ee-4066-9e11-ea96279f4e38)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:1330746d-7b1f-488c-b3e3-c56295117cbb -->
[](1330746d-7b1f-488c-b3e3-c56295117cbb)
##########
superset-frontend/src/pages/ChartList/index.tsx:
##########
@@ -315,6 +315,14 @@ function ChartList(props: ChartListProps) {
};
};
+ const hasFavoritesOnPage = useMemo(
+ () =>
+ charts.length > 0 &&
+ Object.keys(favoriteStatus).length === charts.length &&
+ Object.values(favoriteStatus).some(status => status === true),
+ [charts.length, favoriteStatus],
+ );
Review Comment:
### Inefficient object iteration in useMemo <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The useMemo computation performs unnecessary Object.keys() and
Object.values() operations on every render when favoriteStatus changes, even
when the result would be the same.
###### Why this matters
This creates unnecessary computational overhead as Object.keys() and
Object.values() iterate through the entire favoriteStatus object on each
evaluation, which could be expensive with large datasets.
###### Suggested change ∙ *Feature Preview*
Optimize by using a more efficient approach that avoids full object
iteration:
```typescript
const hasFavoritesOnPage = useMemo(
() => {
if (charts.length === 0) return false;
const statusKeys = Object.keys(favoriteStatus);
if (statusKeys.length !== charts.length) return false;
// Use for...in loop to short-circuit on first true value
for (const id in favoriteStatus) {
if (favoriteStatus[id] === true) return true;
}
return false;
},
[charts.length, favoriteStatus],
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4bca0a2a-1ac9-4c5a-9e2c-1d4c009fe772/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4bca0a2a-1ac9-4c5a-9e2c-1d4c009fe772?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4bca0a2a-1ac9-4c5a-9e2c-1d4c009fe772?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4bca0a2a-1ac9-4c5a-9e2c-1d4c009fe772?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4bca0a2a-1ac9-4c5a-9e2c-1d4c009fe772)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:54414f41-629f-440b-90c6-d1bad62d6961 -->
[](54414f41-629f-440b-90c6-d1bad62d6961)
--
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]
