Media search by title

[nbradbury]

Fixes CMM-1128

Previously, media search only worked on locally cached media. In addition, the network request failed to pass along the search query.

Now the search query is passed to the WordPress.com REST API via the "search" parameter, allowing users to find media that hasn't been loaded yet. I also added filename to the local search.

To test:

• Clear storage
• Login to a site with a lot of media
• My Site > Media
• Scroll down several pages
• Click an image and change its title to something unusual, like "spork"
• Clear storage again
• Log back into that site
• Create a post
• Choose to add an image from the WP media library to the post
• Search for "spork"
• Verify the retitled image appears in the search results

[dangermattic]

	1 Warning
⚠️	PR is not assigned to a milestone.

Generated by 🚫 Danger

[nbradbury]

@claude

[claude]

Claude finished @nbradbury's task in 1m 11s —— View job

---

Code Review Complete

I've reviewed the PR implementing server-side media search functionality. This is a well-structured change that properly threads the search parameter through the architecture layers. Here's my detailed feedback:

Tasks

• Read and analyze all changed files
• Review code quality and implementation
• Check for security concerns
• Assess performance implications
• Verify testing considerations
• Provide comprehensive feedback

---

Overview

The implementation successfully adds server-side search support for media by:

1. Passing the search term from UI through to the REST API
2. Adding filename to local database search queries
3. Properly threading the parameter through all architecture layers

Code Quality ✅

Strengths:

• Clean, focused changes following the existing architectural patterns
• Proper null safety handling for optional searchTerm parameter
• Consistent naming conventions throughout the call chain
• Good separation of concerns across UI, data, and network layers

Minor Observations:

• The parameter was renamed from filter to searchTerm in MediaLibraryDataSource.kt:158-171 for clarity - this is a good improvement
• Proper handling of both mime type and search term as independent optional parameters

Security ✅

No issues found:

• The search term is properly validated before being added to the REST API request (MediaRestClient.java:310-312)
• Using parameterized query building via HashMap prevents injection attacks
• The WellSQL contains() method in MediaSqlUtils.java:190-194 should be SQL injection-safe (parameterized queries)
• Input is passed as a parameter, not concatenated into SQL strings

Performance 📊

Good practices observed:

• Search term only added to network request when non-null and non-empty
• Local database search queries use proper indexing via WellSQL
• The contains() operation on multiple fields may have performance implications on large media libraries, but this is consistent with existing search patterns

Potential consideration:

• For very large media libraries, searching across multiple text fields (title, filename, caption, description) could be slow
• Consider adding a fulltext search index if performance becomes an issue in the future (outside scope of this PR)

Testing ⚠️

Missing test coverage:

• No unit tests added for the new search functionality
• The PR would benefit from tests covering:
  • MediaLibraryDataSource.loadPage() properly passes search term
  • MediaRestClient.fetchMediaList() correctly builds request params with search term
  • MediaSqlUtils search methods include filename in queries
  • Edge cases: null search term, empty search term, special characters

Recommendation: Add unit tests to verify:

// Example test cases to add
@Test
fun `loadPage includes searchTerm in payload when provided`()

@Test  
fun `loadPage handles null searchTerm correctly`()

@Test
fun `fetchMediaList adds search param only when searchTerm is non-empty`()

@Test
fun `searchSiteImages includes filename in search fields`()

Documentation ✅

Well documented:

• The PR description clearly explains the problem and solution
• Test instructions are comprehensive and specific
• Method signatures are self-documenting with clear parameter names

Note: The existing Javadoc comment at MediaRestClient.java:289-295 could be updated to mention the new searchTerm parameter, though this is minor.

Architecture & Design ✅

Excellent layering:

• Changes properly flow through the architecture:
  • UI Layer: MediaLibraryDataSource.kt
  • Network Layer: MediaRestClient.java
  • Persistence Layer: MediaSqlUtils.java
  • Store Layer: MediaStore.java
• The new constructor overload in FetchMediaListPayload maintains backward compatibility
• Search term is nullable throughout, making it truly optional

Additional Observations

1. Line length compliance ✅: All modified lines respect the 120 character limit specified in CLAUDE.md

2. Kotlin conventions ✅: The Kotlin file properly uses parameter formatting and naming conventions

3. Filename search addition ✅: Adding FILE_NAME to the local search fields (lines 191 and 211 in MediaSqlUtils.java) is a valuable enhancement that improves user experience

4. API compatibility: The change only affects WP.com REST API sites (isUsingWpComRestApi() check at line 986), which is correct since other backends may not support the search parameter

Summary

This is a solid, well-implemented PR that properly adds server-side media search functionality. The code is clean, follows existing patterns, and handles edge cases appropriately.

Recommended before merge:

• ✅ Code quality: Excellent
• ✅ Security: No concerns
• ✅ Architecture: Proper layering
• ⚠️ Testing: Add unit tests for search functionality
• ✅ Documentation: Clear PR description and test plan

The main improvement would be adding unit test coverage for the new search parameter flow through the various layers.

---

[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	pr22471-e65762a
	Commit	e65762a
	Direct Download	wordpress-prototype-build-pr22471-e65762a.apk

Note: Google Login is not supported on these builds.

[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	pr22471-e65762a
	Commit	e65762a
	Direct Download	jetpack-prototype-build-pr22471-e65762a.apk

Note: Google Login is not supported on these builds.

[codecov]

Codecov Report

❌ Patch coverage is 37.03704% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.14%. Comparing base (dbbbe71) to head (e65762a).
⚠️ Report is 2 commits behind head on trunk.

Files with missing lines	Patch %	Lines
.../org/wordpress/android/fluxc/store/MediaStore.java	0.00%	10 Missing ⚠️
...etwork/rest/wpapi/media/BaseWPV2MediaRestClient.kt	0.00%	5 Missing ⚠️
...c/network/rest/wpapi/media/MediaRSApiRestClient.kt	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk   #22471      +/-   ##
==========================================
+ Coverage   39.12%   39.14%   +0.02%     
==========================================
  Files        2205     2205              
  Lines      106684   106702      +18     
  Branches    15138    15140       +2     
==========================================
+ Hits        41741    41772      +31     
+ Misses      61445    61424      -21     
- Partials     3498     3506       +8     

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

[nbradbury]

• ⚠️ Testing: Add unit tests for search functionality

Added in a7fbc3c

[adalpari]

I guess self-hosted sites are not affected by these changes, right? Would it be possible to fix the search for those sites as well?

[adalpari]

Perfectly working for WP sites, which I guess is the scope.
I just left a comment about self-hosted sites.

[nbradbury]

I guess self-hosted sites are not affected by these changes, right? Would it be possible to fix the search for those sites as well?

Thanks for catching that! It hadn't occurred to me that self-hosted uses a different endpoint. Fixed in e65762a

[sonarqubecloud]

Quality Gate passed

Issues
0 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

[adalpari]

I guess self-hosted sites are not affected by these changes, right? Would it be possible to fix the search for those sites as well?

Thanks for catching that! It hadn't occurred to me that self-hosted uses a different endpoint. Fixed in e65762a

Thank you for the fix!