CMM-927 support attachments limit

[adalpari]

Description

This PR checks the attachments size before sending them to the server, and shows proper errors if the user reaches the limit.

Testing instructions

1. Log in with a WP.COM account and enable MODERN_SUPPORT FF
2. Create a new ticket or add messages to an existing one

• Check you can attach files, but if you hit the limit (20MB) you see a proper error

[claude]

Claude finished @adalpari's task —— View job

---

Code Review for PR #22338: CMM-927 Support Limit Attachments

I've completed a comprehensive code review of your attachment size validation feature. Overall, the implementation is solid with good test coverage and nice UX design. However, I've identified several issues that should be addressed before merging.

---

Summary

Strengths:

• ✅ Well-structured validation logic with clear separation of concerns
• ✅ Excellent test coverage with 15+ new tests
• ✅ Good UX with visual feedback showing rejected files with thumbnails
• ✅ Proper cleanup of temporary files in error scenarios
• ✅ Attachments cleared after successful send operations

Issues Found:

• 🔴 Critical: Files with unknown size are silently rejected without user feedback (HESupportViewModel.kt:202-206)
• 🟡 High: Only one rejection reason is shown when multiple rejection types occur (HESupportViewModel.kt:231-253)
• 🟡 Medium: Individual file size check may be too strict - rejects files ≥20MB instead of >20MB (HESupportViewModel.kt:209)
• 🟡 Medium: Exception swallowing without logging could mask issues (HESupportViewModel.kt:256-267)
• 🔵 Low: Missing test coverage for edge cases (null file size, exactly 20MB)
• 🔵 Enhancement: Consider showing count of skipped files in UI

---

Detailed Findings

1. Critical Issue: Silent Rejection of Files with Unknown Size

Location: HESupportViewModel.kt:202-206

When getFileSize(uri) returns null, files are added to skippedUris but no rejection reason is set. This means users see their files disappear without explanation.

Impact: Poor user experience - files vanish silently

Recommendation: Add a third rejection reason:

sealed class RejectionReason {
    data object FileTooLarge : RejectionReason()
    data object TotalSizeExceeded : RejectionReason()
    data object UnknownSize : RejectionReason()  // For files where size cannot be determined
}

2. Logic Issue: Multiple Rejection Types Not Handled

Location: HESupportViewModel.kt:231-253

If some files are individually too large AND other files exceed the total size, only the last rejection type is shown to the user.

Recommendation: Track which rejection scenario to prioritize or show a combined message.

3. Boundary Condition: Individual File Size Check

Location: HESupportViewModel.kt:209

The check if (fileSize > MAX_TOTAL_SIZE_BYTES) rejects files that are exactly 20MB. The error message says "max 20MB" which suggests 20MB should be allowed.

Recommendation:

• Use >= if you want to allow exactly 20MB
• Or update error message to say "max 19.99MB" or "must be less than 20MB"

4. Security: Exception Swallowing

Location: HESupportViewModel.kt:256-267

Exceptions when determining file size are silently swallowed. This could mask permission issues or security problems.

Recommendation: Add debug logging:

} catch (e: Exception) {
    appLogWrapper.d(AppLog.T.SUPPORT, "Could not determine file size for URI: $uri - ${e.message}")
    null
}

5. Test Coverage Gaps

Location: HESupportViewModelTest.kt

Missing tests for:

• Files with null size (when getFileSize returns null)
• Boundary condition: exactly 20MB total
• Multiple rejection types occurring simultaneously

---

Code Quality Assessment

Architecture: ✅ Good

• Clean separation between ViewModel and UI
• Proper use of StateFlow for reactive updates
• Well-structured sealed classes for state management

Error Handling: ⚠️ Needs Improvement

• Temporary file cleanup is handled well
• Exception handling needs more logging
• User feedback for edge cases is incomplete

Testing: ✅ Good (with gaps)

• 15+ new tests covering main scenarios
• Good use of mocks and test fixtures
• Missing some edge case coverage

Performance: ✅ Good

• File size checks done on IO dispatcher
• Efficient validation logic with early exits

Security: ✅ Adequate

• No obvious security vulnerabilities
• Proper ContentResolver usage
• Could benefit from additional logging

---

Additional Observations

Strings.xml Warning

The danger bot correctly flagged that strings.xml was modified. This is generally discouraged except on release branches. However, for this feature, the new strings are necessary. Consider:

