From 4ea2499ce7ee988c5da89dc860351818291fce89 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Wed, 14 May 2025 08:04:39 +0200 Subject: [PATCH] Chore(1-3688): improve performance for large lists of legal values (#9978) This PR implements a number of strategies to make the app perform better when you have large lists. For instance, we have a constraint field that has ~600 legal values. Currently in main, it is pretty slow and sloggy to use (about on par with what we see in hosted). With this PR, it becomes pretty snappy, as shown in this video (should hopefully be even better in production mode?): https://www.loom.com/share/2e882bee25a3454a85bec7752e8252dc?sid=7786b22d-6c60-47e8-bd71-cc5f347c4e0f The steps taken are: 1. Change the `useState` hook to instead use `useReducer`. The reason is that its dispatch function is guaranteed to have a stable identity. This lets us use it in memoized functions and components. 2. Because useReducer doesn't update the state variable until the next render, we need to use `useEffect` to update the constraint when it has actually updated instead of just calling it after the reducer. 3. Add a `toggle value` action and use that instead of checking whether the value is equal or not inside an onChange function. If we were to check the state of the value outside the reducer, the memoized function would be re-evaluated every time value or values change, which would result in more renders than necessary. By instead doing this kind of checking inside the reducer, we can cache more aggressively. 4. Because the onChange function can now be memoized, we can memoize all the legal value selector labels too. This is the real goal here, because we don't need to re-render 600 components, because one of them was checked. One side effect of using useEffect to call `onUpdate` is that it will also be called immediately when the hook is invoked the first time, but it will be called with the same value as the constraint that was passed in, so I don't think that's an issue. Second: the `useEffect` call uses `localConstraint` directly as a dep instead of stringifying it. I'm not sure why, but stringifying it makes it not update correctly for legal values. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../EditableConstraint.tsx | 117 +++++++----------- .../LegalValuesSelector.tsx | 92 ++++++++------ .../constraint-reducer.test.ts | 100 +++++++++++++++ .../constraint-reducer.ts | 110 +++++++++------- .../useEditableConstraint.test.tsx | 14 ++- .../useEditableConstraint.tsx | 23 ++-- 6 files changed, 283 insertions(+), 173 deletions(-) diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx index 35334c8e48..826f2f57ab 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx @@ -2,7 +2,7 @@ import { IconButton, styled } from '@mui/material'; import GeneralSelect from 'component/common/GeneralSelect/GeneralSelect'; import { isStringOperator, type Operator } from 'constants/operators'; import useUnleashContext from 'hooks/api/getters/useUnleashContext/useUnleashContext'; -import { useRef, type FC } from 'react'; +import { useCallback, useRef, type FC } from 'react'; import { operatorsForContext } from 'utils/operatorsForContext'; import { ConstraintOperatorSelect } from './ConstraintOperatorSelect'; import { HtmlTooltip } from 'component/common/HtmlTooltip/HtmlTooltip'; @@ -29,7 +29,6 @@ import { isNumberConstraint, isSemVerConstraint, } from './useEditableConstraint/editable-constraint-type'; -import type { ConstraintUpdateAction } from './useEditableConstraint/constraint-reducer'; import type { ConstraintValidationResult } from './useEditableConstraint/constraint-validator'; const Container = styled('article')(({ theme }) => ({ @@ -146,20 +145,22 @@ const StyledCaseSensitiveIcon = styled(CaseSensitiveIcon)(({ theme }) => ({ })); const TopRowInput: FC<{ + addValues: (value: string | string[]) => void; + clearValues: () => void; localConstraint: EditableConstraintType; - updateConstraint: (action: ConstraintUpdateAction) => void; validator: (value: string) => ConstraintValidationResult; addValuesButtonRef: React.RefObject; -}> = ({ localConstraint, updateConstraint, validator, addValuesButtonRef }) => { +}> = ({ + addValues, + clearValues, + localConstraint, + validator, + addValuesButtonRef, +}) => { if (isDateConstraint(localConstraint)) { return ( - updateConstraint({ - type: 'add value(s)', - payload: value, - }) - } + setValue={addValues} value={localConstraint.value} validator={validator} /> @@ -169,13 +170,8 @@ const TopRowInput: FC<{ return ( { - updateConstraint({ - type: 'add value(s)', - payload: newValue, - }); - }} - removeValue={() => updateConstraint({ type: 'clear values' })} + onAddValue={addValues} + removeValue={clearValues} currentValue={localConstraint.value} helpText={'A semver value should be of the format X.Y.Z'} inputType={'text'} @@ -186,13 +182,8 @@ const TopRowInput: FC<{ return ( { - updateConstraint({ - type: 'add value(s)', - payload: newValue, - }); - }} - removeValue={() => updateConstraint({ type: 'clear values' })} + onAddValue={addValues} + removeValue={clearValues} currentValue={localConstraint.value} helpText={'Add a single number'} inputType={'number'} @@ -205,12 +196,7 @@ const TopRowInput: FC<{ validator={validator} helpText='Maximum 100 char length per value' ref={addValuesButtonRef} - onAddValues={(newValues) => { - updateConstraint({ - type: 'add value(s)', - payload: newValues, - }); - }} + onAddValues={addValues} /> ); }; @@ -232,6 +218,31 @@ export const EditableConstraint: FC = ({ validator, legalValueData, } = useEditableConstraint(constraint, onUpdate); + const addValues = useCallback( + (value: string | string[]) => + updateConstraint({ type: 'add value(s)', payload: value }), + [updateConstraint], + ); + const removeValue = useCallback( + (value: string) => + updateConstraint({ type: 'remove value', payload: value }), + [updateConstraint], + ); + const clearAll = useCallback( + () => updateConstraint({ type: 'clear values' }), + [updateConstraint], + ); + const toggleValue = useCallback( + (value: string) => + updateConstraint({ type: 'toggle value', payload: value }), + [updateConstraint], + ); + const onOperatorChange = useCallback( + (operator: Operator) => { + updateConstraint({ type: 'set operator', payload: operator }); + }, + [updateConstraint], + ); const { context } = useUnleashContext(); const { contextName, operator } = localConstraint; @@ -247,10 +258,6 @@ export const EditableConstraint: FC = ({ return { key: context.name, label: context.name }; }); - const onOperatorChange = (operator: Operator) => { - updateConstraint({ type: 'set operator', payload: operator }); - }; - return ( @@ -334,12 +341,7 @@ export const EditableConstraint: FC = ({ ? [localConstraint.value] : undefined } - removeValue={(value) => { - updateConstraint({ - type: 'remove value', - payload: value, - }); - }} + removeValue={removeValue} getExternalFocusTarget={() => addValuesButtonRef.current ?? deleteButtonRef.current @@ -348,7 +350,8 @@ export const EditableConstraint: FC = ({ {legalValueData ? null : ( @@ -373,39 +376,15 @@ export const EditableConstraint: FC = ({ {isMultiValueConstraint(localConstraint) ? ( - updateConstraint({ - type: 'clear values', - }) - } - addValues={(newValues) => - updateConstraint({ - type: 'add value(s)', - payload: newValues, - }) - } - removeValue={(value) => - updateConstraint({ - type: 'remove value', - payload: value, - }) - } + toggleValue={toggleValue} + clearAll={clearAll} + addValues={addValues} {...legalValueData} /> ) : ( - updateConstraint({ - type: 'clear values', - }) - } - addValue={(newValues) => - updateConstraint({ - type: 'add value(s)', - payload: newValues, - }) - } {...legalValueData} /> )} diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/LegalValuesSelector.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/LegalValuesSelector.tsx index 9bdc15972b..df0c324ba1 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/LegalValuesSelector.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/LegalValuesSelector.tsx @@ -1,11 +1,19 @@ import { type FC, useId, useState } from 'react'; -import { Alert, Button, Checkbox, Radio, styled } from '@mui/material'; +import { + Alert, + Button, + Checkbox, + type CheckboxProps, + Radio, + styled, +} from '@mui/material'; import { filterLegalValues, LegalValueLabel, } from 'component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/LegalValueLabel/LegalValueLabel'; import { ConstraintValueSearch } from './ConstraintValueSearch'; import type { ILegalValue } from 'interfaces/context'; +import React from 'react'; const StyledValuesContainer = styled('div')(({ theme }) => ({ display: 'grid', @@ -129,19 +137,15 @@ const BaseLegalValueSelector: FC = ({ role={multiSelect ? undefined : 'radiogroup'} > {filteredValues.map((match) => ( - onChange(match.value)} - disabled={invalidLegalValues?.has(match.value)} - /> - } + Control={Control} + onChange={onChange} + groupNameId={groupNameId} + checked={isInputChecked(match.value)} + disabled={invalidLegalValues?.has(match.value)} /> ))} @@ -149,10 +153,44 @@ const BaseLegalValueSelector: FC = ({ ); }; +const MemoLabel = React.memo(function MemoLabel({ + legal, + filter, + Control, + onChange, + groupNameId, + checked, + disabled, +}: { + legal: ILegalValue; + filter: string; + Control: (props: CheckboxProps) => JSX.Element; + onChange: (value: string) => void; + checked: boolean; + groupNameId: string; + disabled?: boolean; +}) { + return ( + onChange(legal.value)} + disabled={disabled} + /> + } + /> + ); +}); + type LegalValuesSelectorProps = { values: Set; - addValues: (values: string[]) => void; - removeValue: (value: string) => void; + addValues: (value: string | string[]) => void; + toggleValue: (value: string) => void; clearAll: () => void; deletedLegalValues?: Set; invalidLegalValues?: Set; @@ -163,23 +201,15 @@ export const LegalValuesSelector = ({ legalValues, values, addValues, - removeValue, + toggleValue, clearAll, ...baseProps }: LegalValuesSelectorProps) => { - const onChange = (legalValue: string) => { - if (values.has(legalValue)) { - removeValue(legalValue); - } else { - addValues([legalValue]); - } - }; - return ( values.has(inputValue)} - onChange={onChange} + onChange={toggleValue} multiSelect={{ clearAll, selectAll: () => { @@ -194,30 +224,20 @@ export const LegalValuesSelector = ({ type SingleLegalValueSelectorProps = { value: string; - addValue: (value: string) => void; - clear: () => void; deletedLegalValues?: Set; legalValues: ILegalValue[]; + toggleValue: (value: string) => void; invalidLegalValues?: Set; }; export const SingleLegalValueSelector = ({ value, - addValue, - clear, + toggleValue, ...baseProps }: SingleLegalValueSelectorProps) => { - const onChange = (newValue: string) => { - if (value === newValue) { - clear(); - } else { - addValue(newValue); - } - }; - return ( inputValue === value} {...baseProps} /> diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.test.ts b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.test.ts index 80198261b8..3ce1678b09 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.test.ts +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.test.ts @@ -338,6 +338,106 @@ describe('adding values', () => { }); }); }); + +describe('toggling values', () => { + describe('single-value constraints', () => { + test('if the toggle value is the same as the existing value: clears the value', () => { + const input = singleValueConstraint(); + const output = constraintReducer(input, { + type: 'toggle value', + payload: input.value, + }); + expect(output).toStrictEqual({ + ...input, + value: '', + }); + }); + test('if the toggle value is different from the existing value: replaces it', () => { + const input = singleValueConstraint(); + const output = constraintReducer(input, { + type: 'toggle value', + payload: 'updated value', + }); + expect(output).toStrictEqual({ + ...input, + value: 'updated value', + }); + }); + + test('trying to add a deleted legal value results in no change', () => { + const input = singleValueConstraint(); + const output = constraintReducer( + input, + { + type: 'toggle value', + payload: 'deleted', + }, + new Set(['deleted']), + ); + expect(output).toStrictEqual(input); + }); + + test('if both the new value and the old value are deleted legal values, it clears the field', () => { + const input = singleValueConstraint(); + const output = constraintReducer( + input, + { + type: 'toggle value', + payload: 'deleted', + }, + new Set(['deleted', input.value]), + ); + expect(output).toStrictEqual({ + ...input, + value: '', + }); + }); + }); + describe('multi-value constraints', () => { + test('if not present, it will be added', () => { + const input = multiValueConstraint(); + const output = constraintReducer(input, { + type: 'toggle value', + payload: 'new-value', + }); + expect(output).toStrictEqual({ + ...input, + values: new Set([...input.values, 'new-value']), + }); + }); + test('if it is present, it will be removed', () => { + const input = multiValueConstraint(); + const output = constraintReducer(input, { + type: 'toggle value', + payload: 'B', + }); + expect(output).toStrictEqual({ + ...input, + values: new Set(['A']), + }); + }); + + test('deleted legal values are removed from the set before saving new values', () => { + const input = { + ...multiValueConstraint(), + values: new Set(['deleted-old', 'A']), + }; + const output = constraintReducer( + input, + { + type: 'toggle value', + payload: 'deleted-new', + }, + new Set(['deleted-old', 'deleted-new']), + ); + expect(output).toStrictEqual({ + ...input, + values: new Set(['A']), + }); + }); + }); +}); + describe('removing / clearing values', () => { describe('single-value constraints', () => { test('removing a value removes the existing value if it matches', () => { diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.ts b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.ts index 034e96f063..1149ae49cf 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.ts +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.ts @@ -14,6 +14,7 @@ import { type EditableConstraint, type EditableMultiValueConstraint, type EditableSingleValueConstraint, + isMultiValueConstraint, } from './editable-constraint-type'; import { difference, union } from './set-functions'; @@ -24,7 +25,8 @@ export type ConstraintUpdateAction = | { type: 'set context field'; payload: string } | { type: 'set operator'; payload: Operator } | { type: 'toggle case sensitivity' } - | { type: 'toggle inverted operator' }; + | { type: 'toggle inverted operator' } + | { type: 'toggle value'; payload: string }; const withValue = < T extends EditableConstraint & { value?: string; values?: Set }, @@ -50,6 +52,53 @@ export const constraintReducer = ( action: ConstraintUpdateAction, deletedLegalValues?: Set, ): EditableConstraint => { + const removeValue = (value: string) => { + if (isSingleValueConstraint(state)) { + if (state.value === value) { + return { + ...state, + value: '', + }; + } else { + return state; + } + } + const updatedValues = difference(state.values, new Set([value])); + return { + ...state, + values: updatedValues ?? new Set(), + }; + }; + + const addValue = (value: string | string[]) => { + if (isSingleValueConstraint(state)) { + const newValue = Array.isArray(value) ? value[0] : value; + if (deletedLegalValues?.has(newValue)) { + if (deletedLegalValues?.has(state.value)) { + return { + ...state, + value: '', + }; + } + return state; + } + return { + ...state, + value: newValue ?? '', + }; + } + + const newValues = new Set(Array.isArray(value) ? value : [value]); + const combinedValues = union(state.values, newValues); + const filteredValues = deletedLegalValues + ? difference(combinedValues, deletedLegalValues) + : combinedValues; + return { + ...state, + values: filteredValues, + }; + }; + switch (action.type) { case 'set context field': if (action.payload === state.contextName) { @@ -101,60 +150,25 @@ export const constraintReducer = ( operator: action.payload, } as EditableMultiValueConstraint); case 'add value(s)': { - if (isSingleValueConstraint(state)) { - const newValue = Array.isArray(action.payload) - ? action.payload[0] - : action.payload; - if (deletedLegalValues?.has(newValue)) { - if (deletedLegalValues?.has(state.value)) { - return { - ...state, - value: '', - }; - } - return state; - } - return { - ...state, - value: newValue ?? '', - }; - } - - const newValues = new Set( - Array.isArray(action.payload) - ? action.payload - : [action.payload], - ); - const combinedValues = union(state.values, newValues); - const filteredValues = deletedLegalValues - ? difference(combinedValues, deletedLegalValues) - : combinedValues; - return { - ...state, - values: filteredValues, - }; + return addValue(action.payload); } case 'toggle inverted operator': return { ...state, inverted: !state.inverted }; case 'toggle case sensitivity': return { ...state, caseInsensitive: !state.caseInsensitive }; case 'remove value': - if (isSingleValueConstraint(state)) { - if (state.value === action.payload) { - return { - ...state, - value: '', - }; - } else { - return state; - } - } - state.values.delete(action.payload); - return { - ...state, - values: state.values ?? new Set(), - }; + return removeValue(action.payload); case 'clear values': return withValue(null, state); + case 'toggle value': + if ( + (isSingleValueConstraint(state) && + state.value === action.payload) || + (isMultiValueConstraint(state) && + state.values.has(action.payload)) + ) { + return removeValue(action.payload); + } + return addValue(action.payload); } }; diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.test.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.test.tsx index 717dcdcf7b..280b7516cb 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.test.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.test.tsx @@ -20,7 +20,7 @@ const setupApi = (contextField?: ContextFieldSchema) => { testServerRoute(server, '/api/admin/context', [contextField]); }; -test('calls onUpdate with new state', () => { +test('calls onUpdate with new state', async () => { const initial: IConstraint = { contextName: 'context-field', operator: NOT_IN, @@ -32,15 +32,19 @@ test('calls onUpdate with new state', () => { const { result } = renderHook(() => useEditableConstraint(initial, onUpdate), ); + result.current.updateConstraint({ type: 'set operator', payload: IN, }); - expect(onUpdate).toHaveBeenCalledWith({ - contextName: 'context-field', - operator: IN, - values: [], + // gets called by useEffect, so we need to wait for the next render. + await waitFor(() => { + expect(onUpdate).toHaveBeenCalledWith({ + contextName: 'context-field', + operator: IN, + values: [], + }); }); }); diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.tsx index c126bb9b2f..460c1af624 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.tsx @@ -1,4 +1,4 @@ -import { useMemo, useState } from 'react'; +import { useEffect, useMemo, useReducer } from 'react'; import useUnleashContext from 'hooks/api/getters/useUnleashContext/useUnleashContext'; import type { IConstraint } from 'interfaces/strategy'; import { @@ -60,9 +60,13 @@ export const useEditableConstraint = ( constraint: IConstraint, onUpdate: (constraint: IConstraint) => void, ): EditableConstraintState => { - const [localConstraint, setLocalConstraint] = useState(() => { - return fromIConstraint(constraint); - }); + const [localConstraint, updateConstraint] = useReducer( + constraintReducer, + fromIConstraint(constraint), + ); + useEffect(() => { + onUpdate(toIConstraint(localConstraint)); + }, [localConstraint]); const { context } = useUnleashContext(); @@ -105,17 +109,6 @@ export const useEditableConstraint = ( JSON.stringify(localConstraint.operator), ]); - const updateConstraint = (action: ConstraintUpdateAction) => { - const nextState = constraintReducer( - localConstraint, - action, - deletedLegalValues, - ); - - setLocalConstraint(nextState); - onUpdate(toIConstraint(nextState)); - }; - const legalValueData = contextDefinition.legalValues?.length ? { legalValues: contextDefinition.legalValues,