Setting(project): amplitude initial setting#326
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough새로운 Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client App
participant Analytics as Analytics<br/>Facade
participant Provider as Analytics<br/>Provider
participant Amplitude as Amplitude SDK
participant API as Backend API
Client->>Analytics: initAnalytics(apiKey, isDev)
alt isDev or no apiKey
Analytics->>Provider: setProvider(consoleProvider)
else
Analytics->>Provider: setProvider(amplitudeProvider)
Provider->>Amplitude: initAll(apiKey, config)
Amplitude-->>Provider: initialized
end
Client->>API: GET /api/v3/users/properties
API-->>Client: { userId, jobRole }
Client->>Analytics: identify(userId, { job_role })
Analytics->>Provider: identify(userId, properties)
Provider->>Amplitude: setUserId / identify(...)
Amplitude-->>Provider: acknowledged
Client->>Analytics: track(eventName, properties)
Analytics->>Provider: track(eventName, properties)
Provider->>Amplitude: track(...)
Amplitude-->>Provider: recorded
Client->>Analytics: reset()
Analytics->>Provider: reset()
Provider->>Amplitude: reset()
Amplitude-->>Provider: reset complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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: 4
🧹 Nitpick comments (4)
packages/analytics/package.json (1)
4-4: 패키지 엔트리 포인트 설정 개선 권장
main: "index.ts"는 TypeScript 파일을 직접 가리키고 있습니다. 모노레포 내부에서는 동작할 수 있지만, 더 명확한 패키지 해상도를 위해exports필드 추가를 권장합니다.♻️ 제안된 변경
{ "name": "@pinback/analytics", "version": "1.0.0", "main": "index.ts", + "types": "index.ts", + "exports": { + ".": "./index.ts" + }, "scripts": {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/analytics/package.json` at line 4, The package.json currently sets "main": "index.ts" which points to a TypeScript source file; update package.json so the package resolves to built artifacts by changing "main" to the compiled entry (e.g., "dist/index.js") and add an "exports" map (and optionally "types") that exposes the module entry and subpaths; adjust references to "index.ts" to point to the JS build output and ensure your build step outputs the expected dist file so consumers and Node's package resolution use the correct artifact.apps/extension/package.json (1)
19-19: 의존성 그룹화 개선 권장
@pinback/analytics를@pinback/contracts바로 아래에 배치하면 workspace 의존성들이 함께 그룹화되어 가독성이 향상됩니다.♻️ 제안된 변경
"dependencies": { "@pinback/contracts": "workspace:*", + "@pinback/analytics": "workspace:*", "@tanstack/react-query": "^5.85.5", - "@pinback/analytics": "workspace:*", "axios": "^1.11.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/extension/package.json` at line 19, 이 패키지의 dependencies 목록에서 "@pinback/analytics" 항목을 현재 위치에서 잘라내어 "@pinback/contracts" 바로 아래에 붙여넣어 workspace 의존성들이 함께 그룹화되도록 정렬하세요; 대상 식별자: "@pinback/analytics"와 "@pinback/contracts" (apps/extension의 package.json dependencies 섹션에서 항목을 이동하면 됨).packages/analytics/src/providers/amplitude.ts (1)
39-45:userProperties가 제공될 때만identify호출이 발생합니다.현재
GoogleCallback.tsx에서analytics.identify(String(userId))를userProperties없이 호출하고 있습니다. 이 경우setUserId는 호출되지만identify이벤트는 Amplitude로 전송되지 않습니다.의도된 동작이라면 괜찮지만, 사용자 속성 없이도 identify 이벤트를 전송하려면 조건을 수정해야 합니다.
♻️ userProperties 유무와 관계없이 identify 이벤트 전송
identify(userId: string, userProperties?: UserProperties) { setUserId(userId); - if (userProperties) { - const identifyEvent = new Identify(); - Object.entries(userProperties).forEach(([key, value]) => { - identifyEvent.set(key, value as string | number | boolean); - }); - identify(identifyEvent); - } + const identifyEvent = new Identify(); + if (userProperties) { + Object.entries(userProperties).forEach(([key, value]) => { + identifyEvent.set(key, value as string | number | boolean); + }); + } + identify(identifyEvent); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/analytics/src/providers/amplitude.ts` around lines 39 - 45, Currently identify(identifyEvent) is only called when userProperties exists, so calls like analytics.identify(String(userId)) with no userProperties never send an identify to Amplitude; change the logic in the amplitude provider so you always construct an Identify() (using the Identify class) and call identify(identifyEvent) regardless of whether userProperties is present, and if a userId is available set it on the Identify instance (and still loop over userProperties to set other keys when present) so identify events are sent both with and without userProperties.packages/analytics/ampli.json (1)
1-14: 파일 끝에 줄바꿈이 누락되었습니다.대부분의 린터와 에디터 설정에서는 파일 끝에 줄바꿈을 권장합니다.
🔧 줄바꿈 추가
"SDK": "@amplitude/analytics-browser@^1.0", "Path": "./src/ampli" } +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/analytics/ampli.json` around lines 1 - 14, The file ampli.json is missing a trailing newline at EOF; open the ampli.json (identify via keys like "VersionId": "a8d7f423-b996-4b7f-b836-cd64e2319f47" or "Runtime": "browser:typescript-ampli-v2") and add a single newline character at the end of the file so the file ends with a line break.
🤖 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/client/src/pages/onBoarding/GoogleCallback.tsx`:
- Line 69: The analytics.identify(String(userId)) call will be a no-op unless
the analytics SDK is initialized; call initAnalytics({...}) during app startup
(e.g., in your app entry/init function) with the appropriate apiKey and isDev
flag so `@pinback/analytics` uses the real provider instead of noopProvider;
ensure initAnalytics({ apiKey, isDev }) runs before any calls like
analytics.identify(String(userId)).
In `@apps/landing/package.json`:
- Line 16: The package.json adds the dependency "@pinback/analytics" but there
is no usage in the apps/landing codebase; either remove the "@pinback/analytics"
entry from the apps/landing package.json if it's unused, or add a clear
comment/README note in the apps/landing package.json (or a stub import in a
relevant file like src/main.tsx or src/index.ts) indicating it's intentionally
preinstalled for future use; update package.json to reflect the chosen action
and run yarn/npm install to keep lockfiles consistent.
In `@packages/analytics/src/providers/amplitude.ts`:
- Line 1: The amplitudeProvider currently calls initAll from `@amplitude/unified`
while ampli/index.ts creates its own `@amplitude/analytics-browser` instance,
which can cause duplicate SDK initialization when ampli.load() is invoked;
update ampli/index.ts so it does not create a new analytics-browser instance but
instead re-uses the instance initialized by amplitudeProvider (keep using
initAll and expose or import that initialized client rather than constructing a
new one), and ensure ampli.load() delegates to that shared instance; also change
the sessionReplay.sampleRate (referenced at line ~27 in the review) from 1 to a
lower default (e.g., 0.1) to avoid 100% session recording.
In `@packages/analytics/src/providers/noop.ts`:
- Line 6: The noop provider's identify implementation doesn't match the
AnalyticsProvider interface: update the identify function in
packages/analytics/src/providers/noop.ts to accept the second optional parameter
userProperties?: UserProperties (matching the interface signature) and ignore it
inside the body; reference the identify method and the
AnalyticsProvider/UserProperties types so the signature aligns with amplitude.ts
and the interface contract.
---
Nitpick comments:
In `@apps/extension/package.json`:
- Line 19: 이 패키지의 dependencies 목록에서 "@pinback/analytics" 항목을 현재 위치에서 잘라내어
"@pinback/contracts" 바로 아래에 붙여넣어 workspace 의존성들이 함께 그룹화되도록 정렬하세요; 대상 식별자:
"@pinback/analytics"와 "@pinback/contracts" (apps/extension의 package.json
dependencies 섹션에서 항목을 이동하면 됨).
In `@packages/analytics/ampli.json`:
- Around line 1-14: The file ampli.json is missing a trailing newline at EOF;
open the ampli.json (identify via keys like "VersionId":
"a8d7f423-b996-4b7f-b836-cd64e2319f47" or "Runtime":
"browser:typescript-ampli-v2") and add a single newline character at the end of
the file so the file ends with a line break.
In `@packages/analytics/package.json`:
- Line 4: The package.json currently sets "main": "index.ts" which points to a
TypeScript source file; update package.json so the package resolves to built
artifacts by changing "main" to the compiled entry (e.g., "dist/index.js") and
add an "exports" map (and optionally "types") that exposes the module entry and
subpaths; adjust references to "index.ts" to point to the JS build output and
ensure your build step outputs the expected dist file so consumers and Node's
package resolution use the correct artifact.
In `@packages/analytics/src/providers/amplitude.ts`:
- Around line 39-45: Currently identify(identifyEvent) is only called when
userProperties exists, so calls like analytics.identify(String(userId)) with no
userProperties never send an identify to Amplitude; change the logic in the
amplitude provider so you always construct an Identify() (using the Identify
class) and call identify(identifyEvent) regardless of whether userProperties is
present, and if a userId is available set it on the Identify instance (and still
loop over userProperties to set other keys when present) so identify events are
sent both with and without userProperties.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b92f4962-8349-4da5-bbcd-1ff137e8b56c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
apps/client/package.jsonapps/client/src/pages/onBoarding/GoogleCallback.tsxapps/client/src/shared/components/profilePopup/ProfilePopup.tsxapps/extension/package.jsonapps/landing/package.jsonpackages/analytics/ampli.jsonpackages/analytics/index.tspackages/analytics/package.jsonpackages/analytics/src/ampli/index.tspackages/analytics/src/analytics.tspackages/analytics/src/providers/amplitude.tspackages/analytics/src/providers/console.tspackages/analytics/src/providers/noop.tspackages/analytics/src/types.tsturbo.json
| res.data.data; | ||
|
|
||
| authStorage.setUserIdentity(email, userId); | ||
| analytics.identify(String(userId)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify initAnalytics is called in app entry points
# Search for initAnalytics usage in client app
echo "=== Searching for initAnalytics calls in apps/client ==="
rg -n "initAnalytics" apps/client/
# Search in main entry files
echo "=== Checking main.tsx files ==="
fd -t f "main.tsx" apps/ --exec cat {}Repository: Pinback-Team/pinback-client
Length of output: 951
🏁 Script executed:
# Search for initAnalytics in all TypeScript/TSX files
rg -t ts -t tsx "initAnalytics" apps/
# Check App.tsx
fd "App.tsx" apps/client/src/ --exec cat -n {}
# Check for any analytics initialization patterns
rg -t ts -t tsx "analytics" apps/client/src/ -l | head -20Repository: Pinback-Team/pinback-client
Length of output: 451
🏁 Script executed:
# Look at the analytics module itself
fd "@pinback/analytics" apps/ --type d
fd "analytics" apps/client/node_modules/@pinback/ --type d 2>/dev/null || echo "Checking other locations..."
rg "export.*initAnalytics" apps/Repository: Pinback-Team/pinback-client
Length of output: 98
🏁 Script executed:
# Search for `@pinback/analytics` imports
rg "@pinback/analytics" apps/client/src/
# Check GoogleCallback.tsx in detail
fd "GoogleCallback.tsx" apps/client/src/ --exec cat -n {}
# Search for any analytics-related setup or context
rg "noop|Provider|initAnalytics" apps/client/src/ -i -B 2 -A 2Repository: Pinback-Team/pinback-client
Length of output: 50385
🏁 Script executed:
# Check if there's a shared or local analytics module
fd "analytics" apps/client/src/ -type f
# Look for any useEffect or initialization patterns
rg "useEffect|onMount|initialize" apps/client/src/pages/onBoarding/ -A 3Repository: Pinback-Team/pinback-client
Length of output: 5139
initAnalytics() 초기화 호출 누락
analytics.identify()가 올바르게 호출되지만, 앱 전체에서 initAnalytics() 호출을 찾을 수 없습니다. @pinback/analytics는 기본적으로 noopProvider로 시작하므로, 초기화되지 않으면 모든 analytics 호출이 무시됩니다.
main.tsx 또는 앱 초기화 진입점에서 initAnalytics({ apiKey, isDev }) (또는 해당 패키지의 초기화 메서드)를 호출해야 합니다.
🤖 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` at line 69, The
analytics.identify(String(userId)) call will be a no-op unless the analytics SDK
is initialized; call initAnalytics({...}) during app startup (e.g., in your app
entry/init function) with the appropriate apiKey and isDev flag so
`@pinback/analytics` uses the real provider instead of noopProvider; ensure
initAnalytics({ apiKey, isDev }) runs before any calls like
analytics.identify(String(userId)).
| @@ -0,0 +1,51 @@ | |||
| import { Identify, identify, setUserId, reset, track, initAll } from '@amplitude/unified'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@amplitude/unified와 @amplitude/analytics-browser를 함께 사용할 때 권장되는 통합 방법은 무엇인가요?
💡 Result:
@amplitude/unified와 @amplitude/analytics-browser를 함께 사용하는 것은 권장되지 않습니다. @amplitude/unified는 Amplitude의 모든 기능(Analytics, Experiment, Session Replay, Guides and Surveys)을 하나의 패키지로 통합 제공하며, 내부적으로 @amplitude/analytics-browser를 의존성으로 포함하고 있습니다. 따라서 @amplitude/unified만 설치하고 initAll을 사용하여 초기화하는 것이 권장되는 통합 방법입니다. 만약 Analytics만 필요하다면 @amplitude/analytics-browser만 사용하세요. 번들 크기를 최소화하고 싶다면 개별 제품 SDK를 설치할 수 있지만, 둘 다 동시에 사용하는 것은 중복이며 초기화 충돌을 일으킬 수 있습니다. 예시 코드 (Unified SDK 사용): npm install @amplitude/unified import { initAll, track } from '@amplitude/unified'; initAll('YOUR_API_KEY', { analytics: { /* analytics-browser 옵션 */ } }); track('Event Name');
Citations:
- 1: https://amplitude.com/docs/sdks/analytics/browser/browser-unified-sdk
- 2: https://amplitude.com/docs/sdks/analytics/browser/browser-sdk-2
- 3: https://registry.npmjs.org/%40amplitude%2Funified
- 4: https://www.bellock.co.kr
🏁 Script executed:
# Find and examine the amplitude.ts file
find . -type f -name "amplitude.ts" | head -20Repository: Pinback-Team/pinback-client
Length of output: 118
🏁 Script executed:
# Find the ampli/index.ts file
find . -type f -path "*/ampli/index.ts"Repository: Pinback-Team/pinback-client
Length of output: 110
🏁 Script executed:
# Search for any imports of `@amplitude/analytics-browser` in the codebase
rg "@amplitude/analytics-browser" -t ts -t tsx --max-count=20Repository: Pinback-Team/pinback-client
Length of output: 98
🏁 Script executed:
# Search for any imports of `@amplitude/unified` in the codebase
rg "@amplitude/unified" -t ts -t tsx --max-count=20Repository: Pinback-Team/pinback-client
Length of output: 98
🏁 Script executed:
# Search for ampli.load() or ampli initialization in the codebase
rg "ampli\s*\." -t ts -t tsx | head -30Repository: Pinback-Team/pinback-client
Length of output: 98
🏁 Script executed:
# Read the amplitude.ts file to see imports and implementation
cat -n ./packages/analytics/src/providers/amplitude.tsRepository: Pinback-Team/pinback-client
Length of output: 1724
🏁 Script executed:
# Read the ampli/index.ts file to see imports
head -50 ./packages/analytics/src/ampli/index.tsRepository: Pinback-Team/pinback-client
Length of output: 1609
🏁 Script executed:
# Search for ampli.load() or ampli initialization
rg "ampli" --max-count=30Repository: Pinback-Team/pinback-client
Length of output: 5819
🏁 Script executed:
# Check if both initAll and Ampli client are initialized
rg "initAll|new Ampli" --max-count=20Repository: Pinback-Team/pinback-client
Length of output: 347
@amplitude/analytics-browser를 직접 초기화하지 마세요. 대신 @amplitude/unified 사용을 유지하세요.
현재 amplitudeProvider는 @amplitude/unified의 initAll을 사용하고 있으며, 이는 내부적으로 @amplitude/analytics-browser를 포함합니다. 생성된 ampli/index.ts는 @amplitude/analytics-browser를 직접 사용하도록 설정되어 있지만, ampli.load()가 호출되지 않으므로 현재는 문제가 없습니다.
그러나 향후 ampli.load()가 호출되면 같은 하위 SDK의 중복 인스턴스가 생성되어 초기화 충돌을 일으킬 수 있습니다. Ampli의 타입화된 이벤트를 사용하려면, ampli/index.ts를 수정하여 새로운 인스턴스를 생성하는 대신 amplitudeProvider에서 이미 초기화된 인스턴스를 재사용하도록 통합해야 합니다.
또한 line 27의 sessionReplay.sampleRate: 1은 100% 세션 녹화를 의미하므로, 대역폭과 비용이 상당할 수 있습니다. 필요에 따라 샘플링 비율을 조정하세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/analytics/src/providers/amplitude.ts` at line 1, The
amplitudeProvider currently calls initAll from `@amplitude/unified` while
ampli/index.ts creates its own `@amplitude/analytics-browser` instance, which can
cause duplicate SDK initialization when ampli.load() is invoked; update
ampli/index.ts so it does not create a new analytics-browser instance but
instead re-uses the instance initialized by amplitudeProvider (keep using
initAll and expose or import that initialized client rather than constructing a
new one), and ensure ampli.load() delegates to that shared instance; also change
the sessionReplay.sampleRate (referenced at line ~27 in the review) from 1 to a
lower default (e.g., 0.1) to avoid 100% session recording.
| export const noopProvider: AnalyticsProvider = { | ||
| init: () => {}, | ||
| track: () => {}, | ||
| identify: (_userId: string) => {}, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check AnalyticsProvider type definition for identify signature
echo "=== Checking AnalyticsProvider type definition ==="
cat packages/analytics/src/types.ts
echo "=== Checking other providers for consistency ==="
rg -n "identify" packages/analytics/src/providers/Repository: Pinback-Team/pinback-client
Length of output: 1192
identify 메서드 시그니처가 인터페이스와 일치하지 않습니다
AnalyticsProvider 인터페이스는 identify(userId: string, userProperties?: UserProperties): void로 정의되어 있으나, noop 구현에서는 userProperties 매개변수가 누락되어 있습니다. amplitude.ts에서는 올바르게 두 매개변수를 모두 구현하고 있습니다. 인터페이스 계약을 준수하도록 수정 필요합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/analytics/src/providers/noop.ts` at line 6, The noop provider's
identify implementation doesn't match the AnalyticsProvider interface: update
the identify function in packages/analytics/src/providers/noop.ts to accept the
second optional parameter userProperties?: UserProperties (matching the
interface signature) and ignore it inside the body; reference the identify
method and the AnalyticsProvider/UserProperties types so the signature aligns
with amplitude.ts and the interface contract.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/client/src/shared/utils/authStorage.ts (1)
23-26:getUserIdgetter 추가를 고려해 주세요.
setUserIdentity로userId를 저장하지만, 이를 가져오는 getter 메서드가 없습니다.Layout.tsx에서localStorage.getItem('userId')를 직접 사용하고 있는데, 일관성을 위해getUserId메서드를 추가하면 좋겠습니다.♻️ getUserId 메서드 추가 제안
setUserIdentity: (email: string, userId: string) => { localStorage.setItem(AUTH_STORAGE_KEYS.email, email); localStorage.setItem(AUTH_STORAGE_KEYS.userId, userId); }, + getUserId: () => localStorage.getItem(AUTH_STORAGE_KEYS.userId), + getEmail: () => localStorage.getItem(AUTH_STORAGE_KEYS.email), clearSession: () => {🤖 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 23 - 26, The code saves userId via setUserIdentity but lacks a corresponding getter; add a getUserId method to the same authStorage module that returns localStorage.getItem(AUTH_STORAGE_KEYS.userId) (or null) and update consumers (e.g., Layout.tsx) to use authStorage.getUserId() instead of directly calling localStorage.getItem('userId') to keep storage access consistent and centralized.apps/client/src/main.tsx (1)
1-12: import 문을 상단에 그룹화하세요.
initAnalytics()호출이 import 문 사이에 위치해 있어 가독성이 떨어집니다. ES 모듈에서 import는 호이스팅되므로 기능상 문제는 없지만, 코드 구조의 명확성을 위해 모든 import를 상단에 모은 후 실행 코드를 배치하는 것이 좋습니다.♻️ import 그룹화 제안
import { initAnalytics } from '@pinback/analytics'; import { StrictMode } from 'react'; import { createRoot } from 'react-dom/client'; import App from './App.tsx'; - -initAnalytics({ - apiKey: import.meta.env.VITE_AMPLITUDE_API_KEY, - isDev: import.meta.env.DEV, -}); import { QueryClientProvider } from '@tanstack/react-query'; import getQueryClient from '@shared/apis/setting/query/getQueryClient.ts'; import { ReactQueryDevtools } from '@tanstack/react-query-devtools'; + +initAnalytics({ + apiKey: import.meta.env.VITE_AMPLITUDE_API_KEY, + isDev: import.meta.env.DEV, +}); const queryClient = getQueryClient();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/client/src/main.tsx` around lines 1 - 12, Move all import statements to the very top of the file (grouping related imports like react, libraries, and local modules) and then place the initAnalytics(...) call and any other runtime code below them; specifically, relocate the initAnalytics invocation so it appears after the grouped imports (e.g., after importing initAnalytics, StrictMode, createRoot, App, QueryClientProvider, getQueryClient, ReactQueryDevtools) in apps/client/src/main.tsx to improve readability and maintain standard ES module ordering.apps/client/src/layout/Layout.tsx (1)
17-17:authStorage를 사용하여 일관성을 유지하세요.
localStorage.getItem('userId')대신authStorage를 통해 접근하는 것이 바람직합니다. 현재authStorage에는userId를 가져오는 getter가 없으므로, 추가하거나 최소한AUTH_STORAGE_KEYS상수를 사용하는 것을 권장합니다.♻️ authStorage에 getUserId 메서드를 추가하는 제안
authStorage.ts에 getter 추가:getUserId: () => localStorage.getItem(AUTH_STORAGE_KEYS.userId),그 후
Layout.tsx에서 사용:- const userId = localStorage.getItem('userId'); + const userId = authStorage.getUserId();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/client/src/layout/Layout.tsx` at line 17, Replace the direct localStorage access in Layout.tsx (const userId = localStorage.getItem('userId')) with the authStorage API: add a getUserId getter to authStorage (e.g., getUserId: () => localStorage.getItem(AUTH_STORAGE_KEYS.userId)) or, alternatively, use AUTH_STORAGE_KEYS via an existing authStorage.get method; then update Layout.tsx to call authStorage.getUserId() (or authStorage.get(AUTH_STORAGE_KEYS.userId)) and remove the localStorage.getItem usage to maintain consistent auth storage access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/client/src/layout/Layout.tsx`:
- Line 17: Replace the direct localStorage access in Layout.tsx (const userId =
localStorage.getItem('userId')) with the authStorage API: add a getUserId getter
to authStorage (e.g., getUserId: () =>
localStorage.getItem(AUTH_STORAGE_KEYS.userId)) or, alternatively, use
AUTH_STORAGE_KEYS via an existing authStorage.get method; then update Layout.tsx
to call authStorage.getUserId() (or authStorage.get(AUTH_STORAGE_KEYS.userId))
and remove the localStorage.getItem usage to maintain consistent auth storage
access.
In `@apps/client/src/main.tsx`:
- Around line 1-12: Move all import statements to the very top of the file
(grouping related imports like react, libraries, and local modules) and then
place the initAnalytics(...) call and any other runtime code below them;
specifically, relocate the initAnalytics invocation so it appears after the
grouped imports (e.g., after importing initAnalytics, StrictMode, createRoot,
App, QueryClientProvider, getQueryClient, ReactQueryDevtools) in
apps/client/src/main.tsx to improve readability and maintain standard ES module
ordering.
In `@apps/client/src/shared/utils/authStorage.ts`:
- Around line 23-26: The code saves userId via setUserIdentity but lacks a
corresponding getter; add a getUserId method to the same authStorage module that
returns localStorage.getItem(AUTH_STORAGE_KEYS.userId) (or null) and update
consumers (e.g., Layout.tsx) to use authStorage.getUserId() instead of directly
calling localStorage.getItem('userId') to keep storage access consistent and
centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb8c2ba7-5de5-47b2-a2a6-8565caae5a17
📒 Files selected for processing (5)
apps/client/src/layout/Layout.tsxapps/client/src/main.tsxapps/client/src/pages/onBoarding/GoogleCallback.tsxapps/client/src/shared/utils/authStorage.tsapps/extension/src/popup.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/client/src/pages/onBoarding/GoogleCallback.tsx
jjangminii
left a comment
jjangminii
left a comment
There was a problem hiding this comment.
amplitude는 처음이라 보는데 조금 오래걸렸네요,, 세팅 하시느라 수고하셨습니다.
amplitude 트래킹 플랜을 코드로 옮기는 방법도 알아갑니다-!!
그리고 하나 코멘트 남겨요
현재 identify 호출이 Layout과 GoogleCallback 등 여러 곳에 분산되어 있는데 job_role 정보가 업데이트될 때마다 수동으로 identify를 호출하기보다는, 유저 정보가 저장되는 authStorage나 전역 상태의 변경을 구독하여 자동으로 트래킹이 일어나도록 Provider 패턴으로 한 번 더 감싸는 것에 대해 어떻게 생각하시나요? 사용부에서의 로직을 더 단순화할 수 있을 것 같습니다-!
| }, | ||
| }, | ||
| sessionReplay: { | ||
| sampleRate: 1, |
There was a problem hiding this comment.
sampleRate: 1은 모든 유저의 모든 행동을 녹화한다고 알고있습니다
그래서 클라이언트 네트워크 대역폭 부담될 수 있다 생각하는데 아직 초기 단계라 1로 설정 해도 문제 없는건지 아니면 따로 의도한 것이 있는지 궁금합니다-!
There was a problem hiding this comment.
통합 환경인 @amplitude/unified를 사용해서 기능들 다 사용해보려고 했던거라 큰 이유는 없습니다! 말씀해주신 내용을 참고할 때 초기에서는 사용하지 않아도 괜찮을 것 같네요! 감사합니다!
네넵 맞는 말씀입니다! 그래서 app에서만 identify 설정을 하도록 변경했어요. provider 쪽에 두는 것은 단순 값을 던져주는 역할인 provider안에 데이터 패칭 등 훅이 들어가는 것이 어색해서 하지 않았고, 말씀해주신 한번 더 wrapping하는 provider 패턴은 현재 상황에서는 오버 엔지니어링이라고 판단해서 app에 한번만 호출하도록 리팩했습니다! |
2 participants
📌 Related Issues
📄 Tasks
⭐ PR Point (To Reviewer)
1. analytics packages 분리
패키지로 분리한 이유는 모노레포 환경에서 apps에 있는 3개의 환경에서 공통된 설정을 가진 앰플리튜드 함수를 사용하게 되기 때문이에요. 또한 단순 ampli 제공 함수를 사용하는 것이 아니라 패키지로 분리하면서 간단하게 추상화를 해줬어요. 최대한 사용부에서는 내부 트래킹의 로직을 몰라도 사용이 가능하도록 하기 위함이에요.
또한 단순히 amplitude라는 이름으로 패키지를 만드는 것이 아니라 analytics라는 큰 틀의 패키지를 뒀어요.
그 이유는 물론 amplitude를 계속 사용할 것 같지만, 이후에 믹스 패널 등 다른 트래킹 툴로 변경이 될 가능성도 있기 때문에 최대한 네이밍에서도 의존성을 갖지 않도록 했어요.
2. dev/prod/storybook 로깅 환경 분리
사실 트래킹은 dev환경에서 필요가 없을 것이라고 판단했어요. 개발하면서 얻은 데이터는 활용도가 낮기 때문이에요. 또한 storybook에서 컴포넌트에는 이 트래킹이 되면 안돼요.
다만 개발 환경에서는 데이터를 수집할 필요는 없지만 이벤트 함수가 잘 작동을 하는지 테스트를 위한 로그는 필요하다고 생각했어요.
따라서 prod 환경에서는 트래킹이 되고, 나머지 환경에서는
console.log로 테스트를 할 수 있게 하기 위해서 환경을 분리했어요.기본
AnalyticsProviderinterface는 아래와 같아요.따라서 기본적으로 모든
provider가AnalyticsProvider를 따르게 되는데, 처음에는noopProvider로 초기화돼요. 아무런 동작이 없는 메서드를 가지고 있고, 이후initAnalytics를 실행하면서 개발 환경인지 아닌지에 따라서provider를 분기해요.consoleProvider: 각 메서드가 console.log 호출amplitudeProvider: 각 메서드가 @amplitude/unified 내부 메서드 호출이를 다이어그램으로 보면 아래와 같아요.
3. @amplitude/ampli 방식 활용
amplitude 트래킹 플랜을 코드로 옮기는 방법에는 크게 2가지가 존재해요.
쉽게 말해 앰플리튜드에 이벤트 정의를 기획자가 하는지, 개발자가 하는지 차이로 생각하면 돼요. trade-off가 존재하는데, 우리팀은 1번을 선택했어요.
그 이유는 2번의 경우 개발자가 코드에 작성한대로 앰플리튜드에 이벤트/프로퍼티가 자동 생성돼요. 이때 오타도 방지하지 못하고 interface도 직접 다 정의해야하기 때문에 type safe하지 않다는 단점이 있어요. 즉, 빌드 타임이 아닌 런타임에 오류나 에러를 감지하게 돼요. (오타는 아예 감지하지 못할 수도 있죠.)
1번 방법은 이를 방지해주기도 하고, 진실 원천이 보장돼요. 물론 1번도 단점은 존재해요.
ampli pullscript로 interface를 amplitude에서 받아오는 방식이다보니 어느정도 의존성이 생겨요. 따라서 이후에 앰플리튜드가 아니라 다른 툴을 쓰게 되면 이 구조를 바꿔야 하게 돼요.최대한 이 변동성을 최소화하기 위해서 위에서 언급한 것처럼 analytics라는 packages로 분리하게 되었고, 직접 amplitude 메서드를 활용하게 아니라 추상화를 통해 한번 wrapping하여 툴을 변동하게 되면 호출부는 변경할 필요없이 이 패키지만 바꾸면 되는 구조로 설계했어요.
Note
amplitude interface를 update해야한다면 아래와 같이 진행해주세요!
📷 Screenshot
Summary by CodeRabbit