• Adding a note in the PR description about why these strings were added
• Ensuring they'll be included in the next translation cycle

UX Polish

The visual design is excellent! The rejected files section with thumbnails and error overlay is very intuitive. Minor enhancement suggestion: show the count of skipped files in the header.

Documentation

Consider adding KDoc comments to the validation methods explaining:

• What the 20MB limit applies to (individual files? total? both?)
• What happens when file size cannot be determined
• The priority order when multiple rejection reasons occur

---

Recommendations Priority

Must Fix Before Merge:

1. Handle null file size scenario with proper user feedback
2. Clarify the 20MB boundary condition (> vs >=)
3. Add logging for exception cases

Should Fix:
4. Handle multiple rejection types better
5. Add missing test cases

Nice to Have:
6. Show count of skipped files
7. Add KDoc documentation

---

Testing Instructions Verification

I reviewed the screenshots and the implementation matches the testing instructions:

• ✅ Attachments can be added
• ✅ Error shown when limit exceeded
• ✅ Visual feedback with skipped files section
• ✅ Proper error messages displayed

---

Conclusion

This is a well-implemented feature with good attention to UX details. The main issues are around edge case handling and user feedback completeness. Once the critical issue with null file sizes is addressed, this will be ready to merge.

Great work on the comprehensive test coverage and the intuitive visual design for showing rejected files! 🎉

---

[claude]

Claude finished @adalpari's task —— View job

---

Code Review Complete ✓

I've reviewed the current state of the PR and compared it against the previous review feedback. Here's my comprehensive analysis:

---

✅ Issues Successfully Addressed

1. Null File Size Handling (WordPress/src/main/java/org/wordpress/android/support/he/ui/HESupportViewModel.kt:202-217)

The code now properly handles files with unknown sizes by allowing them through (fail-open approach). This is a reasonable design decision:

• Files with null size are accepted and contribute 0 to the total size calculation
• Added logging for debugging (line 249)
• Test coverage added: addAttachments accepts file when size cannot be determined (line 567)

Status: ✅ Resolved

2. Individual File Size Check Fixed (WordPress/src/main/java/org/wordpress/android/support/he/ui/HESupportViewModel.kt:205)

Changed from >= to >, now correctly allowing files exactly 20MB:

