Skip to content

feat: push notification service#987

Open
bethesky01 wants to merge 4 commits into
SB2318:mainfrom
bethesky01:main
Open

feat: push notification service#987
bethesky01 wants to merge 4 commits into
SB2318:mainfrom
bethesky01:main

Conversation

@bethesky01
Copy link
Copy Markdown

@bethesky01 bethesky01 commented Jun 1, 2026

Fixes #979

  • Implemented Expo Push Notifications support.
  • Added push token registration, secure storage, and server sync.
  • Added notification tap handling with deep linking support.

Deep Linking

Added navigation for notification targets:

  • Articles
  • Podcasts
  • Profiles
  • Preferences

Improvements

  • Added secure storage utilities for Expo push tokens.
  • Added API endpoint for push token registration.
  • Improved error handling and authenticated token sync.

Code Quality

  • Added TypeScript module declarations.
  • Updated TypeScript configuration for cleaner diagnostics.
  • Removed duplicate API endpoints.

Documentation

  • Added environment check script.
  • Added setup and run documentation (README-RUN.md).

Testing

  • Verified push token generation and storage.
  • Tested notification response handling.
  • Tested deep link navigation flow.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

Thank you @, for creating the PR and contributing to our UltimateHealth project 💗.
Our team will review the PR and will reach out to you soon! 😇
Make sure that you have marked all the tasks that you are done with ✅.
Thank you for your patience! 😀

@bethesky01 bethesky01 changed the title Added Push Notification System push notification service Jun 1, 2026
@bethesky01 bethesky01 closed this Jun 1, 2026
@bethesky01 bethesky01 reopened this Jun 1, 2026
@bethesky01 bethesky01 changed the title push notification service feat: push notification service Jun 1, 2026
@bethesky01
Copy link
Copy Markdown
Author

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.

Copy link
Copy Markdown
Owner

@SB2318 SB2318 left a comment

Choose a reason for hiding this comment

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

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.

@SB2318
Copy link
Copy Markdown
Owner

SB2318 commented Jun 2, 2026

/review

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 2, 2026

🤖 Gemini AI Code Review

This 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 PushNotificationService and DeepLinkService is commendable, and the use of secure storage for tokens is a strong security practice. However, there are several critical and medium-severity issues that need to be addressed before this PR can be merged, particularly concerning TypeScript configuration, deep link parameter validation, and potential runtime errors.

