Conversation
8 tasks
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
워크스루
변경 사항ConfirmModal 컴포넌트 추가 및 Surface 개선
추정 코드 리뷰 난이도🎯 3 (Moderate) | ⏱️ ~20 minutes 관련 PR
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
🧪 테스트 결과
|
🚦 CI 검증 결과
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/shared/ui/confirm-modal/index.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win배럴에서
ConfirmModalProps도 함께 내보내세요.지금 공개 엔트리에서는 컴포넌트만 import할 수 있어서, 소비자 쪽에서 props 타입을 쓰려면 구현 파일로 깊은 import를 해야 합니다. shared UI 공개 경로를 추가한 PR이라면 타입도 같은 배럴에서 re-export하는 편이 API 표면을 안정적으로 유지합니다.
예시 변경
export { ConfirmModal } from './ConfirmModal'; +export type { ConfirmModalProps } from './ConfirmModal';🤖 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/shared/ui/confirm-modal/index.ts` at line 1, The confirm-modal barrel currently re-exports only ConfirmModal, so consumers cannot import ConfirmModalProps from the public entry. Update the index barrel in the confirm-modal module to also re-export ConfirmModalProps from the same source as ConfirmModal, keeping the shared UI API surface stable and avoiding deep imports.
🤖 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/shared/styles/components/surface.css`:
- Around line 1-29: The keyframes in surface.css use non-kebab-case names, so
update both `@keyframes` identifiers and every animation reference in the
surfaceOverlayFadeIn and surfaceModalSlideUp styles to kebab-case consistently.
Make sure the .surface-modal-overlay-animation and .surface-modal-animation
rules point to the renamed keyframes so Stylelint no longer flags the animation
names.
In `@src/shared/ui/confirm-modal/ConfirmModal.stories.tsx`:
- Around line 69-84: The LoadingConfirmModalExample story can get stuck because
ConfirmModal stays non-dismissible while isLoading is true and there is no way
to release it. Update LoadingConfirmModalExample in ConfirmModal.stories.tsx so
the example has a reset/close path, such as toggling isLoading off after a short
timer or adding a separate control to clear loading and close the modal; use the
existing LoadingConfirmModalExample and ConfirmModal props as the place to fix
it.
In `@src/shared/ui/confirm-modal/ConfirmModal.tsx`:
- Around line 92-95: ConfirmModal의 description 렌더링이 ReactNode를 항상 p로 감싸서 블록 요소가
들어올 때 잘못된 중첩이 생깁니다. ConfirmModalProps.description과 ConfirmModal 컴포넌트의
description 처리 방식을 함께 확인해서, description 타입을 문자열/인라인 콘텐츠로 제한하거나 렌더링 컨테이너를 p에서
div로 변경해 다양한 ReactNode를 안전하게 받도록 수정하세요.
---
Nitpick comments:
In `@src/shared/ui/confirm-modal/index.ts`:
- Line 1: The confirm-modal barrel currently re-exports only ConfirmModal, so
consumers cannot import ConfirmModalProps from the public entry. Update the
index barrel in the confirm-modal module to also re-export ConfirmModalProps
from the same source as ConfirmModal, keeping the shared UI API surface stable
and avoiding deep imports.
🪄 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: 8c176748-2548-4204-b1ea-6e622ba8913d
📒 Files selected for processing (14)
.storybook/main.tssrc/shared/styles/components/surface.csssrc/shared/styles/globals.csssrc/shared/ui/accordion/AccordionLabel.tsxsrc/shared/ui/button/buttonVariants.tssrc/shared/ui/confirm-modal/ConfirmModal.stories.tsxsrc/shared/ui/confirm-modal/ConfirmModal.tsxsrc/shared/ui/confirm-modal/ConfirmModal.types.tssrc/shared/ui/confirm-modal/index.tssrc/shared/ui/surface/SurfaceClose.tsxsrc/shared/ui/surface/SurfaceContent.tsxsrc/shared/ui/surface/SurfaceOverlay.tsxsrc/shared/ui/surface/SurfaceTrigger.tsxsrc/shared/utils/slot.ts
Comment on lines
+1
to
+29
| @keyframes surfaceOverlayFadeIn { | ||
| from { | ||
| opacity: 0; | ||
| } | ||
|
|
||
| to { | ||
| opacity: 1; | ||
| } | ||
| } | ||
|
|
||
| @keyframes surfaceModalSlideUp { | ||
| from { | ||
| opacity: 0; | ||
| transform: translate3d(0, 40px, 0); | ||
| } | ||
|
|
||
| to { | ||
| opacity: 1; | ||
| transform: translate3d(0, 0, 0); | ||
| } | ||
| } | ||
|
|
||
| .surface-modal-overlay-animation { | ||
| animation: surfaceOverlayFadeIn 350ms ease-out both; | ||
| } | ||
|
|
||
| .surface-modal-animation { | ||
| will-change: transform, opacity; | ||
| animation: surfaceModalSlideUp 350ms ease-out both; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
키프레임 이름을 kebab-case로 맞춰 주세요.
Stylelint가 현재 surfaceOverlayFadeIn, surfaceModalSlideUp를 규칙 위반으로 보고 있습니다. @keyframes 이름과 animation 참조를 함께 바꾸지 않으면 린트가 계속 깨집니다.
수정 예시
-@keyframes surfaceOverlayFadeIn {
+@keyframes surface-overlay-fade-in {
@@
-@keyframes surfaceModalSlideUp {
+@keyframes surface-modal-slide-up {
@@
.surface-modal-overlay-animation {
- animation: surfaceOverlayFadeIn 350ms ease-out both;
+ animation: surface-overlay-fade-in 350ms ease-out both;
}
@@
.surface-modal-animation {
will-change: transform, opacity;
- animation: surfaceModalSlideUp 350ms ease-out both;
+ animation: surface-modal-slide-up 350ms ease-out both;
}📝 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
| @keyframes surfaceOverlayFadeIn { | |
| from { | |
| opacity: 0; | |
| } | |
| to { | |
| opacity: 1; | |
| } | |
| } | |
| @keyframes surfaceModalSlideUp { | |
| from { | |
| opacity: 0; | |
| transform: translate3d(0, 40px, 0); | |
| } | |
| to { | |
| opacity: 1; | |
| transform: translate3d(0, 0, 0); | |
| } | |
| } | |
| .surface-modal-overlay-animation { | |
| animation: surfaceOverlayFadeIn 350ms ease-out both; | |
| } | |
| .surface-modal-animation { | |
| will-change: transform, opacity; | |
| animation: surfaceModalSlideUp 350ms ease-out both; | |
| `@keyframes` surface-overlay-fade-in { | |
| from { | |
| opacity: 0; | |
| } | |
| to { | |
| opacity: 1; | |
| } | |
| } | |
| `@keyframes` surface-modal-slide-up { | |
| from { | |
| opacity: 0; | |
| transform: translate3d(0, 40px, 0); | |
| } | |
| to { | |
| opacity: 1; | |
| transform: translate3d(0, 0, 0); | |
| } | |
| } | |
| .surface-modal-overlay-animation { | |
| animation: surface-overlay-fade-in 350ms ease-out both; | |
| } | |
| .surface-modal-animation { | |
| will-change: transform, opacity; | |
| animation: surface-modal-slide-up 350ms ease-out both; |
🧰 Tools
🪛 Stylelint (17.13.0)
[error] 1-1: Expected keyframe name "surfaceOverlayFadeIn" to be kebab-case (keyframes-name-pattern)
(keyframes-name-pattern)
[error] 11-11: Expected keyframe name "surfaceModalSlideUp" to be kebab-case (keyframes-name-pattern)
(keyframes-name-pattern)
🤖 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/shared/styles/components/surface.css` around lines 1 - 29, The keyframes
in surface.css use non-kebab-case names, so update both `@keyframes` identifiers
and every animation reference in the surfaceOverlayFadeIn and
surfaceModalSlideUp styles to kebab-case consistently. Make sure the
.surface-modal-overlay-animation and .surface-modal-animation rules point to the
renamed keyframes so Stylelint no longer flags the animation names.
Source: Linters/SAST tools
Comment on lines
+69
to
+84
| function LoadingConfirmModalExample() { | ||
| const [isOpen, setIsOpen] = useState(false); | ||
|
|
||
| return ( | ||
| <div className="flex min-h-80 items-center justify-center"> | ||
| <Button onClick={() => setIsOpen(true)}>Loading ConfirmModal 열기</Button> | ||
| <ConfirmModal | ||
| cancelLabel="아니오" | ||
| confirmLabel="저장하기" | ||
| description="저장 중에는 모달을 닫을 수 없습니다." | ||
| isLoading | ||
| onConfirm={() => undefined} | ||
| onOpenChange={setIsOpen} | ||
| open={isOpen} | ||
| title="해당 자기소개서를 저장하시겠습니까?" | ||
| /> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Loading 스토리는 한 번 열면 캔버스에서 빠져나올 수 없습니다.
ConfirmModal은 isLoading일 때 취소 버튼, ESC, 바깥 클릭 dismiss를 모두 막는데 이 예제는 로딩을 해제하거나 리셋할 경로가 없습니다. 문서 예제가 그대로 갇힌 상태가 되므로, 타이머로 isLoading을 풀어주거나 별도 reset/close 컨트롤을 두는 편이 좋겠습니다.
🤖 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/shared/ui/confirm-modal/ConfirmModal.stories.tsx` around lines 69 - 84,
The LoadingConfirmModalExample story can get stuck because ConfirmModal stays
non-dismissible while isLoading is true and there is no way to release it.
Update LoadingConfirmModalExample in ConfirmModal.stories.tsx so the example has
a reset/close path, such as toggling isLoading off after a short timer or adding
a separate control to clear loading and close the modal; use the existing
LoadingConfirmModalExample and ConfirmModal props as the place to fix it.
Comment on lines
+92
to
+95
| {description ? ( | ||
| <p className="m-0 body-14 font-medium text-gray-100" id={descriptionId}> | ||
| {description} | ||
| </p> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
description을 <p>로 고정하면 조합 가능한 ReactNode와 충돌합니다.
ConfirmModalProps.description은 ReactNode인데 여기서는 항상 <p> 안에 렌더링하고 있습니다. 호출부가 블록 요소나 다른 문단을 넘기면 잘못된 HTML 중첩이 생기므로, 타입을 문자열/인라인 콘텐츠로 좁히거나 컨테이너를 <div>로 바꾸는 쪽이 안전합니다.
🤖 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/shared/ui/confirm-modal/ConfirmModal.tsx` around lines 92 - 95,
ConfirmModal의 description 렌더링이 ReactNode를 항상 p로 감싸서 블록 요소가 들어올 때 잘못된 중첩이 생깁니다.
ConfirmModalProps.description과 ConfirmModal 컴포넌트의 description 처리 방식을 함께 확인해서,
description 타입을 문자열/인라인 콘텐츠로 제한하거나 렌더링 컨테이너를 p에서 div로 변경해 다양한 ReactNode를 안전하게
받도록 수정하세요.
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
1 participant
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.
#️⃣연관된 이슈
체크 사항
📝작업 내용
ConfirmModal 공통 컴포넌트 구현
Surface의modalvariant를 기반으로ConfirmModal을 구현했습니다.onConfirm,onCancel이벤트를 제공합니다.isLoading상태에서 확인 버튼 로딩 표시와 dismiss 닫기 방지를 처리했습니다.aria-labelledby,aria-describedby를 연결했습니다.Storybook 문서화
Shared/ConfirmModalStorybook을 추가했습니다.Surface 애니메이션 및 유틸 정리
prefers-reduced-motion환경에서는 애니메이션을 비활성화합니다.Slot유틸을shared/ui내부에서shared/utils로 이동해 공용 유틸 위치로 정리했습니다.스크린샷 (선택)
추가한 라이브러리 (선택)
💬리뷰 요구사항(선택)
Summary by CodeRabbit
New Features
Documentation
Bug Fixes