if (fileSize > MAX_TOTAL_SIZE_BYTES) {  // Now accepts exactly 20MB

Test coverage confirms this: addAttachments accepts file exactly at 20MB limit (line 529)

Status: ✅ Resolved

3. Excellent Test Coverage

The test suite is comprehensive with edge cases covered:

• Boundary conditions (exactly 20MB, both single file and total)
• Null file size handling
• Mixed rejection scenarios
• Priority testing for rejection reasons

Status: ✅ Excellent

---

⚠️ Issues Requiring Attention

4. Rejection Reason Priority Logic Issue (WordPress/src/main/java/org/wordpress/android/support/he/ui/HESupportViewModel.kt:228-233)

Current behavior: The code prioritizes TotalSizeExceeded over FileTooLarge:

val rejectionReason = when {
    skippedDueToTotalSize -> RejectionReason.TotalSizeExceeded
    skippedDueToFileSize -> RejectionReason.FileTooLarge
    else -> null
}

Problem: This contradicts the test at line 581-613, which expects FileTooLarge to have higher priority. The test is currently failing or was written incorrectly.

Author's rationale (from review comments): "When a mixed case happens, the skippedDueToTotalSize has higher priority, so the user can focus on one error."

Analysis: The author's logic makes UX sense, BUT:

1. The test addAttachments prioritizes FileTooLarge over TotalSizeExceeded when both occur directly contradicts this
2. The test assertion expects FileTooLarge but the code will return TotalSizeExceeded

Recommendation: Either:

• Option A: Update the test name and assertion to match the intended behavior
• Option B: Swap the priority in the code if FileTooLarge is more actionable (user needs to reduce individual file sizes first)

I lean toward Option A since total size is arguably the more important constraint for the overall upload operation.

Severity: Medium - Test/code mismatch needs resolution

---

5. strings.xml Warning (WordPress/src/main/res/values/strings.xml)

Danger bot flagged this correctly. Per repository guidelines, string resources should only be updated on release branches. This PR adds:

• he_support_skipped_files_header
• he_support_file_too_large
• he_support_total_size_exceeded

Recommendation:

• Acknowledge this is expected for this feature
• Ensure these strings get properly handled during the release process
• Consider if these strings need to be added to the translation system

Severity: Low - Process issue, not a code issue

---

💡 Minor Suggestions

6. Consider Adding File Count to Error Message (WordPress/src/main/java/org/wordpress/android/support/he/ui/TicketMainContentView.kt:315)

The current UI shows "Skipped Files" without indicating how many. For UX enhancement:

Text(
    text = stringResource(R.string.he_support_skipped_files_header, skippedUris.size),
    // ...
)

Then update the string resource to: "Skipped Files (%d)"

Severity: Very Low - Nice-to-have UX improvement

---

7. Magic Number in Size Calculation (WordPress/src/main/java/org/wordpress/android/support/he/ui/HESupportViewModel.kt:37)

The constant MAX_TOTAL_SIZE_BYTES = 20L * 1024 * 1024 is well-defined. Consider adding a comment explaining this is a server-enforced limit:

companion object {
    // Server-enforced maximum total attachment size
    const val MAX_TOTAL_SIZE_BYTES = 20L * 1024 * 1024 // 20MB total
}

Severity: Very Low - Documentation enhancement

---

📊 Overall Assessment

Strengths:

• Comprehensive test coverage with edge cases
• Proper error handling and user feedback
• Clean UI implementation with Material 3 components
• Good separation of concerns (ViewModel validation, UI presentation)
• Fail-open approach for unknown file sizes is pragmatic

Areas for Improvement:

• Critical: Resolve test/code mismatch for rejection priority (Issue Fix for bug #403 #4)
• Minor: Address strings.xml process issue (Issue Fix for bug #403 (new patch) #5)

Recommendation: This PR is in good shape overall, but the test/code contradiction must be resolved before merge. Once Issue #4 is fixed, this will be ready for approval.

---

🎯 Action Items for Author

1. Fix test/code mismatch: Update either the test or the code to align on rejection reason priority
2. Confirm strings.xml approach: Acknowledge the Danger warning and confirm the strings will be handled properly in the release process
3. Optional: Consider the UX enhancement of showing skipped file count

[adalpari]

Issues Found:

🔴 Critical: Files with unknown size are silently rejected without user feedback (HESupportViewModel.kt:202-206)
🟡 High: Only one rejection reason is shown when multiple rejection types occur (HESupportViewModel.kt:231-253)
🟡 Medium: Individual file size check may be too strict - rejects files ≥20MB instead of >20MB (HESupportViewModel.kt:209)
🟡 Medium: Exception swallowing without logging could mask issues (HESupportViewModel.kt:256-267)
🔵 Low: Missing test coverage for edge cases (null file size, exactly 20MB)
🔵 Enhancement: Consider showing count of skipped files in UI

Major issues fixed (see the second Claude iteration)
There's a test issue I'm fixing now.

[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	pr22338-cba99f9
	Commit	cba99f9
	Direct Download	jetpack-prototype-build-pr22338-cba99f9.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	pr22338-cba99f9
	Commit	cba99f9
	Direct Download	wordpress-prototype-build-pr22338-cba99f9.apk

Note: Google Login is not supported on these builds.

[codecov]

Codecov Report

❌ Patch coverage is 23.36449% with 164 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.12%. Comparing base (e95f1e1) to head (cba99f9).
⚠️ Report is 1 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...ess/android/support/he/ui/TicketMainContentView.kt	0.00%	141 Missing ⚠️
...id/support/he/ui/HEConversationReplyBottomSheet.kt	0.00%	11 Missing ⚠️
...ndroid/support/he/ui/HEConversationDetailScreen.kt	0.00%	8 Missing ⚠️
...rdpress/android/support/he/ui/HENewTicketScreen.kt	0.00%	2 Missing ⚠️
...dpress/android/support/he/ui/HESupportViewModel.kt	95.55%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk   #22338      +/-   ##
==========================================
- Coverage   39.16%   39.12%   -0.04%     
==========================================
  Files        2203     2205       +2     
  Lines      105962   106157     +195     
  Branches    15027    15049      +22     
==========================================
+ Hits        41498    41536      +38     
- Misses      60973    61129     +156     
- Partials     3491     3492       +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]

I've changed a bit the flow to include the progressbar iOS used. And now, every time the user removes a selected file, we try to re-add the skipped ones because of the total size.

Screen_recording_20251106_165557.mp4

[jkmassel]

I tested this and it worked as described.

[sonarqubecloud]

Quality Gate passed

Issues
3 New issues
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