fix: Unable to enable Push Notifications (OS Settings disabled)#3939
Merged
fix: Unable to enable Push Notifications (OS Settings disabled)#3939
Conversation
gastonm5
changed the title
WalkthroughThis pull request adds localization strings for disabled notification messaging across seven languages, introduces an authorization status check method in PushNotificationManager, and enhances NotificationsSettingsScreen with scene phase tracking, permission verification, and a settings guidance alert to address macOS notification toggle responsiveness issues. Changes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
VultisigApp/VultisigApp/Views/Settings/Notifications/NotificationsSettingsScreen.swift (2)
144-149: Potential redundant state update loop.
checkForPermissions()callsonNotificationsEnabled(isEnabled)which may trigger unnecessary logic. When the app becomes active and permissions are still denied, this will:
- Call
onNotificationsEnabled(false)- Which sets
notificationsEnabled = falseand callsunregisterForRemoteNotifications()This could cause repeated unregister calls each time the app becomes active.
♻️ Proposed fix to avoid redundant updates
`@MainActor` func checkForPermissions() async { await pushNotificationManager.checkPermissionStatus() let isEnabled = pushNotificationManager.isPermissionGranted - onNotificationsEnabled(isEnabled) + // Only update if state actually changed to avoid redundant side effects + if notificationsEnabled != isEnabled { + notificationsEnabled = isEnabled + if !isEnabled { + pushNotificationManager.setAllVaultsOptIn(vaults, enabled: false) + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@VultisigApp/VultisigApp/Views/Settings/Notifications/NotificationsSettingsScreen.swift` around lines 144 - 149, checkForPermissions() is causing redundant state updates by always calling onNotificationsEnabled(isEnabled) even when notificationsEnabled already equals that value; change the logic so you only invoke onNotificationsEnabled when the permission state actually changes: after awaiting pushNotificationManager.checkPermissionStatus() and reading pushNotificationManager.isPermissionGranted, compare that boolean against the current notificationsEnabled property and call onNotificationsEnabled(isEnabled) (or early-return inside onNotificationsEnabled) only if they differ, thereby preventing repeated calls to unregisterForRemoteNotifications() and unnecessary side-effects.
134-142: Improve consistency in URL handling across platforms.The macOS URL scheme
x-apple.systempreferences:com.apple.preference.notificationsis correct. However, consider adopting the safer URL handling pattern (with optional binding) used in the macOS branch for consistency across both platforms, even thoughUIApplication.openSettingsURLStringis a system constant.♻️ Suggested refactor for consistency
private func openSettings() { `#if` os(iOS) - UIApplication.shared.open(URL(string: UIApplication.openSettingsURLString)!) + if let url = URL(string: UIApplication.openSettingsURLString) { + UIApplication.shared.open(url) + } `#elseif` os(macOS) if let url = URL(string: "x-apple.systempreferences:com.apple.preference.notifications") { NSWorkspace.shared.open(url) } `#endif` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@VultisigApp/VultisigApp/Views/Settings/Notifications/NotificationsSettingsScreen.swift` around lines 134 - 142, Change the iOS branch in openSettings to mirror the safer optional-URL handling used on macOS: instead of force-unwrapping URL(string: UIApplication.openSettingsURLString)!, construct the URL with optional binding (if let url = URL(string: UIApplication.openSettingsURLString) { UIApplication.shared.open(url) }) so the function uses consistent, non-crashing URL handling (see openSettings, UIApplication.openSettingsURLString, UIApplication.shared.open and the macOS NSWorkspace.shared.open pattern).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@VultisigApp/VultisigApp/Views/Settings/Notifications/NotificationsSettingsScreen.swift`:
- Around line 144-149: checkForPermissions() is causing redundant state updates
by always calling onNotificationsEnabled(isEnabled) even when
notificationsEnabled already equals that value; change the logic so you only
invoke onNotificationsEnabled when the permission state actually changes: after
awaiting pushNotificationManager.checkPermissionStatus() and reading
pushNotificationManager.isPermissionGranted, compare that boolean against the
current notificationsEnabled property and call onNotificationsEnabled(isEnabled)
(or early-return inside onNotificationsEnabled) only if they differ, thereby
preventing repeated calls to unregisterForRemoteNotifications() and unnecessary
side-effects.
- Around line 134-142: Change the iOS branch in openSettings to mirror the safer
optional-URL handling used on macOS: instead of force-unwrapping URL(string:
UIApplication.openSettingsURLString)!, construct the URL with optional binding
(if let url = URL(string: UIApplication.openSettingsURLString) {
UIApplication.shared.open(url) }) so the function uses consistent, non-crashing
URL handling (see openSettings, UIApplication.openSettingsURLString,
UIApplication.shared.open and the macOS NSWorkspace.shared.open pattern).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
VultisigApp/VultisigApp/Localizables/de.lproj/Localizable.stringsVultisigApp/VultisigApp/Localizables/en.lproj/Localizable.stringsVultisigApp/VultisigApp/Localizables/es.lproj/Localizable.stringsVultisigApp/VultisigApp/Localizables/hr.lproj/Localizable.stringsVultisigApp/VultisigApp/Localizables/it.lproj/Localizable.stringsVultisigApp/VultisigApp/Localizables/pt.lproj/Localizable.stringsVultisigApp/VultisigApp/Localizables/zh-Hans.lproj/Localizable.stringsVultisigApp/VultisigApp/Services/Notification/PushNotificationManager.swiftVultisigApp/VultisigApp/Views/Settings/Notifications/NotificationsSettingsScreen.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Please include a summary of the change and which issue is fixed.
Fixes #3935
Which feature is affected?
Checklist
Screenshots (if applicable):
Additional context
Summary by CodeRabbit