[Feat] WTH-372: Sanity 팝업 연동 추가#110
Merged
Hidden character warning
The head ref may contain hidden characters: "feat/WTH-372-sainty-\ucd94\uac00\ud558\uae30"
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughSanity 기반 공지 팝업을 추가합니다: Sanity 클라이언트와 API를 추가하고, NoticePopup React 컴포넌트를 통해 홈 페이지에서 팝업을 표시하며 로컬 설정 파일의 permissions.allow 항목을 확장합니다. 변경 사항공지 팝업 기능
로컬 Claude 설정
코드 리뷰 난이도🎯 3 (보통) | ⏱️ ~20분
🚥 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 |
PR 검증 결과⏭️ TypeScript: 건너뜀 |
PR 테스트 결과⏭️ Jest: 건너뜀 |
JIN921
changed the title
|
구현한 기능 Preview: https://weeth-7q1fv4o6m-weethsite-4975s-projects.vercel.app |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/home/NoticePopup.tsx`:
- Line 12: HIDE_KEY is a global constant that causes hide state to be shared
across clubs; change it to be per-club/path by building a dynamic key (e.g.,
`${HIDE_KEY}_${clubId}` or `${HIDE_KEY}_${pathname}`) and update all usages (the
constant declaration for HIDE_KEY and the get/set logic around where you
read/write the hide-until value — the code near HIDE_KEY and the code paths at
the usage site around lines 45-49) to use this composed key so each club/path
has its own localStorage entry.
- Around line 67-79: The NoticePopup component contains many hardcoded Tailwind
arbitrary values (e.g., pt-[60px], w-[320px], max-w-[calc(100vw-32px)],
text-[18px], underline-offset-[3px], shadow-[...]) used directly inside the
cn(...) className calls; refactor these styles into a cva-based style object
(e.g., create cva variants like noticePopupContainer and popupCard) and replace
the arbitrary classes with design-token-first utility classes (or agreed token
names) inside the cva definitions; ensure all occurrences referenced in the diff
(the cn(...) wrapper and the popup card div) are updated, and do not add new
tokens without confirmation—ask the designer/product owner if a new token is
required before introducing one.
- Around line 120-136: The pagination buttons rendering the symbols ‹ and ›
(inside the buttons handled by handlePrev and handleNext, with state
currentIndex and popup.pages) lack screen-reader labels; add descriptive
aria-label attributes to both buttons (for example "Previous page" and "Next
page" or include dynamic context like "Previous page, page X of Y" / "Next page,
page X of Y") so assistive tech can convey their purpose and state; ensure the
labels update using currentIndex and popup.pages.length and keep existing
disabled behavior intact.
- Around line 97-106: The Image in NoticePopup.tsx is loading Sanity-hosted
images via currentPage.imageUrl but next.config.ts lacks the cdn.sanity.io
remotePatterns entry; open next.config.ts and add an entry to the
images.remotePatterns array with protocol: 'https' and hostname: 'cdn.sanity.io'
so next/image can load currentPage.imageUrl at runtime.
In `@src/lib/apis/popup.ts`:
- Around line 27-29: The environment-detection in getActivePopup relies on URL
pattern matching and can misclassify; change it to read an explicit env var
(e.g., NEXT_PUBLIC_APP_ENV) and validate its value is either 'dev' or
'production' before passing to sanityClient.fetch. Update the currentEnv
assignment in getActivePopup to use process.env.NEXT_PUBLIC_APP_ENV, default to
'production' only if the value is missing or invalid, and consider adding a
small warning log when falling back so misconfiguration is visible.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc783def-53d5-4ca3-a343-75166e689192
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsrc/assets/image/popup_default_img_1.pngis excluded by!**/*.png
📒 Files selected for processing (6)
package.jsonsrc/components/home/HomePageEntry.tsxsrc/components/home/NoticePopup.tsxsrc/components/home/index.tssrc/lib/apis/popup.tssrc/lib/sanity/client.ts
| import { getActivePopup, type PopupDocument } from '@/lib/apis/popup'; | ||
| import DefaultPopupImg from '@/assets/image/popup_default_img_1.png'; | ||
|
|
||
| const HIDE_KEY = 'popup_hide_until'; |
There was a problem hiding this comment.
24시간 숨김 키를 클럽 단위로 분리하는 게 안전합니다.
Line 12의 고정 키(popup_hide_until)는 클럽 간 상태가 섞일 수 있습니다. clubId 또는 pathname을 포함해 키를 분리하면 의도치 않은 교차 숨김을 막을 수 있습니다.
Also applies to: 45-49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/home/NoticePopup.tsx` at line 12, HIDE_KEY is a global
constant that causes hide state to be shared across clubs; change it to be
per-club/path by building a dynamic key (e.g., `${HIDE_KEY}_${clubId}` or
`${HIDE_KEY}_${pathname}`) and update all usages (the constant declaration for
HIDE_KEY and the get/set logic around where you read/write the hide-until value
— the code near HIDE_KEY and the code paths at the usage site around lines
45-49) to use this composed key so each club/path has its own localStorage
entry.
Comment on lines
+67
to
+79
| className={cn( | ||
| 'pointer-events-none fixed inset-0 z-50 flex flex-col', | ||
| 'items-center justify-start pt-[60px]', | ||
| 'desktop:items-end desktop:justify-end desktop:p-600', | ||
| )} | ||
| > | ||
| <div className="pointer-events-auto flex flex-col" onClick={(e) => e.stopPropagation()}> | ||
| {/* 팝업 카드 */} | ||
| <div | ||
| className={cn( | ||
| 'bg-container-neutral flex w-[320px] max-w-[calc(100vw-32px)] flex-col overflow-hidden', | ||
| 'rounded-lg border border-[var(--color-line,#e5e7eb)] shadow-[0px_0px_10px_8px_rgba(0,0,0,0.3)]', | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
디자인 토큰 우선 규칙과 cva 스타일 구조를 맞춰주세요.
현재 구간에 임의값 클래스(pt-[60px], w-[320px], max-w-[calc(...)], text-[18px], underline-offset-[3px], shadow-[...] 등)가 다수 포함되어 있어 토큰 우선 원칙과 어긋납니다. 이 컴포넌트 스타일은 cva 기반으로 정리하고, 필요한 토큰은 먼저 합의 후 추가하는 편이 좋습니다.
As per coding guidelines Use Tailwind CSS v4 with class-variance-authority (cva) for styling 및 Always use design token classes first; no hardcoded values. Ask user before adding new tokens.
Also applies to: 123-123, 133-133, 145-145, 157-157
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/home/NoticePopup.tsx` around lines 67 - 79, The NoticePopup
component contains many hardcoded Tailwind arbitrary values (e.g., pt-[60px],
w-[320px], max-w-[calc(100vw-32px)], text-[18px], underline-offset-[3px],
shadow-[...]) used directly inside the cn(...) className calls; refactor these
styles into a cva-based style object (e.g., create cva variants like
noticePopupContainer and popupCard) and replace the arbitrary classes with
design-token-first utility classes (or agreed token names) inside the cva
definitions; ensure all occurrences referenced in the diff (the cn(...) wrapper
and the popup card div) are updated, and do not add new tokens without
confirmation—ask the designer/product owner if a new token is required before
introducing one.
Comment on lines
+97
to
+106
| <Image | ||
| src={ | ||
| currentPage.useDefaultImage || !currentPage.imageUrl | ||
| ? DefaultPopupImg | ||
| : currentPage.imageUrl | ||
| } | ||
| alt={currentPage.title} | ||
| fill | ||
| className="object-cover" | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[1] next.config 파일 탐색"
fd -HI '^next\.config\.(js|mjs|cjs|ts)$'
echo "[2] 이미지 도메인 허용 설정 확인"
rg -n --iglob 'next.config.*' 'images|remotePatterns|domains|cdn\.sanity\.io'Repository: Team-Weeth/weeth-client
Length of output: 193
🏁 Script executed:
cat -n next.config.tsRepository: Team-Weeth/weeth-client
Length of output: 595
🏁 Script executed:
cat -n src/components/home/NoticePopup.tsxRepository: Team-Weeth/weeth-client
Length of output: 6872
🏁 Script executed:
find . -name 'popup.ts' -o -name 'popup.js' | head -5Repository: Team-Weeth/weeth-client
Length of output: 91
🏁 Script executed:
cat -n src/lib/apis/popup.tsRepository: Team-Weeth/weeth-client
Length of output: 1136
Sanity CDN 도메인을 next.config.ts의 remotePatterns에 추가하세요.
라인 97-106에서 Sanity 이미지 에셋(currentPage.imageUrl)을 next/image에 전달하고 있으나, next.config.ts의 remotePatterns에 cdn.sanity.io 도메인이 없어 런타임에서 이미지 렌더링이 실패합니다. remotePatterns 배열에 다음을 추가하세요:
{
protocol: 'https',
hostname: 'cdn.sanity.io',
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/home/NoticePopup.tsx` around lines 97 - 106, The Image in
NoticePopup.tsx is loading Sanity-hosted images via currentPage.imageUrl but
next.config.ts lacks the cdn.sanity.io remotePatterns entry; open next.config.ts
and add an entry to the images.remotePatterns array with protocol: 'https' and
hostname: 'cdn.sanity.io' so next/image can load currentPage.imageUrl at
runtime.
Comment on lines
+120
to
+136
| <button | ||
| onClick={handlePrev} | ||
| disabled={currentIndex === 0} | ||
| className="text-text-normal cursor-pointer border-none bg-transparent text-[18px] disabled:cursor-default disabled:opacity-20" | ||
| > | ||
| ‹ | ||
| </button> | ||
| <span className="typo-caption1 text-text-normal"> | ||
| {currentIndex + 1} / {popup.pages.length} | ||
| </span> | ||
| <button | ||
| onClick={handleNext} | ||
| disabled={currentIndex === popup.pages.length - 1} | ||
| className="text-text-normal cursor-pointer border-none bg-transparent text-[18px] disabled:cursor-default disabled:opacity-20" | ||
| > | ||
| › | ||
| </button> |
There was a problem hiding this comment.
페이지네이션 버튼에 접근성 레이블을 추가해 주세요.
Line 125/135의 기호(‹, ›)만으로는 스크린리더 의미 전달이 부족합니다. aria-label을 명시해 조작 의도를 전달하는 게 안전합니다.
수정 예시
<button
onClick={handlePrev}
disabled={currentIndex === 0}
+ aria-label="이전 팝업 페이지"
className="text-text-normal cursor-pointer border-none bg-transparent text-[18px] disabled:cursor-default disabled:opacity-20"
>
‹
</button>
@@
<button
onClick={handleNext}
disabled={currentIndex === popup.pages.length - 1}
+ aria-label="다음 팝업 페이지"
className="text-text-normal cursor-pointer border-none bg-transparent text-[18px] disabled:cursor-default disabled:opacity-20"
>
›
</button>📝 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.
Suggested change
| <button | |
| onClick={handlePrev} | |
| disabled={currentIndex === 0} | |
| className="text-text-normal cursor-pointer border-none bg-transparent text-[18px] disabled:cursor-default disabled:opacity-20" | |
| > | |
| ‹ | |
| </button> | |
| <span className="typo-caption1 text-text-normal"> | |
| {currentIndex + 1} / {popup.pages.length} | |
| </span> | |
| <button | |
| onClick={handleNext} | |
| disabled={currentIndex === popup.pages.length - 1} | |
| className="text-text-normal cursor-pointer border-none bg-transparent text-[18px] disabled:cursor-default disabled:opacity-20" | |
| > | |
| › | |
| </button> | |
| <button | |
| onClick={handlePrev} | |
| disabled={currentIndex === 0} | |
| aria-label="이전 팝업 페이지" | |
| className="text-text-normal cursor-pointer border-none bg-transparent text-[18px] disabled:cursor-default disabled:opacity-20" | |
| > | |
| ‹ | |
| </button> | |
| <span className="typo-caption1 text-text-normal"> | |
| {currentIndex + 1} / {popup.pages.length} | |
| </span> | |
| <button | |
| onClick={handleNext} | |
| disabled={currentIndex === popup.pages.length - 1} | |
| aria-label="다음 팝업 페이지" | |
| className="text-text-normal cursor-pointer border-none bg-transparent text-[18px] disabled:cursor-default disabled:opacity-20" | |
| > | |
| › | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/home/NoticePopup.tsx` around lines 120 - 136, The pagination
buttons rendering the symbols ‹ and › (inside the buttons handled by handlePrev
and handleNext, with state currentIndex and popup.pages) lack screen-reader
labels; add descriptive aria-label attributes to both buttons (for example
"Previous page" and "Next page" or include dynamic context like "Previous page,
page X of Y" / "Next page, page X of Y") so assistive tech can convey their
purpose and state; ensure the labels update using currentIndex and
popup.pages.length and keep existing disabled behavior intact.
Comment on lines
+27
to
+29
| export async function getActivePopup(): Promise<PopupDocument | null> { | ||
| const currentEnv = process.env.NEXT_PUBLIC_API_URL?.includes('dev') ? 'dev' : 'production'; | ||
| return sanityClient.fetch(QUERY, { currentEnv }, { useCdn: false }); |
There was a problem hiding this comment.
환경 판별 로직이 오탐될 수 있습니다.
Line 28은 URL 문자열 패턴에 의존해서 currentEnv를 결정하므로, 환경변수 누락/도메인 변경 시 production으로 잘못 고정될 수 있습니다. NEXT_PUBLIC_APP_ENV 같은 명시적 env 키(허용값: dev | production)로 분리하는 쪽이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/apis/popup.ts` around lines 27 - 29, The environment-detection in
getActivePopup relies on URL pattern matching and can misclassify; change it to
read an explicit env var (e.g., NEXT_PUBLIC_APP_ENV) and validate its value is
either 'dev' or 'production' before passing to sanityClient.fetch. Update the
currentEnv assignment in getActivePopup to use process.env.NEXT_PUBLIC_APP_ENV,
default to 'production' only if the value is missing or invalid, and consider
adding a small warning log when falling back so misconfiguration is visible.
woneeeee
approved these changes
May 11, 2026
| setIsVisible(true); | ||
| } | ||
| } catch { | ||
| // 팝업 로딩 실패 시 조용히 무시 |
Member
There was a problem hiding this comment.
요거 토스트도 안띄우고 그냥 무시되도록 해도 괜찮겠죵...?? 오히려 토스트가 뜨면 사용자 입장에선 더 이상할라나??
dalzzy
approved these changes
May 11, 2026
Comment on lines
+120
to
+135
| <button | ||
| onClick={handlePrev} | ||
| disabled={currentIndex === 0} | ||
| className="text-text-normal cursor-pointer border-none bg-transparent text-[18px] disabled:cursor-default disabled:opacity-20" | ||
| > | ||
| ‹ | ||
| </button> | ||
| <span className="typo-caption1 text-text-normal"> | ||
| {currentIndex + 1} / {popup.pages.length} | ||
| </span> | ||
| <button | ||
| onClick={handleNext} | ||
| disabled={currentIndex === popup.pages.length - 1} | ||
| className="text-text-normal cursor-pointer border-none bg-transparent text-[18px] disabled:cursor-default disabled:opacity-20" | ||
| > | ||
| › |
Member
There was a problem hiding this comment.
페이지네이션버튼 제가 온보딩모달 만들 때 만들어둔 컴포넌트가 있는데 이걸로 재사용하는 건 어떨까요,,,
한번 PaginationButton.tsx 봐주시고 조금 수정해서 쓰면 좋을 듯 합니다!!
@sanity/client(현재 브랜치) + @sentry/nextjs(develop) 동시 유지 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR 검증 결과⏭️ TypeScript: 건너뜀 |
PR 테스트 결과⏭️ Jest: 건너뜀 |
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
4 participants
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.
✅ PR 유형
어떤 변경 사항이 있었나요?
📌 관련 이슈번호
✅ Key Changes
@sanity/client패키지 설치 및 클라이언트 설정 (src/lib/sanity/client.ts)src/lib/apis/popup.ts) —weeth_v4대상 활성 팝업 조회NoticePopup컴포넌트 구현 (src/components/home/NoticePopup.tsx)/{clubId}/home)에서만 렌더링localStorage기반)useDefaultImage플래그로 기본 이미지(popup_default_img_1.png) 폴백HomePageEntry에NoticePopup마운트📸 스크린샷 or 실행영상
🎸 기타 사항 or 추가 코멘트
studio-weeth)에product필드 추가 및 재배포 완료weeth_v3/weeth_v4선택으로 프로덕트별 노출 구분 가능기존 sainty studio는 그대로 재사용하되, 위드 버전 필드를 구분해 어느 버전에 공지를 띄울지 설정할 수 있도록 했습니다!
Summary by CodeRabbit
릴리스 노트