feat: initial personalization push implementation #88
Conversation
KestasVenslauskas
force-pushed
the
feat/push-notification-changes
branch
2 times, most recently
from
8487903 to
63eb374
Compare
KestasVenslauskas
force-pushed
the
feat/push-notification-changes
branch
from
63eb374 to
f84ab89
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements the initial foundation for personalized push notifications in the mobile app. It refactors the Firebase Cloud Messaging (FCM) topic subscription system to support both guest users (via topic subscriptions) and logged-in users (via backend token management), while also modernizing the video player architecture with a context-based approach.
Changes:
- Refactored FCM topic subscription to use function-based exports with improved error handling and storage management
- Added FCM token synchronization with backend for logged-in users, including device token registration/disassociation on login/logout
- Modernized video player with context-based architecture, extracting hooks and components for better maintainability
- Created new API hooks for managing push notification subscriptions and topics
Reviewed changes
Copilot reviewed 51 out of 51 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| app/util/useFirebaseTopicSubscription.ts | Refactored to export pure functions for topic subscription management instead of React hook |
| app/util/useFCMTokenSync.ts | New hook to sync FCM tokens with backend on login/logout and handle token refresh |
| app/util/useFirebaseMessaging.ts | Updated to use new useFCMTokenSync hook |
| app/screens/userPersonalSettings/UserPersonalSettingsScreen.tsx | Added FCM token cleanup on logout and account deletion |
| app/screens/user/components/UserActions.tsx | Updated notifications action to be disabled for guest users |
| app/screens/user/components/UserActionItem.tsx | Added opacity styling for disabled state |
| app/screens/settings/SettingsNotifications.tsx | Refactored to use new hooks with loading/error states |
| app/api/hooks/usePushNotifications.ts | New API hooks for device token registration and subscription management |
| app/api/hooks/useNotificationTopics.ts | New hooks for fetching and managing FCM topic subscriptions |
| app/components/videoComponent/* | Major refactoring to context-based architecture with extracted components and hooks |
| app/screens/main/tabScreen/mediateka/components/hero/* | Extracted ThumbnailItem component for better modularity |
| app/components/appBar/* | Extracted AppBarBackButton component for reusability |
| app/Theme.ts | Added notifications string constant |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Save subscriptions to storage | ||
| storage.set(TOPICS_STORAGE_KEY, JSON.stringify(allTopicSlugs)); |
There was a problem hiding this comment.
The function saves all topic slugs to storage even if some subscriptions fail. This could lead to an inconsistent state where the storage thinks a topic is subscribed, but Firebase SDK doesn't have the subscription. Consider only saving successfully subscribed topics, or implementing a retry mechanism for failed subscriptions.
| // Save only hidden topics as subscribed | ||
| storage.set(TOPICS_STORAGE_KEY, JSON.stringify(hiddenTopicSlugs)); |
There was a problem hiding this comment.
Similar issue: The function saves hidden topic slugs to storage regardless of unsubscription failures. This could lead to inconsistent state. Consider tracking actual subscription status rather than assuming all operations succeeded.
| // await unsubscribeFromAllTopicsExceptHidden(); | ||
| // await syncSubscriptions(); |
There was a problem hiding this comment.
These lines are commented out but appear to be part of the intended functionality based on the PR description. If this is intentional for this PR phase, consider adding a TODO comment explaining when these will be uncommented. If not intentional, they should either be removed or uncommented and tested.
| } else { | ||
| throw new Error('No user logged in'); | ||
| } |
There was a problem hiding this comment.
The function throws an error if there's no user logged in, but this scenario is already checked in the caller (lines 40-50). This check is redundant and the thrown error would never be caught by the try-catch in handleLogout/handleDeleteAccount since they already check for user existence. Consider removing this else branch or the caller's check.
| <TouchableDebounce | ||
| style={[ | ||
| styles.actionItem, | ||
| {borderColor: colors.border}, | ||
| {borderColor: colors.border, opacity: onPress ? 1 : 0.5}, | ||
| isDestructive && {borderColor: colors.textError, borderWidth: 1}, | ||
| ]} | ||
| onPress={onPress}> |
There was a problem hiding this comment.
The TouchableDebounce component still receives an onPress prop even when it's undefined, which might cause issues. When onPress is undefined, the component becomes non-interactive but still has touch handlers. Consider using disabled={!onPress} or conditionally rendering a View instead of TouchableDebounce when onPress is undefined.
|
No description provided.