-
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: failing push notifications #25340
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. |
const notificationData = { | ||
...data, | ||
type: data?.type ?? data?.data?.kind, | ||
} as NotificationUnion; |
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.
God more bad types we need to resolve.
A raw notification DOES NOT have a type
property, this is something we inject before processing. This adds back this type property if it is missing.
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.
Most of our types are sound. These are edge-cases and artefacts from the portfolio integration.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #25340 +/- ##
===========================================
- Coverage 65.37% 65.37% -0.00%
===========================================
Files 1377 1378 +1
Lines 54623 54630 +7
Branches 14320 14321 +1
===========================================
+ Hits 35709 35712 +3
- Misses 18914 18918 +4 ☔ View full report in Codecov by Sentry. |
import browser from 'webextension-polyfill'; | ||
|
||
export async function getNotificationImage() { | ||
const iconUrl = await browser.runtime.getURL('../../images/icon-64.png'); |
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 correctly gets the browser image (used on other notifications - e.g. transaction notifications)
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.
Well done!
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.
LGTM
Builds ready [6db5bfc]
Page Load Metrics (142 ± 196 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
This fixes push notifications so that they are not silent push notifications.
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist