fix: request notification permission within the user gesture and surface registration errors - #8571
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7383738770
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
📦 Next.js Bundle Analysis for @safe-global/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖
|
| Page | Size (compressed) |
|---|---|
global |
1.53 MB (🔴 +314.12 KB) |
Details
The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.
Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis
If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!
New Page Added
The following page was added to the bundle from the code in this PR:
| Page | Size (compressed) | First Load |
|---|---|---|
/spaces/activity |
546 B |
1.53 MB |
Sixty-one Pages Changed Size
The following pages changed size from the code in this PR compared to its base branch:
| Page | Size (compressed) | First Load |
|---|---|---|
/ |
424 B (🟢 -290 B) |
1.53 MB |
/403 |
635 B (🟢 -86 B) |
1.53 MB |
/404 |
837 B (🟡 +19 B) |
1.53 MB |
/_offline |
1.05 KB (🟢 -133 B) |
1.53 MB |
/addOwner |
551 B (🟢 -1 B) |
1.53 MB |
/address-book |
23.08 KB (🟢 -6.82 KB) |
1.55 MB |
/apps |
44.98 KB (🔴 +9.28 KB) |
1.57 MB |
/apps/custom |
42.42 KB (🔴 +10.01 KB) |
1.57 MB |
/apps/open |
9.27 KB (🟢 -104.05 KB) |
1.54 MB |
/balances |
25.52 KB (🟢 -109.58 KB) |
1.55 MB |
/balances/nfts |
16.29 KB (🟢 -3.56 KB) |
1.55 MB |
/balances/positions |
5.96 KB (🟢 -2.02 KB) |
1.54 MB |
/bridge |
553 B (🟢 -66 B) |
1.53 MB |
/cookie |
7.33 KB (🟢 -48 B) |
1.54 MB |
/dashboard/new |
1.42 KB (🟢 -2.09 KB) |
1.53 MB |
/earn |
575 B (🟢 -55 B) |
1.53 MB |
/home |
195.66 KB (🟢 -72.33 KB) |
1.72 MB |
/hypernative/oauth-callback |
2.07 KB (🟡 +58 B) |
1.53 MB |
/imprint |
1.42 KB (🟢 -9 B) |
1.53 MB |
/licenses |
1.96 KB (🟢 -94 B) |
1.53 MB |
/new-safe/advanced-create |
30.99 KB (🟢 -202.55 KB) |
1.56 MB |
/new-safe/create |
29.46 KB (🟢 -203.28 KB) |
1.56 MB |
/new-safe/load |
9.32 KB (🟢 -4.02 KB) |
1.54 MB |
/privacy |
13.21 KB (🟢 -507 B) |
1.54 MB |
/settings |
770 B (🟢 -5.72 KB) |
1.53 MB |
/settings/appearance |
6.17 KB (🟢 -2.25 KB) |
1.54 MB |
/settings/cookies |
6 KB (🟢 -2.04 KB) |
1.54 MB |
/settings/data |
32.36 KB (🟢 -2.66 KB) |
1.56 MB |
/settings/environment-variables |
7.26 KB (🟢 -2.35 KB) |
1.54 MB |
/settings/modules |
6.92 KB (🟢 -4.81 KB) |
1.54 MB |
/settings/notifications |
11.41 KB (🟢 -2.5 KB) |
1.54 MB |
/settings/safe-apps |
8.88 KB (🟢 -11.09 KB) |
1.54 MB |
/settings/security |
6.49 KB (🟢 -2.2 KB) |
1.54 MB |
/settings/setup |
37.79 KB (🟢 -19.37 KB) |
1.57 MB |
/share/safe-app |
6.63 KB (🟢 -1.66 KB) |
1.54 MB |
/spaces |
611 B (🟡 +2 B) |
1.53 MB |
/spaces/address-book |
509 B (🟡 +2 B) |
1.53 MB |
/spaces/create-space |
440 B (🟡 +3 B) |
1.53 MB |
/spaces/members |
499 B (🟡 +3 B) |
1.53 MB |
/spaces/safe-accounts |
507 B (🟡 +1 B) |
1.53 MB |
/spaces/security |
6.75 KB (🟢 -121.95 KB) |
1.54 MB |
/spaces/settings/about |
508 B (🟡 +4 B) |
1.53 MB |
/spaces/settings/account |
509 B (🟡 +3 B) |
1.53 MB |
/spaces/settings/general |
509 B (🟡 +3 B) |
1.53 MB |
/stake |
1.22 KB (🟡 +124 B) |
1.53 MB |
/swap |
744 B (🟢 -417 B) |
1.53 MB |
/terms |
13.71 KB (🟢 -19 B) |
1.54 MB |
/transactions |
35.4 KB (🟢 -128.78 KB) |
1.56 MB |
/transactions/history |
35.36 KB (🟢 -128.78 KB) |
1.56 MB |
/transactions/messages |
14.17 KB (🟢 -105.46 KB) |
1.54 MB |
/transactions/msg |
6.28 KB (🟢 -106.93 KB) |
1.54 MB |
/transactions/queue |
6.22 KB (🟢 -104 KB) |
1.54 MB |
/transactions/tx |
1.01 KB (🟢 -105.67 KB) |
1.53 MB |
/wc |
586 B (🟡 +1 B) |
1.53 MB |
/welcome |
435 B (🟢 -13.87 KB) |
1.53 MB |
/welcome/accounts |
610 B (🟢 -112 B) |
1.53 MB |
/welcome/create-space |
444 B (🟡 +2 B) |
1.53 MB |
/welcome/invite-members |
445 B (🟡 +4 B) |
1.53 MB |
/welcome/select-safes |
448 B (🟡 +8 B) |
1.53 MB |
/welcome/spaces |
308 B (🟢 -197 B) |
1.53 MB |
/welcome/survey |
697 B (🟡 +1 B) |
1.53 MB |
Details
Only the gzipped size is provided here based on an expert tip.
First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.
Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis
Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
|---|---|---|---|
| 🟢 | Statements | 85.12% (+0.09% 🔼) |
35272/41438 |
| 🟡 | Branches | 70.06% (+0.11% 🔼) |
11528/16454 |
| 🟡 | Functions | 73.5% (+0.12% 🔼) |
5387/7329 |
| 🟢 | Lines | 86.37% (+0.09% 🔼) |
31570/36550 |
Show files with reduced coverage 🔻
St.❔ |
File | Statements | Branches | Functions | Lines |
|---|---|---|---|---|---|
| 🟢 | ... / SeverityIcon.tsx |
100% | 80% (-20% 🔻) |
100% | 100% |
Test suite run success
7709 tests passing in 878 suites.
Report generated by 🧪jest coverage report action from 810a2de
|
Claude (@claude) review |
This comment was marked as outdated.
This comment was marked as outdated.
|
Claude (@claude) review again based on previous findings |
This comment was marked as outdated.
This comment was marked as outdated.
6301579 to
77c8a01
Compare
…ev-works-on-production
|
Claude (@claude) review again based on previous findings |
This comment was marked as outdated.
This comment was marked as outdated.
…ev-works-on-production
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9021a5ff84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| @@ -31,20 +35,23 @@ const registrationFlow = async ( | |||
| // Gateway will return empty data if the device was (un-)registered successfully | |||
| // @see https://github.com/safe-global/safe-client-gateway-nest/blob/27b6b3846b4ecbf938cdf5d0595ca464c10e556b/src/routes/notifications/notifications.service.ts#L29 | |||
| // Success only if no error and data is empty/undefined | |||
There was a problem hiding this comment.
The comment should be updated.
| showErrorNotification( | ||
| error instanceof Error && isWalletRejection(error) | ||
| ? SIGNATURE_REJECTED_MESSAGE | ||
| : 'Failed to enable push notifications. Please try again.', |
There was a problem hiding this comment.
We should also handle API errors and return the actual reason.
Here we are only capturing Wallet rejection errors.
| globalThis.Notification = { | ||
| requestPermission: requestPermissionMock, | ||
| permission: 'granted', | ||
| } as unknown as jest.Mocked<typeof Notification> |
There was a problem hiding this comment.
Can we create a proper Notification object instead of mocking? It applies to other objects in the same file as well.
There was a problem hiding this comment.
i will keep it as is this time, we can refactor it afterwards
2 participants
What it solves
Resolves: WA-3207
Enabling push notifications could fail with
Code 633/messaging/permission-blockedand the failure was completely silent — the toggle just reverted to Off with no feedback.Two defects:
getToken(), which runs only afterawait navigator.serviceWorker.getRegistrations()— outside the click's user activation, so browsers can auto-deny without ever showing a prompt. (Not a shadcn migration regression: the migration only changed the JSX binding; the handler body is unchanged since 2023.)registrationFlowswallowed every failure with onlylogError(ErrorCodes._633)— no toast, no alert, and the promise never rejected to the caller.How this PR fixes it
registerNotificationsnow callsrequestNotificationPermission()before its first await, so the browser prompt opens within the click's user activation. This single placement covers all enable entry points: the per-Safe toggle, the confirmation-requests checkbox, and the renewal flow.registrationFlowgot anonErrorcallback: failures now show an error toast instead of failing silently.groupKey(notifications-error) so a concurrent success toast cannot auto-close an error.GlobalPushNotifications.onSave: removed the now-redundant upfront permission check (the hook performs the same gate and toasts on denial — keeping it would double-toast). Added an emptiness guard so unregister-only saves never trigger a permission prompt (_mergeNotifiableSafescan return a truthy{}).requestNotificationPermissionguards against environments without theNotificationAPI.How to test it
Affected flows
Blast radius
useNotificationRegistrations(registrationFlow,registerNotifications,unregisterSafeNotifications,unregisterDeviceNotifications) — consumers: per-Safe toggle, confirmation-requests checkbox,GlobalPushNotifications.onSave,useNotificationsRenewallogic.ts— newisPermissionBlocked()export;requestNotificationPermissionnow returnsfalsewhen theNotificationAPI is missingnotificationsSliceshowNotification— consumed as-is; newnotifications-errorgroup keyRisks / not checked
Notification.permissiondiagnostic from the ticket still needs to be run onsafe-wallet-web.dev.5afe.devto classify Defect 1 (auto-deny vs. previously blocked origin).firebase-messaging-sw.js) — different layer, intentionally out of scope.Visual summary
flowchart TB subgraph Before A1[Toggle click] --> B1["await getRegistrations()"] B1 --> C1["getToken() requests permission<br/>outside user activation"] C1 -->|auto-denied| D1[registrationFlow catch:<br/>logError only] D1 --> E1[Toggle reverts silently] end subgraph After A2[Toggle click] --> B2["requestNotificationPermission()<br/>within user activation"] B2 -->|granted| C2["getToken() + signature + register"] B2 -->|denied| F2[Error toast:<br/>blocked / permission required] C2 -->|success| D2[Success toast, toggle On] C2 -->|failure| G2[Error toast: failed to enable] endChecklist
CLA signature
With the submission of this Pull Request, I confirm that I have read and agree to the terms of the Contributor License Agreement.