fix(Popper): memoize content subtree updates#2639
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds memoization hooks: a new ChangesPopper re-render performance fix
Sequence Diagram(s)sequenceDiagram
participant TestHarness
participant Combobox
participant PopperContent
participant ListboxItem
TestHarness->>Combobox: mount & open()
Combobox->>PopperContent: render/popover attach
PopperContent->>PopperContent: compute placement (floating-ui)
alt placement updates
PopperContent->>PopperContent: update internal placement state
PopperContent->>ListboxItem: (v-memo prevents full re-render) skip re-render when keys unchanged
else filter input while open
Combobox->>PopperContent: memoDependencies change (filterSearch/filterState)
PopperContent->>ListboxItem: re-render affected items
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/Popper/PopperContent.vue`:
- Line 406: The template references an undefined identifier "as" in the v-memo
and :as binding; update both to use the component prop via props.as (consistent
with props.asChild usage) so the v-memo becomes v-memo="[props.asChild,
props.as, placedSide, placedAlign, isPositioned, $attrs]" and the element's :as
binding uses props.as; this fixes the undefined variable in PopperContentProps
(which extends PrimitiveProps) and keeps prop access consistent across the
template.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7dd9bb6d-3fa5-4ac1-8b91-714a9cff2393
📒 Files selected for processing (2)
packages/core/src/Combobox/Combobox.test.tspackages/core/src/Popper/PopperContent.vue
|
Nice fix direction! Two concerns though:
Also |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/core/src/Combobox/Combobox.test.ts (1)
583-587:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftResizeObserver mock currently prevents Floating UI’s resize-driven autoUpdate cycles from running in this test.
packages/core/src/Popper/PopperContent.vuewireswhileElementsMounted: autoUpdate(...)without disablingelementResize, and@floating-ui/vue’sautoUpdateusesResizeObserverto triggerupdateon reference/floating size changes. The no-op mock inpackages/core/src/Combobox/Combobox.test.ts(observe/unobserve/disconnect do nothing) means thoseResizeObserver-triggered position recalculations won’t happen, so the regression may not exercise the repeated positioning updates memoization is meant to guard against.Use a
ResizeObservermock that captures the callback and triggers it (or otherwise forcesautoUpdateto run multiple update cycles), so the test actually covers the memoization behavior under size-change-driven placement updates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/Combobox/Combobox.test.ts` around lines 583 - 587, The current ResizeObserver mock in Combobox.test.ts is a no-op and prevents `@floating-ui/vue`'s autoUpdate (wired via whileElementsMounted in PopperContent.vue) from running resize-driven update cycles; replace the mock with one that stores the callback passed to its constructor and invokes it (or schedules repeated invocations) when observe is called so autoUpdate's update loop runs multiple times and triggers the placement recalculations; ensure the mock implements observe/unobserve/disconnect and calls the stored callback with a entries array (or at least invokes it) to simulate size changes so the memoization behavior is actually exercised.packages/core/src/Combobox/ComboboxItem.vue (1)
83-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
v-memomay freeze custom option slot content that readsfilterSearchreactively.The deps
[isRender, rootContext.disabled.value, disabled, props.value, props.as, props.asChild, ...Object.values($attrs)]cover the props forwarded toListboxItemand the disabled contract correctly. Highlight/selection still update because they're driven byListboxItem's own reactive context (the parent'sv-memodoesn't block a child's own render effect).The gap is the slot: a consumer that renders search-aware content (e.g., bolding the matched substring of
rootContext.filterSearch) will go stale ifisRenderdoesn't change whenfilterSearchupdates. For example, ifisRenderis alreadytrueand remainstruewhen the search term changes,v-memowill skip the re-render. The default<slot>{{ value }}</slot>is fine, but custom slots that readrootContext.filterSearchreactively will not update.Add
rootContext.filterSearch.valueto thev-memodependency array.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/Combobox/ComboboxItem.vue` around lines 83 - 101, The v-memo dependency array in ComboboxItem.vue is missing rootContext.filterSearch.value which can cause custom slot content that reads rootContext.filterSearch to become stale; update the v-memo deps (the array used on the ListboxItem element) to include rootContext.filterSearch.value so slot render updates when the search term changes, keeping the existing deps (isRender, rootContext.disabled.value, disabled, props.value, props.as, props.asChild, ...Object.values($attrs)) intact.packages/core/src/Popper/PopperContent.vue (1)
409-432:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftUnconditional
v-memoonPopperContentwill serve stale slot content to every consumer except Combobox in popper mode.
v-memocaches the entire memoized subtree—including the<slot />vnodes—and skips re-invoking the slot function when the dependency array is unchanged. This directive is unconditionally applied to the sharedPrimitivewrapper for allPopperContentconsumers (Popover, Select, HoverCard, Menu, etc.), which means any consumer whose slot content reacts to state not captured by the deps (placedSide,placedAlign,isPositioned,$attrsvalues,memoDependencies) will render stale content.Currently, only
ComboboxContentwithposition="popper"suppliesmemoDependencies(filterSearch.valueandfilterState.value). All other consumers—PopoverContent,SelectContent,HoverCardContent,MenuContent—rely solely on placement and attribute changes, exposing them to stale slot rendering when their content depends on other reactive state.Concrete example: A
PopoverContentcontaining<p>{{ message }}</p>will not update whenmessagechanges, because none of the memo dependencies change.Recommend scoping the optimization to the Combobox consumer (e.g., wrap
v-memoin ComboboxContentImpl or a dedicated wrapper) so genericPopperContentslot dynamics remain live. This avoids silent regressions across the component library.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/Popper/PopperContent.vue` around lines 409 - 432, The unconditional v-memo on the Primitive inside PopperContent caches slot vnodes and causes stale slot renders for all consumers; restrict the memoization to only the Combobox popper case by removing or gating the v-memo in PopperContent and moving/applying it only in the Combobox-specific implementation (e.g., ComboboxContentImpl or ComboboxContent when position="popper"), or at minimum only enable v-memo when props.memoDependencies is present/non-empty; update the Primitive usage in PopperContent (and add the v-memo to ComboboxContentImpl) so other consumers render live slot content.
🧹 Nitpick comments (2)
packages/core/src/Combobox/Combobox.test.ts (1)
600-600: ⚡ Quick winTighten test thresholds to exact assertions.
The current thresholds (
toBeLessThanOrEqual(4)) are loose and may not catch subtle regressions in render count. Using exact expected values would make the test more precise and easier to reason about.Consider replacing the threshold checks with exact assertions (e.g.,
toBe(2)ortoBe(3)) once you've determined the actual expected render count with the memoization in place.Also applies to: 605-605
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/Combobox/Combobox.test.ts` at line 600, The test uses loose thresholds (toBeLessThanOrEqual) for render counts; replace those with exact assertions after confirming the real expected counts with current memoization. Locate the assertions referencing renderCountAfterOpen (and the analogous assertion at the second occurrence) in Combobox.test.ts and change toBeLessThanOrEqual(...) to toBe(expectedNumber) using the actual measured render count (e.g., toBe(2) or toBe(3)) so the test asserts an exact render count rather than a range.packages/core/src/Popper/PopperContent.vue (1)
411-419: 💤 Low value
...Object.values($attrs)can defeat memoization via identity-unstable attrs.Spreading attr values is the correct fix for the stable-proxy problem (where
$attrsitself never compares unequal). However, forwarded$attrsfrequently include event handlers and freshly-constructed objects/styles whose identity changes on every parent render. Each such value makesv-memoinvalidate every time (negating the perf win), while still comparing only by reference for nested objects. Worth confirming the forwarded attrs in the Combobox path are identity-stable, otherwise the bounded-render assertion may pass only because the test's forwarded attrs happen to be stable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/Popper/PopperContent.vue` around lines 411 - 419, The issue is that spreading Object.values($attrs) in the v-memo dependencies can cause unnecessary invalidations due to identity-unstable attributes like event handlers or new objects. To fix this in the PopperContent component, investigate and ensure the forwarded $attrs (especially in the Combobox usage path) are identity-stable before including them in v-memo. Alternatively, avoid spreading all $attrs values directly if they are not stable, or selectively include only stable attributes to maintain memoization effectiveness.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/Combobox/ComboboxContentImpl.vue`:
- Around line 131-133: The memoization for the popper branch only depends on
rootContext.filterSearch.value and rootContext.filterState.value, so other
reactive changes (e.g., rootContext.allItems growth, item disabled state
changes, rootContext.ignoreFilter toggles) won't invalidate the memoized
subtree; update the memo-dependencies expression in ComboboxContentImpl.vue (the
:memo-dependencies binding when position === 'popper') to include those
additional reactive refs (e.g., rootContext.allItems, rootContext.ignoreFilter
and any reactive map/refs tracking item disabled state) or alternatively disable
memoization for the popper branch when these other reactives are in use so slot
rendering stays correct.
---
Outside diff comments:
In `@packages/core/src/Combobox/Combobox.test.ts`:
- Around line 583-587: The current ResizeObserver mock in Combobox.test.ts is a
no-op and prevents `@floating-ui/vue`'s autoUpdate (wired via whileElementsMounted
in PopperContent.vue) from running resize-driven update cycles; replace the mock
with one that stores the callback passed to its constructor and invokes it (or
schedules repeated invocations) when observe is called so autoUpdate's update
loop runs multiple times and triggers the placement recalculations; ensure the
mock implements observe/unobserve/disconnect and calls the stored callback with
a entries array (or at least invokes it) to simulate size changes so the
memoization behavior is actually exercised.
In `@packages/core/src/Combobox/ComboboxItem.vue`:
- Around line 83-101: The v-memo dependency array in ComboboxItem.vue is missing
rootContext.filterSearch.value which can cause custom slot content that reads
rootContext.filterSearch to become stale; update the v-memo deps (the array used
on the ListboxItem element) to include rootContext.filterSearch.value so slot
render updates when the search term changes, keeping the existing deps
(isRender, rootContext.disabled.value, disabled, props.value, props.as,
props.asChild, ...Object.values($attrs)) intact.
In `@packages/core/src/Popper/PopperContent.vue`:
- Around line 409-432: The unconditional v-memo on the Primitive inside
PopperContent caches slot vnodes and causes stale slot renders for all
consumers; restrict the memoization to only the Combobox popper case by removing
or gating the v-memo in PopperContent and moving/applying it only in the
Combobox-specific implementation (e.g., ComboboxContentImpl or ComboboxContent
when position="popper"), or at minimum only enable v-memo when
props.memoDependencies is present/non-empty; update the Primitive usage in
PopperContent (and add the v-memo to ComboboxContentImpl) so other consumers
render live slot content.
---
Nitpick comments:
In `@packages/core/src/Combobox/Combobox.test.ts`:
- Line 600: The test uses loose thresholds (toBeLessThanOrEqual) for render
counts; replace those with exact assertions after confirming the real expected
counts with current memoization. Locate the assertions referencing
renderCountAfterOpen (and the analogous assertion at the second occurrence) in
Combobox.test.ts and change toBeLessThanOrEqual(...) to toBe(expectedNumber)
using the actual measured render count (e.g., toBe(2) or toBe(3)) so the test
asserts an exact render count rather than a range.
In `@packages/core/src/Popper/PopperContent.vue`:
- Around line 411-419: The issue is that spreading Object.values($attrs) in the
v-memo dependencies can cause unnecessary invalidations due to identity-unstable
attributes like event handlers or new objects. To fix this in the PopperContent
component, investigate and ensure the forwarded $attrs (especially in the
Combobox usage path) are identity-stable before including them in v-memo.
Alternatively, avoid spreading all $attrs values directly if they are not
stable, or selectively include only stable attributes to maintain memoization
effectiveness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: add44b9e-9fb8-4301-abaa-de2ce8a4a4d7
📒 Files selected for processing (4)
packages/core/src/Combobox/Combobox.test.tspackages/core/src/Combobox/ComboboxContentImpl.vuepackages/core/src/Combobox/ComboboxItem.vuepackages/core/src/Popper/PopperContent.vue
|
Addressed, thank you! I moved the memoization deps so filtering while the popper is open updates the rendered options, included dynamic attr values in the memo dependencies, and added coverage for filtering while open. I also updated the ResizeObserver mock so the test exercises the auto-update path more closely. |
2 participants
🔗 Linked issue
Resolves #2415
❓ Type of change
📚 Description
This PR reduces unnecessary rerenders for
PopperContentconsumers, specifically theComboboxContentposition="popper"case described in #2415.When Floating UI recalculates popper placement, the wrapper needs to update positioning/style metadata, but the inner content subtree does not need to rerender unless the props that affect it actually change. This memoizes the inner
Primitivesubtree so heavy slot content, such as large combobox option lists, is not repeatedly rerendered during popper position updates.A regression test was added for
ComboboxContent position="popper"with 60 options to ensure the option slot content does not rerender excessively after opening and running popper position updates.📸 Screenshots (if appropriate)
Not applicable.
📝 Checklist
Summary by CodeRabbit
Tests
Refactor