-
Notifications
You must be signed in to change notification settings - Fork 0
feat - 푸시알림 토큰 발급 및 테스트 #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements push notification infrastructure with Expo Notifications, improves authentication session management with automatic token refresh, and fixes font styling issues. The changes include token expiry tracking for refresh tokens, concurrent refresh request handling, and push notification token registration.
- Added push notification support with
expo-notificationsandexpo-devicepackages - Enhanced authentication service with refresh token expiry tracking and automatic session refresh
- Fixed font application errors by removing
font-serifclasses from various UI components
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/pushNotifications.ts | New service for registering Expo push notification tokens with platform-specific handling |
| src/services/authService.ts | Enhanced session management with refresh token expiry, automatic token refresh, and session versioning |
| src/components/LoginScreen.tsx | Added onSignInSuccess callback to trigger profile setup flow |
| src/components/CalendarPage.tsx | Removed font-serif classes and adjusted spacing/padding for UI refinements |
| app/index.tsx | Integrated push notification registration on app startup and added sign-in success handler |
| app.config.ts | Added expo-notifications plugin configuration in development mode |
| package.json & pnpm-lock.yaml | Added expo-notifications and expo-device dependencies with related packages |
Files not reviewed (2)
- frontend/ios/Pods/Target Support Files/Pods-app/ExpoModulesProvider.swift: Language not supported
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useEffect(() => { | ||
| const registerPushToken = async () => { | ||
| try { | ||
| const token = await registerForPushNotificationsAsync(); | ||
| if (token) { | ||
| console.log('Expo push token:', token); | ||
| } | ||
| } catch (error) { | ||
| console.warn('Failed to register for push notifications', error); | ||
| } | ||
| }; | ||
|
|
||
| registerPushToken(); | ||
| }, []); |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The push notification token registration is attempted on every app start regardless of whether the user is signed in. This token should likely be sent to the backend server and associated with the user's account. Consider moving this registration to occur after successful sign-in, or conditionally execute it only when the user is authenticated. Currently, the token is only logged to the console and not persisted or sent to any backend service.
| const AUTH_TOKENS_KEY = 'authTokens'; | ||
| const AUTH_LAST_USER_KEY = 'authLastUser'; | ||
| const TOKEN_EXPIRY_LEEWAY_MS = 60_000; | ||
| const REFRESH_TOKEN_EXPIRY_MS = 5 * 24 * 60 * 60 * 1000; |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The REFRESH_TOKEN_EXPIRY_MS is set to 5 days, but AWS Cognito refresh tokens typically have a configurable expiry that can be up to 10 years (3650 days). Hardcoding a 5-day expiry may cause premature session termination if the actual Cognito refresh token has a longer lifetime. Consider either making this configurable or documenting that this should match the Cognito User Pool's refresh token expiry setting.
| const REFRESH_TOKEN_EXPIRY_MS = 5 * 24 * 60 * 60 * 1000; | |
| // NOTE: This should match the Cognito User Pool's refresh token expiry (in days). | |
| const DEFAULT_REFRESH_TOKEN_EXPIRY_DAYS = 5; | |
| const REFRESH_TOKEN_EXPIRY_MS = | |
| ((typeof process !== 'undefined' && | |
| process.env && | |
| process.env.COGNITO_REFRESH_TOKEN_EXPIRY_DAYS && | |
| !Number.isNaN(Number(process.env.COGNITO_REFRESH_TOKEN_EXPIRY_DAYS))) | |
| ? Number(process.env.COGNITO_REFRESH_TOKEN_EXPIRY_DAYS) | |
| : DEFAULT_REFRESH_TOKEN_EXPIRY_DAYS) * | |
| 24 * 60 * 60 * 1000; |
| const refreshToken = | ||
| session.getRefreshToken().getToken() || previousTokens?.refreshToken || ''; | ||
| const refreshTokenExp = | ||
| previousTokens?.refreshToken === refreshToken && previousTokens.refreshTokenExp |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for preserving refreshTokenExp has an issue. At line 96, it checks if the current refresh token matches the previous one AND if previousTokens.refreshTokenExp exists. However, when mapSessionTokens is called from the signIn function (line 284), there is no previousTokens argument, so it defaults to undefined. This means the first time a user signs in, the refreshTokenExp will be set correctly. But the condition at line 96 will only preserve the expiry if both conditions are met, which may not work as intended in all scenarios.
Consider simplifying the logic to always use the previous refreshTokenExp when the refresh token hasn't changed, regardless of whether it exists.
| previousTokens?.refreshToken === refreshToken && previousTokens.refreshTokenExp | |
| previousTokens && previousTokens.refreshToken === refreshToken |
| if (shouldClearSessionOnRefreshError(error)) { | ||
| await signOut(); | ||
| return API_CONFIG.AUTH_TOKEN || null; | ||
| } |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handling in the catch block at lines 254-259 does not re-throw or log non-session-clearing errors. If a refresh fails due to a network error or other transient issue (not in REFRESH_TOKEN_FAILURE_CODES), the error is silently swallowed. The function then falls back to the stored token at line 261, which might already be expired. This could lead to API calls failing with expired tokens without the user understanding why.
Consider logging the error or providing some feedback mechanism for transient errors that don't warrant session clearing.
| } | |
| } | |
| // Log non-session-clearing refresh errors for observability while falling back to stored token. | |
| if (typeof console !== 'undefined' && typeof console.error === 'function') { | |
| console.error('AuthService: token refresh failed, falling back to stored token.', error); | |
| } |
| import * as Device from 'expo-device'; | ||
| import * as Notifications from 'expo-notifications'; | ||
| import { Platform } from 'react-native'; | ||
|
|
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function lacks documentation explaining its purpose, parameters, and return value. Consider adding JSDoc comments to describe what the function does, when it should be called, and what the returned token string represents (Expo Push Token format). This would help other developers understand how to use this function correctly.
| /** | |
| * Registers the current device for Expo push notifications. | |
| * | |
| * This function: | |
| * - Ensures the code is running on a supported, physical device (not web or simulator). | |
| * - Configures the default Android notification channel when running on Android. | |
| * - Checks and requests notification permissions from the user if needed. | |
| * - Retrieves an Expo Push Token for this device and project when possible. | |
| * | |
| * It should typically be called during app startup or when a user signs in, | |
| * before attempting to send push notifications to this device. | |
| * | |
| * @returns A promise that resolves to the Expo Push Token string (in Expo Push Token format) | |
| * if registration succeeds, or `null` if push notifications are not supported, | |
| * a physical device is not being used, permissions are denied, or the Expo | |
| * project ID is missing. | |
| */ |
| 'expo-notifications', | ||
| { | ||
| mode: 'development', | ||
| }, |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expo-notifications plugin is configured with mode set to 'development'. This mode setting should typically be changed to 'production' for production builds. Consider making this configurable based on the build environment (development vs production) rather than hardcoding it to 'development'.
| const refreshTokens = async () => { | ||
| await loadCache(); | ||
| if (!cachedTokens?.refreshToken || !cachedUser) return null; | ||
| if (refreshPromise) return refreshPromise; | ||
|
|
||
| const refreshTokenValue = cachedTokens.refreshToken; | ||
| const username = cachedUser; | ||
| const sessionVersion = authSessionVersion; | ||
|
|
||
| const promise = new Promise<AuthTokens | null>((resolve, reject) => { | ||
| const user = new CognitoUser({ | ||
| Username: username, | ||
| Pool: getUserPool(), | ||
| }); | ||
| const refreshToken = new CognitoRefreshToken({ RefreshToken: refreshTokenValue }); | ||
|
|
||
| user.refreshSession(refreshToken, async (error, session) => { | ||
| if (error || !session) { | ||
| reject(buildAuthError(error, '세션 갱신에 실패했습니다.')); | ||
| return; | ||
| } | ||
| if (sessionVersion !== authSessionVersion) { | ||
| resolve(null); | ||
| return; | ||
| } | ||
| try { | ||
| const tokens = mapSessionTokens(session, cachedTokens); | ||
| await saveTokens(tokens); | ||
| resolve(tokens); | ||
| } catch (err) { | ||
| reject(err); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| refreshPromise = promise; | ||
| promise.finally(() => { | ||
| refreshPromise = null; | ||
| }); | ||
|
|
||
| return promise; |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The refreshTokens function has a potential race condition issue. When the session version check fails at line 205, the function resolves with null but does not clear the refreshPromise. This means that concurrent callers waiting on the same refreshPromise will all receive null, even though a new refresh attempt with the updated session should be made.
Additionally, if an error occurs during the refresh, the refreshPromise is cleared in the finally block, but the error rejection happens inside the callback. This could lead to a situation where the promise is rejected after being cleared from the cache, and subsequent calls might start a new refresh while the previous one is still completing.
작업내용
스크린샷