fix(android): add shared User-Agent to native OkHttp client#7372
Open
nkgotcode wants to merge 2 commits into
Open
fix(android): add shared User-Agent to native OkHttp client#7372nkgotcode wants to merge 2 commits into
nkgotcode wants to merge 2 commits into
Conversation
|
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details🔇 Additional comments (3)
WalkthroughCentralizes User-Agent generation in a new UserAgent class, adds a User-Agent-injecting interceptor to the shared OkHttp client (SSL pinning applied only when an alias is set), updates NotificationHelper to use UserAgent.get(), and adds tests plus VoIP client updates to consume the shared client. ChangesUser-Agent Interceptor Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PMD (7.25.0)android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.ktNo java executable found in PATH Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/app/src/main/java/chat/rocket/reactnative/networking/SSLPinningTurboModule.java (1)
98-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove dead fallback logic in getOkHttpClient()/Elvis callers (and restore DDP pingInterval behavior)
SSLPinningTurboModule.getSharedOkHttpClient()always returns a non-null cachedOkHttpClient, sogetOkHttpClient()’sif (shared != null)check and the fallbackOkHttpClient.Builderblock are unreachable.- Downstream Kotlin Elvis fallbacks are also dead:
MediaCallsAnswerRequest.kt:getSharedOkHttpClient() ?: OkHttpClient()→ theOkHttpClient()branch can be removed.DDPClient.kt:getSharedOkHttpClient() ?: OkHttpClient.Builder().pingInterval(30, ...)→ thepingInterval(30s)branch is never taken. If periodic WebSocket pings are needed, build from the shared client instead (e.g.,getSharedOkHttpClient().newBuilder().pingInterval(30, TimeUnit.SECONDS).build()).♻️ Proposed simplification of getOkHttpClient()
protected OkHttpClient getOkHttpClient() { - OkHttpClient shared = getSharedOkHttpClient(); - if (shared != null) { - return shared; - } - OkHttpClient.Builder builder = new OkHttpClient.Builder() - .connectTimeout(0, TimeUnit.MILLISECONDS) - .readTimeout(0, TimeUnit.MILLISECONDS) - .writeTimeout(0, TimeUnit.MILLISECONDS) - .cookieJar(new ReactCookieJarContainer()); - - if (alias != null) { - SSLSocketFactory sslSocketFactory = getSSLFactory(alias); - X509TrustManager trustManager = getTrustManagerFactory(); - if (sslSocketFactory != null) { - builder.sslSocketFactory(sslSocketFactory, trustManager); - } - } - - return builder.build(); + return getSharedOkHttpClient(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/app/src/main/java/chat/rocket/reactnative/networking/SSLPinningTurboModule.java` around lines 98 - 118, The getOkHttpClient() method contains dead fallback builder logic because getSharedOkHttpClient() always returns a non-null cached OkHttpClient; remove the unreachable if (shared != null) check and the entire fallback OkHttpClient.Builder block in SSLPinningTurboModule.getOkHttpClient(), returning the shared client directly. Update callers that use Elvis fallbacks: in MediaCallsAnswerRequest.kt remove the "?: OkHttpClient()" fallback and just use getSharedOkHttpClient(), and in DDPClient.kt replace the Elvis branch that creates a new OkHttpClient.Builder().pingInterval(30, ...) with building off the shared client (e.g., getSharedOkHttpClient().newBuilder().pingInterval(30, TimeUnit.SECONDS).build()) so pingInterval behavior is restored while still using the cached client.
🧹 Nitpick comments (1)
android/app/src/main/java/chat/rocket/reactnative/networking/SSLPinningTurboModule.java (1)
42-48: 💤 Low valueGuard shared-client state against concurrent access.
sharedClient,sharedClientAlias, andaliasare static mutable fields read/written bygetSharedOkHttpClient()without synchronization. Callers likeDDPClientandMediaCallsAnswerRequestinitialize their clients from lazy initializers that can run on different threads, so the check-then-build sequence here is a TOCTOU race. The worst case is benign (an extra client gets built), but asynchronizedmethod (orvolatile+ double-checked locking) makes the cache reliable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/app/src/main/java/chat/rocket/reactnative/networking/SSLPinningTurboModule.java` around lines 42 - 48, The static cache check in getSharedOkHttpClient() is racy because sharedClient, sharedClientAlias, and alias are mutable statics accessed from multiple threads; make access thread-safe by serializing reads/writes (e.g., declare getSharedOkHttpClient() synchronized or use a volatile sharedClient plus a synchronized block with double-checked locking) so the check-then-build sequence cannot race—update the logic around sharedClient/sharedClientAlias/alias in getSharedOkHttpClient() accordingly to ensure only one client is published for a given alias.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@android/app/src/main/java/chat/rocket/reactnative/networking/SSLPinningTurboModule.java`:
- Around line 98-118: The getOkHttpClient() method contains dead fallback
builder logic because getSharedOkHttpClient() always returns a non-null cached
OkHttpClient; remove the unreachable if (shared != null) check and the entire
fallback OkHttpClient.Builder block in SSLPinningTurboModule.getOkHttpClient(),
returning the shared client directly. Update callers that use Elvis fallbacks:
in MediaCallsAnswerRequest.kt remove the "?: OkHttpClient()" fallback and just
use getSharedOkHttpClient(), and in DDPClient.kt replace the Elvis branch that
creates a new OkHttpClient.Builder().pingInterval(30, ...) with building off the
shared client (e.g., getSharedOkHttpClient().newBuilder().pingInterval(30,
TimeUnit.SECONDS).build()) so pingInterval behavior is restored while still
using the cached client.
---
Nitpick comments:
In
`@android/app/src/main/java/chat/rocket/reactnative/networking/SSLPinningTurboModule.java`:
- Around line 42-48: The static cache check in getSharedOkHttpClient() is racy
because sharedClient, sharedClientAlias, and alias are mutable statics accessed
from multiple threads; make access thread-safe by serializing reads/writes
(e.g., declare getSharedOkHttpClient() synchronized or use a volatile
sharedClient plus a synchronized block with double-checked locking) so the
check-then-build sequence cannot race—update the logic around
sharedClient/sharedClientAlias/alias in getSharedOkHttpClient() accordingly to
ensure only one client is published for a given alias.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3b41eff-0edb-4b5c-8bee-2611f885e1f6
📒 Files selected for processing (4)
android/app/src/main/java/chat/rocket/reactnative/networking/SSLPinningTurboModule.javaandroid/app/src/main/java/chat/rocket/reactnative/networking/UserAgent.javaandroid/app/src/main/java/chat/rocket/reactnative/notification/NotificationHelper.javaandroid/app/src/test/java/chat/rocket/reactnative/networking/SSLPinningTurboModuleTest.kt
📜 Review details
🔇 Additional comments (4)
android/app/src/main/java/chat/rocket/reactnative/networking/UserAgent.java (1)
8-15: LGTM!android/app/src/main/java/chat/rocket/reactnative/networking/SSLPinningTurboModule.java (1)
47-65: User-Agent interceptor logic is correct.The application-level interceptor injects
UserAgent.get()only when noUser-Agentheader is present, preserving explicit overrides. Caching keyed byaliasis sound.android/app/src/main/java/chat/rocket/reactnative/notification/NotificationHelper.java (1)
49-51: LGTM!android/app/src/test/java/chat/rocket/reactnative/networking/SSLPinningTurboModuleTest.kt (1)
32-68: LGTM!
- remove dead null fallbacks around shared OkHttp client access - restore DDP websocket pingInterval on top of the shared client - synchronize shared client cache publication - add DDP regression for preserved ping interval
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
UserAgenthelper for the Rocket.Chat mobile UA formatUser-AgentinSSLPinningTurboModule.getSharedOkHttpClient()when a request does not already define oneUser-Agentheaders untouched and reuse the shared helper fromNotificationHelperTest Plan
./gradlew :app:testDebugUnitTest --tests "chat.rocket.reactnative.networking.SSLPinningTurboModuleTest"./gradlew :app:testDebugUnitTest --tests "chat.rocket.reactnative.notification.NotificationHelperTest"Closes #7342
Summary by CodeRabbit
Bug Fixes
Improvements
Tests