Conversation
maparr
changed the title
maparr
force-pushed
the
fix/1457
branch
3 times, most recently
from
813165c to
b32ea3c
Compare
monikasmolarek
left a comment
monikasmolarek
left a comment
There was a problem hiding this comment.
Please note that I have not yet tested the functionality, please assess whether or not we want to check proactively the node's and network's state to inform the users more about the consequences (lost a few layers, the whole epoch, etc.)
| <Message> | ||
| <p> | ||
| Restarting now is <b>CRITICAL</b> and may impact your node’s | ||
| performance and rewards. | ||
| </p> | ||
| <ul> | ||
| <li> | ||
| Click <b style={{ color: smColors.green }}>RESTART NOW</b> to apply | ||
| the update immediately. Delaying the update could result in | ||
| potential loss of rewards. | ||
| </li> | ||
| <li> | ||
| Click <b style={{ color: smColors.purple }}>POSTPONE</b> to delay | ||
| the update. Be aware that postponing may slow down your node’s | ||
| performance and future rewards. | ||
| </li> | ||
| </ul> | ||
| </Message> |
There was a problem hiding this comment.
This seems to be incorrect. @brusherru please correct me if I'm wrong.
-
The loss of the rewards might be caused by restarting (if the node is supposed to publish a proposal at this particular time) and not caused by the fact that the user updates immediately or a couple of hours later.
-
Postponing the restart(and upgrade) should not have such a big impact on the node's performance, not to the extent that the user should risk losing rewards to update immediately. If there are any breaking changes in the code affecting significantly the node's performance and network compatibility (and thus missed rewards) then the forced update is supposed to be used (so no choice for the user anyway, no need to display any modal).
From the grammatical perspective, it's confusing and quite misleading in several aspects.
Additionally, smapp "knows" the current layer and the number of layers for which the node has rewards eligibility. So as Lane suggested, it would be great to display it only when applicable. There might be users not even smeshing or still generating the POS data. If they see the missed rewards warning they will get lost and scared.
But, we need to also remember about not restarting while publishing ATX and preparing the proof...
If it's too complex and time-consuming for this version, then my suggestion would be:
| <Message> | |
| <p> | |
| Restarting now is <b>CRITICAL</b> and may impact your node’s | |
| performance and rewards. | |
| </p> | |
| <ul> | |
| <li> | |
| Click <b style={{ color: smColors.green }}>RESTART NOW</b> to apply | |
| the update immediately. Delaying the update could result in | |
| potential loss of rewards. | |
| </li> | |
| <li> | |
| Click <b style={{ color: smColors.purple }}>POSTPONE</b> to delay | |
| the update. Be aware that postponing may slow down your node’s | |
| performance and future rewards. | |
| </li> | |
| </ul> | |
| </Message> | |
| <Message> | |
| <p> | |
| To finalize the update, Smapp must be restarted. However, verify your eligibility for upcoming layer rewards. Powering off the node risks missing proposal submissions and losing the rewards. | |
| </p> | |
| <ul> | |
| <li> | |
| Click <b style={{ color: smColors.green }}>RESTART NOW</b> to apply | |
| the update immediately. | |
| </li> | |
| <li> | |
| Click <b style={{ color: smColors.purple }}>POSTPONE</b> to delay | |
| the update. The persistent "restart" button will remind you about the pending update. | |
| </li> | |
| </ul> | |
| </Message> |
Please note that I have not yet tested the functionality, please assess whether or not we want to check proactively the node's and network's state to inform the users more about the consequences (lost a few layers, the whole epoch, etc.)
|
If this adds more clutter to the application. I removed the modal window. In the PR, I removed the dialog prompt to close the application. And let’s partially solve the problem in the task. If possible, the details described in the task and in the PR will remain. Which, if possible, will be added. |
|
Ok so if it's only about removing the modal for the updates, then it's ok, it works as expected. |
brusherru
left a comment
brusherru
left a comment
There was a problem hiding this comment.
The message proposed by Monika is perfect and it will be good to have it (as described in the issue).
However, I'd like to keep it simple:
- Avoid making the Version component stateful
- Avoid having
$isUpdateInProgressin so many places, just for not showing the prompt (are there any problems with canceling installation by choosing the "Cancel" option in the quit prompt?) - I'd like to keep using the system prompt (as it is already implemented)
So I think it can be simplified to calling managers.smeshing.isSmeshing and then displaying a standard prompt or a special one. It will also leave us a space to easily (without a lot of refactoring) add any special conditions.
And managers are already passed into promptBeforeClose.
|
Okay, I can finally summarize my understanding: I'm currently unable to simply use the system modal and manage it effortlessly (I don't see the option right now). I'm looking into how the install and restart function operates:
As observed, the process first triggers the closure of all windows and then the I'm also examining how this exit process functions on different operating systems:
By this result, we can show the system modal, but it will be more difficult than the client solution. Furthermore, I don’t see a straightforward way to simplify this in the current code base. Other companies/repositories either move all modals to the FE or trigger the result as a global variable, which I don't prefer. However, I'm open to exploring some creative solutions. |
3 participants
Partially closing #1457.
How to test:
Windows( we have vm, ask me if needed ), follow the guideyarn package-win