Conversation
Walkthrough앱 웹 패키지의 UI/overlay 관련 의존성을 정리·추가하고, Vaul 기반의 새로운 BottomSheet 컴포넌트(타입·스타일·인덱스 재노출 포함)와 Storybook 스토리(스크롤/비스크롤 예시)를 추가했습니다. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Story as Storybook UI
participant Overlay as useOverlay
participant BS as BottomSheet
participant Vaul as Vaul Drawer
User->>Story: 버튼 클릭 (Open)
Story->>Overlay: overlay.open(render = <BottomSheet...>)
Overlay-->>Story: Portal에 렌더링 지시
Story->>BS: props(open, onClose, theme, radius, ...)
BS->>Vaul: Drawer.Root(open, modal)
Vaul->>BS: Overlay/Content 마운트
User->>BS: 인터랙션(스와이프/닫기)
BS->>Vaul: onOpenChange(false)
Vaul-->>BS: 상태 변경 반영
BS-->>Story: onClose 콜백 실행
Note right of BS: showHandle/handleOnly/locked에 따라 동작 제약 적용
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
📚 Storybook is ready for review! |
![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 (5)
apps/web/src/shared/components/BottomSheet/BottomSheet.tsx (1)
29-38: boolean을 문자열로 변환하는 패턴 개선 제안
showHandleboolean을String()으로 변환하여 SwitchCase에 전달하는 것보다, 단순 조건부 렌더링이 더 명확합니다.다음과 같이 리팩토링을 고려해보세요:
- <SwitchCase - value={String(showHandle)} - caseBy={{ - true: ( - <div className={styles.drawerHandle}> - <Drawer.Handle style={{ width: 36 }} /> - </div> - ), - }} - /> + {showHandle && ( + <div className={styles.drawerHandle}> + <Drawer.Handle style={{ width: 36 }} /> + </div> + )}apps/web/src/shared/components/BottomSheet/BottomSheet.stories.tsx (1)
65-66: eslint-disable 규칙을 구체적으로 명시
eslint-disable주석이 너무 광범위합니다. 특정 규칙만 비활성화하는 것이 좋습니다.다음과 같이 개선하세요:
- // eslint-disable-next-line + // eslint-disable-next-line react-hooks/rules-of-hooks const overlay = useOverlay();apps/web/src/shared/components/BottomSheet/BottomSheet.style.ts (2)
29-30: !important 사용 검토
!important는 CSS 특정성(specificity) 문제를 우회하는 방법이지만, 장기적으로 유지보수를 어렵게 만듭니다. vaul의 기본 스타일과 충돌이 있는지 확인하고, 가능하면 더 구체적인 셀렉터로 해결하는 것이 좋습니다.
4-11: 하드코딩된 디자인 토큰색상 및 spacing 값이 하드코딩되어 있습니다. 디자인 시스템이나 테마 토큰을 사용하면 일관성 있고 유지보수하기 쉬운 코드가 됩니다.
예시:
import { tokens } from "../../../../styled-system/tokens"; const SEED_PALETTE_COLOR_BASE_100 = tokens.colors.base100; // ...apps/web/src/shared/components/BottomSheet/BottomSheet.type.ts (1)
1-11: 상수 네이밍 일관성
BottomSheetRadius는 대문자 키(NONE, SMALL, MEDIUM, LARGE)를 사용하는 반면,BottomSheetTheme는 소문자 키(light, dark)를 사용하여 일관성이 부족합니다.다음과 같이 통일하는 것을 권장합니다:
export const BottomSheetTheme = { - light: "light", - dark: "dark", + LIGHT: "light", + DARK: "dark", } as const;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/web/package.json(1 hunks)apps/web/src/shared/components/BottomSheet/BottomSheet.stories.tsx(1 hunks)apps/web/src/shared/components/BottomSheet/BottomSheet.style.ts(1 hunks)apps/web/src/shared/components/BottomSheet/BottomSheet.tsx(1 hunks)apps/web/src/shared/components/BottomSheet/BottomSheet.type.ts(1 hunks)apps/web/src/shared/components/BottomSheet/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/web/src/shared/components/BottomSheet/BottomSheet.type.ts (1)
apps/web/src/shared/components/BottomSheet/index.ts (1)
BottomSheetProps(2-2)
apps/web/src/shared/components/BottomSheet/BottomSheet.stories.tsx (2)
apps/web/src/shared/components/BottomSheet/BottomSheet.tsx (1)
BottomSheet(8-44)apps/web/src/shared/components/BottomSheet/BottomSheet.style.ts (1)
overlay(13-17)
apps/web/src/shared/components/BottomSheet/BottomSheet.tsx (2)
apps/web/src/shared/components/BottomSheet/BottomSheet.type.ts (1)
BottomSheetProps(17-28)apps/web/src/shared/utils/SwitchCase.tsx (1)
SwitchCase(9-15)
⏰ 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: Deploy Storybook to Chromatic
🔇 Additional comments (4)
apps/web/src/shared/components/BottomSheet/index.ts (1)
1-2: LGTM!표준 배럴(barrel) 패턴으로 컴포넌트와 타입을 재노출하고 있으며, 구현이 적절합니다.
apps/web/src/shared/components/BottomSheet/BottomSheet.stories.tsx (1)
43-57: LGTM!스크롤 가능/불가능한 두 가지 콘텐츠 예시가 명확하게 구현되어 있어 BottomSheet의 다양한 사용 사례를 잘 보여줍니다.
apps/web/src/shared/components/BottomSheet/BottomSheet.type.ts (1)
17-28: LGTM!BottomSheetProps 인터페이스가 명확하게 정의되어 있으며, 모든 필수 prop과 선택적 prop이 적절하게 타이핑되어 있습니다.
apps/web/src/shared/components/BottomSheet/BottomSheet.tsx (1)
21-21: vaul Drawer.Root prop명 검증 필요
vaul의 Drawer.Root가 공식적으로onOpenChange를 사용하는지 문서를 확인하고, 필요 시onClose를onOpenChange로 교체하세요.
| variants: { | ||
| radius: { | ||
| none: { | ||
| borderTop: SEED_SPACING_100, |
There was a problem hiding this comment.
CSS 속성 오류
borderTop: SEED_SPACING_100은 유효하지 않은 CSS입니다. borderTop은 width style color 형식이 필요하며, spacing 값만으로는 작동하지 않습니다. borderRadius: SEED_SPACING_100을 의도한 것으로 보입니다.
다음과 같이 수정하세요:
none: {
- borderTop: SEED_SPACING_100,
+ borderRadius: SEED_SPACING_100,
},📝 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.
| borderTop: SEED_SPACING_100, | |
| none: { | |
| borderRadius: SEED_SPACING_100, | |
| }, |
🤖 Prompt for AI Agents
In apps/web/src/shared/components/BottomSheet/BottomSheet.style.ts around line
36, the property borderTop: SEED_SPACING_100 is invalid CSS (borderTop requires
width/style/color) — replace it with borderRadius: SEED_SPACING_100 (or if you
intended a top border, use borderTopWidth, borderTopStyle and borderTopColor) so
the style uses a valid CSS property and value.
| handleOnly = false, | ||
| radius, | ||
| theme = "light", | ||
| zIndex = 1, |
There was a problem hiding this comment.
zIndex 기본값이 낮을 수 있음
오버레이 컴포넌트의 zIndex 기본값이 1로 설정되어 있는데, 일반적으로 모달/바텀시트는 1000 이상의 높은 z-index를 사용합니다. 다른 UI 요소에 가려질 위험이 있습니다.
다음과 같이 수정을 권장합니다:
- zIndex = 1,
+ zIndex = 1000,📝 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.
| zIndex = 1, | |
| zIndex = 1000, |
🤖 Prompt for AI Agents
In apps/web/src/shared/components/BottomSheet/BottomSheet.tsx around line 16,
the zIndex default is set to 1 which is too low for overlays; update the
component to use a much higher default (e.g., zIndex = 1000) so the bottom
sheet/modal appears above other UI, and ensure any TypeScript prop signature or
defaultProps are adjusted accordingly (and update any Storybook/docs or tests
that rely on the old default).
| "@radix-ui/react-dropdown-menu": "^2.1.16", | ||
| "@tanstack/react-query": "^5.80.7", | ||
| "@tanstack/react-query-devtools": "^5.80.7", | ||
| "@toss/use-overlay": "^1.4.2", |
There was a problem hiding this comment.
https://www.npmjs.com/package/@toss/use-overlay?activeTab=versions
는 deprecated된 것으로 보이는데요!
overlaykit(https://github.com/toss/overlay-kit) 이 아니라 해당 패키지를 사용한 이유가 있으신지 궁금합니다
🙏
There was a problem hiding this comment.
@seungdeok
잘못사용했었네요...!!! 수정해서 올렸습니다! 자료 감사합니다 :)
| export interface BottomSheetProps { | ||
| open?: boolean; | ||
| onClose?: () => void; | ||
| locked?: boolean; | ||
| showHandle?: boolean; | ||
| handleOnly?: boolean; | ||
| zIndex?: number; | ||
| radius?: BottomSheetRadius; | ||
| theme?: BottomSheetTheme; | ||
| className?: string; | ||
| content?: React.ReactNode; | ||
| } |
There was a problem hiding this comment.
Interface가 radix를 따른 것으로 보이긴한데요
props 설명을 알 수 있으면 사용하는 측면에서 더 좋을 것 같습니다!(ex. @description과 같은 주석)
예를 들어서 handleOnly와 같은 props가 직관적으로 어떤 쓰임인지 알기 힘들 것 같습니다🥺
|
📚 Storybook is ready for review! |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
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)
apps/web/package.json(1 hunks)apps/web/src/shared/components/BottomSheet/BottomSheet.stories.tsx(1 hunks)apps/web/src/shared/components/BottomSheet/BottomSheet.type.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/shared/components/BottomSheet/BottomSheet.type.ts
- apps/web/package.json
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/shared/components/BottomSheet/BottomSheet.stories.tsx (2)
apps/web/src/shared/components/BottomSheet/BottomSheet.tsx (1)
BottomSheet(8-44)apps/web/src/shared/components/BottomSheet/BottomSheet.style.ts (1)
overlay(13-17)
| overlay.open(({ isOpen, close }) => ( | ||
| <BottomSheet | ||
| {...args} | ||
| open={isOpen} | ||
| onClose={() => { | ||
| close(); | ||
| }} | ||
| content={<BottomSheetNoScroll />} | ||
| /> | ||
| )); |
There was a problem hiding this comment.
overlay 정리 콜백 누락으로 인한 잠재적 메모리 누수
overlay.open 콜백에서 unmount 파라미터를 사용하지 않고 있습니다. learnings에 따르면, overlay-kit의 콜백은 { isOpen, close, unmount }를 제공하며, unmount는 오버레이가 완전히 종료/언마운트될 때 호출되어야 합니다. 이를 누락하면 오버레이 인스턴스가 메모리에서 제대로 정리되지 않을 수 있습니다.
Based on learnings.
다음 diff를 적용하여 unmount 콜백을 추가하세요:
const openNoScrollBottomSheet = () => {
- overlay.open(({ isOpen, close }) => (
+ overlay.open(({ isOpen, close, unmount }) => (
<BottomSheet
{...args}
open={isOpen}
onClose={() => {
close();
+ unmount();
}}
content={<BottomSheetNoScroll />}
/>
));
};
const openScrollBottomSheet = () => {
- overlay.open(({ isOpen, close }) => (
+ overlay.open(({ isOpen, close, unmount }) => (
<BottomSheet
{...args}
open={isOpen}
onClose={() => {
close();
+ unmount();
}}
content={<BottomSheetScroll />}
/>
));
};참고: BottomSheet 컴포넌트의 onClose prop이 Drawer.Root의 onClose와 어떻게 연결되는지 확인하고, Drawer가 완전히 언마운트된 후 unmount가 호출되도록 타이밍을 조정해야 할 수 있습니다.
Also applies to: 78-87
🤖 Prompt for AI Agents
In apps/web/src/shared/components/BottomSheet/BottomSheet.stories.tsx around
lines 65-74 (and likewise 78-87), the overlay.open callback does not accept or
call the provided unmount parameter which can cause overlay instances to not be
fully cleaned up; update the overlay.open callbacks to accept ({ isOpen, close,
unmount }) and ensure unmount() is invoked after the BottomSheet/Drawer has
fully closed (e.g., call close(), then trigger unmount in the BottomSheet
onClose completion or via a post-close callback/animation end so Drawer has time
to unmount before calling unmount()).
2 participants
📝 PR 유형
📝 PR 설명
BottomSheet 컴포넌트 마이그레이션 작업입니다.
vaul을 추가합니다.use-overlay를 추가합니다.관련된 이슈 넘버
close #55
✅ 작업 목록
vaul@toss/use-overlayMR하기 전에 확인해주세요
📚 논의사항
📚 ETC
Summary by CodeRabbit
새 기능
문서
작업(Chores)