Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display an error if saving new automation times out #23518

Merged
merged 3 commits into from
Dec 31, 2024
Merged

Display an error if saving new automation times out #23518

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3b84e49
Display an error if saving new automation times out
karwosts Dec 31, 2024
81bd127
changes
karwosts Dec 31, 2024
c3a4d91
update
karwosts Dec 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion src/common/util/promise-timeout.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,25 @@
class TimeoutError extends Error {
public timeout: number;

constructor(timeout: number, ...params) {
super(...params);

// Maintains proper stack trace for where our error was thrown (only available on V8)
if (Error.captureStackTrace) {
Error.captureStackTrace(this, TimeoutError);
}

this.name = "TimeoutError";
// Custom debugging information
this.timeout = timeout;
this.message = `Timed out in ${timeout} ms.`;
}
}

export const promiseTimeout = (ms: number, promise: Promise<any> | any) => {
const timeout = new Promise((_resolve, reject) => {
setTimeout(() => {
reject(`Timed out in ${ms} ms.`);
reject(new TimeoutError(ms));
}, ms);
});

Expand Down
45 changes: 40 additions & 5 deletions src/panels/config/automation/ha-automation-editor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import { fireEvent } from "../../../common/dom/fire_event";
import { navigate } from "../../../common/navigate";
import { computeRTL } from "../../../common/util/compute_rtl";
import { afterNextRender } from "../../../common/util/render-status";
import { promiseTimeout } from "../../../common/util/promise-timeout";
import "../../../components/ha-button-menu";
import "../../../components/ha-fab";
import "../../../components/ha-icon";
Expand Down Expand Up @@ -142,6 +143,8 @@ export class HaAutomationEditor extends PreventUnsavedMixin(

@state() private _saving = false;

@state() private _saveFailed = false;

@state()
@consume({ context: fullEntitiesContext, subscribe: true })
_entityRegistry!: EntityRegistryEntry[];
Expand Down Expand Up @@ -307,12 +310,13 @@ export class HaAutomationEditor extends PreventUnsavedMixin(

<ha-list-item
.disabled=${this._blueprintConfig ||
this._saveFailed ||
(!this._readOnly && !this.automationId)}
graphic="icon"
@click=${this._duplicate}
>
${this.hass.localize(
this._readOnly
this._readOnly && !this._saveFailed
? "ui.panel.config.automation.editor.migrate"
: "ui.panel.config.automation.editor.duplicate"
)}
Expand Down Expand Up @@ -423,7 +427,7 @@ export class HaAutomationEditor extends PreventUnsavedMixin(
>
</div>
</ha-alert>`
: this._readOnly
: this._readOnly && !this._saveFailed
? html`<ha-alert alert-type="warning" dismissable
>${this.hass.localize(
"ui.panel.config.automation.editor.read_only"
Expand Down Expand Up @@ -496,7 +500,9 @@ export class HaAutomationEditor extends PreventUnsavedMixin(
class=${classMap({
dirty: !this._readOnly && this._dirty,
})}
.label=${this.hass.localize("ui.panel.config.automation.editor.save")}
.label=${this._saving
? this.hass.localize("ui.panel.config.automation.editor.saving")
: this.hass.localize("ui.panel.config.automation.editor.save")}
.disabled=${this._saving}
extended
@click=${this._saveAutomation}
Expand Down Expand Up @@ -944,8 +950,37 @@ export class HaAutomationEditor extends PreventUnsavedMixin(

// wait for automation to appear in entity registry when creating a new automation
if (entityRegPromise) {
const automation = await entityRegPromise;
entityId = automation.entity_id;
try {
const automation = await promiseTimeout(2000, entityRegPromise);
entityId = automation.entity_id;
} catch (e) {
if (e instanceof Error && e.name === "TimeoutError") {
this._readOnly = true;
this._saveFailed = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we instead of this, just navigate to the new automation and let that handle it?

        navigate(`/config/automation/edit/${id}`, { replace: true });

Copy link
Contributor Author

@karwosts karwosts Dec 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we let user navigate through to the other side, what should the behavior be? Just allow continue editing as normal? I guess editing can continue but any registry based settings will be broken.

Was kind of thinking best to just kick them out ASAP and go fix the config before resuming.

Copy link
Member

@bramkragten bramkragten Dec 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still show the alert message, but then just navigate?

It will be the same when they refresh the page or go back and visit any other automation right?
We should handle no entity registry cases correctly anyhow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I show the dialog and do nothing else but continue, the editor can continue, it will have no entity id, so most of the overflow menu items are disabled. If you rename the automation and again select an area it will show the dialog again.

If that sounds good I'll go for that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

this._dirty = false;
showAlertDialog(this, {
title: this.hass.localize(
"ui.panel.config.automation.editor.new_automation_setup_failed_title",
{
type: this.hass.localize(
"ui.panel.config.automation.editor.type_automation"
),
}
),
text: this.hass.localize(
"ui.panel.config.automation.editor.new_automation_setup_failed_text",
{
type: this.hass.localize(
"ui.panel.config.automation.editor.type_automation"
),
}
),
warning: true,
});
return;
}
throw e;
}
}

if (entityId) {
Expand Down
5 changes: 5 additions & 0 deletions src/translations/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3021,6 +3021,7 @@
"load_error_not_deletable": "Only automations in automations.yaml can be deleted.",
"load_error_unknown": "Error loading automation ({err_no}).",
"save": "Save",
"saving": "Saving",
"unsaved_confirm_title": "Leave editor?",
"unsaved_confirm_text": "Unsaved changes will be lost.",
"alias": "Name",
Expand Down Expand Up @@ -3064,6 +3065,10 @@
"unknown_entity": "unknown entity",
"edit_unknown_device": "Editor not available for unknown device",
"switch_ui_yaml_error": "There are currently YAML errors in the automation, and it cannot be parsed. Switching to UI mode may cause pending changes to be lost. Press cancel to correct any errors before proceeding to prevent loss of pending changes, or continue if you are sure.",
"type_automation": "automation",
"type_script": "script",
"new_automation_setup_failed_title": "New {type} setup failed",
"new_automation_setup_failed_text": "Your new {type} has saved, but waiting for it to setup has timed out. This could be due to errors parsing your configuration.yaml, please check the configuration in developer tools. Your {type} will not be visible or editable until this is corrected, and automations are reloaded.",
"triggers": {
"name": "Triggers",
"header": "When",
Expand Down
Loading