@W-18766706 Fix bug of mixed global and local state, just staying with Global state #3878

Crebs · 2025-06-12T21:00:14Z

We need to move back to global state dependence on the original design.

bbirman · 2025-06-12T21:16:20Z

libs/SalesforceSDKCore/SalesforceSDKCore/Classes/PushNotification/PushNotificationManager.swift

@@ -473,4 +475,14 @@ private extension PushNotificationManager {
        SFSDKCoreLogger.i(Self.self, message: "App entering foreground, re-registering push")
        _ = registerSalesforceNotifications(completionBlock: nil, failBlock: nil)
    }
+
+    func refreshDependencies() {


Are these values being stored to make it more testable vs calling something like RestClient.restClient(for: user) directly in the registration method?

yeah, it was because I was trying to do some dependency injection for unit tests.

I'm not sure if we can always assume the current user if we expose a user parameter in public func registerForSalesforceNotifications(user: UserAccount, completionBlock:@escaping (Result<Bool, PushNotificationManagerError>)->())

Ah, I think I miss read this last comment. Yeah, I think I see what you are saying here. Is it that we have a local state and we are still using the gloabl state instead of the global state in the method?

I cleaned up the parameter interface and global state dependence a bit. Since we kind of depend on the global state for determining our login flow for notification and things like that, I just moved back to using global state as it's the original pattern. Let me know if you think this looks better?

I updated the unit tests as well to mock the global state as well to be more accurate to our original design for Push Notification Manager.

I think there's a bug even in the old code where registerSalesforceNotificationsWithCompletionBlock:(SFUserAccount *)user would use the RestClient for the current user instead of the RestClient for the user in the parameter. It looks like the global state here would have the same issue if the current user and the user in the parameter were different. Is there a way to keep the testability improvements you made and fix that?

cc @wmathurin if you have a different take

If we kept the global state approach I think user switching would also need to be observed

@bbirman You are right, the code has the issue before this PR.
The issue existed also in the original Objective-C code.

See https://github.com/forcedotcom/SalesforceMobileSDK-iOS/blob/v12.2.0/libs/SalesforceSDKCore/SalesforceSDKCore/Classes/PushNotification/SFPushNotificationManager.m#L163

We were sending the request using the default rest client [SFRestAPI sharedInstance] (which is bound to the current user [SFRestAPI sharedInstanceWithUser:[SFUserAccountManager sharedInstance].currentUser]).
Instead we should have used the rest client for the user being passed through.

Maybe we shouldn't have a restClient member in the class and instead get the right one in each method that needs one using the user passed in?

OK, in this update I introduced a protocol-based approach for remote notification registration to keep testability — making it easier to inject mocks in unit tests.

What’s the Fix?

We now fetch the correct RestClient for the user being passed in (instead of relying on the global currentUserAccount). To do this, I updated the protocol to:

func clientForRemoteNotification(for user: UserAccount?) -> RestClient?

This fixes the bug from the original implementation and keeps the code testable and clean.

github-actions · 2025-06-13T16:54:10Z

	1 Warning
⚠️	Static Analysis found an issue with one or more files you modified. Please fix the issue(s).

Clang Static Analysis Issues

File	Type	Category	Description	Line	Col
SFUserAccountManager	Nullability	Memory error	Null passed to a callee that requires a non-null 2nd parameter	1452	15
SFUserAccountManager	Nullability	Memory error	Null passed to a callee that requires a non-null 2nd parameter	1467	15
SFUserAccountManager	Nullability	Memory error	nil passed to a callee that requires a non-null 2nd parameter	2099	13

Generated by 🚫 Danger

Crebs · 2025-06-13T16:56:09Z

libs/SalesforceSDKCore/SalesforceSDKCore/Classes/UserAccount/SFUserAccountManager.m

@@ -670,7 +670,7 @@ - (void)logoutUser:(SFUserAccount *)user reason:(SFLogoutReason)reason {

    // Before starting actual logout (which will tear down SFRestAPI), first unregister from push notifications if needed
    __weak typeof(self) weakSelf = self;
-    [[SFPushNotificationManager sharedInstance] unregisterSalesforceNotificationsWithCompletionBlock:user completionBlock:^void() {


Objc and Swift overlook here. Looks like the original method was this as well.

Can this go back to passing the user parameter?

codecov · 2025-06-13T17:03:49Z

Codecov Report

Attention: Patch coverage is 75.00000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 62.40%. Comparing base (d211e3e) to head (aca61b0).
Report is 31 commits behind head on dev.

Files with missing lines	Patch %	Lines
...ses/PushNotification/PushNotificationManager.swift	84.21%	6 Missing ⚠️
...shNotification/RemoteNotificationRegistering.swift	50.00%	4 Missing ⚠️
...SDKCore/Classes/UserAccount/SFUserAccountManager.m	0.00%	2 Missing ⚠️

❌ Your patch status has failed because the patch coverage (75.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #3878      +/-   ##
==========================================
- Coverage   62.78%   62.40%   -0.39%     
==========================================
  Files         242      242              
  Lines       22198    22208      +10     
==========================================
- Hits        13938    13859      -79     
- Misses       8260     8349      +89

Components	Coverage Δ
Analytics	`70.78% <ø> (ø)`
Common	`69.58% <ø> (ø)`
Core	`52.03% <75.00%> (-0.62%)`	⬇️
SmartStore	`73.68% <ø> (ø)`
MobileSync	`87.69% <ø> (ø)`

Files with missing lines	Coverage Δ
...SDKCore/Classes/UserAccount/SFUserAccountManager.m	`36.64% <0.00%> (-3.30%)`	⬇️
...shNotification/RemoteNotificationRegistering.swift	`45.45% <50.00%> (+12.12%)`	⬆️
...ses/PushNotification/PushNotificationManager.swift	`81.20% <84.21%> (-0.26%)`	⬇️

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bbirman · 2025-06-13T18:34:41Z

libs/SalesforceSDKCore/SalesforceSDKCore/Classes/PushNotification/PushNotificationManager.swift

-    /// - Returns: `true` if unregistration started successfully, otherwise `false`.
-    @discardableResult
-    @objc(unregisterSalesforceNotificationsWithCompletionBlock:completionBlock:)
-    public func unregisterSalesforceNotifications(for user: UserAccount,


Making this private looks like it would be a breaking change from 13.0.x

SalesforceMobileSDK-iOS/libs/SalesforceSDKCore/SalesforceSDKCore/Classes/PushNotification/SFPushNotificationManager.h

Line 100 in 096b4e5

- (BOOL)unregisterSalesforceNotificationsWithCompletionBlock:(SFUserAccount*)user completionBlock:(nullable void (^)(void))completionBlock NS_REFINED_FOR_SWIFT;

Same comment on the latest, I only see a private method for unregistering for a user?

Ah, you’re right — I think I was looking at the Objective-C header and got tripped up by the naming. It does look like I goofed that… seeing completionBlock twice in there threw me off. 🫨

bbirman · 2025-06-13T18:53:38Z

I'm running into the same error on login with the latest: "Cannot register for notifications with Salesforce: no restClient"

Crebs · 2025-06-13T21:13:00Z

@bbirman I ran it locally again after making the additional changes and was able to see a notification after login. Did a couple of times as well.

Crebs · 2025-06-23T19:12:26Z

libs/SalesforceSDKCore/SalesforceSDKCore/Classes/PushNotification/PushNotificationManager.swift

@@ -224,6 +347,8 @@ public class PushNotificationManager: NSObject {
            return false
        }

+        let restClient = notificationRegister.clientForRemoteNotification(for: user)


@wmathurin @bbirman I’ve updated the implementation to use a protocol for interfacing with the rest client. I removed the restClient property from the class and now inject a dependency that conforms to the protocol. This allows us to provide either a mock rest client in tests or fetch the appropriate client from the UserAccountManager.shared (as was done previously), but now in a cleaner and testable way.

bbirman · 2025-06-23T20:03:47Z

libs/SalesforceSDKCore/SalesforceSDKCore/Classes/PushNotification/PushNotificationManager.swift

+        _ = registerSalesforceNotifications(completionBlock: nil, failBlock: nil)
+    }
+
+    private func registerSalesforceNotifications(for user: UserAccount,


Like unregistering, registering notifications for a given user was a public API in 13.0

Yeah, Kept thinking those were typo errors by me. with completionBlock in teh Objc header naming.

…in Swift code

…ity for testing

bbirman · 2025-06-24T17:37:45Z

libs/SalesforceSDKCore/SalesforceSDKCore/Classes/PushNotification/PushNotificationManager.swift

-                   restClient: RestClient.shared,
-                   currentUser: UserAccountManager.shared.currentUserAccount,
-                   preferences: SFPreferences.sharedPreferences(for: .user, user: UserAccountManager.shared.currentUserAccount))
+                  preferences: SFPreferences.currentUserLevel())


I think the user preferences could get out of sync on user logout / login / switch if it's set at init, is it possible to not store it at the class level like you did with the user account and rest client?

Yes. I'll do a similar pattern for preferences

bbirman · 2025-06-24T22:24:26Z

libs/SalesforceSDKCore/SalesforceSDKCore/Classes/PushNotification/PushNotificationManager.swift

@@ -224,6 +202,8 @@ public class PushNotificationManager: NSObject {
            return false
        }

+        let restClient = notificationRegister.client(for: user)


sorry last one, could you also add a guard or something for the rest client and log an error if it's nil for some reason? I ran into a similar case recently and just want to make sure it can't fail silently. same for the similar block in the unregister method

Yes. I'll update that this morning.

Refresh dependencies when user logs in

3e5d73b

Crebs requested a review from bbirman June 12, 2025 21:00

bbirman reviewed Jun 12, 2025

View reviewed changes

cleaning up PushNotification manager Interface

18ed33a

Crebs commented Jun 13, 2025

View reviewed changes

Crebs changed the title ~~@W-18766706 Refresh dependencies when user logs in~~ @W-18766706 Fix bug of mixed global and local state, just staying with Global state Jun 13, 2025

bbirman reviewed Jun 13, 2025

View reviewed changes

ensure rest client updated on login and logout

f8b18f6

Crebs requested review from bbirman and wmathurin June 13, 2025 21:11

Crebs commented Jun 23, 2025

View reviewed changes

bbirman reviewed Jun 23, 2025

View reviewed changes

Add swift style observers so we don't have to depend on Objc runtime …

09c5040

…in Swift code

Crebs force-pushed the @W-18766706 branch from 1cbc20e to 09c5040 Compare June 23, 2025 20:16

pass in user to unregister method

670c1a3

Crebs force-pushed the @W-18766706 branch 3 times, most recently from 0b80b16 to 5720434 Compare June 23, 2025 20:40

we need to pass in the account to follow current pattern for mockabli…

c1b7d23

…ity for testing

Crebs force-pushed the @W-18766706 branch from 5720434 to c1b7d23 Compare June 23, 2025 20:41

bbirman reviewed Jun 24, 2025

View reviewed changes

Support preferences in registry as we do with getting the account

07b7fa2

Crebs requested a review from bbirman June 24, 2025 19:25

bbirman reviewed Jun 24, 2025

View reviewed changes

Add logging for case where there is no rest client

aca61b0

bbirman approved these changes Jun 25, 2025

View reviewed changes

Crebs merged commit de2331a into forcedotcom:dev Jun 25, 2025
17 of 22 checks passed

Crebs deleted the @W-18766706 branch June 25, 2025 17:10

@W-18766706 Fix bug of mixed global and local state, just staying with Global state #3878

@W-18766706 Fix bug of mixed global and local state, just staying with Global state #3878

Uh oh!

Conversation

Crebs commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wmathurin Jun 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Clang Static Analysis Issues

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbirman Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbirman commented Jun 13, 2025

Uh oh!

Crebs commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Crebs commented Jun 12, 2025 •

edited

Loading

wmathurin Jun 14, 2025 •

edited

Loading

github-actions bot commented Jun 13, 2025 •

edited

Loading

codecov bot commented Jun 13, 2025 •

edited

Loading

bbirman Jun 23, 2025 •

edited

Loading

Crebs commented Jun 13, 2025 •

edited

Loading