-
Notifications
You must be signed in to change notification settings - Fork 5k
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
fix: Don't show third party notice for preinstalled Snaps #27319
fix: Don't show third party notice for preinstalled Snaps #27319
Conversation
Quality Gate failedFailed conditions |
Builds ready [78ee5b7]
Page Load Metrics (1505 ± 57 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
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.
Maybe this would benefit from a test-case or two?
It certainly would, but I'm trying to get it cherry-picked ASAP and the entire component is untested 😞 |
getSnapMetadata(state, snapId), | ||
); | ||
|
||
const isPreinstalled = Object.keys(preinstalledSnaps).includes(snapId); |
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.
Seems like this would make us skip the third party notice if multiple Snaps are installed where one is preinstalled? That seems wrong to me 🤔
Description
We show a third party notice when installing or connecting to a Snap for the first time. Preinstalled Snaps are first party Snaps however, so we don't need to show the warning for these Snaps. I've implemented some logic that checks if the requested Snap is a preinstalled Snap, and only show the third party notice if it's not.
Related issues
Fixes: MetaMask/MetaMask-planning#3325.
Manual testing steps
Assuming you have not accepted the third party notice before:
Pre-merge author checklist
Pre-merge reviewer checklist