Allow to fix statistic issue from repairs #22055
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 📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce enhancements to the handling of statistics issues within the developer tools panel. A new function, 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: 12
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/panels/config/repairs/ha-config-repairs.ts (2 hunks)
- src/panels/developer-tools/statistics/developer-tools-statistics.ts (2 hunks)
- src/panels/developer-tools/statistics/dialog-statistics-fix-units-changed.ts (5 hunks)
- src/panels/developer-tools/statistics/fix-statistics.ts (1 hunks)
- src/panels/developer-tools/statistics/show-dialog-statistics-fix-units-changed.ts (1 hunks)
Additional context used
Biome
src/panels/developer-tools/statistics/fix-statistics.ts
[error] 29-29: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 53-53: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 86-86: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 135-135: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 140-140: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
Additional comments not posted (11)
src/panels/developer-tools/statistics/dialog-statistics-fix-units-changed.ts (5)
30-31: LGTM!Moving the cancellation logic to a separate private method
_cancelimproves the separation of concerns and makes the code more maintainable.
Line range hint
33-38: LGTM!The new private method
_closeDialogconsolidates the dialog closing logic in a single place, making it easier to maintain and understand.
47-49: LGTM!Updating the event listener to call
_closeDialoginstead ofcloseDialogensures that the new centralized dialog closing logic is executed when the dialog is closed.
107-109: LGTM!Updating the secondary action button's click event to call
_cancelinstead ofcloseDialogensures that the cancellation logic is executed when the button is clicked.
118-121: LGTM!The new
_cancelmethod handles cancellation actions by invoking acancelCallback(if defined) and then closing the dialog using the centralized_closeDialogmethod. Updating the_fixIssuemethod to call_closeDialoginstead ofcloseDialogensures consistency in the dialog closing logic.Also applies to: 133-134
src/panels/developer-tools/statistics/show-dialog-statistics-fix-units-changed.ts (1)
9-10: Optional parameters enhance flexibilityThe addition of the optional
cancelCallbackand makingfixedCallbackoptional in theDialogStatisticsUnitsChangedParamsinterface improves the flexibility of the dialog handling, accommodating cases where callbacks may not be necessary.src/panels/developer-tools/statistics/fix-statistics.ts (1)
137-139: Add missing import forshowFixStatisticsUnitsChangedDialogEnsure that the
showFixStatisticsUnitsChangedDialogfunction is correctly imported to prevent runtime errors.Run the following script to check if the import path is correct:
src/panels/config/repairs/ha-config-repairs.ts (1)
18-18: Import statement is correctThe import of
fixStatisticsIssuefrom"../../developer-tools/statistics/fix-statistics"is properly specified.src/panels/developer-tools/statistics/developer-tools-statistics.ts (3)
9-9: Approved import ofLocalizeFunc.The addition of the
LocalizeFuncimport ensures proper typing for localization functions.
22-22: Approved import offixStatisticsIssue.Importing
fixStatisticsIssueallows for centralized handling of statistics issues.
261-279: Refactored_fixIssuemethod enhances maintainability.The asynchronous refactoring of the
_fixIssuemethod simplifies issue handling by delegating tofixStatisticsIssue. This reduces code redundancy and improves readability.
src/panels/developer-tools/statistics/show-dialog-statistics-fix-units-changed.ts
Show resolved
Hide resolved
| confirmText: localize("ui.common.delete"), | ||
| destructive: true, | ||
| confirm: async () => { | ||
| await clearStatistics(element.hass, [issue.data.statistic_id]); |
There was a problem hiding this comment.
Handle potential exceptions from asynchronous operations
The await clearStatistics() calls at lines 26, 83, and 132 perform asynchronous operations that might fail. It's good practice to handle potential exceptions to prevent unhandled promise rejections.
Consider wrapping the await calls in try...catch blocks:
confirm: async () => {
- await clearStatistics(element.hass, [issue.data.statistic_id]);
+ try {
+ await clearStatistics(element.hass, [issue.data.statistic_id]);
+ } catch (error) {
+ // Handle error (e.g., show an error dialog)
+ showAlertDialog(element, {
+ title: localize("ui.panel.developer-tools.tabs.statistics.fix_issue.error_title"),
+ text: localize("ui.panel.developer-tools.tabs.statistics.fix_issue.error_text", { error }),
+ });
+ }
},Also applies to: 83-83, 132-132
| await clearStatistics(element.hass, [issue.data.statistic_id]); | ||
| }, | ||
| }); | ||
| break; |
There was a problem hiding this comment.
Remove unreachable break statement after return
At line 86, the break; statement is unreachable because the function has already returned. Removing it will prevent confusion.
Apply this diff to fix the issue:
- break;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.
| break; |
Tools
Biome
[error] 86-86: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
| )}</a | ||
| >`, | ||
| }); | ||
| break; |
There was a problem hiding this comment.
Remove unreachable break statement after return
The break; statement at line 53 is unreachable due to the preceding return statement. Eliminating the break; statement will improve code clarity.
Apply this diff to fix the issue:
- break;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.
| break; |
Tools
Biome
[error] 53-53: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
| return showFixStatisticsUnitsChangedDialog(element, { | ||
| issue, | ||
| }); | ||
| break; |
There was a problem hiding this comment.
Remove unreachable break statement after return
At line 140, the break; statement is unreachable because of the preceding return. It can be safely removed to tidy up the code.
Apply this diff to fix the issue:
- break;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.
| break; |
Tools
Biome
[error] 140-140: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
| } from "../../lovelace/custom-card-helpers"; | ||
| import { showFixStatisticsUnitsChangedDialog } from "./show-dialog-statistics-fix-units-changed"; | ||
|
|
||
| export const fixStatisticsIssue = async (element, localize, issue) => { |
There was a problem hiding this comment.
Add explicit type annotations to function parameters
For improved type safety and code clarity, consider adding explicit type annotations to the fixStatisticsIssue function parameters: element, localize, and issue.
Apply this diff to add type annotations:
-export const fixStatisticsIssue = async (element, localize, issue) => {
+import { HomeAssistant } from "../../../types";
+import { LocalizeFunc } from "../../../common/translations/localize";
+import { StatisticsValidationIssue } from "../../../data/recorder";
+
+export const fixStatisticsIssue = async (
+ element: HTMLElement & { hass: HomeAssistant },
+ localize: LocalizeFunc,
+ issue: StatisticsValidationIssue
+) => {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 fixStatisticsIssue = async (element, localize, issue) => { | |
| import { HomeAssistant } from "../../../types"; | |
| import { LocalizeFunc } from "../../../common/translations/localize"; | |
| import { StatisticsValidationIssue } from "../../../data/recorder"; | |
| export const fixStatisticsIssue = async ( | |
| element: HTMLElement & { hass: HomeAssistant }, | |
| localize: LocalizeFunc, | |
| issue: StatisticsValidationIssue | |
| ) => { |
| [ | ||
| "entity_not_recorded", | ||
| "entity_no_longer_recorded", | ||
| "unsupported_state_class", | ||
| "units_changed", | ||
| ].includes(issue.translation_key) |
There was a problem hiding this comment.
Refactor translation keys into a constant
For better maintainability and readability, consider defining the array of translation keys as a constant outside the method.
Apply this refactor:
- Define the constant at the top of the file or outside the class:
const STATISTICS_ISSUE_TRANSLATION_KEYS = [
"entity_not_recorded",
"entity_no_longer_recorded",
"unsupported_state_class",
"units_changed",
];- Update the condition in the
_openShowMoreDialogmethod:
issue.translation_key &&
- [
- "entity_not_recorded",
- "entity_no_longer_recorded",
- "unsupported_state_class",
- "units_changed",
- ].includes(issue.translation_key)
+ STATISTICS_ISSUE_TRANSLATION_KEYS.includes(issue.translation_key)| } else if ( | ||
| issue.domain === "sensor" && | ||
| issue.translation_key && | ||
| [ | ||
| "entity_not_recorded", | ||
| "entity_no_longer_recorded", | ||
| "unsupported_state_class", | ||
| "units_changed", | ||
| ].includes(issue.translation_key) | ||
| ) { | ||
| const localize = | ||
| await this.hass.loadFragmentTranslation("developer-tools"); | ||
| const data = await fetchRepairsIssueData( | ||
| this.hass.connection, | ||
| issue.domain, | ||
| issue.issue_id | ||
| ); | ||
| if ("issue_type" in data.issue_data) { | ||
| await fixStatisticsIssue(this, localize, { | ||
| type: data.issue_data.issue_type, | ||
| data: data.issue_data, | ||
| }); | ||
| this.hass.callWS({ type: "recorder/update_statistics_issues" }); | ||
| } |
There was a problem hiding this comment.
Add missing await for this.hass.callWS
In line 156, the this.hass.callWS method returns a Promise. To ensure the WebSocket call completes before proceeding, you should use await when calling this.hass.callWS(...).
Apply this diff to fix the issue:
await fixStatisticsIssue(this, localize, {
type: data.issue_data.issue_type,
data: data.issue_data,
});
- this.hass.callWS({ type: "recorder/update_statistics_issues" });
+ await this.hass.callWS({ type: "recorder/update_statistics_issues" });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.
| } else if ( | |
| issue.domain === "sensor" && | |
| issue.translation_key && | |
| [ | |
| "entity_not_recorded", | |
| "entity_no_longer_recorded", | |
| "unsupported_state_class", | |
| "units_changed", | |
| ].includes(issue.translation_key) | |
| ) { | |
| const localize = | |
| await this.hass.loadFragmentTranslation("developer-tools"); | |
| const data = await fetchRepairsIssueData( | |
| this.hass.connection, | |
| issue.domain, | |
| issue.issue_id | |
| ); | |
| if ("issue_type" in data.issue_data) { | |
| await fixStatisticsIssue(this, localize, { | |
| type: data.issue_data.issue_type, | |
| data: data.issue_data, | |
| }); | |
| this.hass.callWS({ type: "recorder/update_statistics_issues" }); | |
| } | |
| } else if ( | |
| issue.domain === "sensor" && | |
| issue.translation_key && | |
| [ | |
| "entity_not_recorded", | |
| "entity_no_longer_recorded", | |
| "unsupported_state_class", | |
| "units_changed", | |
| ].includes(issue.translation_key) | |
| ) { | |
| const localize = | |
| await this.hass.loadFragmentTranslation("developer-tools"); | |
| const data = await fetchRepairsIssueData( | |
| this.hass.connection, | |
| issue.domain, | |
| issue.issue_id | |
| ); | |
| if ("issue_type" in data.issue_data) { | |
| await fixStatisticsIssue(this, localize, { | |
| type: data.issue_data.issue_type, | |
| data: data.issue_data, | |
| }); | |
| await this.hass.callWS({ type: "recorder/update_statistics_issues" }); | |
| } |
| "entity_not_recorded", | ||
| "entity_no_longer_recorded", |
There was a problem hiding this comment.
Core doesn't create issues for these two
There was a problem hiding this comment.
Doesn't harm having them here, rather have all of it 🙃
Co-authored-by: Paul Bottein <[email protected]>
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Co-authored-by: Paul Bottein <[email protected]>
Proposed change
For: home-assistant/core#122595
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
Refactor