Skip to content

Feat/firebase#49

Merged
dasosann merged 3 commits intomainfrom
feat/firebase
Mar 31, 2026
Merged

Feat/firebase#49
dasosann merged 3 commits intomainfrom
feat/firebase

Conversation

@dasosann
Copy link
Copy Markdown
Contributor

No description provided.

@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Firebase 초기화 로직 단순화 및 인증 경로 개선

✨ Enhancement

Grey Divider

Walkthroughs

Description
• 로그인 만료 시 리다이렉트 경로를 /login에서 /로 변경
• Firebase Service Worker 초기화 로직 단순화
  - postMessage 기반 설정 전달 제거
  - Firebase Config를 Service Worker에 직접 포함
• FCM 등록 공개 경로에 홈 경로(/) 추가
Diagram
flowchart LR
  A["로그인 만료 처리"] -->|경로 변경| B["/ 리다이렉트"]
  C["Service Worker 초기화"] -->|postMessage 제거| D["직접 Config 포함"]
  E["Firebase Config"] -->|하드코딩| F["firebase-messaging-sw.js"]
  G["FCM 공개 경로"] -->|추가| H["/ 포함"]
Loading

Grey Divider

File Changes

1. lib/axios.ts ✨ Enhancement +1/-1

로그인 만료 리다이렉트 경로 수정

• 로그인 세션 만료 시 리다이렉트 경로를 /login에서 /로 변경

lib/axios.ts


2. lib/firebase.ts ✨ Enhancement +1/-34

Service Worker Firebase 초기화 로직 단순화

waitForServiceWorkerActive 헬퍼 함수 제거
• Service Worker에 postMessage로 Firebase Config 전달하는 로직 삭제
• Service Worker 등록 후 즉시 Messaging 인스턴스 생성

lib/firebase.ts


3. public/firebase-messaging-sw.js ✨ Enhancement +22/-23

Firebase Config 직접 포함 및 초기화 단순화

• Firebase Config를 Service Worker에 직접 하드코딩
• 메인 앱에서의 postMessage 리스너 제거
• 백그라운드 메시지 처리 로직을 최상위 스코프로 이동
• Firebase 초기화를 Service Worker 로드 시점에 수행

public/firebase-messaging-sw.js


View more (1)
4. components/common/FcmInitializer.tsx ✨ Enhancement +8/-1

FCM 공개 경로에 홈 경로 추가

PUBLIC_PATHS 배열에 홈 경로(/) 추가
• FCM 등록이 필요 없는 공개 경로 확대

components/common/FcmInitializer.tsx


Grey Divider

Qodo Logo


✨ Describe tool usage guide:

Overview:
The describe tool scans the PR code changes, and generates a description for the PR - title, type, summary, walkthrough and labels. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

When commenting, to edit configurations related to the describe tool (pr_description section), use the following template:

/describe --pr_description.some_config1=... --pr_description.some_config2=...

With a configuration file, use the following template:

[pr_description]
some_config1=...
some_config2=...
Enabling\disabling automation
  • When you first install the app, the default mode for the describe tool is:
pr_commands = ["/describe", ...]

meaning the describe tool will run automatically on every PR.

  • Markers are an alternative way to control the generated description, to give maximal control to the user. If you set:
pr_commands = ["/describe --pr_description.use_description_markers=true", ...]

the tool will replace every marker of the form pr_agent:marker_name in the PR description with the relevant content, where marker_name is one of the following:

  • type: the PR type.
  • summary: the PR summary.
  • walkthrough: the PR walkthrough.
  • diagram: the PR sequence diagram (if enabled).

Note that when markers are enabled, if the original PR description does not contain any markers, the tool will not alter the description at all.

Custom labels

The default labels of the describe tool are quite generic: [Bug fix, Tests, Enhancement, Documentation, Other].

