fix+feat(select, listbox): bug on dataset with "sections", add support for scrollshadow #4462
Conversation
🦋 Changeset detectedLatest commit: 848863f The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces substantial modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/components/listbox/src/virtualized-listbox.tsx (1)
78-78: Naming the row component asListBoxRow.
Grouping row-specific logic into this component is a neat approach for clarity. If the logic grows, consider extracting it into its own file for better maintainability.packages/components/select/stories/select.stories.tsx (1)
1394-1430:AVATAR_DECORATIONSobject creation.
This categorization suits your story context. For production-scale usage, consider moving it to a constants or data file if it expands further.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/components/listbox/src/virtualized-listbox.tsx(6 hunks)packages/components/select/src/use-select.ts(2 hunks)packages/components/select/stories/select.stories.tsx(1 hunks)
🔇 Additional comments (11)
packages/components/listbox/src/virtualized-listbox.tsx (8)
1-1: Imports look good.
Using useMemo and useRef is appropriate for optimizing computations and handling DOM references.
17-30: Ensure correct handling of collection items in getItemSizesForCollection.
The logic appears sound for mapping sections and regular items. Consider verifying that all child nodes within a section are consistently typed, especially when mixing sections and leaf items, to avoid runtime errors if item types are used incorrectly.
65-68: Memoized item sizes appear appropriate for performance.
The dependency array [state.collection, itemHeight] is minimal and should foster caching benefits for large collections.
71-71: Confirm using array spread for the collection size.
count: [...state.collection].length works fine. Just be aware of potential overhead for very large collections.
73-73: Check index boundary on estimateSize.
Ensure i is always a valid index in itemSizes; otherwise, out-of-bounds access will cause errors or undefined behavior.
87-89: Gracefully handling null-ish items.
Early return avoids runtime errors. This check looks good.
138-138: scrollbar-hide utility class usage.
It’s fine to hide scrollbars if desired for UX. Double-check if accessibility (e.g., for keyboard users) is unaffected.
152-167: virtualItems.map approach.
This straightforward implementation is clear. For very large lists, confirm that creating inline objects on each render does not significantly impact performance.
packages/components/select/src/use-select.ts (2)
164-166: Increased default itemHeight from 32 to 36.
Looks good for better accommodating sections or extra item padding. Confirm that all styling references align with this new default.
213-213: Utilizing default itemHeight = 36.
It matches the code comment above. Ensure consistent usage throughout the select component, especially when mixing section items with different heights.
packages/components/select/stories/select.stories.tsx (1)
1432-1472: NonVirtualizedVsVirtualizedWithSections story.
Overall, this is a great demonstration comparing both approaches. Verify that large datasets render smoothly in the virtualized mode, and confirm performance meets expectations.
vinroger
changed the title
vinroger
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/components/listbox/src/virtualized-listbox.tsx (3)
101-103: Virtualization approach is correct.Providing the
countas the collection length and referencingitemSizes[i]inestimateSizeis consistent with the Virtualizer pattern. Make sure you handle any scenario where item sizes might change dynamically.
108-110: Scroll shadow base props usage.Merging the base props for scroll shadow with additional logic is a good pattern. Consider wrapping this in its own helper if you foresee expansions in the future.
176-180: Data attributes for scroll state.Exposing
data-*attributes for top/middle/bottom scroll states is beneficial for styling. Ensure each data attribute name is clearly indicative of its boolean nature.packages/components/select/stories/select.stories.tsx (1)
1394-1449: Optimize the valorant theme data structureThe valorant theme contains duplicated items with numeric suffixes (2,3,4). This increases the data size unnecessarily and makes maintenance harder.
Consider refactoring to use a generator function:
- valorant: [ - "a-hint-of-clove", - // ... many duplicated items with suffixes - ], + valorant: [ + "a-hint-of-clove", + "blade-storm", + "cypher", + "frag-out", + "omen-cowl", + "reyna-leer", + "vct-supernova", + "viper", + "yoru", + "carnalito" + ].flatMap(item => [ + item, + ...Array.from({length: 3}, (_, i) => `${item}${i + 2}`) + ]),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/quiet-geese-lay.md(1 hunks)packages/components/autocomplete/src/use-autocomplete.ts(1 hunks)packages/components/listbox/src/virtualized-listbox.tsx(8 hunks)packages/components/select/src/use-select.ts(3 hunks)packages/components/select/stories/select.stories.tsx(1 hunks)
🔇 Additional comments (14)
packages/components/autocomplete/src/use-autocomplete.ts (1)
464-464: New property aligns with the scroll shadow feature.
The addition of scrollShadowProps in the return type of getListBoxProps is cohesive with the scroll shadow functionality. Ensure it's consistently integrated and thoroughly tested to confirm seamless usage across related features.
.changeset/quiet-geese-lay.md (1)
1-7: Changelog clarity is sufficient.
The changeset provides a succinct account of the patches introduced for supporting sectioned datasets and scroll shadows. Everything seems to be in line with the overall objective of enhancing these components’ functionalities.
packages/components/listbox/src/virtualized-listbox.tsx (9)
1-7: Import organization is clear.
The additional imports for useMemo, useState, and references to @nextui-org/scroll-shadow directly address the new scroll shadow and virtualization needs. No issues stand out.
17-18: Allow flexible scroll shadow customization.
Introducing scrollShadowProps in the Props interface nicely decouples scroll behavior from the core logic. Good job keeping it optional for broader generalization.
21-34: Efficient item size calculation for sections.
The getItemSizesForCollection function elegantly handles sections by adding space for headers. This design should effectively reserve heights for sectioned data. Just ensure the +1 for the section header logic matches real-world usage for multiple levels of nested sections, if applicable.
35-60: Scroll state management is robust.
getScrollState clearly identifies top, bottom, and middle states. Using it to set state-based attributes for scroll styling is an excellent approach. Verify it behaves correctly in edge cases (e.g., extremely short lists).
94-98: Memoizing item sizes improves performance.
Using useMemo to cache item sizes can significantly reduce overhead for large or dynamic datasets. Monitor changes in the collection to ensure repeated calculations are minimized.
Line range hint 111-123: Conditional rendering for section items.
The approach to return null if the item is undefined and handle both sections and regular items is well-structured. This ensures a stable rendering path with no extraneous code.
162-167: Scroll state is accurately tracked.
Storing the scroll state (isTop, isBottom, isMiddle) in a useState hook allows for dynamic UI changes. Great approach for real-time updates to styling or shadow effects.
185-187: Handling onScroll event properly.
Calling getScrollState inside the event allows immediate updates. No issues flagged. Good job ensuring minimal re-renders.
197-212: Rendering virtual items in a predictable manner.
Using absolute positioning with inline transforms aligns with standard virtualization patterns. Make sure larger lists with many sections behave well in this arrangement.
packages/components/select/src/use-select.ts (3)
164-166: Improvement to item height documentation.
Changing the default itemHeight to 36 is a better representation of typical list item height, including padding and margins. The new comment clarifies usage for sectioned data. Good improvement to developer clarity.
213-213: Advanced configuration for virtualization.
Exposing isVirtualized, maxListboxHeight, and hideEmptyContent in props simplifies usage for various dataset sizes. This is beneficial for consistent handling of large or empty data.
569-569: Ensuring scroll shadow integration.
Adding scrollShadowProps to the getListboxProps merges seamlessly with the rest of the virtualization logic. Verify that consumers can reliably override defaults when needed.
| export const NonVirtualizedVsVirtualizedWithSections = { | ||
| render: () => { | ||
| const SelectComponent = ({isVirtualized}: {isVirtualized: boolean}) => ( | ||
| <Select | ||
| disallowEmptySelection | ||
| className="max-w-xs" | ||
| color="secondary" | ||
| defaultSelectedKeys={["jinx"]} | ||
| isVirtualized={isVirtualized} | ||
| label={`Avatar Decoration ${isVirtualized ? "(Virtualized)" : "(Non-virtualized)"}`} | ||
| selectedKeys={["jinx"]} | ||
| selectionMode="single" | ||
| variant="bordered" | ||
| > | ||
| {Object.keys(AVATAR_DECORATIONS).map((key) => ( | ||
| <SelectSection | ||
| key={key} | ||
| classNames={{ | ||
| heading: "uppercase text-secondary", | ||
| }} | ||
| title={key} | ||
| > | ||
| {AVATAR_DECORATIONS[key].map((item) => ( | ||
| <SelectItem key={item} className="capitalize" color="secondary" variant="bordered"> | ||
| {item.replace(/-/g, " ")} | ||
| </SelectItem> | ||
| ))} | ||
| </SelectSection> | ||
| ))} | ||
| </Select> | ||
| ); | ||
|
|
||
| return ( | ||
| <div className="flex gap-4 w-full"> | ||
| <SelectComponent isVirtualized={false} /> | ||
| <SelectComponent isVirtualized={true} /> | ||
| </div> | ||
| ); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance component implementation for better accessibility and performance
The implementation could benefit from several improvements:
- The hardcoded
selectedKeys={["jinx"]}duplicates thedefaultSelectedKeysprop - Missing aria-label for better screen reader support
- No error handling for invalid selections
Consider these improvements:
const SelectComponent = ({isVirtualized}: {isVirtualized: boolean}) => (
<Select
disallowEmptySelection
className="max-w-xs"
color="secondary"
defaultSelectedKeys={["jinx"]}
isVirtualized={isVirtualized}
label={`Avatar Decoration ${isVirtualized ? "(Virtualized)" : "(Non-virtualized)"}`}
- selectedKeys={["jinx"]}
+ aria-label="Select an avatar decoration"
+ onSelectionChange={(key) => {
+ if (!AVATAR_DECORATIONS[Object.keys(AVATAR_DECORATIONS)[0]].includes(key as string)) {
+ console.warn('Invalid selection:', key);
+ }
+ }}
selectionMode="single"
variant="bordered"
>Additionally, consider adding a performance optimization:
+const memoizedSections = React.useMemo(
+ () => Object.keys(AVATAR_DECORATIONS).map((key) => (
+ <SelectSection
+ key={key}
+ classNames={{
+ heading: "uppercase text-secondary",
+ }}
+ title={key}
+ >
+ {AVATAR_DECORATIONS[key].map((item) => (
+ <SelectItem key={item} className="capitalize" color="secondary" variant="bordered">
+ {item.replace(/-/g, " ")}
+ </SelectItem>
+ ))}
+ </SelectSection>
+ )),
+ []
+);
const SelectComponent = ({isVirtualized}: {isVirtualized: boolean}) => (
<Select
// ... other props
>
- {Object.keys(AVATAR_DECORATIONS).map((key) => (
- <SelectSection
- // ... section implementation
- >
- {AVATAR_DECORATIONS[key].map((item) => (
- // ... item implementation
- ))}
- </SelectSection>
- ))}
+ {memoizedSections}
</Select>
);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/components/listbox/src/virtualized-listbox.tsx (4)
21-34: Consider memoizing results for large collectionsThe function correctly handles both regular items and sections, but for large collections, you might want to memoize the results at the call site to avoid unnecessary recalculations.
-const getItemSizesForCollection = (collection: Node<object>[], itemHeight: number) => { +const getItemSizesForCollection = (collection: Node<object>[], itemHeight: number): number[] => { const sizes: number[] = []; for (const item of collection) { if (item.type === "section") { sizes.push(([...item.childNodes].length + 1) * itemHeight); } else { sizes.push(itemHeight); } } return sizes; };
36-59: Enhance type safety for scroll state calculationThe scroll state calculation is robust, but we can improve type safety.
-const getScrollState = (element: HTMLDivElement | null) => { +interface ScrollState { + isTop: boolean; + isBottom: boolean; + isMiddle: boolean; +} + +const getScrollState = (element: HTMLDivElement | null): ScrollState => {
Line range hint
111-161: Optimize rendering performanceConsider memoizing the renderRow function to prevent unnecessary re-renders.
-const renderRow = (virtualItem: VirtualItem) => { +const renderRow = useMemo(() => (virtualItem: VirtualItem) => { // ... existing implementation ... -}; +}, [color, state, variant, disableAnimation, hideSelectedIcon, itemClasses]);
Line range hint
165-190: Optimize scroll event handlingConsider debouncing the scroll event handler to improve performance.
+import {debounce} from "@nextui-org/shared-utils"; + const [scrollState, setScrollState] = useState({ isTop: false, isBottom: true, isMiddle: false, }); +const handleScroll = useMemo( + () => + debounce((e: React.UIEvent<HTMLDivElement>) => { + setScrollState(getScrollState(e.target as HTMLDivElement)); + }, 100), + [] +); -onScroll={(e) => { - setScrollState(getScrollState(e.target as HTMLDivElement)); -}} +onScroll={handleScroll}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/listbox/src/virtualized-listbox.tsx(8 hunks)
🔇 Additional comments (2)
packages/components/listbox/src/virtualized-listbox.tsx (2)
1-1: LGTM: Imports and type definitions are appropriate
The added imports and ScrollShadowProps type definition align well with the new scroll shadow functionality.
Also applies to: 6-6, 17-18
94-103: Enhance validation for virtualization configuration
While the error handling for missing props is good, consider validating itemHeight to be positive.
const {maxListboxHeight, itemHeight} = virtualization;
+if (itemHeight <= 0) {
+ throw new Error("itemHeight must be a positive number");
+}
|
Thanks, @vinroger. Please check this scenario: when the select is virtualized and navigated using the keyboard, the highlighted item is visible behind the scroll shadow. This issue doesn’t occur on selects without virtualization CleanShot.2024-12-30.at.17.18.51.mp4 |
Closes #4457
Closes #4403
High-level summary of the two main issues we addressed in this Pull Request and how we fixed them. The issues revolve around how the Select (and by extension Autocomplete) component handles:
<SelectSection />).1. Nested Data Structures (Sections) Causing Errors
Problem
Previously, the
VirtualizedListboxdid not correctly handle data structures with nested sections. For example, a<Listbox>or<Select>that contained<SelectSection>items would fail to compute item heights, resulting in an error like:Why It Happened
<section>node appeared, the code attempted to read certain properties (likeprops) but foundundefinedfor the nested items.Fix
Section-Aware Height Calculation: In
virtualized-listbox.tsx, we introduced a function to calculate the item sizes in a section-aware manner:This way, a “section” node accounts for one line for the section header plus N lines for each child item.
Graceful Handling of Section Nodes
We added logic to differentiate between regular items and section nodes throughout the rendering process. This prevents undefined property accesses on “section” nodes.
Result
<Select>or<Autocomplete>in virtualized mode without errors.2. Scroll Shadow Compatibility
Problem
When using a virtualized listbox, the scroll shadows (the little indicators at the top/bottom to show that there is more scrollable content) were broken or inconsistent. The combination of a custom scroll container plus virtualized content meant that the existing scroll shadow logic wasn’t hooking into the right DOM elements or applying the correct classes.
Why It Happened
<div>inVirtualizedListboxto accurately track scroll positions and apply hide-scrollbar styling.position: relative), the standard approach for hooking scroll shadows in non-virtualized components did not directly apply.Fix
Injecting Scroll Shadow Hooks Directly
We used the
useScrollShadowhook withinvirtualized-listbox.tsx, so the scroll container (thedivwithref={parentRef}) becomes the element on which the scroll shadows are tracked:Tracking Scroll Position
We added a helper function
getScrollState()to detect whether the user is at the top, bottom, or somewhere in between. We store this inscrollState(viauseState), which is used to toggle classes or data attributes for customizing the scroll shadow behavior.Applying
scrollbar-hideThe
useScrollShadowhook can optionally apply thescrollbar-hideclasses or other custom classes for styling the scroll container. By merging thegetBasePropsScrollShadow()with the existingdivprops, the virtualized container properly gains all the needed classes, data attributes, and event handlers.Result
scrollbar-hide.Conclusion
<SelectSection>by accounting for the extra header in each section, avoiding errors and ensuring the correct total height for each “block” in the virtual list.useScrollShadowdirectly into the virtualized list container, we properly render and style the scroll shadows, even when items are absolutely positioned in a virtual list.💣 Is this a Breaking Change: No
The updates enhance existing functionality without breaking compatibility with current usage.
📝 Additional Information
Key Improvements
VirtualizedListboxto handle sectioned datasets seamlessly, allowing for nested structures in dropdowns.itemHeightfrom 32 to 36 for better alignment with typical use cases.Use Cases Addressed
SelectandAutocomplete.Summary by CodeRabbit
New Features
Improvements
disableClearableproperty in favor ofisClearablefor clearer usage.Documentation