Spec: App Seating in Opportunity Home#3776
Conversation
…pp seating and app-related home page functionality.
OrangeAndGreen
requested review from
Jignesh-dimagi,
conroy-ricketts and
shubham1g5
📝 WalkthroughWalkthroughThe PR adds a design specification for Opportunity Home activity composition. It defines an activity that starts without a session, later attaches a session to enable session-dependent behavior, and describes delegate-based extraction for expiration, sync, app update prompts, session launch checks, and crash recovery. It also describes rebasing Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
docs/superpowers/specs/2026-06-22-opportunity-home-composition-design.md (4)
72-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language specifier to class definition code block.
The fenced code block starting at line 74 is missing a language identifier.
- ``` + ```kotlin class OpportunityHomeActivity : BaseDrawerActivity<OpportunityHomeActivity>() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-06-22-opportunity-home-composition-design.md` around lines 72 - 111, The fenced code block under OpportunityHomeActivity is missing a language identifier, so update that code fence to explicitly mark it as Kotlin. Keep the class definition content unchanged, and ensure the opening fence for the OpportunityHomeActivity snippet includes the kotlin specifier so the docs renderer can apply proper syntax highlighting.
53-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language specifier to code block.
The fenced code block at line 55 is missing a language identifier.
<details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/superpowers/specs/2026-06-22-opportunity-home-composition-design.md
around lines 53 - 58, Add a language specifier to the fenced example in the
session-dependent delegates section so the code block renders with syntax
highlighting; update the markdown snippet containing attachSession and
detachSession to include the appropriate language tag on the fence and keep the
rest of the content unchanged.</details> <!-- cr-comment:v1:088dd2afc80c10b715baf73f --> --- `145-149`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _⚡ Quick win_ **Format the 9-step pipeline as a numbered list for clarity.** The parenthetical enumeration of `doLoginLaunchChecksInOrder` at line 148 is dense and error-prone during implementation. Extract it to a numbered sub-list under item 4. ```diff - 4. **Preserve `doLoginLaunchChecksInOrder` semantics.** This method is a strict 9-step ordered pipeline (demo-mode early return → update-info form → form restoration → session restoration → post-update sync → pending FCM sync → update prompt → PIN check → drift check). When `SessionLaunchDelegate` and `AppUpdateDelegate` absorb pieces of it, the ordering and early-return behavior must be preserved, or demo users could start receiving prompts they currently skip. + 4. **Preserve `doLoginLaunchChecksInOrder` semantics.** This method is a strict 9-step ordered pipeline. When `SessionLaunchDelegate` and `AppUpdateDelegate` absorb pieces of it, the ordering and early-return behavior must be preserved, or demo users could start receiving prompts they currently skip. + + The current order is: + 1. Demo-mode early return + 2. Update-info form + 3. Form restoration + 4. Session restoration + 5. Post-update sync + 6. Pending FCM sync + 7. Update prompt + 8. PIN check + 9. Drift check🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-06-22-opportunity-home-composition-design.md` around lines 145 - 149, The item describing doLoginLaunchChecksInOrder is too dense and should be reformatted for clarity; rewrite that 9-step pipeline as a numbered sub-list under the preserve-doLoginLaunchChecksInOrder section so the ordered early-return semantics are easy to follow during implementation. Keep the existing meaning intact while making the steps explicit, and retain the references to doLoginLaunchChecksInOrder, SessionLaunchDelegate, AppUpdateDelegate, and StandardHomeActivity so the implementation can preserve the same behavior.
28-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix parallel structure in non-goals list.
Line 34 breaks parallelism: "Designing entry points, routing through
DispatchActivity, or persistence of 'last accessed opportunity'." Change "persistence" to "persisting" to match the gerund form of the other items.-- Designing entry points, routing through `DispatchActivity`, or persistence of "last accessed opportunity". +- Designing entry points, routing through `DispatchActivity`, or persisting "last accessed opportunity".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-06-22-opportunity-home-composition-design.md` around lines 28 - 36, The Non-goals list needs parallel grammar cleanup in the spec. In the bullet under Opportunity Home composition, change the noun form in “persistence of last accessed opportunity” to the gerund form so it matches the other “Designing…” items. Keep the wording aligned with the surrounding list entries and verify the bullet remains a single parallel phrase in the same section.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/superpowers/specs/2026-06-22-opportunity-home-composition-design.md`:
- Around line 72-111: The fenced code block under OpportunityHomeActivity is
missing a language identifier, so update that code fence to explicitly mark it
as Kotlin. Keep the class definition content unchanged, and ensure the opening
fence for the OpportunityHomeActivity snippet includes the kotlin specifier so
the docs renderer can apply proper syntax highlighting.
- Around line 53-58: Add a language specifier to the fenced example in the
session-dependent delegates section so the code block renders with syntax
highlighting; update the markdown snippet containing attachSession and
detachSession to include the appropriate language tag on the fence and keep the
rest of the content unchanged.
- Around line 145-149: The item describing doLoginLaunchChecksInOrder is too
dense and should be reformatted for clarity; rewrite that 9-step pipeline as a
numbered sub-list under the preserve-doLoginLaunchChecksInOrder section so the
ordered early-return semantics are easy to follow during implementation. Keep
the existing meaning intact while making the steps explicit, and retain the
references to doLoginLaunchChecksInOrder, SessionLaunchDelegate,
AppUpdateDelegate, and StandardHomeActivity so the implementation can preserve
the same behavior.
- Around line 28-36: The Non-goals list needs parallel grammar cleanup in the
spec. In the bullet under Opportunity Home composition, change the noun form in
“persistence of last accessed opportunity” to the gerund form so it matches the
other “Designing…” items. Keep the wording aligned with the surrounding list
entries and verify the bullet remains a single parallel phrase in the same
section.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 63f99f24-b8d2-486a-abb9-aa1ca7542283
📒 Files selected for processing (1)
docs/superpowers/specs/2026-06-22-opportunity-home-composition-design.md
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3776 +/- ##
============================================
+ Coverage 25.96% 26.01% +0.05%
- Complexity 4408 4418 +10
============================================
Files 953 952 -1
Lines 57366 57364 -2
Branches 6824 6828 +4
============================================
+ Hits 14893 14924 +31
+ Misses 40640 40602 -38
- Partials 1833 1838 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
LGTM, would like another set of eyes before creating new tickets
| - Designing the inline silent app-login flow (in-flight work). This spec assumes that capability is available and exposes a session to the activity when it completes. | ||
| - Implementing `WithUIController` on the new activity. | ||
|
|
||
| ## Approach |
There was a problem hiding this comment.
Think one important goal/north-star for this design should be that if in future if we need to add or change a feature to home screen, we should not need to make the changes in both activities/screens.
Accordingly, It would be great to define both Inheritance and Composition Approaches in this spec and try to evaluate those two with the app architecutre goal above to decide on what approach makes better sense instead of going by a rule or feeling to favor composition approach (I know I am going against the earlier discussion during co-working hour but just want us to make a more informed decision here)
| detachSession() | ||
| ``` | ||
|
|
||
| When attached, the delegate has a live session and operates as the existing chain does today. When not attached, every public entry point is a clean no-op (callers receive a documented "no session" result; nothing throws `SessionUnavailableException`). |
There was a problem hiding this comment.
curious why we decided against the current throw SessionUnavailableException here ? Probably the answer here is hidden in how and when attachSession and detachSession is called but is not very clear based on spec.
There was a problem hiding this comment.
Throwing that exception still happens when a delegate encounters an error while a session is attached, but when the delegates shouldn't do anything when there isn't a session detached. This allows them to be wired to lifecycle callbacks, but only active when the owning Activity wants them to be (always active for traditional pages, but optionally for Opp Home).
| | `SyncDelegate` | `SyncCapableCommCareActivity` | Owns `FormAndDataSyncer`, sync state, and the `PullTaskResultReceiver` implementation. Exposes `sendFormsOrSync()` and related entry points. The host activity remains the `CommCareTaskConnector` (see below). | | ||
| | `AppUpdateDelegate` | `HomeScreenBaseActivity` (`AppUpdateController` and drift checks) | App update prompts and drift warnings. **Session-independent** (see below): constructed once and registered on the host lifecycle like `CrashRecoveryDelegate`; only the surfacing of prompts is gated on a session existing. | | ||
| | `SessionLaunchDelegate` | `HomeScreenBaseActivity.doLoginLaunchChecksInOrder`, `SessionNavigator` usage | Form restoration, "Start" → form entry navigation, post-login launch checks, and form-result handling. Must preserve the ordering and early-return semantics of `doLoginLaunchChecksInOrder` (see risks). | | ||
| | `CrashRecoveryDelegate` | `HomeScreenBaseActivity` instance-state and crash-data registration | Persists instance state across recreation. Active regardless of session. | |
There was a problem hiding this comment.
what does instance-state refers to here and how that is only specific to CrashRecoveryDelegate ?
There was a problem hiding this comment.
This is referring to Android's saved instance state, the Bundle accessed via onSaveInstanceState and onCreate/onCreateView.
It isn't specific to CrashRecoveryDelegate, but the spec was assigning behavior to that delegate related to the saving/loading of several variables kept in the state bundle. But looking at the flags to be managed, they seem more appropriate to be handled by the SessionLaunchDelegate. Spec updated to make that change and be more clear. 05ea7f6
There was a problem hiding this comment.
Think all delegates will optionally need to have their respective methods to save and restore the Instance state they own (instead of assigning this responsibility to a single delegate). Additionally controller class should implement these methods as well and should call the corresponding delegates methods and also restore it's own state.
Looking at the current vars -
outState.putBoolean(WAS_EXTERNAL_KEY, wasExternal);
outState.putBoolean(EXTRA_CONSUMED_KEY, loginExtraWasConsumed);
outState.putBoolean(
KEY_PENDING_ENDPOINT_NAV_AFTER_SYNC,
pendingEndpointNavigationAfterSync
);
I anticipate that each of the 3 variables here would probably go to different delegates or live in the conroller class.
There was a problem hiding this comment.
Ah, I like moving those three variables to the Coordinator class now that we have one. 3e35c00
I also did another check to verify, and there's one more instance-state variable currently managed by a delegate: KEY_LAST_ICON_TRIGGER managed by SyncDelegate. I think that one makes sense to leave where it is since it's only relevant when a session is attached and is specific to sync functionality (computing sync state in onCreateSessionSafe). I added some text clarifying the usage of the 4 variables and their different ownership. 889f187
…elegate. Clarified use of Android saved instance state.
…onality from home activities.
|
This is really good documentation. I have left a few questions and comments, but they seem to be mostly for my own understanding :). |
|
|
||
| Five behaviors are extracted from the existing inheritance chain into delegates, and a single `HomeActivityCoordinator` owns those delegates and exposes the home screen's capabilities as host-agnostic actions (see [`HomeActivityCoordinator`](#homeactivitycoordinator) below). A host activity holds one coordinator rather than five delegates directly. The five extracted delegates are: | ||
|
|
||
| | Delegate | Source today | Responsibilities | |
There was a problem hiding this comment.
As stated above, OpportunityHomeActivity can have the following states also: Learning state, learn app not installed, seated, or logged in. How should we handle the login process here? Do we need a dedicated delegate to handle it, or can we re-use ConnectAppLaunchController?
|
|
||
| | Delegate | Source today | Responsibilities | | ||
| |---|---|---| | ||
| | `SessionExpirationDelegate` | `SessionAwareHelper.onResumeHelper`, `onActivityResultHelper` | On resume / activity-result, check session expiration. Exposes a listener interface that the host activity registers; reports session-lost events via that listener rather than redirecting to login. | |
There was a problem hiding this comment.
HomeActivityCoordinator should be the listener instead of the host activity, as it is the ultimate consumer of these events?
There was a problem hiding this comment.
That's correct, I added some text clarifying that the Coordinator will register for life cycle callbacks. ab8d11c
|
|
||
| The coordinator's `onCreate(...)` is what registers the five delegates as lifecycle observers and unconditionally restores `SessionLaunchDelegate`'s instance state; the activity no longer enumerates delegates itself. `onSessionAvailable(...)` is invoked from two places inside the activity: the completion path for inline silent login (driving the situation 2 → 3 and 4 → 5 transitions), and an `onResume` check that calls it whenever a session exists but no session is currently attached. `onSessionLost()` is invoked from two distinct mechanisms, which today reach the chain through different paths: | ||
|
|
||
| - **Manual logout** — `userTriggeredLogout()` → `CommCareApplication.closeUserSession()` (no broadcast). |
There was a problem hiding this comment.
Will this support user logout? If so, what should the state of OpportunityHomeActivity be? Specifically, do we ever navigate away from this activity once a user has an active opportunity in progress, considering this becomes our app's new home screen?
There was a problem hiding this comment.
I think in the latest design we aren't providing a manual logout option for Connect apps. Sessions can expire due to inactivity (in which case the page will automatically start a new session when the user returns), or they can be closed when the user changes to a different app (i.e. reviews learning, goes to a different opp, or loads a traditional app).
|
|
||
| `CrashRecoveryDelegate` and `AppUpdateDelegate` do not implement `attachSession`/`detachSession`. `CrashRecoveryDelegate` is session-independent by nature. `AppUpdateDelegate` is treated as session-independent because the underlying `AppUpdateController` cannot be safely reconstructed per session: `AppUpdateControllerFactory.create(...)` needs only a `Context` and a callback (not a session), and `register()` attaches an `InstallStateUpdatedListener` to the Google Play Core `AppUpdateManager` and kicks off an async info fetch. Reconstructing on each `attachSession` would leak listeners (duplicate callbacks), orphan any in-progress download from the Play Store state machine, and re-fetch update info needlessly. App-binary updates are not opportunity- or seated-app-scoped, so the delegate is constructed once on the host lifecycle (register on `onResume`, unregister on `onDestroy`); only the *surfacing* of an update prompt is gated on a session existing, re-evaluating `shouldShowInAppUpdate()` at prompt time rather than at construction. | ||
|
|
||
| ### `HomeActivityCoordinator` |
There was a problem hiding this comment.
I know designing the state model that picks which UI to show inside OpportunityHome is a non-goal for this document (as it is handled separately). However, I am just trying to understand if HomeActivityCoordinator will be the one controlling the UI via HomeActivityHost based on the opportunity state (e.g., learn not downloaded, learn not completed, delivery not downloaded), or if it will use another coordinator or controller for that purpose?
There was a problem hiding this comment.
Good point to bring up, and it seems worth addressing a bit more in this spec even if we leave some of the details pending for decisions at implementation time.
I introduced an OpportunityHomeStateController class to handle state resolution for Opp Home, including fragment selection as well as auto-login actions and error handling (i.e. login fails due to bad network). There's also a new section addressing some of the plans for how that logic will work. 06f45f0
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
- One other thing that would be good to preclude this work is documenting the current behaviour as it is for HomeActivity with unit tests so that if something functional changes as part of the refactor, tests are better equipped to flag that.
|
|
||
| | Delegate | Source today | Responsibilities | | ||
| |---|---|---| | ||
| | `SessionExpirationDelegate` | `SessionAwareHelper.onResumeHelper`, `onActivityResultHelper` | On resume / activity-result, check session expiration. Exposes a listener interface that the host activity registers; reports session-lost events via that listener rather than redirecting to login. | |
There was a problem hiding this comment.
the onActivityResult should be distribitued across delegates and HomeActivityCoordinator similar to the methods around storing/restoring instance-state
There was a problem hiding this comment.
I think this has been cleared up in other changes but let me know if additional clarification should be added.
The spec now indicates that onActivityResult will be forwarded to the Coordinator, which then calls delegates that it knows are relevant (rather than generically calling all delegates).
Instance-state functionality is handled directly by delegates that require it, and doesn't require pass-through from the Activity or Coordinator.
| |---|---|---| | ||
| | `SessionExpirationDelegate` | `SessionAwareHelper.onResumeHelper`, `onActivityResultHelper` | On resume / activity-result, check session expiration. Exposes a listener interface that the host activity registers; reports session-lost events via that listener rather than redirecting to login. | | ||
| | `SyncDelegate` | `SyncCapableCommCareActivity` | Owns `FormAndDataSyncer`, sync state, and the `PullTaskResultReceiver` implementation. Exposes `sendFormsOrSync()` and related entry points. The host activity remains the `CommCareTaskConnector` (see below). | | ||
| | `AppUpdateDelegate` | `HomeScreenBaseActivity` (`AppUpdateController` and drift checks) | App update prompts and drift warnings. **Session-independent** (see below): constructed once and registered on the host lifecycle like `CrashRecoveryDelegate`; only the surfacing of prompts is gated on a session existing. | |
There was a problem hiding this comment.
drift checks are quite different than app update and think are best separated in different delegates Or we can keep the drift check it in the coordinator given it's a small check.
There was a problem hiding this comment.
Ah, nice callout. I modified the spec to have the Coordinator handle the logic for drift checks since it's pretty simple and already mostly contained by DriftHelper. 282d17f
| | `SessionExpirationDelegate` | `SessionAwareHelper.onResumeHelper`, `onActivityResultHelper` | On resume / activity-result, check session expiration. Exposes a listener interface that the host activity registers; reports session-lost events via that listener rather than redirecting to login. | | ||
| | `SyncDelegate` | `SyncCapableCommCareActivity` | Owns `FormAndDataSyncer`, sync state, and the `PullTaskResultReceiver` implementation. Exposes `sendFormsOrSync()` and related entry points. The host activity remains the `CommCareTaskConnector` (see below). | | ||
| | `AppUpdateDelegate` | `HomeScreenBaseActivity` (`AppUpdateController` and drift checks) | App update prompts and drift warnings. **Session-independent** (see below): constructed once and registered on the host lifecycle like `CrashRecoveryDelegate`; only the surfacing of prompts is gated on a session existing. | | ||
| | `SessionLaunchDelegate` | `HomeScreenBaseActivity.doLoginLaunchChecksInOrder`, `SessionNavigator` usage, `onSaveInstanceState`/`loadInstanceState` | Form restoration, "Start" → form entry navigation, the step bodies it contributes to the launch pipeline (update-info form, form restoration, session restoration), and form-result handling. The *ordering* and early-return semantics of `doLoginLaunchChecksInOrder` are owned by the coordinator's `runLaunchChecks` (see [`HomeActivityCoordinator`](#homeactivitycoordinator)), not by this delegate. Also owns the launch/nav instance-state keys — `WAS_EXTERNAL_KEY` (`wasExternal`), `EXTRA_CONSUMED_KEY` (`loginExtraWasConsumed`), and `KEY_PENDING_ENDPOINT_NAV_AFTER_SYNC` (`pendingEndpointNavigationAfterSync`) — persisting them across recreation (see note below on save/restore vs. attach). | |
There was a problem hiding this comment.
A bit confsed what parts of doLoginLaunchChecksInOrder is owned by the SessionLaunchDelegate given the spec states that The ordering and early-return semantics of doLoginLaunchChecksInOrder are owned by the coordinator's runLaunchChecks (
There was a problem hiding this comment.
I think this is better clarified now after several changes to the spec, so didn't make any additional changes.
The idea now is that the logic that used to live in doLoginLaunchChecksInOrder now lives in the Coordinator's runLaunchChecks function, which calls into delegates in the correct order for the specific checks to be performed. SessionLaunchDelegate owns three of the checks but other delegates own the others.
…r three, and reasoning.
… OpportunityHomeStateController.
Good idea and something Clayton emphasized as well. I had Claude do an analysis and indeed the existing unit testing for home screen functionality is pretty sparse. The existing testing primarily goes through the page incidentally while testing other functionality. 7f2c2fe I added a section at the end of the doc detailing the existing state of testing and recommended pre-refactor tests to add. I'm thinking we should create a ticket to add that testing in so we can get it prioritized for next sprint before taking on the main refactoring work. @shubham1g5 Agreed? |
OrangeAndGreen
requested review from
Jignesh-dimagi,
conroy-ricketts and
shubham1g5
|
@OrangeAndGreen Hey, I am afraid that this spec has expanded a lot in verbosity and I am finding it quite impractical to go over it. Would you be willing to take a pass on it to remove any un-needed details, and try to concise it and focus on changes rather than existing behaviour. I think the spec is very heavy as well on code references and it would be nice to try to simplify the text and make it more read-friendly. I understand that this might just be the nature of this spec as well and if you disagree with my comment, I can take a look as it is on it. |
4 participants
https://dimagi.atlassian.net/browse/CCCT-2522
Product Description
No code changes
Technical Summary
Claude tech spec for a refactor to support optional app seating in the new Opportunity Home activity, so that it can be run before the associated learn and deliver apps are downloaded and installed, and then provide "standard app home" functionality once the relevant apps are downloaded, seated, and silently logged in.
The refactor involves extracting several delegates that the existing framework can rely on (without behavioral changes) while the new activity can optionally use.