Skip to content

Commit

Permalink
[StaticSetFilter] Remove potential layoutEffect infinite loop (#24542)
Browse files Browse the repository at this point in the history
## Summary & Motivation
Fix potential infinite loop caused by useLayoutEffect and
isFirstRenderRef. The layout and regular effects were both gated by
isFirstRenderRef, but since the effect is responsible for setting this
flag, React's behavior allows useLayoutEffect to loop. This PR separates
the refs for layout and regular effects to prevent the looping issue.


## How I Tested These Changes

jest tests

## Changelog

NOCHANGELOG
  • Loading branch information
salazarm authored Sep 17, 2024
1 parent 025390f commit dfa8d75
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,9 @@ export function useQueryPersistedState<T extends QueryPersistedDataType>(
}

// Check if the query has changed. If so, perform a replace. Otherwise, do nothing
// to ensure that we don't end up in a `replace` loop.
if (!areQueriesEqual(currentQueryString, next)) {
// to ensure that we don't end up in a `replace` loop. If we're not in prod, always run
// the `replace` so that we surface any unwanted loops during development.
if (process.env.NODE_ENV !== 'production' || !areQueriesEqual(currentQueryString, next)) {
currentQueryString = next;
history.replace(`${location.pathname}?${qs.stringify(next, {arrayFormat: 'brackets'})}`);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import {
ComponentProps,
Fragment,
useContext,
useEffect,
useLayoutEffect,
useMemo,
useRef,
Expand Down Expand Up @@ -69,7 +68,7 @@ export function useStaticSetFilter<TValue>({
renderActiveStateLabel,
filterBarTagLabel,
isLoadingFilters,
state,
state: controlledState,
getStringValue,
getTooltipText,
onStateChanged,
Expand All @@ -91,32 +90,34 @@ export function useStaticSetFilter<TValue>({
}, [StaticFilterSorter, name, _unsortedValues]);

// This filter can be used as both a controlled and an uncontrolled component necessitating an innerState for the uncontrolled case.
const [innerState, setState] = useState(() => new Set(state || []));
const [innerStateRef, setInnerState] = useState(() => ({
current: new Set(controlledState || []),
}));

const isFirstRenderRef = useRef(true);
const isFirstLayoutEffectRender = useRef(true);

useLayoutEffect(() => {
if (!isFirstRenderRef.current) {
onStateChanged?.(innerState);
if (!isFirstLayoutEffectRender.current) {
onStateChanged?.(innerStateRef.current);
}
isFirstLayoutEffectRender.current = false;
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [innerState]);
}, [innerStateRef.current]);

useEffect(() => {
if (!isFirstRenderRef.current) {
setState(state ? new Set(state) : new Set());
}
isFirstRenderRef.current = false;
}, [state]);
useMemo(() => {
innerStateRef.current = new Set(controlledState);

// eslint-disable-next-line react-hooks/exhaustive-deps
}, [controlledState]);

const currentQueryRef = useRef<string>('');

const filterObj: StaticSetFilter<TValue> = useMemo(
() => ({
name,
icon,
state: innerState,
isActive: innerState.size > 0,
state: innerStateRef.current,
isActive: innerStateRef.current.size > 0,
isLoadingFilters,
getResults: (query) => {
currentQueryRef.current = query;
Expand Down Expand Up @@ -160,7 +161,9 @@ export function useStaticSetFilter<TValue>({
value={selectAllSymbol}
renderLabel={() => <>{selectAllText ?? 'Select all'}</>}
isForcedActive={
(state instanceof Set ? state.size : state?.length) === allValues.length
(controlledState instanceof Set
? controlledState.size
: controlledState?.length) === allValues.length
}
filter={filterObjRef.current}
allowMultipleSelections={allowMultipleSelections}
Expand Down Expand Up @@ -196,13 +199,15 @@ export function useStaticSetFilter<TValue>({
return true;
});
if (hasAnyUnselected) {
setState(new Set([...Array.from(state), ...selectResults.map(({value}) => value)]));
setInnerState({
current: new Set([...Array.from(state), ...selectResults.map(({value}) => value)]),
});
} else {
const stateCopy = new Set(state);
selectResults.forEach(({value}) => {
stateCopy.delete(value);
});
setState(stateCopy);
setInnerState({current: stateCopy});
}
return;
}
Expand All @@ -216,7 +221,7 @@ export function useStaticSetFilter<TValue>({
newState.add(value);
}
}
setState(newState);
setInnerState({current: newState});
if (closeOnSelect) {
close();
}
Expand All @@ -233,26 +238,26 @@ export function useStaticSetFilter<TValue>({
activeJSX: (
<SetFilterActiveState
name={name}
state={innerState}
state={innerStateRef.current}
getStringValue={getStringValue}
getTooltipText={getTooltipText}
renderLabel={renderActiveStateLabel || renderLabel}
onRemove={() => {
setState(new Set());
setInnerState({current: new Set()});
}}
icon={icon}
matchType={matchType}
filterBarTagLabel={filterBarTagLabel}
/>
),
setState,
setState: (val) => setInnerState({current: val}),
menuWidth,
}),
// eslint-disable-next-line react-hooks/exhaustive-deps
[
name,
icon,
innerState,
innerStateRef.current,
getStringValue,
renderActiveStateLabel,
renderLabel,
Expand Down

1 comment on commit dfa8d75

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-fnqkc2pa8-elementl.vercel.app

Built with commit dfa8d75.
This pull request is being automatically deployed with vercel-action

Please sign in to comment.