feat:(listbox) virtualized gap #5645
Conversation
🦋 Changeset detectedLatest commit: 199ea12 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 |
|
@MattBrooks95 is attempting to deploy a commit to the HeroUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds an optional gap property to Listbox virtualization props, wires it into the virtualized listbox via useVirtualizer, updates docs accordingly, introduces a Nix flake for dev shells, and adds a changeset entry for a minor @heroui/listbox release. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App Code
participant LB as Listbox
participant VLB as VirtualizedListbox
participant UV as useVirtualizer
participant DOM as DOM
App->>LB: Render with props { virtualization: { maxListboxHeight, itemHeight, gap? } }
LB->>VLB: Pass virtualization props
VLB->>UV: init({ count, getScrollElement, estimateSize, gap })
UV-->>VLB: Virtual item measurements/spacers
VLB->>DOM: Render items with spacing per gap
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/components/listbox/src/virtualized-listbox.tsx (1)
84-92: Unrelated but important: validation logic allows one required prop to be missingThis condition only throws if both maxListboxHeight and itemHeight are missing. If exactly one is missing, it proceeds and later computes NaN sizes. Change to throw when either is missing.
- if ( - !virtualization || - (!isEmpty(virtualization) && !virtualization.maxListboxHeight && !virtualization.itemHeight) - ) { + if ( + !virtualization || + (!isEmpty(virtualization) && (!virtualization.maxListboxHeight || !virtualization.itemHeight)) + ) {
🧹 Nitpick comments (7)
flake.nix (2)
10-15: Prefer mkShell.packages and remove redundant pkgs qualifiersUse the modern
packagesattr and droppkgs.when already inwith pkgs.Apply:
- buildInputs = pkgs: with pkgs; [ - pkgs.nodejs_22 - pkgs.pnpm - pkgs.nodePackages.typescript-language-server - pkgs.nodePackages.vscode-langservers-extracted - ]; + devPackages = pkgs: with pkgs; [ + nodejs_22 + pnpm + nodePackages.typescript-language-server + nodePackages.vscode-langservers-extracted + ];
16-16: Remove unnecessary rec
recisn’t used to self-reference anything here.Apply:
- in rec { + in {.changeset/wet-ways-boil.md (1)
5-5: Nit: tighten the wording and add a periodConsider clarifying that this is wired through TanStack Virtual’s gap option and end the sentence with a period.
-Allow specifying the gap property for list items in a virtualized listbox component +Allow specifying the virtualization.gap (in pixels) for items in the virtualized Listbox component.packages/components/listbox/src/listbox.tsx (1)
12-16: Public API addition looks good; document units and valid rangeAdding gap?: number is fine. Please add a brief JSDoc that it’s pixels, non-negative, and defaults to 0 when omitted.
export interface VirtualizationProps { maxListboxHeight: number; itemHeight: number; - gap?: number; + /** + * Spacing between virtualized items, in pixels. + * @default 0 + */ + gap?: number; }packages/components/listbox/src/virtualized-listbox.tsx (2)
117-123: Avoid repeated array spread of state.collection in hot pathsrenderRow and count both spread the collection. Cache it once to reduce minor overhead during scrolling.
- const itemSizes = useMemo( - () => getItemSizesForCollection([...state.collection], itemHeight), - [state.collection, itemHeight], - ); + const collectionItems = useMemo(() => [...state.collection], [state.collection]); + const itemSizes = useMemo( + () => getItemSizesForCollection(collectionItems, itemHeight), + [collectionItems, itemHeight], + ); const rowVirtualizer = useVirtualizer({ - count: [...state.collection].length, + count: collectionItems.length, getScrollElement: () => parentRef.current, estimateSize: (i) => itemSizes[i], gap: virtualization.gap ?? 0, }); const renderRow = (virtualItem: VirtualItem) => { - const item = [...state.collection][virtualItem.index]; + const item = collectionItems[virtualItem.index];Also applies to: 154-169
200-210: Minor: height value should be a number for inline stylemaxListboxHeight is numeric; passing number (not string) avoids unnecessary stringification.
- height: `${virtualScrollHeight}px`, + height: virtualScrollHeight,apps/docs/content/docs/components/listbox.mdx (1)
354-359: Add a brief note about units and defaultClarify that virtualization.gap is pixels and defaults to 0 so consumers know what to expect.
description: "Configuration for virtualization, optimizing rendering for large datasets. Required if isVirtualized is set to true.", - default: "-", + default: "- (gap defaults to 0px)",
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.changeset/wet-ways-boil.md(1 hunks)apps/docs/content/docs/components/listbox.mdx(2 hunks)flake.nix(1 hunks)packages/components/listbox/src/listbox.tsx(1 hunks)packages/components/listbox/src/virtualized-listbox.tsx(1 hunks)
🔇 Additional comments (3)
flake.nix (2)
1-6: LGTM on the basic flake shape and nixpkgs pinThe input pin plus devShells output is fine as a starting point.
11-14: Confirm Node and pnpm versions align with package.json
pkgs.nodejs_22satisfies"engines.node": ">=22.x".- Verify
pkgs.pnpm.versionequals10.6.2(perpackageManager: [email protected]); override/pin inflake.nixif it differs.- Check for any nested
package.jsonwith conflictingengines.nodeorpackageManagersettings.packages/components/listbox/src/virtualized-listbox.tsx (1)
102-107: No changes needed — v3.11.3 supportsgapVerified@tanstack/react-virtualv3.11.3 in package.json includes thegapoption in the Virtualizer API.
| attribute: "virtualization", | ||
| type: "Record<\"maxListboxHeight\" & \"itemHeight\", number>", | ||
| type: "Record<\"maxListboxHeight\" & \"itemHeight\", number> & { gap?: number; }", | ||
| description: "Configuration for virtualization, optimizing rendering for large datasets. Required if isVirtualized is set to true.", | ||
| default: "-", | ||
| }, |
There was a problem hiding this comment.
Docs type is incorrect: use union (|), not intersection (&)
Intersection of two string literal types is never. Replace with union and keep gap optional.
- type: "Record<\"maxListboxHeight\" & \"itemHeight\", number> & { gap?: number; }",
+ type: "Record<\"maxListboxHeight\" | \"itemHeight\", number> & { gap?: number; }",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| attribute: "virtualization", | |
| type: "Record<\"maxListboxHeight\" & \"itemHeight\", number>", | |
| type: "Record<\"maxListboxHeight\" & \"itemHeight\", number> & { gap?: number; }", | |
| description: "Configuration for virtualization, optimizing rendering for large datasets. Required if isVirtualized is set to true.", | |
| default: "-", | |
| }, | |
| attribute: "virtualization", | |
| type: "Record<\"maxListboxHeight\" | \"itemHeight\", number> & { gap?: number; }", | |
| description: "Configuration for virtualization, optimizing rendering for large datasets. Required if isVirtualized is set to true.", | |
| default: "-", | |
| }, |
🤖 Prompt for AI Agents
In apps/docs/content/docs/components/listbox.mdx around lines 355-359, the type
for the attribute "virtualization" incorrectly uses an intersection of string
literal types ("maxListboxHeight" & "itemHeight") which is never; change that to
a union so the Record key is "maxListboxHeight" | "itemHeight" and keep the
appended object { gap?: number } unchanged (i.e., use Record<"maxListboxHeight"
| "itemHeight", number> & { gap?: number; }).
| linux = "x86_64-linux"; | ||
| aarch64 = "aarch64-darwin"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Broaden system coverage and fix misleading var names
Only x86_64-linux and aarch64-darwin are covered, and the names suggest CPU rather than full system. Add aarch64-linux and x86_64-darwin and rename for clarity.
Apply:
- linux = "x86_64-linux";
- aarch64 = "aarch64-darwin";
+ x86_64_linux = "x86_64-linux";
+ aarch64_linux = "aarch64-linux";
+ x86_64_darwin = "x86_64-darwin";
+ aarch64_darwin = "aarch64-darwin";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| linux = "x86_64-linux"; | |
| aarch64 = "aarch64-darwin"; | |
| x86_64_linux = "x86_64-linux"; | |
| aarch64_linux = "aarch64-linux"; | |
| x86_64_darwin = "x86_64-darwin"; | |
| aarch64_darwin = "aarch64-darwin"; |
🤖 Prompt for AI Agents
In flake.nix around lines 8 to 9, the mapping only lists "linux = x86_64-linux"
and "aarch64 = aarch64-darwin", which both omits common platforms (aarch64-linux
and x86_64-darwin) and uses misleading keys that look like CPU arches rather
than full system identifiers; update the mapping to include entries for
aarch64-linux and x86_64-darwin and rename the keys/variables to clearly reflect
full system triples (e.g., use platform or system as the key names or use
explicit triples as values) so the file covers x86_64-linux, aarch64-linux,
x86_64-darwin, and aarch64-darwin consistently and with clear naming.
| devShells = { | ||
| "${linux}".default = let pkgs = nixpkgs.legacyPackages."${linux}"; in | ||
| pkgs.mkShell { | ||
| buildInputs = buildInputs pkgs; | ||
| }; | ||
| "${aarch64}".default = let pkgs = nixpkgs.legacyPackages."${aarch64}"; in | ||
| pkgs.mkShell { | ||
| buildInputs = buildInputs pkgs; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define devShells for all common systems and switch to packages
Add shells for x86_64-darwin and aarch64-linux; use packages = devPackages pkgs.
Apply:
- devShells = {
- "${linux}".default = let pkgs = nixpkgs.legacyPackages."${linux}"; in
- pkgs.mkShell {
- buildInputs = buildInputs pkgs;
- };
- "${aarch64}".default = let pkgs = nixpkgs.legacyPackages."${aarch64}"; in
- pkgs.mkShell {
- buildInputs = buildInputs pkgs;
- };
- };
+ devShells = {
+ "${x86_64_linux}".default = let pkgs = nixpkgs.legacyPackages."${x86_64_linux}"; in
+ pkgs.mkShell {
+ packages = devPackages pkgs;
+ };
+ "${aarch64_linux}".default = let pkgs = nixpkgs.legacyPackages."${aarch64_linux}"; in
+ pkgs.mkShell {
+ packages = devPackages pkgs;
+ };
+ "${x86_64_darwin}".default = let pkgs = nixpkgs.legacyPackages."${x86_64_darwin}"; in
+ pkgs.mkShell {
+ packages = devPackages pkgs;
+ };
+ "${aarch64_darwin}".default = let pkgs = nixpkgs.legacyPackages."${aarch64_darwin}"; in
+ pkgs.mkShell {
+ packages = devPackages pkgs;
+ };
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| devShells = { | |
| "${linux}".default = let pkgs = nixpkgs.legacyPackages."${linux}"; in | |
| pkgs.mkShell { | |
| buildInputs = buildInputs pkgs; | |
| }; | |
| "${aarch64}".default = let pkgs = nixpkgs.legacyPackages."${aarch64}"; in | |
| pkgs.mkShell { | |
| buildInputs = buildInputs pkgs; | |
| }; | |
| }; | |
| devShells = { | |
| "${x86_64_linux}".default = let pkgs = nixpkgs.legacyPackages."${x86_64_linux}"; in | |
| pkgs.mkShell { | |
| packages = devPackages pkgs; | |
| }; | |
| "${aarch64_linux}".default = let pkgs = nixpkgs.legacyPackages."${aarch64_linux}"; in | |
| pkgs.mkShell { | |
| packages = devPackages pkgs; | |
| }; | |
| "${x86_64_darwin}".default = let pkgs = nixpkgs.legacyPackages."${x86_64_darwin}"; in | |
| pkgs.mkShell { | |
| packages = devPackages pkgs; | |
| }; | |
| "${aarch64_darwin}".default = let pkgs = nixpkgs.legacyPackages."${aarch64_darwin}"; in | |
| pkgs.mkShell { | |
| packages = devPackages pkgs; | |
| }; | |
| }; |
🤖 Prompt for AI Agents
In flake.nix around lines 17 to 26, the devShells block only defines shells for
two systems and uses mkShell with buildInputs; add entries for the common
systems (at least "x86_64-darwin" and "aarch64-linux" in addition to the
existing ones) and for each system bind pkgs =
nixpkgs.legacyPackages."${system}" then export the development set via packages
= devPackages pkgs (instead of using pkgs.mkShell / buildInputs), so every
supported system returns packages that point to the same devPackages definition.
MattBrooks95
changed the title
1 participant
Closes #
📝 Description
Allow the user to set the gap between the list items in a virtualized list box.
I could not find a way to set a gap between the items in the documentation, but I saw that it was supported by the underyling TanStack library. I added a 'gap' property to the listbox component's
virtualizedprop and passed then passed that through to theuseVirtualizerhook along with the othervirtualizedattributes.⛳️ Current behavior (updates)
Users cannot specify a numerical gap between the items in a virtualized listbox.
🚀 New behavior
Users can specify a numerical gap between the items in a virtualized listbox.
With a gap of
undefinedor 0, the listbox will appear as it did before.With a gap specified, there will be a gap between the list items.
💣 Is this a breaking change (Yes/No):
📝 Additional Information
I wanted to add a unit test for this, but I realized that it would be meaningless. JSDOM doesn't implement rendering, so
getBoundingClientRectwill always return all 0s. The unit test would have verified that there was a gap of N pixels in between the individual list items by usinggetBoundingClientRectto verify the Y coordinates of the list items.In order to get the test to work, I would need to mock
getBoundingClientRect, and the test would then only be testing the mock and seemed pointless.I visually confirmed that a gap does appear in Storybook. However, after setting the
gapproperty, something else must be done to get the list to render with the new value. I accomplished this by lowering the number of items in the list by 1.I added a nix flake so that I could get the correct version of pnpm to be able to contribute.
flake.nixandflake.lockcould be removed from the PR. I will remove them if it's decided that this PR could be merged.Summary by CodeRabbit