Conversation
39 tasks
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🧪 테스트 결과
|
🚦 CI 검증 결과
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough공통 훅과 Changes공통 훅 및 Surface
Possibly related PRs
🎯 4 (Complex) | ⏱️ ~60 minutes🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 11
🤖 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/hooks/useControllableState.test.ts`:
- Around line 53-68: The current functional update test in useControllableState
only verifies a single updater call, so it misses stale-state regressions when
updates are chained in the same act. Extend the supports functional updates case
to call result.current[1] twice with updater functions starting from
defaultValue and assert the final value is 3, and keep the onChange expectation
aligned with the latest resolved value from useControllableState.
In `@src/shared/hooks/useControllableState.ts`:
- Around line 41-58: The functional updater in useControllableState is
calculated from the render-time currentValue, so repeated setValue(updater)
calls can read the same stale state. Update the setValue callback in
useControllableState to apply updater calls against the latest uncontrolled
state, while still honoring the controlled path and onChange behavior, and keep
the Object.is guard intact. Add a regression test covering back-to-back
functional updates to verify consecutive calls accumulate correctly.
In `@src/shared/hooks/useFocusTrap.ts`:
- Around line 53-78: The focus trap logic in handleKeyDown only wraps when
document.activeElement is the first or last focusable element, so focus can
escape when it is already outside the container or on the container itself.
Update useFocusTrap to first check whether the current active element is inside
the container, and if not, intercept Tab to move focus back to firstElement or
lastElement depending on event.shiftKey before allowing navigation to continue.
In `@src/shared/ui/surface/Surface.stories.tsx`:
- Around line 132-193: The Storybook controls defined in the Surface.stories
meta are not wired into the previews because the story render functions ignore
args, so changes in the controls panel do not affect the rendered UI. Update the
Surface story entries such as ModalVariant, PanelVariant, and
PreventCloseWhenDirty to pass the story args through to the example components
or render Surface directly with those args; if these are meant to be static
demos, remove the unused controls from meta.args and argTypes instead.
- Around line 84-125: The PreventCloseWhenDirtyExample in Surface.stories.tsx
keeps the `message` state after the form becomes clean or the Surface is closed,
so the dirty warning can linger incorrectly. Update the example so `message` is
reset whenever `isDirty` becomes false and/or when the Surface closes, using the
existing `useState`, `isDirty`, `onClosePrevented`, and `Surface` example setup
to keep the warning aligned with the current dirty state.
In `@src/shared/ui/surface/Surface.types.ts`:
- Around line 123-127: SurfaceCloseButtonProps currently allows icon-only
Surface.Close usage without guaranteeing an accessible name because aria-label
is optional. Update the Surface.Close contract so the icon-only path cannot omit
an accessible label: either make aria-label required on SurfaceCloseButtonProps
or split the prop types in Surface.types.ts and enforce aria-label as mandatory
for the icon-only branch. Use the SurfaceCloseButtonProps symbol to locate and
adjust the public API accordingly.
In `@src/shared/ui/surface/SurfaceClose.tsx`:
- Around line 62-80: The click/close contract in SurfaceClose’s
createHandleClick is inconsistent with the Surface.Close documentation because
actions.close('close-button') is skipped when either childOnClick or onClick
calls preventDefault. Decide whether vetoing close is intended: if yes, update
the Surface.Close JSDoc to say the close request can be prevented; if no, adjust
createHandleClick so actions.close('close-button') is invoked outside the
preventDefault checks while still preserving the callbacks on
SurfaceCloseChildProps.
In `@src/shared/ui/surface/SurfacePortal.tsx`:
- Around line 21-29: `SurfacePortal` is treating an explicit `container={null}`
the same as “no container” and falling back to `document.body`. Update the
`portalContainer` selection logic in `SurfacePortal` so only an `undefined`
container uses the body fallback, while an explicit `null` remains `null` and
prevents rendering. Keep the existing `useSurfaceContext`, `SurfacePortalProps`,
and `createPortal` flow intact.
In `@src/shared/ui/surface/SurfaceSections.tsx`:
- Around line 25-31: The SurfaceSections layout currently renders these clearly
labeled header/footer areas with a generic div, so update the shared section
component(s) to use semantic header and footer elements instead of div while
preserving the existing props, ref forwarding, and className merging via cn. Use
the relevant SurfaceSections/section wrapper components to map the header-like
and footer-like slots to the proper HTML elements so the document structure and
accessibility tree reflect their meaning.
In `@src/shared/ui/surface/SurfaceStack.ts`:
- Around line 81-88: Surface registration in useSurfaceStackItem currently
happens in useEffect, which is too late for stacking before the first paint.
Move the registration logic to useLayoutEffect so registerSurface runs
synchronously for the new Surface as soon as it mounts, preventing the previous
Surface from handling ESC or outside clicks during overlap transitions.
In `@src/shared/ui/surface/SurfaceTrigger.tsx`:
- Around line 21-32: `asChild` currently accepts `React.Fragment` because
`isValidElement()` passes it, which prevents `cloneElement()` props like
`className`, `onClick`, and `aria-*` from reaching a real interactive node.
Update `getSingleTriggerChild` in `SurfaceTrigger` to explicitly reject
`Fragment` and only allow DOM-like element children before cloning, and apply
the same restriction to the matching child-validation logic in `SurfaceClose` so
both components enforce the same safe `asChild` behavior.
🪄 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: 7e1188be-d19d-46b5-9ecb-1e3353714d30
📒 Files selected for processing (23)
src/shared/hooks/index.tssrc/shared/hooks/useControllableState.test.tssrc/shared/hooks/useControllableState.tssrc/shared/hooks/useEscapeKey.tssrc/shared/hooks/useFocusRestore.tssrc/shared/hooks/useFocusTrap.tssrc/shared/hooks/useOutsideClick.tssrc/shared/hooks/useScrollLock.tssrc/shared/styles/base/z-index.csssrc/shared/styles/globals.csssrc/shared/ui/surface/Surface.stories.tsxsrc/shared/ui/surface/Surface.tsxsrc/shared/ui/surface/Surface.types.tssrc/shared/ui/surface/SurfaceClose.tsxsrc/shared/ui/surface/SurfaceContent.tsxsrc/shared/ui/surface/SurfaceContext.tssrc/shared/ui/surface/SurfaceOverlay.tsxsrc/shared/ui/surface/SurfacePortal.tsxsrc/shared/ui/surface/SurfaceSections.tsxsrc/shared/ui/surface/SurfaceStack.tssrc/shared/ui/surface/SurfaceTrigger.tsxsrc/shared/ui/surface/index.tssrc/shared/ui/textarea/TextAreaField.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/ui/accordion/Accordion.tsx (1)
122-128: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win토글은 함수형 업데이트로 바꾸는 게 안전합니다.
setIsOpen(!isOpen)는 같은 렌더 안에서 연속 토글이 들어오면 같은 값을 기준으로 계산될 수 있습니다.useControllableState가 updater를 받으니 이 경로를 쓰는 편이 맞습니다.수정 예시
- setIsOpen(!isOpen); + setIsOpen((previousOpen) => !previousOpen);🤖 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/accordion/Accordion.tsx` around lines 122 - 128, The Accordion toggleOpen handler currently uses a stale state read via setIsOpen(!isOpen), which can break when multiple toggles happen in the same render. Update the toggle logic in Accordion.tsx’s toggleOpen implementation to use the functional updater form supported by useControllableState so the next state is derived from the previous value. Keep the existing disabled and collapsible guard conditions, but change the state update path to compute from the latest open state instead of the captured isOpen value.
🧹 Nitpick comments (1)
src/shared/hooks/useFocusTrap.test.tsx (1)
1-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win테스트 픽스처에서는
createRef()대신useRef()를 쓰세요.함수 컴포넌트에서
createRef()는 렌더마다 새 ref 객체를 만들어서, 픽스처가 rerender되면 포커스 트랩 대상이 흔들릴 수 있습니다. 여기서는 고정 ref가 맞습니다.수정 예시
-import { createRef } from 'react'; +import { useRef } from 'react'; @@ function FocusTrapFixture() { - const containerRef = createRef<HTMLDivElement>(); + const containerRef = useRef<HTMLDivElement>(null);🤖 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/hooks/useFocusTrap.test.tsx` around lines 1 - 12, The test fixture in FocusTrapFixture is using createRef() inside a function component, which recreates the ref on every render and can make the focus trap target unstable across rerenders. Update the fixture to use useRef() instead, keeping the same ref instance for useFocusTrap and any assertions in useFocusTrap.test.tsx.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@src/shared/ui/accordion/Accordion.tsx`:
- Around line 122-128: The Accordion toggleOpen handler currently uses a stale
state read via setIsOpen(!isOpen), which can break when multiple toggles happen
in the same render. Update the toggle logic in Accordion.tsx’s toggleOpen
implementation to use the functional updater form supported by
useControllableState so the next state is derived from the previous value. Keep
the existing disabled and collapsible guard conditions, but change the state
update path to compute from the latest open state instead of the captured isOpen
value.
---
Nitpick comments:
In `@src/shared/hooks/useFocusTrap.test.tsx`:
- Around line 1-12: The test fixture in FocusTrapFixture is using createRef()
inside a function component, which recreates the ref on every render and can
make the focus trap target unstable across rerenders. Update the fixture to use
useRef() instead, keeping the same ref instance for useFocusTrap and any
assertions in useFocusTrap.test.tsx.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 36edab16-e6e8-43e1-b736-135475082a51
📒 Files selected for processing (14)
src/shared/hooks/useControllableState.test.tssrc/shared/hooks/useControllableState.tssrc/shared/hooks/useFocusTrap.test.tsxsrc/shared/hooks/useFocusTrap.tssrc/shared/ui/accordion/Accordion.tsxsrc/shared/ui/accordion/AccordionLabel.tsxsrc/shared/ui/surface/Surface.stories.tsxsrc/shared/ui/surface/Surface.tsxsrc/shared/ui/surface/Surface.types.tssrc/shared/ui/surface/SurfaceClose.tsxsrc/shared/ui/surface/SurfaceContent.tsxsrc/shared/ui/surface/SurfaceTrigger.tsxsrc/shared/ui/surface/hooks/useSurfaceStack.tssrc/shared/ui/textarea/TextAreaField.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- src/shared/ui/textarea/TextAreaField.tsx
- src/shared/hooks/useFocusTrap.ts
- src/shared/hooks/useControllableState.ts
- src/shared/ui/surface/Surface.tsx
- src/shared/ui/surface/SurfaceContent.tsx
- src/shared/ui/surface/SurfaceClose.tsx
- src/shared/ui/surface/SurfaceTrigger.tsx
- src/shared/ui/surface/Surface.stories.tsx
- src/shared/ui/surface/Surface.types.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ui/surface/SurfaceClose.tsx`:
- Around line 58-65: The `SurfaceClose` `asChild` path is dropping most of the
close-button props, so the slot branch should preserve the same API as the
default button branch. Update the `SurfaceClose` component to pass through the
remaining props when calling `cloneSlot`, using the same prop set handled for
the native `<button>` path, while still merging `aria-label`, `className`, and
`onClick` via `createHandleClick` and `getSingleSlotChild`.
In `@src/shared/ui/surface/SurfaceTrigger.tsx`:
- Around line 74-81: The asChild branch in SurfaceTrigger is dropping any
remaining props passed to Surface.Trigger, so it should forward the full trigger
props set instead of only className, onClick, and auto aria attributes. Update
the SurfaceTrigger asChild path to merge and pass through the same remaining
props used by the non-asChild button path, ensuring values like disabled, id,
data-*, and explicit type are preserved when calling cloneSlot and
createHandleClick.
🪄 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: 526d48d5-1221-4969-9f2f-6524c257dbe2
📒 Files selected for processing (10)
src/shared/ui/accordion/AccordionLabel.tsxsrc/shared/ui/surface/Surface.stories.tsxsrc/shared/ui/surface/Surface.tsxsrc/shared/ui/surface/Surface.types.tssrc/shared/ui/surface/SurfaceClose.tsxsrc/shared/ui/surface/SurfaceContent.tsxsrc/shared/ui/surface/SurfaceSections.tsxsrc/shared/ui/surface/SurfaceTrigger.tsxsrc/shared/ui/surface/hooks/useSurfaceInteractions.tssrc/shared/ui/utils/Slot.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/shared/ui/surface/Surface.types.ts
- src/shared/ui/surface/Surface.tsx
- src/shared/ui/surface/Surface.stories.tsx
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.
#️⃣연관된 이슈
체크 사항
📝작업 내용
Surface 공통 컴포넌트 구현
Surfacecompound component를 추가했습니다.variant="modal" | "panel"로 중앙 모달과 우측 패널 표현을 선택하도록 구성했습니다.Surface.Trigger,Surface.Portal,Surface.Overlay,Surface.Content,Surface.Header,Surface.Body,Surface.Footer,Surface.Close를 구현했습니다.Trigger,Close는 공통Button과 조합할 수 있도록asChild패턴을 지원합니다.Surface밖에서 사용하면 명확한 에러가 발생하도록 Context hook을 구성했습니다.Open / Close 상태 및 인터랙션 처리
useControllableState훅을 추가했습니다.closeOnEscape,closeOnOutsideClick,canClose옵션으로 dismiss 닫기 정책을 제어할 수 있도록 구현했습니다.Surface.Close는 정책과 무관하게 항상 닫히도록 정리했습니다.SurfaceStack을 추가했습니다.접근성 및 스타일
Surface.Content에role="dialog"를 적용했습니다.aria-modal을 적용합니다.Surface.Content는aria-label또는aria-labelledby중 하나를 타입으로 필수화했습니다.aria-controls,aria-expanded를 자동 연결했습니다.z-index.css를 추가해 surface overlay/content와 form floating z-index를 토큰으로 관리하도록 정리했습니다.스크린샷 (선택)
추가한 라이브러리 (선택)
없음
💬리뷰 요구사항(선택)
Summary by CodeRabbit
Surface컴포넌트(Trigger/Overlay/Portal/헤더·본문·푸터/닫기)가 추가되었습니다.ESC, 바깥 클릭, 포커스 트랩·복원, 스크롤 잠금, 제어형 상태 훅을 제공하며 중첩 화면 동작도 포함됩니다.Accordion과TextAreaField의 상태 동기화가 개선되었습니다.