CMM-884 support iteration over the whole flow and style #22310

adalpari · 2025-10-23T12:12:52Z

Description

This PR main purpose is to make the whole new Support UI look more AndroidMaterial3 looking.
In addition, some side improvements have been made around the code and the logic. And markdown messages are now supported.

Testing instructions

Smoke test the whole new support screens and check that everything looks consistent.

Screen_recording_20251027_114907.mp4

…/github.com/wordpress-mobile/WordPress-Android into feat/CMM-843-Create-the-Ask-the-HE-entry-UI

jkmassel

This is generally looking better – a couple of notes:

The "New Ticket" selector has too many colours – we should probably just have the outline and then the inner highlight should be the same colour with 1/2 to 1/3rd of the saturation.
Both conversation types should parse and display markdown as rich text. No need to support composing rich text yet.

…-flow-and-style # Conflicts: # WordPress/src/main/java/org/wordpress/android/support/aibot/ui/AIBotConversationsListScreen.kt # WordPress/src/main/java/org/wordpress/android/support/common/ui/EmptyConversationsView.kt # WordPress/src/main/java/org/wordpress/android/support/he/ui/HEConversationsListScreen.kt # WordPress/src/main/java/org/wordpress/android/support/he/ui/HENewTicketScreen.kt # WordPress/src/main/java/org/wordpress/android/support/he/ui/TicketMainContentView.kt # WordPress/src/main/res/values/strings.xml

adalpari · 2025-10-27T09:28:15Z

@claude

claude · 2025-10-27T09:28:36Z

Claude finished @adalpari's task —— View job

Code Review for PR #22310

I've completed a comprehensive review of this PR. Here are my findings across multiple dimensions:

Overview

This PR implements a significant UI overhaul for the support flow, introducing Compose-based screens and markdown rendering capabilities. The changes span 11 files with 808 additions and 427 deletions.

Key Findings

1. Code Quality & Architecture

✅ Strengths

Well-structured Compose code: The composables follow good practices with proper state management and separation of concerns
Consistent styling: Material3 design system is used consistently across all screens
Good composable breakdown: Complex screens are properly decomposed into smaller, reusable components
Preview coverage: Excellent preview coverage with light/dark themes and WordPress/Jetpack variants
Proper state management: Good use of StateFlow with collectAsState() for reactive UI updates

⚠️ Issues to Address

Critical: Markdown Parser Security & Edge Cases

Location: WordPress/src/main/java/org/wordpress/android/ui/compose/utils/MarkdownUtils.kt:20

Missing documentation: The function lacks KDoc explaining supported syntax, limitations, and security considerations
Nested markdown not handled: Doesn't support nested formatting (e.g., **bold *and italic***)
Escaped characters not handled: Can't escape markdown characters (e.g., \*not italic\*)
Edge cases: Consider scenarios like mixed delimiters (**bold__), very long strings, Unicode characters
Performance: Character-by-character scanning may be slow for long messages (>1000 chars)

Color Scheme Issue - Reviewer Feedback Not Addressed

Location: WordPress/src/main/java/org/wordpress/android/support/he/ui/HENewTicketScreen.kt:344-350

Per @jkmassel's feedback: "The 'New Ticket' selector has too many colours – we should probably just have the outline and then the inner highlight should be the same colour with 1/2 to 1/3rd of the saturation."

Current implementation uses bold primary color for both border and icon when selected. This needs to be toned down with reduced saturation.

Minor Issues

EmptyConversationsView.kt:38 - Hardcoded emoji "💬" may not render consistently across Android versions
EmptyConversationsView.kt:23 - Duplicate import of androidx.compose.ui.unit.dp

2. Test Coverage Issues

❌ Critical Concern: 0% Coverage on New Code

According to Codecov, this PR has 345 lines with missing coverage:

