Skip to content

Conversation

@adalpari
Copy link
Contributor

@adalpari adalpari commented Nov 3, 2025

Description

This PR is the continuation of #22323

In these changes, we are now showing the attachments in a HE conversation. The user can now browse or download attachments in addition to previewing them.

Testing instructions

  1. Log in with a WP.COM account and enable MODERN_SUPPORT
  2. Create a conversation with attachments (or use the previous one you created if you were testing my other PR)
  3. Answer with more attachments from Zendesk (search by the ticket number you will get in your email)
  4. Answer with more attachments from your phone again
  • Verify you see all those attachments in the conversation and you can download them.

NOTE: don't forget to mark your ticket as solved in Zendesk

Screen_recording_20251103_114114.mp4

@dangermattic
Copy link
Collaborator

dangermattic commented Nov 3, 2025

1 Warning
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@adalpari adalpari requested review from Copilot and removed request for Copilot November 3, 2025 10:41
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Nov 3, 2025

App Icon📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack
FlavorJalapeno
Build TypeDebug
Versionpr22333-0e058dc
Commit0e058dc
Direct Downloadjetpack-prototype-build-pr22333-0e058dc.apk
Note: Google Login is not supported on these builds.

@adalpari adalpari changed the base branch from trunk to feat/CMM-885-support-HE-attachments November 3, 2025 10:57
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Nov 3, 2025

App Icon📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress
FlavorJalapeno
Build TypeDebug
Versionpr22333-0e058dc
Commit0e058dc
Direct Downloadwordpress-prototype-build-pr22333-0e058dc.apk
Note: Google Login is not supported on these builds.

@adalpari
Copy link
Contributor Author

adalpari commented Nov 3, 2025

@claude

@claude
Copy link

claude bot commented Nov 3, 2025

Claude finished @adalpari's task —— View job


Code Review Complete ✅

I've completed a comprehensive review of your PR adding HE attachment support. Overall, the implementation is well-structured and follows good practices. Here's my detailed analysis:


⭐ Strengths

  1. Well-organized UI components: The new AttachmentFullscreenImagePreview.kt provides a clean, user-friendly fullscreen image viewer with zoom/pan capabilities
  2. Good separation of concerns: Repository handles data mapping, UI components handle presentation
  3. Excellent test coverage: Both repository and ViewModel tests include attachment-related test cases
  4. Accessibility: Good use of semantic descriptions and content descriptions for screen readers
  5. Preview support: Multiple @Preview functions for different themes and configurations
  6. Proper error handling: Graceful degradation when images fail to load

🔍 Key Findings by Category

Code Quality ✅ Good

  • Clean, readable Kotlin code following conventions
  • Proper use of Jetpack Compose patterns
  • Good use of sealed classes for result types
  • Minor Issue: Some nested loops in HEConversationDetailScreen.kt:204 could be optimized (see inline comment)

Security ⚠️ Minor Concerns

  • File downloads: No storage space check before downloading attachments (see HESupportActivity.kt:203)
  • URL handling: Attachment URLs are loaded directly without validation (consider adding URL whitelist for production)
  • ✅ No sensitive data exposure in logs
  • ✅ Proper access token handling in repository

Performance ⚠️ Minor Optimizations Needed

  • Image loading: Uses Coil with proper caching strategy ✅
  • List rendering: Uses LazyColumn with proper keys ✅
  • State management: Efficient use of StateFlows ✅
  • Concern: Nested loop to find attachment by URL in HEConversationDetailScreen.kt:204 - could be O(n*m) for large conversations
  • Zoom/Pan UX: Minor issue with offset reset logic in AttachmentFullscreenImagePreview.kt:112 (see inline comment)

Testing ✅ Excellent

  • Comprehensive unit tests for repository attachment mapping
  • ViewModel tests cover attachment lifecycle (add, remove, clear)
  • Integration tests for attachment upload flow
  • Tests verify temp file cleanup on success and failure
  • Test coverage appears adequate
  • Suggestion: Consider adding UI tests for attachment preview interactions

