feat: push notification service#987
Conversation
|
Thank you @, for creating the PR and contributing to our UltimateHealth project 💗. |
|
Hello owner, I have completed the implementation for this issue and submitted a pull request. All checks and tests are passing successfully. As I am a beginner contributor and this is my first PR under GSSoC 2026, I would greatly appreciate it if you could review my changes and provide any feedback or suggestions for improvement. If everything looks good, kindly consider approving and merging the PR and marking it under GSSoC 2026. Thank you for your time and support. I look forward to your review. |
SB2318
left a comment
There was a problem hiding this comment.
Hello @bethesky01, Thanks for your contribution! However, please don’t rely solely on AI suggestions. Since you are working on the Version 2.0 production app, any changes should be properly validated with test cases, or at the very least tested locally before raising a PR.
AI tools don’t have the full context of the app’s two-year development journey, so it’s important to ensure changes are verified manually as well.
|
/review |
🤖 Gemini AI Code ReviewThis Pull Request introduces comprehensive support for Expo Push Notifications, addressing issue #979. The implementation covers push token registration, secure storage, server synchronization, and deep linking for various notification targets (articles, podcasts, profiles, preferences). It also includes valuable additions like a preflight environment check script, updated documentation, and TypeScript configuration improvements. Overall, the PR demonstrates a thoughtful approach to integrating a complex feature. The separation of concerns into 🔴 High Severity
🟡 Medium Severity
🟢 Low Severity / Nits
What's Good ✅
VerdictRequest Changes The most critical issue is the removal of |
SB2318
left a comment
There was a problem hiding this comment.
Thanks for the detailed implementation and structured approach for Expo Push Notifications. The separation of services and secure token handling is well done.
However, there are a few important issues that need to be fixed before merging, especially since this is a production-grade change.
🔴 High Severity Issues
1. Removal of expo/tsconfig.base
Removing "extends": "expo/tsconfig.base" from tsconfig.json is a risky change. This base configuration is important for Expo projects and ensures correct TypeScript behavior.
👉 Please revert this change unless there is a strong, justified alternative configuration in place.
2. Unsafe Notification Payload Handling (DeepLinkService.ts)
In resolveNotificationTarget, values like articleId, trackId, userId, etc. are directly converted using Number() without validation.
This can result in NaN being passed into navigation if the payload is malformed, which may lead to runtime issues or incorrect navigation behavior.
👉 Please add proper validation (e.g., isNaN checks) and handle invalid payloads safely before navigating.
3. Unsafe Type Casting in Navigation
Usage of as never in navigation.navigate bypasses TypeScript safety and hides potential type mismatches.
👉 Please refactor navigation typing properly instead of using unsafe assertions.
🟡 Medium Severity Issues
4. Global __DEV__ Declaration in APIUtils.ts
__DEV__ should not be declared inside a utility file. This should be handled via proper global typing or existing environment configuration.
🟢 Low Severity / Cleanup
- Add proper JSDoc for
EXPO_PUSH_TOKEN - Remove redundant
declare module 'react-native'if not required - Minor formatting improvement in
check-environment.js
✅ Summary
Overall, the implementation is solid and well-structured. However, the TypeScript config change and notification payload validation are critical blockers.
👉 Please address these issues and ensure proper local testing before resubmitting.
Fixes #979
Deep Linking
Added navigation for notification targets:
Improvements
Code Quality
Documentation
README-RUN.md).Testing