CCCT-2438 Extract Post-Login Routing And Inline App Seating

[conroy-ricketts]

CCCT-2438

Technical Summary

Phase 2 of the login/Connect decoupling (CCCT-2164 plan), stacked on the CCCT-2437-headless-login-engine branch.

• Introduces PostLoginRouter / PostLoginDestination: a pure, Android-free mapping from a LoginResult + LaunchContext to a navigation destination. DispatchActivity's LOGIN_USER result handler now delegates to it instead of unpacking intent extras inline.
• Extracts app seating out of SeatAppActivity's Thread/Handler into AppSeater, a lifecycle-scoped coroutine wrapper with injectable record-lookup / seat / dispatcher; SeatAppActivity becomes a thin UI host.

Safety Assurance

Safety story

What gives me confidence:

• I ran the login → home flow on an emulator/device and confirmed the app still seats and reaches the home screen.
• New unit coverage for the extracted logic: PostLoginRouter mapping (including negative cases) and AppSeater success / not-found / corrupted / exception paths.
• The diff is a contained restructure of the login → seat → dispatch path, with no schema or data-model changes.

Risks to review:

• The DispatchActivity routing integration and the SeatAppActivity host are exercised manually, not by automated tests.
• SeatAppActivity drops the old savedInstanceState (KEY_IN_PROGRESS) rotation guard; seating now relies on lifecycleScope cancellation, so re-seating behaviour on config change / process recreation is worth a look.
• This is an incremental phase: PostLoginRouter does not yet own the full Connect-vs-home routing decision (that still lives in dispatch()), so today it maps only success → Home and failure → TerminalFailure.

Automated test coverage

• PostLoginRouterTest — success routes to Home carrying login mode and launch context; asserts redirectToConnectOpportunityInfo does not leak into the Home destination; each LoginError maps to TerminalFailure.
• AppSeaterTest — missing record → APP_NOT_FOUND without seating; ready state → Success; corrupted state → CORRUPTED; thrown exceptions from both seatApp and recordLookup propagate (fail-fast); emits Seating progress.

[conroy-ricketts]

Suggested Review Order

• app/src/org/commcare/login/PostLoginDestination.kt — the output contract (sealed Home / TerminalFailure); read first.
• app/src/org/commcare/login/PostLoginRouter.kt — pure mapping that produces a destination from LoginResult + LaunchContext.
• app/unit-tests/src/org/commcare/login/PostLoginRouterTest.kt — documents the routing behaviour, including what must not leak into Home.
• app/src/org/commcare/login/AppSeater.kt — extracted seating core: injectable deps, coroutine wrapper, exception → SEAT_ERROR guard.
• app/unit-tests/src/org/commcare/login/AppSeaterTest.kt — covers the seating outcomes incl. the new exception paths.
• app/src/org/commcare/activities/SeatAppActivity.java — now a thin host consuming AppSeater; note the dropped KEY_IN_PROGRESS guard.
• app/src/org/commcare/activities/DispatchActivity.java — integrates PostLoginRouter at the LOGIN_USER result boundary; read last.

[conroy-ricketts]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 25.80%. Comparing base (4bc6b31) to head (d6eed61).
⚠️ Report is 6 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3743      +/-   ##
============================================
+ Coverage     25.55%   25.80%   +0.25%     
+ Complexity     4368     4364       -4     
============================================
  Files           950      947       -3     
  Lines         57646    56993     -653     
  Branches       6894     6782     -112     
============================================
- Hits          14732    14709      -23     
+ Misses        41088    40455     -633     
- Partials       1826     1829       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR introduces a post-login routing abstraction and refactors two CommCare activities to use it. It adds PostLoginRouter to centralize navigation decisions based on LoginResult and context flags, returning either a Home destination with login state or a TerminalFailure with error. It also extracts app seating into a reusable Kotlin component (AppSeater) that handles app initialization on a lifecycle-aware coroutine scope with injectable dependencies. SeatAppActivity is refactored to delegate to AppSeater and remove internal threading, while DispatchActivity is refactored to route post-login outcomes through PostLoginRouter instead of managing state directly. Tests cover routing logic for success and failure scenarios, and app seating for missing records, ready/corrupted states, and exception cases.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

