FP-3008: IDE - Inconsistent Start and Stop behaviour #369
Conversation
quirinpa
commented
Nov 4, 2024
•
quirinpa
commented
- FP-3008: IDE - Inconsistent Start and Stop behaviour
| @@ -40,7 +40,6 @@ import { buttonStyles, flowTopBarStyles } from "./styles"; | |||
| import FlowSearch from "./FlowSearch"; | |||
|
|
|||
| const BACKEND_CALLBACK_NAME = "backend.FlowTopBar"; | |||
| const FEEDBACK_TIMEOUT = 10000; | |||
There was a problem hiding this comment.
Did you investigate why was this needed in the first place?
Are we sure that removing this won't cause any more regressions?
There was a problem hiding this comment.
It should have never been there. The FE shouldn't be putting timeouts like those.
The timeout seems to have been put there because the person didn't think that it could take more than 10 seconds. Among other things. Like the fact that kind of thing shouldn't be done.
There was a problem hiding this comment.
Do you know the feeling, when you do some work and after a few hours of work you know it's done? You never think about the other people that did work prior to you, you are concentrated on your task and you just go in and try to fix it. In your opinion, people that did code before you, just did it bad.
There's a reason someone wasted their time to add that FEEDBACK_TIMEOUT. It wasn't created out of thin air, it wasn't some idiot that hacked mov.ai and put that there out of spite or something. It was done by a movai developer that had a task, just like you, and tried to solve it.
I'm just asking if you investigated why this was needed, why that person a couple of years ago thought about adding that there, why 2 more people looked at the code and accepted that.
It would be better, if they left a comment explaining why the hell is that needed, but they didn't, so it's up to us to investigate if it's still needed or not. If not, you have my blessing, remove it please! I hate these timeouts. BUT, if it's there, there was a reason to add it. Just do your part, do the investigation and let us know why that was added and why it is no longer needed.
There was a problem hiding this comment.
So perhaps I'm the only person who can make mistakes, then.
Is it possible for you to be missing something?
I'm a developer just like you.
I removed it because we're no longer using a timeout and instead wait for the server to give one.
I'm not perfect, sure. People have some reasons to think stuff. Sure. But I can understand something that the person before didn't, can't I? I'm pretty sure I also have the potential of reason. Not only others.
So let's have this go both ways. You consider my potential for reason. And I consider yours.
You see, I also employ my time coding. And especially, trying to improve my own code. I don't get it right the first time, unlike many others, apparently.
Whatever logic truly exists, it should be applied independently of personal bias. If I have the potential to be an idiot, then so does the last guy. If I have the potential to be clever, so does the last guy. Let's not lose ourselves in pointing fingers and dismissing each-other while doing the same thing we are accusing the other person of.. Let's just look at the particular situation and be clear about it:
Timeouts should be handled by the server, not the client.
There was a problem hiding this comment.
It should have never been there. The FE shouldn't be putting timeouts like those.
The timeout seems to have been put there because the person didn't think that it could take more than 10 seconds. Among other things. Like the fact that kind of thing shouldn't be done.
Is there any resource or tests done that this won't cause any type of regressions?
There was a problem hiding this comment.
Stop talking about yourself. I just want to find out why it was added in the first place. Manuel built this application, or most of it, and he added that timeout there for a reason, perhaps it was because the backend was slower or the application didn't work fine at that time. I just asked if it was safe to remove that. Nothing else. I don't care if I have potencial or not, or whatever you're rambling about. I asked a question, you're supposed to dig in and understand if there might be something breaking if that Timeout is removed or not, and tell me: "It really isn't needed anymore because of X reason", or "Good thing you asked, after digging a bit I found out why it was added in the first place and we should still use it because of X or Y reason."
This is all I'm asking.
There was a problem hiding this comment.
Ok, Then.
It was added because the developer then seemed to think it was sufficient to wait for a fixed timeout on the FE side to find out whether or not the flow had started or stopped.
It's not.
There was a problem hiding this comment.
I don't believe so, Nadia. But it does work.
|
There was a problem hiding this comment.
This might create a regression later on, where the button is active saying the robot started, but it actually didn't start. @quirinpa's code is just assuming it will never fail.
The previous code was waiting for the correct robot status robotStatus.activeFlow to actually make the button show ON. If the robot starting the flow failed for any reason it would send a message "FailedFlowAction" and wouldn't turn the button ON.
The current code, makes the button ON immediately, even if it fails.
I can't do any more of these battles, so, @diasnad @EdwardAngeles @diasdm just approve if you feel confortable with this.
By the way, I'm not saying the old code was good, it was dependent on a Timeout, so it definitely WASN'T good. But the new one is scary from my point of view.
|
My code does not assume it never fails. Only assumes the BE takes the correct action when and if it does. |
|
This pr will bring more issues than solutions |
