korbit-ai[bot] commented on code in PR #35001:
URL: https://github.com/apache/superset/pull/35001#discussion_r2319723693
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -557,13 +561,10 @@ const DashboardBuilder = () => {
],
);
- const dashboardContentMarginLeft =
- !dashboardFiltersOpen &&
- !editMode &&
- nativeFiltersEnabled &&
- filterBarOrientation !== FilterBarOrientation.Horizontal
- ? 0
- : theme.sizeUnit * 8;
+ const dashboardContentMarginLeft = useMemo(
+ () => (!editMode ? theme.sizeUnit * 4 : theme.sizeUnit * 8),
+ [editMode, theme.sizeUnit],
+ );
Review Comment:
### Unnecessary Memoization of Simple Calculation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The useMemo hook is being used for a simple calculation that doesn't justify
the overhead of memoization.
###### Why this matters
Using useMemo for simple calculations can be less performant than doing the
calculation directly, as it requires maintaining the memoization infrastructure
in memory.
###### Suggested change ∙ *Feature Preview*
Remove useMemo and perform the calculation directly:
```typescript
const dashboardContentMarginLeft = !editMode ? theme.sizeUnit * 4 :
theme.sizeUnit * 8;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bec4e7d3-71f6-4b7a-8268-f8ef6ed2979e/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bec4e7d3-71f6-4b7a-8268-f8ef6ed2979e?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bec4e7d3-71f6-4b7a-8268-f8ef6ed2979e?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bec4e7d3-71f6-4b7a-8268-f8ef6ed2979e?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bec4e7d3-71f6-4b7a-8268-f8ef6ed2979e)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:73da7b3c-e267-48bc-ace7-656857eb8287 -->
[](73da7b3c-e267-48bc-ace7-656857eb8287)
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.tsx:
##########
@@ -113,9 +119,12 @@
`}
`;
-type Props = {};
+type Props = {
+ children: ReactNode;
+ dashboardFiltersOpen: boolean;
+};
-const DashboardWrapper: FC<Props> = ({ children }) => {
+const DashboardWrapper: FC<Props> = ({ children, dashboardFiltersOpen }) => {
Review Comment:
### Missing Default Prop Value for Layout Control <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The `dashboardFiltersOpen` prop lacks a default value, which could cause
layout issues if not explicitly provided.
###### Why this matters
Without a default value, if a consumer forgets to provide this prop, the
component's padding logic could behave unexpectedly, affecting the dashboard's
layout.
###### Suggested change ∙ *Feature Preview*
Add a default value to the `dashboardFiltersOpen` prop:
```typescript
const DashboardWrapper: FC<Props> = ({ children, dashboardFiltersOpen =
false }) => {
```
Or define the prop type with a default:
```typescript
type Props = {
children: ReactNode;
dashboardFiltersOpen?: boolean;
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a410d391-dc9c-4601-ac96-472e697b5d40/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a410d391-dc9c-4601-ac96-472e697b5d40?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a410d391-dc9c-4601-ac96-472e697b5d40?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a410d391-dc9c-4601-ac96-472e697b5d40?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a410d391-dc9c-4601-ac96-472e697b5d40)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e5fea1a4-b829-42d6-86c4-f91764999c42 -->
[](e5fea1a4-b829-42d6-86c4-f91764999c42)
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.tsx:
##########
@@ -26,8 +26,8 @@ import { useDragDropManager } from 'react-dnd';
import classNames from 'classnames';
import { debounce } from 'lodash';
-const StyledDiv = styled.div`
- ${({ theme }) => css`
+const StyledDiv = styled.div<{ dashboardFiltersOpen: boolean }>`
Review Comment:
### Inefficient Dynamic Styled-Component <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The styled-component is recreating CSS rules on every prop change of
dashboardFiltersOpen, which can cause unnecessary style recalculations.
###### Why this matters
Dynamic styles based on props in styled-components can lead to stylesheet
regeneration and browser reflow, impacting performance when the prop changes
frequently.
###### Suggested change ∙ *Feature Preview*
Consider using CSS classes or inline styles for the dynamic padding value
instead:
```typescript
const StyledDiv = styled.div`
${({ theme }) => css`
// ... static styles ...
`}
`;
// In component:
<StyledDiv
className={dashboardFiltersOpen ? 'filters-open' : ''}
style={{
'--tab-padding': dashboardFiltersOpen ? '0' : `${theme.sizeUnit * 2}px`
}}
>
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d9f13e3e-aa09-4570-9c1c-d7c2d5ab3e72/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d9f13e3e-aa09-4570-9c1c-d7c2d5ab3e72?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d9f13e3e-aa09-4570-9c1c-d7c2d5ab3e72?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d9f13e3e-aa09-4570-9c1c-d7c2d5ab3e72?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d9f13e3e-aa09-4570-9c1c-d7c2d5ab3e72)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:0357cd39-3378-4857-9d7d-7e16487ae988 -->
[](0357cd39-3378-4857-9d7d-7e16487ae988)
--
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]
