Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| getSelectedLabelFromValue(items, field.value || '') | ||
|
|
||
| // Memoize description computation to avoid recalculating on every render | ||
| const memoizedDescription = useMemo(() => { |
There was a problem hiding this comment.
I'd be surprised if this memo has a meaningful effect on performance
There was a problem hiding this comment.
It could even be more computation than not memoizing — the useMemo call and its dep checks are work too. The point of this would be to prevent the value changing to avoid triggering renders on <Combobox> below, but this is just a string and it wouldn't be changing its value between renders, so it is almost certainly triggering re-renders.
There was a problem hiding this comment.
Why would this trigger a re-render – since description, placeholder and allowArbitraryValues are likely the same values across the lifetime of this component. So the prop on <Combobox> wouldn't change.
There was a problem hiding this comment.
Yeah, that's what I meant about it being a string. If it was constructing an object or array with a spread like the one in disk create, then it would be a new one every time (even if the underlying values are the same) and then it would trigger renders when passed down the chain. So the other ones are more plausible, though in those cases it's still worth determining whether they have much effect. Triggering renders is not always a problem.
There was a problem hiding this comment.
Yeah, the memoization explosion was from before I added in the virtualization; was going to try backing them out to see whether they were having a noticeable effect, but will read the other comments on this first
| }, []) | ||
|
|
||
| // Memoize combined images array | ||
| const images = useMemo(() => [...realImages, ...mockImages], [realImages, mockImages]) |
There was a problem hiding this comment.
Again suspect an improvement is negligible here, suspect spreading two arrays is relatively inexpensive
|
I think we might get better results by migrating to something
|
|
Yeah, having to manually memoize so much in the call sites suggests to me something bigger is wrong. I agree with Ben that it's worth considering changing the library out altogether if we're having so much trouble with Headless. At the very least I'd like to have a clearer sense of which optimizations are responsible for the majority of the improvement here. I would guess it's a pretty small subset of them. Scroll and filter performance is definitely better, though the debounce feels kind of bad because it means filtering still feels a little slow. Maybe a shorter window on it would be better. Saw some fun bugs in Firefox. The double scrollbar is obvious. A much weirder one is that at 10s when it jumps around and starts scrolling up, I am not doing the scrolling myself. All I did was filter for 3, scroll down a while, then hit backspace to clear the 2025-07-24-combobox-double-scroll.mp4 |
4 participants
A little more cleanup to go, but this speeds up Comboboxes on Chrome significantly.