CMM-844/CMM-842: help center and logs in the new support screen #22290
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements help center navigation and application logs functionality for the new support screen (closing CMM-844 and CMM-842). The changes add two new navigation paths: opening the WordPress help center in a browser and displaying application logs grouped by day with detail views.
Key changes:
- Added help center navigation that opens WordPress.com support URL in a browser with analytics tracking
- Implemented application logs feature with list and detail screens showing logs grouped by day
- Added version name display to the support screen
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| SupportViewModel.kt | Added navigation events for help center and application logs |
| SupportActivity.kt | Implemented navigation handlers for help center and logs with analytics |
| SupportScreen.kt | Added version name parameter and display |
| LogsViewModel.kt | New view model handling log parsing, grouping by day, and error states |
| LogsActivity.kt | New activity managing logs navigation and sharing functionality |
| LogsListScreen.kt | New screen displaying logs grouped by day in a list |
| LogDetailScreen.kt | New screen showing detailed log entries for a selected day |
| LogDay.kt | New data model representing a day's worth of logs |
| strings.xml | Added strings for logs screen empty state, log count, and error message |
| AndroidManifest.xml | Registered LogsActivity |
| SupportViewModelTest.kt | Updated tests for help center and application logs click handlers |
| LogsViewModelTest.kt | New comprehensive test suite for LogsViewModel |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
WordPress/src/main/java/org/wordpress/android/support/logs/ui/LogsActivity.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/support/main/ui/SupportViewModel.kt
Outdated
Show resolved
Hide resolved
Generated by 🚫 Danger |
Project manifest changes for WordPressThe following changes in the --- ./build/reports/diff_manifest/WordPress/wordpressVanillaRelease/base_manifest.txt 2025-10-17 09:07:26.775919535 +0000
+++ ./build/reports/diff_manifest/WordPress/wordpressVanillaRelease/head_manifest.txt 2025-10-17 09:07:28.885929573 +0000
@@ -499,6 +499,10 @@
<activity
android:name="org.wordpress.android.support.main.ui.SupportActivity"
android:label="@string/support_screen_title"
+ android:theme="@style/WordPress.NoActionBar" />
+ <activity
+ android:name="org.wordpress.android.support.logs.ui.LogsActivity"
+ android:label="@string/support_screen_application_logs_title"
android:theme="@style/WordPress.NoActionBar" /> <!-- Deep Linking Activity -->
<activity
android:name="org.wordpress.android.ui.deeplinks.DeepLinkingIntentReceiverActivity"Go to https://buildkite.com/automattic/wordpress-android/builds/23649/canvas?sid=0199f168-8c28-4f94-9815-7563cf383277, click on the |
Project manifest changes for WordPressThe following changes in the --- ./build/reports/diff_manifest/WordPress/jetpackVanillaRelease/base_manifest.txt 2025-10-17 09:07:16.482889280 +0000
+++ ./build/reports/diff_manifest/WordPress/jetpackVanillaRelease/head_manifest.txt 2025-10-17 09:07:18.712896421 +0000
@@ -633,6 +633,10 @@
<activity
android:name="org.wordpress.android.support.main.ui.SupportActivity"
android:label="@string/support_screen_title"
+ android:theme="@style/WordPress.NoActionBar" />
+ <activity
+ android:name="org.wordpress.android.support.logs.ui.LogsActivity"
+ android:label="@string/support_screen_application_logs_title"
android:theme="@style/WordPress.NoActionBar" /> <!-- Reader Activities -->
<activity
android:name="org.wordpress.android.ui.reader.ReaderPostListActivity"Go to https://buildkite.com/automattic/wordpress-android/builds/23649/canvas?sid=0199f168-8c28-412e-bfcf-dd86cc15253a, click on the |
|
| App Name |  Jetpack |
|
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22290-f1c2a16 | |
| Commit | f1c2a16 | |
| Direct Download | jetpack-prototype-build-pr22290-f1c2a16.apk |
|
| App Name |  WordPress |
|
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22290-f1c2a16 | |
| Commit | f1c2a16 | |
| Direct Download | wordpress-prototype-build-pr22290-f1c2a16.apk |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #22290 +/- ##
==========================================
- Coverage 39.61% 39.56% -0.06%
==========================================
Files 2176 2180 +4
Lines 103647 103919 +272
Branches 14876 14886 +10
==========================================
+ Hits 41061 41116 +55
- Misses 59106 59322 +216
- Partials 3480 3481 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Testing worked as expected. As part of my PR review flow, I always ask Claude to review it alongside me. I usually incorporate it into my own review comments, but this time I thought sharing it "as is" might be beneficial.
Review from Claude:
🛑 Blockers - Must Address Before Merge
1. Memory Leak Risk - ViewModel receiving Context
Location: WordPress/src/main/java/org/wordpress/android/support/logs/ui/LogsViewModel.kt:30
Current Code:
fun init(context: Context) {
try {
val allLogs = AppLog.toHtmlList(context)
_logDays.value = parseLogsByDay(allLogs)Issue: ViewModels should not receive Context as a parameter. This creates a direct dependency on Android framework classes and can lead to memory leaks if the context is retained.
Recommended Fix:
Inject an application-scoped context via Dagger Hilt:
@HiltViewModel
class LogsViewModel @Inject constructor(
private val appLogWrapper: AppLogWrapper,
@ApplicationContext private val appContext: Context
) : ViewModel() {
fun init() {
try {
val allLogs = AppLog.toHtmlList(appContext)
_logDays.value = parseLogsByDay(allLogs)Then in the Activity:
viewModel.init() // No context parameter needed2. ANR Risk - Heavy Operation on Main Thread
Location: WordPress/src/main/java/org/wordpress/android/support/logs/ui/LogsViewModel.kt:32
Current Code:
fun init(context: Context) {
try {
val allLogs = AppLog.toHtmlList(context) // Potentially expensive!
_logDays.value = parseLogsByDay(allLogs)Issue: AppLog.toHtmlList() could be expensive with many logs, and it's currently called on the main thread from onCreate(). This could cause Application Not Responding (ANR) errors with a large log file.
Recommended Fix:
Move to background thread with proper coroutine scope:
fun init(context: Context) {
viewModelScope.launch(Dispatchers.IO) {
try {
val allLogs = AppLog.toHtmlList(context)
val parsedLogs = parseLogsByDay(allLogs)
withContext(Dispatchers.Main) {
_logDays.value = parsedLogs
}
} catch (throwable: Throwable) {
withContext(Dispatchers.Main) {
_errorMessage.value = ErrorType.GENERAL
}
appLogWrapper.e(AppLog.T.SUPPORT, "Error parsing logs: ${throwable.stackTraceToString()}")
}
}
}Consider also adding a loading state to show progress indicator while logs are loading.
3. Null Safety - Missing Navigation Guard
Location: WordPress/src/main/java/org/wordpress/android/support/logs/ui/LogsActivity.kt:96-103
Current Code:
composable(route = LogsScreen.Detail.name) {
val selectedLogDay by viewModel.selectedLogDay.collectAsState()
selectedLogDay?.let { logDay ->
LogDetailScreen(
logDay = logDay,
onBackClick = { navController.navigateUp() }
)
}
}Issue: If selectedLogDay is null (e.g., due to process death/recreation, deep link, or race condition), the detail screen will be blank with no way to navigate back except the system back button.
Recommended Fix:
Add null handling to navigate back automatically:
composable(route = LogsScreen.Detail.name) {
val selectedLogDay by viewModel.selectedLogDay.collectAsState()
selectedLogDay?.let { logDay ->
LogDetailScreen(
logDay = logDay,
onBackClick = { navController.navigateUp() }
)
} ?: run {
LaunchedEffect(Unit) {
navController.navigateUp()
}
}
}Or consider passing the log day data through navigation arguments instead of relying on ViewModel state.
⚠️ Warnings - Should Address
4. Unused Property with Incorrect Scope
Location: WordPress/src/main/java/org/wordpress/android/support/logs/ui/LogsActivity.kt:34
Current Code:
private lateinit var navController: NavHostControllerIssue: navController is initialized inside NavigableContent() Composable and never accessed outside. Declaring it as a lateinit property is unnecessary and could cause issues if accessed before composition.
Recommended Fix:
Remove the property declaration entirely. The navController from rememberNavController() is already captured by the composable's closure:
@Composable
private fun NavigableContent() {
val navController = rememberNavController() // Use local variable only
// ... rest of code
}Also remove the unused composeView property (line 33).
5. Missing Lifecycle Awareness for Flow Collection
Location: WordPress/src/main/java/org/wordpress/android/support/logs/ui/LogsActivity.kt:54-65
Current Code:
lifecycleScope.launch {
viewModel.errorMessage.collect { errorType ->
val errorMessage = when (errorType) {
LogsViewModel.ErrorType.GENERAL -> getString(R.string.logs_screen_general_error)
null -> null
}
errorMessage?.let {
ToastUtils.showToast(this@LogsActivity, it, ToastUtils.Duration.LONG, Gravity.CENTER)
viewModel.clearError()
}
}
}Issue: Flow is collected without lifecycle awareness. Toasts could potentially show when the activity is in the background or destroyed.
Recommended Fix:
Use repeatOnLifecycle as done in SupportActivity:
lifecycleScope.launch {
repeatOnLifecycle(Lifecycle.State.STARTED) {
viewModel.errorMessage.collect { errorType ->
val errorMessage = when (errorType) {
LogsViewModel.ErrorType.GENERAL -> getString(R.string.logs_screen_general_error)
null -> null
}
errorMessage?.let {
ToastUtils.showToast(this@LogsActivity, it, ToastUtils.Duration.LONG, Gravity.CENTER)
viewModel.clearError()
}
}
}
}6. Date Sorting is Semantically Incorrect
Location: WordPress/src/main/java/org/wordpress/android/support/logs/ui/LogsViewModel.kt:64
Current Code:
}.sortedByDescending { it.date } // Most recent firstIssue: Sorting by string comparison on "Oct-16" format works accidentally within the same month but breaks across month boundaries. For example, "Nov-01" < "Oct-31" alphabetically, which would show October dates before November dates.
Recommended Fix:
Option 1: Parse dates for proper chronological sorting:
}.sortedWith(compareByDescending { logDay ->
try {
SimpleDateFormat("MMM-dd", Locale.getDefault()).parse(logDay.date)
} catch (e: Exception) {
null
}
})Option 2: Change the date format to include year and use ISO format (e.g., "2025-10-16") which sorts correctly as strings.
💡 Ideas - Nice to Have
7. Extract URL Constant
Location: WordPress/src/main/java/org/wordpress/android/support/main/ui/SupportActivity.kt:97
Current Code:
private fun navigateToHelpCenter() {
val intent = Intent(Intent.ACTION_VIEW, "https://wordpress.com/support/".toUri()).apply {Suggestion:
Extract to a constant for better maintainability:
companion object {
private const val HELP_CENTER_URL = "https://wordpress.com/support/"
@JvmStatic
fun createIntent(context: Context): Intent = Intent(context, SupportActivity::class.java)
}
private fun navigateToHelpCenter() {
val intent = Intent(Intent.ACTION_VIEW, HELP_CENTER_URL.toUri()).apply {8. Add More Granular Error Types
Location: WordPress/src/main/java/org/wordpress/android/support/logs/ui/LogsViewModel.kt:88
Current Code:
enum class ErrorType { GENERAL }Suggestion:
Add more specific error types for better user feedback:
enum class ErrorType {
GENERAL,
NO_LOGS_FOUND,
PARSE_ERROR,
IO_ERROR
}9. Privacy Consideration for Log Sharing
Location: WordPress/src/main/java/org/wordpress/android/support/logs/ui/LogsActivity.kt:109-124
Current Code:
private fun shareAppLog() {
val intent = Intent(Intent.ACTION_SEND).apply {
type = "text/plain"
putExtra(Intent.EXTRA_TEXT, AppLog.toPlainText(this@LogsActivity))Suggestion:
Verify that AppLog.toPlainText() properly filters sensitive information such as:
- Passwords and authentication tokens
- API keys and secrets
- Personally Identifiable Information (PII)
- Email addresses and usernames
- Site URLs with authentication parameters
If not, consider adding a sanitization layer before sharing.
10. Add Analytics Tracking
Suggestion:
Consider tracking these events for product insights:
- When users view the Application Logs list screen
- When users open a specific log day detail
- When users share logs (helpful for support metrics)
Example:
composable(route = LogsScreen.List.name) {
LaunchedEffect(Unit) {
AnalyticsTracker.track(Stat.SUPPORT_APPLICATION_LOGS_VIEWED)
}
// ... rest of code
}⛏ Nitpicks - Minor Issues
11. Test Fragility - Reflection Usage
Location: WordPress/src/test/java/org/wordpress/android/support/logs/ui/LogsViewModelTest.kt:105-119, 293-310
Observation:
Tests use reflection to access private methods and fields. While this provides comprehensive coverage, these tests are fragile and will break if implementation details change (method names, signatures).
Note: I understand the limitation here given that AppLog.toHtmlList() is a static method that's difficult to mock. This is acceptable for now, but consider refactoring to use dependency injection for log retrieval in the future.
12. Comment Clarity
Location: WordPress/src/main/java/org/wordpress/android/support/main/ui/SupportActivity.kt:99
Current Code:
setPackage(null) // Ensure it doesn't match internal activitiesSuggestion:
Make the comment more descriptive:
setPackage(null) // Allow system to choose browser/handler instead of matching internal activities❤️ Praise - Great Work!
- ✅ Clean MVVM architecture with proper separation of concerns between Activity, ViewModel, and Composables
- ✅ Excellent Compose previews for light mode, dark mode, and empty states - very helpful for development
- ✅ Good error handling with try-catch blocks and proper error logging
- ✅ Proper navigation pattern using sealed class for navigation events in
SupportViewModel - ✅ Comprehensive test coverage with 20+ unit tests covering edge cases
- ✅ Good UX considerations with empty states, conditional FAB visibility, and proper spacing
- ✅ Proper use of Jetpack Compose Navigation with enum-based routes
- ✅ Analytics tracking already added for Help Center view
- ✅ Monospace font for log entries makes them easy to read
- ✅ Follows project patterns by reusing existing components like
MainTopAppBarandAppThemeM3
Summary
This is solid work with good architecture and UX! Please address the 3 blockers before merging:
- Context injection in ViewModel (memory leak risk)
- Background threading for log parsing (ANR risk)
- Null safety guard in navigation (UX issue)
The warnings should ideally be addressed as well, but could be handled in a follow-up PR if time is constrained. The ideas and nitpicks are optional improvements.
Recommendation: Request changes for the blockers, then approve once addressed.
There was a problem hiding this comment.
A few suggestions:
- We probably want to be able to share a single day's logs – I don't know if it's useful to share all of them at once? This could be a huge file with 6 days of unnecessary data to sort through.
- I don't think we need the number of log entries in the detail view – it's helpful in the list view to give an idea of how much data they'll see by tapping.
- Each log entry isn't really its own entity – the day is the entity as far as the user is concerned. Presentation-wise, that means we should treat all of the log entries for a given day as a single text file instead of drawing each as its own row. Technically-speaking, these files can be very long so for performance reasons it's probably a great idea to use the
LazyColumn-based layout, but we don't need padding or styling around each line.
Individual sharing, no logs count, and more plan style
Thank you for these great catches! |
I do agree!
Changes: 2648ba7 |
|
5 participants
Description
This PR is closing CMM-844 and CMM-842:
Testing instructions
[ ] Verify share logs works
Screen.Recording.2025-10-16.at.13.34.44.mov
Screen.Recording.2025-10-16.at.13.34.58.mov