korbit-ai[bot] commented on code in PR #33054:
URL: https://github.com/apache/superset/pull/33054#discussion_r2038203834
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -287,6 +346,22 @@ export default function PluginFilterSelect(props:
PluginFilterSelectProps) {
setDataMask(dataMask);
}, [JSON.stringify(dataMask)]);
+ const handleExcludeCheckboxChange = (checked: boolean) => {
+ setExcludeFilterValues(checked);
+ };
+
+ useEffect(() => {
+ dispatchDataMask({
+ type: 'filterState',
+ extraFormData: { ...dataMask.extraFormData },
+ filterState: {
+ ...dataMask.filterState,
+ value: dataMask.filterState?.value || [],
+ excludeFilterValues,
+ },
+ });
+ }, [excludeFilterValues]);
Review Comment:
### Incomplete Effect Dependencies <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The useEffect dependency array only includes excludeFilterValues but the
effect uses dataMask which is not included in the dependencies.
###### Why this matters
Missing dependencies can lead to stale closures and outdated values being
used in the effect, potentially causing race conditions and inconsistent state
updates.
###### Suggested change ∙ *Feature Preview*
Add dataMask to the dependency array or extract the required dataMask values
outside the effect:
```typescript
useEffect(() => {
const currentValue = dataMask.filterState?.value || [];
const currentExtraFormData = dataMask.extraFormData;
dispatchDataMask({
type: 'filterState',
extraFormData: { ...currentExtraFormData },
filterState: {
...dataMask.filterState,
value: currentValue,
excludeFilterValues,
},
});
}, [dataMask, excludeFilterValues]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/df7210af-97dd-47be-b0a5-cf84c4de7381/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/df7210af-97dd-47be-b0a5-cf84c4de7381?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/df7210af-97dd-47be-b0a5-cf84c4de7381?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/df7210af-97dd-47be-b0a5-cf84c4de7381?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/df7210af-97dd-47be-b0a5-cf84c4de7381)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:dc3c73a9-922e-427a-b78a-90e4fc62c1d1 -->
[](dc3c73a9-922e-427a-b78a-90e4fc62c1d1)
##########
superset-frontend/src/components/DropdownContainer/index.tsx:
##########
@@ -179,6 +179,46 @@ const DropdownContainer = forwardRef(
[items, overflowingIndex],
);
+ useEffect(() => {
+ const container = current?.children.item(0);
+ if (!container) return;
+
+ const childrenArray = Array.from(container.children);
+
+ const resizeObserver = new ResizeObserver(() => {
+ recalculateItemWidths();
+ });
+
+ for (const child of childrenArray) {
+ resizeObserver.observe(child);
+ }
+
+ // eslint-disable-next-line consistent-return
+ return () => {
+ for (const child of childrenArray) {
+ resizeObserver.unobserve(child);
+ }
+ resizeObserver.disconnect();
+ };
+ }, [current, items.length]);
+
+ // callback to update item widths so that the useLayoutEffect runs whenever
+ // width of any of the child changes
+ const recalculateItemWidths = () => {
+ const container = current?.children.item(0);
+ if (container) {
+ const { children } = container;
+ const childrenArray = Array.from(children);
+
+ const currentWidths = childrenArray.map(
+ child => child.getBoundingClientRect().width,
+ );
Review Comment:
### Layout Thrashing in Width Calculations <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Multiple getBoundingClientRect() calls in a loop force layout reflows for
each iteration.
###### Why this matters
Each getBoundingClientRect() call forces the browser to recalculate layout,
causing performance degradation with many items.
###### Suggested change ∙ *Feature Preview*
Batch the layout reads to prevent multiple reflows:
```typescript
const widths = [];
// Force a single reflow before reading
childrenArray[0]?.getBoundingClientRect();
for (const child of childrenArray) {
widths.push(child.getBoundingClientRect().width);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/52d74cc5-0bc9-4de6-96e9-298a01d28545/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/52d74cc5-0bc9-4de6-96e9-298a01d28545?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/52d74cc5-0bc9-4de6-96e9-298a01d28545?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/52d74cc5-0bc9-4de6-96e9-298a01d28545?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/52d74cc5-0bc9-4de6-96e9-298a01d28545)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:56aea8ce-8946-418e-90cd-7501b3a7d76c -->
[](56aea8ce-8946-418e-90cd-7501b3a7d76c)
--
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]
