CMM-992 reader deeplinks #22374
CMM-992 reader deeplinks #22374
Conversation
Generated by 🚫 Danger |
Project manifest changes for WordPressThe following changes in the --- ./build/reports/diff_manifest/WordPress/wordpressVanillaRelease/base_manifest.txt 2025-11-28 17:03:09.375987331 +0000
+++ ./build/reports/diff_manifest/WordPress/wordpressVanillaRelease/head_manifest.txt 2025-11-28 17:03:18.556028842 +0000
@@ -596,6 +596,62 @@
android:host="wordpress.com"
android:pathPattern="/site-monitoring/.*"
android:scheme="http" />
+ <data
+ android:host="wordpress.com"
+ android:path="/read"
+ android:scheme="https" />
+ <data
+ android:host="wordpress.com"
+ android:path="/read"
+ android:scheme="http" />
+ <data
+ android:host="wordpress.com"
+ android:path="/discover"
+ android:scheme="https" />
+ <data
+ android:host="wordpress.com"
+ android:path="/discover"
+ android:scheme="http" />
+ <data
+ android:host="wordpress.com"
+ android:pathPattern="/read/feeds/.*"
+ android:scheme="https" />
+ <data
+ android:host="wordpress.com"
+ android:pathPattern="/read/feeds/.*"
+ android:scheme="http" />
+ <data
+ android:host="wordpress.com"
+ android:pathPattern="/reader/feeds/.*"
+ android:scheme="https" />
+ <data
+ android:host="wordpress.com"
+ android:pathPattern="/reader/feeds/.*"
+ android:scheme="http" />
+ <data
+ android:host="wordpress.com"
+ android:path="/read/search"
+ android:scheme="https" />
+ <data
+ android:host="wordpress.com"
+ android:path="/read/search"
+ android:scheme="http" />
+ <data
+ android:host="wordpress.com"
+ android:path="/reader/search"
+ android:scheme="https" />
+ <data
+ android:host="wordpress.com"
+ android:path="/reader/search"
+ android:scheme="http" />
+ <data
+ android:host="wordpress.com"
+ android:pathPattern="/tag/.*"
+ android:scheme="https" />
+ <data
+ android:host="wordpress.com"
+ android:pathPattern="/tag/.*"
+ android:scheme="http" />
</intent-filter>
</activity-alias> <!-- Custom Wordpress URI Scheme Deep Linking Activity Alias -->
<activity-aliasGo to https://buildkite.com/automattic/wordpress-android/builds/24019/canvas?sid=019acb66-a166-45eb-b181-b334bcee8e52, click on the |
Project manifest changes for WordPressThe following changes in the --- ./build/reports/diff_manifest/WordPress/jetpackVanillaRelease/base_manifest.txt 2025-11-28 17:03:40.907057743 +0000
+++ ./build/reports/diff_manifest/WordPress/jetpackVanillaRelease/head_manifest.txt 2025-11-28 17:03:52.367112562 +0000
@@ -233,6 +233,62 @@
android:host="wordpress.com"
android:pathPattern="/site-monitoring/.*"
android:scheme="http" />
+ <data
+ android:host="wordpress.com"
+ android:path="/read"
+ android:scheme="https" />
+ <data
+ android:host="wordpress.com"
+ android:path="/read"
+ android:scheme="http" />
+ <data
+ android:host="wordpress.com"
+ android:path="/discover"
+ android:scheme="https" />
+ <data
+ android:host="wordpress.com"
+ android:path="/discover"
+ android:scheme="http" />
+ <data
+ android:host="wordpress.com"
+ android:pathPattern="/read/feeds/.*"
+ android:scheme="https" />
+ <data
+ android:host="wordpress.com"
+ android:pathPattern="/read/feeds/.*"
+ android:scheme="http" />
+ <data
+ android:host="wordpress.com"
+ android:pathPattern="/reader/feeds/.*"
+ android:scheme="https" />
+ <data
+ android:host="wordpress.com"
+ android:pathPattern="/reader/feeds/.*"
+ android:scheme="http" />
+ <data
+ android:host="wordpress.com"
+ android:path="/read/search"
+ android:scheme="https" />
+ <data
+ android:host="wordpress.com"
+ android:path="/read/search"
+ android:scheme="http" />
+ <data
+ android:host="wordpress.com"
+ android:path="/reader/search"
+ android:scheme="https" />
+ <data
+ android:host="wordpress.com"
+ android:path="/reader/search"
+ android:scheme="http" />
+ <data
+ android:host="wordpress.com"
+ android:pathPattern="/tag/.*"
+ android:scheme="https" />
+ <data
+ android:host="wordpress.com"
+ android:pathPattern="/tag/.*"
+ android:scheme="http" />
</intent-filter>
</activity-alias> <!-- Custom Wordpress URI Scheme Deep Linking Activity Alias -->
<activity-aliasGo to https://buildkite.com/automattic/wordpress-android/builds/24019/canvas?sid=019acb66-a167-4617-b186-c2e1d9e51d39, click on the |
|
| App Name | Jetpack | |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22374-529cf47 | |
| Commit | 529cf47 | |
| Direct Download | jetpack-prototype-build-pr22374-529cf47.apk |
|
| App Name | WordPress | |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22374-529cf47 | |
| Commit | 529cf47 | |
| Direct Download | wordpress-prototype-build-pr22374-529cf47.apk |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| context.startActivity(intent); | ||
| } | ||
|
|
||
| public static void viewReaderDiscoverInNewStack(Context context) { |
Check notice
Code scanning / Android Lint
Nullable/NonNull annotation missing on method parameter Note
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #22374 +/- ##
==========================================
+ Coverage 39.02% 39.06% +0.03%
==========================================
Files 2203 2203
Lines 106342 106442 +100
Branches 15061 15106 +45
==========================================
+ Hits 41501 41580 +79
- Misses 61350 61365 +15
- Partials 3491 3497 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Claude finished @adalpari's task —— View job Code Review Complete ✓I've completed a comprehensive review of this PR that adds Reader deeplink support. Overall, the implementation is solid with good test coverage, but there are a few areas that need attention. SummaryThis PR successfully implements deeplink handling for various Reader-related URLs including Strengths:
Areas for Improvement:
Critical Issues1. Missing Null Annotations (Security Scanner Findings)Location:
2. Low Test Coverage for New CodeAffected Files:
Recommendation: While the core logic in Security Considerations1. URL Validation ✓ Generally GoodThe implementation properly validates URLs through multiple checks:
2. Potential ImprovementsInput Validation for Feed IDs (ReaderLinkHandler.kt:103) private fun extractFeedId(uri: UriWrapper): Long? {
return uri.pathSegments.getOrNull(FEED_ID_POSITION)?.toLongOrNull()?.takeIf { it > 0 }
}Consider validating that feed IDs are positive values. Tag Slug Sanitization (ReaderLinkHandler.kt:138) 3. AndroidManifest PatternsThe pathPattern Code Quality Issues1. Line Length Violations
2. Error HandlingGood use of fallback to
3. Code OrganizationThe
Testing AssessmentStrengths
Gaps
Recommendations// Add tests for edge cases:
@Test
fun `handles tag with special characters`() { ... }
@Test
fun `handles extremely long feed id`() { ... }
@Test
fun `handles case sensitivity in paths`() { ... }Performance ConsiderationsNo significant performance concerns. The URL parsing is lightweight and happens on the UI thread appropriately. DocumentationGood
Could Improve
Specific File Reviews
|
| <issue | ||
| id="IntentFilterUniqueDataAttributes" | ||
| message="Consider splitting data tag into multiple tags with individual attributes to avoid confusion" | ||
| errorLine1=" <data" | ||
| errorLine2=" ^"> | ||
| <location | ||
| file="src/main/AndroidManifest.xml" | ||
| line="660" | ||
| column="17"/> | ||
| </issue> | ||
|
|
||
| <issue | ||
| id="IntentFilterUniqueDataAttributes" | ||
| message="Consider splitting data tag into multiple tags with individual attributes to avoid confusion" | ||
| errorLine1=" <data" | ||
| errorLine2=" ^"> | ||
| <location | ||
| file="src/main/AndroidManifest.xml" | ||
| line="666" | ||
| column="17"/> | ||
| </issue> | ||
|
|
There was a problem hiding this comment.
These lines were useless in the lint ignore rules
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
WordPress/src/main/java/org/wordpress/android/ui/ActivityLauncher.java
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/deeplinks/handlers/ReaderLinkHandler.kt
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/deeplinks/handlers/ReaderLinkHandler.kt
Show resolved
Hide resolved
|
|
||
| <data | ||
| android:host="wordpress.com" | ||
| android:pathPattern="/read/feeds/.*" |
There was a problem hiding this comment.
Security: Overly broad pathPattern: The pattern /read/feeds/.* will match ANY path starting with /read/feeds/, including potentially malicious paths like /read/feeds/../../../etc/passwd. While Android's intent filtering doesn't typically allow path traversal attacks, consider using more specific patterns.
The security scanner flagged these data tags as having non-unique attributes. While this is a warning, it might be cleaner to be more explicit with your intent filters. However, this is likely acceptable for your use case.
| @Inject constructor(private val activityNavigator: ActivityNavigator) { | ||
| @Suppress("ComplexMethod", "LongMethod") | ||
| fun handleNavigationAction(navigateAction: NavigateAction, activity: AppCompatActivity) { | ||
| when (navigateAction) { |
There was a problem hiding this comment.
This is hard to achieve, since these are the cases the app is handling. Maybe we should review all cases in a separate PR to clean it up.
| android:pathPattern="/read/feeds/.*/posts/.*" | ||
| android:scheme="https" > | ||
| android:scheme="https" | ||
| tools:ignore="IntentFilterUniqueDataAttributes" > |
There was a problem hiding this comment.
Moving the ignore suppression here instead of having a huuuge lint/baseline file
…her.java Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…dpress-mobile/WordPress-Android into fix/CMM-992-reader-deeplinks
|
I've now achieved some of the clade suggestions |
|
Description
This PR adds support for handling various Reader-related deeplinks, allowing users to open specific Reader content directly from URLs.
More info here: https://github.com/wordpress-mobile/wordpress-mobile-specs/blob/trunk/specs/URL%20Handling.md
https://wordpress.com/read - Reader main feed
https://wordpress.com/discover - Discover stream
https://wordpress.com/read/search - Reader search
https://wordpress.com/tag/dogs - Tag stream
https://wordpress.com/read/feeds/138734090/posts/5879194632 - Specific feed post
https://wordpress.com/reader/feeds/138734090/posts/5879194632
https://wordpress.com/read/feeds/138734090
https://wordpress.com/reader/feeds/138734090 (edited)
Note: This one is missing, but looking for an example to test:
https://wordpress.com/read/blogs/{blogId}/posts/{postId} - Specific blog post
Testing instructions
Note: logged out cases are not tested here as I'll work in a different PR for that scenario
Bonus: try corrupted deeplinks. They shoudl directy open the browser, or show an error inside the app
https://wordpress.com/rad
https://wordpress.com/dicover
https://wordpress.com/read/sarch
https://wordpress.com/tas/dogs
https://wordpress.com/read/feeds/13734090/posts/587194632
https://wordpress.com/reader/feeds/13874090/posts/587194632
https://wordpress.com/read/feeds/1387340
https://wordpress.com/reader/feeds/1387390