Split out service entities #21076
Split out service entities #21076
Conversation
WalkthroughWalkthroughThe updates primarily address the inclusion and categorization of new entities related to conversation, speech-to-text (stt), and text-to-speech (tts) functionalities. This involves altering icon references, enhancing the categorization logic for devices and entities in the device configuration page, and updating the Lovelace configuration generation process to handle these new entity types. Additionally, new translations for 'assist' and 'notify' have been incorporated to ensure clear labeling in the UI. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DeviceConfigPage
participant LovelaceConfig
participant Icons
participant Translations
User ->> DeviceConfigPage: Access Device Config Page
DeviceConfigPage ->> Icons: Use new 'mdiFormTextbox' and 'mdiForumOutline' icons
DeviceConfigPage ->> DeviceConfigPage: Categorize new "assist" and "notify" entities
User ->> LovelaceConfig: Access Lovelace Config
LovelaceConfig ->> DeviceConfigPage: Obtain `ASSIST_ENTITIES`
LovelaceConfig ->> LovelaceConfig: Adjust hidden domains with `ASSIST_ENTITIES`
LovelaceConfig ->> Translations: Fetch new "notify" and "assist" translations
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (4)
Additional context usedBiome
Gitleaks
Additional comments not posted (6)
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 (
|
| ? "sensor" | ||
| : "control" | ||
| ) as Record< | ||
| const result = groupBy(entities, (entry) => { |
There was a problem hiding this comment.
rewrote this to avoid having to compute domain three times
|
I'm not sure if "Services" is the thing we should communicate in terms of UX. While this might hold true for a notification service, it becomes pretty confusing considering we have more things called services in our UI/ecosystem. For example, a switch/light has services too, why wouldn't these be marked as such? Can we think of another grouping we could do? For example: "Notifications" or "Communications" or something like that? |
|
APIs ? |
![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
Outside diff range comments (9)
src/panels/lovelace/common/generate-lovelace-config.ts (5)
Line range hint
62-62: The functionsplitByAreaDevicehas a high cognitive complexity. Consider breaking down this function into smaller, more manageable functions to improve readability and maintainability.- const splitByAreaDevice = ( + const splitEntitiesByArea = ( ... + const splitEntitiesByDevice = (
Line range hint
220-223: The assignments within these conditional expressions can lead to code that is harder to understand and maintain. Consider refactoring these to separate the assignment and the condition.- if (titlePrefix && stateObj && (name = stripPrefixFromEntityName(computeStateName(stateObj), titlePrefix))) { + const name = titlePrefix && stateObj ? stripPrefixFromEntityName(computeState, titlePrefix) : undefined; + if (name) {Also applies to: 234-237
Line range hint
127-127: Several functions in this file, includingcomputeCards,computeDefaultViewStates, andgenerateDefaultViewConfig, have excessive complexity. Consider refactoring these functions to simplify logic and improve code clarity.- const computeCards = ( + function computeIndividualCards(...) { ... + function aggregateCards(...) { ...Also applies to: 248-248, 346-346, 488-488
Line range hint
416-416: Consider avoiding the use of thedeleteoperator for performance reasons. Instead, you could set the property toundefinedor use a different method to manage the object properties.- delete devicesWithEntities[deviceId]; + devicesWithEntities[deviceId] = undefined;
Line range hint
617-617: The non-null assertion operator is used, which can be unsafe. Consider replacing it with optional chaining to ensure the code is more robust and less prone to runtime errors.- grid.flow_from.length > 0 + grid?.flow_from?.length > 0Also applies to: 623-623
src/panels/config/devices/ha-config-device-page.ts (4)
Line range hint
97-97: Please specify more appropriate types instead of usingany. Usinganydisables TypeScript's type checking, which can lead to potential runtime errors and maintenance challenges.- action?: (ev: any) => void; + action?: (ev: Event) => void; // or a more specific event type if applicable - action: (ev) => this._signUrl(ev), + action: (ev: MouseEvent) => this._signUrl(ev), - action = (ev) => { + action = (ev: CustomEvent) => { - action(ev) { + action(ev: KeyboardEvent) {Also applies to: 698-698, 949-949, 1286-1286
Line range hint
764-764: Using non-null assertions can lead to runtime errors if not handled carefully. Consider using optional chaining (?.) to safely access properties and methods.- this.hass.devices[this.deviceId] + this.hass.devices?.[this.deviceId] - this.hass.localize("ui.common.enable") + this.hass.localize?.("ui.common.enable") - device.configuration_url!.replace("homeassistant://", "/") + device.configuration_url?.replace("homeassistant://", "/")Also applies to: 766-766, 770-770, 773-773, 774-774, 778-778, 779-779, 783-783, 785-785, 790-790, 793-793, 1034-1034, 1072-1072
Line range hint
294-294: The complexity of these functions exceeds the recommended limit. Refactoring them to reduce complexity will improve maintainability and readability. Consider breaking down complex logic into smaller, more manageable functions.- private _getDeviceActions() { + private async _getDeviceActions(): Promise<void> { + // Refactor to reduce complexity + } - private _renderIntegrationInfo( + private _renderIntegrationInfo( + device: DeviceRegistryEntry, + integrations: ConfigEntry[], + deviceInfo: TemplateResult[] + ): void { + // Simplify or split the function to reduce complexity + }Also applies to: 812-812
Line range hint
1302-1302: Avoid using the delete operator for performance reasons. Consider setting the property toundefinedinstead.- delete updates.disabled_by; + updates.disabled_by = undefined;
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
One of the approaches we quickly discussed during our frontend standup, was to maybe split it in two groups.
As notify really doesn't match well with the rest. |
| @@ -98,7 +99,7 @@ export const FIXED_DOMAIN_ICONS = { | |||
| calendar: mdiCalendar, | |||
| climate: mdiThermostat, | |||
| configurator: mdiCog, | |||
| conversation: mdiMicrophoneMessage, | |||
| conversation: mdiForumOutline, | |||
There was a problem hiding this comment.
this one was duplicate with STT.
|
Updated per comments. |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range comments (10)
src/panels/lovelace/common/generate-lovelace-config.ts (5)
Line range hint
63-63: Refactor thesplitByAreaDevicefunction to reduce complexity and improve maintainability.Consider breaking down the function into smaller, more manageable parts. For example, handling areas and devices could be separated into different functions.
Line range hint
221-224: Avoid using assignments within expressions as they can lead to code that is difficult to read and maintain.- name = stripPrefixFromEntityName(computeStateName(stateObj), titlePrefix) + const name = stripPrefixFromEntityName(computeStateName(stateObj), titlePrefix)Also applies to: 235-238
Line range hint
249-249: The complexity ofcomputeCardsfunction is too high. Consider refactoring to simplify the logic.Possible refactoring could involve splitting the function into smaller sub-functions that handle specific aspects of card computation.
Line range hint
417-417: Replace the use of the delete operator with setting the property toundefinedto avoid potential performance issues.- delete devicesWithEntities[deviceId]; + devicesWithEntities[deviceId] = undefined;
Line range hint
618-618: Replace non-null assertions with optional chaining to ensure safer access to properties.- grid.flow_from.length > 0 + grid?.flow_from?.length > 0Also applies to: 624-624
src/panels/config/devices/ha-config-device-page.ts (5)
Line range hint
815-815: Excessive complexity detected in this function. Refactoring is recommended to reduce complexity and enhance maintainability.Consider breaking down the function into smaller, more manageable sub-functions or using more streamlined logic to simplify the code.
Line range hint
297-297: The complexity of this function exceeds the maximum allowed. Refactoring is necessary to simplify the logic and improve code readability.Consider breaking down the complex conditionals and loops into smaller helper functions or using data-driven approaches to simplify the decision-making process.
Line range hint
97-97: Usage ofanytype should be avoided as it disables TypeScript's type checking. Specifying explicit types enhances type safety and code maintainability.Consider replacing
anywith specific types or interfaces that accurately describe the expected data structures.Also applies to: 701-701, 953-953, 1290-1290
Line range hint
767-767: Usage of non-null assertion operators is discouraged as they bypass TypeScript's strict null checks. This could lead to runtime errors if the assumptions about non-null values are incorrect.Consider replacing non-null assertions with optional chaining or appropriate runtime checks to ensure safety.
Also applies to: 769-769, 773-773, 776-776, 777-777, 781-781, 782-782, 786-786, 788-788, 793-793, 796-796, 1038-1038, 1076-1076
Line range hint
1306-1306: The use of thedeleteoperator can lead to performance issues due to deoptimizations in JavaScript engines.Consider setting the property to
undefinedor restructuring the code to avoid the need for deletion.- delete someProperty; + someProperty = undefined;
| @@ -31,6 +31,7 @@ import { | |||
| mdiFormatListBulleted, | |||
| mdiFormatListCheckbox, | |||
| mdiFormTextbox, | |||
| mdiForumOutline, | |||
There was a problem hiding this comment.
Consider adding a comment explaining the use of mdiForumOutline for better code clarity.
| const result = groupBy(entities, (entry) => { | ||
| const domain = computeDomain(entry.entity_id); | ||
|
|
||
| if (entry.entity_category) { | ||
| return entry.entity_category; | ||
| } | ||
|
|
||
| if (domain === "event" || domain === "notify") { | ||
| return domain; | ||
| } | ||
|
|
||
| if (SENSOR_ENTITIES.includes(domain)) { | ||
| return "sensor"; | ||
| } | ||
|
|
||
| if (ASSIST_ENTITIES.includes(domain)) { | ||
| return "assist"; | ||
| } | ||
|
|
||
| return "control"; | ||
| }) as Record< |
There was a problem hiding this comment.
Updated _entitiesByCategory method to categorize entities based on their domain and entity category. This aligns with the PR's objectives to handle "notify" and "assist" entities distinctly. However, consider adding a default case to handle any unforeseen categories.
+ default:
+ return "other"; // Handle any unforeseen categoriesCommittable 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.
| const result = groupBy(entities, (entry) => { | |
| const domain = computeDomain(entry.entity_id); | |
| if (entry.entity_category) { | |
| return entry.entity_category; | |
| } | |
| if (domain === "event" || domain === "notify") { | |
| return domain; | |
| } | |
| if (SENSOR_ENTITIES.includes(domain)) { | |
| return "sensor"; | |
| } | |
| if (ASSIST_ENTITIES.includes(domain)) { | |
| return "assist"; | |
| } | |
| return "control"; | |
| }) as Record< | |
| const result = groupBy(entities, (entry) => { | |
| const domain = computeDomain(entry.entity_id); | |
| if (entry.entity_category) { | |
| return entry.entity_category; | |
| } | |
| if (domain === "event" || domain === "notify") { | |
| return domain; | |
| } | |
| if (SENSOR_ENTITIES.includes(domain)) { | |
| return "sensor"; | |
| } | |
| if (ASSIST_ENTITIES.includes(domain)) { | |
| return "assist"; | |
| } | |
| return "control"; | |
| + default: | |
| + return "other"; // Handle any unforeseen categories | |
| }) as Record< |
Breaking change
Proposed change
While ensuring that notify entities are not shown on the generated dashboard, I realized that they are also grouped under "control" on device info page. This PR splits out notify and Assist-related domains to their own cards on the device info page.
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit
New Features
Improvements