HENewTicketScreen.kt: 128 missing lines
SupportScreen.kt: 62 missing lines
TicketMainContentView.kt: 49 missing lines
HEConversationsListScreen.kt: 33 missing lines
AIBotConversationsListScreen.kt: 27 missing lines
EmptyConversationsView.kt: 24 missing lines
LogsListScreen.kt: 21 missing lines

Paradox: MarkdownUtils.kt has comprehensive unit tests (229 lines in MarkdownUtilsTest.kt), yet Codecov reports 0% coverage. This suggests:

Tests may not be running in CI
Test source set configuration issue
Coverage reporting configuration problem

Recommendation:

Verify tests are actually executing in CI pipeline
Add screenshot/integration tests for critical UI flows
At minimum, ensure the existing MarkdownUtils tests show in coverage reports

3. Performance Considerations

✅ Good Performance Practices

Proper use of key = { it.id } in LazyColumn items for efficient recomposition
LaunchedEffect used appropriately for side effects
State hoisting follows best practices

⚠️ Minor Performance Considerations

Markdown parser scans character-by-character; may be slow for very long messages
Consider caching parsed markdown results if messages are re-rendered frequently

4. Security Review

✅ Low Security Risk

No SQL injection vectors
No file system operations
No network operations in UI layer
User input properly handled through Compose state
Markdown parser is text-only (no HTML injection risk)

⚠️ Considerations

Ensure includeAppLogs feature filters sensitive information from logs
Document that markdown parser should only be used with trusted content or add sanitization

5. Documentation

❌ Missing Critical Documentation

PR Description is empty: No description of what the PR does or how to test it
No testing instructions: Reviewers don't know what to test
MarkdownUtils.kt: Missing KDoc for the main function
Complex composables: Components like HENewTicketScreen lack parameter documentation

6. Reviewer Feedback Status

Per @jkmassel's review from 2025-10-26:

✅ Markdown parsing implemented: Both conversation types now parse and display markdown as rich text
- HEConversationDetailScreen.kt:294 uses markdownToAnnotatedString()
- AIBotConversationDetailScreen.kt:278 uses markdownToAnnotatedString()
❌ Color scheme NOT addressed: The "New Ticket" selector still has too many bold colors (see inline comment for specific fix)

Inline Comments Added

I've added inline comments on:

MarkdownUtils.kt:20 - Documentation and edge case handling
HENewTicketScreen.kt:344 - Color scheme fix per reviewer feedback
EmptyConversationsView.kt:23 - Duplicate import

Summary & Recommendation

Overall Assessment: This is a solid Compose implementation with good architectural practices, but several issues need attention before merge:

Must Fix (Blocking)

❌ Address reviewer feedback: Fix the color scheme in ticket selector per @jkmassel's comments
❌ Test coverage: Investigate why MarkdownUtils tests show 0% coverage and ensure they run in CI
❌ PR documentation: Add description and testing instructions

Should Fix (High Priority)

⚠️ Markdown parser: Add KDoc, edge case handling, and performance considerations
⚠️ UI test coverage: Add at least basic UI tests or screenshot tests for critical flows

Nice to Have (Low Priority)

⚠️ Replace hardcoded emoji with drawable
⚠️ Remove duplicate import

Recommendation: Request changes - The code quality is good, but the unaddressed reviewer feedback and test coverage issues should be resolved before merging.

claude · 2025-10-27T09:30:32Z

WordPress/src/main/java/org/wordpress/android/ui/compose/utils/MarkdownUtils.kt

