Fix app sandbox setup #3401

shubham1g5 · 2025-11-05T06:05:18Z

Product Description

Fixes https://dimagi.atlassian.net/browse/QA-8210

Technical Summary

We were not restoring the last app sandbox state correctly leading to this crash

I added more sandbox handling to reset to the initial state and could verify the issue is fixed.

I also have put this code behind the pending flags check as this evaluation can be expensive and doesn't need to happen at the momenet when pending tabs are not turned on

Labels and Review

Do we need to enhance the manual QA test coverage ? If yes, the "QA Note" label is set correctly
Does the PR introduce any major changes worth communicating ? If yes, the "Release Note" label is set and a "Release Note" is specified in PR description.
Risk label is set correctly
The set of people pinged as reviewers is appropriate for the level of risk of the change

coderabbitai · 2025-11-05T06:18:32Z

📝 Walkthrough

Walkthrough

The changes modify PersonalIdWorkHistoryViewModel.kt to add feature-flag-gated pending work history functionality. A new private method getPendingWorkHistory() extracts pending computation logic, while another method getWorkHistoryForAppRecord() generates work history from individual app records with sandbox isolation. The implementation reworks aggregation logic to preserve and restore app context when collecting work histories across multiple records, managing sandbox lifecycle around per-record extraction.

Sequence Diagram

sequenceDiagram
    participant VM as PersonalIdWorkHistoryViewModel
    participant FF as Feature Flag<br/>Checker
    participant AR as ApplicationRecord<br/>Iterator
    participant SB as Sandbox
    participant WH as PersonalIdWorkHistory<br/>Extractor

    VM->>FF: Check if WORK_HISTORY_PENDING_TAB<br/>enabled?
    
    alt Feature Flag Enabled
        VM->>VM: Collect earned work history<br/>from all app records
        VM->>AR: Iterate through app records
        
        loop For each app record
            AR->>SB: setupSandbox(record)
            SB->>WH: Extract work history
            WH-->>AR: Return PersonalIdWorkHistory list
            AR->>SB: teardownSandbox()
        end
        
        AR-->>VM: Return all earned histories
        VM->>VM: getPendingWorkHistory(earned)
        VM->>VM: Compute pending items
        VM->>VM: Post _pendingWorkHistory
    else Feature Flag Disabled
        VM->>VM: Skip pending logic
    end
    
    VM-->>VM: Expose computed work histories

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Sandbox lifecycle management: Verify correct setup/teardown sequencing around per-record extraction to prevent profile isolation issues
App context preservation and restoration: Confirm logic for saving current seated app and properly restoring it after processing multiple records
Feature flag conditional logic: Ensure pending computation only executes when flag is enabled and doesn't introduce side effects when disabled
Variable naming consistency: Validate that the renamed local variable for earned results is used correctly throughout the method

Possibly related PRs

Enables Work History and hides pending tab #3380: Related PR that enables the WORK_HISTORY flag and manages UI visibility for the pending tab feature, complementing this PR's backend implementation of pending work history computation.

Suggested reviewers

OrangeAndGreen
Jignesh-dimagi
jaypanchal-13

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Fix app sandbox setup' is vague and does not clearly convey the specific nature of the fix or the main change from the developer's perspective.	Consider a more specific title that indicates the actual problem being solved, such as 'Fix app sandbox state restoration when computing pending work history' or similar.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description check	✅ Passed	The PR description is mostly complete with a clear technical summary, links to relevant issues and crashes, and explanation of the fix and feature flag gating.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fixAppSandboxSetup

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ef8cad2 and 82edc36.

📒 Files selected for processing (1)

