fix(hooks): handle numeric selectedKeys in Select #2589
Conversation
🦋 Changeset detectedLatest commit: 2c412b3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
WalkthroughThe patch addresses an issue in the "@nextui-org/use-aria-multiselect" library, specifically enhancing the handling of numeric keys in the Select component. It ensures that keys, whether numeric or string, are treated uniformly by converting them to strings for consistent processing within the collection management functions. This change directly targets and resolves a bug where numeric Changes
Assessment against linked issues
Related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@wingkwong is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- .changeset/hot-rivers-move.md (1 hunks)
- packages/hooks/use-aria-multiselect/src/use-multiselect-list-state.ts (2 hunks)
Additional comments: 3
.changeset/hot-rivers-move.md (1)
- 1-5: The changeset description is clear and accurately communicates the patch's purpose. The term "selectedKeys" is correctly used in this context, indicating a false positive from the static analysis tool.
packages/hooks/use-aria-multiselect/src/use-multiselect-list-state.ts (2)
- 32-32: Coercing keys to strings before checking their presence in the collection is a straightforward solution to support numeric keys. Ensure this change does not inadvertently affect other parts of the codebase where keys are expected in their original form.
Verification successful
The search results indicate that
collection.getItemis used in multiple places across the codebase. Specifically, within theuse-aria-multiselectpackage, keys are coerced to strings before being used withcollection.getItem, as seen in two instances inuse-multiselect-list-state.ts. This aligns with the changes reviewed and suggests a consistent approach to handling keys within this context.However, in other parts of the codebase, such as
hidden-select.tsxandtree-keyboard-delegate.tswithin theapps/docs/utilsdirectory, keys are used withcollection.getItemwithout explicit coercion to strings. This variation in usage indicates that while the change inuse-multiselect-list-state.tsis appropriate for its specific context, there is no universal approach to key handling across the codebase.Given the localized nature of the change and its consistency within the
use-aria-multiselectpackage, the modification is verified as appropriate for its intended purpose. However, it's important to note the varied approaches to key handling in different parts of the codebase, which may warrant further review or standardization efforts depending on the broader application requirements.* 42-42: Similarly, coercing keys to strings when retrieving selected items aligns with the PR's objective. This change should ensure consistent handling of numeric and string keys across the component.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences where collection.getItem might be used with keys not coerced to strings. rg "collection.getItem\((?!.*\+" --type tsLength of output: 295
Script:
#!/bin/bash # Corrected search for other occurrences where collection.getItem might be used with keys not coerced to strings, using PCRE2 for regex support. rg "collection.getItem\((?!.*\+" --type ts --pcre2Length of output: 125
Script:
#!/bin/bash # Simplified search for all occurrences of collection.getItem to manually check for key coercion. rg "collection.getItem" --type tsLength of output: 815
Verification successful
The search results indicate that the
collection.getItemmethod is used in various parts of the codebase, including both theuse-aria-multiselecthook and other components/utilities liketree-keyboard-delegateandhidden-select. The usage inuse-aria-multiselectincludes coercing keys to strings, which aligns with the PR's objective for consistent handling of numeric and string keys. Other occurrences in the codebase do not seem to be directly affected by this change, as they either already use string keys or handle keys in a manner consistent with their specific contexts.Given this information, it appears that the change to coerce keys to strings when retrieving selected items in the
use-aria-multiselecthook does not negatively impact the retrieval of items elsewhere in the codebase. Each usage ofcollection.getItemis context-specific and seems to be appropriately handled within its respective component or utility.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the change in key handling does not impact the retrieval of items elsewhere in the codebase. rg "collection.getItem\((?!.*\+" --type tsLength of output: 295
Script:
#!/bin/bash # Verify that the change in key handling does not impact the retrieval of items elsewhere in the codebase. rg "collection.getItem\((?!.*\+" --type ts --pcre2Length of output: 125
Script:
#!/bin/bash # Search for all occurrences of collection.getItem to manually review the context. rg "collection.getItem" --type tsLength of output: 815
packages/hooks/use-aria-multiselect/src/use-multiselect-list-state.ts
Outdated
Show resolved
Hide resolved
packages/hooks/use-aria-multiselect/src/use-multiselect-list-state.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- packages/hooks/use-aria-multiselect/src/use-multiselect-list-state.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/hooks/use-aria-multiselect/src/use-multiselect-list-state.ts
Additional Context Used
Closes #2587
📝 Description
In collection, the key of the map is always string no matter the inputted key is numeric or string. Therefore, handle such case when we get the item from the collection.
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Summary by CodeRabbit