+ * Convert markdown text to Compose AnnotatedString.
+ * Supports basic markdown formatting: bold, italic, bold+italic, and inline code.
+ */
+fun markdownToAnnotatedString(markdownText: String): AnnotatedString = buildAnnotatedString {


Missing Documentation: This function needs KDoc explaining:

Supported markdown syntax (bold, italic, bold+italic, inline code)

Known limitations (no nesting, no character escaping, no block-level elements)

Security considerations (should only be used with trusted content)

Example usage

Also consider:

Adding a max length parameter to prevent performance issues with very long strings

Handling escaped characters (e.g., \*not italic\*)

Adding input validation/sanitization if this will be used with user-generated content

claude · 2025-10-27T09:30:33Z

WordPress/src/main/java/org/wordpress/android/support/he/ui/HENewTicketScreen.kt

+        colors = CardDefaults.cardColors(
+            containerColor = MaterialTheme.colorScheme.surface
+        ),
+        border = BorderStroke(


Per reviewer feedback from @jkmassel: The "New Ticket" selector has too many colors. The border and icon colors should use a more subtle color scheme.

Suggestion: When selected, use the primary color with reduced saturation (e.g., primary.copy(alpha = 0.3f)) for the inner highlight to maintain visual hierarchy without overwhelming the user with multiple bold colors.

Suggested change

border = BorderStroke(

border = BorderStroke(

width = if (isSelected) 2.dp else 1.dp,

color = if (isSelected) {

MaterialTheme.colorScheme.primary.copy(alpha = 0.3f)

} else {

MaterialTheme.colorScheme.outline.copy(alpha = 0.5f)

}

),

I did achieve it, but finally didn't use alpha. I opted to use "selected/unselected" status. So it looks very clear) and imo clean) from the user perspective.

WordPress/src/main/java/org/wordpress/android/support/common/ui/EmptyConversationsView.kt

adalpari · 2025-10-27T09:38:20Z

Must Fix (Blocking)

❌ Address reviewer feedback: Fix the color scheme in ticket selector per @jkmassel's comments

❌ Test coverage: Investigate why MarkdownUtils tests show 0% coverage and ensure they run in CI

❌ PR documentation: Add description and testing instructions

I've addressed it. No alpha used, though. I just outlined the card (removing the extra color) and made icons having two tints: selected or unselected. Which I think looks good.
MarkdownUtils HAS a test class: MarkdownUtilsTest
PR documentation is comming

adalpari · 2025-10-27T10:36:29Z

Critical: Markdown Parser Security & Edge Cases

Location: WordPress/src/main/java/org/wordpress/android/ui/compose/utils/MarkdownUtils.kt:20

Missing documentation: The function lacks KDoc explaining supported syntax, limitations, and security considerations

Nested markdown not handled: Doesn't support nested formatting (e.g., **bold *and italic***)

Escaped characters not handled: Can't escape markdown characters (e.g., \*not italic\*)

Edge cases: Consider scenarios like mixed delimiters (**bold__), very long strings, Unicode characters

Performance: Character-by-character scanning may be slow for long messages (>1000 chars)

Critical 😅 ?

Done
Done
Done
Performance concerns???? This is literally O(n). I'm moving the call to the VM, inside a coroutine, though.

adalpari · 2025-10-27T11:31:47Z

@jkmassel I think the PR is now ready to review it :)

adalpari · 2025-10-28T17:23:14Z

I refined the markdown parsing (finally used a well-known library), and this is how it looks:

sonarqubecloud · 2025-10-28T17:27:31Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarQube Cloud

wpmobilebot · 2025-10-28T17:28:06Z

Project dependencies changes

list

+ New Dependencies
org.commonmark:commonmark:0.24.0

tree

+\--- org.commonmark:commonmark:0.24.0

adalpari added 30 commits October 17, 2025 12:23

Adding basic UI

80d4ed3

Renaming

4836d41

Some styling

37541d0

Renaming and dummy data

51b01a7

Using proper "new conversation icon"

6c83ae3

Conversation details screen

8d7ea50

Creating the reply bottomsheet

a0a146b

Linking to the support screen

eebc0ab

bottomsheet fix

37676c8

Mov navigation form activity to viewmodel

703f4c4

Adding create ticket screen

f6be7fd

More screen adjustments

d345864

Extracting common code

05773ec

Margin fix

b442787

detekt

cf4762e

Style

7d318b1

Merge branch 'trunk' into feat/CMM-843-Create-the-Ask-the-HE-entry-UI

1ab8f3b

New ticket check

d585a4a

Creating tests

8c651fc

Merge branch 'feat/CMM-843-Create-the-Ask-the-HE-entry-UI' of https:/…

fdf926b

…/github.com/wordpress-mobile/WordPress-Android into feat/CMM-843-Create-the-Ask-the-HE-entry-UI

Creating repository and load conversations function

3515fd0

Adding createConversation function

3d99919

Creating loadConversation func

0be28b4

Loading conversations form the viewmodel

40a5880

Adding loading spinner

a55994e

Pull to refresh

c82458c

Proper ionitialization

73a434a

Adding empty screen

087d07a

Handling send new conversation

3b5a1e5

Show loading when sending

4febc3d

adalpari added 3 commits October 24, 2025 11:47

Adding preview for EmptyConversationsView

7232fb2

Button fix

a6e3e65

detekt

19fcdf6

jkmassel reviewed Oct 26, 2025

View reviewed changes

Base automatically changed from feat/CMM-872-support-HE-conversations-and-tickets-logic to trunk October 27, 2025 08:04

adalpari added 4 commits October 27, 2025 09:14

Ticket selection change

6ddffcf

Supporting markdown text

30fb83f

detekt

3f908f2

claude bot reviewed Oct 27, 2025

View reviewed changes

WordPress/src/main/java/org/wordpress/android/support/common/ui/EmptyConversationsView.kt Outdated Show resolved Hide resolved

adalpari added 3 commits October 27, 2025 10:54

Improving MarkdownUtils

1f6f555

Formatting text in the repository layer instead the ui

727644c

Renaming

3eb939c

adalpari added the [Type] Task label Oct 27, 2025

Fixing tests

455c100

adalpari marked this pull request as ready for review October 27, 2025 11:31

adalpari added 3 commits October 28, 2025 17:59

Parsing markdown more exhaustively

4744542

New links support

ce4641f

Detekt

55d37a5

adalpari requested a review from a team as a code owner October 28, 2025 17:23

-        border = BorderStroke(
+        border = BorderStroke(
+            width = if (isSelected) 2.dp else 1.dp,
+            color = if (isSelected) {
+                MaterialTheme.colorScheme.primary.copy(alpha = 0.3f)
+            } else {
+                MaterialTheme.colorScheme.outline.copy(alpha = 0.5f)
+            }
+        ),

Uh oh!

CMM-884 support iteration over the whole flow and style #22310

Are you sure you want to change the base?

CMM-884 support iteration over the whole flow and style #22310

Conversation

adalpari commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing instructions

Uh oh!

jkmassel left a comment

Choose a reason for hiding this comment

Uh oh!

adalpari commented Oct 27, 2025

Uh oh!

claude bot commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review for PR #22310

Overview

Key Findings

1. Code Quality & Architecture

✅ Strengths

⚠️ Issues to Address

2. Test Coverage Issues

❌ Critical Concern: 0% Coverage on New Code

3. Performance Considerations

✅ Good Performance Practices

⚠️ Minor Performance Considerations

4. Security Review

✅ Low Security Risk

⚠️ Considerations

5. Documentation

❌ Missing Critical Documentation

6. Reviewer Feedback Status

Inline Comments Added

Summary & Recommendation

Must Fix (Blocking)

Should Fix (High Priority)

Nice to Have (Low Priority)

Uh oh!

claude bot Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

adalpari Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adalpari commented Oct 27, 2025

Must Fix (Blocking)

Uh oh!

adalpari commented Oct 27, 2025

Uh oh!

adalpari commented Oct 27, 2025

Uh oh!

adalpari commented Oct 28, 2025

Uh oh!

sonarqubecloud bot commented Oct 28, 2025

Quality Gate passed

Uh oh!

wpmobilebot commented Oct 28, 2025

Project dependencies changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adalpari commented Oct 23, 2025 •

edited

Loading

claude bot commented Oct 27, 2025 •

edited

Loading