Refactor(client, extension, setting): Extension/client communication 로직 추상화 및 연결 공통 상수 패키지 추가#311
Open
constantly-dev wants to merge 8 commits intodevelopfrom
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
개요
변경 사항
코드 검토 예상 시간🎯 4 (복잡) | ⏱️ ~50분 관련 PR
제안 레이블
제안 검토자
시
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Storybook chromatic 배포 확인: |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/extension/package.json (1)
14-18:⚠️ Potential issue | 🟠 MajorReact 19 런타임과 타입 패키지 버전이 어긋나 있습니다.
Lines 17-18에서
react와react-dom이 19.1.1로 설정되어 있으나, Lines 30-31의@types/react@^18.3.5와@types/react-dom@^18.3.0은 React 18 타입입니다. 이 조합은 React 19의 타입 변경사항을 반영하지 못하거나 잘못된 타입 에러를 발생시킬 수 있습니다.@types/react@^19와@types/react-dom@^19로 업그레이드해야 합니다.🔧 제안 수정
- "@types/react": "^18.3.5", - "@types/react-dom": "^18.3.0", + "@types/react": "^19.0.0", + "@types/react-dom": "^19.0.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/extension/package.json` around lines 14 - 18, The package.json lists react and react-dom at 19.1.1 but still depends on `@types/react`@^18.3.5 and `@types/react-dom`@^18.3.0; update the type packages to match React 19 by replacing `@types/react` and `@types/react-dom` with versions compatible with React 19 (e.g., `@types/react`@^19 and `@types/react-dom`@^19) so the runtime and type definitions align; locate and edit the dependencies/devDependencies entries for "react", "react-dom", "@types/react", and "@types/react-dom" in package.json and bump the two `@types` entries accordingly.
🧹 Nitpick comments (4)
apps/client/src/shared/utils/authStorage.ts (1)
1-27: 인증 스토리지 추상화가 잘 구현되었습니다.localStorage 접근을 중앙화하여 키 관리와 세션 정리 로직이 깔끔해졌습니다. 몇 가지 개선 사항을 제안드립니다:
- getter 메서드 추가 고려:
setRefreshToken,setHasJob,setUserIdentity에 대응하는 getter 메서드가 없습니다. 향후 이 값들을 읽어야 할 경우를 대비해 추가하는 것이 좋습니다.♻️ getter 메서드 추가 제안
export const authStorage = { getAccessToken: () => localStorage.getItem(AUTH_STORAGE_KEYS.token), hasAccessToken: () => !!localStorage.getItem(AUTH_STORAGE_KEYS.token), + getRefreshToken: () => localStorage.getItem(AUTH_STORAGE_KEYS.refreshToken), + getEmail: () => localStorage.getItem(AUTH_STORAGE_KEYS.email), + getUserId: () => localStorage.getItem(AUTH_STORAGE_KEYS.userId), + getHasJob: () => localStorage.getItem(AUTH_STORAGE_KEYS.hasJob) === 'true', setAccessToken: (token: string) =>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/client/src/shared/utils/authStorage.ts` around lines 1 - 27, Add missing getter methods for refreshToken, hasJob, email and userId so stored values can be read consistently: implement getRefreshToken() to return localStorage.getItem(AUTH_STORAGE_KEYS.refreshToken), getHasJob() to read AUTH_STORAGE_KEYS.hasJob and return a boolean (e.g. JSON/Boolean coercion or comparing to "true"), and getEmail()/getUserId() (or a single getUserIdentity() that returns { email, userId }) to read AUTH_STORAGE_KEYS.email and AUTH_STORAGE_KEYS.userId; place these alongside existing methods like getAccessToken(), hasAccessToken(), setRefreshToken(), setHasJob(), and setUserIdentity() and ensure return types are typed correctly.apps/client/src/shared/apis/setting/axiosInstance.ts (2)
58-60: noAuthNeeded URL 매칭 로직 개선 권장
includes()를 사용한 URL 매칭은 의도치 않은 매칭이 발생할 수 있습니다. 예를 들어/api/v1/auth/token-refresh라는 엔드포인트가 있다면/api/v1/auth/token과 매칭됩니다.♻️ 더 정확한 매칭 로직 제안
- const isNoAuth = noAuthNeeded.some((url) => - originalRequest.url?.includes(url) - ); + const isNoAuth = noAuthNeeded.some( + (url) => originalRequest.url === url || originalRequest.url?.startsWith(url + '?') + );또는 정규 표현식이나 완전 일치(exact match)를 사용하는 것이 더 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/client/src/shared/apis/setting/axiosInstance.ts` around lines 58 - 60, The URL matching using noAuthNeeded.some(... originalRequest.url?.includes(url)) can produce false positives (e.g., "/api/v1/auth/token" matching "/api/v1/auth/token-refresh"); change the matching to a stricter check by using exact equality or a safe RegExp anchored to the path segment. Update the isNoAuth computation to compare originalRequest.url (or its pathname) against each entry in noAuthNeeded with either url === originalRequest.url or new RegExp(`^${escapeForRegExp(url)}(?:$|\\?)`).test(originalRequest.url) (ensure to add an escapeForRegExp helper), referencing isNoAuth, noAuthNeeded, and originalRequest.url in axiosInstance.ts.
64-91: 동시 401 응답 시 토큰 재발급 경합 상태(Race Condition) 가능성여러 API 요청이 동시에 401을 받으면 각각
reissueToken()을 호출할 수 있습니다. 이로 인해 불필요한 재발급 요청이 발생하거나, 첫 번째 재발급 후 발급된 토큰이 두 번째 재발급으로 덮어씌워질 수 있습니다.일반적인 해결 방법은 재발급 Promise를 공유하여 중복 호출을 방지하는 것입니다.
♻️ 토큰 재발급 경합 방지 패턴
+ let isRefreshing = false; + let refreshSubscribers: ((token: string) => void)[] = []; + + const subscribeTokenRefresh = (cb: (token: string) => void) => { + refreshSubscribers.push(cb); + }; + + const onRefreshed = (token: string) => { + refreshSubscribers.forEach((cb) => cb(token)); + refreshSubscribers = []; + }; apiRequest.interceptors.response.use( (response) => response, async (error) => { const originalRequest = error.config; // ... existing checks ... if (/* 401/403 conditions */) { originalRequest._retry = true; + if (isRefreshing) { + return new Promise((resolve) => { + subscribeTokenRefresh((token) => { + originalRequest.headers.Authorization = `Bearer ${token}`; + resolve(apiRequest(originalRequest)); + }); + }); + } + + isRefreshing = true; try { const res = await reissueToken(); const newAccessToken = res.data?.data?.token; // ... syncAccessToken(newAccessToken); + onRefreshed(newAccessToken); // ... } catch (reissueError) { // ... + } finally { + isRefreshing = false; } } } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/client/src/shared/apis/setting/axiosInstance.ts` around lines 64 - 91, Concurrent 401s can trigger multiple reissueToken() calls; introduce a shared module-level promise (e.g., reissueTokenPromise) to serialize token refresh: in the axios error handler (where reissueToken(), syncAccessToken(), originalRequest, and apiRequest are used) check if reissueTokenPromise exists and await it, otherwise set reissueTokenPromise = reissueToken() and await it, then extract newAccessToken, call syncAccessToken(newAccessToken), update originalRequest.headers.Authorization and call apiRequest(originalRequest); ensure reissueTokenPromise is cleared (null) after resolution or rejection, and on rejection perform clearAuthSessionAndRedirect() and return a rejected promise so existing rejection logic continues.apps/client/src/pages/onBoarding/GoogleCallback.tsx (1)
12-22: useEffect 의존성 배열 검토
useEffect가 빈 의존성 배열[]을 사용하여 마운트 시 한 번만 실행됩니다.searchParams와loginWithCode를 사용하지만 의존성에 포함되어 있지 않습니다. 이는 OAuth 콜백 처리에서 흔한 패턴이지만, ESLintexhaustive-deps규칙에서 경고가 발생할 수 있습니다.현재 동작이 의도된 것이라면 주석으로 명시하는 것을 권장합니다.
의도된 동작임을 명시하는 주석 추가
useEffect(() => { const code = searchParams.get('code'); if (!code) { alert('로그인 실패. 다시 시도해주세요.'); navigate('/onboarding?step=SOCIAL_LOGIN'); return; } loginWithCode(code); + // eslint-disable-next-line react-hooks/exhaustive-deps }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/client/src/pages/onBoarding/GoogleCallback.tsx` around lines 12 - 22, The useEffect currently omits searchParams and loginWithCode from its dependency array which will trigger eslint exhaustive-deps warnings; if the one-time-on-mount behavior is intentional, add a clear inline comment above the useEffect explaining the intent and silence the rule for this line (e.g., // intentional: run once for OAuth callback; eslint-disable-next-line react-hooks/exhaustive-deps) so searchParams and loginWithCode are not added, otherwise include searchParams and loginWithCode in the dependency array of the useEffect callback to satisfy exhaustive-deps and ensure correct updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/extension/src/content.ts`:
- Line 1: Incoming MessageEvent.data must be validated at runtime before
property access; update the message listeners in apps/extension/src/content.ts
to first check that event.data is non-null and typeof event.data === 'object',
then narrow on ('type' in data) and compare against EXTENSION_MESSAGE_TYPE
values (e.g., EXTENSION_MESSAGE_TYPE.SET_TOKEN) in a safe switch/if; when
handling the SET_TOKEN branch validate that (data as any).token is a string
before calling the setToken path or forwarding it, and add analogous type guards
for any other branches referenced around the listeners (lines near the current
handlers) so no property is accessed on null/invalid payloads.
---
Outside diff comments:
In `@apps/extension/package.json`:
- Around line 14-18: The package.json lists react and react-dom at 19.1.1 but
still depends on `@types/react`@^18.3.5 and `@types/react-dom`@^18.3.0; update the
type packages to match React 19 by replacing `@types/react` and `@types/react-dom`
with versions compatible with React 19 (e.g., `@types/react`@^19 and
`@types/react-dom`@^19) so the runtime and type definitions align; locate and edit
the dependencies/devDependencies entries for "react", "react-dom",
"@types/react", and "@types/react-dom" in package.json and bump the two `@types`
entries accordingly.
---
Nitpick comments:
In `@apps/client/src/pages/onBoarding/GoogleCallback.tsx`:
- Around line 12-22: The useEffect currently omits searchParams and
loginWithCode from its dependency array which will trigger eslint
exhaustive-deps warnings; if the one-time-on-mount behavior is intentional, add
a clear inline comment above the useEffect explaining the intent and silence the
rule for this line (e.g., // intentional: run once for OAuth callback;
eslint-disable-next-line react-hooks/exhaustive-deps) so searchParams and
loginWithCode are not added, otherwise include searchParams and loginWithCode in
the dependency array of the useEffect callback to satisfy exhaustive-deps and
ensure correct updates.
In `@apps/client/src/shared/apis/setting/axiosInstance.ts`:
- Around line 58-60: The URL matching using noAuthNeeded.some(...
originalRequest.url?.includes(url)) can produce false positives (e.g.,
"/api/v1/auth/token" matching "/api/v1/auth/token-refresh"); change the matching
to a stricter check by using exact equality or a safe RegExp anchored to the
path segment. Update the isNoAuth computation to compare originalRequest.url (or
its pathname) against each entry in noAuthNeeded with either url ===
originalRequest.url or new
RegExp(`^${escapeForRegExp(url)}(?:$|\\?)`).test(originalRequest.url) (ensure to
add an escapeForRegExp helper), referencing isNoAuth, noAuthNeeded, and
originalRequest.url in axiosInstance.ts.
- Around line 64-91: Concurrent 401s can trigger multiple reissueToken() calls;
introduce a shared module-level promise (e.g., reissueTokenPromise) to serialize
token refresh: in the axios error handler (where reissueToken(),
syncAccessToken(), originalRequest, and apiRequest are used) check if
reissueTokenPromise exists and await it, otherwise set reissueTokenPromise =
reissueToken() and await it, then extract newAccessToken, call
syncAccessToken(newAccessToken), update originalRequest.headers.Authorization
and call apiRequest(originalRequest); ensure reissueTokenPromise is cleared
(null) after resolution or rejection, and on rejection perform
clearAuthSessionAndRedirect() and return a rejected promise so existing
rejection logic continues.
In `@apps/client/src/shared/utils/authStorage.ts`:
- Around line 1-27: Add missing getter methods for refreshToken, hasJob, email
and userId so stored values can be read consistently: implement
getRefreshToken() to return
localStorage.getItem(AUTH_STORAGE_KEYS.refreshToken), getHasJob() to read
AUTH_STORAGE_KEYS.hasJob and return a boolean (e.g. JSON/Boolean coercion or
comparing to "true"), and getEmail()/getUserId() (or a single getUserIdentity()
that returns { email, userId }) to read AUTH_STORAGE_KEYS.email and
AUTH_STORAGE_KEYS.userId; place these alongside existing methods like
getAccessToken(), hasAccessToken(), setRefreshToken(), setHasJob(), and
setUserIdentity() and ensure return types are typed correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7239f19d-9b5f-4154-a0e2-d65b903b858a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
apps/client/package.jsonapps/client/src/layout/Layout.tsxapps/client/src/pages/onBoarding/GoogleCallback.tsxapps/client/src/shared/apis/queries.tsapps/client/src/shared/apis/setting/axiosInstance.tsapps/client/src/shared/components/profilePopup/ProfilePopup.tsxapps/client/src/shared/utils/authStorage.tsapps/client/src/shared/utils/extensionBridge.tsapps/extension/package.jsonapps/extension/src/background.tsapps/extension/src/content.tspackages/contracts/package.jsonpackages/contracts/src/extension-messages.tspackages/contracts/tsconfig.json
| @@ -1,8 +1,10 @@ | |||
| import { EXTENSION_MESSAGE_TYPE } from '@pinback/contracts/extension-messages'; | |||
There was a problem hiding this comment.
메시지 payload를 런타임에서 검증해야 합니다.
MessageEvent.data는 null이나 임의의 값일 수 있는데, 여기서는 두 리스너 모두 event.data.type을 바로 읽고 있습니다. 그래서 잘못된 postMessage 하나만 들어와도 예외가 날 수 있고, setToken 경로는 token이 문자열인지 확인하지 않은 채 그대로 저장/전달합니다. packages/contracts/src/extension-messages.ts의 타입은 컴파일 타임 계약일 뿐이라, 이 경계에서는 별도 가드가 필요합니다.
🛡️ 제안 수정
-import { EXTENSION_MESSAGE_TYPE } from '@pinback/contracts/extension-messages';
+import {
+ EXTENSION_MESSAGE_TYPE,
+ type ExtensionMessage,
+} from '@pinback/contracts/extension-messages';
+
+function isExtensionMessage(data: unknown): data is ExtensionMessage {
+ if (!data || typeof data !== 'object') return false;
+
+ const candidate = data as { type?: unknown; token?: unknown };
+
+ if (candidate.type === EXTENSION_MESSAGE_TYPE.setToken) {
+ return typeof candidate.token === 'string';
+ }
+
+ return candidate.type === EXTENSION_MESSAGE_TYPE.logout;
+}
window.addEventListener('message', (event) => {
if (event.source !== window) return;
+ if (!isExtensionMessage(event.data)) return;
if (event.data.type === EXTENSION_MESSAGE_TYPE.setToken) {
chrome.runtime.sendMessage({
type: EXTENSION_MESSAGE_TYPE.setToken,
@@
window.addEventListener('message', (event) => {
if (event.source !== window) return;
+ if (!isExtensionMessage(event.data)) return;
if (event.data.type === EXTENSION_MESSAGE_TYPE.logout) {
chrome.storage.local.remove('token', () => {
console.log('Token removed!');Also applies to: 3-8, 16-18
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/extension/src/content.ts` at line 1, Incoming MessageEvent.data must be
validated at runtime before property access; update the message listeners in
apps/extension/src/content.ts to first check that event.data is non-null and
typeof event.data === 'object', then narrow on ('type' in data) and compare
against EXTENSION_MESSAGE_TYPE values (e.g., EXTENSION_MESSAGE_TYPE.SET_TOKEN)
in a safe switch/if; when handling the SET_TOKEN branch validate that (data as
any).token is a string before calling the setToken path or forwarding it, and
add analogous type guards for any other branches referenced around the listeners
(lines near the current handlers) so no property is accessed on null/invalid
payloads.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📌 Related Issues
📄 Tasks
⭐ PR Point (To Reviewer)
원래는 axiosInstance만 리팩터링 하려다가,,, extension과 client 연결 로직의 문제가 많은 것 같아 같이 수정했습니다.
🤔 기존 코드 문제
Pinback 프로젝트는 잘 아시겠지만 client와 extension 사이에 연결이 필요해요. 자세히 말해 client에서 로그인하고 받은 token 등의 auth 관련 데이터를 extension에도 넘겨줘야하는(send message) 로직이 필요하죠.
물론 이전 코드가 해당 요구사항을 잘 반영하기는 했지만 아래와 같은 문제가 있다고 판단했어요.
즉, 반복되는 로직을 계속해서 사용해야했고 key값이 2개의 apps에서 하드 코딩되어 유지보수 측면에서 리팩터링이 필요하다고 느꼈어요.
1️⃣ authStorage/extensionBridge 추상화
위 1번의 문제를 해결하기 위해 추상화를 진행했어요.
사실 처음에는 아래와 같이 localStorage관련 로직과 send token 로직을 한번에 담은 함수로 추상화하려고 했어요.
하지만 이 comment를 참고하시면 아실 수 있듯이 두 개는 너무 다른 역할의 로직이라고 판단했어요. 따라서 이를 추상화하면 오히려 예측 가능성이 낮아질 수 있다고 생각했어요.
따라서 이 두개를 따로 추상화를 하고 각각 import해 사용하는 것으로 최종 결론을 내렸어요. 이를 authStorage + extensionBridge로 추상화했어요.
물론 이렇게 추상화 했을 때 추가적인 상황에 대한 메서드를 개발자가 매번 추가해야한다는 trade-off가 존재할 수 있어요. 하지만 auth 관련 로직이라 큰 변동이 없을 것이라고 생각했고, 재사용 및 가독성 측면의 장점이 더 크다고 생각이 들었어요.
2️⃣ 공통 연결 상수 패키지 추가
위 2번의 문제를 해결하기 위해 extension/client 두 apps를 연결할 공통 key 상수를 관리할 패키지인 contracts를 추가했어요.
물론 처음에는 이런 type, 상수를 packages에 두는 것이 맞나 싶었어요. 하지만 단순 상수가 아닌 모노레포에서 apps간 연결/계약(contracts)을 위한 공통 상수이고, 이를 각각 하드코딩을 하는 경우 이후에 생길 일치하지 않았을 때의 에러 및 유지보수를 위해서는 패키지로 두는 것이 이상하지 않다고 판단했어요.
추가로 turborepo에서 공유 유틸 등을 패키지로 두는 것을 예시로 둘만큼 적절한 판단이고, 이는 이 공통 상수와도 비슷한 맥락이라고 생각했어요.
https://vercel.com/academy/production-monorepos/add-shared-utils#add-shared-utils
따라서 packages/contracts를 추가했고, 이 문자열 상수를 공통으로 관리해서 extension/client에서 의존성으로 추가하도록 했어요.
혹시나,,,,,, 이슈나 의견이 있으시다면 언제든 환영입니다!!!!!!!!!!
Summary by CodeRabbit
릴리스 노트
버그 수정
리팩토링