Display an error if saving new automation times out #23518
Conversation
| ), | ||
| warning: true, | ||
| }); | ||
| }, 5000); | ||
| const automation = await entityRegPromise; |
There was a problem hiding this comment.
Can we instead use the promiseTimeout helper?
| const automation = await entityRegPromise; | |
| try { | |
| const automation = await promiseTimeout(5000, entityRegPromise); | |
| } catch (e) { | |
| showAlertDialog(); | |
| } |
There was a problem hiding this comment.
I almost used this, but I didn't discover clean way to detect if the error caught was from the timeout or from the entityRegPromise. If it's not a timeout, I want to fall through to the regular error handling that already existed.
I could catch the error and see if the string contained the words "Timed out" but seemed a bit messy.
There was a problem hiding this comment.
Agree, we should make the reject an instanceof Error instead of just a string, that we can give a custom name:
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(new TimeoutError(ms));
}, ms);
});
// Returns a race between our timeout and the passed in promise
return Promise.race([promise, timeout]);
};Then the error handling would be:
try {
const automation = await promiseTimeout(5000, entityRegPromise);
} catch (e) {
if (e instanceof Error && e.name === "TimeoutError") {
showAlertDialog();
} else {
throw e;
}
}
| ), | ||
| warning: true, | ||
| }); | ||
| }, 5000); |
There was a problem hiding this comment.
5 seconds seems long, I think we make this fail quicker?
There was a problem hiding this comment.
Have a favorite number? I would like to be careful not to fire prematurely for someone just on a slow connection, but I don't know what is standard for such things.
There was a problem hiding this comment.
Yeah, I'm mainly worried because we don't show any progress indicator or what so ever, so if I was looking at this screen for 5 seconds I would be whats happening?! In general the rule of thumb is that if it takes longer than a second we should show a progress indicator.
I would normally say this action should be done within a second, but let's set it at 2 seconds?
|
updated |
| this._readOnly = true; | ||
| this._saveFailed = true; |
There was a problem hiding this comment.
Shouldn't we instead of this, just navigate to the new automation and let that handle it?
navigate(`/config/automation/edit/${id}`, { replace: true });
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Just saw this merged in Lokalise. The way these two sentences are created with "automation" or "script" as variables does not work in languages that gender. In German it's "die Automatisierung" (f.), but "das Skript" (n.). So "Your new …" becomes "Deine neue Automatisierung …" and "Dein neues Skript …". I could put this into the type_automation and type_script keys, but then the other three occurrences will break. |
Proposed change
If user tries to create a new automation in the visual editor when there is an unrelated error in the configuration.yaml file, when you try to save that automation the editor will appear to just break.
After clicking
Savethe button will become disabled (grey color), you cannot make any further edits, you cannot leave the automation without going through the "lose your unsaved changes" dialog, and your new automation will appear to have vanished (its not in the automation list).The automation is actually successfully saved, but it never appears in the entity registry due to configuration parsing error, so our save flow never actually completes.
This PR adds a warning dialog if a user gets in this situation, otherwise it is very confusing what is happening.
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: