Fixes for Entity Cache and Lazy loading work #2998
Conversation
…n-async) case list
…ues emitted after we subscribe
📝 WalkthroughWalkthroughThe changes update the entity list progress reporting and the handling of entity loading in multiple modules. In the XML resource file, several string resources were removed and replaced with updated versions, consolidating the loading messages and updating the finishing message. In the activity handling entity selection, a null check was added in the progress observation method and the progress calculation was refactored to a weighted approach with a unified progress message. In the entity loader helper, a control flow change now cancels background cache work when using non-asynchronous factories and later reschedules the cache worker if needed. Changes in the prime entity cache and its helper improve null-safety and shift the state update mechanism from StateFlow to SharedFlow, with progress states updated to allow nullable values. Sequence Diagram(s)sequenceDiagram
participant ESA as EntitySelectActivity
participant PCH as PrimeEntityCacheHelper
participant UI as User Interface
ESA->>ESA: observePrimeCacheWorker (null check on triple)
PCH-->>ESA: Emit weighted progress update
ESA->>UI: setProgressText(with unified message)
sequenceDiagram
participant ELH as EntityLoaderHelper
participant Factory as EntityFactory
participant CacheHelper as PrimeEntityCacheHelper
ELH->>Factory: Check if AsyncNodeEntityFactory?
alt Factory is NOT Async
ELH->>CacheHelper: cancelWork()
end
ELH->>ELH: Load entities within try block
alt Post-load and factory is NOT Async
ELH->>CacheHelper: schedulePrimeEntityCacheWorker()
end
Possibly related PRs
Suggested labels
✨ 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: 1
🧹 Nitpick comments (4)
app/src/org/commcare/tasks/EntityLoaderHelper.kt (1)
58-61: Improved control flow for synchronous entity loading.The code now properly cancels background cache work when using non-asynchronous entity factories, preventing potential database lock issues.
Consider adding a comment explaining why background cache work needs to be canceled specifically for synchronous factories to improve code maintainability.
app/src/org/commcare/activities/EntitySelectActivity.java (1)
1016-1029: Enhanced progress calculation with weighted approach.The progress calculation has been significantly improved with a weighted approach that accounts for multiple phases when caching is enabled. This provides a smoother progress experience for users.
Consider extracting this progress calculation logic to a separate method for better readability and maintainability. Also, adding comments explaining the weighting formula would be beneficial for future developers.
- int phaseProgress = values[1] * 100 / values[2]; - int totalPhases = 1; - if(shortSelect.isCacheEnabled()){ - // with caching we deliver progress in 3 separate phases - totalPhases = 3; - } - int weightedProgress = (phase.getValue() - 1) * 100 / totalPhases + phaseProgress / totalPhases; - if (weightedProgress != lastProgress) { - lastProgress = weightedProgress; - String progressDisplay = weightedProgress + "%"; - setProgressText( - StringUtils.getStringRobust(this, R.string.entity_list_loading, - new String[]{progressDisplay})); - } + // Calculate the weighted progress across multiple phases + private int calculateWeightedProgress(EntityLoadingProgressListener.EntityLoadingProgressPhase phase, int current, int total) { + int phaseProgress = current * 100 / total; + int totalPhases = shortSelect.isCacheEnabled() ? 3 : 1; + // Formula: progress = (currentPhase - 1) * percentPerPhase + (currentProgress / totalPhases) + return (phase.getValue() - 1) * 100 / totalPhases + phaseProgress / totalPhases; + } + + @Override + public void deliverProgress(Integer[] values) { + EntityLoadingProgressListener.EntityLoadingProgressPhase phase = + EntityLoadingProgressListener.EntityLoadingProgressPhase.fromInt(values[0]); + int weightedProgress = calculateWeightedProgress(phase, values[1], values[2]); + if (weightedProgress != lastProgress) { + lastProgress = weightedProgress; + String progressDisplay = weightedProgress + "%"; + setProgressText( + StringUtils.getStringRobust(this, R.string.entity_list_loading, + new String[]{progressDisplay})); + } + }app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt (2)
119-121: Using runBlocking for flow emissions may cause thread blocking.The implementation uses
runBlockingto synchronously emit values to flows from non-suspending contexts. While this works, it blocks the current thread until emission completes, which could be problematic if these methods are called from the main UI thread.Consider alternatives:
- Make these methods suspending functions (
suspend fun) to better integrate with coroutines- Use a CoroutineScope with an appropriate dispatcher to avoid blocking the current thread
Example refactoring using CoroutineScope:
+ private val scope = CoroutineScope(Dispatchers.IO) fun primeEntityCache() { checkPreConditions() try { primeEntityCacheForApp(CommCareApplication.instance().commCarePlatform) } finally { clearState() - runBlocking { - _completionStatus.emit(true) - } + scope.launch { + _completionStatus.emit(true) + } } }Similar changes would apply to the other methods using
runBlocking.Also applies to: 141-143, 202-204
247-249: Non-null assertions may cause runtime exceptions.The code uses non-null assertions (
currentDatumInProgress!!andcurrentDetailInProgress!!) when publishing entity loading progress. While the logic flow suggests these should never be null during this method call, it would be more robust to handle potential null cases safely.override fun publishEntityLoadingProgress( phase: EntityLoadingProgressListener.EntityLoadingProgressPhase, progress: Int, total: Int ) { + val datumId = currentDatumInProgress + val detailId = currentDetailInProgress + if (datumId != null && detailId != null) { _progressState.postValue( Triple( - currentDatumInProgress!!, - currentDetailInProgress!!, + datumId, + detailId, arrayOf(phase.value, progress, total) ) ) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/res/values/strings.xml(1 hunks)app/src/org/commcare/activities/EntitySelectActivity.java(2 hunks)app/src/org/commcare/tasks/EntityLoaderHelper.kt(2 hunks)app/src/org/commcare/tasks/PrimeEntityCache.kt(1 hunks)app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt(5 hunks)
🔇 Additional comments (9)
app/src/org/commcare/tasks/EntityLoaderHelper.kt (2)
62-69: LGTM: Clean encapsulation of entity loading logic.Wrapping the entity loading process in a try block is a good practice, ensuring proper execution flow and error handling.
70-74: Good resource management with task rescheduling.The finally block ensures that we restart the canceled task after entity loading is complete, maintaining proper cache state regardless of the execution path.
app/res/values/strings.xml (1)
471-472: Improved user-facing progress messages.The new string resources provide more consistent and clear messaging for the entity list loading process. The terminology change from "cases" to "list" makes the messages more generic and applicable across different contexts.
app/src/org/commcare/activities/EntitySelectActivity.java (1)
499-500: Fixed potential null pointer exception.Added a null check before accessing the triple's first element, improving robustness of the progress observation method.
app/src/org/commcare/tasks/PrimeEntityCache.kt (2)
14-14: Made primeEntityCacheHelper nullable.This change allows for better null safety handling in the codebase.
26-26: Improved null safety with safe call operators.Using the safe call operator
?.when calling methods on potentially null objects prevents NullPointerExceptions.Also applies to: 32-32
app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt (3)
10-15: Flow type imports aligned with the event-based approach.The imports have been updated to support the transition from
StateFlowtoSharedFlow, along with the required flow operators and coroutine utilities. This shift indicates a move toward a more event-driven approach for state management.
51-55:Details
❓ Verification inconclusive
Changed from StateFlow to SharedFlow for better event handling.
The transition from
MutableStateFlowtoMutableSharedFlowwithreplay = 0for both_completionStatusand_cachedEntitiesStateis appropriate for an event-based approach rather than maintaining persistent state. This change aligns well with the notification pattern where subscribers only need to receive new events rather than access a current state value.However, with
replay = 0, new subscribers won't receive previously emitted values. Make sure that collectors aren't depending on receiving the most recent state immediately upon subscription.
🏁 Script executed:
#!/bin/bash # Check if there are any subscribers that might expect to receive the latest state immediately rg -A 5 -B 5 "collect(.*completionStatus|.*cachedEntitiesState" --include "*.kt" --include "*.java"Length of output: 543
Action Required: Confirm Collector Behavior with Updated Query
The transition from
MutableStateFlowtoMutableSharedFlow(withreplay = 0) is appropriate for handling events rather than maintaining persistent state. However, please verify that none of the collectors rely on receiving a retained (latest) value immediately upon subscription. If any part of the code subscribes to eithercompletionStatusorcachedEntitiesStateexpecting an initial value, adjustments will be necessary.
- Confirm that no component collects from these flows expecting a replayed value.
- If such a dependency exists, consider modifying the replay configuration or reintroducing a state-based approach.
To aid in this verification, please run the updated RG command below, which uses the appropriate include flags to search for collectors in Kotlin and Java files:
#!/bin/bash rg -A 5 -B 5 "collect(.*completionStatus|.*cachedEntitiesState" -g "*.kt" -g "*.java"
57-58:Details
❓ Verification inconclusive
Made progressState nullable to handle reset scenarios.
Making
progressStateexplicitly nullable (LiveData<Triple<String, String, Array<Int>>?>) suggests that progress reporting can be reset or might be optional in certain scenarios. This change seems reasonable, especially since_progressStateis initialized with null.Ensure that all observers of this LiveData properly handle null values to prevent runtime exceptions.
🏁 Script executed:
#!/bin/bash # Check observers of progressState to ensure they handle null values rg -A 5 "observe.*progressState" --include "*.kt" --include "*.java"Length of output: 512
Subject: Verify Observer Handling for Nullable progressState
The change to make
progressStatenullable is valid for handling reset scenarios. However, please ensure that all observers of this LiveData in the codebase explicitly handle null values to avoid potential runtime issues. As the initial shell command failed due to incorrect flag usage, the updated script above should be run to verify that observer implementations properly handle null.
🏁 Script executed:
#!/bin/bash # Re-run observer check with correct file inclusion flags rg -A 5 "observe.*progressState" -g "*.kt" -g "*.java"Length of output: 178
Inspection Notice: Review Observer Handling for Nullable progressState
The change to make
progressStatenullable is valid for handling reset scenarios. However, our automated observer search did not return any usage, so it's unclear whether existing observers explicitly check for null values. Please manually verify that any observer implementations forprogressStatecorrectly handle null to avoid runtime exceptions.
| if (CommCareApplication.isSessionActive()) { | ||
| primeEntityCacheHelper = CommCareApplication.instance().currentApp.primeEntityCacheHelper | ||
| primeEntityCacheHelper.primeEntityCache() | ||
| primeEntityCacheHelper!!.primeEntityCache() |
There was a problem hiding this comment.
Risk of NullPointerException with not-null assertion.
The use of the not-null assertion operator !! could potentially cause a runtime exception if primeEntityCacheHelper is null.
Consider using a safer approach that handles the null case gracefully:
- primeEntityCacheHelper!!.primeEntityCache()
+ primeEntityCacheHelper?.let {
+ it.primeEntityCache()
+ return Result.success()
+ }
+ return Result.failure()📝 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.
| primeEntityCacheHelper!!.primeEntityCache() | |
| primeEntityCacheHelper?.let { | |
| it.primeEntityCache() | |
| return Result.success() | |
| } | |
| return Result.failure() |
avazirna
left a comment
avazirna
left a comment
There was a problem hiding this comment.
@shubham1g5 left a couple of questions.
| return Pair<List<Entity<TreeReference>>, List<TreeReference>>(entities, references) | ||
| if (factory !is AsyncNodeEntityFactory) { | ||
| // if we are into synchronous mode, cancel background cache work for now to not lock the user db | ||
| CommCareApplication.instance().currentApp.primeEntityCacheHelper.cancelWork() |
There was a problem hiding this comment.
Two questions:
- Given that
cancelUniqueWork()is async, are we certain that while the cancellation is not complete, there won't be any db locking issues? - It seems that calling
cancelLoading()in sync mode leads to its default implementation which throws anRuntimeException. Is that expected?
There was a problem hiding this comment.
- Db might block for a little bit (probably a fraction of second) while the cancellation completes but should be able to get the lock as soon as the canncelled thread interrupts and release the lock.
- Yeah that's expected, as we only call this method for it's Async implementations.
|
@damagatchi retest this please |
2 similar comments
|
@damagatchi retest this please |
|
@damagatchi retest this please |
Technical Summary
This PR makes following changes for cache and index work -
Feature Flag
CACHE AND LAZY LOADING
Safety Assurance
Safety story
All the impacted areas should be behind the feature flag.
Labels and Review