Add sensor offset to time trigger UI #21957
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces modifications to the automation system, primarily focusing on the Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/data/automation.ts (1 hunks)
- src/data/automation_i18n.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-time.ts (4 hunks)
- src/translations/en.json (1 hunks)
Additional context used
Biome
src/panels/config/automation/trigger/types/ha-automation-trigger-time.ts
[error] 139-139: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 140-140: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 142-142: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 144-144: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Additional comments not posted (7)
src/panels/config/automation/trigger/types/ha-automation-trigger-time.ts (3)
23-26: Approved: Enhanced input mode handling.The modification to
_inputModesupports the new functionality effectively by allowing the component to handle different types of inputs, enhancing the flexibility and usability of the time trigger configuration.
33-76: Approved: Dynamic schema generation enhancements.The updates to the
_schemamethod enhance its functionality by making the schema generation dynamic and responsive to the user's input mode and offset visibility. This is a key improvement for supporting the new sensor offset feature in the time trigger UI.
100-119: Approved: Render method updates for enhanced UI responsiveness.The changes in the
rendermethod ensure that the UI components are updated dynamically based on the trigger configuration. This is crucial for the usability and accuracy of the time trigger settings, especially with the new sensor offset feature.src/data/automation.ts (1)
156-156: Approve the modification with a suggestion to verify integration.The change to the
TimeTriggerinterface, allowing theatproperty to accept both a string and a complex object, is a significant enhancement for flexibility in automation triggers. This aligns well with the PR's objectives.However, ensure that this change is thoroughly tested to confirm that it integrates seamlessly with the UI and that all parts of the system that interact with the
TimeTriggerare updated to handle the new type structure.Run the following script to verify the integration of the
TimeTriggerchanges:src/translations/en.json (3)
3046-3046: Localization Key Added: "offset"The key
"offset"has been added with the value referencing another localization key for the offset configuration in automation triggers. This is a standard practice for maintaining localization consistency and reusability.
3047-3047: Localization Key Added: "entity"The key
"entity"has been added with a descriptive value "Entity with timestamp". This is clear and provides a good description for the type of entity expected, which is useful for clarity in the user interface.
3048-3048: Localization Key Added: "offset_by"The key
"offset_by"has been added with a formatted string "offset by {offset}". This allows for dynamic insertion of the offset value in the UI, enhancing the flexibility of the localization. The use of placeholders in localization strings is a good practice as it supports dynamic content.
| const newValue = { ...ev.detail.value }; | ||
| this._inputMode = newValue.mode; | ||
| if (newValue.mode === MODE_TIME) { | ||
| delete newValue.entity; | ||
| delete newValue.offset; | ||
| } else { | ||
| delete newValue.time; | ||
| if (!newValue.entity?.startsWith("sensor.")) { | ||
| delete newValue.offset; | ||
| } | ||
| } | ||
| fireEvent(this, "value-changed", { | ||
| value: { | ||
| platform: "time", | ||
| at: newValue.offset | ||
| ? { | ||
| entity_id: newValue.entity, | ||
| offset: newValue.offset, | ||
| } | ||
| : newValue.entity || newValue.time, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Consider replacing the delete operator for better performance.
The use of the delete operator in the _valueChanged method could impact performance. Consider setting properties to undefined instead, which can be more performance-friendly in JavaScript environments.
Here's a suggested change:
- delete newValue.entity;
- delete newValue.offset;
- delete newValue.time;
+ newValue.entity = undefined;
+ newValue.offset = undefined;
+ newValue.time = undefined;Verify that this change does not affect the functionality, as the semantic meaning of undefined and a property not existing can differ in some contexts.
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.
| const newValue = { ...ev.detail.value }; | |
| this._inputMode = newValue.mode; | |
| if (newValue.mode === MODE_TIME) { | |
| delete newValue.entity; | |
| delete newValue.offset; | |
| } else { | |
| delete newValue.time; | |
| if (!newValue.entity?.startsWith("sensor.")) { | |
| delete newValue.offset; | |
| } | |
| } | |
| fireEvent(this, "value-changed", { | |
| value: { | |
| platform: "time", | |
| at: newValue.offset | |
| ? { | |
| entity_id: newValue.entity, | |
| offset: newValue.offset, | |
| } | |
| : newValue.entity || newValue.time, | |
| }, | |
| }); | |
| const newValue = { ...ev.detail.value }; | |
| this._inputMode = newValue.mode; | |
| if (newValue.mode === MODE_TIME) { | |
| newValue.entity = undefined; | |
| newValue.offset = undefined; | |
| } else { | |
| newValue.time = undefined; | |
| if (!newValue.entity?.startsWith("sensor.")) { | |
| newValue.offset = undefined; | |
| } | |
| } | |
| fireEvent(this, "value-changed", { | |
| value: { | |
| platform: "time", | |
| at: newValue.offset | |
| ? { | |
| entity_id: newValue.entity, | |
| offset: newValue.offset, | |
| } | |
| : newValue.entity || newValue.time, | |
| }, | |
| }); |
Tools
Biome
[error] 139-139: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 140-140: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 142-142: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 144-144: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
src/data/automation_i18n.ts
Outdated
| @@ -349,7 +349,7 @@ const tryDescribeTrigger = ( | |||
| if (trigger.platform === "time" && trigger.at) { | |||
| const result = ensureArray(trigger.at).map((at) => | |||
| typeof at !== "string" | |||
| ? at | |||
| ? `entity ${hass.states[at.entity_id] ? computeStateName(hass.states[at.entity_id]) : at.entity_id} ${at.offset ? " " + hass.localize(`${triggerTranslationBaseKey}.time.offset_by`, { offset: describeDuration(hass.locale, at.offset) }) : ""}` | |||
There was a problem hiding this comment.
Confirm and enhance the handling of at parameter in time triggers.
The updated implementation for handling the at parameter in time triggers is a significant improvement, providing more detailed descriptions based on the entity's state and optional offset. This change aligns with the PR's goal to enhance the user interface by adding sensor offset functionality.
However, consider enhancing the readability of this block by breaking down the complex ternary operation into more manageable parts or using helper functions. This could improve maintainability and make the code easier to understand for future contributors.
There was a problem hiding this comment.
I'll follow the rabbit comment here when reviewing. The ternary operator is being used correctly and efficiently. However, combining multiple ternaries in a single line can make the code harder to read and maintain. It might be clearer to refactor this into if-else blocks for better readability.
There was a problem hiding this comment.
@silamon: Thank you for considering the suggestion! Refactoring the complex ternary operations into if-else blocks will certainly enhance readability and make the code easier to maintain. Please feel free to reach out if you need any assistance.
(^.^)/
| const data = { | ||
| mode: inputMode ? "input" : "value", | ||
| ...this.trigger, | ||
| mode, | ||
| entity, | ||
| time, | ||
| offset, | ||
| }; |
There was a problem hiding this comment.
We should make data a memoized function so it wont create a new object on every render
|
@karwosts will this also support template based timestamp triggers? I just discovered that in the current UI, the drop-down doesn't support template sensors which are defined as being timestamps (I'm aware,that's before your changes). |
I'm surprised to hear that, doesn't sound right. The dropdown just filters for Maybe check that again. |
|
Yes, looks like it works now.. don't know why it didn't seem to work earlier. |
* Add sensor offset to time trigger UI * refactor long expression * memoize data * fix for trigger platform migration
Proposed change
Frontend for home-assistant/core#120858
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes