-
Notifications
You must be signed in to change notification settings - Fork 106
fix: resync tracked properties editor state #9387
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
05288a4
976a212
b9ea077
e7033e4
c23b88d
04df9d6
8430fde
b013a19
83d966d
b3c89aa
2fa32a4
ee2d6c0
820739c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| import { SimpleDictionary } from '../dictionary/simpledictionary'; | ||
| import { IntlProvider } from 'react-intl'; | ||
| import userEvent from '@testing-library/user-event'; | ||
| import { render, screen, waitFor } from '@testing-library/react'; | ||
| import type { ReactElement } from 'react'; | ||
| import { describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| const renderWithIntl = (ui: ReactElement) => render(<IntlProvider locale="en">{ui}</IntlProvider>); | ||
|
|
||
| describe('ui/settings/simpledictionary', () => { | ||
| it('keeps the editor in sync when the incoming dictionary value changes', async () => { | ||
| const onChange = vi.fn(); | ||
|
|
||
| const { rerender } = renderWithIntl(<SimpleDictionary value={{ ClientId: 'first-value' }} onChange={onChange} />); | ||
|
|
||
| await waitFor(() => expect(screen.getByDisplayValue('first-value')).toBeInTheDocument()); | ||
| expect(onChange).not.toHaveBeenCalled(); | ||
|
|
||
| rerender( | ||
| <IntlProvider locale="en"> | ||
| <SimpleDictionary value={{ ClientId: 'second-value' }} onChange={onChange} /> | ||
| </IntlProvider> | ||
| ); | ||
|
|
||
| await waitFor(() => expect(screen.getByDisplayValue('second-value')).toBeInTheDocument()); | ||
| expect(screen.queryByDisplayValue('first-value')).not.toBeInTheDocument(); | ||
| expect(onChange).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('ignores unsafe dictionary keys when emitting changes', async () => { | ||
| const onChange = vi.fn(); | ||
| const user = userEvent.setup(); | ||
|
|
||
| renderWithIntl(<SimpleDictionary onChange={onChange} />); | ||
|
|
||
| const [keyInput] = screen.getAllByRole('textbox'); | ||
| await user.type(keyInput, '__proto__'); | ||
|
|
||
| await waitFor(() => expect(onChange).toHaveBeenCalled()); | ||
| expect(onChange).toHaveBeenLastCalledWith(undefined); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,10 @@ import { useId } from '../../../useId'; | |
| import { SimpleDictionaryItem } from './simpledictionaryitem'; | ||
| import type { SimpleDictionaryRowModel, SimpleDictionaryChangeModel } from './simpledictionaryitem'; | ||
| import type React from 'react'; | ||
| import { useEffect, useState } from 'react'; | ||
| import { useEffect, useRef, useState } from 'react'; | ||
| import { useIntl } from 'react-intl'; | ||
| import { useStyles } from './simpledictionary.styles'; | ||
| import { deepCompareObjects } from '@microsoft/logic-apps-shared'; | ||
|
|
||
| export interface SimpleDictionaryProps { | ||
| disabled?: boolean; | ||
|
|
@@ -16,6 +17,31 @@ export interface SimpleDictionaryProps { | |
| onChange?: EventHandler<Record<string, string> | undefined>; | ||
| } | ||
|
|
||
| const createValues = (dictionaryValue?: Record<string, string>): SimpleDictionaryRowModel[] => [ | ||
| ...Object.entries(dictionaryValue ?? {}).map(([key, value], index) => ({ | ||
| key, | ||
| value, | ||
| index, | ||
| })), | ||
| { key: '', value: '', index: Object.keys(dictionaryValue ?? {}).length }, | ||
| ]; | ||
|
|
||
| const isSafeDictionaryKey = (key: string): boolean => key !== '__proto__' && key !== 'constructor' && key !== 'prototype'; | ||
|
|
||
| const valuesToDictionary = (dictionaryRows: SimpleDictionaryRowModel[]): Record<string, string> | undefined => { | ||
| const nextDictionary = dictionaryRows.reduce((acc, row) => { | ||
| if (row.key && isSafeDictionaryKey(row.key)) { | ||
| acc[row.key] = row.value; | ||
| } | ||
| return acc; | ||
| }, {} as Record<string, string>); | ||
|
|
||
| return Object.keys(nextDictionary).length > 0 ? nextDictionary : undefined; | ||
| }; | ||
|
|
||
| const normalizeDictionary = (dictionary?: Record<string, string>): Record<string, string> | undefined => | ||
| Object.keys(dictionary ?? {}).length > 0 ? dictionary : undefined; | ||
|
|
||
| export const SimpleDictionary: React.FC<SimpleDictionaryProps> = ({ | ||
| disabled, | ||
| customLabel, | ||
|
|
@@ -24,28 +50,42 @@ export const SimpleDictionary: React.FC<SimpleDictionaryProps> = ({ | |
| onChange, | ||
| ariaLabel, | ||
| }): JSX.Element => { | ||
| const [values, setValues] = useState([ | ||
| ...Object.entries(value ?? {}).map(([key, value], index) => ({ | ||
| key, | ||
| value, | ||
| index, | ||
| })), | ||
| { key: '', value: '', index: Object.keys(value ?? {}).length }, | ||
| ]); | ||
| const [values, setValues] = useState(createValues(value)); | ||
| const valuesRef = useRef(values); | ||
| const isInitialRenderRef = useRef(true); | ||
| const isSyncingFromParentRef = useRef(false); | ||
|
|
||
| const intl = useIntl(); | ||
|
|
||
| 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); | ||
| const currentDictionary = normalizeDictionary(valuesToDictionary(valuesRef.current)); | ||
| const nextDictionary = normalizeDictionary(valuesToDictionary(nextValues)); | ||
|
|
||
| if (!deepCompareObjects(currentDictionary, nextDictionary)) { | ||
| isSyncingFromParentRef.current = true; | ||
| setValues(nextValues); | ||
| } | ||
|
Comment on lines
+60
to
+68
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| }, [value]); | ||
|
|
||
| useEffect(() => { | ||
| valuesRef.current = values; | ||
| }, [values]); | ||
|
|
||
| useEffect(() => { | ||
| if (isInitialRenderRef.current) { | ||
| isInitialRenderRef.current = false; | ||
| return; | ||
| } | ||
|
Comment on lines
71
to
+79
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| if (isSyncingFromParentRef.current) { | ||
| isSyncingFromParentRef.current = false; | ||
| return; | ||
| } | ||
|
|
||
| onChange?.(valuesToDictionary(values)); | ||
| }, [onChange, values]); | ||
|
|
||
| const handleItemDelete = (e: SimpleDictionaryRowModel): void => { | ||
| setValues((oldValues) => oldValues.filter((x) => x.index !== e.index).map((x, i) => ({ ...x, index: i }))); | ||
| }; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.