Skip to content
1 change: 1 addition & 0 deletions app/assets/locales/android_translatable_strings.txt
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ sync.recover.started=Recovering local DB State. Please do not turn off the app!
form.entry.segfault=There was an unrecoverable error during form entry! If the problem persists, seek CommCare Support
form.entry.processing=Processing your Form
form.entry.processing.title=Processing Form
form.entry.loading.next.question=Loading next question…
form.entry.complete.save.success=Form successfully completed
form.entry.incomplete.save.success=Form saved as incomplete
form.entry.save.error=Sorry, form save failed. Please contact CommCare Support to look into the issue.
Expand Down
29 changes: 29 additions & 0 deletions app/res/layout/screen_form_entry.xml
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,35 @@
android:background="@color/transclucent_nearly_solid_grey"
android:clickable="true"
android:visibility="gone"/>

<FrameLayout
android:id="@+id/form_entry_loading_overlay"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:background="@color/transclucent_nearly_solid_grey"
android:clickable="true"
android:focusable="true"
android:visibility="gone">

<LinearLayout
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_gravity="center"
android:orientation="vertical"
android:gravity="center_horizontal">

<ProgressBar
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:indeterminate="true"/>

<TextView
android:id="@+id/form_entry_loading_label"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_marginTop="8dp"/>
</LinearLayout>
</FrameLayout>
</FrameLayout>


Expand Down
51 changes: 51 additions & 0 deletions app/src/org/commcare/activities/AsyncFormNavigator.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package org.commcare.activities

import androidx.lifecycle.LifecycleOwner
import androidx.lifecycle.lifecycleScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext

