Improve Error Handling Logic in Enter Seed Phrase Screen #22252
Conversation
Jenkins BuildsClick to see older builds (31)
|
|
hey @Parveshdhull im not sure if it relevant but in create profile (keycard) flow there is a button also i think this check should be conditional, in some flows we expect phrase from profile on device |
|
Thank you very much @flexsurfer for checking the PR.
Thank you, I will make it conditional |
|
thank you . this is the issue #22013 |
Parveshdhull
force-pushed
the
fix/recovery-phrase-error
branch
from
6c9dcee to
da58d9a
Compare
Parveshdhull
changed the title
Parveshdhull
force-pushed
the
fix/recovery-phrase-error
branch
3 times, most recently
from
0ac6316 to
dd5d37d
Compare
Parveshdhull
force-pushed
the
fix/recovery-phrase-error
branch
from
dd5d37d to
e0c0ae6
Compare
|
@flexsurfer @ilmotta PR is ready for review. |
71% of end-end tests have passedFailed tests (4)Click to expandClass TestWalletCollectibles:
Class TestWalletOneDevice:
Passed tests (10)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
|
| @@ -227,10 +230,12 @@ | |||
|
|
|||
| (defn view | |||
| [] | |||
| (let [{:keys [on-success]} (rf/sub [:get-screen-params])] | |||
| (let [{:keys [on-success on-unmount]} (rf/sub [:get-screen-params])] | |||
There was a problem hiding this comment.
This looks like a smell @Parveshdhull. Fundamentally we shouldn't couple the event and subscription layers with the components lifecycles directly. The subs are a bit more tied, but not really, because re-frame does the heavy lifting based on reactions.
I don't know if what I'm going to suggest is a good fit, but sometimes, instead of passing opaque callbacks all around, we can pass open/transparent data, which the consumer can contextualize and decide what to do. The tradeoff of this choice might be that the code may not be as general (since a callback can do absolutely anything) but the code can end up being easier to inspect/debug.
What are your thoughts on this pattern @ulisesmac @clauxx? Do you see a better way?
There was a problem hiding this comment.
Hi, Are you suggesting that, instead of passing on-unmount, I pass the onboarding-flow? parameter and then call (rf/dispatch [:onboarding/clear-navigated-to-enter-seed-phrase-from-screen]) on on-unmount when it is true?
There was a problem hiding this comment.
Or alternatively, I can remove cleanup call on-unmount and cleanup this state while opening this screen?
There was a problem hiding this comment.
Another solution, instead of passing on-unmount or onboarding-flow?, is to create an additional wrapper view specifically for onboarding. This approach aligns with the DX task - https://www.notion.so/Explicit-is-better-than-implicit-1a78f96fb65c80e4967bfb43f2e09280
There was a problem hiding this comment.
@ilmotta I think your suggestion is great, and also what @Parveshdhull said about passing onboarding-flow? as param solves it.
I also don't like to pass callbacks because of their opaqueness, although sometimes it's needed.
There was a problem hiding this comment.
@Parveshdhull your suggestion to cleanup on mount if it works is usually my preferred choice of cleanup strategy because it's guaranteed any state would converge to the correct state at all times. Cleaning up on exit points tends to be fragile.
As @ulisesmac said above, I also prefer a more data-driven approach or what you suggest with a wrapper.
Another solution, instead of passing on-unmount or onboarding-flow?, is to create an additional wrapper view specifically for onboarding.
This can work well too. If I understood the Notion reference, basically general parts should receive general inputs and the conditional logic should be handled by a more concrete parent/wrapper function or component. If that's the case, makes sense 👍🏼
| @@ -149,21 +139,34 @@ | |||
| error-in-words? (or (not last-partial-word-valid?) | |||
| (not butlast-words-valid?)) | |||
| upper-case? (boolean (re-find #"[A-Z]" @seed-phrase)) | |||
| error (rf/sub [:enter-seed-phrase/error]) | |||
There was a problem hiding this comment.
What is the rationale to prefer the state in the app-db instead of a local state? The local state wouldn't need to cater for unmounting situations. If the error is mostly limited to this component, and also prop drilling wouldn't be an issue, a local state can be better.
There was a problem hiding this comment.
Hi @ilmotta,
Once the seed phrase is validated, we dispatch the on-success function. This function further validates based on the flow. For example:
- In the account creation flow, the key-uid of the entered seed phrase should not be present in the profiles.
- In the backup keycard flow, the key-uid of the entered seed phrase should match the key-uid of the current profile.
Since the validations are happening outside of this screen, it seems simpler to store the error state in the app-db. I could alternatively pass the set-error function, but we’re not supposed to call functions from events. Therefore, I need to create an effect and pass this function to that effect.
There was a problem hiding this comment.
Since the validations are happening outside of this screen,
By the way, I also tried moving the flow-based validation into the screen itself, but it seemed more noisy to me (passing unnecessary data based on the flow, if-else statements, etc.). This approach seems cleaner to me.
There was a problem hiding this comment.
UPD: Renamed error to global-error
There was a problem hiding this comment.
If you tried and you prefer this way I trust your decision! Thanks @Parveshdhull
|
Hey @Parveshdhull! Thanks for the PR. I see there are couple unresolved review comments above. Should I wait for approval or PR is ready for testing? |
|
Hi @pavloburykh, Thank you very much for taking the PR for testing
Yes, lets wait for @ilmotta's approval. |
Sounds good @Parveshdhull Meanwhile, I have a questions and small UI issues: QUEStTION:According to designs in case of validation error instead of Actual result:
Expected result: 
|
ISSUE 1 Validation error has wrong padding in some placesActual result: Android 12, Samsung Galaxy A 52
IOS 17.6.1, iPhone 15
|
|
Thank you @pavloburykh for reporting these issues. Should be fixed now. I also updated the validation error message for the keycard migration flow. It was similar to the backup flow, but not exactly the same. UPD: Oops I forget to change on-press for "Enter another recovery phrase" (fixed) |
|
Thank you @Parveshdhull! Both issues are fixed. Same on IOS and Android.
|
|
Thank you @pavloburykh should be fixed now.
It's weird, I don't see the blurred background (with or without cutoff) on my Android & Emulator 🤔
|
There was a problem hiding this comment.
Thank you for engaging in my review feedback @Parveshdhull. Code looks good 💯
fixes #22013
fixes issue 6 in #21970
Originally reported here - #21947 (review)
Video:
signal-2025-03-06-181020.mp4
Affected Area:
All screens that take seed phrase as input
status: ready