8000 fix(android): Improve push notification loading reliability and add diagnostic logging by diegolmello · Pull Request #6711 · RocketChat/Rocket.Chat.ReactNative · GitHub
[go: up one dir, main page]

Skip to content

Conversation

diegolmello
Copy link
Member
@diegolmello diegolmello commented Oct 9, 2025

Proposed changes

This PR fixes critical bugs in Android push notification handling that caused "message-id-only" notifications to display placeholder text ("You have a new message") instead of loading the actual message content.

The issue was difficult to reproduce and only affected certain users. Investigation revealed multiple bugs:

  1. Missing callback calls when validation failed, causing the notification flow to hang
  2. Array index out of bounds in retry logic
  3. No HTTP timeouts allowing requests to hang indefinitely
  4. Poor error handling making debugging impossible

This PR also adds comprehensive production-safe diagnostic logging to help identify and resolve similar issues in the future.

Issue(s)

https://rocketchat.atlassian.net/browse/SUP-882

How to test or reproduce

Testing the fix:

  1. Configure a Rocket.Chat server to send message-id-only push notifications
  2. Receive a notification while app is in background
  3. Notification should display actual message content (not placeholder)

Verifying logs (debug build):

  1. Build a debug APK
  2. Run adb logcat | grep RocketChat
  3. Receive a notification
  4. Logs should show the full diagnostic flow without exposing sensitive data

Affected screens:

  • Push notifications (system tray)
  • All notification-related background processing

Screenshots

N/A - This is a backend notification handling fix with no UI changes

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves a current function)
  • New feature (non-breaking change which adds functionality)
  • Documentation update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • I have added necessary documentation (if applicable)
  • Any dependent changes have been merged and published in downstream modules

Further comments

Technical Details:

Bug Fixes (LoadNotification.java):

  • Fixed early return to always call callback.call(null) ensuring proper flow continuation
  • Fixed array bounds check from RETRY_COUNT <= TIMEOUT.length to RETRY_COUNT < TIMEOUT.length - 1
  • Added HTTP timeouts (10s connect, 30s read/write) via OkHttpClient.Builder
  • Added null safety for response body, serverURL, and messageId
  • Separated exception handling for IOException, JsonSyntaxException, and generic exceptions

Logging (Production-Safe):

  • Added structured logging with appropriate levels (DEBUG/INFO/WARN/ERROR)
  • Verbose logs gated behind BuildConfig.DEBUG (debug builds only)
  • Production logs never expose: user IDs, auth tokens, or full URLs
  • URLs sanitized to show only protocol and host
  • Follows Android logging best practices

Files Changed:

  • LoadNotification.java - Core bug fixes and error handling
  • CustomPushNotification.java - Added flow logging
  • Ejson.java - Added debug-only verbose logging with BuildConfig guards

The logging addition makes future issues like this much easier to diagnose while maintaining security and following industry standards for production logging.

- Added detailed logging for notification loading process in LoadNotification.java.
- Implemented error handling for null or empty userId and userToken.
- Enhanced retry logic for network requests with clear logging of attempts and failures.
- Updated Ejson.java to include logging for token and userId retrieval.
- Improved fallback notification handling in CustomPushNotification.java with additional logging for failures.
Copy link
Contributor
coderabbitai bot commented Oct 9, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix.android-push-secured

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@diegolmello diegolmello temporarily deployed to experimental_android_build October 9, 2025 14:29 — with GitHub Actions Inactive
@diegolmello diegolmello requested a deployment to experimental_ios_build October 9, 2025 14:29 — with GitHub Actions Waiting
@diegolmello diegolmello requested a deployment to official_android_build October 9, 2025 14:29 — with GitHub Actions Waiting
@diegolmello diegolmello requested a deployment to upload_experimental_android October 9, 2025 15:01 — with GitHub Actions Waiting
Copy link
github-actions bot commented Oct 9, 2025

Android Build Available

Rocket.Chat Experimental 4.65.0.107532

Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNT_WZhhUi5TNV4tmGX1GR2QFqcHIvQXdxflAusqcJtIUJIV51K9uLFChkhR_6W5bnYKp535DUtnt0lmIRY_

Copy link
Contributor
@OtavioStasiak OtavioStasiak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

0