Documentation ✅ Adequate

  • Code is self-documenting with clear naming
  • Compose components have proper parameter documentation via naming
  • ✅ Semantic descriptions for accessibility
  • Minor: Could benefit from KDoc comments on public functions in HEConversationUtils.kt

📝 Detailed Findings

Critical Issues

None found ✅

Important Issues

  1. Pan boundaries in fullscreen preview - The zoom/pan gesture in AttachmentFullscreenImagePreview.kt allows unlimited panning, which could pan the image completely off-screen. See inline comment for suggested improvement.

  2. Inefficient attachment lookup - Finding attachments by URL requires nested iteration. Consider passing the attachment object directly instead of just the URL.

Minor Issues/Suggestions

  1. Storage check before download - Add available storage verification before downloading attachments
  2. Content type support - Consider supporting more specific attachment types (PDF, documents, audio) for better UX
  3. Error messaging - Download failures should provide user-friendly error messages

🎯 Recommendations

Before merging:

  1. ✅ Fix the zoom/pan offset reset logic (minor UX issue)
  2. ✅ Add storage check before downloading files
  3. Consider the attachment lookup optimization

Future enhancements:

  1. Add pan boundaries to prevent image from going completely off-screen
  2. Support more attachment types (PDF, documents, audio)
  3. Add UI/integration tests for attachment preview
  4. Consider implementing attachment size limits

📊 Summary

Overall Assessment: ✅ Approved with minor suggestions

This is a solid implementation that adds valuable functionality for HE attachment support. The code is well-tested, follows Android best practices, and provides good UX. The issues identified are minor and mostly relate to edge cases and potential enhancements.

Risk Level: Low
Test Coverage: Excellent
Code Quality: High


Inline Comments Created

I've added 5 inline comments on specific code sections with detailed suggestions:

  • 2 comments on AttachmentFullscreenImagePreview.kt (zoom/pan behavior)
  • 1 comment on HEConversationDetailScreen.kt (attachment lookup efficiency)
  • 1 comment on HESupportActivity.kt (storage check before download)
  • 1 comment on HESupportRepository.kt (content type support)

Great work on this feature! 🎉


@codecov
Copy link

codecov bot commented Nov 3, 2025

Codecov Report

❌ Patch coverage is 2.78884% with 244 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.21%. Comparing base (e65b8cc) to head (0e058dc).
⚠️ Report is 1 commits behind head on feat/CMM-885-support-HE-attachments.

Files with missing lines Patch % Lines
.../support/he/ui/AttachmentFullscreenImagePreview.kt 0.00% 108 Missing ⚠️
...ndroid/support/he/ui/HEConversationDetailScreen.kt 0.00% 80 Missing ⚠️
...ess/android/support/he/util/HEConversationUtils.kt 0.00% 32 Missing ⚠️
...droid/support/he/repository/HESupportRepository.kt 21.73% 18 Missing ⚠️
...rdpress/android/support/he/model/SupportMessage.kt 25.00% 6 Missing ⚠️
Additional details and impacted files
@@                           Coverage Diff                           @@
##           feat/CMM-885-support-HE-attachments   #22333      +/-   ##
=======================================================================
- Coverage                                39.29%   39.21%   -0.09%     
=======================================================================
  Files                                     2200     2201       +1     
  Lines                                   105559   105788     +229     
  Branches                                 14995    15009      +14     
=======================================================================
+ Hits                                     41484    41485       +1     
- Misses                                   60583    60811     +228     
  Partials                                  3492     3492              

☔ 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.

@adalpari adalpari marked this pull request as ready for review November 3, 2025 12:02
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 3, 2025

Copy link
Contributor

@jkmassel jkmassel left a comment

Choose a reason for hiding this comment

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

Works fine for viewing images, but attaching them to replies didn't work for me at all.

@adalpari adalpari merged commit e6ec19c into feat/CMM-885-support-HE-attachments Nov 4, 2025
23 checks passed
@adalpari adalpari deleted the feat/CMM-885-support-HE-attachments-i2 branch November 4, 2025 17:56
adalpari added a commit that referenced this pull request Nov 4, 2025
* Adding basic UI

