Migrate base automation config to plurals #22053
Conversation
|
Warning Rate limit exceeded@bramkragten has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 56 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughWalkthroughThe changes across multiple files involve a consistent update to the naming conventions of properties related to automation configurations. The singular properties 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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/components/trace/hat-script-graph.ts (2 hunks)
- src/data/automation.ts (2 hunks)
- src/data/config.ts (1 hunks)
- src/panels/config/automation/ha-automation-editor.ts (3 hunks)
- src/panels/config/automation/manual-automation-editor.ts (7 hunks)
Additional context used
Biome
src/data/automation.ts
[error] 398-398: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 404-404: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 410-410: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Additional comments not posted (17)
src/data/config.ts (1)
13-13: LGTM! The type definition change aligns with the PR objective.The update to the
ValidKeystype definition from singular to plural keys is consistent with the PR objective of migrating to a plural-based configuration system for automation in Home Assistant. This change suggests that the configuration will now support multiple triggers, actions, and conditions.As mentioned in the AI-generated summary, ensure that:
- The code that uses the
ValidKeystype is updated to align with the new structure.- The migration function is implemented correctly to facilitate the transition from the old configuration format to the new one.
src/panels/config/automation/manual-automation-editor.ts (4)
92-92: LGTM!The change from
triggertotriggersis consistent with the overall migration to plural property names. Looks good!
122-122: LGTM!The change from
conditiontoconditionsis consistent with the overall migration to plural property names. Looks good!
134-134: LGTM!The change from
conditiontoconditionsis consistent with the overall migration to plural property names. Looks good!
163-163: LGTM!The changes from
actiontoactions,triggertotriggers, andconditiontoconditionsare consistent with the overall migration to plural property names. Looks good across all these instances!Also applies to: 174-174, 188-188, 197-197, 205-205
src/components/trace/hat-script-graph.ts (5)
575-577: LGTM!The changes to use the
triggersproperty and pass it toflattenTriggersare correct and consistent with the migration to plural property names.
588-590: LGTM!The changes to use the
conditionsproperty and pass it toensureArrayare correct and consistent with the migration to plural property names.
593-595: LGTM!The changes to use the
actionsproperty and pass it toensureArrayare correct and consistent with the migration to plural property names.
575-577: This code segment was already reviewed earlier.
588-590: This code segment was already reviewed earlier.src/data/automation.ts (4)
30-38: Good use of deprecation for backward compatibilityThe migration to plural property names (
triggers,conditions,actions) while keeping the singular versions (trigger,condition,action) as deprecated ensures backward compatibility. This allows existing configurations to continue functioning while encouraging users to adopt the new plural properties.
371-372: Integrate migration into normalization processIncluding the
migrateAutomationConfig(config)call withinnormalizeAutomationConfigensures that configurations using deprecated singular properties are properly converted to the new plural properties before normalization.
375-378: Update normalization to handle plural propertiesUpdating the loop to iterate over the plural keys
["triggers", "conditions", "actions"]ensures that these properties are correctly normalized as arrays, maintaining consistency in the configuration format.
382-383: Migrate actions using plural propertyThe check and migration for
config.actionsnow correctly uses the plural form, aligning with the updated property naming convention.src/panels/config/automation/ha-automation-editor.ts (3)
49-49: ImportingmigrateAutomationConfigImporting
migrateAutomationConfigensures that the migration functionality is available for use.
524-526: Updating property names to plural form invalidateConfigChanging
trigger,condition, andactiontotriggers,conditions, andactionsaligns with the new plural-based configuration structure.
641-644: Applying migration to YAML editor changesProcessing the YAML editor value with
migrateAutomationConfigbefore updating_configensures that any legacy configurations are properly migrated to the new format.
| export const migrateAutomationConfig = < | ||
| T extends Partial<AutomationConfig> | AutomationConfig, | ||
| >( | ||
| config: T | ||
| ) => { | ||
| if ("trigger" in config) { | ||
| if (!("triggers" in config)) { | ||
| config.triggers = config.trigger; | ||
| } | ||
| delete config.trigger; | ||
| } | ||
| if ("condition" in config) { | ||
| if (!("conditions" in config)) { | ||
| config.conditions = config.condition; | ||
| } | ||
| delete config.condition; | ||
| } | ||
| if ("action" in config) { | ||
| if (!("actions" in config)) { | ||
| config.actions = config.action; | ||
| } | ||
| delete config.action; | ||
| } | ||
| return config; | ||
| }; |
There was a problem hiding this comment.
Avoid using the delete operator to improve performance
Using the delete operator on objects can degrade performance because it alters the object's shape, leading to deoptimizations in JavaScript engines. Consider setting the properties to undefined instead, or refactoring the code to avoid removing properties.
Apply this diff to replace delete operations with assignments to undefined:
if ("trigger" in config) {
if (!("triggers" in config)) {
config.triggers = config.trigger;
}
- delete config.trigger;
+ config.trigger = undefined;
}
if ("condition" in config) {
if (!("conditions" in config)) {
config.conditions = config.condition;
}
- delete config.condition;
+ config.condition = undefined;
}
if ("action" in config) {
if (!("actions" in config)) {
config.actions = config.action;
}
- delete config.action;
+ config.action = undefined;
}Alternatively, consider restructuring the code to avoid the need to remove properties, such as creating a new object to store the migrated configuration.
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.
| export const migrateAutomationConfig = < | |
| T extends Partial<AutomationConfig> | AutomationConfig, | |
| >( | |
| config: T | |
| ) => { | |
| if ("trigger" in config) { | |
| if (!("triggers" in config)) { | |
| config.triggers = config.trigger; | |
| } | |
| delete config.trigger; | |
| } | |
| if ("condition" in config) { | |
| if (!("conditions" in config)) { | |
| config.conditions = config.condition; | |
| } | |
| delete config.condition; | |
| } | |
| if ("action" in config) { | |
| if (!("actions" in config)) { | |
| config.actions = config.action; | |
| } | |
| delete config.action; | |
| } | |
| return config; | |
| }; | |
| export const migrateAutomationConfig = < | |
| T extends Partial<AutomationConfig> | AutomationConfig, | |
| >( | |
| config: T | |
| ) => { | |
| if ("trigger" in config) { | |
| if (!("triggers" in config)) { | |
| config.triggers = config.trigger; | |
| } | |
| config.trigger = undefined; | |
| } | |
| if ("condition" in config) { | |
| if (!("conditions" in config)) { | |
| config.conditions = config.condition; | |
| } | |
| config.condition = undefined; | |
| } | |
| if ("action" in config) { | |
| if (!("actions" in config)) { | |
| config.actions = config.action; | |
| } | |
| config.action = undefined; | |
| } | |
| return config; | |
| }; |
Tools
Biome
[error] 398-398: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 404-404: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 410-410: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (5)
src/data/trace.ts (1)
190-204: LGTM! Consider a minor refactoring for improved readability.The changes successfully implement the migration to plural forms for triggers, conditions, and actions while maintaining backward compatibility. This aligns well with the PR objectives.
Consider refactoring the repeated checks into a more concise form:
const pluralMap = { trigger: "triggers", condition: "conditions", action: "actions" }; let tempResult = result[raw]; if (!tempResult && raw in pluralMap) { tempResult = result[pluralMap[raw as keyof typeof pluralMap]]; }This refactoring would make the code more maintainable and easier to extend in the future if needed.
gallery/src/data/traces/motion-light-trace.ts (2)
136-143: LGTM! Consider adding a comment for clarity.The change from
triggertotriggersaligns with the PR objective of migrating to plurals for automation config. This change improves flexibility by allowing multiple triggers.Consider adding a comment to explain the purpose of this change, especially if it's part of a larger migration effort. For example:
// Migrated to plural 'triggers' to support multiple trigger configurations triggers: [ // ... existing configuration ... ],
Line range hint
144-170: LGTM! Consider adding a comment for consistency.The change from
actiontoactionsaligns with the PR objective and maintains the existing functionality of supporting multiple actions.For consistency with the
triggerssection, consider adding a similar comment here:// Migrated to plural 'actions' to maintain support for multiple action configurations actions: [ // ... existing configurations ... ],gallery/src/data/traces/basic_trace.ts (1)
Singular Keys Still Present in the Codebase
The verification revealed that singular forms (
trigger,condition,action) are still being used in multiple files. Please update these occurrences to their plural counterparts (triggers,conditions,actions) to maintain consistency and prevent potential issues.Key areas to address:
gallery/src/data/traces/basic_trace.ts
- Lines referencing
trigger,condition, andactionwithin thetracesections.src/panels/config/automation/ha-automation-editor.ts
- References to
basicTrace.config.trigger,basicTrace.config.condition, andbasicTrace.config.action.- Various Selector and Action Components
- Files such as
ha-selector.ts,hui-action-editor.ts, and others still use singular keys.Ensuring all parts of the codebase consistently use the plural forms will enhance flexibility and reduce the risk of errors.
Analysis chain
Line range hint
220-280: Consistent renaming to plural forms, but consider broader implications.The changes from singular to plural forms (
triggertotriggers,conditiontoconditions, andactiontoactions) in theconfigsection are consistent with the PR objective. This modification enhances flexibility by allowing multiple entries for each category.However, there are a few points to consider:
- Ensure that all code consuming this
basicTraceobject is updated to use the new plural keys.- The
tracesection of thebasicTraceobject still uses singular forms (e.g.,trigger/0,condition/0,action/0). Consider whether these should also be updated for consistency.- The
logbookEntriessection might need updates if it relies on the structure of theconfigsection.To ensure consistency across the codebase, run the following script:
This script will help identify any inconsistencies or areas that might need further updates.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining singular forms and usages of the old structure # Test 1: Check for remaining singular forms in TypeScript/JavaScript files echo "Checking for remaining singular forms:" rg --type ts --type js 'trigger:' -C 2 rg --type ts --type js 'condition:' -C 2 rg --type ts --type js 'action:' -C 2 # Test 2: Check for usages of the old structure echo "Checking for usages of the old structure:" rg --type ts --type js 'basicTrace\.config\.trigger' -C 2 rg --type ts --type js 'basicTrace\.config\.condition' -C 2 rg --type ts --type js 'basicTrace\.config\.action' -C 2 # Test 3: Check if trace section uses singular forms echo "Checking trace section for singular forms:" rg --type ts --type js '"trigger/\d+"' -C 2 rg --type ts --type js '"condition/\d+"' -C 2 rg --type ts --type js '"action/\d+"' -C 2Length of output: 219866
src/data/script.ts (1)
407-407: LGTM! Consider adding a comment for clarity.The change improves backward compatibility by allowing both "action" and "service" properties to identify a service action. This aligns well with the PR objective of migrating to a more flexible configuration system.
Consider adding a brief comment explaining the reason for checking both "action" and "service" properties. This would help future maintainers understand the rationale behind this change. For example:
- if ("action" in action || "service" in action) { + // Check for both "action" and "service" to support both new and legacy configurations + if ("action" in action || "service" in action) {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- gallery/src/data/traces/basic_trace.ts (1 hunks)
- gallery/src/data/traces/mock-demo-trace.ts (1 hunks)
- gallery/src/data/traces/motion-light-trace.ts (1 hunks)
- gallery/src/pages/automation/describe-action.ts (1 hunks)
- src/components/trace/hat-script-graph.ts (2 hunks)
- src/data/script.ts (1 hunks)
- src/data/trace.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/components/trace/hat-script-graph.ts
Additional comments not posted (3)
gallery/src/data/traces/mock-demo-trace.ts (1)
34-35: LGTM! Verify usage of mock data in tests.The changes from
triggertotriggersandactiontoactionsare consistent with the PR objectives of migrating to plurals for automation configuration. Initializing these as empty arrays is appropriate for the new structure.To ensure consistency across the codebase, please verify that all tests and components using this mock data have been updated to expect arrays for
triggersandactions. Run the following script to check for any remaining usage of the singular forms in test files:If any results are returned, those test files may need to be updated to use the new plural forms.
gallery/src/pages/automation/describe-action.ts (1)
124-127:⚠️ Potential issueEnsure consistency with the new automation configuration structure
The change from
servicetoactionin theinitialActionobject is a step towards the new automation configuration structure. However, there are a few points to consider:
- The value "light.turn_on" might need to be updated to match the new
actionproperty format.- This change creates an inconsistency with the rest of the file, particularly the
ACTIONSarray anddescribeActionfunction calls, which still use the oldserviceformat.To maintain consistency and fully implement the new structure:
- Update the
actionvalue to match the new format (if different from the oldserviceformat).- Review and update the
ACTIONSarray to use the newactionproperty instead ofservice.- Ensure that the
describeActionfunction and its calls throughout the file support the newactionproperty.- Update any related tests or documentation to reflect this change.
To verify the usage of
serviceandactionproperties in other files:This will help identify other files that may need similar updates.
Verification successful
Consistency confirmed with the new automation configuration structure
All instances of the
serviceproperty have been successfully replaced withactioningallery/src/pages/automation/describe-action.ts. The change aligns with the new automation configuration, and no inconsistent usages were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for 'service' and 'action' properties in automation-related files echo "Files using 'service' property:" rg --type typescript --type javascript -g '!**/node_modules/**' '"service":' src/ echo "\nFiles using 'action' property:" rg --type typescript --type javascript -g '!**/node_modules/**' '"action":' src/Length of output: 396
Script:
#!/bin/bash # Search for 'service' and 'action' properties in automation-related files echo "Files using 'service' property:" rg --type ts --type js -g '!**/node_modules/**' '"service":' src/ echo "\nFiles using 'action' property:" rg --type ts --type js -g '!**/node_modules/**' '"action":' src/Length of output: 294
Script:
#!/bin/bash # Search for 'service' and 'action' properties in 'describe-action.ts' echo "Occurrences of 'service' property in 'describe-action.ts':" rg '"service":' gallery/src/pages/automation/describe-action.ts echo "\nOccurrences of 'action' property in 'describe-action.ts':" rg '"action":' gallery/src/pages/automation/describe-action.tsLength of output: 410
gallery/src/data/traces/motion-light-trace.ts (1)
Line range hint
136-170: Verify impact on dependent code and documentation.The changes to
triggersandactionsimprove the configuration structure and align with the PR objectives. However, it's important to ensure that these changes are properly handled in any code that consumes this data structure.Please run the following script to identify potential areas that might need updates:
This script will help identify areas that might need attention due to these changes.
Verification successful
Corrected Script for Comprehensive Verification
The previous script failed to identify TypeScript files due to incorrect file type specifications. Please run the updated script below to accurately search for usages of
motionLightTrace,trigger, andactionwithin TypeScript files and related documentation.This corrected script uses the appropriate
rgfile types (tsandtsx) to ensure all relevant TypeScript files are scanned. Executing this will help identify any dependent code or documentation that requires updates based on the recent changes.
Updated Script to Accurately Search TypeScript Files
The previous script failed to recognize
.tsxfiles due to incorrect type specifications. Please use the updated script below, which correctly specifiestypescriptandtypescriptreactas the file types. This will ensure that all relevant TypeScript files are searched formotionLightTrace,trigger, andaction.This updated script correctly identifies TypeScript and TypeScript React files, ensuring a thorough search across the codebase. Please execute this script to gather the necessary information for verifying the review comment.
Revised Script with File Extension Patterns
The previous script encountered issues with unrecognized file types. To ensure all TypeScript files are correctly searched, the updated script uses glob patterns to target
.tsand.tsxfiles directly.This revised script eliminates the reliance on
--typeflags and instead uses--globpatterns to target.tsand.tsxfiles. Executing this script will accurately identify all relevant TypeScript files and any necessary documentation updates related tomotionLightTrace,trigger, andaction.
Verification Successful: No Impact on Dependent Code Found
The changes to
triggersandactionsinmotion-light-trace.tsare localized. No usages of the oldtriggeroractionkeys were found in other TypeScript files, indicating no impact on dependent code.
- Files using
motionLightTrace:
gallery/src/pages/automation/trace.tsgallery/src/data/traces/motion-light-trace.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of motionLightTrace and potential areas needing updates # Search for files using motionLightTrace echo "Files using motionLightTrace:" rg --type typescript "motionLightTrace" --files-with-matches # Search for potential usages of the old 'trigger' and 'action' keys echo "\nPotential usages of old 'trigger' key:" rg --type typescript "motionLightTrace.*trigger[^s]" echo "\nPotential usages of old 'action' key:" rg --type typescript "motionLightTrace.*action[^s]" # Search for documentation files that might need updates echo "\nDocumentation files that might need updates:" rg --type markdown "motionLightTrace|trigger|action"Length of output: 3720
Script:
#!/bin/bash # Description: Find usages of motionLightTrace and potential areas needing updates # Search for files using motionLightTrace echo "Files using motionLightTrace:" rg --type ts --type tsx "motionLightTrace" --files-with-matches # Search for potential usages of the old 'trigger' key echo "\nPotential usages of old 'trigger' key:" rg --type ts --type tsx "motionLightTrace.*trigger[^s]" # Search for potential usages of the old 'action' key echo "\nPotential usages of old 'action' key:" rg --type ts --type tsx "motionLightTrace.*action[^s]" # Search for documentation files that might need updates echo "\nDocumentation files that might need updates:" rg --type markdown "motionLightTrace|trigger|action"Length of output: 3708
Script:
#!/bin/bash # Description: Find usages of motionLightTrace and potential areas needing updates # Search for files using motionLightTrace echo "Files using motionLightTrace:" rg --type typescript --type typescriptreact "motionLightTrace" --files-with-matches # Search for potential usages of the old 'trigger' key echo "\nPotential usages of old 'trigger' key:" rg --type typescript --type typescriptreact "motionLightTrace.*trigger[^s]" # Search for potential usages of the old 'action' key echo "\nPotential usages of old 'action' key:" rg --type typescript --type typescriptreact "motionLightTrace.*action[^s]" # Search for documentation files that might need updates echo "\nDocumentation files that might need updates:" rg --type markdown "motionLightTrace|trigger|action"Length of output: 3789
Script:
#!/bin/bash # Description: Find usages of motionLightTrace and potential areas needing updates # Search for files using motionLightTrace echo "Files using motionLightTrace:" rg "motionLightTrace" --glob "*.ts" --glob "*.tsx" --files-with-matches # Search for potential usages of the old 'trigger' key echo "\nPotential usages of old 'trigger' key:" rg "motionLightTrace.*trigger[^s]" --glob "*.ts" --glob "*.tsx" # Search for potential usages of the old 'action' key echo "\nPotential usages of old 'action' key:" rg "motionLightTrace.*action[^s]" --glob "*.ts" --glob "*.tsx" # Search for documentation files that might need updates echo "\nDocumentation files that might need updates:" rg "motionLightTrace|trigger|action" --type markdownLength of output: 3732
Proposed change
Adds support and migrates automation config to use plurals for triggers, conditions and actions.
Needs: home-assistant/core#123823
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