Commcare 2.56 #3027
Commcare 2.56 #3027
Conversation
…me after logging in
MInor Tweaks for Cache and Lazy Load
Additional logging to troubleshoot the repeated login issue
…n-async) case list
…ues emitted after we subscribe
Fixes for Entity Cache and Lazy loading work
Update Firebase dependencies
This is necessary to be able to populate cache correctly in primeCache as otherwise we don't have values like mTemplateIsCachable and mCacheHost correctly initialized
Cache Prime Fix: Reuse entity factory for cache prime
|
@damagatchi retest this please |
app/AndroidManifest.xml
Outdated
| xmlns:tools="http://schemas.android.com/tools" | ||
| android:versionCode="106" | ||
| android:versionName="2.56"> | ||
| android:versionName="2.56.0"> |
There was a problem hiding this comment.
we should set it to 2.57.0 for master now.
There was a problem hiding this comment.
Not just 2.57? I thought the hotfix was only added at release?
📝 WalkthroughWalkthroughThe pull request implements several changes across the project. The Android manifest now marks a more detailed version naming scheme, and the build configuration upgrades Firebase Analytics and Crashlytics dependencies. The string resources for entity list progress messages have been updated with consolidated and reworded texts. In the application code, logging in session management has been enhanced, and the handling of entity cache invalidation has been restructured. Specifically, the scheduling of cache invalidation is shifted from the application singleton to a dedicated helper class. Additionally, methods in the entity loading and progress reporting processes have been modified to improve robustness, including added null checks and a weighted progress calculation. Other changes include adjustments to optional parameters in constructors and improved null safety in cache-related methods, as well as updates in unit test initialization. Sequence Diagram(s)sequenceDiagram
participant SyncActivity as SyncCapableCommCareActivity
participant CacheHelper as PrimeEntityCacheHelper
participant WM as WorkManager
participant Worker as EntityCacheInvalidationWorker
SyncActivity->>CacheHelper: scheduleEntityCacheInvalidation()
CacheHelper->>WM: Enqueue OneTimeWorkRequest (EntityCacheInvalidationWorker)
WM->>Worker: Trigger doWork()
alt If cache is not empty
Worker->>Worker: processShallowRecords()
end
Worker-->>CacheHelper: (finally) schedulePrimeEntityCacheWorker()
sequenceDiagram
participant User as End User
participant Activity as EntitySelectActivity
User->>Activity: Initiate entity list load
Activity->>Activity: deliverProgress(Integer[] values)
Note right of Activity: Calculate weighted progress across phases
Activity->>Activity: observePrimeCacheWorker()
Note right of Activity: Added null check to prevent exceptions
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
app/src/org/commcare/tasks/PrimeEntityCache.kt (1)
20-20: Avoid forced null unwrapping.This code uses
!!which will throw an exception ifprimeEntityCacheHelperis null. If there's any possibility the helper is absent, consider using a safe-call or a clear error message to handle unexpected nulls more gracefully.app/src/org/commcare/tasks/EntityLoaderHelper.kt (3)
36-49: Nested factory-initialization logic.While the if-else ladder for choosing factory implementations works, consider moving this selection logic into a dedicated method or strategy pattern to simplify readability and reduce nesting.
66-71: Forced unwrap of factory.Calls like
factory!!.expandReferenceListassume the factory is non-null. If there's any chance of a null factory, throw a more helpful exception or handle it gracefully.
93-93: Cache method also uses forced unwrap.If there’s any risk of
factorybeing null, add a check or use a safe call to avoid unexpected runtime errors.app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt (5)
59-59: LiveData usage for progress state.Switching to a
MutableLiveDatacan simplify UI observation, but consider if a Flow-based approach would be more consistent.
120-122: Blocking on the main thread.
runBlockingcan freeze the UI if invoked on the main thread. Consider using a coroutine scope with the appropriate dispatcher to avoid potential ANR issues.
143-145: Another runBlocking usage.Same concern regarding blocking. If it’s within a background context, it’s acceptable; otherwise, consider a coroutine-based approach.
195-196: Factory set with forced unwrap.If
factorycan actually be null, handle it gracefully or throw an exception instead of forcing an unwrap.
205-206: Emitting cached entity state under runBlocking.Consider using a non-blocking emit if the current thread must remain responsive.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app/AndroidManifest.xml(1 hunks)app/build.gradle(1 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/CommCareApplication.java(3 hunks)app/src/org/commcare/activities/EntitySelectActivity.java(2 hunks)app/src/org/commcare/activities/SyncCapableCommCareActivity.java(2 hunks)app/src/org/commcare/entity/AndroidAsyncNodeEntityFactory.kt(2 hunks)app/src/org/commcare/logging/analytics/TimedStatsTracker.java(2 hunks)app/src/org/commcare/tasks/DataPullTask.java(3 hunks)app/src/org/commcare/tasks/EntityCacheInvalidationWorker.kt(2 hunks)app/src/org/commcare/tasks/EntityLoaderHelper.kt(6 hunks)app/src/org/commcare/tasks/EntityLoaderTask.java(1 hunks)app/src/org/commcare/tasks/ManageKeyRecordTask.java(4 hunks)app/src/org/commcare/tasks/PrimeEntityCache.kt(1 hunks)app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt(6 hunks)app/unit-tests/src/org/commcare/android/tests/DemoUserRestoreTest.java(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
app/src/org/commcare/tasks/DataPullTask.java (1)
app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt (1)
scheduleEntityCacheInvalidation(89-101)
app/src/org/commcare/tasks/EntityLoaderTask.java (2)
app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt (1)
evalCtx(212-214)app/src/org/commcare/tasks/EntityLoaderHelper.kt (1)
detail(23-131)
app/src/org/commcare/logging/analytics/TimedStatsTracker.java (1)
app/src/org/commcare/network/CTInterceptorConfig.kt (1)
log(40-47)
app/src/org/commcare/tasks/ManageKeyRecordTask.java (1)
app/src/org/commcare/network/CTInterceptorConfig.kt (1)
log(40-47)
app/unit-tests/src/org/commcare/android/tests/DemoUserRestoreTest.java (1)
app/src/org/commcare/CommCareApplication.java (1)
CommCareApplication(154-1260)
app/src/org/commcare/tasks/EntityLoaderHelper.kt (1)
app/src/org/commcare/models/database/user/models/CommCareEntityStorageCache.java (1)
CommCareEntityStorageCache(42-438)
app/src/org/commcare/activities/SyncCapableCommCareActivity.java (1)
app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt (1)
scheduleEntityCacheInvalidation(89-101)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (43)
app/AndroidManifest.xml (1)
5-5: Version name updated to follow semantic versioning patternThe version name has been updated from "2.56" to "2.56.0", following a more explicit semantic versioning format. This improves version clarity by explicitly indicating this is the initial release of version 2.56.
app/build.gradle (2)
104-104:Details
✅ Verification successful
Firebase Analytics dependency updated
The Firebase Analytics dependency has been updated from an older version to 20.1.2. This update ensures compatibility with the latest Firebase services and provides access to newer features and bug fixes.
🏁 Script executed:
#!/bin/bash # Verify the compatibility and release notes for this Firebase Analytics version echo "Checking Firebase Analytics 20.1.2 release information..." curl -s "https://firebase.google.com/support/release-notes/android" | grep -A 10 "Analytics.*20.1.2"Length of output: 1358
Firebase Analytics Updated to 20.1.2 – Verified
The dependency in
app/build.gradle(line 104) has been successfully updated to version 20.1.2. Verification via the Firebase release notes confirms that this version introduces enhanced data collection customization and improved ecommerce reporting support. The update ensures full compatibility with the latest Firebase services and incorporates essential service enhancements.
106-106:Details
✅ Verification successful
Firebase Crashlytics dependency updated
The Firebase Crashlytics dependency has been significantly updated from an older version to 18.3.7. This is a major version update that likely includes important stability improvements, security fixes, and new features.
🏁 Script executed:
#!/bin/bash # Verify the compatibility and release notes for this Firebase Crashlytics version echo "Checking Firebase Crashlytics 18.3.7 release information..." curl -s "https://firebase.google.com/support/release-notes/android" | grep -A 10 "Crashlytics.*18.3.7"Length of output: 2276
Verified: Firebase Crashlytics dependency updated to 18.3.7
The release notes confirm that using version 18.3.7 introduces important enhancements such as VCS integration support and other improvements. The dependency update in
app/build.gradleon line 106 is correct, and no further action is needed unless you plan to leverage the new VCS integration features (which require settingandroid.enableVcsInfo=trueingradle.properties).app/src/org/commcare/tasks/DataPullTask.java (2)
454-454: Refactored entity cache invalidation to use dedicated helperThe entity cache invalidation scheduling has been moved from
CommCareApplication.instance().schedulePrimeEntityCacheWorker()to the more appropriatePrimeEntityCacheHelper.scheduleEntityCacheInvalidation(). This improves code organization by centralizing cache management logic in a dedicated helper class rather than keeping it in the application singleton.
483-483: Added logging for user login wipe processEnhanced logging has been added to track when user login is being wiped, which improves troubleshooting capability and makes debugging user session issues easier.
app/src/org/commcare/logging/analytics/TimedStatsTracker.java (2)
62-62: Added session start loggingAdded logging to record when a user session begins, which provides better visibility into user session lifecycles. This will help with diagnosing session-related issues and monitoring application usage patterns.
64-65: Replaced Android logging with CommCare's Logger systemMigrated from Android's native
Log.i()to CommCare'sLogger.log()system with appropriate log type categorization. This improves logging consistency across the application and ensures logs are properly categorized for filtering and analysis.app/src/org/commcare/tasks/EntityLoaderTask.java (1)
45-45: Updated constructor call to match new API signatureThe change adds a null parameter to the EntityLoaderHelper constructor, adapting to the updated helper class which now accepts an optional NodeEntityFactory parameter. This is correctly aligned with the EntityLoaderHelper signature change.
app/src/org/commcare/tasks/ManageKeyRecordTask.java (4)
126-127: Enhanced logging for debugging session stateAdding logging for null user after key record request helps identify cases where authentication succeeded but user creation failed.
140-141: Improved session closure loggingAdding this log statement provides better visibility into why user sessions are being closed, which helps with troubleshooting session management issues.
513-513: Added user setup loggingLogging the user being set up helps track session establishment and aids in debugging user-specific issues.
518-518: Added session retrieval failure loggingThis log entry captures cases where login appears successful but session retrieval fails, helping identify session management edge cases.
app/unit-tests/src/org/commcare/android/tests/DemoUserRestoreTest.java (1)
109-110: Initialize WorkManager before entity activity launchThe test has been updated to initialize WorkManager before launching the EntitySelectActivity, which is necessary for proper test execution since entity cache operations now use WorkManager for background processing.
app/src/org/commcare/activities/SyncCapableCommCareActivity.java (1)
265-265: Moved entity cache invalidation responsibilityThe code now calls PrimeEntityCacheHelper.scheduleEntityCacheInvalidation() instead of using CommCareApplication to schedule cache invalidation. This refactoring appropriately consolidates cache management logic in a dedicated helper class.
app/src/org/commcare/entity/AndroidAsyncNodeEntityFactory.kt (2)
60-62: Passingthisreference ensures proper factory accessThe addition of
thisas a parameter toprimeEntityCacheForDetailpasses the current instance ofAndroidAsyncNodeEntityFactoryto the helper method, allowing it to access the factory's methods and properties during cache priming operations.
74-76: Consistent usage of factory referenceThis change mirrors the similar update at lines 60-62, ensuring consistent parameter passing across all calls to
primeEntityCacheForDetailin this class. Good consistency in implementation.app/src/org/commcare/tasks/EntityCacheInvalidationWorker.kt (2)
17-19: Added empty-cache check for optimized processingThe addition of this null check prevents unnecessary processing of shallow records when the entity storage cache is empty, which is a good optimization.
23-26: Ensuring consistent scheduling of Prime workerThe added
finallyblock guarantees thatPrimeEntityCacheHelper.schedulePrimeEntityCacheWorker()is called regardless of whether the invalidation process succeeds or fails. This improves robustness by ensuring the Prime worker is always scheduled, even when encountering exceptions.app/res/values/strings.xml (1)
471-472: Improved entity list progress message wordingThe string resources for entity list progress have been consolidated and reworded for better clarity:
entity_list_loadingnow provides a more general loading messageentity_list_finishingclearly indicates final calculation phaseThese changes improve user communication by using more intuitive wording and maintaining consistency in status messaging throughout the loading process.
app/src/org/commcare/CommCareApplication.java (3)
369-370: Enhanced session transition loggingAdding a log entry when closing a user session to start a new one provides better visibility into session lifecycle transitions, which is helpful for debugging session-related issues.
382-383: Improved session closure visibilityThis log entry provides explicit documentation when a user session is being closed, complementing the log added for session transitions and enhancing the observability of the session lifecycle.
809-812: Reset cache helper state on service connectionThe addition of
getCurrentApp().getPrimeEntityCacheHelper().clearState()ensures that the prime entity cache helper starts with a clean state when a new session is established. This is a good practice for preventing stale cache data from affecting the new session.app/src/org/commcare/tasks/PrimeEntityCache.kt (3)
14-14: Nullable helper reference introduced.Switching from a non-nullable/lazy-initialized helper to a nullable reference helps avoid initialization issues. However, ensure consumers handle cases where
primeEntityCacheHelpermight remain null.
26-26: Safe call is consistent.Using
primeEntityCacheHelper?.clearState()ensures no null pointer exceptions. This approach aligns with making the helper nullable.
32-32: Null-safe cancelation.Graceful cancelation with
?.cancel()is good. Ensure there's no concurrency edge case where the helper is cleared while another routine is using it.app/src/org/commcare/tasks/EntityLoaderHelper.kt (7)
5-5: No issue with import.
27-28: Constructor allows optional factory.Adopting a nullable factory parameter provides flexibility in controlling entity loading strategies. Verify that all call sites handle the case when no factory is provided.
61-64: Synchronous vs. async approach.Canceling background cache when we aren’t in async mode prevents potential database locks or concurrency issues. This looks valid, but confirm there's no scenario where partial caching is beneficial in synchronous loading.
73-78: Restarting cache after sync load.Rescheduling
PrimeEntityCacheHelper.schedulePrimeEntityCacheWorker()ensures caching resumes post-synchronous load. Verify there's no race condition if the user navigates away or cancels mid-load.
86-86: Continued forced unwrap.Again,
factory!!requires certainty that the factory is set. If that’s guaranteed by prior logic, this is fine. Otherwise, consider a precondition check.
115-115: Forcibly unwrapping entity creation.Ensure the factory is never null at this point. Otherwise, throw an exception or log a clear message.
129-129: Marking factory as canceled.If
factorycould be null, consider a safe call. If null is impossible, the forced unwrap is acceptable.app/src/org/commcare/activities/EntitySelectActivity.java (2)
499-499: Prevent null pointer in worker observation.The null check on
tripleavoids potential crashes. Good defensive coding for uncertain data flows from the LiveData observer.
1016-1028: Weighted progress approach.The phase-based progress calculation improves accuracy across multiple phases. However, watch out for possible integer division issues (e.g., if totalPhases changes) and ensure no overhead if caching becomes more complex than three phases.
app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt (9)
7-8: Minor import and flow changes detected.Also applies to: 10-15, 18-18
52-53: Switch to MutableSharedFlow for completion status.Using a shared flow can be helpful for broadcasting completion events to multiple consumers without retaining the last state. This is appropriate if you only need to emit ephemeral updates.
55-56: SharedFlow for cached entity state.Same principle: ephemeral events about cached states. Ensure you do not need the last known value after configuration changes.
64-65: New invalidation request name constant.Constant naming is consistent with existing patterns. No issue found.
72-82: UniqueWorkRequest with KEEP policy.Using
.KEEPavoids duplicating tasks, which is valuable if a prior prime job is running. Ensure the logic handles subsequent requests that might be valid but get ignored due to the keep policy.
89-101: scheduleEntityCacheInvalidation added.Expedited work is used here but falls back to non-expedited if quota is exceeded. This approach is valid for critical tasks. Confirm that using
ExistingWorkPolicy.REPLACEis intended to override any still-running invalidation tasks.
135-136: Optional factory parameter in primeEntityCacheForDetail.This addition mirrors the pattern in
EntityLoaderHelper. Fine as long as call sites provide the correct factory when required.
140-140: Delegates to primeCacheForDetail with new param.Straightforward forwarding call. No issues identified.
189-189: Null default for async factory.Consistent with the rest of the changes. No immediate concerns.
PR for merging CommCare 2.56 changes into master
cross-request: dimagi/commcare-core#1468