Refactor SKAN related code and BranchOpenRequest Class#1570
Refactor SKAN related code and BranchOpenRequest Class#1570NidhiDixit09 merged 20 commits into4.0.0-alpha.0from
Conversation
Summary By MatterAI
🔄 What ChangedThis PR executes a significant structural refactor of the Branch iOS SDK. Key changes include moving SKAdNetwork (SKAN) logic from 🔍 Impact of the ChangeThe refactor significantly reduces technical debt by eliminating duplicate SKAN logic and isolating response processing. Defensive key validation for 📁 Total Files ChangedClick to Expand
🧪 Test Added/RecommendedAdded
Recommended
🔒 Security VulnerabilitiesNo critical vulnerabilities detected. The implementation of defensive checks for device tokens and synchronized initialization guards in ⏳ Estimated code review effortMEDIUM (~20 minutes) Tip Quality Recommendations
♫ Tanka Poem
Sequence DiagramsequenceDiagram
participant S as Branch Server
participant BOR as BranchOpenRequest
participant SKAN as BNCSKAdNetwork
participant PH as BNCPreferenceHelper
S->>BOR: HTTP Response (JSON)
Note over BOR: processResponse()
BOR->>BOR: sessionDataFromResponseData(data)
BOR->>BOR: handleSKANWithResponseData(data)
BOR->>SKAN: updateConversionValue(responseData)
SKAN->>PH: read/write SKAN settings
Note over BOR: Update Session State
|
There was a problem hiding this comment.
🧪 PR Review is completed: Refactoring looks solid overall, moving SKAN logic to a dedicated class is a good improvement. Found a critical logic issue in randomizedDeviceTokenFromResponseData where the fallback code is unreachable, and a type safety improvement for userIdentityFromResponseData.
wpinho-branch
left a comment
There was a problem hiding this comment.
Overall the refactoring direction looks great — consolidating SKAN logic into BNCSKAdNetwork and extracting helpers from processResponse makes the code much more readable and testable. Nice work!
A few minor things I noticed:
- releaseOpenResponseLock ordering (BranchOpenRequest.m:146) — moved after SKAN handling, which extends the lock scope slightly. See inline comment.
- randomizedDeviceTokenFromResponseData: fallback (BranchOpenRequest.m:169) — subtle change in when the deprecated key fallback is attempted. See inline comment.
- Commit hygiene — there are duplicate commit messages and a "Test commit" (b4ef55e). Consider squashing before merge for a cleaner history.
None of these are blockers — the core refactoring is solid.
- Added API - `- (void)setConsumerProtectionAttributionLevel:(BranchAttributionLevel)level resetSession:(BOOL)resetSession` to provide an option to disable resetting session. - Remove silent SDK initialization - initSafetyCheck removed.Now APIs will return or log BNCInitError error. - Added @synchronized(self) around all 5 init status checks to ensure it waits for any in-progress initSession code (which also uses @synchronized(self)) to finish and update the status before evaluating.
|
✅ Reviewed the changes: Refactoring looks good. Found a few minor issues: a typo in the log messages ('intialize' -> 'initialize') and a convention issue (using |
|
✅ Reviewed the changes: The provided changes in the test files and testbed app look good. The addition of |
Reference
[iOS] Refactor SKAN related code and BranchOpenRequest Class
https://branch.atlassian.net/browse/EMT-3068
Summary
This PR contains no behavioral changes. It has purely structural refactoring to remove duplicate codes and simplify functions for easier testing and debugging.
ReFactored BranchOpenRequest and BranchEvent class to move SKAN related code to BNCSKAdNetwork class.
Moved code out of processResponse function into more focused helper functions.
cc @BranchMetrics/saas-sdk-devs for visibility.