fix: resync tracked properties editor state#9387
Conversation
🔒 AI Validation PendingThis PR is from an external contributor. AI validation will be performed after manual review by a maintainer. Maintainers: Add the |
There was a problem hiding this comment.
Pull request overview
This PR aims to fix “tracked properties” editor desynchronization by ensuring tracked properties are not passed around by reference and by keeping the dictionary editor UI state aligned with refreshed parent values.
Changes:
- Deep-clone
trackedPropertieswhen extracting settings from the action definition to avoid shared-reference mutations. - Add prop→state synchronization to
SimpleDictionaryso the editor reflects refreshed incoming values. - Add a unit test covering rerendering with a changed incoming dictionary value.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| libs/designer-v2/src/lib/core/actions/bjsworkflow/settings.ts | Clone trackedProperties before returning it as a setting value to prevent shared-reference state issues. |
| libs/designer-ui/src/lib/settings/settingsection/dictionary/simpledictionary.tsx | Add a useEffect to resync internal editor rows when the value prop changes. |
| libs/designer-ui/src/lib/settings/settingsection/tests/simpledictionary.spec.tsx | New test to verify the dictionary editor updates when the incoming value changes on rerender. |
Comments suppressed due to low confidence (1)
libs/designer-ui/src/lib/settings/settingsection/dictionary/simpledictionary.tsx:55
- The new
useEffect(() => setValues(createValues(value)), [value])turnsSimpleDictionaryinto a controlled-ish component, but the existinguseEffectthat callsonChangeon everyvaluesupdate will now create a feedback loop for the common patternvalue={state} onChange={setState}(e.g.payloadPopover.tsxusesonChange={setHeadersValue}), potentially causing repeated re-renders / "Maximum update depth exceeded". To avoid this, only emitonChangewhen the derived dictionary actually differs from the incomingvalue(and includevalue/onChangein deps so the eslint suppression can be removed).
// Keep the editor in sync if the tracked-properties value is refreshed from the parent.
useEffect(() => {
setValues(createValues(value));
}, [value]);
useEffect(() => {
onChange?.(
values
.filter((x) => x.key && x.key !== '')
.reduce((acc: any, val) => {
acc[val.key] = val.value;
return acc;
}, {})
);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [values]);
| const getTrackedProperties = (isTrigger: boolean, manifest?: OperationManifest, definition?: LogicAppsV2.ActionDefinition): any => { | ||
| const supported = areTrackedPropertiesSupported(isTrigger, manifest); | ||
| return supported && definition ? getPropertyValue(definition as any, 'trackedProperties') : undefined; | ||
| const trackedProperties = supported && definition ? getPropertyValue(definition as any, 'trackedProperties') : undefined; | ||
| return trackedProperties ? clone(trackedProperties) : trackedProperties; | ||
| }; |
| // Keep the editor in sync if the tracked-properties value is refreshed from the parent. | ||
| useEffect(() => { | ||
| setValues(createValues(value)); | ||
| }, [value]); | ||
|
|
There was a problem hiding this comment.
Agreed. I’ve changed SimpleDictionary to resync local state only when the incoming prop value actually differs, and onChange now only fires when the local dictionary differs from the current prop value. That prevents redundant dispatches and avoids the prop/state feedback loop.
|
The PR looks blocked only by the AI validation approval gate. The workflow in .github/workflows/pr-ai-validation.yml stops external contributors until a maintainer adds the external-approved label. Once that label is added, the validation job should proceed. |
| import { useEffect, useRef, useState } from 'react'; | ||
| import { useIntl } from 'react-intl'; | ||
| import { useStyles } from './simpledictionary.styles'; | ||
| import { equals } from '@microsoft/logic-apps-shared'; |
There was a problem hiding this comment.
Good catch. I’ve updated getTrackedProperties to only clone plain objects, so primitive trackedProperties values now pass through unchanged and won’t be converted into an object shape.
| useEffect(() => { | ||
| const nextValues = createValues(value); | ||
| if (!equals(nextValues, valuesRef.current)) { | ||
| setValues(nextValues); | ||
| } |
There was a problem hiding this comment.
Good catch. I replaced the comparison with deepCompareObjects and compare the normalized dictionary value instead of the row array. That keeps the prop-sync behavior without runtime errors or redundant onChange updates.
| // Keep the editor in sync if the tracked-properties value is refreshed from the parent. | ||
| useEffect(() => { | ||
| const nextValues = createValues(value); | ||
| if (!deepCompareObjects(valuesToDictionary(nextValues), value)) { | ||
| setValues(nextValues); | ||
| } | ||
| }, [value]); |
There was a problem hiding this comment.
Good catch. I’ve hoisted the helper functions out of the component, so the effect no longer depends on unstable local function identities and react-hooks/exhaustive-deps should stay clean.
| useEffect(() => { | ||
| onChange?.( | ||
| values | ||
| .filter((x) => x.key && x.key !== '') | ||
| .reduce((acc: any, val) => { | ||
| acc[val.key] = val.value; | ||
| return acc; | ||
| }, {}) | ||
| ); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [values]); | ||
| const nextDictionary = valuesToDictionary(values); | ||
| if (!deepCompareObjects(nextDictionary, value)) { | ||
| onChange?.(nextDictionary); | ||
| } |
There was a problem hiding this comment.
Good catch. I’ve also hoisted valuesToDictionary out of the component, so this effect now uses stable helpers and won’t trigger the dependency warning.
| // Keep the editor in sync if the tracked-properties value is refreshed from the parent. | ||
| useEffect(() => { | ||
| const nextValues = createValues(value); | ||
| if (!deepCompareObjects(valuesToDictionary(nextValues), value)) { | ||
| setValues(nextValues); |
There was a problem hiding this comment.
Good catch. I updated the effect flow so prop-driven updates only resync local state, while onChange is skipped during the initial render and parent syncs. That prevents the stale emit/feedback loop and keeps the editor aligned with incoming values.
| useEffect(() => { | ||
| onChange?.( | ||
| values | ||
| .filter((x) => x.key && x.key !== '') | ||
| .reduce((acc: any, val) => { | ||
| acc[val.key] = val.value; | ||
| return acc; | ||
| }, {}) | ||
| ); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| const nextValues = createValues(value); | ||
| if (!deepCompareObjects(valuesToDictionary(valuesRef.current), value)) { | ||
| isSyncingFromParentRef.current = true; | ||
| setValues(nextValues); |
There was a problem hiding this comment.
Good catch. I normalized empty dictionaries to undefined before comparing, so {} and an empty editor state are treated the same and we avoid unnecessary resyncs.
| const trackedProperties = supported && definition ? getPropertyValue(definition as any, 'trackedProperties') : undefined; | ||
| return isObject(trackedProperties) ? clone(trackedProperties) : trackedProperties; |
There was a problem hiding this comment.
Good catch. I hardened the tracked-properties copy path to skip prototype-pollution keys like proto, constructor, and prototype before cloning recursively.
| const nextDictionary = dictionaryRows.reduce((acc, row) => { | ||
| if (row.key) { | ||
| acc[row.key] = row.value; | ||
| } | ||
| return acc; | ||
| }, {} as Record<string, string>); |
There was a problem hiding this comment.
Good catch. I’ll harden valuesToDictionary the same way as the clone path by skipping proto, constructor, and prototype when building the emitted dictionary, so user-entered keys can’t pollute the accumulator object.
| const nextValues = createValues(value); | ||
| const currentDictionary = normalizeDictionary(valuesToDictionary(valuesRef.current)); | ||
| const nextDictionary = normalizeDictionary(value); |
There was a problem hiding this comment.
Good catch. I’ve normalized the incoming value through the same dictionary conversion path before comparing, so both sides use the same filtering rules and we avoid unnecessary resyncs on ignored keys or blank entries.
| const cloneTrackedProperties = (trackedProperties: Record<string, any>): Record<string, any> => { | ||
| const safeClone: Record<string, any> = {}; | ||
|
|
||
| for (const key of Object.keys(trackedProperties)) { | ||
| if (key === '__proto__' || key === 'constructor' || key === 'prototype') { | ||
| continue; | ||
| } | ||
|
|
||
| const value = trackedProperties[key]; | ||
| if (Array.isArray(value)) { | ||
| safeClone[key] = value.map((item) => (isObject(item) ? cloneTrackedProperties(item) : item)); | ||
| } else if (isObject(value)) { | ||
| safeClone[key] = cloneTrackedProperties(value); | ||
| } else { | ||
| safeClone[key] = value; | ||
| } | ||
| } | ||
|
|
||
| return safeClone; | ||
| }; |
There was a problem hiding this comment.
Good catch. I’ll make the array clone recursive so nested arrays are copied by value too, not just plain-object items. That keeps trackedProperties fully detached from the source definition and avoids shared mutable sub-structures.
|
Copilot review is now clean and there are no new comments. Could you please add the external-approved label or otherwise approve the PR so the AI validation workflow can proceed? |
fix: resync tracked properties editor state
Commit Type
Risk Level
What & Why
trackedPropertiesis now cloned when loaded into settings to reduce the chance of shared-reference mutation.valueprop changes, so the UI stays aligned with the latest workflow state.Impact of Change
Test Plan
vitestis not available in this environment through the package manager setupContributors
@jibin7joseScreenshots/Videos