* Renaming

* Some styling

* Renaming and dummy data

* Using proper "new conversation icon"

* Conversation details screen

* Creating the reply bottomsheet

* Linking to the support screen

* bottomsheet fix

* Mov navigation form activity to viewmodel

* Adding create ticket screen

* More screen adjustments

* Extracting common code

* Margin fix

* detekt

* Style

* New ticket check

* Creating tests

* Creating repository and load conversations function

* Adding createConversation function

* Creating loadConversation func

* Loading conversations form the viewmodel

* Adding loading spinner

* Pull to refresh

* Proper ionitialization

* Adding empty screen

* Handling send new conversation

* Show loading when sending

* New ticket creation fix

* Using snackbar for errors

* Error handling

* Answering conversation

* Adding some test to the repository

* More tests!

* Compile fixes

* Similarities improvements

* Using snackbar in bots activity

* Extracting EmptyConversationsView

* Renaming

* Extracting VM and UI common code

* Extracting navigation common code

* Renaming VMs for clarification

* More refactor

* Capitalise text fields

* Updating rs library

* Loading conversation UX

* Style fix

* Fixing scaffolds paddings

* userID fix

* Fixing the padding problem in bot chat when the keyboard is opened

* Apply padding to create ticket screen when the keyboard is opened

* Fixing scroll state in reply bottomsheet

* Adding tests for the new common viewmodel

* Fixing AIBotSupportViewModel tests

* detekt

* Improvements int he conversation interaction

* Adding tests for HE VM

* Saving draft state

* Properly navigating when a ticket is selected

* Error parsing improvement

* accessToken suggestion improvements

* General suggestions

* Send message error UX improvement

* Fixing tests

* Converting the UI to more AndroidMaterial style

* Bots screen renaming

* Bots screens renaming

* Make NewTicket screen more Android Material theme as well

* Adding preview for EmptyConversationsView

* Button fix

* detekt

* Ticket selection change

* Supporting markdown text

* detekt

* Improving MarkdownUtils

* Formatting text in the repository layer instead the ui

* Renaming

* Fixing tests

* Support pagination

* Triggering in the 4th element

* detekt

* TODO for debug purposes

* Claude PR suggestions

Mutex and constant

* Put ConversationListView in common between bots and HE

* Empty and error state

* Skip site capitalization

* Adding a11c labels

* Adding headings labels

* adding accessible labels to chat bubbles

* detekt

* Fixing tests

* PR suggestion about bot chat bubble

* Fixing tests

* Updating rust

* Adding attachments UI

* Parsing markdown more exhaustively

* New links support

* Detekt

* Supporting in conversation as well

* Keeping the screen when select images

* Add attachments to the message data class

* Showing attachments in the UI

* Downloading attachments

* detekt

* Support pagination

* Triggering in the 4th element

* detekt

* TODO for debug purposes

* Claude PR suggestions

Mutex and constant

* Detekt

* Removing testing code

* Updating RS library version

* Opening images in fullscreen

* Improving full screen image UX

* Improving semantics

* Extracting strings

* Using rs PR fix

* Showing attachment preview

* Clearing attachments on new ticket screen close

* Removing selected images limit

* Unifying attachments handling inside the VM

* Using a launcher instead of startActivityForResult

* Remove unused parameter

* Handling temp files inside the VM

* Removing files

* detekt

* Throwing copy file error

* Extracting some individual composables from HEConversation screen file

* Reducing arguments

* Catch file creation error

* Using proper file extension

* General improvements

* Update RS version and some fixes

* Extracting temp attachment utils

* Adding new tests

* Some refactoring

* Removing attachments preview to open a dedicated PR

* Useless changes

* Useless changes

* Minor refactor

* String fix

* CMM-885 support HE attachments i2 (#22333)

* Showing attachments previews

* Typo

* Fixing pan issue

* Passing attachments directly instead of searching for then when tapped for full screen

* Compile fix

* Fixing the send state message

* Checking network availability

* Saving message state when error

* Tests

* Reverting non-related commits done by mistake
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants