CMM-926 support offline bug and better handling

[adalpari]

Description

This PR fixes a problem where, after showing an "error" because of being offline, the "Reply" button inside a HE conversation was not working anymore. This was because of the error state not being reset.
Apart from that, in this PR a new, more specific error message is used when the app is offline. So now, trying to create new conversations, loading, or answering them will show a more proper error.

Testing instructions

1. Log in with a WP.COM account and enable MODERN_SUPPORT
2. Go off-line on-line and play with the feature.

• Verify you do see proper errors and the app is responsive.

Before/After

[adalpari]

@claude

[claude]

Claude finished @adalpari's task —— View job

---

Code Review Complete ✅

I've completed a comprehensive review of this PR. Overall, this is a well-implemented bug fix with good test coverage. The changes properly address the offline error handling issue and add more specific error messaging.

---

Summary

This PR fixes a critical bug where the Reply button becomes non-functional after an offline error, and introduces a new OFFLINE error type for better user feedback when the device is offline.

---

Positive Aspects ✨

1. Excellent Test Coverage: Added 86 lines of comprehensive unit tests covering all the new offline scenarios
2. Proper State Management: The fix correctly clears error state by moving the dismissal logic from the bottom sheet to the parent screen
3. Consistent Implementation: The offline check pattern is applied consistently across all network operations
4. Good Architecture: The new ErrorType.OFFLINE enum value follows the existing error handling pattern
5. Clean Refactoring: The removal of error handling from HEConversationReplyBottomSheet properly separates concerns

---

Issues & Recommendations

1. Minor: Unnecessary State Update (Low Priority)

Location: HEConversationDetailScreen.kt:216

onSend = { message, includeAppLogs ->
    draftMessageText = message  // ⚠️ This line seems unnecessary
    onSendMessage(message, includeAppLogs)
}

The draftMessageText is already being set from initialMessageText in the bottom sheet's remember state. This extra assignment happens just before sending, and the draft is cleared on success anyway. Consider removing this line to avoid confusion.

---

2. Code Duplication in Bottom Sheet Dismissal (Medium Priority)

Location: HEConversationDetailScreen.kt:174-182

The dismissSheet() function is defined inside a LaunchedEffect and is called from both success and failure branches. Consider extracting this to reduce nesting:

LaunchedEffect(messageSendResult) {
    val shouldDismiss = messageSendResult != null
    if (shouldDismiss) {
        onClearMessageSendResult()
        scope.launch {
            sheetState.hide()
        }.invokeOnCompletion {
            showBottomSheet = false
        }
        
        // Clear draft only on success
        if (messageSendResult is HESupportViewModel.MessageSendResult.Success) {
            draftMessageText = ""
            draftIncludeAppLogs = false
        }
    }
}

This reduces duplication and makes the logic clearer.

---

3. Network Check Timing Issue (Low Priority)

Location: ConversationsSupportViewModel.kt:143-146 and HESupportViewModel.kt:116-120

The network availability check happens at the start of the operation, but network status can change during the async operation. Consider:

• Adding a try-catch to handle network failures that occur during the operation
• The current approach is acceptable for most cases, but could lead to generic errors if network drops mid-operation

Current behavior is acceptable since there are already catch blocks for exceptions, but documenting this behavior or adding more specific network error handling in the catch blocks could be beneficial.

---

4. Missing Tests for ViewModel Network Checks (Medium Priority)

Location: ConversationsSupportViewModelTest.kt

While there are excellent tests for ConversationsSupportViewModel, there are no tests for the HESupportViewModel offline checks in:

• onSendNewConversation() (line 62-106)
• onAddMessageToConversation() (line 113-165)

Recommendation: Add test coverage for:

@Test
fun `onSendNewConversation sets OFFLINE error when network unavailable`()

@Test
fun `onAddMessageToConversation sets OFFLINE error when network unavailable`()

---

Security & Performance Notes

• ✅ No security concerns identified
• ✅ Network checks are performed before expensive operations
• ✅ Proper error logging throughout
• ✅ No memory leaks or resource issues

---

Architecture & Code Quality

• ✅ Follows existing patterns and conventions
• ✅ Proper use of Kotlin coroutines and StateFlow
• ✅ Good separation of concerns
• ✅ Consistent error handling strategy
• ⚠️ Could benefit from extracting the dismissal logic (see issue Implementing WordPress.com Notifications Activities #2)

---

Testing Recommendations

1. Unit Tests: Add tests for HESupportViewModel offline scenarios
2. Manual Testing:
   • Test network state changes during message composition
   • Verify error messages are user-friendly
   • Test rapid network on/off toggling
   • Verify the Reply button remains functional after errors

---

Documentation

The code is well-structured and self-documenting. The PR description clearly explains the issue and fix. No additional documentation needed.

---

Final Verdict

Approve with minor recommendations ✅

The core fix is solid and addresses the reported bug. The issues identified are minor improvements that can be addressed in a follow-up PR if desired. The excellent test coverage and consistent implementation across the codebase demonstrate good engineering practices.

Key Strengths:

• Fixes the critical bug effectively
• Comprehensive test coverage
• Clean separation of concerns
• Consistent error handling

Suggested Follow-ups:

• Add HESupportViewModel offline tests
• Consider extracting bottom sheet dismissal logic
• Review network timing edge cases

---

[adalpari]

1. Minor: Unnecessary State Update (Low Priority)

This suggestion is not accurate

2. Code Duplication in Bottom Sheet Dismissal (Medium Priority)

Done!

4. Missing Tests for ViewModel Network Checks (Medium Priority)

Done!

[wpmobilebot]

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr22334-c2e1a45
	Commit	c2e1a45
	Direct Download	wordpress-prototype-build-pr22334-c2e1a45.apk

Note: Google Login is not supported on these builds.

[wpmobilebot]

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr22334-c2e1a45
	Commit	c2e1a45
	Direct Download	jetpack-prototype-build-pr22334-c2e1a45.apk

Note: Google Login is not supported on these builds.

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.22%. Comparing base (7b13497) to head (c2e1a45).

Files with missing lines	Patch %	Lines
...ndroid/support/he/ui/HEConversationDetailScreen.kt	0.00%	11 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            trunk   #22334   +/-   ##
=======================================
  Coverage   39.21%   39.22%           
=======================================
  Files        2201     2201           
  Lines      105788   105795    +7     
  Branches    15009    15012    +3     
=======================================
+ Hits        41485    41498   +13     
+ Misses      60811    60806    -5     
+ Partials     3492     3491    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[dangermattic]

	1 Warning
⚠️	PR is not assigned to a milestone.

Generated by 🚫 Danger

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.9% Duplication on New Code

See analysis details on SonarQube Cloud