Fix device integration filter for entityless devices #21136
Conversation
WalkthroughWalkthroughThe recent changes aim to enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HaAreaSelector
participant HaDeviceSelector
participant HaFloorSelector
participant ConfigEntryService
participant DeviceRegistry
participant FilterSelectorDevices
User->>HaAreaSelector: Select Device
HaAreaSelector->>ConfigEntryService: Fetch config entries
ConfigEntryService-->>HaAreaSelector: Return config entries
HaAreaSelector->>DeviceRegistry: Get devices and config entries
DeviceRegistry-->>HaAreaSelector: Return device integration lookup
HaAreaSelector->>FilterSelectorDevices: Apply filter to devices with integration lookup
User->>HaDeviceSelector: Select Device
HaDeviceSelector->>ConfigEntryService: Fetch config entries
ConfigEntryService-->>HaDeviceSelector: Return config entries
HaDeviceSelector->>DeviceRegistry: Get devices and config entries
DeviceRegistry-->>HaDeviceSelector: Return device integration lookup
HaDeviceSelector->>FilterSelectorDevices: Apply filter to devices with integration lookup
User->>HaFloorSelector: Select Device
HaFloorSelector->>ConfigEntryService: Fetch config entries
ConfigEntryService-->>HaFloorSelector: Return config entries
HaFloorSelector->>DeviceRegistry: Get devices and config entries
DeviceRegistry-->>HaFloorSelector: Return device integration lookup
HaFloorSelector->>FilterSelectorDevices: Apply filter to devices with integration lookup
Assessment against linked issues
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Additional context usedBiome
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 (
|
![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
Outside diff range comments (4)
src/components/ha-selector/ha-selector-device.ts (1)
Line range hint
147-147: Remove non-null assertion.Using non-null assertions can lead to runtime errors if not handled properly. It's safer to use optional chaining.
- const deviceIntegrations = this._entitySources ? this._deviceIntegrationLookup(this._entitySources, Object.values(this.hass.entities), Object.values(this.hass.devices), this._configEntries) : undefined; + const deviceIntegrations = this._entitySources ? this._deviceIntegrationLookup(this._entitySources, Object.values(this.hass.entities), Object.values(this.hass.devices), this._configEntries!) : undefined;src/components/ha-selector/ha-selector-area.ts (1)
Line range hint
29-29: Avoid usinganytype for thevalueproperty.It's recommended to specify a more precise type instead of
anyto leverage TypeScript's type checking.- @property() public value?: any; + @property() public value?: AreaSelector[] | AreaSelector;src/components/ha-selector/ha-selector-floor.ts (1)
Line range hint
29-29: Avoid usinganytype for thevalueproperty.It's better to specify a more specific type instead of
anyto fully utilize TypeScript's type safety features.- @property() public value?: any; + @property() public value?: FloorSelector[] | FloorSelector;src/data/device_registry.ts (1)
Line range hint
158-158: Remove non-null assertions.Non-null assertions can lead to runtime errors if not properly handled. It's safer to use optional chaining.
- if (!deviceIntegrations[entity.device_id!]) { + if (!deviceIntegrations[entity.device_id]) { - deviceIntegrations[entity.device_id!] = []; + deviceIntegrations[entity.device_id] = []; } - deviceIntegrations[entity.device_id!].push(source.domain); + deviceIntegrations[entity.device_id]?.push(source.domain);Also applies to: 159-159, 161-161
src/data/device_registry.ts
Outdated
| if (entry?.domain) { | ||
| ( | ||
| deviceIntegrations[device.id] || | ||
| (deviceIntegrations[device.id] = []) |
There was a problem hiding this comment.
Avoid assignment within an expression.
Using assignments within expressions can be confusing. It's clearer to separate them.
- (deviceIntegrations[device.id] || (deviceIntegrations[device.id] = [])).push(entry.domain);
+ deviceIntegrations[device.id] = deviceIntegrations[device.id] || [];
+ deviceIntegrations[device.id].push(entry.domain);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.
| (deviceIntegrations[device.id] = []) | |
| deviceIntegrations[device.id] = deviceIntegrations[device.id] || []; | |
| deviceIntegrations[device.id].push(entry.domain); |
Tools
Biome
[error] 174-174: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
There was a problem hiding this comment.
I do agree with coderabbit on this
There was a problem hiding this comment.
@bramkragten, I'm glad you agree with the suggestion! If you need further assistance or have more questions as you implement this change, feel free to reach out. Happy coding! 🚀
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
src/data/device_registry.ts
Outdated
| ( | ||
| deviceIntegrations[device.id] || | ||
| (deviceIntegrations[device.id] = []) | ||
| ).push(entry.domain); |
There was a problem hiding this comment.
We will probably add domains multiple time now for devices with entities, should we check if the domain is already in there?
There was a problem hiding this comment.
The original code above (ln 158) seems to push the domain once per entity already, without ill effect, so I was just going with similar. I could adds checks to avoid duplicates if that's helpful.
There was a problem hiding this comment.
Maybe best to just make it a Set?
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range comments (1)
src/data/selector.ts (1)
Line range hint
699-716: Improve handling of optional properties.Using optional chaining (
?.) instead of non-null assertions (!) can prevent runtime errors if the property is undefined.- if (!deviceIntegrationLookup?.[device.id]?.has(filterIntegration)) { + if (!deviceIntegrationLookup?.[device.id].has(filterIntegration)) { return false; }
| deviceIntegrations[entity.device_id!] = | ||
| deviceIntegrations[entity.device_id!] || new Set<string>(); | ||
| deviceIntegrations[entity.device_id!].add(source.domain); |
There was a problem hiding this comment.
Avoid non-null assertions.
The non-null assertion operator ! is used here, which can lead to runtime errors if the value is actually null. Consider using optional chaining or proper checks before accessing properties.
- deviceIntegrations[entity.device_id!] = deviceIntegrations[entity.device_id!] || new Set<string>();
- deviceIntegrations[entity.device_id!].add(source.domain);
+ deviceIntegrations[entity.device_id] = deviceIntegrations[entity.device_id] || new Set<string>();
+ if (deviceIntegrations[entity.device_id]) {
+ deviceIntegrations[entity.device_id].add(source.domain);
+ }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.
| deviceIntegrations[entity.device_id!] = | |
| deviceIntegrations[entity.device_id!] || new Set<string>(); | |
| deviceIntegrations[entity.device_id!].add(source.domain); | |
| deviceIntegrations[entity.device_id] = | |
| deviceIntegrations[entity.device_id] || new Set<string>(); | |
| if (deviceIntegrations[entity.device_id]) { | |
| deviceIntegrations[entity.device_id].add(source.domain); | |
| } |
Tools
Biome
[error] 158-158: Forbidden non-null assertion. (lint/style/noNonNullAssertion)
[error] 159-159: Forbidden non-null assertion. (lint/style/noNonNullAssertion)
[error] 160-160: Forbidden non-null assertion. (lint/style/noNonNullAssertion)
| ): Record<string, Set<string>> => { | ||
| const deviceIntegrations: Record<string, Set<string>> = {}; | ||
|
|
||
| for (const entity of entities) { | ||
| const source = entitySources[entity.entity_id]; | ||
| if (!source?.domain || entity.device_id === null) { | ||
| continue; | ||
| } | ||
|
|
||
| if (!deviceIntegrations[entity.device_id!]) { | ||
| deviceIntegrations[entity.device_id!] = []; | ||
| deviceIntegrations[entity.device_id!] = | ||
| deviceIntegrations[entity.device_id!] || new Set<string>(); | ||
| deviceIntegrations[entity.device_id!].add(source.domain); | ||
| } | ||
| // Lookup devices that have no entities | ||
| if (devices && configEntries) { | ||
| for (const device of devices) { | ||
| for (const config_entry_id of device.config_entries) { | ||
| const entry = configEntries.find((e) => e.entry_id === config_entry_id); | ||
| if (entry?.domain) { | ||
| deviceIntegrations[device.id] = | ||
| deviceIntegrations[device.id] || new Set<string>(); | ||
| deviceIntegrations[device.id].add(entry.domain); | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider refactoring to reduce complexity.
The getDeviceIntegrationLookup function has been flagged for excessive complexity. Breaking down this function into smaller, more manageable parts could enhance readability and maintainability.
- export const getDeviceIntegrationLookup = (
+ function addEntityIntegrations(deviceIntegrations, entitySources, entities) {
+ for (const entity of entities) {
+ const source = entitySources[entity.entity_id];
+ if (!source?.domain || entity.device_id === null) {
+ continue;
+ }
+ deviceIntegrations[entity.device_id] = deviceIntegrations[entity.device_id] || new Set<string>();
+ deviceIntegrations[entity.device_id].add(source.domain);
+ }
+ }
+
+ function addDeviceConfigIntegrations(deviceIntegrations, devices, configEntries) {
+ for (const device of devices) {
+ for (const config_entry_id of device.config_entries) {
+ const entry = configEntries.find((e) => e.entry_id === config_entry_id);
+ if (entry?.domain) {
+ deviceIntegrations[device.id] = deviceIntegrations[device.id] || new Set<string>();
+ deviceIntegrations[device.id].add(entry.domain);
+ }
+ }
+ }
+ }
+
+ export const getDeviceIntegrationLookup = (
entitySources: EntitySources,
entities: EntityRegistryDisplayEntry[] | EntityRegistryEntry[],
devices?: DeviceRegistryEntry[],
configEntries?: ConfigEntry[]
): Record<string, Set<string>> => {
const deviceIntegrations: Record<string, Set<string>> = {};
addEntityIntegrations(deviceIntegrations, entitySources, entities);
if (devices && configEntries) {
addDeviceConfigIntegrations(deviceIntegrations, devices, configEntries);
}
return deviceIntegrations;
};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.
| ): Record<string, Set<string>> => { | |
| const deviceIntegrations: Record<string, Set<string>> = {}; | |
| for (const entity of entities) { | |
| const source = entitySources[entity.entity_id]; | |
| if (!source?.domain || entity.device_id === null) { | |
| continue; | |
| } | |
| if (!deviceIntegrations[entity.device_id!]) { | |
| deviceIntegrations[entity.device_id!] = []; | |
| deviceIntegrations[entity.device_id!] = | |
| deviceIntegrations[entity.device_id!] || new Set<string>(); | |
| deviceIntegrations[entity.device_id!].add(source.domain); | |
| } | |
| // Lookup devices that have no entities | |
| if (devices && configEntries) { | |
| for (const device of devices) { | |
| for (const config_entry_id of device.config_entries) { | |
| const entry = configEntries.find((e) => e.entry_id === config_entry_id); | |
| if (entry?.domain) { | |
| deviceIntegrations[device.id] = | |
| deviceIntegrations[device.id] || new Set<string>(); | |
| deviceIntegrations[device.id].add(entry.domain); | |
| } | |
| } | |
| function addEntityIntegrations(deviceIntegrations, entitySources, entities) { | |
| for (const entity of entities) { | |
| const source = entitySources[entity.entity_id]; | |
| if (!source?.domain || entity.device_id === null) { | |
| continue; | |
| } | |
| deviceIntegrations[entity.device_id] = deviceIntegrations[entity.device_id] || new Set<string>(); | |
| deviceIntegrations[entity.device_id].add(source.domain); | |
| } | |
| } | |
| function addDeviceConfigIntegrations(deviceIntegrations, devices, configEntries) { | |
| for (const device of devices) { | |
| for (const config_entry_id of device.config_entries) { | |
| const entry = configEntries.find((e) => e.entry_id === config_entry_id); | |
| if (entry?.domain) { | |
| deviceIntegrations[device.id] = deviceIntegrations[device.id] || new Set<string>(); | |
| deviceIntegrations[device.id].add(entry.domain); | |
| } | |
| } | |
| } | |
| } | |
| export const getDeviceIntegrationLookup = ( | |
| entitySources: EntitySources, | |
| entities: EntityRegistryDisplayEntry[] | EntityRegistryEntry[], | |
| devices?: DeviceRegistryEntry[], | |
| configEntries?: ConfigEntry[] | |
| ): Record<string, Set<string>> => { | |
| const deviceIntegrations: Record<string, Set<string>> = {}; | |
| addEntityIntegrations(deviceIntegrations, entitySources, entities); | |
| if (devices && configEntries) { | |
| addDeviceConfigIntegrations(deviceIntegrations, devices, configEntries); | |
| } | |
| return deviceIntegrations; | |
| }; |
Tools
Biome
[error] 158-158: Forbidden non-null assertion. (lint/style/noNonNullAssertion)
[error] 159-159: Forbidden non-null assertion. (lint/style/noNonNullAssertion)
[error] 160-160: Forbidden non-null assertion. (lint/style/noNonNullAssertion)
[error] 149-149: Excessive complexity detected. (lint/complexity/noExcessiveCognitiveComplexity)
Please refactor this function to reduce its complexity score from 17 to the max allowed complexity 15.
Proposed change
The current way we lookup device integrations is to use a core command to get the integration for every entity, and then check all the entities of a device, and then map the integrations of those entities to the device.
This does not work for devices without entities, as per #21060. This change leverages the config registry to get all the config entries when needed, and then checks the config entries for a device and gets the domains from there.
I'm not confident if this method should be used in addition to the existing per entity lookup, or if it can entirely replace the old method. Not sure if devices can have multiple domains or can have entities from different domains. So to be safe I just added this check in addition to the previous check, so it just adds more possible integrations that a device can be associated with.
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