-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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: Phishing page metrics #28364
base: develop
Are you sure you want to change the base?
fix: Phishing page metrics #28364
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
I have read the CLA Document and I hereby sign the CLA |
Builds ready [e634bfc]
Page Load Metrics (2184 ± 85 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
const blockedUrl = | ||
blockReason === 'c2DomainBlocklist' ? details.initiator : hostname; | ||
|
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.
So there's a couple of things here:
- We should try to avoid having inline strings spread out, as it will make it much harder to update the blockReason value in the future (we would need to update the controller, and then remember to search for the actual string value in the Extension codebase). Here we could use the type from the PhishingController (if it is not exported, we can export it).
- we already have an if block above where we set a specific blockReason. And here we are adding another if clause for the blockReason to set the blockedUrl. We should collapse into the pre-existing if block that sets the blockReason and set the blockUrl as well.
Suggestion, update the above if block as below:
let blockReason;
let blockedUrl = hostname;
if (phishingTestResponse?.result && blockedRequestResponse.result) {
blockReason = `${phishingTestResponse.type} and ${blockedRequestResponse.type}`;
} else if (phishingTestResponse?.result) {
blockReason = phishingTestResponse.type;
} else {
blockReason = blockedRequestResponse.type;
blockedUrl = details.initiator;
}
Let me know your thoughts?
Description
Related issues
Fixes: https://github.com/orgs/MetaMask/projects/103/views/1?pane=issue&itemId=86413189
Manual testing steps
See Issue attached in Related Issues.
Pre-merge author checklist
Pre-merge reviewer checklist