/**
* Runs form-navigation stepping on a background thread and delivers the
* resulting renderable event back on the main thread.
*/
class AsyncFormNavigator(
private val lifecycleOwner: LifecycleOwner,
private val stepWork: StepWork
) {

fun interface StepWork {
fun step(): NavResult
}

fun interface StartCallback {
fun onStart()
}

fun interface ResultCallback {
fun onResult(result: NavResult)
}

private var job: Job? = null
private var currentNavId: Long = 0

fun navigate(resuming: Boolean, onStart: StartCallback, onResult: ResultCallback) {
val navId = ++currentNavId
onStart.onStart()
job = lifecycleOwner.lifecycleScope.launch {
val result = withContext(Dispatchers.Default) { stepWork.step() }
if (navId != currentNavId) {
return@launch
}
onResult.onResult(result)
}
}

fun cancel() {
currentNavId++
job?.cancel()
job = null
}
Comment on lines +34 to +50

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, locate and read the AsyncFormNavigator.kt file
fd -t f AsyncFormNavigator.kt

Repository: dimagi/commcare-android

Length of output: 120


🏁 Script executed:

# Look at the file structure and content around lines 34-50
cat -n app/src/org/commcare/activities/AsyncFormNavigator.kt | head -60

Repository: dimagi/commcare-android

Length of output: 1723


🏁 Script executed:

# Check the entire class to understand the context better
wc -l app/src/org/commcare/activities/AsyncFormNavigator.kt

Repository: dimagi/commcare-android

Length of output: 123


🏁 Script executed:

# Find usages of AsyncFormNavigator
rg -t kotlin "AsyncFormNavigator" --max-count 20

Repository: dimagi/commcare-android

Length of output: 146


🏁 Script executed:

# Search for StepWork implementations to understand what step() does
rg -t kotlin "StepWork" -A 3 --max-count 20

Repository: dimagi/commcare-android

Length of output: 550


🏁 Script executed:

# Look for FormEntryActivity and how cancelNavigation is called
fd -t f FormEntryActivity.kt

Repository: dimagi/commcare-android

Length of output: 49


🏁 Script executed:

# Search for cancelNavigation usage
rg -t kotlin "cancelNavigation" -B 2 -A 2

Repository: dimagi/commcare-android

Length of output: 49


🏁 Script executed:

# Search for FormEntry related files
fd -t f "FormEntry" --iglob "*FormEntry*"

Repository: dimagi/commcare-android

Length of output: 269


🏁 Script executed:

# Check git log for AsyncFormNavigator to see when it was added
git log --oneline -- app/src/org/commcare/activities/AsyncFormNavigator.kt | head -5

Repository: dimagi/commcare-android

Length of output: 127


🏁 Script executed:

# Check git diff to see if this is recent
git log -1 --format="%ai" -- app/src/org/commcare/activities/AsyncFormNavigator.kt

Repository: dimagi/commcare-android

Length of output: 92


🏁 Script executed:

# Get the full diff for AsyncFormNavigator to see the context
git diff HEAD~1 -- app/src/org/commcare/activities/AsyncFormNavigator.kt

Repository: dimagi/commcare-android

Length of output: 97


🏁 Script executed:

# Check the commit details
git show --stat 21f76cc29

Repository: dimagi/commcare-android

Length of output: 50380


🏁 Script executed:

# Search for all recent files that might use AsyncFormNavigator
git diff HEAD~1 --name-only | head -20

Repository: dimagi/commcare-android

Length of output: 253


🏁 Script executed:

# Search for FormEntryActivityUIController
fd -t f "FormEntryActivityUIController.kt"

Repository: dimagi/commcare-android

Length of output: 49


🏁 Script executed:

# Search for where cancel() might be called
rg -t kotlin "\.cancel\(\)" -B 2 -A 2 | head -50

Repository: dimagi/commcare-android

Length of output: 2905


🏁 Script executed:

# Search for navigationInFlight mentioned in commit message
rg -t kotlin "navigationInFlight" -B 2 -A 2

Repository: dimagi/commcare-android

Length of output: 49


🏁 Script executed:

# Check if lifecycleScope.launch is synchronous or asynchronous
rg -t kotlin "lifecycleScope" -B 2 -A 5 | head -80

Repository: dimagi/commcare-android

Length of output: 2778


🏁 Script executed:

# Look for how cancel() is intended to be called based on the comment
rg -t kotlin "onPause\|cancelNavigation" -B 3 -A 3

Repository: dimagi/commcare-android

Length of output: 49


🏁 Script executed:

# Check what withContext does with blocking work
rg -t kotlin "withContext\(Dispatchers" -B 2 -A 5 | head -60

Repository: dimagi/commcare-android

Length of output: 5293


🏁 Script executed:

# Look for kotlin coroutine documentation references or patterns
rg -t kotlin "cancelAndJoin\|runBlocking" -B 2 -A 2

Repository: dimagi/commcare-android

Length of output: 49


🏁 Script executed:

# Check if there are any InternalCoroutinesApi violations in this file
rg -t kotlin "InternalCoroutinesApi|tryResume" app/src/org/commcare/activities/AsyncFormNavigator.kt

Repository: dimagi/commcare-android

Length of output: 49


🏁 Script executed:

# Verify the actual behavior of lifecycleScope.launch - is it synchronous or async?
# Check the common patterns in the codebase for launching and cancelling
rg -t kotlin "lifecycleScope.launch" -A 8 | head -100

Repository: dimagi/commcare-android

Length of output: 1545


🏁 Script executed:

# Look for any existing serialization guards or job handling patterns
rg -t kotlin "AtomicBoolean|atomic|synchronized" app/src/org/commcare/activities/ -B 2 -A 2

Repository: dimagi/commcare-android

Length of output: 49


🏁 Script executed:

# Check what FormEntryController is and if it's thread-safe
fd -t f "FormEntryController.kt" -o "FormEntryController.java"

Repository: dimagi/commcare-android

Length of output: 234


🏁 Script executed:

# Look for any synchronization patterns around FormEntryController mutations
rg -t kotlin -t java "FormEntryController" -B 2 -A 5 | head -80

Repository: dimagi/commcare-android

Length of output: 6475


🏁 Script executed:

# Verify kotlinx.coroutines version to understand cancellation semantics
rg -t gradle "kotlinx-coroutines" app/build.gradle

Repository: dimagi/commcare-android

Length of output: 143


🏁 Script executed:

# Check Kotlin coroutine documentation on lifecycleScope.launch behavior
rg -t kotlin "suspend\|launch" app/src/org/commcare/activities/AsyncFormNavigator.kt -B 2 -A 5

Repository: dimagi/commcare-android

Length of output: 49


🏁 Script executed:

# Verify if there's a real window where two navigate() calls could both start executing
# by checking if there are any barriers between navigate() calls
rg -t kotlin "navigate\(" -B 3 -A 8 --max-count 10

Repository: dimagi/commcare-android

Length of output: 1190


cancel() is not effective for blocking stepWork.step() and leaves a window for stale results.

Two related issues:

  1. stepWork.step() runs via withContext(Dispatchers.Default) but is pure blocking Java with no cooperative cancellation checks. job?.cancel() will only prevent onResult from firing (already guarded by navId), it will not stop the FormEntryController mutations on the background thread. Callers that assume a synchronous post-cancel state (e.g., FormEntryActivity.onPause()cancelNavigation()saveAnswersForCurrentScreen) will race with the still-running step on the shared, non-thread-safe controller.

  2. lifecycleScope.launch dispatches asynchronously even for the main body; between onStart.onStart() and entering the coroutine, another navigate() can be invoked, potentially overlapping work. Consider whether invocations must be serialized.

At minimum, consider job?.cancelAndJoin() from a runBlocking/suspend context at cancel time, or add an AtomicBoolean "navigating" guard and block new navigate calls until the previous step returns. Also consider capturing job before launch returns in a way that guarantees cancel() observes the latest job.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/src/org/commcare/activities/AsyncFormNavigator.kt` around lines 34 - 50,
The navigate/cancel flow leaves blocking Java work uncontrolled and races stale
results; fix by (1) running the blocking stepWork.step() on a dedicated
single-thread ExecutorService (store its Future in a field like stepFuture)
instead of relying on cooperative cancellation, call stepFuture.cancel(true) in
cancel() to attempt to interrupt the background thread, and ensure you stop any
FormEntryController mutations from that thread; (2) assign the coroutine Job to
job immediately by doing val assignedJob = lifecycleOwner.lifecycleScope.launch
{ ... } then job = assignedJob (or construct a Job and pass it into launch) so
cancel() always sees the latest job, and make cancel perform
job?.cancelAndJoin() (use runBlocking if cancel() must be synchronous) to wait
for the coroutine to finish; and (3) add a simple AtomicBoolean navigating guard
in navigate() to serialize invocations (set true at start, false on completion)
to prevent overlap between onStart/onResult windows. Ensure references to
stepWork.step(), lifecycleOwner.lifecycleScope.launch, job, currentNavId,
cancel(), and stepFuture/Future are updated accordingly.

}
2 changes: 2 additions & 0 deletions app/src/org/commcare/activities/FormEntryActivity.java
Original file line number Diff line number Diff line change
Expand Up @@ -911,6 +911,8 @@ public void taskCancelled() {
protected void onPause() {
super.onPause();

uiController.cancelNavigation();

if (!isFinishing() && uiController.questionsView != null && currentPromptIsQuestion()) {
saveAnswersForCurrentScreen(false);
}
Expand Down
166 changes: 102 additions & 64 deletions app/src/org/commcare/activities/FormEntryActivityUIController.java
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,12 @@ public class FormEntryActivityUIController implements CommCareActivityUIControll
private BlockingActionsManager blockingActionsManager;

private boolean formRelevanciesUpdateInProgress = false;
private boolean navigationInFlight = false;

private static final String KEY_LAST_CHANGED_WIDGET = "index-of-last-changed-widget";
private TextView finishText;
private FormLoadingOverlay formLoadingOverlay;
private AsyncFormNavigator asyncFormNavigator;

enum AnimationType {
LEFT, RIGHT, FADE
Expand Down Expand Up @@ -144,6 +147,13 @@ public void setupUI() {


mViewPane = activity.findViewById(R.id.form_entry_pane);
formLoadingOverlay = new FormLoadingOverlay(
activity.findViewById(R.id.form_entry_loading_overlay),
activity.findViewById(R.id.form_entry_loading_label),
Localization.get("form.entry.loading.next.question"));
asyncFormNavigator = new AsyncFormNavigator(
activity,
this::stepToRenderableEvent);

activity.requestMajorLayoutUpdates();

Expand Down Expand Up @@ -417,71 +427,90 @@ private void showNextView(boolean resuming) {
// Any info stored about the last changed widget is useless when we move to a new view
resetLastChangedWidget();

if (FormEntryActivity.mFormController.getEvent() != FormEntryController.EVENT_END_OF_FORM) {
int event;
if (FormEntryActivity.mFormController.getEvent() == FormEntryController.EVENT_END_OF_FORM) {
return;
}

try {
group_skip:
do {
event = FormEntryActivity.mFormController.stepToNextEvent(FormEntryController.STEP_OVER_GROUP);
switch (event) {
case FormEntryController.EVENT_QUESTION:
QuestionsView next = createView();
if (!resuming) {
showView(next, LocalePreferences.isLocaleRTL() ? AnimationType.LEFT : AnimationType.RIGHT);
} else {
showView(next, AnimationType.FADE, false);
}
break group_skip;
case FormEntryController.EVENT_END_OF_FORM:
// auto-advance questions might advance past the last form quesion
// In special case when questionsView is null (there is no question),
// to avoid exit dialog without saving option, shown from refreshCurrentView(),
// save and exit method called.
if (questionsView != null) {
refreshCurrentView();
} else {
activity.triggerUserFormComplete();
}
break group_skip;
case FormEntryController.EVENT_PROMPT_NEW_REPEAT:
createRepeatDialog();
break group_skip;
case FormEntryController.EVENT_GROUP:
//We only hit this event if we're at the _opening_ of a field
//list, so it seems totally fine to do it this way, technically
//though this should test whether the index is the field list
//host.
if (FormEntryActivity.mFormController.indexIsInFieldList()
&& FormEntryActivity.mFormController.getQuestionPrompts().length != 0) {
QuestionsView nextGroupView = createView();
if (!resuming) {
showView(nextGroupView, LocalePreferences.isLocaleRTL() ? AnimationType.LEFT : AnimationType.RIGHT);
} else {
showView(nextGroupView, AnimationType.FADE, false);
}
break group_skip;
}
// otherwise it's not a field-list group, so just skip it
break;
case FormEntryController.EVENT_REPEAT:
Log.i(TAG, "repeat: " + FormEntryActivity.mFormController.getFormIndex().getReference());
// skip repeats
break;
case FormEntryController.EVENT_REPEAT_JUNCTURE:
Log.i(TAG, "repeat juncture: "
+ FormEntryActivity.mFormController.getFormIndex().getReference());
// skip repeat junctures until we implement them
break;
default:
Log.w(TAG,
"JavaRosa added a new EVENT type and didn't tell us... shame on them.");
break;
}
} while (event != FormEntryController.EVENT_END_OF_FORM);
} catch (XPathException e) {
new UserfacingErrorHandling<>().logErrorAndShowDialog(activity, e, FormEntryConstants.EXIT);
navigationInFlight = true;
asyncFormNavigator.navigate(
resuming,
formLoadingOverlay::show,
result -> {
navigationInFlight = false;
formLoadingOverlay.hide();
renderNavResult(result, resuming);
});
}

/**
* Walks the form forward via {@link FormEntryController#stepToNextEvent} until a renderable
* event is reached. Pure controller-state access; no view creation or UI side effects.
*/
private NavResult stepToRenderableEvent() {
try {
while (true) {
int event = FormEntryActivity.mFormController.stepToNextEvent(FormEntryController.STEP_OVER_GROUP);
switch (event) {
case FormEntryController.EVENT_QUESTION:
return NavResult.Question.INSTANCE;
case FormEntryController.EVENT_END_OF_FORM:
return NavResult.EndOfForm.INSTANCE;
case FormEntryController.EVENT_PROMPT_NEW_REPEAT:
return NavResult.PromptNewRepeat.INSTANCE;
case FormEntryController.EVENT_GROUP:
//We only hit this event if we're at the _opening_ of a field
//list, so it seems totally fine to do it this way, technically
//though this should test whether the index is the field list
//host.
if (FormEntryActivity.mFormController.indexIsInFieldList()
&& FormEntryActivity.mFormController.getQuestionPrompts().length != 0) {
return NavResult.FieldListGroup.INSTANCE;
}
// otherwise it's not a field-list group, so just skip it
break;
case FormEntryController.EVENT_REPEAT:
Log.i(TAG, "repeat: " + FormEntryActivity.mFormController.getFormIndex().getReference());
// skip repeats
break;
case FormEntryController.EVENT_REPEAT_JUNCTURE:
Log.i(TAG, "repeat juncture: "
+ FormEntryActivity.mFormController.getFormIndex().getReference());
// skip repeat junctures until we implement them
break;
default:
Log.w(TAG,
"JavaRosa added a new EVENT type and didn't tell us... shame on them.");
break;
}
}
} catch (XPathException e) {
return new NavResult.Error(e);
}
}
Comment on lines +449 to +489

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Only XPathException is caught — other runtime exceptions from stepping will crash the coroutine.

FormEntryController.stepToNextEvent, indexIsInFieldList, and getQuestionPrompts can throw other unchecked exceptions (e.g., XPathTypeMismatchException extends XPathException so OK, but generic RuntimeException from evaluator or IllegalStateException do not). On the old synchronous path those propagated up to callers that wrapped the flow with UserfacingErrorHandling. Here, an uncaught exception inside the Dispatchers.Default coroutine will be swallowed/logged by the coroutine exception handler and the overlay will never be hidden, leaving the UI stuck with navigationInFlight=true and the overlay visible after 150 ms.

Consider widening the catch to Throwable (or at least RuntimeException) and surfacing via NavResult.Error, or wrapping the navigate callback to guarantee navigationInFlight/overlay are cleared on any failure.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/src/org/commcare/activities/FormEntryActivityUIController.java` around
lines 449 - 489, The stepToRenderableEvent method currently only catches
XPathException, so unchecked exceptions from
FormEntryController.stepToNextEvent/indexIsInFieldList/getQuestionPrompts can
crash the coroutine and leave navigationInFlight/overlay stuck; widen the catch
to Throwable (or RuntimeException) in stepToRenderableEvent and return new
NavResult.Error(throwable) so all failures surface as NavResult.Error, or
alternatively wrap the caller's navigate callback to always clear
navigationInFlight and hide the overlay in a finally block—update references in
stepToRenderableEvent, NavResult.Error, and the navigate/navigationInFlight
overlay handling to ensure any thrown Throwable is converted to NavResult.Error
or always clears the in-flight state.


private void renderNavResult(NavResult result, boolean resuming) {
if (result instanceof NavResult.Question || result instanceof NavResult.FieldListGroup) {
QuestionsView next = createView();
if (!resuming) {
showView(next, LocalePreferences.isLocaleRTL() ? AnimationType.LEFT : AnimationType.RIGHT);
} else {
showView(next, AnimationType.FADE, false);
}
} else if (result instanceof NavResult.PromptNewRepeat) {
createRepeatDialog();
} else if (result instanceof NavResult.EndOfForm) {
// auto-advance questions might advance past the last form question
// In special case when questionsView is null (there is no question),
// to avoid exit dialog without saving option, shown from refreshCurrentView(),
// save and exit method called.
if (questionsView != null) {
refreshCurrentView();
} else {
activity.triggerUserFormComplete();
}
} else if (result instanceof NavResult.Error) {
new UserfacingErrorHandling<>().logErrorAndShowDialog(
activity, ((NavResult.Error)result).getException(), FormEntryConstants.EXIT);
}
}

Expand Down Expand Up @@ -598,7 +627,16 @@ protected void next() {
}

protected boolean shouldIgnoreNavigationAction() {
return blockingActionsManager.isBlocked();
return blockingActionsManager.isBlocked() || navigationInFlight;
}

protected void cancelNavigation() {
if (!navigationInFlight) {
return;
}
asyncFormNavigator.cancel();
navigationInFlight = false;
formLoadingOverlay.hide();
}

protected boolean shouldIgnoreSwipeAction() {
Expand Down
40 changes: 40 additions & 0 deletions app/src/org/commcare/activities/FormLoadingOverlay.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package org.commcare.activities

import android.os.Handler
import android.os.Looper
import android.view.View
import android.widget.TextView

/**
* Wraps the form-entry loading overlay view. [show] posts a delayed
* visibility change so fast navigations don't flash the overlay; [hide]
* cancels any pending show and hides the view immediately.
*/
class FormLoadingOverlay @JvmOverloads constructor(
private val overlayView: View,
labelView: TextView,
labelText: String,
private val showDelayMillis: Long = DEFAULT_SHOW_DELAY_MILLIS
) {

private val handler = Handler(Looper.getMainLooper())
private val showRunnable = Runnable { overlayView.visibility = View.VISIBLE }

init {
labelView.text = labelText
}

fun show() {
handler.removeCallbacks(showRunnable)
handler.postDelayed(showRunnable, showDelayMillis)
}

fun hide() {
handler.removeCallbacks(showRunnable)
overlayView.visibility = View.GONE
}

companion object {
const val DEFAULT_SHOW_DELAY_MILLIS: Long = 150
}
}
14 changes: 14 additions & 0 deletions app/src/org/commcare/activities/NavResult.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package org.commcare.activities

import org.javarosa.xpath.XPathException

/**
* Outcome of stepping the form to the next renderable event.
*/
sealed class NavResult {
object Question : NavResult()
object FieldListGroup : NavResult()
object PromptNewRepeat : NavResult()
object EndOfForm : NavResult()
data class Error(val exception: XPathException) : NavResult()
}
Loading