Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Walkthrough이미지 로딩 UI/UX를 개선하기 위해 스켈레톤 로더 컴포넌트를 추가하고, 1초 지연 후 스켈레톤을 표시하며, 이미지 로딩 상태를 관리합니다. 또한 스탬프 클릭 핸들러에 로그인 가드를 추가하고 디버그 로그를 제거합니다. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI
participant Image
User->>UI: 페이지 방문
activate UI
UI->>UI: isLoading = true 설정
Note over UI: 0초: 아무것도 표시 안 함
par 1초 대기
UI->>UI: setTimeout 1000ms
end
Note over UI: 1초 경과<br/>showSkeleton = true
UI->>UI: 스켈레톤 표시
UI->>Image: 이미지 로드 시작
Image-->>UI: 이미지 로드 중...
Image-->>UI: onLoadingComplete 콜백
activate UI
UI->>UI: imageLoaded = true
UI->>UI: 스켈레톤 숨김 (opacity 전환)
UI->>UI: 이미지 표시
UI->>UI: 스탐프 버튼 활성화
deactivate UI
deactivate UI
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
|
🏷️ Labeler has automatically applied labels based on your PR title, branch name, or commit message. |
1 similar comment
|
🏷️ Labeler has automatically applied labels based on your PR title, branch name, or commit message. |
![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
🧹 Nitpick comments (3)
src/shared/components/skeleton/Skeleton.tsx (1)
1-8: 스켈레톤 컴포넌트 구현은 단순·명확해서 좋습니다.
cn으로 기본 스타일과className을 합치는 구조가 재사용하기에도 적절해 보입니다.- 타입은 인라인 객체 대신 아래처럼
SkeletonProps인터페이스로 분리하면 추후 확장 시 가독성이 조금 더 좋아질 수 있습니다 (선택 사항입니다).-export const Skeleton = ({ className }: { className?: string }) => { +interface SkeletonProps { + className?: string; +} + +export const Skeleton = ({ className }: SkeletonProps) => {
- 향후 필요하다면 스켈레톤이 완전히 장식용일 경우
aria-hidden='true'같은 접근성 속성을 추가하는 것도 고려해 볼 만합니다.src/pages/main/node/[placeId].tsx (2)
1-4: Pages Router에서는'use client'지시어가 불필요합니다.이 프로젝트는 Next.js Pages Router를 사용하고 있어서,
src/pages아래 컴포넌트는 기본적으로 클라이언트 컴포넌트로 동작합니다. 따라서 Line 1의'use client';지시어는 의미가 없고 스타일/린트 규칙에 따라 오히려 혼동을 줄 수 있어 제거하는 편이 좋습니다.-'use client'; - -import { useState, useEffect } from 'react'; +import { useState, useEffect } from 'react';Based on learnings
123-162: 이미지 스켈레톤 + 페이드인 처리, 그리고 스탬프 버튼 조건부 렌더링이 UI/UX 관점에서 잘 구성되어 있습니다.
imageLoaded상태로 이미지 스켈레톤을 제어하고,onLoadingComplete에서 페이드인(opacity+transition-opacity) 처리하는 흐름은 의도대로 동작할 것으로 보입니다.fill+relative w-full h-[256px]구조도 Next/Image 레이아웃 요구사항에 맞게 잘 짜여 있습니다.- 이미지를 다 받은 뒤에만 스탬프 버튼을 렌더링하도록
imageLoaded && (...)로 감싼 것도 UX 상 자연스럽습니다.다만, 아주 사소한 리팩토링 여지는 있습니다.
imageLoaded중복 조건
버튼 자체가imageLoaded &&조건으로 감싸져 있어서, 클래스 안의 삼항은 사실상 항상imageLoaded === true입니다.{imageLoaded && ( <button className={cn( 'absolute bottom-0 right-0', isCompleted && 'p-[2.5rem]', imageLoaded ? 'opacity-100' : 'opacity-0 h-0', )} ... >여기서는 단순히
'opacity-100'만 남겨도 동작은 동일합니다.
cn('absolute bottom-0 right-0',isCompleted && 'p-[2.5rem]',imageLoaded ? 'opacity-100' : 'opacity-0 h-0',)}
cn('absolute bottom-0 right-0 opacity-100',isCompleted && 'p-[2.5rem]',)}
스켈레톤 클래스 중복(선택 사항)
Skeleton컴포넌트 자체가 이미animate-pulse bg-gray-200 rounded-md를 가지고 있어서, 이미지 위 스켈레톤에 넘기는 클래스 중 일부는 살짝 중복이 될 수 있습니다.<Skeleton className='absolute inset-0 w-full h-full rounded-[16px] animate-pulse bg-gradient-to-br from-gray-200 to-gray-100' />실제 렌더링에는 문제가 없으니 그대로 두어도 되지만, 필요하다면
rounded-[16px]/bg-gradient-to-br쪽만 덮어쓰는 형태로 정리하면 클래스가 좀 더 깔끔해질 수 있습니다.전반적으로 이미지 로딩 상태와 연계된 스켈레톤/버튼 UI는 잘 구성된 것 같고, 위 내용은 전부 선택적인 정리 수준입니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pages/main/node/[placeId].tsx(7 hunks)src/shared/components/skeleton/Skeleton.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-31T17:34:04.352Z
Learnt from: jjangminii
Repo: geulDa/FE PR: 64
File: src/pages/map/result/components/ResultMap.tsx:3-3
Timestamp: 2025-10-31T17:34:04.352Z
Learning: The geulDa/FE project uses Next.js Pages Router (not App Router), so components should import useRouter from 'next/router' and should not use the 'use client' directive.
Applied to files:
src/pages/main/node/[placeId].tsx
🧬 Code graph analysis (2)
src/shared/components/skeleton/Skeleton.tsx (1)
src/shared/lib/utils.ts (1)
cn(71-73)
src/pages/main/node/[placeId].tsx (2)
src/shared/components/skeleton/Skeleton.tsx (1)
Skeleton(4-8)src/shared/lib/utils.ts (1)
cn(71-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (2)
src/pages/main/node/[placeId].tsx (2)
69-112: 스탬프 클릭 시 로그인 가드 추가는 좋지만, 하드코딩 좌표는 릴리즈 전에 확인이 필요합니다.
!isLoggedIn일 때 바로 로그인 팝업을 띄우고return하는 가드는 플로우 상 자연스럽고, 불필요한 API 호출도 막아줘서 좋습니다.다만
body에 들어가는 좌표가 아직 고정값으로 남아 있습니다:const body = { latitude: 37.48585193654532, longitude: 126.80355242431538, // 실제 위치 사용 시: // latitude: pos.coords.latitude, // longitude: pos.coords.longitude, };QA 용도라면 괜찮지만, 운영 배포 전에는 실제 위치를 사용할 것인지, 혹은 특정 기준 좌표를 쓸 것인지 확정하는 편이 좋습니다. 실제 위치를 쓸 계획이라면, 아래처럼 주석 부분을 되살리고 하드코딩 값을 제거하는 방향을 추천합니다.
const body = {latitude: 37.48585193654532,longitude: 126.80355242431538,// 실제 위치 사용 시:// latitude: pos.coords.latitude,// longitude: pos.coords.longitude,};
const body = {latitude: pos.coords.latitude,longitude: pos.coords.longitude,};
- 에러 처리(
onError, 위치 정보 실패 콜백)도 잘 연결돼 있어서, 추가로 로그 수준/로깅 방식만 팀 규칙에 맞는지만 확인해 보시면 될 것 같습니다.
174-190: 로그인/에러 팝업 플로우는 기존 UX를 해치지 않으면서 명확합니다.
- 로그인 팝업에서 바로
/auth로 보내는 흐름과, 위치 확인 실패 시 별도의 에러 팝업을 띄워주는 구조가 명확하고 단순합니다.- 주석으로 “팝업 영역”을 명시해 둔 것도 추후 유지보수 시 영역 파악에 도움이 될 것 같습니다.
추가로, 팝업이 열렸을 때 백그라운드 스크롤 잠금이나 포커스 트랩 같은 접근성 요구사항이 팀 수준에서 있다면, 공통
PopupSet컴포넌트 차원에서만 잘 처리되어 있는지 한 번만 확인해 보시면 좋겠습니다.
| // 이미지 로딩 상태 | ||
| const [imageLoaded, setImageLoaded] = useState(false); | ||
|
|
||
| // 스켈레톤 표시 여부 (로딩이 1초 이상일 때만 true) | ||
| const [showSkeleton, setShowSkeleton] = useState(false); | ||
|
|
There was a problem hiding this comment.
로딩 딜레이 + 에러 처리 로직에 버그가 있고, 타이머 타입도 안전하지 않습니다.
-
로딩 중 에러 메시지가 먼저 떠버리는 문제 (크리티컬)
현재 흐름:if (isLoading && showSkeleton) { /* 스켈레톤 */ } if (isError || !data) return <p>데이터를 불러오지 못했습니다 😢</p>;
초기 요청 시 보통
isLoading === true,data === undefined,showSkeleton === false입니다.
이 상태에서는 스켈레톤은 안 나오고,!data조건 때문에 곧바로 “데이터를 불러오지 못했습니다 😢” 에러 문구가 노출됩니다. 로딩이 정상적으로 진행되는 동안에도 잠깐 에러 화면이 깜빡일 수 있는 버그입니다.제안 수정: 로딩이 끝난 이후에만
!data를 에러로 취급하도록!isLoading을 조건에 추가하는 편이 안전합니다.
- if (isLoading && showSkeleton) {
- if (isLoading && showSkeleton) {
return (
{/* 스켈레톤 UI */}
);
}
- if (isError || !data)
- if (!isLoading && (isError || !data))
return데이터를 불러오지 못했습니다 😢
;
-
timer타입 및 초기화 문제 (TS 컴파일 에러 가능성)
useEffect안에서:let timer: NodeJS.Timeout; if (isLoading) { timer = setTimeout(...); } return () => clearTimeout(timer);
와 같이 작성되면, TypeScript에서는
isLoading === false인 코드 경로에서timer가 할당되지 않은 채clearTimeout(timer)가 호출될 수 있다고 판단해 “변수가 할당되기 전에 사용될 수 있다”는 컴파일 에러가 날 수 있습니다.브라우저 환경에서는
setTimeout리턴 타입이number이기도 해서,NodeJS.Timeout타입을 강제하는 것보다는ReturnType<typeof setTimeout>를 사용하는 편이 더 일반적입니다.제안 수정:
- useEffect(() => {
- let timer: NodeJS.Timeout;
- if (isLoading) {
-
timer = setTimeout(() => setShowSkeleton(true), 1000); - } else {
-
setShowSkeleton(false); - }
- return () => clearTimeout(timer);
- }, [isLoading]);
- useEffect(() => {
- let timer: ReturnType | null = null;
- if (isLoading) {
-
timer = setTimeout(() => setShowSkeleton(true), 1000); - } else {
-
setShowSkeleton(false); - }
- return () => {
-
if (timer) { -
clearTimeout(timer); -
} - };
- }, [isLoading]);
이 두 부분을 함께 고치면, 1초 지연 스켈레톤 UI 의도는 그대로 유지하면서도 초기 로딩 시 잘못된 에러 화면 노출과 TS 타입 이슈를 동시에 방지할 수 있습니다.
Also applies to: 41-49, 51-65
🤖 Prompt for AI Agents
In src/pages/main/node/[placeId].tsx around lines 27-32 (and also apply same
changes to 41-49, 51-65): the error display logic shows the "데이터를 불러오지 못했습니다"
message during initial loading because it treats !data as an error even when
isLoading is true, and the timer variable is typed/used unsafely causing
possible TS errors; change the error check to only treat missing data as an
error after loading finishes (e.g., require !isLoading && (isError || !data)),
and update the useEffect timer handling to use a safe type like let timer:
ReturnType<typeof setTimeout> | undefined (or initialize to undefined), only
assign timer when setting the timeout, and guard the cleanup with if (timer)
clearTimeout(timer) so clearTimeout is never called with an uninitialized value.
skyblue1232
left a comment
skyblue1232
left a comment
There was a problem hiding this comment.
로딩 관련해서 ux 적으로도 안정감을 줄 수 있을 것 같아서 좋은 것 같네요:) 수고하셨습니다!
2 participants
🔥 작업 내용
🤔 추후 작업 사항
🔗 이슈
PR Point (To Reviewer)
📸 피그마 스크린샷 or 기능 GIF
(작업 내역 스크린샷)
Summary by CodeRabbit
릴리스 노트
New Features
Bug Fixes