-
Notifications
You must be signed in to change notification settings - Fork 637
feat!: Ensure user has onboarded before allowing usage of SnapController #3731
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
7876e2a to
a526ba5
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3731 +/- ##
=======================================
Coverage 98.28% 98.28%
=======================================
Files 418 418
Lines 12163 12177 +14
Branches 1881 1886 +5
=======================================
+ Hits 11954 11968 +14
Misses 209 209 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| expect(result).toStrictEqual({ [MOCK_LOCAL_SNAP_ID]: truncatedSnap }); | ||
|
|
||
| expect(messenger.call).toHaveBeenCalledTimes(12); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These seem to be changes in timing for when lifecycle hooks are called, due to the added async code we don't get as long in processing the side-effect before we reach the assertion.
| snapController.removeSnap(snap.id), | ||
| promise, | ||
| ]); | ||
| const removeSnap = async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a hack, but unsure how else to reproduce the race condition now
| * | ||
| * @returns A promise that resolves when onboarding is complete. | ||
| */ | ||
| ensureOnboardingComplete: () => Promise<void>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not get this from the onboarding controller state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately onboarding controller only exists on extension. On mobile the onboarding state is stored in Redux only, so we'll have to come up with a way to monitor that.
Adds a new required constructor argument
ensureOnboardingComplete. This should be a promise that resolves once the user has been onboarded. We can use this to ensure that a user has fully onboarded and decided whether to opt-out of Snaps before using Snaps at all. The function is used as part ofassertCanUsePlatform.Note
Require onboarding completion via new
ensureOnboardingCompletehook and asynchronously gate Snap operations (install, start, handle requests, registry updates) until onboarding finishes.ensureOnboardingComplete(): Promise<void>and store internally.#assertCanUsePlatformnow awaits onboarding completion, then validates feature flags.updateRegistry,startSnap,installSnaps,#updateSnap, andhandleRequest.ensureOnboardingCompleteby default.Written by Cursor Bugbot for commit dd19141. This will update automatically on new commits. Configure here.