skip-integration-tests

Suggested reviewers

• OrangeAndGreen

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main changes: extracting post-login routing logic and moving app seating into a dedicated component, matching the core refactoring work in this PR.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The PR description covers all required template sections: product description omitted as appropriate for non-visible changes, technical summary with ticket link and design rationale provided, safety story with confidence factors and risks outlined, automated test coverage documented, and labels/review guidance included.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch CCCT-2438-login-routing-extraction-and-inline-app-seating

---

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

[coderabbitai]

🧹 Nitpick comments (3)

app/src/org/commcare/activities/SeatAppActivity.java (2)

33-41: 💤 Low value

Remove the in-code comment.

Per the repository convention, in-code comments should not be added unless explicitly requested. The result-mapping logic itself is correct.

♻️ Proposed change

 private void finishWithResult(SeatResult result) {
-    // A corrupted seat still returns RESULT_OK; DispatchActivity detects
-    // STATE_CORRUPTED and routes to recovery.
     boolean treatAsSeated = result instanceof SeatResult.Success
             || (result instanceof SeatResult.Failed
             && ((SeatResult.Failed)result).getReason() == SeatFailure.CORRUPTED);
     setResult(treatAsSeated ? RESULT_OK : RESULT_CANCELED, new Intent(getIntent()));
     finish();
 }

As per coding guidelines: "Do not add any in-code comments unless explicitly requested".

🤖 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 `@app/src/org/commcare/activities/SeatAppActivity.java` around lines 33 - 41,
Remove the in-code explanatory comment inside the finishWithResult method:
delete the two-line comment beginning with "A corrupted seat still returns
RESULT_OK; DispatchActivity detects STATE_CORRUPTED and routes to recovery." so
the method contains only the result-mapping logic using SeatResult,
SeatResult.Success, SeatResult.Failed, SeatFailure.CORRUPTED and the
setResult/finish calls; leave the logic unchanged.

---

29-30: ⚡ Quick win

Guard against a missing KEY_APP_TO_SEAT extra in SeatAppActivity

getIntent().getStringExtra(KEY_APP_TO_SEAT) can be null; passing that into new AppSeater().start(this, appId, ...) can crash if appId is treated as non-null. Java launchers currently set the extra, but this defensive check prevents unexpected NPEs.

🛡️ Proposed guard

 String appId = getIntent().getStringExtra(KEY_APP_TO_SEAT);
+if (appId == null) {
+    setResult(RESULT_CANCELED, new Intent(getIntent()));
+    finish();
+    return;
+}
 new AppSeater().start(this, appId, progress -> {}, this::finishWithResult);

🤖 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 `@app/src/org/commcare/activities/SeatAppActivity.java` around lines 29 - 30,
SeatAppActivity currently calls getIntent().getStringExtra(KEY_APP_TO_SEAT) and
passes the possibly-null appId into new AppSeater().start(...), which can lead
to NPEs; update SeatAppActivity to guard against a missing KEY_APP_TO_SEAT by
checking if appId is null (from getIntent().getStringExtra(KEY_APP_TO_SEAT)) and
handle that early (e.g., log the error, show a user-friendly message or set an
error result and call finishWithResult / finish) instead of calling
AppSeater.start with a null value; reference the KEY_APP_TO_SEAT constant, the
appId variable, the AppSeater.start(this, appId, progress -> {},
this::finishWithResult) invocation, and the finishWithResult method when making
the change.

app/unit-tests/src/org/commcare/login/AppSeaterTest.kt (1)

13-13: 💤 Low value

Replace STATE_READY = 2 with a non-corrupted value derived from CommCareApplication.STATE_CORRUPTED

AppSeater.seatIfNeeded only checks resourceState == CommCareApplication.STATE_CORRUPTED; any other value returns SeatResult.Success, so the test doesn’t need to hardcode “ready” as 2.

private const val STATE_READY = 2

Prefer returning a value like CommCareApplication.STATE_CORRUPTED + 1 (inline or via a renamed constant) to make the test intent (“not corrupted”) explicit and avoid brittle numeric coupling.

🤖 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 `@app/unit-tests/src/org/commcare/login/AppSeaterTest.kt` at line 13, Replace
the hardcoded numeric STATE_READY with a value derived from
CommCareApplication.STATE_CORRUPTED so the test clearly represents a "not
corrupted" state: update the STATE_READY constant (used by
AppSeater.seatIfNeeded in the test) to CommCareApplication.STATE_CORRUPTED + 1
(or rename to STATE_NOT_CORRUPTED and assign that expression) so the test no
longer relies on the magic number 2 and explicitly conveys "not corrupted".

🤖 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 `@app/src/org/commcare/activities/SeatAppActivity.java`:
- Around line 33-41: Remove the in-code explanatory comment inside the
finishWithResult method: delete the two-line comment beginning with "A corrupted
seat still returns RESULT_OK; DispatchActivity detects STATE_CORRUPTED and
routes to recovery." so the method contains only the result-mapping logic using
SeatResult, SeatResult.Success, SeatResult.Failed, SeatFailure.CORRUPTED and the
setResult/finish calls; leave the logic unchanged.
- Around line 29-30: SeatAppActivity currently calls
getIntent().getStringExtra(KEY_APP_TO_SEAT) and passes the possibly-null appId
into new AppSeater().start(...), which can lead to NPEs; update SeatAppActivity
to guard against a missing KEY_APP_TO_SEAT by checking if appId is null (from
getIntent().getStringExtra(KEY_APP_TO_SEAT)) and handle that early (e.g., log
the error, show a user-friendly message or set an error result and call
finishWithResult / finish) instead of calling AppSeater.start with a null value;
reference the KEY_APP_TO_SEAT constant, the appId variable, the
AppSeater.start(this, appId, progress -> {}, this::finishWithResult) invocation,
and the finishWithResult method when making the change.

In `@app/unit-tests/src/org/commcare/login/AppSeaterTest.kt`:
- Line 13: Replace the hardcoded numeric STATE_READY with a value derived from
CommCareApplication.STATE_CORRUPTED so the test clearly represents a "not
corrupted" state: update the STATE_READY constant (used by
AppSeater.seatIfNeeded in the test) to CommCareApplication.STATE_CORRUPTED + 1
(or rename to STATE_NOT_CORRUPTED and assign that expression) so the test no
longer relies on the magic number 2 and explicitly conveys "not corrupted".

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 61b37eb8-d6c6-4f44-8a2a-fc93b8ec5431

📥 Commits

Reviewing files that changed from the base of the PR and between 748b6e7 and 507f6d7.

📒 Files selected for processing (7)

• app/src/org/commcare/activities/DispatchActivity.java
• app/src/org/commcare/activities/SeatAppActivity.java
• app/src/org/commcare/login/AppSeater.kt
• app/src/org/commcare/login/PostLoginDestination.kt
• app/src/org/commcare/login/PostLoginRouter.kt
• app/unit-tests/src/org/commcare/login/AppSeaterTest.kt
• app/unit-tests/src/org/commcare/login/PostLoginRouterTest.kt

[shubham1g5]

Have not reviewed tests yet as wanted to get some clarification on design before diving into those.

[OrangeAndGreen]

All looks good to me, just one question to consider

The base branch was changed.

[OrangeAndGreen]

While reviewing the next PR after this one (3753) I noticed we remove two of the classes introduced in this PR. Given that this one is still open, it seems like it would make more sense to remove those classes here rather than in a follow-up PR