From 57fce934731b378202814a38458a8dbae1d5fc61 Mon Sep 17 00:00:00 2001
From: Noah Sgorbati <noahsgorbati@gmail.com>
Date: Tue, 5 Nov 2024 14:16:14 +0000
Subject: [PATCH] fix(combobox): controlled combobox onchange (#17858)

* fix(combobox): controlled combobox onchange

* updated useEffect hook for controlled combobox so onChange doesn't fire twice
when selection is made and selectedItem prop changes
* updated onSelectedItemChange to not fire onChange if selectedItem is unchanged
* unit tests

* fix(combobox): controlled combobox onchange

* updated comparison of new selectedItem to use react-fast-compare isEqual

---------

Co-authored-by: Taylor Jones <tay1orjones@users.noreply.github.com>
---
 .../src/components/ComboBox/ComboBox-test.js  | 55 +++++++++++++++++--
 .../src/components/ComboBox/ComboBox.tsx      | 20 +++++--
 2 files changed, 65 insertions(+), 10 deletions(-)

diff --git a/packages/react/src/components/ComboBox/ComboBox-test.js b/packages/react/src/components/ComboBox/ComboBox-test.js
index c0de4bd07629..a1916be39a4c 100644
--- a/packages/react/src/components/ComboBox/ComboBox-test.js
+++ b/packages/react/src/components/ComboBox/ComboBox-test.js
@@ -5,7 +5,7 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-import React from 'react';
+import React, { useState } from 'react';
 import { render, screen, within, fireEvent, act } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 import {
@@ -27,6 +27,32 @@ const openMenu = async () => {
 
 const prefix = 'cds';
 
+const ControlledComboBox = ({ controlledItem }) => {
+  const items = generateItems(5, generateGenericItem);
+  const [value, setValue] = useState(controlledItem || items[0]);
+  const [onChangeCallCount, setOnChangeCallCount] = useState(0);
+  const controlledOnChange = ({ selectedItem }) => {
+    setValue(selectedItem);
+    setOnChangeCallCount(onChangeCallCount + 1);
+  };
+
+  return (
+    <div>
+      <ComboBox
+        id="test-combobox"
+        items={items}
+        selectedItem={value}
+        onChange={controlledOnChange}
+        placeholder="Filter..."
+        type="default"
+      />
+      <div>value: {value.label}</div>
+      <div>onChangeCallCount: {onChangeCallCount}</div>
+      <button onClick={() => setValue(items[2])}>Choose item 3</button>
+    </div>
+  );
+};
+
 describe('ComboBox', () => {
   let mockProps;
   window.HTMLElement.prototype.scrollIntoView = function () {};
@@ -309,7 +335,7 @@ describe('ComboBox', () => {
       await waitForPosition();
       expect(findInputNode()).toHaveDisplayValue(mockProps.items[1]);
     });
-    it('should update and call `onChange` when selection is updated from the combobox', async () => {
+    it('should update and call `onChange` once when selection is updated from the combobox', async () => {
       render(<ComboBox {...mockProps} selectedItem={mockProps.items[0]} />);
       expect(mockProps.onChange).not.toHaveBeenCalled();
       await openMenu();
@@ -319,7 +345,28 @@ describe('ComboBox', () => {
         screen.getByRole('combobox', { value: 'Item 2' })
       ).toBeInTheDocument();
     });
-    it('should update and call `onChange` when selection is updated externally', async () => {
+    it('should not call `onChange` when current selection is selected again', async () => {
+      render(<ComboBox {...mockProps} selectedItem={mockProps.items[0]} />);
+      expect(mockProps.onChange).not.toHaveBeenCalled();
+      await openMenu();
+      await userEvent.click(screen.getByRole('option', { name: 'Item 0' }));
+      expect(mockProps.onChange).toHaveBeenCalledTimes(0);
+      expect(
+        screen.getByRole('combobox', { value: 'Item 0' })
+      ).toBeInTheDocument();
+    });
+    it('should update and call `onChange` once when selection is updated from the combobox and the external state managing selectedItem is updated', async () => {
+      render(<ControlledComboBox />);
+      expect(screen.getByText('onChangeCallCount: 0')).toBeInTheDocument();
+      await openMenu();
+      await userEvent.click(screen.getByRole('option', { name: 'Item 2' }));
+      expect(screen.getByText('onChangeCallCount: 1')).toBeInTheDocument();
+      expect(screen.getByText('value: Item 2')).toBeInTheDocument();
+      expect(
+        screen.getByRole('combobox', { value: 'Item 2' })
+      ).toBeInTheDocument();
+    });
+    it('should update and call `onChange` once when selection is updated externally', async () => {
       const { rerender } = render(
         <ComboBox {...mockProps} selectedItem={mockProps.items[0]} />
       );
@@ -334,7 +381,7 @@ describe('ComboBox', () => {
       await userEvent.click(
         screen.getByRole('button', { name: 'Clear selected item' })
       );
-      expect(mockProps.onChange).toHaveBeenCalled();
+      expect(mockProps.onChange).toHaveBeenCalledTimes(1);
       expect(findInputNode()).toHaveDisplayValue('');
     });
     it('should clear selected item when `selectedItem` is updated to `null` externally', async () => {
diff --git a/packages/react/src/components/ComboBox/ComboBox.tsx b/packages/react/src/components/ComboBox/ComboBox.tsx
index d56c4b7b0c04..75f34be51f02 100644
--- a/packages/react/src/components/ComboBox/ComboBox.tsx
+++ b/packages/react/src/components/ComboBox/ComboBox.tsx
@@ -34,6 +34,7 @@ import {
   WarningAltFilled,
   WarningFilled,
 } from '@carbon/icons-react';
+import isEqual from 'react-fast-compare';
 import ListBox, {
   PropTypes as ListBoxPropTypes,
   ListBoxSize,
@@ -509,11 +510,15 @@ const ComboBox = forwardRef(
           selectedItem: selectedItemProp,
           prevSelectedItem: prevSelectedItemProp.current,
         });
-        setInputValue(currentInputValue);
-        onChange({
-          selectedItem: selectedItemProp,
-          inputValue: currentInputValue,
-        });
+
+        // selectedItem has been updated externally, need to update state and call onChange
+        if (inputValue !== currentInputValue) {
+          setInputValue(currentInputValue);
+          onChange({
+            selectedItem: selectedItemProp,
+            inputValue: currentInputValue,
+          });
+        }
         prevSelectedItemProp.current = selectedItemProp;
       }
     }, [selectedItemProp]);
@@ -746,7 +751,10 @@ const ComboBox = forwardRef(
         setHighlightedIndex(indexToHighlight(normalizedInput));
       },
       onSelectedItemChange({ selectedItem }) {
-        onChange({ selectedItem });
+        // only call onChange if new selection is updated from previous
+        if (!isEqual(selectedItem, selectedItemProp)) {
+          onChange({ selectedItem });
+        }
       },
       onHighlightedIndexChange: ({ highlightedIndex }) => {
         if (highlightedIndex! > -1 && typeof window !== undefined) {