Fix buggy activity count logic #1270

gdeluna-branch · 2025-05-23T05:11:02Z

Reference

SDK-XXX -- <TITLE>.

Description

Testing Instructions

Risk Assessment [`HIGH` || `MEDIUM` || `LOW`]

I, the PR creator, have tested — integration, unit, or otherwise — this code.

Reviewer Checklist (To be checked off by the reviewer only)

JIRA Ticket is referenced in PR title.
Correctness & Style
- Conforms to AOSP Style Guides
- Mission critical pieces are documented in code and out of code as needed.
Unit Tests reviewed and test issue sufficiently.
Functionality was reviewed in QA independently by another engineer on the team.

cc @BranchMetrics/saas-sdk-devs for visibility.

Copilot

Pull Request Overview

This PR addresses issues with the activity count logic in the BranchActivityLifecycleObserver by adjusting log output and resetting the activity counter when necessary. Key changes include:

Removing the activities-on-stack information from some log statements while adding separate logs for activity count and activities on stack.
Resetting activityCnt_ to 0 when it is negative to prevent unintended session clearance.
Adding additional debug logging in onActivityResumed, onActivityPaused, onActivityStopped, and onActivityDestroyed.

Copilot · 2025-05-23T05:11:30Z

Branch-SDK/src/main/java/io/branch/referral/BranchActivityLifecycleObserver.java

    public void onActivityResumed(@NonNull Activity activity) {
        Branch branch = Branch.getInstance();
-        BranchLogger.v("onActivityResumed, activity = " + activity + " branch: " + branch + " Activities on stack: " + activitiesOnStack_);
+        BranchLogger.v("onActivityResumed, activity = " + activity + " branch: " + branch);


[nitpick] Consider including the activitiesOnStack_ information in the onActivityResumed log for consistency with the logging in other lifecycle methods, as it can aid in debugging the activity stack state.

Copilot · 2025-05-23T05:11:30Z

Branch-SDK/src/main/java/io/branch/referral/BranchActivityLifecycleObserver.java

+            In such cases, activityCnt_ could be set to -1, which could cause the above line to clear
+            session parameters. Just reset to 0 if we're here.
+            */
+            activityCnt_ = 0;


While resetting activityCnt_ to 0 is a defensive measure for negative values, consider also logging a warning when such a reset occurs to help diagnose unexpected lifecycle inconsistencies.

Suggested change

activityCnt_ = 0;

activityCnt_ = 0;

BranchLogger.w("Unexpected lifecycle inconsistency: activityCnt_ was negative and has been reset to 0.");

matter-code-review · 2025-05-23T05:11:43Z

Reference

SDK-XXX -- Fix buggy activity count logic.

Summary By MatterAI

🔄 What Changed

Fixed a bug in the activity count logic in BranchActivityLifecycleObserver.java
Added additional logging to track activity lifecycle events
Added a reset mechanism for activityCnt_ to prevent negative values
Added detailed comments explaining the issue and fix

🔍 Impact of the Change

This change prevents the activity counter from going negative, which could cause session parameters to be incorrectly cleared. The fix ensures proper session management across different app integration patterns.

📁 Total Files Changed

1 file modified: Branch-SDK/src/main/java/io/branch/referral/BranchActivityLifecycleObserver.java with 18 additions and 3 deletions.

🧪 Test Added

N/A - No explicit test additions in the PR.

🔒Security Vulnerabilities

N/A - No security vulnerabilities detected.

Testing Instructions

Risk Assessment `LOW`

I, the PR creator, have tested — integration, unit, or otherwise — this code.

Reviewer Checklist (To be checked off by the reviewer only)

JIRA Ticket is referenced in PR title.
Correctness & Style
- Conforms to AOSP Style Guides
- Mission critical pieces are documented in code and out of code as needed.
Unit Tests reviewed and test issue sufficiently.
Functionality was reviewed in QA independently by another engineer on the team.

cc @BranchMetrics/saas-sdk-devs for visibility.

Tip

Quality Recommendations

Consider adding unit tests to verify the fixed activity count logic
Add more descriptive comments explaining the activityCnt_ variable's purpose and expected values
Consider adding null checks for activitiesOnStack_ before logging
Implement a more robust solution for tracking activity lifecycle that doesn't rely on counter resets
Consider using a more structured logging approach with consistent log levels

Sequence Diagram

sequenceDiagram
    participant App as Android App
    participant Observer as BranchActivityLifecycleObserver
    participant Branch as Branch SDK
    participant Logger as BranchLogger
    
    Note over Observer: Fix for buggy activity count logic
    
    App->>Observer: onActivityResumed(activity)
    Observer->>Branch: getInstance()
    Observer->>Logger: v("onActivityResumed, activity = " + activity + " branch: " + branch)
    Observer->>Observer: activitiesOnStack_.add(activity.toString())
    Observer->>Logger: v("activityCnt_: " + activityCnt_)
    Observer->>Logger: v("activitiesOnStack_: " + activitiesOnStack_)
    
    App->>Observer: onActivityPaused(activity)
    Observer->>Branch: getInstance()
    Observer->>Logger: v("onActivityPaused, activity = " + activity + " branch: " + branch)
    Observer->>Branch: getShareLinkManager()
    Observer->>Logger: v("activityCnt_: " + activityCnt_)
    Observer->>Logger: v("activitiesOnStack_: " + activitiesOnStack_)
    
    App->>Observer: onActivityStopped(activity)
    Observer->>Branch: getInstance()
    Observer->>Observer: activityCnt_--
    Note over Observer: Check if activityCnt_ < 1
    Observer->>Branch: setInstantDeepLinkPossible(false)
    Observer->>Branch: closeSessionInternal()
    Observer->>Observer: activityCnt_ = 0
    Observer->>Logger: v("activityCnt_: reset to 0")
    Observer->>Logger: v("activitiesOnStack_: " + activitiesOnStack_)
    
    App->>Observer: onActivityDestroyed(activity)
    Observer->>Branch: getInstance()
    Observer->>Logger: v("onActivityDestroyed, activity = " + activity + " branch: " + branch)
    Observer->>Observer: activitiesOnStack_.remove(activity.toString())
    Observer->>Logger: v("activitiesOnStack_: " + activitiesOnStack_)

matter-code-review

This PR addresses an important bug in the activity count logic. The changes look good overall, with improved logging and a fix for the negative counter issue. I have a few suggestions to further improve the implementation.

matter-code-review · 2025-05-23T05:11:44Z

Branch-SDK/src/main/java/io/branch/referral/BranchActivityLifecycleObserver.java

    public void onActivityResumed(@NonNull Activity activity) {
        Branch branch = Branch.getInstance();
-        BranchLogger.v("onActivityResumed, activity = " + activity + " branch: " + branch + " Activities on stack: " + activitiesOnStack_);
+        BranchLogger.v("onActivityResumed, activity = " + activity + " branch: " + branch);


🛠️ Code Refactor

Issue: The logging statements are added individually for each lifecycle method, which could lead to inconsistent logging patterns.
Fix: Consider extracting the logging into a helper method to ensure consistency.
Impact: Improves code maintainability and ensures consistent logging patterns across lifecycle methods.

Suggested change

BranchLogger.v("onActivityResumed, activity = " + activity + " branch: " + branch);

BranchLogger.v("onActivityResumed, activity = " + activity + " branch: " + branch + ", activityCnt_: " + activityCnt_ + ", activitiesOnStack_: " + activitiesOnStack_);

matter-code-review · 2025-05-23T05:11:45Z

Branch-SDK/src/main/java/io/branch/referral/BranchActivityLifecycleObserver.java

        if (activityCnt_ < 1) {
            branch.setInstantDeepLinkPossible(false);
            branch.closeSessionInternal();
+
+            /* It is possible some integrations do not call Branch.getAutoInstance() before the first
+            activity's lifecycle methods execute.
+            In such cases, activityCnt_ could be set to -1, which could cause the above line to clear
+            session parameters. Just reset to 0 if we're here.
+            */
+            activityCnt_ = 0;
+            BranchLogger.v("activityCnt_: reset to 0");


🐛 Bug Fix

Issue: The code resets activityCnt_ to 0 when it's less than 1, but doesn't handle the case where it might be exactly 0.
Fix: Add a more explicit condition to only reset when negative.
Impact: Prevents unnecessary resets and makes the intent of the code clearer.

Suggested change

if (activityCnt_ < 1) {

branch.setInstantDeepLinkPossible(false);

branch.closeSessionInternal();

/* It is possible some integrations do not call Branch.getAutoInstance() before the first

activity's lifecycle methods execute.

In such cases, activityCnt_ could be set to -1, which could cause the above line to clear

session parameters. Just reset to 0 if we're here.

*/

activityCnt_ = 0;

BranchLogger.v("activityCnt_: reset to 0");

if (activityCnt_ < 1) {

branch.setInstantDeepLinkPossible(false);

branch.closeSessionInternal();

/* It is possible some integrations do not call Branch.getAutoInstance() before the first

activity's lifecycle methods execute.

In such cases, activityCnt_ could be set to -1, which could cause the above line to clear

session parameters. Just reset to 0 if we're negative.

*/

if (activityCnt_ < 0) {

activityCnt_ = 0;

BranchLogger.v("activityCnt_: reset from negative to 0");

}

}

matter-code-review · 2025-05-23T05:11:45Z

Branch-SDK/src/main/java/io/branch/referral/BranchActivityLifecycleObserver.java

+            BranchLogger.v("activityCnt_: reset to 0");
        }
+
+        BranchLogger.v("activitiesOnStack_: " + activitiesOnStack_);


⚡️ Performance Improvement

Issue: The logging of activitiesOnStack_ collection might be expensive if the collection is large.
Fix: Add a conditional check to only log in debug builds or when verbose logging is enabled.
Impact: Reduces unnecessary string concatenation and logging overhead in production builds.

Suggested change

BranchLogger.v("activitiesOnStack_: " + activitiesOnStack_);

if (BranchLogger.isDebugEnabled()) {

BranchLogger.v("activitiesOnStack_: " + activitiesOnStack_);

}

matter-code-review · 2025-05-23T05:11:45Z

Branch-SDK/src/main/java/io/branch/referral/BranchActivityLifecycleObserver.java

    public void onActivityDestroyed(@NonNull Activity activity) {
        Branch branch = Branch.getInstance();
-        BranchLogger.v("onActivityDestroyed, activity = " + activity + " branch: " + branch + " Activities on stack: " + activitiesOnStack_);
+        BranchLogger.v("onActivityDestroyed, activity = " + activity + " branch: " + branch);


💡 Optional Recommendation

Issue: The logging statements for activity lifecycle events are scattered throughout the code.
Fix: Consider adding a debug flag to control all lifecycle logging.
Impact: Makes it easier to enable/disable all lifecycle logging at once for debugging purposes.

Suggested change

BranchLogger.v("onActivityDestroyed, activity = " + activity + " branch: " + branch);

BranchLogger.v("onActivityDestroyed, activity = " + activity + " branch: " + branch);

// TODO: Consider adding a debug flag to control all lifecycle logging

Update BranchActivityLifecycleObserver.java

43c8b68

gdeluna-branch requested review from NidhiDixit09 and Copilot May 23, 2025 05:11

Copilot AI reviewed May 23, 2025

View reviewed changes

matter-code-review bot reviewed May 23, 2025

View reviewed changes

NidhiDixit09 approved these changes May 23, 2025

View reviewed changes

gdeluna-branch merged commit 68f1480 into master May 27, 2025
5 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix buggy activity count logic #1270

Fix buggy activity count logic #1270

Uh oh!

gdeluna-branch commented May 23, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI May 23, 2025

Uh oh!

Copilot AI May 23, 2025

Uh oh!

matter-code-review bot commented May 23, 2025

Quality Recommendations

Uh oh!

matter-code-review bot left a comment

Uh oh!

matter-code-review bot May 23, 2025

Uh oh!

matter-code-review bot May 23, 2025

Uh oh!

matter-code-review bot May 23, 2025

Uh oh!

matter-code-review bot May 23, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	activityCnt_ = 0;
	activityCnt_ = 0;
	BranchLogger.w("Unexpected lifecycle inconsistency: activityCnt_ was negative and has been reset to 0.");

	BranchLogger.v("onActivityResumed, activity = " + activity + " branch: " + branch);
	BranchLogger.v("onActivityResumed, activity = " + activity + " branch: " + branch + ", activityCnt_: " + activityCnt_ + ", activitiesOnStack_: " + activitiesOnStack_);

	BranchLogger.v("onActivityDestroyed, activity = " + activity + " branch: " + branch);
	BranchLogger.v("onActivityDestroyed, activity = " + activity + " branch: " + branch);
	// TODO: Consider adding a debug flag to control all lifecycle logging

Fix buggy activity count logic #1270

Fix buggy activity count logic #1270

Uh oh!

Conversation

gdeluna-branch commented May 23, 2025

Reference

Description

Testing Instructions

Risk Assessment [HIGH || MEDIUM || LOW]

Reviewer Checklist (To be checked off by the reviewer only)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI May 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 23, 2025

Choose a reason for hiding this comment

Uh oh!

matter-code-review bot commented May 23, 2025

Reference

Summary By MatterAI

🔄 What Changed

🔍 Impact of the Change

📁 Total Files Changed

🧪 Test Added

🔒Security Vulnerabilities

Testing Instructions

Risk Assessment LOW

Reviewer Checklist (To be checked off by the reviewer only)

Quality Recommendations

Sequence Diagram

Uh oh!

matter-code-review bot left a comment

Choose a reason for hiding this comment

Uh oh!

matter-code-review bot May 23, 2025

Choose a reason for hiding this comment

Uh oh!

matter-code-review bot May 23, 2025

Choose a reason for hiding this comment

Uh oh!

matter-code-review bot May 23, 2025

Choose a reason for hiding this comment

Uh oh!

matter-code-review bot May 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Risk Assessment [`HIGH` || `MEDIUM` || `LOW`]

Risk Assessment `LOW`