Primes Entity cache ahead of time #2928
Conversation
…e process if necessary
📝 WalkthroughWalkthroughThis pull request introduces comprehensive changes to improve entity loading and caching mechanisms in the CommCare application. The modifications span multiple files and include enhancements to the UI, string resources, and backend processes for entity selection and loading. Key changes involve adding a new progress container in the layout, introducing new string resources for loading states, implementing a new Sequence DiagramsequenceDiagram
participant User
participant EntitySelectActivity
participant EntityLoaderTask
participant PrimeEntityCacheHelper
participant EntityLoaderHelper
User->>EntitySelectActivity: Initiate Entity Selection
EntitySelectActivity->>EntityLoaderTask: Start Loading Entities
EntityLoaderTask->>EntityLoaderHelper: Load Entities
EntityLoaderHelper->>PrimeEntityCacheHelper: Check/Prime Cache
PrimeEntityCacheHelper-->>EntityLoaderHelper: Cache Status
EntityLoaderTask->>EntitySelectActivity: Publish Progress
EntitySelectActivity->>EntitySelectActivity: Update Progress UI
EntityLoaderTask->>EntitySelectActivity: Deliver Final Results
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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.
| 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()
|
@coderabbitai full review |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt (1)
175-181:⚠️ Potential issue** Potential race condition after
clearState()**
This code setsinstancetonullinclearState(), allowing a new instance to be created while old tasks might still be shutting down. To avoid concurrency pitfalls, consider a more robust approach, such as an internal immutable state or a dedicated “shutdown” lifecycle state that awaited tasks can reference.
🧹 Nitpick comments (11)
app/src/org/commcare/tasks/EntityLoaderHelper.kt (3)
Line range hint
28-36: Consider separating caching and async strategy decisions.The factory initialization combines two separate concerns (async strategy and caching) in a single condition. Consider:
- Separating these decisions for better maintainability
- Making the cache type configurable instead of hardcoding "case"
init { evalCtx.addFunctionHandler(EntitySelectActivity.getHereFunctionHandler()) - if (detail.useAsyncStrategy() || detail.shouldCache()) { + val shouldUseAsyncFactory = detail.useAsyncStrategy() || detail.shouldCache() + if (shouldUseAsyncFactory) { - val entityStorageCache: EntityStorageCache = CommCareEntityStorageCache("case") + val cacheType = detail.getCacheType() ?: "case" // Add getCacheType() to Detail + val entityStorageCache: EntityStorageCache = CommCareEntityStorageCache(cacheType) factory = AndroidAsyncNodeEntityFactory(detail, evalCtx, entityStorageCache) } else { factory = NodeEntityFactory(detail, evalCtx)
57-69: Consider adding progress tracking to cache priming.The caching methods look good but have two potential improvements:
- The
cacheEntities(nodeset: TreeReference)method doesn't use progress tracking, which might cause UI freezes during cache priming- The documentation could be more detailed about the caching behavior and its implications
Consider adding progress tracking:
-fun cacheEntities(nodeset: TreeReference): Pair<List<Entity<TreeReference>>, List<TreeReference>> { +fun cacheEntities( + nodeset: TreeReference, + progressListener: EntityLoadingProgressListener? = null +): Pair<List<Entity<TreeReference>>, List<TreeReference>> { val references = factory.expandReferenceList(nodeset) - val entities = loadEntitiesWithReferences(references, null) + val entities = loadEntitiesWithReferences(references, progressListener)
74-86: Consider making the progress phase configurable.The progress tracking implementation is good, but the phase is hardcoded to PHASE_PROCESSING. Consider making it configurable to support different loading phases (e.g., initialization, processing, finalization).
private fun loadEntitiesWithReferences( references: List<TreeReference>, - progressListener: EntityLoadingProgressListener? + progressListener: EntityLoadingProgressListener?, + phase: EntityLoadingProgressListener.EntityLoadingProgressPhase = + EntityLoadingProgressListener.EntityLoadingProgressPhase.PHASE_PROCESSING ) { // ... progressListener?.publishEntityLoadingProgress( - EntityLoadingProgressListener.EntityLoadingProgressPhase.PHASE_PROCESSING, + phase, index, references.size )app/src/org/commcare/tasks/EntityLoaderListener.java (1)
17-17: Consider adding Javadoc for the new method.Adding documentation would help implementers understand:
- The meaning of the varargs values (e.g., what each position represents)
- Expected value ranges
- When this callback will be triggered
+ /** + * Reports progress during entity loading operations. + * @param values Progress indicators. Could include completed items, total items, or percentage. + */ void deliverProgress(Integer... values);app/src/org/commcare/tasks/PrimeEntityCache.kt (3)
11-15: Consider more granular exception handling
Catching the genericExceptiontype might obscure different error scenarios (e.g., network vs. I/O issues). Using more specific exception types can help isolate recoverable errors and improve logging clarity.
17-19: Validate state transition infinallyblock
While clearing the state is crucial, ensure that the cache-priming logic remains consistent ifclearState()is called after a partial failure. Consider verifying whether partial results remain accessible or should be disposed of.
23-25: Proactively manage resource teardown upon stop
onStopped()currently callsPrimeEntityCacheHelper.getInstance().cancel(), but consider accounting for partial or pending state changes. Ensuring the helper is synchronized with the worker lifecycle avoids data inconsistencies.app/src/org/commcare/CommCareApplication.java (1)
802-802: Background scheduling of entity cache
Scheduling the prime entity cache upon session initialization helps improve performance. Confirm that feature flags (if any) do not inadvertently skip scheduling in scenarios where caching is required for a stable user experience.app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt (2)
76-80: Validate concurrency upon repeated cache prime calls
IfprimeEntityCache()is triggered multiple times in quick succession, concurrency checks might fail to detect repeated calls. Consider robust synchronization or queueing to ensure that partial primes do not overwrite or cancel each other inadvertently.
130-137: Swallowing XPath exceptions can mask important issues
While you skip one failing datum to continue with others, logs might not reveal the significance of repeated or critical XPath failures. Consider more granular logging or counters to help identify patterns of repeated errors.app/res/values/strings.xml (1)
459-463: LGTM with a minor suggestion for placeholder formatting.The string resources are well-structured, follow naming conventions, and are properly marked as translatable.
Consider using
%1$sinstead of%1sfor better clarity and consistency with Android's string formatting conventions. While both work, the$notation is preferred as it makes the parameter index explicit:- <string name="entity_list_processing" cc:translatable="true">Processing %1s out of %2s</string> + <string name="entity_list_processing" cc:translatable="true">Processing %1$s out of %2$s</string> - <string name="entity_list_calculating" cc:translatable="true">Calculating %1s out of %2s</string> + <string name="entity_list_calculating" cc:translatable="true">Calculating %1$s out of %2$s</string>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
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(11 hunks)app/src/org/commcare/entity/AndroidAsyncNodeEntityFactory.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(6 hunks)app/src/org/commcare/tasks/EntityLoaderListener.java(1 hunks)app/src/org/commcare/tasks/EntityLoaderTask.java(4 hunks)app/src/org/commcare/tasks/PrimeEntityCache.kt(1 hunks)app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt(1 hunks)
🧰 Additional context used
🪛 detekt (1.23.7)
app/src/org/commcare/entity/AndroidAsyncNodeEntityFactory.kt
[warning] 82-82: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (31)
app/src/org/commcare/tasks/EntityLoaderHelper.kt (3)
7-10: LGTM! Import and constructor changes align with new functionality.
The addition of progress tracking imports and the switch to AndroidAsyncNodeEntityFactory support the PR's objective of implementing progress indicators. The trailing comma in the constructor follows Kotlin conventions.
Also applies to: 19-19
Line range hint 42-54: LGTM! Clean integration of progress tracking.
The progress tracking integration is well-implemented:
- Progress listener is properly set in the factory
- Existing functionality is preserved
- Null safety is handled appropriately
103-105: LGTM! Proper cleanup in cancel method.
The addition of factory.cancelLoading() ensures proper cleanup of factory resources during cancellation.
app/src/org/commcare/tasks/EntityLoaderListener.java (1)
17-17: Method addition looks good, verify implementations.
The new progress callback method follows the interface's existing patterns and aligns with the PR objective of adding progress tracking for entity loading.
Let's verify all implementations of this interface to ensure they handle the new method:
✅ Verification successful
Implementation verification completed - all implementations are up to date
The verification shows that all implementations of EntityLoaderListener already have the deliverProgress method implemented:
EntitySelectActivityhas a full implementation that handles progress phasesEntitySubnodeDetailFragmenthas an empty implementation (with a comment indicating it's intentionally empty)
The interface addition is safe as all implementing classes have already been updated accordingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all implementations of EntityLoaderListener
ast-grep --pattern 'class $_ implements $$$EntityLoaderListener$$$'
Length of output: 69
Script:
#!/bin/bash
# Let's try a different approach to find implementations
# First, let's search for the interface definition to understand its full name
rg -l "interface.*EntityLoaderListener"
# Then search for class definitions that mention EntityLoaderListener
rg "class.*EntityLoaderListener" -A 2
Length of output: 500
Script:
#!/bin/bash
# Let's check the implementation in EntitySubnodeDetailFragment to see if it already implements the new method
rg "class EntitySubnodeDetailFragment" -A 50 app/src/org/commcare/fragments/EntitySubnodeDetailFragment.java
# Also search for any existing deliverProgress implementations
rg "deliverProgress.*\(" -A 2
Length of output: 3393
app/src/org/commcare/tasks/PrimeEntityCache.kt (1)
20-20: Revisit immediate Result.failure()
Returning Result.failure() as a fallback might hide specific errors from the WorkManager logs. Verifying the nature of the failure or providing more details (e.g., using Result.failure(Data)) can streamline debugging.
app/src/org/commcare/CommCareApplication.java (2)
100-101: Imports for new worker classes
These new imports for PrimeEntityCache and PrimeEntityCacheHelper align with the cache-priming logic. Ensure that any obfuscation or proguard rules are updated (if applicable) to preserve Worker classes.
392-392: Ensure concurrency handling when calling PrimeEntityCacheHelper.cancelWork()
Canceling work at user logout is a good approach. However, confirm whether any in-flight operations (like partial caches) need a different cleanup strategy.
app/src/org/commcare/entity/AndroidAsyncNodeEntityFactory.kt (1)
82-87: Re-execution loop might risk infinitely recursive calls
Catching TimeoutCancellationException and recursively calling observePrimeCacheWork() could lead to an unbounded wait in certain scenarios. Confirm whether a maximum retry or fallback path is necessary to prevent infinite recursion.
🧰 Tools
🪛 detekt (1.23.7)
[warning] 82-82: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
app/src/org/commcare/tasks/DataPullTask.java (1)
440-440: LGTM! Logical placement for cache priming.
The addition of PrimeEntityCacheHelper.schedulePrimeEntityCacheWorker() after successful recovery is appropriate as it ensures the entity cache is primed when the data is in a known good state.
Let's verify the integration with the new helper class:
✅ Verification successful
Integration with PrimeEntityCacheHelper is properly implemented
The verification confirms that:
PrimeEntityCacheHelperis a well-implemented Kotlin singleton class with the requiredschedulePrimeEntityCacheWorker()static method- The method schedules a one-time background work request using Android's WorkManager with proper uniqueness constraints
- The helper is used consistently across the codebase:
- In
DataPullTask.javaafter successful recovery - In
CommCareApplication.javaduring session initialization - Proper cleanup with
cancelWork()during session cleanup
- In
The placement in DataPullTask aligns with other usage patterns in the codebase, ensuring entity cache is primed when data is in a good state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the PrimeEntityCacheHelper class exists and has the required method
ast-grep --pattern 'class PrimeEntityCacheHelper {
$$$
static $_ schedulePrimeEntityCacheWorker() {
$$$
}
$$$
}'
Length of output: 126
Script:
#!/bin/bash
# Let's try a broader search to find the helper class and its implementation
rg -l "PrimeEntityCacheHelper"
# Also search for the method name to ensure it exists
rg "schedulePrimeEntityCacheWorker"
Length of output: 754
Script:
#!/bin/bash
# Let's examine the implementation of PrimeEntityCacheHelper
cat app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt
# Also check where else it's being used in CommCareApplication.java for context
rg -A 5 "PrimeEntityCacheHelper" app/src/org/commcare/CommCareApplication.java
Length of output: 8290
app/src/org/commcare/activities/EntitySelectActivity.java (12)
37-37: Import addition recognized
The import for EntityLoadingProgressListener is necessary to support progress callbacks and looks appropriate.
61-61: No issues found
This added import for StringUtils is consistent with usage in this file.
170-171: Helpful addition of new UI references
Defining visibleView and progressTv clarifies the code and makes progress reporting more straightforward.
273-273: UI initialization
Connecting progressTv to the corresponding layout element is correct.
362-362: Visibility toggled
Hiding the progress container after adapter setup is consistent with showing real content.
482-482: Loading status text
Using a localized string resource here is a good practice for i18n.
859-859: Finalizing status text
Setting a localized text here provides clear UI feedback.
881-881: Hide progress container
Ensuring the progress container is hidden after successful load keeps the UI clean.
905-907: New setter method
The dedicated setProgressText(String message) method improves readability and maintainability for progress updates.
935-935: Show progress container
Toggling visibility before performing long-running tasks is a sensible UX practice.
997-999: Implementing progress delivery
Implementing deliverProgress allows granular reporting of loading phases.
1000-1014: Sensible phased progress handling
Each phase’s logic is clear, and throttling updates at every 100th calculation helps reduce UI overhead.
app/src/org/commcare/tasks/EntityLoaderTask.java (5)
7-7: Import usage
Bringing in EntityLoadingProgressListener to enable detailed progress feedback is appropriate.
22-23: Interface alignment
Extending ManagedAsyncTask and implementing EntityLoadingProgressListener suitably integrates progress reporting into the async workflow.
117-117: No functional changes
This blank line is harmless and presumably for formatting.
118-126: Progress publishing
Forwarding progress through publishEntityLoadingProgress and onProgressUpdate aligns with standard AsyncTask patterns and is well-structured.
39-39:
Ensure nodeset safety
Before referencing nodeset[0], make sure nodeset is non-null and non-empty. Guard conditions would avoid potential ArrayIndexOutOfBoundsException.
@@ -37,6 +37,11 @@
protected Pair<List<Entity<TreeReference>>, List<TreeReference>> doInBackground(TreeReference... nodeset) {
try {
+ if (nodeset == null || nodeset.length == 0) {
+ // handle error, log warning, or return null
+ mException = new IllegalArgumentException("nodeset must not be empty or null");
+ return null;
+ }
return entityLoaderHelper.loadEntities(nodeset[0], this);
} catch (XPathException xe) {Likely invalid or redundant comment.
app/src/org/commcare/fragments/EntitySubnodeDetailFragment.java (1)
106-110: Method stub for deliverProgress
Currently does nothing, which is acceptable if progress updates aren’t relevant here. Revisit if subnode details require incremental feedback in the future.
app/res/layout/entity_select_layout.xml (4)
56-56: New container for progress UI
Introducing @+id/progress_container is a clear approach for controlling all progress-related widgets in one place.
67-67: ProgressBar style
Using style="?android:attr/progressBarStyleLarge" ensures a consistent look on various devices.
68-75: Text-based progress feedback
The TextView provides clarity to users about loading state. Good use of center alignment for readability.
78-78: No substantive changes
This blank line does not affect functionality or layout flow.
| override fun prepareEntitiesInternal( | ||
| entities: MutableList<Entity<TreeReference>> | ||
| ) { | ||
| if (detail.shouldCache()) { | ||
| // we only want to block if lazy load is not enabled | ||
| if (!detail.shouldLazyLoad()) { | ||
| val primeEntityCacheHelper = PrimeEntityCacheHelper.getInstance() | ||
| if (primeEntityCacheHelper.isInProgress()) { | ||
| // if we are priming something else at the moment, expedite the current detail | ||
| if (!primeEntityCacheHelper.isDetailInProgress(detail.id)) { | ||
| primeEntityCacheHelper.expediteDetailWithId( | ||
| getCurrentCommandId(), | ||
| detail, | ||
| entities, | ||
| progressListener | ||
| ) | ||
| } else { | ||
| primeEntityCacheHelper.setListener(progressListener) | ||
| observePrimeCacheWork(primeEntityCacheHelper, entities) | ||
| } | ||
| } else { | ||
| // we either have not started priming or already completed. In both cases | ||
| // we want to re-prime to make sure we calculate any uncalculated data first | ||
| primeEntityCacheHelper.primeEntityCacheForDetail( | ||
| getCurrentCommandId(), | ||
| detail, | ||
| entities, | ||
| progressListener | ||
| ) | ||
| } | ||
| } | ||
| } else { | ||
| super.prepareEntitiesInternal(entities) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Decouple blocking from UI for better responsiveness
Blocking the user when lazy load is disabled ensures data consistency. However, if any part of the UI uses this in the main thread, it might degrade performance. Ensure this logic runs on background threads to maintain a smooth UI experience.
There was a problem hiding this comment.
This code is only ever invoked from a background thread. Is there a way I can be explicit about that ?
There was a problem hiding this comment.
@shubham1g5, one straightforward approach is to document it both in your method’s Javadoc/KDoc and with a runtime check that asserts you’re not on the main thread. This way, future maintainers (and automated tests) will be aware of the requirement. For example, you could add something like this in Kotlin:
private fun ensureNotOnMainThread() {
check(Looper.myLooper() != Looper.getMainLooper()) {
"This method must not be called on the main thread."
}
}Then call ensureNotOnMainThread() at the beginning of your background-only function. This makes the requirement explicit and prevents accidental misuse.
There was a problem hiding this comment.
avoiding to do this for now as the code is shared with CLI which will actually calls this on main thread.
There was a problem hiding this comment.
@shubham1g5, understood. Thank you for clarifying the usage with the CLI. It makes sense to hold off on adding the runtime check in that scenario. If in the future you decide to split out the CLI logic or change the threading model, we can revisit the idea then. Let me know if anything else comes up.
…entially be associated with more than one case lists in the app
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