If you specify custom labels in the repo's labels page or via configuration file, you can get tailored labels for your use cases.
Examples for custom labels:

  • Main topic:performance - pr_agent:The main topic of this PR is performance
  • New endpoint - pr_agent:A new endpoint was added in this PR
  • SQL query - pr_agent:A new SQL query was added in this PR
  • Dockerfile changes - pr_agent:The PR contains changes in the Dockerfile
  • ...

The list above is eclectic, and aims to give an idea of different possibilities. Define custom labels that are relevant for your repo and use cases.
Note that Labels are not mutually exclusive, so you can add multiple label categories.
Make sure to provide proper title, and a detailed and well-phrased description for each label, so the tool will know when to suggest it.

Inline File Walkthrough 💎

For enhanced user experience, the describe tool can add file summaries directly to the "Files changed" tab in the PR page.
This will enable you to quickly understand the changes in each file, while reviewing the code changes (diffs).

To enable inline file summary, set pr_description.inline_file_summary in the configuration file, possible values are:

  • 'table': File changes walkthrough table will be displayed on the top of the "Files changed" tab, in addition to the "Conversation" tab.
  • true: A collapsable file comment with changes title and a changes summary for each file in the PR.
  • false (default): File changes walkthrough will be added only to the "Conversation" tab.
Utilizing extra instructions

The describe tool can be configured with extra instructions, to guide the model to a feedback tailored to the needs of your project.

Be specific, clear, and concise in the instructions. With extra instructions, you are the prompter. Notice that the general structure of the description is fixed, and cannot be changed. Extra instructions can change the content or style of each sub-section of the PR description.

Examples for extra instructions:

[pr_description]
extra_instructions="""- The PR title should be in the format: '<PR type>: <title>'
- The title should be short and concise (up to 10 words)
- ...
"""

Use triple quotes to write multi-line instructions. Use bullet points to make the instructions more readable.

More PR-Agent commands

To invoke the PR-Agent, add a comment using one of the following commands:

  • /review: Request a review of your Pull Request.
  • /describe: Update the PR title and description based on the contents of the PR.
  • /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
  • /ask <QUESTION>: Ask a question about the PR.
  • /update_changelog: Update the changelog based on the PR's contents.
  • /help_docs <QUESTION>: Given a path to documentation (either for this repository or for a given one), ask a question.
  • /add_docs 💎: Generate docstring for new components introduced in the PR.
  • /generate_labels 💎: Generate labels for the PR based on the PR's contents.
  • /analyze 💎: Automatically analyzes the PR, and presents changes walkthrough for each component.

See the tools guide for more details.
To list the possible configuration parameters, add a /config comment.

See the describe usage page for a comprehensive guide on using this tool.

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Mar 31, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Action required

1. FCM always skipped🐞 Bug ✓ Correctness
Description
Adding "/" to PUBLIC_PATHS makes pathname.startsWith("/") true for every route, so
FcmInitializer always returns early and never registers the service worker nor posts the FCM
token. This fully disables FCM token registration across the app.
Code

components/common/FcmInitializer.tsx[R9-16]

+const PUBLIC_PATHS = [
+  "/",
+  "/login",
+  "/register",
+  "/terms",
+  "/privacy",
+  "/reset",
+];
Evidence
FcmInitializer uses a prefix check (startsWith) against all entries in PUBLIC_PATHS; since every
Next pathname begins with "/", adding "/" causes the early-return guard to trigger for all routes
and prevents FCM registration from ever running.

