From 967ee13e624f54e2b2d10bcb4f1d6ed56af7b6a4 Mon Sep 17 00:00:00 2001 From: Fredrik Strand Oseberg Date: Tue, 16 Jan 2024 13:47:04 +0100 Subject: [PATCH] fix: add symbols as constraint ids (#5913) This PR adds uuids as ids using a symbol in order to make sure we only use this to keep internal order in the viritual DOM. This makes us able to have predictable mutable lists on the frontend, and makes it easy to not pass this property along to the backend. --- .../Changes/Change/NewEditChange.tsx | 23 ++++++-- .../changeRequest/changeRequest.types.ts | 6 ++- .../createEmptyConstraint.ts | 4 ++ .../NewConstraintAccordionList.tsx | 54 ++++++++++--------- .../NewFeatureStrategyEdit.tsx | 20 ++++++- frontend/src/interfaces/strategy.ts | 2 + 6 files changed, 77 insertions(+), 32 deletions(-) diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/NewEditChange.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/NewEditChange.tsx index b2840c1f23..86e89f6c9b 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/NewEditChange.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/NewEditChange.tsx @@ -16,6 +16,8 @@ import { useChangeRequestsEnabled } from 'hooks/useChangeRequestsEnabled'; import { useChangeRequestApi } from 'hooks/api/actions/useChangeRequestApi/useChangeRequestApi'; import { comparisonModerator } from 'component/feature/FeatureStrategy/featureStrategy.utils'; import { + ChangeRequestAddStrategy, + ChangeRequestEditStrategy, IChangeRequestAddStrategy, IChangeRequestUpdateStrategy, } from 'component/changeRequest/changeRequest.types'; @@ -25,6 +27,8 @@ import { useUiFlag } from 'hooks/useUiFlag'; import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; import { NewFeatureStrategyForm } from 'component/feature/FeatureStrategy/FeatureStrategyForm/NewFeatureStrategyForm'; import { NewStrategyVariants } from 'component/feature/StrategyTypes/NewStrategyVariants'; +import { constraintId } from 'component/common/ConstraintAccordion/ConstraintAccordionList/createEmptyConstraint'; +import { v4 as uuidv4 } from 'uuid'; interface IEditChangeProps { change: IChangeRequestAddStrategy | IChangeRequestUpdateStrategy; @@ -36,6 +40,16 @@ interface IEditChangeProps { onClose: () => void; } +const addIdSymbolToConstraints = ( + strategy?: ChangeRequestAddStrategy | ChangeRequestEditStrategy, +) => { + if (!strategy) return; + + return strategy?.constraints.map((constraint) => { + return { ...constraint, [constraintId]: uuidv4() }; + }); +}; + export const NewEditChange = ({ change, changeRequestId, @@ -50,9 +64,12 @@ export const NewEditChange = ({ const [tab, setTab] = useState(0); const newStrategyConfiguration = useUiFlag('newStrategyConfiguration'); - const [strategy, setStrategy] = useState>( - change.payload, - ); + const constraintsWithId = addIdSymbolToConstraints(change.payload); + + const [strategy, setStrategy] = useState>({ + ...change.payload, + constraints: constraintsWithId, + }); const { segments: allSegments } = useSegments(); const strategySegments = (allSegments || []).filter((segment) => { diff --git a/frontend/src/component/changeRequest/changeRequest.types.ts b/frontend/src/component/changeRequest/changeRequest.types.ts index e46e9f11ab..75015cad06 100644 --- a/frontend/src/component/changeRequest/changeRequest.types.ts +++ b/frontend/src/component/changeRequest/changeRequest.types.ts @@ -216,7 +216,7 @@ type ChangeRequestEnabled = { enabled: boolean }; type ChangeRequestAddDependency = { feature: string }; -type ChangeRequestAddStrategy = Pick< +export type ChangeRequestAddStrategy = Pick< IFeatureStrategy, | 'parameters' | 'constraints' @@ -226,7 +226,9 @@ type ChangeRequestAddStrategy = Pick< | 'variants' > & { name: string }; -type ChangeRequestEditStrategy = ChangeRequestAddStrategy & { id: string }; +export type ChangeRequestEditStrategy = ChangeRequestAddStrategy & { + id: string; +}; type ChangeRequestDeleteStrategy = { id: string; diff --git a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionList/createEmptyConstraint.ts b/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionList/createEmptyConstraint.ts index 5ad8f8edfa..2b2a0c24aa 100644 --- a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionList/createEmptyConstraint.ts +++ b/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionList/createEmptyConstraint.ts @@ -2,6 +2,9 @@ import { dateOperators } from 'constants/operators'; import { IConstraint } from 'interfaces/strategy'; import { oneOf } from 'utils/oneOf'; import { operatorsForContext } from 'utils/operatorsForContext'; +import { v4 as uuidv4 } from 'uuid'; + +export const constraintId = Symbol('id'); export const createEmptyConstraint = (contextName: string): IConstraint => { const operator = operatorsForContext(contextName)[0]; @@ -17,5 +20,6 @@ export const createEmptyConstraint = (contextName: string): IConstraint => { values: [], caseInsensitive: false, inverted: false, + [constraintId]: uuidv4(), }; }; diff --git a/frontend/src/component/common/NewConstraintAccordion/NewConstraintAccordionList/NewConstraintAccordionList.tsx b/frontend/src/component/common/NewConstraintAccordion/NewConstraintAccordionList/NewConstraintAccordionList.tsx index be19f38377..dc38f9b180 100644 --- a/frontend/src/component/common/NewConstraintAccordion/NewConstraintAccordionList/NewConstraintAccordionList.tsx +++ b/frontend/src/component/common/NewConstraintAccordion/NewConstraintAccordionList/NewConstraintAccordionList.tsx @@ -1,16 +1,14 @@ -import React, { - forwardRef, - Fragment, - RefObject, - useImperativeHandle, -} from 'react'; -import { Button, styled, Tooltip } from '@mui/material'; +import React, { forwardRef, Fragment, useImperativeHandle } from 'react'; +import { styled, Tooltip } from '@mui/material'; import { HelpOutline } from '@mui/icons-material'; import { IConstraint } from 'interfaces/strategy'; import produce from 'immer'; import useUnleashContext from 'hooks/api/getters/useUnleashContext/useUnleashContext'; import { IUseWeakMap, useWeakMap } from 'hooks/useWeakMap'; -import { createEmptyConstraint } from 'component/common/ConstraintAccordion/ConstraintAccordionList/createEmptyConstraint'; +import { + constraintId, + createEmptyConstraint, +} from 'component/common/ConstraintAccordion/ConstraintAccordionList/createEmptyConstraint'; import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; import { StrategySeparator } from 'component/common/StrategySeparator/StrategySeparator'; import { NewConstraintAccordion } from 'component/common/NewConstraintAccordion/NewConstraintAccordion'; @@ -162,26 +160,30 @@ export const NewConstraintAccordionList = forwardRef< return ( - {constraints.map((constraint, index) => ( + {constraints.map((constraint, index) => { // biome-ignore lint: reason=objectId would change every time values change - this is no different than using index - - 0} - show={} - /> + const id = constraint[constraintId]; - - - ))} + return ( + + 0} + show={} + /> + + + + ); + })} ); }); diff --git a/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyEdit/NewFeatureStrategyEdit.tsx b/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyEdit/NewFeatureStrategyEdit.tsx index f5c1912c5d..09fc2e9640 100644 --- a/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyEdit/NewFeatureStrategyEdit.tsx +++ b/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyEdit/NewFeatureStrategyEdit.tsx @@ -28,6 +28,8 @@ import { usePendingChangeRequests } from 'hooks/api/getters/usePendingChangeRequ import { usePlausibleTracker } from 'hooks/usePlausibleTracker'; import { NewFeatureStrategyForm } from 'component/feature/FeatureStrategy/FeatureStrategyForm/NewFeatureStrategyForm'; import { NewStrategyVariants } from 'component/feature/StrategyTypes/NewStrategyVariants'; +import { constraintId } from 'component/common/ConstraintAccordion/ConstraintAccordionList/createEmptyConstraint'; +import { v4 as uuidv4 } from 'uuid'; const useTitleTracking = () => { const [previousTitle, setPreviousTitle] = useState(''); @@ -75,6 +77,14 @@ const useTitleTracking = () => { }; }; +const addIdSymbolToConstraints = (strategy?: IFeatureStrategy) => { + if (!strategy) return; + + return strategy?.constraints.map((constraint) => { + return { ...constraint, [constraintId]: uuidv4() }; + }); +}; + export const NewFeatureStrategyEdit = () => { const projectId = useRequiredPathParam('projectId'); const featureId = useRequiredPathParam('featureId'); @@ -133,7 +143,15 @@ export const NewFeatureStrategyEdit = () => { const savedStrategy = data?.environments .flatMap((environment) => environment.strategies) .find((strategy) => strategy.id === strategyId); - setStrategy((prev) => ({ ...prev, ...savedStrategy })); + + const constraintsWithId = addIdSymbolToConstraints(savedStrategy); + + const formattedStrategy = { + ...savedStrategy, + constraints: constraintsWithId, + }; + + setStrategy((prev) => ({ ...prev, ...formattedStrategy })); setPreviousTitle(savedStrategy?.title || ''); }, [strategyId, data]); diff --git a/frontend/src/interfaces/strategy.ts b/frontend/src/interfaces/strategy.ts index 6ac90c33dd..1b858d8e74 100644 --- a/frontend/src/interfaces/strategy.ts +++ b/frontend/src/interfaces/strategy.ts @@ -1,5 +1,6 @@ import { Operator } from 'constants/operators'; import { IFeatureVariant } from './featureToggle'; +import { constraintId } from 'component/common/ConstraintAccordion/ConstraintAccordionList/createEmptyConstraint'; export interface IFeatureStrategy { id: string; @@ -61,6 +62,7 @@ export interface IConstraint { caseInsensitive?: boolean; operator: Operator; contextName: string; + [constraintId]?: string; } export interface IFeatureStrategySortOrder {