app/src/org/commcare/activities/connect/viewmodel/PersonalIdWorkHistoryViewModel.kt (3 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-05-08T11:08:18.530Z

Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.

Applied to files:

app/src/org/commcare/activities/connect/viewmodel/PersonalIdWorkHistoryViewModel.kt

🧬 Code graph analysis (1)

app/src/org/commcare/activities/connect/viewmodel/PersonalIdWorkHistoryViewModel.kt (2)

app/src/org/commcare/personalId/PersonalIdFeatureFlagChecker.kt (1)

isFeatureEnabled (20-29)

app/src/org/commcare/CommCareApp.java (1)

CommCareApp (58-473)

app/src/org/commcare/activities/connect/viewmodel/PersonalIdWorkHistoryViewModel.kt

Jignesh-dimagi · 2025-11-05T13:19:16Z

@shubham1g5 I think I have less context here but has question why its complicated to teardown, restore the sandbox. May be I will go through code more in details so its ok.

conroy-ricketts

LGTM - I don't have any concerns. I'd like another set of eyes though

conroy-ricketts · 2025-11-05T14:39:00Z

app/src/org/commcare/activities/connect/viewmodel/PersonalIdWorkHistoryViewModel.kt

+    private fun getWorkHistoryForAppRecord(record: ApplicationRecord): Iterable<PersonalIdWorkHistory> {
+        val commcareApp = CommCareApp(record)
+        try {
            commcareApp.setupSandbox()
-            val profile = commcareApp.initApplicationProfile();
-            profile.credentials.map { credential ->
+            val profile = commcareApp.initApplicationProfile()
+            return profile.credentials.map { credential ->
                PersonalIdWorkHistory().apply {
                    appId = record.applicationId
                    title = record.displayName ?: ""
                    level = credential.level
                }
            }
+        } finally {
+            commcareApp.teardownSandbox()
        }
    }


That's a very interesting use of try/finally that I haven't seen before

OrangeAndGreen

I think this all looks good, but just making sure I understand:
The issue is that we need to load the sandbox for each app that we have locally in order to check for pending work history, so when we're done we need to make sure to put the previously seated app back. Is that correct?

I'm curious as to why this would cause a crash specifically when trying to download/install a new app (as described by QA)?

shubham1g5 · 2025-11-06T03:51:08Z

The issue is that we need to load the sandbox for each app that we have locally in order to check for pending work history, so when we're done we need to make sure to put the previously seated app back. Is that correct?

Yes.

I'm curious as to why this would cause a crash specifically when trying to download/install a new app (as described by QA)?

This is the related stack trace for that, if we don't tear the sandbox and call setup on the same sandbox, it errors with this.

…hat we setup

shubham1g5 · 2025-11-06T08:48:06Z

why its complicated to teardown, restore the sandbox

@Jignesh-dimagi The compication is mostly that we don't parse HQ App xml files in DB and setting up sandbox initializes the file paths for thes to get parsed correctly and loaded into memory , due to these constraints we also don't support multi-login into more than one HQ app (you will have to logout to login into another one)

shubham1g5 requested review from Jignesh-dimagi, OrangeAndGreen, conroy-ricketts and jaypanchal-13 November 5, 2025 06:05

shubham1g5 added the skip-integration-tests Skip android tests. label Nov 5, 2025

coderabbitai bot reviewed Nov 5, 2025

View reviewed changes

app/src/org/commcare/activities/connect/viewmodel/PersonalIdWorkHistoryViewModel.kt Show resolved Hide resolved

shubham1g5 added 3 commits November 5, 2025 11:57

Safely restore app sandboxes after getting work history records

b393e12

Put the pending apps evaluation behind feature flag

a51a768

lint

f326af0

shubham1g5 force-pushed the fixAppSandboxSetup branch from 884b5f6 to f326af0 Compare November 5, 2025 06:27

conroy-ricketts reviewed Nov 5, 2025

View reviewed changes

OrangeAndGreen previously approved these changes Nov 5, 2025

View reviewed changes

move setupSandbox out of try to make sure we only close the sandbox t…

c87ff40

…hat we setup

shubham1g5 dismissed OrangeAndGreen’s stale review via c87ff40 November 6, 2025 07:20

shubham1g5 requested review from OrangeAndGreen and conroy-ricketts November 6, 2025 08:45

OrangeAndGreen approved these changes Nov 6, 2025

View reviewed changes

shubham1g5 merged commit d3267ee into master Nov 6, 2025
9 checks passed

shubham1g5 deleted the fixAppSandboxSetup branch November 6, 2025 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix app sandbox setup #3401

Fix app sandbox setup #3401

Uh oh!

shubham1g5 commented Nov 5, 2025

Uh oh!

coderabbitai bot commented Nov 5, 2025

Walkthrough

Sequence Diagram

Estimated code review effort

Possibly related PRs

Suggested reviewers

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Jignesh-dimagi commented Nov 5, 2025

Uh oh!

conroy-ricketts left a comment

Uh oh!

conroy-ricketts Nov 5, 2025

Uh oh!

OrangeAndGreen left a comment

Uh oh!

shubham1g5 commented Nov 6, 2025

Uh oh!

shubham1g5 commented Nov 6, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Fix app sandbox setup #3401

Fix app sandbox setup #3401

Uh oh!

Conversation

shubham1g5 commented Nov 5, 2025

Product Description

Technical Summary

Labels and Review

Uh oh!

coderabbitai bot commented Nov 5, 2025

Walkthrough

Sequence Diagram

Estimated code review effort

Possibly related PRs

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jignesh-dimagi commented Nov 5, 2025

Uh oh!

conroy-ricketts left a comment

Choose a reason for hiding this comment

Uh oh!

conroy-ricketts Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

OrangeAndGreen left a comment

Choose a reason for hiding this comment

Uh oh!

shubham1g5 commented Nov 6, 2025

Uh oh!

shubham1g5 commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shubham1g5 commented Nov 6, 2025 •

edited

Loading