components/common/FcmInitializer.tsx[9-23]
app/layout.tsx[72-76]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`PUBLIC_PATHS` now contains `"/"` while the guard uses `pathname.startsWith(path)`. Because every pathname starts with `/`, FCM initialization is skipped on every page.
## Issue Context
FCM is mounted globally in the app layout, so this guard controls whether FCM ever registers.
## Fix Focus Areas
- components/common/FcmInitializer.tsx[9-23]
## Suggested fix
Update the public-path check to avoid treating `"/"` as a prefix-match-all. For example:
- Use exact match for `/`: `if (pathname === "/") return;`
- Keep prefix logic for other public paths: `if (PUBLIC_PATHS.filter(p => p !== "/").some(p => pathname.startsWith(p))) return;`
(or implement a helper: `isPublicPath(pathname)` that treats `/` specially).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Hardcoded SW Firebase config🐞 Bug ⚙ Maintainability
Description
The service worker hardcodes Firebase config, while the app uses env-driven NEXT_PUBLIC_FIREBASE_*
config; this prevents per-environment configuration and can cause FCM to point at the wrong Firebase
project outside production. It also makes config rotation require code changes and redeploys rather
than environment updates.
Code

public/firebase-messaging-sw.js[R12-24]

+const firebaseConfig = {
+  apiKey: "AIzaSyCmwNjO2gXNy92-FBguQUXvPcQDaVk7lD0",
+  authDomain: "comatching5.firebaseapp.com",
+  projectId: "comatching5",
+  storageBucket: "comatching5.firebasestorage.app",
+  messagingSenderId: "179266935580",
+  appId: "1:179266935580:web:3b56e0f8cf763787f838d9",
+  measurementId: "G-P0WBX4VND7",
+};
-// 메인 앱에서 Firebase Config를 받아서 초기화
-self.addEventListener("message", (event) => {
-  if (event.data && event.data.type === "INIT_FIREBASE") {
-    const firebaseConfig = event.data.config;
+firebase.initializeApp(firebaseConfig);
-    if (!firebase.apps.length) {
-      firebase.initializeApp(firebaseConfig);
-      messaging = firebase.messaging();
+const messaging = firebase.messaging();
Evidence
The app’s Firebase config is explicitly designed to come from environment variables, but the service
worker now embeds a single fixed Firebase project config. This creates a divergence between the main
app initialization and the service worker initialization and removes the ability to switch Firebase
projects by changing environment variables (as implied by the repo’s .env/.env.example).

public/firebase-messaging-sw.js[1-24]
lib/firebase.ts[7-15]
.env.example[7-15]
.env[4-12]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`public/firebase-messaging-sw.js` hardcodes Firebase config, diverging from the rest of the app which is configured via `NEXT_PUBLIC_FIREBASE_*` environment variables. This blocks multi-environment setup (local/staging/prod) and makes config rotation require code edits.
## Issue Context
Service workers can’t read Next.js runtime env vars, but the project already expects Firebase config to be environment-driven.
## Fix Focus Areas
- public/firebase-messaging-sw.js[1-24]
- lib/firebase.ts[7-15]
## Suggested fix options
Pick one approach and make SW config environment-specific again:
1) **Serve the SW JS dynamically** (recommended for multi-env): create a Next route (e.g., `/firebase-messaging-sw.js`) that returns JS with config values injected server-side, and update registration to that URL.
2) **Build-time templating**: keep a template SW file (with placeholders) in source, and generate/copy into `public/` during build using environment variables.
3) **Config fetch**: have the SW `fetch('/api/firebase-config')` (public endpoint) and initialize Firebase from the returned JSON before calling `firebase.messaging()`.
Ensure the SW initializes the same Firebase project as `lib/firebase.ts` in each environment.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

이번 풀 리퀘스트는 서비스 워커 내부에서 Firebase를 직접 초기화하도록 로직을 변경하고, 인증 만료 시 리다이렉트 경로를 루트('/')로 수정했습니다. 리뷰 결과, PUBLIC_PATHS 배열에 '/'를 추가함에 따라 모든 경로에서 FCM 등록 로직이 중단되는 논리적 오류가 발견되어 수정이 필요합니다. 또한, 서비스 워커 내부에 Firebase 설정을 하드코딩하는 방식은 환경별 관리와 SDK 버전 일관성 유지 측면에서 개선이 권장됩니다.

@dasosann dasosann merged commit 4da5dc4 into main Mar 31, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant