This differential introduces new screen that will carry downloading backup message, signing it and using for restoration. AS of this differential backup support for SIWE will just match what we currently have for password users - backup via testing buttons. However most of the code in this stack will be reused to implement full backup-restore feature.
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-7345
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
You should brush up on React basics, specifically around memoization in hooks and function components. We should always memoize non-primitives
| native/account/registration/siwe-backup-message-creation.react.js | ||
|---|---|---|
| 229–232 ↗ | (On Diff #40171) | This needs to be wrapped in React.useMemo |
| 244–245 ↗ | (On Diff #40171) | Let's use the replacement I introduced in D11993:
|
| 257–261 ↗ | (On Diff #40171) | I'm not sure this button makes sense in the final design. It feels aggressive... "Discard" implies that it will be lost, but really we are just "going back" here and ending the recovery attempt, right? I think in the final design, it would be best to remove this button, and just have the user navigate back to the main LoggedOutModal if they want to go back. What do you think? Is there an alternative approach we can use for this screen in the interim? If you really need to keep this button, we should at least change the text here. |
| native/backup/restore-siwe-backup.react.js | ||
| 58–60 ↗ | (On Diff #40171) |
|
| native/backup/use-client-backup.js | ||
| 55–59 ↗ | (On Diff #40171) | Why is this necessary? Do we unset it later? |
| native/account/registration/siwe-backup-message-creation.react.js | ||
|---|---|---|
| 257–261 ↗ | (On Diff #40171) | The reason I introduced this button is the same why we introduced Skip button in screen that prompts for siwe backup message creation after SIWE log in - SIWE can break and we can't keep user stuck if it happens. This screen is subject to change in final design that I will work on during June - I will remember to apply you suggestion about simply going back to LoggedOutModal. For now I will keep this button but rename to to Skip. |
| native/backup/restore-siwe-backup.react.js | ||
| 58–60 ↗ | (On Diff #40171) | Mistake by accident. |
| native/backup/use-client-backup.js | ||
| 55–59 ↗ | (On Diff #40171) |
Currently our services don't check those values but I am almost certain that the request will be rejected if there is nothing there. I will verify it and report shortly.
No it is not unset later - is that a concern? Note: that this line will be deleted after identity launch when devices will be getting real instances of this. |
Comment Actions
Accepting, but would be good if @kamil took a look as well, as he is more familiar with the backup testing code
| native/backup/use-client-backup.js | ||
|---|---|---|
| 58–62 | I guess I'm confused why this is necessary. Is it only for testing with !usingCommServicesAccessToken? It's my impression that after D11021, the CSAM should always be set when the user is logged in (assuming usingCommServicesAccessToken) | |
| native/backup/use-client-backup.js | ||
|---|---|---|
| 58–62 | It was introduced before usingCommServicesAccessToken - I think we can now remove it | |
| native/backup/use-client-backup.js | ||
|---|---|---|
| 58–62 | Basically @kamil is right - if someone is testing with staging services they should flip usingCommServicesAccessToken flag and it is not necessary to use this function. However this won't work for production and I think it is good to be able to use backup buttons on production if isStaffRelease is true. I've recently discovered some bugs because I was able to use backup buttons on production. After identity launch on production we can remove this function. | |