[FIX] PWA에서 서비스 워커가 실행되지 않는 문제 해결 #244
Conversation
WalkthroughThis pull request updates the service worker registration and lifecycle management. The Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/main.tsx(2 hunks)frontend/src/service-worker.js(2 hunks)frontend/vite.config.ts(2 hunks)
🔇 Additional comments (5)
frontend/src/main.tsx (1)
26-31: Service worker registration improvements look goodMaking the registration function asynchronous and using an absolute path for the service worker aligns with moving it from the public directory to the src directory. The code properly awaits the registration process.
frontend/src/service-worker.js (2)
2-7: Good implementation of Workbox from CDNUsing importScripts to load Workbox from CDN and adding the defensive check for its existence before using its methods is a robust approach. This ensures better compatibility and error handling.
9-15: Excellent service worker lifecycle managementAdding these lifecycle event handlers significantly improves the PWA experience:
self.skipWaiting()ensures new service worker versions activate immediatelyclients.claim()ensures the service worker takes control of all open clientsThis implementation aligns with best practices for PWA service workers and directly addresses the issue where the service worker wasn't executing properly.
frontend/vite.config.ts (2)
13-13: Correct configuration for injectManifest strategyAdding
srcDir: 'src'is exactly what's needed to fix the issue. When using the injectManifest strategy (as specified on line 12), the service worker must be located in the src directory to operate correctly. This change ensures Vite looks for the service worker file in the right location.
33-35: Simplified devOptions configurationThe streamlined devOptions configuration maintains the essential functionality while removing unnecessary settings, making the configuration more maintainable.
frontend/src/main.tsx
Outdated
| @@ -49,6 +49,7 @@ enableMocking().then(() => { | |||
| <ReactQueryDevtools initialIsOpen={false} /> | |||
| </QueryProvider> | |||
| </ToastProvider> | |||
| <ReactQueryDevtools initialIsOpen={false} /> | |||
There was a problem hiding this comment.
Remove duplicate ReactQueryDevtools component
There's already a ReactQueryDevtools component on line 49 inside the QueryProvider. This duplicate instance placed outside the QueryProvider will cause errors since it won't have access to the query client context.
</ToastProvider>
- <ReactQueryDevtools initialIsOpen={false} />
</RootErrorBoundary>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <ReactQueryDevtools initialIsOpen={false} /> | |
| </ToastProvider> | |
| </RootErrorBoundary> |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/package.json (1)
42-42: Consider moving Firebase from devDependencies to dependenciesFirebase is being added as a development dependency, but since it will be used at runtime for service worker and messaging functionality (as seen in the new
settingFCM.tsfile), it should typically be placed in thedependenciessection instead.- "firebase": "^11.3.1",Add to the dependencies section:
"dependencies": { "@tanstack/react-query": "^5.64.2", "@tanstack/react-query-devtools": "^5.64.2", "lottie-react": "^2.4.1", "react": "^18.3.1", "react-dom": "^18.3.1", "react-error-boundary": "^5.0.0", "react-router": "^7.1.3", "styled-components": "^6.1.14", + "firebase": "^11.3.1" }frontend/src/config/settingFCM.ts (2)
1-14: Add error handling for Firebase initializationThe Firebase initialization lacks error handling. If the environment variables are missing or invalid, this could lead to runtime errors. Consider adding try-catch blocks and validation for the environment variables.
import { initializeApp } from 'firebase/app'; import { getMessaging, Messaging } from 'firebase/messaging'; const firebaseConfig = { apiKey: import.meta.env.VITE_FIREBASE_API_KEY, authDomain: import.meta.env.VITE_FIREBASE_AUTH_DOMAIN, projectId: import.meta.env.VITE_FIREBASE_PROJECT_ID, storageBucket: import.meta.env.VITE_FIREBASE_STORAGE_BUCKET, messagingSenderId: import.meta.env.VITE_FIREBASE_MESSAGING_SENDER_ID, appId: import.meta.env.VITE_FIREBASE_APP_ID, }; -export const app = initializeApp(firebaseConfig); +let app; +try { + // Validate that required config values are present + const requiredKeys = ['apiKey', 'projectId', 'messagingSenderId', 'appId']; + const missingKeys = requiredKeys.filter(key => !firebaseConfig[key]); + + if (missingKeys.length > 0) { + throw new Error(`Missing required Firebase configuration: ${missingKeys.join(', ')}`); + } + + app = initializeApp(firebaseConfig); +} catch (error) { + console.error('Failed to initialize Firebase:', error); + // Provide a fallback or clearly indicate Firebase is not available +} + +export { app };
15-22: Add error handling for messaging initializationSimilar to the Firebase app initialization, the messaging initialization should include error handling to gracefully manage any issues that might occur when getting the messaging instance.
export let messaging: Messaging | undefined = undefined; export const isNotificationSupported = 'Notification' in window; const isServiceWorkerSupported = 'serviceWorker' in navigator; if (isNotificationSupported && isServiceWorkerSupported) { - messaging = getMessaging(app); + try { + if (app) { + messaging = getMessaging(app); + } + } catch (error) { + console.error('Failed to initialize Firebase messaging:', error); + } }frontend/public/firebase-messaging-sw.js (1)
1-7: These changes align with PWA best practices.Adding proper service worker lifecycle management with
skipWaiting()andclients.claim()is essential when using the injectManifest strategy in a PWA. This change helps address the issue where the service worker wasn't executing properly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
frontend/eslint.config.js(1 hunks)frontend/package.json(1 hunks)frontend/public/firebase-messaging-sw.js(1 hunks)frontend/src/config/settingFCM.ts(1 hunks)frontend/src/main.tsx(2 hunks)frontend/vite.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/main.tsx
🔇 Additional comments (4)
frontend/eslint.config.js (1)
73-73:❓ Verification inconclusive
Consider the implications of disabling import/named rule
Turning off this ESLint rule means that checks for whether named imports actually exist in the imported module will be skipped. This could potentially lead to runtime errors if imports are incorrect.
If this change is specifically needed for Firebase-related imports, consider using a more targeted approach that only disables the rule for specific files or patterns.
🏁 Script executed:
#!/bin/bash # Check if there are any import errors in the codebase related to Firebase rg -A 5 -B 5 -p "import .* from ['\"]firebase/.*['\"]"Length of output: 403
Action: Revisit the Global Disabling of 'import/named'
The ESLint configuration currently turns off the
'import/named'rule (line 73 infrontend/eslint.config.js). While it appears this might have been done to accommodate Firebase imports—as seen infrontend/src/config/settingFCM.ts—disabling the rule globally can lead to unchecked named imports elsewhere in the codebase, potentially causing runtime errors.
- If the disablement is solely intended to work around Firebase import issues, consider using ESLint's override mechanism to target only Firebase-related files or patterns.
- Otherwise, re-enable
'import/named'to ensure proper validation of named imports across the project.Please review and adjust the configuration accordingly.
frontend/vite.config.ts (1)
4-4:❓ Verification inconclusive
Verify implications of removing PWA plugin configuration
You've commented out the entire VitePWA plugin configuration, not just moved it. This aligns with your goal of fixing the service worker execution in PWA, but has several implications:
- PWA manifest settings (icons, colors, display preferences) are no longer configured
- Service worker registration will now rely solely on the manual registration in your code
- The injectManifest strategy mentioned in the PR is no longer being used
Ensure that the manual Firebase service worker approach provides all the PWA functionality that was previously configured here.
Also applies to: 11-37
🏁 Script executed:
#!/bin/bash # Check if there's any other service worker configuration in the codebase echo "Checking for service worker registration code..." rg -p "serviceWorker.register" --type ts --type tsx --type js echo "Checking for PWA manifest configuration..." rg -p "manifest" --type json --type htmlLength of output: 332
Action: Verify Manual Firebase Service Worker Setup Provides Complete PWA Features
The removal of the VitePWA plugin configuration appears intentional to prioritize a manual Firebase service worker approach. However, please confirm the following:
- Ensure that the manual registration code (likely located in your main entry points) correctly registers the service worker.
- Verify that the PWA manifest (icons, colors, display preferences) is configured and loaded from an alternative source.
- Confirm that the removal of the
injectManifeststrategy hasn’t left any gaps in the PWA functionality compared to the previous setup.Since our automated search (now refined via file globbing) returned minimal output—and given the prior error with file type detection in TSX files—a manual verification of these aspects is recommended to ensure all intended functionality is preserved.
--- 🏁 Script executed: ```shell [run_scripts] #!/bin/bash # Search for service worker registration code using file globs in TS, TSX, and JS files echo "Searching for service worker registration code using file globs in TS, TSX, and JS files..." rg "serviceWorker\.register" -g "*.{ts,tsx,js}" # Search for PWA manifest configuration in JSON and HTML files echo "Searching for PWA manifest configuration in JSON and HTML files..." rg "manifest" -g "*.{json,html}"Length of output: 633
Review Verification: Confirm Manual Service Worker and Manifest Configurations
It is confirmed that a service worker registration exists in
frontend/src/main.tsxvia:
await navigator.serviceWorker.register('/firebase-messaging-sw.js');However, our search for a PWA manifest configuration in JSON/HTML files did not yield any explicit manifest settings. Given that the VitePWA plugin (and its associated
injectManifeststrategy) has been entirely removed, please ensure that:
- The manual Firebase service worker approach covers all required PWA functionalities.
- Any necessary PWA manifest settings (icons, colors, display preferences) are being provided elsewhere, such as in a standalone
manifest.jsonfile or directly in your HTML templates.- All instances where the VitePWA configuration was previously utilized (including lines 11-37) have been verified to remain compliant with your desired functionalities.
Please manually verify these aspects to ensure that no critical PWA features are inadvertently omitted.
frontend/public/firebase-messaging-sw.js (2)
1-3: Good addition of service worker installation handler.Adding
self.skipWaiting()in the install event ensures that the new service worker activates immediately instead of waiting for existing clients to close, which improves the update experience for users.
5-7: Appropriate service worker activation handling.The
clients.claim()call in the activate event ensures that the service worker takes control of all open pages immediately after activation, rather than waiting for a page refresh. This complements theskipWaiting()call and provides a better PWA experience.
frontend/src/config/settingFCM.ts
Outdated
| export const isNotificationSupported = 'Notification' in window; | ||
| const isServiceWorkerSupported = 'serviceWorker' in navigator; | ||
|
|
||
| if (isNotificationSupported && isServiceWorkerSupported) { | ||
| messaging = getMessaging(app); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check if notification permission is granted
You're checking if notifications are supported, but not if permission has been granted. For a better user experience, consider adding logic to check the notification permission status and prompt for permission if needed.
export const isNotificationSupported = 'Notification' in window;
const isServiceWorkerSupported = 'serviceWorker' in navigator;
+export const checkNotificationPermission = async (): Promise<boolean> => {
+ if (!isNotificationSupported) return false;
+
+ if (Notification.permission === 'granted') {
+ return true;
+ }
+
+ if (Notification.permission !== 'denied') {
+ try {
+ const permission = await Notification.requestPermission();
+ return permission === 'granted';
+ } catch (error) {
+ console.error('Error requesting notification permission:', error);
+ return false;
+ }
+ }
+
+ return false;
+};
+
if (isNotificationSupported && isServiceWorkerSupported) {
messaging = getMessaging(app);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const isNotificationSupported = 'Notification' in window; | |
| const isServiceWorkerSupported = 'serviceWorker' in navigator; | |
| if (isNotificationSupported && isServiceWorkerSupported) { | |
| messaging = getMessaging(app); | |
| } | |
| export const isNotificationSupported = 'Notification' in window; | |
| const isServiceWorkerSupported = 'serviceWorker' in navigator; | |
| export const checkNotificationPermission = async (): Promise<boolean> => { | |
| if (!isNotificationSupported) return false; | |
| if (Notification.permission === 'granted') { | |
| return true; | |
| } | |
| if (Notification.permission !== 'denied') { | |
| try { | |
| const permission = await Notification.requestPermission(); | |
| return permission === 'granted'; | |
| } catch (error) { | |
| console.error('Error requesting notification permission:', error); | |
| return false; | |
| } | |
| } | |
| return false; | |
| }; | |
| if (isNotificationSupported && isServiceWorkerSupported) { | |
| messaging = getMessaging(app); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/api/notification.ts (1)
8-41: Consider removing commented-out code.The old PushSubscription implementation is commented out but not removed. While keeping it temporarily during transition is understandable, consider removing it once the new FCM implementation is confirmed working.
frontend/src/main.tsx (2)
22-24: Consider removing commented-out code.The mocking code has been commented out but not removed. Consider removing it if it's no longer needed.
35-59: Firebase Cloud Messaging token handling properly implemented.The new functions to handle notification permissions and device token retrieval are well-structured:
handleAllowNotificationproperly orchestrates the permission request and service worker registrationgetDeviceTokenhandles the Firebase token retrieval with appropriate error handlingHowever, the alert on line 50 might be intended for debugging and should probably be removed before merging.
- alert('토큰: ' + currentToken);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/eslint.config.js(2 hunks)frontend/src/api/notification.ts(1 hunks)frontend/src/config/settingFCM.ts(1 hunks)frontend/src/constants/url.ts(1 hunks)frontend/src/main.tsx(4 hunks)frontend/src/pages/PlanPage/PlanPage.styled.ts(1 hunks)frontend/src/pages/PlanPage/components/PlanInfoHeader/PlanInfoHeader.styled.ts(0 hunks)frontend/vite.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/pages/PlanPage/components/PlanInfoHeader/PlanInfoHeader.styled.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/src/pages/PlanPage/PlanPage.styled.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/eslint.config.js
- frontend/src/config/settingFCM.ts
🔇 Additional comments (9)
frontend/src/constants/url.ts (1)
49-49: URL added for FCM token API endpoint.Good addition of the FCM token endpoint which aligns with the PR objective of fixing service worker functionality in the PWA.
frontend/src/api/notification.ts (2)
1-5: Firebase messaging integration looks good.The imports for Firebase Cloud Messaging functionality have been correctly added, which supports the service worker implementation mentioned in the PR objective.
43-71: Implementation migrated from Web Push API to Firebase Cloud Messaging.The function has been successfully refactored to use Firebase Cloud Messaging instead of the browser's native Push API. This aligns with the PR's objective of fixing service worker functionality.
A few observations:
- The notification permission request is now handled here
- Device token retrieval and error handling look good
- The token is properly sent to the new FCM endpoint
frontend/src/main.tsx (4)
1-10: Firebase messaging configuration imports added correctly.The required imports for Firebase Cloud Messaging have been added, which supports the service worker implementation.
27-33: Service worker registration path updated.The service worker registration has been correctly updated to use the absolute path to the Firebase messaging service worker file, which addresses the PR objective of fixing service worker functionality.
61-62: Service worker registration strategy improved.The change from immediate service worker registration to the notification permission flow is a good improvement that aligns with the PR objective.
79-79: Removed ReactQueryDevtools component.Good job removing the duplicate ReactQueryDevtools component.
frontend/vite.config.ts (2)
12-17: Service worker strategy updated to match Firebase messaging requirements.The PWA configuration has been properly updated to use the
generateSWstrategy instead ofinjectManifest, which addresses the PR objective. The filename has been correctly set to match the new Firebase messaging service worker.This change is essential for the service worker to run correctly in the PWA environment.
18-18: File path corrected for apple-touch-icon.The path has been updated to include the file extension, which is a good correction.
| @@ -0,0 +1,51 @@ | |||
|
|
|||
| importScripts('https://storage.googleapis.com/workbox-cdn/releases/6.5.4/workbox-sw.js'); | |||
There was a problem hiding this comment.
💭 질문 💭
FCM에서도 이 파일도 쓰는건가요??
Issue Number
close #243
As-Is
To-Be
Check List
Test Screenshot
(Optional) Additional Description
PWA injectManifest 전략을 사용할 경우 서비스워커를 src 폴더에 두어야 합니다.
https://vite-pwa-org.netlify.app/guide/inject-manifest
Summary by CodeRabbit
New Features
Chores
HorizontalLinecomponent to improve layout spacing.HorizontalLinecomponent from thePlanInfoHeaderstyling.