🔴 High Severity

  • Issue: Removal of expo/tsconfig.base in tsconfig.json
    The tsconfig.json file removes "extends": "expo/tsconfig.base". This base configuration provides essential TypeScript settings tailored for Expo projects, including module resolution, library definitions, JSX configuration, and various strictness checks. Removing it without explicitly re-declaring all necessary options will likely lead to significant compilation errors, missing type definitions, or unexpected runtime behavior across the entire frontend application. This is a fundamental change to the project's TypeScript setup and is highly prone to breaking the build or introducing subtle type-related bugs.

    • Impact: Potential for widespread TypeScript errors, broken builds, and runtime issues due to incorrect type inference or missing declarations. This could severely impact developer experience and application stability.
    • Fix: Revert the removal of "extends": "expo/tsconfig.base". If specific diagnostics need to be cleaned up, those should be addressed by overriding specific options or adding skipLibCheck if necessary, rather than removing the entire base configuration.
    --- a/frontend/tsconfig.json
    +++ b/frontend/tsconfig.json
    @@ -1,5 +1,4 @@
     {
    -  "extends": "expo/tsconfig.base",
       "compilerOptions": {
         "jsx": "react-native",
         "strict": true,

    Proposed Fix (Revert):

    {
      "extends": "expo/tsconfig.base", // Keep this line
      "compilerOptions": {
        "jsx": "react-native",
        "strict": true,
        "baseUrl": ".",
        "paths": {
          "@/*": ["src/*"]
        },
        "allowSyntheticDefaultImports": true,
        "esModuleInterop": true
      },
      "include": [
        "**/*.ts",
        "**/*.tsx",
        "**/*.d.ts"
      ],
      "exclude": [
        "node_modules"
      ]
    }

    If "cleaner diagnostics" was the goal, please specify which diagnostics were problematic and we can find a targeted solution.

🟡 Medium Severity

  • Issue: Unsafe Type Coercion and Missing Validation for Deep Link Parameters
    In DeepLinkService.ts, the resolveNotificationTarget function uses Number(data.articleId) without checking if the result is a valid number (i.e., not NaN). If data.articleId from the notification payload is a non-numeric string (e.g., "abc"), Number("abc") will result in NaN, which is then passed as articleId to ArticleScreen. This could lead to unexpected behavior, crashes, or incorrect data fetching in the ArticleScreen. Similar issues could arise if authorId, recordId, trackId, userId, author_handle, or userHandle are malformed or missing, as they are passed directly without validation.

    • Impact: Potential runtime errors, crashes, or incorrect navigation/data display if notification payloads contain malformed or unexpected data. This reduces the robustness of the deep linking mechanism.
    • Fix: Add explicit isNaN checks after Number() conversions and consider adding default values or more robust validation for other parameters.
    --- a/frontend/src/helper/DeepLinkService.ts
    +++ b/frontend/src/helper/DeepLinkService.ts
    @@ -176,10 +176,13 @@ export const resolveNotificationTarget = (
     }
    
     if (typeof data.articleId !== 'undefined') {
    -    return {
    -      name: 'ArticleScreen',
    -      params: {
    -        articleId: Number(data.articleId),
    +    const articleId = Number(data.articleId);
    +    if (!isNaN(articleId)) { // Add NaN check
    +      return {
    +        name: 'ArticleScreen',
    +        params: {
    +          articleId: articleId,
     +        authorId: data.authorId,
     +        recordId: data.recordId,
     +      },

    Proposed Fix (for articleId):

    if (typeof data.articleId !== 'undefined') {
      const articleId = Number(data.articleId);
      if (!isNaN(articleId)) { // Ensure it's a valid number
        return {
          name: 'ArticleScreen',
          params: {
            articleId: articleId,
            authorId: data.authorId, // Consider validation/default for authorId/recordId
            recordId: data.recordId,
          },
        };
      } else {
        console.warn('Invalid articleId received in notification data:', data.articleId);
      }
    }
    // Similar checks for trackId, userId if they are expected to be numbers or have specific formats.
    // For string parameters like author_handle, userHandle, consider trimming or length checks if empty strings are problematic.
  • Issue: Global __DEV__ Declaration in APIUtils.ts
    The line declare const __DEV__: boolean; is added to APIUtils.ts. __DEV__ is a global variable typically injected by the React Native bundler (Metro) or other build tools. Declaring it in a regular .ts file as an ambient module declaration is unusual and potentially problematic. If the bundler already provides this global, this declaration might be redundant. If it doesn't, this declaration only provides type information but doesn't define the variable at runtime, which could lead to a ReferenceError if __DEV__ is ever used in APIUtils.ts or other files that import it without the bundler defining it.

    • Impact: Potential runtime errors (ReferenceError) if __DEV__ is used and not properly defined by the build environment. It also clutters a utility file with a global declaration that should ideally reside in a dedicated global.d.ts or be handled by the tsconfig.json types array.
    • Fix: Remove declare const __DEV__: boolean; from APIUtils.ts. If __DEV__ is needed for type checking and not implicitly provided by the tsconfig.json or expo/tsconfig.base (which usually handles this), it should be added to a global.d.ts file or the tsconfig.json types array.
    --- a/frontend/src/helper/APIUtils.ts
    +++ b/frontend/src/helper/APIUtils.ts
    @@ -0,0 +1,3 @@
    +
    +declare const __DEV__: boolean; // Remove this line
     // API URL configuration
  • Issue: as never Type Assertions in navigateDeepLink
    The navigateDeepLink function uses navigation.navigate(target.name as never, target.params as never);. While as never can sometimes be a quick fix for type mismatches, it effectively bypasses TypeScript's type checking for these arguments. This reduces type safety and can hide potential issues if target.name or target.params do not strictly conform to the expected types for navigation.navigate.

    • Impact: Reduced type safety, making it harder to catch errors at compile time if the DeepLinkTarget type or the navigation object's expected parameters change.
    • Fix: Improve the type definition of DeepLinkTarget to align more closely with RootStackParamList (or whatever the main navigation parameter list is called). This might involve using a discriminated union or mapping types to ensure target.name and target.params are correctly typed for navigation.navigate. If RootStackParamList is not available globally, consider importing it or defining a local type that extends it.
    // Example (conceptual, actual implementation depends on your RootStackParamList)
    // Assuming you have a RootStackParamList defined somewhere, e.g., in navigations/StackNavigation.tsx
    import { RootStackParamList } from '../navigations/StackNavigation'; // Adjust path as needed
    
    export type DeepLinkTarget = {
      name: keyof RootStackParamList; // Use keyof RootStackParamList
      params?: RootStackParamList[keyof RootStackParamList]; // Map params to the correct type
      requiresAuth?: boolean;
    };
    
    export const navigateDeepLink = (
      navigation: NavigationProp<RootStackParamList>, // Type navigation properly
      target: DeepLinkTarget,
      isAuthenticated: boolean,
    ) => {
      // ... existing logic ...
      navigation.navigate(target.name, target.params); // No 'as never' needed if types align
    };

🟢 Low Severity / Nits

  • frontend/src/components/AppContent.tsx - Redundant initDeepLinking import/call:
    The initDeepLinking function is imported from ../helper/DeepLinkService but is not called in the AppContent component. It seems the deep linking logic for notifications is now handled by handleNotificationResponse and navigateDeepLink. If initDeepLinking is no longer needed, it should be removed to avoid dead code.

    • Fix: Remove import {initDeepLinking} from '../helper/DeepLinkService'; and any corresponding call if it's truly unused.
  • frontend/src/helper/SecureStorageUtils.ts - Missing JSDoc for new key:
    The SECURE_KEYS object has a new key EXPO_PUSH_TOKEN. While the file itself has a JSDoc block, it would be good to add a specific comment for this new key, similar to USER_TOKEN, explaining its purpose.

    • Fix: Add a JSDoc comment for EXPO_PUSH_TOKEN.
    --- a/frontend/src/helper/SecureStorageUtils.ts
    +++ b/frontend/src/helper/SecureStorageUtils.ts
    @@ -10,6 +10,7 @@
    
     export const SECURE_KEYS = {
       USER_TOKEN: 'SECURE_USER_TOKEN',
    +  // Stores the Expo Push Notification token for the device.
       EXPO_PUSH_TOKEN: 'SECURE_EXPO_PUSH_TOKEN',
     } as const;
  • frontend/src/types/ambient-modules.d.ts - Redundant react-native declaration:
    The declare module 'react-native'; line is likely redundant. React Native projects typically have react-native types automatically included via tsconfig.json (especially with expo/tsconfig.base or by having @types/react-native installed). This explicit declaration might not be necessary and could potentially conflict if other, more specific type definitions exist.

    • Fix: Remove declare module 'react-native'; unless there's a specific reason it's needed that isn't covered by existing type installations.
  • frontend/scripts/check-environment.js - Minor output formatting:
    The "Next steps" section could benefit from a blank line before it for better readability.

    • Fix: Add an extra console.log('') before console.log('\nNext steps:');.

What's Good ✅

  1. Clear Separation of Concerns: The creation of PushNotificationService.ts and DeepLinkService.ts is excellent. It keeps the AppContent.tsx component clean and focused, making the notification and deep linking logic highly modular, testable, and maintainable.
  2. Robust Deep Linking Logic: The resolveNotificationTarget and navigateDeepLink functions in DeepLinkService.ts are well-structured, handling various target types and incorporating authentication checks and restricted routes. This demonstrates a thorough approach to user experience and security for deep links.
  3. Secure Token Handling: The use of expo-secure-store for storing the Expo Push Token (SECURE_KEYS.EXPO_PUSH_TOKEN) is a critical security best practice. This ensures sensitive tokens are encrypted at rest, significantly reducing the risk of exposure.

Verdict

Request Changes

The most critical issue is the removal of expo/tsconfig.base from tsconfig.json. This change has a high probability of breaking the build or introducing widespread type-related issues across the application. This must be addressed before the PR can be merged. Additionally, the deep link parameter validation needs improvement to prevent runtime errors from malformed notification payloads.

Copy link
Copy Markdown
Owner

@SB2318 SB2318 left a comment

Choose a reason for hiding this comment

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


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.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

💡[Feature]: Push Notification System for User Engagement

2 participants