Skip to content

Commit f7b12b6

Browse files
feat(autocomplete): add phase 2 tradeoffs and outcome documentation, implement bug test for onHighlightChange, and enhance useAutocomplete logic
1 parent 5b9b8d2 commit f7b12b6

File tree

3 files changed

+129
-22
lines changed

3 files changed

+129
-22
lines changed

README.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,3 +138,9 @@ Instructions:
138138

139139
We're excited and looking forward to seeing what you'll create!
140140
Good luck 🚀
141+
142+
## Phase 2 tradeoffs and outcome
143+
144+
- Added an effect in `useAutocomplete` that resynchronizes the highlighted option whenever the filtered options array changes. This ensures late-arriving async options promote the correct `onHighlightChange` payload, at the cost of an extra highlight event when new data arrives.
145+
- Considered recalculating the highlight during every reset or `syncHighlightedIndex` tick, but those approaches either missed async updates or ran more often than needed. The chosen solution isolates the work to actual option changes while reusing the existing keyboard/navigation logic.
146+
- Introduced a regression test (`packages/material-ui/src/Autocomplete/Autocomplete.test.js`) that simulates the reported scenario—typing “1”, then “12”, then receiving updated options, to prevent the bug from resurfacing.

packages/material-ui/src/Autocomplete/Autocomplete.test.js

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,55 @@ describe('<Autocomplete />', () => {
167167
checkHighlightIs(getByRole('listbox'), 'two');
168168
});
169169

