-
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: disable notifications when basic functionality off #28045
fix: disable notifications when basic functionality off #28045
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. |
@@ -83,6 +85,9 @@ export function useDisableProfileSyncing(): { | |||
try { | |||
// disable profile syncing | |||
await dispatch(disableProfileSyncingAction()); | |||
|
|||
// list notifications to update the counter | |||
await listNotifications(); |
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.
Updating the list after the notifications have been disabled, the counter is set to zero
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.
nice, we can ticket this for a core change in future.
@@ -74,15 +74,14 @@ export function NotificationsSettingsAllowNotifications({ | |||
}, [isMetamaskNotificationsEnabled]); | |||
|
|||
useEffect(() => { | |||
if (isMetamaskNotificationsEnabled && !error) { | |||
if (!error) { |
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.
In this way, the update of the list and therefore the counter is triggered even when the notifications are disabled
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.
We need to update the list when the notifications are enabled AND disabled (because we want to reset the counter)
FUTURE - move logic to core library instead of doing this error/reset.
…b.com:MetaMask/metamask-extension into fix/basic-functionality-disable-notifications
@@ -31,14 +30,14 @@ import { getUseExternalServices } from '../../../../selectors'; | |||
|
|||
function ProfileSyncBasicFunctionalitySetting() { | |||
const basicFunctionality: boolean = useSelector(getUseExternalServices); | |||
const { setIsProfileSyncingEnabled } = useSetIsProfileSyncingEnabled(); | |||
const { disableProfileSyncing } = useDisableProfileSyncing(); |
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.
Nice, this hook will disable profile syncing and disable notifications.
FUTURE - we probs want to clean up the hooks.
ui/pages/settings/security-tab/profile-sync-toggle/profile-sync-toggle.tsx
Show resolved
Hide resolved
Builds ready [6a5814f]
Page Load Metrics (2071 ± 109 ms)
Bundle size diffs
|
Description
This PR ensures that notifications are correctly disabled when the user deactivates Basic functionality. The current implementation only disables profile sync when Basic functionality is turned off, leaving notifications in an incorrect state—they can’t be fetched but remain active. This causes a series of UI errors, the most obvious being that the counters stay visible.
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist