Added ability to silently call API if required #3399
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jignesh-dimagi
requested review from
OrangeAndGreen,
conroy-ricketts and
shubham1g5
📝 WalkthroughWalkthroughThis pull request extends the Connect API handler and job helper infrastructure to support loading UI state management. New optional parameters ( Sequence Diagram(s)sequenceDiagram
participant Caller
participant ConnectJobHelper
participant BaseApiHandler
participant BaseApiCallback
Caller->>ConnectJobHelper: updateDeliveryProgress(context, job, showLoading, view, listener)
ConnectJobHelper->>BaseApiHandler: new instance(loading, view)
BaseApiHandler->>BaseApiCallback: createCallback()
BaseApiCallback->>BaseApiHandler: onStart() [if loading enabled]
alt Success Path
BaseApiCallback->>BaseApiHandler: processSuccess(response)
BaseApiHandler->>BaseApiHandler: onStop() [if loading enabled]
BaseApiHandler->>Caller: listener callback
else Failure Path
BaseApiCallback->>BaseApiHandler: stopLoadingAndInformError(errorCode, throwable)
BaseApiHandler->>BaseApiHandler: onStop() [if loading enabled]
BaseApiHandler->>BaseApiHandler: onFailure(errorCode, throwable)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/src/org/commcare/activities/LoginActivity.java(1 hunks)app/src/org/commcare/activities/StandardHomeActivity.java(1 hunks)app/src/org/commcare/connect/ConnectJobHelper.kt(4 hunks)app/src/org/commcare/connect/network/base/BaseApiCallback.kt(1 hunks)app/src/org/commcare/connect/network/base/BaseApiHandler.kt(3 hunks)app/src/org/commcare/connect/network/connect/ConnectApiHandler.kt(1 hunks)app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.java(1 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java(1 hunks)app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java(1 hunks)app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java(1 hunks)app/src/org/commcare/pn/workers/NotificationsSyncWorker.kt(1 hunks)app/src/org/commcare/services/PaymentAcknowledgeReceiver.java(1 hunks)
🧰 Additional context used
🧠 Learnings (23)
📓 Common learnings
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.
📚 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/fragments/connect/ConnectDeliveryProgressFragment.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.javaapp/src/org/commcare/connect/network/connect/ConnectApiHandler.ktapp/src/org/commcare/activities/StandardHomeActivity.javaapp/src/org/commcare/connect/ConnectJobHelper.ktapp/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.javaapp/src/org/commcare/activities/LoginActivity.javaapp/src/org/commcare/services/PaymentAcknowledgeReceiver.javaapp/src/org/commcare/fragments/connect/ConnectJobsListsFragment.javaapp/src/org/commcare/pn/workers/NotificationsSyncWorker.ktapp/src/org/commcare/connect/network/base/BaseApiHandler.kt
📚 Learning: 2025-04-18T20:13:29.655Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3040
File: app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryFlagRecord.java:39-55
Timestamp: 2025-04-18T20:13:29.655Z
Learning: In the CommCare Android Connect feature, the JSON object passed to `ConnectJobDeliveryFlagRecord.fromJson()` method should never be null based on the implementation design.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.javaapp/src/org/commcare/connect/network/connect/ConnectApiHandler.ktapp/src/org/commcare/activities/StandardHomeActivity.javaapp/src/org/commcare/connect/ConnectJobHelper.ktapp/src/org/commcare/activities/LoginActivity.javaapp/src/org/commcare/services/PaymentAcknowledgeReceiver.javaapp/src/org/commcare/fragments/connect/ConnectJobsListsFragment.javaapp/src/org/commcare/pn/workers/NotificationsSyncWorker.kt
📚 Learning: 2025-06-20T15:51:14.157Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/models/connect/ConnectLoginJobListModel.java:79-92
Timestamp: 2025-06-20T15:51:14.157Z
Learning: The ConnectLoginJobListModel class in app/src/org/commcare/models/connect/ConnectLoginJobListModel.java does not need to implement Parcelable interface as it is not passed between Android activities or fragments.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.javaapp/src/org/commcare/activities/StandardHomeActivity.javaapp/src/org/commcare/connect/ConnectJobHelper.ktapp/src/org/commcare/activities/LoginActivity.javaapp/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-05-09T10:57:41.073Z
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 3093
File: app/res/navigation/nav_graph_connect_messaging.xml:41-45
Timestamp: 2025-05-09T10:57:41.073Z
Learning: In the CommCare Android codebase, the navigation graph for Connect messaging (`nav_graph_connect_messaging.xml`) intentionally uses `channel_id` as the argument name in the connectMessageFragment, despite using `channelId` in other parts of the same navigation graph. This naming difference is by design in the refactored code.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.javaapp/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java
📚 Learning: 2025-07-29T14:11:36.386Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java:0-0
Timestamp: 2025-07-29T14:11:36.386Z
Learning: In ConnectJobsListsFragment.java error handling for JSON parsing, if the JSONObject obj is null when passed to handleCorruptJob(), the team prefers to let the code crash rather than adding defensive null checks. This follows the fail-fast philosophy used throughout the Connect feature to catch programming errors immediately rather than masking them.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.javaapp/src/org/commcare/connect/network/connect/ConnectApiHandler.ktapp/src/org/commcare/activities/LoginActivity.javaapp/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-06-06T19:54:26.428Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectDownloadingFragment.java:74-78
Timestamp: 2025-06-06T19:54:26.428Z
Learning: In ConnectDownloadingFragment.java and similar Connect-related code, the team prefers to let "should never happen" scenarios like null app records crash rather than add defensive null checks, following a fail-fast philosophy to catch programming errors during development.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.javaapp/src/org/commcare/connect/network/connect/ConnectApiHandler.ktapp/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.javaapp/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-05-22T14:28:35.959Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/src/org/commcare/activities/CommCareSetupActivity.java:360-364
Timestamp: 2025-05-22T14:28:35.959Z
Learning: In CommCareSetupActivity.java, the call to installFragment.showConnectErrorMessage() after fragment transactions is intentionally unguarded with null checks. This follows the app's design pattern where critical error paths prefer immediate crashes over silent failures, making potential issues immediately visible during development rather than hiding them with defensive programming.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.javaapp/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.javaapp/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-07-29T14:10:58.243Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java:0-0
Timestamp: 2025-07-29T14:10:58.243Z
Learning: In ConnectJobsListsFragment.java, the team intentionally uses different error handling strategies: JSONException should throw RuntimeException to crash the app (fail-fast for data contract violations), while IOException should be logged and allow graceful continuation (for network/stream issues). This follows the established Connect module pattern of treating different exception types differently based on their nature.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.javaapp/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-06-06T19:52:53.173Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java:163-180
Timestamp: 2025-06-06T19:52:53.173Z
Learning: In the CommCare Android Connect feature, database operations like ConnectJobUtils.upsertJob should be allowed to crash rather than being wrapped in try-catch blocks. The team prefers fail-fast behavior for database errors instead of graceful error handling.
Applied to files:
app/src/org/commcare/connect/network/connect/ConnectApiHandler.ktapp/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-02-19T15:15:01.935Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2956
File: app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java:58-60
Timestamp: 2025-02-19T15:15:01.935Z
Learning: Error handling for message retrieval in ConnectMessageChannelListFragment's retrieveMessages callback is not required as per user preference.
Applied to files:
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java
📚 Learning: 2025-07-29T14:14:07.954Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/adapters/ConnectMessageAdapter.java:0-0
Timestamp: 2025-07-29T14:14:07.954Z
Learning: In the CommCare Android Connect messaging system (ConnectMessageAdapter.java), the team prefers to let getMessage() potentially return null and crash rather than adding defensive null checks. This follows the fail-fast philosophy used throughout the Connect feature to catch programming errors immediately during development.
Applied to files:
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java
📚 Learning: 2025-07-29T14:09:49.805Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java:0-0
Timestamp: 2025-07-29T14:09:49.805Z
Learning: In the CommCare Android Connect messaging system, message retry logic is implemented at the MessageManager level via MessageManager.sendUnsentMessages(). This method is called automatically when the channel list loads (ConnectMessageChannelListFragment), and it attempts to resend any outgoing messages that are not yet confirmed. Individual message fragments do not need to implement their own retry logic.
Applied to files:
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java
📚 Learning: 2025-05-22T14:26:41.341Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/src/org/commcare/fragments/SelectInstallModeFragment.java:201-205
Timestamp: 2025-05-22T14:26:41.341Z
Learning: In the Connect error handling flow of CommCare Android, error messages are shown once and then automatically cleared because the underlying error record gets deleted after being displayed. This is why there's no need for explicit methods to hide these messages.
Applied to files:
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java
📚 Learning: 2025-07-29T14:45:13.470Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/connect/MessageManager.java:0-0
Timestamp: 2025-07-29T14:45:13.470Z
Learning: In the CommCare Android Connect messaging system (MessageManager.java), the error handling logic in updateChannelConsent's processFailure callback intentionally attempts to retrieve the channel encryption key even after consent update failure. This behavior was recommended by the server team and the worst case scenario is silently failing to retrieve the encryption key, which is considered acceptable.
Applied to files:
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java
📚 Learning: 2025-05-22T14:26:41.341Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/src/org/commcare/fragments/SelectInstallModeFragment.java:201-205
Timestamp: 2025-05-22T14:26:41.341Z
Learning: In SelectInstallModeFragment.java, the showConnectErrorMessage method intentionally omits null checks because it's called at a specific point in the startup flow where UI is guaranteed to be loaded. It's designed to crash if activity or view is null to make potential issues immediately visible rather than hiding them with defensive programming.
Applied to files:
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java
📚 Learning: 2025-06-13T15:53:12.951Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/connect/network/PersonalIdApiHandler.java:51-56
Timestamp: 2025-06-13T15:53:12.951Z
Learning: For `PersonalIdApiHandler`, the team’s convention is to propagate `JSONException` as an unchecked `RuntimeException` so the app crashes, signalling a contract/implementation bug rather than attempting a graceful retry.
Applied to files:
app/src/org/commcare/connect/network/base/BaseApiCallback.ktapp/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java
📚 Learning: 2025-01-21T18:20:43.883Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:206-213
Timestamp: 2025-01-21T18:20:43.883Z
Learning: In the Connect ID feature, server responses containing user data should be parsed using ConnectUserResponseParser to ensure consistent handling of payment information (owner_name and phone_number) and secondary phone verification across all authentication flows.
Applied to files:
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java
📚 Learning: 2025-02-04T21:22:56.537Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2949
File: app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java:244-275
Timestamp: 2025-02-04T21:22:56.537Z
Learning: Direct JSONObject parsing is acceptable for handling user data responses in ConnectIdPinFragment, as decided by the team. No need to enforce ConnectUserResponseParser usage in this context.
Applied to files:
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java
📚 Learning: 2025-01-21T18:19:05.799Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:214-215
Timestamp: 2025-01-21T18:19:05.799Z
Learning: In ConnectIdPasswordVerificationFragment, when creating a ConnectUserRecord, it's acceptable for payment information (paymentName and paymentPhone) to be empty strings if the server response doesn't include payment info in the CONNECT_PAYMENT_INFO field.
Applied to files:
app/src/org/commcare/services/PaymentAcknowledgeReceiver.java
📚 Learning: 2025-07-29T14:10:55.131Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:0-0
Timestamp: 2025-07-29T14:10:55.131Z
Learning: In ConnectUnlockFragment.java, opportunityId values are expected to always contain valid integer strings. Integer.parseInt() should be allowed to throw NumberFormatException if invalid data is encountered, following the fail-fast philosophy used throughout the Connect feature for data contract violations.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-07-29T14:08:55.466Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:0-0
Timestamp: 2025-07-29T14:08:55.466Z
Learning: In ConnectUnlockFragment.java and similar Connect-related code, the team has agreed that JSONExceptions should crash the app by throwing RuntimeException (fail-fast approach for data contract violations), while IOExceptions should be logged and allow processing to continue (graceful handling for network/stream issues). This intentional inconsistency in error handling reflects different treatment for different types of failures.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-01-27T09:08:32.722Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2847
File: app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecord.java:86-93
Timestamp: 2025-01-27T09:08:32.722Z
Learning: The JSON fields (status, unitName, slug, entityId, entityName, reason) in ConnectJobDeliveryRecord's fromJson method are guaranteed to be non-null through the application's data contract, except for the 'id' field which is explicitly checked.
Applied to files:
app/src/org/commcare/pn/workers/NotificationsSyncWorker.kt
🧬 Code graph analysis (8)
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (1)
app/src/org/commcare/connect/ConnectJobHelper.kt (1)
updateDeliveryProgress(85-128)
app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.java (1)
app/src/org/commcare/connect/ConnectJobHelper.kt (1)
updateDeliveryProgress(85-128)
app/src/org/commcare/activities/StandardHomeActivity.java (1)
app/src/org/commcare/connect/ConnectJobHelper.kt (1)
updateDeliveryProgress(85-128)
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java (1)
app/src/org/commcare/pn/workers/PNApiResponseStatus.kt (1)
success(5-8)
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java (2)
app/src/org/commcare/connect/network/base/BaseApiHandler.kt (1)
loading(13-93)app/src/org/commcare/connect/network/connect/ConnectApiHandler.kt (1)
loading(17-64)
app/src/org/commcare/activities/LoginActivity.java (1)
app/src/org/commcare/connect/ConnectJobHelper.kt (1)
updateJobProgress(37-57)
app/src/org/commcare/services/PaymentAcknowledgeReceiver.java (1)
app/src/org/commcare/connect/ConnectJobHelper.kt (1)
updateDeliveryProgress(85-128)
app/src/org/commcare/connect/network/base/BaseApiHandler.kt (3)
app/src/org/commcare/connect/ConnectJobHelper.kt (8)
onSuccess(66-76)onSuccess(94-121)onSuccess(140-146)onSuccess(171-173)onFailure(78-81)onFailure(123-126)onFailure(148-156)onFailure(164-169)app/src/org/commcare/connect/workers/ConnectHeartbeatWorker.kt (2)
onSuccess(27-29)onFailure(23-25)app/src/org/commcare/connect/network/base/BaseApi.kt (1)
onFailure(60-65)
🔇 Additional comments (16)
app/src/org/commcare/activities/StandardHomeActivity.java (1)
366-366: LGTM: Appropriate use of silent API call.The two
nullparameters correctly disable the loading indicator for this background sync operation, which is consistent with the PR's objective to support silent API calls.app/src/org/commcare/pn/workers/NotificationsSyncWorker.kt (1)
138-139: LGTM: Correct usage for background worker.Passing
nullfor the loading UI parameters is appropriate for this background worker context, as workers may execute when the app UI is not visible.app/src/org/commcare/connect/network/base/BaseApiCallback.kt (2)
23-55: LGTM: Consistent error handling refactor.The replacement of
onFailure(...)withstopLoadingAndInformError(...)across all error paths properly integrates loading UI state management. This ensures loading indicators are stopped before error messages are displayed.
60-84: LGTM: Consistent application of loading state management.All remaining error paths correctly use
stopLoadingAndInformError(...)to coordinate loading UI state with error handling.app/src/org/commcare/connect/ConnectJobHelper.kt (2)
37-56: LGTM: Well-designed optional parameter propagation.The new optional parameters (
showLoading,baseConnectView) are properly propagated through the DELIVERING path with sensible defaults. Note that the LEARNING path (line 46) doesn't yet support loading UI parameters, which is acceptable if learning progress updates are quick or already have their own loading indicators.
85-93: LGTM: Correct parameter forwarding to API handler.The new parameters are properly passed to the
ConnectApiHandlerconstructor, enabling loading UI state management for delivery progress updates.app/src/org/commcare/services/PaymentAcknowledgeReceiver.java (1)
36-36: LGTM: Appropriate silent operation for broadcast receiver.Passing
nullfor loading UI parameters is correct for this broadcast receiver, which executes without a visible activity context.app/src/org/commcare/activities/LoginActivity.java (1)
510-510: LGTM: Reasonable silent call during login flow.Passing
nullfor loading parameters is appropriate here since the login flow likely already displays its own progress indicators (e.g., the data pull task progress dialog at lines 703-708).app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java (1)
42-48: LGTM: Proper constructor overloading for backward compatibility.The two constructors correctly support both existing usage (no-arg) and the new loading UI integration pattern (with loading flag and view context), maintaining backward compatibility while enabling the new functionality.
app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.java (1)
127-127: LGTM! Loading UI properly delegated to API handler.The call now passes
trueto enable loading indicators andthisas the view reference, allowingConnectJobHelper.updateDeliveryProgressto manage the loading state centrally via the updated API handler infrastructure.app/src/org/commcare/connect/network/connect/ConnectApiHandler.kt (1)
12-17: LGTM! Constructor properly forwards loading UI parameters.The updated constructor signature accepts optional
loadingandviewparameters with sensible defaults (falseandnull), maintaining backward compatibility while enabling loading UI control when needed. The parameters are correctly forwarded toBaseApiHandler.app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (1)
80-93: LGTM! Loading state management properly delegated.The API handler instantiation now passes
truefor loading andthisas the view reference, enabling centralized loading state management. The removal of manualshowLoading()/hideLoading()calls (referenced in the AI summary) is the correct approach sinceBaseApiHandlernow handles this lifecycle automatically.app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (1)
120-120: LGTM! Consistent with loading UI pattern.The updated call correctly passes loading UI parameters, consistent with the pattern applied across other fragments in this PR. The centralized loading management improves code maintainability.
app/src/org/commcare/connect/network/base/BaseApiHandler.kt (3)
4-4: LGTM! Constructor enables centralized loading UI management.The new constructor parameters (
loading: Boolean?andview: BaseConnectView?) with default values enable optional loading UI control while maintaining backward compatibility for existing callers.Also applies to: 13-13
55-72: LGTM! Callback lifecycle properly manages loading state.The refactoring ensures:
onStart()is called synchronously when the callback is created (line 55)onStop()is called after successful parsing (line 60)stopLoadingAndInformError()handles both cleanup and error reporting for parsing failures (lines 63-66, 69)This guarantees loading state is always cleaned up, preventing stuck loading indicators on errors.
75-91: LGTM! Loading lifecycle methods are well-structured.The public helper methods provide:
- Null-safe operations:
loading?.letpattern prevents NPEs- Conditional UI updates: Loading state only toggled when explicitly enabled
- Composable error handling:
stopLoadingAndInformErrorcombines cleanup with error propagationThe public visibility allows external code to manually control loading state when needed, providing flexibility for special cases.
Comment on lines
109
to
112
| if (!success) { | ||
| refreshUi(); | ||
| } else { | ||
| Toast.makeText(requireContext(), getString(R.string.connect_messaging_retrieve_messages_fail), Toast.LENGTH_SHORT).show(); |
There was a problem hiding this comment.
Critical: Inverted success logic causes incorrect UI behavior.
The condition is backwards—when success is false, the UI is refreshed; when success is true, an error toast is displayed. This will show error messages on successful message retrieval and silently refresh on failures.
Apply this diff to fix the inverted logic:
- if (!success) {
+ if (success) {
refreshUi();
} else {
Toast.makeText(requireContext(), getString(R.string.connect_messaging_retrieve_messages_fail), Toast.LENGTH_SHORT).show();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!success) { | |
| refreshUi(); | |
| } else { | |
| Toast.makeText(requireContext(), getString(R.string.connect_messaging_retrieve_messages_fail), Toast.LENGTH_SHORT).show(); | |
| if (success) { | |
| refreshUi(); | |
| } else { | |
| Toast.makeText(requireContext(), getString(R.string.connect_messaging_retrieve_messages_fail), Toast.LENGTH_SHORT).show(); | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java
around lines 109 to 112, the success logic is inverted: the code refreshes the
UI when success is false and shows an error toast when success is true. Swap the
branches so that when success is true you call refreshUi(), otherwise show the
error Toast with getString(R.string.connect_messaging_retrieve_messages_fail);
ensure you only use requireContext() for the Toast as currently written.
Contributor
There was a problem hiding this comment.
This seems important, was flipping the condition a mistake?
There was a problem hiding this comment.
@OrangeAndGreen Yes, this appears to be a mistake! The condition flip inverts the success logic:
Current (incorrect) behavior:
- When
successisfalse→ refreshes UI - When
successistrue→ shows error toast
Expected behavior:
- When
successistrue→ should refresh UI - When
successisfalse→ should show error toast
This change seems unrelated to the PR's main objective (adding silent API calls with optional loading indicators). The logic should be reverted to:
if (success) {
refreshUi();
} else {
Toast.makeText(requireContext(), getString(R.string.connect_messaging_retrieve_messages_fail), Toast.LENGTH_SHORT).show();
}Was this an accidental change during the refactoring?
🧠 Learnings used
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2956
File: app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java:58-60
Timestamp: 2025-02-19T15:15:01.935Z
Learning: Error handling for message retrieval in ConnectMessageChannelListFragment's retrieveMessages callback is not required as per user preference.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java:0-0
Timestamp: 2025-07-29T14:09:49.805Z
Learning: In the CommCare Android Connect messaging system, message retry logic is implemented at the MessageManager level via MessageManager.sendUnsentMessages(). This method is called automatically when the channel list loads (ConnectMessageChannelListFragment), and it attempts to resend any outgoing messages that are not yet confirmed. Individual message fragments do not need to implement their own retry logic.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/adapters/ConnectMessageAdapter.java:0-0
Timestamp: 2025-07-29T14:14:07.954Z
Learning: In the CommCare Android Connect messaging system (ConnectMessageAdapter.java), the team prefers to let getMessage() potentially return null and crash rather than adding defensive null checks. This follows the fail-fast philosophy used throughout the Connect feature to catch programming errors immediately during development.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/connect/MessageManager.java:0-0
Timestamp: 2025-07-29T14:45:13.470Z
Learning: In the CommCare Android Connect messaging system (MessageManager.java), the error handling logic in updateChannelConsent's processFailure callback intentionally attempts to retrieve the channel encryption key even after consent update failure. This behavior was recommended by the server team and the worst case scenario is silently failing to retrieve the encryption key, which is considered acceptable.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/src/org/commcare/activities/CommCareSetupActivity.java:360-364
Timestamp: 2025-05-22T14:28:35.959Z
Learning: In CommCareSetupActivity.java, the call to installFragment.showConnectErrorMessage() after fragment transactions is intentionally unguarded with null checks. This follows the app's design pattern where critical error paths prefer immediate crashes over silent failures, making potential issues immediately visible during development rather than hiding them with defensive programming.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/src/org/commcare/fragments/SelectInstallModeFragment.java:201-205
Timestamp: 2025-05-22T14:26:41.341Z
Learning: In the Connect error handling flow of CommCare Android, error messages are shown once and then automatically cleared because the underlying error record gets deleted after being displayed. This is why there's no need for explicit methods to hide these messages.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/src/org/commcare/fragments/SelectInstallModeFragment.java:201-205
Timestamp: 2025-05-22T14:26:41.341Z
Learning: In SelectInstallModeFragment.java, the showConnectErrorMessage method intentionally omits null checks because it's called at a specific point in the startup flow where UI is guaranteed to be loaded. It's designed to crash if activity or view is null to make potential issues immediately visible rather than hiding them with defensive programming.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectDownloadingFragment.java:74-78
Timestamp: 2025-06-06T19:54:26.428Z
Learning: In ConnectDownloadingFragment.java and similar Connect-related code, the team prefers to let "should never happen" scenarios like null app records crash rather than add defensive null checks, following a fail-fast philosophy to catch programming errors during development.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/activities/StandardHomeActivityUIController.java:0-0
Timestamp: 2025-06-20T13:21:20.908Z
Learning: User OrangeAndGreen prefers to handle code issues in separate PRs rather than immediately fixing them in the current PR when they are not directly related to the main changes.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/connect/network/ApiPersonalId.java:0-0
Timestamp: 2025-07-29T14:56:40.681Z
Learning: User OrangeAndGreen prefers to defer ApiPersonalId refactoring (specifically InputStream resource management improvements) to separate PRs rather than mixing them with Connect feature work, even when the code is already in master.
Contributor
Author
There was a problem hiding this comment.
That was by mistake as testing something, +1 to catch it. Its correct here
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
OrangeAndGreen
requested changes
Nov 4, 2025
| * Base class for all API handlers | ||
| */ | ||
| abstract class BaseApiHandler<T> { | ||
| abstract class BaseApiHandler<T>(val loading: Boolean? = false, val view: BaseConnectView? = null) { |
Contributor
There was a problem hiding this comment.
Nit: Defaulting "loading" to null seems more consistent with calling conventions and slightly more efficient in onStart/onStop
Contributor
There was a problem hiding this comment.
I would think that a boolean value should be rather defined as a non-null param Boolean instead of Boolean?
Contributor
There was a problem hiding this comment.
Ah, agreed that makes more sense
| abstract class ConnectApiHandler<T>(loading: Boolean?=false, view: BaseConnectView?=null) : BaseApiHandler<T>(loading,view){ | ||
|
|
||
| fun getConnectOpportunities(context: Context, user: ConnectUserRecord) { | ||
| fun getConnectOpportunities(context: Context, user: ConnectUserRecord) { |
Contributor
There was a problem hiding this comment.
Nit: Removed a good indent
|
|
||
|
|
||
| public PersonalIdApiHandler() { | ||
| super(); |
Contributor
There was a problem hiding this comment.
Are these necessary if all they're doing is calling super?
|
|
||
| ConnectJobRecord.STATUS_DELIVERING -> { | ||
| updateDeliveryProgress(context, job, listener) | ||
| updateDeliveryProgress(context, job,showLoading,baseConnectView, listener) |
Contributor
There was a problem hiding this comment.
Nit: spacing after comas
| ) { | ||
| val user = ConnectUserDatabaseUtil.getUser(context) | ||
| object : ConnectApiHandler<DeliveryAppProgressResponseModel>() { | ||
| object : ConnectApiHandler<DeliveryAppProgressResponseModel>(showLoading,baseConnectView) { |
Comment on lines
109
to
112
| if (!success) { | ||
| refreshUi(); | ||
| } else { | ||
| Toast.makeText(requireContext(), getString(R.string.connect_messaging_retrieve_messages_fail), Toast.LENGTH_SHORT).show(); |
Contributor
There was a problem hiding this comment.
This seems important, was flipping the condition a mistake?
conroy-ricketts
requested changes
Nov 4, 2025
Contributor
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
These changes look good, but we should definitely take a look at what Coderabbit pointed out
shubham1g5
requested changes
Nov 4, 2025
Contributor
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Blocking on the issue Code Rabbit flagged but everything else seems good to be.
shubham1g5
approved these changes
Nov 5, 2025
Jignesh-dimagi
requested review from
OrangeAndGreen,
conroy-ricketts and
shubham1g5
shubham1g5
approved these changes
Nov 5, 2025
Contributor
Author
|
@OrangeAndGreen @conroy-ricketts if you can review again please |
conroy-ricketts
approved these changes
Nov 5, 2025
OrangeAndGreen
approved these changes
Nov 5, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Technical Summary
https://dimagi.atlassian.net/browse/CCCT-1699
Added the ability to call API with / without progress loading. This is currently added for opportunities fetching and delivery progress update. This can be further added to others calls.
Labels and Review