CCCT-2441 Silent App Launch For Job Intro And Download Screens#3765
CCCT-2441 Silent App Launch For Job Intro And Download Screens#3765conroy-ricketts wants to merge 9 commits into
Conversation
[AI] Migrated the job-intro and downloading screens to the silent Connect launch path and removed the now-dead IS_LAUNCH_FROM_CONNECT flag and its LoginActivity/DispatchActivity auto-login branches. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
[AI] Guarded the Connect app-launch controller against a destroyed fragment view so an in-flight launch fired from an async callback can no longer crash with IllegalStateException. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
[AI] Collapsed the redundant nested seated-app login check into a single condition and trimmed the login-engine doc to describe only the current Connect launch behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
[AI] Made the job-intro and download launch surfaces drop themselves from the Connect back stack after a successful launch, deferring the pop until Home is in front so no blank frame flashes on the way to Home. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ining-connect-pages
[AI] Fixed a crash when launching freshly-installed Connect apps back-to-back: the already-logged-in shortcut now confirms the active session belongs to the seated app, so an install that re-seats a different app while a prior session is alive no longer skips re-login. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
[AI] Stopped offering endless retries on a failing Connect app launch: after the worker retries twice, the dialog now shows a dismiss-only "check your connection and try again later" message instead of another Retry prompt. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-2441-new-login-path-for-intro-and-download
[AI] Added QA notes for the job-intro and post-download Connect launch surfaces, back-to-back launch stability, and the after-2-retries persistent-error dialog. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Suggested Review Order
|
📝 WalkthroughWalkthroughThis PR refactors the Connect app launch flow by removing legacy "launched from Connect" state tracking from Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 docstrings
🧪 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 |
conroy-ricketts
requested review from
a team,
Jignesh-dimagi,
OrangeAndGreen and
shubham1g5
and removed request for
a team
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3765 +/- ##
============================================
- Coverage 25.87% 25.86% -0.02%
Complexity 4389 4389
============================================
Files 950 950
Lines 57186 57189 +3
Branches 6810 6809 -1
============================================
- Hits 14799 14792 -7
- Misses 40558 40569 +11
+ Partials 1829 1828 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
[FIXED] The code changes look great but I'm seeing a functionality issue. Here are steps to reproduce:
- Go to the opportunity list and press Resume on an opp where the learn/deliver app needs to be downloaded
- Wait while the Download page appears, the app downloads, and logs in
- Once on App Home, navigate back (i.e. swipe)
The code should navigate to the opportunity list, but instead it navigates to the Download page (showing complete progress)
|
[FIXED] I'm also getting a repeatable crash that seems like it might be related to these changes. Steps:
Here's the exception: |
| if (fragment.view == null) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Does this achive something else than fragment.isAdded() check ? If not, it would be nice to be consistent here and use the isAdded() check itsef.
| .buildHomeLaunchIntent(activity) | ||
| .addFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP), | ||
| ) | ||
| if (target.popLaunchingScreenOnSuccess) { |
There was a problem hiding this comment.
Should this happen before the call to startActivity, given we are adding a lifecycle listener here and doing it after startActivity runs the risk of listener getting registered after the lifecycle change.
| launchDialog = null | ||
| } | ||
|
|
||
| private fun popLaunchingScreenOnceHidden() { |
There was a problem hiding this comment.
this creates a pre-condition for this class that all callers implement a NavHostFragment , think we should validate this at constructor level and throw an IllegalStateEx if there is noNavHostFragment .
| boolean appInstalled = AppUtils.isAppInstalled(appId); | ||
| if (appInstalled) { | ||
| new ConnectAppLaunchController(ConnectJobIntroFragment.this).launchApp(appId, true); | ||
| new ConnectAppLaunchController(ConnectJobIntroFragment.this).launchApp(appId, true, true); |
There was a problem hiding this comment.
Reg. back behaviour here, does The Job Intro page do not appear once user goes into the app ? If so, is that because of an underlying opportunity state change that happens when we launch the App from this page ?
| val sessionUuid = instance.session.userKeyRecordUUID ?: return false | ||
| return instance | ||
| .getAppStorage(UserKeyRecord::class.java) | ||
| .getRecordsForValue(UserKeyRecord.META_SANDBOX_ID, sessionUuid) | ||
| .isNotEmpty() | ||
| } |
There was a problem hiding this comment.
this check should strictly happen with the Currently logged into Personal ID user id to establish that it's the current personal ID user that's logged into the app and not just any user.
| appId: String, | ||
| isLearning: Boolean, | ||
| ) = launch(LaunchTarget(appId, isLearning)) | ||
| popLaunchingScreenOnSuccess: Boolean = false, |
There was a problem hiding this comment.
I don't like that the controller here is manipulating the fragment stack and think it's extending it's responsibilities more than it need to, instead the calling fragment should handle it's own state i.e. it should finish itself on Resume when there is an undelying state change after pressing back that makes that fragment no longer relevant.
There was a problem hiding this comment.
Yes, I agree with that comment. I think the best approach is to have ConnectAppLaunchController -> handleLaunchOutcome inform the calling fragment of the final outcome, and let that fragment manage its own lifecycle. Maybe we can use an abstract method in BaseConnectFragment to handle this, as the controller is currently overloaded with lifecycle and fragment management responsibilities.
| appId: String, | ||
| isLearning: Boolean, | ||
| ) = launch(LaunchTarget(appId, isLearning)) | ||
| popLaunchingScreenOnSuccess: Boolean = false, |
There was a problem hiding this comment.
Yes, I agree with that comment. I think the best approach is to have ConnectAppLaunchController -> handleLaunchOutcome inform the calling fragment of the final outcome, and let that fragment manage its own lifecycle. Maybe we can use an abstract method in BaseConnectFragment to handle this, as the controller is currently overloaded with lifecycle and fragment management responsibilities.
|
@conroy-ricketts |
|
Noting the two issues I reported are resolved with the latest code |
4 participants
CCCT-2441
Product Description
Completes the rollout of the silent Connect app-launch path to its last two surfaces: starting a job from the opportunity intro screen, and the launch that runs right after an app finishes downloading/installing. These now open the app directly behind the single progress dialog — no
LoginActivity/SeatAppActivityflashing through — and backing out of the app returns to the originating Connect screen rather than the transient setup screen. After repeated launch failures (two retries), the failure dialog now shows a dismiss-only "try again later" message instead of offering endless retries.Here's a video of the retry prompt dialog:
Screen_Recording_20260617_142435_CommCare.Debug.mp4
Here's a video of the download and job intro pages. Note that I show two different cases: 1) the app was pre-installed and thus the download page is skipped, 2) the app was not pre-installed and we show the download page:
Screen_Recording_20260617_133207_CommCare.Debug.mp4
Technical Summary
ConnectJobIntroFragmentandConnectDownloadingFragmentontoConnectAppLaunchController, the shared silent-launch orchestration already used by the other Connect surfaces.IS_LAUNCH_FROM_CONNECTflag, theappLaunchedFromConnectauto-login branch inLoginActivity, and theCONNECT_MANAGED_LOGIN/redirectToConnectHomeback-out path inDispatchActivity— none have remaining writers once these two surfaces stop using the legacy path.LaunchOutcomeRouterescalates a retryable failure to a dismiss-only message onceMAX_LAUNCH_ATTEMPTS(3) is reached.Safety Assurance
Safety story
What gives me confidence:
NoSuchElementExceptioncrash before the session-shortcut fix and confirmed it no longer occurs after.LaunchOutcomeRouterandConnectAppLauncherunit tests, and the change is scoped to the Connect launch path.Risks to review:
LoginActivity/DispatchActivityby deleting theappLaunchedFromConnectauto-login andCONNECT_MANAGED_LOGIN/redirectToConnectHomeback-out paths. This is behavior-preserving (those paths were only reachable via the now-removed flag) but worth a careful read.Automated test coverage
LaunchOutcomeRouterTestcovers the retry → escalation threshold (below the limit prompts retry; at the limit shows the persistent error). The existingConnectAppLauncherTestcovers the launcher's seat / login / outcome seams. The fragment-side dialog and lifecycle code is not unit-covered.