Filter selected entities in entities picker using includeEntities property #22059
Conversation
WalkthroughWalkthroughThe changes involve refactoring the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HaEntitiesPickerLight
participant _includeEntities
participant _excludeEntities
User->>HaEntitiesPickerLight: Request to render entities
HaEntitiesPickerLight->>_excludeEntities: Call _excludeEntities(value, excludeEntities)
_excludeEntities-->>HaEntitiesPickerLight: Return excluded entities
HaEntitiesPickerLight->>_includeEntities: Call _includeEntities(value, includeEntities)
_includeEntities-->>HaEntitiesPickerLight: Return included entities
HaEntitiesPickerLight-->>User: Render entities based on filtered results
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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/components/entity/ha-entities-picker.ts (1)
142-154: Well-implemented_includeEntitiesmethod with optimizationThe new
_includeEntitiesmethod is well-implemented and addresses the issue described in the PR objectives. The use ofmemoizeOneis a good choice for optimization, especially if this method is called frequently during rendering.The filtering logic correctly removes entities from
includeEntitiesthat are already present invalue, which prevents duplicate selections and improves the user experience.Consider adding a brief comment explaining the purpose of this method for improved readability. For example:
// Filter out entities from includeEntities that are already selected (present in value) private _includeEntities = memoizeOne( ( value: string[] | undefined, includeEntities: string[] | undefined ): string[] | undefined => { // ... rest of the method } );
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/components/entity/ha-entities-picker.ts (3 hunks)
Additional comments not posted (3)
src/components/entity/ha-entities-picker.ts (3)
97-100: Improved handling ofincludeEntitiesThe change to use
_includeEntitiesmethod is a good improvement. It allows for dynamic filtering of theincludeEntitiesbased on the currentvalue, which can prevent duplicate entity selections and provide a more accurate list of available entities to pick from.
123-126: Consistent application of_includeEntitiesThe use of
_includeEntitieshere maintains consistency with the earlier implementation. This ensures that the new filtering logic is applied uniformly across the component, which is a good practice for maintainability and predictable behavior.
Line range hint
1-254: Summary of changes and their impactThe changes in this file successfully address the issue of
entityFilterbeing ignored when using theincludeEntitiesproperty. The new_includeEntitiesmethod provides a clean and optimized way to filter entities, ensuring that already selected entities are not included in the list of available entities to pick from.The implementation is consistent throughout the component, and the use of
memoizeOnehelps with performance optimization. These changes improve the functionality of the entity picker without introducing breaking changes.Overall, this is a well-implemented bugfix that enhances the usability of the
ha-entities-pickercomponent.To ensure that these changes don't have any unintended side effects, it would be beneficial to run the following verification:
This will help identify any other places in the codebase that might be affected by these changes and ensure that relevant tests are updated if necessary.
|
Shouldnt we use exclude entities instead? 🤔 |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
src/components/entity/ha-entities-picker.ts (1)
135-144: LGTM: New _excludeEntities method implemented correctly.The new
_excludeEntitiesmethod is well-implemented:
- Uses
memoizeOnefor performance optimization.- Correctly handles undefined
value.- Uses spread operator for immutability.
- Aligns with the PR objective of combining current value with
excludeEntities.Consider adding a type annotation for the return value of the memoized function for better clarity:
private _excludeEntities = memoizeOne( ( value: string[] | undefined, excludeEntities: string[] | undefined ): string[] | undefined => { // ... (rest of the implementation) } );src/components/entity/ha-entity-picker.ts (1)
Line range hint
1-461: Suggestions for improvementWhile the changes look good, here are a few suggestions to enhance the implementation:
Local Testing: The PR checklist indicates that the code change has not been tested locally. It's recommended to perform local testing to ensure the changes work as expected in various scenarios.
Performance Consideration: For large sets of entities, the filtering operations might impact performance. Consider adding a comment about potential optimization if performance issues are observed with extensive entity lists.
Error Handling: Consider adding explicit checks for
undefinedvalues ofincludeEntitiesandexcludeEntitiesto prevent potential runtime errors.Here's a suggestion for adding error handling:
if (includeEntities) { entityIds = entityIds.filter((entityId) => Array.isArray(includeEntities) ? includeEntities.includes(entityId) : true ); } if (excludeEntities) { entityIds = entityIds.filter((entityId) => Array.isArray(excludeEntities) ? !excludeEntities.includes(entityId) : true ); }This change ensures that filtering only occurs when
includeEntitiesandexcludeEntitiesare properly defined arrays, preventing potential runtime errors.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/components/entity/ha-entities-picker.ts (4 hunks)
- src/components/entity/ha-entity-picker.ts (2 hunks)
Additional comments not posted (5)
src/components/entity/ha-entities-picker.ts (3)
6-6: LGTM: Import statement refactored.The import statement has been updated to include both
HomeAssistantandValueChangedEventtypes from the same location. This change improves code organization without affecting functionality.
Line range hint
1-214: Summary: PR objectives achieved with improved entity filtering logic.The changes in this file successfully address the PR objective of filtering selected entities using the
includeEntitiesproperty. Key improvements include:
- Simplified entity filtering by directly using the
entityFilterproperty.- Introduction of the
_excludeEntitiesmethod to combine current value withexcludeEntities.- Removal of the
_getEntityFiltermethod, streamlining the code.These changes enhance the clarity and efficiency of the entity filtering process without introducing breaking changes. The implementation aligns well with the PR's goals and maintains the existing functionality while resolving the reported issue.
100-100: LGTM: Entity filtering logic improved.The changes simplify the code and improve the entity filtering logic:
- Direct use of
entityFilterproperty (lines 100 and 123) simplifies the code.- The new
_excludeEntitiesmethod (lines 117-120) combines current value withexcludeEntities, addressing the PR objective.These changes effectively implement the filtering of selected entities using the
includeEntitiesproperty while maintaining existing functionality.To ensure the changes work as expected, please run the following verification script:
Also applies to: 117-120, 123-123
Verification successful
Verified: Entity filtering logic confirmed.
The verification confirms that:
entityFilteris directly used inha-entities-picker.ts(lines 100 and 123).- The
_excludeEntitiesmethod is properly invoked (lines 117-120).- There are no remaining references to
_getEntityFilter, ensuring deprecated methods are removed.These findings validate that the entity filtering logic has been correctly improved and aligns with the PR objectives.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of _excludeEntities method and entityFilter property # Check for direct usage of entityFilter echo "Checking direct usage of entityFilter:" rg --type typescript 'entityFilter=' src/components/entity/ # Check for usage of _excludeEntities method echo "Checking usage of _excludeEntities method:" rg --type typescript '_excludeEntities\(' src/components/entity/ # Check for any remaining usage of _getEntityFilter method (should be none) echo "Checking for any remaining usage of _getEntityFilter method (should be none):" rg --type typescript '_getEntityFilter' src/components/entity/Length of output: 641
Script:
#!/bin/bash # Verify the usage of _excludeEntities method and entityFilter property # Check for direct usage of entityFilter in .ts and .tsx files echo "Checking direct usage of entityFilter:" fd --extension ts --extension tsx --type f . src/components/entity/ | xargs rg 'entityFilter=' # Check for usage of _excludeEntities method in .ts and .tsx files echo "Checking usage of _excludeEntities method:" fd --extension ts --extension tsx --type f . src/components/entity/ | xargs rg '_excludeEntities\(' # Check for any remaining usage of _getEntityFilter method (should be none) in .ts and .tsx files echo "Checking for any remaining usage of _getEntityFilter method (should be none):" fd --extension ts --extension tsx --type f . src/components/entity/ | xargs rg '_getEntityFilter'Length of output: 919
src/components/entity/ha-entity-picker.ts (2)
223-224: Approved: Improved consistency and fixed filtering issueThese changes correctly implement the filtering logic for
includeEntitiesandexcludeEntities. By using the parameters directly instead of accessing them throughthis., the code becomes more consistent with the method signature and easier to understand. This modification also addresses the issue mentioned in the PR objectives where theentityFilterwas ignored when using theincludeEntitiesproperty.The implementation now correctly filters entities based on both
includeEntitiesandexcludeEntities, ensuring that the entity selection process works as expected.Also applies to: 229-230
223-224: Addressing the suggestion to use "exclude entities"@bramkragten suggested using "exclude entities" instead of the current approach. It's worth noting that the current implementation already handles both
includeEntitiesandexcludeEntities. This approach provides more flexibility by allowing users to both include specific entities and exclude others.The current implementation:
- Filters entities to include only those in
includeEntities(if specified).- Then filters out entities listed in
excludeEntities(if specified).This dual-filtering approach offers more control over entity selection than using exclusion alone. It allows users to:
- Specify a subset of entities to consider (inclusion).
- Remove specific entities from that subset (exclusion).
If there are specific use cases where exclusion alone would be preferable, please provide more context so we can discuss potential optimizations or alternative implementations.
Also applies to: 229-230
Proposed change
This logic existed when using
entityFilter. I added toincludeEntitiestoo because theentityFilteris ignored when usingincludeEntitiesproperty.Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit
New Features
Bug Fixes