Show an error message if resetting identity takes too long - #34715
Show an error message if resetting identity takes too long#34715andybalaam wants to merge 8 commits into
Conversation
52bedd5 to
df393c5
Compare
df393c5 to
94ff26c
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
806029e to
94bfeb6
Compare
|
This is now ready for review again, after some github-down-faffing. |
richvdh
left a comment
richvdh
left a comment
There was a problem hiding this comment.
LGTM. Some low-importance comments.
| // clicked again, and warn the user not to close the window. | ||
| const [inProgress, setInProgress] = useState(false); | ||
|
|
||
| async function onClick(): Promise<void> { |
There was a problem hiding this comment.
shouldn't this and doOnClick have a useCallback ?
| await act(async () => panel.getByRole("button", { name: "Continue" }).click()); | ||
|
|
||
| // And wait more than the timeout | ||
| await sleep(10); |
There was a problem hiding this comment.
rather than sleeping in a test, and threading a timeout value through 10 layers of components, could we use fake timers?
| const client = MatrixClientPeg.safeGet(); | ||
|
|
||
| // Given reset will freeze forever when we do it | ||
| client.getCrypto()!.resetEncryption = vi.fn().mockImplementation(() => sleep(20000)); |
There was a problem hiding this comment.
"A promise that never resolves" can also be spelt new Promise(() => {}), which might be better than having a dangling timeout?
|
So 5s seems not a very long time for someone on a poor connection, additionally the error makes it sound guaranteed that it failed, which might not be true given the flow doesn't actually get cancelled - so it may continue running in the background after the error? Also looking at the doc comment for |
|
We had a conversation within the Crypto team about whether this is actually better than just freezing, since the freezing does demonstrate to the user that something went wrong. If I add on doubts expressed by t3chguy, I think I will abandon this and leave it as it is. The underlying bug is fixed by matrix-org/matrix-js-sdk#5485 |
3 participants
Exposes the error that is happening in #34642 but doesn't fix it:
If we take longer than 5 seconds to reset identity, we show this:
and log this:
... which is better than what we did before, which was to freeze forever on the "resetting digital identity" screen.
Checklist
public/exportedsymbols have accurate TSDoc documentation.