Primes Entity cache ahead of time #2928
Conversation
…e process if necessary
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive enhancement to entity loading and caching mechanisms in the CommCare application. The changes span multiple files and focus on improving the user experience during entity selection and loading processes. Key modifications include adding a new progress container in the layout, introducing new string resources for loading states, and implementing a sophisticated entity caching system. The changes introduce several new classes and interfaces such as The modifications also update existing activities and tasks like Suggested Labels
Suggested Reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 4
🧹 Nitpick comments (17)
app/src/org/commcare/tasks/EntityLoaderListener.java (1)
17-17: Add documentation for the progress reporting method.The new method aligns well with the interface's purpose, but would benefit from JavaDoc comments explaining:
- The meaning of the varargs values (e.g., current progress, total items)
- Expected value ranges
- When this method will be called during the loading process
+ /** + * Reports progress during entity loading operations + * @param values Progress indicators. Typically [current, total] but may include additional metrics + */ void deliverProgress(Integer... values);app/src/org/commcare/tasks/EntityLoadingProgressListener.java (1)
3-6: Add documentation to clarify usage and thread safety requirements.The interface would benefit from comprehensive documentation to guide implementers. Consider adding:
- Interface purpose and usage context
- Parameter constraints (e.g., valid ranges for progress and total)
- Thread safety requirements for implementations
Add Javadoc documentation:
package org.commcare.tasks; +/** + * Listener interface for reporting progress during entity loading operations. + * Implementations should be thread-safe as progress updates may come from + * background threads. + */ public interface EntityLoadingProgressListener { + /** + * Reports the current progress of entity loading operation. + * + * @param progress Current number of entities processed (must be >= 0) + * @param total Total number of entities to process (must be >= progress) + * @throws IllegalArgumentException if progress < 0 or total < progress + */ void publishEntityLoadingProgress(int progress, int total); }app/src/org/commcare/tasks/EntityLoaderHelper.kt (3)
42-45: Clarify null returns when no entities are loaded
When loadEntities returns null (if no entities are loaded), it could lead to potential NullPointerExceptions in the caller if not carefully handled. Ensure that all call sites properly handle a null result.
49-51: Ensure debug traces are turned off in production
factory.printAndClearTraces("build") might cause unnecessary overhead in production. Consider tying this call to a debug or feature flag.
80-85: Consider early loop termination for performance
Within loadEntitiesWithReferences, you’re iterating through every reference but stopLoading can cause early termination. If you anticipate frequent partial loading, consider a break statement or more explicit check to reduce overhead.app/src/org/commcare/tasks/EntityLoaderTask.java (3)
21-22: Favor composition or interface over inheritance
EntityLoaderTask extends ManagedAsyncTask. Evaluate if composition or an independent approach might simplify code. This might reduce coupling to ManagedAsyncTask’s life cycle and internal implementation details.
63-65: Consider logging or raising an exception for null results
In onPostExecute, if result is null, the method silently returns. Recommend logging or handling the case more explicitly so that the user or maintainer knows why the result was null.
117-125: Check nullability before using listener
In onProgressUpdate(), you’re calling listener.deliverProgress(values). While it typically won’t be null when onProgressUpdate is triggered, consider a quick safety check to avoid potential NullPointerExceptions.app/src/org/commcare/entity/AndroidAsyncNodeEntityFactory.kt (1)
28-55: Balance blocking with user experience
prepareEntitiesInternal uses waitForCachePrimeWork if another prime is in progress. This might block user interactions for up to TWO_MINUTES. Explore a non-blocking or more user-friendly strategy (e.g., progress indicator with cancel option or partial data loading).app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt (3)
25-27: Enforce concurrency constraints
This singleton approach might not be fully thread-safe if multiple threads simultaneously call getInstance(). Kotlin’s double-checked locking is generally safe, but combining it with clearing instance = null (line 151) can reintroduce concurrency concerns.
120-133: Review scheduling logic
primeCacheForDetail attempt to handle caching for either sessionDatum or a list of entities. If neither is provided, the function returns early without caching. Consider logging or clarifying the fallback scenario.
159-162: Check impacts of clearing state in cancel()
cancel() calls clearState(), which sets instance to null. This means a new instance can be created soon after. Ensure that is the intended design, or consider a more persistent approach that retains partial states.app/src/org/commcare/entity/PrimeEntityCacheListener.kt (2)
3-6: Consider using Kotlin's Pair instead of Android's PairThe import of
android.util.Paircould be replaced with Kotlin's built-inPairclass for better Kotlin interoperability.-import android.util.Pair +import kotlin.Pair
7-13: Add KDoc documentation to interface and methodThe interface and method would benefit from documentation explaining:
- The purpose of the interface
- When callbacks are triggered
- The meaning of currentDetailInProgress
- The structure and purpose of cachedEntitiesWithRefs
+/** + * Listener interface for entity cache priming completion events. + */ interface PrimeEntityCacheListener { + /** + * Called when entity cache priming completes for a detail + * @param currentDetailInProgress The identifier of the detail that was cached + * @param cachedEntitiesWithRefs Pair of cached entities and their corresponding references + */ fun onPrimeEntityCacheComplete( currentDetailInProgress: String, cachedEntitiesWithRefs: Pair<List<Entity<TreeReference>>, List<TreeReference>> )app/src/org/commcare/tasks/PrimeEntityCache.kt (1)
21-23: Consider logging cancellation eventsAdd logging when the worker is stopped to aid in debugging.
override fun onStopped() { + Logger.debug("PrimeEntityCache worker stopped") PrimeEntityCacheHelper.getInstance().cancel() }app/src/org/commcare/fragments/EntitySubnodeDetailFragment.java (1)
107-110: Document why progress updates are ignoredThe empty implementation with "nothing to do" comment doesn't explain the rationale. Consider either:
- Implementing progress updates for better UX
- Adding documentation explaining why progress updates are intentionally ignored
@Override public void deliverProgress(Integer[] values) { - // nothing to do + // Progress updates are not shown in subnode detail view as they would + // interfere with the parent view's progress indication }app/src/org/commcare/tasks/DataPullTask.java (1)
440-440: Consider adding error handling for cache schedulingWhile scheduling cache priming after recovery is logical, consider adding error handling to prevent any scheduling failures from affecting the recovery process.
- PrimeEntityCacheHelper.schedulePrimeEntityCacheWorker(); + try { + PrimeEntityCacheHelper.schedulePrimeEntityCacheWorker(); + } catch (Exception e) { + Logger.log(LogTypes.TYPE_WARNING, + "Failed to schedule entity cache priming: " + e.getMessage()); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/res/layout/entity_select_layout.xml(2 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/CommCareApplication.java(3 hunks)app/src/org/commcare/activities/EntitySelectActivity.java(10 hunks)app/src/org/commcare/entity/AndroidAsyncNodeEntityFactory.kt(1 hunks)app/src/org/commcare/entity/PrimeEntityCacheListener.kt(1 hunks)app/src/org/commcare/fragments/EntitySubnodeDetailFragment.java(1 hunks)app/src/org/commcare/tasks/DataPullTask.java(1 hunks)app/src/org/commcare/tasks/EntityLoaderHelper.kt(1 hunks)app/src/org/commcare/tasks/EntityLoaderListener.java(1 hunks)app/src/org/commcare/tasks/EntityLoaderTask.java(3 hunks)app/src/org/commcare/tasks/EntityLoadingProgressListener.java(1 hunks)app/src/org/commcare/tasks/PrimeEntityCache.kt(1 hunks)app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt(1 hunks)
🔇 Additional comments (15)
app/src/org/commcare/tasks/EntityLoaderListener.java (1)
17-17: Verify implementation in all classes implementing EntityLoaderListener.
This interface change requires updates to all implementing classes.
✅ Verification successful
All implementing classes have been verified and are compatible
The search results show that all classes implementing EntityLoaderListener already have the correct deliverProgress method signature:
EntitySelectActivity: implementsdeliverProgress(Integer[] values)EntitySubnodeDetailFragment: implementsdeliverProgress(Integer[] values)
Both implementations are compatible with the interface method deliverProgress(Integer... values) as varargs and array parameters are interchangeable in Java.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all classes that implement EntityLoaderListener
ast-grep --pattern 'class $_ implements $$$EntityLoaderListener$$$'
# Find all classes that extend classes implementing EntityLoaderListener
ast-grep --pattern 'class $_ extends $impl {
$$$
}' | rg -l 'EntityLoaderListener'
# Search for existing deliverProgress implementations
ast-grep --pattern 'void deliverProgress(Integer... values) {
$$$
}'
Length of output: 227
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find implementations
# First, find files containing EntityLoaderListener
rg -l "EntityLoaderListener"
# Then look for deliverProgress implementations with context
rg -A 3 "deliverProgress.*\("
Length of output: 1463
app/src/org/commcare/tasks/EntityLoadingProgressListener.java (1)
5-5: Consider adding parameter validation in implementing classes.
The method parameters should be validated to ensure progress is non-negative and total is greater than or equal to progress.
Let's verify if any implementations already include parameter validation:
Consider creating an abstract base class that handles parameter validation:
public abstract class BaseEntityLoadingProgressListener implements EntityLoadingProgressListener {
@Override
public void publishEntityLoadingProgress(int progress, int total) {
if (progress < 0) {
throw new IllegalArgumentException("Progress must be non-negative");
}
if (total < progress) {
throw new IllegalArgumentException("Total must be greater than or equal to progress");
}
doPublishProgress(progress, total);
}
protected abstract void doPublishProgress(int progress, int total);
}app/src/org/commcare/tasks/EntityLoaderHelper.kt (2)
24-25: Use of mutable factory field can lead to concurrency issues
Since factory is publicly accessible and assigned during initialization, it might be susceptible to concurrency issues if multiple threads can change the factory or call its methods. Consider restricting direct access or using thread-safe patterns to protect state.
97-99: Review cancellation mechanism
stopLoading is set to true in cancel(), but other references to the object might continue execution if they don’t regularly check stopLoading. Make sure that all relevant tasks poll for stopLoading or otherwise handle the canceled state.
app/src/org/commcare/tasks/EntityLoaderTask.java (1)
28-32: Initialize listener consistently
You’re assigning the listener later in attachListener(), but you already have references to it in some places. Ensure the listener is always non-null, or add appropriate guards (e.g., if listener != null) to avoid race conditions.
app/src/org/commcare/entity/AndroidAsyncNodeEntityFactory.kt (1)
81-89: Null checks for detail.id
onPrimeEntityCacheComplete compares detail.id with currentDetailInProgress, but detail.id could be null if detail was not fully initialized. Ensure you handle or log the case where detail.id is null.
app/src/org/commcare/fragments/EntitySubnodeDetailFragment.java (1)
106-110: Verify if progress updates should be propagated
Since this is a subnode detail fragment, verify if progress updates should be propagated to the parent fragment/activity.
✅ Verification successful
Based on the search results, I can now provide a final response:
Empty progress implementation is appropriate for this fragment
The search results show that only EntitySelectActivity meaningfully implements progress updates by showing a loading message with progress counts. Since EntitySubnodeDetailFragment is a detail view component and not a list selection activity, it's appropriate for it to have an empty implementation of deliverProgress().
Key findings:
EntitySelectActivityuses progress updates to show loading status for entity list processing- No other implementations were found that propagate progress
- The fragment's purpose (showing entity details) doesn't require progress indication
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other implementations of EntityLoaderListener propagate progress
rg -t java "implements.*EntityLoaderListener" -A 10
rg -t java "deliverProgress.*values.*" -A 5
Length of output: 3941
app/res/layout/entity_select_layout.xml (1)
Line range hint 56-75: Well-structured progress container layout!
The progress container implementation follows Android layout best practices with proper positioning and styling.
app/res/values/strings.xml (1)
459-461: Well-defined localization strings!
The new string resources are properly structured with:
- Clear naming convention
- Correct use of string formatting
- Proper translatable attribute
app/src/org/commcare/activities/EntitySelectActivity.java (4)
169-170: LGTM: Progress text UI member variables
The addition of visibleView and progressTv member variables provides proper UI feedback during entity loading operations.
272-272: LGTM: Progress text initialization and visibility management
The progress text initialization in setupUI() and visibility management in setupUIFromAdapter() are properly implemented.
Also applies to: 361-361
481-481: LGTM: Entity loading progress updates
The progress text updates during entity loading provide clear user feedback, with appropriate throttling to prevent excessive UI updates.
Also applies to: 996-1003
904-906: LGTM: Progress text helper method
The setProgressText() helper method properly encapsulates progress text updates.
app/src/org/commcare/CommCareApplication.java (2)
392-392: LGTM: Entity cache Work Manager task cancellation
The PrimeEntityCacheHelper.cancelWork() call is properly integrated with the existing Work Manager task cleanup during user session termination.
802-802: LGTM: Entity cache Work Manager task scheduling
The PrimeEntityCacheHelper.schedulePrimeEntityCacheWorker() call is properly placed after user session initialization, alongside other background task scheduling.
| factory.prepareEntities(full); | ||
| factory.printAndClearTraces("build"); | ||
| return new Pair<>(full, references); | ||
| return entityLoaderHelper.loadEntities(nodeset[0], this); |
There was a problem hiding this comment.
Validate array index usage in doInBackground()
doInBackground performs nodeset[0]. Always verify that the nodeset array is not empty or null to avoid ArrayIndexOutOfBoundsException.
+ if (nodeset == null || nodeset.length == 0) {
+ // handle error or return null
+ }
return entityLoaderHelper.loadEntities(nodeset[0], this);Committable suggestion skipped: line range outside the PR's diff.
| private fun waitForCachePrimeWork( | ||
| entities: MutableList<Entity<TreeReference>>, | ||
| primeEntityCacheHelper: PrimeEntityCacheHelper | ||
| ) { | ||
| val startTime = System.currentTimeMillis() | ||
| while (!completedCachePrime && (System.currentTimeMillis() - startTime) < TWO_MINUTES) { | ||
| // wait for it to be completed | ||
| try { | ||
| Thread.sleep(100) | ||
| } catch (_: InterruptedException) { | ||
| } | ||
| } | ||
| if (!completedCachePrime) { | ||
| Logger.log(LogTypes.TYPE_MAINTENANCE, "Still Waiting for cache priming work to complete") | ||
| // confirm if we are still priming in the worker. If yes, wait more | ||
| // otherwise recall prepareEntitiesInternal to re-evaluate the best thing to do | ||
| if (primeEntityCacheHelper.isInProgress() && primeEntityCacheHelper.isDetailInProgress(detail.id)) { | ||
| waitForCachePrimeWork(entities, primeEntityCacheHelper) | ||
| } else { | ||
| prepareEntitiesInternal(entities) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid long sleep loops
waitForCachePrimeWork uses Thread.sleep(100) in a loop, potentially hogging system resources. Consider using notifications, signals, or coroutines for more efficient waiting.
| fun primeEntityCache() { | ||
| checkPreConditions() | ||
| primeEntityCacheForApp(CommCareApplication.instance().commCarePlatform) | ||
| clearState() | ||
| } |
There was a problem hiding this comment.
Potential race condition after clearState()
PrimeEntityCache sets inProgress to true, but calls clearState() at the end which sets instance to null. If primeEntityCache() is called multiple times in quick succession, concurrency issues might arise. Consider more robust synchronization.
| override fun doWork(): Result { | ||
| try { | ||
| PrimeEntityCacheHelper.getInstance().primeEntityCache() | ||
| return Result.success() | ||
| } catch (e: Exception) { | ||
| Logger.exception("Error while priming cache in worker", e) | ||
| } | ||
| return Result.failure() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and worker configuration
The error handling could be improved by:
- Catching specific exceptions
- Considering retry scenarios for recoverable errors
- Adding work constraints if needed (e.g., network, battery)
override fun doWork(): Result {
try {
PrimeEntityCacheHelper.getInstance().primeEntityCache()
return Result.success()
- } catch (e: Exception) {
+ } catch (e: IOException) {
Logger.exception("Error while priming cache in worker", e)
+ // Retry for network-related issues
+ return Result.retry()
+ } catch (e: Exception) {
+ Logger.exception("Unrecoverable error while priming cache in worker", e)
+ return Result.failure()
}
- return Result.failure()
}Consider adding work constraints in the scheduling code:
val constraints = Constraints.Builder()
.setRequiredNetworkType(NetworkType.CONNECTED)
.setRequiresBatteryNotLow(true)
.build()
Summary
https://dimagi.atlassian.net/browse/SC-4055
Spec
Makes 2 improvements for a Cache backed entity list -
Adds a progress indicator to show the loading progress
Uses a Work manager task to schedule entity list cache calculation ahead of time.
Tech Overview
Feature Flag
CACHE_AND_INDEX
PR Checklist
Automated test coverage
Safety story