Improve logging around resizing in QuestionsView

[avazirna]

Product Description

This is to improve visibility around this issue. It seems that an issue occurs during layout updates when returning to CommCare after launching a third-party app.

Labels and Review

• Do we need to enhance the manual QA test coverage ? If yes, the "QA Note" label is set correctly
• Does the PR introduce any major changes worth communicating ? If yes, the "Release Note" label is set and a "Release Note" is specified in PR description.
• Risk label is set correctly
• The set of people pinged as reviewers is appropriate for the level of risk of the change

[coderabbitai]

📝 Walkthrough

Walkthrough

The scrollToWidget method in QuestionsView.java has been modified to add error handling and logging to its scroll operation. The previous single-line lambda that directly called scrollTo(0, widget.getTop()) has been replaced with a multi-line lambda that attempts to retrieve the widget's prompt text ID within a try-catch block, logs a soft assertion with that ID, and then performs the scroll operation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Rationale: Single-file change affecting one method. The modification introduces straightforward error handling (try-catch) and logging instrumentation with localized scope, requiring minimal context to verify correctness and understand intent.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The pull request description is significantly incomplete compared to the required template. While the author references a Crashlytics issue and mentions the motivation for the change, critical sections are missing or inadequately filled: the Technical Summary lacks design rationale and decision-making details; the entire Safety Assurance section (safety story, automated test coverage, and QA plan) is absent; and supporting details for the checked label review items are not provided. The Product Description is also vague about user-facing effects, though this may be acceptable given the logging-focused nature of the change.	The author should add the missing sections to the description: expand the Technical Summary to explain why these specific logging changes help investigate the layout/resume crash and detail the design decisions; provide a comprehensive Safety Assurance section including how the change was tested locally, why it is inherently safe, details of automated test coverage, and the QA plan with any related QA ticket links; and add specific details or links to support the checked checkbox items in the Labels and Review section (such as the QA ticket and Release Note content).
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The pull request title "Improve logging around scrolling in QuestionsView" directly and accurately reflects the main change in the changeset. The raw summary confirms that the modification focuses on enhancing the scrollToWidget method with improved logging, including try-catch handling and soft assertion logging. The title is concise, specific, and clearly indicates both what is being improved (logging) and where (scrolling functionality in QuestionsView). It avoids vague language and accurately summarizes the primary change from the developer's perspective.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch improve-logging-when-resizing-questionview

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

app/src/org/commcare/views/QuestionsView.java (2)

350-350: Consider using a class-level Handler instance.

Creating a new Handler() on every scroll operation is inefficient and creates unnecessary object allocations. Since handlers are thread-safe and reusable, declare a single instance at the class level.

Apply this diff to optimize Handler usage:

 public class QuestionsView extends ScrollView
         implements OnLongClickListener, WidgetChangedListener {
 
     // starter random number for view IDs
     private final static int VIEW_ID = 12345;
+    private final Handler handler = new Handler();

Then update the method:

 private void scrollToWidget(final QuestionWidget widget) {
-    new Handler().post(() -> {
+    handler.post(() -> {
         String widgetTextId = "None";

---

352-354: Consider more specific exception handling.

While catching all exceptions prevents the logging from breaking the scroll operation, silently ignoring exceptions could hide real issues with widget state. Consider at least logging the exception type if one occurs, or catching only the specific exceptions expected (e.g., NullPointerException).

-try {
-    widgetTextId = widget.getPrompt().getQuestion().getTextID();
-} catch (Exception ignore) {}
+try {
+    widgetTextId = widget.getPrompt().getQuestion().getTextID();
+} catch (Exception e) {
+    Logger.log(LogTypes.TYPE_ERROR_DESIGN, "Failed to get widget text ID: " + e.getClass().getSimpleName());
+}

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0be4d61 and 07432fd.

📒 Files selected for processing (1)

• app/src/org/commcare/views/QuestionsView.java (1 hunks)

🔇 Additional comments (1)

app/src/org/commcare/views/QuestionsView.java (1)

355-355: Verify that SOFT_ASSERT is the appropriate logging level.

SOFT_ASSERT typically indicates unexpected conditions that should be investigated, whereas this logs every normal scroll operation. If the goal is diagnostic logging to help debug the Crashlytics issue, consider whether TYPE_USER_ACTION or a debug-level log would be more appropriate. However, if you intentionally want this to stand out in logs during investigation, SOFT_ASSERT may be acceptable temporarily.

Can you confirm whether SOFT_ASSERT is intended here for prominent visibility during the Crashlytics investigation, or should this use a different log level?

[avazirna]

@damagatchi retest this please

[shubham1g5]

@avazirna you can instead catch the exception in this call and rethrow with the additional details, that way we will only log this in case of errrors, otherwise I am afraid this is a rather high frequency log that we will want to avoid logging.

[shubham1g5]

Also think we should not crash on failure to scroll to a widget here but just log it as a non-fatal. Crashing a user out during form entry is the worst outcome as user loose all their filled data.

[avazirna]

Yes, agree. The problem is that this is not when the issue occurs, but I'm suspecting that it's from the UI redrawn this causes. I can catch on onSizeChanged()

[avazirna]

@shubham1g5 I moved the log to onSizeChanged() 9aca919

[shubham1g5]

@avazirna Makes sense, Assuming you are not able to repro this issue, curious how do you know if the exception is getting triggered after scrollToWidget ? Or you are just taking a guess here ?

[avazirna]

@shubham1g5 I haven't been able to reproduce, but one common factor between the forms where this issue is happening is the app callout question, so I'm trying to assess whether it's related. Within QuestionsView there is the restoreFocusToQuestionThatCalledOut() which triggers the logic to scrollToWidget(), so the previous log was to assess whether this is the cause and what's the widget causing the issue.

[shubham1g5]

what will findFocus equate to as String ?

[avazirna]

@shubham1g5 I changed this to get the name of the view class and its parent, which is supposed to be a QuestionWidget - 7913cde

[Jignesh-dimagi]

Log exception as it will gives exception details also?

[avazirna]

We rethrow the exception, so the crashes will continue to happen and we will be able to get the details from the crash reports. But I'm happy to send add these to the log as well.

[Jignesh-dimagi]

What can be parent of QuestionView ?

[avazirna]

The parent here is of the view who has the focus which is likely to be a QuestionWidget, like a StringWidget for instance. A StringWidget is composed by an EditText which if it has the focus, I'm expecting this to return something like Resizing 1from 0X0 to 720X1600 failed with focus on: class androidx.widget.EditText/class org.commcare.views.widgets.StringWidget

[Jignesh-dimagi]

Ok, in case I think you can use simple class name by getClass().getSimpleName() or if full path required by getClass().getName()