170+
it('should update the highlighted option when the options change asynchronously', () => {
171+
const handleHighlightChange = spy();
172+
const optionsForOne = Array.from({ length: 5 }, (_, index) => `option 1 ${index + 1}`);
173+
174+
const { getByRole, setProps } = render(
175+
<Autocomplete
176+
open
177+
autoHighlight
178+
filterOptions={(x) => x}
179+
options={[]}
180+
onHighlightChange={handleHighlightChange}
181+
renderInput={(params) => <TextField {...params} autoFocus />}
182+
/>,
183+
);
184+
185+
const textbox = getByRole('textbox');
186+
187+
act(() => {
188+
fireEvent.change(textbox, { target: { value: '1' } });
189+
});
190+
191+
act(() => {
192+
setProps({ options: optionsForOne });
193+
});
194+
195+
expect(handleHighlightChange.callCount).to.be.greaterThan(0);
196+
expect(handleHighlightChange.lastCall.args[1]).to.equal('option 1 1');
197+
198+
act(() => {
199+
fireEvent.change(textbox, { target: { value: '12' } });
200+
});
201+
202+
// Simulate stale results still containing the old first option.
203+
act(() => {
204+
setProps({ options: optionsForOne });
205+
});
206+
207+
const callCountBeforeFinalUpdate = handleHighlightChange.callCount;
208+
expect(handleHighlightChange.lastCall.args[1]).to.equal('option 1 1');
209+
210+
act(() => {
211+
setProps({ options: ['option 2 12'] });
212+
});
213+
214+
expect(handleHighlightChange.callCount).to.be.greaterThan(callCountBeforeFinalUpdate);
215+
expect(handleHighlightChange.lastCall.args[1]).to.equal('option 2 12');
216+
checkHighlightIs(getByRole('listbox'), 'option 2 12');
217+
});
218+
170219
it('should keep the current highlight if possible', () => {
171220
const { getByRole } = render(
172221
<Autocomplete

packages/material-ui/src/useAutocomplete/useAutocomplete.js

Lines changed: 74 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,7 @@ export default function useAutocomplete(props) {
129129
const [focusedTag, setFocusedTag] = React.useState(-1);
130130
const defaultHighlighted = autoHighlight ? 0 : -1;
131131
const highlightedIndexRef = React.useRef(defaultHighlighted);
132+
const highlightedOptionRef = React.useRef(null);
132133

133134
const [value, setValueState] = useControlled({
134135
controlled: valueProp,
@@ -184,27 +185,44 @@ export default function useAutocomplete(props) {
184185

185186
const popupOpen = open;
186187

187-
const filteredOptions = popupOpen
188-
? filterOptions(
189-
options.filter((option) => {
190-
if (
191-
filterSelectedOptions &&
192-
(multiple ? value : [value]).some(
193-
(value2) => value2 !== null && getOptionSelected(option, value2),
194-
)
195-
) {
196-
return false;
197-
}
198-
return true;
199-
}),
200-
// we use the empty string to manipulate `filterOptions` to not filter any options
201-
// i.e. the filter predicate always returns true
202-
{
203-
inputValue: inputValueIsSelectedValue && inputPristine ? '' : inputValue,
204-
getOptionLabel,
205-
},
206-
)
207-
: [];
188+
const filteredOptions = React.useMemo(() => {
189+
if (!popupOpen) {
190+
return [];
191+
}
192+
193+
const filtered = filterOptions(
194+
options.filter((option) => {
195+
if (
196+
filterSelectedOptions &&
197+
(multiple ? value : [value]).some(
198+
(value2) => value2 !== null && getOptionSelected(option, value2),
199+
)
200+
) {
201+
return false;
202+
}
203+
return true;
204+
}),
205+
// we use the empty string to manipulate `filterOptions` to not filter any options
206+
// i.e. the filter predicate always returns true
207+
{
208+
inputValue: inputValueIsSelectedValue && inputPristine ? '' : inputValue,
209+
getOptionLabel,
210+
},
211+
);
212+
return filtered;
213+
}, [
214+
popupOpen,
215+
filterOptions,
216+
options,
217+
filterSelectedOptions,
218+
multiple,
219+
value,
220+
getOptionSelected,
221+
inputValueIsSelectedValue,
222+
inputPristine,
223+
inputValue,
224+
getOptionLabel,
225+
]);
208226

209227
const focusTag = useEventCallback((tagToFocus) => {
210228
if (tagToFocus === -1) {
@@ -317,6 +335,8 @@ export default function useAutocomplete(props) {
317335

318336
const setHighlightedIndex = useEventCallback(({ event, index, reason = 'auto' }) => {
319337
highlightedIndexRef.current = index;
338+
highlightedOptionRef.current =
339+
index === -1 || !filteredOptions[index] ? null : filteredOptions[index];
320340

321341
// does the index exist?
322342
if (index === -1) {
@@ -433,7 +453,8 @@ export default function useAutocomplete(props) {
433453
return newIndex;
434454
};
435455

436-
const nextIndex = validOptionIndex(getNextIndex(), direction);
456+
const proposedIndex = getNextIndex();
457+
const nextIndex = validOptionIndex(proposedIndex, direction);
437458
setHighlightedIndex({ index: nextIndex, reason, event });
438459

439460
// Sync the content of the input with the highlighted option.
@@ -455,6 +476,37 @@ export default function useAutocomplete(props) {
455476
},
456477
);
457478

479+
React.useEffect(() => {
480+
const highlightedIndex = highlightedIndexRef.current;
481+
const newHighlightedOption =
482+
highlightedIndex === -1 ? null : filteredOptions[highlightedIndex] ?? null;
483+
const previousHighlightedOption = highlightedOptionRef.current;
484+
485+
highlightedOptionRef.current = newHighlightedOption;
486+
487+
if (!popupOpen || highlightedIndex === -1) {
488+
return;
489+
}
490+
491+
if (newHighlightedOption == null) {
492+
changeHighlightedIndex({ diff: 'reset', reason: 'option-change' });
493+
return;
494+
}
495+
496+
if (
497+
previousHighlightedOption == null ||
498+
!getOptionSelected(newHighlightedOption, previousHighlightedOption)
499+
) {
500+
setHighlightedIndex({ index: highlightedIndex, reason: 'option-change' });
501+
}
502+
}, [
503+
filteredOptions,
504+
popupOpen,
505+
changeHighlightedIndex,
506+
setHighlightedIndex,
507+
getOptionSelected,
508+
]);
509+
458510
const isTouch = React.useRef(false);
459511

460512
const syncHighlightedIndex = React.useCallback(() => {

0 commit comments

Comments
 (0)