CMM-972 authenticate media requests with wp.com credentials

[adalpari]

Description

We made some changes at the server side, so now attachment files are hosted in the WP api, so an authentication is needed to browse them.
This PR is adding an authentication header to properly show the images and videos attached to HE tickets.

Important note: Images worked straightforwardly, but videos were not properly loaded by ExoPlayer, even using the same auth configuration we use for Media videos. Something seems to be working differently at the server side for the attachments since the media fullscreen is able to reproduce WP videos requiring authorization, but no the attached ones.
All in all, we are now downloading the videos before playing them. It can cause some delay when loading a heavy video, but the result feels way better.

Testing instructions

1. Log into a WP.COM account with an active subscription
2. Create a ticket with images and videos attached
3. Close the app and open it again

• Verify the attachments are properly shown

Before:

After:

[adalpari]

@claude

[dangermattic]

	2 Warnings
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️	PR is not assigned to a milestone.

Generated by 🚫 Danger

[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	pr22359-5f73fa6
	Commit	5f73fa6
	Direct Download	jetpack-prototype-build-pr22359-5f73fa6.apk

Note: Google Login is not supported on these builds.

[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	pr22359-5f73fa6
	Commit	5f73fa6
	Direct Download	wordpress-prototype-build-pr22359-5f73fa6.apk

Note: Google Login is not supported on these builds.

[codecov]

Codecov Report

❌ Patch coverage is 28.00000% with 90 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.05%. Comparing base (89633eb) to head (5f73fa6).
⚠️ Report is 1 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...ess/android/support/he/util/TempAttachmentsUtil.kt	0.00%	30 Missing ⚠️
...ndroid/support/he/ui/HEConversationDetailScreen.kt	0.00%	26 Missing ⚠️
...d/support/he/ui/AttachmentFullscreenVideoPlayer.kt	0.00%	24 Missing and 1 partial ⚠️
.../support/he/ui/AttachmentFullscreenImagePreview.kt	0.00%	7 Missing ⚠️
...dpress/android/support/he/ui/HESupportViewModel.kt	93.75%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            trunk   #22359   +/-   ##
=======================================
  Coverage   39.05%   39.05%           
=======================================
  Files        2201     2201           
  Lines      106104   106169   +65     
  Branches    15040    15048    +8     
=======================================
+ Hits        41434    41469   +35     
- Misses      61182    61211   +29     
- Partials     3488     3489    +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.

[adalpari]

@claude

[claude]

Claude finished @adalpari's task —— View job

---

Code Review Complete

I've performed a comprehensive review of this PR. Overall, the implementation is well-structured with good test coverage. Below are my findings organized by category:

Summary

✅ Strengths:

• Excellent test coverage (284 lines of unit tests added)
• Good security practices with Bearer token authentication
• Proper resource cleanup with onDestroy() and onCleared()
• Smart caching for video files to improve performance
• Robust error handling in most places

⚠️ Key Issues to Address:

1. Deprecated ExoPlayer APIs - Project has explicit rules against deprecated APIs
2. SonarQube reliability rating failure (B rating instead of A)
3. Low test coverage (31% patch coverage per Codecov)
4. Potential recomposition performance issues

---

1. Code Quality Issues

Critical: Deprecated API Usage

File: AttachmentFullscreenVideoPlayer.kt:42,77,80

The code uses deprecated ExoPlayer APIs (SimpleExoPlayer, SimpleExoPlayer.Builder, Player.EventListener) despite the project's code quality guidelines explicitly stating "No Deprecated APIs: Avoid using deprecated methods and classes in new code".

Recommendation: Migrate to the non-deprecated APIs:

• SimpleExoPlayer → ExoPlayer
• Player.EventListener → Player.Listener

I understand from review comments that ExoPlayer upgrade is out of scope, but this should be tracked as technical debt. The codebase guidelines are clear about avoiding deprecated APIs in new features.

Recomposition Performance

File: HEConversationDetailScreen.kt:474

val authorizationHeader = remember { onGetAuthorizationHeaderArgument() }

Good optimization! This prevents repeated function calls during recomposition. However, there's an issue in the same file at line 96:

addHeader(AUTHORIZATION_TAG, onGetAuthorizationHeaderArgument.invoke())

This lambda is invoked on every recomposition. Should be cached with remember.

Line Length Violations

File: TempAttachmentsUtil.kt:45-46,128-129

Several lines exceed the 120-character limit specified in project guidelines:

Line 45: "Error removing attachment temp files temp files: " + e.stackTraceToString()
Line 128: "Failed to download video. Deleted: ${deleted} - Response code: $responseCode"

Recommendation: Break into multiple lines or use multiline strings.

---

2. Security Review

✅ Good: Authorization Header Handling

File: HESupportViewModel.kt:351

fun getAuthorizationHeader():String = "$BEARER_TAG ${accountStore.accessToken}"

Proper Bearer token format. However:

• Consider adding token validation before returning
• Token is logged in debug statements - ensure this doesn't leak in production logs

⚠️ Moderate: Authorization Header in Image Requests

Files: AttachmentFullscreenImagePreview.kt:96, HEConversationDetailScreen.kt:506

The authorization header is added to Coil image requests. Ensure:

1. These requests don't get cached with the header in shared caches
2. Coil's cache respects authentication boundaries

✅ Good: Error Handling in Video Download

File: TempAttachmentsUtil.kt:98-139

The video download function has improved significantly with:

• Proper connection/read timeouts (30s/60s)
• Resource cleanup in finally block
• Null return on errors (fail-safe approach)

---

3. Performance Analysis

✅ Good: Video Caching Strategy

File: HESupportViewModel.kt:54-56,299-309

Excellent caching implementation:

• Stores file paths (not File objects) to reduce memory
• Checks cache before downloading
• Validates cached files still exist

Suggestion: Consider adding:

• Cache size limit (currently unbounded)
• LRU eviction policy
• Periodic cleanup for stale files

⚠️ Potential Issue: UI Thread Blocking

File: AttachmentFullscreenVideoPlayer.kt:62-64

LaunchedEffect(videoUrl) {
    onStartVideoDownload(videoUrl)
}

This triggers download immediately on composition. For large videos, this could cause:

• Delayed UI rendering while download starts
• Wasted bandwidth if user closes immediately

Recommendation: Consider debouncing or user-triggered download for large files.

Good: State Management

The video download uses proper state machine (VideoDownloadState) with clear transitions:

• Idle → Downloading → Success/Error

---

4. Testing Review

✅ Excellent: Unit Test Coverage

File: HESupportViewModelTest.kt

284 lines of well-structured tests covering:

• Video download state machine
• Caching behavior
• Error scenarios
• Resource cleanup

⚠️ Low Overall Coverage: 31% Patch Coverage

Per Codecov report, 75 lines are missing coverage:

• AttachmentFullscreenVideoPlayer.kt: 25 lines missing
• HEConversationDetailScreen.kt: 25 lines missing
• TempAttachmentsUtil.kt: 19 lines missing
• AttachmentFullscreenImagePreview.kt: 6 lines missing

Recommendation: Add UI tests or screenshot tests for Compose UI components. The ViewModel is well-tested, but UI composables lack coverage.

Missing Test Scenarios:

1. Network interruption during video download - What happens if connection drops mid-stream?
2. Disk full scenario - Does temp file creation handle storage exhaustion?
3. Concurrent downloads - Multiple videos downloading simultaneously
4. Token expiration - Authenticated requests with expired tokens

---

5. Documentation Review

⚠️ Missing Documentation:

File: VideoDownloadState.kt

• No KDoc explaining the state machine or when each state occurs

File: TempAttachmentsUtil.kt:98

• Function lacks documentation about return value (null on error)
• Missing documentation about authentication requirements
• No mention of file cleanup responsibility

File: HESupportViewModel.kt:290-293

• Good function documentation, but missing:
  • Thread safety guarantees
  • Cache eviction policy
  • Maximum cache size

Recommendation: Add KDoc for all public APIs, especially:

• State classes and their transitions
• Functions with side effects (file I/O, network)
• Caching behavior and limitations

---

6. Architecture Review

✅ Good: Separation of Concerns

• TempAttachmentsUtil: Handles file I/O
• HESupportViewModel: Business logic and state
• UI Composables: Presentation only

✅ Good: Dependency Injection

All dependencies properly injected via Hilt:

• AccountStore
• TempAttachmentsUtil
• Coroutine dispatchers

⚠️ Consideration: Video Download Responsibility

Currently, video download logic spans:

• TempAttachmentsUtil (actual download)
• HESupportViewModel (caching, state)
• AttachmentFullscreenVideoPlayer (triggering)

Suggestion: Consider a dedicated VideoDownloadManager to encapsulate all video-related logic, making it reusable and easier to test.

---

7. SonarQube Issues

The PR shows a Quality Gate failure with "B Reliability Rating" instead of required "A" rating.

Common causes:

• Unhandled exceptions in critical paths
• Resource leaks (though I see proper cleanup in most places)
• Deprecated API usage

Recommendation:

1. Review SonarQube detailed report at the provided link
2. Address the specific reliability issues flagged
3. Consider adding @Suppress annotations with justifications for unavoidable issues

---

Recommendations Summary

High Priority (Should Fix):

1. ✅ Already addressed: Exception handling in video download (fixed in commit 47f8c5b)
2. ✅ Already addressed: Memory leak concern with videoCache (fixed in commit 5e3d697)
3. ❌ Resolve SonarQube reliability rating failure
4. ❌ Fix line length violations (CLAUDE.md specifies 120 char max)
5. ❌ Cache authorization header in AttachmentFullscreenImagePreview.kt:96 to avoid recomputation

Medium Priority (Consider):

1. Add UI/screenshot tests to improve coverage from 31%
2. Add KDoc documentation for public APIs
3. Add network interruption test scenarios
4. Consider cache size limits for video downloads

Low Priority (Technical Debt):

1. Track ExoPlayer migration as technical debt
2. Consider extracting VideoDownloadManager for better separation
3. Add token expiration handling

---

Positive Highlights

🎉 Excellent additions:

• Comprehensive unit tests with 284 lines covering edge cases
• Proper resource cleanup with both onDestroy() and onCleared()
• Smart caching strategy to avoid redundant downloads
• Good error handling with user-friendly error states
• Proper timeout configuration (30s connect, 60s read)

The core implementation is solid. The main concerns are around deprecated APIs (per project guidelines), test coverage for UI components, and the SonarQube failure that needs investigation.

---

[adalpari]

1. Code Quality Issues

Critical: Deprecated API Usage

Deprecated ExoPlayer APIs - Project has explicit rules against deprecated APIs

I know Claude, I know. You have complained about it 10 times 😓
We are using an outdated version, and upgrading it is not in the scope.

Potential Issue: UI Thread Blocking File: AttachmentFullscreenVideoPlayer.kt:62-64
LaunchedEffect(videoUrl) {
onStartVideoDownload(videoUrl)
}
This triggers download immediately on composition

This is missing part of the story. The download is inside the ViewModel, called by a coroutine. I assume Claude got panicked because of the function name.

[sonarqubecloud]

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

[jkmassel]

I tested this against some existing threads, and they worked fine – both images and videos.