[Step4] hippo - 재사용 가능한 컴포넌트 설계 - #6
Conversation
meteorqz6
changed the title
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (1)
src/App.jsx (1)
41-50: ⚡ Quick win레스토랑 추가 상태 갱신은 함수형 업데이트가 더 안전합니다.
현재 구현은
restaurants클로저를 직접 참조하므로 연속 업데이트 상황에서 누락 가능성이 있습니다. 함수형 업데이트로 변경해 안전성을 높여 주세요.🔧 제안 diff
function handleRestaurantSubmit({ category, name, description }) { - setRestaurants([ - ...restaurants, - { - id: Date.now(), - category, - name, - description, - }, - ]); + setRestaurants((prevRestaurants) => [ + ...prevRestaurants, + { + id: Date.now(), + category, + name, + description, + }, + ]); setIsAddRestaurantModalOpen(false); }🤖 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/App.jsx` around lines 41 - 50, The handleRestaurantSubmit function mutates state using the restaurants closure which can miss updates; change setRestaurants to a functional update by passing an updater callback to setRestaurants (e.g., setRestaurants(prev => [...prev, { id: Date.now(), category, name, description }])) so additions use the latest state; update the implementation in handleRestaurantSubmit to reference setRestaurants and restaurants only via the functional prev parameter.
🤖 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 `@README.md`:
- Around line 42-43: Update the README wording to correct the
controlled/uncontrolled terminology: change the line about "폼 데이터 (category,
name, description) → 지역 state로 자체 관리 → uncontrolled" to state that binding
inputs to local React state (category, name, description) makes them controlled
inputs, and clarify separately that "모달 열림/닫힘 → App이 소유" refers to which
component owns the open/close state (parent-owned) not the controlledness of the
inputs; also add a short note that uncontrolled inputs are those managed by the
DOM (refs) rather than React state to avoid confusion.
In `@src/App.jsx`:
- Around line 33-35: Ensure the add modal and detail modal are mutually
exclusive: update handleAddButtonClick to clear any open detail modal (call
setClickedRestaurant(null)) before opening the add modal
(setIsAddRestaurantModalOpen(true)), and likewise wherever you set
clickedRestaurant (the detail-opening logic) ensure you close the add modal
first by calling setIsAddRestaurantModalOpen(false) before setting
clickedRestaurant; reference functions/variables: handleAddButtonClick,
setIsAddRestaurantModalOpen, clickedRestaurant, setClickedRestaurant.
In `@src/components/AddRestaurantModal/AddRestaurantModal.jsx`:
- Around line 70-76: Add an explicit Cancel button to the modal footer so
keyboard-only users can close it: inside the JSX block using
styles.modal__buttonContainer (in the AddRestaurantModal component), add a
secondary <button type="button">취소</button> that calls the modal’s onClose prop
(e.g., onClick={onClose}) and keep the existing submit button as type="submit";
ensure the Cancel button uses the secondary button class (e.g., styles.button)
and an accessible label so it closes the modal without submitting.
In `@src/components/Modal/Modal.jsx`:
- Around line 6-10: Update the Modal component (Modal.jsx) to add proper
accessibility attributes and Escape-key handling: assign role="dialog" and
aria-modal="true" to the container div, give the title element a stable id
(e.g., modalTitleId) and set aria-labelledby on the dialog container to that id
(or aria-label if no title), and add a useEffect hook that registers a keydown
listener which calls onClose when event.key === 'Escape' (clean up the listener
on unmount). Ensure the title element uses the same id (modalTitleId) so screen
readers can associate the dialog with its heading.
In `@src/components/Modal/Modal.module.css`:
- Line 8: Remove the unnecessary blank lines before CSS declarations that
trigger stylelint's declaration-empty-line-before rule in Modal.module.css:
delete the empty line immediately before the background: rgba(0, 0, 0, 0.35);
declaration and likewise remove the blank lines before the declarations at the
other flagged locations (the ones around lines referenced 16 and 18) so each
property sits directly under its selector or previous property without extra
empty lines.
---
Nitpick comments:
In `@src/App.jsx`:
- Around line 41-50: The handleRestaurantSubmit function mutates state using the
restaurants closure which can miss updates; change setRestaurants to a
functional update by passing an updater callback to setRestaurants (e.g.,
setRestaurants(prev => [...prev, { id: Date.now(), category, name, description
}])) so additions use the latest state; update the implementation in
handleRestaurantSubmit to reference setRestaurants and restaurants only via the
functional prev parameter.
🪄 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 Plus
Run ID: c211c130-1d13-49f5-a244-898cc5bff7b4
📒 Files selected for processing (12)
.coderabbit.yaml04-form/README.mdREADME.mdsrc/App.jsxsrc/components/AddRestaurantModal/AddRestaurantModal.jsxsrc/components/AddRestaurantModal/AddRestaurantModal.module.csssrc/components/CategoryFilter/CategoryFilter.jsxsrc/components/Header/Header.jsxsrc/components/Modal/Modal.jsxsrc/components/Modal/Modal.module.csssrc/components/RestaurantDetailModal/RestaurantDetailModal.jsxsrc/components/RestaurantDetailModal/RestaurantDetailModal.module.css
💤 Files with no reviewable changes (2)
- src/components/AddRestaurantModal/AddRestaurantModal.module.css
- src/components/RestaurantDetailModal/RestaurantDetailModal.module.css
| - **폼 데이터** (category, name, description) → 지역 state로 자체 관리 → **uncontrolled** | ||
| - **모달 열림/닫힘** → App이 소유 → **controlled** |
There was a problem hiding this comment.
controlled/uncontrolled 용어 정의가 반대로 서술되어 있습니다.
Line 42-43에서 category, name, description을 “지역 state로 관리하므로 uncontrolled”라고 적었는데, React 기준으로는 입력값을 state로 바인딩해 관리하면 controlled input입니다. state 소유 위치(부모/자식)와 controlled 여부를 분리해서 서술하는 쪽이 정확합니다.
🤖 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 `@README.md` around lines 42 - 43, Update the README wording to correct the
controlled/uncontrolled terminology: change the line about "폼 데이터 (category,
name, description) → 지역 state로 자체 관리 → uncontrolled" to state that binding
inputs to local React state (category, name, description) makes them controlled
inputs, and clarify separately that "모달 열림/닫힘 → App이 소유" refers to which
component owns the open/close state (parent-owned) not the controlledness of the
inputs; also add a short note that uncontrolled inputs are those managed by the
DOM (refs) rather than React state to avoid confusion.
| function handleAddButtonClick() { | ||
| setIsAddRestaurantModalOpen(true); | ||
| } |
There was a problem hiding this comment.
상세 모달과 추가 모달이 동시에 열린 상태가 될 수 있습니다.
clickedRestaurant와 isAddRestaurantModalOpen가 독립이라, 상세 모달이 열린 상태에서도 추가 모달을 열 수 있습니다. 추가 모달 오픈 시 상세 모달 상태를 먼저 닫아 모달을 상호배타적으로 유지해 주세요.
🔧 제안 diff
function handleAddButtonClick() {
+ setClickedRestaurant(null);
setIsAddRestaurantModalOpen(true);
}Also applies to: 68-79
🤖 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/App.jsx` around lines 33 - 35, Ensure the add modal and detail modal are
mutually exclusive: update handleAddButtonClick to clear any open detail modal
(call setClickedRestaurant(null)) before opening the add modal
(setIsAddRestaurantModalOpen(true)), and likewise wherever you set
clickedRestaurant (the detail-opening logic) ensure you close the add modal
first by calling setIsAddRestaurantModalOpen(false) before setting
clickedRestaurant; reference functions/variables: handleAddButtonClick,
setIsAddRestaurantModalOpen, clickedRestaurant, setClickedRestaurant.
| <div className={styles.modal__buttonContainer}> | ||
| <button | ||
| className={`${styles.button} ${styles["button--primary"]} text-caption`} | ||
| > | ||
| 추가하기 | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
폼 모달에 명시적인 취소 버튼을 추가해 주세요.
Line 70-76에서 submit 버튼만 제공되어, 마우스 없이 사용 시(특히 백드롭 클릭 불가 상황) 모달 종료 경로가 부족합니다. type="button" 취소 버튼을 추가해 onClose를 직접 호출해 주세요.
🔧 제안 diff
<div className={styles.modal__buttonContainer}>
+ <button
+ type="button"
+ className={`${styles.button} text-caption`}
+ onClick={onClose}
+ >
+ 취소
+ </button>
<button
+ type="submit"
className={`${styles.button} ${styles["button--primary"]} text-caption`}
>
추가하기
</button>
</div>📝 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.
| <div className={styles.modal__buttonContainer}> | |
| <button | |
| className={`${styles.button} ${styles["button--primary"]} text-caption`} | |
| > | |
| 추가하기 | |
| </button> | |
| </div> | |
| <div className={styles.modal__buttonContainer}> | |
| <button | |
| type="button" | |
| className={`${styles.button} text-caption`} | |
| onClick={onClose} | |
| > | |
| 취소 | |
| </button> | |
| <button | |
| type="submit" | |
| className={`${styles.button} ${styles["button--primary"]} text-caption`} | |
| > | |
| 추가하기 | |
| </button> | |
| </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/components/AddRestaurantModal/AddRestaurantModal.jsx` around lines 70 -
76, Add an explicit Cancel button to the modal footer so keyboard-only users can
close it: inside the JSX block using styles.modal__buttonContainer (in the
AddRestaurantModal component), add a secondary <button type="button">취소</button>
that calls the modal’s onClose prop (e.g., onClick={onClose}) and keep the
existing submit button as type="submit"; ensure the Cancel button uses the
secondary button class (e.g., styles.button) and an accessible label so it
closes the modal without submitting.
| <div className={styles.modal__backdrop} onClick={onClose}></div> | ||
| <div className={styles.modal__container}> | ||
| <h2 className={`${styles.modal__title} text-title`}>{title}</h2> | ||
| {children} | ||
| </div> |
There was a problem hiding this comment.
모달 공통 컴포넌트에 접근성 대화상자 속성과 ESC 닫기 처리가 필요합니다.
Line 6-10 기준으로 현재 구현은 시각적 오버레이만 제공하고, 스크린리더/키보드 사용자 기준의 모달 인식·종료 경로가 부족합니다. role="dialog", aria-modal, 제목 연결(예: aria-label 또는 aria-labelledby)과 Escape 키 닫기를 공통 컴포넌트에서 제공해 주세요.
🔧 제안 diff
+import { useEffect } from "react";
import styles from "./Modal.module.css";
export default function Modal({ children, title, onClose }) {
+ useEffect(() => {
+ function handleKeyDown(e) {
+ if (e.key === "Escape") onClose();
+ }
+
+ document.addEventListener("keydown", handleKeyDown);
+ return () => document.removeEventListener("keydown", handleKeyDown);
+ }, [onClose]);
+
return (
<>
<div className={styles.modal__backdrop} onClick={onClose}></div>
- <div className={styles.modal__container}>
+ <div
+ className={styles.modal__container}
+ role="dialog"
+ aria-modal="true"
+ aria-label={title}
+ >
<h2 className={`${styles.modal__title} text-title`}>{title}</h2>
{children}
</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/components/Modal/Modal.jsx` around lines 6 - 10, Update the Modal
component (Modal.jsx) to add proper accessibility attributes and Escape-key
handling: assign role="dialog" and aria-modal="true" to the container div, give
the title element a stable id (e.g., modalTitleId) and set aria-labelledby on
the dialog container to that id (or aria-label if no title), and add a useEffect
hook that registers a keydown listener which calls onClose when event.key ===
'Escape' (clean up the listener on unmount). Ensure the title element uses the
same id (modalTitleId) so screen readers can associate the dialog with its
heading.
| bottom: 0; | ||
| left: 0; | ||
|
|
||
| background: rgba(0, 0, 0, 0.35); |
There was a problem hiding this comment.
Stylelint 에러가 발생하는 빈 줄을 정리해 주세요.
Line 8, Line 16, Line 18의 선언 앞 빈 줄 때문에 declaration-empty-line-before 에러가 납니다. 린트 통과를 위해 불필요한 공백 줄을 제거해 주세요.
Also applies to: 16-16, 18-18
🧰 Tools
🪛 Stylelint (17.12.0)
[error] 8-8: Expected no empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 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/Modal/Modal.module.css` at line 8, Remove the unnecessary
blank lines before CSS declarations that trigger stylelint's
declaration-empty-line-before rule in Modal.module.css: delete the empty line
immediately before the background: rgba(0, 0, 0, 0.35); declaration and likewise
remove the blank lines before the declarations at the other flagged locations
(the ones around lines referenced 16 and 18) so each property sits directly
under its selector or previous property without extra empty lines.
Source: Linters/SAST tools
| </p> | ||
| </div> | ||
| <li key={restaurant.id} className={styles.restaurant}> | ||
| <button className={styles.restaurant__button} onClick={() => onRestaurantClick(restaurant)}> |
There was a problem hiding this comment.
[배움]
저는 <li>에 role="button", tabIndex, onKeyDown을 직접 달았는데, <button>으로 감싸면 키보드 접근성이 기본으로 해결되고 더 시맨틱하네요. 저도 이 방식으로 적용해보겠습니다!
|
|
||
| function App() { | ||
| const [category, setCategory] = useState("전체"); | ||
| const [filterCategory, setFilterCategory] = useState("전체"); |
There was a problem hiding this comment.
[배움]
저는 category로 선언했는데, filterCategory로 쓰면 필터용 카테고리라는 게 바로 읽히네요. 폼의 category랑 혼동될 여지도 없고 더 명확한 것 같아요.
|
|
||
| function handleChange(e) { | ||
| setCategory(e.target.value); | ||
| function handleFilterCategoryChange(e) { |
There was a problem hiding this comment.
[배움]
handle + 대상 + 동작, props는 on-으로 통일하는 규칙을 쓰셨는데, 규칙이 있는 점이 가독성 면에서도 좋은 것 같아요!요?
| const [clickedRestaurant, setClickedRestaurant] = useState(null); | ||
| const [isAddRestaurantModalOpen, setIsAddRestaurantModalOpen] = | ||
| useState(false); | ||
| const [restaurants, setRestaurants] = useState(RESTAURANTS); |
There was a problem hiding this comment.
[논의]
저는 RESTAURANTS 상수와 구분하려고 newRestaurants로 이름 지었는데, 상수는 대문자라 소문자 restaurants만으로도 충분히 구분되는 것 같더라고요. 어느 쪽이 더 자연스러운지 얘기해보면 좋을 것 같아요!
|
|
||
| export default function Modal({ children, title, onClose }) { | ||
| return ( | ||
| <> |
There was a problem hiding this comment.
[배움]
저는 래퍼 <div>를 그대로 뒀는데, backdrop과 container가 둘 다 position: fixed라 부모 레이아웃에 영향을 안 받는다는 걸 몰랐어요. 불필요한 DOM 노드를 줄이는 관점에서 배웠습니다!
| setRestaurants((prev) => [ | ||
| ...prev, | ||
| { | ||
| id: Date.now(), |
There was a problem hiding this comment.
[배움]
저는 id 생성을 AddRestaurantModal에서 했는데, id는 음식점 데이터의 일부고 실제로 데이터를 관리하는 곳은 App이니까 생성 책임도 App에 있는 게 더 자연스러운 것 같아요. 저도 옮겨보겠습니다!
[제안]
Date.now()는 밀리초 단위라 짧은 시간 안에 두 번 호출되면 같은 값이 나올 수 있다고 해요. crypto.randomUUID()로 바꾸면 충돌 없는 고유 id를 보장할 수 있어요. 적용해보는 건 어떠신가요?
There was a problem hiding this comment.
제안 감사합니다. Date.now()를 사용하는 것보다 cryto.randomUUID()를 사용하는 것이 더 적절할 것 같네요!
|
[배움] |
2 participants
개인 목표 달성 여부
리뷰어에게
Summary by CodeRabbit
릴리스 노트
새 기능
문서