#2070 replace Alert() with the in-app prompt dialog
#2091
Conversation
Passing run #2487 ↗︎
Details:
Review all test suite changes for PR #2091 ↗︎ |
|||||||||||||||
There was a problem hiding this comment.
The report the error link tag currently doesn't navigate to Application log page and goes to the /dashboard instead. This can be investigated/worked on separately from this issue later on.
Why not fix it in this PR? FYI, similar problems relating to that link were fixed in #2005 that was just merged
I've tried it again after merging this but it doesn't fix the issue either.
I actually already spent a few hours investigating this problem and the root cause is that, this block of navigation guard logic triggers before this block of silent login in I couldn't come up with the best solution for it straight away and thought it deserves some more time alone. it's a typical little code updates but with longer effort kind of issue I think. |
|
@taoeffect Just updated this PR for the fix of #2096 too 
|
|
@taoeffect some comment re above : |
There was a problem hiding this comment.
Overall, I think this looks fine and is an improvement over alert(). There are two relatively small issues that I think should be addressed. I'm marking this as approved because I don't think either of the things I pointed out are blockers, but you should consider addressing my points.
frontend/model/contracts/group.js
Outdated
| primaryButton: L('Close') | ||
| } | ||
|
|
||
| await sbp('gi.ui/prompt', promptOptions) |
There was a problem hiding this comment.
If you look at how it was previously, it was written in a non-blocking way, which I believe is no longer the case. One reason not to await here is that now you've made an error handler that could potentially throw. Ideally, error handlers should not result in an error.
I'd make it non-blocking again because ideally contracts should not have code that depends on user interation, even when it doesn't block message processing. In this case, I think you can resolve it by removing the await and adding a .catch() handler (this is because not using a catch handler would result in an unhandled promise rejection, if the promise throws).
There was a problem hiding this comment.
@corrideat fair point. I think we can just remove async / await here, because there is no required action followed by clicking on that 'Close' button on the prompt.
| console.error('Error during contract sync upon login (syncing all contractIDs)', err) | ||
|
|
||
| const humanErr = L('Sync error during login: {msg}', { msg: err?.message || 'unknown error' }) | ||
| throw new GIErrorUIRuntimeError(humanErr) |
There was a problem hiding this comment.
In cases where you throw, you should use the cause option including the original error (see example below). This allows error handlers access to the original error, which can be helpful for error handling and debugging.
throw new GIErrorUIRuntimeError(humanErr, { cause: err })
frontend/controller/actions/group.js
Outdated
| const primaryClicked = await sbp('gi.ui/prompt', { | ||
| heading: L('Failed to join the group'), | ||
| question: L('Error details:{br_}{err}', { err: e.message, ...LTags() }), | ||
| primaryButton: L('Refresh') | ||
| }) | ||
|
|
||
| if (primaryClicked) { | ||
| const url = (window.location.href).split('?')[0] | ||
| window.open(url, '_self') | ||
| } |
There was a problem hiding this comment.
In this PR, I think it's best to preserve the behavior of master as much as we can, as it will save both of us headaches on having to think through the consequences of changing the behavior.
Before there was no call to refresh the page, and no there shouldn't be either. It's good to change the behavior to have it show the error, but let's limit it to that for now unless there's a strong reason to do more.
There was a problem hiding this comment.
@taoeffect Will do so. I added the page refresh logic here as it was part of the suggested solution of the issue #2096, BTW.
There was a problem hiding this comment.
Hmm, but isn't #2096 about updating the PendingApproval page? 🤔
There was a problem hiding this comment.
@taoeffect
Yes, I started off looking at the PendingApproval.vue but then realised /pending-approval page is merely the place redirected to as a follow-up step of an actual async action where the error is thrown, meaning PendingApproval.vue cannot catch that error.
For example,
await sbp('gi.actions/group/joinWithInviteSecret') here leads to a chain of several sbp calls which involves sbp('gi.actions/group/join') (here in identity.js contract) down the line. that's actually where the target error mostly occurs. So I decided to add the prompt logic there instead.
So even after we land /pending-approval page, there is still a chain of async operations (which started before the page navigation) going on behind the scene. so the error thrown as part of that looks like it occurred within the pending approval page. This is what it appeared from my investigation.
|
@taoeffect |
work on #2070
closes #2096
Below are the screenshots of some places where I repalced the
alert()instances.@taoeffect
The
report the errorlink tag currently doesn't navigate toApplication logpage and goes to the/dashboardinstead. This can be investigated/worked on separately from this issue later on.