Migrate trigger platform key to trigger #22054
Conversation
WalkthroughWalkthroughThe changes involve a significant renaming of the Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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: 7
Outside diff range and nitpick comments (1)
src/data/automation.ts (1)
72-74: Consider proper property handling to avoid de-optimizing object shapesCombining the concerns regarding the use of
deleteand setting a property toundefined, it might be beneficial to rethink how deprecated properties are managed. Consider whether it's necessary to modify the object in place or if creating a new, sanitized object would be more efficient.Consider refactoring the migration function as follows:
export const migrateAutomationTrigger = ( trigger: Trigger | Trigger[] ): Trigger | Trigger[] => { if (Array.isArray(trigger)) { return trigger.map(migrateAutomationTrigger) as Trigger[]; } if ("platform" in trigger && !("trigger" in trigger)) { return { ...trigger, trigger: trigger.platform, platform: undefined, // or omit this line to leave the property as is }; } return trigger; };Also applies to: 396-399
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (33)
- gallery/src/pages/automation/describe-action.ts (1 hunks)
- gallery/src/pages/automation/describe-trigger.ts (1 hunks)
- src/components/device/ha-device-trigger-picker.ts (1 hunks)
- src/components/ha-selector/ha-selector-action.ts (2 hunks)
- src/components/ha-selector/ha-selector-template.ts (1 hunks)
- src/components/ha-selector/ha-selector-trigger.ts (2 hunks)
- src/data/automation.ts (4 hunks)
- src/data/automation_i18n.ts (13 hunks)
- src/data/device_automation.ts (2 hunks)
- src/data/script.ts (2 hunks)
- src/panels/config/automation/action/ha-automation-action-row.ts (3 hunks)
- src/panels/config/automation/action/ha-automation-action.ts (2 hunks)
- src/panels/config/automation/structs.ts (1 hunks)
- src/panels/config/automation/trigger/ha-automation-trigger-row.ts (7 hunks)
- src/panels/config/automation/trigger/ha-automation-trigger.ts (2 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-calendar.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-conversation.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-device.ts (2 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-event.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-geo_location.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-homeassistant.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-mqtt.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-numeric_state.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-persistent_notification.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-state.ts (2 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-sun.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-tag.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-template.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-time.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-time_pattern.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-webhook.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-zone.ts (1 hunks)
- src/panels/config/tags/ha-config-tags.ts (1 hunks)
Additional context used
Biome
src/data/automation.ts
[error] 399-399: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Additional comments not posted (42)
src/panels/config/automation/structs.ts (1)
4-4: LGTM! The property name change aligns with the standardization effort.The renaming of the
platformproperty totriggerin thebaseTriggerStructobject is a semantic update that better reflects the purpose of the property. This change is part of a larger effort to standardize the terminology used for defining triggers in the automation configuration.Please ensure that all references to the
platformproperty in thebaseTriggerStructare updated to usetriggerinstead throughout the codebase.src/components/device/ha-device-trigger-picker.ts (1)
29-29: LGTM!The change from
platform: "device"totrigger: "device"in the object returned by the constructor is consistent with the PR objective and the new naming convention. The change is localized and does not affect the rest of the class.src/components/ha-selector/ha-selector-action.ts (2)
21-26: LGTM!The new
_actionsmethod is a good addition to theHaActionSelectorclass. It efficiently processes theactionusingmemoizeOnefor optimization and handles the case when noactionis provided by returning an empty array. CallingmigrateAutomationActionensures that theactionis migrated to the latest format before being processed further.
33-33: LGTM!Updating the
rendermethod to use the new_actionsmethod is a good change. It ensures that the actions are processed efficiently and consistently before being passed to theha-automation-actioncomponent.src/components/ha-selector/ha-selector-trigger.ts (3)
3-4: LGTM!The added imports are relevant and necessary for the changes introduced in this file.
21-26: LGTM!The
_triggersmethod is a good addition to theHaTriggerSelectorclass. It encapsulates the logic of migrating triggers and provides a consistent way to access migrated triggers within the class. The use ofmemoizeOnefor caching the results is a nice optimization.
33-33: LGTM!The change in the
rendermethod to use the_triggersmethod is a good improvement. It ensures that the triggers are always migrated before being passed to theha-automation-triggercomponent, providing a consistent and reliable way to handle trigger migrations.src/panels/config/automation/trigger/types/ha-automation-trigger-mqtt.ts (1)
24-24: LGTM! The property renaming aligns with the PR objective.The change from
platformtotriggerin the default configuration object is consistent with the goal of migrating the trigger platform key to a new trigger structure.Note that this is a breaking change for any code that relies on the
platformproperty. Existing code will need to be updated to use thetriggerproperty instead.src/panels/config/automation/trigger/types/ha-automation-trigger-time_pattern.ts (1)
25-25: LGTM!The change from
platformtotriggerin thedefaultConfigmethod aligns with the PR objective of migrating the trigger platform key to trigger. It maintains consistency with the updated naming convention across the codebase, as mentioned in the AI-generated high-level summary.src/panels/config/automation/trigger/types/ha-automation-trigger-tag.ts (1)
23-23: Verify the usage of thedefaultConfigmethod in the codebase.The change in the return value of the
defaultConfigmethod is consistent with the PR objective of migrating the trigger platform key to a new trigger structure. The change is isolated to the return value and does not affect the rest of the class implementation.Run the following script to verify the usage of the
defaultConfigmethod:Verification successful
Usage of
defaultConfigmethod verified.The
HaTagTrigger.defaultConfigmethod is used appropriately in the codebase, and no residual references to the old configuration structure were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `defaultConfig` method in the codebase. # Test: Search for the method usage. Expect: No occurrences of the old structure. rg --type typescript -A 5 $'HaTagTrigger\.defaultConfig'Length of output: 92
Script:
#!/bin/bash # Description: Verify the usage of the `defaultConfig` method in the codebase. # Test: Search for the method usage using the correct file type or a glob pattern. rg --type ts -A 5 'HaTagTrigger\.defaultConfig'Length of output: 462
src/components/ha-selector/ha-selector-template.ts (1)
10-10: LGTM!The change to the
WARNING_STRINGSconstant is consistent with the PR objective of migrating the trigger platform key to a new trigger structure. The change is localized and unlikely to introduce any bugs or regressions.src/panels/config/automation/trigger/types/ha-automation-trigger-sun.ts (1)
48-48: Verify the usage of thetriggerproperty across the codebase.The renaming of the
platformproperty totriggerin thedefaultConfigmethod looks good and aligns with the PR objective.To ensure consistency and avoid potential bugs, please verify that all references to the
platformproperty have been updated totriggeracross the codebase, especially in the following areas:
- Configuration schemas and default values
- Automation and trigger-related components
- Documentation and examples
You can use the following script to search for occurrences of the
platformproperty in TypeScript files:Review the search results and ensure that all relevant occurrences have been updated to
trigger.src/panels/config/automation/trigger/types/ha-automation-trigger-geo_location.ts (1)
48-48: LGTM!The change is consistent with the PR objective and the AI-generated summary. The property renaming from
platformtotriggeris a straightforward change and is unlikely to cause any issues.src/panels/config/automation/trigger/types/ha-automation-trigger-template.ts (1)
26-26: LGTM! The property renaming aligns with the PR objectives.The change from
platformtotriggerin thedefaultConfigreturn value is consistent with the migration of the trigger platform key to a new trigger structure, as outlined in the PR objectives and AI summary.This is a breaking change, but it appears to be an intentional part of the larger migration effort.
src/panels/config/automation/trigger/types/ha-automation-trigger-event.ts (1)
23-23: LGTM!The renaming of the
platformproperty totriggerin thedefaultConfigmethod aligns with the PR objective and contributes to the standardization of terminology for defining triggers. The change is isolated and does not introduce any functional alterations.src/panels/config/automation/trigger/types/ha-automation-trigger-persistent_notification.ts (1)
75-75: LGTM!The renaming of the
platformproperty totriggeris more semantically accurate and consistent with the PR objective.Please ensure that all references to the
platformproperty have been updated totriggeracross the codebase to maintain consistency.Run the following script to verify the property usage:
Verification successful
Verification Successful!
All instances of the
platformproperty have been successfully renamed totriggeracross the codebase.
- src/panels/config/automation/trigger/types/ha-automation-trigger-persistent_notification.ts
- src/data/automation.ts
- gallery/src/pages/automation/describe-trigger.ts
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the `platform` property have been updated to `trigger`. # Test: Search for the old property name. Expect: No occurrences. rg --type typescript $'platform:(\s+)?"persistent_notification"' # Test: Search for the new property name. Expect: Only occurrences of the new property name. rg --type typescript $'trigger:(\s+)?"persistent_notification"'Length of output: 200
Script:
#!/bin/bash # Description: Verify all references to the `platform` property have been updated to `trigger`. # Test: Search for the old property name. Expect: No occurrences. rg --type ts 'platform\s*:\s*"persistent_notification"' # Test: Search for the new property name. Expect: Only occurrences of the new property name. rg --type ts 'trigger\s*:\s*"persistent_notification"'Length of output: 400
src/panels/config/automation/trigger/types/ha-automation-trigger-time.ts (1)
23-23: LGTM! Ensure all necessary updates are made to consuming code.The change from
platformtotriggeris consistent with the overall migration effort and does not introduce any logical errors.However, this is a breaking change for any code that relies on the
platformproperty being set in the default config. Please ensure that all necessary updates are made to any code that consumes the default config and expects theplatformproperty.Run the following script to verify all occurrences of
defaultConfigusage:Verification successful
Verification Successful:
platformproperty correctly replaced withtrigger.The
platformproperty inha-automation-trigger-time.tshas been successfully updated totrigger. No instances ofplatformproperty access remain in this file.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all occurrences of `defaultConfig` usage have been updated. # Test: Search for `defaultConfig` usage. Expect: No occurrences of `platform` property access. rg --type typescript $'HaTimeTrigger\.defaultConfig' -A 5Length of output: 93
Script:
#!/bin/bash # Description: Verify all occurrences of `defaultConfig` usage have been updated. # Test: Search for `defaultConfig` usage. Expect: No occurrences of `platform` property access. rg --type ts 'HaTimeTrigger\.defaultConfig' -A 5Length of output: 472
Script:
#!/bin/bash # Description: Check for any remaining occurrences of the `platform` property in the codebase. # Test: Search for the `platform` property in all TypeScript files. Expect: No matches. rg --type ts 'platform' -A 2Length of output: 19092
gallery/src/pages/automation/describe-trigger.ts (2)
25-26: LGTM!The change from
platformtotriggeris consistent with the goal of standardizing trigger definitions. The modification looks good.
28-28: Looks good!The renaming of
platformtotriggerhas been applied consistently across all the trigger definitions in thetriggersarray and theinitialTriggerobject. This change aligns with the goal of standardizing the terminology used for defining triggers.The modifications have been made correctly and there are no apparent issues with the logic or syntax.
Also applies to: 33-33, 35-35, 41-45, 47-47, 52-55, 57-57, 60-60, 64-64
src/panels/config/automation/trigger/types/ha-automation-trigger-zone.ts (1)
28-28: LGTM! Ensure dependent code is updated.Renaming the property from
platformtotriggeraligns with the trigger definition structure and improves consistency. This change enhances code readability.Verify that any code depending on the old
platformproperty has been updated to usetrigger. Run the following script to find any potential occurrences:gallery/src/pages/automation/describe-action.ts (1)
51-53: LGTM! The change aligns with the PR objective.The modification of the key from
platformtotriggerfor defining the trigger within the wait condition is consistent with the goal of migrating the trigger platform key across the codebase.Please ensure that:
- Existing configurations and code that rely on the
platformkey for defining triggers within wait conditions are updated to usetriggerinstead.- The migration process is carefully managed to maintain compatibility, as this is a breaking change.
- The
input_boolean.toggle_1entity used in the trigger definition exists in the actual codebase (it's not present in the providedENTITIESarray in this file).src/panels/config/automation/trigger/types/ha-automation-trigger-conversation.ts (1)
29-29: LGTM! The property name change aligns with the new naming convention.The change from
platformtotriggerin thedefaultConfigmethod is part of the larger refactor to standardize the terminology used for defining triggers. The functionality remains the same, as the value of"conversation"is preserved.src/panels/config/automation/trigger/types/ha-automation-trigger-device.ts (1)
43-43: LGTM!The changes in the
HaDeviceTriggerclass are consistent and align with the renaming of theplatformproperty totrigger. The changes are localized to this class and do not introduce any logical errors or bugs.Also applies to: 158-158
src/panels/config/automation/trigger/types/ha-automation-trigger-webhook.ts (1)
41-41: LGTM!The change from
platformtotriggeraligns with the PR objective of migrating the trigger platform key to a new trigger structure.Verify that all the code referencing this configuration has been updated to use the new
triggerkey instead ofplatform. Run the following script to search for any remaining references to the old key:Verification successful
Verified!
All references to the old
platformkey have been successfully removed and replaced withtrigger. No remaining instances ofplatform: "webhook"were found in the TypeScript files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining references to the old `platform` key. Expect: No results. # Test: Search for the old key rg --type typescript $'platform: "webhook"'Length of output: 79
Script:
#!/bin/bash # Description: Search for any remaining references to the old `platform` key in TypeScript files. Expect: No results. # Test: Search for the old key in .ts and .tsx files rg --glob "*.ts" --glob "*.tsx" 'platform: "webhook"'Length of output: 55
src/panels/config/automation/trigger/types/ha-automation-trigger-state.ts (2)
52-52: Update to default config looks good, but verify dependent code.Updating the default configuration to use the
triggerproperty instead ofplatformis consistent with the previous change. However, please ensure that any code relying on thedefaultConfigmethod is updated to expect thetriggerproperty in the returned object.Run the following script to verify the usage of the
defaultConfigmethod:#!/bin/bash # Description: Verify code relying on the `defaultConfig` method is updated to expect the `trigger` property. # Test: Search for usage of the `defaultConfig` method. Expect: Only occurrences handling the `trigger` property. rg --type typescript $'HaStateTrigger\\.defaultConfig\\(\\)'
32-32: Renaming property is fine, but verify all references are updated.The change from
platformtotriggeris a good refactor for consistency. However, please ensure that all references to theplatformproperty have been updated totriggeracross the codebase to avoid any inconsistencies or errors.Run the following script to verify the property usage:
Verification successful
All references to
platformhave been successfully updated totrigger.No remaining instances of the
platformproperty were found in the TypeScript files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the `platform` property have been updated to `trigger`. # Test: Search for the `platform` property usage. Expect: No occurrences found. rg --type typescript $'platform: literal\\("state"\\)'Length of output: 93
Script:
#!/bin/bash # Description: Verify that all references to the `platform` property have been updated to `trigger` in TypeScript files. # Use ast-grep to search for any usage of the `platform` property in TypeScript files. ast-grep --lang typescript --pattern 'platform: $_'Length of output: 53
src/data/device_automation.ts (1)
94-94: Looks good!Updating the
deviceAutomationIdentifiersarray to include"trigger"instead of"platform"keeps it in sync with theDeviceTriggertype change.This ensures that the
"trigger"property is correctly included in comparisons made by thedeviceAutomationsEqualfunction.src/panels/config/automation/trigger/ha-automation-trigger.ts (2)
133-133: LGTM!The change correctly renames the
platformproperty totriggerwhen accessing the clipboard item, aligning with the overall renaming across the codebase. The use of the optional chaining operator?.ensures safe access to the nested properties.
142-144: LGTM!The changes correctly rename the
platformvariable totrigger, update the custom element retrieval string, and cast the variable fromvalueasTrigger["trigger"]. These changes align with the overall renaming ofplatformtotriggeracross the codebase.src/panels/config/automation/action/ha-automation-action.ts (1)
246-246: Simplification of the assignment logic looks good, but verify compatibility.The change simplifies the assignment of the action value by directly assigning
ev.detail.valuetonewValue, eliminating the conditional logic that involved themigrateAutomationActionfunction.However, please verify that removing the migration step does not lead to any compatibility issues with existing automation configurations. Run the following script to check for potential issues:
src/panels/config/tags/ha-config-tags.ts (1)
216-216: LGTM!The change to the
triggerproperty in the_handleAutomationClickmethod is consistent with the PR objective of migrating the trigger platform key to a new trigger structure. The update fromplatform: "tag"totrigger: "tag"aligns with the broader changes mentioned in the AI-generated summary.src/panels/config/automation/trigger/types/ha-automation-trigger-numeric_state.ts (1)
242-242: Verify the impact of renamingplatformtotriggerin the codebase.The change aligns with the PR objective and improves clarity. However, ensure that all references to the
platformkey in trigger configurations have been updated accordingly.Run the following script to verify the usage of the
platformkey in trigger configurations:Verification successful
All instances of the
platformkey in trigger configurations have been successfully renamed totrigger. No remaining references toplatformwere found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the `platform` key in trigger configurations have been updated to `trigger`. # Test: Search for the `platform` key usage in trigger configurations. Expect: No occurrences of `platform` key. rg --type typescript $'"platform":\\s*"numeric_state"' src/data/automation.tsLength of output: 112
Script:
#!/bin/bash # Description: Verify all references to the `platform` key in trigger configurations have been updated to `trigger`. # Test: Search for the `platform` key usage in trigger configurations across TypeScript files. Expect: No occurrences of `platform` key. rg --type ts '"platform":\s*"numeric_state"' src/data/automation.tsLength of output: 69
src/data/script.ts (1)
484-487: LGTM!The new conditional block correctly handles the migration of the
wait_for_triggeraction type by callingmigrateAutomationTriggeron thewait_for_triggerproperty. This change is consistent with the existing migration logic for other action types and does not appear to introduce any issues.src/panels/config/automation/action/ha-automation-action-row.ts (3)
58-58: LGTM!The import statement for the
migrateAutomationActionfunction is correctly added.
80-80: LGTM!The import statement for the
ha-automation-action-sequencecomponent is correctly added.
568-570: LGTM!The
_onYamlChangemethod is correctly updated to callmigrateAutomationActionon theev.detail.valuebefore firing thevalue-changedevent. This ensures that the automation action is in the new format when the event is fired.src/data/automation_i18n.ts (1)
Line range hint
106-701: Standardized trigger terminology. LGTM!The changes consistently replace
trigger.platformwithtrigger.triggeracross all trigger types. This standardizes the terminology used for defining triggers, aligning with the PR objective of migrating the trigger platform key to a new trigger structure.The changes are unlikely to introduce any functional issues as the core logic remains the same.
src/data/automation.ts (3)
72-74: Deprecation Warning: Ensure consistent use of the newtriggerpropertyThe
BaseTriggerinterface now deprecates theplatformproperty in favor oftrigger. Confirm that all implementations and usages across the codebase are updated to usetriggerto prevent potential mismatches or errors.
Line range hint
81-180: Consistent Update totriggerProperty in Trigger InterfacesAll trigger interfaces correctly replace the
platformproperty with thetriggerproperty, ensuring consistency across the codebase. This aids in standardizing the trigger configuration.
387-402: Confirm recursive handling of triggers inmigrateAutomationTriggerfunctionThe
migrateAutomationTriggerfunction recursively migrates triggers from usingplatformtotrigger. Ensure that this function correctly handles all possible trigger configurations, including nested arrays, to prevent any unexpected behavior during runtime.Run the following script to check the migration function with various trigger configurations:
Verification successful
migrateAutomationTriggerFunction Correctly Handles All Trigger ConfigurationsAfter reviewing all usages of
migrateAutomationTrigger, no instances of improper or deeply nested trigger configurations were found. The function handles both single triggers and arrays of triggers recursively as intended.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that migrateAutomationTrigger handles triggers correctly. # Test: Search for usages of migrateAutomationTrigger. Expect: Proper handling of different trigger structures. rg 'migrateAutomationTrigger' -A 3 -B 3Length of output: 4000
Script:
#!/bin/bash # Description: Verify that migrateAutomationTrigger handles nested trigger configurations correctly. # Search for migrateAutomationTrigger calls with array arguments rg 'migrateAutomationTrigger\(\s*\[' -A 2 -B 2 # Search for migrateAutomationTrigger calls with nested arrays rg 'migrateAutomationTrigger\(\s*\[\s*\[' -A 2 -B 2Length of output: 102
Tools
Biome
[error] 399-399: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
src/panels/config/automation/trigger/ha-automation-trigger-row.ts (2)
44-48: Imports Updated to Include Migration UtilitiesThe addition of
Trigger,migrateAutomationTrigger, andsubscribeTriggerfrom"../../../../data/automation"is appropriate for the migration changes.
78-78: ImportpreventDefaultfrom Common UtilityGood job importing
preventDefaultfrom the common utility instead of defining it locally. This enhances code reuse and maintainability.
src/panels/config/automation/trigger/types/ha-automation-trigger-homeassistant.ts
Show resolved
Hide resolved
src/panels/config/automation/trigger/types/ha-automation-trigger-calendar.ts
Show resolved
Hide resolved
| @@ -144,7 +147,7 @@ export default class HaAutomationTriggerRow extends LitElement { | |||
| if (!this.trigger) return nothing; | |||
|
|
|||
| const supported = | |||
| customElements.get(`ha-automation-trigger-${this.trigger.platform}`) !== | |||
| customElements.get(`ha-automation-trigger-${this.trigger.trigger}`) !== | |||
There was a problem hiding this comment.
Consider Renaming trigger.trigger to Improve Readability
Using this.trigger.trigger may be confusing and could reduce code readability. Consider renaming the inner trigger property to type or platform to enhance clarity and avoid redundancy.
Also applies to: 171-171, 339-339, 369-369
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
gallery/src/data/traces/motion-light-trace.ts (2)
Line range hint
163-168: Inconsistency in trigger key namingWhile the main trigger has been updated to use
trigger: "state", there's an inconsistency in thewait_for_triggerarray whereplatform: "state"is still being used. To maintain consistency across all trigger-related configurations, consider updating this as well.Here's the suggested change:
{ wait_for_trigger: [ { - platform: "state", + trigger: "state", entity_id: "binary_sensor.pauluss_macbook_pro_camera_in_use", from: "on", to: "off", }, ], },
Line range hint
1-191: Consider updating all trigger-related keys in demo filesWhile the main change has been implemented correctly, there's an inconsistency in the
wait_for_triggersection. Given that this is a demo trace file, it might be beneficial to update all occurrences ofplatformtotriggerfor consistency, even in demo files. This would ensure that developers referring to these demo traces see the most up-to-date trigger structure throughout.Consider doing a thorough check of all demo and example files to ensure complete consistency with the new trigger structure.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- gallery/src/data/traces/basic_trace.ts (1 hunks)
- gallery/src/data/traces/motion-light-trace.ts (1 hunks)
- gallery/src/pages/automation/editor-trigger.ts (1 hunks)
Additional comments not posted (2)
gallery/src/pages/automation/editor-trigger.ts (1)
114-114: LGTM! Verify consistency across the codebase.The change from
platformtotriggeraligns with the PR objective of migrating the trigger platform key. However, to ensure a smooth transition:
- Check if
HaConversationTrigger.defaultConfig(used earlier in this file) needs a similar update.- Verify that this change is consistently applied across the entire codebase, especially in parts that interact with the automation system.
To assist in verifying the consistency of this change, you can run the following script:
This script will help identify any inconsistencies in the migration and highlight areas that might need attention.
Verification successful
Verified! All instances of
platform: "conversation"have been successfully migrated totrigger: "conversation".
- No remaining
platform: "conversation"instances found.trigger: "conversation"is consistently used across the codebase.HaConversationTrigger.defaultConfigis appropriately utilized ineditor-trigger.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of 'platform: "conversation"' in the codebase # and instances of the new 'trigger: "conversation"' syntax. echo "Checking for remaining 'platform: \"conversation\"' instances:" rg --type typescript --type javascript 'platform:\s*"conversation"' echo -e "\nChecking for new 'trigger: \"conversation\"' instances:" rg --type typescript --type javascript 'trigger:\s*"conversation"' echo -e "\nChecking for HaConversationTrigger.defaultConfig usage:" rg --type typescript --type javascript 'HaConversationTrigger\.defaultConfig'Length of output: 716
Script:
#!/bin/bash # Description: Check for any remaining instances of 'platform: "conversation"' in the codebase # and instances of the new 'trigger: "conversation"' syntax. Also check for HaConversationTrigger.defaultConfig usage. echo "Checking for remaining 'platform: \"conversation\"' instances in .ts and .tsx files:" rg --glob "*.ts" --glob "*.tsx" 'platform:\s*"conversation"' echo -e "\nChecking for new 'trigger: \"conversation\"' instances in .ts and .tsx files:" rg --glob "*.ts" --glob "*.tsx" 'trigger:\s*"conversation"' echo -e "\nChecking for HaConversationTrigger.defaultConfig usage in .ts and .tsx files:" rg --glob "*.ts" --glob "*.tsx" 'HaConversationTrigger\.defaultConfig'Length of output: 1272
gallery/src/data/traces/basic_trace.ts (1)
222-224: Confirm consistent application oftriggerkey replacementThe change from
platformtotriggeraligns with the PR objective to migrate the trigger platform key. This enhances consistency in the trigger definition structure.To ensure this change has been applied consistently:
Consider the following to ensure a smooth transition:
- Implement a migration strategy for existing configurations using the old
platformkey.- Update relevant documentation to reflect this change in trigger definition structure.
- If not already done, add unit tests to verify correct handling of both old and new trigger definition formats.
Verification successful
Verification Successful:
platformkey successfully replaced withtriggerAll instances of the
platformkey with the value"state"have been successfully replaced with thetriggerkey across the codebase. No remaining occurrences ofplatformwere found, ensuring consistency with the PR objectives.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of 'platform' in trigger definitions rg --type typescript 'platform:\s*"state"' src/ # Search for new instances of 'trigger' key in trigger definitions rg --type typescript 'trigger:\s*"state"' src/Length of output: 168
Script:
#!/bin/bash # Search for any remaining instances of 'platform' in trigger definitions using file extension rg --glob '*.ts' 'platform:\s*"state"' src/ # Search for new instances of 'trigger' key in trigger definitions using file extension rg --glob '*.ts' 'trigger:\s*"state"' src/Length of output: 255
bramkragten
force-pushed
the
trigger-platform
branch
from
5337a96 to
206d5fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (4)
src/data/automation.ts (3)
78-80: LGTM! Consider adding a migration guide.The changes to the
BaseTriggerinterface look good. The deprecation ofplatformand introduction oftriggeris a step towards standardizing trigger definitions.Consider adding a link to a migration guide in the deprecation notice to help users update their code:
- /** @deprecated Use `trigger` instead */ + /** @deprecated Use `trigger` instead. See migration guide: [URL] */
411-417: LGTM! Consider adding error handling.The updates to
migrateAutomationConfiglook good. They ensure that both triggers and actions are migrated to the new format.Consider adding error handling to gracefully manage any migration failures:
if (config.triggers) { - config.triggers = migrateAutomationTrigger(config.triggers); + try { + config.triggers = migrateAutomationTrigger(config.triggers); + } catch (error) { + console.error("Failed to migrate triggers:", error); + // Handle the error appropriately + } } if (config.actions) { - config.actions = migrateAutomationAction(config.actions); + try { + config.actions = migrateAutomationAction(config.actions); + } catch (error) { + console.error("Failed to migrate actions:", error); + // Handle the error appropriately + } }
Line range hint
1-538: Consider updating documentation and adding migration guidesThe changes in this file are part of a significant refactoring to standardize trigger definitions in the automation system. While the code changes look good, it's important to ensure that users can easily migrate their existing automations.
- Update the main documentation to reflect the new
triggerproperty usage instead ofplatform.- Create a comprehensive migration guide that explains:
- Why this change was made
- How to update existing automations
- Any potential breaking changes or edge cases to be aware of
- Consider adding a deprecation warning in the runtime that detects old-style automations and points users to the migration guide.
These steps will help ensure a smooth transition for users and minimize potential issues during the migration process.
src/panels/config/automation/ha-automation-editor.ts (1)
Line range hint
1-924: Overall changes look good, but consider documentation updates.The changes in this file are focused on updating the handling of automation configurations, specifically:
- Removing the
migrateAutomationConfigimport and usage.- Adding an explicit import for
substituteBlueprint.- Simplifying the config normalization in the
_yamlChangedmethod.These changes align well with the PR objectives of migrating the trigger platform key to a new trigger structure. The code looks cleaner and more straightforward.
However, given the significance of these changes in how automation configurations are handled, consider the following:
- Update any relevant documentation to reflect the new way of handling automation configurations.
- Add comments in the code to explain the rationale behind using
normalizeAutomationConfigdirectly, especially if it differs significantly from the previousmigrateAutomationConfigapproach.- Consider adding a brief comment near the
substituteBlueprintimport to explain its purpose, as it seems to play a crucial role in the new configuration handling.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (37)
- gallery/src/data/traces/basic_trace.ts (1 hunks)
- gallery/src/data/traces/motion-light-trace.ts (1 hunks)
- gallery/src/pages/automation/describe-action.ts (1 hunks)
- gallery/src/pages/automation/describe-trigger.ts (1 hunks)
- gallery/src/pages/automation/editor-trigger.ts (1 hunks)
- src/components/device/ha-device-trigger-picker.ts (1 hunks)
- src/components/ha-selector/ha-selector-action.ts (2 hunks)
- src/components/ha-selector/ha-selector-template.ts (1 hunks)
- src/components/ha-selector/ha-selector-trigger.ts (2 hunks)
- src/data/automation.ts (4 hunks)
- src/data/automation_i18n.ts (13 hunks)
- src/data/device_automation.ts (2 hunks)
- src/data/script.ts (2 hunks)
- src/panels/config/automation/action/ha-automation-action-row.ts (3 hunks)
- src/panels/config/automation/action/ha-automation-action.ts (2 hunks)
- src/panels/config/automation/ha-automation-editor.ts (2 hunks)
- src/panels/config/automation/structs.ts (1 hunks)
- src/panels/config/automation/trigger/ha-automation-trigger-row.ts (7 hunks)
- src/panels/config/automation/trigger/ha-automation-trigger.ts (2 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-calendar.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-conversation.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-device.ts (2 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-event.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-geo_location.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-homeassistant.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-mqtt.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-numeric_state.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-persistent_notification.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-state.ts (2 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-sun.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-tag.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-template.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-time.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-time_pattern.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-webhook.ts (1 hunks)
- src/panels/config/automation/trigger/types/ha-automation-trigger-zone.ts (1 hunks)
- src/panels/config/tags/ha-config-tags.ts (1 hunks)
Files skipped from review as they are similar to previous changes (35)
- gallery/src/data/traces/basic_trace.ts
- gallery/src/data/traces/motion-light-trace.ts
- gallery/src/pages/automation/describe-action.ts
- gallery/src/pages/automation/describe-trigger.ts
- gallery/src/pages/automation/editor-trigger.ts
- src/components/device/ha-device-trigger-picker.ts
- src/components/ha-selector/ha-selector-action.ts
- src/components/ha-selector/ha-selector-template.ts
- src/components/ha-selector/ha-selector-trigger.ts
- src/data/automation_i18n.ts
- src/data/device_automation.ts
- src/data/script.ts
- src/panels/config/automation/action/ha-automation-action-row.ts
- src/panels/config/automation/action/ha-automation-action.ts
- src/panels/config/automation/structs.ts
- src/panels/config/automation/trigger/ha-automation-trigger-row.ts
- src/panels/config/automation/trigger/ha-automation-trigger.ts
- src/panels/config/automation/trigger/types/ha-automation-trigger-calendar.ts
- src/panels/config/automation/trigger/types/ha-automation-trigger-conversation.ts
- src/panels/config/automation/trigger/types/ha-automation-trigger-device.ts
- src/panels/config/automation/trigger/types/ha-automation-trigger-event.ts
- src/panels/config/automation/trigger/types/ha-automation-trigger-geo_location.ts
- src/panels/config/automation/trigger/types/ha-automation-trigger-homeassistant.ts
- src/panels/config/automation/trigger/types/ha-automation-trigger-mqtt.ts
- src/panels/config/automation/trigger/types/ha-automation-trigger-numeric_state.ts
- src/panels/config/automation/trigger/types/ha-automation-trigger-persistent_notification.ts
- src/panels/config/automation/trigger/types/ha-automation-trigger-state.ts
- src/panels/config/automation/trigger/types/ha-automation-trigger-sun.ts
- src/panels/config/automation/trigger/types/ha-automation-trigger-tag.ts
- src/panels/config/automation/trigger/types/ha-automation-trigger-template.ts
- src/panels/config/automation/trigger/types/ha-automation-trigger-time.ts
- src/panels/config/automation/trigger/types/ha-automation-trigger-time_pattern.ts
- src/panels/config/automation/trigger/types/ha-automation-trigger-webhook.ts
- src/panels/config/automation/trigger/types/ha-automation-trigger-zone.ts
- src/panels/config/tags/ha-config-tags.ts
Additional context used
Biome
src/data/automation.ts
[error] 434-434: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Additional comments not posted (4)
src/data/automation.ts (2)
87-87: LGTM! Consistent updates across all trigger interfaces.The changes to replace
platformwithtriggerin all trigger interfaces are consistent and improve the overall structure of the automation system.Also applies to: 96-96, 102-102, 109-109, 114-114, 124-124, 129-129, 135-135, 142-142, 149-149, 155-155, 162-162, 168-168, 173-173, 179-179, 186-186
422-437:⚠️ Potential issueImprove type safety and performance in migrateAutomationTrigger
The new
migrateAutomationTriggerfunction looks good overall, but there are a couple of areas for improvement:
- The use of
@ts-ignoresuppresses TypeScript errors but can mask underlying type issues.- The
deleteoperator can negatively impact performance.Apply this diff to address these issues:
export const migrateAutomationTrigger = ( trigger: Trigger | Trigger[] ): Trigger | Trigger[] => { if (Array.isArray(trigger)) { return trigger.map(migrateAutomationTrigger) as Trigger[]; } if ("platform" in trigger) { if (!("trigger" in trigger)) { - // @ts-ignore - trigger.trigger = trigger.platform; + (trigger as any).trigger = trigger.platform; } - delete trigger.platform; + trigger.platform = undefined; } return trigger; };These changes improve type safety by using a type assertion instead of
@ts-ignore, and enhance performance by settingplatformtoundefinedinstead of usingdelete.Tools
Biome
[error] 434-434: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
src/panels/config/automation/ha-automation-editor.ts (2)
642-642: LGTM: Simplified config normalization.The direct use of
normalizeAutomationConfigonev.detail.valuesimplifies the code and aligns with the removal ofmigrateAutomationConfig. This change is consistent with the PR objectives of updating the trigger structure.
Line range hint
1-924: Final thoughts: Changes look good, consider testing and documentation.The changes in this file successfully update the handling of automation configurations, aligning with the PR objectives. The code is now more streamlined, with the removal of
migrateAutomationConfigand the direct use ofnormalizeAutomationConfig.To ensure a smooth transition and maintain code quality:
- Thoroughly test the new configuration handling process, especially with various types of automation configs.
- Update any related documentation or comments in other files that might reference the old
migrateAutomationConfigapproach.- Consider adding a brief comment in the
_yamlChangedmethod explaining the rationale behind usingnormalizeAutomationConfigdirectly.Overall, good job on simplifying the code while maintaining functionality!
Proposed change
For home-assistant/core#124357
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
Documentation