feat: 조회수 페이지 UI #97
feat: 조회수 페이지 UI #97
Conversation
WalkthroughThis update introduces animated visitor count displays to the homepage, utilizing the GSAP animation library and its React integration. A new custom hook is added to fetch and post visitor data from an API. The homepage now features two animated visitor count banners—one at the top and one at the bottom—each triggered by scroll events. The visitor count is incrementally animated when the corresponding section enters the viewport. A reusable Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HomePage
participant VisitorCount
participant useApiTotalView
participant API
User->>HomePage: Loads page
HomePage->>useApiTotalView: Fetch total visitor count (GET)
useApiTotalView->>API: GET /user-service/api/v1/visitor/total-view
API-->>useApiTotalView: Respond with total count
useApiTotalView-->>HomePage: Return visitor count
HomePage->>VisitorCount: Render with initial count
User->>HomePage: Scrolls to banner section
HomePage->>HomePage: Detect scroll trigger
HomePage->>useApiTotalView: POST /user-service/api/v1/visitor
useApiTotalView->>API: POST /user-service/api/v1/visitor
API-->>useApiTotalView: Acknowledge
HomePage->>HomePage: Animate visitor count to new value
HomePage->>VisitorCount: Update displayed count
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/app/home/index.tsx (4)
168-200: Consider extracting CSS for reusabilityThe CSS for the visitor count banner is quite complex with pseudo-elements and positioning. Since you have a similar banner at the bottom of the page, consider extracting this into a reusable styled component.
// Example of how it could be extracted const VisitorBadge = styled.div` position: relative; width: 31.5rem; height: 4rem; padding: 0px; background: #2f3b4b; color: #ffffff; border-radius: 1rem; display: flex; justify-content: center; align-items: center; &::after { content: ""; position: absolute; width: 22px; height: 20px; background: #2f3b4b; clip-path: path("M0,0 L22,0 L13,16 Q11,20 9,16 L0,0"); display: block; z-index: 1; margin-left: -2.6rem; bottom: -1rem; left: 13%; } ${BREAK_POINTS.TABLET} { // tablet styles } ${BREAK_POINTS.MOBILE} { // mobile styles } `;
641-722: Consider consolidating responsive image stylesAll five images in the bottom banner share similar responsive patterns but have duplicate code for tablet and mobile breakpoints. Consider creating a reusable styled component for these images.
const BannerImage = styled.img` position: absolute; ${BREAK_POINTS.TABLET} { width: ${props => props.tabletWidth || '6rem'}; } ${BREAK_POINTS.MOBILE} { width: ${props => props.mobileWidth || '3rem'}; } `; // Then use it like: // <BannerImage // src={BlackEtherImg} // tabletWidth="7rem" // mobileWidth="3rem" // css={css` // bottom: 2px; // left: 16%; // `} // />This would reduce repetition and make the styles more maintainable.
596-640: Improve responsive text scaling for the bannerWhile you've implemented responsive font sizes, the jump from 2.8rem to 1.2rem between tablet and mobile is quite dramatic. Consider using a more gradual scale or CSS clamp for smoother transitions.
- font-size: 2.8rem; + font-size: clamp(1.2rem, 5vw, 2.8rem);This will provide a smoother transition as the viewport size changes.
581-725: Align style colors with design system variablesYou're using hardcoded color values like
#0044a1and#79deffin the bottom banner section, but the project appears to use design system variables likeDESIGN_SYSTEM_COLOR.BRAND_BLUEelsewhere.Consider using the design system color variables for consistency:
- background-color: #0044a1; + background-color: ${DESIGN_SYSTEM_COLOR.BRAND_BLUE}; - color: #79deff; + color: ${DESIGN_SYSTEM_COLOR.BRAND_LIGHT_BLUE}; // assuming this existsThis ensures color consistency throughout the application and makes theme changes easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
src/assets/black_ether.svgis excluded by!**/*.svgsrc/assets/green_bit.svgis excluded by!**/*.svgsrc/assets/orange_bit.svgis excluded by!**/*.svgsrc/assets/skyblue_L.svgis excluded by!**/*.svgsrc/assets/yellow_bit.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
src/app/home/index.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
src/app/home/index.tsx (2)
23-27: LGTM: New image imports added correctlyThese new SVG imports are properly structured following the project's existing import pattern for assets.
126-126: Minor formatting fixAdded missing comma to the object literal in the
openSubscriptionModalfunction.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
vite.config.ts (1)
31-39: Reassess “secure: false” in the proxy configuration.Disabling SSL certificate checks may pose security risks. If the target endpoint supports valid HTTPS, consider using
secure: true.src/app/home/index.tsx (3)
68-92: Optimize frequent re-renders from animation loop.Repeatedly calling
setCurrentNumin quick succession may cause performance overhead. Minimizing state updates (e.g., batching updates or leveraging a ref for the counter) can help.
105-129: Deactivate debug markers for production.ScrollTrigger markers are useful in development but should typically be disabled for live environments.
- markers: true, + // markers: true, // Uncomment if needed for debugging
666-811: DRY up repeated banner logic.The top and bottom banners share similar logic for showing the visitor count. Consider extracting into a reusable component or utility function to adhere to DRY principles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.json(1 hunks)src/app/home/index.tsx(8 hunks)src/hooks/api/visitor/useApiTotalView.ts(1 hunks)vite.config.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
src/app/home/index.tsx
[error] 60-60: 'refetch' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: build
src/app/home/index.tsx
[failure] 60-60:
'refetch' is declared but its value is never read.
🪛 GitHub Actions: Build Test
src/app/home/index.tsx
[error] 60-60: 'refetch' is declared but its value is never read.
🔇 Additional comments (4)
package.json (1)
21-21: Approved addition of gsap dependency.No concerns here. The library is correctly pinned for future reference, and it aligns with the new scroll-based animations.
src/hooks/api/visitor/useApiTotalView.ts (1)
10-15: Verify long staleTime and gcTime.Two hours is quite long for caching visitor counts. Confirm whether the frequent changes in visitor data still require such a lengthy cache duration.
src/app/home/index.tsx (2)
3-4: Smooth integration of GSAP and ScrollTrigger.Registering the plugin at line 47 is good practice for scroll-based animations. No concerns with these imports.
94-98: Confirm hardcoded test value.Line 96 uses a fixed value of 10,404,174 for animation. Verify if the intended approach is to use the real API data (
totalView.data).
| const retrieveTotalView = async () => { | ||
| const response = await api.get("/user-service/api/v1/visitor/total-view"); | ||
| return response.data; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for non-200 responses.
Currently, the hook assumes a successful GET request. Handling error states (e.g., 4xx or 5xx) will improve robustness.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/app/home/index.tsx (3)
60-60: Refetch usage within the app.
refetchis later called inside the animation function. Frequent scrolling might trigger multiple API calls.Consider decoupling the animation from the GET request if performance or rate limits become a concern.
97-97: Send a POST request on mount.Consider adding error handling or a try/catch for
sendTotalView()in case the request fails.
98-104: Remove console log in production.Leaving
console.log("totalView", totalView.data)may clutter the console in production builds.- console.log("totalView", totalView.data);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/home/index.tsx(8 hunks)src/hooks/api/visitor/useApiTotalView.ts(1 hunks)
🔇 Additional comments (14)
src/hooks/api/visitor/useApiTotalView.ts (3)
1-2: Imports look good.No concerns with these import statements.
5-8: Consider adding error handling for non-200 responses.Currently, the hook assumes a successful GET request. Handling potential 4xx or 5xx errors would improve robustness.
10-13: Likewise, consider handling POST request errors.Similar to the GET request, it's good practice to handle unsuccessful POST outcomes or network issues.
src/app/home/index.tsx (11)
3-4: GSAP import for animations.These imports are necessary for advanced scroll-triggered animations. All good here.
25-29: SVG imports for additional visual elements.No issues—these assets appear to be used in the new UI sections.
31-31: React hooks import.The added hooks (
useEffect,useRef,useState) are standard for managing state and side effects.
34-34: Subscription modal import.Importing
SubscriptionModalContentis straightforward; looks fine.
37-37: New hook usage.Bringing in
useApiTotalViewis consistent with the API design. Great.
47-47: Register GSAP plugin.
gsap.registerPlugin(ScrollTrigger);is correctly placed for enabling scroll-trigger functionality.
61-66: Refs and state initialization.Using separate refs for top/bottom sections and local state for the counters is coherent and maintainable.
106-141: Multiple triggers for slotAnimation.Scrolling beyond top/bottom sections triggers
slotAnimationonEnter, onEnterBack, and again directly at line 135. Verify if this repetition is intentional.
207-210: JSDoc commentary for subscription.Documenting the function behavior is helpful; this is clear and straightforward.
241-241: Reference assignment to topSectionRef.Ref usage here is consistent with ScrollTrigger logic. No issues.
671-817: Bottom banner design.The additional styling and positioned images enhance the UI, and referencing the same animation for consistency appears logical.
src/app/home/index.tsx
Outdated
| // const slotAnimation = () => { // 애니메이션 테스트용 | ||
| const slotAnimation = async () => { // API 호출 테스트용 | ||
| if (!topNumberRef.current || !bottomNumberRef.current) return; | ||
|
|
||
| await refetch(); // API 호출 테스트용 | ||
|
|
||
| setCurrentNum(0); | ||
|
|
||
| let current = 0; | ||
| const step = Math.ceil(animationTotal / 9000000000); | ||
|
|
||
| const animate = () => { | ||
| current += step; | ||
| if (current >= animationTotal) { | ||
| setCurrentNum(animationTotal); | ||
| setCurrentNum(animationTotal.toLocaleString()); | ||
| return; | ||
| } | ||
|
|
||
| setCurrentNum(current.toLocaleString()); | ||
| requestAnimationFrame(animate); | ||
| }; | ||
|
|
||
| requestAnimationFrame(animate); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
slotAnimation function may trigger unintended repeated API calls.
At line 72, await refetch() runs whenever slotAnimation is called, including on multiple scroll triggers. Ensure this aligns with usage limits and performance requirements.
joeunSong
force-pushed
the
feature/user-count-banner
branch
from
032708c to
3d5cdf6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/app/home/index.tsx (1)
61-66:⚠️ Potential issueType confusion risk in
currentNum
BecausecurrentNumis set to both numeric and string values, subsequent calls tocurrentNum.toLocaleString()may throw errors whencurrentNumis already a string.Consider storing only numbers in state and doing the
.toLocaleString()conversion in the JSX:- const [currentNum, setCurrentNum] = useState<number|string>(0); ... - setCurrentNum(animationTotal.toLocaleString()); + setCurrentNum(animationTotal); - {currentNum.toLocaleString()} + {typeof currentNum === "number" ? currentNum.toLocaleString() : currentNum}
🧹 Nitpick comments (3)
src/app/home/index.tsx (3)
94-104: Consider removing console logs and verifying page-visit increments
console.log("totalView", totalView.data);is useful for debugging but might be best removed from production code.- Calling
sendTotalView()on mount may increment your visitor count each time a user revisits.Please confirm if the increment-on-mount behavior is intentional and remove the console log if not needed.
106-141: Multiple ScrollTrigger instances re-invokingslotAnimation
onEnter&onEnterBackboth fireslotAnimation, which callsrefetch()each time. Rapid scroll up/down can spam your API.If it’s required only once, limit triggers or store a state flag to skip repeated calls.
240-303: Potential duplication with the bottom banner
This top banner for displaying “조회수” closely resembles the bottom banner logic at lines 672+. Extracting both into a shared component would reduce repetition.- // Repeated markup in two places + // Factor out to a <ViewCountBanner ref={...} ... /> component accepting props
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/home/index.tsx(8 hunks)src/hooks/api/visitor/useApiTotalView.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/api/visitor/useApiTotalView.ts
🔇 Additional comments (7)
src/app/home/index.tsx (7)
3-4: Successfully integrated GSAP imports
These imports for GSAP and ScrollTrigger look straightforward and functional.
25-29: Additional SVG imports are fine
No issues observed with these newly imported assets. They appear ready for usage in the new UI banners.
31-31: New hook & component imports
The new imports (e.g.,useApiTotalViewandSubscriptionModalContent) are aligned with their usage in the file.Also applies to: 34-34, 37-37
47-47: Registering the ScrollTrigger plugin
Callinggsap.registerPlugin(ScrollTrigger)is appropriate for enabling scroll-based animations.
60-60: Potential heavy usage ofrefetch
You are destructuringrefetchfromuseApiTotalView. Since it’s called insideslotAnimation, be mindful of network usage if the scroll triggers call it frequently.Could you confirm that your API service usage plan is sufficient for repeated, scroll-driven calls?
207-210: Sufficiently documented modal logic
The inline doc comment foropenSubscriptionModalis clear and aligns with usage.
672-817: Repeated banner code & string/number mismatch
- This bottom banner largely duplicates the top banner’s DOM and logic, suggesting a shared component might help maintain consistency.
- The same
currentNum.toLocaleString()type risk applies here (see prior comment).
src/app/home/index.tsx
Outdated
| // const slotAnimation = () => { // 애니메이션 테스트용 | ||
| const slotAnimation = async () => { // API 호출 테스트용 | ||
| if (!topNumberRef.current || !bottomNumberRef.current) return; | ||
|
|
||
| await refetch(); // API 호출 테스트용 | ||
|
|
||
| setCurrentNum(0); | ||
|
|
||
| let current = 0; | ||
| const step = Math.ceil(animationTotal / 180); | ||
|
|
||
| const animate = () => { | ||
| current += step; | ||
| if (current >= animationTotal) { | ||
| setCurrentNum(animationTotal); | ||
| setCurrentNum(animationTotal.toLocaleString()); | ||
| return; | ||
| } | ||
|
|
||
| setCurrentNum(current.toLocaleString()); | ||
| requestAnimationFrame(animate); | ||
| }; | ||
|
|
||
| requestAnimationFrame(animate); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Repeated slot animation calls and API refetch
slotAnimation() triggers await refetch() every time. This can cause performance overhead and potential flicker if the user scrolls repeatedly.
Either remove the repeated refetch() once data is already loaded or restrict it with a guard to prevent excessive API requests.
src/app/home/index.tsx
Outdated
| useEffect(() => { | ||
| if (!totalView) return; | ||
| setAnimationTotal(totalView.data); // API 호출 테스트용 | ||
| console.log("totalView", totalView.data); // API 호출 테스트용 |
There was a problem hiding this comment.
console.log 부분은 확인 후 제거해주셔도 좋을 것 같아요!
There was a problem hiding this comment.
헙.. 마지막에 테스트하다가 제거하는거 놓쳤나봐요
제거하겠습니다!
| }; | ||
| }; | ||
|
|
||
| gsap.registerPlugin(ScrollTrigger); |
There was a problem hiding this comment.
gsap 이라는 라이브러리를 처음봐서 검색을 조금 해보았는데요!
https://www.npmjs.com/package/@gsap/react
이런 라이브러리가 있는데, gsap 그대로 사용을 하신 이유가 있을까요?
There was a problem hiding this comment.
gsap 정보가 많아서 그걸로 개발했었는데, 댓글 보고 @gsap/react를 알게 됐습니다!
찾아보고 써보니까 React에서 GSAP을 좀 더 깔끔하게 쓸 수 있더라구요
@gsap/react로 수정해서 올리겠습니다~!
vite.config.ts
Outdated
| server: { | ||
| proxy: { | ||
| '/user-service': { | ||
| target: 'https://api.flowbit.co.kr', | ||
| changeOrigin: true, | ||
| secure: false, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
혹시 프록시 설정은 왜 하신지 여쭤봐도 될까요?!
There was a problem hiding this comment.
env 파일이 삭제된줄 모르고 api 연결이 안 되서 강제로 했었는데 삭제된걸 지금 확인했어요ㅠ
이 부분 삭제하겠습니다!
src/app/home/index.tsx
Outdated
| > | ||
| {/* LEFT SIDE */} | ||
| <div> | ||
| <div |
There was a problem hiding this comment.
해당 부분은 컴포넌트로 빼서 임포트 해보는건 어떨까요~?
그리고 관련 함수들도요!
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/app/home/index.tsx (2)
65-88: 🛠️ Refactor suggestionOptimize animation function to avoid redundant API calls.
The
slotAnimationfunction currently fetches new data on every call, which can lead to performance issues and unnecessary API calls when scrolling triggers animations multiple times.const slotAnimation = async () => { if (!topNumberRef.current || !bottomNumberRef.current) return; - await refetch(); - setCurrentNum(0); let current = 0; const step = Math.ceil(animationTotal / 20000); const animate = () => { current += step; if (current >= animationTotal) { setCurrentNum(animationTotal); setCurrentNum(animationTotal.toLocaleString()); return; } setCurrentNum(current.toLocaleString()); requestAnimationFrame(animate); }; requestAnimationFrame(animate); };
57-57:⚠️ Potential issueOptimize API refetch strategy.
The
refetchfunction is not being used optimally, as it's called on every animation trigger. This might lead to excessive API calls.-const { data: totalView, refetch, sendTotalView } = useApiTotalView(); +const { data: totalView, sendTotalView } = useApiTotalView();Then modify the slotAnimation function to remove the unnecessary refetch.
🧹 Nitpick comments (3)
src/components/app/home/VisitorCount.tsx (2)
9-14: Props interface looks good, but consider optional sectionRef naming.The interface properties are well-defined with appropriate types. Since
sectionRefis only used whenisBottomis true, consider naming it more specifically (e.g.,bottomSectionRef) or adding a comment explaining its purpose.
157-205: Top banner (speech bubble) styling is incomplete for responsive layouts.The top banner has empty media query blocks for tablet and mobile breakpoints. Consider adding proper responsive styling to ensure the speech bubble displays correctly on smaller screens.
${BREAK_POINTS.TABLET} { + width: 32rem; + height: 3.6rem; + font-size: 0.9rem; } ${BREAK_POINTS.MOBILE} { + width: 26rem; + height: 3.2rem; + font-size: 0.8rem; }src/app/home/index.tsx (1)
90-117: Consider debouncing scroll triggers to prevent excessive animations.Multiple scroll triggers for the same animation function can cause performance issues if triggered too frequently. Consider implementing a debounce mechanism to limit how often animations can be triggered.
useGSAP(() => { if (!topSectionRef.current || !bottomSectionRef.current) { return; } + // Track if animation is currently running + let isAnimating = false; + + // Create debounced animation function + const debouncedAnimation = () => { + if (isAnimating) return; + + isAnimating = true; + slotAnimation().finally(() => { + // Allow new animations after a short delay + setTimeout(() => { + isAnimating = false; + }, 1000); + }); + }; const ctx = gsap.context(() => { ScrollTrigger.create({ trigger: topSectionRef.current, start: "top 5%", end: "top 5%", toggleActions: "restart complete restart reset", - onEnter: () => slotAnimation(), - onEnterBack: () => slotAnimation(), + onEnter: debouncedAnimation, + onEnterBack: debouncedAnimation, }); ScrollTrigger.create({ trigger: bottomSectionRef.current, start: "bottom 80%", toggleActions: "restart complete restart reset", - onEnter: () => slotAnimation(), - onEnterBack: () => slotAnimation(), + onEnter: debouncedAnimation, + onEnterBack: debouncedAnimation, }); - slotAnimation(); + // Initial animation without need for debouncing + slotAnimation(); }); return () => ctx.revert(); }, { dependencies: [animationTotal] });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
package.json(1 hunks)src/app/home/index.tsx(8 hunks)src/components/app/home/VisitorCount.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/components/app/home/VisitorCount.tsx (1)
src/style/variable.ts (1)
BREAK_POINTS(162-165)
src/app/home/index.tsx (4)
src/hooks/api/visitor/useApiTotalView.ts (1)
useApiTotalView(4-24)src/components/common/modal/SubscriptionModalContent.tsx (1)
SubscriptionModalContent(13-296)src/components/app/home/VisitorCount.tsx (1)
VisitorCount(16-206)src/style/variable.ts (1)
BREAK_POINTS(162-165)
🔇 Additional comments (5)
package.json (1)
17-17: GSAP animation libraries successfully added.The addition of GSAP animation libraries (
@gsap/reactandgsap) is appropriate for implementing the visitor count animations as described in the PR objectives. These libraries will enable the smooth counting animations triggered by scroll events.Also applies to: 22-22
src/components/app/home/VisitorCount.tsx (2)
16-21: Default value for isBottom prop is appropriate.Setting
isBottom = falseas the default is a good practice, allowing the component to be used without explicitly setting this prop when the top variant is needed.
22-154: Bottom visitor count banner implementation is well-structured.The conditional rendering and styling for the bottom banner looks good. The responsive styling adjustments for tablet and mobile breakpoints are also well implemented.
src/app/home/index.tsx (2)
119-126: Good implementation of API call and data handling.The effects for making the initial API call and updating the animation total are well-structured and correctly separated.
242-245: Top visitor count banner implementation is clean.The integration of the VisitorCount component for the top banner is well-implemented with appropriate props.
| {currentNum.toLocaleString()} | ||
| </span> |
There was a problem hiding this comment.
Handle string values in toLocaleString call.
The currentNum prop can be a string or number, but toLocaleString() is only available on numbers. This could cause runtime errors if a string is passed.
-{currentNum.toLocaleString()}
+{typeof currentNum === "number" ? currentNum.toLocaleString() : currentNum}Also applies to: 200-201
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/app/home/index.tsx (3)
57-57: Consider renaming variables for clarity.The use of
data: totalViewmight be confusing as it suggests it's the total view data rather than the API response object. Consider renaming for clarity:-const { data: totalView, refetch, sendTotalView } = useApiTotalView(); +const { data: visitorData, refetch, sendTotalView } = useApiTotalView();And then update references accordingly.
90-117: Consider simplifying animation setup with GSAP's native capabilities.Instead of manually implementing animation with
requestAnimationFrame, you could use GSAP's built-in animation features for cleaner code:useGSAP(() => { if (!topSectionRef.current || !bottomSectionRef.current) { return; } const ctx = gsap.context(() => { + // Define the number animation once + const animateNumber = () => { + gsap.to({value: 0}, { + value: animationTotal, + duration: 2, + ease: "power1.inOut", + onUpdate: function() { + setCurrentNum(Math.floor(this.targets()[0].value).toLocaleString()); + } + }); + } + ScrollTrigger.create({ trigger: topSectionRef.current, start: "top 5%", end: "top 5%", toggleActions: "restart complete restart reset", - onEnter: () => slotAnimation(), - onEnterBack: () => slotAnimation(), + onEnter: animateNumber, + onEnterBack: animateNumber, }); ScrollTrigger.create({ trigger: bottomSectionRef.current, start: "bottom 80%", toggleActions: "restart complete restart reset", - onEnter: () => slotAnimation(), - onEnterBack: () => slotAnimation(), + onEnter: animateNumber, + onEnterBack: animateNumber, }); - slotAnimation(); + animateNumber(); }); return () => ctx.revert(); }, { dependencies: [animationTotal] });This approach uses GSAP's animation engine directly, which is optimized for performance and simplifies your code.
123-126: Type safety for animationTotal should be improved.Currently, if
totalViewis undefined or doesn't have adataproperty,setAnimationTotalwill be called withundefined. This could cause runtime errors.useEffect(() => { if (!totalView) return; - setAnimationTotal(totalView.data); + setAnimationTotal(totalView.data || 0); }, [totalView]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/home/index.tsx(7 hunks)src/components/app/home/VisitorCount.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/app/home/VisitorCount.tsx
🔇 Additional comments (4)
src/app/home/index.tsx (4)
613-621: Good implementation of the bottom visitor count.The bottom visitor count banner implementation follows the component-based approach and efficiently reuses the
VisitorCountcomponent with theisBottomprop to differentiate its styling.
241-245: Good implementation of reusable component for visitor count.The top visitor count implementation properly uses the
VisitorCountcomponent, passing appropriate props for the animation. This follows good React patterns by extracting reusable UI elements into separate components.
3-5: Great choice using @gsap/react integration.Using the official React integration for GSAP is excellent for optimizing performance and ensuring proper cleanup when components unmount. The
useGSAPhook provides a clean way to use GSAP in a React component.
33-34: Good separation of concerns with API hook and dedicated component.The implementation properly separates data fetching (via
useApiTotalView) from presentation (viaVisitorCountcomponent), which follows good software design principles.
| useEffect(() => { | ||
| sendTotalView(); | ||
| }, []); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
API call should have appropriate dependency.
The sendTotalView function is called in a useEffect with an empty dependency array, which means it will only run once when the component mounts. However, this function lacks an explicit dependency.
If sendTotalView depends on any state or props, they should be included in the dependency array. If it's truly a side-effect that should only run once, add a comment explaining this intention.
🏁 Script executed:
#!/bin/bash
# Check how sendTotalView is implemented in the hook
rg -A 5 "useApiTotalView" --type tsLength of output: 1334
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' src/hooks/api/visitor/useApiTotalView.tsLength of output: 678
Stabilize sendTotalView in useEffect dependencies
The sendTotalView function returned by useApiTotalView is re‑created on every render, so React’s exhaustive‑deps rule will flag its omission from the dependency array. You have two options:
• Memoize sendTotalView inside the hook and include it in deps:
- In
src/hooks/api/visitor/useApiTotalView.tswrap it inuseCallback:const sendTotalView = useCallback(async () => { const response = await api.post("/user-service/api/v1/visitor"); return response.data; }, []);
- In your component’s
useEffect, add it to the array:useEffect(() => { sendTotalView(); }, [sendTotalView]);
• If you truly intend a “run once on mount” side‑effect, explicitly disable the lint rule with a comment:
useEffect(() => {
sendTotalView();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);Please pick one approach to satisfy exhaustive‑deps and document your intent.
Locations to update:
- src/hooks/api/visitor/useApiTotalView.ts (
sendTotalView→ wrap inuseCallback) - src/app/home/index.tsx (adjust
useEffectdeps or add ESLint disable)
|
@joeunSong 님~ 피알 코멘트 반영 완료되셨으면 PR 재요청 부탁드립니다! |
| }), | ||
| sendTotalView, // POST 요청 함수 반환 | ||
| }; | ||
| }; |
There was a problem hiding this comment.
혹시 useQuery와 sendTotalView함수 총 2개를 반환해주신 이유가 있을까요?
하나는 get이고 하나는 post여서 이 부분 궁금했어요!
There was a problem hiding this comment.
방문자 수 기능은 조회와 기록 기능이 필요하기 때문에 페이지 방문 시 방문 기록을 남기고(post), 총 방문자 수를 조회하기 위해(get) post와 get을 함께 사용했습니다!
혹시 이 답변이 맞을까요?? 위에서 말씀해주신 충돌도 처리했습니다!
| width: 100%; | ||
| height: 16.4rem; | ||
| border-radius: 1.6rem; | ||
| background-color: #0044a1; |
There was a problem hiding this comment.
이 속성은 사전에 반영해놓은 CSS 커스텀 프로퍼티를 사용해주세요!
| return ( | ||
| <div | ||
| css={css` | ||
| padding: 12rem 0; |
There was a problem hiding this comment.
새로운 컴포넌트의 경우 인라인 CSS props 형태가 아니라 변수 형태로 별도의 영역 구분을 해보면 어떨까요?! 물론 나머지 컴포넌트들도 리팩토링을 진행해야하지만 추가되는 것부터요 ㅎㅎ..
| <p> | ||
| 지금까지 플로우빗의 예측가격이{" "} | ||
| <span | ||
| ref={numberRef} | ||
| css={css` | ||
| color: #33c2ff; | ||
| `} | ||
| > | ||
| {currentNum.toLocaleString()} | ||
| </span> | ||
| 번 조회됐어요 | ||
| </p> |
There was a problem hiding this comment.
이 친구는 컴포넌트 내부에서 <Message> 라는 컴포넌트로 띄어놓을 수도 있을 것 같아요! 어떻게 생각하시나요!
There was a problem hiding this comment.
좋아요!! 위에 내용까지 다 반영했습니다!
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/app/home/index.tsx (1)
121-123: 🛠️ Refactor suggestionStabilize
sendTotalViewinuseEffectdependencies.The
sendTotalViewfunction is likely recreated on every render, which means it should be included in the dependency array or the dependency should be properly handled.You have two options to fix this:
Option 1: Memoize
sendTotalViewin the hook and include it in deps (preferred approach):useEffect(() => { sendTotalView(); - }, []); + }, [sendTotalView]);Option 2: If you truly intend this as a "run once on mount" effect:
useEffect(() => { sendTotalView(); + // eslint-disable-next-line react-hooks/exhaustive-deps }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/home/index.tsx(6 hunks)
🧰 Additional context used
🪛 GitHub Check: build
src/app/home/index.tsx
[failure] 30-30:
Duplicate identifier 'SubscriptionModalContent'.
🪛 GitHub Actions: Build Test
src/app/home/index.tsx
[error] 30-30: TypeScript error TS2300: Duplicate identifier 'SubscriptionModalContent'.
🔇 Additional comments (5)
src/app/home/index.tsx (5)
59-65: LGTM! API integration for visitor counts works well.The setup of the visitor count with API and ref handling is well-structured. Destructuring totalView, refetch, and sendTotalView from useApiTotalView makes the code more readable.
92-119: LGTM! Well-structured GSAP integration with ScrollTrigger.Good use of
useGSAPhook for cleanup and proper GSAP context. The ScrollTrigger configurations for both top and bottom sections are set up correctly with appropriate trigger points and callback actions.
125-128: LGTM! Proper handling oftotalViewdata update.The effect correctly updates the
animationTotalstate when new data arrives from the API, with a proper null check.
243-247: LGTM! Good UI component composition for the top VisitorCount.The
VisitorCountcomponent is well-implemented with appropriate props for the top banner.
617-625: LGTM! Clean implementation of bottom visitor count.Good reuse of the
VisitorCountcomponent with theisBottomprop to differentiate styling. This follows the DRY principle by reusing the same component with different configurations.
| if (!topNumberRef.current || !bottomNumberRef.current) return; | ||
|
|
||
| await refetch(); | ||
|
|
||
| setCurrentNum(0); | ||
|
|
||
| let current = 0; | ||
| const step = Math.ceil(animationTotal / 20000); | ||
|
|
||
| const animate = () => { | ||
| current += step; | ||
| if (current >= animationTotal) { | ||
| setCurrentNum(animationTotal); | ||
| setCurrentNum(animationTotal.toLocaleString()); | ||
| return; | ||
| } | ||
|
|
||
| setCurrentNum(current.toLocaleString()); | ||
| requestAnimationFrame(animate); | ||
| }; | ||
|
|
||
| requestAnimationFrame(animate); | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize animation and API calling strategy.
The current implementation has several issues:
await refetch()is called on every animation run, which can lead to excessive API calls- There are two consecutive
setCurrentNumcalls at lines 80-81, with the second overriding the first - No mechanism to prevent multiple animations from running simultaneously when scrolling rapidly
const slotAnimation = async () => {
+ // Prevent multiple animations from running simultaneously
+ if (isAnimating.current) return;
if (!topNumberRef.current || !bottomNumberRef.current) return;
+ isAnimating.current = true;
- await refetch();
+ // Only refetch if needed (could add a time-based check)
+ // await refetch();
setCurrentNum(0);
let current = 0;
const step = Math.ceil(animationTotal / 20000);
const animate = () => {
current += step;
if (current >= animationTotal) {
- setCurrentNum(animationTotal);
setCurrentNum(animationTotal.toLocaleString());
+ isAnimating.current = false;
return;
}
setCurrentNum(current.toLocaleString());
requestAnimationFrame(animate);
};
requestAnimationFrame(animate);
};To implement this, add this ref at the top of your component:
const isAnimating = useRef(false);…eature/user-count-banner # Conflicts: # src/app/home/index.tsx
joeunSong
force-pushed
the
feature/user-count-banner
branch
from
826b385 to
2c47d36
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/app/home/VisitorCount.tsx (1)
188-189:⚠️ Potential issueHandle string values in toLocaleString call.
The
currentNumprop can be a string or number, buttoLocaleString()is only available on numbers. This could cause runtime errors if a string is passed.-{currentNum.toLocaleString()} +{typeof currentNum === "number" ? currentNum.toLocaleString() : currentNum}
🧹 Nitpick comments (4)
src/components/app/home/VisitorCount.tsx (4)
22-57: Consider using variables for styling organizationThe styling is currently defined with inline CSS props. For better maintainability, consider organizing these styles as variables in a separate section within the file, as suggested in previous feedback.
-const containerStyle = css` +// Container styles +const styles = { + container: css` padding: 12rem 0; ${BREAK_POINTS.TABLET} { padding: 8rem 0; } ${BREAK_POINTS.MOBILE} { padding: 0; } -`; + `, -const visitorCountStyle = css` + visitorCount: css` ${DESIGN_SYSTEM_TEXT.T2} position: relative; width: 100%; height: 16.4rem; border-radius: 1.6rem; background-color: ${DESIGN_SYSTEM_COLOR.BRAND_BLUE}; color: ${DESIGN_SYSTEM_COLOR.GRAY_50}; display: flex; justify-content: center; align-items: center; text-align: center; ${BREAK_POINTS.TABLET} { ${DESIGN_SYSTEM_TEXT.T4} height: 14rem; } ${BREAK_POINTS.MOBILE} { ${DESIGN_SYSTEM_TEXT.CAPTION} height: 7.2rem; border-radius: 1rem; } -`; + `, // ... other styles };
180-199: Extract text constants for easier maintenanceConsider extracting the text strings to constants to facilitate maintenance and future localization efforts.
+const TEXT = { + VISITOR_MESSAGE: "지금까지 플로우빗의 예측가격이", + VISITOR_COUNT_SUFFIX: "번 조회됐어요", + CALL_TO_ACTION: "지금 바로 시작하세요!" +}; const Message = ({ currentNum, numberRef, isBottom = false }: MessageProps) => { return ( <p> - 지금까지 플로우빗의 예측가격이{" "} + {TEXT.VISITOR_MESSAGE}{" "} <span ref={numberRef} css={isBottom ? countNumberStyle : topCountNumberStyle} > {typeof currentNum === "number" ? currentNum.toLocaleString() : currentNum} </span> - 번 조회됐어요 + {TEXT.VISITOR_COUNT_SUFFIX} {isBottom && ( <> <br /> - 지금 바로 시작하세요! + {TEXT.CALL_TO_ACTION} </> )} </p> ); };
201-255: Consider using composition over conditional renderingThe component has two distinct rendering paths based on
isBottom. For better maintainability, consider splitting this into two separate components that share common logic:export const VisitorCount = (props: VisitorCountProps) => { return props.isBottom ? <BottomVisitorCount {...props} /> : <TopVisitorCount {...props} />; }; const BottomVisitorCount = ({ currentNum, numberRef, sectionRef }: VisitorCountProps) => ( <div css={styles.container}> <div ref={sectionRef} css={styles.visitorCount}> <Message currentNum={currentNum} numberRef={numberRef} isBottom={true} /> <img src={BlackEtherImg} css={styles.blackEther} /> <img src={SkyblueLImg} css={styles.skyblueL} /> <img src={GreenBitImg} css={styles.greenBit} /> <img src={YellowBitImg} css={styles.yellowBit} /> <img src={OrangeBitImg} css={styles.orangeBit} /> </div> </div> ); const TopVisitorCount = ({ currentNum, numberRef, sectionRef }: VisitorCountProps) => ( <div css={styles.topContainer}> <div ref={sectionRef} css={styles.topVisitorCount}> <Message currentNum={currentNum} numberRef={numberRef} isBottom={false} /> </div> </div> );
219-238: Consider using map for rendering repetitive image elementsFor the five decorative images, consider using an array and map to reduce repetition:
const decorativeImages = [ { src: BlackEtherImg, style: styles.blackEther }, { src: SkyblueLImg, style: styles.skyblueL }, { src: GreenBitImg, style: styles.greenBit }, { src: YellowBitImg, style: styles.yellowBit }, { src: OrangeBitImg, style: styles.orangeBit } ]; // Then in the component: {decorativeImages.map((img, index) => ( <img key={index} src={img.src} css={img.style} alt="" /> ))}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsrc/assets/black_ether.svgis excluded by!**/*.svgsrc/assets/green_bit.svgis excluded by!**/*.svgsrc/assets/orange_bit.svgis excluded by!**/*.svgsrc/assets/skyblue_L.svgis excluded by!**/*.svgsrc/assets/yellow_bit.svgis excluded by!**/*.svg
📒 Files selected for processing (4)
package.json(1 hunks)src/app/home/index.tsx(6 hunks)src/components/app/home/VisitorCount.tsx(1 hunks)src/hooks/api/visitor/useApiTotalView.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- src/hooks/api/visitor/useApiTotalView.ts
- src/app/home/index.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/app/home/VisitorCount.tsx (1)
src/style/variable.ts (3)
BREAK_POINTS(162-165)DESIGN_SYSTEM_TEXT(3-121)DESIGN_SYSTEM_COLOR(123-160)
🔇 Additional comments (2)
src/components/app/home/VisitorCount.tsx (2)
9-14: LGTM: Well-structured component props interfaceThe interface is well-defined with appropriate types, including the optional properties marked with the
?symbol.
136-174:Details
❓ Verification inconclusive
Custom shape with clip-path may have browser compatibility issues
The speech bubble effect using clip-path with a complex path might not be supported in all browsers. Consider adding a fallback style or testing across various browsers.
🌐 Web query:
Which browsers support the CSS clip-path property with path() function?💡 Result:
The CSS
clip-pathproperty with thepath()function allows you to define complex clipping paths using SVG path syntax. As of May 2025, support for this feature varies across different browsers:
Chrome: Full support from version 88 onwards. (caniuse.com)
Firefox: Full support from version 71 onwards. (caniuse.com)
Safari: Full support from version 10 onwards. (caniuse.com)
Edge: Full support from version 88 onwards. (caniuse.com)
Opera: Full support from version 74 onwards. (caniuse.com)
Internet Explorer: Not supported. (caniuse.com)
For mobile browsers, support is generally aligned with their desktop counterparts. However, it's important to note that support can vary based on specific versions and updates. Therefore, it's advisable to test your implementation across the target browsers and devices to ensure consistent behavior.
Citations:
- 1: https://caniuse.com/mdn-css_properties_clip-path_path?utm_source=openai
- 2: https://caniuse.com/mdn-css_properties_clip-path_path?utm_source=openai
- 3: https://caniuse.com/mdn-css_properties_clip-path_path?utm_source=openai
- 4: https://caniuse.com/mdn-css_properties_clip-path_path?utm_source=openai
- 5: https://caniuse.com/mdn-css_properties_clip-path_path?utm_source=openai
- 6: https://caniuse.com/mdn-css_properties_clip-path_path?utm_source=openai
Clip-Path Path() Compatibility Check
Theclip-path: path()function is supported in modern browsers but not in Internet Explorer. Please confirm your browser support targets and, if IE compatibility is required, provide a fallback or alternative implementation.Browser support (source: caniuse.com):
- Chrome ≥ 88
- Firefox ≥ 71
- Safari ≥ 10
- Edge ≥ 88
- Opera ≥ 74
- Internet Explorer: Not supported
Suggested fallback options:
- Use an SVG
<clipPath>- Approximate the shape with
clip-path: polygon()- Provide a simple rectangular or border-based “speech bubble” for unsupported browsers
| <img | ||
| src={BlackEtherImg} | ||
| css={blackEtherStyle} | ||
| /> | ||
| <img | ||
| src={SkyblueLImg} | ||
| css={skyblueLStyle} | ||
| /> | ||
| <img | ||
| src={GreenBitImg} | ||
| css={greenBitStyle} | ||
| /> | ||
| <img | ||
| src={YellowBitImg} | ||
| css={yellowBitStyle} | ||
| /> | ||
| <img | ||
| src={OrangeBitImg} | ||
| css={orangeBitStyle} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add alt text to images for accessibility
The decorative images currently don't have alt attributes. For accessibility, add empty alt attributes to indicate they're decorative:
-<img src={BlackEtherImg} css={blackEtherStyle} />
+<img src={BlackEtherImg} css={blackEtherStyle} alt="" />(Same applies to all images)
📝 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.
| <img | |
| src={BlackEtherImg} | |
| css={blackEtherStyle} | |
| /> | |
| <img | |
| src={SkyblueLImg} | |
| css={skyblueLStyle} | |
| /> | |
| <img | |
| src={GreenBitImg} | |
| css={greenBitStyle} | |
| /> | |
| <img | |
| src={YellowBitImg} | |
| css={yellowBitStyle} | |
| /> | |
| <img | |
| src={OrangeBitImg} | |
| css={orangeBitStyle} | |
| /> | |
| <img | |
| src={BlackEtherImg} | |
| css={blackEtherStyle} | |
| alt="" | |
| /> | |
| <img | |
| src={SkyblueLImg} | |
| css={skyblueLStyle} | |
| alt="" | |
| /> | |
| <img | |
| src={GreenBitImg} | |
| css={greenBitStyle} | |
| alt="" | |
| /> | |
| <img | |
| src={YellowBitImg} | |
| css={yellowBitStyle} | |
| alt="" | |
| /> | |
| <img | |
| src={OrangeBitImg} | |
| css={orangeBitStyle} | |
| alt="" | |
| /> |
--- 조회수 배너 UI 구현
🙋 Summary (요약)
Summary by CodeRabbit
New Features
Bug Fixes
Chores